Duplicate header params across protected & per-recipient headers (violates RFC 7516 §7.2.1)

[ilya-korotya]

Summary
When creating a JWE with JSON Serialization, the same header parameters (e.g., kid, alg) appear in both the protected and per-recipient unprotected (header) sections.
RFC 7516 requires that the header parameter names across the three locations—protected, unprotected, and per-recipient header—“MUST be disjoint.”

RFC reference: https://www.rfc-editor.org/rfc/rfc7516#section-7.2.1

---

package jwxmergeissue

import (
	"crypto/rand"
	"crypto/rsa"
	"encoding/base64"
	"encoding/json"
	"testing"

	"github.com/lestrrat-go/jwx/v3/jwa"
	"github.com/lestrrat-go/jwx/v3/jwe"
	"github.com/stretchr/testify/require"
)

func TestDuplicateHeaders(t *testing.T) {
	recipientPrivateKey, err := rsa.GenerateKey(rand.Reader, 2048)
	require.NoError(t, err)

	// Per-recipient unprotected header includes kid
	recipientHeaders := jwe.NewHeaders()
	require.NoError(t, recipientHeaders.Set("kid", "recipient1"))

	recipient := jwe.WithKey(
		jwa.RSA_OAEP_256(),
		recipientPrivateKey.PublicKey,
		jwe.WithPerRecipientHeaders(recipientHeaders),
	)

	// Produce JSON Serialization (flattened for single recipient)
	ret, err := jwe.Encrypt([]byte("Hello!"), jwe.WithJSON(), recipient)
	require.NoError(t, err)

	var jweJSON map[string]any
	require.NoError(t, json.Unmarshal(ret, &jweJSON))

	// Decode protected header (Base64URL)
	protectedB64, ok := jweJSON["protected"].(string)
	require.True(t, ok)

	protectedJSON, err := base64.RawURLEncoding.DecodeString(protectedB64)
	require.NoError(t, err)

	var protected map[string]any
	require.NoError(t, json.Unmarshal(protectedJSON, &protected))

	// Per-recipient header
	recipHeader, ok := jweJSON["header"].(map[string]any)
	require.True(t, ok)

	// Fail if the same names appear in both places (violates RFC 7516 §7.2.1)
	if _, has := protected["kid"]; has {
		if _, also := recipHeader["kid"]; also {
			t.Fatalf("duplicate 'kid' present in both protected and per-recipient headers")
		}
	}
	if _, has := protected["alg"]; has {
		if _, also := recipHeader["alg"]; also {
			t.Fatalf("duplicate 'alg' present in both protected and per-recipient headers")
		}
	}
}

Expected behavior
Library should prevent duplicate header names across protected, unprotected, and per-recipient headers

go.mod file:

module github.com/ilya-korotya/jwx_merge_issue

go 1.24.4

toolchain go1.24.8

require (
	github.com/lestrrat-go/jwx/v3 v3.0.11
	github.com/stretchr/testify v1.11.1
)

require (
	github.com/davecgh/go-spew v1.1.1 // indirect
	github.com/decred/dcrd/dcrec/secp256k1/v4 v4.4.0 // indirect
	github.com/goccy/go-json v0.10.3 // indirect
	github.com/lestrrat-go/blackmagic v1.0.4 // indirect
	github.com/lestrrat-go/httpcc v1.0.1 // indirect
	github.com/lestrrat-go/httprc/v3 v3.0.1 // indirect
	github.com/lestrrat-go/option v1.0.1 // indirect
	github.com/lestrrat-go/option/v2 v2.0.0 // indirect
	github.com/pmezard/go-difflib v1.0.0 // indirect
	github.com/segmentio/asm v1.2.0 // indirect
	golang.org/x/crypto v0.42.0 // indirect
	golang.org/x/sys v0.36.0 // indirect
	gopkg.in/yaml.v3 v3.0.1 // indirect
)

[ilya-korotya]

This merge seems to be the cause:

    // If there's only one recipient, you want to include that in the
	// protected header
	if len(recipients) == 1 {
		h, err := protected.Merge(recipients[0].Headers())
		if err != nil {
			return nil, fmt.Errorf(`failed to merge protected headers: %w`, err)
		}
		protected = h
	}

After merging, the library should remove the headers from the recipient to avoid duplication.

