Skip to content

Added support for multiple keys in SignedWith validation#1011

Closed
rhertogh wants to merge 1 commit into
lcobucci:5.1.xfrom
rhertogh:support_multiple_signed_with_keys
Closed

Added support for multiple keys in SignedWith validation#1011
rhertogh wants to merge 1 commit into
lcobucci:5.1.xfrom
rhertogh:support_multiple_signed_with_keys

Conversation

@rhertogh

@rhertogh rhertogh commented Apr 22, 2023

Copy link
Copy Markdown
Contributor

In order to support key rotation, this pull request adds the possibility to pass multiple keys to the SignedWith validator to verify against.

@rhertogh

Copy link
Copy Markdown
Contributor Author

@lcobucci Hi, could you please approve the workflows?

@Slamdunk Slamdunk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Changing the current constraint would weaken the library, I suggest to propose something like a new AtLeastOne constraint which accepts multiple SignedWith constraints.

@lcobucci

Copy link
Copy Markdown
Owner

Changing the current constraint would weaken the library, I suggest to propose something like a new AtLeastOne constraint which accepts multiple SignedWith constraints.

Let's also wrap the array. JWK has the concept of key sets, we can use this as opportunity to introduce the it.

@lcobucci

Copy link
Copy Markdown
Owner

@lcobucci Hi, could you please approve the workflows?

@rhertogh you may also run everything locally with make

@rhertogh

Copy link
Copy Markdown
Contributor Author

@Slamdunk @lcobucci

Changing the current constraint would weaken the library, I suggest to propose something like a new AtLeastOne constraint which accepts multiple SignedWith constraints.

Let's also wrap the array. JWK has the concept of key sets, we can use this as opportunity to introduce the it.

Using a "JWK Set" does indeed make sense.
However, in that case we're passing only 1 thing (the entire set) to the constraint. But when going that path, AtLeastOne, which would accept multiple SignedWith, sounds contradicting to me.

I see two options:

  1. Have SignedWith accept Key|KeySet
  2. Have SignedWith only accept Key and add a second constraint like SignedWithOneInSet which then accepts a KeySet

Option 1 makes more sense to me, since it's still validating against either the Key or KeySet. And since the purpose of the KeySet is to be a list of keys (e.g. key rotation and/or different algorithms) there is no risk of confusion or misuse.

@Slamdunk @lcobucci What are your thoughts on this?

@lcobucci

Copy link
Copy Markdown
Owner

@rhertogh many apologies for my delay... doing OSS has been a little challenging lately.

And since the purpose of the KeySet is to be a list of keys (e.g. key rotation and/or different algorithms) there is no risk of confusion or misuse.

I'm not sure if I understood what you mean...
A JWK key does point to an algorithm and a key set may contain keys with different algorithms.

Going with option 1 might lead us to abstractions that don't match the real-life possibilities.

Perhaps forgetting about the key set for now and having something like the implementation bellow is indeed the most flexible solution for now:

use Lcobucci\JWT\Validation\SignedWith as SignedWithInterface;

final class SignedWithOneInSet implements SignedWithInterface
{
    /** @var list<SignedWithInterface> */
    private readonly array $constraints;
    
    public function __construct(SignedWithInterface ...$constraints)
    {
        $this->constraints = $constraints;
    }

    public function assert(object $token): void
    {
        // (...)
    }
}

@lcobucci

lcobucci commented Nov 4, 2023

Copy link
Copy Markdown
Owner

@rhertogh I'll close this one in favour of #1033

Thanks for your help!

@lcobucci lcobucci closed this Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants