-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(alg): Add ES512 support #566
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for your pull request! 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). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
I rebased the branch/resolved conflicts, can someone pls review? :-) |
composer.json
Outdated
| "require": { | ||
| "php": "^8.0" | ||
| "php": "^8.0", | ||
| "ext-openssl": "*" |
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.
Why would we require the extension? This would block existing users from upgrading who are not currently using openssl.
src/JWT.php
Outdated
| } elseif ($alg === 'ES384') { | ||
| $signature = self::signatureFromDER($signature, 384); | ||
| } elseif ($alg === 'ES512') { | ||
| $signature = self::signatureFromDER($signature, 512); |
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.
am I right in understanding this is the only real code change (other than adding ES512 to the list of supported algorithms)? If so, then this is a small enough change, but I would like to see some tests! See here for examples
Line 487 in 27179e1
| public function provideEncodeDecode() |
|
@Ninos are you still hoping to add this? If so please resolve the conflicts and respond to my questions! thank you. |
|
To be honest, I gave up after getting an empty openssl error & have no much requirements for this lib... Pushed my latest stage, if someone wants to fix, you're welcome :) The folder After that you should get following error: Best part of this, |
| $length = max(1, (int) (\strlen($sig) / 2)); | ||
| $length = match ($alg) { | ||
| 'ES512' => 66, | ||
| default => max(1, (int) (\strlen($sig) / 2)), |
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.
| default => max(1, (int) (\strlen($sig) / 2)), | |
| 'ES256', 'ES256K', 'ES384' => max(1, (int) (\strlen($sig) / 2)), | |
| default => throw new InvalidArgumentException('unknown ECDNA alg: ' . $alg), |
| * | ||
| * @param string $der binary signature in DER format | ||
| * @param int $keySize the number of bits in the key | ||
| * @param string $alg The 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.
$alg parameter here is never used
| * @param string $alg The algorithm |
| * @return string the signature | ||
| */ | ||
| private static function signatureFromDER(string $der, int $keySize): string | ||
| private static function signatureFromDER(string $der, int $keySize, string $alg): string |
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.
$alg parameter here is never used
| private static function signatureFromDER(string $der, int $keySize, string $alg): string | |
| private static function signatureFromDER(string $der, int $keySize): string |
ES512algorithm