-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 cached keyset #397
Conversation
e40232e
to
9051308
Compare
From @swiffer in another issue:
I'm replying here to keep it in this thread for review. I do not have a reason for choosing PSR-6 over PSR-16. If there's a strong argument to supporting one over the other (or both) I'm all ears. Why is it difficult to implement in CakePHP? Does the minimum requirement of PHP 8.0 have something to do with it, or is it because you're using PSR-16 instead? |
cakephp/authentication 2.9.0 currently targets CakePHP 4.0 which requires >= PHP 7.2. this will be raised to PHP 7.4 in the next minor of CakePHP and PHP 8.0 in CakePHP 5.0. Therefore maybe it'll take some time until it can actually be used in the cakephp/authentication plugin but should work out. It's more that CakePHP only has a PSR 16 compatible cache implementation and introducing additional dependencies is increasing complexity. |
} | ||
} | ||
|
||
if (!isset($this->keySet[$keyId])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code is currently not proof against ddos attacks via key spamming, or is it?
this scenario was described here: https://github.com/auth0/node-jwks-rsa#rate-limiting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it does not. We can add this to the cache without too much issue, however. I'll take a look. Thanks for pointing this out.
I can cut a release with the minimum version at PHP 7.x (probably 7.1).
This is a design decision by CakePHP, and doesn't really indicate so much what would be better for this repo. So unless it's possible to support both (which would require PHP 8.0 union types), I'd prefer to stick with PSR-6 unless there's a better argument to switch. |
f9b9faa
to
7341c6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made a few minor suggestions.
Some might call them nitpicks - feel free to ignore them 😃
I would like to bump phpspec/prophecy-phpunit
to v2, though, but that would break support for PHP 7.1 and 7.2.
Maybe you want to consider dropping support for those versions by now anyway since they've been unsupported for a while.
That would remove the deprecation warnings from PHPUnit and would be as simple as bumping the version and adding a trait in tests/CachedKeySetTest.php
.
I can do that if you'd like.
Btw, I can see a bunch of old issues still being open.
Let me know if you like some help with those 🙂
Co-authored-by: Morten Hauberg-Lund <[email protected]>
Co-authored-by: Morten Hauberg-Lund <[email protected]>
Co-authored-by: Morten Hauberg-Lund <[email protected]>
@mortenhauberg Thanks for your review! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). For more information, open the CLA check for this pull request. As for dropping support for PHP 7.1 and 7.2, I cringe at the idea of leaving some developers in the lurch because of testing libraries. If I'm going to drop PHP support, I want a better reason than that. And truthfully, the very end of 2020 (When 7.2 dropped support) was less than a year and a half ago, which is not very long in the relatively slow moving world of PHP. |
@bshaffer CLA signed 👍🏻 I didn't necessarily mean for you to bump the supported versions now, and I do agree that doing it for the sake of a few tests is a bad reason. I can also see that you still have users stuck on older versions. #399 (comment) So I totally see your point 😊 Please let me know if there's anything else I can do |
Despite the fact that we're on PHP 7.0, I actually agree with dropping support for EOL versions of PHP. We shouldn't be using this version for many reasons, and if libraries don't drop support, then they're essentially enabling the use of potentially insecure software. This is even more relevant for a library dealing with aspects of security. Yes, backporting one or two features to 6.0.x would be useful to us, but that's ultimately "our problem" – in the case of the linked issue, we can either stick with RSA, or consider backporting it ourselves (with or without an official 6.0.x release, though that would be nice). |
…add-cached-keyset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor notes so far, still working through the core of the logic in the CacheKeySet and the tests.
Co-authored-by: David Supplee <[email protected]>
Co-authored-by: David Supplee <[email protected]>
Co-authored-by: David Supplee <[email protected]>
Co-authored-by: David Supplee <[email protected]>
…add-cached-keyset
see #384
Adds
CachedKeySet
to automatically cache and fetch from a JWK Set URI. Features:kid
is requested which doesn't exist (automatic key rotation)ratelimit
on invalid keys to ensure the JWKS URL is not spammed (default 10 requests per second when enabled)Usage: