Why is the 'alg' parameter required?

[SamMousa]

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: https://github.com/firebase/php-jwt/pull/376#discussion_r861654996

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:

• OIDC relies on discovery documents (https://login.microsoftonline.com/markteffect.nl/v2.0/.well-known/openid-configuration)
• OIDC relies on a jwks_uri configured in those documents, for example (https://login.microsoftonline.com/fd227a0d-5bcf-4e54-98c7-46a67c8cab14/discovery/v2.0/keys)

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]

This is a good question, and is related to the resolution of https://github.com/advisories/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 https://github.com/advisories/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]

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.

[SamMousa]

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

[bshaffer]

@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.