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.

Sign up for GitHub

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

Add support for OpenID Connect implicit authentication flow. #42069

Merged
merged 7 commits into from
Aug 8, 2019
Merged

Add support for OpenID Connect implicit authentication flow. #42069

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
39e6237
Add support for OpenID Connect implicit authentication flow.
azasypkin Jul 26, 2019
1bf2b13
Merge branch 'master' into issue-xxx-oidc-implicit-flow
azasypkin Aug 5, 2019
a2932f0
Merge branch 'master' into issue-xxx-oidc-implicit-flow
azasypkin Aug 6, 2019
99a593d
Merge branch 'master' into issue-xxx-oidc-implicit-flow
azasypkin Aug 8, 2019
d73fd18
Review#1: add validation for the authenticationResponseURI query stri…
azasypkin Aug 8, 2019
293de31
Integrate upstream API integration tests changes.
azasypkin Aug 8, 2019
f325744
Merge branch 'master' into issue-xxx-oidc-implicit-flow
azasypkin Aug 8, 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
8 changes: 8 additions & 0 deletions renovate.json5
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,14 @@
'@types/history',
],
},
{
groupSlug: 'jsdom',
groupName: 'jsdom related packages',
packageNames: [
'jsdom',
'@types/jsdom',
],
},
{
groupSlug: 'jsonwebtoken',
groupName: 'jsonwebtoken related packages',
Expand Down
86 changes: 70 additions & 16 deletions x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@
import Boom from 'boom';
import Joi from 'joi';
import { schema } from '@kbn/config-schema';
import { canRedirectRequest, wrapError } from '../../../../../../../plugins/security/server';
import { canRedirectRequest, wrapError, OIDCAuthenticationFlow } from '../../../../../../../plugins/security/server';
import { KibanaRequest } from '../../../../../../../../src/core/server';
import { createCSPRuleString, generateCSPNonce } from '../../../../../../../../src/legacy/server/csp';

export function initAuthenticateApi({ authc: { login, logout }, config }, server) {

Expand Down Expand Up @@ -82,6 +83,36 @@ export function initAuthenticateApi({ authc: { login, logout }, config }, server
}
});

/**
* 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
* the `/api/security/v1/oidc` route.
*/
server.route({
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: didn't write unit tests for this endpoint as it's covered by integration tests and we'll need to rewrite them soon in Jest and TypeScript anyway (when we migrate to NP routes).

method: 'GET',
path: '/api/security/v1/oidc/implicit',
config: { auth: false },
async handler(request, h) {
const legacyConfig = server.config();
const basePath = legacyConfig.get('server.basePath');

const nonce = await generateCSPNonce();
const cspRulesHeader = createCSPRuleString(legacyConfig.get('csp.rules'), nonce);
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: using CSP here feels like an overkill here, but it doesn't hurt to have it I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to do anything about server.customResponseHeaders explicitly, or does the global hapi configuration apply automatically?

I've been trying to think of potential issues that we get from rendering all of the HTML ourselves, but I haven't been able to think of any.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need to do anything about server.customResponseHeaders explicitly, or does the global hapi configuration apply automatically?

Hapi's onPreResponse hooks do everything for us "automagically". I actually was surprised that CSP isn't done via onPreResponse too... But I admit I don't know much about all the variables we had to account for when we were implementing it.

return h.response(`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is likely a naive question. When reading through the OIDC spec, section 3.2.2.5 states

When using the Implicit Flow, all response parameters are added to the fragment component of the Redirection URI, as specified in OAuth 2.0 Multiple Response Type Encoding Practices [OAuth.Responses], unless a different Response Mode was specified.

And section 2.1 of the OAuth2.0 Response Type Encoding Practices makes it appear that we could potentially use the query response mode https://openid.net/specs/oauth-v2-multiple-response-types-1_0.html#rfc.section.2.1 to get around needing to use this page to parse the fragment.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is likely a naive question.

And what is the question? 🙂 So, I assume you're asking why we can't just say OP and RP (ES) to use response_mode=query for the implicit flow. Per OAuth 2.0 Multiple Response Type Encoding Practices it seems that query is forbidden for id_token and token response types:

The default Response Mode for this Response Type is the fragment encoding and the query encoding MUST NOT be used.

cc @jkakavas

Copy link
Member Author

Choose a reason for hiding this comment

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

Okta seems to obey this rule too https://developer.okta.com/docs/reference/api/oidc/#request-parameters

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, thanks for the clarification. I stopped reading the specs too soon.

Copy link
Member

Choose a reason for hiding this comment

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

fragment is used on purpose so that the access token and the id token are not sent as part of any requests to a back-end server ( The assumption was that the implicit flow would be best used by javascript clients running in the browser ) and thus don't get leaked in logs or be available more than needed on the wire. We (and many other clients) have the validation on the server side so we end up making a request with it to the kibana server eitherway so there goes that.. In any case as @azasypkin mentioned we're not allowed to use anything else for response_type

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @jkakavas

<!DOCTYPE html>
<title>Kibana OpenID Connect Login</title>
<script nonce="${nonce}">
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: It seems it'd be better to include <!DOCTYPE html> at least, will do.

Copy link
Contributor

Choose a reason for hiding this comment

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

The w3c validator makes it seem like we should include <title>foo</title> as well: https://validator.w3.org/nu/#textarea

Borrowing from https://mathiasbynens.be/notes/minimal-html who found whatwg/html@5c7cf94, the title can be omitted when dealing with e-mails, but it sounds like we should be using it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, would never thought we may need title :) Let me read through this, thanks for sharing!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the spec agrees with you: https://html.spec.whatwg.org/multipage/semantics.html#the-title-element: Neither tag is omissible. 🙂

window.location.replace(
'${basePath}/api/security/v1/oidc?authenticationResponseURI=' + encodeURIComponent(window.location.href)
);
</script>
`)
.header('cache-control', 'private, no-cache, no-store')
.header('content-security-policy', cspRulesHeader)
.type('text/html');
}
});

server.route({
// POST is only allowed for Third Party initiated authentication
method: ['GET', 'POST'],
Expand All @@ -103,25 +134,48 @@ export function initAuthenticateApi({ authc: { login, logout }, config }, server
},
async handler(request, h) {
try {
const query = request.query || {};
const payload = request.payload || {};
Copy link
Contributor

Choose a reason for hiding this comment

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

It predates this PR, but should we considering add payload validation to this route?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know to be honest, I was even planning to eventually get rid of query validation as well since we (Kibana) neither parse nor try to interpret them and just blindly pass to Elasticsearch anyway (and we allow unknown parameters as well). If it happens so that these args are malformed, imo, it's better to allow ES to parse them and log more detailed error in their log than having this validation in Kibana.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think?

We likely get little benefit from doing it ourselves because we end up passing it directly to Elasticsearch. It could potentially help us if we received the wrong "data-type" for a specific parameter. So if we received a payload.iss which was an Object as opposed to a string, we could potentially have more informative error messages instead of something breaking in a somewhat obscure manner downstream.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with it either way, I defer to your opinion :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just tried to add a payload validation and was hit by this limitation 😢 That means we can't have payload validation if route defines multiple methods and some of them don't support payload. The only alternative is to split this route into two very similar ones. Since you're fine with the current implementation I'll keep it simple and leave it as is for now. Let's get back to it when we migrate this route to NP (I leave a code comment).

I'll add a validation for authenticationResponseURI query parameter as you suggested below though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!!


// An HTTP GET request with a query parameter named `authenticationResponseURI` that includes URL fragment OpenID
// Connect Provider sent during implicit authentication flow to the Kibana own proxy page that extracted that URL
// fragment and put it into `authenticationResponseURI` query string parameter for this endpoint. See more details
// at https://openid.net/specs/openid-connect-core-1_0.html#ImplicitFlowAuth
let loginAttempt;
if (query.authenticationResponseURI) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add authenticationResponseURI to the route's query validation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Replied above, but let me know if you think it makes sense to treat our own authenticationResponseURI differently.

loginAttempt = {
flow: OIDCAuthenticationFlow.Implicit,
authenticationResponseURI: query.authenticationResponseURI,
};
} else if (query.code || query.error) {
// An HTTP GET request with a query parameter named `code` (or `error`) as the response to a successful (or
// failed) authentication from an OpenID Connect Provider during authorization code authentication flow.
// See more details at https://openid.net/specs/openid-connect-core-1_0.html#CodeFlowAuth.
loginAttempt = {
flow: OIDCAuthenticationFlow.AuthorizationCode,
// We pass the path only as we can't be sure of the full URL and Elasticsearch doesn't need it anyway.
authenticationResponseURI: request.url.path,
};
} else if (query.iss || payload.iss) {
// An HTTP GET request with a query parameter named `iss` or an HTTP POST request with the same parameter in the
// payload as part of a 3rd party initiated authentication. See more details at
// https://openid.net/specs/openid-connect-core-1_0.html#ThirdPartyInitiatedLogin
loginAttempt = {
flow: OIDCAuthenticationFlow.InitiatedBy3rdParty,
iss: query.iss || payload.iss,
loginHint: query.login_hint || payload.login_hint,
};
}

if (!loginAttempt) {
throw Boom.badRequest('Unrecognized login attempt.');
}

// We handle the fact that the user might get redirected to Kibana while already having an session
// Return an error notifying the user they are already logged in.
const authenticationResult = await login(KibanaRequest.from(request), {
provider: 'oidc',
// Checks if the request object represents an HTTP request regarding authentication with OpenID Connect.
// This can be
// - An HTTP GET request with a query parameter named `iss` as part of a 3rd party initiated authentication
// - An HTTP POST request with a parameter named `iss` as part of a 3rd party initiated authentication
// - An HTTP GET request with a query parameter named `code` as the response to a successful authentication from
// an OpenID Connect Provider
// - An HTTP GET request with a query parameter named `error` as the response to a failed authentication from
// an OpenID Connect Provider
value: {
code: request.query && request.query.code,
iss: (request.query && request.query.iss) || (request.payload && request.payload.iss),
loginHint:
(request.query && request.query.login_hint) ||
(request.payload && request.payload.login_hint),
},
value: loginAttempt
});
if (authenticationResult.succeeded()) {
return Boom.forbidden(
Expand Down
2 changes: 2 additions & 0 deletions x-pack/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
"@types/jest": "^24.0.9",
"@types/joi": "^13.4.2",
"@types/js-yaml": "^3.11.1",
"@types/jsdom": "^12.2.4",
"@types/json-stable-stringify": "^1.0.32",
"@types/jsonwebtoken": "^7.2.7",
"@types/lodash": "^3.10.1",
Expand Down Expand Up @@ -109,6 +110,7 @@
"babel-plugin-require-context-hook": "npm:babel-plugin-require-context-hook-babel7@1.0.0",
"babel-plugin-transform-react-remove-prop-types": "0.4.24",
"base64-js": "^1.2.1",
"base64url": "^3.0.1",
"chalk": "^2.4.1",
"chance": "1.0.18",
"checksum": "0.1.1",
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/security/server/authentication/authenticator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,8 @@ export class Authenticator {

const sessionStorage = this.options.sessionStorageFactory.asScoped(request);

// If we detect an existing session that belongs to a different provider than the one request to
// perform a login we should clear such session.
// If we detect an existing session that belongs to a different provider than the one requested
// to perform a login we should clear such session.
let existingSession = await this.getSessionValue(sessionStorage);
if (existingSession && existingSession.provider !== attempt.provider) {
this.logger.debug(
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/security/server/authentication/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export { canRedirectRequest } from './can_redirect_request';
export { Authenticator, ProviderLoginAttempt } from './authenticator';
export { AuthenticationResult } from './authentication_result';
export { DeauthenticationResult } from './deauthentication_result';
export { BasicCredentials } from './providers';
export { BasicCredentials, OIDCAuthenticationFlow } from './providers';

interface SetupAuthenticationParams {
core: CoreSetup;
Expand Down Expand Up @@ -116,7 +116,7 @@ export async function setupAuthentication({
const authResponseHeaders = authenticationResult.authResponseHeaders;
for (const [headerName, headerValue] of Object.entries(authResponseHeaders || {})) {
if (error.output.headers[headerName] !== undefined) {
authLogger.warn(`Server rewrites a error response header [${headerName}].`);
authLogger.warn(`Server rewrites an error response header [${headerName}].`);
}
// Hapi typings don't support headers that are `string[]`.
error.output.headers[headerName] = headerValue as any;
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/security/server/authentication/providers/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,4 @@ export { BasicAuthenticationProvider, BasicCredentials } from './basic';
export { KerberosAuthenticationProvider } from './kerberos';
export { SAMLAuthenticationProvider, isSAMLRequestQuery } from './saml';
export { TokenAuthenticationProvider } from './token';
export { OIDCAuthenticationProvider } from './oidc';
export { OIDCAuthenticationProvider, OIDCAuthenticationFlow } from './oidc';
Loading