Skip to content
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

Why is the 'alg' parameter required? #498

Closed
SamMousa opened this issue Apr 13, 2023 · 4 comments
Closed

Why is the 'alg' parameter required? #498

SamMousa opened this issue Apr 13, 2023 · 4 comments
Assignees

Comments

@SamMousa
Copy link

In 6.0 a change was introduced that made alg required when parsing keys.

At the time someone already noted that it was a breaking change: #376 (comment)

This is a breaking change that is currently not obvious from the 6.0 release notes.
As a notable example, Microsoft do not output JWK with the alg key populated:
https://login.microsoftonline.com/common/discovery/keys
I think the release notes should encourage developers to inspect JWK::parseKeySet beyond just its return type.
Thanks!

The solution was implemented in #426 where a default algorithm value could be set.

As far as I can tell no motivation for making this algorithm a required parameter was given. I'm working on an OIDC implementation and to me this feels like a very brittle approach, given that:

This means that in advance I cannot know what default algorithm to use. Sure, I could check the JWKS contents or the tokens I receive and see that it is RS256 today, but the whole point of this approach to configuration is that the other party can change it whenever they want. This means if Microsoft decides to switch to RS384, my production code will break.

Could you elaborate on why this requirement, which as is mentioned in the source code, is stricter than the spec was added?

@bshaffer
Copy link
Collaborator

bshaffer commented Jun 28, 2023

This is a good question, and is related to the resolution of GHSA-8xf4-w7qw-pjjw

Since that resolution, every key needs to have an associated algorithm. This seems like a reasonable request to me. However, in the case of Microsoft's JWKS, they're providing keys without the algorithm. To their credit, in the JWK spec the "alg" claim is indeed optional (although I cannot imagine why they'd want to leave it out). The quick solution here is to simply provide the known algorithm as $defaultAlg when calling JWK::parseKeySet:

$keys = JWK::parseKeySet($microsoftUrl, 'RS256');

To address your concern: If Microsoft updated their JWKS to accept RS384 and do not update the algorithms specified in their JWKS, this does not seem like our problem, but theirs. Doing this would break existing JWTs as well (anything RS256 would not be able to be verified using RS384 keys). The solution would be for them to provide a JWKS with both types of keys until a migration period was complete (and it was guaranteed that all previous RS256 keys are expired).

To support this kind of generation without matching the algorithms, and match keys based strictly on KID, bypassing the check that the keys are the same algorithm, could be done by allowing a wildcard algorithm:

$keyOrKeyArray = [
    new Key($key1, Key::ANY_ALG),
    new Key($key2, Key::ANY_ALG),
];

Then in the JWT::decode code, we would have something like:

// Check the algorithm
if (!self::constantTimeEquals($key->getAlgorithm(), $header->alg) && $key->getAlgorithm() !== Key::ANY_ALG) {
    // See issue #351
    throw new UnexpectedValueException('Incorrect key for this algorithm');
}

However, this type of solution would open the door to GHSA-8xf4-w7qw-pjjw again. I do not see how we could safely support this type of behavior.

Providing the default algorithm of the microsoft JKWS to JWK::parseKeySet is a reasonable solution, and I don't see any other way. If I'm missing something, please let me know, as I am certainly opening to safely solving this problem.

@bshaffer
Copy link
Collaborator

bshaffer commented Sep 8, 2023

Closing this issue as it's been over two months without receiving a response from the author. Please open another issue if you'd like to propose a way to make this work without introducing a security vulnerability.

@bshaffer bshaffer closed this as completed Sep 8, 2023
@SamMousa
Copy link
Author

I'm sorry, your first reply didn't ask for more information from me right?

@bshaffer
Copy link
Collaborator

@SamMousa only if you disagree with my decision to not support the feature and have a suggestion on how to implement it while addressing the concerns I've raised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants