fix(encryption): allow a sender key with a static key agreement - #696
Merged
Conversation
`JWEBuilder::withSenderKey()` derived the key management mode and checked the key against it, so it could not be used with ECDH-SS: called before `addRecipient()` the content encryption algorithm was still unknown, and called after it the "agree" mode was rejected as a foreign key management mode. The sender key does not add a recipient: it no longer takes part in the compatibility check and is verified by `build()`, once the recipients and the content encryption algorithm are known whatever the call order is. `determineCEK()` read the sender key from a per-recipient entry that is never written, so a direct key agreement always ended up with no sender key at all. It now falls back to the sender key of the builder, as `processRecipient()` does. `JWEDecrypter::decryptUsingKey()` passed the sender key as the fourth argument of `decryptUsingKeySet()`, which is the output key parameter: the sender key never reached the algorithm and such a token could not be decrypted back. Closes #680
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #680.
JWEBuilder::withSenderKey()could not be used with a direct static key agreement such asECDH-SS, andthe sender key was dropped even when the mode check was passed.
What was wrong
withSenderKey()derived the key management mode from the shared headers and checked the key against it.Called before
addRecipient()it threwInvalid content encryption algorithm, becausecheckKey()needs the content encryption algorithm and only
addRecipient()sets it. Called after it threwForeign key management mode forbidden., becauseagree+agreeis not a supported combination.determineCEK()read the sender key from$this->recipients[0]['sender_key'], an entry that is neverwritten anywhere, so a direct key agreement always reached
ECDHSS::getAgreementKey()with no sender keyand threw
The sender key shall be set.processRecipient()did not have the problem thanks to its?? $this->senderKeyfallback, which is whyECDH-SS+A128KWand friends worked.JWEDecrypter::decryptUsingKey()passed$senderKeyas the fourth argument ofdecryptUsingKeySet(),which is the
&$jwkoutput parameter. The sender key never reached the algorithm, so a token builtwith a static key agreement could not be decrypted back through that method. Found while writing the
round-trip test: without this one, the builder fix produces tokens nothing can read.
What changed
withSenderKey()only stores the key. The sender key does not add a recipient, so it must not go throughthe key management mode compatibility matrix.
build()(newcheckSenderKey()), against the key encryption algorithm of eachrecipient, where the content encryption algorithm is known whatever the call order is. This also settles
the
TODOthat sat on top of the method.determineCEK()falls back to$this->senderKeylikeprocessRecipient()does.decryptUsingKey()passes the sender key in the right position.Tests
tests/Component/Encryption/JWEBuilderSenderKeyTest.php—withSenderKey()had no coverage at all and notest exercised
ECDH-SS. Six cases: sender key set before and afteraddRecipient()with anECDH-SSround trip, missing sender key,
ECDH-SS+A128KW,ECDH-ESwhere the sender key replaces the ephemeral key(the published
epkis the given key), and the deferred key check at build time. Five of the six fail on4.2.x.Note for the reviewer: on the decryption side of a static key agreement the roles are swapped — the public
key of the sender is the key of the key set and the private key of the recipient is the sender key, since
epkis not part of the token. That is the existing convention ofECDHSSAESKWand its tests, notsomething introduced here.
Checks
Full test suite (874 tests), ECS and PHPStan are green. One PHPStan baseline entry goes from
count: 1to2(Cannot access offset 'key_encryption_algorithm' on mixed):$recipientsis an untyped array and thenew loop adds one more access already covered by that rule.