-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Provide realm name for OIDC/SAML authenticate requests. #44984
Provide realm name for OIDC/SAML authenticate requests. #44984
Conversation
Pinging @elastic/kibana-security |
💔 Build Failed |
CI used my ES branch based on ES master and it seems there is a ML change in ES that breaks Kibana ML tests ^^^. It will be fun day for our CI when super outdated 15th Aug ES snapshot is replaced with the fresh one :) |
Is this primarily for improving the performance of Elasticsearch, or is there some other tangible benefit we get? |
This solves for this: elastic/elasticsearch#45331 which is the main driver, but is also relevant for SAML so that we don't need to try and authenticate ( possibly decrypt , verify signatures and parse ) a SAML response in many realms. It also makes sense , if we were building the SAML/OIDC realms and providers now, I'd put this in by default |
What Ioannis said plus this allows us to simplify code for validating "new" SAML response payload: previously we couldn't be sure that new SAML response (in case of IdP initiated login with existing session) was processed by the same realm as was used to authenticate user previously, now we can guarantee that and hence ditch that code. |
newUserAuthenticationResult.user.username !== user.username || | ||
newUserAuthenticationResult.user.authentication_realm.name !== user.authentication_realm.name | ||
) { | ||
if (newState.username !== existingState.username) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's a user logged into Kibana in version 7.4, their state won't have a username. Now after the 7.5 upgrade happens, if there's an IdP initiated login they're going to see the overwritten session page. This is a super corner-case...
If we changed this to:
if (existingState.username != null && newState.username !== existingState.username) {
We could not display the overwritten session page in this situation, but we also lose the warning if the session is changing who the user is logged in as. Perhaps the way you have it right now is safest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly never considered such thing as a problem that is worth maintaining even a tiny BWC fix for 🤷♂️ These are such corner cases that both options look acceptable to me, but the current one is cheaper since it doesn't require us to have a clarifying comment for existingState.username != null
that we'll need to remove in 9.0...
Having said that if you think it's worth fixing I can just keep passing additional user: AuthenticatedUser
argument to this function that will have a username of the current user (we'll have it anyway since we use existing state to authenticate user even if new SAML response is present) and we won't need to rely on existingState
here and handle both corner cases. I just removed it in attempt to simplify code:
if (user.username !== newState.username) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to leave this the way it is, I haven't been able to think of another simple solution that doesn't cause its own problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - tested locally after building ES from source and everything is working properly.
💔 Build Failed |
Cancelled tests as there were mistakenly run against my own ES branch, deleted now and retest against ES master. |
retest |
💚 Build Succeeded |
7.x/7.5.0: 7b12301 |
Since elastic/elasticsearch#45767 we can provide realm name when we call
_security/saml/authenticate
and_security/oidc/authenticate
and we should do that in Kibana because of the fact that we already provide realm name when we prepare/initiate SAML/OIDC request.It also allows us to simplify code that checks whether new SAML response is for the same user or not.
For 7.x we'll send realm name only if it's specified in config and preserve aforementioned check.
Blocked by: https://github.com/elastic/infra/issues/14224 (very old 8.0.0 ES snapshot doesn't include these changes)(unblocked via #45750)cc @jkakavas