From 8175de5b841fbe3fd97d2d49b3fc15c4ecb39a73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lu=C3=ADs=20Cobucci?= Date: Mon, 27 Sep 2021 15:36:39 +0200 Subject: [PATCH] Ensure key contents is used for all hashing algorithms On v3.4.0 we introduced the `Lcobucci\JWT\Signer\Key\LocalFileReference`, which was designed to be used with OpenSSL-based algorithms and avoid having to load the file contents into user-land to sign/verify tokens. However, other algorithms don't understand the scheme `file://` and will incorrectly use the file path as the signing key. This modifies and deprecates `LocalFileReference` to avoid that misleading behaviour. Co-authored-by: Anton Smirnov Co-authored-by: Marco Pivetta --- docs/configuration.md | 5 +-- docs/upgrading.md | 3 +- phpstan.neon.dist | 1 + src/Signer/Key/LocalFileReference.php | 8 +++- test/functional/HmacTokenTest.php | 40 +++++++++++++++++++ test/unit/Signer/EcdsaTest.php | 2 + .../Signer/Key/LocalFileReferenceTest.php | 26 ++++-------- test/unit/Signer/RsaTest.php | 4 ++ 8 files changed, 64 insertions(+), 25 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 3afcf67a..b0c9f05b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -41,10 +41,8 @@ Or provide a file path: ```php use Lcobucci\JWT\Signer\Key\InMemory; -use Lcobucci\JWT\Signer\Key\LocalFileReference; $key = InMemory::file(__DIR__ . '/path-to-my-key-stored-in-a-file.pem'); // this reads the file and keeps its contents in memory -$key = LocalFileReference::file(__DIR__ . '/path-to-my-key-stored-in-a-file.pem'); // this just keeps a reference to file ``` #### For symmetric algorithms @@ -78,13 +76,12 @@ This means that it's fine to distribute your **public key**. However, the **priv ```php use Lcobucci\JWT\Configuration; use Lcobucci\JWT\Signer; -use Lcobucci\JWT\Signer\Key\LocalFileReference; use Lcobucci\JWT\Signer\Key\InMemory; $configuration = Configuration::forAsymmetricSigner( // You may use RSA or ECDSA and all their variations (256, 384, and 512) new Signer\Rsa\Sha256(), - LocalFileReference::file(__DIR__ . '/my-private-key.pem'), + InMemory::file(__DIR__ . '/my-private-key.pem'), InMemory::base64Encoded('mBC5v1sOKVvbdEitdSBenu59nfNfhwkedkJVNabosTw=') // You may also override the JOSE encoder/decoder if needed by providing extra arguments here ); diff --git a/docs/upgrading.md b/docs/upgrading.md index 7610e457..fa070d8b 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -96,9 +96,8 @@ You can find more information on how to use the configuration object, [here](con ### Use new `Key` objects `Lcobucci\JWT\Signer\Key` has been converted to an interface in `v4.0`. -We provide two new implementations: `Lcobucci\JWT\Signer\Key\InMemory` and `Lcobucci\JWT\Signer\Key\LocalFileReference`. -`Lcobucci\JWT\Signer\Key\InMemory` is a drop-in replacement of the behaviour for `Lcobucci\JWT\Signer\Key` in `v3.x`. +We provide `Lcobucci\JWT\Signer\Key\InMemory`, a drop-in replacement of the behaviour for `Lcobucci\JWT\Signer\Key` in `v3.x`. You will need to pick the appropriated named constructor to migrate your code: ```diff diff --git a/phpstan.neon.dist b/phpstan.neon.dist index 0cd9727b..2d6cbacc 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -6,3 +6,4 @@ parameters: ignoreErrors: - '#.*OpenSSLAsymmetricKey.*#' + - '#Call to method .* of deprecated class Lcobucci\\JWT\\Signer\\Key\\LocalFileReference#' diff --git a/src/Signer/Key/LocalFileReference.php b/src/Signer/Key/LocalFileReference.php index 4663e9e2..b6cb3f12 100644 --- a/src/Signer/Key/LocalFileReference.php +++ b/src/Signer/Key/LocalFileReference.php @@ -9,12 +9,14 @@ use function strpos; use function substr; +/** @deprecated please use {@see InMemory} instead */ final class LocalFileReference implements Key { private const PATH_PREFIX = 'file://'; private string $path; private string $passphrase; + private string $contents; private function __construct(string $path, string $passphrase) { @@ -38,7 +40,11 @@ public static function file(string $path, string $passphrase = ''): self public function contents(): string { - return self::PATH_PREFIX . $this->path; + if (! isset($this->contents)) { + $this->contents = InMemory::file($this->path)->contents(); + } + + return $this->contents; } public function passphrase(): string diff --git a/test/functional/HmacTokenTest.php b/test/functional/HmacTokenTest.php index d87771a6..a7a9a34a 100644 --- a/test/functional/HmacTokenTest.php +++ b/test/functional/HmacTokenTest.php @@ -7,12 +7,17 @@ use Lcobucci\JWT\Signer\Hmac\Sha256; use Lcobucci\JWT\Signer\Hmac\Sha512; use Lcobucci\JWT\Signer\Key\InMemory; +use Lcobucci\JWT\Signer\Key\LocalFileReference; use Lcobucci\JWT\Token; use Lcobucci\JWT\Validation\Constraint\SignedWith; use Lcobucci\JWT\Validation\RequiredConstraintsViolated; use PHPUnit\Framework\TestCase; use function assert; +use function file_put_contents; +use function is_string; +use function sys_get_temp_dir; +use function tempnam; /** * @covers \Lcobucci\JWT\Configuration @@ -131,4 +136,39 @@ public function everythingShouldWorkWhenUsingATokenGeneratedByOtherLibs(): void self::assertTrue($this->config->validator()->validate($token, $constraint)); self::assertEquals('world', $token->claims()->get('hello')); } + + /** @test */ + public function signatureValidationWithLocalFileKeyReferenceWillOperateWithKeyContents(): void + { + $key = tempnam(sys_get_temp_dir(), 'key'); + assert(is_string($key)); + + file_put_contents($key, 'just a dummy key'); + + $validKey = LocalFileReference::file($key); + $invalidKey = InMemory::plainText('file://' . $key); + $signer = new Sha256(); + $configuration = Configuration::forSymmetricSigner($signer, $validKey); + $validator = $configuration->validator(); + + $token = $configuration->builder() + ->withClaim('foo', 'bar') + ->getToken($configuration->signer(), $configuration->signingKey()); + + self::assertFalse( + $validator->validate( + $token, + new SignedWith($signer, $invalidKey) + ), + 'Token cannot be validated against the **path** of the key' + ); + + self::assertTrue( + $validator->validate( + $token, + new SignedWith($signer, $validKey) + ), + 'Token can be validated against the **contents** of the key' + ); + } } diff --git a/test/unit/Signer/EcdsaTest.php b/test/unit/Signer/EcdsaTest.php index 4354ec07..eb9c9580 100644 --- a/test/unit/Signer/EcdsaTest.php +++ b/test/unit/Signer/EcdsaTest.php @@ -56,6 +56,7 @@ private function getSigner(): Ecdsa * * @uses \Lcobucci\JWT\Signer\Ecdsa::__construct * @uses \Lcobucci\JWT\Signer\Key\LocalFileReference + * @uses \Lcobucci\JWT\Signer\Key\InMemory */ public function signShouldReturnTheAHashBasedOnTheOpenSslSignature(): void { @@ -88,6 +89,7 @@ public function signShouldReturnTheAHashBasedOnTheOpenSslSignature(): void * * @uses \Lcobucci\JWT\Signer\Ecdsa::__construct * @uses \Lcobucci\JWT\Signer\Key\LocalFileReference + * @uses \Lcobucci\JWT\Signer\Key\InMemory */ public function verifyShouldDelegateToEcdsaSignerUsingPublicKey(): void { diff --git a/test/unit/Signer/Key/LocalFileReferenceTest.php b/test/unit/Signer/Key/LocalFileReferenceTest.php index 05e088d8..e4aa501c 100644 --- a/test/unit/Signer/Key/LocalFileReferenceTest.php +++ b/test/unit/Signer/Key/LocalFileReferenceTest.php @@ -6,7 +6,11 @@ use org\bovigo\vfs\vfsStream; use PHPUnit\Framework\TestCase; -/** @coversDefaultClass \Lcobucci\JWT\Signer\Key\LocalFileReference */ +/** + * @coversDefaultClass \Lcobucci\JWT\Signer\Key\LocalFileReference + * + * @uses \Lcobucci\JWT\Signer\Key\InMemory + */ final class LocalFileReferenceTest extends TestCase { /** @before */ @@ -33,20 +37,6 @@ public function thereShouldBeNoReferenceToAFileThatDoesNotExist(): void LocalFileReference::file(vfsStream::url('root/test2.pem')); } - /** - * @test - * - * @covers ::file - * @covers ::__construct - * @covers ::contents - */ - public function pathShouldBeNormalised(): void - { - $key = LocalFileReference::file('file://' . vfsStream::url('root/test.pem')); - - self::assertSame('file://vfs://root/test.pem', $key->contents()); - } - /** * @test * @@ -55,11 +45,11 @@ public function pathShouldBeNormalised(): void * @covers ::contents * @covers ::passphrase */ - public function contentsShouldReturnOnlyTheReferenceToTheFile(): void + public function pathShouldBeNormalised(): void { - $key = LocalFileReference::file(vfsStream::url('root/test.pem'), 'test'); + $key = LocalFileReference::file('file://' . vfsStream::url('root/test.pem'), 'test'); - self::assertSame('file://vfs://root/test.pem', $key->contents()); + self::assertSame('testing', $key->contents()); self::assertSame('test', $key->passphrase()); } } diff --git a/test/unit/Signer/RsaTest.php b/test/unit/Signer/RsaTest.php index 9909b11c..3dd504e4 100644 --- a/test/unit/Signer/RsaTest.php +++ b/test/unit/Signer/RsaTest.php @@ -30,6 +30,7 @@ final class RsaTest extends TestCase * @covers \Lcobucci\JWT\Signer\OpenSSL * * @uses \Lcobucci\JWT\Signer\Key\LocalFileReference + * @uses \Lcobucci\JWT\Signer\Key\InMemory */ public function signShouldReturnAValidOpensslSignature(): void { @@ -100,6 +101,7 @@ public function signShouldRaiseAnExceptionWhenKeyIsNotParseable(): void * @covers \Lcobucci\JWT\Signer\InvalidKeyProvided * * @uses \Lcobucci\JWT\Signer\Key\LocalFileReference + * @uses \Lcobucci\JWT\Signer\Key\InMemory */ public function signShouldRaiseAnExceptionWhenKeyTypeIsNotRsa(): void { @@ -119,6 +121,7 @@ public function signShouldRaiseAnExceptionWhenKeyTypeIsNotRsa(): void * @covers \Lcobucci\JWT\Signer\OpenSSL * * @uses \Lcobucci\JWT\Signer\Key\LocalFileReference + * @uses \Lcobucci\JWT\Signer\Key\InMemory */ public function verifyShouldReturnTrueWhenSignatureIsValid(): void { @@ -161,6 +164,7 @@ public function verifyShouldRaiseAnExceptionWhenKeyIsNotParseable(): void * @covers \Lcobucci\JWT\Signer\InvalidKeyProvided * * @uses \Lcobucci\JWT\Signer\Key\LocalFileReference + * @uses \Lcobucci\JWT\Signer\Key\InMemory */ public function verifyShouldRaiseAnExceptionWhenKeyTypeIsNotRsa(): void {