Implement PS256/384/512 #1090
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.
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.
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.
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.
@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.
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. |
|
I don't feel I'd be initially involved there, am I? I don't like leaving half-finished work, however I for sure lack admin permissions to create anything within @lcobucci 's projects. :) How to proceed? |
|
I'll setup the new repo and give access to you folks. I'm just having limited time atm |
|
No need to rush, I'm AFK until Monday.
|
|
https://github.com/lcobucci/jwt-rsassa-pss was created and permissions were granted. I configured the very basic stuff (branch names, protection rules, keys for the auto-release stuff) and can help further another day! |
|
Moved to lcobucci/jwt-rsassa-pss#1 |
I'm eager to see what needs to be done next, having prepared an independent variant already - but it's just fragments still, no coherent project. |
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