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

Open
wants to merge 1 commit into
base: 5.5.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -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
Contributor 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).

"psr/clock": "^1.0"
},
"require-dev": {
Expand Down
229 changes: 228 additions & 1 deletion composer.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

72 changes: 72 additions & 0 deletions src/Signer/RsaPss.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
<?php
declare(strict_types=1);

namespace Lcobucci\JWT\Signer;

use Lcobucci\JWT\Signer;
use phpseclib3\Crypt\PublicKeyLoader;
use phpseclib3\Crypt\RSA;
use phpseclib3\Crypt\RSA\PrivateKey;
use phpseclib3\Crypt\RSA\PublicKey;
use phpseclib3\Exception\NoKeyLoadedException;

use function assert;
use function is_string;

abstract class RsaPss implements Signer
{
private const MINIMUM_KEY_LENGTH = 2048;

final public function sign(string $payload, Key $key): string
{
try {
$private = PublicKeyLoader::loadPrivateKey($key->contents(), $key->passphrase());
} catch (NoKeyLoadedException $e) {
throw new InvalidKeyProvided('It was not possible to parse your key, reason: ' . $e->getMessage());

Check warning on line 25 in src/Signer/RsaPss.php

View workflow job for this annotation

GitHub Actions / Mutation tests (locked, 8.2, ubuntu-latest)

Escaped Mutant for Mutator "Concat": @@ @@ try { $private = PublicKeyLoader::loadPrivateKey($key->contents(), $key->passphrase()); } catch (NoKeyLoadedException $e) { - throw new InvalidKeyProvided('It was not possible to parse your key, reason: ' . $e->getMessage()); + throw new InvalidKeyProvided($e->getMessage() . 'It was not possible to parse your key, reason: '); } if (!$private instanceof PrivateKey) { throw InvalidKeyProvided::incompatibleKeyType('RSA', $private::class);

Check warning on line 25 in src/Signer/RsaPss.php

View workflow job for this annotation

GitHub Actions / Mutation tests (locked, 8.2, ubuntu-latest)

Escaped Mutant for Mutator "ConcatOperandRemoval": @@ @@ try { $private = PublicKeyLoader::loadPrivateKey($key->contents(), $key->passphrase()); } catch (NoKeyLoadedException $e) { - throw new InvalidKeyProvided('It was not possible to parse your key, reason: ' . $e->getMessage()); + throw new InvalidKeyProvided('It was not possible to parse your key, reason: '); } if (!$private instanceof PrivateKey) { throw InvalidKeyProvided::incompatibleKeyType('RSA', $private::class);
}

if (! $private instanceof PrivateKey) {
throw InvalidKeyProvided::incompatibleKeyType('RSA', $private::class);
}

if ($private->getLength() < self::MINIMUM_KEY_LENGTH) {
throw InvalidKeyProvided::tooShort(self::MINIMUM_KEY_LENGTH, $private->getLength());
}

$signature = $private
->withPadding(RSA::SIGNATURE_PSS)
->withHash($this->algorithm())
->withMGFHash($this->algorithm())
->sign($payload);

assert(is_string($signature) && $signature !== '');

Check warning on line 42 in src/Signer/RsaPss.php

View workflow job for this annotation

GitHub Actions / Mutation tests (locked, 8.2, ubuntu-latest)

Escaped Mutant for Mutator "LogicalAnd": @@ @@ throw InvalidKeyProvided::tooShort(self::MINIMUM_KEY_LENGTH, $private->getLength()); } $signature = $private->withPadding(RSA::SIGNATURE_PSS)->withHash($this->algorithm())->withMGFHash($this->algorithm())->sign($payload); - assert(is_string($signature) && $signature !== ''); + assert(is_string($signature) || $signature !== ''); return $signature; } final public function verify(string $expected, string $payload, Key $key): bool

return $signature;
}

final public function verify(string $expected, string $payload, Key $key): bool
{
try {
$public = PublicKeyLoader::loadPublicKey($key->contents());
} catch (NoKeyLoadedException $e) {
throw new InvalidKeyProvided('It was not possible to parse your key, reason: ' . $e->getMessage());

Check warning on line 52 in src/Signer/RsaPss.php

View workflow job for this annotation

GitHub Actions / Mutation tests (locked, 8.2, ubuntu-latest)

Escaped Mutant for Mutator "Concat": @@ @@ try { $public = PublicKeyLoader::loadPublicKey($key->contents()); } catch (NoKeyLoadedException $e) { - throw new InvalidKeyProvided('It was not possible to parse your key, reason: ' . $e->getMessage()); + throw new InvalidKeyProvided($e->getMessage() . 'It was not possible to parse your key, reason: '); } if (!$public instanceof PublicKey) { throw InvalidKeyProvided::incompatibleKeyType('RSA', $public::class);

Check warning on line 52 in src/Signer/RsaPss.php

View workflow job for this annotation

GitHub Actions / Mutation tests (locked, 8.2, ubuntu-latest)

Escaped Mutant for Mutator "ConcatOperandRemoval": @@ @@ try { $public = PublicKeyLoader::loadPublicKey($key->contents()); } catch (NoKeyLoadedException $e) { - throw new InvalidKeyProvided('It was not possible to parse your key, reason: ' . $e->getMessage()); + throw new InvalidKeyProvided('It was not possible to parse your key, reason: '); } if (!$public instanceof PublicKey) { throw InvalidKeyProvided::incompatibleKeyType('RSA', $public::class);
}

if (! $public instanceof PublicKey) {
throw InvalidKeyProvided::incompatibleKeyType('RSA', $public::class);
}

return $public
->withPadding(RSA::SIGNATURE_PSS)
->withHash($this->algorithm())
->withMGFHash($this->algorithm())
->verify($payload, $expected);
}

/**
* Returns which algorithm to be used to create/verify the signature (using phpseclib hash identifiers)
*
* @internal
*/
abstract public function algorithm(): string;
}
19 changes: 19 additions & 0 deletions src/Signer/RsaPss/Sha256.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?php
declare(strict_types=1);

namespace Lcobucci\JWT\Signer\RsaPss;

use Lcobucci\JWT\Signer\RsaPss;

final class Sha256 extends RsaPss
{
public function algorithmId(): string
{
return 'PS256';
}

public function algorithm(): string
{
return 'sha256';
}
}
19 changes: 19 additions & 0 deletions src/Signer/RsaPss/Sha384.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?php
declare(strict_types=1);

namespace Lcobucci\JWT\Signer\RsaPss;

use Lcobucci\JWT\Signer\RsaPss;

final class Sha384 extends RsaPss
{
public function algorithmId(): string
{
return 'PS384';
}

public function algorithm(): string
{
return 'sha384';
}
}
19 changes: 19 additions & 0 deletions src/Signer/RsaPss/Sha512.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?php
declare(strict_types=1);

namespace Lcobucci\JWT\Signer\RsaPss;

use Lcobucci\JWT\Signer\RsaPss;

final class Sha512 extends RsaPss
{
public function algorithmId(): string
{
return 'PS512';
}

public function algorithm(): string
{
return 'sha512';
}
}
Loading
Loading