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:

  • ContainerRemoteCredentialStore over container_remote_repositories' tmp_plaintext_username / tmp_plaintext_password, with read, set, and clear. Cleartext never comes back from a read — the store returns LabKit Secret values, pinned by a test that exercises six renderings including %#v and a JSON envelope.
  • RecordBearerDiscovery and RecordNoAuthDiscovery, both conditional on the url observed when discovery ran, so a discovery against a remote whose url has since moved is a no-op rather than a write against the new host.
  • ClearAuthDiscovery, unconditional on url, because a token-endpoint 404 has to clear a verdict on the url it was discovered against.
  • oci.ValidateUpstreamCredentials, the RFC 7617 colon rule, reached from the store through the ContainerRemoteCredentialValidator seam 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.

  1. auth_status and both its CHECKs. ADR-007 publishes auth_url as 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.
  2. tmp_plaintext_username / tmp_plaintext_password (text) where ADR-007 publishes encrypted_* (bytea). S13's interim scheme; encrypted_* returns with #417. The shipped Maven and npm stores already do this.
  3. Parent gate uses repositories.format IN (0, 3). ADR-007:153 publishes 0=container, 1=maven, 2=npm with no oci. The amendment to 0=docker, 1=maven, 2=npm, 3=oci is recorded at docs/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 on main, in container_remote_repositories.go and container_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:

  • dupl on the CredentialManager section of container_remote_credentials_integration_test.go against the Maven suite, plus the reciprocal finding it adds to the Maven file. The mirroring is what the dropped-subtest rule asks for.
  • gosec G202 on the seed helper's softDeleteExpr(...) 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.ValidateUpstreamCredentials from internal/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 from remote.CredentialManager, the same path Maven's store sits behind.
  • The auth_status guard is proposed as AND (auth_status IS NULL OR auth_status = $none). The column is smallint DEFAULT 0 NOT NULL, so the IS NULL arm is dead and a fresh row reads 0, not 1 — the predicate would reject the initial no-token write, losing the memoization the column exists for. Corrected to <> $bearer it 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, and golangci-lint all clean on both packages.
  • Untagged go test ./internal/datastore/ ./internal/format/oci/ — 2,538 pass.
  • Integration -run TestContainerRemote on ./internal/datastore/ — 214 pass.
  • Integration ./internal/format/oci/... — 1,515 pass.
  • markdownlint-cli2, vale (0 errors), and lychee pass on all four changed docs.
  • Two mutation checks, each restored and verified by checksum. Dropping AuthURL from the no-token builder reddens the SET (auth_url, auth_status) subtest alone. Removing the s.validate call from setCredentials reddens 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

Edited by Radamanthus Batnag

Merge request reports

Loading
Loading