feat(datastore): container remote credential and auth-discovery writes (S16 Step 9)
What this does
Implements remote.CredentialRepo for container remote repositories, plus the
two conditional auth-discovery writes and their paired clear:
ContainerRemoteCredentialStoreovercontainer_remote_repositories'tmp_plaintext_username/tmp_plaintext_password, with read, set, and clear. Cleartext never comes back from a read — the store returns LabKitSecretvalues, pinned by a test that exercises six renderings including%#vand a JSON envelope.RecordBearerDiscoveryandRecordNoAuthDiscovery, both conditional on theurlobserved when discovery ran, so a discovery against a remote whoseurlhas since moved is a no-op rather than a write against the new host.ClearAuthDiscovery, unconditional onurl, because a token-endpoint404has to clear a verdict on theurlit was discovered against.oci.ValidateUpstreamCredentials, the RFC 7617 colon rule, reached from the store through theContainerRemoteCredentialValidatorseam its constructor requires. See The colon rule is enforced at the store below.
Every statement pins namespace_id so the write prunes to one hash partition,
and the EXPLAIN suite asserts single-partition access on repositories as well
as container_remote_repositories — the parent gate reaches the second table
as a correlated EXISTS, and it is partitioned the same way.
Size
Measured against main:
| lines | |
|---|---|
| production Go | ~820 (container_remote_credentials.go, oci/remote_credentials.go) |
| test Go | ~2,250 across four files |
| docs | ~210 across three files |
The overshoot is test volume, not step scope. The step is one store plus one
validator; the integration suite covers every entry point against all three
kind values, both container formats, and soft-delete, per the mirrored-suite
rule. Diffed against the Maven and npm credential suites for dropped
subtests, not only added ones — the container suite is a strict superset, and
both container formats get positive-hit coverage rather than one positive and
one negative.
The plan's Est. for this step is deliberately left alone. Re-anchoring these
counts has churned across review rounds before, and S14 left its stale Est.
alone on the same grounds.
Three ADR-007 divergences, all sanctioned
ADR-007 is handbook-synced and cannot be edited from this repo, so these are recorded elsewhere rather than fixed here.
auth_statusand both its CHECKs. ADR-007 publishesauth_urlas the only auth column. Sanctioned by S16's Data Model as items 6-11 of the standing amendment, tracked in #30. The column and both constraints already exist in the landed migration.tmp_plaintext_username/tmp_plaintext_password(text) where ADR-007 publishesencrypted_*(bytea). S13's interim scheme;encrypted_*returns with #417. The shipped Maven and npm stores already do this.- Parent gate uses
repositories.format IN (0, 3). ADR-007:153 publishes0=container, 1=maven, 2=npmwith nooci. The amendment to0=docker, 1=maven, 2=npm, 3=ociis recorded atdocs/specs/S17-rest-management-api.md:151, which names the handbook path — in S17, not S16, so a reviewer working from S16 alone has no local record. Deliberately not added to S16's amendment list: S17 already owns that obligation, and duplicating it gives one amendment two owners. The divergence is also pre-existing onmain, incontainer_remote_repositories.goandcontainer_repository.go.
The spec correction rides in this MR
docs/specs/S16-container-remote.md gave the no-token discovery write as one
column, SET auth_status = 1. Postgres refuses that against a row already in
the bearer state, because CHECK ((auth_status = 2) = (auth_url IS NOT NULL))
ties the pair — and the bearer state is exactly the one that most needs the
write. Two concurrent discoveries reaching different verdicts land there.
Corrected in the statement, the auth_status column definition, and the
sequence diagram, with new ## Resolutions entries. Settled on sibling
precedent rather than local reasoning: S14 refused to give 0 a third,
opposite meaning on a CHECK-bounded column, and S13 states its clearing rule as
behavior over a set of columns rather than as a literal SET list — so the
one-column form was this spec's own shorthand and never S13's requirement.
Normally spec changes ship as their own spec MR. This one rides along because the contradiction blocked this step's code. Say so if you'd rather split it out — it is one file and cleanly separable.
One knowing departure from the siblings
That spec edit also added auth_discovery_writes_total to the Observability
table, so docs/dev/observability.md gains a catalog row here even though no
emitter ships in this MR.
All three siblings do the opposite. S12's rows landed in the same MR as the
code registering them and the emitter exists. S14 and S15 both say outright
that catalog entries "are added to docs/dev/observability.md by the
implementing MR", and neither has any rows in the catalog. The catalog's own
preamble agrees: rows are added "when their wide events, metrics, or spans
ship".
S16 is the outlier and was before this MR — its other eight rows were seeded by the spec MR with no emitters anywhere, and its catalog sentence reads "live in" rather than the siblings' "are added by". So the choice was between two inconsistencies: nine rows and no emitters (matches S16, breaks the siblings), or eight rows and one absent metric (matches the siblings, leaves a hole a reader cannot explain). Took the first, because the spec's "live in" claim is checkable and was about to be false, and because reworking S16's other eight rows is a decision from the spec MR rather than this step's to reverse.
To flip it: drop the observability.md row and reword the spec's
Catalog entries ... sentence to S14's. One line each; Step 17 then adds all
nine rows. Happy either way — flagging it so it reads as a decision rather
than drift.
The colon rule is enforced at the store
The write-time : rejection (Auth-challenge criterion 8) is enforced by this
MR, which is a change from how the plan first scoped the step.
The plan wrote criterion 8 off as having no store half, on the grounds that
internal/datastore cannot import internal/format/oci. That is true of a
direct import and does not follow for the criterion: the spec's Resolutions had
already settled the shape as a narrow validator seam wired at the composition
root, and the seam was simply never built. Left that way, the rule would have
been enforced by nothing, with no signature on either side forcing a caller to
run it.
So ContainerRemoteCredentialStore takes a ContainerRemoteCredentialValidator
and the constructor panics on a nil one. Required rather than optional is the
point: an omitted validator fails open and stores exactly the usernames the rule
refuses, and nothing downstream can tell that it did. The call sits in
setCredentials, the unexported qrm.DB-accepting internal, not in the
exported SetCredentials — a composer of the url update and the credential
write drives the internal, so a check reachable only from the exported method
would skip that path.
What is left for S17 is one wiring line at the composition root, which already imports both sides. The plan's Gated bullet and its criterion-ownership table are corrected in this MR rather than left claiming the earlier scoping.
Verdict transitions are unguarded, deliberately
The no-token write carries no auth_status arm, so it may overwrite a bearer
verdict and whichever discovery commits last decides the row. Settled on
compareAndSetHealthStatus, the shipped twin pair over last_health_status in
the container and Maven repository stores: it admits every transition and
arbitrates concurrent writers by recency, never by preferring a value, so a
<> bearer arm here would be a shape no sibling has. The recency half needs a
timestamp column discovery does not have and whose migration has landed.
The per-image granularity exposure this leaves open is #543, and the pointer to
it now sits on RecordNoAuthDiscovery rather than only here and in the spec —
on the exported symbol whose contract it qualifies, with the statement comment
referring to that one site.
Two linter findings CI never surfaces
Both live in build-tagged files, which no pipeline lints, so review is the only place they appear. Both are deliberate:
duplon theCredentialManagersection ofcontainer_remote_credentials_integration_test.goagainst the Maven suite, plus the reciprocal finding it adds to the Maven file. The mirroring is what the dropped-subtest rule asks for.gosecG202 on the seed helper'ssoftDeleteExpr(...)interpolation. False positive — the helper returns the literal"NOW()"or"NULL"with no caller input, and the identical construct already exists in the Maven suite.
AppSec review
Answered in the confidential thread; the detail is there rather than here. Both findings were acted on, and neither recommended fix was applied as written — each has a defect:
- The colon-rule fix calls
oci.ValidateUpstreamCredentialsfrominternal/datastore, which is an import cycle and does not compile. The gap it describes was real, and is closed by the validator seam above. Its stated reason is also wrong on the facts: container credentials do not come from the WWW-Authenticate exchange (that yields the token endpoint) but fromremote.CredentialManager, the same path Maven's store sits behind. - The
auth_statusguard is proposed asAND (auth_status IS NULL OR auth_status = $none). The column issmallint DEFAULT 0 NOT NULL, so theIS NULLarm is dead and a fresh row reads0, not1— the predicate would reject the initial no-token write, losing the memoization the column exists for. Corrected to<> $bearerit builds, but still does not address the sequential case #543 describes, which needs no race. No predicate change; the requested code-level pointer to #543 is added.
Guardrails
- e2e scenario catalogs (
docs/testing/): no scenario added or affected. This step ships a datastore store that nothing constructs yet — S17 owns the composition-root wiring — so no request path reaches it and there is nothing to exercise end to end. The auth-challenge scenarios land with the token-exchange flow. - Conformance tests: not applicable — no OCI protocol behavior here.
- Configuration reference /
config.example.yaml: untouched. - Bruno / OpenAPI: untouched.
Verification
go build ./...,go vet -tags=integration,gofmt, andgolangci-lintall clean on both packages.- Untagged
go test ./internal/datastore/ ./internal/format/oci/— 2,538 pass. - Integration
-run TestContainerRemoteon./internal/datastore/— 214 pass. - Integration
./internal/format/oci/...— 1,515 pass. markdownlint-cli2,vale(0 errors), andlycheepass on all four changed docs.- Two mutation checks, each restored and verified by checksum. Dropping
AuthURLfrom the no-token builder reddens theSET (auth_url, auth_status)subtest alone. Removing thes.validatecall fromsetCredentialsreddens the new validator tests, unit and integration both.
Known drift, deliberately not fixed here
docs/dev/observability.md's oci_remote_upstream_responses_total row omits
unsupported_content from its outcome set; the spec's own table carries it.
Pre-existing on main and unrelated to this step's metric — flagging rather
than folding a second catalog correction into this MR.
Related to #288