Handle access tokens that expire after authentication stage. #122155

azasypkin · 2021-12-30T07:55:37Z

Summary

In this PR we introduce a new Authenticator.reauthenticate method that is invoked in a Core's setUnauthorizedErrorHandler handler to refresh user's access token if it's expired after authentication stage.

Fixes: #104893

Release note: Previously long-running requests could lead to sporadic logouts in certain cases even though user's session was still active. It shouldn't be the case now.

elasticmachine · 2022-01-19T15:52:08Z

Pinging @elastic/kibana-security (Team:Security)

jportner

I have just a couple of nits, really scraping the bottom of the barrel here.
Nice job!

jportner · 2022-01-24T18:02:07Z

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

+
+    const existingSessionValue = await this.getSessionValue(request);
+    if (!existingSessionValue) {
+      this.logger.warn('It is not possible to extend session since it is no longer available.');


I understand that a successful re-authentication always attempts to extend the session. However, if I was reading through the logs, I think I would find the log messages a bit confusing:

WARN: Re-authenticating request due to error: blah WARN: It is not possible to extend session since it is no longer available.

Perhaps we could reword this to make it a bit clearer what's happening. WDYT about this?

Suggested change

this.logger.warn('It is not possible to extend session since it is no longer available.');

this.logger.warn('Session is no longer available and cannot be re-authenticated.');

Perhaps we could reword this to make it a bit clearer what's happening. WDYT about this?

Yeah, sounds better, thanks

jportner · 2022-01-24T18:03:38Z

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

+    // We can ignore `undefined` value here since it's ruled out on the previous step, if provider isn't
+    // available then `getSessionValue` should have returned `null`.


👍 I appreciate the comment here

jportner · 2022-01-24T18:11:43Z

x-pack/plugins/security/server/authentication/authentication_service.ts

+        return toolkit.notHandled();
+      }
+
+      this.logger.warn(`Re-authenticating request due to error: ${getDetailedErrorMessage(error)}`);


We're seeing this behavior pretty regularly for some users, and AFAIK there's nothing "wrong" with their configuration. So maybe this should be a debug log level instead of warn?

Yeah, that's good point - it was useful during development, but can be quite confusing in production.

jportner · 2022-01-24T18:38:08Z

x-pack/plugins/security/server/authentication/authentication_service.test.ts

+        expect(reauthenticate).not.toHaveBeenCalled();
+      });
+
+      it('does not handle error when security is disabled in elasticsearch.', async () => {


Question: this is just defensive coding, we don't really ever expect a 401 error if security is disabled in ES, right?

Correct, just covering if code branches in the hook, but we shouldn't get 401 here. Unless security was disabled when request reached Kibana, then application handler was busy with something, in the meantime ES enabled security, and only then application handler decided to query Elasticsearch and obviously got 401 - 0.00001% chance 🙈

…ted scenarios.

kibana-ci · 2022-01-25T10:23:45Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #18398 succeeded 3cd3617

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kibanamachine · 2022-01-25T11:18:08Z

💔 All backports failed

Status	Branch	Result
❌	8.0	Backport failed because of merge conflicts

How to fix

Re-run the backport manually:

node scripts/backport --pr 122155

Questions ?

Please refer to the Backport tool documentation

#123703) (cherry picked from commit 0491214) # Conflicts: # x-pack/test/security_api_integration/tests/saml/saml_login.ts

azasypkin mentioned this pull request Dec 30, 2021

Add API to refresh authc headers and retry ES request when 401 is encountered #120677

Merged

azasypkin force-pushed the issue-104893-acess-token-refresh branch 2 times, most recently from 306ab6d to b84d178 Compare January 4, 2022 13:32

azasypkin force-pushed the issue-104893-acess-token-refresh branch 2 times, most recently from f9c8b6d to 7b31989 Compare January 19, 2022 14:30

azasypkin removed skip-ci v7.17.0 labels Jan 19, 2022

Handle access tokens that expire after authentication stage.

3cd3617

azasypkin force-pushed the issue-104893-acess-token-refresh branch from 7b31989 to 3cd3617 Compare January 19, 2022 14:32

azasypkin marked this pull request as ready for review January 19, 2022 15:52

azasypkin requested a review from a team as a code owner January 19, 2022 15:52

jportner approved these changes Jan 24, 2022

View reviewed changes

azasypkin added 2 commits January 25, 2022 09:45

Merge branch 'main' into issue-104893-acess-token-refresh

759b19b

Review#1: improve log messages, do not use warn log level for expec…

9bb894e

…ted scenarios.

azasypkin merged commit 0491214 into elastic:main Jan 25, 2022

azasypkin deleted the issue-104893-acess-token-refresh branch January 25, 2022 11:15

azasypkin mentioned this pull request Jan 25, 2022

[8.0] Handle access tokens that expire after authentication stage. (#122155) #123703

Merged

azasypkin added a commit that referenced this pull request Jan 25, 2022

Handle access tokens that expire after authentication stage. (#122155) (

a3ef45f

#123703) (cherry picked from commit 0491214) # Conflicts: # x-pack/test/security_api_integration/tests/saml/saml_login.ts

legrego mentioned this pull request Jan 25, 2022

Improve handling of 401 responses #110264

Closed

joshdover mentioned this pull request Jun 10, 2022

[Fleet] Output preconfiguration can break on re-authentication attempts #134165

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle access tokens that expire after authentication stage. #122155

Handle access tokens that expire after authentication stage. #122155

azasypkin commented Dec 30, 2021 •

edited

Loading

elasticmachine commented Jan 19, 2022

jportner left a comment

jportner Jan 24, 2022

azasypkin Jan 25, 2022

jportner Jan 24, 2022

jportner Jan 24, 2022

azasypkin Jan 25, 2022

jportner Jan 24, 2022

azasypkin Jan 25, 2022

kibana-ci commented Jan 25, 2022

kibanamachine commented Jan 25, 2022

	this.logger.warn('It is not possible to extend session since it is no longer available.');
	this.logger.warn('Session is no longer available and cannot be re-authenticated.');

		// We can ignore `undefined` value here since it's ruled out on the previous step, if provider isn't
		// available then `getSessionValue` should have returned `null`.

Handle access tokens that expire after authentication stage. #122155

Handle access tokens that expire after authentication stage. #122155

Conversation

azasypkin commented Dec 30, 2021 • edited Loading

Summary

elasticmachine commented Jan 19, 2022

jportner left a comment

Choose a reason for hiding this comment

jportner Jan 24, 2022

Choose a reason for hiding this comment

azasypkin Jan 25, 2022

Choose a reason for hiding this comment

jportner Jan 24, 2022

Choose a reason for hiding this comment

jportner Jan 24, 2022

Choose a reason for hiding this comment

azasypkin Jan 25, 2022

Choose a reason for hiding this comment

jportner Jan 24, 2022

Choose a reason for hiding this comment

azasypkin Jan 25, 2022

Choose a reason for hiding this comment

kibana-ci commented Jan 25, 2022

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented Jan 25, 2022

💔 All backports failed

How to fix

Questions ?

azasypkin commented Dec 30, 2021 •

edited

Loading