Skip to content

Commit

Permalink
Preserve URL fragment during SAML handshake. (#44513)
Browse files Browse the repository at this point in the history
  • Loading branch information
azasypkin committed Oct 9, 2019
1 parent 019a3f1 commit 0210ce4
Show file tree
Hide file tree
Showing 28 changed files with 1,538 additions and 507 deletions.
17 changes: 16 additions & 1 deletion docs/user/security/authentication/index.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ xpack.security.authc.saml.realm: realm-name
+
[source,yaml]
--------------------------------------------------------------------------------
server.xsrf.whitelist: [/api/security/v1/saml]
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_.
Expand All @@ -119,6 +119,21 @@ 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 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]
--------------------------------------------------------------------------------
xpack.security.authc.saml.maxRedirectURLSize: 1kb
--------------------------------------------------------------------------------

[[oidc]]
==== OpenID Connect Single Sign-On

Expand Down
24 changes: 16 additions & 8 deletions renovate.json5
Original file line number Diff line number Diff line change
Expand Up @@ -807,6 +807,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',
Expand Down Expand Up @@ -919,14 +935,6 @@
'@types/parse-link-header',
],
},
{
groupSlug: 'xml2js',
groupName: 'xml2js related packages',
packageNames: [
'xml2js',
'@types/xml2js',
],
},
{
packagePatterns: [
'^@kbn/.*',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ kibana_vars=(
xpack.security.authc.providers
xpack.security.authc.oidc.realm
xpack.security.authc.saml.realm
xpack.security.authc.saml.maxRedirectURLSize
xpack.security.cookieName
xpack.security.enabled
xpack.security.encryptionKey
Expand Down
4 changes: 3 additions & 1 deletion x-pack/legacy/plugins/security/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -128,17 +129,18 @@ 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({
xpackInfo,
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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -58,37 +58,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
Expand Down
2 changes: 2 additions & 0 deletions x-pack/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@
"@types/tar-fs": "^1.16.1",
"@types/tinycolor2": "^1.4.1",
"@types/uuid": "^3.4.4",
"@types/xml2js": "^0.4.5",
"@types/xml-crypto": "^1.4.0",
"abab": "^1.0.4",
"ansicolors": "0.3.2",
"axios": "^0.19.0",
Expand Down
18 changes: 18 additions & 0 deletions x-pack/plugins/security/server/authentication/index.mock.ts
Original file line number Diff line number Diff line change
@@ -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<Authentication> => ({
login: jest.fn(),
createAPIKey: jest.fn(),
getCurrentUser: jest.fn(),
invalidateAPIKey: jest.fn(),
isAuthenticated: jest.fn(),
logout: jest.fn(),
}),
};
12 changes: 10 additions & 2 deletions x-pack/plugins/security/server/authentication/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
IClusterClient,
CoreSetup,
Expand All @@ -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;
Expand All @@ -31,6 +37,8 @@ interface SetupAuthenticationParams {
getLegacyAPI(): LegacyAPI;
}

export type Authentication = UnwrapPromise<ReturnType<typeof setupAuthentication>>;

export async function setupAuthentication({
core,
clusterClient,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Loading

0 comments on commit 0210ce4

Please sign in to comment.