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

Include Client ID (kid) parameter in the JWT header #932

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

BVMiko
Copy link

@BVMiko BVMiko commented May 22, 2018

Currently, Oauth2\Encryption\FirebaseJwt::encode has a different signature from OAuth2\Encryption\Jwt::encode; the FirebaseJwt function has an additional $keyId parameter. This parameter, when included, is passed into the kid parameter of the JWT header.

Unfortunately, neither of the two classes which use it (OAuth2\OpenID\ResponseType\IdToken and OAuth2\ResponseType\JwtAccessToken) include the parameter by default, so a lot of code is required to get this parameter added into the JWT header.

This PR modifies the signature of OAuth2\Encryption\EncryptionInterface::encode to include the $keyId parameter, thus standardizing between the two provided encryption implementations to offer a way to incorporate the kid parameter. It also modifies the two ResponseTypes which use the encode method to pass the $client_id.

There is also one more related issue included in this PR: modern versions of firebase/php-jwt use namespaces, while the references in Oauth2\Encryption\FirebaseJwt do not. I've modified this class to use the proper namespace parameters, and also upgraded the minimum suggestion of firebase/php-jwt package from ~2.2 (released 2015-06-22) to ~3.0 (released 2015-07-22).

@BVMiko
Copy link
Author

BVMiko commented May 22, 2018

I also want to reference to issue #363; though this PR doesn't offer a way to modify the JWT header with arbitrary data; it does automatically provide the kid parameter; which I think may solve the issue for most.

@menturion
Copy link

+1

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.

2 participants