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

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 all 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
3 changes: 2 additions & 1 deletion Resources/doc/1-configuration-reference.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ lexik_jwt_authentication:
public_key: '%kernel.project_dir%/config/jwt/public.pem' # path to the public key OR raw public key, required for verifying tokens
pass_phrase: 'yourpassphrase' # required for creating tokens
# Additional public keys are used to verify signature of incoming tokens, if the key provided in "public_key" configuration node doesn't verify the token
# Can be paths to public key files OR raw public keys
additional_public_keys:
- '%kernel.project_dir%/config/jwt/public1.pem'
- '%kernel.project_dir%/config/jwt/public2.pem'
- '%kernel.project_dir%/config/jwt/public3.pem'
- '%env(PUBLIC3_KEY_TEXT)%'
```

#### Using HMAC
Expand Down
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.

Copy link

Choose a reason for hiding this comment

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

Hello! I'm facing a similar problem: using additional string certs instead of files to verify tokens.

IMHO, looking at the code, the original intention of the AbstractKeyLoader was to allow both cases because the $key is expected to be stored in the $contents array in its raw format even if the file is missing. Furthermore a similar behavior is defined in the getSigningKey() method.

Unfortunately, due to the (wrong?) if statement at the beginning of the loop, this situation never happens because when the key is properly set but it does not point to a file, then the exception is thrown.

@MatthewHepburn beware that what you tried to achieve in your first commit did not solve this problem because, when the $key is properly configured, !$key always solves to false and the condition on the right side is never considered 🙂

Also, I'd leave the \RuntimeException if the string is empty, because in my opinion it is a config-related issue and it should not be addressed here 🤔

That said, I'd change the code as following:

if (!$key || (is_file($key) && !is_readable($key)) {
    throw new \RuntimeException(...);
}

What do you think?

}
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;
}
}