Preserve URL fragment during SAML handshake. by azasypkin · Pull Request #44513 · elastic/kibana

azasypkin · 2019-08-30T16:12:08Z

Preserve URL fragment during SAML handshake (based on the plan outlined in #18392 (comment), client-side page to extract fragment + RelayState).

Current SAML handshake flow

Unauthenticated user navigates to https://kibana/app/kibana#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())...
Kibana stores /app/kibana in the cookie and redirects user to IdP
User authenticates at IdP and is redirected back to Kibana via HTTP POST (that's where we lose #/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())... part of the initial URL)
Kibana extracts /app/kibana from the cookie and redirects user to this URL

Proposed SAML handshake flow

Unauthenticated user navigates to https://kibana/app/kibana#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())...
Kibana extracts /app/kibana, puts it into intermediate cookie and redirects user to /api/security/saml/capture-url-fragment to capture URL fragment part
At the /capture-url-fragment page Kibana extracts #/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())... part and redirects user to /api/security/saml/start appending combined and encoded #/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())... as redirectURLFragment query string parameter
Kibana combines previously saved /app/kibana with #/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())..., puts it into cookie together with SAML request ID and redirects user to IdP
User authenticates at IdP and is redirected back to Kibana via HTTP POST to /api/security/saml/callback
Kibana extracts /app/kibana/#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())... from the cookie and redirects user to this URL

Note 1: if the path extracted at the step 2 is larger than the value specified in xpack.security.authc.saml.maxRedirectURLSize (2kb by default) then URL path isn't put into the cookie and user redirected directly to IdP. After successful authentication user will be redirected to the Kibana root/home page.

Note 2: if URL fragment combined with the URL path at the step 4 is larger than the value specified in xpack.security.authc.saml.maxRedirectURLSize (2kb by default) then URL fragment is dropped and only URL path is stored in the cookie. After successful authentication user will be redirected to the Kibana path used to initiate SAML handshake (URL fragment isn't recovered).

Proposed SAML handshake flow with `RelayState` (ABANDONED)

Unauthenticated user navigates to https://kibana/app/kibana#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())...
Kibana extracts /app/kibana and redirects user to https://kibana/api/security/authc/saml/capture-url-fragment EITHER appending encoded /app/kibana as redirectURLPath query string parameter OR putting redirectURLPath into the intermediate cookie
At the /capture-url-fragment page Kibana extracts #/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())... part, EITHER combines it with the redirectURLPath query string parameter OR not (if we chose to put it into cookie) and redirects user to /api/security/authc/saml/start appending combined and encoded /app/kibana/#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())... as redirectURLFragment query string parameter
Kibana or Elasticsearch appends /app/kibana/#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())... to IdP redirect URL as RelayState query string parameter and redirects user to IdP
User authenticates at IdP and is redirected back to Kibana via HTTP POST with theRelayState body parameter Kibana sent to IdP at the previous step
Kibana extracts /app/kibana/#/visualize/edit/9c6f83f0-bb4d-11e8-9c84-77068524bcab?_g=(filters:!())... from the RelayState body parameter, validates that URL is relative to Kibana root and redirects user to this URL

Note: with the proposed approach IdP initiated login can also be accompanied by a custom RelayState argument that will redirect user to any Kibana URL after login.

Notes to myself:

Test in IE11 (on 09-10-2019)
Decide if benefits of having URL in RelayState in clear text over-weight possible risks, see 6.46 of Security and Privacy Considerations forthe OASIS SAML V2.0. Should we encrypt it? Or just store a hash of it in the cookie together with requestId? That would disallow "targeted" IdP initiated logins. (we'll tackle this separately, for now we'll store URL in encrypted cookie)

~~Blocked by: elastic/elasticsearch#46232, #44855~~
Fixes: #18392

"Release Note: Kibana now fully preserves the URL user used to login with SAML."

elasticmachine · 2019-08-30T16:12:10Z

Pinging @elastic/kibana-security

azasypkin · 2019-09-02T09:32:23Z

docs/security/authentication/index.asciidoc

 [source,yaml]
 --------------------------------------------------------------------------------
-server.xsrf.whitelist: [/api/security/v1/saml]
+server.xsrf.whitelist: [/security/api/authc/saml/callback]


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....

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.

azasypkin · 2019-09-02T09:33:02Z

x-pack/legacy/plugins/security/index.js

      isSystemAPIRequest: server.plugins.kibana.systemApi.isSystemApiRequest.bind(
        server.plugins.kibana.systemApi
      ),
+      cspRules: createCSPRuleString(config.get('csp.rules')),


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

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.

azasypkin · 2019-09-02T09:49:39Z

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

 import { createCSPRuleString } from '../../../../../../../../src/legacy/server/csp';

 export function initAuthenticateApi({ authc: { login, logout }, config }, server) {
+  const xsrfWhitelist = server.config().get('server.xsrf.whitelist');


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)

azasypkin · 2019-09-02T10:16:02Z

x-pack/plugins/security/server/authentication/providers/saml.ts

+      // 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)}`;


note: there was the idea to somehow encrypt RelayState so that it can't be altered, but I doubt it's a big a problem if we don't. Also keeping RelayState as a plain path#fragment would allow administrators to specify any URL for IdP initiated login, e.g. like described in Auth0 docs.

azasypkin · 2019-09-02T11:31:41Z

x-pack/plugins/security/server/authentication/providers/saml.ts

-        stateRedirectURL || `${this.options.basePath.get(request)}/`,
-        { state: { accessToken, refreshToken } }
-      );
+      return AuthenticationResult.redirectTo(nextURL || `${this.options.basePath.get(request)}/`, {


note: we should probably validate if it's a relative path at least. What do you think?

azasypkin · 2019-09-02T11:51:15Z

@kobelb PR is ready for the preliminary feedback whenever you have time, thanks!

alexbrasetvik · 2019-09-03T21:42:02Z

x-pack/plugins/security/server/routes/authentication.ts

+          window.location.replace(
+            '${basePath.get(
+              request
+            )}/security/api/authc/saml/start?currentURL=' + encodeURIComponent('${currentPath}' + window.location.hash)


currentPath looks like it can be any user-controlled string, which would make this an XSS vector?

(e.g. provide a currentPath along the lines of ')); alert('hi!')//

currentPath looks like it can be any user-controlled string, which would make this an XSS vector?

Yep, it's exactly what you see here, glaring XSS vulnerable code (with a bit tweaked payload) 🙂 We're just not sure where to place this currentPath yet, initially we stored it in the intermediate encrypted cookie so that we don't need to do a "is it a relative path" check and care about proper escaping. If we still decide to keep it here I'll take care of properly handling this including the "is it a relative path" check.

Thank you for being on guard!

alexbrasetvik · 2019-09-03T21:51:39Z

x-pack/plugins/security/server/routes/authentication.ts

+      try {
+        const authenticationResult = await authc.login(request, {
+          provider: 'saml',
+          value: { step: SAMLLoginStep.UserURLCaptured, currentURL: request.query.currentURL },


What would catch currentURL not being relative, but e.g. starting with https://attacker.com/...?

Mentioned it in #44513 (comment), but will have a check before we forward user to Kibaba after a successful authentication assuming we keep the current approach.

…cookie to store `redirectURLPath`.

azasypkin · 2019-09-05T18:15:14Z

docs/security/authentication/index.asciidoc

 [source,yaml]
 --------------------------------------------------------------------------------
-server.xsrf.whitelist: [/api/security/v1/saml]
+server.xsrf.whitelist: [/api/security/authc/saml/callback]


note: previous comment was vanished with new commit. But essential I wasn't sure whether we need authc part here. The more I think about it the more it seems unnecessary....

I think I'd prefer we leave the authc part out. I don't know if it gets us much, and not including it more closely mirrors the ES security routes.

azasypkin · 2019-09-05T18:24:41Z

x-pack/plugins/security/server/routes/authentication.ts

+          `
+          <!DOCTYPE html>
+          <title>Kibana SAML Login</title>
+          <link rel="icon" href="data:,">


note: alternatively we can makefavicon.ico or/and anything that ends with .ext as special case in canRedirectRequest....

This becomes a problem only if we store redirectPath in the cookie when we render this custom HTML favicon.ico request may cause us to create a new cookie :/

For static resources like the favicon.ico, we don't require auth. It looks like we're using the equivalent of http://localhost:5601/ui/favicons/favicon.ico normally, should we be doing this here instead?

For static resources like the favicon.ico, we don't require auth.

We don't, but we also run auth code before we figure out that the request is targeting non-existent resource (as it's done by browser automatically if we don't define favicon, somehost/favicon.ico) and for security this request looks like any other non-resource request. But now I started wondering if it's expected that we call auth hook for the 404 route, checking with Platform team....

It looks like we're using the equivalent of http://localhost:5601/ui/favicons/favicon.ico normally, should we be doing this here instead?

I considered this option, but this ui/favicons path is maintained in another place and if someday they change the path this code will silently be broken (unless we try to simulate SAML IdP and write a functional test for that). Ideally this should be solved with #41964 and until then we can either use current workaround or switch to ui/favicons/favicon.ico and hope that it won't change before #41964. Both options sound good to me, what would you prefer?

Talked to @restrry and there is a chance we'll treat it (i.e. that we call auth hook for non-defined routes) as a bug, will link issue later.

but this ui/favicons path is maintained in another place and if someday they change the path this code will silently be broken (unless we try to simulate SAML IdP and write a functional test for that)

That makes sense.

Talked to @restrry and there is a chance we'll treat it (i.e. that we call auth hook for non-defined routes) as a bug, will link issue later.

I don't mind us merging with the current work-around also.

I don't mind us merging with the current work-around also.

We still have time, but good to know! :)

azasypkin · 2019-09-06T05:43:21Z

x-pack/plugins/security/server/routes/authentication.ts

+            value: {
+              step: SAMLLoginStep.SAMLResponseReceived,
+              samlResponse: request.body.SAMLResponse,
+              redirectURL: request.body.RelayState,


note: we may validate that URL is relative here assuming we keep transmitting it as it's

kobelb

This is looking good! Outside of what was discussed over Slack this morning about no longer relying upon RelayState to store the full because of limitations on the max-size, I just have small nit-picks.

I don't see any issues with existing SAML logins when rolling out this new approach. Would you mind confirming this?

kobelb · 2019-09-11T18:01:41Z

docs/security/authentication/index.asciidoc

 [source,yaml]
 --------------------------------------------------------------------------------
-server.xsrf.whitelist: [/api/security/v1/saml]
+server.xsrf.whitelist: [/api/security/authc/saml/callback]


I think I'd prefer we leave the authc part out. I don't know if it gets us much, and not including it more closely mirrors the ES security routes.

kobelb · 2019-09-11T20:58:20Z

x-pack/plugins/security/server/routes/authentication.ts

+          `
+          <!DOCTYPE html>
+          <title>Kibana SAML Login</title>
+          <link rel="icon" href="data:,">


For static resources like the favicon.ico, we don't require auth. It looks like we're using the equivalent of http://localhost:5601/ui/favicons/favicon.ico normally, should we be doing this here instead?

kobelb · 2019-09-11T21:34:24Z

x-pack/plugins/security/server/routes/authentication.ts

+          <!DOCTYPE html>
+          <title>Kibana SAML Login</title>
+          <link rel="icon" href="data:,">
+          <script src="${currentBasePath}/api/security/authc/saml/capture-url-fragment.js"></script>


This scares me from the XSS perspective. Currently, the spaces OnPreAuth handler isn't setting the basePath unless it matches this regex

kibana/x-pack/legacy/plugins/spaces/server/lib/spaces_url_parser.ts

Line 18 in c0dd0ca

const matchResult = pathToCheck.match(/^\/s\/([a-z0-9_\-]+)/);

but I don't think we want to necessarily rely on that. If this wasn't using a strict regex, there's potential an XSS could be introduced here with a URL like the following: http://localhost:5601/s/${encodeURIComponent('"/><script>alert("xss");')}

Perhaps we should use the following:

ReactDOMServer.renderToString(React.createElement('script', { src: `${currentBasePath}/api/security/authc/saml/capture-url-fragment.js` }))

or some other method which does context specific escaping?

Hmmm, hmmmmm, I'm concerned that we say that we can't trust core base path service to provide us with non-malformed base path.... Let's discuss offline.

Actually, related discussion made me think that we don't need to bother with something that user can influence here (space ID) and rely on plain server base path instead.

kobelb · 2019-09-11T22:09:11Z

x-pack/plugins/security/server/authentication/providers/saml.ts

+      this.logger.debug('Captured redirect URL.');
+      return this.authenticateViaHandshake(
+        request,
+        `${state.redirectURLPath}${attempt.redirectURLFragment}`


We're dropping the "basePath" here, so the space information is lost.

Wow, good catch! I should store URL with basepath (and space-id) into redirectURLPath at the previous step and be more spaces aware in general :)

kobelb · 2019-10-08T16:57:48Z

I can't think of any way of redirecting to a fully qualified third-party-domain as well, but I was initially thinking about implementing a check that would validate the redirectURLFragment URL query string parameter and make sure that it starts with # or we can just make sure # is presented when we concatenate redirectURL with redirectURLFragment and add it if it's not there.

I think that's a reasonable mitigation.

It's hard to imagine how you can exploit this to do something that's worse than already having malicious/phishing app hosted using the same domain name though.

I generally agree, and it's something we were willing to treat as an edge-case for the CSP changes to use script-src: 'self'; however, I think the minimal level of effort that is required makes the above mitigation beneficial.

docs/user/security/authentication/index.asciidoc

Co-Authored-By: gchaps <33642766+gchaps@users.noreply.github.com>

elasticmachine · 2019-10-09T12:16:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: dca6431

azasypkin · 2019-10-09T13:31:07Z

x-pack/plugins/security/server/authentication/providers/saml.test.ts

+          { body: { realm: 'test-realm' } }
+        );
+
+        expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1);


note: I know and agree in advance that testing things like this (logger.warn) feels redundant, but it doesn't hurt and I can't think of any occasion when I suffered from anything like this during whole auth provider code life.

Ensuring a warn is called seems reasonable here to me!

azasypkin · 2019-10-09T13:34:25Z

@gchaps, I incorporated all suggested changes, thanks. Would you mind taking a second look?

@kobelb, I added 2 additional commits since your last review (one to remove legacy serverBasePath and the other one to handle URL fragments that don't start with #). Does it still look good to you?

kobelb

LGTM, nice work!

kobelb · 2019-10-09T13:41:36Z

x-pack/plugins/security/server/authentication/providers/saml.test.ts

+          { body: { realm: 'test-realm' } }
+        );
+
+        expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1);


Ensuring a warn is called seems reasonable here to me!

elasticmachine · 2019-10-09T14:42:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8b51a1c

gchaps

Text LGTM

elasticmachine · 2019-10-09T17:28:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8b51a1c

azasypkin · 2019-10-10T12:37:39Z

7.x/7.5.0: da745fa

azasypkin added release_note:enhancement Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// enhancement New value added to drive a business result Feature:Security/Authentication Platform Security - Authentication v7.5.0 labels Aug 30, 2019

Preserve URL fragment during SAML handshake.

71f2980

azasypkin force-pushed the issue-18392-saml-hash branch from 845b508 to 71f2980 Compare September 2, 2019 09:30

azasypkin commented Sep 2, 2019

View reviewed changes

azasypkin added the blocked label Sep 2, 2019

azasypkin commented Sep 2, 2019

View reviewed changes

azasypkin added the feedback_needed label Sep 2, 2019

azasypkin mentioned this pull request Sep 2, 2019

SAML _security/saml/prepare API should allow consumer to specify RelayState elastic/elasticsearch#46232

Closed

azasypkin mentioned this pull request Sep 2, 2019

Use /security/login instead of /login? #44619

Closed

alexbrasetvik reviewed Sep 3, 2019

View reviewed changes

azasypkin added 5 commits September 4, 2019 16:02

Merge branch 'master' into issue-18392-saml-hash

c891059

Merge branch 'master' into issue-18392-saml-hash

a3e32fd

Merge branch 'master' into issue-18392-saml-hash

ca8942a

Merge branch 'master' into issue-18392-saml-hash

19e6fb3

Sync with upstream and use route URLs that are relative to root. Use …

1f7faff

…cookie to store `redirectURLPath`.

azasypkin commented Sep 5, 2019

View reviewed changes

azasypkin commented Sep 6, 2019

View reviewed changes

kobelb self-requested a review September 11, 2019 17:59

kobelb reviewed Sep 11, 2019

View reviewed changes

gchaps reviewed Oct 8, 2019

View reviewed changes

azasypkin and others added 2 commits October 9, 2019 13:08

Apply suggestions from code review

dca6431

Co-Authored-By: gchaps <33642766+gchaps@users.noreply.github.com>

Merge branch 'master' into issue-18392-saml-hash

0a86b77

Make sure redirect URL fragment always starts with #.

8b51a1c

azasypkin commented Oct 9, 2019

View reviewed changes

azasypkin requested review from gchaps and kobelb October 9, 2019 13:31

kobelb approved these changes Oct 9, 2019

View reviewed changes

gchaps approved these changes Oct 9, 2019

View reviewed changes

azasypkin merged commit 0210ce4 into elastic:master Oct 9, 2019

azasypkin deleted the issue-18392-saml-hash branch October 9, 2019 16:12

azasypkin added the backport pending label Oct 9, 2019

azasypkin mentioned this pull request Oct 9, 2019

[7.x] Preserve URL fragment during SAML handshake. (#44513) #47742

Merged

azasypkin added backported and removed backport pending feedback_needed labels Oct 10, 2019

azasypkin mentioned this pull request Oct 11, 2019

Drop /api/security/v1/saml route in favour of /api/security/saml/callback. #47929

Merged

1 task

kobelb mentioned this pull request Nov 13, 2019

[DOCS] 7.5.0 Release Notes #50425

Merged

1 task

kobelb mentioned this pull request Nov 21, 2019

Using the server's basePath when building the SAML ACS #51391

Merged

legrego mentioned this pull request Dec 18, 2019

Properly handle very long URL fragments captured during SAML handshake #53478

Closed

azasypkin mentioned this pull request Dec 19, 2019

Kibana server fails to handle long query string parameters #53464

Closed

ssanchezjoss mentioned this pull request Oct 16, 2020

Preserve URL fragment during SAML handshake. opensearch-project/security-dashboards-plugin#543

Closed

azasypkin mentioned this pull request Apr 8, 2021

[7.3] [DOCS] Adds authentication settings #96473

Merged

jerrywang-db mentioned this pull request Jun 19, 2023

URL fragment get lost in redirect opensearch-project/security-dashboards-plugin#1476

Closed

Conversation

azasypkin commented Aug 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current SAML handshake flow

Proposed SAML handshake flow

Uh oh!

elasticmachine commented Aug 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Sep 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Sep 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin commented Sep 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kobelb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

azasypkin commented Aug 30, 2019 •

edited

Loading

azasypkin Sep 2, 2019 •

edited

Loading

azasypkin Sep 2, 2019 •

edited

Loading

azasypkin Sep 12, 2019 •

edited

Loading

azasypkin Sep 13, 2019 •

edited

Loading