-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: enable simultaneous auth flows by creating client related csrf co… #3059
Conversation
Codecov Report
@@ 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@aarmam any reason why hashing with murmur3 is necessary in this case? Does appending the client name directly cause any security issues? |
Admin API, POST /clients endpoint allows such characters for Client ID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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? |
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. |
@aeneasr |
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) |
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! |
3b6c96f
to
1357f3f
Compare
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! |
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two minor things in tests
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! :) |
1357f3f
to
77d4532
Compare
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 |
You can help by making this PR against branch v2.x but I understand if that is too much work right now |
f45b95a
to
14705ba
Compare
wow 😄 |
b101925
to
abbb095
Compare
4bd1e31
to
21b74b9
Compare
21b74b9
to
3f9c538
Compare
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
introduces a new feature.
contributing code guidelines.
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.
works.