[akonradi]

Commit Message:
RFC 9110 Section 5.3 mandates that header fields sent as a single line
with comma-joined values be treated as equivalent to the same field sent
on multiple header lines. The JWT extractor already handled the
multiple-header-lines form (iterating each physical header occurrence)
but took a single comma-joined header line as one opaque value, so e.g.
"Authorization: Bearer , Bearer " extracted a single,
invalid token instead of two valid ones.

Additional Description:
Found this by breaking JWT handling by a well-known header registration. The registered header appeared in a from_headers config. Registering it caused Envoy to collapse multiple values, which caused JWT extraction to barf.

AI disclosure: the author wrote this PR with assistance of AI tools but takes responsibility for the contents.

Risk Level: med, changes JWT header parsing
Testing: ran unit tests
Docs Changes: n/a
Release Notes: none
Platform Specific Features: none

[akonradi]

This PR took pains to keep the existing handling of ,suffix but that probably shouldn't be allowed since it's ambiguous with header concatenation. I don't know how often that's used but ideally it'd be migrated to something like ; instead.

[agrawroh]

/retest

[agrawroh]

I'll also run this by @wbpcode once.

[agrawroh]

@akonradi I discussed this with @wbpcode offline. We believe that some clients out their might still be relying on the old behavior. Could you please add a Runtime Guard for it so that people get time to adapt to this "correct" behavior and we don't hard break them?

[agrawroh]

/wait

[wbpcode]

A defensive runtime flag. Considering the runtime flag check is not that cheap as I expected before, may could fetch the flag value and store it at the ExtractorImpl.

[akonradi]

SGTM, done. While I've got y'all, WDYT of the switch I proposed above from , to ; for adding an uninterpreted suffix? Also would need to be guarded, obviously, but I don't have a feel for how "standard" the existing behavior is or who's using it.

[repokitteh-read-only]

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #48517 was synchronize by akonradi.

see: more, trace.

[wbpcode]

WDYT of the switch I proposed above from , to ; for adding an uninterpreted suffix? Also would need to be guarded, obviously, but I don't have a feel for how "standard" the existing behavior is or who's using it.

Note, no matter ,suffix or ;suffix, it’s an invalid token and should result in a 401 before? If we split the value based on , directly, seems some previous invalid token will become valid in new code path.
Although TBH, I think it never been considered before, but it's a vague behavior change.

cc @agrawroh

[akonradi]

That behavior was added in #6384, so definitely intentional, but not necessarily still used. I think the minimal change would be to keep the extra functionality but narrow the set of non-base64 characters allowed to exclude ,.

[akonradi]

/retest