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.

Sign up for GitHub

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

Jump to bottom

Allow additional public keys to be set by value, as well as by path #1008

Open
wants to merge 5 commits into
base: 2.x
Choose a base branch
from
Open

Allow additional public keys to be set by value, as well as by path #1008

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
86c484f
Allow additional public keys to be set by value, as well as by path
pfmhepburn Apr 20, 2022
ebc2592
Adds test case for null additional key
pfmhepburn Apr 20, 2022
11bf6c4
Adds test cases for loading keys as files or as strings
pfmhepburn Apr 20, 2022
1f6c83c
Skip falsy keys, to make config more flexible
pfmhepburn Apr 20, 2022
015da58
Update documentation to note that keys can be passed as strings
pfmhepburn Apr 21, 2022
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
7 changes: 5 additions & 2 deletions Services/KeyLoader/AbstractKeyLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,11 @@ public function getAdditionalPublicKeys(): array
$contents = [];

foreach ($this->additionalPublicKeys as $key) {
if (!$key || !is_file($key) || !is_readable($key)) {
throw new \RuntimeException(sprintf('Additional public key "%s" does not exist or is not readable. Did you correctly set the "lexik_jwt_authentication.additional_public_keys" configuration key?', $key));
if (!$key) {
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the motivation for this change?

Copy link
Author

Choose a reason for hiding this comment

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

I want to support having a dynamic number of additional keys using a static configuration. For example 1 additional key while a key rotation is in progress, and 0 additional keys otherwise.

I haven't been able to come up with a better way of expressing that than

lexik_jwt_authentication:
  public_key: '%env(MAIN_KEY)%'
  additional_public_keys:
    - '%env(string:default::SECONDARY_KEY)%'

But then we end up with this invalid array element when SECONDARY_KEY is not set that needs to be skipped for that to work.

I did consider trying to set the whole array via JSON like

lexik_jwt_authentication:
  public_key: '%env(MAIN_KEY)%'
  additional_public_keys: '%env(json:SECONDARY_KEY_JSON)%'

But that's not allowed - we get the error A dynamic value is not compatible with a "Symfony\Component\Config\Definition\PrototypedArrayNode" node type at path "lexik_jwt_authentication.additional_public_keys".

I have limited experience with the Symfony config system, so perhaps I'm missing a better way of expressing this. I would be happier keeping the behaviour of throwing on encountering a falsy value and handling this situation at the config level instead, but I'm not seeing a way to do that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let me think about it :)

Copy link

Choose a reason for hiding this comment

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

We also have a similar use case.

}
if (is_file($key) && !is_readable($key)) {
throw new \RuntimeException(sprintf('Additional public key "%s" is not readable. Did you correctly set the "lexik_jwt_authentication.additional_public_keys" configuration key?', $key));
}

$contents[] = is_file($key) ? file_get_contents($key) : $key;
Expand Down
59 changes: 50 additions & 9 deletions Tests/Services/KeyLoader/AbstractTestKeyLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Lexik\Bundle\JWTAuthenticationBundle\Tests\Services\KeyLoader;

use Lexik\Bundle\JWTAuthenticationBundle\Services\KeyLoader\AbstractKeyLoader;
use Lexik\Bundle\JWTAuthenticationBundle\Services\KeyLoader\KeyLoaderInterface;
use Lexik\Bundle\JWTAuthenticationBundle\Tests\ForwardCompatTestCaseTrait;
use PHPUnit\Framework\TestCase;
Expand Down Expand Up @@ -34,6 +35,48 @@ public function testLoadKeyFromWrongType()
$this->keyLoader->loadKey('wrongType');
}

public function testFalsyAdditionalPublicKeysSkipped()
{
$className = $this->getClassName();
/** @var AbstractKeyLoader $loader */
$loader = new $className('private.pem', 'public.pem', 'foobar', [null, false, '']);
$this->assertSame([], $loader->getAdditionalPublicKeys());
}

public function testLoadingAdditionalPublicKeysAsStrings()
{
$additionalPublicKeys = ['myKeyText1', 'myKeyText2'];

$className = $this->getClassName();
/** @var AbstractKeyLoader $loader */
$loader = new $className('private.pem', 'public.pem', 'foobar', $additionalPublicKeys);

$this->assertSame($additionalPublicKeys, $loader->getAdditionalPublicKeys());
}

public function testLoadingAdditionalPublicKeysFromFiles()
{
file_put_contents('additional-public-1.pem', 'myKeyTextFromFile1');
file_put_contents('additional-public-2.pem', 'myKeyTextFromFile2');

$className = $this->getClassName();
/** @var AbstractKeyLoader $loader */
$loader = new $className('private.pem', 'public.pem', 'foobar', ['additional-public-1.pem', 'additional-public-2.pem']);

$this->assertSame(['myKeyTextFromFile1', 'myKeyTextFromFile2'], $loader->getAdditionalPublicKeys());
}

public function testLoadingAdditionalPublicKeysFromFilesAndAsStrings()
{
file_put_contents('additional-public-1.pem', 'myKeyTextFromFile1');

$className = $this->getClassName();
/** @var AbstractKeyLoader $loader */
$loader = new $className('private.pem', 'public.pem', 'foobar', ['additional-public-1.pem', 'myKeyText2']);

$this->assertSame(['myKeyTextFromFile1', 'myKeyText2'], $loader->getAdditionalPublicKeys());
}

/**
* {@inheritdoc}
*/
Expand All @@ -49,15 +92,13 @@ public function doTearDown()
*/
protected function removeKeysIfExist()
{
$privateKey = 'private.pem';
$publicKey = 'public.pem';

if (file_exists($publicKey)) {
unlink($publicKey);
}

if (file_exists($privateKey)) {
unlink($privateKey);
$keys = ['private.pem', 'public.pem', 'additional-public-1.pem', 'additional-public-2.pem'];
foreach ($keys as $key) {
if (file_exists($key)) {
unlink($key);
}
}
}

abstract protected function getClassName(): string;
}
5 changes: 5 additions & 0 deletions Tests/Services/KeyLoader/OpenSSLKeyLoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,9 @@ public function testLoadInvalidPrivateKey()

$this->keyLoader->loadKey('private');
}

protected function getClassName(): string
{
return OpenSSLKeyLoader::class;
}
}
5 changes: 5 additions & 0 deletions Tests/Services/KeyLoader/RawKeyLoaderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,9 @@ public function testLoadPrivateKey()
{
$this->assertSame('private.pem', $this->keyLoader->loadKey('private'));
}

protected function getClassName(): string
{
return RawKeyLoader::class;
}
}