-
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
Add support for SAML IdP initiated login when user already has active session. #33644
Add support for SAML IdP initiated login when user already has active session. #33644
Conversation
Pinging @elastic/kibana-security |
💔 Build Failed |
I think this is an interesting approach, and I really appreciate the detailed description of the issues it presents. What if we were to change the behavior to allow the user to continue as the "new user", that way we wouldn't be abusing the |
Tokens can be invalidated explicitly using the respective API: https://www.elastic.co/guide/en/elasticsearch/reference/6.6/security-api-invalidate-token.html |
I think this has a lot to do with UX (deciding what to do with the user's session, overwrite or throw error, allowing them to decide etc ) and in such cases I find it helpful to see how other SSO enabled apps are handling it to get ideas about good and bad experiences. I don't have a strong opinion but I believe session overwriting (invalidating the old) is the way to go
this issue would be no longer relevant. We can assume that the IDP session is already invalidated when the user switched auth context so we won;t need to do an SLO for the old session , just a local logout, or simply invalidating the access/refresh tokens we have tied to the old session. |
Yeah, that's an option too. The more I think about it the more I'm leaning towards it. Technically almost everything will stay the same, except for these things:
Is there any reason to not return name of authentication realm from
Great, wasn't sure that it was enough to invalidate SAML session. So we have a "local logout" support, one problem less.
Yeah, that's funny, but the only production-like SSO enabled app I have multiple accounts in is GMail and it just supports multiple concurrent sessions. I'll see if I can deploy some locally, but honestly I'm not super worried about the best possible UX for the use case with different credentials as majority of the users won't ever experience it, except for admins maybe. |
No not really. It's not currently returned as there was no use case for it, but I have nothing against making that change. If we move forward with #26171 (which I really need to pick up and get the tests added :/ ) , there might be the case that kibana knows the realm name in advance. In any case though, since Kibana can only use 1 ES SAML Realm at any given time, this shouldn't be that much of a concern. right ? |
Oh, haven't seen this one.
Hmm, the use case I have in mind with the current implementation:
Am I over-complicating things and we don't want to support this use case or it doesn't work like I assume? |
No, you are right, I was thinking of only SP initiated SSO. (I know you know this but )for future reference, the way we currently match Kibana instance to a SAML Realm in ES is that kibana builds an In order for this to be supported as a use case for SP initiated SSO, we would need to implement a SAML Discovery Service of sorts where the user gets to select the IDP they want to authenticate against. For the IDP initiated SSO use case that you were describing, my gut feeling is that the case where multiple IDPs will be configured and where the principal will be mapped to an unscoped SAML Attribute ( i.e. |
Yeah, that makes sense, I used very naive
Good, I'll incorporate your and Brandon's feedback into this WIP and we'll see how it works :) |
cd0a7fc
to
1eb7722
Compare
@@ -49,8 +49,6 @@ IMPORTANT: The {kib} user-facing origin should be the same in {kib}, {es}, and t | |||
|
|||
Users will be able to log in to {kib} via SAML Single Sign-On by navigating directly to the {kib} URL. Users who aren't authenticated are redirected to the Identity Provider for login. Most Identity Providers maintain a long-lived session—users who logged in to a different application using the same Identity Provider in the same browser are automatically authenticated. An exception is if {es} or the Identity Provider is configured to force user to re-authenticate. This login scenario is called _Service Provider initiated login_. | |||
|
|||
NOTE: Some Identity Providers support a portal or dashboard from which users can open an application that is integrated with the Identity Provider. This login scenario is known as _Identity Provider initiated login_, and {kib} has limitations with this scenario. Users cannot open {kib} in multiple tabs from the Identity Provider portal in the same browser context if an active {kib} session exists. No issues exist if users open {kib} in multiple tabs using _direct_ {kib} URL. |
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.
note: I was thinking about mentioning the case when user logs in with different credentials, but it should be a rare case and probably we don't want to overload docs with such details....
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.
That sounds reasonable to me.
@@ -0,0 +1,25 @@ | |||
.overwrittenSession { |
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.
note: proudly copied from logged_out
view :)
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.
Is there ANY way we can start making this pattern a re-usable component instead of copy/pasting? It's getting hard to manage all the usages to make sure updates get applied everywhere.
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.
We can try, let me check how many times this one is actually copy-pasted. Do you have any re-usable component in mind we can use as an example here?
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.
The latest one we tackled was the help menu popover that had default content but also allowed for custom content https://github.com/elastic/kibana/blob/master/src/legacy/ui/public/chrome/directives/header_global_nav/components/header_help_menu.tsx.
I'd assume the customizable bits would be the title string and the main content.
<h1> | ||
<FormattedMessage | ||
id="xpack.security.overwrittenSession.title" | ||
defaultMessage="You had an active session previously, but logged in as a different user." |
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.
note: wording is the hardest part, would gladly accept any better alternatives.
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 don't mind the current wording, perhaps @gchaps can provide her input. For some context Gail, we're displaying this screen when the user was previously logged into Kibana as a different user, and they're now logging in as a new user.
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.
How about making it a little simpler:
You previously logged in as a different user.
Notes:
- In the demo, I noticed this text "Last time you logged in with" in the login screen. It should be "Last time you logged in as"
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.
Thanks Gail!
In the demo, I noticed this text "Last time you logged in with" in the login screen. It should be "Last time you logged in as
Haha, luckily that's Auth0 login page, not Kibana :)
// If user has been authenticated via session, but request also includes SAML payload | ||
// we should check whether this payload is for the exactly same user and if not | ||
// we'll re-authenticate user and forward to a page with the respective warning. | ||
authenticationResult = await this.authenticateViaNewPayload( |
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.
note: couldn't find a better name for authenticateViaNewPayload
:/
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.
When reading through the code and seeing this name and the comment, I wasn't surprised, so it seems like a find name for the time being.
this.debug('Local logout has been initiated by the Identity Provider.'); | ||
|
||
// First invalidate old access token. | ||
const { |
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.
note: we do the same for token provider (copied from there), we'll need to unify that at some point.
type: string; | ||
} | ||
|
||
export interface User { |
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.
note: just defined a couple of props we need or may need soon.
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.
note I'm also introducing a User interface as part of #30977 (https://github.com/elastic/kibana/pull/30977/files#diff-31444acbfefac2d86a416260887ab8cbR7)
Whoever merges last should try to consolidate the interfaces
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.
Ha, good to know! Any reason why authentication_realm
and lookup_realm
are optional?
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 marked them optional because we don't always have that information in all UI contexts. We only get authentication_realm
and lookup_realm
for the currently logged in user, but pages like the user management screen allow you to view/create/update arbitrary users, and that information isn't available nor necessary then.
If it's easier/better for your use case to mark them required, then maybe we end up having a couple of interfaces, where one extends the other? Maybe something like User
and AuthenticatedUser
?
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.
Yeah, I think these two are different interfaces even though they look similar. The one I use is more like AuthenticatedUser\Principal\Identity
.
*/ | ||
private debug(message: string) { | ||
this.options.log(['debug', 'security', 'saml'], message); | ||
} | ||
} |
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.
note: if we need to add anything else to saml we may want to create a dedicated folder for it and split this file..... It's getting too big.
Hey @kobelb! Would you mind giving a preliminary feedback on the UX and the approach itself ( |
1eb7722
to
26cef9f
Compare
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.
This is looking and working great!
@@ -49,8 +49,6 @@ IMPORTANT: The {kib} user-facing origin should be the same in {kib}, {es}, and t | |||
|
|||
Users will be able to log in to {kib} via SAML Single Sign-On by navigating directly to the {kib} URL. Users who aren't authenticated are redirected to the Identity Provider for login. Most Identity Providers maintain a long-lived session—users who logged in to a different application using the same Identity Provider in the same browser are automatically authenticated. An exception is if {es} or the Identity Provider is configured to force user to re-authenticate. This login scenario is called _Service Provider initiated login_. | |||
|
|||
NOTE: Some Identity Providers support a portal or dashboard from which users can open an application that is integrated with the Identity Provider. This login scenario is known as _Identity Provider initiated login_, and {kib} has limitations with this scenario. Users cannot open {kib} in multiple tabs from the Identity Provider portal in the same browser context if an active {kib} session exists. No issues exist if users open {kib} in multiple tabs using _direct_ {kib} URL. |
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.
That sounds reasonable to me.
<h1> | ||
<FormattedMessage | ||
id="xpack.security.overwrittenSession.title" | ||
defaultMessage="You had an active session previously, but logged in as a different user." |
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 don't mind the current wording, perhaps @gchaps can provide her input. For some context Gail, we're displaying this screen when the user was previously logged into Kibana as a different user, and they're now logging in as a new user.
// If user has been authenticated via session, but request also includes SAML payload | ||
// we should check whether this payload is for the exactly same user and if not | ||
// we'll re-authenticate user and forward to a page with the respective warning. | ||
authenticationResult = await this.authenticateViaNewPayload( |
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.
When reading through the code and seeing this name and the comment, I wasn't surprised, so it seems like a find name for the time being.
['debug', 'security', 'saml'], | ||
'User session has been successfully invalidated.' | ||
); | ||
const redirect = isSAMLRequestQuery(request.query) |
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.
🥇
26cef9f
to
fd297be
Compare
retest |
@@ -295,17 +295,15 @@ describe('Authentication routes', () => { | |||
const unhandledException = new Error('Something went wrong.'); | |||
serverStub.plugins.security.authenticate.throws(unhandledException); | |||
|
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.
note: actually these tests were potentially buggy, we should never have assertions in a catch
or then
, if we have main assertions in catch
then then
should fail the test if it's called (noticed that because one of the tests was green, but I expected it to be red :)).
Removing review request for now:
|
…ponent from `LoggedOut` and `OverwrittenSession` views.
fd297be
to
3ff0e00
Compare
lookup_realm: UserRealm; | ||
} | ||
|
||
export function canUserChangePassword(user: AuthenticatedUser) { |
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.
note: if I read code correctly this method can only be used with AuthenticatedUser
from ChangePassword
component (not ChangePasswordForm
) that is rendered only within AccountManagementPage
that is rendered only for currently authenticated user. That's why I moved this method here.
@legrego please correct me if I didn't get this right :)
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.
that's absolutely right!
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.
Awesome, thanks!
@@ -1,22 +1,21 @@ | |||
|
|||
.loggedOut { | |||
.authenticationState { |
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.
note: that the only name I managed to find that would be somewhat related to both logged_out
and overwritten_session
:) Please share any other naming ideas that would work better!
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.
How about just changing State
to Page
? Then it could work for the login page as well. Also, these need a 3-letter prefix to conform to our BEM naming scheme. Can you add sec
as the prefix to all of these? So this one would be: .secAuthenticationPage
.
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.
How about just changing State to Page?
Hmm, I can change it to ...StatePage
since this is going to describe the finite "state of the authentication". Does that sound good?
Then it could work for the login page as well.
Not sure, I don't feel these have enough in common (logically) to generalize, the intention here is a bit different. I'd rather wait and see how it evolves.
Also, these need a 3-letter prefix to conform to our BEM naming scheme. Can you add sec as the prefix to all of these?
Sure!
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.
Not sure, I don't feel these have enough in common (logically) to generalize, the intention here is a bit different. I'd rather wait and see how it evolves.
Well the point of making this layout re-usable is more for the layout and visuals that come with it rather than for "purpose". So eventually, I'd like this to be surfaced even higher so that the login and welcome screens can use it as well so we minimize the copy/pasting of styles since that's all it's really applying here.
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.
Yeah, I definitely see your point and thanks for sharing your thoughts it's just that it should be a broader unification effort that's clearly out of scope of this PR IMO. This PR is already not a small/easy-to-review one.
So eventually, I'd like this to be surfaced even higher so that the login and welcome screens can use it as well so we minimize the copy/pasting of styles since that's all it's really applying here.
That would be great, but will take some time 🙈
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.
Yeah I don't think that needs to happen now. Just wanted to respond to the worry about it not being general enough. When we do surface it up for those other pages we can rename the classes again so ...StatePage
is fine for now.
@@ -22,7 +23,18 @@ chrome | |||
const domNode = document.getElementById('reactLoggedOutRoot'); | |||
render( | |||
<I18nContext> | |||
<LoggedOutPage addBasePath={chrome.addBasePath} /> | |||
<AuthenticationState |
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.
note: I defined this entire tree here, but let me know if you prefer it to be in a separate file.
💚 Build Succeeded |
…me `AuthenticationState` to `AuthenticationStatePage`.
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.
Code lgtm, don't know how to test it
💚 Build Succeeded |
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.
Nice work! This is working great with good test coverage!
💚 Build Succeeded |
7.x/7.1.0: eb4940d |
Contrary to what happens in the SAML realm, we do not support consuming unsolicited Responses from OpenID Connect Providers. This means that the only way that a behavior such as the one described in elastic#33644 can be observed is when - a user initiates a login in a tab but stalls in the OP authentication form - user opens a new tab in the same browser and completes authentication - user returns to original tab, submits the OP form and lands in Kibana's oidc endpoint while actually already having a session. Not sure if this is a corner case we need to cater for, so removing this for now.
Summary
This PR adds support for the Identity Provider (IdP) initiated login when user already has an active session. This usually happens when user opens Kibana from IdP app dashboard. Previously we were returning
403
error to the user explaining that we don't support that scenario.There are two cases we'll start to support with this PR:
If IdP initiated login is for the same user we'll exchange new SAML payload to new pair of access/refresh tokens invalidating existing ones (SAML local logout) and user will be automatically redirected to Kibana home page.
If IdP initiated login is for different user (either usernames or authentication realms are different) we'll exchange new SAML payload to new pair of access/refresh tokens invalidating existing ones (SAML local logout) as in the case above, but redirect user to a special page that will warn user that now they will act on behalf of a different user. We do this to avoid any confusion since new user may have a different set of roles, permissions etc. We don't expect this scenario to be hit frequently, it's mostly for admins who may test or configure Kibana on behalf of different users.
I've tested two aforementioned scenarios with Auth0 and Okta. PR includes unit and integration tests.
"Release Note: user is now able to open Kibana from SAML Identity Provider dashboard (e.g. Okta App Dashboard or Auth0 Single Sign-On (SSO) Dashboard). Previously this way of accessing Kibana wasn't supported."
Original description
In this PR I'm experimenting with one of the options mentioned in https://github.com//issues/26714:Once we receive
SAMLResponse
, but we already have an active valid session, we:Get user for the existing session via
/_security/_authenticate
Send this
SAMLResponse
to/_security/saml/authenticate
to see if it's valid one401
Demo
Issues
Once user chooses different credentials at the IdP page, old IdP session is invalidated. So when they try to open Kibana and see this new page with a warning they won't be able to do a proper SLO since ES will create
LogoutRequest
for the IdP session that doesn't exist anymore. E.g with Auth0 ES will properly invalidate ES tokens, but when user is forwarded to Auth0 portal for SLO user will seeOops!, something went wrong
withNo active session(s) found matching LogoutRequest
. We likely can't anything about that.Just to get this username for the
SAMLResponse
we have to use/_security/saml/authenticate
that will create unnecessary access and refresh tokens which is bad. Ideally we'd rather have another lighter endpoint to do that, but the use case is very narrow 🤷♂️The
/_security/saml/authenticate
returns only username and doesn't return real name. And if I understand correctly we can easily have the same username within different SAML realms, so we need to take realm name into account if we want to make sure that newSAMLResponse
corresponds to exactly same user? If true then either/_security/saml/authenticate
should return realm name or we'll have to additionally query/_security/_authenticate
to get full user info using this new access tokenIf we keep using
/_security/saml/authenticate
just to get username to cover this use case we'll have to invalidate created tokens right after we created them and the only way to that I know of is calling/_security/saml/logout
. This endpoint may returnLogoutRequest
that Kibana we'll always drop.Would be great to hear your thoughts on this @kobelb @jkakavas! Based on the
Issues
it feels a bit like we'll be abusing/_security/saml/authenticate
and/_security/saml/logout
if we go this route (andauthenticate/logout, authenticate/logout, ...
ES logs may look confusing).Fixes: #26714