Provide realm name for OIDC/SAML authenticate requests. #44984
Conversation
azasypkin
added
blocked
Team:Security
|
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.
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.
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.
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.
💔 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/authenticateand_security/oidc/authenticateand 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