Fix cookie expiry issues from IDP/JWT auth methods, disables keepalive for JWT/IDP

[derek-ho]

Description

There have been several complaints about timeouts within 1 hour for OSD, even when the token/assertion passed back from the IDP lasts longer.

For SAML one comment on the issue was to disable keepalive setting. This is because this block of logic in the authentication for keepalive would keep over-writing the expiryTime of the cookie to be a shorter value. In order to fix it, I took the max of the current value and the current time + session time. I believe this should have no regression, since keeping the session alive should never shorten the existing expiry of the cookie. However, this change is in a shared part of the code for all flows, so I may need extra eyes on this. I believe we need to close on #1711 before the full confusion around this is resolved.

For OIDC, the expiry time is confusingly set on two values, here and here. For requests, there seems to be both validation we perform on the plugin side of the cookie here and validation that Hapi performs on the cookie here. To me it seems like there is a mismatch of focusing on the cookie.credentials.expires_at, instead of cookie.expiryTime. In order to be more consistent with the other forms of auth, I removed the cookie.credentials.expires_at property and instead opted to keep cookie.expiryTime, like the other forms of auth, and this fixed the issue.

After initial review, the scope of this PR expanded to disable keepalive support for JWT based auth, including JWT, SAML, OIDC. In these cases, the session should not be kept alive beyond what the exp in the claim/assertion/JWT itself is. It also adds support to read the expiry of JWT if it exists and use the minimum of that the session TTL setting.

Category

[Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation]

Why these changes are required?

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

The behavior for multi auth, proxy auth, basic auth remains unchanged.
For JWT: the previous behavior was that sessions were the length of config.session.ttl. Subsequent user calls invoked the keepalive block to extend the session by config.session.ttl. The new behavior is that sessions are the minimum of (config.session.ttl and the expiry claim on the JWT, if it exists). Subsequent user calls invoke the keepalive block to extend the session to the lesser of (config.session.ttl and the expiry claim on the JWT, if it exists).
For SAML: The previous behavior was the sessions were the length of the assertion. However, subsequent user calls if keepalive was enabled were potentially decreasing the value of the expirytime of the cookie. The new behavior is that sessions are the length of the assertion. Subsequent user calls to keep alive, even if it is enabled, will do nothing to the expirytime of the cookie.
FOR OIDC: The previous behavior was that sessions were the length of config.session.ttl. This was due to a bug/reading of the wrong variable on the security cookie. The new behavior is that sessions are the length of the exp claim of the OIDC token, or the expiry of the token itself. Subsequent user calls to keep alive, even if it is enabled, will do nothing to the expirytime of the cookie.

Issues Resolved

Not sure if it solves them completely, but should fix #828 #159 much better. I think this PR can close those two issues, and we can keep #1711 as a tracking issue for session related confusion. Also fix: #1784

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4dc984b) 67.41% compared to head (5e7e45b) 67.41%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1773   +/-   ##
=======================================
  Coverage   67.41%   67.41%           
=======================================
  Files          94       94           
  Lines        2409     2409           
  Branches      321      321           
=======================================
  Hits         1624     1624           
  Misses        707      707           
  Partials       78       78           

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

[derek-ho]

@jochen-kressin can you take a look at this PR when you have the chance?

[cwperks]

@derek-ho wdyt about the abstract authentication_type having an abstract method (maybe supportsKeepalive) that defaults to true, but can be overridden by the saml authentication type to set it to false?

This condition could be updated to check if the current auth type supports keepalive.

[derek-ho]

@derek-ho wdyt about the abstract authentication_type having an abstract method (maybe supportsKeepalive) that defaults to true, but can be overridden by the saml authentication type to set it to false?

This condition could be updated to check if the current auth type supports keepalive.

At face value I think this is a good suggestion to clean up the code. However, I think some things around the session and settings overall need to be cleaned up as a result of #1711 before I would feel more confident about doing that. Although keepalive is only used there, IMHO it's a sweeping change to say that SAML/OIDC do not do keep alive. Not saying it's wrong, but I think this is a PR that does the least to ensure a non-confusing experience. If/when we come to agreement on the actual definition of the settings and its documented, I would be ok with moving it to a function that is true of all IDP configs. WDYT?

[cwperks]

@derek-ho Should there be any sort of warning in the logs if keepalive is set to true and session.ttl is configured as a lower value than what the cookie expiry time has? Effectively, its ignoring the user's configuration and taking the expiry returned from the SAML Authn Response.

What should be the behavior in the scenario below? Can/should a test be written for this scenario?

1. Setup multi-auth with basic auth + SAML.
2. Set keepalive to true and session.ttl to 3600 so that basic auth user's get an hour long session
3. First, set the SAML IdP for a session length of 30 min

In this case, the SAML user will get a 1 hr long session since keepalive is set to true and session.ttl is 3600?

4. Now change the SAML IdP to a session length of 90 min

After the SAML IdP is changed, can it be verified that the session length for the SAML user is 90 min and not 1 hr? <-- This PR will address this case

[cwperks]

TY @derek-ho This change looks good to me. I left a couple of comments, but don't see any major issues.

I like that this uses expiryTime consistently to determine the validity of the cookie because it was odd having the openid flow rely on a separate field called credentials.expires_at.

This change would also help in cases where autologout is not enabled by calculating the cookie's expiry time more precisely when JWTs are being used.

[DarshitChanpura]

thanks @derek-ho !

[cwperks]

Took one more pass to focus on tests. I think some of the tests can be shortened since their is config that is not used. Can you see if some of those can be updated?

[cwperks]

2 last minor comments on variable names, otherwise this LGTM!

[DarshitChanpura]

@derek-ho Can you check why the CI is failing?

[derek-ho]

@derek-ho Can you check why the CI is failing?

GHA seems to have flaky internet connectivity and/or some links are not currently available, possibly related to AT&T outage. Can you re-run the failed steps? There are two of them.

[DarshitChanpura]

GHA seems to have flaky internet connectivity and/or some links are not currently available, possibly related to AT&T outage. Can you re-run the failed steps? There are two of them.

Re-ran all actions thrice. Let's wait for some time before re-running these.

[derek-ho]

Potentially blocked on: opensearch-project/OpenSearch-Dashboards#5926