feat: enable simultaneous auth flows by creating client related csrf co…

[aarmam]

This pull request enables simultaneous auth flows by creating client related csrf cookie names.

oauth2_authentication_csrf -> oauth2_authentication_csrf_%murmur3(client_id)% oauth2_authentication_csrf_insecure -> oauth2_authentication_csrf_%murmur3(client_id)%insecure oauth2_consent_csrf -> oauth2_consent_csrf%murmur3(client_id)% oauth2_consent_csrf_insecure -> oauth2_consent_csrf_%murmur3(client_id)%_insecure

Additionally max age is set for cookie using ttl.login_consent_request configuration property.

Related issue(s)

Fixes #3019

Checklist

• [x] I have read the contributing guidelines.
• [x] I have referenced an issue containing the design document if my change introduces a new feature.
• [x] I am following the contributing code guidelines.
• [x] I have read the security policy.
• [x] I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security. vulnerability, I confirm that I got green light (please contact [email protected]) from the maintainers to push the changes.
• [x] I have added tests that prove my fix is effective or that my feature works.
• [x] I have added or changed the documentation.

[codecov[bot]]

Codecov Report

Merging #3059 (3f9c538) into master (cda9fd4) will increase coverage by 0.04%. The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master    #3059      +/-   ##
==========================================
+ Coverage   76.84%   76.88%   +0.04%     
==========================================
  Files         123      123              
  Lines        8913     8920       +7     
==========================================
+ Hits         6849     6858       +9     
+ Misses       1636     1635       -1     
+ Partials      428      427       -1     

Impacted Files	Coverage Δ
consent/strategy_default.go	70.92% <75.00%> (+0.30%)	:arrow_up:
consent/helper.go	90.00% <100.00%> (+0.20%)	:arrow_up:
persistence/sql/persister_oauth2.go	82.88% <0.00%> (+0.76%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[sidharthramesh]

@aarmam any reason why hashing with murmur3 is necessary in this case? Does appending the client name directly cause any security issues?

[aarmam]

@aarmam any reason why hashing with murmur3 is necessary in this case? Does appending the client name directly cause any security issues?

Set-Cookie spec.

A can contain any US-ASCII characters except for: the control character, space, or a tab. It also must not contain a separator characters like the following: ( ) < > @ , ; : \ " / [ ] ? = { }.

Admin API, POST /clients endpoint allows such characters for Client ID.

[sidharthramesh]

In my use case there are multiple iframes - each a separate OAuth2 client executing the login flow simultaneously. So it’s different client executing the oauth flow simultaneously, always!

I understand your use case of users accidentally initiating multiple auth flows (maybe click the back button before a full redirect?) - but they are usually rare and with the same client, so this PR is unlikely to solve those use cases.

Regarding your solution of waiting for the flow to end before resetting the CSRF token might work for both situations if we make the season specific for user + client combination.

Using the state sounds like a good idea. It’s 8 digits long and the probability of collisions are sort of low? In case of collision maybe a fallback on some other method?

[aeneasr]

I guess the problem with state is that it is purely user-set and we have no control over the randomness. Using it as the basis for defensive measures introduced server-side may open up specific attack bectors that, depending on use case, could be severe.

[aarmam]

Thank you for these changes! On a high level, I think they look good. One question I have though is whether this is the right approach? Are there many use cases where multiple clients perform simoultaneously OAuth2 flows that a user is executing?

@aeneasr Our use case actually is silent refresh (prompt=none)

[mig5]

Are there many use cases where multiple clients perform simoultaneously OAuth2 flows that a user is executing

Each of my RPs uses a different oAuth2.0 client, and it only takes opening 2 such RPs in different tabs, which users seem to do all the time. These apps are automatically initiating the auth flow just on trying to load the front page, e.g the user is not clicking on a 'Login' button or anything like that which would 'skew' the timing in order to see the second tab 'auto log in' thanks to auth having completed via the first.

Example: https://github.com/vouch/vouch-proxy (auth_request in Nginx to force authentication of a site via the OP)

[aeneasr]

I see, thank you for the use case examples. That makes sense. If possible, I would like to use this PR to come up with a solution that works even better for simoultaneous OAuth2 flows from either distinct OAuth2 Clients, or even the same OAuth2 clients. I highly appreciate your input here, if you have ideas!

[sidharthramesh]

This PR is extremely useful for a lot of our use cases in its current state. We were using a very old fork of Hydra with our own local changes, but just tried this PR and everything works nicely. Would be really great if this feature gets merged!

[aeneasr]

You're right, also changing the cookie mechanism should not lead to many issues later on if we decide to change the CSRF approach, so this is certainly already an improvement worthy to be merged!

[sidharthramesh]

Hey @aarmam let me know if you need any help! Saw that you marked this as a draft.

[aarmam]

Hey @aarmam let me know if you need any help! Saw that you marked this as a draft.

Hey @sidharthramesh I will update all my pull requests next week and mark them ready for review again! Sorry to keep you waiting - will take this PR first! :)

[sidharthramesh]

Thank you @aarmam! I looked through your changes and also built the image and tried it out in our staging environment. I can confirm that we can have multiple oauth clients simultaneously logging on using iframes successfully!

@aeneasr looking forward to your review.

[aeneasr]

Hey, this looks pretty good I think! We're currently freezing any pushes to master as we're working on v2.x and have a tremendous amount of merge conflicts which we want to avoid. We're planning to not introduce new features to hydra v1.x branch for the time being, unless it's critical

[aeneasr]

You can help by making this PR against branch v2.x but I understand if that is too much work right now

[sidharthramesh]

wow 😄