chore(auth): development-stubs bootstrap validator and build-tag fence

Moaz Khalifarequested to merge
mkhalifa/authentication-stub-step-5 into main

Summary

Step 5 of plan docs/plans/2026-05-12-authentication-stub.md — the development-stubs bootstrap-token validator, the build-tag fence (now a depguard rule, mirroring !298 (merged)), and the binary wiring that mounts auth.Middleware at chain position #9. Depends on Step 2 (!266 (merged)) and Step 3 (!300 (merged)). Closes Spec section Acceptance Criteria items 3 and 4.

Implements:

  • internal/auth/bootstrap package (build-tag development_stubs): New() (auth.Validator, error) resolves AR_BOOTSTRAP_TOKEN via internal/secret.Resolve and, on an unconfigured env var, returns ErrTokenUnconfigured wrapped together with the underlying secret-resolver sentinel (secret.ErrNotFound for unset, internal/secret.ErrEmptyValue for empty / whitespace) so callers can discriminate via errors.Is; Validate uses crypto/subtle.ConstantTimeCompare; init() calls auth.Register(New).
  • .golangci.yaml adds the no-devstubs-in-prod depguard rule under the existing golangci-lint CI job. Mirrors !298 (merged)'s no-authtest-in-prod shape. Scopes files: to non-_test.go paths outside **/bootstrap/** and outside the build-tag-gated link shim cmd/artifact-registry/bootstrap_link_dev.go; lists internal/auth/bootstrap under deny: with a desc: naming the build-tag-fence consequence and the shim exemption.
  • cmd/artifact-registry/main.go: auth.MustResolve() in assembleApp, validator threaded into server.Deps.AuthValidator. The no-factory panic is the runtime backstop of the build-tag fence; a deferred recover at assembleApp's top converts it into a composition: resolving auth validator: ... error that flows through cli.Exit(1) and slog.Error("artifact-registry exited with error", ...). A no-op cli.Command.ExitErrHandler lets cmd.Run return the error instead of cli/v3's default HandleExitCoder short-circuit.
  • cmd/artifact-registry/bootstrap_link_dev.go (//go:build development_stubs): single-statement blank-import shim that links the bootstrap package into the dev binary so its init() runs and registers the validator factory with the auth seam. Without this link, a -tags=development_stubs build compiled but panicked at auth.MustResolve because nothing in the binary's build graph imported the package. The shim is exempted from no-devstubs-in-prod by file path (see the depguard bullet above).
  • internal/server/server.go: AuthValidator auth.Validator added to Deps; buildMiddleware wires auth.Middleware(v) immediately above the mux (runtime outer→inner: ResponseInterceptor → HeaderHardening → BodySize → RoutePattern → Auth → mux), matching spec § Middleware position.
  • docs/dev/configuration.md: one-paragraph note that the bootstrap-token validator is build-tag-gated; links to S08.

MR size justification

Diffstat: +1145 / -121 LOC across 15 files (against current main, including the review-feedback follow-up d5c3496). Plan estimated ~175 LOC. Above estimate because:

  • bootstrap_test.go is ~300 LOC — table-driven Validate (5 outcomes), three Unconfigured subtests with per-case inner-sentinel pinning, registration smoke test, and a 2001-iteration timing-variance test backed by a race_on_test.go / race_off_test.go build-tag pair for the raceEnabled constant.
  • Wiring landed in internal/server, not main.go. Plan-MR's Research Findings assumed a Go-stdlib main.go placeholder; S01 (composition root) landed between the plan MR and this step. Auth threads through server.Deps.AuthValidator and buildMiddleware. Cross-cutting changes: server.go (+35), server_test.go (+125 — includes AC#2a integration test + pre-existing test reroute through /foo/npm/* since auth 404s anything off the dispatch table), server_helpers_test.go (+42 — shared retry helper with authHeader parameter), main_test.go (-66 — trimmed to keep build-tag-agnostic helpers after the TestMain split below), main_default_test.go (+211 — TestMain factory registration, tokenMatchValidator, TestPanicWhenNoFactoryRegistered, TestAssembleApp_RecoverPathFlowsThroughCLIExit), main_dev_test.go (+32 — dev-tag TestMain setting AR_BOOTSTRAP_TOKEN so the bootstrap validator authenticates the lifecycle suites).
  • Post-author follow-up commits (7 of 12 total): doc-pairing, citation strip, AC#2a integration test, AC#4 subprocess panic test, nil-AuthValidator panic test, plan amendment, depguard swap, and the latest review-feedback bundle (d5c3496).

Test ratio is ~3.5x impl. Typical for security-boundary code: every constructor input variant, the timing assertion, the registration path, the wiring-site panic, the recover-wrapped startup path, and the chain-integration round-trip each carry dedicated coverage.

/validate-step + /review-branch findings addressed

  • BLOCKER doc pairing — 447b43e adds the bootstrap-validator build-tag note to docs/dev/configuration.md (Secrets section).
  • BLOCKER plan drift — 2b72be0 amends Step 5 Files/Tests/Scope entries to reflect the actual landing (internal/server wiring, the cmd/artifact-registry/main.go path correction, the test files for AC#2a + AC#4 + nil-validator, the race-flag build-tag pair). The contract is unchanged.
  • WARN doc citations — 5b7e059 strips docs/specs/S08-authentication.md Spec reference lines from bootstrap.go + bootstrap_test.go per the project's no-doc-cites-in-source convention; generic contract prose retained.
  • WARN AC#2a wiring proof — 76a9140 adds TestServer_UnauthenticatedV2_ReturnsChallenge: boots the full server chain over a real loopback listener and asserts an unauthenticated GET /v2/ returns 401 + the verbatim Bearer challenge with the loopback Host interpolated into the realm.
  • WARN AC#4 wiring-site panic — 1ff204d adds TestPanicWhenNoFactoryRegistered: re-exec's the test binary with AR_TEST_PANIC_SUBPROC=1; the TestMain branch under that env var skips registration and calls MustResolve, so the subprocess panics with the registry's auth: ... no factory registered text and exits with Go's panic code (2). Pins the build-tag fence's runtime backstop at the wiring site (the in-isolation panic contract is already pinned by internal/auth/registry_test.go from Step 2).
  • O4 nil-AuthValidator panic — 9700d65 adds TestServer_New_PanicsOnNilAuthValidator symmetric to the existing nil-Logger / nil-Config panic assertions on server.New.
  • W1 commit scope — 249d732 keeps refactor: without (auth) scope; squash-title at merge carries the chore(auth): prefix per plan.
  • Late-cycle review observation — 743da9e swaps the shell-script devstubs fence (scripts/check-no-devstubs.sh + lint:no-devstubs-in-prod CI job) for a depguard rule mirroring !298 (merged). See the dedicated section below.
  • Pre-mortem follow-ups (not blocking, captured for the team): a future S09 may share the development_stubs tag and need a registration-coordination story; an exhaustruct linter on Deps would catch a future test-helper drift; a static-analysis rule banning bytes.Equal / == on the bootstrap validator's secret type would catch a constant-time regression the timing test only catches at the order-of-magnitude scale.

Post-review-feedback updates (d5c3496)

Addresses six blocking threads from the latest review pass:

  • !308#note_3390096490 (link shim + depguard exemption) — added cmd/artifact-registry/bootstrap_link_dev.go blank-importing internal/auth/bootstrap so its init() runs in dev binaries; added !**/cmd/artifact-registry/bootstrap_link_dev.go to the no-devstubs-in-prod files: block.
  • !308#note_3390096549 (incorrect rationale for tokenMatchValidator) — reworded the docstring to name the real mismatch: authtest.Fake needs testing.TB for its empty-token guard; TestMain only has *testing.M.
  • !308#note_3390096594 (recover the MustResolve panic) — deferred recover at assembleApp's top wraps the panic as composition: resolving auth validator: .... The slog.Error("artifact-registry exited with error", ...) block in main() was previously dead code post cli/v3's default HandleExitCoder; setting cli.Command.ExitErrHandler to a no-op lets cmd.Run return the error so the slog line actually fires.
  • !308#note_3390096613 (preserve inner sentinel under ErrTokenUnconfigured) — bootstrap.New now returns fmt.Errorf("%w: %w", ErrTokenUnconfigured, inner). bootstrap_test.go table extended with a wantInner column so the inner sentinel (secret.ErrNotFound for unset, internal/secret.ErrEmptyValue for empty or whitespace-only) is pinned per case.
  • !308#note_3390096634 (timing test doesn't discriminate CT from ==) — swapped the mismatch input to a byte-0-differing variant; rewrote the docstring to scope the claim to a gross-regression smoke (per-call overhead dwarfs the bytewise compare; the cryptographic property is enforced by static analysis, not this test).
  • !308#note_3390096670 (exact exit-code assertion) — TestPanicWhenNoFactoryRegistered now asserts assert.Equal(t, 2, exitErr.ExitCode()) with an error message naming the two ways the test would otherwise silently regress (exit 1 = recover or cli.Exit fired; exit 0 = MustResolve returned).

Side effects and audit cleanup landed alongside the blocking fixes:

  • TestMain split by build tag. The shim links bootstrap into the dev test binary, which made TestMain's auth.Register double-register against bootstrap.init(). The default-tag TestMain, tokenMatchValidator, and TestPanicWhenNoFactoryRegistered moved into cmd/artifact-registry/main_default_test.go (!development_stubs); new main_dev_test.go (development_stubs) sets AR_BOOTSTRAP_TOKEN to the lifecycle token so the bootstrap validator authenticates the lifecycle suites.
  • New TestAssembleApp_RecoverPathFlowsThroughCLIExit subprocess test covers the full panic → recover → cli.Exit(1) → slog.Error chain end-to-end via a sibling subprocess that runs the same cli.Command shape main() uses.
  • Pre-existing Step-N / AC-#N citations stripped from bootstrap.go (step 4 in Validate's docstring), main_default_test.go (AC #4, Step 2's), and main.go (AC #4 in run's docstring), tightening the no-doc-cites-in-source policy that the earlier 5b7e059 strip missed.
  • ErrTokenUnconfigured and New docstrings in bootstrap.go document the double-wrap contract so future callers know they can discriminate via errors.Is.
  • docs/plans/2026-05-12-authentication-stub.md Step 5 refreshed: file list adds the link shim and the test-file split, acceptance describes the recover-wrapped chain, and the runtime-backstop / static-enforcement paragraphs match the landed shape.

Devstubs fence: depguard swap (743da9e)

The shell-script fence (scripts/check-no-devstubs.sh + lint:no-devstubs-in-prod CI job) is replaced by a no-devstubs-in-prod depguard rule under the existing golangci-lint CI job. The rule mirrors !298 (merged)'s no-authtest-in-prod shape, keeping all build-tag-fence enforcement in the same place every Go contributor already reads.

The original plan's "transitive unreachability" framing was overstated. Every transitive entry into the default build's dep graph bottoms out at one file's import statement; depguard catches that originator. Where depguard does not run (the default-tag build path where bootstrap has no compilable files), Go's own typecheck catches the same regression earlier with a "build constraints exclude all Go files" error. The two paths together cover every case the shell-script fence covered.

Verified matrix on this tree:

  • Clean tree under either tag set: golangci-lint run exits 0.
  • Non-test file importing bootstrap, default tag: typecheck error "build constraints exclude all Go files in internal/auth/bootstrap" fails the build before lint runs.
  • Non-test file importing bootstrap with -tags=development_stubs: depguard fires with the rule's desc: and exits non-zero.
  • Same file renamed to _test.go, same tag: depguard does not fire and the run exits 0.
  • cmd/artifact-registry/bootstrap_link_dev.go (the build-tag-gated link shim) imports bootstrap with -tags=development_stubs: depguard does not fire because the files: block exempts that path by name.

The runtime backstop (auth.MustResolve panicking with no factory registered) is unchanged in contract; assembleApp's deferred recover now surfaces it through cli.Exit(1) + slog.Error rather than as a raw panic, as called out in the "Post-review-feedback updates" section above.

Rebase note

The branch was rebased onto current main (post-Step 4 !302 (merged) and post-!298) before the depguard swap to absorb both changes cleanly. All commit SHAs in this MR were rewritten by that rebase; the orphan pre-rebase SHAs are no longer reachable from this branch's history. The history is otherwise unchanged in shape.

Plan-amendment commit

This branch carries plan edits in 2b72be0 (server-wiring landing), 9c741a2 (Step 5 Status row → !308 (merged)), 743da9e (depguard-swap sync), and d5c3496 (review-feedback refresh of Step 5 file list and acceptance). All edits stay within Step 5's authoring scope.

Spec coverage

The full spec-coverage table (acceptance criteria, error cases, security considerations, and the cross-step boundaries this step does and does not own) lives in the ba9eead test-author commit body. Retrievable with:

git log -1 --format=%B ba9eead

Closes the Step-5-owned portions of Acceptance Criteria items 3 (constant-time compare) and 4 (build-tag fence). AC-1, AC-2, and AC-5 are owned by Steps 3, 4, and 1 respectively.

Hook bypass note

Exactly one --no-verify commit on the branch — ba9eead test-author, the documented carve-out per CLAUDE.md § Guardrails. The panic-skeleton intentionally fails the go-test pre-commit hook by design; every other commit on the branch ran the full hook chain.

Edited by Moaz Khalifa

Merge request reports