2026-05-15 — Universe Onboarding PR1

Resumed /plans/universe-onboarding.md at PR1 (Broker config foundations: WorldConfig.PublicURL).

Changes

• tools/demarkus-broker/internal/broker/config.go — added WorldConfig.PublicURL string with a doc comment explaining the field belongs on the broker (single source of truth for the client-facing address), is optional, and that a blank value means "skip in /me/install."
• deploy/helm/demarkus-broker/values.yaml — documented publicURL in the commented worlds[] example with the rationale for setting it.
• deploy/helm/demarkus-broker/templates/secret-config.yaml — render publicURL: "<value-or-empty>" for every world. Always emitted so the YAML shape is stable across set/unset, and operators can grep for the field.
• tools/demarkus-broker/internal/broker/config_test.go — two new subtests: empty default round-trip + explicit mark:// value round-trip.
• deploy/helm/demarkus-broker/tests/secret-config_test.yaml — two new cases: blank renders as publicURL: "", operator-supplied value renders verbatim.

Verification

• go test ./internal/broker/ -run TestLoadConfig → 29/29 pass (2 new).
• helm unittest -f tests/secret-config_test.yaml . → 13/13 pass (2 new).
• bash pre-commit.sh → format, vet, lint clean across protocol/server/client/tools.

Scope discipline

