From 71f29802eee124c75051c3a3bdb0e7b3a220f0eb Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Fri, 30 Aug 2019 18:09:09 +0200 Subject: [PATCH 01/11] Preserve URL fragment during SAML handshake. --- docs/security/authentication/index.asciidoc | 2 +- renovate.json5 | 16 + x-pack/legacy/plugins/security/index.js | 4 +- .../server/routes/api/v1/authenticate.js | 52 ++- x-pack/package.json | 2 + .../security/server/authentication/index.ts | 12 +- .../server/authentication/providers/index.ts | 2 +- .../authentication/providers/saml.test.ts | 41 ++- .../server/authentication/providers/saml.ts | 111 +++--- x-pack/plugins/security/server/plugin.ts | 51 ++- .../security/server/routes/authentication.ts | 112 ++++++ .../plugins/security/server/routes/index.ts | 25 ++ x-pack/scripts/functional_tests.js | 8 +- .../apis/{index.js => index.ts} | 6 +- .../apis/security/{index.js => index.ts} | 4 +- .../security/{saml_login.js => saml_login.ts} | 322 ++++++++++++------ .../{config.js => config.ts} | 11 +- .../fixtures/{saml_tools.js => saml_tools.ts} | 64 ++-- .../ftr_provider_context.d.ts | 11 + x-pack/test/saml_api_integration/services.ts | 13 + yarn.lock | 15 + 21 files changed, 626 insertions(+), 258 deletions(-) create mode 100644 x-pack/plugins/security/server/routes/authentication.ts create mode 100644 x-pack/plugins/security/server/routes/index.ts rename x-pack/test/saml_api_integration/apis/{index.js => index.ts} (66%) rename x-pack/test/saml_api_integration/apis/security/{index.js => index.ts} (71%) rename x-pack/test/saml_api_integration/apis/security/{saml_login.js => saml_login.ts} (66%) rename x-pack/test/saml_api_integration/{config.js => config.ts} (84%) rename x-pack/test/saml_api_integration/fixtures/{saml_tools.js => saml_tools.ts} (75%) create mode 100644 x-pack/test/saml_api_integration/ftr_provider_context.d.ts create mode 100644 x-pack/test/saml_api_integration/services.ts diff --git a/docs/security/authentication/index.asciidoc b/docs/security/authentication/index.asciidoc index 1b326c95e20b52..3161b146d802a6 100644 --- a/docs/security/authentication/index.asciidoc +++ b/docs/security/authentication/index.asciidoc @@ -100,7 +100,7 @@ xpack.security.authc.saml.realm: realm-name + [source,yaml] -------------------------------------------------------------------------------- -server.xsrf.whitelist: [/api/security/v1/saml] +server.xsrf.whitelist: [/security/api/authc/saml/callback] -------------------------------------------------------------------------------- Users will be able to log in to {kib} via SAML Single Sign-On by navigating directly to the {kib} URL. Users who aren't authenticated are redirected to the Identity Provider for login. Most Identity Providers maintain a long-lived session—users who logged in to a different application using the same Identity Provider in the same browser are automatically authenticated. An exception is if {es} or the Identity Provider is configured to force user to re-authenticate. This login scenario is called _Service Provider initiated login_. diff --git a/renovate.json5 b/renovate.json5 index e3b887d52149c5..e7d7253aa73deb 100644 --- a/renovate.json5 +++ b/renovate.json5 @@ -761,6 +761,22 @@ '@types/tinycolor2', ], }, + { + groupSlug: 'xml2js', + groupName: 'xml2js related packages', + packageNames: [ + 'xml2js', + '@types/xml2js', + ], + }, + { + groupSlug: 'xml-crypto', + groupName: 'xml-crypto related packages', + packageNames: [ + 'xml-crypto', + '@types/xml-crypto', + ], + }, { groupSlug: 'intl-relativeformat', groupName: 'intl-relativeformat related packages', diff --git a/x-pack/legacy/plugins/security/index.js b/x-pack/legacy/plugins/security/index.js index 0626629b7c4170..980af19cc8362b 100644 --- a/x-pack/legacy/plugins/security/index.js +++ b/x-pack/legacy/plugins/security/index.js @@ -31,6 +31,7 @@ import { SecureSavedObjectsClientWrapper } from './server/lib/saved_objects_clie import { deepFreeze } from './server/lib/deep_freeze'; import { createOptionalPlugin } from '../../server/lib/optional_plugin'; import { KibanaRequest } from '../../../../src/core/server'; +import { createCSPRuleString } from '../../../../src/legacy/server/csp'; export const security = (kibana) => new kibana.Plugin({ id: 'security', @@ -128,6 +129,7 @@ export const security = (kibana) => new kibana.Plugin({ throw new Error('New Platform XPack Security plugin is not available.'); } + const config = server.config(); const xpackMainPlugin = server.plugins.xpack_main; const xpackInfo = xpackMainPlugin.info; securityPlugin.registerLegacyAPI({ @@ -135,10 +137,10 @@ export const security = (kibana) => new kibana.Plugin({ isSystemAPIRequest: server.plugins.kibana.systemApi.isSystemApiRequest.bind( server.plugins.kibana.systemApi ), + cspRules: createCSPRuleString(config.get('csp.rules')), }); const plugin = this; - const config = server.config(); const xpackInfoFeature = xpackInfo.feature(plugin.id); // Register a function that is called whenever the xpack info changes, diff --git a/x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js b/x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js index 5e2bfce7ada133..5f6549723f55a7 100644 --- a/x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js +++ b/x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js @@ -12,6 +12,27 @@ import { KibanaRequest } from '../../../../../../../../src/core/server'; import { createCSPRuleString } from '../../../../../../../../src/legacy/server/csp'; export function initAuthenticateApi({ authc: { login, logout }, config }, server) { + const xsrfWhitelist = server.config().get('server.xsrf.whitelist'); + server.ext('onRequest', (request, h) => { + // We handle this route in new platform plugin, but use different URL. + if (request.path === '/api/security/v1/saml') { + server.log( + ['warning', 'security', 'deprecation'], + `The following URL is deprecated and will stop working in the next major version: ${request.path}` + ); + + // If deprecated route (`/api/security/v1/saml`) is included into XSRF whitelist we should + // implicitly include `/security/api/authc/saml/callback` as well to preserve the same behavior. + // We can't modify config, but we can mark route as ^safe^ via `kbn-xsrf` header. + if (xsrfWhitelist.includes(request.path)) { + request.headers['kbn-xsrf'] = true; + } + + request.setUrl('/security/api/authc/saml/callback'); + } + + return h.continue; + }); server.route({ method: 'POST', @@ -52,37 +73,6 @@ export function initAuthenticateApi({ authc: { login, logout }, config }, server } }); - server.route({ - method: 'POST', - path: '/api/security/v1/saml', - config: { - auth: false, - validate: { - payload: Joi.object({ - SAMLResponse: Joi.string().required(), - RelayState: Joi.string().allow('') - }) - } - }, - async handler(request, h) { - try { - // When authenticating using SAML we _expect_ to redirect to the SAML Identity provider. - const authenticationResult = await login(KibanaRequest.from(request), { - provider: 'saml', - value: { samlResponse: request.payload.SAMLResponse } - }); - - if (authenticationResult.redirected()) { - return h.redirect(authenticationResult.redirectURL); - } - - return Boom.unauthorized(authenticationResult.error); - } catch (err) { - return wrapError(err); - } - } - }); - /** * The route should be configured as a redirect URI in OP when OpenID Connect implicit flow * is used, so that we can extract authentication response from URL fragment and send it to diff --git a/x-pack/package.json b/x-pack/package.json index 06404068b1d657..643cfcf3b5421e 100644 --- a/x-pack/package.json +++ b/x-pack/package.json @@ -104,6 +104,8 @@ "@types/tar-fs": "^1.16.1", "@types/tinycolor2": "^1.4.1", "@types/uuid": "^3.4.4", + "@types/xml2js": "^0.4.4", + "@types/xml-crypto": "^1.4.0", "abab": "^1.0.4", "ansicolors": "0.3.2", "axios": "^0.19.0", diff --git a/x-pack/plugins/security/server/authentication/index.ts b/x-pack/plugins/security/server/authentication/index.ts index 6a88c5cddc4414..5647cff522b496 100644 --- a/x-pack/plugins/security/server/authentication/index.ts +++ b/x-pack/plugins/security/server/authentication/index.ts @@ -3,6 +3,7 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ +import { UnwrapPromise } from '@kbn/utility-types'; import { ClusterClient, CoreSetup, @@ -20,8 +21,13 @@ export { canRedirectRequest } from './can_redirect_request'; export { Authenticator, ProviderLoginAttempt } from './authenticator'; export { AuthenticationResult } from './authentication_result'; export { DeauthenticationResult } from './deauthentication_result'; -export { OIDCAuthenticationFlow } from './providers'; -export { CreateAPIKeyResult } from './api_keys'; +export { OIDCAuthenticationFlow, SAMLLoginStep } from './providers'; +export { + CreateAPIKeyResult, + InvalidateAPIKeyResult, + CreateAPIKeyParams, + InvalidateAPIKeyParams, +} from './api_keys'; interface SetupAuthenticationParams { core: CoreSetup; @@ -31,6 +37,8 @@ interface SetupAuthenticationParams { getLegacyAPI(): LegacyAPI; } +export type Authentication = UnwrapPromise>; + export async function setupAuthentication({ core, clusterClient, diff --git a/x-pack/plugins/security/server/authentication/providers/index.ts b/x-pack/plugins/security/server/authentication/providers/index.ts index af0b90766e8594..1ec6dfb67a81d5 100644 --- a/x-pack/plugins/security/server/authentication/providers/index.ts +++ b/x-pack/plugins/security/server/authentication/providers/index.ts @@ -11,7 +11,7 @@ export { } from './base'; export { BasicAuthenticationProvider } from './basic'; export { KerberosAuthenticationProvider } from './kerberos'; -export { SAMLAuthenticationProvider, isSAMLRequestQuery } from './saml'; +export { SAMLAuthenticationProvider, isSAMLRequestQuery, SAMLLoginStep } from './saml'; export { TokenAuthenticationProvider } from './token'; export { OIDCAuthenticationProvider, OIDCAuthenticationFlow } from './oidc'; export { PKIAuthenticationProvider } from './pki'; diff --git a/x-pack/plugins/security/server/authentication/providers/saml.test.ts b/x-pack/plugins/security/server/authentication/providers/saml.test.ts index 4d4fa796851d0b..463d2ee0ad4f90 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.test.ts @@ -15,7 +15,7 @@ import { mockScopedClusterClient, } from './base.mock'; -import { SAMLAuthenticationProvider } from './saml'; +import { SAMLAuthenticationProvider, SAMLLoginStep } from './saml'; describe('SAMLAuthenticationProvider', () => { let provider: SAMLAuthenticationProvider; @@ -49,8 +49,12 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.login( request, - { samlResponse: 'saml-response-xml' }, - { requestId: 'some-request-id', nextURL: '/test-base-path/some-path' } + { + step: SAMLLoginStep.SAMLResponseReceived, + samlResponse: 'saml-response-xml', + nextURL: '/test-base-path/some-path', + }, + { requestId: 'some-request-id' } ); sinon.assert.calledWithExactly( @@ -72,8 +76,12 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.login( request, - { samlResponse: 'saml-response-xml' }, - { nextURL: '/test-base-path/some-path' } + { + step: SAMLLoginStep.SAMLResponseReceived, + samlResponse: 'saml-response-xml', + nextURL: '/test-base-path/some-path', + }, + {} ); sinon.assert.notCalled(mockOptions.client.callAsInternalUser); @@ -91,7 +99,7 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.login( request, - { samlResponse: 'saml-response-xml' }, + { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' }, { requestId: 'some-request-id' } ); @@ -114,6 +122,7 @@ describe('SAMLAuthenticationProvider', () => { }); const authenticationResult = await provider.login(request, { + step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml', }); @@ -141,8 +150,12 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.login( request, - { samlResponse: 'saml-response-xml' }, - { requestId: 'some-request-id', nextURL: '/test-base-path/some-path' } + { + step: SAMLLoginStep.SAMLResponseReceived, + samlResponse: 'saml-response-xml', + nextURL: '/test-base-path/some-path', + }, + { requestId: 'some-request-id' } ); sinon.assert.calledWithExactly( @@ -171,7 +184,7 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.login( request, - { samlResponse: 'saml-response-xml' }, + { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' }, { accessToken: 'some-valid-token', refreshToken: 'some-valid-refresh-token' } ); @@ -212,7 +225,7 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.login( request, - { samlResponse: 'saml-response-xml' }, + { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' }, { accessToken: 'existing-valid-token', refreshToken: 'existing-valid-refresh-token' } ); @@ -247,7 +260,7 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.login( request, - { samlResponse: 'saml-response-xml' }, + { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' }, tokenPair ); @@ -281,7 +294,7 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.login( request, - { samlResponse: 'saml-response-xml' }, + { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' }, tokenPair ); @@ -329,7 +342,7 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.login( request, - { samlResponse: 'saml-response-xml' }, + { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' }, tokenPair ); @@ -377,7 +390,7 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.login( request, - { samlResponse: 'saml-response-xml' }, + { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' }, tokenPair ); diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts index ecd88f511bf8c8..18c49ea4f6455a 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.ts @@ -21,19 +21,28 @@ interface ProviderState extends Partial { * Unique identifier of the SAML request initiated the handshake. */ requestId?: string; +} +/** + * Describes possible SAML Login steps. + */ +export enum SAMLLoginStep { /** - * URL to redirect user to after successful SAML handshake. + * The final login step when IdP responds with SAML Response payload. */ - nextURL?: string; + SAMLResponseReceived = 'saml-response-received', + /** + * The login step when we've captured user URL (including URL fragment) and ready to start SAML handshake. + */ + UserURLCaptured = 'user-url-captured', } /** * Describes the parameters that are required by the provider to process the initial login request. */ -interface ProviderLoginAttempt { - samlResponse: string; -} +type ProviderLoginAttempt = + | { step: SAMLLoginStep.UserURLCaptured; currentURL: string } + | { step: SAMLLoginStep.SAMLResponseReceived; samlResponse: string; nextURL?: string }; /** * Checks whether request query includes SAML request from IdP. @@ -73,18 +82,24 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { */ public async login( request: KibanaRequest, - { samlResponse }: ProviderLoginAttempt, + attempt: ProviderLoginAttempt, state?: ProviderState | null ) { this.logger.debug('Trying to perform a login.'); + if (attempt.step === SAMLLoginStep.UserURLCaptured) { + this.logger.debug('Captured user URL.'); + return this.authenticateViaHandshake(request, attempt.currentURL); + } + + const { samlResponse, nextURL } = attempt; const authenticationResult = state ? await this.authenticateViaState(request, state) : AuthenticationResult.notHandled(); // Let's check if user is redirected to Kibana from IdP with valid SAMLResponse. if (authenticationResult.notHandled()) { - return await this.loginWithSAMLResponse(request, samlResponse, state); + return await this.loginWithSAMLResponse(request, { samlResponse, nextURL }, state); } if (authenticationResult.succeeded()) { @@ -93,7 +108,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { // we'll re-authenticate user and forward to a page with the respective warning. return await this.loginWithNewSAMLResponse( request, - samlResponse, + { samlResponse, nextURL }, (authenticationResult.state || state) as ProviderState, authenticationResult.user as AuthenticatedUser ); @@ -139,10 +154,10 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { } } - // If we couldn't authenticate by means of all methods above, let's try to + // If we couldn't authenticate by means of all methods above, let's try to capture user URL and // initiate SAML handshake, otherwise just return authentication result we have. - return authenticationResult.notHandled() - ? await this.authenticateViaHandshake(request) + return authenticationResult.notHandled() && canRedirectRequest(request) + ? this.captureCurrentURL(request) : authenticationResult; } @@ -227,23 +242,21 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { * initiated login. * @param request Request instance. * @param samlResponse SAMLResponse payload string. + * @param [nextURL] Optional URL to redirect user to (isn't present for IdP initiated login). * @param [state] Optional state object associated with the provider. */ private async loginWithSAMLResponse( request: KibanaRequest, - samlResponse: string, + { samlResponse, nextURL }: { samlResponse: string; nextURL?: string }, state?: ProviderState | null ) { this.logger.debug('Trying to log in with SAML response payload.'); // If we have a `SAMLResponse` and state, but state doesn't contain all the necessary information, // then something unexpected happened and we should fail. - const { requestId: stateRequestId, nextURL: stateRedirectURL } = state || { - requestId: '', - nextURL: '', - }; - if (state && (!stateRequestId || !stateRedirectURL)) { - const message = 'SAML response state does not have corresponding request id or redirect URL.'; + const { requestId: stateRequestId } = state || { requestId: '' }; + if (state && !stateRequestId) { + const message = 'SAML response state does not have corresponding request id.'; this.logger.debug(message); return AuthenticationResult.failed(Boom.badRequest(message)); } @@ -269,10 +282,9 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { }); this.logger.debug('Login has been performed with SAML response.'); - return AuthenticationResult.redirectTo( - stateRedirectURL || `${this.options.basePath.get(request)}/`, - { state: { accessToken, refreshToken } } - ); + return AuthenticationResult.redirectTo(nextURL || `${this.options.basePath.get(request)}/`, { + state: { accessToken, refreshToken }, + }); } catch (err) { this.logger.debug(`Failed to log in with SAML response: ${err.message}`); return AuthenticationResult.failed(err); @@ -289,19 +301,23 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { * we'll forward user to a page with the respective warning. * @param request Request instance. * @param samlResponse SAMLResponse payload string. + * @param [nextURL] Optional URL to redirect user to (isn't present for IdP initiated login). * @param existingState State existing user session is based on. * @param user User returned for the existing session. */ private async loginWithNewSAMLResponse( request: KibanaRequest, - samlResponse: string, + { samlResponse, nextURL }: { samlResponse: string; nextURL?: string }, existingState: ProviderState, user: AuthenticatedUser ) { this.logger.debug('Trying to log in with SAML response payload and existing valid session.'); // First let's try to authenticate via SAML Response payload. - const payloadAuthenticationResult = await this.loginWithSAMLResponse(request, samlResponse); + const payloadAuthenticationResult = await this.loginWithSAMLResponse(request, { + samlResponse, + nextURL, + }); if (payloadAuthenticationResult.failed()) { return payloadAuthenticationResult; } @@ -417,9 +433,9 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { if (refreshedTokenPair === null) { if (canRedirectRequest(request)) { this.logger.debug( - 'Both access and refresh tokens are expired. Re-initiating SAML handshake.' + 'Both access and refresh tokens are expired. Capturing current URL and re-initiating SAML handshake.' ); - return this.authenticateViaHandshake(request); + return this.captureCurrentURL(request); } return AuthenticationResult.failed( @@ -444,8 +460,9 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { /** * Tries to start SAML handshake and eventually receive a token. * @param request Request instance. + * @param nextURL URL to redirect user to after successful SAML handshake. */ - private async authenticateViaHandshake(request: KibanaRequest) { + private async authenticateViaHandshake(request: KibanaRequest, nextURL: string) { this.logger.debug('Trying to initiate SAML handshake.'); // If client can't handle redirect response, we shouldn't initiate SAML handshake. @@ -457,19 +474,22 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { try { // This operation should be performed on behalf of the user with a privilege that normal // user usually doesn't have `cluster:admin/xpack/security/saml/prepare`. - const { id: requestId, redirect } = await this.options.client.callAsInternalUser( - 'shield.samlPrepare', - { body: { realm: this.realm } } - ); + const { + id: requestId, + redirect: redirectWithoutRelayState, + } = await this.options.client.callAsInternalUser('shield.samlPrepare', { + body: { realm: this.realm }, + }); + + // HACK: we manually add a `RelayState` query string parameter with URL to redirect user to after + // successful SAML handshake since Elasticsearch doesn't support it yet. We may break something + // here eventually if we keep this workaround for too long. + const redirect = `${redirectWithoutRelayState}&RelayState=${encodeURIComponent(nextURL)}`; this.logger.debug('Redirecting to Identity Provider with SAML request.'); - return AuthenticationResult.redirectTo( - redirect, - // Store request id in the state so that we can reuse it once we receive `SAMLResponse`. - { - state: { requestId, nextURL: `${this.options.basePath.get(request)}${request.url.path}` }, - } - ); + + // Store request id in the state so that we can reuse it once we receive `SAMLResponse`. + return AuthenticationResult.redirectTo(redirect, { state: { requestId } }); } catch (err) { this.logger.debug(`Failed to initiate SAML handshake: ${err.message}`); return AuthenticationResult.failed(err); @@ -517,4 +537,19 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { return redirect; } + + /** + * Redirects user to the client-side page that will grab current URL fragment with the current URL + * path and redirect user back to Kibana to initiate SAML handshake. + * @param request Request instance. + */ + private captureCurrentURL(request: KibanaRequest) { + return AuthenticationResult.redirectTo( + `${this.options.basePath.get( + request + )}/security/api/authc/saml/capture-current-url?currentPath=${encodeURIComponent( + request.url.path! + )}` + ); + } } diff --git a/x-pack/plugins/security/server/plugin.ts b/x-pack/plugins/security/server/plugin.ts index 353c64836a9392..dbbbd20305e67f 100644 --- a/x-pack/plugins/security/server/plugin.ts +++ b/x-pack/plugins/security/server/plugin.ts @@ -15,15 +15,9 @@ import { } from '../../../../src/core/server'; import { deepFreeze } from '../../../../src/core/utils'; import { XPackInfo } from '../../../legacy/plugins/xpack_main/server/lib/xpack_info'; -import { AuthenticatedUser } from '../common/model'; -import { Authenticator, setupAuthentication } from './authentication'; +import { setupAuthentication, Authentication } from './authentication'; import { createConfig$ } from './config'; -import { - CreateAPIKeyParams, - CreateAPIKeyResult, - InvalidateAPIKeyParams, - InvalidateAPIKeyResult, -} from './authentication/api_keys'; +import { defineRoutes } from './routes'; /** * Describes a set of APIs that is available in the legacy platform only and required by this plugin @@ -32,26 +26,14 @@ import { export interface LegacyAPI { xpackInfo: Pick; isSystemAPIRequest: (request: KibanaRequest) => boolean; + cspRules: string; } /** * Describes public Security plugin contract returned at the `setup` stage. */ export interface PluginSetupContract { - authc: { - login: Authenticator['login']; - logout: Authenticator['logout']; - getCurrentUser: (request: KibanaRequest) => Promise; - isAuthenticated: (request: KibanaRequest) => Promise; - createAPIKey: ( - request: KibanaRequest, - params: CreateAPIKeyParams - ) => Promise; - invalidateAPIKey: ( - request: KibanaRequest, - params: InvalidateAPIKeyParams - ) => Promise; - }; + authc: Authentication; config: RecursiveReadonly<{ sessionTimeout: number | null; @@ -90,16 +72,25 @@ export class Plugin { plugins: [require('../../../legacy/server/lib/esjs_shield_plugin')], }); + const authc = await setupAuthentication({ + core, + config, + clusterClient: this.clusterClient, + loggers: this.initializerContext.logger, + getLegacyAPI: this.getLegacyAPI, + }); + + defineRoutes({ + router: core.http.createRouter(), + basePath: core.http.basePath, + logger: this.initializerContext.logger.get('routes'), + config, + authc, + }); + return deepFreeze({ registerLegacyAPI: (legacyAPI: LegacyAPI) => (this.legacyAPI = legacyAPI), - - authc: await setupAuthentication({ - core, - config, - clusterClient: this.clusterClient, - loggers: this.initializerContext.logger, - getLegacyAPI: this.getLegacyAPI, - }), + authc, // We should stop exposing this config as soon as only new platform plugin consumes it. The only // exception may be `sessionTimeout` as other parts of the app may want to know it. diff --git a/x-pack/plugins/security/server/routes/authentication.ts b/x-pack/plugins/security/server/routes/authentication.ts new file mode 100644 index 00000000000000..f79d2df5ceccff --- /dev/null +++ b/x-pack/plugins/security/server/routes/authentication.ts @@ -0,0 +1,112 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { schema } from '@kbn/config-schema'; +import { RouteDefinitionParams } from '.'; +import { SAMLLoginStep } from '../authentication'; + +export function defineAuthenticationRoutes(params: RouteDefinitionParams) { + if (params.config.authc.providers.includes('saml')) { + defineSAMLRoutes(params); + } +} + +/** + * Defines routes required for SAML authentication. + */ +function defineSAMLRoutes({ basePath, router, logger, authc }: RouteDefinitionParams) { + router.get( + { + path: '/api/authc/saml/capture-current-url', + validate: { query: schema.object({ currentPath: schema.string() }) }, + options: { authRequired: false }, + }, + (context, request, response) => { + const { currentPath } = request.query; + return response.custom({ + body: ` + + Kibana SAML Login + + `, + headers: { + 'content-type': 'text/html', + 'cache-control': 'private, no-cache, no-store', + // HACK: inline scripts aren't allowed yet. + // 'content-security-policy': getLegacyAPI().cspRules, + }, + statusCode: 200, + }); + } + ); + + router.get( + { + path: '/api/authc/saml/start', + validate: { query: schema.object({ currentURL: schema.string() }) }, + options: { authRequired: false }, + }, + async (context, request, response) => { + try { + const authenticationResult = await authc.login(request, { + provider: 'saml', + value: { step: SAMLLoginStep.UserURLCaptured, currentURL: request.query.currentURL }, + }); + + // When authenticating using SAML we _expect_ to redirect to the SAML Identity provider. + if (authenticationResult.redirected()) { + return response.redirected({ headers: { location: authenticationResult.redirectURL! } }); + } + + return response.unauthorized(); + } catch (err) { + logger.error(err); + return response.internalError(); + } + } + ); + + router.post( + { + path: '/api/authc/saml/callback', + validate: { + body: schema.object({ + SAMLResponse: schema.string(), + RelayState: schema.maybe(schema.string()), + }), + }, + options: { authRequired: false }, + }, + async (context, request, response) => { + try { + // When authenticating using SAML we _expect_ to redirect to the SAML Identity provider. + const authenticationResult = await authc.login(request, { + provider: 'saml', + value: { + step: SAMLLoginStep.SAMLResponseReceived, + samlResponse: request.body.SAMLResponse, + nextURL: request.body.RelayState, + }, + }); + + if (authenticationResult.redirected()) { + return response.redirected({ headers: { location: authenticationResult.redirectURL! } }); + } + + return response.unauthorized({ body: authenticationResult.error }); + } catch (err) { + logger.error(err); + return response.internalError(); + } + } + ); +} diff --git a/x-pack/plugins/security/server/routes/index.ts b/x-pack/plugins/security/server/routes/index.ts new file mode 100644 index 00000000000000..8088f44baa1cc6 --- /dev/null +++ b/x-pack/plugins/security/server/routes/index.ts @@ -0,0 +1,25 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { CoreSetup, IRouter, Logger } from '../../../../../src/core/server'; +import { Authentication } from '../authentication'; +import { ConfigType } from '../config'; +import { defineAuthenticationRoutes } from './authentication'; + +/** + * Describes parameters used to define HTTP routes. + */ +export interface RouteDefinitionParams { + router: IRouter; + basePath: CoreSetup['http']['basePath']; + logger: Logger; + config: ConfigType; + authc: Authentication; +} + +export function defineRoutes(params: RouteDefinitionParams) { + defineAuthenticationRoutes(params); +} diff --git a/x-pack/scripts/functional_tests.js b/x-pack/scripts/functional_tests.js index f513d117e08aed..39651a0fe06b8b 100644 --- a/x-pack/scripts/functional_tests.js +++ b/x-pack/scripts/functional_tests.js @@ -17,10 +17,10 @@ require('@kbn/test').runTestsCli([ require.resolve('../test/plugin_api_integration/config.js'), require.resolve('../test/kerberos_api_integration/config'), require.resolve('../test/kerberos_api_integration/anonymous_access.config'), - require.resolve('../test/saml_api_integration/config.js'), - require.resolve('../test/token_api_integration/config.js'), - require.resolve('../test/oidc_api_integration/config.ts'), - require.resolve('../test/oidc_api_integration/implicit_flow.config.ts'), + require.resolve('../test/saml_api_integration/config'), + require.resolve('../test/token_api_integration/config'), + require.resolve('../test/oidc_api_integration/config'), + require.resolve('../test/oidc_api_integration/implicit_flow.config'), // require.resolve('../test/pki_api_integration/config.ts'), require.resolve('../test/spaces_api_integration/spaces_only/config'), require.resolve('../test/spaces_api_integration/security_and_spaces/config_trial'), diff --git a/x-pack/test/saml_api_integration/apis/index.js b/x-pack/test/saml_api_integration/apis/index.ts similarity index 66% rename from x-pack/test/saml_api_integration/apis/index.js rename to x-pack/test/saml_api_integration/apis/index.ts index b4e6503f201e5d..84c0b5e78c79ec 100644 --- a/x-pack/test/saml_api_integration/apis/index.js +++ b/x-pack/test/saml_api_integration/apis/index.ts @@ -4,8 +4,10 @@ * you may not use this file except in compliance with the Elastic License. */ -export default function ({ loadTestFile }) { - describe('apis SAML', function () { +import { FtrProviderContext } from '../ftr_provider_context'; + +export default function({ loadTestFile }: FtrProviderContext) { + describe('apis SAML', function() { this.tags('ciGroup1'); loadTestFile(require.resolve('./security')); }); diff --git a/x-pack/test/saml_api_integration/apis/security/index.js b/x-pack/test/saml_api_integration/apis/security/index.ts similarity index 71% rename from x-pack/test/saml_api_integration/apis/security/index.js rename to x-pack/test/saml_api_integration/apis/security/index.ts index 0c6825c94c08e5..ef5520b967ca34 100644 --- a/x-pack/test/saml_api_integration/apis/security/index.js +++ b/x-pack/test/saml_api_integration/apis/security/index.ts @@ -4,7 +4,9 @@ * you may not use this file except in compliance with the Elastic License. */ -export default function ({ loadTestFile }) { +import { FtrProviderContext } from '../../ftr_provider_context'; + +export default function({ loadTestFile }: FtrProviderContext) { describe('security', () => { loadTestFile(require.resolve('./saml_login')); }); diff --git a/x-pack/test/saml_api_integration/apis/security/saml_login.js b/x-pack/test/saml_api_integration/apis/security/saml_login.ts similarity index 66% rename from x-pack/test/saml_api_integration/apis/security/saml_login.js rename to x-pack/test/saml_api_integration/apis/security/saml_login.ts index 401ffdea0418d4..e5fbed1e0dde4d 100644 --- a/x-pack/test/saml_api_integration/apis/security/saml_login.js +++ b/x-pack/test/saml_api_integration/apis/security/saml_login.ts @@ -7,11 +7,13 @@ import querystring from 'querystring'; import url from 'url'; import { delay } from 'bluebird'; -import { getLogoutRequest, getSAMLRequestId, getSAMLResponse } from '../../fixtures/saml_tools'; import expect from '@kbn/expect'; -import request from 'request'; +import request, { Cookie } from 'request'; +import { JSDOM } from 'jsdom'; +import { getLogoutRequest, getSAMLRequestId, getSAMLResponse } from '../../fixtures/saml_tools'; +import { FtrProviderContext } from '../../ftr_provider_context'; -export default function ({ getService }) { +export default function({ getService }: FtrProviderContext) { const chance = getService('chance'); const supertest = getService('supertestWithoutAuth'); const config = getService('config'); @@ -20,13 +22,13 @@ export default function ({ getService }) { function createSAMLResponse(options = {}) { return getSAMLResponse({ - destination: `http://localhost:${kibanaServerConfig.port}/api/security/v1/saml`, + destination: `http://localhost:${kibanaServerConfig.port}/security/api/authc/saml/callback`, sessionIndex: chance.natural(), ...options, }); } - function createLogoutRequest(options = {}) { + function createLogoutRequest(options: { sessionIndex: string }) { return getLogoutRequest({ destination: `http://localhost:${kibanaServerConfig.port}/logout`, ...options, @@ -55,37 +57,92 @@ export default function ({ getService }) { const { body: user } = await supertest .get('/api/security/v1/me') .set('kbn-xsrf', 'xxx') - .set('Cookie', request.cookie(cookies[0]).cookieString()) + .set('Cookie', request.cookie(cookies[0])!.cookieString()) .expect(200); expect(user.username).to.eql(username); expect(user.authentication_realm).to.eql({ name: 'reserved', type: 'reserved' }); }); - describe('initiating handshake', () => { - it('should properly set cookie and redirect user', async () => { - const handshakeResponse = await supertest.get('/abc/xyz/handshake?one=two three') + describe('capturing current URL', () => { + it('should redirect user to a page that would capture client URL', async () => { + const handshakeResponse = await supertest + .get('/abc/xyz/handshake?one=two three') .expect(302); + expect(handshakeResponse.headers['set-cookie']).to.be(undefined); + expect(handshakeResponse.headers.location).to.be( + '/security/api/authc/saml/capture-current-url?currentPath=%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three' + ); + }); + + it('should return an HTML page that will extract URL fragment', async () => { + const response = await supertest + .get( + '/security/api/authc/saml/capture-current-url?currentPath=%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three' + ) + .expect(200); + const dom = new JSDOM(response.text, { + runScripts: 'dangerously', + beforeParse(window) { + // JSDOM doesn't support changing of `window.location` and throws an exception if script + // tries to do that and we have to workaround this behaviour. + Object.defineProperty(window, 'location', { + value: { + hash: '#/workpad', + href: + 'https://kibana.com/security/api/authc/saml/capture-current-url?currentPath=%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three#/workpad', + replace(newLocation: string) { + this.href = newLocation; + }, + }, + }); + }, + }); + + // Check that proxy page is returned with proper headers. + expect(response.headers['content-type']).to.be('text/html; charset=utf-8'); + expect(response.headers['cache-control']).to.be('private, no-cache, no-store'); + /* expect(response.headers['content-security-policy']).to.be( + `script-src 'unsafe-eval' 'self'; worker-src blob:; child-src blob:; style-src 'unsafe-inline' 'self'` + );*/ + + // Check that script that forwards URL fragment worked correctly. + expect(dom.window.location.href).to.be( + '/security/api/authc/saml/start?currentURL=%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three%23%2Fworkpad' + ); + }); + }); + + describe('initiating handshake', () => { + const encodedNextURL = '%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three%23%2Fworkpad'; + const initiateHandshakeURL = `/security/api/authc/saml/start?currentURL=${encodedNextURL}`; + + it('should properly set cookie and redirect user to IdP', async () => { + const handshakeResponse = await supertest.get(initiateHandshakeURL).expect(302); + const cookies = handshakeResponse.headers['set-cookie']; expect(cookies).to.have.length(1); - const handshakeCookie = request.cookie(cookies[0]); + const handshakeCookie = request.cookie(cookies[0])!; expect(handshakeCookie.key).to.be('sid'); expect(handshakeCookie.value).to.not.be.empty(); expect(handshakeCookie.path).to.be('/'); expect(handshakeCookie.httpOnly).to.be(true); - const redirectURL = url.parse(handshakeResponse.headers.location, true /* parseQueryString */); - expect(redirectURL.href.startsWith(`https://elastic.co/sso/saml`)).to.be(true); + const redirectURL = url.parse( + handshakeResponse.headers.location, + true /* parseQueryString */ + ); + expect(redirectURL.href!.startsWith(`https://elastic.co/sso/saml`)).to.be(true); + expect(redirectURL.href!.endsWith(`&RelayState=${encodedNextURL}`)).to.be(true); expect(redirectURL.query.SAMLRequest).to.not.be.empty(); }); it('should not allow access to the API', async () => { - const handshakeResponse = await supertest.get('/abc/xyz/handshake?one=two three') - .expect(302); + const handshakeResponse = await supertest.get(initiateHandshakeURL).expect(302); - const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0]); + const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0])!; await supertest .get('/api/security/v1/me') .set('kbn-xsrf', 'xxx') @@ -94,7 +151,8 @@ export default function ({ getService }) { }); it('AJAX requests should not initiate handshake', async () => { - const ajaxResponse = await supertest.get('/abc/xyz/handshake?one=two three') + const ajaxResponse = await supertest + .get(initiateHandshakeURL) .set('kbn-xsrf', 'xxx') .expect(401); @@ -103,38 +161,45 @@ export default function ({ getService }) { }); describe('finishing handshake', () => { - let handshakeCookie; - let samlRequestId; + const encodedNextURL = '%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three%23%2Fworkpad'; + let handshakeCookie: Cookie; + let samlRequestId: string; beforeEach(async () => { - const handshakeResponse = await supertest.get('/abc/xyz/handshake?one=two three') + const handshakeResponse = await supertest + .get(`/security/api/authc/saml/start?currentURL=${encodedNextURL}`) .expect(302); - handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0]); + handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0])!; samlRequestId = await getSAMLRequestId(handshakeResponse.headers.location); }); it('should fail if SAML response is not complemented with handshake cookie', async () => { - await supertest.post('/api/security/v1/saml') + await supertest + .post('/security/api/authc/saml/callback') .set('kbn-xsrf', 'xxx') - .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }) }, {}) + .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }) }) .expect(401); }); it('should succeed if both SAML response and handshake cookie are provided', async () => { - const samlAuthenticationResponse = await supertest.post('/api/security/v1/saml') + const samlAuthenticationResponse = await supertest + .post('/security/api/authc/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) - .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }) }, {}) + .send({ + SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }), + RelayState: encodedNextURL, + }) .expect(302); // User should be redirected to the URL that initiated handshake. - expect(samlAuthenticationResponse.headers.location).to.be('/abc/xyz/handshake?one=two%20three'); + expect(samlAuthenticationResponse.headers.location).to.be(encodedNextURL); const cookies = samlAuthenticationResponse.headers['set-cookie']; expect(cookies).to.have.length(1); - const sessionCookie = request.cookie(cookies[0]); + const sessionCookie = request.cookie(cookies[0])!; expect(sessionCookie.key).to.be('sid'); expect(sessionCookie.value).to.not.be.empty(); expect(sessionCookie.path).to.be('/'); @@ -161,20 +226,21 @@ export default function ({ getService }) { }); it('should succeed in case of IdP initiated login', async () => { - // Don't pass handshake cookie and don't include `inResponseTo` into SAML response - // to simulate IdP initiated login. - const samlAuthenticationResponse = await supertest.post('/api/security/v1/saml') + // Don't pass handshake cookie, RelayState query string parameter and don't include + // `inResponseTo` into SAML response to simulate IdP initiated login. + const samlAuthenticationResponse = await supertest + .post('/security/api/authc/saml/callback') .set('kbn-xsrf', 'xxx') - .send({ SAMLResponse: await createSAMLResponse() }, {}) + .send({ SAMLResponse: await createSAMLResponse() }) .expect(302); - // User should be redirected to the URL that initiated handshake. + // User should be redirected to the base URL. expect(samlAuthenticationResponse.headers.location).to.be('/'); const cookies = samlAuthenticationResponse.headers['set-cookie']; expect(cookies).to.have.length(1); - const sessionCookie = request.cookie(cookies[0]); + const sessionCookie = request.cookie(cookies[0])!; expect(sessionCookie.key).to.be('sid'); expect(sessionCookie.value).to.not.be.empty(); expect(sessionCookie.path).to.be('/'); @@ -201,31 +267,29 @@ export default function ({ getService }) { }); it('should fail if SAML response is not valid', async () => { - await supertest.post('/api/security/v1/saml') + await supertest + .post('/security/api/authc/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) - .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: 'some-invalid-request-id' }) }, {}) + .send({ + SAMLResponse: await createSAMLResponse({ inResponseTo: 'some-invalid-request-id' }), + }) .expect(401); }); }); describe('API access with active session', () => { - let sessionCookie; + let sessionCookie: Cookie; beforeEach(async () => { - const handshakeResponse = await supertest.get('/abc/xyz') - .expect(302); - - const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0]); - const samlRequestId = await getSAMLRequestId(handshakeResponse.headers.location); - - const samlAuthenticationResponse = await supertest.post('/api/security/v1/saml') + // Imitate IdP initiated login. + const samlAuthenticationResponse = await supertest + .post('/security/api/authc/saml/callback') .set('kbn-xsrf', 'xxx') - .set('Cookie', handshakeCookie.cookieString()) - .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }) }, {}) + .send({ SAMLResponse: await createSAMLResponse() }) .expect(302); - sessionCookie = request.cookie(samlAuthenticationResponse.headers['set-cookie'][0]); + sessionCookie = request.cookie(samlAuthenticationResponse.headers['set-cookie'][0])!; }); it('should extend cookie on every successful non-system API call', async () => { @@ -236,7 +300,7 @@ export default function ({ getService }) { .expect(200); expect(apiResponseOne.headers['set-cookie']).to.not.be(undefined); - const sessionCookieOne = request.cookie(apiResponseOne.headers['set-cookie'][0]); + const sessionCookieOne = request.cookie(apiResponseOne.headers['set-cookie'][0])!; expect(sessionCookieOne.value).to.not.be.empty(); expect(sessionCookieOne.value).to.not.equal(sessionCookie.value); @@ -248,7 +312,7 @@ export default function ({ getService }) { .expect(200); expect(apiResponseTwo.headers['set-cookie']).to.not.be(undefined); - const sessionCookieTwo = request.cookie(apiResponseTwo.headers['set-cookie'][0]); + const sessionCookieTwo = request.cookie(apiResponseTwo.headers['set-cookie'][0])!; expect(sessionCookieTwo.value).to.not.be.empty(); expect(sessionCookieTwo.value).to.not.equal(sessionCookieOne.value); @@ -278,37 +342,44 @@ export default function ({ getService }) { }); describe('logging out', () => { - let sessionCookie; - let idpSessionIndex; + let sessionCookie: Cookie; + let idpSessionIndex: string; beforeEach(async () => { - const handshakeResponse = await supertest.get('/abc/xyz') + const handshakeResponse = await supertest + .get(`/security/api/authc/saml/start?currentURL=${encodeURIComponent('/abc/xyz')}`) .expect(302); - const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0]); + const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0])!; const samlRequestId = await getSAMLRequestId(handshakeResponse.headers.location); idpSessionIndex = chance.natural(); - const samlAuthenticationResponse = await supertest.post('/api/security/v1/saml') + const samlAuthenticationResponse = await supertest + .post('/security/api/authc/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) .send({ - SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId, sessionIndex: idpSessionIndex }) - }, {}) + SAMLResponse: await createSAMLResponse({ + inResponseTo: samlRequestId, + sessionIndex: idpSessionIndex, + }), + RelayState: encodeURIComponent('/abc/xyz'), + }) .expect(302); - sessionCookie = request.cookie(samlAuthenticationResponse.headers['set-cookie'][0]); + sessionCookie = request.cookie(samlAuthenticationResponse.headers['set-cookie'][0])!; }); it('should redirect to IdP with SAML request to complete logout', async () => { - const logoutResponse = await supertest.get('/api/security/v1/logout') + const logoutResponse = await supertest + .get('/api/security/v1/logout') .set('Cookie', sessionCookie.cookieString()) .expect(302); const cookies = logoutResponse.headers['set-cookie']; expect(cookies).to.have.length(1); - const logoutCookie = request.cookie(cookies[0]); + const logoutCookie = request.cookie(cookies[0])!; expect(logoutCookie.key).to.be('sid'); expect(logoutCookie.value).to.be.empty(); expect(logoutCookie.path).to.be('/'); @@ -316,7 +387,7 @@ export default function ({ getService }) { expect(logoutCookie.maxAge).to.be(0); const redirectURL = url.parse(logoutResponse.headers.location, true /* parseQueryString */); - expect(redirectURL.href.startsWith(`https://elastic.co/slo/saml`)).to.be(true); + expect(redirectURL.href!.startsWith(`https://elastic.co/slo/saml`)).to.be(true); expect(redirectURL.query.SAMLRequest).to.not.be.empty(); // Tokens that were stored in the previous cookie should be invalidated as well and old @@ -330,20 +401,20 @@ export default function ({ getService }) { expect(apiResponse.body).to.eql({ error: 'Bad Request', message: 'Both access and refresh tokens are expired.', - statusCode: 400 + statusCode: 400, }); }); it('should redirect to home page if session cookie is not provided', async () => { - const logoutResponse = await supertest.get('/api/security/v1/logout') - .expect(302); + const logoutResponse = await supertest.get('/api/security/v1/logout').expect(302); expect(logoutResponse.headers['set-cookie']).to.be(undefined); expect(logoutResponse.headers.location).to.be('/'); }); it('should reject AJAX requests', async () => { - const ajaxResponse = await supertest.get('/api/security/v1/logout') + const ajaxResponse = await supertest + .get('/api/security/v1/logout') .set('kbn-xsrf', 'xxx') .set('Cookie', sessionCookie.cookieString()) .expect(400); @@ -352,20 +423,21 @@ export default function ({ getService }) { expect(ajaxResponse.body).to.eql({ error: 'Bad Request', message: 'Client should be able to process redirect response.', - statusCode: 400 + statusCode: 400, }); }); it('should invalidate access token on IdP initiated logout', async () => { const logoutRequest = await createLogoutRequest({ sessionIndex: idpSessionIndex }); - const logoutResponse = await supertest.get(`/api/security/v1/logout?${querystring.stringify(logoutRequest)}`) + const logoutResponse = await supertest + .get(`/api/security/v1/logout?${querystring.stringify(logoutRequest)}`) .set('Cookie', sessionCookie.cookieString()) .expect(302); const cookies = logoutResponse.headers['set-cookie']; expect(cookies).to.have.length(1); - const logoutCookie = request.cookie(cookies[0]); + const logoutCookie = request.cookie(cookies[0])!; expect(logoutCookie.key).to.be('sid'); expect(logoutCookie.value).to.be.empty(); expect(logoutCookie.path).to.be('/'); @@ -373,7 +445,7 @@ export default function ({ getService }) { expect(logoutCookie.maxAge).to.be(0); const redirectURL = url.parse(logoutResponse.headers.location, true /* parseQueryString */); - expect(redirectURL.href.startsWith(`https://elastic.co/slo/saml`)).to.be(true); + expect(redirectURL.href!.startsWith(`https://elastic.co/slo/saml`)).to.be(true); expect(redirectURL.query.SAMLResponse).to.not.be.empty(); // Tokens that were stored in the previous cookie should be invalidated as well and old session @@ -387,19 +459,20 @@ export default function ({ getService }) { expect(apiResponse.body).to.eql({ error: 'Bad Request', message: 'Both access and refresh tokens are expired.', - statusCode: 400 + statusCode: 400, }); }); it('should invalidate access token on IdP initiated logout even if there is no Kibana session', async () => { const logoutRequest = await createLogoutRequest({ sessionIndex: idpSessionIndex }); - const logoutResponse = await supertest.get(`/api/security/v1/logout?${querystring.stringify(logoutRequest)}`) + const logoutResponse = await supertest + .get(`/api/security/v1/logout?${querystring.stringify(logoutRequest)}`) .expect(302); expect(logoutResponse.headers['set-cookie']).to.be(undefined); const redirectURL = url.parse(logoutResponse.headers.location, true /* parseQueryString */); - expect(redirectURL.href.startsWith(`https://elastic.co/slo/saml`)).to.be(true); + expect(redirectURL.href!.startsWith(`https://elastic.co/slo/saml`)).to.be(true); expect(redirectURL.query.SAMLResponse).to.not.be.empty(); // Elasticsearch should find and invalidate access and refresh tokens that correspond to provided @@ -414,31 +487,36 @@ export default function ({ getService }) { expect(apiResponse.body).to.eql({ error: 'Bad Request', message: 'Both access and refresh tokens are expired.', - statusCode: 400 + statusCode: 400, }); }); }); describe('API access with expired access token.', () => { - let sessionCookie; + let sessionCookie: Cookie; beforeEach(async () => { - const handshakeResponse = await supertest.get('/abc/xyz') + const handshakeResponse = await supertest + .get(`/security/api/authc/saml/start?currentURL=${encodeURIComponent('/abc/xyz')}`) .expect(302); - const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0]); + const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0])!; const samlRequestId = await getSAMLRequestId(handshakeResponse.headers.location); - const samlAuthenticationResponse = await supertest.post('/api/security/v1/saml') + const samlAuthenticationResponse = await supertest + .post('/security/api/authc/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) - .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }) }, {}) + .send({ + SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }), + RelayState: encodeURIComponent('/abc/xyz'), + }) .expect(302); - sessionCookie = request.cookie(samlAuthenticationResponse.headers['set-cookie'][0]); + sessionCookie = request.cookie(samlAuthenticationResponse.headers['set-cookie'][0])!; }); - const expectNewSessionCookie = (cookie) => { + const expectNewSessionCookie = (cookie: Cookie) => { expect(cookie.key).to.be('sid'); expect(cookie.value).to.not.be.empty(); expect(cookie.path).to.be('/'); @@ -446,7 +524,7 @@ export default function ({ getService }) { expect(cookie.value).to.not.be(sessionCookie.value); }; - it('expired access token should be automatically refreshed', async function () { + it('expired access token should be automatically refreshed', async function() { this.timeout(40000); // Access token expiration is set to 15s for API integration tests. @@ -464,7 +542,7 @@ export default function ({ getService }) { const firstResponseCookies = firstResponse.headers['set-cookie']; expect(firstResponseCookies).to.have.length(1); - const firstNewCookie = request.cookie(firstResponseCookies[0]); + const firstNewCookie = request.cookie(firstResponseCookies[0])!; expectNewSessionCookie(firstNewCookie); // Request with old cookie should reuse the same refresh token if within 60 seconds. @@ -478,7 +556,7 @@ export default function ({ getService }) { const secondResponseCookies = secondResponse.headers['set-cookie']; expect(secondResponseCookies).to.have.length(1); - const secondNewCookie = request.cookie(secondResponseCookies[0]); + const secondNewCookie = request.cookie(secondResponseCookies[0])!; expectNewSessionCookie(secondNewCookie); expect(firstNewCookie.value).not.to.eql(secondNewCookie.value); @@ -500,25 +578,30 @@ export default function ({ getService }) { }); describe('API access with missing access token document.', () => { - let sessionCookie; + let sessionCookie: Cookie; beforeEach(async () => { - const handshakeResponse = await supertest.get('/abc/xyz') + const handshakeResponse = await supertest + .get(`/security/api/authc/saml/start?currentURL=${encodeURIComponent('/abc/xyz')}`) .expect(302); - const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0]); + const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0])!; const samlRequestId = await getSAMLRequestId(handshakeResponse.headers.location); - const samlAuthenticationResponse = await supertest.post('/api/security/v1/saml') + const samlAuthenticationResponse = await supertest + .post('/security/api/authc/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) - .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }) }, {}) + .send({ + SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }), + RelayState: encodeURIComponent('/abc/xyz'), + }) .expect(302); - sessionCookie = request.cookie(samlAuthenticationResponse.headers['set-cookie'][0]); + sessionCookie = request.cookie(samlAuthenticationResponse.headers['set-cookie'][0])!; }); - it('should properly set cookie and start new SAML handshake', async function () { + it('should properly set cookie and start new SAML handshake', async function() { // Let's delete tokens from `.security` index directly to simulate the case when // Elasticsearch automatically removes access/refresh token document from the index // after some period of time. @@ -527,56 +610,72 @@ export default function ({ getService }) { q: 'doc_type:token', refresh: true, }); - expect(esResponse).to.have.property('deleted').greaterThan(0); + expect(esResponse) + .to.have.property('deleted') + .greaterThan(0); - const handshakeResponse = await supertest.get('/abc/xyz/handshake?one=two three') + const handshakeResponse = await supertest + .get('/abc/xyz/handshake?one=two three') .set('Cookie', sessionCookie.cookieString()) .expect(302); const cookies = handshakeResponse.headers['set-cookie']; expect(cookies).to.have.length(1); - const handshakeCookie = request.cookie(cookies[0]); + const handshakeCookie = request.cookie(cookies[0])!; expect(handshakeCookie.key).to.be('sid'); expect(handshakeCookie.value).to.not.be.empty(); expect(handshakeCookie.path).to.be('/'); expect(handshakeCookie.httpOnly).to.be(true); - const redirectURL = url.parse(handshakeResponse.headers.location, true /* parseQueryString */); - expect(redirectURL.href.startsWith(`https://elastic.co/sso/saml`)).to.be(true); - expect(redirectURL.query.SAMLRequest).to.not.be.empty(); + expect(handshakeResponse.headers.location).to.be( + '/security/api/authc/saml/capture-current-url?currentPath=%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three' + ); }); }); describe('IdP initiated login with active session', () => { const existingUsername = 'a@b.c'; - let existingSessionCookie; + let existingSessionCookie: Cookie; beforeEach(async () => { - const handshakeResponse = await supertest.get('/abc/xyz') + const handshakeResponse = await supertest + .get(`/security/api/authc/saml/start?currentURL=${encodeURIComponent('/abc/xyz')}`) .expect(302); - const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0]); + const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0])!; const samlRequestId = await getSAMLRequestId(handshakeResponse.headers.location); - const samlAuthenticationResponse = await supertest.post('/api/security/v1/saml') + const samlAuthenticationResponse = await supertest + .post('/security/api/authc/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) - .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId, username: existingUsername }) }, {}) + .send({ + SAMLResponse: await createSAMLResponse({ + inResponseTo: samlRequestId, + username: existingUsername, + }), + RelayState: encodeURIComponent('/abc/xyz'), + }) .expect(302); - existingSessionCookie = request.cookie(samlAuthenticationResponse.headers['set-cookie'][0]); + existingSessionCookie = request.cookie( + samlAuthenticationResponse.headers['set-cookie'][0] + )!; }); it('should renew session and redirect to the home page if login is for the same user', async () => { - const samlAuthenticationResponse = await supertest.post('/api/security/v1/saml') + const samlAuthenticationResponse = await supertest + .post('/security/api/authc/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', existingSessionCookie.cookieString()) - .send({ SAMLResponse: await createSAMLResponse({ username: existingUsername }) }, {}) + .send({ SAMLResponse: await createSAMLResponse({ username: existingUsername }) }) .expect('location', '/') .expect(302); - const newSessionCookie = request.cookie(samlAuthenticationResponse.headers['set-cookie'][0]); + const newSessionCookie = request.cookie( + samlAuthenticationResponse.headers['set-cookie'][0] + )!; expect(newSessionCookie.value).to.not.be.empty(); expect(newSessionCookie.value).to.not.equal(existingSessionCookie.value); @@ -586,7 +685,10 @@ export default function ({ getService }) { .set('kbn-xsrf', 'xxx') .set('Cookie', existingSessionCookie.cookieString()) .expect(400); - expect(rejectedResponse.body).to.have.property('message', 'Both access and refresh tokens are expired.'); + expect(rejectedResponse.body).to.have.property( + 'message', + 'Both access and refresh tokens are expired.' + ); // Only tokens from new session are valid. const acceptedResponse = await supertest @@ -599,14 +701,17 @@ export default function ({ getService }) { it('should create a new session and redirect to the `overwritten_session` if login is for another user', async () => { const newUsername = 'c@d.e'; - const samlAuthenticationResponse = await supertest.post('/api/security/v1/saml') + const samlAuthenticationResponse = await supertest + .post('/security/api/authc/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', existingSessionCookie.cookieString()) - .send({ SAMLResponse: await createSAMLResponse({ username: newUsername }) }, {}) + .send({ SAMLResponse: await createSAMLResponse({ username: newUsername }) }) .expect('location', '/overwritten_session') .expect(302); - const newSessionCookie = request.cookie(samlAuthenticationResponse.headers['set-cookie'][0]); + const newSessionCookie = request.cookie( + samlAuthenticationResponse.headers['set-cookie'][0] + )!; expect(newSessionCookie.value).to.not.be.empty(); expect(newSessionCookie.value).to.not.equal(existingSessionCookie.value); @@ -616,7 +721,10 @@ export default function ({ getService }) { .set('kbn-xsrf', 'xxx') .set('Cookie', existingSessionCookie.cookieString()) .expect(400); - expect(rejectedResponse.body).to.have.property('message', 'Both access and refresh tokens are expired.'); + expect(rejectedResponse.body).to.have.property( + 'message', + 'Both access and refresh tokens are expired.' + ); // Only tokens from new session are valid. const acceptedResponse = await supertest diff --git a/x-pack/test/saml_api_integration/config.js b/x-pack/test/saml_api_integration/config.ts similarity index 84% rename from x-pack/test/saml_api_integration/config.js rename to x-pack/test/saml_api_integration/config.ts index 1b262008b6d00e..52a9d89f1d4329 100644 --- a/x-pack/test/saml_api_integration/config.js +++ b/x-pack/test/saml_api_integration/config.ts @@ -5,9 +5,12 @@ */ import { resolve } from 'path'; +import { FtrConfigProviderContext } from '@kbn/test/types/ftr'; -export default async function ({ readConfigFile }) { - const kibanaAPITestsConfig = await readConfigFile(require.resolve('../../../test/api_integration/config.js')); +export default async function({ readConfigFile }: FtrConfigProviderContext) { + const kibanaAPITestsConfig = await readConfigFile( + require.resolve('../../../test/api_integration/config.js') + ); const xPackAPITestsConfig = await readConfigFile(require.resolve('../api_integration/config.js')); const kibanaPort = xPackAPITestsConfig.get('servers.kibana.port'); @@ -36,7 +39,7 @@ export default async function ({ readConfigFile }) { 'xpack.security.authc.realms.saml.saml1.idp.entity_id=http://www.elastic.co', `xpack.security.authc.realms.saml.saml1.sp.entity_id=http://localhost:${kibanaPort}`, `xpack.security.authc.realms.saml.saml1.sp.logout=http://localhost:${kibanaPort}/logout`, - `xpack.security.authc.realms.saml.saml1.sp.acs=http://localhost:${kibanaPort}/api/security/v1/saml`, + `xpack.security.authc.realms.saml.saml1.sp.acs=http://localhost:${kibanaPort}/security/api/authc/saml/callback`, 'xpack.security.authc.realms.saml.saml1.attributes.principal=urn:oid:0.0.7', ], }, @@ -46,7 +49,7 @@ export default async function ({ readConfigFile }) { serverArgs: [ ...xPackAPITestsConfig.get('kbnTestServer.serverArgs'), '--optimize.enabled=false', - '--server.xsrf.whitelist=[\"/api/security/v1/saml\"]', + '--server.xsrf.whitelist=["/security/api/authc/saml/callback"]', `--xpack.security.authc.providers=${JSON.stringify(['saml', 'basic'])}`, '--xpack.security.authc.saml.realm=saml1', ], diff --git a/x-pack/test/saml_api_integration/fixtures/saml_tools.js b/x-pack/test/saml_api_integration/fixtures/saml_tools.ts similarity index 75% rename from x-pack/test/saml_api_integration/fixtures/saml_tools.js rename to x-pack/test/saml_api_integration/fixtures/saml_tools.ts index 7515326e0a3b2c..f8862e6fb209dc 100644 --- a/x-pack/test/saml_api_integration/fixtures/saml_tools.js +++ b/x-pack/test/saml_api_integration/fixtures/saml_tools.ts @@ -9,7 +9,7 @@ import fs from 'fs'; import querystring from 'querystring'; import url from 'url'; import zlib from 'zlib'; -import { promisify } from 'bluebird'; +import { promisify } from 'util'; import { parseString } from 'xml2js'; import { SignedXml } from 'xml-crypto'; @@ -27,18 +27,26 @@ const parseStringAsync = promisify(parseString); const signingKey = fs.readFileSync(require.resolve('../../../../test/dev_certs/server.key')); const signatureAlgorithm = 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha256'; -export async function getSAMLRequestId(urlWithSAMLRequestId) { - const inflatedSAMLRequest = await inflateRawAsync( - Buffer.from(url.parse(urlWithSAMLRequestId, true /* parseQueryString */).query.SAMLRequest, 'base64') - ); +export async function getSAMLRequestId(urlWithSAMLRequestId: string) { + const inflatedSAMLRequest = (await inflateRawAsync( + Buffer.from( + url.parse(urlWithSAMLRequestId, true /* parseQueryString */).query.SAMLRequest as string, + 'base64' + ) + )) as Buffer; - const parsedSAMLRequest = await parseStringAsync(inflatedSAMLRequest.toString()); + const parsedSAMLRequest = (await parseStringAsync(inflatedSAMLRequest.toString())) as any; return parsedSAMLRequest['saml2p:AuthnRequest'].$.ID; } -export async function getSAMLResponse({ destination, inResponseTo, sessionIndex, username = 'a@b.c' } = {}) { - const issueInstant = (new Date()).toISOString(); - const notOnOrAfter = (new Date(Date.now() + 3600 * 1000)).toISOString(); +export async function getSAMLResponse({ + destination, + inResponseTo, + sessionIndex, + username = 'a@b.c', +}: { destination?: string; inResponseTo?: string; sessionIndex?: string; username?: string } = {}) { + const issueInstant = new Date().toISOString(); + const notOnOrAfter = new Date(Date.now() + 3600 * 1000).toISOString(); const samlAssertionTemplateXML = ` ${signature.getSignedXml()} - `).toString('base64'); + ` + ).toString('base64'); } -export async function getLogoutRequest({ destination, sessionIndex }) { - const issueInstant = (new Date()).toISOString(); +export async function getLogoutRequest({ + destination, + sessionIndex, +}: { + destination: string; + sessionIndex: string; +}) { + const issueInstant = new Date().toISOString(); const logoutRequestTemplateXML = ` = { SAMLRequest: deflatedLogoutRequest.toString('base64'), - SigAlg: signatureAlgorithm + SigAlg: signatureAlgorithm, }; const signer = crypto.createSign('RSA-SHA256'); signer.update(querystring.stringify(queryStringParameters)); - queryStringParameters.Signature = signer.sign(signingKey, 'base64'); + queryStringParameters.Signature = signer.sign(signingKey.toString(), 'base64'); return queryStringParameters; } diff --git a/x-pack/test/saml_api_integration/ftr_provider_context.d.ts b/x-pack/test/saml_api_integration/ftr_provider_context.d.ts new file mode 100644 index 00000000000000..e3add3748f56d7 --- /dev/null +++ b/x-pack/test/saml_api_integration/ftr_provider_context.d.ts @@ -0,0 +1,11 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { GenericFtrProviderContext } from '@kbn/test/types/ftr'; + +import { services } from './services'; + +export type FtrProviderContext = GenericFtrProviderContext; diff --git a/x-pack/test/saml_api_integration/services.ts b/x-pack/test/saml_api_integration/services.ts new file mode 100644 index 00000000000000..3cbe8fb37356af --- /dev/null +++ b/x-pack/test/saml_api_integration/services.ts @@ -0,0 +1,13 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { services as apiIntegrationServices } from '../api_integration/services'; + +export const services = { + chance: apiIntegrationServices.chance, + es: apiIntegrationServices.es, + supertestWithoutAuth: apiIntegrationServices.supertestWithoutAuth, +}; diff --git a/yarn.lock b/yarn.lock index f82b220d9c2553..b4b5343f65dec9 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4065,6 +4065,21 @@ "@types/events" "*" "@types/node" "*" +"@types/xml-crypto@^1.4.0": + version "1.4.0" + resolved "https://registry.yarnpkg.com/@types/xml-crypto/-/xml-crypto-1.4.0.tgz#b586e4819f6bdd0571a3faa9a8098049d5c3cc5a" + integrity sha512-F4bCSHdmXgrbLdbQn5kf77U94gb4Ifn8G6u+97BJ5wGzg3xK4uLlJUDFuOIKLf9pZrEGUSSAU/8/0d3GqVXnYQ== + dependencies: + "@types/node" "*" + xpath "0.0.27" + +"@types/xml2js@^0.4.4": + version "0.4.4" + resolved "https://registry.yarnpkg.com/@types/xml2js/-/xml2js-0.4.4.tgz#2093d94359a201806d997dccefc80153db311c66" + integrity sha512-O6Xgai01b9PB3IGA0lRIp1Ex3JBcxGDhdO0n3NIIpCyDOAjxcIGQFmkvgJpP8anTrthxOUQjBfLdRRi0Zn/TXA== + dependencies: + "@types/node" "*" + "@types/yargs-parser@*": version "13.0.0" resolved "https://registry.yarnpkg.com/@types/yargs-parser/-/yargs-parser-13.0.0.tgz#453743c5bbf9f1bed61d959baab5b06be029b2d0" From 1f7faffe67d64272be552f1a14e4ff7660c7737e Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Thu, 5 Sep 2019 20:12:32 +0200 Subject: [PATCH 02/11] Sync with upstream and use route URLs that are relative to root. Use cookie to store `redirectURLPath`. --- docs/security/authentication/index.asciidoc | 2 +- .../server/routes/api/v1/authenticate.js | 22 --- .../authentication/providers/saml.test.ts | 6 +- .../server/authentication/providers/saml.ts | 76 ++++---- x-pack/plugins/security/server/plugin.ts | 1 + .../security/server/routes/authentication.ts | 158 ++++++++++------ .../plugins/security/server/routes/index.ts | 2 + .../apis/security/saml_login.ts | 171 +++++++++++++----- x-pack/test/saml_api_integration/config.ts | 4 +- 9 files changed, 278 insertions(+), 164 deletions(-) diff --git a/docs/security/authentication/index.asciidoc b/docs/security/authentication/index.asciidoc index 3161b146d802a6..5a91ab5be7ced3 100644 --- a/docs/security/authentication/index.asciidoc +++ b/docs/security/authentication/index.asciidoc @@ -100,7 +100,7 @@ xpack.security.authc.saml.realm: realm-name + [source,yaml] -------------------------------------------------------------------------------- -server.xsrf.whitelist: [/security/api/authc/saml/callback] +server.xsrf.whitelist: [/api/security/authc/saml/callback] -------------------------------------------------------------------------------- Users will be able to log in to {kib} via SAML Single Sign-On by navigating directly to the {kib} URL. Users who aren't authenticated are redirected to the Identity Provider for login. Most Identity Providers maintain a long-lived session—users who logged in to a different application using the same Identity Provider in the same browser are automatically authenticated. An exception is if {es} or the Identity Provider is configured to force user to re-authenticate. This login scenario is called _Service Provider initiated login_. diff --git a/x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js b/x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js index 59d29a46b0b7b5..d22cf0aef4db73 100644 --- a/x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js +++ b/x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js @@ -19,28 +19,6 @@ export function initAuthenticateApi({ authc: { login, logout }, config }, server .type(contentType); } - const xsrfWhitelist = server.config().get('server.xsrf.whitelist'); - server.ext('onRequest', (request, h) => { - // We handle this route in new platform plugin, but use different URL. - if (request.path === '/api/security/v1/saml') { - server.log( - ['warning', 'security', 'deprecation'], - `The following URL is deprecated and will stop working in the next major version: ${request.path}` - ); - - // If deprecated route (`/api/security/v1/saml`) is included into XSRF whitelist we should - // implicitly include `/security/api/authc/saml/callback` as well to preserve the same behavior. - // We can't modify config, but we can mark route as ^safe^ via `kbn-xsrf` header. - if (xsrfWhitelist.includes(request.path)) { - request.headers['kbn-xsrf'] = true; - } - - request.setUrl('/security/api/authc/saml/callback'); - } - - return h.continue; - }); - server.route({ method: 'POST', path: '/api/security/v1/login', diff --git a/x-pack/plugins/security/server/authentication/providers/saml.test.ts b/x-pack/plugins/security/server/authentication/providers/saml.test.ts index 463d2ee0ad4f90..320782591eec32 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.test.ts @@ -52,7 +52,7 @@ describe('SAMLAuthenticationProvider', () => { { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml', - nextURL: '/test-base-path/some-path', + redirectURL: '/test-base-path/some-path', }, { requestId: 'some-request-id' } ); @@ -79,7 +79,7 @@ describe('SAMLAuthenticationProvider', () => { { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml', - nextURL: '/test-base-path/some-path', + redirectURL: '/test-base-path/some-path', }, {} ); @@ -153,7 +153,7 @@ describe('SAMLAuthenticationProvider', () => { { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml', - nextURL: '/test-base-path/some-path', + redirectURL: '/test-base-path/some-path', }, { requestId: 'some-request-id' } ); diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts index 18c49ea4f6455a..dace54c30f52c4 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.ts @@ -21,6 +21,11 @@ interface ProviderState extends Partial { * Unique identifier of the SAML request initiated the handshake. */ requestId?: string; + /** + * Stores path component of the URL that was used to initiate SAML handshake and where we + * should redirect user after successful authentication. + */ + redirectURLPath?: string; } /** @@ -32,17 +37,17 @@ export enum SAMLLoginStep { */ SAMLResponseReceived = 'saml-response-received', /** - * The login step when we've captured user URL (including URL fragment) and ready to start SAML handshake. + * The login step when we've captured user URL fragment and ready to start SAML handshake. */ - UserURLCaptured = 'user-url-captured', + RedirectURLFragmentCaptured = 'redirect-url-fragment-captured', } /** * Describes the parameters that are required by the provider to process the initial login request. */ type ProviderLoginAttempt = - | { step: SAMLLoginStep.UserURLCaptured; currentURL: string } - | { step: SAMLLoginStep.SAMLResponseReceived; samlResponse: string; nextURL?: string }; + | { step: SAMLLoginStep.RedirectURLFragmentCaptured; redirectURLFragment: string } + | { step: SAMLLoginStep.SAMLResponseReceived; samlResponse: string; redirectURL?: string }; /** * Checks whether request query includes SAML request from IdP. @@ -87,19 +92,28 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { ) { this.logger.debug('Trying to perform a login.'); - if (attempt.step === SAMLLoginStep.UserURLCaptured) { - this.logger.debug('Captured user URL.'); - return this.authenticateViaHandshake(request, attempt.currentURL); + if (attempt.step === SAMLLoginStep.RedirectURLFragmentCaptured) { + if (!state || !state.redirectURLPath) { + const message = 'State does not include path to redirect to.'; + this.logger.debug(message); + return AuthenticationResult.failed(Boom.badRequest(message)); + } + + this.logger.debug('Captured redirect URL.'); + return this.authenticateViaHandshake( + request, + `${state.redirectURLPath}${attempt.redirectURLFragment}` + ); } - const { samlResponse, nextURL } = attempt; + const { samlResponse, redirectURL } = attempt; const authenticationResult = state ? await this.authenticateViaState(request, state) : AuthenticationResult.notHandled(); // Let's check if user is redirected to Kibana from IdP with valid SAMLResponse. if (authenticationResult.notHandled()) { - return await this.loginWithSAMLResponse(request, { samlResponse, nextURL }, state); + return await this.loginWithSAMLResponse(request, { samlResponse, redirectURL }, state); } if (authenticationResult.succeeded()) { @@ -108,7 +122,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { // we'll re-authenticate user and forward to a page with the respective warning. return await this.loginWithNewSAMLResponse( request, - { samlResponse, nextURL }, + { samlResponse, redirectURL }, (authenticationResult.state || state) as ProviderState, authenticationResult.user as AuthenticatedUser ); @@ -157,7 +171,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { // If we couldn't authenticate by means of all methods above, let's try to capture user URL and // initiate SAML handshake, otherwise just return authentication result we have. return authenticationResult.notHandled() && canRedirectRequest(request) - ? this.captureCurrentURL(request) + ? this.captureRedirectURL(request) : authenticationResult; } @@ -242,12 +256,12 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { * initiated login. * @param request Request instance. * @param samlResponse SAMLResponse payload string. - * @param [nextURL] Optional URL to redirect user to (isn't present for IdP initiated login). + * @param [redirectURL] Optional URL to redirect user to (isn't present for IdP initiated login). * @param [state] Optional state object associated with the provider. */ private async loginWithSAMLResponse( request: KibanaRequest, - { samlResponse, nextURL }: { samlResponse: string; nextURL?: string }, + { samlResponse, redirectURL }: { samlResponse: string; redirectURL?: string }, state?: ProviderState | null ) { this.logger.debug('Trying to log in with SAML response payload.'); @@ -282,9 +296,10 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { }); this.logger.debug('Login has been performed with SAML response.'); - return AuthenticationResult.redirectTo(nextURL || `${this.options.basePath.get(request)}/`, { - state: { accessToken, refreshToken }, - }); + return AuthenticationResult.redirectTo( + redirectURL || `${this.options.basePath.get(request)}/`, + { state: { accessToken, refreshToken } } + ); } catch (err) { this.logger.debug(`Failed to log in with SAML response: ${err.message}`); return AuthenticationResult.failed(err); @@ -301,13 +316,13 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { * we'll forward user to a page with the respective warning. * @param request Request instance. * @param samlResponse SAMLResponse payload string. - * @param [nextURL] Optional URL to redirect user to (isn't present for IdP initiated login). + * @param [redirectURL] Optional URL to redirect user to (isn't present for IdP initiated login). * @param existingState State existing user session is based on. * @param user User returned for the existing session. */ private async loginWithNewSAMLResponse( request: KibanaRequest, - { samlResponse, nextURL }: { samlResponse: string; nextURL?: string }, + { samlResponse, redirectURL }: { samlResponse: string; redirectURL?: string }, existingState: ProviderState, user: AuthenticatedUser ) { @@ -316,7 +331,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { // First let's try to authenticate via SAML Response payload. const payloadAuthenticationResult = await this.loginWithSAMLResponse(request, { samlResponse, - nextURL, + redirectURL, }); if (payloadAuthenticationResult.failed()) { return payloadAuthenticationResult; @@ -433,9 +448,9 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { if (refreshedTokenPair === null) { if (canRedirectRequest(request)) { this.logger.debug( - 'Both access and refresh tokens are expired. Capturing current URL and re-initiating SAML handshake.' + 'Both access and refresh tokens are expired. Capturing redirect URL and re-initiating SAML handshake.' ); - return this.captureCurrentURL(request); + return this.captureRedirectURL(request); } return AuthenticationResult.failed( @@ -460,9 +475,9 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { /** * Tries to start SAML handshake and eventually receive a token. * @param request Request instance. - * @param nextURL URL to redirect user to after successful SAML handshake. + * @param redirectURL URL to redirect user to after successful SAML handshake. */ - private async authenticateViaHandshake(request: KibanaRequest, nextURL: string) { + private async authenticateViaHandshake(request: KibanaRequest, redirectURL: string) { this.logger.debug('Trying to initiate SAML handshake.'); // If client can't handle redirect response, we shouldn't initiate SAML handshake. @@ -484,7 +499,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { // HACK: we manually add a `RelayState` query string parameter with URL to redirect user to after // successful SAML handshake since Elasticsearch doesn't support it yet. We may break something // here eventually if we keep this workaround for too long. - const redirect = `${redirectWithoutRelayState}&RelayState=${encodeURIComponent(nextURL)}`; + const redirect = `${redirectWithoutRelayState}&RelayState=${encodeURIComponent(redirectURL)}`; this.logger.debug('Redirecting to Identity Provider with SAML request.'); @@ -539,17 +554,14 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { } /** - * Redirects user to the client-side page that will grab current URL fragment with the current URL - * path and redirect user back to Kibana to initiate SAML handshake. + * Redirects user to the client-side page that will grab URL fragment and redirect user back to Kibana + * to initiate SAML handshake. * @param request Request instance. */ - private captureCurrentURL(request: KibanaRequest) { + private captureRedirectURL(request: KibanaRequest) { return AuthenticationResult.redirectTo( - `${this.options.basePath.get( - request - )}/security/api/authc/saml/capture-current-url?currentPath=${encodeURIComponent( - request.url.path! - )}` + `${this.options.basePath.get(request)}/api/security/authc/saml/capture-url-fragment`, + { state: { redirectURLPath: request.url.path } } ); } } diff --git a/x-pack/plugins/security/server/plugin.ts b/x-pack/plugins/security/server/plugin.ts index dbbbd20305e67f..8d43cc909f5f62 100644 --- a/x-pack/plugins/security/server/plugin.ts +++ b/x-pack/plugins/security/server/plugin.ts @@ -86,6 +86,7 @@ export class Plugin { logger: this.initializerContext.logger.get('routes'), config, authc, + getLegacyAPI: this.getLegacyAPI, }); return deepFreeze({ diff --git a/x-pack/plugins/security/server/routes/authentication.ts b/x-pack/plugins/security/server/routes/authentication.ts index f79d2df5ceccff..3b63933c65add7 100644 --- a/x-pack/plugins/security/server/routes/authentication.ts +++ b/x-pack/plugins/security/server/routes/authentication.ts @@ -17,49 +17,83 @@ export function defineAuthenticationRoutes(params: RouteDefinitionParams) { /** * Defines routes required for SAML authentication. */ -function defineSAMLRoutes({ basePath, router, logger, authc }: RouteDefinitionParams) { +function defineSAMLRoutes({ + basePath, + router, + logger, + authc, + getLegacyAPI, +}: RouteDefinitionParams) { + function createCustomResourceResponse(body: string, contentType: string) { + return { + body, + headers: { + 'content-type': contentType, + 'cache-control': 'private, no-cache, no-store', + 'content-security-policy': getLegacyAPI().cspRules, + }, + statusCode: 200, + }; + } + router.get( { - path: '/api/authc/saml/capture-current-url', - validate: { query: schema.object({ currentPath: schema.string() }) }, + path: '/api/security/authc/saml/capture-url-fragment', + validate: false, options: { authRequired: false }, }, (context, request, response) => { - const { currentPath } = request.query; - return response.custom({ - body: ` - - Kibana SAML Login - + `, + 'text/html' + ) + ); + } + ); + + router.get( + { + path: '/api/security/authc/saml/capture-url-fragment.js', + validate: false, + options: { authRequired: false }, + }, + (context, request, response) => { + const currentBasePath = basePath.get(request); + return response.custom( + createCustomResourceResponse( + ` window.location.replace( - '${basePath.get( - request - )}/security/api/authc/saml/start?currentURL=' + encodeURIComponent('${currentPath}' + window.location.hash) + '${currentBasePath}/api/security/authc/saml/start?redirectURLFragment=' + encodeURIComponent(window.location.hash) ); - - `, - headers: { - 'content-type': 'text/html', - 'cache-control': 'private, no-cache, no-store', - // HACK: inline scripts aren't allowed yet. - // 'content-security-policy': getLegacyAPI().cspRules, - }, - statusCode: 200, - }); + `, + 'text/javascript' + ) + ); } ); router.get( { - path: '/api/authc/saml/start', - validate: { query: schema.object({ currentURL: schema.string() }) }, + path: '/api/security/authc/saml/start', + validate: { query: schema.object({ redirectURLFragment: schema.string() }) }, options: { authRequired: false }, }, async (context, request, response) => { try { const authenticationResult = await authc.login(request, { provider: 'saml', - value: { step: SAMLLoginStep.UserURLCaptured, currentURL: request.query.currentURL }, + value: { + step: SAMLLoginStep.RedirectURLFragmentCaptured, + redirectURLFragment: request.query.redirectURLFragment, + }, }); // When authenticating using SAML we _expect_ to redirect to the SAML Identity provider. @@ -75,38 +109,52 @@ function defineSAMLRoutes({ basePath, router, logger, authc }: RouteDefinitionPa } ); - router.post( - { - path: '/api/authc/saml/callback', - validate: { - body: schema.object({ - SAMLResponse: schema.string(), - RelayState: schema.maybe(schema.string()), - }), + // Generate two identical routes with new and deprecated URL and issue a warning if route with + // deprecated URL is ever used. + for (const path of ['/api/security/authc/saml/callback', '/api/security/v1/saml']) { + router.post( + { + path, + validate: { + body: schema.object({ + SAMLResponse: schema.string(), + RelayState: schema.maybe(schema.string()), + }), + }, + options: { authRequired: false }, }, - options: { authRequired: false }, - }, - async (context, request, response) => { - try { - // When authenticating using SAML we _expect_ to redirect to the SAML Identity provider. - const authenticationResult = await authc.login(request, { - provider: 'saml', - value: { - step: SAMLLoginStep.SAMLResponseReceived, - samlResponse: request.body.SAMLResponse, - nextURL: request.body.RelayState, - }, - }); + async (context, request, response) => { + try { + if (path === '/api/security/v1/saml') { + const currentBasePath = basePath.get(request); + logger.warn( + `The "${currentBasePath}${path}" URL is deprecated and will stop working in the next major version, please use "${currentBasePath}/api/security/authc/saml/callback" URL instead.`, + { tags: ['deprecation'] } + ); + } - if (authenticationResult.redirected()) { - return response.redirected({ headers: { location: authenticationResult.redirectURL! } }); - } + // When authenticating using SAML we _expect_ to redirect to the SAML Identity provider. + const authenticationResult = await authc.login(request, { + provider: 'saml', + value: { + step: SAMLLoginStep.SAMLResponseReceived, + samlResponse: request.body.SAMLResponse, + redirectURL: request.body.RelayState, + }, + }); - return response.unauthorized({ body: authenticationResult.error }); - } catch (err) { - logger.error(err); - return response.internalError(); + if (authenticationResult.redirected()) { + return response.redirected({ + headers: { location: authenticationResult.redirectURL! }, + }); + } + + return response.unauthorized({ body: authenticationResult.error }); + } catch (err) { + logger.error(err); + return response.internalError(); + } } - } - ); + ); + } } diff --git a/x-pack/plugins/security/server/routes/index.ts b/x-pack/plugins/security/server/routes/index.ts index 8088f44baa1cc6..289f87d70b1dea 100644 --- a/x-pack/plugins/security/server/routes/index.ts +++ b/x-pack/plugins/security/server/routes/index.ts @@ -8,6 +8,7 @@ import { CoreSetup, IRouter, Logger } from '../../../../../src/core/server'; import { Authentication } from '../authentication'; import { ConfigType } from '../config'; import { defineAuthenticationRoutes } from './authentication'; +import { LegacyAPI } from '../plugin'; /** * Describes parameters used to define HTTP routes. @@ -18,6 +19,7 @@ export interface RouteDefinitionParams { logger: Logger; config: ConfigType; authc: Authentication; + getLegacyAPI: () => LegacyAPI; } export function defineRoutes(params: RouteDefinitionParams) { diff --git a/x-pack/test/saml_api_integration/apis/security/saml_login.ts b/x-pack/test/saml_api_integration/apis/security/saml_login.ts index e5fbed1e0dde4d..ea6f6e6316e3e4 100644 --- a/x-pack/test/saml_api_integration/apis/security/saml_login.ts +++ b/x-pack/test/saml_api_integration/apis/security/saml_login.ts @@ -22,7 +22,7 @@ export default function({ getService }: FtrProviderContext) { function createSAMLResponse(options = {}) { return getSAMLResponse({ - destination: `http://localhost:${kibanaServerConfig.port}/security/api/authc/saml/callback`, + destination: `http://localhost:${kibanaServerConfig.port}/api/security/authc/saml/callback`, sessionIndex: chance.natural(), ...options, }); @@ -64,62 +64,88 @@ export default function({ getService }: FtrProviderContext) { expect(user.authentication_realm).to.eql({ name: 'reserved', type: 'reserved' }); }); - describe('capturing current URL', () => { - it('should redirect user to a page that would capture client URL', async () => { + describe('capture URL fragment', () => { + it('should redirect user to a page that would capture URL fragment', async () => { const handshakeResponse = await supertest .get('/abc/xyz/handshake?one=two three') .expect(302); - expect(handshakeResponse.headers['set-cookie']).to.be(undefined); + // The cookie should capture current path. + const cookies = handshakeResponse.headers['set-cookie']; + expect(cookies).to.have.length(1); + + const handshakeCookie = request.cookie(cookies[0])!; + expect(handshakeCookie.key).to.be('sid'); + expect(handshakeCookie.value).to.not.be.empty(); + expect(handshakeCookie.path).to.be('/'); + expect(handshakeCookie.httpOnly).to.be(true); + expect(handshakeResponse.headers.location).to.be( - '/security/api/authc/saml/capture-current-url?currentPath=%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three' + '/api/security/authc/saml/capture-url-fragment' ); }); it('should return an HTML page that will extract URL fragment', async () => { const response = await supertest - .get( - '/security/api/authc/saml/capture-current-url?currentPath=%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three' - ) + .get('/api/security/authc/saml/capture-url-fragment') .expect(200); + + const kibanaBaseURL = url.format({ ...config.get('servers.kibana'), auth: false }); const dom = new JSDOM(response.text, { + url: kibanaBaseURL, runScripts: 'dangerously', + resources: 'usable', beforeParse(window) { // JSDOM doesn't support changing of `window.location` and throws an exception if script - // tries to do that and we have to workaround this behaviour. - Object.defineProperty(window, 'location', { - value: { - hash: '#/workpad', - href: - 'https://kibana.com/security/api/authc/saml/capture-current-url?currentPath=%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three#/workpad', - replace(newLocation: string) { - this.href = newLocation; + // tries to do that and we have to workaround this behaviour. We also need to wait until our + // script is loaded and executed, __isScriptExecuted__ is used exactly for that. + (window as Record).__isScriptExecuted__ = new Promise(resolve => { + Object.defineProperty(window, 'location', { + value: { + hash: '#/workpad', + href: `${kibanaBaseURL}/api/security/authc/saml/capture-url-fragment#/workpad`, + replace(newLocation: string) { + this.href = newLocation; + resolve(); + }, }, - }, + }); }); }, }); + await (dom.window as Record).__isScriptExecuted__; + // Check that proxy page is returned with proper headers. expect(response.headers['content-type']).to.be('text/html; charset=utf-8'); expect(response.headers['cache-control']).to.be('private, no-cache, no-store'); - /* expect(response.headers['content-security-policy']).to.be( + expect(response.headers['content-security-policy']).to.be( `script-src 'unsafe-eval' 'self'; worker-src blob:; child-src blob:; style-src 'unsafe-inline' 'self'` - );*/ + ); // Check that script that forwards URL fragment worked correctly. expect(dom.window.location.href).to.be( - '/security/api/authc/saml/start?currentURL=%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three%23%2Fworkpad' + '/api/security/authc/saml/start?redirectURLFragment=%23%2Fworkpad' ); }); }); describe('initiating handshake', () => { - const encodedNextURL = '%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three%23%2Fworkpad'; - const initiateHandshakeURL = `/security/api/authc/saml/start?currentURL=${encodedNextURL}`; + const encodedRedirectURLFragment = '%23%2Fworkpad'; + const encodedRedirectURLPath = '%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three'; + const initiateHandshakeURL = `/api/security/authc/saml/start?redirectURLFragment=${encodedRedirectURLFragment}`; + + let captureURLCookie: Cookie; + beforeEach(async () => { + const response = await supertest.get('/abc/xyz/handshake?one=two three').expect(302); + captureURLCookie = request.cookie(response.headers['set-cookie'][0])!; + }); it('should properly set cookie and redirect user to IdP', async () => { - const handshakeResponse = await supertest.get(initiateHandshakeURL).expect(302); + const handshakeResponse = await supertest + .get(initiateHandshakeURL) + .set('Cookie', captureURLCookie.cookieString()) + .expect(302); const cookies = handshakeResponse.headers['set-cookie']; expect(cookies).to.have.length(1); @@ -135,12 +161,19 @@ export default function({ getService }: FtrProviderContext) { true /* parseQueryString */ ); expect(redirectURL.href!.startsWith(`https://elastic.co/sso/saml`)).to.be(true); - expect(redirectURL.href!.endsWith(`&RelayState=${encodedNextURL}`)).to.be(true); + expect( + redirectURL.href!.endsWith( + `&RelayState=${encodedRedirectURLPath}${encodedRedirectURLFragment}` + ) + ).to.be(true); expect(redirectURL.query.SAMLRequest).to.not.be.empty(); }); it('should not allow access to the API', async () => { - const handshakeResponse = await supertest.get(initiateHandshakeURL).expect(302); + const handshakeResponse = await supertest + .get(initiateHandshakeURL) + .set('Cookie', captureURLCookie.cookieString()) + .expect(302); const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0])!; await supertest @@ -154,6 +187,7 @@ export default function({ getService }: FtrProviderContext) { const ajaxResponse = await supertest .get(initiateHandshakeURL) .set('kbn-xsrf', 'xxx') + .set('Cookie', captureURLCookie.cookieString()) .expect(401); expect(ajaxResponse.headers['set-cookie']).to.be(undefined); @@ -161,13 +195,20 @@ export default function({ getService }: FtrProviderContext) { }); describe('finishing handshake', () => { - const encodedNextURL = '%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three%23%2Fworkpad'; + const encodedRedirectURLFragment = '%23%2Fworkpad'; + const encodedRedirectURL = `%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three${encodedRedirectURLFragment}`; let handshakeCookie: Cookie; let samlRequestId: string; beforeEach(async () => { + const captureURLResponse = await supertest + .get('/abc/xyz/handshake?one=two three') + .expect(302); + const captureURLCookie = request.cookie(captureURLResponse.headers['set-cookie'][0])!; + const handshakeResponse = await supertest - .get(`/security/api/authc/saml/start?currentURL=${encodedNextURL}`) + .get(`/api/security/authc/saml/start?redirectURLFragment=${encodedRedirectURLFragment}`) + .set('Cookie', captureURLCookie.cookieString()) .expect(302); handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0])!; @@ -176,7 +217,7 @@ export default function({ getService }: FtrProviderContext) { it('should fail if SAML response is not complemented with handshake cookie', async () => { await supertest - .post('/security/api/authc/saml/callback') + .post('/api/security/authc/saml/callback') .set('kbn-xsrf', 'xxx') .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }) }) .expect(401); @@ -184,17 +225,17 @@ export default function({ getService }: FtrProviderContext) { it('should succeed if both SAML response and handshake cookie are provided', async () => { const samlAuthenticationResponse = await supertest - .post('/security/api/authc/saml/callback') + .post('/api/security/authc/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }), - RelayState: encodedNextURL, + RelayState: encodedRedirectURL, }) .expect(302); // User should be redirected to the URL that initiated handshake. - expect(samlAuthenticationResponse.headers.location).to.be(encodedNextURL); + expect(samlAuthenticationResponse.headers.location).to.be(encodedRedirectURL); const cookies = samlAuthenticationResponse.headers['set-cookie']; expect(cookies).to.have.length(1); @@ -229,7 +270,7 @@ export default function({ getService }: FtrProviderContext) { // Don't pass handshake cookie, RelayState query string parameter and don't include // `inResponseTo` into SAML response to simulate IdP initiated login. const samlAuthenticationResponse = await supertest - .post('/security/api/authc/saml/callback') + .post('/api/security/authc/saml/callback') .set('kbn-xsrf', 'xxx') .send({ SAMLResponse: await createSAMLResponse() }) .expect(302); @@ -268,7 +309,7 @@ export default function({ getService }: FtrProviderContext) { it('should fail if SAML response is not valid', async () => { await supertest - .post('/security/api/authc/saml/callback') + .post('/api/security/authc/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) .send({ @@ -284,7 +325,7 @@ export default function({ getService }: FtrProviderContext) { beforeEach(async () => { // Imitate IdP initiated login. const samlAuthenticationResponse = await supertest - .post('/security/api/authc/saml/callback') + .post('/api/security/authc/saml/callback') .set('kbn-xsrf', 'xxx') .send({ SAMLResponse: await createSAMLResponse() }) .expect(302); @@ -346,8 +387,16 @@ export default function({ getService }: FtrProviderContext) { let idpSessionIndex: string; beforeEach(async () => { + const captureURLResponse = await supertest + .get('/abc/xyz/handshake?one=two three') + .expect(302); + const captureURLCookie = request.cookie(captureURLResponse.headers['set-cookie'][0])!; + const handshakeResponse = await supertest - .get(`/security/api/authc/saml/start?currentURL=${encodeURIComponent('/abc/xyz')}`) + .get( + `/api/security/authc/saml/start?redirectURLFragment=${encodeURIComponent('#workpad')}` + ) + .set('Cookie', captureURLCookie.cookieString()) .expect(302); const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0])!; @@ -355,7 +404,7 @@ export default function({ getService }: FtrProviderContext) { idpSessionIndex = chance.natural(); const samlAuthenticationResponse = await supertest - .post('/security/api/authc/saml/callback') + .post('/api/security/authc/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) .send({ @@ -363,7 +412,7 @@ export default function({ getService }: FtrProviderContext) { inResponseTo: samlRequestId, sessionIndex: idpSessionIndex, }), - RelayState: encodeURIComponent('/abc/xyz'), + RelayState: encodeURIComponent('/abc/xyz#workpad'), }) .expect(302); @@ -496,20 +545,28 @@ export default function({ getService }: FtrProviderContext) { let sessionCookie: Cookie; beforeEach(async () => { + const captureURLResponse = await supertest + .get('/abc/xyz/handshake?one=two three') + .expect(302); + const captureURLCookie = request.cookie(captureURLResponse.headers['set-cookie'][0])!; + const handshakeResponse = await supertest - .get(`/security/api/authc/saml/start?currentURL=${encodeURIComponent('/abc/xyz')}`) + .get( + `/api/security/authc/saml/start?redirectURLFragment=${encodeURIComponent('#workpad')}` + ) + .set('Cookie', captureURLCookie.cookieString()) .expect(302); const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0])!; const samlRequestId = await getSAMLRequestId(handshakeResponse.headers.location); const samlAuthenticationResponse = await supertest - .post('/security/api/authc/saml/callback') + .post('/api/security/authc/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }), - RelayState: encodeURIComponent('/abc/xyz'), + RelayState: encodeURIComponent('/abc/xyz#workpad'), }) .expect(302); @@ -581,20 +638,28 @@ export default function({ getService }: FtrProviderContext) { let sessionCookie: Cookie; beforeEach(async () => { + const captureURLResponse = await supertest + .get('/abc/xyz/handshake?one=two three') + .expect(302); + const captureURLCookie = request.cookie(captureURLResponse.headers['set-cookie'][0])!; + const handshakeResponse = await supertest - .get(`/security/api/authc/saml/start?currentURL=${encodeURIComponent('/abc/xyz')}`) + .get( + `/api/security/authc/saml/start?redirectURLFragment=${encodeURIComponent('#workpad')}` + ) + .set('Cookie', captureURLCookie.cookieString()) .expect(302); const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0])!; const samlRequestId = await getSAMLRequestId(handshakeResponse.headers.location); const samlAuthenticationResponse = await supertest - .post('/security/api/authc/saml/callback') + .post('/api/security/authc/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }), - RelayState: encodeURIComponent('/abc/xyz'), + RelayState: encodeURIComponent('/abc/xyz#workpad'), }) .expect(302); @@ -629,7 +694,7 @@ export default function({ getService }: FtrProviderContext) { expect(handshakeCookie.httpOnly).to.be(true); expect(handshakeResponse.headers.location).to.be( - '/security/api/authc/saml/capture-current-url?currentPath=%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three' + '/api/security/authc/saml/capture-url-fragment' ); }); }); @@ -639,15 +704,23 @@ export default function({ getService }: FtrProviderContext) { let existingSessionCookie: Cookie; beforeEach(async () => { + const captureURLResponse = await supertest + .get('/abc/xyz/handshake?one=two three') + .expect(302); + const captureURLCookie = request.cookie(captureURLResponse.headers['set-cookie'][0])!; + const handshakeResponse = await supertest - .get(`/security/api/authc/saml/start?currentURL=${encodeURIComponent('/abc/xyz')}`) + .get( + `/api/security/authc/saml/start?redirectURLFragment=${encodeURIComponent('#workpad')}` + ) + .set('Cookie', captureURLCookie.cookieString()) .expect(302); const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0])!; const samlRequestId = await getSAMLRequestId(handshakeResponse.headers.location); const samlAuthenticationResponse = await supertest - .post('/security/api/authc/saml/callback') + .post('/api/security/authc/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) .send({ @@ -655,7 +728,7 @@ export default function({ getService }: FtrProviderContext) { inResponseTo: samlRequestId, username: existingUsername, }), - RelayState: encodeURIComponent('/abc/xyz'), + RelayState: encodeURIComponent('/abc/xyz#workpad'), }) .expect(302); @@ -666,7 +739,7 @@ export default function({ getService }: FtrProviderContext) { it('should renew session and redirect to the home page if login is for the same user', async () => { const samlAuthenticationResponse = await supertest - .post('/security/api/authc/saml/callback') + .post('/api/security/authc/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', existingSessionCookie.cookieString()) .send({ SAMLResponse: await createSAMLResponse({ username: existingUsername }) }) @@ -702,7 +775,7 @@ export default function({ getService }: FtrProviderContext) { it('should create a new session and redirect to the `overwritten_session` if login is for another user', async () => { const newUsername = 'c@d.e'; const samlAuthenticationResponse = await supertest - .post('/security/api/authc/saml/callback') + .post('/api/security/authc/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', existingSessionCookie.cookieString()) .send({ SAMLResponse: await createSAMLResponse({ username: newUsername }) }) diff --git a/x-pack/test/saml_api_integration/config.ts b/x-pack/test/saml_api_integration/config.ts index 52a9d89f1d4329..a92623cbc2704c 100644 --- a/x-pack/test/saml_api_integration/config.ts +++ b/x-pack/test/saml_api_integration/config.ts @@ -39,7 +39,7 @@ export default async function({ readConfigFile }: FtrConfigProviderContext) { 'xpack.security.authc.realms.saml.saml1.idp.entity_id=http://www.elastic.co', `xpack.security.authc.realms.saml.saml1.sp.entity_id=http://localhost:${kibanaPort}`, `xpack.security.authc.realms.saml.saml1.sp.logout=http://localhost:${kibanaPort}/logout`, - `xpack.security.authc.realms.saml.saml1.sp.acs=http://localhost:${kibanaPort}/security/api/authc/saml/callback`, + `xpack.security.authc.realms.saml.saml1.sp.acs=http://localhost:${kibanaPort}/api/security/authc/saml/callback`, 'xpack.security.authc.realms.saml.saml1.attributes.principal=urn:oid:0.0.7', ], }, @@ -49,7 +49,7 @@ export default async function({ readConfigFile }: FtrConfigProviderContext) { serverArgs: [ ...xPackAPITestsConfig.get('kbnTestServer.serverArgs'), '--optimize.enabled=false', - '--server.xsrf.whitelist=["/security/api/authc/saml/callback"]', + '--server.xsrf.whitelist=["/api/security/authc/saml/callback"]', `--xpack.security.authc.providers=${JSON.stringify(['saml', 'basic'])}`, '--xpack.security.authc.saml.realm=saml1', ], From f564987bbc13b4d8908e62896fc40a840d32939a Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Thu, 12 Sep 2019 12:09:37 +0200 Subject: [PATCH 03/11] Review#1: get rid of `authc` part in SAML API routes. --- docs/security/authentication/index.asciidoc | 2 +- .../server/authentication/providers/saml.ts | 2 +- .../security/server/routes/authentication.ts | 14 ++--- .../apis/security/saml_login.ts | 60 +++++++------------ x-pack/test/saml_api_integration/config.ts | 4 +- 5 files changed, 34 insertions(+), 48 deletions(-) diff --git a/docs/security/authentication/index.asciidoc b/docs/security/authentication/index.asciidoc index 5a91ab5be7ced3..c0f078df2c00a3 100644 --- a/docs/security/authentication/index.asciidoc +++ b/docs/security/authentication/index.asciidoc @@ -100,7 +100,7 @@ xpack.security.authc.saml.realm: realm-name + [source,yaml] -------------------------------------------------------------------------------- -server.xsrf.whitelist: [/api/security/authc/saml/callback] +server.xsrf.whitelist: [/api/security/saml/callback] -------------------------------------------------------------------------------- Users will be able to log in to {kib} via SAML Single Sign-On by navigating directly to the {kib} URL. Users who aren't authenticated are redirected to the Identity Provider for login. Most Identity Providers maintain a long-lived session—users who logged in to a different application using the same Identity Provider in the same browser are automatically authenticated. An exception is if {es} or the Identity Provider is configured to force user to re-authenticate. This login scenario is called _Service Provider initiated login_. diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts index dace54c30f52c4..bfbc10dce6c26a 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.ts @@ -560,7 +560,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { */ private captureRedirectURL(request: KibanaRequest) { return AuthenticationResult.redirectTo( - `${this.options.basePath.get(request)}/api/security/authc/saml/capture-url-fragment`, + `${this.options.basePath.get(request)}/api/security/saml/capture-url-fragment`, { state: { redirectURLPath: request.url.path } } ); } diff --git a/x-pack/plugins/security/server/routes/authentication.ts b/x-pack/plugins/security/server/routes/authentication.ts index 3b63933c65add7..6204df8bb0a573 100644 --- a/x-pack/plugins/security/server/routes/authentication.ts +++ b/x-pack/plugins/security/server/routes/authentication.ts @@ -38,7 +38,7 @@ function defineSAMLRoutes({ router.get( { - path: '/api/security/authc/saml/capture-url-fragment', + path: '/api/security/saml/capture-url-fragment', validate: false, options: { authRequired: false }, }, @@ -51,7 +51,7 @@ function defineSAMLRoutes({ Kibana SAML Login - + `, 'text/html' ) @@ -61,7 +61,7 @@ function defineSAMLRoutes({ router.get( { - path: '/api/security/authc/saml/capture-url-fragment.js', + path: '/api/security/saml/capture-url-fragment.js', validate: false, options: { authRequired: false }, }, @@ -71,7 +71,7 @@ function defineSAMLRoutes({ createCustomResourceResponse( ` window.location.replace( - '${currentBasePath}/api/security/authc/saml/start?redirectURLFragment=' + encodeURIComponent(window.location.hash) + '${currentBasePath}/api/security/saml/start?redirectURLFragment=' + encodeURIComponent(window.location.hash) ); `, 'text/javascript' @@ -82,7 +82,7 @@ function defineSAMLRoutes({ router.get( { - path: '/api/security/authc/saml/start', + path: '/api/security/saml/start', validate: { query: schema.object({ redirectURLFragment: schema.string() }) }, options: { authRequired: false }, }, @@ -111,7 +111,7 @@ function defineSAMLRoutes({ // Generate two identical routes with new and deprecated URL and issue a warning if route with // deprecated URL is ever used. - for (const path of ['/api/security/authc/saml/callback', '/api/security/v1/saml']) { + for (const path of ['/api/security/saml/callback', '/api/security/v1/saml']) { router.post( { path, @@ -128,7 +128,7 @@ function defineSAMLRoutes({ if (path === '/api/security/v1/saml') { const currentBasePath = basePath.get(request); logger.warn( - `The "${currentBasePath}${path}" URL is deprecated and will stop working in the next major version, please use "${currentBasePath}/api/security/authc/saml/callback" URL instead.`, + `The "${currentBasePath}${path}" URL is deprecated and will stop working in the next major version, please use "${currentBasePath}/api/security/saml/callback" URL instead.`, { tags: ['deprecation'] } ); } diff --git a/x-pack/test/saml_api_integration/apis/security/saml_login.ts b/x-pack/test/saml_api_integration/apis/security/saml_login.ts index ea6f6e6316e3e4..4008a90fadeac6 100644 --- a/x-pack/test/saml_api_integration/apis/security/saml_login.ts +++ b/x-pack/test/saml_api_integration/apis/security/saml_login.ts @@ -22,7 +22,7 @@ export default function({ getService }: FtrProviderContext) { function createSAMLResponse(options = {}) { return getSAMLResponse({ - destination: `http://localhost:${kibanaServerConfig.port}/api/security/authc/saml/callback`, + destination: `http://localhost:${kibanaServerConfig.port}/api/security/saml/callback`, sessionIndex: chance.natural(), ...options, }); @@ -80,15 +80,11 @@ export default function({ getService }: FtrProviderContext) { expect(handshakeCookie.path).to.be('/'); expect(handshakeCookie.httpOnly).to.be(true); - expect(handshakeResponse.headers.location).to.be( - '/api/security/authc/saml/capture-url-fragment' - ); + expect(handshakeResponse.headers.location).to.be('/api/security/saml/capture-url-fragment'); }); it('should return an HTML page that will extract URL fragment', async () => { - const response = await supertest - .get('/api/security/authc/saml/capture-url-fragment') - .expect(200); + const response = await supertest.get('/api/security/saml/capture-url-fragment').expect(200); const kibanaBaseURL = url.format({ ...config.get('servers.kibana'), auth: false }); const dom = new JSDOM(response.text, { @@ -103,7 +99,7 @@ export default function({ getService }: FtrProviderContext) { Object.defineProperty(window, 'location', { value: { hash: '#/workpad', - href: `${kibanaBaseURL}/api/security/authc/saml/capture-url-fragment#/workpad`, + href: `${kibanaBaseURL}/api/security/saml/capture-url-fragment#/workpad`, replace(newLocation: string) { this.href = newLocation; resolve(); @@ -125,7 +121,7 @@ export default function({ getService }: FtrProviderContext) { // Check that script that forwards URL fragment worked correctly. expect(dom.window.location.href).to.be( - '/api/security/authc/saml/start?redirectURLFragment=%23%2Fworkpad' + '/api/security/saml/start?redirectURLFragment=%23%2Fworkpad' ); }); }); @@ -133,7 +129,7 @@ export default function({ getService }: FtrProviderContext) { describe('initiating handshake', () => { const encodedRedirectURLFragment = '%23%2Fworkpad'; const encodedRedirectURLPath = '%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three'; - const initiateHandshakeURL = `/api/security/authc/saml/start?redirectURLFragment=${encodedRedirectURLFragment}`; + const initiateHandshakeURL = `/api/security/saml/start?redirectURLFragment=${encodedRedirectURLFragment}`; let captureURLCookie: Cookie; beforeEach(async () => { @@ -207,7 +203,7 @@ export default function({ getService }: FtrProviderContext) { const captureURLCookie = request.cookie(captureURLResponse.headers['set-cookie'][0])!; const handshakeResponse = await supertest - .get(`/api/security/authc/saml/start?redirectURLFragment=${encodedRedirectURLFragment}`) + .get(`/api/security/saml/start?redirectURLFragment=${encodedRedirectURLFragment}`) .set('Cookie', captureURLCookie.cookieString()) .expect(302); @@ -217,7 +213,7 @@ export default function({ getService }: FtrProviderContext) { it('should fail if SAML response is not complemented with handshake cookie', async () => { await supertest - .post('/api/security/authc/saml/callback') + .post('/api/security/saml/callback') .set('kbn-xsrf', 'xxx') .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }) }) .expect(401); @@ -225,7 +221,7 @@ export default function({ getService }: FtrProviderContext) { it('should succeed if both SAML response and handshake cookie are provided', async () => { const samlAuthenticationResponse = await supertest - .post('/api/security/authc/saml/callback') + .post('/api/security/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) .send({ @@ -270,7 +266,7 @@ export default function({ getService }: FtrProviderContext) { // Don't pass handshake cookie, RelayState query string parameter and don't include // `inResponseTo` into SAML response to simulate IdP initiated login. const samlAuthenticationResponse = await supertest - .post('/api/security/authc/saml/callback') + .post('/api/security/saml/callback') .set('kbn-xsrf', 'xxx') .send({ SAMLResponse: await createSAMLResponse() }) .expect(302); @@ -309,7 +305,7 @@ export default function({ getService }: FtrProviderContext) { it('should fail if SAML response is not valid', async () => { await supertest - .post('/api/security/authc/saml/callback') + .post('/api/security/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) .send({ @@ -325,7 +321,7 @@ export default function({ getService }: FtrProviderContext) { beforeEach(async () => { // Imitate IdP initiated login. const samlAuthenticationResponse = await supertest - .post('/api/security/authc/saml/callback') + .post('/api/security/saml/callback') .set('kbn-xsrf', 'xxx') .send({ SAMLResponse: await createSAMLResponse() }) .expect(302); @@ -393,9 +389,7 @@ export default function({ getService }: FtrProviderContext) { const captureURLCookie = request.cookie(captureURLResponse.headers['set-cookie'][0])!; const handshakeResponse = await supertest - .get( - `/api/security/authc/saml/start?redirectURLFragment=${encodeURIComponent('#workpad')}` - ) + .get(`/api/security/saml/start?redirectURLFragment=${encodeURIComponent('#workpad')}`) .set('Cookie', captureURLCookie.cookieString()) .expect(302); @@ -404,7 +398,7 @@ export default function({ getService }: FtrProviderContext) { idpSessionIndex = chance.natural(); const samlAuthenticationResponse = await supertest - .post('/api/security/authc/saml/callback') + .post('/api/security/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) .send({ @@ -551,9 +545,7 @@ export default function({ getService }: FtrProviderContext) { const captureURLCookie = request.cookie(captureURLResponse.headers['set-cookie'][0])!; const handshakeResponse = await supertest - .get( - `/api/security/authc/saml/start?redirectURLFragment=${encodeURIComponent('#workpad')}` - ) + .get(`/api/security/saml/start?redirectURLFragment=${encodeURIComponent('#workpad')}`) .set('Cookie', captureURLCookie.cookieString()) .expect(302); @@ -561,7 +553,7 @@ export default function({ getService }: FtrProviderContext) { const samlRequestId = await getSAMLRequestId(handshakeResponse.headers.location); const samlAuthenticationResponse = await supertest - .post('/api/security/authc/saml/callback') + .post('/api/security/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) .send({ @@ -644,9 +636,7 @@ export default function({ getService }: FtrProviderContext) { const captureURLCookie = request.cookie(captureURLResponse.headers['set-cookie'][0])!; const handshakeResponse = await supertest - .get( - `/api/security/authc/saml/start?redirectURLFragment=${encodeURIComponent('#workpad')}` - ) + .get(`/api/security/saml/start?redirectURLFragment=${encodeURIComponent('#workpad')}`) .set('Cookie', captureURLCookie.cookieString()) .expect(302); @@ -654,7 +644,7 @@ export default function({ getService }: FtrProviderContext) { const samlRequestId = await getSAMLRequestId(handshakeResponse.headers.location); const samlAuthenticationResponse = await supertest - .post('/api/security/authc/saml/callback') + .post('/api/security/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) .send({ @@ -693,9 +683,7 @@ export default function({ getService }: FtrProviderContext) { expect(handshakeCookie.path).to.be('/'); expect(handshakeCookie.httpOnly).to.be(true); - expect(handshakeResponse.headers.location).to.be( - '/api/security/authc/saml/capture-url-fragment' - ); + expect(handshakeResponse.headers.location).to.be('/api/security/saml/capture-url-fragment'); }); }); @@ -710,9 +698,7 @@ export default function({ getService }: FtrProviderContext) { const captureURLCookie = request.cookie(captureURLResponse.headers['set-cookie'][0])!; const handshakeResponse = await supertest - .get( - `/api/security/authc/saml/start?redirectURLFragment=${encodeURIComponent('#workpad')}` - ) + .get(`/api/security/saml/start?redirectURLFragment=${encodeURIComponent('#workpad')}`) .set('Cookie', captureURLCookie.cookieString()) .expect(302); @@ -720,7 +706,7 @@ export default function({ getService }: FtrProviderContext) { const samlRequestId = await getSAMLRequestId(handshakeResponse.headers.location); const samlAuthenticationResponse = await supertest - .post('/api/security/authc/saml/callback') + .post('/api/security/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) .send({ @@ -739,7 +725,7 @@ export default function({ getService }: FtrProviderContext) { it('should renew session and redirect to the home page if login is for the same user', async () => { const samlAuthenticationResponse = await supertest - .post('/api/security/authc/saml/callback') + .post('/api/security/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', existingSessionCookie.cookieString()) .send({ SAMLResponse: await createSAMLResponse({ username: existingUsername }) }) @@ -775,7 +761,7 @@ export default function({ getService }: FtrProviderContext) { it('should create a new session and redirect to the `overwritten_session` if login is for another user', async () => { const newUsername = 'c@d.e'; const samlAuthenticationResponse = await supertest - .post('/api/security/authc/saml/callback') + .post('/api/security/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', existingSessionCookie.cookieString()) .send({ SAMLResponse: await createSAMLResponse({ username: newUsername }) }) diff --git a/x-pack/test/saml_api_integration/config.ts b/x-pack/test/saml_api_integration/config.ts index a92623cbc2704c..a9edd3864919b4 100644 --- a/x-pack/test/saml_api_integration/config.ts +++ b/x-pack/test/saml_api_integration/config.ts @@ -39,7 +39,7 @@ export default async function({ readConfigFile }: FtrConfigProviderContext) { 'xpack.security.authc.realms.saml.saml1.idp.entity_id=http://www.elastic.co', `xpack.security.authc.realms.saml.saml1.sp.entity_id=http://localhost:${kibanaPort}`, `xpack.security.authc.realms.saml.saml1.sp.logout=http://localhost:${kibanaPort}/logout`, - `xpack.security.authc.realms.saml.saml1.sp.acs=http://localhost:${kibanaPort}/api/security/authc/saml/callback`, + `xpack.security.authc.realms.saml.saml1.sp.acs=http://localhost:${kibanaPort}/api/security/saml/callback`, 'xpack.security.authc.realms.saml.saml1.attributes.principal=urn:oid:0.0.7', ], }, @@ -49,7 +49,7 @@ export default async function({ readConfigFile }: FtrConfigProviderContext) { serverArgs: [ ...xPackAPITestsConfig.get('kbnTestServer.serverArgs'), '--optimize.enabled=false', - '--server.xsrf.whitelist=["/api/security/authc/saml/callback"]', + '--server.xsrf.whitelist=["/api/security/saml/callback"]', `--xpack.security.authc.providers=${JSON.stringify(['saml', 'basic'])}`, '--xpack.security.authc.saml.realm=saml1', ], From 776ca981c7d48f7a6654c189f274a74c497b7fc2 Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Thu, 12 Sep 2019 12:40:00 +0200 Subject: [PATCH 04/11] Review#1: capture basepath and space ID. --- .../security/server/authentication/providers/saml.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts index bfbc10dce6c26a..7ff66e6728d9cd 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.ts @@ -559,9 +559,9 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { * @param request Request instance. */ private captureRedirectURL(request: KibanaRequest) { - return AuthenticationResult.redirectTo( - `${this.options.basePath.get(request)}/api/security/saml/capture-url-fragment`, - { state: { redirectURLPath: request.url.path } } - ); + const basePath = this.options.basePath.get(request); + return AuthenticationResult.redirectTo(`${basePath}/api/security/saml/capture-url-fragment`, { + state: { redirectURLPath: `${basePath}${request.url.path}` }, + }); } } From 8270f194d39ba1b253db03eda78b25b84c2cbe92 Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Thu, 12 Sep 2019 15:02:32 +0200 Subject: [PATCH 05/11] Review#1: migrate to alternative solution to store full URL in the cookie instead. --- docs/security/authentication/index.asciidoc | 13 +++ .../resources/bin/kibana-docker | 1 + .../server/authentication/providers/saml.ts | 87 +++++++++++-------- x-pack/plugins/security/server/config.ts | 8 +- .../security/server/routes/authentication.ts | 1 - 5 files changed, 70 insertions(+), 40 deletions(-) diff --git a/docs/security/authentication/index.asciidoc b/docs/security/authentication/index.asciidoc index c0f078df2c00a3..b0475b81766470 100644 --- a/docs/security/authentication/index.asciidoc +++ b/docs/security/authentication/index.asciidoc @@ -119,6 +119,19 @@ The order of `saml` and `basic` is important. Users who open {kib} will go throu Basic authentication is supported _only_ if `basic` authentication provider is explicitly declared in `xpack.security.authc.providers` setting in addition to `saml`. +[float] +===== SAML and long URLs + +At the beginning of the SAML handshake {kib} stores initial URL in the session cookie to be able to redirect user back to this URL after successful SAML authentication. +In case the URL is very long session cookie may exceed the maximum size supported by the browser (usually it's around 4KB for all cookies per domain) and it will truncate +or drop such cookie completely. If you experience sporadic failures during SAML authentication that may be the reason why. To remedy this issue you need to decrease the maximum +size of the URL {kib} is allowed to store during SAML handshake, the default value is 2KB: + +[source,yaml] +-------------------------------------------------------------------------------- +xpack.security.authc.saml.maxURLToStoreSize: 1kb +-------------------------------------------------------------------------------- + [[oidc]] ==== OpenID Connect Single Sign-On diff --git a/src/dev/build/tasks/os_packages/docker_generator/resources/bin/kibana-docker b/src/dev/build/tasks/os_packages/docker_generator/resources/bin/kibana-docker index 1ec359d881972b..57f9bf22ab9d24 100755 --- a/src/dev/build/tasks/os_packages/docker_generator/resources/bin/kibana-docker +++ b/src/dev/build/tasks/os_packages/docker_generator/resources/bin/kibana-docker @@ -147,6 +147,7 @@ kibana_vars=( xpack.security.authc.providers xpack.security.authc.oidc.realm xpack.security.authc.saml.realm + xpack.security.authc.saml.maxURLToStoreSize xpack.security.cookieName xpack.security.enabled xpack.security.encryptionKey diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts index 7ff66e6728d9cd..6b1f31f5c51496 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.ts @@ -5,6 +5,7 @@ */ import Boom from 'boom'; +import { ByteSizeValue } from '@kbn/config-schema'; import { KibanaRequest } from '../../../../../../src/core/server'; import { AuthenticatedUser } from '../../../common/model'; import { AuthenticationResult } from '../authentication_result'; @@ -22,10 +23,10 @@ interface ProviderState extends Partial { */ requestId?: string; /** - * Stores path component of the URL that was used to initiate SAML handshake and where we - * should redirect user after successful authentication. + * Stores path component of the URL only or in a combination with URL fragment that was used to + * initiate SAML handshake and where we should redirect user after successful authentication. */ - redirectURLPath?: string; + redirectURL?: string; } /** @@ -47,7 +48,7 @@ export enum SAMLLoginStep { */ type ProviderLoginAttempt = | { step: SAMLLoginStep.RedirectURLFragmentCaptured; redirectURLFragment: string } - | { step: SAMLLoginStep.SAMLResponseReceived; samlResponse: string; redirectURL?: string }; + | { step: SAMLLoginStep.SAMLResponseReceived; samlResponse: string }; /** * Checks whether request query includes SAML request from IdP. @@ -66,9 +67,14 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { */ private readonly realm: string; + /** + * Maximum size of the URL we store in the session during SAML handshake. + */ + private readonly maxURLToStoreSize: ByteSizeValue; + constructor( protected readonly options: Readonly, - samlOptions?: Readonly<{ realm?: string }> + samlOptions?: Readonly<{ realm?: string; maxURLToStoreSize?: ByteSizeValue }> ) { super(options); @@ -76,7 +82,12 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { throw new Error('Realm name must be specified'); } + if (!samlOptions.maxURLToStoreSize) { + throw new Error(`maxURLToStoreSize must be specified`); + } + this.realm = samlOptions.realm; + this.maxURLToStoreSize = samlOptions.maxURLToStoreSize; } /** @@ -93,27 +104,34 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { this.logger.debug('Trying to perform a login.'); if (attempt.step === SAMLLoginStep.RedirectURLFragmentCaptured) { - if (!state || !state.redirectURLPath) { + if (!state || !state.redirectURL) { const message = 'State does not include path to redirect to.'; this.logger.debug(message); return AuthenticationResult.failed(Boom.badRequest(message)); } - this.logger.debug('Captured redirect URL.'); - return this.authenticateViaHandshake( - request, - `${state.redirectURLPath}${attempt.redirectURLFragment}` - ); + let redirectURL = state.redirectURL + attempt.redirectURLFragment; + const redirectURLSize = new ByteSizeValue(Buffer.byteLength(redirectURL)); + if (this.maxURLToStoreSize.isLessThan(redirectURLSize)) { + this.logger.warn( + `Max URL size should not exceed ${this.maxURLToStoreSize.toString()} but it was ${redirectURLSize.toString()}. Only URL path is captured.` + ); + redirectURL = state.redirectURL; + } else { + this.logger.debug('Captured redirect URL.'); + } + + return this.authenticateViaHandshake(request, redirectURL); } - const { samlResponse, redirectURL } = attempt; + const { samlResponse } = attempt; const authenticationResult = state ? await this.authenticateViaState(request, state) : AuthenticationResult.notHandled(); // Let's check if user is redirected to Kibana from IdP with valid SAMLResponse. if (authenticationResult.notHandled()) { - return await this.loginWithSAMLResponse(request, { samlResponse, redirectURL }, state); + return await this.loginWithSAMLResponse(request, samlResponse, state); } if (authenticationResult.succeeded()) { @@ -122,7 +140,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { // we'll re-authenticate user and forward to a page with the respective warning. return await this.loginWithNewSAMLResponse( request, - { samlResponse, redirectURL }, + samlResponse, (authenticationResult.state || state) as ProviderState, authenticationResult.user as AuthenticatedUser ); @@ -256,20 +274,22 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { * initiated login. * @param request Request instance. * @param samlResponse SAMLResponse payload string. - * @param [redirectURL] Optional URL to redirect user to (isn't present for IdP initiated login). * @param [state] Optional state object associated with the provider. */ private async loginWithSAMLResponse( request: KibanaRequest, - { samlResponse, redirectURL }: { samlResponse: string; redirectURL?: string }, + samlResponse: string, state?: ProviderState | null ) { this.logger.debug('Trying to log in with SAML response payload.'); // If we have a `SAMLResponse` and state, but state doesn't contain all the necessary information, // then something unexpected happened and we should fail. - const { requestId: stateRequestId } = state || { requestId: '' }; - if (state && !stateRequestId) { + const { requestId: stateRequestId, redirectURL: stateRedirectURL } = state || { + requestId: '', + redirectURL: '', + }; + if (state && (!stateRequestId || !stateRedirectURL)) { const message = 'SAML response state does not have corresponding request id.'; this.logger.debug(message); return AuthenticationResult.failed(Boom.badRequest(message)); @@ -297,7 +317,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { this.logger.debug('Login has been performed with SAML response.'); return AuthenticationResult.redirectTo( - redirectURL || `${this.options.basePath.get(request)}/`, + stateRedirectURL || `${this.options.basePath.get(request)}/`, { state: { accessToken, refreshToken } } ); } catch (err) { @@ -316,23 +336,19 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { * we'll forward user to a page with the respective warning. * @param request Request instance. * @param samlResponse SAMLResponse payload string. - * @param [redirectURL] Optional URL to redirect user to (isn't present for IdP initiated login). * @param existingState State existing user session is based on. * @param user User returned for the existing session. */ private async loginWithNewSAMLResponse( request: KibanaRequest, - { samlResponse, redirectURL }: { samlResponse: string; redirectURL?: string }, + samlResponse: string, existingState: ProviderState, user: AuthenticatedUser ) { this.logger.debug('Trying to log in with SAML response payload and existing valid session.'); // First let's try to authenticate via SAML Response payload. - const payloadAuthenticationResult = await this.loginWithSAMLResponse(request, { - samlResponse, - redirectURL, - }); + const payloadAuthenticationResult = await this.loginWithSAMLResponse(request, samlResponse); if (payloadAuthenticationResult.failed()) { return payloadAuthenticationResult; } @@ -489,22 +505,17 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { try { // This operation should be performed on behalf of the user with a privilege that normal // user usually doesn't have `cluster:admin/xpack/security/saml/prepare`. - const { - id: requestId, - redirect: redirectWithoutRelayState, - } = await this.options.client.callAsInternalUser('shield.samlPrepare', { - body: { realm: this.realm }, - }); - - // HACK: we manually add a `RelayState` query string parameter with URL to redirect user to after - // successful SAML handshake since Elasticsearch doesn't support it yet. We may break something - // here eventually if we keep this workaround for too long. - const redirect = `${redirectWithoutRelayState}&RelayState=${encodeURIComponent(redirectURL)}`; + const { id: requestId, redirect } = await this.options.client.callAsInternalUser( + 'shield.samlPrepare', + { + body: { realm: this.realm }, + } + ); this.logger.debug('Redirecting to Identity Provider with SAML request.'); // Store request id in the state so that we can reuse it once we receive `SAMLResponse`. - return AuthenticationResult.redirectTo(redirect, { state: { requestId } }); + return AuthenticationResult.redirectTo(redirect, { state: { requestId, redirectURL } }); } catch (err) { this.logger.debug(`Failed to initiate SAML handshake: ${err.message}`); return AuthenticationResult.failed(err); @@ -561,7 +572,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { private captureRedirectURL(request: KibanaRequest) { const basePath = this.options.basePath.get(request); return AuthenticationResult.redirectTo(`${basePath}/api/security/saml/capture-url-fragment`, { - state: { redirectURLPath: `${basePath}${request.url.path}` }, + state: { redirectURL: `${basePath}${request.url.path}` }, }); } } diff --git a/x-pack/plugins/security/server/config.ts b/x-pack/plugins/security/server/config.ts index 8df8641dddbed8..8f1168ca19f7fc 100644 --- a/x-pack/plugins/security/server/config.ts +++ b/x-pack/plugins/security/server/config.ts @@ -38,7 +38,13 @@ export const ConfigSchema = schema.object( authc: schema.object({ providers: schema.arrayOf(schema.string(), { defaultValue: ['basic'], minSize: 1 }), oidc: providerOptionsSchema('oidc', schema.maybe(schema.object({ realm: schema.string() }))), - saml: providerOptionsSchema('saml', schema.maybe(schema.object({ realm: schema.string() }))), + saml: providerOptionsSchema( + 'saml', + schema.object({ + realm: schema.string(), + maxURLToStoreSize: schema.byteSize({ defaultValue: '2kb', min: '100b' }), + }) + ), }), }, // This option should be removed as soon as we entirely migrate config from legacy Security plugin. diff --git a/x-pack/plugins/security/server/routes/authentication.ts b/x-pack/plugins/security/server/routes/authentication.ts index 6204df8bb0a573..c78b4d4100e34a 100644 --- a/x-pack/plugins/security/server/routes/authentication.ts +++ b/x-pack/plugins/security/server/routes/authentication.ts @@ -139,7 +139,6 @@ function defineSAMLRoutes({ value: { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: request.body.SAMLResponse, - redirectURL: request.body.RelayState, }, }); From 6febc2ccbebb8f901e4118ed1d52e7b0df7fa834 Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Fri, 13 Sep 2019 12:44:51 +0200 Subject: [PATCH 06/11] Review#1: use server base path instead of the request scoped one for API routes. --- x-pack/legacy/plugins/security/index.js | 1 + .../server/authentication/providers/saml.ts | 3 ++ x-pack/plugins/security/server/plugin.ts | 1 + .../security/server/routes/authentication.ts | 20 ++++------ .../apis/security/saml_login.ts | 37 +++++-------------- 5 files changed, 21 insertions(+), 41 deletions(-) diff --git a/x-pack/legacy/plugins/security/index.js b/x-pack/legacy/plugins/security/index.js index 980af19cc8362b..b396ef6cabe2f7 100644 --- a/x-pack/legacy/plugins/security/index.js +++ b/x-pack/legacy/plugins/security/index.js @@ -138,6 +138,7 @@ export const security = (kibana) => new kibana.Plugin({ server.plugins.kibana.systemApi ), cspRules: createCSPRuleString(config.get('csp.rules')), + serverBasePath: config.get('server.basePath'), }); const plugin = this; diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts index 6b1f31f5c51496..26db1f7fae9648 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.ts @@ -570,6 +570,9 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { * @param request Request instance. */ private captureRedirectURL(request: KibanaRequest) { + // We should use `serverBasePath` for `capture-url-fragment` URL as soon as it's provided by the core. + // Using request scoped base path doesn't hurt, but isn't consistent. We should still use request scoped + // base path for the `redirectURL` though. const basePath = this.options.basePath.get(request); return AuthenticationResult.redirectTo(`${basePath}/api/security/saml/capture-url-fragment`, { state: { redirectURL: `${basePath}${request.url.path}` }, diff --git a/x-pack/plugins/security/server/plugin.ts b/x-pack/plugins/security/server/plugin.ts index 8d43cc909f5f62..695001c09d5f76 100644 --- a/x-pack/plugins/security/server/plugin.ts +++ b/x-pack/plugins/security/server/plugin.ts @@ -27,6 +27,7 @@ export interface LegacyAPI { xpackInfo: Pick; isSystemAPIRequest: (request: KibanaRequest) => boolean; cspRules: string; + serverBasePath?: string; } /** diff --git a/x-pack/plugins/security/server/routes/authentication.ts b/x-pack/plugins/security/server/routes/authentication.ts index c78b4d4100e34a..049cab811edff6 100644 --- a/x-pack/plugins/security/server/routes/authentication.ts +++ b/x-pack/plugins/security/server/routes/authentication.ts @@ -17,13 +17,7 @@ export function defineAuthenticationRoutes(params: RouteDefinitionParams) { /** * Defines routes required for SAML authentication. */ -function defineSAMLRoutes({ - basePath, - router, - logger, - authc, - getLegacyAPI, -}: RouteDefinitionParams) { +function defineSAMLRoutes({ router, logger, authc, getLegacyAPI }: RouteDefinitionParams) { function createCustomResourceResponse(body: string, contentType: string) { return { body, @@ -43,7 +37,7 @@ function defineSAMLRoutes({ options: { authRequired: false }, }, (context, request, response) => { - const currentBasePath = basePath.get(request); + const serverBasePath = getLegacyAPI().serverBasePath || ''; // We're also preventing `favicon.ico` request since it can cause new SAML handshake. return response.custom( createCustomResourceResponse( @@ -51,7 +45,7 @@ function defineSAMLRoutes({ Kibana SAML Login - + `, 'text/html' ) @@ -66,12 +60,12 @@ function defineSAMLRoutes({ options: { authRequired: false }, }, (context, request, response) => { - const currentBasePath = basePath.get(request); + const serverBasePath = getLegacyAPI().serverBasePath || ''; return response.custom( createCustomResourceResponse( ` window.location.replace( - '${currentBasePath}/api/security/saml/start?redirectURLFragment=' + encodeURIComponent(window.location.hash) + '${serverBasePath}/api/security/saml/start?redirectURLFragment=' + encodeURIComponent(window.location.hash) ); `, 'text/javascript' @@ -126,9 +120,9 @@ function defineSAMLRoutes({ async (context, request, response) => { try { if (path === '/api/security/v1/saml') { - const currentBasePath = basePath.get(request); + const serverBasePath = getLegacyAPI().serverBasePath || ''; logger.warn( - `The "${currentBasePath}${path}" URL is deprecated and will stop working in the next major version, please use "${currentBasePath}/api/security/saml/callback" URL instead.`, + `The "${serverBasePath}${path}" URL is deprecated and will stop working in the next major version, please use "${serverBasePath}/api/security/saml/callback" URL instead.`, { tags: ['deprecation'] } ); } diff --git a/x-pack/test/saml_api_integration/apis/security/saml_login.ts b/x-pack/test/saml_api_integration/apis/security/saml_login.ts index 4008a90fadeac6..abe4da5e2a7762 100644 --- a/x-pack/test/saml_api_integration/apis/security/saml_login.ts +++ b/x-pack/test/saml_api_integration/apis/security/saml_login.ts @@ -127,9 +127,7 @@ export default function({ getService }: FtrProviderContext) { }); describe('initiating handshake', () => { - const encodedRedirectURLFragment = '%23%2Fworkpad'; - const encodedRedirectURLPath = '%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three'; - const initiateHandshakeURL = `/api/security/saml/start?redirectURLFragment=${encodedRedirectURLFragment}`; + const initiateHandshakeURL = `/api/security/saml/start?redirectURLFragment=%23%2Fworkpad`; let captureURLCookie: Cookie; beforeEach(async () => { @@ -157,11 +155,6 @@ export default function({ getService }: FtrProviderContext) { true /* parseQueryString */ ); expect(redirectURL.href!.startsWith(`https://elastic.co/sso/saml`)).to.be(true); - expect( - redirectURL.href!.endsWith( - `&RelayState=${encodedRedirectURLPath}${encodedRedirectURLFragment}` - ) - ).to.be(true); expect(redirectURL.query.SAMLRequest).to.not.be.empty(); }); @@ -191,8 +184,6 @@ export default function({ getService }: FtrProviderContext) { }); describe('finishing handshake', () => { - const encodedRedirectURLFragment = '%23%2Fworkpad'; - const encodedRedirectURL = `%2Fabc%2Fxyz%2Fhandshake%3Fone%3Dtwo%2520three${encodedRedirectURLFragment}`; let handshakeCookie: Cookie; let samlRequestId: string; @@ -203,7 +194,7 @@ export default function({ getService }: FtrProviderContext) { const captureURLCookie = request.cookie(captureURLResponse.headers['set-cookie'][0])!; const handshakeResponse = await supertest - .get(`/api/security/saml/start?redirectURLFragment=${encodedRedirectURLFragment}`) + .get(`/api/security/saml/start?redirectURLFragment=%23%2Fworkpad`) .set('Cookie', captureURLCookie.cookieString()) .expect(302); @@ -224,14 +215,13 @@ export default function({ getService }: FtrProviderContext) { .post('/api/security/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) - .send({ - SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }), - RelayState: encodedRedirectURL, - }) + .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }) }) .expect(302); // User should be redirected to the URL that initiated handshake. - expect(samlAuthenticationResponse.headers.location).to.be(encodedRedirectURL); + expect(samlAuthenticationResponse.headers.location).to.be( + '/abc/xyz/handshake?one=two%20three#/workpad' + ); const cookies = samlAuthenticationResponse.headers['set-cookie']; expect(cookies).to.have.length(1); @@ -263,8 +253,7 @@ export default function({ getService }: FtrProviderContext) { }); it('should succeed in case of IdP initiated login', async () => { - // Don't pass handshake cookie, RelayState query string parameter and don't include - // `inResponseTo` into SAML response to simulate IdP initiated login. + // Don't pass handshake cookie and don't include `inResponseTo` into SAML response to simulate IdP initiated login. const samlAuthenticationResponse = await supertest .post('/api/security/saml/callback') .set('kbn-xsrf', 'xxx') @@ -406,7 +395,6 @@ export default function({ getService }: FtrProviderContext) { inResponseTo: samlRequestId, sessionIndex: idpSessionIndex, }), - RelayState: encodeURIComponent('/abc/xyz#workpad'), }) .expect(302); @@ -556,10 +544,7 @@ export default function({ getService }: FtrProviderContext) { .post('/api/security/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) - .send({ - SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }), - RelayState: encodeURIComponent('/abc/xyz#workpad'), - }) + .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }) }) .expect(302); sessionCookie = request.cookie(samlAuthenticationResponse.headers['set-cookie'][0])!; @@ -647,10 +632,7 @@ export default function({ getService }: FtrProviderContext) { .post('/api/security/saml/callback') .set('kbn-xsrf', 'xxx') .set('Cookie', handshakeCookie.cookieString()) - .send({ - SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }), - RelayState: encodeURIComponent('/abc/xyz#workpad'), - }) + .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }) }) .expect(302); sessionCookie = request.cookie(samlAuthenticationResponse.headers['set-cookie'][0])!; @@ -714,7 +696,6 @@ export default function({ getService }: FtrProviderContext) { inResponseTo: samlRequestId, username: existingUsername, }), - RelayState: encodeURIComponent('/abc/xyz#workpad'), }) .expect(302); From ff90be4bb5ef8a362f5bbfb24a6524d8479e9e0a Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Fri, 13 Sep 2019 16:42:45 +0200 Subject: [PATCH 07/11] Properly handle max redirect URL size. --- .../security/authentication/index.asciidoc | 2 +- .../resources/bin/kibana-docker | 2 +- .../server/authentication/providers/saml.ts | 40 ++++++++++++------- x-pack/plugins/security/server/config.ts | 2 +- 4 files changed, 29 insertions(+), 17 deletions(-) diff --git a/docs/user/security/authentication/index.asciidoc b/docs/user/security/authentication/index.asciidoc index b0475b81766470..99763955ced85c 100644 --- a/docs/user/security/authentication/index.asciidoc +++ b/docs/user/security/authentication/index.asciidoc @@ -129,7 +129,7 @@ size of the URL {kib} is allowed to store during SAML handshake, the default val [source,yaml] -------------------------------------------------------------------------------- -xpack.security.authc.saml.maxURLToStoreSize: 1kb +xpack.security.authc.saml.maxRedirectURLSize: 1kb -------------------------------------------------------------------------------- [[oidc]] diff --git a/src/dev/build/tasks/os_packages/docker_generator/resources/bin/kibana-docker b/src/dev/build/tasks/os_packages/docker_generator/resources/bin/kibana-docker index 57f9bf22ab9d24..dc3ae08f3317c0 100755 --- a/src/dev/build/tasks/os_packages/docker_generator/resources/bin/kibana-docker +++ b/src/dev/build/tasks/os_packages/docker_generator/resources/bin/kibana-docker @@ -147,7 +147,7 @@ kibana_vars=( xpack.security.authc.providers xpack.security.authc.oidc.realm xpack.security.authc.saml.realm - xpack.security.authc.saml.maxURLToStoreSize + xpack.security.authc.saml.maxRedirectURLSize xpack.security.cookieName xpack.security.enabled xpack.security.encryptionKey diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts index 26db1f7fae9648..3828e3722845de 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.ts @@ -70,11 +70,11 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { /** * Maximum size of the URL we store in the session during SAML handshake. */ - private readonly maxURLToStoreSize: ByteSizeValue; + private readonly maxRedirectURLSize: ByteSizeValue; constructor( protected readonly options: Readonly, - samlOptions?: Readonly<{ realm?: string; maxURLToStoreSize?: ByteSizeValue }> + samlOptions?: Readonly<{ realm?: string; maxRedirectURLSize?: ByteSizeValue }> ) { super(options); @@ -82,12 +82,12 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { throw new Error('Realm name must be specified'); } - if (!samlOptions.maxURLToStoreSize) { - throw new Error(`maxURLToStoreSize must be specified`); + if (!samlOptions.maxRedirectURLSize) { + throw new Error(`maxRedirectURLSize must be specified`); } this.realm = samlOptions.realm; - this.maxURLToStoreSize = samlOptions.maxURLToStoreSize; + this.maxRedirectURLSize = samlOptions.maxRedirectURLSize; } /** @@ -105,16 +105,16 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { if (attempt.step === SAMLLoginStep.RedirectURLFragmentCaptured) { if (!state || !state.redirectURL) { - const message = 'State does not include path to redirect to.'; + const message = 'State does not include URL path to redirect to.'; this.logger.debug(message); return AuthenticationResult.failed(Boom.badRequest(message)); } - let redirectURL = state.redirectURL + attempt.redirectURLFragment; + let redirectURL = `${state.redirectURL}${attempt.redirectURLFragment}`; const redirectURLSize = new ByteSizeValue(Buffer.byteLength(redirectURL)); - if (this.maxURLToStoreSize.isLessThan(redirectURLSize)) { + if (this.maxRedirectURLSize.isLessThan(redirectURLSize)) { this.logger.warn( - `Max URL size should not exceed ${this.maxURLToStoreSize.toString()} but it was ${redirectURLSize.toString()}. Only URL path is captured.` + `Max URL size should not exceed ${this.maxRedirectURLSize.toString()} but it was ${redirectURLSize.toString()}. Only URL path is captured.` ); redirectURL = state.redirectURL; } else { @@ -289,7 +289,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { requestId: '', redirectURL: '', }; - if (state && (!stateRequestId || !stateRedirectURL)) { + if (state && !stateRequestId) { const message = 'SAML response state does not have corresponding request id.'; this.logger.debug(message); return AuthenticationResult.failed(Boom.badRequest(message)); @@ -570,12 +570,24 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { * @param request Request instance. */ private captureRedirectURL(request: KibanaRequest) { - // We should use `serverBasePath` for `capture-url-fragment` URL as soon as it's provided by the core. - // Using request scoped base path doesn't hurt, but isn't consistent. We should still use request scoped - // base path for the `redirectURL` though. const basePath = this.options.basePath.get(request); + const redirectURL = `${basePath}${request.url.path}`; + + // If the size of the path already exceeds the maximum allowed size of the URL to store in the + // session there is no reason to try to capture URL fragment and we start handshake immediately. + // In this case user will be redirected to the Kibana home/root after successful login. + const redirectURLSize = new ByteSizeValue(Buffer.byteLength(redirectURL)); + if (this.maxRedirectURLSize.isLessThan(redirectURLSize)) { + this.logger.warn( + `Max URL path size should not exceed ${this.maxRedirectURLSize.toString()} but it was ${redirectURLSize.toString()}. URL is not captured.` + ); + return this.authenticateViaHandshake(request, ''); + } + + // We should use `serverBasePath` here as soon as it's provided by the core. Using request scoped + // base path doesn't hurt, but isn't consistent with the rest of API routes. return AuthenticationResult.redirectTo(`${basePath}/api/security/saml/capture-url-fragment`, { - state: { redirectURL: `${basePath}${request.url.path}` }, + state: { redirectURL }, }); } } diff --git a/x-pack/plugins/security/server/config.ts b/x-pack/plugins/security/server/config.ts index 8f1168ca19f7fc..6fe3fc73e458c3 100644 --- a/x-pack/plugins/security/server/config.ts +++ b/x-pack/plugins/security/server/config.ts @@ -42,7 +42,7 @@ export const ConfigSchema = schema.object( 'saml', schema.object({ realm: schema.string(), - maxURLToStoreSize: schema.byteSize({ defaultValue: '2kb', min: '100b' }), + maxRedirectURLSize: schema.byteSize({ defaultValue: '2kb' }), }) ), }), From 50fada9e0120301b0435642a56dea6ea800eb30c Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Tue, 17 Sep 2019 11:47:31 +0200 Subject: [PATCH 08/11] Update tests. --- .../routes/api/v1/__tests__/authenticate.js | 86 ----- .../server/authentication/index.mock.ts | 18 + .../authentication/providers/saml.test.ts | 340 +++++++++++++++--- .../server/authentication/providers/saml.ts | 6 +- x-pack/plugins/security/server/config.test.ts | 232 +++++++----- x-pack/plugins/security/server/plugin.test.ts | 6 +- .../server/routes/authentication.test.ts | 219 +++++++++++ .../apis/security/saml_login.ts | 154 +++++--- x-pack/test/saml_api_integration/config.ts | 1 + 9 files changed, 792 insertions(+), 270 deletions(-) create mode 100644 x-pack/plugins/security/server/authentication/index.mock.ts create mode 100644 x-pack/plugins/security/server/routes/authentication.test.ts diff --git a/x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/authenticate.js b/x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/authenticate.js index 96b47d1407bf19..5e07ec7ee06182 100644 --- a/x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/authenticate.js +++ b/x-pack/legacy/plugins/security/server/routes/api/v1/__tests__/authenticate.js @@ -257,90 +257,4 @@ describe('Authentication routes', () => { expect(response).to.eql({ username: 'user' }); }); }); - - describe('SAML assertion consumer service endpoint', () => { - let samlAcsRoute; - let request; - - beforeEach(() => { - samlAcsRoute = serverStub.route - .withArgs(sinon.match({ path: '/api/security/v1/saml' })) - .firstCall - .args[0]; - - request = requestFixture({ payload: { SAMLResponse: 'saml-response-xml' } }); - }); - - it('correctly defines route.', async () => { - expect(samlAcsRoute.method).to.be('POST'); - expect(samlAcsRoute.path).to.be('/api/security/v1/saml'); - expect(samlAcsRoute.handler).to.be.a(Function); - expect(samlAcsRoute.config).to.eql({ - auth: false, - validate: { - payload: Joi.object({ - SAMLResponse: Joi.string().required(), - RelayState: Joi.string().allow('') - }) - } - }); - }); - - it('returns 500 if authentication throws unhandled exception.', async () => { - const unhandledException = new Error('Something went wrong.'); - loginStub.throws(unhandledException); - - const response = await samlAcsRoute.handler(request, hStub); - - sinon.assert.notCalled(hStub.redirect); - expect(response.isBoom).to.be(true); - expect(response.output.payload).to.eql({ - statusCode: 500, - error: 'Internal Server Error', - message: 'An internal server error occurred' - }); - }); - - it('returns 401 if authentication fails.', async () => { - const failureReason = new Error('Something went wrong.'); - loginStub.resolves(AuthenticationResult.failed(failureReason)); - - const response = await samlAcsRoute.handler(request, hStub); - - sinon.assert.notCalled(hStub.redirect); - expect(response.isBoom).to.be(true); - expect(response.message).to.be(failureReason.message); - expect(response.output.statusCode).to.be(401); - }); - - it('returns 401 if authentication is not handled.', async () => { - loginStub.resolves(AuthenticationResult.notHandled()); - - const response = await samlAcsRoute.handler(request, hStub); - - sinon.assert.notCalled(hStub.redirect); - expect(response.isBoom).to.be(true); - expect(response.message).to.be('Unauthorized'); - expect(response.output.statusCode).to.be(401); - }); - - it('returns 401 if authentication completes with unexpected result.', async () => { - loginStub.resolves(AuthenticationResult.succeeded({})); - - const response = await samlAcsRoute.handler(request, hStub); - - sinon.assert.notCalled(hStub.redirect); - expect(response.isBoom).to.be(true); - expect(response.message).to.be('Unauthorized'); - expect(response.output.statusCode).to.be(401); - }); - - it('redirects if required by the authentication process.', async () => { - loginStub.resolves(AuthenticationResult.redirectTo('http://redirect-to/path')); - - await samlAcsRoute.handler(request, hStub); - - sinon.assert.calledWithExactly(hStub.redirect, 'http://redirect-to/path'); - }); - }); }); diff --git a/x-pack/plugins/security/server/authentication/index.mock.ts b/x-pack/plugins/security/server/authentication/index.mock.ts new file mode 100644 index 00000000000000..dcaf26f53fe010 --- /dev/null +++ b/x-pack/plugins/security/server/authentication/index.mock.ts @@ -0,0 +1,18 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { Authentication } from '.'; + +export const authenticationMock = { + create: (): jest.Mocked => ({ + login: jest.fn(), + createAPIKey: jest.fn(), + getCurrentUser: jest.fn(), + invalidateAPIKey: jest.fn(), + isAuthenticated: jest.fn(), + logout: jest.fn(), + }), +}; diff --git a/x-pack/plugins/security/server/authentication/providers/saml.test.ts b/x-pack/plugins/security/server/authentication/providers/saml.test.ts index b510b3a7a5e282..732472230cfe9f 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.test.ts @@ -6,6 +6,7 @@ import Boom from 'boom'; import sinon from 'sinon'; +import { ByteSizeValue } from '@kbn/config-schema'; import { httpServerMock } from '../../../../../../src/core/server/mocks'; import { mockAuthenticatedUser } from '../../../common/model/authenticated_user.mock'; @@ -22,7 +23,10 @@ describe('SAMLAuthenticationProvider', () => { let mockOptions: MockAuthenticationProviderOptions; beforeEach(() => { mockOptions = mockAuthenticationProviderOptions(); - provider = new SAMLAuthenticationProvider(mockOptions, { realm: 'test-realm' }); + provider = new SAMLAuthenticationProvider(mockOptions, { + realm: 'test-realm', + maxRedirectURLSize: new ByteSizeValue(100), + }); }); it('throws if `realm` option is not specified', () => { @@ -39,6 +43,22 @@ describe('SAMLAuthenticationProvider', () => { ); }); + it('throws if `maxRedirectURLSize` option is not specified', () => { + const providerOptions = mockAuthenticationProviderOptions(); + + expect( + () => new SAMLAuthenticationProvider(providerOptions, { realm: 'test-realm' }) + ).toThrowError('Maximum redirect URL size must be specified'); + + expect( + () => + new SAMLAuthenticationProvider(providerOptions, { + realm: 'test-realm', + maxRedirectURLSize: undefined, + }) + ).toThrowError('Maximum redirect URL size must be specified'); + }); + describe('`login` method', () => { it('gets token and redirects user to requested URL if SAML Response is valid.', async () => { const request = httpServerMock.createKibanaRequest(); @@ -51,12 +71,8 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.login( request, - { - step: SAMLLoginStep.SAMLResponseReceived, - samlResponse: 'saml-response-xml', - redirectURL: '/test-base-path/some-path', - }, - { requestId: 'some-request-id' } + { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' }, + { requestId: 'some-request-id', redirectURL: '/test-base-path/some-path#some-app' } ); sinon.assert.calledWithExactly( @@ -66,7 +82,7 @@ describe('SAMLAuthenticationProvider', () => { ); expect(authenticationResult.redirected()).toBe(true); - expect(authenticationResult.redirectURL).toBe('/test-base-path/some-path'); + expect(authenticationResult.redirectURL).toBe('/test-base-path/some-path#some-app'); expect(authenticationResult.state).toEqual({ username: 'user', accessToken: 'some-token', @@ -79,11 +95,7 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.login( request, - { - step: SAMLLoginStep.SAMLResponseReceived, - samlResponse: 'saml-response-xml', - redirectURL: '/test-base-path/some-path', - }, + { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' }, {} ); @@ -91,29 +103,36 @@ describe('SAMLAuthenticationProvider', () => { expect(authenticationResult.failed()).toBe(true); expect(authenticationResult.error).toEqual( - Boom.badRequest( - 'SAML response state does not have corresponding request id or redirect URL.' - ) + Boom.badRequest('SAML response state does not have corresponding request id.') ); }); - it('fails if SAML Response payload is presented but state does not contain redirect URL.', async () => { + it('redirects to the default location if state contains empty redirect URL.', async () => { const request = httpServerMock.createKibanaRequest(); + mockOptions.client.callAsInternalUser.withArgs('shield.samlAuthenticate').resolves({ + access_token: 'user-initiated-login-token', + refresh_token: 'user-initiated-login-refresh-token', + }); + const authenticationResult = await provider.login( request, { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' }, - { requestId: 'some-request-id' } + { requestId: 'some-request-id', redirectURL: '' } ); - sinon.assert.notCalled(mockOptions.client.callAsInternalUser); - - expect(authenticationResult.failed()).toBe(true); - expect(authenticationResult.error).toEqual( - Boom.badRequest( - 'SAML response state does not have corresponding request id or redirect URL.' - ) + sinon.assert.calledWithExactly( + mockOptions.client.callAsInternalUser, + 'shield.samlAuthenticate', + { body: { ids: ['some-request-id'], content: 'saml-response-xml', realm: 'test-realm' } } ); + + expect(authenticationResult.redirected()).toBe(true); + expect(authenticationResult.redirectURL).toBe('/base-path/'); + expect(authenticationResult.state).toEqual({ + accessToken: 'user-initiated-login-token', + refreshToken: 'user-initiated-login-refresh-token', + }); }); it('redirects to the default location if state is not presented.', async () => { @@ -153,12 +172,8 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.login( request, - { - step: SAMLLoginStep.SAMLResponseReceived, - samlResponse: 'saml-response-xml', - redirectURL: '/test-base-path/some-path', - }, - { requestId: 'some-request-id' } + { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' }, + { requestId: 'some-request-id', redirectURL: '/test-base-path/some-path' } ); sinon.assert.calledWithExactly( @@ -187,7 +202,7 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.login( request, - { samlResponse: 'saml-response-xml' }, + { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' }, { username: 'user', accessToken: 'some-valid-token', @@ -272,7 +287,7 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.login( request, - { samlResponse: 'saml-response-xml' }, + { step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' }, state ); @@ -338,6 +353,139 @@ describe('SAMLAuthenticationProvider', () => { expect(authenticationResult.redirectURL).toBe('/base-path/overwritten_session'); }); }); + + describe('User initiated login with captured redirect URL', () => { + it('fails if state is not available', async () => { + const request = httpServerMock.createKibanaRequest(); + + const authenticationResult = await provider.login(request, { + step: SAMLLoginStep.RedirectURLFragmentCaptured, + redirectURLFragment: '#some-fragment', + }); + + sinon.assert.notCalled(mockOptions.client.callAsInternalUser); + + expect(authenticationResult.failed()).toBe(true); + expect(authenticationResult.error).toEqual( + Boom.badRequest('State does not include URL path to redirect to.') + ); + }); + + it('does not handle AJAX requests.', async () => { + const request = httpServerMock.createKibanaRequest({ headers: { 'kbn-xsrf': 'xsrf' } }); + + const authenticationResult = await provider.login( + request, + { + step: SAMLLoginStep.RedirectURLFragmentCaptured, + redirectURLFragment: '#some-fragment', + }, + { redirectURL: '/test-base-path/some-path' } + ); + + sinon.assert.notCalled(mockOptions.client.callAsInternalUser); + + expect(authenticationResult.notHandled()).toBe(true); + }); + + it('redirects non-AJAX requests to the IdP remembering combined redirect URL.', async () => { + const request = httpServerMock.createKibanaRequest(); + + mockOptions.client.callAsInternalUser.withArgs('shield.samlPrepare').resolves({ + id: 'some-request-id', + redirect: 'https://idp-host/path/login?SAMLRequest=some%20request%20', + }); + + const authenticationResult = await provider.login( + request, + { + step: SAMLLoginStep.RedirectURLFragmentCaptured, + redirectURLFragment: '#some-fragment', + }, + { redirectURL: '/test-base-path/some-path' } + ); + + sinon.assert.calledWithExactly( + mockOptions.client.callAsInternalUser, + 'shield.samlPrepare', + { body: { realm: 'test-realm' } } + ); + + expect(mockOptions.logger.warn).not.toHaveBeenCalled(); + + expect(authenticationResult.redirected()).toBe(true); + expect(authenticationResult.redirectURL).toBe( + 'https://idp-host/path/login?SAMLRequest=some%20request%20' + ); + expect(authenticationResult.state).toEqual({ + requestId: 'some-request-id', + redirectURL: '/test-base-path/some-path#some-fragment', + }); + }); + + it('redirects non-AJAX requests to the IdP remembering only redirect URL path if fragment is too large.', async () => { + const request = httpServerMock.createKibanaRequest(); + + mockOptions.client.callAsInternalUser.withArgs('shield.samlPrepare').resolves({ + id: 'some-request-id', + redirect: 'https://idp-host/path/login?SAMLRequest=some%20request%20', + }); + + const authenticationResult = await provider.login( + request, + { + step: SAMLLoginStep.RedirectURLFragmentCaptured, + redirectURLFragment: '#some-fragment'.repeat(10), + }, + { redirectURL: '/test-base-path/some-path' } + ); + + sinon.assert.calledWithExactly( + mockOptions.client.callAsInternalUser, + 'shield.samlPrepare', + { body: { realm: 'test-realm' } } + ); + + expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1); + expect(mockOptions.logger.warn).toHaveBeenCalledWith( + 'Max URL size should not exceed 100b but it was 165b. Only URL path is captured.' + ); + + expect(authenticationResult.redirected()).toBe(true); + expect(authenticationResult.redirectURL).toBe( + 'https://idp-host/path/login?SAMLRequest=some%20request%20' + ); + expect(authenticationResult.state).toEqual({ + requestId: 'some-request-id', + redirectURL: '/test-base-path/some-path', + }); + }); + + it('fails if SAML request preparation fails.', async () => { + const request = httpServerMock.createKibanaRequest(); + + const failureReason = new Error('Realm is misconfigured!'); + mockOptions.client.callAsInternalUser.withArgs('shield.samlPrepare').rejects(failureReason); + + const authenticationResult = await provider.login( + request, + { + step: SAMLLoginStep.RedirectURLFragmentCaptured, + redirectURLFragment: '#some-fragment', + }, + { redirectURL: '/test-base-path/some-path' } + ); + + sinon.assert.calledWithExactly( + mockOptions.client.callAsInternalUser, + 'shield.samlPrepare', + { body: { realm: 'test-realm' } } + ); + + expect(authenticationResult.failed()).toBe(true); + expect(authenticationResult.error).toBe(failureReason); + }); + }); }); describe('`authenticate` method', () => { @@ -365,7 +513,7 @@ describe('SAMLAuthenticationProvider', () => { expect(authenticationResult.notHandled()).toBe(true); }); - it('redirects non-AJAX request that can not be authenticated to the IdP.', async () => { + it('redirects non-AJAX request that can not be authenticated to the "capture fragment" page.', async () => { const request = httpServerMock.createKibanaRequest({ path: '/s/foo/some-path' }); mockOptions.client.callAsInternalUser.withArgs('shield.samlPrepare').resolves({ @@ -373,24 +521,49 @@ describe('SAMLAuthenticationProvider', () => { redirect: 'https://idp-host/path/login?SAMLRequest=some%20request%20', }); - const authenticationResult = await provider.authenticate(request, null); + const authenticationResult = await provider.authenticate(request); + + sinon.assert.notCalled(mockOptions.client.callAsInternalUser); + + expect(authenticationResult.redirected()).toBe(true); + expect(authenticationResult.redirectURL).toBe( + '/base-path/api/security/saml/capture-url-fragment' + ); + expect(authenticationResult.state).toEqual({ redirectURL: '/base-path/s/foo/some-path' }); + }); + + it('redirects non-AJAX request that can not be authenticated to the IdP if request path is too large.', async () => { + const request = httpServerMock.createKibanaRequest({ + path: `/s/foo/${'some-path'.repeat(10)}`, + }); + + mockOptions.client.callAsInternalUser.withArgs('shield.samlPrepare').resolves({ + id: 'some-request-id', + redirect: 'https://idp-host/path/login?SAMLRequest=some%20request%20', + }); + + const authenticationResult = await provider.authenticate(request); sinon.assert.calledWithExactly(mockOptions.client.callAsInternalUser, 'shield.samlPrepare', { body: { realm: 'test-realm' }, }); + expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1); + expect(mockOptions.logger.warn).toHaveBeenCalledWith( + 'Max URL path size should not exceed 100b but it was 107b. URL is not captured.' + ); + expect(authenticationResult.redirected()).toBe(true); expect(authenticationResult.redirectURL).toBe( 'https://idp-host/path/login?SAMLRequest=some%20request%20' ); - expect(authenticationResult.state).toEqual({ - requestId: 'some-request-id', - nextURL: `/base-path/s/foo/some-path`, - }); + expect(authenticationResult.state).toEqual({ requestId: 'some-request-id', redirectURL: '' }); }); it('fails if SAML request preparation fails.', async () => { - const request = httpServerMock.createKibanaRequest({ path: '/some-path' }); + const request = httpServerMock.createKibanaRequest({ + path: `/s/foo/${'some-path'.repeat(10)}`, + }); const failureReason = new Error('Realm is misconfigured!'); mockOptions.client.callAsInternalUser.withArgs('shield.samlPrepare').rejects(failureReason); @@ -547,7 +720,7 @@ describe('SAMLAuthenticationProvider', () => { ); }); - it('initiates SAML handshake for non-AJAX requests if access token document is missing.', async () => { + it('re-capture URL for non-AJAX requests if access token document is missing.', async () => { const request = httpServerMock.createKibanaRequest({ path: '/s/foo/some-path' }); const state = { username: 'user', @@ -555,6 +728,39 @@ describe('SAMLAuthenticationProvider', () => { refreshToken: 'expired-refresh-token', }; + mockScopedClusterClient( + mockOptions.client, + sinon.match({ headers: { authorization: `Bearer ${state.accessToken}` } }) + ) + .callAsCurrentUser.withArgs('shield.authenticate') + .rejects({ + statusCode: 500, + body: { error: { reason: 'token document is missing and must be present' } }, + }); + + mockOptions.tokens.refresh.withArgs(state.refreshToken).resolves(null); + + const authenticationResult = await provider.authenticate(request, state); + + sinon.assert.notCalled(mockOptions.client.callAsInternalUser); + + expect(authenticationResult.redirected()).toBe(true); + expect(authenticationResult.redirectURL).toBe( + '/base-path/api/security/saml/capture-url-fragment' + ); + expect(authenticationResult.state).toEqual({ redirectURL: '/base-path/s/foo/some-path' }); + }); + + it('initiates SAML handshake for non-AJAX requests if access token document is missing and request path is too large.', async () => { + const request = httpServerMock.createKibanaRequest({ + path: `/s/foo/${'some-path'.repeat(10)}`, + }); + const state = { + username: 'user', + accessToken: 'expired-token', + refreshToken: 'expired-refresh-token', + }; + mockOptions.client.callAsInternalUser.withArgs('shield.samlPrepare').resolves({ id: 'some-request-id', redirect: 'https://idp-host/path/login?SAMLRequest=some%20request%20', @@ -578,17 +784,19 @@ describe('SAMLAuthenticationProvider', () => { body: { realm: 'test-realm' }, }); + expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1); + expect(mockOptions.logger.warn).toHaveBeenCalledWith( + 'Max URL path size should not exceed 100b but it was 107b. URL is not captured.' + ); + expect(authenticationResult.redirected()).toBe(true); expect(authenticationResult.redirectURL).toBe( 'https://idp-host/path/login?SAMLRequest=some%20request%20' ); - expect(authenticationResult.state).toEqual({ - requestId: 'some-request-id', - nextURL: `/base-path/s/foo/some-path`, - }); + expect(authenticationResult.state).toEqual({ requestId: 'some-request-id', redirectURL: '' }); }); - it('initiates SAML handshake for non-AJAX requests if refresh token is expired.', async () => { + it('re-capture URL for non-AJAX requests if refresh token is expired.', async () => { const request = httpServerMock.createKibanaRequest({ path: '/s/foo/some-path' }); const state = { username: 'user', @@ -596,6 +804,36 @@ describe('SAMLAuthenticationProvider', () => { refreshToken: 'expired-refresh-token', }; + mockScopedClusterClient( + mockOptions.client, + sinon.match({ headers: { authorization: `Bearer ${state.accessToken}` } }) + ) + .callAsCurrentUser.withArgs('shield.authenticate') + .rejects({ statusCode: 401 }); + + mockOptions.tokens.refresh.withArgs(state.refreshToken).resolves(null); + + const authenticationResult = await provider.authenticate(request, state); + + sinon.assert.notCalled(mockOptions.client.callAsInternalUser); + + expect(authenticationResult.redirected()).toBe(true); + expect(authenticationResult.redirectURL).toBe( + '/base-path/api/security/saml/capture-url-fragment' + ); + expect(authenticationResult.state).toEqual({ redirectURL: '/base-path/s/foo/some-path' }); + }); + + it('initiates SAML handshake for non-AJAX requests if refresh token is expired and request path is too large.', async () => { + const request = httpServerMock.createKibanaRequest({ + path: `/s/foo/${'some-path'.repeat(10)}`, + }); + const state = { + username: 'user', + accessToken: 'expired-token', + refreshToken: 'expired-refresh-token', + }; + mockOptions.client.callAsInternalUser.withArgs('shield.samlPrepare').resolves({ id: 'some-request-id', redirect: 'https://idp-host/path/login?SAMLRequest=some%20request%20', @@ -616,14 +854,16 @@ describe('SAMLAuthenticationProvider', () => { body: { realm: 'test-realm' }, }); + expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1); + expect(mockOptions.logger.warn).toHaveBeenCalledWith( + 'Max URL path size should not exceed 100b but it was 107b. URL is not captured.' + ); + expect(authenticationResult.redirected()).toBe(true); expect(authenticationResult.redirectURL).toBe( 'https://idp-host/path/login?SAMLRequest=some%20request%20' ); - expect(authenticationResult.state).toEqual({ - requestId: 'some-request-id', - nextURL: `/base-path/s/foo/some-path`, - }); + expect(authenticationResult.state).toEqual({ requestId: 'some-request-id', redirectURL: '' }); }); it('succeeds if `authorization` contains a valid token.', async () => { diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts index 374703423af6f9..87e1a11591a35a 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.ts @@ -87,7 +87,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { } if (!samlOptions.maxRedirectURLSize) { - throw new Error(`maxRedirectURLSize must be specified`); + throw new Error('Maximum redirect URL size must be specified'); } this.realm = samlOptions.realm; @@ -496,9 +496,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { // user usually doesn't have `cluster:admin/xpack/security/saml/prepare`. const { id: requestId, redirect } = await this.options.client.callAsInternalUser( 'shield.samlPrepare', - { - body: { realm: this.realm }, - } + { body: { realm: this.realm } } ); this.logger.debug('Redirecting to Identity Provider with SAML request.'); diff --git a/x-pack/plugins/security/server/config.test.ts b/x-pack/plugins/security/server/config.test.ts index 991841b4fd399d..943d582bf484a7 100644 --- a/x-pack/plugins/security/server/config.test.ts +++ b/x-pack/plugins/security/server/config.test.ts @@ -13,45 +13,45 @@ import { createConfig$, ConfigSchema } from './config'; describe('config schema', () => { it('generates proper defaults', () => { expect(ConfigSchema.validate({})).toMatchInlineSnapshot(` -Object { - "authc": Object { - "providers": Array [ - "basic", - ], - }, - "cookieName": "sid", - "encryptionKey": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - "secureCookies": false, - "sessionTimeout": null, -} -`); + Object { + "authc": Object { + "providers": Array [ + "basic", + ], + }, + "cookieName": "sid", + "encryptionKey": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "secureCookies": false, + "sessionTimeout": null, + } + `); expect(ConfigSchema.validate({}, { dist: false })).toMatchInlineSnapshot(` -Object { - "authc": Object { - "providers": Array [ - "basic", - ], - }, - "cookieName": "sid", - "encryptionKey": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", - "secureCookies": false, - "sessionTimeout": null, -} -`); + Object { + "authc": Object { + "providers": Array [ + "basic", + ], + }, + "cookieName": "sid", + "encryptionKey": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + "secureCookies": false, + "sessionTimeout": null, + } + `); expect(ConfigSchema.validate({}, { dist: true })).toMatchInlineSnapshot(` -Object { - "authc": Object { - "providers": Array [ - "basic", - ], - }, - "cookieName": "sid", - "secureCookies": false, - "sessionTimeout": null, -} -`); + Object { + "authc": Object { + "providers": Array [ + "basic", + ], + }, + "cookieName": "sid", + "secureCookies": false, + "sessionTimeout": null, + } + `); }); it('should throw error if xpack.security.encryptionKey is less than 32 characters', () => { @@ -89,15 +89,15 @@ Object { authc: { providers: ['oidc'], oidc: { realm: 'realm-1' } }, }).authc ).toMatchInlineSnapshot(` -Object { - "oidc": Object { - "realm": "realm-1", - }, - "providers": Array [ - "oidc", - ], -} -`); + Object { + "oidc": Object { + "realm": "realm-1", + }, + "providers": Array [ + "oidc", + ], + } + `); }); it(`returns a validation error when authc.providers is "['oidc', 'basic']" and realm is unspecified`, async () => { @@ -114,16 +114,16 @@ Object { authc: { providers: ['oidc', 'basic'], oidc: { realm: 'realm-1' } }, }).authc ).toMatchInlineSnapshot(` -Object { - "oidc": Object { - "realm": "realm-1", - }, - "providers": Array [ - "oidc", - "basic", - ], -} -`); + Object { + "oidc": Object { + "realm": "realm-1", + }, + "providers": Array [ + "oidc", + "basic", + ], + } + `); }); it(`realm is not allowed when authc.providers is "['basic']"`, async () => { @@ -152,15 +152,18 @@ Object { authc: { providers: ['saml'], saml: { realm: 'realm-1' } }, }).authc ).toMatchInlineSnapshot(` -Object { - "providers": Array [ - "saml", - ], - "saml": Object { - "realm": "realm-1", - }, -} -`); + Object { + "providers": Array [ + "saml", + ], + "saml": Object { + "maxRedirectURLSize": ByteSizeValue { + "valueInBytes": 2048, + }, + "realm": "realm-1", + }, + } + `); }); it('`realm` is not allowed if saml provider is not enabled', async () => { @@ -168,6 +171,73 @@ Object { ConfigSchema.validate({ authc: { providers: ['basic'], saml: { realm: 'realm-1' } } }) ).toThrowErrorMatchingInlineSnapshot(`"[authc.saml]: a value wasn't expected to be present"`); }); + + it('`maxRedirectURLSize` accepts any positive value that can coerce to `ByteSizeValue`', async () => { + expect( + ConfigSchema.validate({ + authc: { providers: ['saml'], saml: { realm: 'realm-1' } }, + }).authc.saml + ).toMatchInlineSnapshot(` + Object { + "maxRedirectURLSize": ByteSizeValue { + "valueInBytes": 2048, + }, + "realm": "realm-1", + } + `); + + expect( + ConfigSchema.validate({ + authc: { providers: ['saml'], saml: { realm: 'realm-1', maxRedirectURLSize: 100 } }, + }).authc.saml + ).toMatchInlineSnapshot(` + Object { + "maxRedirectURLSize": ByteSizeValue { + "valueInBytes": 100, + }, + "realm": "realm-1", + } + `); + + expect( + ConfigSchema.validate({ + authc: { providers: ['saml'], saml: { realm: 'realm-1', maxRedirectURLSize: '1kb' } }, + }).authc.saml + ).toMatchInlineSnapshot(` + Object { + "maxRedirectURLSize": ByteSizeValue { + "valueInBytes": 1024, + }, + "realm": "realm-1", + } + `); + + expect( + ConfigSchema.validate({ + authc: { providers: ['saml'], saml: { realm: 'realm-1', maxRedirectURLSize: '100b' } }, + }).authc.saml + ).toMatchInlineSnapshot(` + Object { + "maxRedirectURLSize": ByteSizeValue { + "valueInBytes": 100, + }, + "realm": "realm-1", + } + `); + + expect( + ConfigSchema.validate({ + authc: { providers: ['saml'], saml: { realm: 'realm-1', maxRedirectURLSize: 0 } }, + }).authc.saml + ).toMatchInlineSnapshot(` + Object { + "maxRedirectURLSize": ByteSizeValue { + "valueInBytes": 0, + }, + "realm": "realm-1", + } + `); + }); }); }); @@ -183,12 +253,12 @@ describe('createConfig$()', () => { expect(config).toEqual({ encryptionKey: 'ab'.repeat(16), secureCookies: true }); expect(loggingServiceMock.collect(contextMock.logger).warn).toMatchInlineSnapshot(` -Array [ - Array [ - "Generating a random key for xpack.security.encryptionKey. To prevent sessions from being invalidated on restart, please set xpack.security.encryptionKey in kibana.yml", - ], -] -`); + Array [ + Array [ + "Generating a random key for xpack.security.encryptionKey. To prevent sessions from being invalidated on restart, please set xpack.security.encryptionKey in kibana.yml", + ], + ] + `); }); it('should log a warning if SSL is not configured', async () => { @@ -203,12 +273,12 @@ Array [ expect(config).toEqual({ encryptionKey: 'a'.repeat(32), secureCookies: false }); expect(loggingServiceMock.collect(contextMock.logger).warn).toMatchInlineSnapshot(` -Array [ - Array [ - "Session cookies will be transmitted over insecure connections. This is not recommended.", - ], -] -`); + Array [ + Array [ + "Session cookies will be transmitted over insecure connections. This is not recommended.", + ], + ] + `); }); it('should log a warning if SSL is not configured yet secure cookies are being used', async () => { @@ -223,12 +293,12 @@ Array [ expect(config).toEqual({ encryptionKey: 'a'.repeat(32), secureCookies: true }); expect(loggingServiceMock.collect(contextMock.logger).warn).toMatchInlineSnapshot(` -Array [ - Array [ - "Using secure cookies, but SSL is not enabled inside Kibana. SSL must be configured outside of Kibana to function properly.", - ], -] -`); + Array [ + Array [ + "Using secure cookies, but SSL is not enabled inside Kibana. SSL must be configured outside of Kibana to function properly.", + ], + ] + `); }); it('should set xpack.security.secureCookies if SSL is configured', async () => { diff --git a/x-pack/plugins/security/server/plugin.test.ts b/x-pack/plugins/security/server/plugin.test.ts index 6fe28fe56c417a..8cd029946d0713 100644 --- a/x-pack/plugins/security/server/plugin.test.ts +++ b/x-pack/plugins/security/server/plugin.test.ts @@ -6,6 +6,7 @@ import { coreMock, elasticsearchServiceMock } from '../../../../src/core/server/mocks'; +import { ByteSizeValue } from '@kbn/config-schema'; import { Plugin } from './plugin'; import { ClusterClient, CoreSetup } from '../../../../src/core/server'; @@ -18,7 +19,10 @@ describe('Security Plugin', () => { coreMock.createPluginInitializerContext({ cookieName: 'sid', sessionTimeout: 1500, - authc: { providers: ['saml', 'token'], saml: { realm: 'saml1' } }, + authc: { + providers: ['saml', 'token'], + saml: { realm: 'saml1', maxRedirectURLSize: new ByteSizeValue(2048) }, + }, }) ); diff --git a/x-pack/plugins/security/server/routes/authentication.test.ts b/x-pack/plugins/security/server/routes/authentication.test.ts new file mode 100644 index 00000000000000..c7d35565a6fcb6 --- /dev/null +++ b/x-pack/plugins/security/server/routes/authentication.test.ts @@ -0,0 +1,219 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { Type } from '@kbn/config-schema'; +import { Authentication, AuthenticationResult, SAMLLoginStep } from '../authentication'; +import { defineAuthenticationRoutes } from './authentication'; +import { + httpServerMock, + httpServiceMock, + loggingServiceMock, +} from '../../../../../src/core/server/mocks'; +import { ConfigType } from '../config'; +import { IRouter, RequestHandler, RouteConfig } from '../../../../../src/core/server'; +import { LegacyAPI } from '../plugin'; +import { authenticationMock } from '../authentication/index.mock'; +import { mockAuthenticatedUser } from '../../common/model/authenticated_user.mock'; + +describe('SAML authentication routes', () => { + let router: jest.Mocked; + let authc: jest.Mocked; + beforeEach(() => { + router = httpServiceMock.createRouter(); + authc = authenticationMock.create(); + + defineAuthenticationRoutes({ + router, + basePath: httpServiceMock.createBasePath(), + logger: loggingServiceMock.create().get(), + config: { authc: { providers: ['saml'] } } as ConfigType, + authc, + getLegacyAPI: () => + ({ cspRules: 'test-csp-rule', serverBasePath: '/test-base-path' } as LegacyAPI), + }); + }); + + it('does not register any SAML related routes if SAML auth provider is not enabled', () => { + const testRouter = httpServiceMock.createRouter(); + defineAuthenticationRoutes({ + router: testRouter, + basePath: httpServiceMock.createBasePath(), + logger: loggingServiceMock.create().get(), + config: { authc: { providers: ['basic'] } } as ConfigType, + authc: authenticationMock.create(), + getLegacyAPI: () => + ({ cspRules: 'test-csp-rule', serverBasePath: '/test-base-path' } as LegacyAPI), + }); + + const samlRoutePathPredicate = ([{ path }]: [{ path: string }, any]) => + path.startsWith('/api/security/saml/'); + expect(testRouter.get.mock.calls.find(samlRoutePathPredicate)).toBeUndefined(); + expect(testRouter.post.mock.calls.find(samlRoutePathPredicate)).toBeUndefined(); + expect(testRouter.put.mock.calls.find(samlRoutePathPredicate)).toBeUndefined(); + expect(testRouter.delete.mock.calls.find(samlRoutePathPredicate)).toBeUndefined(); + }); + + describe('Assertion consumer service endpoint', () => { + let routeHandler: RequestHandler; + let routeConfig: RouteConfig; + beforeEach(() => { + const [acsRouteConfig, acsRouteHandler] = router.post.mock.calls.find( + ([{ path }]) => path === '/api/security/saml/callback' + )!; + + routeConfig = acsRouteConfig; + routeHandler = acsRouteHandler; + }); + + it('additionally registers BWC route', () => { + expect( + router.post.mock.calls.find(([{ path }]) => path === '/api/security/saml/callback') + ).toBeDefined(); + expect( + router.post.mock.calls.find(([{ path }]) => path === '/api/security/v1/saml') + ).toBeDefined(); + }); + + it('correctly defines route.', () => { + expect(routeConfig.options).toEqual({ authRequired: false }); + expect(routeConfig.validate).toEqual({ + body: expect.any(Type), + query: undefined, + params: undefined, + }); + + const bodyValidator = (routeConfig.validate as any).body as Type; + expect(bodyValidator.validate({ SAMLResponse: 'saml-response' })).toEqual({ + SAMLResponse: 'saml-response', + }); + + expect(bodyValidator.validate({ SAMLResponse: 'saml-response', RelayState: '' })).toEqual({ + SAMLResponse: 'saml-response', + RelayState: '', + }); + + expect( + bodyValidator.validate({ SAMLResponse: 'saml-response', RelayState: 'relay-state' }) + ).toEqual({ SAMLResponse: 'saml-response', RelayState: 'relay-state' }); + + expect(() => bodyValidator.validate({})).toThrowErrorMatchingInlineSnapshot( + `"[SAMLResponse]: expected value of type [string] but got [undefined]"` + ); + + expect(() => + bodyValidator.validate({ SAMLResponse: 'saml-response', UnknownArg: 'arg' }) + ).toThrowErrorMatchingInlineSnapshot(`"[UnknownArg]: definition for this key is missing"`); + }); + + it('returns 500 if authentication throws unhandled exception.', async () => { + const unhandledException = new Error('Something went wrong.'); + authc.login.mockRejectedValue(unhandledException); + + const internalServerErrorResponse = Symbol('error'); + const responseFactory = httpServerMock.createResponseFactory(); + responseFactory.internalError.mockReturnValue(internalServerErrorResponse as any); + + const request = httpServerMock.createKibanaRequest({ + body: { SAMLResponse: 'saml-response' }, + }); + + await expect(routeHandler({} as any, request, responseFactory)).resolves.toBe( + internalServerErrorResponse + ); + + expect(authc.login).toHaveBeenCalledWith(request, { + provider: 'saml', + value: { + step: SAMLLoginStep.SAMLResponseReceived, + samlResponse: 'saml-response', + }, + }); + }); + + it('returns 401 if authentication fails.', async () => { + const failureReason = new Error('Something went wrong.'); + authc.login.mockResolvedValue(AuthenticationResult.failed(failureReason)); + + const unauthorizedErrorResponse = Symbol('error'); + const responseFactory = httpServerMock.createResponseFactory(); + responseFactory.unauthorized.mockReturnValue(unauthorizedErrorResponse as any); + + await expect( + routeHandler( + {} as any, + httpServerMock.createKibanaRequest({ body: { SAMLResponse: 'saml-response' } }), + responseFactory + ) + ).resolves.toBe(unauthorizedErrorResponse); + + expect(responseFactory.unauthorized).toHaveBeenCalledWith({ body: failureReason }); + }); + + it('returns 401 if authentication is not handled.', async () => { + authc.login.mockResolvedValue(AuthenticationResult.notHandled()); + + const unauthorizedErrorResponse = Symbol('error'); + const responseFactory = httpServerMock.createResponseFactory(); + responseFactory.unauthorized.mockReturnValue(unauthorizedErrorResponse as any); + + await expect( + routeHandler( + {} as any, + httpServerMock.createKibanaRequest({ body: { SAMLResponse: 'saml-response' } }), + responseFactory + ) + ).resolves.toBe(unauthorizedErrorResponse); + + expect(responseFactory.unauthorized).toHaveBeenCalledWith({ body: undefined }); + }); + + it('returns 401 if authentication completes with unexpected result.', async () => { + authc.login.mockResolvedValue(AuthenticationResult.succeeded(mockAuthenticatedUser())); + + const unauthorizedErrorResponse = Symbol('error'); + const responseFactory = httpServerMock.createResponseFactory(); + responseFactory.unauthorized.mockReturnValue(unauthorizedErrorResponse as any); + + await expect( + routeHandler( + {} as any, + httpServerMock.createKibanaRequest({ body: { SAMLResponse: 'saml-response' } }), + responseFactory + ) + ).resolves.toBe(unauthorizedErrorResponse); + + expect(responseFactory.unauthorized).toHaveBeenCalledWith({ body: undefined }); + }); + + it('redirects if required by the authentication process.', async () => { + authc.login.mockResolvedValue(AuthenticationResult.redirectTo('http://redirect-to/path')); + + const redirectResponse = Symbol('error'); + const responseFactory = httpServerMock.createResponseFactory(); + responseFactory.redirected.mockReturnValue(redirectResponse as any); + + const request = httpServerMock.createKibanaRequest({ + body: { SAMLResponse: 'saml-response' }, + }); + + await expect(routeHandler({} as any, request, responseFactory)).resolves.toBe( + redirectResponse + ); + + expect(authc.login).toHaveBeenCalledWith(request, { + provider: 'saml', + value: { + step: SAMLLoginStep.SAMLResponseReceived, + samlResponse: 'saml-response', + }, + }); + + expect(responseFactory.redirected).toHaveBeenCalledWith({ + headers: { location: 'http://redirect-to/path' }, + }); + }); + }); +}); diff --git a/x-pack/test/saml_api_integration/apis/security/saml_login.ts b/x-pack/test/saml_api_integration/apis/security/saml_login.ts index abe4da5e2a7762..925d28b62f9bb8 100644 --- a/x-pack/test/saml_api_integration/apis/security/saml_login.ts +++ b/x-pack/test/saml_api_integration/apis/security/saml_login.ts @@ -35,6 +35,32 @@ export default function({ getService }: FtrProviderContext) { }); } + async function checkSessionCookie(sessionCookie: Cookie) { + expect(sessionCookie.key).to.be('sid'); + expect(sessionCookie.value).to.not.be.empty(); + expect(sessionCookie.path).to.be('/'); + expect(sessionCookie.httpOnly).to.be(true); + + const apiResponse = await supertest + .get('/api/security/v1/me') + .set('kbn-xsrf', 'xxx') + .set('Cookie', sessionCookie.cookieString()) + .expect(200); + + expect(apiResponse.body).to.only.have.keys([ + 'username', + 'full_name', + 'email', + 'roles', + 'metadata', + 'enabled', + 'authentication_realm', + 'lookup_realm', + ]); + + expect(apiResponse.body.username).to.be('a@b.c'); + } + describe('SAML authentication', () => { it('should reject API requests if client is not authenticated', async () => { await supertest @@ -226,30 +252,7 @@ export default function({ getService }: FtrProviderContext) { const cookies = samlAuthenticationResponse.headers['set-cookie']; expect(cookies).to.have.length(1); - const sessionCookie = request.cookie(cookies[0])!; - expect(sessionCookie.key).to.be('sid'); - expect(sessionCookie.value).to.not.be.empty(); - expect(sessionCookie.path).to.be('/'); - expect(sessionCookie.httpOnly).to.be(true); - - const apiResponse = await supertest - .get('/api/security/v1/me') - .set('kbn-xsrf', 'xxx') - .set('Cookie', sessionCookie.cookieString()) - .expect(200); - - expect(apiResponse.body).to.only.have.keys([ - 'username', - 'full_name', - 'email', - 'roles', - 'metadata', - 'enabled', - 'authentication_realm', - 'lookup_realm', - ]); - - expect(apiResponse.body.username).to.be('a@b.c'); + await checkSessionCookie(request.cookie(cookies[0])!); }); it('should succeed in case of IdP initiated login', async () => { @@ -266,30 +269,7 @@ export default function({ getService }: FtrProviderContext) { const cookies = samlAuthenticationResponse.headers['set-cookie']; expect(cookies).to.have.length(1); - const sessionCookie = request.cookie(cookies[0])!; - expect(sessionCookie.key).to.be('sid'); - expect(sessionCookie.value).to.not.be.empty(); - expect(sessionCookie.path).to.be('/'); - expect(sessionCookie.httpOnly).to.be(true); - - const apiResponse = await supertest - .get('/api/security/v1/me') - .set('kbn-xsrf', 'xxx') - .set('Cookie', sessionCookie.cookieString()) - .expect(200); - - expect(apiResponse.body).to.only.have.keys([ - 'username', - 'full_name', - 'email', - 'roles', - 'metadata', - 'enabled', - 'authentication_realm', - 'lookup_realm', - ]); - - expect(apiResponse.body.username).to.be('a@b.c'); + await checkSessionCookie(request.cookie(cookies[0])!); }); it('should fail if SAML response is not valid', async () => { @@ -775,5 +755,83 @@ export default function({ getService }: FtrProviderContext) { expect(acceptedResponse.body).to.have.property('username', newUsername); }); }); + + describe('handshake with very long URL path or fragment', () => { + it('should not try to capture URL fragment if path is too big already', async () => { + // 1. Initiate SAML handshake. + const handshakeResponse = await supertest + .get(`/abc/xyz/${'handshake'.repeat(10)}?one=two three`) + .expect(302); + const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0])!; + const redirectURL = url.parse( + handshakeResponse.headers.location, + true /* parseQueryString */ + ); + + expect(redirectURL.href!.startsWith(`https://elastic.co/sso/saml`)).to.be(true); + expect(redirectURL.query.SAMLRequest).to.not.be.empty(); + + // 2. Finish SAML handshake + const samlRequestId = await getSAMLRequestId(handshakeResponse.headers.location); + const samlAuthenticationResponse = await supertest + .post('/api/security/saml/callback') + .set('kbn-xsrf', 'xxx') + .set('Cookie', handshakeCookie.cookieString()) + .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }) }) + .expect(302); + + // User should be redirected to the root URL since we couldn't even save URL path. + expect(samlAuthenticationResponse.headers.location).to.be('/'); + + await checkSessionCookie( + request.cookie(samlAuthenticationResponse.headers['set-cookie'][0])! + ); + }); + + it('should capture only URL path if URL fragment is too big', async () => { + // 1. Capture current path + const captureURLResponse = await supertest + .get('/abc/xyz/handshake?one=two three') + .expect(302); + const captureURLCookie = request.cookie(captureURLResponse.headers['set-cookie'][0])!; + + expect(captureURLResponse.headers.location).to.be( + '/api/security/saml/capture-url-fragment' + ); + + // 2. Initiate SAML handshake. + const handshakeResponse = await supertest + .get(`/api/security/saml/start?redirectURLFragment=%23%2F${'workpad'.repeat(10)}`) + .set('Cookie', captureURLCookie.cookieString()) + .expect(302); + + const handshakeCookie = request.cookie(handshakeResponse.headers['set-cookie'][0])!; + const redirectURL = url.parse( + handshakeResponse.headers.location, + true /* parseQueryString */ + ); + + expect(redirectURL.href!.startsWith(`https://elastic.co/sso/saml`)).to.be(true); + expect(redirectURL.query.SAMLRequest).to.not.be.empty(); + + // 3. Finish SAML handshake + const samlRequestId = await getSAMLRequestId(handshakeResponse.headers.location); + const samlAuthenticationResponse = await supertest + .post('/api/security/saml/callback') + .set('kbn-xsrf', 'xxx') + .set('Cookie', handshakeCookie.cookieString()) + .send({ SAMLResponse: await createSAMLResponse({ inResponseTo: samlRequestId }) }) + .expect(302); + + // User should be redirected to the URL path that initiated SAML handshake. + expect(samlAuthenticationResponse.headers.location).to.be( + '/abc/xyz/handshake?one=two%20three' + ); + + await checkSessionCookie( + request.cookie(samlAuthenticationResponse.headers['set-cookie'][0])! + ); + }); + }); }); } diff --git a/x-pack/test/saml_api_integration/config.ts b/x-pack/test/saml_api_integration/config.ts index a9edd3864919b4..8fb807016afeb6 100644 --- a/x-pack/test/saml_api_integration/config.ts +++ b/x-pack/test/saml_api_integration/config.ts @@ -52,6 +52,7 @@ export default async function({ readConfigFile }: FtrConfigProviderContext) { '--server.xsrf.whitelist=["/api/security/saml/callback"]', `--xpack.security.authc.providers=${JSON.stringify(['saml', 'basic'])}`, '--xpack.security.authc.saml.realm=saml1', + '--xpack.security.authc.saml.maxRedirectURLSize=100b', ], }, }; From 2b88f7b4347c7e38055720d18c5f2978c5b108cc Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Tue, 8 Oct 2019 11:02:29 +0200 Subject: [PATCH 09/11] Use `serverBasePath` provided by the core instead of the one provided via legacy shim. --- x-pack/legacy/plugins/security/index.js | 1 - .../server/authentication/providers/saml.test.ts | 6 +++--- .../server/authentication/providers/saml.ts | 9 ++++----- x-pack/plugins/security/server/plugin.ts | 1 - .../server/routes/authentication.test.ts | 6 ++---- .../security/server/routes/authentication.ts | 16 ++++++++++------ 6 files changed, 19 insertions(+), 20 deletions(-) diff --git a/x-pack/legacy/plugins/security/index.js b/x-pack/legacy/plugins/security/index.js index b396ef6cabe2f7..980af19cc8362b 100644 --- a/x-pack/legacy/plugins/security/index.js +++ b/x-pack/legacy/plugins/security/index.js @@ -138,7 +138,6 @@ export const security = (kibana) => new kibana.Plugin({ server.plugins.kibana.systemApi ), cspRules: createCSPRuleString(config.get('csp.rules')), - serverBasePath: config.get('server.basePath'), }); const plugin = this; diff --git a/x-pack/plugins/security/server/authentication/providers/saml.test.ts b/x-pack/plugins/security/server/authentication/providers/saml.test.ts index 732472230cfe9f..1fc49d5d344840 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.test.ts @@ -527,7 +527,7 @@ describe('SAMLAuthenticationProvider', () => { expect(authenticationResult.redirected()).toBe(true); expect(authenticationResult.redirectURL).toBe( - '/base-path/api/security/saml/capture-url-fragment' + '/mock-server-basepath/api/security/saml/capture-url-fragment' ); expect(authenticationResult.state).toEqual({ redirectURL: '/base-path/s/foo/some-path' }); }); @@ -746,7 +746,7 @@ describe('SAMLAuthenticationProvider', () => { expect(authenticationResult.redirected()).toBe(true); expect(authenticationResult.redirectURL).toBe( - '/base-path/api/security/saml/capture-url-fragment' + '/mock-server-basepath/api/security/saml/capture-url-fragment' ); expect(authenticationResult.state).toEqual({ redirectURL: '/base-path/s/foo/some-path' }); }); @@ -819,7 +819,7 @@ describe('SAMLAuthenticationProvider', () => { expect(authenticationResult.redirected()).toBe(true); expect(authenticationResult.redirectURL).toBe( - '/base-path/api/security/saml/capture-url-fragment' + '/mock-server-basepath/api/security/saml/capture-url-fragment' ); expect(authenticationResult.state).toEqual({ redirectURL: '/base-path/s/foo/some-path' }); }); diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts index 87e1a11591a35a..712ddf3fb62109 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.ts @@ -571,10 +571,9 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { return this.authenticateViaHandshake(request, ''); } - // We should use `serverBasePath` here as soon as it's provided by the core. Using request scoped - // base path doesn't hurt, but isn't consistent with the rest of API routes. - return AuthenticationResult.redirectTo(`${basePath}/api/security/saml/capture-url-fragment`, { - state: { redirectURL }, - }); + return AuthenticationResult.redirectTo( + `${this.options.basePath.serverBasePath}/api/security/saml/capture-url-fragment`, + { state: { redirectURL } } + ); } } diff --git a/x-pack/plugins/security/server/plugin.ts b/x-pack/plugins/security/server/plugin.ts index dd076ffdc6c32f..18717f3e132b9f 100644 --- a/x-pack/plugins/security/server/plugin.ts +++ b/x-pack/plugins/security/server/plugin.ts @@ -27,7 +27,6 @@ export interface LegacyAPI { xpackInfo: Pick; isSystemAPIRequest: (request: KibanaRequest) => boolean; cspRules: string; - serverBasePath?: string; } /** diff --git a/x-pack/plugins/security/server/routes/authentication.test.ts b/x-pack/plugins/security/server/routes/authentication.test.ts index c7d35565a6fcb6..67fd86587a0ce5 100644 --- a/x-pack/plugins/security/server/routes/authentication.test.ts +++ b/x-pack/plugins/security/server/routes/authentication.test.ts @@ -31,8 +31,7 @@ describe('SAML authentication routes', () => { logger: loggingServiceMock.create().get(), config: { authc: { providers: ['saml'] } } as ConfigType, authc, - getLegacyAPI: () => - ({ cspRules: 'test-csp-rule', serverBasePath: '/test-base-path' } as LegacyAPI), + getLegacyAPI: () => ({ cspRules: 'test-csp-rule' } as LegacyAPI), }); }); @@ -44,8 +43,7 @@ describe('SAML authentication routes', () => { logger: loggingServiceMock.create().get(), config: { authc: { providers: ['basic'] } } as ConfigType, authc: authenticationMock.create(), - getLegacyAPI: () => - ({ cspRules: 'test-csp-rule', serverBasePath: '/test-base-path' } as LegacyAPI), + getLegacyAPI: () => ({ cspRules: 'test-csp-rule' } as LegacyAPI), }); const samlRoutePathPredicate = ([{ path }]: [{ path: string }, any]) => diff --git a/x-pack/plugins/security/server/routes/authentication.ts b/x-pack/plugins/security/server/routes/authentication.ts index 049cab811edff6..35a63e87339c42 100644 --- a/x-pack/plugins/security/server/routes/authentication.ts +++ b/x-pack/plugins/security/server/routes/authentication.ts @@ -17,7 +17,13 @@ export function defineAuthenticationRoutes(params: RouteDefinitionParams) { /** * Defines routes required for SAML authentication. */ -function defineSAMLRoutes({ router, logger, authc, getLegacyAPI }: RouteDefinitionParams) { +function defineSAMLRoutes({ + router, + logger, + authc, + getLegacyAPI, + basePath, +}: RouteDefinitionParams) { function createCustomResourceResponse(body: string, contentType: string) { return { body, @@ -37,7 +43,6 @@ function defineSAMLRoutes({ router, logger, authc, getLegacyAPI }: RouteDefiniti options: { authRequired: false }, }, (context, request, response) => { - const serverBasePath = getLegacyAPI().serverBasePath || ''; // We're also preventing `favicon.ico` request since it can cause new SAML handshake. return response.custom( createCustomResourceResponse( @@ -45,7 +50,7 @@ function defineSAMLRoutes({ router, logger, authc, getLegacyAPI }: RouteDefiniti Kibana SAML Login - + `, 'text/html' ) @@ -60,12 +65,11 @@ function defineSAMLRoutes({ router, logger, authc, getLegacyAPI }: RouteDefiniti options: { authRequired: false }, }, (context, request, response) => { - const serverBasePath = getLegacyAPI().serverBasePath || ''; return response.custom( createCustomResourceResponse( ` window.location.replace( - '${serverBasePath}/api/security/saml/start?redirectURLFragment=' + encodeURIComponent(window.location.hash) + '${basePath.serverBasePath}/api/security/saml/start?redirectURLFragment=' + encodeURIComponent(window.location.hash) ); `, 'text/javascript' @@ -120,7 +124,7 @@ function defineSAMLRoutes({ router, logger, authc, getLegacyAPI }: RouteDefiniti async (context, request, response) => { try { if (path === '/api/security/v1/saml') { - const serverBasePath = getLegacyAPI().serverBasePath || ''; + const serverBasePath = basePath.serverBasePath; logger.warn( `The "${serverBasePath}${path}" URL is deprecated and will stop working in the next major version, please use "${serverBasePath}/api/security/saml/callback" URL instead.`, { tags: ['deprecation'] } From dca64319e824476dc5a584488313e6169381175e Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Wed, 9 Oct 2019 13:08:31 +0200 Subject: [PATCH 10/11] Apply suggestions from code review Co-Authored-By: gchaps <33642766+gchaps@users.noreply.github.com> --- docs/user/security/authentication/index.asciidoc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/user/security/authentication/index.asciidoc b/docs/user/security/authentication/index.asciidoc index 99763955ced85c..8a4678e051490b 100644 --- a/docs/user/security/authentication/index.asciidoc +++ b/docs/user/security/authentication/index.asciidoc @@ -122,10 +122,12 @@ Basic authentication is supported _only_ if `basic` authentication provider is e [float] ===== SAML and long URLs -At the beginning of the SAML handshake {kib} stores initial URL in the session cookie to be able to redirect user back to this URL after successful SAML authentication. -In case the URL is very long session cookie may exceed the maximum size supported by the browser (usually it's around 4KB for all cookies per domain) and it will truncate -or drop such cookie completely. If you experience sporadic failures during SAML authentication that may be the reason why. To remedy this issue you need to decrease the maximum -size of the URL {kib} is allowed to store during SAML handshake, the default value is 2KB: +At the beginning of the SAML handshake, {kib} stores the initial URL in the session cookie, so it can redirect the user back to that URL after successful SAML authentication. +If the URL is long, the session cookie might exceed the maximum size supported by the browser--typically 4KB for all cookies per domain. When this happens, the session cookie is truncated, +or dropped completely, and you might experience sporadic failures during SAML authentication. + +To remedy this issue, you can decrease the maximum +size of the URL that {kib} is allowed to store during the SAML handshake. The default value is 2KB. [source,yaml] -------------------------------------------------------------------------------- From 8b51a1c0433758fe5fe236b884460a36c78589ba Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Wed, 9 Oct 2019 15:26:44 +0200 Subject: [PATCH 11/11] Make sure redirect URL fragment always starts with `#`. --- .../authentication/providers/saml.test.ts | 38 +++++++++++++++++++ .../server/authentication/providers/saml.ts | 8 +++- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/security/server/authentication/providers/saml.test.ts b/x-pack/plugins/security/server/authentication/providers/saml.test.ts index 1fc49d5d344840..7ef1d934a7d130 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.test.ts @@ -423,6 +423,44 @@ describe('SAMLAuthenticationProvider', () => { }); }); + it('prepends redirect URL fragment with `#` if it does not have one.', async () => { + const request = httpServerMock.createKibanaRequest(); + + mockOptions.client.callAsInternalUser.withArgs('shield.samlPrepare').resolves({ + id: 'some-request-id', + redirect: 'https://idp-host/path/login?SAMLRequest=some%20request%20', + }); + + const authenticationResult = await provider.login( + request, + { + step: SAMLLoginStep.RedirectURLFragmentCaptured, + redirectURLFragment: '../some-fragment', + }, + { redirectURL: '/test-base-path/some-path' } + ); + + sinon.assert.calledWithExactly( + mockOptions.client.callAsInternalUser, + 'shield.samlPrepare', + { body: { realm: 'test-realm' } } + ); + + expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1); + expect(mockOptions.logger.warn).toHaveBeenCalledWith( + 'Redirect URL fragment does not start with `#`.' + ); + + expect(authenticationResult.redirected()).toBe(true); + expect(authenticationResult.redirectURL).toBe( + 'https://idp-host/path/login?SAMLRequest=some%20request%20' + ); + expect(authenticationResult.state).toEqual({ + requestId: 'some-request-id', + redirectURL: '/test-base-path/some-path#../some-fragment', + }); + }); + it('redirects non-AJAX requests to the IdP remembering only redirect URL path if fragment is too large.', async () => { const request = httpServerMock.createKibanaRequest(); diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts index 712ddf3fb62109..b21a23718f8612 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.ts @@ -114,7 +114,13 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { return AuthenticationResult.failed(Boom.badRequest(message)); } - let redirectURL = `${state.redirectURL}${attempt.redirectURLFragment}`; + let redirectURLFragment = attempt.redirectURLFragment; + if (redirectURLFragment.length > 0 && !redirectURLFragment.startsWith('#')) { + this.logger.warn('Redirect URL fragment does not start with `#`.'); + redirectURLFragment = `#${redirectURLFragment}`; + } + + let redirectURL = `${state.redirectURL}${redirectURLFragment}`; const redirectURLSize = new ByteSizeValue(Buffer.byteLength(redirectURL)); if (this.maxRedirectURLSize.isLessThan(redirectURLSize)) { this.logger.warn(