diff --git a/devenv.lock b/devenv.lock index 951c7a2d4..c8afb143f 100644 --- a/devenv.lock +++ b/devenv.lock @@ -426,7 +426,8 @@ "go-overlay": "go-overlay", "hk": "hk", "nix2container": "nix2container", - "nixpkgs": "nixpkgs" + "nixpkgs": "nixpkgs", + "secretspec-nixpkgs": "secretspec-nixpkgs" } }, "rust-overlay": { @@ -450,6 +451,22 @@ "type": "github" } }, + "secretspec-nixpkgs": { + "locked": { + "lastModified": 1788549839, + "narHash": "sha256-kOrCcSIA6w9J1hX5DqHy2k9pDTJymExTsbV74U9UtCA=", + "owner": "NixOS", + "repo": "nixpkgs", + "rev": "17de0b976395537756f30a3e78f2f06e5cec89ed", + "type": "github" + }, + "original": { + "owner": "NixOS", + "ref": "nixpkgs-unstable", + "repo": "nixpkgs", + "type": "github" + } + }, "systems": { "locked": { "lastModified": 1681028828, diff --git a/devenv.nix b/devenv.nix index 920943bc7..12716fec8 100644 --- a/devenv.nix +++ b/devenv.nix @@ -238,6 +238,21 @@ in pkgs.cloud-hypervisor pkgs.virtiofsd pkgs.passt + ] + # secretspec: the CLI the Go secrets write path spawns BY NAME for + # `set`/`delete` (go/internal/secrets/resolver.go's `cli` default), so the + # write path is unreachable unless this shell puts one on PATH. Resolved from + # the `secretspec-nixpkgs` input rather than this shell's own nixpkgs because + # that channel's rev still carries 0.14.0, which has no `age` provider + # compiled in — the encrypted-at-rest default the server-secret resolver + # writes through. This input's version matches the Go SDK pin in go/go.mod, so + # the read path (SDK + native lib) and the write path (this CLI) advance + # together; `internal/secrets` asserts both halves rather than assuming them. + # A dotted input reference, so it is appended OUTSIDE the parsed `with pkgs` + # literal (same reason as skopeo-nix2container: the toolchain-parity gate + # resolves every bare attr in that literal, including on macOS). + ++ [ + inputs.secretspec-nixpkgs.legacyPackages.${pkgs.stdenv.system}.secretspec ]; env = { diff --git a/devenv.yaml b/devenv.yaml index 72e249a2e..ddb917169 100644 --- a/devenv.yaml +++ b/devenv.yaml @@ -54,3 +54,20 @@ inputs: inputs: nixpkgs: follows: nixpkgs + # secretspec-nixpkgs: a SECOND nixpkgs, pinned by rev in devenv.lock, solely + # for the `secretspec` CLI the Go secrets write path spawns by name. The + # `age://` provider it needs to write encrypted-at-rest secrets only exists + # from 0.15 on (it is a default-on cargo feature), and the rolling channel + # this shell's own nixpkgs is locked to still resolves 0.14.0 — a build with + # no `age` backend compiled in at all, which fails a write with `Provider + # backend 'age' not found` rather than degrading. This input tracks the + # channel that carries a version matching the Go SDK pin (go/go.mod's + # secretspec module), so the read path (SDK) and the write path (CLI) move + # together instead of skewing across an independent seam. It deliberately + # does NOT `follows: nixpkgs` — following would defeat the entire purpose by + # collapsing it back onto the rev that lacks the provider. Consumed as a + # dotted attr in devenv.nix, OUTSIDE the parsed `with pkgs` packages literal, + # because the toolchain-parity gate resolves every bare attr in that literal + # (the same reason skopeo-nix2container sits outside it). + secretspec-nixpkgs: + url: github:NixOS/nixpkgs/nixpkgs-unstable diff --git a/docs/designs/DECISIONS.md b/docs/designs/DECISIONS.md index 408c520d5..cdfe672ec 100644 --- a/docs/designs/DECISIONS.md +++ b/docs/designs/DECISIONS.md @@ -72,13 +72,13 @@ check enforces the mechanical half. Full rationale: | DL-018 | The frozen `RunnerService` transport recommendation #2 is socket-only, superseding the earlier off-stdio carrier clause | Active (Matt, 2026-07-22) | [agent comms tools §The frozen transport](agent/compass-agent-comms-tools/design.md#the-frozen-transport-this-rides-was-the-keystone-fork-now-decided) | | DL-313 | NATS is the single eventing substrate — run as a standalone stack service (alongside Postgres and the OTel collector) in every deployment, reached over `nats://`, with no embedded/in-process mode and no transport phase; single-node or clustered NATS is selected by connection string, never by application code. JetStream is the durable comms-delivery transport; core NATS carries routing/binding invalidation; queue groups partition delivery work. Connect stays the synchronous RPC edge; the agent↔Runner hop stays vsock (RIG-2394); no LISTEN/NOTIFY phase. Supersedes DL-014 and DL-021 (RIG-2861 OQ-1) | Active (Matt, 2026-08-31) | [multi-tenancy & NATS substrate](infra/runtime/compass-managed-multitenancy/design.md#q3--the-eventing-substrate-one-nats-eventfabric-a-standalone-stack-service-jetstream-as-the-delivery-transport) | | DL-316 | Server↔Runner transport is TWO-PLANE (amends DL-013's Runner↔Server clause, RIG-2861 OQ-5 Variant B): async command-push + Runner event fan-in ride NATS (per-Runner command subjects + queue-group fan-in — `Sessions`/`PublishEvents` reshaped to pub/sub for the non-sticky-wake fabric); the typed request/reply legs (enrollment, unary `Relay*Call`s/`CommitConversationFrame`/`FetchSecrets`, bulk `FetchAgentConfig`) stay on the reduced Connect/gRPC edge (deadline propagation, typed proto errors, generated stubs); the per-Runner provisioned token is retained as the NATS-credentials seed via auth-callout. Client↔Server stays Connect; Runner↔Agent stays vsock. Supersedes DL-013 | Active (Matt, 2026-08-31) | [multi-tenancy & NATS substrate](infra/runtime/compass-managed-multitenancy/design.md#resolved-decisions-freeze--matt-2026-08-31) | -| DL-327 | The delivery work-queue consumer's JetStream ack for a HELD deliver (agent-authored message held until its author's session settles) is ACK-ON-RECEIVE (OQ-1): the fabric callback acks as soon as the message is classified and held/dispatched, not at fire — the held registry stays in-RAM and a crash between hold and fire recovers via the Postgres delivery-cursor sweep, exactly as today's in-process bus. Ack-on-fire is rejected (AckWait=30s << an agent turn ⇒ healthy held messages redeliver mid-turn and DLQ-park after MaxDeliver=5, absent per-message InProgress heartbeats) | Active (Matt, 2026-09-05) | [delivery cutover §OQ-1](infra/runtime/compass-managed-delivery-cutover/design.md#oq-1-load-bearing-jetstream-ack-timing-for-held-delivers) | -| DL-328 | The migrated delivery consumer runs `onEventRef` (re-read + classify + hold/dispatch) DIRECTLY on the fabric callback goroutine under `c.mu` + the per-session gates (OQ-2), concurrent with the settle/start drain loop — NOT enqueued onto a single goroutine. The relaxed cross-channel ordering is one today's non-deterministic `select` never actually guaranteed; the cursor sweep remains the no-loss floor. Two Option-A obligations are invariants, not forks: `scanMissedMentions`'s held-check and `MarkMentionsRouted` run under one critical section (scan-vs-hold), and per-callback dispatch work stays bounded so the ack does not block past AckWait behind a long `sweepSession` | Active (Matt, 2026-09-05) | [delivery cutover §OQ-2](infra/runtime/compass-managed-delivery-cutover/design.md#oq-2-load-bearing-concurrency-model--callback-direct-vs-loop-enqueue) | -| DL-329 | The delivery consumer splits its DB role once each event carries an explicit `ref.Tenant` (OQ-3 part 1): the inherently cross-tenant background sweeps/drains keep `WithSystemRole` (BYPASSRLS), but per-event processing runs under `store.WithTenant(baseCtx, ref.Tenant)` for the `MessageByID` re-read and the whole `onMessagePosted` chain — and `heldEntry` gains a `tenant` field so the `fireHeld` re-read is tenant-scoped too. Fail-closed: a forged/corrupted ref whose row belongs to another tenant reads zero rows under RLS instead of cross-tenant-delivering under BYPASSRLS. Whole-loop system-role (ref.Tenant routing-only) rejected — forfeits the stamped ref's isolation dividend | Active (Matt, 2026-09-05) | [delivery cutover §OQ-3](infra/runtime/compass-managed-delivery-cutover/design.md#oq-3-load-bearing-rls-scope-split--lag-recovery-replacement) | -| DL-330 | Recovery after a publish-SIDE fabric failure (commit ok, publish fails — the loss mode the infallible bus never had) is triggered by a FABRIC-RECONNECT HOOK plus a minutes-scale PERIODIC FLOOR TICK, each running `sweepAllLive` + `scanMissedMentions` (OQ-3 part 2). This replaces the deleted `sub.Lagged()` bus-ring branch. The draft's original mapping was falsified by the design-critic red-team: `scanMissedMentions` routes only mentions/ask-answers (never plain delivers) and NATS auto-reconnect (`MaxReconnects(-1)`) keeps the ConsumeContext alive so a "re-subscribe" trigger never fires across an outage — leaving a publish-failed plain deliver to an always-live agent silently undelivered until its next session restart. `sweepAllLive` (not just the scan) is the load-bearing plain-deliver recovery path; publisher-side bounded retry MAY be added but is not sufficient alone | Active (Matt, 2026-09-05) | [delivery cutover §OQ-3 part 2](infra/runtime/compass-managed-delivery-cutover/design.md#oq-3-load-bearing-rls-scope-split--lag-recovery-replacement) | +| DL-334 | The delivery work-queue consumer's JetStream ack for a HELD deliver (agent-authored message held until its author's session settles) is ACK-ON-RECEIVE (OQ-1): the fabric callback acks as soon as the message is classified and held/dispatched, not at fire — the held registry stays in-RAM and a crash between hold and fire recovers via the Postgres delivery-cursor sweep, exactly as today's in-process bus. Ack-on-fire is rejected (AckWait=30s << an agent turn ⇒ healthy held messages redeliver mid-turn and DLQ-park after MaxDeliver=5, absent per-message InProgress heartbeats) | Active (Matt, 2026-09-05) | [delivery cutover §OQ-1](infra/runtime/compass-managed-delivery-cutover/design.md#oq-1-load-bearing-jetstream-ack-timing-for-held-delivers) | +| DL-335 | The migrated delivery consumer runs `onEventRef` (re-read + classify + hold/dispatch) DIRECTLY on the fabric callback goroutine under `c.mu` + the per-session gates (OQ-2), concurrent with the settle/start drain loop — NOT enqueued onto a single goroutine. The relaxed cross-channel ordering is one today's non-deterministic `select` never actually guaranteed; the cursor sweep remains the no-loss floor. Two Option-A obligations are invariants, not forks: `scanMissedMentions`'s held-check and `MarkMentionsRouted` run under one critical section (scan-vs-hold), and per-callback dispatch work stays bounded so the ack does not block past AckWait behind a long `sweepSession` | Active (Matt, 2026-09-05) | [delivery cutover §OQ-2](infra/runtime/compass-managed-delivery-cutover/design.md#oq-2-load-bearing-concurrency-model--callback-direct-vs-loop-enqueue) | +| DL-336 | The delivery consumer splits its DB role once each event carries an explicit `ref.Tenant` (OQ-3 part 1): the inherently cross-tenant background sweeps/drains keep `WithSystemRole` (BYPASSRLS), but per-event processing runs under `store.WithTenant(baseCtx, ref.Tenant)` for the `MessageByID` re-read and the whole `onMessagePosted` chain — and `heldEntry` gains a `tenant` field so the `fireHeld` re-read is tenant-scoped too. Fail-closed: a forged/corrupted ref whose row belongs to another tenant reads zero rows under RLS instead of cross-tenant-delivering under BYPASSRLS. Whole-loop system-role (ref.Tenant routing-only) rejected — forfeits the stamped ref's isolation dividend | Active (Matt, 2026-09-05) | [delivery cutover §OQ-3](infra/runtime/compass-managed-delivery-cutover/design.md#oq-3-load-bearing-rls-scope-split--lag-recovery-replacement) | +| DL-337 | Recovery after a publish-SIDE fabric failure (commit ok, publish fails — the loss mode the infallible bus never had) is triggered by a FABRIC-RECONNECT HOOK plus a minutes-scale PERIODIC FLOOR TICK, each running `sweepAllLive` + `scanMissedMentions` (OQ-3 part 2). This replaces the deleted `sub.Lagged()` bus-ring branch. The draft's original mapping was falsified by the design-critic red-team: `scanMissedMentions` routes only mentions/ask-answers (never plain delivers) and NATS auto-reconnect (`MaxReconnects(-1)`) keeps the ConsumeContext alive so a "re-subscribe" trigger never fires across an outage — leaving a publish-failed plain deliver to an always-live agent silently undelivered until its next session restart. `sweepAllLive` (not just the scan) is the load-bearing plain-deliver recovery path; publisher-side bounded retry MAY be added but is not sufficient alone | Active (Matt, 2026-09-05) | [delivery cutover §OQ-3 part 2](infra/runtime/compass-managed-delivery-cutover/design.md#oq-3-load-bearing-rls-scope-split--lag-recovery-replacement) | | DL-331 | SINGLE-INSTANCE is a transitional deployment constraint for the delivery cutover (OQ-4, design-critic HIGH): the cutover moves the delivery TRIGGER cross-instance (durable queue-group single-claim) but the dispatch plane it feeds — `SessionForAccount`/`LiveAgentSessions`, the held registry, settle edges, per-session gates — is instance-local hub RAM, so a two-instance deploy breaks hold/settle for a message claimed by the instance not hosting the author's session (immediate fire from partial mid-turn blocks, late-block mention loss, undelivered settled suffix). A single Server is assumed until the parent record's durable session bindings land (parent T4, sequenced after this cutover); this record's two-instance integration proof is scoped to fabric TRANSPORT claim semantics only, not multi-instance delivery correctness | Active (Matt, 2026-09-05) | [delivery cutover §OQ-4](infra/runtime/compass-managed-delivery-cutover/design.md#oq-4-load-bearing-cross-instance-session-locality-single-instance-transitional-constraint) | | DL-332 | Publishing `message_posted` on BOTH the in-process `events.Bus` (client gRPC stream + presence, until the client edge migrates) and the fabric (delivery) during the phased, multi-step migration is NOT a violation of DL-313's "one eventing substrate — NATS only": that constraint bans a second SWAPPABLE `EventFabric` implementation (an in-process channel impl of the seam), not the pre-existing bus coexisting during the phased cutover. The two publishes serve disjoint consumer sets (no consumer reads both), so no double-handling occurs; the transitional shape ends when the client edge migrates and the bus retires. Interpretation frozen here (per the red-team) so the bus-retirement inherits it explicitly | Active (Matt, 2026-09-05) | [delivery cutover §double-publish](infra/runtime/compass-managed-delivery-cutover/design.md#why-the-double-publish-is-not-a-global-constraint-violation) | -| DL-333 | The OQ-3-part-2 recovery trigger's fabric-reconnect hook (DL-330) needs a seam the frozen 3-method `EventFabric` (DL-316/DL-313 — `Publish`/`Subscribe`/`SubscribeKind`) does not expose; the fabric's own `ReconnectHandler` is log-only, set once at `New()`, and REPLACED (not chained) via `Config.Options` (a replacing caller loses the fabric's outage diagnostics). Ruling: ADD an `EventFabric` method `OnReconnect(fn func()) (Unsubscribe, error)` on the interface and `*Fabric`, chained onto the fabric's existing reconnect handler so its log survives, reached by the delivery consumer through the interface value it holds. Grows the seam to four methods (accepted, consistent with the `SubscribeKind` precedent). Assembly-side wiring through `Config.Options` rejected — splits the trigger across packages and re-implements the fabric's diagnostics | Active (Matt, 2026-09-05) | [delivery cutover §OQ-3](infra/runtime/compass-managed-delivery-cutover/design.md#oq-3-load-bearing-rls-scope-split--lag-recovery-replacement) | +| DL-333 | The OQ-3-part-2 recovery trigger's fabric-reconnect hook (DL-337) needs a seam the frozen 3-method `EventFabric` (DL-316/DL-313 — `Publish`/`Subscribe`/`SubscribeKind`) does not expose; the fabric's own `ReconnectHandler` is log-only, set once at `New()`, and REPLACED (not chained) via `Config.Options` (a replacing caller loses the fabric's outage diagnostics). Ruling: ADD an `EventFabric` method `OnReconnect(fn func()) (Unsubscribe, error)` on the interface and `*Fabric`, chained onto the fabric's existing reconnect handler so its log survives, reached by the delivery consumer through the interface value it holds. Grows the seam to four methods (accepted, consistent with the `SubscribeKind` precedent). Assembly-side wiring through `Config.Options` rejected — splits the trigger across packages and re-implements the fabric's diagnostics | Active (Matt, 2026-09-05) | [delivery cutover §OQ-3](infra/runtime/compass-managed-delivery-cutover/design.md#oq-3-load-bearing-rls-scope-split--lag-recovery-replacement) | ## Storage diff --git a/docs/designs/agent/compass-agent-container-runtime.md b/docs/designs/agent/compass-agent-container-runtime.md index 396519ea5..78367f8f8 100644 --- a/docs/designs/agent/compass-agent-container-runtime.md +++ b/docs/designs/agent/compass-agent-container-runtime.md @@ -711,7 +711,7 @@ repo manifest, no grants table). All types redact like `Credentials` generic channels; `SecretGH` rows carry `Host string` (default `github.com`) so T5 routes them to `GHCredentials.SetupScript` (Decision 3's gh placement), never the generic file path. - - `type Resolver interface { Resolve(ctx context.Context, reason string) ([]ResolvedSecret, error); Set(ctx context.Context, name, value string) error; Delete(ctx context.Context, name string) error }` + - `type Resolver interface { Resolve(ctx context.Context, reason string) ([]ResolvedSecret, error); Set(ctx context.Context, name, value, reason string) error; Delete(ctx context.Context, name string) error }` — `Resolve` resolves the **whole registry** (inject-all; a `names []string` parameter returns with the future grants seam); `Set`/`Delete` are the provider **write** path T7's diff --git a/docs/designs/infra/runtime/compass-managed-delivery-cutover/design.md b/docs/designs/infra/runtime/compass-managed-delivery-cutover/design.md index 76c33008a..ec7cc253e 100644 --- a/docs/designs/infra/runtime/compass-managed-delivery-cutover/design.md +++ b/docs/designs/infra/runtime/compass-managed-delivery-cutover/design.md @@ -3,7 +3,7 @@ Status: Active Ratified: OQ-1..OQ-4 decided by Matt (2026-09-05, see Resolved decisions); frozen on merge Parent: `docs/designs/infra/runtime/compass-managed-multitenancy/design.md` (frozen), T3 -Ledger-impact: appends DL-327..333 for the OQ-1/OQ-2/OQ-3/OQ-4 rulings, the reconnect-seam shape, and the double-publish interpretation (design-ledger-gate) +Ledger-impact: appends DL-331..337 for the OQ-1/OQ-2/OQ-3/OQ-4 rulings, the reconnect-seam shape, and the double-publish interpretation (design-ledger-gate) ## Problem / Intent @@ -132,7 +132,7 @@ an IMMEDIATE in-process trigger (`sub.Lagged()` → `sweepAllLive` + with the lag branch; and (2) `scanMissedMentions` routes ONLY mentions + ask-answers (`scan.go:35-70`), never plain delivers, so it recovers a publish-failed PLAIN message not at all. **What triggers full-set recovery after -a publish-side failure is resolved by OQ-3 part 2 / DL-330: a fabric-reconnect +a publish-side failure is resolved by OQ-3 part 2 / DL-337: a fabric-reconnect hook plus a minutes-scale periodic floor tick, each running `sweepAllLive` + `scanMissedMentions`.** No outbox table in this PR — Postgres remains "the sole durability source of @@ -404,7 +404,7 @@ exists anymore) and re-derive the no-loss argument from JetStream durability. overrun. - **Test cycle:** a red-green unit test that a publish-failed PLAIN (non-mention) message to a live, never-restarting recipient IS recovered by - the ruled trigger — this is the DL-330 silent-stall hole the red-team + the ruled trigger — this is the DL-337 silent-stall hole the red-team promoted to CRITICAL, so the record's headline recovery ruling ships with a test proving it closes; a test that the start-time scan still runs before the first event; and a test that `OnReconnect`'s chained callback fires the @@ -417,7 +417,7 @@ exists anymore) and re-derive the no-loss argument from JetStream durability. - **Interfaces:** consumes `docs/designs/DECISIONS.md`; this record. Produces: the changelog entry and this record's cross-references. The DL rows for the - ratified OQ rulings and the reconnect-seam shape (DL-327..333, incl. the + ratified OQ rulings and the reconnect-seam shape (DL-331..337, incl. the double-publish-is-not-a-Global-Constraint-violation interpretation) landed WITH this record's own freeze PR per the "Ledger delta owed" Global Constraint — they are NOT re-produced here (the append-only unique-ID rule @@ -445,7 +445,7 @@ exists anymore) and re-derive the no-loss argument from JetStream durability. (`sweepAllLive` / `scanMissedMentions`) per OQ-3 part 2; PRODUCES the reconnect seam (does not exist yet); lands in T2's PR; plain-deliver recovery test -- [ ] T6: changelog + record cross-references (DL-327..333 already landed with +- [ ] T6: changelog + record cross-references (DL-331..337 already landed with this record's freeze PR) ## Resolved decisions diff --git a/docs/designs/server/compass-gateway-credentials-at-rest-encryption.md b/docs/designs/server/compass-gateway-credentials-at-rest-encryption.md index 7bcc578d6..35d34d439 100644 --- a/docs/designs/server/compass-gateway-credentials-at-rest-encryption.md +++ b/docs/designs/server/compass-gateway-credentials-at-rest-encryption.md @@ -107,11 +107,11 @@ key from `crypto/rand` and — serialized against concurrent booters through a Postgres advisory lock (T2) — provisions it: - writes the value into the provider via `secrets.Resolver.Set` - (`go/internal/secrets/resolver.go:219`, `func (r *SpecResolver) Set(ctx - context.Context, name, value string) error` — "Set writes value into the - provider for name via the pinned CLI, feeding the value on stdin (never + (`go/internal/secrets/resolver.go:238`, `func (r *SpecResolver) Set(ctx + context.Context, name, value, reason string) error` — "Set writes value into + the provider for name via the pinned CLI, feeding the value on stdin (never argv, so it is not visible in the host process list)", - resolver.go:206-207); + resolver.go:216-217); - registers the name in the SEPARATE `server_secrets` store (D6) via the server-internal `DeclareServerSecret` (T0) — a mirror of `store.DeclareSecret` (`go/internal/store/secrets.go:82`, `func (s *Store) @@ -805,9 +805,10 @@ is declared into it) and T1. - `func provisionGatewayMasterKey(ctx context.Context, resolver secrets.Resolver, st *store.Store) (envelope.Key, error)` — `resolver` is the SERVER-SECRET resolver instance (T0). Resolve `GATEWAY_CREDENTIALS_MASTER_KEY` through it; on absence: - `envelope.NewKey()` → `resolver.Set(ctx, name, encodedKey)` - (resolver.go:219; the value rides stdin, never argv, - resolver.go:206-207) → `st.DeclareServerSecret(ctx, "", name)` with + `envelope.NewKey()` → `resolver.Set(ctx, name, encodedKey, "compass: + provision gateway credentials master key")` + (resolver.go:238; the value rides stdin, never argv, + resolver.go:216-217) → `st.DeclareServerSecret(ctx, "", name)` with `declared_by = NULL` (server-provisioned; T0's nullable FK). No delivery, no kind — those columns do not exist on `server_secrets`. - **Concurrency — advisory-lock serialized (mandatory):** the whole diff --git a/flake.nix b/flake.nix index be50dcd8f..1d08a22d7 100644 --- a/flake.nix +++ b/flake.nix @@ -53,7 +53,7 @@ # touched (guest-image/default.nix:82-87). vendorHash pins the fetched set — # the whole module graph, so it matches guestd's proxyVendor hash. Recompute # with lib.fakeHash on a go.mod/go.sum move. - vendorHash = "sha256-GHZsEfvnu1tY6Bd7Fxg7SEWEI+HS0NlQuBbm6pz/UK4="; + vendorHash = "sha256-FsKtsXc6t9FkxxlIXRgjXyqzel/KLMWo70ve1+lnxbI="; in { packages = forAllSystems ( diff --git a/go/go.mod b/go/go.mod index 460e6873e..a25568c6c 100644 --- a/go/go.mod +++ b/go/go.mod @@ -19,7 +19,7 @@ require ( connectrpc.com/cors v0.1.0 connectrpc.com/otelconnect v0.9.0 github.com/BurntSushi/toml v1.6.0 - github.com/cachix/secretspec/secretspec-go v0.15.0 + github.com/cachix/secretspec/secretspec-go v0.20.0 github.com/hashicorp/golang-lru/v2 v2.0.7 github.com/insomniacslk/dhcp v0.0.0-20260728151720-c308df0fdcef github.com/jackc/pgx/v5 v5.10.0 diff --git a/go/go.sum b/go/go.sum index ea81bd03b..d1b05fbf6 100644 --- a/go/go.sum +++ b/go/go.sum @@ -10,6 +10,8 @@ github.com/adrg/xdg v0.5.3 h1:xRnxJXne7+oWDatRhR1JLnvuccuIeCoBu2rtuLqQB78= github.com/adrg/xdg v0.5.3/go.mod h1:nlTsY+NNiCBGCK2tpm09vRqfVzrc2fLmXGpBLF0zlTQ= github.com/cachix/secretspec/secretspec-go v0.15.0 h1:DMxh5/hkgZyMysSyFzf9RwUxoj+NfgmlnS6UKJ8n0k4= github.com/cachix/secretspec/secretspec-go v0.15.0/go.mod h1:4QjSax/Qd3JXEqNFUwvov93jnhVD1PmcDbeQBLg2r9Y= +github.com/cachix/secretspec/secretspec-go v0.20.0 h1:bPLSWJV85EC2DDPrtChkr3beXv1lRG67KsG8PoT+0Zg= +github.com/cachix/secretspec/secretspec-go v0.20.0/go.mod h1:4QjSax/Qd3JXEqNFUwvov93jnhVD1PmcDbeQBLg2r9Y= github.com/cenkalti/backoff/v5 v5.0.3 h1:ZN+IMa753KfX5hd8vVaMixjnqRZ3y8CuJKRKj1xcsSM= github.com/cenkalti/backoff/v5 v5.0.3/go.mod h1:rkhZdG3JZukswDf7f0cwqPNk4K0sa+F97BxZthm/crw= github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= diff --git a/go/internal/runnerhub/secrets_test.go b/go/internal/runnerhub/secrets_test.go index 4ab28e7f4..cffd03216 100644 --- a/go/internal/runnerhub/secrets_test.go +++ b/go/internal/runnerhub/secrets_test.go @@ -43,8 +43,8 @@ func (f *fakeResolverSecrets) Resolve(_ context.Context, _ string) ([]secrets.Re return f.set, nil } -func (f *fakeResolverSecrets) Set(_ context.Context, _, _ string) error { return nil } -func (f *fakeResolverSecrets) Delete(_ context.Context, _ string) error { return nil } +func (f *fakeResolverSecrets) Set(_ context.Context, _, _, _ string) error { return nil } +func (f *fakeResolverSecrets) Delete(_ context.Context, _ string) error { return nil } // runnerResolverForFetch is the token resolver the FetchSecrets door uses: it // accepts a single Runner token and rejects everything else, modelling the real diff --git a/go/internal/secrets/resolver.go b/go/internal/secrets/resolver.go index 7e7527466..2019dade9 100644 --- a/go/internal/secrets/resolver.go +++ b/go/internal/secrets/resolver.go @@ -22,6 +22,12 @@ const manifestProject = "compass" // configured — the Server owns one project, one profile. const defaultProfile = "default" +// defaultCLI is the SecretSpec binary the write path spawns by name, resolved +// off PATH (the dev shell and the deployed image both stage it). Named so the +// drift guard asserting the staged binary's version floor and the resolver +// agree on which binary that is. +const defaultCLI = "secretspec" + // declarations is the read surface the Resolver needs from the store: the whole // declared set. store.Store satisfies it. An interface (not the concrete // *store.Store) so the pure resolve logic is unit-testable with a fake, without @@ -42,8 +48,11 @@ type Resolver interface { // hold it). Resolve(ctx context.Context, reason string) ([]ResolvedSecret, error) // Set writes a value into the provider for an already-declared name. The - // value is fed to the pinned CLI over stdin, never argv. - Set(ctx context.Context, name, value string) error + // value is fed to the pinned CLI over stdin, never argv. reason is recorded + // in the SecretSpec audit log and is required: an empty reason is rejected + // before the CLI is spawned, so the audit reason travels with every write + // exactly as it does on the read path. + Set(ctx context.Context, name, value, reason string) error // Delete removes a value from the provider for a name. Delete(ctx context.Context, name string) error } @@ -88,7 +97,7 @@ func NewSpecResolver(st declarations, stateDir string, opts ...SpecOption) *Spec store: st, profile: defaultProfile, stateDir: stateDir, - cli: "secretspec", + cli: defaultCLI, } for _, opt := range opts { opt(r) @@ -140,14 +149,10 @@ func (r *SpecResolver) Resolve(ctx context.Context, reason string) ([]ResolvedSe if len(decls) == 0 { return nil, nil } - // Normalize the profile once so the manifest header and the resolving + // One accessor for the profile so the manifest header and the resolving // profile can never diverge: buildManifest emits [profiles.] and - // the SDK resolves the same . Empty (explicit WithProfile("")) maps - // to defaultProfile, exactly as buildManifest's own fallback would. - profile := r.profile - if profile == "" { - profile = defaultProfile - } + // the SDK resolves the same . + profile := r.resolvedProfile() manifestPath, err := r.writeManifest(profile, decls) if err != nil { return nil, err @@ -206,17 +211,26 @@ func (r *SpecResolver) Resolve(ctx context.Context, reason string) ([]ResolvedSe // Set writes value into the provider for name via the pinned CLI, feeding the // value on stdin (never argv, so it is not visible in the host process list). // The SDK is read-shaped, so the write path shells the CLI. name must be a -// valid secret name; an empty value is rejected up front. +// valid secret name; an empty value and an empty reason are both rejected up +// front. reason is recorded in the SecretSpec audit log and can be required by +// the provider policy, so it travels with every write exactly as it does on +// the read path. // -// Verified against secretspec v0.15.0 source (secrets.rs:1635-1647, compass -// ruling RIG-1327 f63edea3): `set ` with the value omitted from argv and -// stdin not a tty takes the piped-stdin branch — a first-class -// io::stdin().read_to_string() with no interactive prompt constructed — then -// trims the value and rejects an empty one. So `secretspec set -// --provider

--profile

` with the value on stdin is the write path, no -// positional VALUE. The live exec is exercised at T7 where a staged CLI binary -// is available; construction (argv + stdin plan) is unit-tested here. -func (r *SpecResolver) Set(ctx context.Context, name, value string) error { +// The write is pointed at a generated manifest through the global --file flag, +// the same explicit-manifest treatment Resolve gives the read path: the +// registry is the source of truth and no secretspec.toml is committed, so a +// CLI left to discover one walks up from the process cwd and finds nothing. +// The generated manifest declares exactly the name being written. +// +// Verified against secretspec v0.20.0 source (secrets.rs:4423-4427 for the +// piped-stdin branch and trim, :4430-4433 for empty-value rejection): `set +// ` with the value omitted from argv and stdin not a tty takes the +// piped-stdin branch — a first-class io::stdin().read_to_string() with no +// interactive prompt constructed — then trims the value and rejects an empty +// one. So `secretspec --file --reason set --provider

+// --profile

` with the value on stdin is the write path, no positional +// VALUE. +func (r *SpecResolver) Set(ctx context.Context, name, value, reason string) error { if err := ValidateName(name); err != nil { return err } @@ -225,7 +239,23 @@ func (r *SpecResolver) Set(ctx context.Context, name, value string) error { if strings.TrimSpace(value) == "" { return fmt.Errorf("secrets: set %q: value is empty", name) } - args := r.setArgs(name) + // The reason is the audit record, and the CLI's own require_reason policy is + // an environment heuristic (it gates on agent-env detection), so an omitted + // reason makes the same write succeed on one host and be refused on another. + // Screen it here for a deterministic caller error instead. + if strings.TrimSpace(reason) == "" { + return fmt.Errorf("secrets: set %q: reason is empty", name) + } + // The CLI loads the profile's declared set from a manifest; generate one + // declaring just this name rather than letting it search the process cwd. + manifestPath, err := r.writeManifest(r.resolvedProfile(), []store.SecretDeclaration{{Name: name}}) + if err != nil { + return err + } + // A transient input to the CLI, exactly as on the read path — remove it once + // the write returns; the registry, not this file, is the durable source. + defer func() { _ = os.Remove(manifestPath) }() + args := r.setArgs(name, reason, manifestPath) //nolint:gosec // G204: the SecretSpec write seam — spawns the operator-pinned // secretspec CLI (r.cli) with a Runner-assembled argv whose only variable is // the secret name, validated against the env-var-name grammar (ValidateName) @@ -253,10 +283,24 @@ func (r *SpecResolver) Delete(ctx context.Context, name string) error { return nil } +// resolvedProfile is the SecretSpec profile every invocation runs under: the +// pinned profile, or defaultProfile when none is configured (an explicit +// WithProfile("")). One accessor for both paths so the generated manifest +// header and the profile the CLI/SDK acts under can never diverge. +func (r *SpecResolver) resolvedProfile() string { + if r.profile == "" { + return defaultProfile + } + return r.profile +} + // setArgs builds the argv for the write path (pure, so it is unit-testable // without executing the binary). The value never appears here — it rides stdin. -func (r *SpecResolver) setArgs(name string) []string { - args := []string{"set", name} +// --file and --reason are global flags, accepted on either side of the `set` +// subcommand; both are emitted before it as the canonical, unambiguous +// position, and a test pins that ordering so the argv shape stays stable. +func (r *SpecResolver) setArgs(name, reason, manifestPath string) []string { + args := []string{"--file", manifestPath, "--reason", reason, "set", name} if r.provider != "" { args = append(args, "--provider", r.provider) } diff --git a/go/internal/secrets/resolver_test.go b/go/internal/secrets/resolver_test.go index a2b53915d..6fc9565fd 100644 --- a/go/internal/secrets/resolver_test.go +++ b/go/internal/secrets/resolver_test.go @@ -11,8 +11,10 @@ import ( "context" "io" "os" + "os/exec" "path/filepath" "slices" + "strconv" "strings" "sync" "testing" @@ -80,25 +82,32 @@ func TestBuildManifest(t *testing.T) { } func TestSetArgs(t *testing.T) { - // Bare resolver: just the verb and name, no provider/profile flags, and - // crucially no VALUE anywhere in the argv. + const reason = "compass: unit test write" + const manifest = "/tmp/state/secretspec-123.toml" + + // Bare resolver: the two global flags, the verb and the name, no + // provider/profile flags, and crucially no VALUE anywhere in the argv. bare := NewSpecResolver(nil, "/tmp/state", WithProfile("")) - got := bare.setArgs("API_KEY") + got := bare.setArgs("API_KEY", reason, manifest) const setVerb = "set" - if want := []string{setVerb, "API_KEY"}; !equalArgs(got, want) { + if want := []string{"--file", manifest, "--reason", reason, setVerb, "API_KEY"}; !equalArgs(got, want) { t.Errorf("setArgs bare = %v, want %v", got, want) } // Provider + profile set → their flags appear; the name is still the only - // positional after the verb. + // positional after the verb, and both globals still lead the argv. full := NewSpecResolver(nil, "/tmp/state", WithProvider("keyring://"), WithProfile("production")) - gotFull := full.setArgs("API_KEY") - if want := []string{setVerb, "API_KEY", "--provider", "keyring://", "--profile", "production"}; !equalArgs(gotFull, want) { + gotFull := full.setArgs("API_KEY", reason, manifest) + want := []string{ + "--file", manifest, "--reason", reason, setVerb, "API_KEY", + "--provider", "keyring://", "--profile", "production", + } + if !equalArgs(gotFull, want) { t.Errorf("setArgs full = %v, want %v", gotFull, want) } // The value must NEVER be in the constructed argv — it rides stdin. - for _, a := range full.setArgs("API_KEY") { + for _, a := range gotFull { if strings.Contains(a, "the-secret-value") { t.Errorf("value leaked into argv: %v", gotFull) } @@ -208,23 +217,30 @@ func TestSetEmptyValueRejected(t *testing.T) { // front as a deterministic caller error, before shelling out. r := NewSpecResolver(nil, "/tmp/state") for _, empty := range []string{"", " ", "\t", "\n", " \n\t "} { - if err := r.Set(context.Background(), "API_KEY", empty); err == nil { + if err := r.Set(context.Background(), "API_KEY", empty, "compass: unit test write"); err == nil { t.Errorf("Set with empty value %q = nil, want an error", empty) } } // A bad name is still rejected first, independent of value. - if err := r.Set(context.Background(), "bad-name", "value"); err == nil { + if err := r.Set(context.Background(), "bad-name", "value", "compass: unit test write"); err == nil { t.Error("Set with invalid name = nil, want an error") } } -// TestSecretSpecVersionPin is a drift guard: the resolver's stdin/trim/empty- -// reject write contract and the runtime FFI dlopen were verified against -// secretspec-go v0.15.0 source (compass ruling RIG-1327 f63edea3). If a devenv -// fork-sync moves the pin, this fails loudly so the set() contract is re-checked -// against the new source rather than silently drifting. +// TestSecretSpecVersionPin is a drift guard for the SDK HALF of the secretspec +// seam only — the module version in go.mod, which governs the read path (the +// builder API and the native lib it dlopens). It says nothing about the CLI the +// write path spawns; TestSecretSpecCLIVersionFloor guards that half, and the +// two can drift independently because the read and write paths cross different +// seams (SDK vs shelled binary). +// +// The resolver's stdin/trim/empty-reject write contract and the runtime FFI +// dlopen were verified against secretspec v0.20.0 source (secrets.rs:4423-4427 +// for the piped-stdin branch and trim, :4430-4433 for empty-value rejection). +// If a devenv fork-sync moves the pin, this fails loudly so the set() contract +// is re-checked against the new source rather than silently drifting. func TestSecretSpecVersionPin(t *testing.T) { - const wantVersion = "v0.15.0" + const wantVersion = "v0.20.0" const modulePath = "github.com/cachix/secretspec/secretspec-go" // Assert the pin at its source of truth, the module's go.mod — deterministic @@ -246,7 +262,61 @@ func TestSecretSpecVersionPin(t *testing.T) { t.Fatalf("%s not found in go.mod; expected it pinned at %s", modulePath, wantVersion) } if got != wantVersion { - t.Fatalf("secretspec-go pinned at %s, want %s — the write-path contract (stdin/trim/empty-reject) was verified against %s; re-verify set() semantics against the new source before moving the pin (RIG-1327 f63edea3)", got, wantVersion, wantVersion) + t.Fatalf("secretspec-go pinned at %s, want %s — the write-path contract was verified against secretspec v0.20.0 source (secrets.rs:4423-4427 for piped stdin and trim, :4430-4433 for empty rejection); re-verify set() semantics against the new source before moving the pin", got, wantVersion) + } +} + +// TestSecretSpecCLIVersionFloor guards the CLI half of the seam: the write path +// spawns `secretspec` by name, so the binary the shell resolves — not go.mod — +// decides whether `--reason` is accepted, whether the require_reason policy +// exists, and whether the `age` provider is compiled in at all. Those are the +// behaviors the write path depends on, and none of them are visible to the SDK +// pin, so without this assertion the CLI could drift arbitrarily far while +// every other test stayed green. +// +// The floor is the version whose provider set and flag grammar the write path +// was verified against. Below it the `age` provider does not exist (it became a +// default-on cargo feature after 0.14), so an encrypted-at-rest write fails +// with `Provider backend 'age' not found` rather than degrading to something +// safe. +// +// Skips when no binary is on PATH so a hermetic run (or a CI job that does not +// enter the dev shell) stays green, matching the DSN-gated skip posture used +// elsewhere in this package's suites. +func TestSecretSpecCLIVersionFloor(t *testing.T) { + const minMajor, minMinor = 0, 20 + + bin, err := exec.LookPath(defaultCLI) + if err != nil { + t.Skipf("%s not on PATH; skipping the CLI floor guard", defaultCLI) + } + + out, err := exec.CommandContext(context.Background(), bin, "--version").Output() + if err != nil { + t.Fatalf("%s --version: %v", bin, err) + } + // `secretspec --version` prints "secretspec ". + fields := strings.Fields(string(out)) + if len(fields) < 2 { + t.Fatalf("%s --version = %q, want \"secretspec \"", bin, strings.TrimSpace(string(out))) + } + version := fields[len(fields)-1] + + parts := strings.SplitN(version, ".", 3) + if len(parts) < 2 { + t.Fatalf("%s reported version %q, want a dotted semver", bin, version) + } + major, err := strconv.Atoi(parts[0]) + if err != nil { + t.Fatalf("%s reported version %q: parse major: %v", bin, version, err) + } + minor, err := strconv.Atoi(parts[1]) + if err != nil { + t.Fatalf("%s reported version %q: parse minor: %v", bin, version, err) + } + + if major < minMajor || (major == minMajor && minor < minMinor) { + t.Fatalf("%s is version %s, want >= %d.%d — the write path needs the `age` provider (absent before 0.15) and the --reason flag; a shell resolving an older CLI fails encrypted-at-rest writes with \"Provider backend 'age' not found\"", bin, version, minMajor, minMinor) } } @@ -284,11 +354,20 @@ const helperCaptureEnv = "GO_HELPER_CAPTURE_FILE" func TestMain(m *testing.M) { if os.Getenv(helperProcessEnv) == "1" { // We are the re-exec'd stand-in CLI. Capture argv (\x00-joined so no - // argument boundary is ambiguous) and the entire piped stdin verbatim. + // argument boundary is ambiguous), the entire piped stdin verbatim, and + // the body of the manifest --file points at — read HERE, while the + // parent's temp file still exists, so the parent can assert the manifest + // was really on disk and really declared the name at exec time. stdin, _ := io.ReadAll(os.Stdin) capture := os.Getenv(helperCaptureEnv) - // Sentinel separates the argv record from the raw stdin bytes. - payload := strings.Join(os.Args, "\x00") + "\x1e" + string(stdin) + var manifest []byte + if i := slices.Index(os.Args, "--file"); i >= 0 && i+1 < len(os.Args) { + // A read failure leaves the body empty, which reddens the parent's + // manifest assertions rather than passing silently. + manifest, _ = os.ReadFile(os.Args[i+1]) + } + // Sentinels separate the argv record, the raw stdin bytes and the manifest. + payload := strings.Join(os.Args, "\x00") + "\x1e" + string(stdin) + "\x1e" + string(manifest) if err := os.WriteFile(capture, []byte(payload), 0o600); err != nil { os.Exit(2) } @@ -298,19 +377,25 @@ func TestMain(m *testing.M) { } // TestSetFeedsValueOnStdinNeverArgv defends finding #1 (GATING): the value→stdin, -// never→argv invariant on the REAL exec boundary in Set. setArgs is pure and -// cannot regress the exec wiring; this drives Set through an actual process -// spawn (the test binary re-exec'd as the pinned CLI) and asserts what the child -// truly received. A future edit that appends the value as a positional arg, or -// breaks cmd.Stdin, reddens this. +// never→argv invariant on the REAL exec boundary in Set, the audit-reason +// contract the provider's require_reason policy enforces, and the explicit +// manifest the CLI is pointed at. setArgs is pure and cannot regress the exec +// wiring; this drives Set through an actual process spawn (the test binary +// re-exec'd as the pinned CLI) and asserts what the child truly received. A +// future edit that appends the value as a positional arg, breaks cmd.Stdin, +// drops --reason or --file, or moves either after the `set` subcommand reddens +// this. func TestSetFeedsValueOnStdinNeverArgv(t *testing.T) { const value = "the-secret-value" + const reason = "compass: operator secret write via SetSecret RPC" capture := filepath.Join(t.TempDir(), "capture") // Pin the CLI to this test binary and route it into the TestMain stand-in // branch via env. os.Args[0] is the running test executable; Set execs it as - // ` set API_KEY --provider ...`, and TestMain (guarded) plays the CLI. - r := NewSpecResolver(nil, t.TempDir(), + // ` --file --reason set API_KEY --provider ...`, and TestMain + // (guarded) plays the CLI. + stateDir := t.TempDir() + r := NewSpecResolver(nil, stateDir, WithCLI(os.Args[0]), WithProvider("keyring://"), WithProfile("production"), @@ -318,7 +403,7 @@ func TestSetFeedsValueOnStdinNeverArgv(t *testing.T) { t.Setenv(helperProcessEnv, "1") t.Setenv(helperCaptureEnv, capture) - if err := r.Set(context.Background(), "API_KEY", value); err != nil { + if err := r.Set(context.Background(), "API_KEY", value, reason); err != nil { t.Fatalf("Set = %v, want nil", err) } @@ -326,12 +411,12 @@ func TestSetFeedsValueOnStdinNeverArgv(t *testing.T) { if err != nil { t.Fatalf("read capture file (stand-in CLI never ran or never wrote): %v", err) } - parts := strings.SplitN(string(raw), "\x1e", 2) - if len(parts) != 2 { + parts := strings.SplitN(string(raw), "\x1e", 3) + if len(parts) != 3 { t.Fatalf("malformed capture payload: %q", raw) } argv := strings.Split(parts[0], "\x00") - stdin := parts[1] + stdin, manifest := parts[1], parts[2] // (a) argv carries the verb and the name... if !slices.Contains(argv, "set") { @@ -348,7 +433,47 @@ func TestSetFeedsValueOnStdinNeverArgv(t *testing.T) { } } - // (b) the value rides stdin exactly, with the trailing newline the CLI trims. + // (b) --reason is present, carries the caller's reason, and sits ahead of + // `set` in argv: it is a global flag the CLI accepts on either side of the + // subcommand, and before it is the canonical position this test pins so the + // argv shape stays stable. argv[0] is the binary itself, so index comparison + // is over the real invocation the child received. + setIdx := slices.Index(argv, "set") + reasonIdx := slices.Index(argv, "--reason") + if reasonIdx < 0 { + t.Fatalf("argv %v missing the global --reason flag; the provider's require_reason policy fails such a write", argv) + } + if got := argv[reasonIdx+1]; got != reason { + t.Errorf("argv --reason value = %q, want %q", got, reason) + } + if reasonIdx > setIdx { + t.Errorf("argv %v places --reason (index %d) after the 'set' subcommand (index %d); pin it before, the canonical position", argv, reasonIdx, setIdx) + } + + // (c) --file points the CLI at a generated manifest in the resolver's state + // dir, ahead of `set` for the same reason. Without it the CLI walks up from + // the process cwd looking for a secretspec.toml the repo deliberately never + // commits, so every production write fails "No secretspec.toml found". + fileIdx := slices.Index(argv, "--file") + if fileIdx < 0 { + t.Fatalf("argv %v missing the global --file flag; without a manifest the CLI fails 'No secretspec.toml found'", argv) + } + if got := argv[fileIdx+1]; filepath.Dir(got) != stateDir { + t.Errorf("argv --file = %q, want a manifest under the resolver state dir %q", got, stateDir) + } + if fileIdx > setIdx { + t.Errorf("argv %v places --file (index %d) after the 'set' subcommand (index %d); pin it before, the canonical position", argv, fileIdx, setIdx) + } + + // ...and that manifest really existed at exec time, declaring exactly the + // name being written under the resolver's profile. + for _, want := range []string{"[profiles.production]", "API_KEY = {", "required = true"} { + if !strings.Contains(manifest, want) { + t.Errorf("manifest handed to the CLI missing %q:\n%s", want, manifest) + } + } + + // (d) the value rides stdin exactly, with the trailing newline the CLI trims. if want := value + "\n"; stdin != want { t.Errorf("captured stdin = %q, want %q", stdin, want) } @@ -365,10 +490,31 @@ func TestSetEmptyValueNeverInvokesCLI(t *testing.T) { t.Setenv(helperProcessEnv, "1") t.Setenv(helperCaptureEnv, capture) - if err := r.Set(context.Background(), "API_KEY", ""); err == nil { + if err := r.Set(context.Background(), "API_KEY", "", "compass: unit test write"); err == nil { t.Fatal("Set with empty value = nil, want an error") } if _, err := os.Stat(capture); !os.IsNotExist(err) { t.Errorf("capture file exists (err=%v): the CLI was invoked for an empty value; it must be rejected before exec", err) } } + +// TestSetEmptyReasonNeverInvokesCLI pins the audit-reason contract at the same +// pre-exec boundary as the empty value: the CLI's own require_reason policy is +// an environment heuristic (it gates on agent-env detection), so a reasonless +// write succeeds on one host and is refused on another. Set screens it instead, +// and the capture file's absence proves no process was spawned. +func TestSetEmptyReasonNeverInvokesCLI(t *testing.T) { + capture := filepath.Join(t.TempDir(), "capture") + r := NewSpecResolver(nil, t.TempDir(), WithCLI(os.Args[0])) + t.Setenv(helperProcessEnv, "1") + t.Setenv(helperCaptureEnv, capture) + + for _, reason := range []string{"", " \t\n"} { + if err := r.Set(context.Background(), "API_KEY", "the-secret-value", reason); err == nil { + t.Errorf("Set with reason %q = nil, want an error", reason) + } + if _, err := os.Stat(capture); !os.IsNotExist(err) { + t.Errorf("capture file exists (err=%v): the CLI was invoked with reason %q; an empty reason must be rejected before exec", err, reason) + } + } +} diff --git a/go/server/secrets_service.go b/go/server/secrets_service.go index d4eaf558a..5a02b635f 100644 --- a/go/server/secrets_service.go +++ b/go/server/secrets_service.go @@ -121,7 +121,9 @@ func (s *secretsService) SetSecret( return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("declaring secret: %w", declErr)) } - if err := s.resolver.Set(ctx, msg.GetName(), msg.GetValue()); err != nil { + // A fixed, operator-meaningful reason: this write is only ever reachable + // through the SetSecret RPC, so the audit log records that provenance. + if err := s.resolver.Set(ctx, msg.GetName(), msg.GetValue(), "compass: operator secret write via SetSecret RPC"); err != nil { // The name was validated by DeclareSecret and the value was screened // non-empty above, so a Set failure here is a provider/exec fault // (CLI unreachable, non-zero exit) — retryable and operator-side, never diff --git a/go/server/secrets_service_pgtest_test.go b/go/server/secrets_service_pgtest_test.go index a941ee3fc..b95985afa 100644 --- a/go/server/secrets_service_pgtest_test.go +++ b/go/server/secrets_service_pgtest_test.go @@ -20,6 +20,7 @@ package server import ( "context" "errors" + "strings" "testing" "connectrpc.com/connect" @@ -39,6 +40,7 @@ import ( type recordingResolver struct { setErr error setNames []string + setReasons []string deleteNames []string resolveHit bool } @@ -48,11 +50,12 @@ func (r *recordingResolver) Resolve(_ context.Context, _ string) ([]secrets.Reso return nil, errors.New("ListSecrets must not resolve values") } -func (r *recordingResolver) Set(_ context.Context, name, _ string) error { +func (r *recordingResolver) Set(_ context.Context, name, _, reason string) error { if r.setErr != nil { return r.setErr } r.setNames = append(r.setNames, name) + r.setReasons = append(r.setReasons, reason) return nil } @@ -177,6 +180,11 @@ func TestSetSecretUserOnly(t *testing.T) { if len(f.resolver.setNames) != 1 || f.resolver.setNames[0] != "DB_URL" { t.Fatalf("resolver.Set names = %v, want [DB_URL]", f.resolver.setNames) } + // The handler must hand the resolver a non-empty reason: the provider's + // require_reason policy refuses a reasonless write outright. + if len(f.resolver.setReasons) != 1 || strings.TrimSpace(f.resolver.setReasons[0]) == "" { + t.Fatalf("resolver.Set reasons = %q, want one non-empty reason", f.resolver.setReasons) + } } // TestSetSecretBumpsSecretsVersion: a successful Set bumps the secrets version diff --git a/go/server/serve_forge_test.go b/go/server/serve_forge_test.go index b96179e22..7630ca7b7 100644 --- a/go/server/serve_forge_test.go +++ b/go/server/serve_forge_test.go @@ -42,8 +42,8 @@ func (r *fakeResolver) Resolve(_ context.Context, _ string) ([]secrets.ResolvedS return r.resolved, nil } -func (r *fakeResolver) Set(context.Context, string, string) error { return nil } -func (r *fakeResolver) Delete(context.Context, string) error { return nil } +func (r *fakeResolver) Set(context.Context, string, string, string) error { return nil } +func (r *fakeResolver) Delete(context.Context, string) error { return nil } func TestForgeConfigEnableAndDefaults(t *testing.T) { t.Run("board ingestion disabled by default", func(t *testing.T) { diff --git a/guest-image/default.nix b/guest-image/default.nix index d6716ad8c..80ef6b820 100644 --- a/guest-image/default.nix +++ b/guest-image/default.nix @@ -96,7 +96,7 @@ let }; subPackages = [ "cmd/compass-guestd" ]; proxyVendor = true; - vendorHash = "sha256-GHZsEfvnu1tY6Bd7Fxg7SEWEI+HS0NlQuBbm6pz/UK4="; + vendorHash = "sha256-FsKtsXc6t9FkxxlIXRgjXyqzel/KLMWo70ve1+lnxbI="; env.CGO_ENABLED = 0; ldflags = [ "-s"