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

SAML redirect and Kibana's hash based routing #18392

Closed
elasticmachine opened this issue Apr 10, 2018 · 13 comments · Fixed by #44513
Closed

SAML redirect and Kibana's hash based routing #18392

elasticmachine opened this issue Apr 10, 2018 · 13 comments · Fixed by #44513
Assignees
Labels
enhancement New value added to drive a business result Feature:Security/Authentication Platform Security - Authentication Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects

Comments

@elasticmachine
Copy link
Contributor

Original comment by @kobelb:

After logging in via SAML, @toddferg reported that one of his customers was brought to the default application within Kibana and we aren't preserving the original URL that the user tried to visit before being prompted to login.

@azasypkin is this a limitation that we're stuck with until we switch Kibana's routing to no longer be hash-based, or is there a potential solution?

@elasticmachine
Copy link
Contributor Author

Original comment by @toddferg:

Just attempted duplicating the problem. I was able to duplicate it on 6.2.3.
link I used: LINK REDACTED)%2Ctime%3A(from%3Anow-7d%2Cmode%3Aquick%2Cto%3Anow)) from my server

Here's a quick gif.
redirectissue

If I am already authenticated to Kibana/okta I am transferred in just fine.

@elasticmachine
Copy link
Contributor Author

elasticmachine commented Apr 11, 2018

Original comment by @azasypkin:

After logging in via SAML, @toddferg reported that one of his customers was brought to the default application within Kibana and we aren't preserving the original URL that the user tried to visit before being prompted to login.

@azasypkin is this a limitation that we're stuck with until we switch Kibana's routing to no longer be hash-based, or is there a potential solution?

User should be brought not to default application, but to the application that initiated SAML handshake (/app/kibana, /app/ml etc.) as it's part of the URL path that's preserved during SAML handshake.

URL fragment is a different story as server doesn't have access to it, so HTTP/1.1 7.1.2 instructs browsers to preserve URL fragment if it's not overridden in the 30x Location header. That's why URL fragments are preserved for authenticated users: in this case SAML handshake is just a set of redirects and browser takes care of URL fragment without our involvement.

For unathenticated users (without active IdP session) URL fragment is lost at the IdP login page as it uses FORM POST to redirect user back to Kibana and obviously doesn't preserve URL fragment. Some old custom IdPs used to extract URL fragments with the help of JavaScript and used it in login form action field, but I suspect nowadays nobody does that.

The best solution is to get rid of fragment-based URLs in Kibana. It's a huge breaking change that we should do eventually though.