No validation in PR1 — plan calls for "optional; validation only requires it when used." Shape validation (mark:// URL, port present, etc.) lives with the /me/install consumer in PR5.

KnownFields(true) means older broker binaries would reject the new chart's rendered config because of the unknown publicURL line. Chart appVersion gates broker upgrade as usual; not new for this PR.

Next

PR2 — broker discovery doc (GET /.well-known/openid-configuration proxying the IdP discovery with device-endpoint overrides). Resume at tools/demarkus-broker/internal/broker/discovery.go (new file) + route registration in server.go.

Pending review: should PR1 ship as a standalone PR or be folded into PR2? Plan calls for independent reviewability per PR, so default is standalone.

PR2 — Broker discovery doc

PR1 (#135) merged. Continuing through plan PR2: broker-hosted /.well-known/openid-configuration that proxies the IdP's discovery doc with broker-hosted endpoint overrides.

Changes

• tools/demarkus-broker/internal/broker/discovery.go (new) — Discovery type. Fetches <idpIssuer>/.well-known/openid-configuration once at NewDiscovery, parses into map[string]any so unknown fields proxy through unchanged, rewrites issuer + device_authorization_endpoint + token_endpoint to broker URLs, caches the rendered body with a 5-minute TTL. Lazy refresh on TTL expiry, stale-while-error on refresh failure. Constructor blocks on initial fetch so a misconfigured/unreachable IdP fails the broker at boot rather than at first /soul-join.
• tools/demarkus-broker/internal/broker/discovery_test.go (new) — 13 test cases via httptest fake-IdP: constructor validation (broker URL + issuer required), upstream-failure failure paths, override correctness, proxy of jwks_uri + userinfo_endpoint + authorization_endpoint + arrays, response headers, cache hit count within TTL, refresh after TTL, stale-while-error, recovery from transient failure, trailing-slash trim, malformed JSON rejection.
• tools/demarkus-broker/internal/broker/server.go — Server gains a discovery *Discovery field and NewServer takes it as a constructor arg. Routes() registers GET /.well-known/openid-configuration only when discovery != nil, so existing tests pass nil and skip the route cleanly.
• tools/demarkus-broker/internal/broker/server_test.go — TestWellKnownDiscoveryRouteRegistered (wires real Discovery into NewServer, asserts override applied through the mounted route) + TestWellKnownDiscoveryRouteSkippedWhenNil (404 when constructed nil).
• tools/demarkus-broker/main.go — NewDiscovery called between NewVerifier and NewServer; passes cfg.Server.PublicURL + cfg.OIDC.Issuer. Failure is fatal at startup, same shape as NewVerifier.
• tools/demarkus-broker/internal/broker/config.go — new ServerConfig.PublicURL field, required at load, trailing slash stripped once at load so every downstream consumer sees the canonical form. Doc comment notes it intentionally diverges from OIDC.RedirectURL (which is purpose-specific to the IdP redirect).
• tools/demarkus-broker/internal/broker/config_test.go — 2 new subtests: required-rejection + trailing-slash trim.
• deploy/helm/demarkus-broker/values.yaml — server.publicURL: "" with full operator-facing comment.
• deploy/helm/demarkus-broker/templates/secret-config.yaml — renders publicURL: {{ required ... }} so chart-render fails clearly when omitted (same shape as oidc.redirectURL).
• deploy/helm/demarkus-broker/tests/secret-config_test.yaml — 2 new cases (renders value, fail-render when blank) + added server.publicURL to the suite-wide set: block.
• deploy/helm/demarkus-broker/tests/deployment_test.yaml — same suite-wide set: addition (this suite also templates secret-config).
• deploy/kind/values-broker.yaml, deploy/kind/values-broker-argo.yaml — set server.publicURL: "http://localhost:8080" to mirror the existing redirectURL shape. Stage 2/4 harnesses don't exercise the well-known route but the broker's LoadConfig now requires the field.

Verification

• go test ./internal/broker/ -count=1 → all green (4.3s). 13 new Discovery tests + 2 new server route tests + 2 new config tests.
• helm unittest . → 55/55 pass across all 7 suites (was 52/52 before; 3 new).
• bash pre-commit.sh → format + vet + lint clean.

Design notes (worth remembering)

• Override shape is intentionally lossy at the seam. Override issuer to broker URL but leave jwks_uri pointed at the IdP. ID tokens during PR2/PR3 are still IdP-signed and carry IdP's iss. Strict client validation against this discovery doc would fail; the plugin's device-flow client (PR6) only consumes device_authorization_endpoint + token_endpoint from this doc so the mismatch is invisible to it. PR4 closes the gap by minting broker-signed id_tokens with a broker-hosted jwks_uri. Doc comment in discovery.go calls this out explicitly so a future reader doesn't trip over it.
• map[string]any proxy beats a typed struct. Tried a typed idpDiscovery struct first; killed it because real IdPs ship 20+ fields each (Google's code_challenge_methods_supported, Auth0's mfa_challenge_endpoint, Okta's request_object_signing_alg_values_supported, etc.) and the broker has no business knowing or filtering them. Decode → mutate three keys → re-encode is the right shape; tests assert both the overridden fields AND a couple proxy fields so regressions surface.
• Lazy refresh, not background goroutine. Plan §Risks talks about "5-minute TTL on the discovery cache." Two implementation shapes: background ticker, or in-handler lazy. Picked lazy because (a) one fewer goroutine lifecycle to wire into the broker shutdown path, (b) the load is human-scale — even with N replicas behind a Service, the per-instance refresh rate caps at 1/5min, (c) thundering herd on simultaneous expiry is negligible at this rate (two replicas might both refresh once; the IdP doesn't notice). Stale-while-error keeps the well-known endpoint available during transient upstream blips.
• Constructor signature pattern. NewDiscovery takes a DiscoveryConfig struct, not a long positional arg list — per /guidelines.md "more than 4 parameters is a smell." Same pattern as the existing Issuer/Verifier constructors take typed configs.
• Test isolation gotcha. Existing oidc_test.go already had fakeIdP + newFakeIdP. Renamed mine to fakeDiscoveryIdP + newFakeDiscoveryIdP rather than touch the existing symbol — both are test-package-scope so the rename is purely local. Worth keeping in mind if a third test file ever needs a fake IdP: extract to a shared testhelpers_test.go helper instead of accumulating per-suite copies.
• http.NoBody, not nil, on GET requests. gocritic flagged the nil body on http.NewRequestWithContext; switched to http.NoBody (the idiomatic singleton for empty request bodies). Same fix in tests using httptest.NewRequest.

Scope discipline

PR2 spec called for ~150 lines + tests. Final tally: ~210 lines of production code (discovery.go) + ~310 lines of tests + ~70 lines of config/chart plumbing + ~50 lines of dev-values updates. Over the 150-line estimate but every piece is load-bearing for PR3 (verification_uri uses brokerURL) and PR4 (broker-signed tokens use brokerURL as issuer).

Next

PR3 — broker device flow (RFC 8628). New file device.go with /device/authorize + /device (HTML form + /auth/login redirect glue) + /device/token polling. ~600 lines + tests, ~2 days. Will exercise the route registration path that PR2 sets up; will also be the first time the broker holds short-lived per-flow state in-memory (device_code map with janitor goroutine).

Open question to resolve before PR3

mock-oauth2-server device-code support — plan §Open Questions item 1. Need to check navikt/mock-oauth2-server v2.x source for /device_authorization endpoint before designing the kind Stage 5 harness in PR7. Not blocking PR3 itself (broker-side tests use a fake verifier), but if mock-oauth2-server doesn't support it, the kind end-to-end story diverges.

End of session — PR2 merged, PR3 plan written

• PR2 (#136, broker discovery doc) merged after two rounds of CodeRabbit feedback. Final state included Server.PublicURL config field (required, trimmed, absolute-URL validated), Discovery type with 5-min TTL caching + refresh-mutex stampede protection + hard fetch deadline, route registration on /.well-known/openid-configuration, 16 broker tests + 5 helm-unittest cases.
• Coderabbit lessons worth saving:
  • t.Fatalf inside spawned goroutines panics that goroutine without releasing pending channel sends. Concurrency tests need an error-returning helper that surfaces failures through the channel; the main goroutine asserts after <-. Wrote serveAndDecodeErr as the canonical pattern for this kind of test.
  • When a constructor accepts an injected *http.Client via config struct, that client may not have a Timeout set. Wrap the fetch context with context.WithTimeout inside the operation so the deadline holds regardless of what the caller injected. Belt-and-suspenders on top of the default client's own timeout.
  • helm install regression tests in CI (test-upgrade-wipe.sh invocation) need explicit --set for every newly-required chart value. Adding a required field to values.yaml without updating CI passes helm-unittest but fails the actual install regression. Worth grepping .github/workflows/ for helm install calls when adding a required field to any chart.
  • README install examples count as a test surface — coderabbit flagged the missing --set server.publicURL in the dev-install README block. Update README install commands alongside the chart change.
• PR3 plan published to /plans/universe-onboarding-pr3.md with full architecture, sub-task breakdown, open design questions, and tomorrow-morning resume steps. Index updated to surface it.
• Two design questions deferred to tomorrow's session:
  1. Verifier.Exchange signature change (lean: change it, three call sites, clean refactor) vs. parallel ExchangeWithTokens method. Latter is messier and we'd refactor anyway in PR4.
  2. RFC 8628 slow_down interval-increment policy. Lean: not for PR3 (RFC says MAY, not MUST); revisit if a customer reports IdP abuse.
• Tomorrow's first move: mark_fetch /plans/universe-onboarding-pr3.md, confirm the two open questions, branch from main, start at Step 1 (Verifier refactor) as its own commit.

PR3 implementation — broker device flow (RFC 8628)

Picked up PR3 cold from /plans/universe-onboarding-pr3.md. Both deferred design questions confirmed:

1. Verifier.Exchange signature changed to return ExchangeResult (Claims + RawIDToken + AccessToken + Expiry) — three call sites touched.
2. slow_down policy is "enforce configured interval, no incremental bump" — pure minimum-interval check in deviceStore.Poll.

Changes

• tools/demarkus-broker/internal/broker/oidc.go — added ExchangeResult struct; Verifier.Exchange returns it.
• tools/demarkus-broker/internal/broker/oidc_test.go — fakeVerifier gained rawIDToken/accessToken/expiry fields and the new return type.
• tools/demarkus-broker/internal/broker/server.go — Server gained deviceStore. Routes() registers POST /device/authorize, GET /device, POST /device, POST /device/token. The first and last sit behind ipRateLimit; /device form has no rate-limit middleware. authCallback gained a device-cookie early-return dispatch.
• tools/demarkus-broker/internal/broker/device.go (new) — four handlers + deviceCallback (the device branch of /auth/callback) + RunDeviceJanitor lifecycle + clearDeviceCookie helper. HTML templates embedded via embed.FS. ~280 lines.
• tools/demarkus-broker/internal/broker/device_store.go (new) — deviceStore state machine. Methods: Authorize, LookupByUserCode, LookupByDeviceCode, Bind, Deny, Poll, Sweep. User-code alphabet 30 chars (excludes 0 1 I L O U); 8-char codes formatted with a hyphen for display, canonicalized to no-hyphen-uppercase at lookup. ~290 lines.
• tools/demarkus-broker/internal/broker/templates/device_form.html, device_done.html (new) — server-rendered, embedded into the binary.
• tools/demarkus-broker/internal/broker/config.go — ServerConfig.DeviceCodeTTL + DevicePollInterval with defaults (10m / 5s). Extracted ServerConfig.applyDeviceFlowDefaults to keep Config.validate under the gocyclo budget.
• tools/demarkus-broker/main.go — calls srv.RunDeviceJanitor(sweepCtx) in the existing shutdown WaitGroup so the goroutine tears down with the sweeper.
• tools/demarkus-broker/internal/broker/device_store_test.go (new) — Authorize, Lookup, Bind, Deny, Poll (incl. slow_down + expiry), Sweep, canonicalizer round-trip, alphabet invariants. ~330 lines.
• tools/demarkus-broker/internal/broker/device_test.go (new) — handler tests (authorize, form GET/POST, token states), TestAuthCallbackUnchangedWithoutDeviceCookie regression guard, full end-to-end happy-path integration test, IdP-deny integration test, janitor cancel test. ~510 lines.

Verification

• go test -race ./internal/broker/ → green (~6s) across all 30 existing + new tests.
• helm unittest . (broker chart) → 55/55 still green; no chart changes in PR3.
• bash pre-commit.sh → format + vet + lint clean. Initial run flagged 5 lint issues (gocritic hugeParam on Bind, gocyclo on validate, two minmax, one rangeint) — all addressed: Bind takes *ExchangeResult, validate extracted helper, max() replacements, for range N loop.

Design decisions that landed differently from the plan

• deviceStore.Bind takes *ExchangeResult (not value). Plan showed it as value. gocritic's hugeParam flagged the 120-byte value at every call site. Pointer keeps the API as cheap as value semantics without changing ownership (*result is copied into deviceCodeState.Result). All call sites pass &exchange / &ExchangeResult{...}.
• User-code alphabet is 30 characters, not 32. Plan said "32-char alphabet (no I, L, O, 0, 1, U)" but 36 − 6 = 30. Used 30; collision space at TTL-aligned issue rates is still negligible (30^8 ≈ 6.6×10^11). Crockford base32's standard alphabet preserves 0 and 1; the plan's exclusion list was the load-bearing constraint, not the alphabet size.
• Cookie clearing must happen BEFORE WriteHeader. First pass used defer s.clearDeviceCookie(w). Integration test caught it: WriteHeader flushes headers, so the deferred SetCookie is silently dropped. Restructured to explicit clearDeviceCookie(w) calls at each early-return + before the render.
• State cookie is also cleared on the device branch. Plan mentioned device cookie only. Mirrored the browser path's state-cookie clear (Path=/auth/callback, MaxAge=-1) so a replay can't reuse the nonce.
• Janitor is a plain time.NewTicker loop, no leader election. Per-replica in-memory state means each replica sweeps its own store; no shared resource to elect on. Lifecycle shares sweepCtx with the Kubernetes-side Sweeper for a single shutdown signal.
• Open Question 1 (user-denial branch): YES, translate. /auth/callback?error=... is detected before the Exchange call. deviceStore.Deny(deviceCode) is invoked and the done page renders. Polling client sees access_denied on next poll instead of waiting for expired_token. ~10 lines.
• Open Question 7 (client_id requirement on /device/authorize): accept-but-ignore. POST /device/authorize reads client_id from the form (RFC-compliant client), logs at debug, but does not gate on it. Broker is the only relying party in this flow.

Risks specific to PR3 that did NOT bite

• Cross-cookie path confusion (state cookie at /auth/callback, device cookie at /) — guarded by TestAuthCallbackUnchangedWithoutDeviceCookie, regression-safe.
• Verifier interface refactor breakage — rg confirmed only three call sites; refactor was a single commit's worth of mechanical changes.
• In-memory store loss on broker restart — documented in deviceStore doc comment; acceptable for 10-minute TTL.

Next

PR3 ready to open. No commits made — Fritz handles those. Branch is still main; he'll cut a feat-tools-broker-device-flow (or similar) branch before committing. PR4 (broker-signed id_tokens + refresh tokens via ExchangeResult plumbing) is the natural next sub-plan.

PR3 merged (#137) — review lessons

CodeRabbit ran two rounds before approving. Lessons worth saving:

Security / correctness regressions caught

• Stale-device-cookie dispatch hijack. First-pass dispatch on /auth/callback keyed on the device cookie alone — a cookie that lived DeviceCodeTTL long. Abandoning the flow before /auth/callback left the cookie in the jar; a later legitimate browser /auth/callback would silently route through deviceCallback and bind the wrong grant. Fix: extended the signed State struct with DeviceCode string, consume the device cookie at /auth/login, dispatch on state.DeviceCode in /auth/callback. The cookie is now a one-shot pass-through; the State cookie is the authoritative signal. Pattern worth remembering: never dispatch on an ambient cookie when a signed state value is available.
• Origin-only CSRF check passes scheme-mismatched origins. http://broker.example.com matched r.Host == "broker.example.com" even though that's cross-origin. Fix: compare scheme+host, derive expected scheme from X-Forwarded-Proto / r.TLS. Pattern: scheme is part of origin in the spec; host-only same-origin checks are broken by construction.
• Deny on Exchange failure. Conflated user-denied-at-IdP with broker-side transient failure. A network blip during Exchange permanently marked the device_code as access_denied to the polling client. Fix: only the explicit ?error= query branch maps to Deny; everything else leaves the grant pending so the client retries or sees truthful expired_token. Pattern: terminal states must reflect what actually happened, not the closest convenient enum value.
• Bearer tokens cacheable by default. POST /device/token success response didn't carry Cache-Control: no-store. Standard OAuth2 §5.1 posture. Easy miss when writing JSON helpers.
• RFC 8628 client_id is REQUIRED, not optional. The plan §Open Question 7 leaned "accept-but-ignore". CodeRabbit pushed back citing the spec; the right answer is require presence, don't validate content — filters malformed clients without putting weight on a useless registration check.

Process lessons

• Header writes after WriteHeader are silently dropped. First-pass cookie clearing in deviceCallback used defer s.clearDeviceCookie(w) — the defer fired after renderDeviceDone had already flushed headers, so the Set-Cookie never landed. Integration test caught it. Always set headers BEFORE the first body write or WriteHeader call.
• Sed-with-newlines breaks on multiline calls. Used sed to rewrite store.Bind(deviceCode, ExchangeResult{...}) to store.Bind(deviceCode, &ExchangeResult{...}) and missed one site where the struct literal spanned multiple lines. Always grep after a sed-based refactor.
• gocritic hugeParam flags 120-byte value parameters. Took ExchangeResult by value at deviceStore.Bind; lint flagged. Switched to *ExchangeResult with a copy-into-state-on-store-side. Cleaner API anyway.
• Test config divergence from production validation. Tests construct Config{} directly without calling validate(), so new required fields (DeviceCodeTTL, DevicePollInterval) need fallback defaults in NewServer as well as validate(). Production-only validation paths surprise tests.
• CSRF gate compatibility with Go's http.Client. Go's client doesn't send Origin on POST by default. The check is "if Origin present, must match" — non-browser clients (curl, Go's client) hit the no-Origin branch and pass, which is correct (CSRF needs a puppeted user-agent).

Final state

• 8 commits squashed-merged as d0697b9. ~1100 LOC production + ~870 LOC tests across device.go, device_store.go, device_test.go, device_store_test.go, templates, config, server, oidc, main.
• All four PRs of the universe-onboarding plan now merged in order: #135 (PR1), #136 (PR2), #137 (PR3). Plan §PR4 next.

PR4 implementation — broker refresh-grant + JWKS + composite verifier

Cold-started PR4 after #138 (Steps 1+2) merged. Resumed at Step 3 with Open Question 1 resolved as (b) broker re-signs at refresh time — Fritz's "never ship anything broken" directive ruled out the cached-id_token-verbatim path that would have returned a guaranteed-expired bearer.

Changes

Step 3 — IDTokenSigner + JWKS + composite verifier + refresh-grant

• tools/demarkus-broker/internal/broker/idtoken.go (new) — IDTokenSigner wraps ECDSA P-256, signs JWTs with kid header, exposes PublicJWK() and VerifyIDToken(). PKCS#8 + SEC1 PEM accepted. Pinned to ES256.
• tools/demarkus-broker/internal/broker/jwks.go (new) — GET /.well-known/jwks.json. Body rendered once at construction (no rotation in PR4); Cache-Control max-age=300 matches discovery doc.
• tools/demarkus-broker/internal/broker/oidc.go — added compositeVerifier that wraps a primary (IdP) Verifier with broker-key verification leg. Dispatch policy: broker-first (no network, kid-match short-circuit); any failure BEFORE kid match returns ErrIDTokenKidUnknown → fall through to IdP; any failure AFTER kid match (bad sig, bad iss, expired) is terminal. Also: NewVerifier(ctx, *OIDCConfig) — pointer because PR4's BrokerSigningKey tipped the struct over gocritic's hugeParam threshold.
• tools/demarkus-broker/internal/broker/discovery.go — jwks_uri now overridden to broker. Doc comment updated to call out the iss/key consistency (broker as both issuer and key authority).
• tools/demarkus-broker/internal/broker/device.go — deviceToken now dispatches on grant_type: deviceTokenDeviceFlow (existing PR3 path) vs deviceTokenRefresh (new). Refresh response returns broker-signed JWT in BOTH id_token and access_token (same string — PR5's /me/install accepts either; the alternative was a broken/empty access_token).
• tools/demarkus-broker/internal/broker/server.go — Server.idTokenSigner + Server.jwks + JWKS route registration. NewServer now takes *IDTokenSigner (nilable for tests); when non-nil it wraps the supplied Verifier in compositeVerifier transparently. Test helper newTestServerWithSigner for refresh-grant-specific tests.
• tools/demarkus-broker/main.go — calls NewIDTokenSigner after LoadConfig, passes through to NewServer. Logs the kid for operator audit.
• tools/demarkus-broker/internal/broker/config.go — OIDCConfig.BrokerSigningKey (PEM string) + ServerConfig.IDTokenTTL (default 15m). BROKER_SIGNING_KEY env-var override added alongside OIDC_CLIENT_SECRET so the PEM travels through an externally-managed Secret via secretKeyRef rather than baking into helm release history.

Step 4 — RFC 7009 /token/revoke

• tools/demarkus-broker/internal/broker/revoke.go (new) — POST /token/revoke under ipRateLimit. Idempotent: unknown / already-revoked → 204 (no enumeration oracle). Missing token form param → 400 invalid_request. token_type_hint accepted and ignored. Anonymous endpoint — possession of the token is the authz signal.

Step 5 — Sweeper integration

• tools/demarkus-broker/internal/broker/refresh.go — exported refreshStore → RefreshStore so both Server and Sweeper can hold the same instance. Field on Server stays lowercase (private); exposed via Server.RefreshStore() method for main.go to wire into the Sweeper.
• tools/demarkus-broker/internal/broker/sweeper.go — Sweeper.refreshStore field (optional, nilable for back-compat). Per-tick loop calls refreshStore.Sweep after the issuance sweep; logs the count.
• tools/demarkus-broker/main.go — NewSweeper(issuer, srv.RefreshStore(), interval, log).

Step 6 — config validation + Helm chart wiring

• tools/demarkus-broker/internal/broker/config.go — applyRefreshDefaults + OIDCConfig.validate() extracted to keep Config.validate under the gocyclo budget. New validation: BrokerSigningKey required; RefreshTokenTTL default 90d; IDTokenTTL default 15m; IDTokenTTL < RefreshTokenTTL invariant (degenerate config rejected).
• deploy/helm/demarkus-broker/values.yaml — server.refreshTokenTTL, server.idTokenTTL, server.refreshTokensSecret, oidc.brokerSigningKey, oidc.existingSigningKeyRef all documented.
• deploy/helm/demarkus-broker/templates/secret-config.yaml — renders the new server fields + brokerSigningKey. Mutual-exclusion guard mirrors clientSecret (set cleartext OR existingSigningKeyRef, not both).
• deploy/helm/demarkus-broker/templates/secret-refresh-tokens.yaml (new) — seeds empty refresh-tokens Secret with helm.sh/resource-policy: keep, same shape as secret-issuances.yaml.
• deploy/helm/demarkus-broker/templates/rbac-broker-ns.yaml — broker SA's get/update on secrets now covers BOTH issuances + refresh-tokens names.
• deploy/helm/demarkus-broker/templates/deployment.yaml — BROKER_SIGNING_KEY env mounted via secretKeyRef when existingSigningKeyRef.name is set.
• deploy/helm/demarkus-broker/templates/_helpers.tpl — demarkus-broker.refreshTokensSecretName helper.
• deploy/kind/values-broker.yaml + values-broker-argo.yaml — static test PEM baked in for kind harness.
• deploy/helm/demarkus-broker/README.md — install example documents brokerSigningKey + openssl genpkey recipe.
• .github/workflows/ci.yml — test-upgrade-wipe.sh invocation for demarkus-broker now passes --set oidc.brokerSigningKey=test-pem-placeholder.

Tests added

• idtoken_test.go: 8 tests — sign/verify round-trip, expired, wrong iss, foreign kid (ErrIDTokenKidUnknown), bad signature, PKCS#8 + SEC1 acceptance, non-ECDSA rejection, P-384 rejection, PublicJWK public-only invariant.
• jwks_test.go: 2 tests — handler serves public key with kid+alg+use, JWKS fetch round-trip.
• oidc_test.go: 5 composite-verifier tests — pass-through AuthCodeURL+Exchange, broker-signed VerifyIDToken, fall through on unknown kid, NO fall through on broker-side signature fail (security gate), nil-signer pass-through.
• device_test.go: 3 refresh-grant tests — happy path, error matrix (missing/empty/unknown token), revoked token rejection, cross-grant isolation. Plus deviceCallback refresh-mint-failure-leaves-pending test (Step 2 fixup).
• revoke_test.go: 5 tests — valid, unknown is no-op (no enumeration), missing/empty token rejection, token_type_hint accepted, idempotency.
• sweeper_test.go: 2 tests — refresh-token sweep removes expired only, nil-refreshStore is no-op.
• config_test.go: 8 new validate cases — required key, TTL defaults, TTL overrides, TTL invariants, Secret name default+override.
• helm-unittest: 17 new chart tests (secret-config: 11 covering TTLs/signing-key modes/mutual-exclusion; secret-refresh-tokens: 5; deployment: 2 for BROKER_SIGNING_KEY env wiring). 72/72 pass.

Design decisions worth remembering

• Composite verifier broker-first, no IdP fallback on signature fail. Earlier draft fell through to IdP on any broker-side error; CodeRabbit-style threat model says no — a tampered token with the broker's kid that the IdP's permissive verifier accepts would be a forgery. Only "not a broker-shaped JWT" (parse fail, wrong alg, kid mismatch) defers; everything past kid-match is terminal.

• PEM via env var, not file mount. Multi-line PEMs travel fine through k8s secretKeyRef → env var — kubelet preserves newlines. File-mount path would have required a new config field for the path; env var keeps OIDCConfig stable.

• access_token == id_token. Refresh response sets both to the same broker-signed JWT. Spec-fuzzy in OAuth2 vs OIDC, but PR5's /me/install accepts either; an empty access_token would force a per-broker client quirk. Same-value is honest: the broker issues one identity bearer.

• Exported RefreshStore (was lowercase). Step 5 needed Sweeper to share the same instance with Server. Three options considered: (a) move construction up to main.go (signature churn), (b) Sweeper takes a closure (leaks abstraction), (c) export the type + add Server.RefreshStore() getter. Picked (c) — smallest surface change.

• IDTokenTTL < RefreshTokenTTL config invariant. A degenerate config where the bearer outlives its refresh credential cannot renew. Rejected at LoadConfig validate so the pod CrashLoopBackOffs at startup rather than the first refresh poll.

• Kind harness ships a static PEM. Reproducible builds need a stable kid across rebuilds. Generated once with openssl genpkey; never used in production. Same posture as the dev cookieKey baseline.

Verification

• go test -race -count=1 ./internal/broker/ → all green (~6s, 100+ tests after PR4)
• helm unittest . (broker chart) → 72/72 pass (was 55; +17 PR4 surfaces)
• helm lint . → clean (info-level warnings on chart icon, unrelated)
• bash pre-commit.sh → format + vet + lint clean across protocol/server/client/tools

Next

PR opens against feat-tools-broker-refresh-grant. After review/merge: PR5 (/me/install) consumes the broker-signed id_token as a bearer — broker's compositeVerifier accepts both broker-signed (refresh-renewed) and IdP-signed (device-code-completion) tokens, so PR5's requireAuth works on either. The PR4 broker is the resource server for refresh tokens; world tokens remain on DELETE /tokens/{label} as before.

PR4 review round — 11 CodeRabbit comments addressed

PR139 opened with the PR4 implementation; CodeRabbit flagged 11 issues. Worked through them with craftsman bar (no shortcut "minimal patch" fixes — each addressed at the right architectural layer).

Code-side bugs (5)

• NewVerifier(nil) panic (oidc.go). PR4 changed the signature to *OIDCConfig; nil callers crashed instead of returning a structured error. Added nil-guard at the boundary + TestNewVerifierRejectsNilConfig.
• NewServer discovery-without-signer invariant (server.go). Discovery overrides jwks_uri to broker (PR4 change); if idTokenSigner is nil, the doc advertises a route that isn't registered. Fail-fast panic at construction + TestNewServerPanicsOnDiscoveryWithoutSigner. Existing TestWellKnownDiscoveryRouteRegistered updated to provide a signer.
• Parse-at-validate for BrokerSigningKey (config.go). OIDCConfig.validate() now calls NewIDTokenSigner and propagates parse errors as oidc.brokerSigningKey is invalid: …. Catches malformed PEMs at LoadConfig instead of letting them survive past OIDC discovery + kube client setup, where the error message is noisier and burns startup time.
• TestLoadConfig env leak (config_test.go). applyEnvOverrides reads BROKER_SIGNING_KEY and OIDC_CLIENT_SECRET; ambient env from CI or a developer masks the missing-YAML test path. t.Setenv("BROKER_SIGNING_KEY", "") + same for OIDC_CLIENT_SECRET at test start.
• Revoke 500 was text/plain (revoke.go). RFC 7009 §2.2.1 says JSON. http.Error → writeJSON(deviceTokenError{Error: "server_error"}) + reactor-driven TestTokenRevokeServerErrorIsJSON that forces a k8s outage and asserts the JSON shape.

Security hygiene (3, all about PEMs in repo)

• Kind harness PEMs removed. Both deploy/kind/values-broker.yaml and values-broker-argo.yaml previously embedded a static test PEM. Replaced with existingSigningKeyRef.name: broker-signing-key. deploy/kind/up.sh got ensure_broker_signing_key() which generates a fresh ECDSA P-256 PEM per harness run, applies it via kubectl create secret ... --dry-run | kubectl apply -f - (idempotent), and cleans up the temp file via a function-scoped trap RETURN.
• Helm test fixture sentinel. tests/secret-config_test.yaml's "cleartext brokerSigningKey renders" test had a PEM-shaped string with real BEGIN PRIVATE KEY markers. Secret scanners flagged it. Replaced with TEST_BROKER_SIGNING_KEY_LINE_* synthetic sentinels.
• Go test fixture PEM via init(). validConfig const had a "test-pem-placeholder" literal that worked when validate didn't parse, but parse-at-validate broke it. Converted to a var initialized in func init() that generates an ephemeral ECDSA P-256 PEM and substitutes it into the YAML template. No PEM lives as a checked-in string; scanners are quiet; parse-at-validate sees real material. Companion validConfigNoSigningKey for the "brokerSigningKey is required" test case so it doesn't depend on brittle string replacement against the embedded PEM.

CI / docs (3)

• CI workflow generates real PEM per run. ci.yml's test-upgrade-wipe step uses mktemp + openssl genpkey + helm --set-file oidc.brokerSigningKey="$tmpfile" + trap 'rm -f' EXIT. PEM is ephemeral; no shell-history leak.
• README cleanup. Dropped "PR4:" internal prefix from a user-facing comment; added -out broker-key.pem to the openssl example so users copying it actually save the key.

Design decisions worth keeping

• Parse-at-validate vs late-in-main. Moving NewIDTokenSigner call into OIDCConfig.validate cost microseconds at startup and bought a clean operator-facing error surface. A malformed PEM now fails with oidc.brokerSigningKey is invalid: parse PKCS#8 private key: … at LoadConfig — referenceable directly in chart docs — instead of after OIDC discovery and kube client setup have already produced noisy startup logs.
• NewServer panic, not error. Discovery-without-signer is a programming-error contract violation, not a recoverable runtime condition. NewServer's signature stays error-less; the failure is loud and obvious. Tests can defer func() { recover() }() to assert.
• Ephemeral test PEMs in init(). Same approach pays off twice: secret scanners don't trip on test fixtures, AND parse-at-validate has real material to verify. Pattern is var x string; func init() { x = generateAndSubstitute(...) }.
• trap '...' RETURN for function-scoped cleanup in bash. Function-local, fires on both normal-return AND set -e propagation. Empirically verified against set -euo pipefail + a forced mid-function false: trap fires, temp file is removed, function returns nonzero. Cleaner than the 4-branch if/return-1 pattern CodeRabbit initially proposed.

PR4 merged (#139) — workflow-YAML ${{ }} quirk

PR139 merged. CI on main immediately failed: Invalid workflow file: .github/workflows/ci.yml#L1 (Line: 180, Col: 14): An expression was expected.

Root cause: I'd written ${{ }}-safe inside a shell comment in a run: block, intending to note that the surrounding logic was safe against literal-tilde substitution. GitHub Actions parses every run: block byte-by-byte for ${{ … }} expressions BEFORE shell-comment semantics apply. An unmatched ${{ opener is a syntax error, period.

Fix on fix-ci-broker-signing-key-comment: rewrote the comment to avoid the ${{ token entirely + added a meta-note inside the file pointing at the issue so future edits don't reintroduce it. Validated grep -c '\${{' == grep -c '}}' == 5 (all balanced, legitimate expressions). Merged.

Lesson

Workflow YAML comments are NOT shell comments to GitHub Actions. The parser doesn't know # starts a shell comment — it scans the literal bytes for expression syntax. Any ${{ in a run: block requires a matching }} even inside a #-prefixed line. pre-commit.sh doesn't catch this because it doesn't run actionlint. Worth adding actionlint to pre-commit (or a CI prevalidate step) as a follow-up.

Status

• All four PRs of the universe-onboarding plan merged in order: #135 (PR1), #136 (PR2), #137 (PR3), #139 (PR4 — broker refresh tokens + JWKS + revoke + sweeper integration + Helm wiring). #138 was PR4-Steps-1+2 merged separately mid-implementation.
• Plan §PR5 next. Sub-plan published to /plans/universe-onboarding-pr5.md with full architecture, sub-task breakdown, open design questions, and next-session resume steps.
• Tomorrow's first move: per the PR5 sub-plan's §Next-Session Resume Steps.

Related documents

• Universe onboarding plan: PR1 through PR4 implemented in this session
• Universe onboarding PR3 sub-plan: device flow plan written and executed here
• Universe onboarding PR5 sub-plan: next sub-plan published at end of session
• Guidelines: constructor parameter rule cited for NewDiscovery