Fix OIDC refresh token flow when using the cookie splitter #1580

jochen-kressin · 2023-09-11T14:04:06Z

Description

This PR fixes a regression regarding the OIDC refresh token flow, caused by the move to the split cookies here: #1352

It closes #1522 and is based on the research made by @cwperks in #1569.

In short, while the refresh token flow was still working correctly, the cookie splitter would not have access to the new idToken that was written to the cookie in the process of refreshing the token. Even though the new value was written, the cookie splitter could not access that value until the next request lifecycle. This caused a "Token expired" and the user would be unauthenticated.

TODO

End to end testing, but we will most likely do that in a separate PR.

Why these changes are required?

To make sure that the OIDC refresh token flow works as expected.

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

If an idToken expires while the user is idle, the user would be unauthenticated even though the IdP provided a refreshed token.

The new behaviour corrects the refresh token flow, and the user stays authenticated.

Issues Resolved

#1522

Testing

Unit tests were added

Check List

New functionality includes testing
New functionality has been documented N/A
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.

…n set within the current request Signed-off-by: Jochen Kressin <[email protected]>

jochen-kressin · 2023-09-11T14:08:12Z

server/session/cookie_splitter.ts

+  // We don't want to mix and match between _states and .state.
+  // If we find the first additional cookie in _states, we
+  // use _states for all subsequent additional cookies
+  const requestHasNewerCookieState = rawRequest._states && rawRequest._states[cookiePrefix + 1];


@cwperks Here I deviated a bit from the original code that I posted in your PR.

I added a check to make sure that we don't mix and match between whatever is in '_states' and '.state'.
Probably just a theoretical risk, but if the new token would use less cookies than the previous one, the unsplit cookie would contain a mix between the two without this check in place. Hope that's ok.

This makes sense to me. We would not want to end up with a bad cookie caused due to mix-up.

jochen-kressin · 2023-09-13T10:29:20Z

I removed the Draft status from this PR. While we still need integration tests for this, it probably makes sense to add those in a separate PR even though they should end up in this repository.

So from my side I'd consider this PR finished unless there are any changes wanted. Feedback is always welcome :)

DarshitChanpura

LGTM!

DarshitChanpura · 2023-09-28T16:27:21Z

server/session/cookie_splitter.ts

+  // We don't want to mix and match between _states and .state.
+  // If we find the first additional cookie in _states, we
+  // use _states for all subsequent additional cookies
+  const requestHasNewerCookieState = rawRequest._states && rawRequest._states[cookiePrefix + 1];


This makes sense to me. We would not want to end up with a bad cookie caused due to mix-up.

DarshitChanpura · 2023-09-29T21:04:12Z

CI is being fixed by: #1596

jochen-kressin · 2023-10-03T15:29:09Z

@DarshitChanpura Thanks for merging!

As per Peter's comment: #1567 (comment), this should be backported to 1.x together with the "offending" original cookie splitting PR: #1352

What is the process for getting things into the correct branch...?
Should I try to combine the two PRs into one, and then open a PR with 1.x as source and target branch?

codecov · 2023-10-03T15:33:56Z

Codecov Report

Merging #1580 (08b83b3) into main (1e7b421) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1580   +/-   ##
=======================================
  Coverage   66.18%   66.18%           
=======================================
  Files          93       93           
  Lines        2339     2339           
  Branches      317      317           
=======================================
  Hits         1548     1548           
  Misses        722      722           
  Partials       69       69

DarshitChanpura · 2023-10-03T15:42:57Z

@DarshitChanpura Thanks for merging!

As per Peter's comment: #1567 (comment), this should be backported to 1.x together with the "offending" original cookie splitting PR: #1352

What is the process for getting things into the correct branch...? Should I try to combine the two PRs into one, and then open a PR with 1.x as source and target branch?

IMO they should be combine into one so we have a clean commit history.

RyanL1997 · 2023-10-05T20:06:06Z

I have tested @jochen-kressin's security frontend fork with my local set up based on main for these failing cypress tests and everything is all green:

aggregation_view.js:

multi_tenancy.js :

default_tenant.js :

