Add start of state to CSRF cookie name #1504

YoavOst · 2022-01-05T20:27:04Z

Description

This change adds a part of the state nonce to the CSRF name, making it possible to have parallel callback requests.

When having parallel ajax requests after the session cookie has expired multiple callbacks occur at the same time, creating multiple CSRF tokens with the name making it impossible for the proxy to authenticate.
It solves issue 1451 and possibly issue 817

How Has This Been Tested?

Deployed using docker-compose with OIDC (keycloak) provider and cookie session store and solve the parallel AJAX requests issue.

JoelSpeed

I think this is a good improvement to the project on first glance, but it would be good to seem some tests and maybe make the code a bit easier to read if possible

JoelSpeed · 2022-02-14T13:43:58Z

pkg/cookies/csrf.go

-	cookie, err := req.Cookie(csrfCookieName(opts))
+
+	// csrfCookieName will include state to have multiple csrf in case of parallel requests
+	state := req.URL.Query()["state"][0][0:csrfStateLength - 1]	


Is there a way we can make this a bit nicer to read and also handle the possibility of index out of bounds errors? Wondering what happens if state isn't there for example, or for some reason it ends up empty

I'm also curious if we can potentially manage multiple CSRF state checks in an array structure serialized in 1 CSRF struct serialized into the cookie?

Things on my mind:

The cookie manages the OIDC nonce across requests too. How will that handle multiple (seems fine).

We encrypt the cookie payload (oauth state & nonce contents). Making the state part of the cookie name will slightly weaken that posture. But I bet it's fine, the encryption was likely overkill and those would only expose a portion of the OAuth state nonce.

I'm also not convinced the multiple tab issue was the sole source of these intermittent CSRF failures.

At my previous company where I ran this in production, whenever users reported this error (and when it happened to me) multiple tabs logging in a once was never a symptom.

This long running issue has more samples of tips people have (and theories on why this happens): #817

Do we think think this will fix some of the prefetch, favicon & JS issues people have uncovered as reasons?

Hi @JoelSpeed, Added some more testing for edge cases.
@NickMeves I'm not sure about the multiple tabs, I have to say that from my experience, issues with multiple tabs can result from session management problems on the provider side.
I do suspect the parallel requests to favicon and prefetch are related to this.

NickMeves · 2022-02-19T23:17:54Z

pkg/cookies/csrf.go