[lestrrat]

Thanks. I remember that piece of code, and I think it's ancient -- it probably was there from pre-v1 days in some form, and I'm pretty sure I copied that from somewhere ... :)

---

(below is me basically thinking out loud so that I can remember it later; definitely not my final conclusion on this matter yet)

So, merge and destroy the old headers? is it safe...? -> it should be safe, as when there is only one recipient, there is only one header.

if len(recipients) == 1 {
   ...
   recipents[0].SetHeaders( nil ) // I forget if I could assign nil, but this is the intention
}

Although it being the correct thing to do, I think removing the headers would have to be optional to keep backwards compatibility.

// globally
jwe.Settings(jwe.WithDisjointHeaders(true))
jwe.Encrypt(....) // always new behavior

// per-call
jwe.Encrpt(...) // old behavior
jwe.Encrypt(jwe.WithDisjointHeaders(true), ...) // new behavior

[lestrrat]

@ilya-korotya In your test code, Do you expect kid/alg to be in the public header portion? I think they should only be available in the protected headers?

[ilya-korotya]

Hi @lestrrat. I'm not fully sure which headers should be moved to the protected section and which should stay in the participant header section.
Here is how I understand it (only in case if jwe.WithJSON() option was passed and only one recipient is presented):

1. Integrity-protected headers — like alg, enc, tag, etc. — should be moved by the library from the participant header section to the protected header section.
2. Non-integrity-protected headers — like kid, typ, cty, etc. — can stay in the participant header section, but we must make sure they are not duplicated in the protected header section.

I think we can simplify the logic. The library can copy all headers from the per-participant header section to the protected header section, and then clear the per-participant headers. This should happen only when the JSON serialization option is used and there is only one recipient.

Our proposal with jwe.WithDisjointHeaders(true) option LGTM.

[lestrrat]

This should happen only when the JSON serialization option is used and there is only one recipient.

Nah, it needs to happen for compact format too, as there are no per-recipient headers.
Also, there's this thing called the flattened JSON format... (rolls eyes)

I need to take sometime to think this over to make sure I can actually fix this without breaking stuff

[lestrrat]

I'm just thinking out loud, and leaving a comment so I can remember what I was thinking:

• I think the current way we pack things into protected header works, but is incorrect
• Ideally, the serialization should be exactly the same as the data specified the user, but this only applies for full JSON format
  • This has security implications, because the user could include fields that should be integrity-protected in an open header.
• For flattened JSON and compact format, packing things into protected headers is necessary

I probably wrote that code to pack things into the protected header when this library did not have any support for full JSON format, and that's why everything looked peachy -- the encryption works as intended. It's just that when using the full-JSON format, unpacked representations come out in a format that's unexpected.

Now, how to fix this...

We're probably going to need a "keep the old behavior mode" and "do the right thing mode".
The implementation is probably pretty straightforward. It would mostly come down to naming the option, and documentation so users know what they should do.

For starters, we need to preserve the old behavior, because we don't want to break people's existing code. So to get the new (correct) behavior, we need to add an option to jwe.Encrypt:

// old behavior
jwe.Encrypt(....)

// new behavior
// nevermind the option name. it's a placeholder for now
jwe.Encrypt(...., jwe.PreserveOriginalHeaders())

We would need to add ample documentation in both jwe.Encrypt and the new option.

[lestrrat]

More memo.

Upon further debugging, though this library needs to be fixed I think the user's expectations are also wrong.

This library should NEVER merge per-recipient headers to the protected headers -- we need to use the protected headers as-is.
The user cannot specify fields such as "kid" (which should be in the protected header) in the per-recipient header.

So this library also needs to flat out reject per-recipient headers when using compact serialization / flattened JSON serialization.

We should allow the user to request opt-in no-merging behavior.

For v4, no-merging should be the default.

[lestrrat]

@ilya-korotya please take a look at #1477, see if it does what you wanted. You do need to opt-in by specifying a new option.

[ilya-korotya]

Hi @lestrrat. The PR LGTM. Thanks!

I found one more issue. Now the same unit test works and returns a token in this format:

