Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve URL fragment during SAML handshake. #44513

Merged
merged 23 commits into from
Oct 9, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
71f2980
Preserve URL fragment during SAML handshake.
azasypkin Aug 30, 2019
c891059
Merge branch 'master' into issue-18392-saml-hash
azasypkin Sep 4, 2019
a3e32fd
Merge branch 'master' into issue-18392-saml-hash
azasypkin Sep 5, 2019
ca8942a
Merge branch 'master' into issue-18392-saml-hash
azasypkin Sep 5, 2019
19e6fb3
Merge branch 'master' into issue-18392-saml-hash
azasypkin Sep 5, 2019
1f7faff
Sync with upstream and use route URLs that are relative to root. Use …
azasypkin Sep 5, 2019
017f7bb
Merge branch 'master' into issue-18392-saml-hash
azasypkin Sep 12, 2019
f564987
Review#1: get rid of `authc` part in SAML API routes.
azasypkin Sep 12, 2019
776ca98
Review#1: capture basepath and space ID.
azasypkin Sep 12, 2019
8270f19
Review#1: migrate to alternative solution to store full URL in the co…
azasypkin Sep 12, 2019
6febc2c
Review#1: use server base path instead of the request scoped one for …
azasypkin Sep 13, 2019
432d7bb
Merge branch 'master' into issue-18392-saml-hash
azasypkin Sep 13, 2019
ff90be4
Properly handle max redirect URL size.
azasypkin Sep 13, 2019
184c977
Merge branch 'master' into issue-18392-saml-hash
azasypkin Sep 16, 2019
3302508
Merge branch 'master' into issue-18392-saml-hash
azasypkin Sep 17, 2019
50fada9
Update tests.
azasypkin Sep 17, 2019
170ee51
Merge branch 'master' into issue-18392-saml-hash
elasticmachine Sep 20, 2019
a38be2e
Merge branch 'master' into issue-18392-saml-hash
elasticmachine Sep 23, 2019
a361167
Merge branch 'master' into issue-18392-saml-hash
azasypkin Oct 8, 2019
2b88f7b
Use `serverBasePath` provided by the core instead of the one provided…
azasypkin Oct 8, 2019
dca6431
Apply suggestions from code review
azasypkin Oct 9, 2019
0a86b77
Merge branch 'master' into issue-18392-saml-hash
azasypkin Oct 9, 2019
8b51a1c
Make sure redirect URL fragment always starts with `#`.
azasypkin Oct 9, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/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: [/security/api/authc/saml/callback]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: not completely sure if we need authc part. It may seem redundant, but at the same the security plugin API surface will expand significantly over time with various authz endpoints and such so it may be beneficial to separate them....

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once #44855 is merged I'll change URLs from /security/api/* to /api/security/*. The question regarding authc part in URL is still open though.

--------------------------------------------------------------------------------

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 Down
16 changes: 16 additions & 0 deletions renovate.json5
Original file line number Diff line number Diff line change
Expand Up @@ -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',
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')),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: if we still need this we'll include that into NP blockers for Security plugin.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Included as blocker, for now we agreed with Platform to provide this through legacy API shim and hopefully get support for "lightweight" apps by 8.0 so that we don't need to set CSP manually anymore.

});

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
52 changes: 21 additions & 31 deletions x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Member Author

@azasypkin azasypkin Sep 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: that the only BWC route deprecation approach I managed to find that wouldn't require NP plugin to expose SAML APIs directly (so that we can call them here). I don't like it, I'd rather be able to temporarily define routes with any URL for BWC reasons or have something like "URL alias" that we could define in NP for the new route. Will discuss with the Platform Team (discussing here #44620)

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',
Expand Down Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions x-pack/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
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 {
ClusterClient,
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';
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
mockScopedClusterClient,
} from './base.mock';

import { SAMLAuthenticationProvider } from './saml';
import { SAMLAuthenticationProvider, SAMLLoginStep } from './saml';

describe('SAMLAuthenticationProvider', () => {
let provider: SAMLAuthenticationProvider;
Expand Down Expand Up @@ -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(
Expand All @@ -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);
Expand All @@ -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' }
);

Expand All @@ -114,6 +122,7 @@ describe('SAMLAuthenticationProvider', () => {
});

const authenticationResult = await provider.login(request, {
step: SAMLLoginStep.SAMLResponseReceived,
samlResponse: 'saml-response-xml',
});

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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' }
);

Expand Down Expand Up @@ -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' }
);

Expand Down Expand Up @@ -247,7 +260,7 @@ describe('SAMLAuthenticationProvider', () => {

const authenticationResult = await provider.login(
request,
{ samlResponse: 'saml-response-xml' },
{ step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' },
tokenPair
);

Expand Down Expand Up @@ -281,7 +294,7 @@ describe('SAMLAuthenticationProvider', () => {

const authenticationResult = await provider.login(
request,
{ samlResponse: 'saml-response-xml' },
{ step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' },
tokenPair
);

Expand Down Expand Up @@ -329,7 +342,7 @@ describe('SAMLAuthenticationProvider', () => {

const authenticationResult = await provider.login(
request,
{ samlResponse: 'saml-response-xml' },
{ step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' },
tokenPair
);

Expand Down Expand Up @@ -377,7 +390,7 @@ describe('SAMLAuthenticationProvider', () => {

const authenticationResult = await provider.login(
request,
{ samlResponse: 'saml-response-xml' },
{ step: SAMLLoginStep.SAMLResponseReceived, samlResponse: 'saml-response-xml' },
tokenPair
);

Expand Down
Loading