-func csrfCookieName(opts *options.Cookie) string {
-	return fmt.Sprintf("%v_csrf", opts.Name)
+func csrfCookieName(opts *options.Cookie, state string) string {
+	return fmt.Sprintf("%v_%v_csrf", opts.Name, state)


NIT -- can we potentially flip these so the dynamic part of the cookie name is the suffix? (%v_csrf_%v). And it might be worth renaming the state variable to signify this is a subset of the state.

In the CHANGELOG we will also need to signify this is potentially breaking for any users that had the cookieName option set close to the maximum (255 characters right?) -- Although I hope no one did that 🤣

@NickMeves Sure. Fixed both

mazenaissa · 2022-03-21T11:41:43Z

Hello @YoavOst ,
Could you please make the suggested changes? I am facing the 403 FORBIDDEN issue due to multiple oauth2 proxy callbacks invoked from multiple browser tabs.
Thanks,

BronzeDeer · 2022-04-13T07:46:05Z

@NickMeves @JoelSpeed @YoavOst Anyway other people can help get this reviewed and merged faster? At work we're also running into the parallel Ajax problem which is currently blocking

mazenaissa · 2022-04-18T10:57:27Z

@NickMeves @JoelSpeed @YoavOst Anyway other people can help get this reviewed and merged faster? At work we're also running into the parallel Ajax problem which is currently blocking

any updates guys?

JoelSpeed

Has anyone tested this at all? Looking at the PR there are no test changes, can we make sure there are unit tests to ensure we don't regress on this functionality in the future?

quantumdecoherence · 2022-05-16T13:05:08Z

I've tested the PR, and can confirm that it solves the 403 FORBIDDEN problem. Thank you for that!

However, when the oauth2 session is expired, CSRF cookies are issued for every request made. The only CSRF cookie that's cleaned up is the one leading to a successful oauth2 login. This in turn leads to large headers containing all the uncleaned cookies being sent by the browser, and eventually to requests running into max header size.

tahoell · 2022-05-17T14:37:25Z

Hello all! Since we would be very happy about this PR in a project, could someone please resolve this merge conflict?

pfragoso · 2022-06-22T12:45:28Z

We suffer from the #817 so happy to test this as the merge conflict is fixed.

miguelborges99 · 2022-06-27T22:18:25Z

In face of multiple oauth2 proxy callbacks, the CSRF cookie faces several concurrency problems:

some requests may fail due to missing CSRF token, in case a concurrent request cleans up the cookie, or
some requests may fail with CSRF token mismatch, since CSRF cookie value was overriden by other concurrent request.

This fix seems to solve both issues #1451 and #817 which have more than 2 years.

Can conflits be fixed?

github-actions · 2022-08-28T00:09:50Z

This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed.

JoelSpeed · 2022-08-29T09:30:03Z

Let's give this a go, hopefully this resolves the issues. It would be nice if someone could follow up with some unit tests for this

JoelSpeed · 2022-08-29T09:35:49Z

Ok this doesn't seem like it's working correctly as it's causing a panic within the tests, anyone got time to work out why and resolve that?

Tests aren't passing

mazenaissa · 2022-08-30T15:54:53Z

Hello @JoelSpeed ,
Could you check this pull request #1708
One of the twos must be accepted to fix the CSRF issue
Thanks,

WolfgangMehner · 2022-08-31T16:47:58Z

May I suggest the branch from @mazenaissa #1708 .
It implements some nice features, like e.g. some configuration for the CSRF cookies. In particular, you can configure a separate expiry for the CSRF-Cookies, which is very useful.
Additionally, this branch has been tested by us with a production system, and several hundred users.

JoelSpeed · 2022-08-31T20:11:57Z

I'm going to close this in favour of #1708 as I think that's pretty much ready to go and seems to have had a bunch of testing from others

Add start of state to CSRF cookie name

d0faaad

YoavOst requested a review from a team as a code owner January 5, 2022 20:27

JoelSpeed reviewed Feb 14, 2022

View reviewed changes

NickMeves reviewed Feb 19, 2022

View reviewed changes

YoavOst and others added 3 commits March 28, 2022 20:18

Merge branch 'oauth2-proxy:master' into enable-parallel-csrf

766e7d9

Cleancode and docs

9169a0e

revert dockerfile

a263f6d

YoavOst changed the title ~~Add start of state to CSRF cookie name~~ WIP: Add start of state to CSRF cookie name Mar 28, 2022

types and format

e5c4d8c

YoavOst changed the title ~~WIP: Add start of state to CSRF cookie name~~ Add start of state to CSRF cookie name Mar 31, 2022

JoelSpeed reviewed Apr 22, 2022

View reviewed changes

miguelborges99 mentioned this pull request Jun 30, 2022

Add flags to define CSRF cookie expiration time and to allow CSRF cookies per request #1708

Merged

3 tasks

johnswarbrick mentioned this pull request Aug 2, 2022

CSRF validation fails (missing cookie) when user has bookmarked Keycloak login page #1736

Closed

github-actions bot added the Stale label Aug 28, 2022

Merge branch 'master' into enable-parallel-csrf

167d37c

JoelSpeed previously approved these changes Aug 29, 2022

View reviewed changes

JoelSpeed self-requested a review August 29, 2022 09:36

JoelSpeed removed the Stale label Aug 30, 2022

JoelSpeed closed this Aug 31, 2022

Uh oh!

Add start of state to CSRF cookie name #1504

Add start of state to CSRF cookie name #1504

Uh oh!

Conversation

YoavOst commented Jan 5, 2022

Description

How Has This Been Tested?

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazenaissa commented Mar 21, 2022

Uh oh!

BronzeDeer commented Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mazenaissa commented Apr 18, 2022

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

quantumdecoherence commented May 16, 2022

Uh oh!

tahoell commented May 17, 2022

Uh oh!

pfragoso commented Jun 22, 2022

Uh oh!

miguelborges99 commented Jun 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 28, 2022

Uh oh!

JoelSpeed commented Aug 29, 2022

Uh oh!

JoelSpeed commented Aug 29, 2022

Uh oh!

mazenaissa commented Aug 30, 2022

Uh oh!

WolfgangMehner commented Aug 31, 2022

Uh oh!

JoelSpeed commented Aug 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

BronzeDeer commented Apr 13, 2022 •

edited

Loading

miguelborges99 commented Jun 27, 2022 •

edited

Loading