{
  "ciphertext": "NR5BBCPc",
  "encrypted_key": "PENNLqJYzXYbKgiZCTe61Qhasls8wrqGQ7ytsvvm5_E6Yya79QyrtDaX_lYm2FitLPMkw4K-63NJf1Ltp9Ur-zZUJB2rQmGkhB4toBO-SkLSkSPhUr6lILKIFWrnbBjjy63g_Awfqb7tsWT1kD0rqO_xPhhN9s0xoHMpgFYY80QQxUBVYiADiu01H_o9tsh66Npch3HAI6t6bxUfVwyMjpWK7N6k74vc95l3-ZcAGxl6aeWAtUTD37bE5bVbvILfcb6HdK0Z81fmPcRe2o9W4wZ_SP2fEV67I-jrsx73rgL1a1c_9TB4vZX3XxppvBhwKHUBAksaq3aWQXr8gkWt-g",
  "header": {},
  "iv": "TPrbYWe0oUeC9Eop",
  "protected": "eyJhbGciOiJSU0EtT0FFUC0yNTYiLCJlbmMiOiJBMjU2R0NNIiwia2lkIjoicmVjaXBpZW50MSJ9",
  "tag": "tOk33fmFy80qZVNyb22s0g"
}

By the RFC, if the recipient header is empty, it should be omitted. If I remove the header field and try to parse the flattened JWE, I get an error. In this case, all the headers needed to parse/decrypt the token are in the protected headers. I think the library should combine all possible headers before parse/decrypting and if the headings overlap, return an error.

echo eyJhbGciOiJSU0EtT0FFUC0yNTYiLCJlbmMiOiJBMjU2R0NNIiwia2lkIjoicmVjaXBpZW50MSJ9 | base64 -d |
jq .
{
  "alg": "RSA-OAEP-256",
  "enc": "A256GCM",
  "kid": "recipient1"
}

func TestParseWithoutHeaderForFlattedToken(t *testing.T) {
	token := `{
    "ciphertext": "NR5BBCPc",
    "encrypted_key": "PENNLqJYzXYbKgiZCTe61Qhasls8wrqGQ7ytsvvm5_E6Yya79QyrtDaX_lYm2FitLPMkw4K-63NJf1Ltp9Ur-zZUJB2rQmGkhB4toBO-SkLSkSPhUr6lILKIFWrnbBjjy63g_Awfqb7tsWT1kD0rqO_xPhhN9s0xoHMpgFYY80QQxUBVYiADiu01H_o9tsh66Npch3HAI6t6bxUfVwyMjpWK7N6k74vc95l3-ZcAGxl6aeWAtUTD37bE5bVbvILfcb6HdK0Z81fmPcRe2o9W4wZ_SP2fEV67I-jrsx73rgL1a1c_9TB4vZX3XxppvBhwKHUBAksaq3aWQXr8gkWt-g",
    "iv": "TPrbYWe0oUeC9Eop",
    "protected": "eyJhbGciOiJSU0EtT0FFUC0yNTYiLCJlbmMiOiJBMjU2R0NNIiwia2lkIjoicmVjaXBpZW50MSJ9",
    "tag": "tOk33fmFy80qZVNyb22s0g"
}`

	_, err := jwe.Parse([]byte(token))
	require.NoError(t, err)
}

Output:

=== RUN   TestParseWithoutHeaderForFlattedToken
    ./src/github.com/ilya-korotya/jwx_merge_issue/merge_test.go:76:
        	Error Trace:	./jwx_merge_issue/merge_test.go:76
        	Error:      	Received unexpected error:
        	            	jwe.Parse: failed to parse JSON: failed to decode headers field: EOF
        	Test:       	TestParseWithoutHeaderForFlattedToken
--- FAIL: TestParseWithoutHeaderForFlattedToken (0.00s)
FAIL
FAIL	github.com/ilya-korotya/jwx_merge_issue	0.209s

It looks like jwe.Parse fails when header is missing in a flattened token, even though an empty header should be equivalent to omitting it. Could you confirm if this is a bug? If yes, we may need to treat a missing header the same as {}.

[lestrrat]

Ah, nice catch! I made some fixes in 4eabe77
Please check the PR again?

[lestrrat]

Thanks, merged. I'm not going to make immediate releases; history (i.e. my own dumb actions) has taught me that rushing a release is never a good idea, and so it will be resting in the repo for at least a few days if not more. If you want this feature now, please pin the git commit ID for the time being.