-
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
Add Ed25519 support to JWT #343
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
1 similar comment
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@pwolanin this is great, thanks for adding this functionality! Are you willing to write some tests? If not I'll try to find time to add some, but we definitely need tests before merging this PR. |
@bshaffer can you approve the workflows so the tests run here? Yes, I can add some tests, but I'd like to be sure they will pass here |
@pwolanin I can approve to run the workflow once you've added tests. You haven't added any, so there's no reason to run them at this time... |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
1 similar comment
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Pushed a first test and start on doc. I expect when this runs it will fail on older PHP versions that will need the shim installed |
Fixed the test error in #344 |
$key = base64_decode(end($lines)); | ||
return sodium_crypto_sign_detached($msg, $key); | ||
} catch (Exception $e) { | ||
throw new DomainException($e->getMessage(), 0, $e); |
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.
Is there a reason (other than staying consistent with the other exceptions thrown from this method) that we are wrapping the libsodium exceptions in DomainException
?
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 for consistency and so we are not adding new exception types for BC. Also the other exception types may not be defined in PHP without the library or shim present, so anyone trying to write generic try/catch code will have a harder time.
Comparing to other PHP JWT libraries this is an important missing feature, and fairly easy to implement
e.g. see: https://github.com/lcobucci/jwt/blob/4.2.x/src/Signer/Eddsa.php
This can be suggested for people on old versions of php or without libsodium (default in php 7.2+)
https://github.com/paragonie/sodium_compat