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

Implement PS256/384/512 #1090

Closed
wants to merge 1 commit into from
Closed

Conversation

SvenRtbg
Copy link
Collaborator

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

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",
Copy link
Collaborator

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.

Copy link
Owner

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?

Copy link
Collaborator

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.

Copy link
Collaborator Author

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).

Copy link
Collaborator

@Ocramius Ocramius left a 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:

  1. people started using it
  2. 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.

@lcobucci
Copy link
Owner

Let's pull it to a separate lib, then, and see what the stats show us.

@SvenRtbg
Copy link
Collaborator Author

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?

@Ocramius
Copy link
Collaborator

@SvenRtbg given you are alway very active here, I would say that we could totally (pending @lcobucci's opinion, obviously) give you maintainership on the new repo.

@lcobucci
Copy link
Owner

I'll setup the new repo and give access to you folks. I'm just having limited time atm

@SvenRtbg
Copy link
Collaborator Author

SvenRtbg commented Nov 27, 2024 via email

@lcobucci
Copy link
Owner

lcobucci commented Dec 4, 2024

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!

@Slamdunk
Copy link
Collaborator

Slamdunk commented Dec 5, 2024

Moved to lcobucci/jwt-rsassa-pss#1

@SvenRtbg
Copy link
Collaborator Author

SvenRtbg commented Dec 5, 2024

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!

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PS256 algorithm support?
4 participants