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

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

Conversation

MatthewHepburn
Copy link

#928 added an excellent feature, allowing additional public keys to be specified.

However, unlike the main public key, this can only be specified as a file path, rather than being passed directly as a string. The ability to pass keys as strings is useful when setting keys via environment variables, so I want to be able to configure the additional public keys as either file paths or as raw strings.

@chalasr
Copy link
Collaborator

chalasr commented Apr 20, 2022

Sounds good! Let me know when you think this is ready

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.

@MatthewHepburn MatthewHepburn marked this pull request as ready for review April 21, 2022 07:26
@Ardenexal
Copy link

@chalasr @MatthewHepburn any update on this I running into the same issue where I want to pass through the Public key string rather than the file location and this PR would fix that issue

@1ed
Copy link

1ed commented Oct 20, 2023

What is missing to get this merged?

@TekSign
Copy link

TekSign commented Feb 28, 2024

Is there any update on this change please ? Because this would be very helpful and all seems ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants