Add support for OpenID Connect implicit authentication flow. #42069

azasypkin · 2019-07-26T13:57:36Z

Kibana OIDC authentication provider currently supports Authentication using the Authorization Code Flow only. The main reason is that it is a recommended authentication flow and should be used by default whenever it's possible.

The authorization code flow implies that Elasticsearch has access to the Token endpoint of the OpenID Connect Provider (OP), but it's neither always possible nor desired. So in the scenarios like this having support for another, so called Implicit Flow would be beneficial.

This PR adds support for the OpenID Connect Authentication using the Implicit Flow. The tricky part here is that OP passes arguments required for the authentication using the implicit flow via URL fragment. That means we can't directly receive these arguments on the server side and have to provide a client-side proxy page that UA will be forwarded to and where we'll be extracting URL fragment and only then sending them to the Kibana Server OIDC endpoint to perform actual authentication.

~~Since this PR modifies OIDC authentication provider we should consider enabling OIDC integration tests first, that's blocked by #36959~~ (tests were re-enabled).

Notes to myself:

Test in IE11
Test with non-empty basepath
Include new oidc/implicit URL into ES Docs (Document support of OIDC Implicit flow in Kibana. elasticsearch#45693)
Include new oidc/implicit URL into Cloud Docs (only when this PR is merged/backported)

How to test

Run Elasticsearch with (ping me to get values for client_id and client_secret):

$ yarn es snapshot --license trial \
    -E xpack.security.authc.token.enabled=true \
    -E xpack.security.authc.realms.native.native1.order=0 \
    -E xpack.security.authc.realms.oidc.oidc1.order=1 \
    -E xpack.security.authc.realms.oidc.oidc1.rp.client_id=xxxx \
    -E xpack.security.authc.realms.oidc.oidc1.rp.client_secret=xxxx \
    -E xpack.security.authc.realms.oidc.oidc1.rp.response_type="id_token token" \
    -E xpack.security.authc.realms.oidc.oidc1.rp.requested_scopes=profile \
    -E xpack.security.authc.realms.oidc.oidc1.rp.redirect_uri=https://localhost:5601/api/security/v1/oidc/implicit \
    -E xpack.security.authc.realms.oidc.oidc1.op.authorization_endpoint=https://elastic-dev.auth0.com/authorize \
    -E xpack.security.authc.realms.oidc.oidc1.op.token_endpoint=https://elastic-dev.auth0.com/oauth/token \
    -E xpack.security.authc.realms.oidc.oidc1.op.userinfo_endpoint=https://elastic-dev.auth0.com/userinfo \
    -E xpack.security.authc.realms.oidc.oidc1.op.issuer=https://elastic-dev.auth0.com/ \
    -E xpack.security.authc.realms.oidc.oidc1.op.jwkset_path=https://elastic-dev.auth0.com/.well-known/jwks.json \
    -E xpack.security.authc.realms.oidc.oidc1.claims.principal=sub \
    -E xpack.security.authc.realms.oidc.oidc1.claims.groups=https://myapp.example.com/kbn_groups \
    -E xpack.security.authc.realms.oidc.oidc1.claims.name=name

Add role mapping for users authenticated with OIDC realm, e.g.:

POST http://localhost:9200/_security/role_mapping/oidc1
Content-Type: application/json

{
  "roles": [ "superuser" ],
  "enabled": true,
  "rules": { "field" : { "realm.name" : "oidc1" } }
}

Run Kibana with xpack.security.authc.providers: [oidc, basic]

Blocked by: #36959
Fixes: #41984

"Release Note: Adding support for the OpenID Connect Authentication using the Implicit Flow"

/cc @jkakavas

elasticmachine · 2019-07-26T13:57:39Z

Pinging @elastic/kibana-security

azasypkin · 2019-07-26T14:02:25Z

x-pack/legacy/plugins/security/public/views/oidc_implicit/oidc_implicit.tsx

+  .setRootController('oidc_implicit', ($scope: any, $window: any) => {
+    const urlFragment: string = window.location.hash;
+
+    // HACK: an ugly hack to overcome automatic Kibana URL fragment modifications.


note: that the main thing I need to get rid of (hopefully) before this PR is ready for review

Alternatively we could just render tiny HTML page outside of Kibana context with inline "nonced" onliner script.... That way we won't have rison that garbles URL fragment right now.

azasypkin · 2019-07-26T14:03:38Z

x-pack/legacy/plugins/security/public/views/oidc_implicit/oidc_implicit.tsx

+    const startKibanaStateIndex = urlFragment.indexOf('?_g=');
+    const endFragmentIndex = startKibanaStateIndex >= 0 ? startKibanaStateIndex : undefined;
+
+    $window.location.href = chrome.addBasePath(


note: I was thinking about using POST here, but couldn't convince myself enough since all these args are already came through URL fragment 🤷‍♂️ Would be glad to hear any thoughts on this.

azasypkin · 2019-07-26T14:05:04Z

x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js

          //  - 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,
+          value: request.query && request.query.redirectURI ? {


note: actually, it probably makes sense to have a dedicated endpoint for implicit flow, haven't thought thoroughly about this.

I'll defer to your opinion on this one. I don't think the current implementation is un-readable, but if you think it makes it easier to have a separate end-point, that seems reasonable also.

I don't think the current implementation is un-readable

Great, I don't have strong opinion on that. Let's keep it as is for now then and split whenever we feel we really need to split.

azasypkin · 2019-07-26T14:05:48Z

x-pack/legacy/plugins/xpack_main/public/services/path.js

@@ -6,9 +6,10 @@

 import chrome from 'ui/chrome';

+const paths = ['/login', '/logout', '/logged_out', '/status', '/oidc_implicit'];


note: we should find a better way of solving the underlying problem, but not in this PR likely.

azasypkin · 2019-07-30T13:36:01Z

x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js

+      const basePath = legacyConfig.get('server.basePath');
+
+      const nonce = await generateCSPNonce();
+      const cspRulesHeader = createCSPRuleString(legacyConfig.get('csp.rules'), nonce);


note: using CSP here feels like an overkill here, but it doesn't hurt to have it I think.

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.

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.

azasypkin · 2019-07-30T13:38:19Z

x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js

+      const cspRulesHeader = createCSPRuleString(legacyConfig.get('csp.rules'), nonce);
+      return h.response(`
+        <script nonce="${nonce}">
+          const redirectURI = window.location.pathname + window.location.search + window.location.hash;


note: we can just pass entire window.location.href instead, it will also work and make this code a tad easier. Now when I look at this I can't think of any reason to not use entire window.location.href 🙈 What do you think?

That seems easy enough to me, and potentially less fragile.

Yep, will switch to window.location.href.

azasypkin · 2019-07-30T14:01:21Z

@kobelb would you mind giving a preliminary feedback on the approach itself? Feel free to ping me to get user credentials and client secret for the test environment.

Please, skip changes in @kbn/es, @kbn/test and x-pack/test - these are from #42239 that is the blocker for the current PR.

azasypkin · 2019-07-30T14:47:37Z

x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js

+      const nonce = await generateCSPNonce();
+      const cspRulesHeader = createCSPRuleString(legacyConfig.get('csp.rules'), nonce);
+      return h.response(`
+        <script nonce="${nonce}">


note: It seems it'd be better to include <!DOCTYPE html> at least, will do.

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.

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

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

kobelb

I think the approach is fine. Perhaps long-term we can add the ability to render custom HTML content without all of the other "cruft" in the core application, but what we have now seems reasonable.

kobelb · 2019-07-30T19:40:24Z

x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js

          //  - 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,
+          value: request.query && request.query.redirectURI ? {


I'll defer to your opinion on this one. I don't think the current implementation is un-readable, but if you think it makes it easier to have a separate end-point, that seems reasonable also.

azasypkin · 2019-07-30T20:11:55Z

Perhaps long-term we can add the ability to render custom HTML content without all of the other "cruft" in the core application

++, that's what we'll need to have for sure - custom HTML with only the absolutely necessary stuff (CSP, xsrf, HSTS, CORS etc. etc.). Not a blocker for now, but at some point managing this manually and outside of core's control in every such case will hurt us.

elasticmachine · 2019-08-01T20:46:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2019-08-02T12:03:41Z

@kobelb this PR should be ready for review whenever you have time 🙂 Thanks!

jbudz

renovate portion LGTM

kobelb · 2019-08-06T21:34:11Z

x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js

@@ -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 || {};


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

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?

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.

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

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.

Sounds good!!

kobelb · 2019-08-06T21:45:30Z

x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js

+
+      const nonce = await generateCSPNonce();
+      const cspRulesHeader = createCSPRuleString(legacyConfig.get('csp.rules'), nonce);
+      return h.response(`


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.

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

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

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

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

Thanks @jkakavas

kobelb · 2019-08-06T21:52:19Z

x-pack/legacy/plugins/security/server/routes/api/v1/authenticate.js

+        // 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) {


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

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

…ng parameter.

elasticmachine · 2019-08-08T09:33:18Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-08T10:49:47Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-08T12:58:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2019-08-08T16:03:29Z

7.x/7.4.0: 4c396f8

elasticmachine · 2019-09-06T06:30:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin added blocked Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Security/Authentication Platform Security - Authentication Feature:New Feature New feature not correlating to an existing feature label v7.5.0 labels Jul 26, 2019

azasypkin added the release_note:enhancement label Jul 26, 2019

azasypkin force-pushed the issue-xxx-oidc-implicit-flow branch from e30d427 to 653d4e8 Compare July 26, 2019 14:00

azasypkin commented Jul 26, 2019

View reviewed changes

azasypkin force-pushed the issue-xxx-oidc-implicit-flow branch from 653d4e8 to f82980a Compare July 30, 2019 13:33

azasypkin commented Jul 30, 2019

View reviewed changes

azasypkin force-pushed the issue-xxx-oidc-implicit-flow branch from f82980a to a47f882 Compare July 30, 2019 13:50

azasypkin added the feedback_needed label Jul 30, 2019

azasypkin commented Jul 30, 2019

View reviewed changes

kobelb self-requested a review July 30, 2019 18:46

kobelb reviewed Jul 30, 2019

View reviewed changes

azasypkin removed blocked feedback_needed labels Aug 1, 2019

azasypkin force-pushed the issue-xxx-oidc-implicit-flow branch from a47f882 to dcfabee Compare August 1, 2019 15:25

azasypkin marked this pull request as ready for review August 1, 2019 15:25

azasypkin requested a review from a team as a code owner August 1, 2019 15:25

This comment has been minimized.

Sign in to view

azasypkin force-pushed the issue-xxx-oidc-implicit-flow branch from dcfabee to 776cda2 Compare August 1, 2019 15:44

azasypkin requested a review from a team as a code owner August 1, 2019 15:44

azasypkin added v7.4.0 and removed v7.5.0 labels Aug 1, 2019

azasypkin requested a review from kobelb August 2, 2019 12:02

jbudz approved these changes Aug 2, 2019

View reviewed changes

Merge branch 'master' into issue-xxx-oidc-implicit-flow

1bf2b13

azasypkin mentioned this pull request Aug 5, 2019

Introduce PKI authentication provider. #42606

Merged

7 tasks

Merge branch 'master' into issue-xxx-oidc-implicit-flow

a2932f0

kobelb reviewed Aug 6, 2019

View reviewed changes

kobelb approved these changes Aug 7, 2019

View reviewed changes

azasypkin added 2 commits August 8, 2019 09:34

Merge branch 'master' into issue-xxx-oidc-implicit-flow

99a593d

Review#1: add validation for the authenticationResponseURI query stri…

d73fd18

…ng parameter.

Integrate upstream API integration tests changes.

293de31

Merge branch 'master' into issue-xxx-oidc-implicit-flow

f325744

azasypkin merged commit 0d31f52 into elastic:master Aug 8, 2019

azasypkin deleted the issue-xxx-oidc-implicit-flow branch August 8, 2019 13:39

azasypkin added the backport pending label Aug 8, 2019

This was referenced Aug 8, 2019

[7.x] Add support for OpenID Connect implicit authentication flow. (#42069) #42938

Merged

OpenID Connect Implicit Flow #41984

Closed

azasypkin added backported and removed backport pending labels Aug 8, 2019

jkakavas mentioned this pull request Aug 19, 2019

Handle 3rd party initiated login in OIDC in a different endpoint #43258

Closed

kobelb mentioned this pull request Sep 4, 2019

CSP breaks OIDC authentication flow #44668

Closed

mshustov mentioned this pull request Sep 5, 2019

[New platform] Security plugin integration #33775

Closed

34 tasks

		@@ -6,9 +6,10 @@

		import chrome from 'ui/chrome';

		const paths = ['/login', '/logout', '/logged_out', '/status', '/oidc_implicit'];

Add support for OpenID Connect implicit authentication flow. #42069

Add support for OpenID Connect implicit authentication flow. #42069

Conversation

azasypkin commented Jul 26, 2019 • edited Loading

How to test

elasticmachine commented Jul 26, 2019

Choose a reason for hiding this comment

azasypkin Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobelb Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin commented Jul 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobelb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin commented Jul 30, 2019

This comment has been minimized.

elasticmachine commented Aug 1, 2019

💚 Build Succeeded

azasypkin commented Aug 2, 2019

jbudz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 8, 2019

💔 Build Failed

elasticmachine commented Aug 8, 2019

💚 Build Succeeded

elasticmachine commented Aug 8, 2019

💚 Build Succeeded

azasypkin commented Aug 8, 2019

elasticmachine commented Sep 6, 2019

💚 Build Succeeded

azasypkin commented Jul 26, 2019 •

edited

Loading

azasypkin Jul 29, 2019 •

edited

Loading

kobelb Jul 30, 2019 •

edited

Loading