-
-
Notifications
You must be signed in to change notification settings - Fork 601
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
Implement PS256/384/512 #1090
base: 5.5.x
Are you sure you want to change the base?
Implement PS256/384/512 #1090
Conversation
The heavy work is offloaded to phpseclib/phpseclib v3, which is added as a dependency.
@@ -20,6 +20,7 @@ | |||
"php": "~8.2.0 || ~8.3.0 || ~8.4.0", | |||
"ext-openssl": "*", | |||
"ext-sodium": "*", | |||
"phpseclib/phpseclib": "^3.0.36", |
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.
Here I disagree with @lcobucci #1074 (comment)
While phpseclib/phpseclib
is indeed good, I don't like adding that entire dependency for such and uncommon and superseded signature algorithm.
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.
We can disagree, and I totally see your point (specially for this use case).
I don't like adding dependencies but, sometimes, they can help us going further with reduced effort.
Can we quantify things to have a more complete analysis? What are the pros/cons? How much is the lib footprint growing with each approach? How is our maintenability affected?
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.
My concern is that as a user I have no pratical way to tell if a flaw or security issue on the dependency directly affects (or not) the part of the main library I use.
(Native extensions have been less affected by this problem so far.)
Forcing every user to install such a huge dependency for something they will likely not use is annoying:
"replace": {
"symfony/polyfill-ctype": "*",
"symfony/polyfill-iconv": "*",
"symfony/polyfill-intl-grapheme": "*",
"symfony/polyfill-intl-idn": "*",
"symfony/polyfill-intl-normalizer": "*",
"symfony/polyfill-mbstring": "*",
"symfony/polyfill-php70": "*",
"symfony/polyfill-php72": "*",
"symfony/polyfill-php73": "*",
"symfony/polyfill-php80": "*",
"symfony/polyfill-php81": "*",
"symfony/polyfill-php82": "*",
"symfony/polyfill-php83": "*"
},
I have no scientific threshold of (dep weight) to (benefit gained) ratio to respect.
But I can tell you this is the exact case I'd publish a separate jwt-rssassa-pss
package for.
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.
@Slamdunk I wonder what that list of replacements is supposed to say. It does not seem to be anything that is added by PhpSecLib.
Anyways, so far I had some fun doing hands-on work, and I admit I'm not the person promoting this schema, and I'm not pushing it getting merged - so whatever decision is made is fine by me.
Keep in mind though, that some keywords for longstanding feature requests do appear in PhpSecLib, like JWK. And in fact the documentation explicitly uses JWT as an example for signing the tokens, although in quite a low-level approach. It could power all schemas that are to offer here, maybe without Blake2B.
If you decide to move on, I'll start fighting the mutants next week (which didn't appear locally - I was running make
, no clue what happens behind the scene for now, but will look into it).
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.
Before proceeding with this, I am noticing one interesting (and very good!) detail about this patch: it is all added files only.
I'm wondering if such a patch could sit in lcobucci/jwt-rsassa-pss
instead.
Such an approach would fully externalize the dependency, and could be re-absorbed in the main lcobucci/jwt
library after:
- people started using it
- upstream PHP has bindings needed for this to work without an added dependency (in SSL or
ext-sodium
)
I fully agree with @Slamdunk that adding a dependency here is a problem.
Let's pull it to a separate lib, then, and see what the stats show us. |
The heavy work is offloaded to phpseclib/phpseclib v3, which is added as a dependency.
Note: Please focus review onto the fact that everyone states that RSASSA-PSS key pairs are somehow special.
My understanding is that PSS is just a different kind of padding that utilizes randomness as a salt, and the signing part is just basic RSA. I have tested with dedicated RSA-PSS key pairs, and the only difference is that the key is explicitly labeled as RSA-PSS, and may contain additional info about the expected hash, mgfhash and salt length. It wasn't noticed by the PHPSecLib implementation, though.
Keep in mind I might have missed an important point here, as I implemented the obvious part, and maybe some non-obvious things, but I wouldn't consider myself the expert here.
Most importantly, I would like to see someone testing against a real-world token use case, as the tests inside are basically verifying that the implementation in the class matches the implementation in the test, which is effectively the same code.
closes #1074