Switch Kerberos authentication provider to a dedicated `_kerberos` grant. Introduce `Tokens` for common access/refresh token tasks. #39366

azasypkin · 2019-06-20T17:46:19Z

Since elastic/elasticsearch#42847 we should be using a dedicated _kerberos grant to exchange SPNEGO token to a pair of access and refresh tokens. Previously with client_credentials grant we were getting only access token.

Since this change would have required yet another copy-paste of token refresh/invalidation code, I decided to move this code to a separate class, instance of which is available to every authentication provider.

Related to: elastic/elasticsearch#42847

elasticmachine · 2019-06-20T17:46:21Z

Pinging @elastic/kibana-security

…ant. Introduce `Tokens` for common access/refresh token tasks.

azasypkin · 2019-06-21T07:30:14Z

x-pack/legacy/plugins/security/server/lib/authentication/authenticator.ts

    basePath: config.get<string>('server.basePath'),
+    tokens: new Tokens({ client, log }),


note: basic is the only provider that doesn't use tokens and it's pretty cheap to instantiate it even if basic provider is used, so I decided to have it as provider option instead of re-creating it for every provider that uses tokens.

azasypkin · 2019-06-21T07:33:07Z

x-pack/legacy/plugins/security/server/lib/authentication/providers/base.mock.ts

 import { AuthenticationProviderOptions } from './base';

 export function mockAuthenticationProviderOptions(
-  providerOptions: Partial<AuthenticationProviderOptions> = {}
+  providerOptions: Partial<Pick<AuthenticationProviderOptions, 'basePath'>> = {}


note: we don't need anything except for basePath as an argument here, but I decided to keep "args as an object" pattern here for now, to have less updates and account for the future options we may want to tweak in tests.

azasypkin · 2019-06-21T07:34:18Z

x-pack/legacy/plugins/security/server/lib/authentication/providers/base.ts

@@ -23,6 +24,7 @@ export interface AuthenticationProviderOptions {
  basePath: string;
  client: Legacy.Plugins.elasticsearch.Cluster;
  log: (tags: string[], message: string) => void;
+  tokens: PublicMethodsOf<Tokens>;


note: relying on PublicMethodsOf makes it super simple to mock tokens in TS tests. I hate that tests dictate how we should write code, but here it's more or less logically acceptable.

This feels similar to what you have to do in Java/C# when you extract an interface that only has the public properties for the sake of testing. What you have here seems less intrusive, and makes me wonder if it's a pattern that we should embrace more widely?

I have a bad habit of pretending that TS is more similar to the type systems in Java/C# than it really is, so it's great learning techniques like these from you.

What you have here seems less intrusive, and makes me wonder if it's a pattern that we should embrace more widely?

In a NP world we won't have too many of these cases (contracts are usually plain objects with public only stuff on them, that's is trivial to mock and work with without additional TS voodoo), hopefully, but in places where we still want something like this I believe we embrace this approach.

Yes, the core has the same problems. We pass around ES6 classes and later in tests have type errors passing POJO with the same interface. We haven't decided on the official solution in the core #33396
but in some places already started using a separate interface for the contracts https://github.com/skaapgif/kibana/blob/a52d038cbaf605c29a94a3872d4eb70fb60c3fd2/src/legacy/server/saved_objects/service/saved_objects_client.ts#L132
Probably declaring TokenContract is more self-describing than PublicMethodsOf<Tokens>

azasypkin · 2019-06-21T07:36:43Z

x-pack/legacy/plugins/security/server/lib/authentication/providers/kerberos.ts

    this.debug(`Trying to deauthenticate user via ${request.url.path}.`);

-    if (!state || !state.accessToken) {
+    if (!state) {


note: if we have a state that doesn't have an accessToken in it - it's a bug, and I'd rather notice it earlier.

azasypkin · 2019-06-21T07:40:17Z

x-pack/legacy/plugins/security/server/lib/authentication/providers/kerberos.ts

+      // We can't just set `authorization` to `undefined` or `null`, we should remove this property
+      // entirely, otherwise `authorization` header without value will cause `callWithRequest` to fail if
+      // it's called with this request once again down the line (e.g. in the next authentication provider).
+      delete request.headers.authorization;


note: I was about to delete all these delete request.headers.authorization; in catch and mutate request only if _autenticate succeeds using this new headers argument:

const user = await this.options.client.callWithRequest(request, 'shield.authenticate', { headers: { authorization } });

But it doesn't allow to override header that is already in request, it's a bummer, we'll discuss this in the upcoming NP context-bound services RFC.

azasypkin · 2019-06-21T07:41:32Z

x-pack/legacy/plugins/security/server/lib/authentication/providers/oidc.ts

@@ -8,9 +8,9 @@ import Boom from 'boom';
 import type from 'type-detect';


note: OIDC integration tests are disabled :/ And we should enable them as soon as possible #36959. But for now I tweaked kbn-es locally and run the tests - they passed with the latest version of this PR.

azasypkin · 2019-06-21T07:44:32Z

x-pack/legacy/plugins/security/server/lib/authentication/providers/token.test.ts

@@ -47,15 +50,6 @@ describe('TokenAuthenticationProvider', () => {
      );
    });

-    it('does not handle authentication if state exists, but accessToken property is missing.', async () => {


note: same here, if we have a state that doesn't have an accessToken in it - it's a bug, and I'd rather notice it earlier.

azasypkin · 2019-06-21T07:49:08Z

x-pack/legacy/plugins/security/server/lib/authentication/tokens.ts

+
+    let invalidationError;
+    let invalidatedTokensCount;
+    if (refreshToken) {


note: I know it's kind of stupid since invalidation will be super quick, but I feel bit safer if I invalidate refresh token first as having it active feels more dangerous 🙈

Another change in behavior here is that I'll try to invalidate access token even if invalidation of refresh token failed previously.

elasticmachine · 2019-06-21T17:16:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

kobelb · 2019-06-21T15:34:32Z

x-pack/legacy/plugins/security/server/lib/authentication/tokens.ts

+   * @param message Message to log.
+   */
+  private debug(message: string) {
+    this.options.log(['debug', 'security', 'tokens'], message);


Are we worried about losing the context of which auth provider is performing these token operations? Perhaps if we added a providerName constructor argument to Token and instantiate an instance of providerOptions for each actual provider, it'd let us do this transparently to the consumer? If you don't foresee this being an issue when diagnosing issues using the logs, we don't have to do this.

Are we worried about losing the context of which auth provider is performing these token operations?

Yeah, I thought about this, but I'm not really worried: when we see logs, these will be surrounded by something like this, so it's pretty easy to derive the correct context:

[debug, security, saml] authenticating ......
[debug, security, tokens] refreshing tokens.....
[debug, security, saml] authentication completed.....

Having said that, our logs need some love and unifying :) Different providers were written by different people, and since there were no guidelines logs don't look as consistent as they should be.

In NP Logger may provide factory method to extend parent context #39695

// class SamlProvider // saml.ts const log = log.get('saml'); new Tokens({log}) // token.ts class Token { constructor({log: Logger}){ this.log = log.get('tokens'); this.log.debug('....'); // ] ['security', 'saml', 'tokens'] .... }

kobelb · 2019-06-21T16:25:03Z

x-pack/legacy/plugins/security/server/lib/authentication/providers/base.ts

@@ -23,6 +24,7 @@ export interface AuthenticationProviderOptions {
  basePath: string;
  client: Legacy.Plugins.elasticsearch.Cluster;
  log: (tags: string[], message: string) => void;
+  tokens: PublicMethodsOf<Tokens>;


This feels similar to what you have to do in Java/C# when you extract an interface that only has the public properties for the sake of testing. What you have here seems less intrusive, and makes me wonder if it's a pattern that we should embrace more widely?

I have a bad habit of pretending that TS is more similar to the type systems in Java/C# than it really is, so it's great learning techniques like these from you.

kobelb · 2019-06-21T16:43:10Z

x-pack/legacy/plugins/security/server/lib/authentication/tokens.ts

+        this.debug('Refresh token has been successfully invalidated.');
+      } else {
+        this.debug(
+          `${invalidatedTokensCount} refresh tokens were invalidated, this is unexpected.`


If the call on line 99 fails, won't invalidatedTokensCount be undefined here and hit this branch?

Ha, good catch! Previously we were immediately throwing if access token invalidation failed, sooo bad copy-pasting 🙈 Will fix.

kobelb · 2019-06-21T16:44:47Z

x-pack/legacy/plugins/security/server/lib/authentication/tokens.ts

+
+    if (accessToken) {
+      try {
+        invalidatedTokensCount = (await this.options.client.callWithInternalUser(


Re-using the invalidatedTokensCount variable seems potentially problematic. If this call fails, we potentially see the value that was set by a successful call to invalidate the refresh token.

I probably should just throw directly from second catch since I re-throw only the latest error anyway, wdyt?

That seems fine to me. The other thing I thought of was to just declare let invalidatedTokensCount right inside the if (accessToken) check. I'm fine with either though.

The other thing I thought of was to just declare let invalidatedTokensCount right inside the if (accessToken) check.

I like it better, thanks!

kobelb · 2019-06-21T16:46:08Z

x-pack/legacy/plugins/security/server/lib/authentication/tokens.ts

+      } else if (invalidatedTokensCount === 1) {
+        this.debug('Access token has been successfully invalidated.');
+      } else {
+        this.debug(`${invalidatedTokensCount} access tokens were invalidated, this is unexpected.`);


If we're only invalidating an access token, and the call on line 122 fails won't invalidatedTokensCount be undefined as well.

kobelb · 2019-06-21T16:46:50Z

x-pack/legacy/plugins/security/server/lib/authentication/tokens.ts

+        )).invalidated_tokens;
+      } catch (err) {
+        this.debug(`Failed to invalidate access token: ${err.message}`);
+        invalidationError = err;


We're potentially overwriting the error from invalidating the refresh token, is there a way to combine these errors?

Yeah, that's a bit clumsy, I decided to just throw the latest error, but log both. Frankly speaking I can't see any good way of merging these since they can even have different status codes that we return to the client, but I'm all ears if you have any suggestion!

Yeah, I don't have any real suggestions and I'm fine with the current approach.

kobelb · 2019-06-21T17:02:57Z

x-pack/legacy/plugins/security/server/lib/authentication/providers/kerberos.ts

+      return AuthenticationResult.failed(err);
+    }
+
+    // If refresh token is no longer valid, then we should clear session and redirect user to the


super nit:

Suggested change

// If refresh token is no longer valid, then we should clear session and redirect user to the

// If refresh token is no longer valid, then we should clear session and renegotiate using SPNEGO

kobelb · 2019-06-21T17:03:07Z

x-pack/legacy/plugins/security/server/lib/authentication/providers/kerberos.ts

+    }
+
+    // If refresh token is no longer valid, then we should clear session and redirect user to the
+    // login page to re-authenticate, or fail if redirect isn't possible.


Suggested change

// login page to re-authenticate, or fail if redirect isn't possible.

kobelb · 2019-06-21T17:09:53Z

x-pack/legacy/plugins/security/server/lib/authentication/tokens.ts

+      // that has been used already.
+      //
+      // When user has neither valid access nor refresh token, the only way to resolve this issue is to get new
+      // SAML LoginResponse and exchange it for a new access/refresh token pair. To do that we initiate a new SAML


super nit: these docs talk specifically about the SAML flow, when this is now used by both the token and kerberos providers.

Good catch!

kobelb · 2019-06-21T18:12:37Z

x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts

      await getService('esSupertest')
        .post('/_security/role_mapping/krb5')
        .send({
-          roles: ['krb5-user'],
+          roles: ['kibana_user'],


… invalidation failed.

elasticmachine · 2019-06-24T14:02:24Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

azasypkin · 2019-06-24T14:04:06Z

retest

elasticmachine · 2019-06-24T14:07:28Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

azasypkin · 2019-06-24T14:07:36Z

retest

elasticmachine · 2019-06-24T15:04:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…en-manager

elasticmachine · 2019-07-01T21:48:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…ant. Introduce `Tokens` for common access/refresh token tasks. (#39366) (#40101) * Switch Kerberos authentication provider to a dedicated `_kerberos` grant. Introduce `Tokens` for common access/refresh token tasks. * Review#1: improve/fix code comments, properly log the case when token invalidation failed.

azasypkin · 2019-07-02T09:33:46Z

7.x/7.3.0: 9d86c55

azasypkin added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Security/Authentication Platform Security - Authentication release_note:skip Skip the PR/issue when compiling release notes labels Jun 20, 2019

azasypkin added the v7.3.0 label Jun 21, 2019

Switch Kerberos authentication provider to a dedicated _kerberos gr…

5fdd55b

…ant. Introduce `Tokens` for common access/refresh token tasks.

azasypkin force-pushed the issue-xxx-token-manager branch from cb217e7 to 5fdd55b Compare June 21, 2019 07:24

azasypkin marked this pull request as ready for review June 21, 2019 07:24

azasypkin requested a review from a team as a code owner June 21, 2019 07:24

azasypkin commented Jun 21, 2019

View reviewed changes

azasypkin requested a review from kobelb June 21, 2019 10:31

azasypkin mentioned this pull request Jun 21, 2019

Migrate authentication subsystem to the new platform. #39446

Merged

10 tasks

Merge branch 'master' into issue-xxx-token-manager

9353068

elastic deleted a comment from elasticmachine Jun 21, 2019

kobelb reviewed Jun 21, 2019

View reviewed changes

azasypkin added 2 commits June 24, 2019 08:46

Merge branch 'master' into issue-xxx-token-manager

bec52bc

Review#1: improve/fix code comments, properly log the case when token…

977c132

… invalidation failed.

azasypkin requested a review from kobelb June 24, 2019 15:41

kobelb approved these changes Jul 1, 2019

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into issue-xxx-tok…

635a86d

…en-manager

kobelb merged commit 743f631 into elastic:master Jul 1, 2019

kobelb mentioned this pull request Jul 1, 2019

[7.x] Switch Kerberos authentication provider to a dedicated _kerberos grant. Introduce Tokens for common access/refresh token tasks. (#39366) #40101

Merged

azasypkin deleted the issue-xxx-token-manager branch July 2, 2019 09:33

azasypkin added the backported label Jul 2, 2019

		basePath: config.get<string>('server.basePath'),
		tokens: new Tokens({ client, log }),

		@@ -8,9 +8,9 @@ import Boom from 'boom';
		import type from 'type-detect';

	// If refresh token is no longer valid, then we should clear session and redirect user to the
	// If refresh token is no longer valid, then we should clear session and renegotiate using SPNEGO

Switch Kerberos authentication provider to a dedicated _kerberos grant. Introduce Tokens for common access/refresh token tasks. #39366

Switch Kerberos authentication provider to a dedicated _kerberos grant. Introduce Tokens for common access/refresh token tasks. #39366

Conversation

azasypkin commented Jun 20, 2019 • edited Loading

elasticmachine commented Jun 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin Jun 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jun 21, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kobelb Jun 21, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Jun 24, 2019

💔 Build Failed

azasypkin commented Jun 24, 2019

elasticmachine commented Jun 24, 2019

💔 Build Failed

azasypkin commented Jun 24, 2019

elasticmachine commented Jun 24, 2019

💚 Build Succeeded

elasticmachine commented Jul 1, 2019

💚 Build Succeeded

azasypkin commented Jul 2, 2019

Switch Kerberos authentication provider to a dedicated `_kerberos` grant. Introduce `Tokens` for common access/refresh token tasks. #39366

Switch Kerberos authentication provider to a dedicated `_kerberos` grant. Introduce `Tokens` for common access/refresh token tasks. #39366

azasypkin commented Jun 20, 2019 •

edited

Loading

azasypkin Jun 21, 2019 •

edited

Loading

mshustov Jun 27, 2019 •

edited

Loading

kobelb Jun 21, 2019 •

edited

Loading