Skip to content

Conversation

@bshaffer
Copy link
Collaborator

@bshaffer bshaffer commented Nov 10, 2021

@google-cla google-cla bot added the cla: yes label Nov 10, 2021
Comment on lines +50 to +58
if (isset($v['alg'])) {
$keys[$kid] = new Key($key, $v['alg']);
} else {
// The "alg" parameter is optional in a KTY, but is required
// for parsing in this library. Add it manually to your JWK
// array if it doesn't already exist.
// @see https://datatracker.ietf.org/doc/html/rfc7517#section-4.4
throw new InvalidArgumentException('JWK key is missing "alg"');
}
Copy link

@Nextra Nextra Apr 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @bshaffer

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just updated the release notes, @Nextra, thanks for the feedback.

I think it would be a better user-experience to have a second optional argument $algorithm to parseKeySet, to help with this edgecase.

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

I didn't initially add it because I was hoping in practice that most JWKS would be populating the alg parameter. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #426

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4 participants