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

• I have read the contributing guidelines.
• I have referenced an issue containing the design document if my change
  introduces a new feature.
• I am following the
  contributing code guidelines.
• I have read the security policy.
• 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
  security@ory.sh) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works.
• I have added or changed the documentation.

[codecov]

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%)	⬆️
consent/helper.go	90.00% <100.00%> (+0.20%)	⬆️
persistence/sql/persister_oauth2.go	82.88% <0.00%> (+0.76%)	⬆️

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.

[aeneasr]

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? What for sure is a problem is that a user might initiate two or more OAuth2 flows by accident, and then ends up in a situation where they can't proceed due to CSRF failures.

The underlying problem is that in Ory Hydra we regenerate the CSRF token regardless of whether the principal has changed or not, as we usually do not know whether the principal has changed in the first place.

In an optimal system, CSRF tokens are regenerated when the authentication session changes (logout, different user logs in) and then stay consistent for the remainder of the cookie lifetime. This helps prevent issues where a user loads two pages with forms and ends up in a race condition where the CSRF cookie from form B overrides the one from form A, making it impossible to submit form A without reloading it (hence setting a new cookie).

In Hydra the problem is that we don't really know what the principal is until we accepted the login challenge, however, by then, it is too late for us to set the CSRF token.

So I'm wondering whether there is a "better" solution to this problem? We could also, for example, set the CSRF cookie when the flow initializes and only reset it once the flow is completed successfully? That could still cause issues when performing multiple flows at the same time, but it would alleviate the use case I mentioned above:

What for sure is a problem is that a user might initiate two or more OAuth2 flows by accident, and then ends up in a situation where they can't proceed due to CSRF failures.

Another option I thought of is to set the CSRF cookie based on some other parameter, for example the state parameter as that still represents a unique identifier that could be used to track the flow from start to finish. But then again, the state parameter can be user-chosen and is not guaranteed to be collision free.

Looking forward to hear some ideas.

[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!

[aeneasr]

Just two minor things in tests

[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 😄