-
-
Notifications
You must be signed in to change notification settings - Fork 601
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
base: 5.5.x
Are you sure you want to change the base?
Implement PS256/384/512 #1090
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 GitHub Actions / Mutation tests (locked, 8.2, ubuntu-latest)
Check warning on line 25 in src/Signer/RsaPss.php GitHub Actions / Mutation tests (locked, 8.2, ubuntu-latest)
|
||
} | ||
|
||
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 GitHub Actions / Mutation tests (locked, 8.2, ubuntu-latest)
|
||
|
||
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 GitHub Actions / Mutation tests (locked, 8.2, ubuntu-latest)
Check warning on line 52 in src/Signer/RsaPss.php GitHub Actions / Mutation tests (locked, 8.2, ubuntu-latest)
|
||
} | ||
|
||
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; | ||
} |
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'; | ||
} | ||
} |
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'; | ||
} | ||
} |
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'; | ||
} | ||
} |
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.
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.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.
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?
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.
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:
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.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.
@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).