-
-
Notifications
You must be signed in to change notification settings - Fork 949
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
fix: IDToken nonce should not be checked (PS-385) #3994
base: master
Are you sure you want to change the base?
Conversation
Not sure I understand the reason here. What issue does this solve? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3994 +/- ##
==========================================
+ Coverage 78.10% 78.12% +0.01%
==========================================
Files 362 362
Lines 25329 25313 -16
==========================================
- Hits 19783 19775 -8
+ Misses 4028 4022 -6
+ Partials 1518 1516 -2 ☔ View full report in Codecov by Sentry. |
Hi Jonas!
|
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.
There are two nonces:
- The nonce we keep in our application and send as verification to Ory Kratos
- The nonce the upstream server send in the ID token
Since the second nonce (the user-provided value) is in a signed ID token, and the first nonce comes (most likely) from you initializing the OIDC call, the verification very much makes sense.
Nonce checking only makes sense for party which generates the nonce. And the client libs out there do not necessarily provide nonce value that was originally generated. Client-side code can take it only from the received idtoken. |
By using this endpoint you delegate "Nonce checking only makes sense for party which generates the nonce." to Ory Kratos. Therefore, in my view, it is totally legitimate for Ory Kratos to do the heavy lifting of giving it's client the ability to verify the nonce it generated against the verified claims of the ID token. If no nonce is availalbe, omit it and it will be ignored |
If no generated nonce is available we still need to send in the |
The nonce in the ID Token is only set if the OIDC request included a nonce ( |
We're looking into a solution for this now! |
After careful review, it appears that some SDKs do not correctly handle the nonce (don't return it) or even return the same token multiple times. So I think this PR is valid, we'll just have to see if we disable it completely or on a provider-per-provider basis. |
We have exchanged emails in april with the same vulnerability description but it was impossible to make you consider this important so I gave up. Simply removing the nonce check does not address the elephant in the room. The nonce is still useless as it is right now, and @splaunov is absolutely right: it gives "a false sense of security. We might be thinking that we are protected from replay attacks but we are not". If you are confident enough to assume that your users' ID tokens will never get leaked (by any mean such as accidental server-side logging, local network breach, etc.) I still think it's better just to drop the nonce support altogether, to reduce cognitive and maintenance burden. Below are the emails I sent.
|
This PR deletes code which checks nonce in passed OIDC IDToken against another string passed in the same request.
Replay protection cannot be based on comparing two strings taken from the same request.
Related issue(s)
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further Comments