-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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 _security/saml/prepare
API should allow consumer to specify RelayState
#46232
Comments
Pinging @elastic/es-security |
This isn't going to work. Per the specs
|
Good catch Tim ! Link to the aforementioned spec: https://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf |
That's a bummer, 80 byte RelayState isn't super useful. Looks like some (many?) IdPs (e.g. Auth0) don't enforce this limit though. |
Okta doesn't and AFAIK shibboleth will only print a warning about it. The downside of this is that we will be building functionality that explicitly violates the spec and depends on other implementers current leniency which is not to be taken for granted and is not future-proof. I was digging up some old mailing list threads and it seems that (At least one of) the reason this size limit was added was to discourage SPs from sharing the entire return URL in the relaystate, in order to protect users' privacy |
Yeah, I agree. We have 80 bytes limit for RelayState and ~4KB limit for all cookies per domain Kibana server relies on. The correct and future-proof solution would probably be to store full URL in ES somehow (option 3 here). But the question here is whether Kibana or ES would be the best candidate to do that and how to automatically clean it up in case of SAML auth attempts that are never completed. It's not always obvious where to put that functionality in case of such a composite SP :) If ES does that it could include some opaque token in Kibana can emulate that too via some dedicated Saved Object in .kibana-* index. We can put ID of such object to the cookie together with SAML Request ID and retrieve it later. The only problematic thing here is to somehow remove this object automatically after some period of time if it's not used. I can dig deeper into this option if the the option above is a no-go for ES.
Interesting, I see Auth0 proposes to use URL in |
The reason I looked up the spec is because web servers have varying limits on the length of a request line, some as low as 4k. Have you considered using |
We don't keep state at all ( we rely on the caller of the APIs to maintain the request IDs also ) so it would definitely not be preferable to store this in ES.
This is a different use case and this destination URL would be a generic app URL , which is fine. The problem with SP initiated SSO and deep links is that the IDP gets to know potentially private browsing behaviors of the user ( i.e. |
It depends, we always encrypt cookie content that quite significantly affects the amount of info we can store there and 4k limit is for all cookies per domain (Kibana uses just one right now, but it can easily be hosted behind a custom path in a domain that is shared with other apps that also can use cookies).
Yes, we considered
I see, thanks. So
Right, right. I just meant that if we don't use |
Currently Kibana heavily relies on URL fragments to navigate between the apps and store applied filters. And since URL fragments are never sent to the server Kibana didn't preserve them during SAML handshake. Moreover URL fragments used in Kibana can be pretty large so we can't store them in the session cookie like we do for SAML request ID. We're planning to change this in 7.5 (see elastic/kibana#44513) and store full URL in the
RelayState
query string parameter we send to IdP and receive back once user is authenticated.As we discussed with @jkakavas in elastic/kibana#18392 (comment) it'd make sense for
_security/saml/prepare
API to accept the arbitrary string as an additional argument with that will eventually be used asRelayState
query string parameter in the IdP redirect URL this API returns.cc @kobelb
The text was updated successfully, but these errors were encountered: