Take JWK alg into account during key selection

[multun]

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.

[ramosbugs]

Hey, thanks for the PR. According to the spec, this issue should never arise:

If there are multiple keys in the referenced JWK Set document, a kid value MUST be provided in the JOSE Header.

I'm hesitant to remove this MUST requirement in case there are clever ways for an attacker to take advantage of key confusion. This is similar to past high-profile JWT vulnerabilities that allowed attackers to change the alg value in a JWT to HS256 to coerce a library into treating an RSA public key as as a symmetric secret. I don't think this particular change will introduce security issues, but the reason standards exist is so that individual developers don't have to guess about these things without review from other experts.

Is this issue popping up with a popular OIDC provider?

[multun]

Hey! thanks for the review

I'm very sorry for the lack of context of my initial PR, I should have done better

I'm hesitant to remove this MUST requirement in case there are clever ways for an attacker to take advantage of key confusion.

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. kids SHOULD be unique but are explicitly allowed not to.

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 kid, and openidconnect-rs will pick the right one just fine. Another such case which is already handled by the current code is key usage: you could have two keys with the same kid, one for signing, and one for encryption. I'd argue that honoring the alg field isn't different.

This is an optional setting of OpenAM, called Include all kty and alg combinations in jwk_uri and it's there for a good reason: It's designed to enforce a list of allowed algorithms per key, which enables deprecating vulnerable algorithms:

[
    {
        "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"
    }
]

This is similar to past high-profile JWT vulnerabilities that allowed attackers to change the alg value in a JWT to HS256 to coerce a library into treating an RSA public key as as a symmetric secret. I don't think this particular change will introduce security issues, but the reason standards exist is so that individual developers don't have to guess about these things without review from other experts.

I doubt this change could introduce key confusion issues: it makes key selection stricter by honoring the key's alg field.

Is this issue popping up with a popular OIDC provider?

It depends on what you mean by provider:

• The software used on the server side is OpenAM, which I believe counts as popular
• This OP instance is public but meant for internal use (the only way to get an account is to work for the company)

[multun]

@ramosbugs things should be better now. A few things worth mentioning:

• One of the tests used a key management algorithm in a key
• I retained the check_key_compatibility function, as it's used both when filtering keys and when verifying signatures

Thanks a lot for your review, it's been amazingly smooth so far :+1:

[codecov[bot]]

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (b336c20) 51.57% compared to head (c871b0b) 51.56%. Report is 1 commits behind head on main.

Additional details and impacted files

```diff @@ 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 ``` | [Files](https://app.codecov.io/gh/ramosbugs/openidconnect-rs/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Ramos) | Coverage Δ | | |---|---|---| | [src/core/mod.rs](https://app.codecov.io/gh/ramosbugs/openidconnect-rs/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Ramos#diff-c3JjL2NvcmUvbW9kLnJz) | `52.76% <ø> (ø)` | | | [src/lib.rs](https://app.codecov.io/gh/ramosbugs/openidconnect-rs/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Ramos#diff-c3JjL2xpYi5ycw==) | `47.61% <ø> (-1.06%)` | :arrow_down: | | [src/verification.rs](https://app.codecov.io/gh/ramosbugs/openidconnect-rs/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Ramos#diff-c3JjL3ZlcmlmaWNhdGlvbi5ycw==) | `76.13% <100.00%> (-0.37%)` | :arrow_down: | | [src/core/jwk.rs](https://app.codecov.io/gh/ramosbugs/openidconnect-rs/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Ramos#diff-c3JjL2NvcmUvandrLnJz) | `78.07% <87.50%> (-0.42%)` | :arrow_down: | | [src/types.rs](https://app.codecov.io/gh/ramosbugs/openidconnect-rs/pull/131?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=David+Ramos#diff-c3JjL3R5cGVzLnJz) | `68.36% <84.21%> (+0.47%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[ramosbugs]

Thanks! I'll probably cut a release in the next week or so, once I have a chance to review and merge #130.

[ramosbugs]

This is now released in 3.4.0!