Allow JWKSet with single key without kid set.

[MaPePeR]

Fixes SocialConnect/auth#139.

Sadly I couldn't find any tests for verifySignature to extend for this case, so this is untested.

[ghost]

any idea when this could be merged ?

[MaPePeR]

From my point of view the PR is finished and "ready to be merged" until further feedback. Maybe @ovr or @ADmad know more.

[ADmad]

I checked another popular JWT handling lib firebase/php-jwt and it too requires the kid to be specified regardless of how many keys the JWKS contains. https://github.com/firebase/php-jwt/blob/edda0f9ee45b8367699804f792a9be6d5175e816/src/JWT.php#L397-L407

Even though a JWKS might only have 1 key, the key could change in future and hence your JWT header must always have the kid. So that when the key in JWSK is changed the existing JWT using the older kid get invalidated and proper error message can be provided on auth failure.

So I don't think this change should be merged.

[MaPePeR]

I checked another popular JWT handling lib firebase/php-jwt and it too requires the kid to be specified regardless of how many keys the JWKS contains. https://github.com/firebase/php-jwt/blob/edda0f9ee45b8367699804f792a9be6d5175e816/src/JWT.php#L397-L407

That check is only done if there is an array of keys and not a single key. See the first 3 lines in the method.

The JWK specification says:

The "kid" value is a case-sensitive string. Use of this member is OPTIONAL.

The JWT specification has no mention of kid at all.

Even though a JWKS might only have 1 key, the key could change in future and hence your JWT header must always have the kid. So that when the key in JWSK is changed the existing JWT using the older kid get invalidated and proper error message can be provided on auth failure.

So I don't think this change should be merged.

I agree, that, not having the kid set, is bad, because its needed to properly change a key. But that is a problem/decision of the identity provider, that we probably cannot fix in all identity providers out there.
If this would be an identity provider library that exposes a keyset and my proposal was to not set the kid when there is only a single key: I'd totally agree with you. That would be stupid.
But: That's not the case. This is a library that tries to interface with other (possibly non-ideal) implementations and configurations out there and therefore should not refuse to do something that is valid according to the specification.

[ADmad]

But that is a problem/decision of the identity provider, that we probably cannot fix in all identity providers out there.

Fair enough, we can't control what various providers do.

[ADmad]

@ovr The testsuite CI is not hooked to show the results for commit/PRs?

[ovr]

The testsuite CI is not hooked to show the results for commit/PRs?

You are right! I forget to connect this repo to GitLab CI as an external repo. I migrated this repo to GitHub actions. We need to rebase this PR to affect changes. Can you please rebase your PR @MaPePeR?

Thanks

[ADmad]

@ovr You need to allow the workflow to run. I don't have write perms on this repo 🙂.

[ovr]

@ADmad Done, I've added you as a collaborator to this repository to allow that too. I'm sorry for late reply.

[ADmad]

@ovr Should I do a new minor release?

[ovr]

@ADmad yeah, minor or path. It's really hard to classify this change.

[ADmad]

It's a feature addition IMO and since the public API has changed a new minor 1.3.0 would be better.