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

Support for JSON Web Key Sets #505

Merged
merged 3 commits into from
Jan 6, 2022
Merged

Support for JSON Web Key Sets #505

merged 3 commits into from
Jan 6, 2022

Conversation

swiffer
Copy link
Contributor

@swiffer swiffer commented Dec 26, 2021

Solution for #504

  • The way secretKey is currently used to built a Key and decode the token does not allow JWKS
  • A different configuration key jsonWebKeySet has been introduced to not dilute the meaning of the secretKey

Let me know what you think

@swiffer
Copy link
Contributor Author

swiffer commented Dec 26, 2021

For now removed guidance of caching / storing the public key as this can lead to unexpected behaviour when keys are rotated.

Auth0 has some guidance regards caching:

https://community.auth0.com/t/caching-jwks-signing-key/17654/2

Unclear currently if this is something that should be supported in php-jwt, here or within the app. Guess it would be best to have it in firebase/php-jwt and beeing able to configure it.

Opened a Issue there:

firebase/php-jwt#384

docs/en/authenticators.rst Outdated Show resolved Hide resolved
docs/en/authenticators.rst Outdated Show resolved Hide resolved
docs/en/authenticators.rst Show resolved Hide resolved
docs/en/authenticators.rst Outdated Show resolved Hide resolved
docs/en/authenticators.rst Outdated Show resolved Hide resolved
src/Authenticator/JwtAuthenticator.php Show resolved Hide resolved
tests/Data/rsa-jwkset.json Outdated Show resolved Hide resolved
@swiffer
Copy link
Contributor Author

swiffer commented Dec 27, 2021

@ADmad / @markstory - thanks for you reviews - added a test and made several improvements (see comments)

src/Authenticator/JwtAuthenticator.php Outdated Show resolved Hide resolved
tests/Data/rsa-jwkset.json Outdated Show resolved Hide resolved
tests/TestCase/Authenticator/JwtAuthenticatorTest.php Outdated Show resolved Hide resolved
tests/TestCase/Authenticator/JwtAuthenticatorTest.php Outdated Show resolved Hide resolved
@ADmad ADmad changed the base branch from 2.x to 2.next December 27, 2021 15:20
@othercorey
Copy link
Member

@swiffer looks like the target branch was change to 2.next. Can you rebase the PR?

@ADmad
Copy link
Member

ADmad commented Dec 28, 2021

looks like the target branch was change to 2.next

Thus is a non trivial feature addition, so better to target the next minor release, hence I changed it to 2.next.

@swiffer
Copy link
Contributor Author

swiffer commented Dec 30, 2021

Can we get it merged or is something missing? :)

@@ -0,0 +1,27 @@
-----BEGIN RSA PRIVATE KEY-----
Copy link
Member

Choose a reason for hiding this comment

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

Is this file used? I don't see it referenced in this diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not used directly at the moment. But as the fake JSON response is containing a key set with two public keys I thought it's helpful to also include both private keys... Should I remove it?

Copy link
Member

Choose a reason for hiding this comment

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

No that's ok.

@markstory markstory merged commit 71cfa8a into cakephp:2.next Jan 6, 2022
@othercorey
Copy link
Member

@swiffer Nice work!

@swiffer
Copy link
Contributor Author

swiffer commented Jan 6, 2022

Thanks! Hopefully useful for others as well. Looking forward to the next release :)

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

4 participants