-
-
Notifications
You must be signed in to change notification settings - Fork 98
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
Take JWK alg into account during key selection #131
Conversation
Without this change, there may be multiple compatible keys, which causes an AmbiguousKeyId error. Using the key algorithm to narrow the list down further fixes the issue. We've seen this edge case in the wild on a specific OpenAM instance.
Hey, thanks for the PR. According to the spec, this issue should never arise:
I'm hesitant to remove this Is this issue popping up with a popular OIDC provider? |
Hey! thanks for the review I'm very sorry for the lack of context of my initial PR, I should have done better
No requirement is removed, as a kid is indeed always provided: it's just not unique, there are multiple keys with the same kid but a different alg field. The specification gives an example for key types, which is correctly handled by the current code: you can have an RSA and an EC key with the same This is an optional setting of OpenAM, called [
{
"kty": "RSA",
"kid": "eGxIf8PmOKJeqN/jPAj85u6LMB0=",
"use": "sig",
"n": "rV7qnTenVeZwYP4m0ji5xbsFvqNRTW8-i0Ys98oBVKlWJpEXdPUQMBi4TdjHPUBRmueW74v1v8Uw-1NeE8WvI0bStvH7P2zxeP5bdL6onVNIZUdb1L1lkBjlYQP30TtsRZuJ2d-vmf3BKEvtd3V47A8gSuAJO9q8dT-Rby7ZOMWw_ZU_dJTIGplhgpJlQMXi3wLZyHU-oi7V5PmRE0ZYEn0LLXtQXQj1bYW-5AjU6TykXQVqISqImGiONpnKQYkOgZ56vXR9nU-_ZSmyc_VTBTnA0Xwj_aWfOokaFqft0LhH1gykhq9IIgHaxo55SqRm4lymxx13Hpe1lA3BlWzWVQ",
"e": "AQAB",
"alg": ["RS512", "PS256"]
}
] Of course, the example above is not compliant, as alg isn't a list of allowed algorithms, so it has to be de-normalized: [
{
"kty": "RSA",
"kid": "eGxIf8PmOKJeqN/jPAj85u6LMB0=",
"use": "sig",
"n": "rV7qnTenVeZwYP4m0ji5xbsFvqNRTW8-i0Ys98oBVKlWJpEXdPUQMBi4TdjHPUBRmueW74v1v8Uw-1NeE8WvI0bStvH7P2zxeP5bdL6onVNIZUdb1L1lkBjlYQP30TtsRZuJ2d-vmf3BKEvtd3V47A8gSuAJO9q8dT-Rby7ZOMWw_ZU_dJTIGplhgpJlQMXi3wLZyHU-oi7V5PmRE0ZYEn0LLXtQXQj1bYW-5AjU6TykXQVqISqImGiONpnKQYkOgZ56vXR9nU-_ZSmyc_VTBTnA0Xwj_aWfOokaFqft0LhH1gykhq9IIgHaxo55SqRm4lymxx13Hpe1lA3BlWzWVQ",
"e": "AQAB",
"alg": "RS512"
},
{
"kty": "RSA",
"kid": "eGxIf8PmOKJeqN/jPAj85u6LMB0=",
"use": "sig",
"n": "rV7qnTenVeZwYP4m0ji5xbsFvqNRTW8-i0Ys98oBVKlWJpEXdPUQMBi4TdjHPUBRmueW74v1v8Uw-1NeE8WvI0bStvH7P2zxeP5bdL6onVNIZUdb1L1lkBjlYQP30TtsRZuJ2d-vmf3BKEvtd3V47A8gSuAJO9q8dT-Rby7ZOMWw_ZU_dJTIGplhgpJlQMXi3wLZyHU-oi7V5PmRE0ZYEn0LLXtQXQj1bYW-5AjU6TykXQVqISqImGiONpnKQYkOgZ56vXR9nU-_ZSmyc_VTBTnA0Xwj_aWfOokaFqft0LhH1gykhq9IIgHaxo55SqRm4lymxx13Hpe1lA3BlWzWVQ",
"e": "AQAB",
"alg": "PS256"
}
]
I doubt this change could introduce key confusion issues: it makes key selection stricter by honoring the key's
It depends on what you mean by provider:
|
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.
thanks for the additional explanation! this change makes sense now and seems consistent with the standards.
@ramosbugs things should be better now. A few things worth mentioning:
Thanks a lot for your review, it's been amazingly smooth so far 👍 |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #131 +/- ##
==========================================
- Coverage 51.57% 51.56% -0.02%
==========================================
Files 16 16
Lines 2059 2075 +16
==========================================
+ Hits 1062 1070 +8
- Misses 997 1005 +8
☔ View full report in Codecov by Sentry. |
Thanks! I'll probably cut a release in the next week or so, once I have a chance to review and merge #130. |
This is now released in 3.4.0! |
PR #131 introduced the `jwk-alg` feature to avoid introducing a breaking change into 3.x. In preparation for the 4.0 major release, the feature flag is removed, and its functionality is enabled by default. BREAKING CHANGES: - `jwk-alg` is no longer a valid feature name - The `CoreJsonWebKey` type will now deserialize the optional `alg` field. If a JWK contains an unrecognized algorithm, deserialization may fail. - If a JWK contains an `alg` field that is incompatible with the signing algorithm specified in a JWT's JOSE header's `alg` field, signature verification will fail (as a security measure against key confusion attacks).
Without this change, there may be multiple compatible keys, which causes an AmbiguousKeyId error. Using the key algorithm to narrow the list down further fixes the issue.
We've seen this edge case in the wild on a specific OpenAM instance.