[skip ci] feedback on approach for token provider by epixa · Pull Request #25431 · elastic/kibana

epixa · 2018-11-08T21:53:04Z

do not merge

I highlighted the key parts via inline comments.

This isn't a functional change yet, so don't bother running it. I'm looking for feedback on the authentication flow for the token provider and the proposed change to how login attempts are handled in authentication as configured through the login HTTP endpoint.

elasticmachine · 2018-11-08T21:53:06Z

Pinging @elastic/kibana-platform

epixa · 2018-11-08T21:55:49Z

x-pack/plugins/security/server/routes/api/v1/authenticate.js

-          BasicCredentials.decorateRequest(request, username, password)
-        );
+        request.loginAttempt.setCredentials(username, password);
+        const authenticationResult = await server.plugins.security.authenticate(request);


Rather than extracting the username and password from the payload and faking a basic auth header on the request to force a new login during authenticate, login attempts in authenticate will now be handled exclusively through the new loginAttempt object on the request.

👍 I like this much more

epixa · 2018-11-08T21:56:59Z

x-pack/plugins/security/server/routes/api/v1/authenticate.js


 export function initAuthenticateApi(server) {

+  server.decorate('request', 'loginAttempt', (route, options) => {


Feedback on the loginAttempt implementation is welcome, but it's not critical at this point. The internal details of this decorator are not yet my priority.

question: I'm wondering if we want to make this a bit more generic/opaque (without hardcoding of username and password parameters, like we do state - every providers knows how to decode its state) so that we can use loginAttempt for SAML Response payload as well?

I think that's a good thing to consider, but I'm not sure if I'll implement it that way initially. I'll ponder it more, but my current thinking is that broader refactoring across all authentication providers should wait until we have one more two more SSO-based providers like kerberos and oauth2. SAML's working pretty well at the moment, and at least currently this loginAttempt logic only matters to providers that explicitly need to power the login form.

epixa · 2018-11-08T21:59:21Z

x-pack/plugins/security/server/lib/authentication/providers/token.js

+   * @returns {Promise.<AuthenticationResult>}
+   * @private
+   */
+  async _authenticateViaLoginAttempt(request) {


This is the key function that is invoked when login credentials exist for a request, which it retrieves from the loginAttempt object on the request.

The basic provider would be updated to use this mechanism as well. The only real difference between this function in the token provider and the basic provider is the interaction with Elasticsearch using those credentials.

epixa · 2018-11-08T22:04:59Z

x-pack/plugins/security/server/lib/authentication/providers/token.js

+   * @param {Object} [state] Optional state object associated with the provider.
+   * @returns {Promise.<AuthenticationResult>}
+   */
+  async authenticate(request, state) {


This is the entire proposed authentication flow for the token provider. It is sort of a merging of logic between the basic and saml providers.

It supports true basic auth headers, similar to both basic and saml.

It supports login from credentials, similar to the basic provider.

It supports token auth and token refresh similar to saml, but it does not include the handshake process handling that saml does.

I'd appreciate feedback on the high level logic in the flow itself.

elasticmachine · 2018-11-08T22:07:37Z

Pinging @elastic/kibana-security

epixa · 2018-11-08T22:07:49Z

@azasypkin @kobelb Can y'all take a look at this when you get a moment? Hopefully none of it is a surprise since it's based on the discussions I've had with both of you.

kobelb · 2018-11-08T22:20:27Z

x-pack/plugins/security/server/lib/authentication/providers/token.js

+   * @param {Hapi.Request} request HapiJS request instance.
+   * @returns {Promise.<DeauthenticationResult>}
+   */
+  async deauthenticate(request) {


We'll want to also invalidate the access/refresh tokens when they deauthenticate, like SAML does here: https://github.com/elastic/kibana/blob/master/x-pack/plugins/security/server/lib/authentication/providers/saml.js#L374

kobelb · 2018-11-08T22:23:27Z

x-pack/plugins/security/server/lib/authentication/providers/token.js

+
+      this._options.log(['debug', 'security', 'token'], 'Request has been authenticated via header.');
+
+      return AuthenticationResult.succeeded(user, { authorization });


When we authenticate using the basic auth headers, I don't think that we want to put the user into the session, as this would be putting the username/password encrypted in the session cookie, which can't be invalidated with the logout. We'll likely want to perform the same flow as the "authenticate via login attempt"

I'll have to double check, but I think passing user to succeeded isn't the problem here as that shouldn't include the credentials (at least not the password). But I think passing authorization here will have the effect you describe, so we definitely don't want that.

I'll have to double check, but I think passing user to succeeded isn't the problem here as that shouldn't include the credentials (at least not the password)

It shouldn't be a problem, and moreover it's required for succeeded and it's not used for session.

which can't be invalidated with the logout. We'll likely want to perform the same flow as the "authenticate via login attempt"

Hmm, correct me if I'm wrong, but if it's in cookie, the deauthenticate that is called on logout should clear it as well. But having said that I agree that we don't want to store Bearer xxxx in the cookie when we authenticate user via header (and should remove this "feature" from basic provider auth via header as well since now we have login attempt thing), but if we don't store it in the session cookie during "login attempt", where do we get access/refresh tokens from for the next request?

Agreed on the user vs authenticated point, I was wrong.

But having said that I agree that we don't want to store Bearer xxxx in the cookie when we authenticate user via header (and should remove this "feature" from basic provider auth via header as well since now we have login attempt thing), but if we don't store it in the session cookie during "login attempt", where do we get access/refresh tokens from for the next request?

So, just to be clear, we're talking about not using tokens when the user authenticates using basic auth headers, and additionally not storing the username/password in the session cookie itself; but instead requiring subsequent requests to provide the basic auth headers themselves?

Hmm, I was talking about two cases:

_authenticateViaHeader - here we expect to receive Authorization: Bearer xxxx and hence we shouldn't store anything in the cookie since we expect every request to have this header (e.g. they have some OAuth proxy in front of Kibana that supplies it). Currently basic auth provider stores it in the cookie, but with loginAttempt we can finally fix that.

_authenticateViaLoginAttempt - here we expect to receive credentials that we exchange for the token pair. It's probably one time action (e.g. initiated from the login page) so we should store token pair in the cookie (and "forget" credentials) and allow consequent requests to rely on generated tokens from the cookie instead.

Or I'm misunderstanding something? :)

I believe we're on the same page, I forgot that we could see either the Authorization: Bearer xxxx or the Authorization: Basic xxxx headers when using the _authenticateViaHeader method, but I believe it makes sense to not store these in the cookie in either situation.

I believe we're on the same page

Great!

, but I believe it makes sense to not store these in the cookie in either situation.

Yes, that's where loginAttempt that we didn't have before can help us.

kobelb · 2018-11-08T22:27:25Z

x-pack/plugins/security/server/routes/api/v1/authenticate.js

-          BasicCredentials.decorateRequest(request, username, password)
-        );
+        request.loginAttempt.setCredentials(username, password);
+        const authenticationResult = await server.plugins.security.authenticate(request);


👍 I like this much more

kobelb · 2018-11-08T22:29:02Z

x-pack/plugins/security/server/lib/authentication/providers/token.js

+    if (authenticationResult.notHandled()) {
+      authenticationResult = await this._authenticateViaState(request, state);
+      if (authenticationResult.failed() && isAccessTokenExpiredError(authenticationResult.error)) {
+        authenticationResult = this._authenticateViaRefreshToken(request, state);


I'm not seeing the _authenticateViaRefreshToken implementation yet. It's likely this is known and intentional, but I wanted to note it just in case it wasn't.

Ah yeah, I meant to call that out. It's not implemented yet, but it's essentially a copy+paste job from saml.

azasypkin · 2018-11-12T12:12:27Z

x-pack/plugins/security/server/lib/authentication/providers/token.js

+    }
+
+    const authenticationSchema = authorization.split(/\s+/)[0];
+    if (authenticationSchema.toLowerCase() !== 'basic') {


nit: basic ---> bearer or you want this provider to support both or just basic for some reason? If someone is using Basic ... header directly I think they are supposed to use basic auth provider. Using Bearer here would allow us to use any ES realm that understands tokens (e.g custom OAuth realms + OAuth proxy in front of Kibana).

I double checked the SAML provider logic and sure enough it uses bearer. I was under the impression that we had hardcoded true basic auth header support into each provider to make sure people could still do API calls programmatically while also supporting UI-based auth workflows, but clearly that's not the case.

Unfortunately, this gets more tricky with basic/token providers. The login form itself can only work with one of those two providers, so I worry about the subtly in ordering of those providers in the configuration accidentally exposing people to issues they didn't realize. Like when you switch between token and basic auth for the login form, they will appear to work in exactly the same way, and you'd need to examine the contents of the encrypted cookie (not possible for end users) to even know which provider was handling login.

I wonder if it makes sense to introduce an explicit login handling configuration so you can configure things like basic and token providers at the same time while simultaneously ensuring that the login form only works with one specific provider.

Thoughts?

Just a couple of thoughts:

The order of auth providers in [basic, token] or [token, basic] only influences login page, for programmatic API it won't matter (basic intentionally fails if it notices non-empty Authorization header that it doesn't understand to allow next provider to handle that, token should do the same). For [saml, token] the order will matter though. But as far as I understand the order of realms can be tricky for ES as well and administrators should know what they do when they configure the order of realms.

Having said that, I agree, "the first auth provider always manages login" rule isn't obvious, we can introduce config option to make it explicit which auth provider should handle login and then we can "sign" loginAttempt with provider name/id (like we do for session) based on this config. Otherwise there may be an issue when provider can try to use loginAttempt to authenticate user just because its shape looks like what provider expects even though it was supposed to be used by another provider (idk, like ldap-based auth provider that expects username and password and basic that expects the same).

I feel like I'm missing something. I thought this provider was going to essentially be a "basic auth provider using tokens for sessions internally". In what situations would the requests from the end-users browser be presenting "bearer" authorization headers?

@kobelb That was how I was thinking about it originally, but I'm coming around to the idea that the token provider is not a special basic auth provider at all. So the idea would be that if you want to support both token auth and basic auth, you'd have to enable both providers. You could lock out basic auth entirely if you wanted.

With this in mind, we don't have to support header-based auth at all with the token provider. The only real use case I can think of would be people building custom SSO solutions (ldap perhaps?) that leverage the token provider rather than the basic provider as they do today. Correct me if I'm wrong about that possibility, though.

@epixa gotcha, I'm with you all now, thanks for catching me up.

Allowing custom SSO using the header makes sense conceptually, I don't see any real harm in including it if it opens up this possibility. I honestly haven't played around with various intelligent reverse proxies that would make this type of workflow possible enough to know the feasibility, but it'd be a ton nicer than the run-as approach that we have now.

epixa · 2018-11-20T19:50:03Z

Replaced with #25971

epixa added 2 commits November 8, 2018 16:47

wip token provider

a9c223e

clean up state auth

7f8d8a1

epixa added WIP Work in progress Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// labels Nov 8, 2018

epixa self-assigned this Nov 8, 2018

epixa commented Nov 8, 2018

View reviewed changes

epixa requested a review from azasypkin November 8, 2018 22:06

epixa added Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// and removed Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// labels Nov 8, 2018

kobelb reviewed Nov 8, 2018

View reviewed changes

azasypkin reviewed Nov 12, 2018

View reviewed changes

epixa closed this Nov 20, 2018

epixa deleted the token-provider branch November 20, 2018 19:31

epixa mentioned this pull request Dec 12, 2018

[security] Support alternate auth providers for login #26979

Merged

kobelb mentioned this pull request May 20, 2019

Add OpenID Connect auth provider #36201

Merged


		export function initAuthenticateApi(server) {

		server.decorate('request', 'loginAttempt', (route, options) => {


		this._options.log(['debug', 'security', 'token'], 'Request has been authenticated via header.');

		return AuthenticationResult.succeeded(user, { authorization });

Conversation

epixa commented Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

do not merge

Uh oh!

elasticmachine commented Nov 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Nov 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Nov 8, 2018

Uh oh!

epixa commented Nov 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azasypkin Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epixa commented Nov 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

epixa commented Nov 8, 2018 •

edited

Loading

azasypkin Nov 12, 2018 •

edited

Loading

azasypkin Nov 9, 2018 •

edited

Loading