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

feat: add defaultAlg param #426

Merged
merged 3 commits into from
May 13, 2022
Merged

feat: add defaultAlg param #426

merged 3 commits into from
May 13, 2022

Conversation

bshaffer
Copy link
Collaborator

@bshaffer bshaffer commented May 2, 2022

Add parameter to JWK::parseKeySet and JWK::parseKey to prevent "missing alg" error.

$defaultAlg = 'RS256'; // default algorithm used when "alg" isn't set
$jwks = JWK::parseKeySet($jwks, $defaultAlg);

See https://github.com/firebase/php-jwt/pull/376/files#r861654996

@bshaffer bshaffer mentioned this pull request May 2, 2022
4 tasks
@bshaffer
Copy link
Collaborator Author

bshaffer commented May 2, 2022

cc @Nextra

@Nextra
Copy link

Nextra commented May 5, 2022

This would certainly make using the parseKeySet easier without having to manually go through the key set before passing it over, which is enough for the Microsoft example I mentioned.

I have a question that I didn't quite find an obvious answer to in the code:

Prior to the change, did "kty": "RSA" always lead to RS256 being used, or was a JWK with just kty set able to "handle" all variants of the matching algorithm? I guess this boils down to why alg is marked optional in the first place, and if either the strict or the new defaultArg solution prevents a user of this library from utilizing (exploiting) a "feature" of the spec.

@bshaffer
Copy link
Collaborator Author

bshaffer commented May 5, 2022

@Nextra the value of kty has never been used to determine the algorithm, it's only ever been used to decode the public key. Prior to this change, the user was required to specify the "allowed algorithms" when calling JWT::decode in addition to supplying the keys array from the result of JWK::parseKeySet. So the user would have manually passed 'RSA256' in as an allowed algorithm. Since we've removed the allowed algorithms, we require each public key to be explicitly tied to an algorithm, so at the time the JWKs are parsed, we need to know their expected algorithm.

I am not sure how this could be exploited by anyone. I agree that it's strange that "alg" is optional in the first place, but I assume it's because many JWKS's have a known algorithm type, and so it's unnecessary to define it twice.

@Nextra
Copy link

Nextra commented May 6, 2022

The way I could see it as being useful is if one JWK were to be usable for decoding all variants of the respective algorithm, so passing ['RS256', 'RS384', 'RS512'] into the allowed algorithms with just one matching RSA JWK.

I don't know if that makes any sense.

@bshaffer
Copy link
Collaborator Author

We no longer use an array of "allowed algorithms", because the keys must be tied to an algorithm when they're used to verify the JWT. So to me, your suggestion does not make sense.

@bshaffer bshaffer merged commit d28e6df into main May 13, 2022
@bshaffer bshaffer deleted the add-alg-param branch May 13, 2022 20:54
@Nextra
Copy link

Nextra commented May 13, 2022

Thought as much. Thanks for the change!

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

Successfully merging this pull request may close these issues.

None yet

3 participants