But for now I can only think of custom "mediator" client side page:

  1. Unauthenticated user opens Kibana with URL that includes fragment/hash
  2. Kibana server detects that user should be authenticated with SAML and redirects user to another Kibana "saml_handshake" client side page
  3. "saml_handshake" page extracts URL fragment (and original path) and passes this data to Kibana's "saml" endpoint. Alternatively "saml_handshake" could save fragment in sessionStorage and recover once SAML handshake is completed, but I don't like it :)
  4. "saml" endpoint embeds fragment (or entire path#fragment) into RelayState (we can't use cookie for that anymore since with huge fragment we can easily exceed max cookie size)
  5. SAML IdP must respond with RelayState we previously sent to it so we can now restore full URL.

Note: SAML redirect URL is constructed by ES so it would be reasonable for ES to include RelayState for us as well, we just need to pass its content to /_xpack/security/saml/prepare and get it back from /_xpack/security/saml/authenticate.

@elasticmachine elasticmachine added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! enhancement New value added to drive a business result labels Apr 25, 2018
@azasypkin azasypkin added the Feature:Security/Authentication Platform Security - Authentication label May 9, 2019
@elasticmachine
Copy link
Contributor Author

Pinging @elastic/kibana-security

@kobelb kobelb added this to Backlog in Security Jun 19, 2019
@kobelb kobelb moved this from Backlog to Analysis in Security Aug 2, 2019
@azasypkin azasypkin self-assigned this Aug 20, 2019
@azasypkin
Copy link
Member

Re-evaluating what I wrote in #18392 (comment) and other possible options....

@azasypkin
Copy link
Member

azasypkin commented Aug 20, 2019

I gave this problem some thought today and I think it's better to split the solution for it into two parts:

1. capturing full URL to redirect user to after successful SAML handshake
2. and storing it somewhere.

Capturing URL

Currently when unauthenticated user navigates to Kibana and we want to start a SAML handshake we grab current URL path, store it in the cookie and redirect user to the IdP. The problem is that fragment (#) part of the URL is never passed to the server and I see two way to solve this:

1. Get rid of fragment-based URLs in Kibana and switch to "normal" URLs (#6219)

Pros:

  • That's is something we want to do eventually anyway
  • There is no need to change anything in security plugin, the issue will be automagically solved

Cons:

  • It's a huge undertaking and would be a breaking change for Kibana

2. Introduce a client side "SAML Step 1" page that unauthenticated user will be redirected to as an initial step of the SAML handshake. At this page we'll extract URL path and fragment, URL-encode it and send as a query string parameter to a "SAML Step 2" endpoint that will store this full URL and only then redirect user to the IdP.

Pros:

  • It's not a breaking change, we can implement this in any minor version
  • Not complex to implement

Cons:

  • Two additional redirects during SAML handshake
  • The SAML flow will become even more complex (from maintenance point of view)

Storing and retrieving URL

Currently we store URL path in the cookie so that we can easily retrieve it once user is redirected back to Kibana with SAML response. It works reasonably well since the URL path is usually short (e.g. /app/kibana, /app/ml etc.). But as soon as we want to store path + URL fragment cookie size may increase significantly since Kibana URL fragment stores state, filters and what not. Moreover cookie content is encrypted that increases its size even more. So there is a risk that at some point we may hit the recommended 4 KB limit and it won't be possible to decrypt cookie anymore. I see 3 possible solutions for this problem:

1. Store URL + fragment in the cookie, cross the fingers and hope for the best. We can approximately estimate the size of the encrypted cookie and fall back to the old behavior if its size is near the limit.

Pros:

  • Easy to implement

Cons:

  • May not work in all cases that will lead to a user confusion and support cases that are hard to debug
  • Some proxies/intermediate parties may have a different idea on what acceptable cookie size is...

2. Store URL + fragment in the RelayState query string parameter that is defined by SAML spec and is sent to IdP together with SAML request. It should be returned unmodified with SAML response and we can extract the URL + fragment from there.

Pros:

  • Perfectly valid use case for RelayState from SAML point of view
  • Easy to implement (on Kibana side, see below)

Cons:

  • Since Elasticsearch is responsible for constructing redirect URLs with SAML request it will need to deal with RelayState as well. That means Elasticsearch /_security/saml/prepare API will have to add support for an additional argument for the relay state that Kinaba will use. And it's not implemented yet.
  • The RelayState query string parameter is obviously not a part of the SAML request/response. That means it can potentially be tampered with anything. Maybe it's not a huge issue in this case, but we'll still need to consider that (thanks to @jkakavas for pointing this out). We can probably encrypt it on the Kibana side, but Easy to implement point may not hold though.
  • There is no 100% guarantee that all IdP implementations support RelayState even though they should theoretically

3. Store URL + fragment in a Saved Object (short URL?) that will be deleted as soon as we get SAML response back. But for not finished login attempts (user started SAML handshake, but then changed their mind) we have to somehow automatically clean up these Saved Objects. One idea was to leverage Task Manager for that with something like please call this function that removes short URL saved object in 30 mins unless we remove this earlier manually and cancel that task.

Pros:

  • We never expose final URL to IdP or any other intermediaries
  • We don't have a dependency on Elasticsearch and IdPs to implement RelayState support
  • Seems to work for all cases

Cons:

  • Relatively complex to implement properly especially if we end up using something like Task Manager. Task Manager can potentially be disabled as well.

Summary: if there is a real need to fix this issue now, I'd probably go with option 2 in Capturing URL and option 3 (or option 2 at least) in Storing and retrieving URL part with any further refinements we may need.

I may be missing some other options/pros/cons/complexities, please speak up if you have any ideas!

What do you think @kobelb @arisonl ?

@kobelb
Copy link
Contributor

kobelb commented Aug 21, 2019

With regard to Capturing the URL, option 2 seems reasonable given the time-frame that it'll take for us to move away from hash based routing...

With regard to Storing the URL, is there potential to capture the full URL in the browser's session storage since we'll already have a "page" to do so if we go with option 2 and then use that when the user is redirected back to Kibana?

@azasypkin
Copy link
Member

With regard to Storing the URL, is there potential to capture the full URL in the browser's session storage since we'll already have a "page" to do so if we go with option 2 and then use that when the user is redirected back to Kibana?

Well, I was considering sessionStorage in #18392 (comment), but ruled it out eventually for the same reasons I didn't include it into my short list this time. But I don't feel strongly about that, let me dump my thoughts and you'll tell me what you think.

The reasons I had in mind are:

  • Additional redirect (will be the 3rd additional redirect already). See explanation below.
  • Adding the 3rd type of storage into the mix that we'll spread "authentication data" over (in addition to cookie and ES). We can argue that SO is a 3rd type of storage as well though.
  • Wasn't sure we want to put redirect URL to a location that can be modified by user. That means we'll need to make sure we use this URL as a relative only (should be easy, silly attack scenario: I log out of Kibana and redirected to IdP to do a SLO, then Idp redirects me back to Kibana and hence we start new SAML handshake and store some URL in the sessionStorage. I forget to lock my PC and go for a lunch (cause I'm stupid, well that's a much larger vulnerability already), malicious colleague opens /login in a separate tab and hence has access to original key in sessionStorage and can modify it. I return from lunch, enter my credentials at already opened IdP login page and being redirected to ....somewhere else....)

Based on the 4.1.2.5 5.Identity Provider issues to Service Provider IdP is supposed to return <Response> using HTTP POST binding (or HTTP Artifact, but ES/Kibana doesn't support it), HTTP Redirect binding must not be used. That means we'll have to add one more redirect to extract data from sessionStorage: IdP sends <Response> with HTTP POST to existing /api/security/v1/saml, then we redirect user to "SAML Step 3" (I'm not sure we can re-use page from "SAML Step 1") where we extract URL from sessionStorage and finally redirect user to the page that initiated SAML handshake.

In addition to that we'll need to change SAML authentication provider a bit so that it doesn't redirect to "SAML Step 3" if it's IdP initiated login, not a big deal though.

@kobelb
Copy link
Contributor

kobelb commented Aug 22, 2019

@azasypkin thanks for the additional explanation!

I'm not as concerned about someone maliciously being able to modify the session storage, because if an attacker has that level of control to your computer there's a larger issue. It'd be similar to someone changing the "next" query-string parameter in the login page. However, I'm in agreement with you on your other concerns.

The following focuses solely on the "store and retrieve URL" options.

Option 2 seems super easy from the Kibana side, but it feels like we're pushing the complexity to ES, so perhaps overall it isn't easier.

Option 3 seems like we're almost building "stateful server sessions" in Kibana, just for a very limited use-case. I'm not entirely opposed to this, and I have a fear that at some point we're going to end up needing it, we've just gotten away with doing so thus far.

@azasypkin
Copy link
Member

Option 2 seems super easy from the Kibana side, but it feels like we're pushing the complexity to ES, so perhaps overall it isn't easier.

It shouldn't be super hard for ES since saml/prepare is a dedicated endpoint and asking to include RelayState fits nicely into "prepare" logic (logically), but the question is if we are okay with saying that IdPs that don't support RelayState won't have this feature. Maybe it's not that bad after all.

Option 3 seems like we're almost building "stateful server sessions" in Kibana, just for a very limited use-case. I'm not entirely opposed to this, and I have a fear that at some point we're going to end up needing it, we've just gotten away with doing so thus far.

I'm with you on this. Wouldn't want to do that unless we don't have other choice.

@kobelb
Copy link
Contributor

kobelb commented Aug 22, 2019

It shouldn't be super hard for ES since saml/prepare is a dedicated endpoint and asking to include RelayState fits nicely into "prepare" logic (logically), but the question is if we are okay with saying that IdPs that don't support RelayState won't have this feature. Maybe it's not that bad after all.

Are you aware of a popular IdP which doesn't support RelayState? Based on some extremely simplistic searching, It looks like Auth0, Okta, and ADFS support it.

@azasypkin
Copy link
Member

Are you aware of a popular IdP which doesn't support RelayState? Based on some extremely simplistic searching, It looks like Auth0, Okta, and ADFS support it.

I'm not, @jkakavas do you know any off-hand?

@jkakavas
Copy link
Member

jkakavas commented Aug 27, 2019

No. none that I'm aware of. I just raised it as something we should check out before deciding the approach to take. If Okta, Auth0, ADFS supports it ( and I I know Shibboleth does too ) then we got the largest part of the IDPs covered eitherway. If a custom, seldomly seen IDP doesn't support it, it would be easier to push back.

It won't be hard to support the RelayState handling in the prepare API either, so I'm +1 for option 2

@azasypkin
Copy link
Member

@tvernum brought up a good point that per SAML spec RelayState may not exceed 80 bytes. Even though Auth0, Okta, Shibboleth don't enforce this limitation right now we can't be sure that this will work in the future with these or others IdPs. Back to the drawing board.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Security/Authentication Platform Security - Authentication Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants