Skip to content

Conversation

@Ninos
Copy link

@Ninos Ninos commented May 25, 2024

  • Added: ES512 algorithm
  • CS: strict types & phpdoc

@google-cla
Copy link

google-cla bot commented May 25, 2024

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.

@Ninos
Copy link
Author

Ninos commented Oct 9, 2024

I rebased the branch/resolved conflicts, can someone pls review? :-)

composer.json Outdated
"require": {
"php": "^8.0"
"php": "^8.0",
"ext-openssl": "*"
Copy link
Collaborator

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);
Copy link
Collaborator

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

public function provideEncodeDecode()

@bshaffer
Copy link
Collaborator

@Ninos are you still hoping to add this? If so please resolve the conflicts and respond to my questions! thank you.

@Ninos
Copy link
Author

Ninos commented Jan 27, 2026

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 ./tests/examples/ can be removed after, also the container (podman/docker) part, if not welcomed. To test, run following:
php tests/examples/jwtTest.php ES512

After that you should get following error:

PHP Fatal error:  Uncaught DomainException: OpenSSL error:  in /srv/app/src/JWT.php:327
Stack trace:
#0 /srv/app/src/JWT.php(150): Firebase\JWT\JWT::verify()
#1 /srv/app/tests/examples/jwtTest.php(37): Firebase\JWT\JWT::decode()
#2 {main}
  thrown in /srv/app/src/JWT.php on line 327

Best part of this, \openssl_error_string() returns empty string :-)

$length = max(1, (int) (\strlen($sig) / 2));
$length = match ($alg) {
'ES512' => 66,
default => max(1, (int) (\strlen($sig) / 2)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Collaborator

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

Suggested change
* @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
Copy link
Collaborator

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

Suggested change
private static function signatureFromDER(string $der, int $keySize, string $alg): string
private static function signatureFromDER(string $der, int $keySize): string

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants