OIDC session expires with ID token expiry and refresh token is never used.

[Alankarsharma]

Description

In OIDC flow ID token expiry time was used to set cookie expiry time. Because of this cookie was getting expired at same time as ID token and alternate flow in isValidCookie function to fetch new ID token using refresh token is not getting triggered as cookie itself is expired.

Category

Bug fix

Why these changes are required?

What is the old behavior before changes and new behavior after changes?

This code was correct in version 2.11.1, issue is only in 2.13.0 version. I haven't verified 2.12.0 version.

Issues Resolved

[List any issues this PR will resolve (Is this a backport? If so, please add backport PR # and/or commits #)]

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

• New functionality includes testing
• New functionality has been documented
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[derek-ho]

Hi @Alankarsharma thanks for the PR! Just providing some initial feedback here - I was the person who made the PR that changed the OIDC cookie expiry - initial PR is here. I am not an OIDC expert, so maybe I got some things wrong, but we can discuss here and get to the bottom of this (I still have seen some complaints about auto logouts in the public slack, and I am actually trying to replicate and dive deep on this myself - see conversation here: https://opensearch.slack.com/archives/C051Y637FKK/p1712521239951639).

Some basic background is that there is actually two checks that go into seeing if the cookie is still valid - the place you modified in this PR, but also here. In my initial PR in which I consolidated the expiryTime variables, and used the expiry from the token response is because from my understanding for external IDP login setups we should not be blindly giving a session length of TTL, and should instead rely on the timeout from the credentials given by the IDP. Is that not correct/can you explain a little more the reasoning behind why you think this PR would fix the issues?

[Alankarsharma]

OIDC response has two JWT token, ID Token and Refresh token. You are taking expiry time of ID token and setting it as expiry of cookie, ID token are short lived token and once they are expired we can get new token using Refresh token, code for this is present in isValidCookie.
In login flow, I have set ID token expiry value in 'cookie.credentials.expiry' and checking if ID token is still valid or not. If it is valid then true is returned, if it is not valid a new ID token will be fetched using Refresh token and value of this token are updated in cookie again.
User is logged-out only if both ID token and refresh token are expired or cookie is invalidated.

[derek-ho]

Thanks @Alankarsharma I took another look through the whole flow - I think this PR probably also needs to introduce a check before this line:

security-dashboards-plugin/server/session/security_cookie.ts

Line 71 in 6425893

if (sessionStorage.expiryTime === undefined || sessionStorage.expiryTime < Date.now()) {

to switch the expirytime variable for OIDC. Additionally, we may want to change the keepAlive setting to add the TTL onto the expiry cookie instead of being a no-op here:

security-dashboards-plugin/server/auth/types/openid/openid_auth.ts

Line 258 in 6425893

return cookie.expiryTime!;

. Can you also run yarn lint:es --fix to address the lint failures on this PR and fix any test cases that are failing? @cwperks do you agree with this assessment?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.61%. Comparing base (9abc57d) to head (d6c989f).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1924   +/-   ##
=======================================
  Coverage   70.61%   70.61%           
=======================================
  Files          97       97           
  Lines        2600     2600           
  Branches      387      380    -7     
=======================================
  Hits         1836     1836           
  Misses        668      668           
  Partials       96       96           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[RyanL1997]

Hi @Alankarsharma, thanks for taking this on.

[Alankarsharma]

@derek-ho I checked two code location that you mentioned. No code change is required there.

[derek-ho]

@Alankarsharma I think this looks good - one quick question, maybe we can take this on as a separate follow up item: how can we add a test case around this refresh token flow? Do you have any ideas? I think a simple unit test may not suffice since it seems that the issue was that the cookie was invalidated prior to reaching the isValidCookie function?

[Alankarsharma]

@derek-ho I will check test case for refresh token, will create a separate pull request for that.

[cwperks]

@Alankarsharma Thank you for this PR! I branched off of this and added a test to verify the refreshToken workflow with this fix included: #1990

I believe there's one more area where the expiryTime of the credentials needs to be updated instead of the cookie here:

security-dashboards-plugin/server/auth/types/openid/openid_auth.ts

Lines 296 to 300 in 9abc57d

	cookie.credentials = {
	authHeaderValueExtra: true,
	refresh_token: refreshTokenResponse.refreshToken,
	};
	cookie.expiryTime = getExpirationDate(refreshTokenResponse);

The changes in this PR will work for the first refresh, but would fail on subsequent refresh since it would be setting the expiry time of the cookie to the expiry time of the access token after first refresh.

[derek-ho]

@Alankarsharma will be closing this out since the change was merged with @cwperks PR. Thank you for your contribution! I hope to see more from you in the future :)