…n set within the current request (#1580) Signed-off-by: Jochen Kressin <[email protected]> Co-authored-by: Darshit Chanpura <[email protected]> (cherry picked from commit d575e00)

RyanL1997 · 2023-10-05T21:26:25Z

Merging, for more details about the cypress failure lets followup on #1599

…n set within the current request (#1580) (#1603) Signed-off-by: Jochen Kressin <[email protected]> Co-authored-by: Darshit Chanpura <[email protected]> (cherry picked from commit d575e00) Co-authored-by: Jochen Kressin <[email protected]>

…n set within the current request (#1580) (#1602) Signed-off-by: Jochen Kressin <[email protected]> Co-authored-by: Darshit Chanpura <[email protected]> (cherry picked from commit d575e00) Co-authored-by: Jochen Kressin <[email protected]> Co-authored-by: Darshit Chanpura <[email protected]>

The cookie splitter should be able to use cookie values that have bee…

a4a84e7

…n set within the current request Signed-off-by: Jochen Kressin <[email protected]>

jochen-kressin requested review from cliu123, cwperks, DarshitChanpura, davidlago, peternied, RyanL1997 and stephen-crawford as code owners September 11, 2023 14:04

jochen-kressin commented Sep 11, 2023

View reviewed changes

jochen-kressin changed the title ~~Draft: Fix OIDC refresh token flow when using the cookie splitter~~ Fix OIDC refresh token flow when using the cookie splitter Sep 13, 2023

DarshitChanpura approved these changes Sep 28, 2023

View reviewed changes

cwperks mentioned this pull request Sep 28, 2023

Fix refresh token flow in OIDC accounting for cookie split #1569

Closed

3 tasks

cwperks approved these changes Sep 28, 2023

View reviewed changes

cwperks added the backport 2.x backport to 2.x branch label Sep 28, 2023

cwperks mentioned this pull request Oct 3, 2023

[BUG] OpenID Token not refreshed #1522

Closed

Merge branch 'main' into oidc-cookie-split-regression

08b83b3

DarshitChanpura added the v2.11.0 label Oct 3, 2023

RyanL1997 approved these changes Oct 3, 2023

View reviewed changes

RyanL1997 merged commit d575e00 into opensearch-project:main Oct 5, 2023
12 of 14 checks passed

opensearch-trigger-bot bot mentioned this pull request Oct 5, 2023

[Backport 2.x] Fix OIDC refresh token flow when using the cookie splitter #1602

Merged

RyanL1997 added the backport 2.11 Backport to 2.11 branch label Oct 5, 2023

opensearch-trigger-bot bot mentioned this pull request Oct 5, 2023

[Backport 2.11] Fix OIDC refresh token flow when using the cookie splitter #1603

Merged

adrian-golawski-eliatra mentioned this pull request Nov 21, 2023

Backport cookie splitter #1662

Closed

3 tasks

cwperks mentioned this pull request Dec 18, 2023

[Backport 1.x] Split up a value into multiple cookie payloads #1702

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix OIDC refresh token flow when using the cookie splitter #1580

Fix OIDC refresh token flow when using the cookie splitter #1580

jochen-kressin commented Sep 11, 2023 •

edited

Loading

jochen-kressin Sep 11, 2023

DarshitChanpura Sep 28, 2023

jochen-kressin commented Sep 13, 2023

DarshitChanpura left a comment

DarshitChanpura Sep 28, 2023

DarshitChanpura commented Sep 29, 2023

jochen-kressin commented Oct 3, 2023

codecov bot commented Oct 3, 2023

DarshitChanpura commented Oct 3, 2023

RyanL1997 commented Oct 5, 2023 •

edited

Loading

RyanL1997 commented Oct 5, 2023

Fix OIDC refresh token flow when using the cookie splitter #1580

Fix OIDC refresh token flow when using the cookie splitter #1580

Conversation

jochen-kressin commented Sep 11, 2023 • edited Loading

Description

TODO

Category

Why these changes are required?

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

Issues Resolved

Testing

Check List

jochen-kressin Sep 11, 2023

Choose a reason for hiding this comment

DarshitChanpura Sep 28, 2023

Choose a reason for hiding this comment

jochen-kressin commented Sep 13, 2023

DarshitChanpura left a comment

Choose a reason for hiding this comment

DarshitChanpura Sep 28, 2023

Choose a reason for hiding this comment

DarshitChanpura commented Sep 29, 2023

jochen-kressin commented Oct 3, 2023

codecov bot commented Oct 3, 2023

Codecov Report

DarshitChanpura commented Oct 3, 2023

RyanL1997 commented Oct 5, 2023 • edited Loading

RyanL1997 commented Oct 5, 2023

jochen-kressin commented Sep 11, 2023 •

edited

Loading

RyanL1997 commented Oct 5, 2023 •

edited

Loading