-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Implement Kibana Login Selector #53010
Conversation
Pinging @elastic/kibana-security (Team:Security) |
@azasypkin Can you elaborate on this point:
Is this:
For SAML, Elasticsearch should be able handle IdP initiated login against an unspecified realm and process the auth with the first valid realm (typically which ever realm matches the SP and IdP Entity IDs). |
I think at this stage it's a little bit of everything @tvernum.
Yeah, but as far as I understand even though this parameter is an optional one it still helps Elasticsearch to process request more efficiently (for both SAML and OIDC) and also a bit more secure (for OIDC case, elastic/elasticsearch#45331). So after elastic/elasticsearch#45767 Kibana started to always pass realm name to Elasticsearch in We can probably stop sending realm name for the IdP initiated logins/logouts, but does Elasticsearch force SAML realms to have a unque (SP Entity ID, IdP Entity ID) tuple? If it doesn't and even though it's hard to think of a valid use case where admins would want to configure two very similar realms I'm still a bit worried that for the IdP initiated logins/logouts we'll depend on realm order unlike user initiated logins/logouts. Optional, but nice-to-have feature request for ES here would be to return realm name within So it sounds like here we'd need a trade-off anyway. For now I picked the one that's easier and seems to favor what Kibana user would want (based on "default" realm name configured in What do you think? |
For the SAML case, if it's the same SP and the same IdP then I think it's quite reasonable to rely on realm order. We need to support IdP initiated auth from 2 different IdPs. |
Agree, that's reasonable. The thing is that right now you don't have to rely on order for IdP initiated logins since you can configure two different Kibana instances differently (specifying different realms for each of them to have different role mappings or something like that as a result). But obviously this is just purely theoretical use case and if we don't see much value in it (or just see it as a not recommended/wrong configuration) and want to have IdP initiated login for non-default realm (in Kibana we should have something as a default anyway, at least until next major) we can surely change that. Let's see how we can do that for OIDC and try to come up with the solutions that work in a similar way.
It's good to know, thanks. |
Alternatively we can expose separate login/logout endpoints for separate realms (technically same endpoint, but realm name may be embedded into URL). |
I don't have much to add to the discussion outside of what has already been discussed, and perhaps the following is just stating the obvious. It seems like instead of having a singular However, we do potentially want the ability for one of the realms to not be displayed on the "login selector screen", but to be usable with an IdP initiated login. Instead of requiring the user to configure the realms in Kibana, should Elasticsearch have an API to return all of the realms for Kibana to use for displaying on the login selector screen? Or should we be requiring the end-user to specify all of the realms themselves, and allow them to provide additional descriptions and whether or not they should be hidden? |
Maybe. Looking at the 3 configuration values Kibana currently needs for SAML
I think we can derive each of those from info in ES
There are edge cases where the auto realm config could do the wrong thing, but as long as there are options to configure Kibana explicitly then I think we can accept them:
We don't provide an API that would give you this info right now, but I think we can. All you would need is a list of I think we'd just skip disabled realms (either by config or license), but probably have ES issue a warning in its logs if a realm is skipped via licensing. We should also consider including whenther tokens and api-keys are enabled in that response, something roughly like:
("other" isn't a nice name, but we can work out the details). From our side, it seems like a simple enough API that we could squeeze it in for 7.7 if it's useful to you. CC: @elastic/es-security |
Whichever option we choose, I believe Kibana admin should be able to completely control what authc mechanisms are used at the optional "selector" screen (including "synthetic" authc providers that don't map 1:1 to ES realms, pick Maybe we can quickly sync once you all get back from holidays and discuss pros and cons of these options.
I'm pretty sure we'll get rid of it entirely, I'm talking to the platform team about dropping this setting and just "marking" all necessary routes as whitelisted at the definition time (no admin involvement). We should be able to do that since Security is now New Platform plugin. |
f3c57a0
to
cbdae0b
Compare
cbdae0b
to
b39c5cb
Compare
b39c5cb
to
f442437
Compare
@@ -314,7 +314,8 @@ export const internals = Joi.extend([ | |||
for (const [entryKey, entryValue] of value) { | |||
const { value: validatedEntryKey, error: keyError } = Joi.validate( | |||
entryKey, | |||
params.key | |||
params.key, |
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: This fixes the issue that's is a blocker for us (without this we can't have required fields within schema.recordOf
value schema), I'll prepare a separate PR for kbn/config-schema next week or ask Platform team for a help if it turns out to be more complex than I think.
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.
Handled in #60406.
} | ||
|
||
const loginSelector = |
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 probably would need a separate component for that button, but it doesn't feel mandatory at this point.
}; | ||
|
||
private login = (providerType: string, providerName: string) => { | ||
const query = parse(window.location.href, true).query; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@@ -225,11 +241,18 @@ export class Authenticator { | |||
this.setupHTTPAuthenticationProvider( | |||
Object.freeze({ | |||
...providerCommonOptions, | |||
name: '__http__', |
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: have mixed feelings about this "reserved" name, but haven't managed to come with anything better yet. We can also forbid this name during config validation instead, but I'm not sure if it's worth the complexity 🤷♂️
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 what you have here is reasonable
idleTimeoutExpiration, | ||
lifespanExpiration, | ||
path: this.serverBasePath, | ||
this.updateSessionValue(sessionStorage, { |
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: it seems we can finally re-use this function in login
as well.
public async logout(request: KibanaRequest, state?: ProviderState | null) { | ||
this.logger.debug(`Trying to log user out via ${request.url.path}.`); | ||
|
||
if (!state) { |
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: need this check here and in other providers since I can call logout
on multiple providers if there is no session (e.g. in case IdP initiated logout).
* Performs initial login request. | ||
* @param request Request instance. | ||
*/ | ||
public async login(request: KibanaRequest) { |
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: didn't test this scenario. Will test later to make sure it works as expected.
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.
Struggling to setup environment for some reason, but technically it works now 🙂 Will make sure to check that it actually works before merging though.
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.
Kerberos Login Selector integration tests are passing, we should be good.
// Since we don't know upfront what realm is targeted by the Identity Provider initiated login | ||
// there is a chance that it failed because of realm mismatch and hence we should return | ||
// `notHandled` and give other SAML providers a chance to properly handle it instead. | ||
return isIdPInitiatedLogin |
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: more correct solution here would be to not send realm
to Elasticsearch and:
- If request failed - return
failed()
- If request succeeded and returned realm equal to the one this provider relies on - return
succeeded()
- If request succeeded, but returned realm not equal to the one this provider relies on - invalidate created tokens and return
notHandled
But I really don't like the invalidate created tokens
step.
The other solution I was thinking about was to re-use the same provider instance for all enabled providers of the same type. This solution wouldn't have the problem I'm trying to solve here, but it has the set of its own issues. Happy to discuss if you think current solution isn't acceptable.
Would do you think?
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.
Does having to support xpack.security.public.*
settings in 7.x affect this at all? Perhaps we could discuss this specific flow over Zoom. I'm not sure I'm following all of the complexities entirely.
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.
Does having to support xpack.security.public.* settings in 7.x affect this at all?
As I replied to another comment I expect to not have problem with xpack.security.public.*
if we forbid multiple SAML providers if there any without realm
specified (basically when Kibana is already configured to use SAML with old config format without specified realm
).
Perhaps we could discuss this specific flow over Zoom. I'm not sure I'm following all of the complexities entirely.
Sure, let's discuss over Zoom.
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.
What we discussed on Zoom makes sense, no concerns on this point.
if (attempt.step === SAMLLoginStep.RedirectURLFragmentCaptured) { | ||
// It may happen that Kibana is re-configured to use different realm for the same provider name, | ||
// we should clear such session an log user out. | ||
if (state?.realm && state.realm !== this.realm) { |
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: this is check is the main/only reason why I started to store realm
in the session.
return undefined; | ||
} | ||
|
||
router.get( |
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 need to properly handle redirects that's why I'm using GET
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.
Redirects from other web-servers using a 301/302?
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 meant that this endpoint responds with 302 and we won't be able to easily handle this on the client side if use HTTP client to do that POST
.
We can alternatively use hidden <form />
to do the POST
here and mark route as requireXsrf: false
and somehow preserve current hash fragment (I'm not sure if browser preserves it if we get redirect on form submit like it does for normal redirects), but I'm not sure how I feel about that added complexity yet...
What do you think? Am I missing something 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.
I think it's worth seeing how the hidden <form />
works or using a XHR and manually doing the redirect. If either of these aren't super ugly, they'd add a bit of additional protection.
@@ -40,15 +112,8 @@ export const ConfigSchema = schema.object({ | |||
}), | |||
secureCookies: schema.boolean({ defaultValue: false }), | |||
authc: schema.object({ | |||
providers: schema.arrayOf(schema.string(), { defaultValue: ['basic'], minSize: 1 }), |
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 still considering two different ways of changing configuration structure:
-
Define only new structure here and handle transformation of the old format to the new one via deprecation transformation (current one). The benefit is that config schema is a bit easier to understand and handle. The drawback is that validation messages for old format will be confusing (e.g. if user enables
saml
and forget to configurexpack.security.authc.saml.realm
the error message will be complaining aboutxpack.security.authc.providers.saml.saml.realm
instead - that's ideally - currentlyrecordOf
validation errors are even worse and need to be fixed). Another potential drawback is that we won't have a reliable way to know whether admin used old config format in case we need it (e.g. to not enable login selector by default, see below). -
Define both old and new structures in the schema via
schema.oneOf([oldSchema, newSchema])
- benefits and drawbacks are opposite to those described in the previous approach.
As for enabling Login Selector UI by default (xpack.security.authc.selector.enabled
) Ideally I'd enable it only if:
- new config format is used
and - admin enabled more than one provider (e.g when Cloud enables SAML in addition to basic or anything else Kibana admin would specify in new format)
and xpack.security.authc.selector.enabled
isn't explicitly set tofalse
Soo because of the first point I'll see what it'd cost to switch to option 2 instead.
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.
Implemented approach 2. and now enabling selector
as described above.
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!
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* master: (34 commits) [APM] add service map config options to legacy plugin (elastic#61002) [App Arch] migrate legacy CSS to new platform (core_plugins/kibana_react) (elastic#59882) Migrated styles for "share" plugin to new platform (elastic#59981) [ML] Module setup with dynamic model memory estimation (elastic#60656) Drilldowns (elastic#59632) Upgrade mocha dev-dependency from 6.2.2 to 7.1.1 (elastic#60779) [SIEM] Overview: Recent cases widget (elastic#60993) [ML] Functional tests - stabilize df analytics clone tests (elastic#60497) [SIEM] Updates process and TLS tables to use ECS 1.5 fields (elastic#60854) Migrate doc view part of discover (elastic#58094) Revert "[APM] Collect telemetry about data/API performance (elastic#51612)" fix(NA): log rotation watchers usage (elastic#60956) [SIEM] [CASES] Build lego blocks case details view (elastic#60864) Create Painless Lab app (elastic#57538) [SIEM] Move Timeline Template field to first step of rule creation (elastic#60840) [Reporting/New Platform Migration] Use a new config service on server-side (elastic#55882) [Alerting] allow email action to not require auth (elastic#60839) [Maps] Default ES document layer scaling type to clusters and show scaling UI in the create wizard (elastic#60668) [APM] Collect telemetry about data/API performance (elastic#51612) Implement Kibana Login Selector (elastic#53010) ...
7.x/7.7.0: 89a76b0 |
setting xpack.security.authc.selector.enabled introduced in 7.7.0 by #53010 is currently missing for docker entrypoint script
setting xpack.security.authc.selector.enabled introduced in 7.7.0 by #53010 is currently missing for docker entrypoint script
setting xpack.security.authc.selector.enabled introduced in 7.7.0 by #53010 is currently missing for docker entrypoint script
setting xpack.security.authc.selector.enabled introduced in 7.7.0 by #53010 is currently missing for docker entrypoint script
Summary:
This PR includes two major parts:
Notable changes:
optional
authentication mode is usedxpack.security.authc.providers
(includingxpack.security.authc.saml|oidc
) is deprecated and became mutually exclusive with the new oneserver.xsrf.whitelist
setting anymore.optional
authentication mode for the Login view route, Cannot open default log in page if SAML authentication is enabled #25257 and Don't rely just on presence of the cookie to detect active user session #50634 should be fixed nowsecureCookies
injected variableNote to reviewers: only existing tests are fixed and just a few new were added. I'm working on covering new functionality with tests at the moment. Also cannot test UI in IE 11 because of #58108.
Login Selector UI states:
Old description
In this PoC I'm going to check the following:There are three major changes here:
Authorization: xxx
HTTP header) is moved out of separate providers and consolidated in a dedicated authentication provider that is enabled by default (likely will be split into dedicated PR)login-selector
chromeless app that we can use to choose what provider and what parameters for login we should use exactly (either through shortcut URL or through a dedicated screen)Results:
Problems/limitations I've noticed so far:
realm
name it used to authentication request via providedSAMLResponse
whenrealm
name is not specified (e.g. during IdP initiated login) SAML_security/saml/authenticate
API should return realm name used for authentication elasticsearch#52053 (not blocker, but nice to have)_security/saml/invalidate
API requires realm name, but we don't have one for IdP initiated logouts when user doesn't have an active session. It's not super critical issue, but we need to figure out if we can handle it better.IdP initiated login and logouts (without active session) will always be used with default realm. We can protentially expose separate login/logout routes for different realms (technically same route, but realm name can be embedded into URL)We decided to not provide ES with realm name in this case at all.Recently we started to define provider specific routes only if provider is enabled explicitly through configuration, we may need to drop this idea so that we always have all routes (I believe ES always defines all security related routes as well). Plugin's own configuration can be used to configure any behavior we'd like to support without affecting Security plugin.It seems it's not an issue for now.How to test:
Blocked by: #60406, #53823, elastic/elasticsearch#52053, #58126Fixes: #39313, #25257, #50634
"Release Note: Kibana can now be configured to work with multiple Single Sign-On authentication mechanisms at the same time."
cc @arisonl