feat(oci): WWW-Authenticate challenge parser (S16 Step 4)
What
Step 4 of the S16 container remote slice: the WWW-Authenticate challenge
parser and auth_url assembly.
Pure functions — RFC 7235 quoted-string parsing including \" escaping,
case-insensitive Bearer, realm and service extraction, an absolute-https
requirement, and auth_url assembly through url.Values. No HTTP, no state, no
database. Every symbol is unexported and has no production caller yet: Step 12
consumes them when it wires the token exchange.
Plan: docs/plans/2026-07-30-container-remote.md,
"Step 4: WWW-Authenticate challenge parser".
Notes for the reviewer
The .gitlab-ci.yml hunk is in scope. Step 4's Files entry names only
remote_challenge.go, but its Tests entry requires a fuzz target over the
header parser, and a target that is not wired into fuzz:oci only ever runs its
seed corpus. The six lines add FuzzParseBearerChallenge to the existing job.
The plan's Files entries list production files only — they omit
remote_challenge_test.go too.
Three deliberate divergences from the Container Registry reference parser.
session.go floors a short token lifetime up to 60s, resolves access_token
over token, and computes expiry from the response's issued_at. None is
reproduced; the first and third belong to Step 5, the second to Step 12. The
parser patterns that are copied — expectTokenOrQuoted's escaped-quote
rebuild, lowercasing the scheme and every parameter key, walking every header
value — are copied deliberately and noted where they land.
One challenge is read per header value, which is the reference parser's
shape. RFC 7235 also permits several challenges inside one comma-separated
value, where a Bearer challenge after a Basic one is not found. That is the
fail-closed direction — no challenge found reads as "no token needed" rather
than as a realm the upstream never stated — and a test row pins it.
A realm carrying userinfo is refused. The reason ParseUpstreamBase gives
for refusing it does not reach this path: net/http synthesizes an
Authorization header from userinfo in http.Client.send, and
upstreamhttp.Client.Do issues through Transport.RoundTrip, so it would never
reach the wire. It is refused because the assembled URL is persisted on
container_remote_repositories.auth_url, which makes userinfo in it a credential
at rest that the management API reads back and any log line printing the column
carries. This goes beyond S16, which asks only for an absolute https realm;
refusing is fail-closed and no known registry states credentials in a realm.
Three rows cover the password-bearing, bare-username, and empty-password forms.
A realm's fragment is kept and pinned, since it reaches neither the wire nor a
credential column.
One upstream WWW-Authenticate value is capped at 8 KiB before the byte
scan, mirroring maxChallengeValue in internal/auth/challenge.go:71. The value
is upstream-controlled and net/http's default
Transport.MaxResponseHeaderBytes allows 10 MiB of response headers. An
oversized value is skipped rather than failing the walk, so a legitimate
challenge in a later header value is still found.
Acceptance criteria 16 and 24 are only half-owned here. The plan's ownership
table assigns both to this step with no gated marker, but each criterion also
states the request outcome — serve the cache if a copy exists, else 503 UNAVAILABLE with Retry-After. A pure parser cannot assert that; it lands in
Step 13, whose Acceptance is a table over every row of the spec's Error Cases.
What this step owns is the distinct sentinel that keeps the two outcomes apart:
errChallengeRealmUnusable for a stated-but-unusable realm against
errChallengeIncomplete for a missing parameter, which the spec explicitly warns
against collapsing.
No e2e scenario is added or affected. This step adds no route, no response,
and no client-visible behavior, so neither docs/testing/e2e/oci.md nor
docs/testing/e2e/docker.md changes. Conformance is not reachable for the same
reason: the distribution suite drives client-facing routes and this MR adds
none.
Size
Above the 500-line ceiling in
development-model.md, and above the plan's
Est. ~400 for this step. The excess is test surface rather than branching:
one production file, and a suite carrying the ported reference case set, three
property tables, a fuzz target, and two benchmarks.
Review passes
Three review passes ran on this branch.
The first, before the MR opened, found and fixed:
- A quadratic allocation in
unescapeChallengeQuoted, which sized its scratch buffer from the remaining header value rather than from the quoted-string it was unescaping. A 1 MiB header measured 3.80s and 73.7 GB of allocation churn; it now costs 4.9ms. - Four comment claims that promised more than the code delivered.
- An unreached fail-closed branch: a value that opens a backslash escape and never closes its quote had no test row and no fuzz seed.
The second, a coherence check against the plan and spec, found and fixed:
- A credential leak in the
url.Parserejection. The message wrappednet/url's own cause, andurl.EscapeErrorquotes back the escape it rejected, so a malformed percent-escape inside the realm's password put credential bytes into the error. The rejection now names the component and drops the cause, which is whatParseUpstreamBasedoes for the same reason. - The realm's userinfo and fragment handling, until then recorded only in a commit body, documented in the code and pinned by table rows.
- Three comment corrections:
challengeSeparatorsalso contains HTAB and so is not exactly the printable-ASCII bytestcharexcludes; the measured allocation figures were duplicated betweenunescapeChallengeQuotedand the benchmark header and disagreed with the benchmark's own sizes (1 MiB against a 256 KiB constant), so both now rest on the 4×-input/16×-bytes ratio instead; and the paragraph arguing whyparseBearerChallenge's boolean does not name its verdict is cut to the contract a caller needs. Comments across the file went from 173 lines against 138 of code to 150 against 131.
The third, the automated AppSec and GitLab Duo reviews, found and fixed:
- Userinfo on a realm is now refused rather than assembled into
auth_url, per the section above. Both reviews raised it; both offered stripping or rejecting, and rejecting is what shipped. - The 8 KiB cap on one
WWW-Authenticatevalue, also above. It moved the quadratic-allocation guard: at 64 KiB and 256 KiB the adversarial benchmark rows would now be skipped and measure nothing, so they callparseChallengeValuedirectly andBenchmarkParseBearerChallengekeeps the realistic entry-point row. - The credential-leak test strengthened to place credential bytes on both
sides of the malformed escape (
secret%ZZword), so it proves the escape's neighbours stay out of the message rather than only that the escape does.
Related to #288