From 7a331f2dda4abbb891a30a1c4b62d35499e9ea62 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sat, 5 Sep 2026 22:53:21 -0400 Subject: [PATCH 1/9] chore(secrets): bump secretspec to v0.20, thread an audit reason, and stage an age-capable CLI (RIG-3320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The secrets write path shells the `secretspec` CLI (the Go SDK is read-shaped), and three things were wrong with that seam. **1. The SDK pin moved to v0.20.0.** `go build` needs no source change; the write-path contract is unchanged and re-verified against v0.20.0 source (`secrets.rs:4423-4427` for the piped-stdin branch and trim, `:4430-4433` for empty-value rejection). The intentional tripwire `TestSecretSpecVersionPin` moves with it. **2. `require_reason` (0.17+) hard-fails a reasonless `set`.** The policy defaults to `agents` and refuses a write with no reason, so the old argv broke on the new CLI. `Set` now takes a `reason` and emits `--reason` before the subcommand. An empty reason is **rejected up front** rather than omitting the flag: the CLI's policy gates on agent-environment detection, so an omitted reason makes the same write succeed on one host and fail on another, surfacing to operators as a retryable provider fault. Screening it makes the failure a deterministic caller error and makes the interface's "the reason travels with every write" promise literally true. **3. `Set` never told the CLI where the manifest was.** No `cmd.Dir`, no `--file`, and no `secretspec.toml` is committed by design (the registry is the source of truth), so every operator write failed `No secretspec.toml found in current or any parent directory`. `Set` now generates a manifest declaring exactly the name being written and passes it via the global `--file` flag — the same explicit-manifest treatment `Resolve` already gave the read path. Verified red/green against the real CLI from a manifest-less cwd: exit 1 without `--file`, exit 0 with it, and an exact readback. ## Staging an age-capable CLI The write path spawns `secretspec` by name, and nothing staged one, so `set` was unreachable from the dev shell. A bare `secretspec` is not enough: this shell's nixpkgs channel still resolves **0.14.0**, which has no `age` provider compiled in at all and fails an encrypted-at-rest write with `Provider backend 'age' not found` rather than degrading. It is therefore resolved from a second nixpkgs input pinned in `devenv.lock`, carrying **0.20.0** — the same version as the SDK pin, so the read path (SDK + native lib) and the write path (shelled CLI) move together instead of skewing across an independent seam. Consumed as a dotted attr outside the parsed `with pkgs` literal, matching `skopeo-nix2container`, because the toolchain-parity gate resolves every bare attr in that list. `age://` is proven end to end through the real dev shell: write with the value on stdin (absent from argv), a 499-byte `-----BEGIN AGE ENCRYPTED FILE-----` blob with the plaintext absent, and an exact readback. ## Drift guards `TestSecretSpecVersionPin` only ever covered the SDK half; its doc now says so. `TestSecretSpecCLIVersionFloor` covers the other half, asserting the staged binary's version floor — the CLI decides whether `--reason` is accepted, whether the policy exists, and whether `age` exists at all, and none of that is visible to a go.mod pin. It skips when no binary is on PATH so hermetic runs stay green, and it fails loudly against 0.14.0 with the provider diagnostic. ## Also Both `vendorHash` literals that consume `go/go.mod` are refreshed — `flake.nix` and `guest-image/default.nix`. Missing the second one failed `moon (nix)` on the `compass-guestd` fixed-output derivation. Three design-record sites that quote or prescribe the old 3-arg `Set` are amended to the new signature, including the T2 provisioning instruction, which now passes a concrete audit reason. Ledger-impact: none. Refs RIG-3320 Co-authored-by: Matt Wilkinson --- devenv.lock | 19 +- devenv.nix | 15 ++ devenv.yaml | 17 ++ .../agent/compass-agent-container-runtime.md | 2 +- ...-gateway-credentials-at-rest-encryption.md | 15 +- flake.nix | 2 +- go/go.mod | 2 +- go/go.sum | 2 + go/internal/runnerhub/secrets_test.go | 4 +- go/internal/secrets/resolver.go | 91 ++++++-- go/internal/secrets/resolver_test.go | 219 +++++++++++++++--- go/server/secrets_service.go | 4 +- go/server/secrets_service_pgtest_test.go | 10 +- go/server/serve_forge_test.go | 4 +- guest-image/default.nix | 2 +- 15 files changed, 333 insertions(+), 75 deletions(-) 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/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/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..db03a0cc4 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,25 @@ 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. The joined form binds each value to its flag, so a leading-dash +// reason is recorded as the reason rather than parsed as a flag. +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..4a1f75b6f 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,38 @@ 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) } + // A caller-supplied reason beginning with a dash stays one joined argument. + hostile := bare.setArgs("API_KEY", "--provider=evil://", manifest) + if !slices.Contains(hostile, "--reason=--provider=evil://") || slices.Contains(hostile, "--provider=evil://") || slices.Contains(hostile, "--provider") { + t.Errorf("setArgs hostile reason = %v, want one joined reason token and no provider flag", hostile) + } + // 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 +223,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 +268,56 @@ 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. +// +// This guard is dev-shell-only: no CI lane stages the secretspec binary. It is +// resolved from a pinned input outside the parsed `packages` literal, so this +// test always skips in CI; the CLI half of the seam is asserted on a developer's +// machine instead. +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 +355,24 @@ 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. - stdin, _ := io.ReadAll(os.Stdin) + // 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, err := io.ReadAll(os.Stdin) + if err != nil { + os.Exit(2) + } 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.IndexFunc(os.Args, func(arg string) bool { return strings.HasPrefix(arg, "--file=") }); i >= 0 { + manifest, err = os.ReadFile(strings.TrimPrefix(os.Args[i], "--file=")) + if err != nil { + os.Exit(2) + } + } + // 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 +382,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 +408,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 +416,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 +438,47 @@ func TestSetFeedsValueOnStdinNeverArgv(t *testing.T) { } } - // (b) the value rides stdin exactly, with the trailing newline the CLI trims. + // (b) --reason is present as a joined token 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.IndexFunc(argv, func(arg string) bool { return strings.HasPrefix(arg, "--reason=") }) + if reasonIdx < 0 { + t.Fatalf("argv %v missing the joined global --reason flag; the provider's require_reason policy fails such a write", argv) + } + if got := strings.TrimPrefix(argv[reasonIdx], "--reason="); 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.IndexFunc(argv, func(arg string) bool { return strings.HasPrefix(arg, "--file=") }) + if fileIdx < 0 { + t.Fatalf("argv %v missing the joined global --file flag; without a manifest the CLI fails 'No secretspec.toml found'", argv) + } + if got := strings.TrimPrefix(argv[fileIdx], "--file="); 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 +495,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" From 66b4cc07276046e38953cc0ba920db76a5becf3a Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 6 Sep 2026 12:35:38 -0400 Subject: [PATCH 2/9] fix(secrets): pin the write profile explicitly and require joined flag=value argv (RIG-3320) Review follow-ups on the secretspec v0.20 bump: - `setArgs` now takes the resolved profile and emits `--profile` unconditionally. The generated manifest header and the argv are fed from one `resolvedProfile()` call, so the CLI cannot act under a different profile than the manifest declares by silently falling back to its own built-in default. - Document that the joined `--flag=value` form is required rather than stylistic: the two-token form parses a leading-dash reason as the next flag and exits 2. - Widen the `//nolint:gosec` justification to cover all three argv variables (name, reason, manifest path) instead of only the name. - Drop the stale `secretspec-go v0.15.0` hashes left in `go.sum`. - Cover the both-empty guard precedence in `Set`: the value guard fires before the reason guard, which `server.SetSecret` relies on when it maps a Set failure to `CodeUnavailable`. - Refresh the design records line references onto the moved code. Refs RIG-3320 Co-authored-by: Matt Wilkinson --- ...-gateway-credentials-at-rest-encryption.md | 56 +++++++++---------- go/go.sum | 2 - go/internal/secrets/resolver.go | 36 ++++++++---- go/internal/secrets/resolver_test.go | 43 ++++++++++---- 4 files changed, 83 insertions(+), 54 deletions(-) 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 35d34d439..d37fd827d 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:238`, `func (r *SpecResolver) Set(ctx + (`go/internal/secrets/resolver.go:234`, `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:216-217); + resolver.go:211-212); - 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) @@ -208,11 +208,11 @@ default-open-minus-a-filter. Why the table boundary IS the delivery boundary: the delivery surface is the resolver's MANIFEST. `SpecResolver` reads its declared set through the `declarations` interface — `DeclaredSecrets(ctx context.Context) -([]store.SecretDeclaration, error)` (`go/internal/secrets/resolver.go:29-31`; -the `store declarations` struct field, resolver.go:57) — and `buildManifest` +([]store.SecretDeclaration, error)` (`go/internal/secrets/resolver.go:35-37`; +the `store declarations` struct field, resolver.go:66) — and `buildManifest` "renders the SecretSpec manifest TOML for a declared set: one `[project]` block and one `[profiles.]` block with every declared name as a -required key" (resolver.go:99-101; the function, resolver.go:105-128); +required key" (resolver.go:108-110; the function, resolver.go:114-137); `Resolve` can only return names present in that manifest. Today ONE resolver instance (`resolver := secrets.NewSpecResolver(st, secretsStateDir(cfg))`, `go/server/serve.go:528`) serves BOTH the container path (FetchSecrets → @@ -252,7 +252,7 @@ with the per-tenant defer (RIG-3237). C1 keeps the SAME SecretSpec profile for both instances — the shared project is `manifestProject = "compass"` (resolver.go:19) and the profile `defaultProfile = "default"` (resolver.go:23; `WithProfile` exists, -resolver.go:78, but C1 does not use it), and BY DEFAULT one provider URI +resolver.go:87, but C1 does not use it), and BY DEFAULT one provider URI configures both resolver instances (F2 WIRING SEAM) — so the provider keyspace is shared unless the operator opts Layer B onto a different provider. Because the reserved-prefix partition renames the six forge secrets under @@ -338,7 +338,7 @@ rejected branches are the ones a future reader will reach for first. contract — where C1's proto delta is two additive `SecretsService` methods. - **Mechanism C2: separate table + a separate SecretSpec PROFILE — considered, DEFERRED.** The same `server_secrets` table, but the server - resolver pinned to its own profile (`WithProfile`, resolver.go:78) so even + resolver pinned to its own profile (`WithProfile`, resolver.go:87) so even the provider keyspace is isolated. Fullest isolation — but it is NOT the cheaper-migration loser it might appear: under the reserved-prefix partition BOTH mechanisms now require a provider write under new keys (the six values @@ -396,7 +396,7 @@ rejected branches are the ones a future reader will reach for first. - AES-256-GCM only; nonces from `crypto/rand`, 96-bit, fresh per encryption, never counter-derived; key is 256-bit from `crypto/rand`. - The master key NEVER appears in the DB, in logs, in argv (Set feeds stdin, - resolver.go:206-207), or in error strings. + resolver.go:211-212), or in error strings. - Auto-provisioning is zero-human-step (rule://no-human-clicks): first boot generates, stores, and declares the key with no operator action. - The names-only invariant (`secrets.go:20-22`) is preserved for EVERYTHING @@ -491,7 +491,7 @@ declared into a store that does not exist. `ServerDeclaredSecrets(ctx)` — a thin store view whose `DeclaredSecrets(ctx context.Context) ([]store.SecretDeclaration, error)` method (the `declarations` - interface shape, resolver.go:29-31) reads `server_secrets`, mapping + interface shape, resolver.go:35-37) reads `server_secrets`, mapping rows to `store.SecretDeclaration` with generic kind / zero delivery (the resolver uses only the name to build its manifest), so `NewSpecResolver` is reused UNCHANGED. @@ -620,7 +620,7 @@ declared into a store that does not exist. (`awssm` (any pin) or `awsps` (0.18+) on AWS, `akv` (any pin) or `aac` (0.20+) on Azure), and one already running Vault/OpenBao uses that — the provider is a per-resolver-INSTANCE config - choice (`WithProvider`, resolver.go:73/75), not a hardcode, so this is a + choice (`WithProvider`, resolver.go:82/84), not a hardcode, so this is a recommended default rather than a fixed backend. Two things make this an EXECUTABLE T0 deliverable rather than prose: (1) DEPENDENCY PREREQUISITE — `age` is a secretspec 0.17+ provider behind an `age` build feature, but @@ -637,18 +637,18 @@ declared into a store that does not exist. default — a single local identity file rather than a GPG keyring). The bump covers TWO separate closures, since the SDK read path and the CLI write path are distinct binaries: (a) the Go module `secretspec-go` `>= - 0.17` for the READ path (`b.Load()`, resolver.go:165); AND (b) the + 0.17` for the READ path (`b.Load()`, resolver.go:170); AND (b) the `secretspec` CLI BINARY the WRITE path shells via `resolver.Set` - (`exec.CommandContext(ctx, r.cli, args...)`, resolver.go:233; `r.cli` - defaults to bare `"secretspec"` on PATH, resolver.go:91) — also `>= + (`exec.CommandContext(ctx, r.cli, args...)`, resolver.go:270; `r.cli` + defaults to bare `"secretspec"` on PATH, resolver.go:29/100) — also `>= 0.17` built with the `age` feature and pinned explicitly via `WithCLI` - (resolver.go:81) into the Server's closure so the read and write halves + (resolver.go:90) into the Server's closure so the read and write halves cannot drift to different provider capability sets (a separate prerequisite PR, Matt-ruled). (2) WIRING SEAM — today serve.go:528 constructs the single resolver with NO provider option (the SDK default chain); T0 threads the operator-configured URI, from a NEW server flag/env (defaulting to the `age://` path), through - `secrets.WithProvider()` (resolver.go:75). By DEFAULT the SAME URI + `secrets.WithProvider()` (resolver.go:84). By DEFAULT the SAME URI configures BOTH resolver instances — the SERVER (server-secret) resolver AND the container/user resolver at serve.go:528 — so the provider keyspace is shared by construction and the F1 guard + D2 read-back below are @@ -666,7 +666,7 @@ declared into a store that does not exist. (the T0 CLI below) writes a value through `resolver.Set` for rotation on a running server. The prefix RENAMES them (`LINEAR_FORGE_CLIENT_SECRET` → `SERVER_LINEAR_FORGE_CLIENT_SECRET`) and the provider keyspace is keyed by - NAME (`setArgs`, resolver.go:258-267), so a value is populated under the + NAME (`setArgs`, resolver.go:315-321), so a value is populated under the prefixed name via the `serverSecretName()` seam (CONSUMER-REPOINT above). The NAMES are declared into `server_secrets` at boot from the RESOLVED config `cfg.Forge.resolved()` (serve.go:232-246) — the same accessor every @@ -788,7 +788,7 @@ serves; no import cycle — it depends on nothing in `secrets`). auth failure (tamper OR aad mismatch) returns an error naming no plaintext/key material. - Key encoding for provider storage: base64(std) of the 32 raw bytes - (SecretSpec values are strings; `Set` rejects empty, resolver.go:225-227). + (SecretSpec values are strings; `Set` rejects empty, resolver.go:239-241). - Consumes: `crypto/aes`, `crypto/cipher`, `crypto/rand` only. - Tests: round-trip; tamper (flip a ciphertext/nonce byte → error); `Open` under a different `aad` → error; nonce uniqueness across calls; redaction @@ -807,8 +807,8 @@ is declared into it) and T1. `GATEWAY_CREDENTIALS_MASTER_KEY` through it; on absence: `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 + (resolver.go:234; the value rides stdin, never argv, + resolver.go:211-212) → `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 @@ -839,10 +839,10 @@ is declared into it) and T1. the fleet):** the provider round-trips inside the lock inherit only the caller's ctx, which at boot is long-lived — but the two halves bound DIFFERENTLY. `SpecResolver.Set` IS ctx-bounded: it shells out via - `exec.CommandContext(ctx, r.cli, …)` (resolver.go:233), so a ctx deadline + `exec.CommandContext(ctx, r.cli, …)` (resolver.go:270), so a ctx deadline genuinely kills it. `SpecResolver.Resolve` is NOT: it threads ctx only into - `DeclaredSecrets` (resolver.go:136); the actual provider round-trip is - `b.Load()` (resolver.go:165), whose SDK signature carries NO ctx + `DeclaredSecrets` (resolver.go:145); the actual provider round-trip is + `b.Load()` (resolver.go:170), whose SDK signature carries NO ctx (`func (b *Builder) Load() (*Resolved, error)`, verified against secretspec-go v0.15.0 secretspec.go:245, the current pin — re-verify this signature after the `>= 0.17` bump T0 requires (F2/H-1 prerequisite); if @@ -889,7 +889,7 @@ is declared into it) and T1. subsequent boot, re-resolve the name and byte-compare against the key the process is about to encrypt with; on mismatch, refuse to serve gateway-credential writes (fail closed). This re-resolve is the SAME - uncancellable `Load` (resolver.go:165) and uses the SAME bounded-offload + uncancellable `Load` (resolver.go:170) and uses the SAME bounded-offload path as the provisioning resolve (timeout ctx + own goroutine + buffered cap-1 channel), so on a steady-state boot — key already provisioned, nothing to serialize — a hung provider still yields a bounded, diagnosable @@ -924,7 +924,7 @@ is declared into it) and T1. `SetSecret`/`DeleteSecret` RPC (`authenticatedOpen`, any authenticated account — admin_gate.go:122-125) declares into `secrets` but then calls `resolver.Set`, which shells `secretspec set --profile default` - (resolver.go:258-267) against the keyspace that is SHARED under the default + (resolver.go:315-321) against the keyspace that is SHARED under the default single-URI wiring (F2) — so absent a guard a user calling `SetSecret` with name `GATEWAY_CREDENTIALS_MASTER_KEY` would OVERWRITE the master key's provider value (the running process keeps its @@ -1081,7 +1081,7 @@ RPC exactly as the frozen record already specifies. WRITABLE SecretSpec provider (self-hosted default `age://` — writable, encrypted-at-rest, headless; a cloud store or Vault/OpenBao where present — F2) resolved by a SERVER resolver constructed - `secrets.WithProvider()` (resolver.go:75) off a + `secrets.WithProvider()` (resolver.go:84) off a NEW server flag/env for that URI (serve.go:528's container resolver unchanged), populated by the operator directly (deploy tooling seeds the age file) or, for rotation on a running server, through the NEW `compass @@ -1144,10 +1144,10 @@ RPC exactly as the frozen record already specifies. separate PR): bump `github.com/cachix/secretspec/secretspec-go` from the pinned v0.15.0 (go/go.mod:22) to `>= 0.17` with the `age` build feature, covering BOTH closures the two code paths shell separately — - the Go module for the SDK READ path (`b.Load()`, resolver.go:165) AND + the Go module for the SDK READ path (`b.Load()`, resolver.go:170) AND the `secretspec` CLI BINARY the WRITE path runs (`resolver.Set` shells - `r.cli`, resolver.go:233/91), the latter pinned via `WithCLI` - (resolver.go:81) into the Server's closure so read and write cannot drift + `r.cli`, resolver.go:270/29), the latter pinned via `WithCLI` + (resolver.go:90) into the Server's closure so read and write cannot drift to different provider capabilities. `age` is a 0.17+ build-feature provider, so on the current pin the `age://` default does not resolve and T2's master-key write-back has no writable target; a deployment on a diff --git a/go/go.sum b/go/go.sum index d1b05fbf6..9c3ce3886 100644 --- a/go/go.sum +++ b/go/go.sum @@ -8,8 +8,6 @@ github.com/BurntSushi/toml v1.6.0 h1:dRaEfpa2VI55EwlIW72hMRHdWouJeRF7TPYhI+AUQjk github.com/BurntSushi/toml v1.6.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho= 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= diff --git a/go/internal/secrets/resolver.go b/go/internal/secrets/resolver.go index db03a0cc4..d5d99ca40 100644 --- a/go/internal/secrets/resolver.go +++ b/go/internal/secrets/resolver.go @@ -227,9 +227,10 @@ func (r *SpecResolver) Resolve(ctx context.Context, reason string) ([]ResolvedSe // ` 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

+// one. So `secretspec --file= --reason= set --provider

// --profile

` with the value on stdin is the write path, no positional -// VALUE. +// VALUE. The joined --flag=value form is required, not stylistic: the +// two-token form parses a leading-dash reason as the next flag and exits 2. func (r *SpecResolver) Set(ctx context.Context, name, value, reason string) error { if err := ValidateName(name); err != nil { return err @@ -248,18 +249,24 @@ func (r *SpecResolver) Set(ctx context.Context, name, value, reason string) erro } // 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}}) + // One resolved profile feeds both the manifest header and the argv below, so + // the two cannot describe different profiles. + profile := r.resolvedProfile() + manifestPath, err := r.writeManifest(profile, []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) + args := r.setArgs(name, reason, manifestPath, profile) //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) - // before it reaches here; the value rides stdin, never argv. + // secretspec CLI (r.cli) with an argv slice passed straight to exec, so no + // shell interprets any of it. Three variables ride it: name, validated + // against the env-var-name grammar (ValidateName) above; and reason plus + // manifestPath, each a single joined --flag=value token, so neither can + // introduce a new argv element or be re-parsed as a flag. The value rides + // stdin, never argv. cmd := exec.CommandContext(ctx, r.cli, args...) cmd.Stdin = strings.NewReader(value + "\n") var stderr bytes.Buffer @@ -300,15 +307,17 @@ func (r *SpecResolver) resolvedProfile() string { // subcommand; both are emitted before it as the canonical, unambiguous // position. The joined form binds each value to its flag, so a leading-dash // reason is recorded as the reason rather than parsed as a flag. -func (r *SpecResolver) setArgs(name, reason, manifestPath string) []string { +// +// profile is the caller's resolvedProfile(), hence non-empty by construction, +// so --profile is emitted unconditionally: the CLI acts under exactly the +// profile the generated manifest declares instead of falling back to its own +// built-in default and agreeing only by coincidence. +func (r *SpecResolver) setArgs(name, reason, manifestPath, profile string) []string { args := []string{"--file=" + manifestPath, "--reason=" + reason, "set", name} if r.provider != "" { args = append(args, "--provider", r.provider) } - if r.profile != "" { - args = append(args, "--profile", r.profile) - } - return args + return append(args, "--profile", profile) } // writeManifest renders the manifest for the current declared set and writes it @@ -330,6 +339,9 @@ func (r *SpecResolver) writeManifest(profile string, decls []store.SecretDeclara return "", fmt.Errorf("secrets: create manifest: %w", err) } // CreateTemp makes the file 0600 already; write the body and close. + // Cleanup discards below are deliberate: on these paths the write/close + // error is what the caller needs, and a failed remove of a temp file we + // are already abandoning is not actionable. if _, err := f.WriteString(body); err != nil { _ = f.Close() _ = os.Remove(f.Name()) diff --git a/go/internal/secrets/resolver_test.go b/go/internal/secrets/resolver_test.go index 4a1f75b6f..82e1cfe3e 100644 --- a/go/internal/secrets/resolver_test.go +++ b/go/internal/secrets/resolver_test.go @@ -84,30 +84,37 @@ func TestBuildManifest(t *testing.T) { func TestSetArgs(t *testing.T) { const reason = "compass: unit test write" const manifest = "/tmp/state/secretspec-123.toml" + const setVerb = "set" - // Bare resolver: the two global flags, the verb and the name, no - // provider/profile flags, and crucially no VALUE anywhere in the argv. + // An explicit WithProfile("") still resolves to defaultProfile, and --profile + // is emitted unconditionally: the CLI acts under exactly the profile the + // generated manifest declares, rather than agreeing only because the CLI's + // own built-in default happens to match. No provider flag (none pinned), and + // crucially no VALUE anywhere in the argv. bare := NewSpecResolver(nil, "/tmp/state", WithProfile("")) - got := bare.setArgs("API_KEY", reason, manifest) - const setVerb = "set" - if want := []string{"--file=" + manifest, "--reason=" + reason, setVerb, "API_KEY"}; !equalArgs(got, want) { + got := bare.setArgs("API_KEY", reason, manifest, bare.resolvedProfile()) + want := []string{ + "--file=" + manifest, "--reason=" + reason, setVerb, "API_KEY", + "--profile", defaultProfile, + } + if !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, and both globals still lead the argv. full := NewSpecResolver(nil, "/tmp/state", WithProvider("keyring://"), WithProfile("production")) - gotFull := full.setArgs("API_KEY", reason, manifest) - want := []string{ + gotFull := full.setArgs("API_KEY", reason, manifest, full.resolvedProfile()) + wantFull := []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) + if !equalArgs(gotFull, wantFull) { + t.Errorf("setArgs full = %v, want %v", gotFull, wantFull) } // A caller-supplied reason beginning with a dash stays one joined argument. - hostile := bare.setArgs("API_KEY", "--provider=evil://", manifest) + hostile := bare.setArgs("API_KEY", "--provider=evil://", manifest, bare.resolvedProfile()) if !slices.Contains(hostile, "--reason=--provider=evil://") || slices.Contains(hostile, "--provider=evil://") || slices.Contains(hostile, "--provider") { t.Errorf("setArgs hostile reason = %v, want one joined reason token and no provider flag", hostile) } @@ -227,6 +234,18 @@ func TestSetEmptyValueRejected(t *testing.T) { t.Errorf("Set with empty value %q = nil, want an error", empty) } } + // Both empty: the value guard runs FIRST, so the error names the value, not + // the reason. The precedence is load-bearing, not cosmetic — server.SetSecret + // maps a Set failure to CodeUnavailable on the stated premise that the value + // was already screened non-empty, so a caller that sent neither must still be + // told about the value. Swapping the two guard blocks reddens this. + err := r.Set(context.Background(), "API_KEY", "", "") + if err == nil { + t.Fatal("Set with empty value and empty reason = nil, want an error") + } + if !strings.Contains(err.Error(), "value is empty") { + t.Errorf("Set with both empty = %q, want the value guard to fire first (\"value is empty\")", err) + } // A bad name is still rejected first, independent of value. 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") @@ -317,7 +336,7 @@ func TestSecretSpecCLIVersionFloor(t *testing.T) { } 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) + t.Fatalf("%s is version %s, want >= %d.%d — the floor is parity with the secretspec-go SDK pin in go.mod, so the SDK read half and the CLI write half act under one release rather than skewing; %d.%d also subsumes the older, separate 0.15 `age`-provider floor, below which encrypted-at-rest writes fail with \"Provider backend 'age' not found\"", bin, version, minMajor, minMinor, minMajor, minMinor) } } @@ -397,7 +416,7 @@ func TestSetFeedsValueOnStdinNeverArgv(t *testing.T) { // 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 - // ` --file --reason set API_KEY --provider ...`, and TestMain + // ` --file= --reason= set API_KEY --provider ...`, and TestMain // (guarded) plays the CLI. stateDir := t.TempDir() r := NewSpecResolver(nil, stateDir, From ce78ccf971983c84129ee04f9fd08a960ede683e Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 6 Sep 2026 13:49:47 -0400 Subject: [PATCH 3/9] fix(secrets): join every write flag and correct the age provider floor (RIG-3320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit setArgs used the joined --flag=value form for --reason and --file but left --provider and --profile in the two-token form the same function's comment declares unsafe. That rationale is a property of the value's shape, not of which flag carries it: ValidateProfile admits a leading dash and the provider URI is unvalidated, so a dash-leading value was parsed as the next flag and the CLI exited 2 — surfacing through SetSecret as a misleading retryable CodeUnavailable rather than a config error. Join all four, and cover the operator-configured flags in the hostile-input test that previously exercised only reason. The version-floor test's failure message and devenv.yaml both cited a 0.15 age floor, contradicting the design record's 0.17. Upstream's changelog puts age:// under 0.17.0, so the record was right and both citations are corrected rather than the record. Also refresh three stale artifacts: WithProfile's doc still promised the SDK default an explicit --profile now precludes, the record's Load citation still named the v0.15.0 pin (re-verified at v0.20.0 secretspec.go:293 — still no ctx, so the offload design stands), and the audit reason was anonymous despite the handler holding the caller ID, which is half the provenance the reason exists for. Refs RIG-3320 Co-authored-by: Matt Wilkinson --- devenv.yaml | 4 ++-- ...-gateway-credentials-at-rest-encryption.md | 7 +++---- go/internal/secrets/resolver.go | 15 ++++++++----- go/internal/secrets/resolver_test.go | 21 ++++++++++++++++--- go/server/secrets_service.go | 10 ++++++--- 5 files changed, 40 insertions(+), 17 deletions(-) diff --git a/devenv.yaml b/devenv.yaml index ddb917169..8b0e82433 100644 --- a/devenv.yaml +++ b/devenv.yaml @@ -56,8 +56,8 @@ inputs: 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 + # `age://` provider it needs to write encrypted-at-rest secrets was added in + # 0.17.0 (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 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 d37fd827d..c38f58e75 100644 --- a/docs/designs/server/compass-gateway-credentials-at-rest-encryption.md +++ b/docs/designs/server/compass-gateway-credentials-at-rest-encryption.md @@ -844,10 +844,9 @@ is declared into it) and T1. `DeclaredSecrets` (resolver.go:145); the actual provider round-trip is `b.Load()` (resolver.go:170), whose SDK signature carries NO ctx (`func (b *Builder) Load() (*Resolved, error)`, verified against - secretspec-go v0.15.0 secretspec.go:245, the current pin — re-verify this - signature after the `>= 0.17` bump T0 requires (F2/H-1 prerequisite); if - `Load` gains a ctx-carrying form, the goroutine-offload design below - simplifies to a plain ctx-bounded call) and which blocks in an uncancellable + secretspec-go v0.20.0 secretspec.go:293, the current pin — `Load` still + carries no ctx after the bump, so the goroutine-offload design below + stands) and which blocks in an uncancellable FFI call (`nativeResolve` → `C.secretspec_resolve`, binding_cgo.go:30 / binding_purego.go:118). A hung provider (1Password awaiting biometric diff --git a/go/internal/secrets/resolver.go b/go/internal/secrets/resolver.go index d5d99ca40..0318f55dd 100644 --- a/go/internal/secrets/resolver.go +++ b/go/internal/secrets/resolver.go @@ -83,7 +83,8 @@ type SpecOption func(*SpecResolver) // "onepassword://Production"). Empty uses the SDK's default provider chain. func WithProvider(uri string) SpecOption { return func(r *SpecResolver) { r.provider = uri } } -// WithProfile pins the SecretSpec profile. Empty uses the SDK default. +// WithProfile pins the SecretSpec profile. Empty resolves to defaultProfile +// (see resolvedProfile), never the SDK/CLI built-in default. func WithProfile(profile string) SpecOption { return func(r *SpecResolver) { r.profile = profile } } // WithCLI pins the secretspec CLI binary used for the write path. @@ -305,8 +306,12 @@ func (r *SpecResolver) resolvedProfile() string { // without executing the binary). The value never appears here — it rides stdin. // --file and --reason are global flags, accepted on either side of the `set` // subcommand; both are emitted before it as the canonical, unambiguous -// position. The joined form binds each value to its flag, so a leading-dash -// reason is recorded as the reason rather than parsed as a flag. +// position. Every flag uses the joined form, which binds each value to its +// flag: the two-token form parses a leading-dash value as the next flag and +// exits 2. That is a property of the value's shape, not of which flag carries +// it, so it holds for the operator-configured provider and profile just as it +// does for a caller-supplied reason — ValidateProfile admits a leading dash, +// and the provider string is unvalidated. // // profile is the caller's resolvedProfile(), hence non-empty by construction, // so --profile is emitted unconditionally: the CLI acts under exactly the @@ -315,9 +320,9 @@ func (r *SpecResolver) resolvedProfile() string { func (r *SpecResolver) setArgs(name, reason, manifestPath, profile string) []string { args := []string{"--file=" + manifestPath, "--reason=" + reason, "set", name} if r.provider != "" { - args = append(args, "--provider", r.provider) + args = append(args, "--provider="+r.provider) } - return append(args, "--profile", profile) + return append(args, "--profile="+profile) } // writeManifest renders the manifest for the current declared set and writes it diff --git a/go/internal/secrets/resolver_test.go b/go/internal/secrets/resolver_test.go index 82e1cfe3e..c1e7948e1 100644 --- a/go/internal/secrets/resolver_test.go +++ b/go/internal/secrets/resolver_test.go @@ -95,7 +95,7 @@ func TestSetArgs(t *testing.T) { got := bare.setArgs("API_KEY", reason, manifest, bare.resolvedProfile()) want := []string{ "--file=" + manifest, "--reason=" + reason, setVerb, "API_KEY", - "--profile", defaultProfile, + "--profile=" + defaultProfile, } if !equalArgs(got, want) { t.Errorf("setArgs bare = %v, want %v", got, want) @@ -107,7 +107,7 @@ func TestSetArgs(t *testing.T) { gotFull := full.setArgs("API_KEY", reason, manifest, full.resolvedProfile()) wantFull := []string{ "--file=" + manifest, "--reason=" + reason, setVerb, "API_KEY", - "--provider", "keyring://", "--profile", "production", + "--provider=keyring://", "--profile=production", } if !equalArgs(gotFull, wantFull) { t.Errorf("setArgs full = %v, want %v", gotFull, wantFull) @@ -119,6 +119,21 @@ func TestSetArgs(t *testing.T) { t.Errorf("setArgs hostile reason = %v, want one joined reason token and no provider flag", hostile) } + // Same guarantee for the operator-configured flags: ValidateProfile admits a + // leading dash, and the provider string is unvalidated, so a dash-leading + // value must stay bound to its own flag rather than being parsed as the next + // one (which the CLI rejects with a bare exit 2). + dashCfg := NewSpecResolver(nil, "/tmp/state", WithProvider("--reason=evil"), WithProfile("-prod")) + gotDash := dashCfg.setArgs("API_KEY", reason, manifest, dashCfg.resolvedProfile()) + if !slices.Contains(gotDash, "--profile=-prod") || !slices.Contains(gotDash, "--provider=--reason=evil") { + t.Errorf("setArgs dash-leading config = %v, want joined --profile/--provider tokens", gotDash) + } + for _, a := range gotDash { + if a == "-prod" || a == "--reason=evil" { + t.Errorf("dash-leading config value became its own argv token: %v", gotDash) + } + } + // The value must NEVER be in the constructed argv — it rides stdin. for _, a := range gotFull { if strings.Contains(a, "the-secret-value") { @@ -336,7 +351,7 @@ func TestSecretSpecCLIVersionFloor(t *testing.T) { } if major < minMajor || (major == minMajor && minor < minMinor) { - t.Fatalf("%s is version %s, want >= %d.%d — the floor is parity with the secretspec-go SDK pin in go.mod, so the SDK read half and the CLI write half act under one release rather than skewing; %d.%d also subsumes the older, separate 0.15 `age`-provider floor, below which encrypted-at-rest writes fail with \"Provider backend 'age' not found\"", bin, version, minMajor, minMinor, minMajor, minMinor) + t.Fatalf("%s is version %s, want >= %d.%d — the floor is parity with the secretspec-go SDK pin in go.mod, so the SDK read half and the CLI write half act under one release rather than skewing; %d.%d also subsumes the older, separate 0.17 `age`-provider floor (age:// was added in 0.17.0), below which encrypted-at-rest writes fail with \"Provider backend 'age' not found\"", bin, version, minMajor, minMinor, minMajor, minMinor) } } diff --git a/go/server/secrets_service.go b/go/server/secrets_service.go index 5a02b635f..0d1eec529 100644 --- a/go/server/secrets_service.go +++ b/go/server/secrets_service.go @@ -121,9 +121,13 @@ func (s *secretsService) SetSecret( return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("declaring secret: %w", declErr)) } - // 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 audit reason carries the authenticated caller, so the provider's log + // distinguishes which operator wrote a secret rather than recording every + // write anonymously. The RPC is the only path that reaches this write, so + // the prefix also records that provenance. The CLI JSON-escapes the reason + // into its audit record, so an ID cannot forge a log entry. + reason := fmt.Sprintf("compass: operator secret write via SetSecret RPC (caller %s)", callerID) + if err := s.resolver.Set(ctx, msg.GetName(), msg.GetValue(), reason); 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 From 06be6510b6f79f657cd74257a57570ae559d6988 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 6 Sep 2026 14:34:09 -0400 Subject: [PATCH 4/9] fix(secrets): refresh shifted design-record citations and pin the audit-reason contract (RIG-3320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 review findings, all documentation/coverage — no behavior change. - Design record: the round-3 commit added 5 lines to resolver.go and silently invalidated 23 file:line citations in the record it also edits. Re-point them all to the head tree, and fix the two SDK binding citations left at their v0.15.0 offsets inside the very sentence that was refreshed to v0.20.0 (binding_cgo.go:30 -> :28, binding_purego.go:118 -> :142). - resolver.go: the Set doc comment still illustrated the two-token '--provider

--profile

' form that the next sentence declares broken and that setArgs no longer emits. Show the joined form and the widened value-shape scope. - secrets_service.go: lead the audit-reason comment with the structural guarantee (callerID is a bearer-resolved, server-minted hex account id, never a request field) and keep the CLI's JSON escaping as defense in depth. - secrets_service_pgtest_test.go: the caller-bound audit reason was a new observable contract with no test. Bind the assertion to the authenticated user's account id and pin that the reason never carries the value. Refs RIG-3320 Co-authored-by: Matt Wilkinson --- ...-gateway-credentials-at-rest-encryption.md | 48 +++++++++---------- go/internal/secrets/resolver.go | 9 ++-- go/server/secrets_service.go | 8 +++- go/server/secrets_service_pgtest_test.go | 14 +++++- 4 files changed, 47 insertions(+), 32 deletions(-) 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 c38f58e75..d4fd521e1 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:234`, `func (r *SpecResolver) Set(ctx + (`go/internal/secrets/resolver.go:236`, `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:211-212); + resolver.go:212-213); - 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) @@ -212,7 +212,7 @@ resolver's MANIFEST. `SpecResolver` reads its declared set through the the `store declarations` struct field, resolver.go:66) — and `buildManifest` "renders the SecretSpec manifest TOML for a declared set: one `[project]` block and one `[profiles.]` block with every declared name as a -required key" (resolver.go:108-110; the function, resolver.go:114-137); +required key" (resolver.go:109-111; the function, resolver.go:115-138); `Resolve` can only return names present in that manifest. Today ONE resolver instance (`resolver := secrets.NewSpecResolver(st, secretsStateDir(cfg))`, `go/server/serve.go:528`) serves BOTH the container path (FetchSecrets → @@ -252,7 +252,7 @@ with the per-tenant defer (RIG-3237). C1 keeps the SAME SecretSpec profile for both instances — the shared project is `manifestProject = "compass"` (resolver.go:19) and the profile `defaultProfile = "default"` (resolver.go:23; `WithProfile` exists, -resolver.go:87, but C1 does not use it), and BY DEFAULT one provider URI +resolver.go:88, but C1 does not use it), and BY DEFAULT one provider URI configures both resolver instances (F2 WIRING SEAM) — so the provider keyspace is shared unless the operator opts Layer B onto a different provider. Because the reserved-prefix partition renames the six forge secrets under @@ -338,7 +338,7 @@ rejected branches are the ones a future reader will reach for first. contract — where C1's proto delta is two additive `SecretsService` methods. - **Mechanism C2: separate table + a separate SecretSpec PROFILE — considered, DEFERRED.** The same `server_secrets` table, but the server - resolver pinned to its own profile (`WithProfile`, resolver.go:87) so even + resolver pinned to its own profile (`WithProfile`, resolver.go:88) so even the provider keyspace is isolated. Fullest isolation — but it is NOT the cheaper-migration loser it might appear: under the reserved-prefix partition BOTH mechanisms now require a provider write under new keys (the six values @@ -396,7 +396,7 @@ rejected branches are the ones a future reader will reach for first. - AES-256-GCM only; nonces from `crypto/rand`, 96-bit, fresh per encryption, never counter-derived; key is 256-bit from `crypto/rand`. - The master key NEVER appears in the DB, in logs, in argv (Set feeds stdin, - resolver.go:211-212), or in error strings. + resolver.go:212-213), or in error strings. - Auto-provisioning is zero-human-step (rule://no-human-clicks): first boot generates, stores, and declares the key with no operator action. - The names-only invariant (`secrets.go:20-22`) is preserved for EVERYTHING @@ -637,12 +637,12 @@ declared into a store that does not exist. default — a single local identity file rather than a GPG keyring). The bump covers TWO separate closures, since the SDK read path and the CLI write path are distinct binaries: (a) the Go module `secretspec-go` `>= - 0.17` for the READ path (`b.Load()`, resolver.go:170); AND (b) the + 0.17` for the READ path (`b.Load()`, resolver.go:171); AND (b) the `secretspec` CLI BINARY the WRITE path shells via `resolver.Set` - (`exec.CommandContext(ctx, r.cli, args...)`, resolver.go:270; `r.cli` - defaults to bare `"secretspec"` on PATH, resolver.go:29/100) — also `>= + (`exec.CommandContext(ctx, r.cli, args...)`, resolver.go:272; `r.cli` + defaults to bare `"secretspec"` on PATH, resolver.go:29/101) — also `>= 0.17` built with the `age` feature and pinned explicitly via `WithCLI` - (resolver.go:90) into the Server's closure so the read and write halves + (resolver.go:91) into the Server's closure so the read and write halves cannot drift to different provider capability sets (a separate prerequisite PR, Matt-ruled). (2) WIRING SEAM — today serve.go:528 constructs the single resolver with NO provider option @@ -666,7 +666,7 @@ declared into a store that does not exist. (the T0 CLI below) writes a value through `resolver.Set` for rotation on a running server. The prefix RENAMES them (`LINEAR_FORGE_CLIENT_SECRET` → `SERVER_LINEAR_FORGE_CLIENT_SECRET`) and the provider keyspace is keyed by - NAME (`setArgs`, resolver.go:315-321), so a value is populated under the + NAME (`setArgs`, resolver.go:321-327), so a value is populated under the prefixed name via the `serverSecretName()` seam (CONSUMER-REPOINT above). The NAMES are declared into `server_secrets` at boot from the RESOLVED config `cfg.Forge.resolved()` (serve.go:232-246) — the same accessor every @@ -788,7 +788,7 @@ serves; no import cycle — it depends on nothing in `secrets`). auth failure (tamper OR aad mismatch) returns an error naming no plaintext/key material. - Key encoding for provider storage: base64(std) of the 32 raw bytes - (SecretSpec values are strings; `Set` rejects empty, resolver.go:239-241). + (SecretSpec values are strings; `Set` rejects empty, resolver.go:241-243). - Consumes: `crypto/aes`, `crypto/cipher`, `crypto/rand` only. - Tests: round-trip; tamper (flip a ciphertext/nonce byte → error); `Open` under a different `aad` → error; nonce uniqueness across calls; redaction @@ -807,8 +807,8 @@ is declared into it) and T1. `GATEWAY_CREDENTIALS_MASTER_KEY` through it; on absence: `envelope.NewKey()` → `resolver.Set(ctx, name, encodedKey, "compass: provision gateway credentials master key")` - (resolver.go:234; the value rides stdin, never argv, - resolver.go:211-212) → `st.DeclareServerSecret(ctx, "", name)` with + (resolver.go:236; the value rides stdin, never argv, + resolver.go:212-213) → `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 @@ -839,17 +839,17 @@ is declared into it) and T1. the fleet):** the provider round-trips inside the lock inherit only the caller's ctx, which at boot is long-lived — but the two halves bound DIFFERENTLY. `SpecResolver.Set` IS ctx-bounded: it shells out via - `exec.CommandContext(ctx, r.cli, …)` (resolver.go:270), so a ctx deadline + `exec.CommandContext(ctx, r.cli, …)` (resolver.go:272), so a ctx deadline genuinely kills it. `SpecResolver.Resolve` is NOT: it threads ctx only into - `DeclaredSecrets` (resolver.go:145); the actual provider round-trip is - `b.Load()` (resolver.go:170), whose SDK signature carries NO ctx + `DeclaredSecrets` (resolver.go:146); the actual provider round-trip is + `b.Load()` (resolver.go:171), whose SDK signature carries NO ctx (`func (b *Builder) Load() (*Resolved, error)`, verified against secretspec-go v0.20.0 secretspec.go:293, the current pin — `Load` still carries no ctx after the bump, so the goroutine-offload design below stands) and which blocks in an uncancellable FFI call - (`nativeResolve` → `C.secretspec_resolve`, binding_cgo.go:30 / - binding_purego.go:118). A hung provider (1Password awaiting biometric + (`nativeResolve` → `C.secretspec_resolve`, binding_cgo.go:28 / + binding_purego.go:142). A hung provider (1Password awaiting biometric approval, an unreachable Vault, a half-open TCP) would otherwise hold the transaction-scoped lock indefinitely, and because the key is a shared constant EVERY other booting instance blocks on it — one stuck provider @@ -888,7 +888,7 @@ is declared into it) and T1. subsequent boot, re-resolve the name and byte-compare against the key the process is about to encrypt with; on mismatch, refuse to serve gateway-credential writes (fail closed). This re-resolve is the SAME - uncancellable `Load` (resolver.go:170) and uses the SAME bounded-offload + uncancellable `Load` (resolver.go:171) and uses the SAME bounded-offload path as the provisioning resolve (timeout ctx + own goroutine + buffered cap-1 channel), so on a steady-state boot — key already provisioned, nothing to serialize — a hung provider still yields a bounded, diagnosable @@ -923,7 +923,7 @@ is declared into it) and T1. `SetSecret`/`DeleteSecret` RPC (`authenticatedOpen`, any authenticated account — admin_gate.go:122-125) declares into `secrets` but then calls `resolver.Set`, which shells `secretspec set --profile default` - (resolver.go:315-321) against the keyspace that is SHARED under the default + (resolver.go:321-327) against the keyspace that is SHARED under the default single-URI wiring (F2) — so absent a guard a user calling `SetSecret` with name `GATEWAY_CREDENTIALS_MASTER_KEY` would OVERWRITE the master key's provider value (the running process keeps its @@ -1143,10 +1143,10 @@ RPC exactly as the frozen record already specifies. separate PR): bump `github.com/cachix/secretspec/secretspec-go` from the pinned v0.15.0 (go/go.mod:22) to `>= 0.17` with the `age` build feature, covering BOTH closures the two code paths shell separately — - the Go module for the SDK READ path (`b.Load()`, resolver.go:170) AND + the Go module for the SDK READ path (`b.Load()`, resolver.go:171) AND the `secretspec` CLI BINARY the WRITE path runs (`resolver.Set` shells - `r.cli`, resolver.go:270/29), the latter pinned via `WithCLI` - (resolver.go:90) into the Server's closure so read and write cannot drift + `r.cli`, resolver.go:272/29), the latter pinned via `WithCLI` + (resolver.go:91) into the Server's closure so read and write cannot drift to different provider capabilities. `age` is a 0.17+ build-feature provider, so on the current pin the `age://` default does not resolve and T2's master-key write-back has no writable target; a deployment on a diff --git a/go/internal/secrets/resolver.go b/go/internal/secrets/resolver.go index 0318f55dd..c820e0dae 100644 --- a/go/internal/secrets/resolver.go +++ b/go/internal/secrets/resolver.go @@ -228,10 +228,11 @@ func (r *SpecResolver) Resolve(ctx context.Context, reason string) ([]ResolvedSe // ` 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. The joined --flag=value form is required, not stylistic: the -// two-token form parses a leading-dash reason as the next flag and exits 2. +// one. So `secretspec --file= --reason= set --provider=

+// --profile=

` with the value on stdin is the write path, no positional +// VALUE. The joined --flag=value form is required on every flag, not +// stylistic: the two-token form parses a leading-dash value as the next flag +// and exits 2 (see setArgs). func (r *SpecResolver) Set(ctx context.Context, name, value, reason string) error { if err := ValidateName(name); err != nil { return err diff --git a/go/server/secrets_service.go b/go/server/secrets_service.go index 0d1eec529..27067996f 100644 --- a/go/server/secrets_service.go +++ b/go/server/secrets_service.go @@ -124,8 +124,12 @@ func (s *secretsService) SetSecret( // The audit reason carries the authenticated caller, so the provider's log // distinguishes which operator wrote a secret rather than recording every // write anonymously. The RPC is the only path that reaches this write, so - // the prefix also records that provenance. The CLI JSON-escapes the reason - // into its audit record, so an ID cannot forge a log entry. + // the prefix also records that provenance. callerID is resolved from the + // bearer token (auth.CallerFrom -> the token subject), never a request + // field, and every account id is server-minted hex (store/ids.go), so it + // cannot carry a quote or newline into the reason; the CLI additionally + // JSON-escapes the reason into its audit record, so a forged log entry is + // doubly unreachable. reason := fmt.Sprintf("compass: operator secret write via SetSecret RPC (caller %s)", callerID) if err := s.resolver.Set(ctx, msg.GetName(), msg.GetValue(), reason); err != nil { // The name was validated by DeclareSecret and the value was screened diff --git a/go/server/secrets_service_pgtest_test.go b/go/server/secrets_service_pgtest_test.go index b95985afa..818bfcceb 100644 --- a/go/server/secrets_service_pgtest_test.go +++ b/go/server/secrets_service_pgtest_test.go @@ -83,6 +83,7 @@ type secretsFixture struct { client compassv1connect.SecretsServiceClient userToken string agentToken string + userID store.AccountID resolver *recordingResolver signaler *recordingSignaler } @@ -129,6 +130,7 @@ func newSecretsFixture(t *testing.T) secretsFixture { client: newSecretsH2CClient(t, url), userToken: userTok, agentToken: agentTok, + userID: user.ID, resolver: resolver, signaler: signaler, } @@ -180,11 +182,19 @@ 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. + // The handler must hand the resolver a non-empty reason bound to the + // AUTHENTICATED caller: the provider's require_reason policy refuses a + // reasonless write outright, and the audit record is only useful if it names + // which operator wrote the secret. The reason must never carry the value. 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) } + if !strings.Contains(f.resolver.setReasons[0], string(f.userID)) { + t.Fatalf("resolver.Set reason = %q, want it to name the calling user %q", f.resolver.setReasons[0], f.userID) + } + if strings.Contains(f.resolver.setReasons[0], "postgres://x") { + t.Fatalf("resolver.Set reason = %q, must never carry the secret value", f.resolver.setReasons[0]) + } } // TestSetSecretBumpsSecretsVersion: a successful Set bumps the secrets version From cde2e778767e3c7db5a3f4cd38aaa0e11073a148 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 6 Sep 2026 15:23:11 -0400 Subject: [PATCH 5/9] fix(secrets): refresh the secrets_service citations and correct two stale v0.15 claims (RIG-3320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 review findings. No behavior change. - Design record: round 4 swept resolver.go citations only, but commit 1 shifted go/server/secrets_service.go by +10 lines, so four citations pointed at real-but-wrong code — including the F1 master-key guard's placement, which named an audit comment and a nil-resolver early return instead of the two resolver call sites. Re-point them, and audit EVERY citation into a file this PR touches rather than one filename: all 48 now resolve (resolver.go x34, secrets_service.go x12, go.mod x2). - resolver.go + secrets_service.go: the `Delete` no-op was rationalized as "no CLI verb upstream". True at v0.15.0, false at this pin — `secretspec delete` shipped in 0.18 and is in the staged binary. Say the verb exists and that wiring it is a deliberate deferral (RIG-3436): it makes the operation destructive against a keyspace shared by default, so it needs its own F1-guard and ordering analysis. Same correction in the record. - resolver.go: `defaultCLI` claimed the dev shell and the deployed image both stage the binary. Only the dev shell does; nothing stages it into the shipped artifact. Narrow the claim to what holds and point at RIG-3437. - secrets_service_pgtest_test.go: hoist the secret literal so the value-absence assertion cannot silently stop testing anything when the input changes. Refs RIG-3320 Co-authored-by: Matt Wilkinson --- ...ass-gateway-credentials-at-rest-encryption.md | 9 +++++---- go/internal/secrets/resolver.go | 16 ++++++++++------ go/server/secrets_service.go | 7 ++++--- go/server/secrets_service_pgtest_test.go | 9 ++++++--- 4 files changed, 25 insertions(+), 16 deletions(-) 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 d4fd521e1..9a88d5229 100644 --- a/docs/designs/server/compass-gateway-credentials-at-rest-encryption.md +++ b/docs/designs/server/compass-gateway-credentials-at-rest-encryption.md @@ -288,7 +288,7 @@ delivery/kind, classified `adminOnly` in `classifyProcedure` is treated as adminOnly — fail closed, never admit an unknown method as open", admin_gate.go:44-46). Today's user-facing `SetSecret` cannot declare a row *as* a server secret — `secretRoutingFromProto` admits only File/Env -delivery (secrets_service.go:275-284), with no server-only delivery value — so +delivery (secrets_service.go:285-307), with no server-only delivery value — so operators declare server secrets through the new RPC. It CAN, however, mint an ordinary `secrets` row under a NAME that collides with a server secret (delivery=File/Env), which is exactly why the F1 prefix guard is mandatory on @@ -578,7 +578,7 @@ declared into a store that does not exist. every agent container, inverting D6. - RPC: `SetServerSecret`/`DeleteServerSecret` on the secrets service — mirrors the `SetSecret` declare-then-Set flow and its rollback - discipline (secrets_service.go:92-145) minus delivery/kind, targeting + discipline (secrets_service.go:92-155) minus delivery/kind, targeting `server_secrets`; admin-gated (`adminOnly` in `classifyProcedure`, admin_gate.go:47). Carries the F1 PREFIX guard: it REQUIRES the declared name to carry a reserved server-secret prefix (`SERVER_` for the six forge @@ -605,7 +605,7 @@ declared into a store that does not exist. door, so the shadow row can never be created at all) MUST REJECT any name carrying a reserved server-secret prefix (`SERVER_` or `GATEWAY_CREDENTIALS_`) — checked BEFORE `resolver.Set`/`Delete` - (secrets_service.go:124/208). A pure string check, not a membership SELECT: + (secrets_service.go:134/218). A pure string check, not a membership SELECT: it needs no read of `server_secrets` and no cross-tenant visibility. A T0 deliverable on the existing user path, not only the new admin RPC. @@ -942,7 +942,8 @@ is declared into it) and T1. unchanged). Rotation is OQ-1's machinery, never a raw overwrite through either surface. The name-keyed global user delete ("a row is keyed by name alone, not (actor, name)", go/internal/store/secrets.go:150-153) is why the - guard covers the delete path too, once a real provider hard-delete lands. + guard covers the delete path too, once the provider hard-delete is wired + (the `secretspec delete` verb exists at the 0.20 pin; RIG-3436). - **Nil-resolver deployment:** a server built with no secrets surface is legitimate today ("resolver may be nil on a server built with no secrets surface (FetchSecrets then fails CodeFailedPrecondition rather diff --git a/go/internal/secrets/resolver.go b/go/internal/secrets/resolver.go index c820e0dae..78d75b66f 100644 --- a/go/internal/secrets/resolver.go +++ b/go/internal/secrets/resolver.go @@ -23,9 +23,10 @@ const manifestProject = "compass" 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. +// off PATH. The dev shell stages it (devenv.nix); nothing stages it into the +// shipped artifact, so a deployment must put a binary at or above the floor on +// the server's PATH — see TestSecretSpecCLIVersionFloor and RIG-3437. Named so +// that floor guard 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 @@ -282,9 +283,12 @@ func (r *SpecResolver) Set(ctx context.Context, name, value, reason string) erro // Delete removes name's value from the provider. See the package/record note: // with a manifest-driven resolver only declared names ever resolve, so removing // the store declaration (store.DeleteSecretDeclaration) is the effective MVP -// delete; a provider-value hard-delete has no CLI verb upstream. This method is -// the seam for that write once a verb exists; today it validates the name and -// is a no-op success so the T7 handler can call one uniform surface. +// delete. A provider-value hard-delete IS available at this pin (`secretspec +// delete`, 0.18+); wiring it is a deliberate deferral (RIG-3436), not an +// upstream gap — it makes the operation destructive against a keyspace shared +// by default, so it needs its own F1-guard and ordering analysis. This method +// is the seam for that write; today it validates the name and is a no-op +// success so the T7 handler can call one uniform surface. func (r *SpecResolver) Delete(ctx context.Context, name string) error { if err := ValidateName(name); err != nil { return err diff --git a/go/server/secrets_service.go b/go/server/secrets_service.go index 27067996f..d8619de3c 100644 --- a/go/server/secrets_service.go +++ b/go/server/secrets_service.go @@ -208,9 +208,10 @@ func (s *secretsService) DeleteSecret( return nil, connect.NewError(connect.CodeUnavailable, errNoResolver) } name := req.Msg.GetName() - // Ordering note: resolver.Delete is a validate-only no-op today (no upstream - // provider hard-delete verb), so calling it before DeleteSecretDeclaration is - // inert. When a real provider delete lands, this MUST flip to + // Ordering note: resolver.Delete is a validate-only no-op today, so calling + // it before DeleteSecretDeclaration is inert. The provider verb it would + // shell EXISTS at this pin (`secretspec delete`, 0.18+); wiring it is a + // deferral (RIG-3436), not an upstream gap. When it lands, this MUST flip to // declaration-first: the declaration is the source of truth Resolve reads, and // deleting the provider value before the row would leave a required=true // declaration pointing at a missing value — the same global resolve-poison as a diff --git a/go/server/secrets_service_pgtest_test.go b/go/server/secrets_service_pgtest_test.go index 818bfcceb..9f9a7e173 100644 --- a/go/server/secrets_service_pgtest_test.go +++ b/go/server/secrets_service_pgtest_test.go @@ -175,8 +175,11 @@ func TestSetSecretUserOnly(t *testing.T) { t.Fatalf("resolver.Set called %v on a rejected agent SetSecret, want none", f.resolver.setNames) } - // User: succeeds and writes the value. - if _, err := f.client.SetSecret(ctx, setReq(f.userToken, "DB_URL", "postgres://x")); err != nil { + // User: succeeds and writes the value. The literal is hoisted because the + // value-absence assertion below asserts on it — inlining it twice lets the + // two drift, silently retiring that assertion. + const secretValue = "postgres://x" + if _, err := f.client.SetSecret(ctx, setReq(f.userToken, "DB_URL", secretValue)); err != nil { t.Fatalf("SetSecret as user = %v, want success", err) } if len(f.resolver.setNames) != 1 || f.resolver.setNames[0] != "DB_URL" { @@ -192,7 +195,7 @@ func TestSetSecretUserOnly(t *testing.T) { if !strings.Contains(f.resolver.setReasons[0], string(f.userID)) { t.Fatalf("resolver.Set reason = %q, want it to name the calling user %q", f.resolver.setReasons[0], f.userID) } - if strings.Contains(f.resolver.setReasons[0], "postgres://x") { + if strings.Contains(f.resolver.setReasons[0], secretValue) { t.Fatalf("resolver.Set reason = %q, must never carry the secret value", f.resolver.setReasons[0]) } } From 77e48e327b9f80d2d75978107582702f69e1f0f1 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 6 Sep 2026 16:17:25 -0400 Subject: [PATCH 6/9] docs(secrets): re-point every citation this stack shifted, computed not hand-counted (RIG-3320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 review found a high. The previous commit re-pointed the secrets_service.go citations and, in the same commit, edited resolver.go — adding a line to the defaultCLI comment and three to the Delete comment — without re-pointing the 34 resolver.go citations. 29 went stale, 7 of them onto real-but-wrong executable code: resolver.go:171 is cited four times as the uncancellable FFI `b.Load()` that anchors T2's whole bounded-offload design, and it had come to resolve to `b = b.WithProfile(profile)`, a pure builder call with no FFI and nothing to block on. An agent implementing from that reads the offload machinery as unnecessary. Same class of break on the F1 master-key guard citation: :218 landed on a comment, one line above the `resolver.Delete` call it names. The root cause is not carelessness on any one line, it is that the audit was done by hand twice and both times scoped to the file I had most recently thought about. So this pass computes it: diff each touched file against the last pushed revision, build a parent-line -> head-line map from the matching blocks, and rewrite every citation through the map. 31 refs moved. Then verify in the other direction — every cited anchor is checked to still name the construct the prose says it does (b.Load(), the Set/setArgs signatures, exec.CommandContext, the F1 pre-call sites, go.mod:22), because in-range is not the same as correct and only the second check would have caught this. Also from the review: - Two F2 passages said the repo "pins ... at v0.15.0" while citing go/go.mod:22, which this PR changes to v0.20.0 — the citation contradicted the sentence quoting it. Reframed as what it is: the pin as of writing, with the bump noted. - Dropped the value-never-in-argv loop from TestSetArgs. setArgs takes no value parameter, so the string it scans for is never in scope and no mutation could redden it; it read like the guard while asserting nothing. The comment now points at TestSetFeedsValueOnStdinNeverArgv, which spawns a real process and checks the child's actual argv. Refs RIG-3320 Co-authored-by: Matt Wilkinson --- ...-gateway-credentials-at-rest-encryption.md | 68 ++++++++++--------- go/internal/secrets/resolver_test.go | 11 ++- 2 files changed, 40 insertions(+), 39 deletions(-) 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 9a88d5229..e9b310683 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:236`, `func (r *SpecResolver) Set(ctx + (`go/internal/secrets/resolver.go:237`, `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:212-213); + resolver.go:213-214); - 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) @@ -208,11 +208,11 @@ default-open-minus-a-filter. Why the table boundary IS the delivery boundary: the delivery surface is the resolver's MANIFEST. `SpecResolver` reads its declared set through the `declarations` interface — `DeclaredSecrets(ctx context.Context) -([]store.SecretDeclaration, error)` (`go/internal/secrets/resolver.go:35-37`; -the `store declarations` struct field, resolver.go:66) — and `buildManifest` +([]store.SecretDeclaration, error)` (`go/internal/secrets/resolver.go:36-38`; +the `store declarations` struct field, resolver.go:67) — and `buildManifest` "renders the SecretSpec manifest TOML for a declared set: one `[project]` block and one `[profiles.]` block with every declared name as a -required key" (resolver.go:109-111; the function, resolver.go:115-138); +required key" (resolver.go:110-112; the function, resolver.go:116-139); `Resolve` can only return names present in that manifest. Today ONE resolver instance (`resolver := secrets.NewSpecResolver(st, secretsStateDir(cfg))`, `go/server/serve.go:528`) serves BOTH the container path (FetchSecrets → @@ -252,7 +252,7 @@ with the per-tenant defer (RIG-3237). C1 keeps the SAME SecretSpec profile for both instances — the shared project is `manifestProject = "compass"` (resolver.go:19) and the profile `defaultProfile = "default"` (resolver.go:23; `WithProfile` exists, -resolver.go:88, but C1 does not use it), and BY DEFAULT one provider URI +resolver.go:89, but C1 does not use it), and BY DEFAULT one provider URI configures both resolver instances (F2 WIRING SEAM) — so the provider keyspace is shared unless the operator opts Layer B onto a different provider. Because the reserved-prefix partition renames the six forge secrets under @@ -288,7 +288,7 @@ delivery/kind, classified `adminOnly` in `classifyProcedure` is treated as adminOnly — fail closed, never admit an unknown method as open", admin_gate.go:44-46). Today's user-facing `SetSecret` cannot declare a row *as* a server secret — `secretRoutingFromProto` admits only File/Env -delivery (secrets_service.go:285-307), with no server-only delivery value — so +delivery (secrets_service.go:286-308), with no server-only delivery value — so operators declare server secrets through the new RPC. It CAN, however, mint an ordinary `secrets` row under a NAME that collides with a server secret (delivery=File/Env), which is exactly why the F1 prefix guard is mandatory on @@ -338,7 +338,7 @@ rejected branches are the ones a future reader will reach for first. contract — where C1's proto delta is two additive `SecretsService` methods. - **Mechanism C2: separate table + a separate SecretSpec PROFILE — considered, DEFERRED.** The same `server_secrets` table, but the server - resolver pinned to its own profile (`WithProfile`, resolver.go:88) so even + resolver pinned to its own profile (`WithProfile`, resolver.go:89) so even the provider keyspace is isolated. Fullest isolation — but it is NOT the cheaper-migration loser it might appear: under the reserved-prefix partition BOTH mechanisms now require a provider write under new keys (the six values @@ -396,7 +396,7 @@ rejected branches are the ones a future reader will reach for first. - AES-256-GCM only; nonces from `crypto/rand`, 96-bit, fresh per encryption, never counter-derived; key is 256-bit from `crypto/rand`. - The master key NEVER appears in the DB, in logs, in argv (Set feeds stdin, - resolver.go:212-213), or in error strings. + resolver.go:213-214), or in error strings. - Auto-provisioning is zero-human-step (rule://no-human-clicks): first boot generates, stores, and declares the key with no operator action. - The names-only invariant (`secrets.go:20-22`) is preserved for EVERYTHING @@ -491,7 +491,7 @@ declared into a store that does not exist. `ServerDeclaredSecrets(ctx)` — a thin store view whose `DeclaredSecrets(ctx context.Context) ([]store.SecretDeclaration, error)` method (the `declarations` - interface shape, resolver.go:35-37) reads `server_secrets`, mapping + interface shape, resolver.go:36-38) reads `server_secrets`, mapping rows to `store.SecretDeclaration` with generic kind / zero delivery (the resolver uses only the name to build its manifest), so `NewSpecResolver` is reused UNCHANGED. @@ -605,7 +605,7 @@ declared into a store that does not exist. door, so the shadow row can never be created at all) MUST REJECT any name carrying a reserved server-secret prefix (`SERVER_` or `GATEWAY_CREDENTIALS_`) — checked BEFORE `resolver.Set`/`Delete` - (secrets_service.go:134/218). A pure string check, not a membership SELECT: + (secrets_service.go:134/219). A pure string check, not a membership SELECT: it needs no read of `server_secrets` and no cross-tenant visibility. A T0 deliverable on the existing user path, not only the new admin RPC. @@ -620,12 +620,13 @@ declared into a store that does not exist. (`awssm` (any pin) or `awsps` (0.18+) on AWS, `akv` (any pin) or `aac` (0.20+) on Azure), and one already running Vault/OpenBao uses that — the provider is a per-resolver-INSTANCE config - choice (`WithProvider`, resolver.go:82/84), not a hardcode, so this is a + choice (`WithProvider`, resolver.go:83/85), not a hardcode, so this is a recommended default rather than a fixed backend. Two things make this an EXECUTABLE T0 deliverable rather than prose: (1) DEPENDENCY PREREQUISITE — `age` is a secretspec 0.17+ provider behind an `age` build feature, but - the repo pins `github.com/cachix/secretspec/secretspec-go` at v0.15.0 - (go/go.mod:22), where `age://` does not resolve and T2's master-key + the repo pinned `github.com/cachix/secretspec/secretspec-go` at v0.15.0 + when this record was written (go/go.mod:22 now reads v0.20.0, bumped by + RIG-3320), and at that pin `age://` did not resolve and T2's master-key write-back has no writable target. This gates the self-hosted `age://` DEFAULT specifically, NOT T0/T2 wholesale: on the current pin the matrix already marks Write yes for `keyring`, `dotenv`, `pass`, `gopass` @@ -637,18 +638,18 @@ declared into a store that does not exist. default — a single local identity file rather than a GPG keyring). The bump covers TWO separate closures, since the SDK read path and the CLI write path are distinct binaries: (a) the Go module `secretspec-go` `>= - 0.17` for the READ path (`b.Load()`, resolver.go:171); AND (b) the + 0.17` for the READ path (`b.Load()`, resolver.go:172); AND (b) the `secretspec` CLI BINARY the WRITE path shells via `resolver.Set` - (`exec.CommandContext(ctx, r.cli, args...)`, resolver.go:272; `r.cli` - defaults to bare `"secretspec"` on PATH, resolver.go:29/101) — also `>= + (`exec.CommandContext(ctx, r.cli, args...)`, resolver.go:273; `r.cli` + defaults to bare `"secretspec"` on PATH, resolver.go:30/102) — also `>= 0.17` built with the `age` feature and pinned explicitly via `WithCLI` - (resolver.go:91) into the Server's closure so the read and write halves + (resolver.go:92) into the Server's closure so the read and write halves cannot drift to different provider capability sets (a separate prerequisite PR, Matt-ruled). (2) WIRING SEAM — today serve.go:528 constructs the single resolver with NO provider option (the SDK default chain); T0 threads the operator-configured URI, from a NEW server flag/env (defaulting to the `age://` path), through - `secrets.WithProvider()` (resolver.go:84). By DEFAULT the SAME URI + `secrets.WithProvider()` (resolver.go:85). By DEFAULT the SAME URI configures BOTH resolver instances — the SERVER (server-secret) resolver AND the container/user resolver at serve.go:528 — so the provider keyspace is shared by construction and the F1 guard + D2 read-back below are @@ -666,7 +667,7 @@ declared into a store that does not exist. (the T0 CLI below) writes a value through `resolver.Set` for rotation on a running server. The prefix RENAMES them (`LINEAR_FORGE_CLIENT_SECRET` → `SERVER_LINEAR_FORGE_CLIENT_SECRET`) and the provider keyspace is keyed by - NAME (`setArgs`, resolver.go:321-327), so a value is populated under the + NAME (`setArgs`, resolver.go:325-331), so a value is populated under the prefixed name via the `serverSecretName()` seam (CONSUMER-REPOINT above). The NAMES are declared into `server_secrets` at boot from the RESOLVED config `cfg.Forge.resolved()` (serve.go:232-246) — the same accessor every @@ -788,7 +789,7 @@ serves; no import cycle — it depends on nothing in `secrets`). auth failure (tamper OR aad mismatch) returns an error naming no plaintext/key material. - Key encoding for provider storage: base64(std) of the 32 raw bytes - (SecretSpec values are strings; `Set` rejects empty, resolver.go:241-243). + (SecretSpec values are strings; `Set` rejects empty, resolver.go:242-244). - Consumes: `crypto/aes`, `crypto/cipher`, `crypto/rand` only. - Tests: round-trip; tamper (flip a ciphertext/nonce byte → error); `Open` under a different `aad` → error; nonce uniqueness across calls; redaction @@ -807,8 +808,8 @@ is declared into it) and T1. `GATEWAY_CREDENTIALS_MASTER_KEY` through it; on absence: `envelope.NewKey()` → `resolver.Set(ctx, name, encodedKey, "compass: provision gateway credentials master key")` - (resolver.go:236; the value rides stdin, never argv, - resolver.go:212-213) → `st.DeclareServerSecret(ctx, "", name)` with + (resolver.go:237; the value rides stdin, never argv, + resolver.go:213-214) → `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 @@ -839,10 +840,10 @@ is declared into it) and T1. the fleet):** the provider round-trips inside the lock inherit only the caller's ctx, which at boot is long-lived — but the two halves bound DIFFERENTLY. `SpecResolver.Set` IS ctx-bounded: it shells out via - `exec.CommandContext(ctx, r.cli, …)` (resolver.go:272), so a ctx deadline + `exec.CommandContext(ctx, r.cli, …)` (resolver.go:273), so a ctx deadline genuinely kills it. `SpecResolver.Resolve` is NOT: it threads ctx only into - `DeclaredSecrets` (resolver.go:146); the actual provider round-trip is - `b.Load()` (resolver.go:171), whose SDK signature carries NO ctx + `DeclaredSecrets` (resolver.go:147); the actual provider round-trip is + `b.Load()` (resolver.go:172), whose SDK signature carries NO ctx (`func (b *Builder) Load() (*Resolved, error)`, verified against secretspec-go v0.20.0 secretspec.go:293, the current pin — `Load` still carries no ctx after the bump, so the goroutine-offload design below @@ -888,7 +889,7 @@ is declared into it) and T1. subsequent boot, re-resolve the name and byte-compare against the key the process is about to encrypt with; on mismatch, refuse to serve gateway-credential writes (fail closed). This re-resolve is the SAME - uncancellable `Load` (resolver.go:171) and uses the SAME bounded-offload + uncancellable `Load` (resolver.go:172) and uses the SAME bounded-offload path as the provisioning resolve (timeout ctx + own goroutine + buffered cap-1 channel), so on a steady-state boot — key already provisioned, nothing to serialize — a hung provider still yields a bounded, diagnosable @@ -923,7 +924,7 @@ is declared into it) and T1. `SetSecret`/`DeleteSecret` RPC (`authenticatedOpen`, any authenticated account — admin_gate.go:122-125) declares into `secrets` but then calls `resolver.Set`, which shells `secretspec set --profile default` - (resolver.go:321-327) against the keyspace that is SHARED under the default + (resolver.go:325-331) against the keyspace that is SHARED under the default single-URI wiring (F2) — so absent a guard a user calling `SetSecret` with name `GATEWAY_CREDENTIALS_MASTER_KEY` would OVERWRITE the master key's provider value (the running process keeps its @@ -1081,7 +1082,7 @@ RPC exactly as the frozen record already specifies. WRITABLE SecretSpec provider (self-hosted default `age://` — writable, encrypted-at-rest, headless; a cloud store or Vault/OpenBao where present — F2) resolved by a SERVER resolver constructed - `secrets.WithProvider()` (resolver.go:84) off a + `secrets.WithProvider()` (resolver.go:85) off a NEW server flag/env for that URI (serve.go:528's container resolver unchanged), populated by the operator directly (deploy tooling seeds the age file) or, for rotation on a running server, through the NEW `compass @@ -1142,12 +1143,13 @@ RPC exactly as the frozen record already specifies. startup with the actionable static error). DEPENDENCY PREREQUISITE (gating for the self-hosted `age://` default path only, Matt-ruled separate PR): bump `github.com/cachix/secretspec/secretspec-go` from the - pinned v0.15.0 (go/go.mod:22) to `>= 0.17` with the `age` build + v0.15.0 pinned when this record was written to `>= 0.17` (DONE in + RIG-3320: go/go.mod:22 now reads v0.20.0) with the `age` build feature, covering BOTH closures the two code paths shell separately — - the Go module for the SDK READ path (`b.Load()`, resolver.go:171) AND + the Go module for the SDK READ path (`b.Load()`, resolver.go:172) AND the `secretspec` CLI BINARY the WRITE path runs (`resolver.Set` shells - `r.cli`, resolver.go:272/29), the latter pinned via `WithCLI` - (resolver.go:91) into the Server's closure so read and write cannot drift + `r.cli`, resolver.go:273/30), the latter pinned via `WithCLI` + (resolver.go:92) into the Server's closure so read and write cannot drift to different provider capabilities. `age` is a 0.17+ build-feature provider, so on the current pin the `age://` default does not resolve and T2's master-key write-back has no writable target; a deployment on a diff --git a/go/internal/secrets/resolver_test.go b/go/internal/secrets/resolver_test.go index c1e7948e1..c813d7763 100644 --- a/go/internal/secrets/resolver_test.go +++ b/go/internal/secrets/resolver_test.go @@ -134,12 +134,11 @@ func TestSetArgs(t *testing.T) { } } - // The value must NEVER be in the constructed argv — it rides stdin. - for _, a := range gotFull { - if strings.Contains(a, "the-secret-value") { - t.Errorf("value leaked into argv: %v", gotFull) - } - } + // The value-never-in-argv invariant is NOT asserted here: setArgs takes no + // value parameter, so no mutation of it could put the value in this argv and + // any check would pass vacuously. It is defended at the exec boundary by + // TestSetFeedsValueOnStdinNeverArgv, which spawns a real process, captures + // the child's actual argv, and asserts the value arrived on stdin instead. } func equalArgs(a, b []string) bool { From d56c9dbe85f90922e24abe4102c1536741fb0ce7 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 6 Sep 2026 17:22:23 -0400 Subject: [PATCH 7/9] docs(secrets): re-point the citations this PR shifted in OTHER records too (RIG-3320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-5 review. Last round's fix computed the line map correctly and then applied it to exactly one document — the record this PR already edits. But the citations of a shifted file are not confined to the record that happens to discuss it: resolver.go moved +67 lines and devenv.nix +15, and 81 more citations of those two files live in sixteen OTHER frozen records this PR never touched. They were right at the merge base and wrong at head. Same defect as round 4, one scope level out, and the previous commit message's claim to 'rewrite every citation through the map' described the map's construction rather than where it got applied. So the unit of the audit is now the SHIFTED FILE, not the edited document: build a base->head line map for every code file this stack touches, then sweep every markdown file in docs/ for citations into any of them. 27 resolver.go refs across four records (native-client-mode, ownership-layer, linear-agent-responder, forge-poll-driver) and 54 devenv.nix refs across twelve more. The worst was the same b.Load() failure round 4 found: native-client-mode cited resolver.go:162-164 for the WithProvider/WithProfile/Load chain, which at head is a comment about temp-file cleanup. Verified by text identity rather than by range, in both directions: for all 156 citation endpoints, the line the base document pointed at and the line the head document points at are byte-identical. That check is what distinguishes a correct citation from a merely in-range one, and it is the check that would have caught both this and the round-4 high. Two more from the review, both in the record this PR edits: - The v0.15 reframing fixed the sentence quoting go.mod:22 but left the surrounding 'on the current pin' prose meaning v0.15, so each paragraph contradicted itself within four lines — stating the bump is done at v0.20.0 while enumerating the v0.15 capability matrix and calling age:// gated. An agent implementing T0/T2's provider wiring could not tell whether the record's own ruled default was available. Both paragraphs now speak of v0.15 in the past tense and state the prerequisite is met. - The record's one inline spelling of the write command was the pre-PR shape, missing the two joined global flags this PR makes mandatory — it taught the argv form this PR's own test reddens. Refs RIG-3320 Co-authored-by: Matt Wilkinson --- .../agent/compass-forks-reversal/design.md | 12 +++---- .../compass-forks-reversal/oq-resolutions.md | 2 +- .../ci/compass-agent-image-publish/design.md | 6 ++-- .../ci/compass-devenv-source-dry/design.md | 16 ++++----- .../infra/ci/compass-dogfood-e2e/design.md | 10 +++--- .../infra/ci/compass-local-dev/design.md | 36 +++++++++---------- .../infra/ci/compass-pr-validation/design.md | 2 +- .../compass-runner-arbitrary-uid/design.md | 6 ++-- docs/designs/repo/compass-drop-proto.md | 2 +- .../repo/compass-renovate-migration.md | 6 ++-- .../compass-forge-poll-driver/design.md | 6 ++-- ...-gateway-credentials-at-rest-encryption.md | 32 +++++++++-------- .../compass-linear-agent-responder/design.md | 18 +++++----- .../compass-server-ownership-layer/design.md | 26 +++++++------- .../ui/compass-gtk4-migration/design.md | 4 +-- docs/designs/ui/compass-native-app/design.md | 6 ++-- .../ui/compass-native-client-mode/design.md | 4 +-- 17 files changed, 99 insertions(+), 95 deletions(-) diff --git a/docs/designs/agent/compass-forks-reversal/design.md b/docs/designs/agent/compass-forks-reversal/design.md index 24e08cbc6..ae6c06c4f 100644 --- a/docs/designs/agent/compass-forks-reversal/design.md +++ b/docs/designs/agent/compass-forks-reversal/design.md @@ -85,7 +85,7 @@ the two, compass keeps building against the previous pinned rev. the fork's CLI is also invoked raw (`nix run path:../forks/#…`) in the raw-CLI call sites (six files, incl. `tools/agent-image-env-gate/index.ts`) that bypass the lock (see L1/L2 Interfaces). Today a single `path:` tree -makes CLI-rev == module-rev by construction (`devenv.nix:442-445` names exactly +makes CLI-rev == module-rev by construction (`devenv.nix:457-460` names exactly this as the reason for the pin shape). The flake-input side follows the internal monorepo's frozen default — `github:RigelBuild/` pinned via `devenv.lock` — but the six raw-CLI sites bypass that lock, so the reversal MUST separately @@ -124,7 +124,7 @@ is compass's review point — not a mechanical path swap. (`"path": "../forks/devenv"`). - The fork's own CLI is invoked by path everywhere the image is built: `agent-image/moon.yml:44` (`command: 'nix run path:../forks/devenv#devenv -- - container build agent'`), `agent-image/publish.sh:50-51`, `devenv.nix:461` + container build agent'`), `agent-image/publish.sh:50-51`, `devenv.nix:476` (`nix run path:../forks/devenv#devenv -- container copy agent`), `.github/workflows/ci.yml:812`, and `tools/agent-image-env-gate/index.ts:100` — the fail-closed image-env gate @@ -405,7 +405,7 @@ Interfaces: - CLI invocations `nix run path:../forks/devenv#devenv` → `nix run github:RigelBuild/devenv/#devenv` (rev-pinned literal, same rationale as L1): `agent-image/moon.yml:44`, `agent-image/publish.sh:50-51`, - `devenv.nix:461`, `.github/workflows/ci.yml:812`, + `devenv.nix:476`, `.github/workflows/ci.yml:812`, `tools/agent-image-env-gate/index.ts:100`. - Deletes: `forks/devenv/` (incl. `.upstream-sync`, `moon.yml`); `.moon/workspace.yml:78` (`devenv-fork: 'forks/devenv'`); @@ -433,7 +433,7 @@ Interfaces: surface" — the flake is now fetched from `RigelBuild/devenv` at a pinned rev, not an in-repo file). - Gate: `moon run agent-image:build` green; `dogfood:agent-image` - (`devenv.nix:456-464`) loads the image; agent container smoke ($HOME = + (`devenv.nix:471-479`) loads the image; agent container smoke ($HOME = `/home/agent`, nix usable) — the exact property the patch protects; the OQ2 pin shape applied consistently with L1. @@ -573,7 +573,7 @@ Interfaces: imminent and named. 2. **Rev-pinned CLI literals vs one lockfile for the raw `nix run` invocations** (`agent-image/publish.sh:32,50-51`, `agent-image/moon.yml:44`, - `devenv.nix:461`, `ci.yml:812`, `publish-agent-image.yml:139,160`, + `devenv.nix:476`, `ci.yml:812`, `publish-agent-image.yml:139,160`, `tools/agent-image-env-gate/index.ts:100,118`) — **LOAD-BEARING, but narrower than first framed.** The *flake-input* half is settled: the internal monorepo's prior art froze the nix-flake-input class as @@ -586,7 +586,7 @@ Interfaces: compass invokes the fork CLI raw at the six sites above, which bypass `devenv.lock` entirely. Today a single `path:` tree makes the CLI rev and the locked - module-set rev identical by construction (`devenv.nix:442-445` names this as + module-set rev identical by construction (`devenv.nix:457-460` names this as the reason for the pin shape; the frozen dogfood-loop record makes the same argument, `docs/designs/platform/compass-dogfood-loop/design.md:225-229`). Scattering a `github:…/` literal across those six lockfile-bypass sites diff --git a/docs/designs/agent/compass-forks-reversal/oq-resolutions.md b/docs/designs/agent/compass-forks-reversal/oq-resolutions.md index c6a3238ec..62bd7e56b 100644 --- a/docs/designs/agent/compass-forks-reversal/oq-resolutions.md +++ b/docs/designs/agent/compass-forks-reversal/oq-resolutions.md @@ -85,7 +85,7 @@ raw invocation. Impact on **L1** (`design.md:348-386`) and **L2** (`design.md:388-435`): - The six raw-`nix run` sites — `agent-image/publish.sh:32,50-51`, - `agent-image/moon.yml:44`, `devenv.nix:461`, `.github/workflows/ci.yml:812`, + `agent-image/moon.yml:44`, `devenv.nix:476`, `.github/workflows/ci.yml:812`, `.github/workflows/publish-agent-image.yml:139,160`, `tools/agent-image-env-gate/index.ts:100,118` — install and invoke the fork tools by explicit name from the `github:RigelBuild/{devenv,nix2container}` diff --git a/docs/designs/infra/ci/compass-agent-image-publish/design.md b/docs/designs/infra/ci/compass-agent-image-publish/design.md index 540a7143b..f95229c2a 100644 --- a/docs/designs/infra/ci/compass-agent-image-publish/design.md +++ b/docs/designs/infra/ci/compass-agent-image-publish/design.md @@ -186,7 +186,7 @@ silent-staleness failure the doctrine targets. What it *does* buy: `cancel-in-progress: false` group, which a separate workflow gives cleanly. - **Off the hot path, not a required check.** The image closure is the heavy nix build that motivates CI's 90m timeout (`ci.yml:93-95`); the dogfood task - is opt-in for the same reason (`devenv.nix:340-348`). A separate workflow + is opt-in for the same reason (`devenv.nix:355-363`). A separate workflow keeps PR latency untouched, and a publish flake never reds the required merge gate. - **Failure ownership.** `agent-image/` is not a moon project, so the gate @@ -325,7 +325,7 @@ visibility ever changes. - Comments and docs explain non-obvious WHY (compass `AGENTS.md`). - The image build stays OFF the hot `up`/PR path: publish is main-only + `workflow_dispatch`, mirroring the opt-in `dogfood:agent-image` posture - (`devenv.nix:345-348` — "NOT wired `after` into up — the image closure is + (`devenv.nix:360-363` — "NOT wired `after` into up — the image closure is large"). - Reproducibility: the published `:git-` and the local dogfood load are copies of the SAME nix derivation — both flow through @@ -349,7 +349,7 @@ locally (with a PAT-backed `skopeo login`) and from CI identically. - Build: `nix run path:../forks/devenv#devenv -- container build agent` executed with cwd `agent-image/` (the same fork-pinned invocation shape as - `dogfood:agent-image`, `devenv.nix:349-354`); capture the printed image-spec + `dogfood:agent-image`, `devenv.nix:364-369`); capture the printed image-spec store path. - Skopeo: `nix run path:../forks/nix2container#skopeo-nix2container --` (exposed at `forks/nix2container/flake.nix:31`; `pkgs.skopeo`'s diff --git a/docs/designs/infra/ci/compass-devenv-source-dry/design.md b/docs/designs/infra/ci/compass-devenv-source-dry/design.md index 06b4bc19b..a1463e45d 100644 --- a/docs/designs/infra/ci/compass-devenv-source-dry/design.md +++ b/docs/designs/infra/ci/compass-devenv-source-dry/design.md @@ -18,8 +18,8 @@ grep for `15a81f3e` this session): `agent-image/moon.yml:45` (`command: 'nix run github:RigelBuild/devenv/15a81f3e…#devenv -- container build agent'`), `agent-image/publish.sh:62` (`BUILD_OUT="$(nix run github:RigelBuild/devenv/15a81f3e…#devenv -- container build agent)"`), -`devenv.nix:537` (the `dogfood:agent-image` task — whose own comment, -devenv.nix:519-521, claims the pin "cannot diverge from the fork source the +`devenv.nix:552` (the `dogfood:agent-image` task — whose own comment, +devenv.nix:534-536, claims the pin "cannot diverge from the fork source the agent-image module set is pinned to", precisely the drift this record proves IS possible), and `tools/agent-image-env-gate/index.ts:103` (the env gate's `nix run … container build`, which runs in the CI moon graph: @@ -210,7 +210,7 @@ renovate.yml tracks the root lock. bun is on PATH in this job (ci.yml:1032-1042's toolchain bootstrap precedes 1153). **The four other hand-pins** (`agent-image/moon.yml:45`, -`agent-image/publish.sh:61-62`, `devenv.nix:537`, and +`agent-image/publish.sh:61-62`, `devenv.nix:552`, and `tools/agent-image-env-gate/index.ts:103`) are the same drift class but different execution contexts (moon and devenv.nix command strings can't shell out to compose a flakeref; publish.sh runs in a workflow with no bun bootstrap, @@ -347,7 +347,7 @@ comment block (ci.yml:1120-1122) to describe lock-resolution. Convert all four (own PR, after T2/T3): -- `agent-image/moon.yml:45` and `devenv.nix:537` — command strings that cannot +- `agent-image/moon.yml:45` and `devenv.nix:552` — command strings that cannot compose a flakeref inline; each becomes a small wrapper invocation (a `script:`/wrapper entry point that runs `bun … devenv-cli … --mode flakeref` then `nix run "$src"`; exact mechanism at impl). @@ -365,7 +365,7 @@ Convert all four (own PR, after T2/T3): - Consumes: `--lock agent-image/devenv.lock --mode flakeref` (shell sites) or the `core.ts` exports directly (env-gate). - Produces: `agent-image/moon.yml:45`, `agent-image/publish.sh:61` (log) and - `:62` (executable), `devenv.nix:537`, + `:62` (executable), `devenv.nix:552`, `tools/agent-image-env-gate/index.ts:103` all free of literal revs. ### T2b — (from Alternative (f)) Repo-wide literal-devenv-rev gate @@ -461,7 +461,7 @@ automatically. - [ ] T3 — ci.yml:1153 seed step → resolve via tool (`--lock agent-image/devenv.lock --mode flakeref`) + `nix run "$src"` - [ ] T2a — (ruled RD-2) de-pin the four remaining sites via the tool: - agent-image/moon.yml:45, publish.sh:61 (log) + :62 (exec), devenv.nix:537, + agent-image/moon.yml:45, publish.sh:61 (log) + :62 (exec), devenv.nix:552, tools/agent-image-env-gate/index.ts:103 - [ ] T2b — repo-wide literal-devenv-rev gate with a day-one carve-out for docs/designs/** + comment/log sites (kills the class; lands after T2a) @@ -522,8 +522,8 @@ source-of-truth lock, not a hand-pin): 1. `ci.yml:1153` — dogfood-e2e seed (task T3). 2. `agent-image/moon.yml:45` — `build.command` (task T2a). 3. `agent-image/publish.sh:62` — publish build (task T2a). -4. `devenv.nix:537` — the `dogfood:agent-image` task, whose own comment - (`devenv.nix:519-521`) claims the pin "cannot diverge from the fork source +4. `devenv.nix:552` — the `dogfood:agent-image` task, whose own comment + (`devenv.nix:534-536`) claims the pin "cannot diverge from the fork source the agent-image module set is pinned to" — a claim this record disproves. 5. `tools/agent-image-env-gate/index.ts:103` — the env gate's `nix run … container build`, which runs in the CI moon graph diff --git a/docs/designs/infra/ci/compass-dogfood-e2e/design.md b/docs/designs/infra/ci/compass-dogfood-e2e/design.md index 9a828cc3e..06368beb2 100644 --- a/docs/designs/infra/ci/compass-dogfood-e2e/design.md +++ b/docs/designs/infra/ci/compass-dogfood-e2e/design.md @@ -191,7 +191,7 @@ A new `go/e2e` (name final at implementation) package with a fixture that: **Real agent image, not alpine.** The capstone's `Config.AgentImage` is `compass-agent:latest` built+loaded into containers-storage by the dogfood-loop -task (`devenv.nix:349-354` `dogfood:agent-image`, opt-in). Because +task (`devenv.nix:364-369` `dogfood:agent-image`, opt-in). Because `EnsureImage` unconditionally `podman pull`s — "no pre-existence check is done by deliberate choice — the pull IS the ensure" (`adapters/image.go:52-65`) — and a containers-storage-local image is not pullable, this gap has TWO halves. @@ -416,7 +416,7 @@ harness does NOT wait for T5's CLI to land (the RPC contracts are on main now; the sequencing note is OQ4-adjacent, resolved in-plan: no dependency). What leg 2 DOES depend on is a runnable `compass-agent:latest` — RIG-1359's runtime activation (artifacts merged: `packages/compass-agent/src/cli.ts`, -`agent-image/`, `devenv.nix:281` `--image compass-agent:latest`; final +`agent-image/`, `devenv.nix:296` `--image compass-agent:latest`; final activation in progress) — flagged in H2's red case, not an open fork. ## Alternatives considered @@ -424,9 +424,9 @@ activation in progress) — flagged in H2's red case, not an open fork. - **Option B — `devenv up` (RIG-1360) + shell-script orchestration (the T7 shape).** The dogfood loop's own mechanism: `processes.{compass-server, compass-runner}` + `services.postgres` with ordered start and a - GetServerInfo readiness probe (`devenv.nix:166-290`), the real + GetServerInfo readiness probe (`devenv.nix:166-305`), the real `compass-agent:latest` image via the opt-in `dogfood:agent-image` task - (`devenv.nix:349-354`). Its genuine strength: it IS leg 1's shipped + (`devenv.nix:364-369`). Its genuine strength: it IS leg 1's shipped bring-up mechanism, and the sibling record's T7 smoke already rides it. It loses to C for an AUTOMATED, scenario-bearing harness: orchestration is process-compose/shell, a scenario-authoring API with typed assertions over @@ -869,7 +869,7 @@ D2 (see §Decisions); OQ3/OQ4 remain open. 2. **OQ4 — Leg-2 activation dependency (RIG-1359).** Leg 2 needs a runnable `compass-agent:latest` doing a real (canned or live) turn. The artifacts are on main (`packages/compass-agent/src/cli.ts`, `agent-image/`, - `devenv.nix:281` runner `--image compass-agent:latest`) but RIG-1359's + `devenv.nix:296` runner `--image compass-agent:latest`) but RIG-1359's final runtime activation is In Progress. Is the capstone's H2/H3 sequenced strictly after RIG-1359 closes, or may H3's deterministic backend land as part of the activation itself (one image change instead of two)? diff --git a/docs/designs/infra/ci/compass-local-dev/design.md b/docs/designs/infra/ci/compass-local-dev/design.md index 286249ac9..12314803b 100644 --- a/docs/designs/infra/ci/compass-local-dev/design.md +++ b/docs/designs/infra/ci/compass-local-dev/design.md @@ -22,7 +22,7 @@ no route to the gRPC-Web door — `apps/ui/vite.config.ts:4-6` says so itself: "The UI consumes the generated @compass/client; the daemon transport + dev proxy arrive with the local-transport work." Second, macOS has no path at all: `services.postgres` (devenv.nix:211), `processes` (devenv.nix:218), and -`tasks` (devenv.nix:346) are all `lib.optionalAttrs pkgs.stdenv.isLinux`, and +`tasks` (devenv.nix:361) are all `lib.optionalAttrs pkgs.stdenv.isLinux`, and the native desktop shell has no darwin entrypoint (grounded in §A2c). Third, the pre-push gate (`hk.pkl:31-33`, `check = "moon ci"`) runs in jj-vine's temp worktree `/tmp/jj-hooks-worktree-*`, which has the tree but no direnv @@ -56,7 +56,7 @@ process. this consumer — `devenv.nix:219-220`: "compass-server: serves compass.v1 on a Unix domain socket (the shipped local door) plus a loopback gRPC-Web port for the browser UI dev server", with `ports.devhttp.allocate = 50051` -(devenv.nix:260). The UI dials whatever `VITE_COMPASS_BASE_URL` resolves — +(devenv.nix:275). The UI dials whatever `VITE_COMPASS_BASE_URL` resolves — `apps/ui/src/live/connection.ts:46-50`: ```ts @@ -81,7 +81,7 @@ exact consumer. **Direct-dial (decided).** No proxy at all: the compass-ui process sets `VITE_COMPASS_BASE_URL = "http://127.0.0.1:${toString config.processes.compass-server.ports.devhttp.value}"` — the same binding -the server's own `--dev-http` flag reads (devenv.nix:253) — and the browser +the server's own `--dev-http` flag reads (devenv.nix:268) — and the browser fetch streams the gRPC-Web response natively, with no middlebox to buffer it. `connection.ts` was built for exactly this client: "`token` undefined is a deliberate no-auth client (the dev door)" @@ -124,7 +124,7 @@ outside `up`) and `VITE_COMPASS_BASE_URL=http://127.0.0.1:5173` (the vite origin). **The process.** A third devenv process, launched the way the existing two -resolve proto's shims (devenv.nix:243-244 sets +resolve proto's shims (devenv.nix:258-259 sets `PROTO_HOME`/`PATH` in compass-server's exec preamble; same pattern here for `bunx`): @@ -145,7 +145,7 @@ compass-ui = { }; ``` -`after` the server's readiness probe (devenv.nix:288-294 gates on a real +`after` the server's readiness probe (devenv.nix:303-309 gates on a real `GetServerInfo` answer over the dev-http door) so the first browser load never races the migrating store. The command matches the existing moon task (`apps/ui/moon.yml:11-12`: `dev: command: 'bunx vite'`) — one convention, two @@ -184,9 +184,9 @@ Linux-bound: postgres is a stock devenv service, and the server/cert/mint lanes are pure-Go builds run through the pinned toolchain. The guard relaxes by removing `lib.optionalAttrs pkgs.stdenv.isLinux` from `services.postgres` (devenv.nix:211) and from the server process + its two tasks, leaving -Linux-only: `compass-runner` (devenv.nix:321 — native podman loop), -`dogfood:agent-image` (devenv.nix:401-406 — the vendored-fork nix container -build), and `dogfood:clean` (devenv.nix:413-426 — host rootless podman). The +Linux-only: `compass-runner` (devenv.nix:336 — native podman loop), +`dogfood:agent-image` (devenv.nix:416-421 — the vendored-fork nix container +build), and `dogfood:clean` (devenv.nix:428-441 — host rootless podman). The `PKG_CONFIG_PATH` env stays Linux-guarded exactly as it is — its comment already states the macOS posture (devenv.nix:122-123: "on macOS the app links the system WebKit framework, so the closure is Linux's alone"). @@ -205,7 +205,7 @@ process runs INSIDE the VM — per Matt's 2026-08-14 macOS ruling ("reuse the existing Linux-native loop unchanged; the VM is the new moving part") — and composes with the runner's own posture: "Runners are remote by design, so this dials -the authenticated TLS door" (devenv.nix:302-304). Shape: +the authenticated TLS door" (devenv.nix:317-319). Shape: - VM engine: `podman machine` (recommended over colima; OQ1). - `devenv up` on macOS does NOT provision or start the VM (mutating global @@ -220,7 +220,7 @@ the authenticated TLS door" (devenv.nix:302-304). Shape: launched with the enrollment token from `dogfood:mint-runner-token` and the gen-cert trust anchor. - The VM dials the host's TLS network door (`ports.network.allocate = - 50052`, devenv.nix:265). But compass-server binds that door to LOOPBACK + 50052`, devenv.nix:280). But compass-server binds that door to LOOPBACK today: `--listen "127.0.0.1:${toString config.processes.compass-server.ports.network.value}"` (the compass-server exec's `--listen` line, devenv.nix). A podman-machine @@ -237,7 +237,7 @@ the authenticated TLS door" (devenv.nix:302-304). Shape: narrowest-address constraint. Consequences T5 owns: the spike-resolved dial/bind address, and the gen-cert SAN set — "SAN defaults (127.0.0.1,::1,localhost)" - (devenv.nix:349-350) — must grow that address, so `compass-gen-cert` + (devenv.nix:364-365) — must grow that address, so `compass-gen-cert` gains a `--san` flag the macOS lane passes. The darwin `--listen` variance touches the compass-server process attr T4 unguards: T4 moves it out of the Linux guard unchanged, T5 may add darwin variance (e.g. @@ -245,7 +245,7 @@ the authenticated TLS door" (devenv.nix:302-304). Shape: guard move) — the two tasks co-edit this attr, and both their interfaces acknowledge it. - Agent-image delivery into the VM (the `compass-agent:latest` ref the - runner resolves, devenv.nix:333) is `podman machine`'s containers-storage; + runner resolves, devenv.nix:348) is `podman machine`'s containers-storage; the nix image build (`dogfood:agent-image`) stays Linux-only, and macOS pulls the GHCR-published image per DL-112 (DECISIONS.md:211) once that publish lane exists — until then macOS runs runner-loop-less by default @@ -369,7 +369,7 @@ OQ4, not a task. the devenv.nix surfaces this record edits: proto/.prototools removal, pins moving into nix derivations, the `enterShell` proto activation (devenv.nix:155-161), and the exec preambles' `PROTO_HOME` shims - (devenv.nix:243-244). This record grounds against CURRENT main (still + (devenv.nix:258-259). This record grounds against CURRENT main (still .prototools: bun 1.3.13, node 24.18.0, moon 2.4.2, go 1.26.5 — .prototools:6-13) and does NOT guess the post-cutover shape. **Implementation of T3/T4/T5 MUST be sequenced after the RIG-1983 cutover @@ -382,10 +382,10 @@ OQ4, not a task. - **Tool pins (as of this record):** bun 1.3.13 / node 24.18.0 / moon 2.4.2 / go 1.26.5 (.prototools:6-13); biome 2.5.4 (biome.json:2, bun.lock:237); vite port 5173 strictPort (apps/ui/vite.config.ts:11); dev-http door 50051, - TLS network door 50052 (devenv.nix:260,265). + TLS network door 50052 (devenv.nix:275,265). - **Never-heavy-on-up:** nothing added to `devenv up` may pull a heavy closure (the precedent: `dogfood:agent-image` is deliberately opt-in, - devenv.nix:397-400). + devenv.nix:412-415). - **Port ownership:** devenv is the single owner of allocated port numbers (`config.processes.compass-server.ports.*`); no second hardcoded copy — consumers take them via process env. @@ -426,7 +426,7 @@ carries no proxy work.) verify `curl -X POST http://127.0.0.1:50051/compass.v1.CompassService/GetServerInfo -H 'Content-Type: application/json' -d '{}'` answers (the same probe the - readiness check uses, devenv.nix:288-294), and a browser session loads + readiness check uses, devenv.nix:303-309), and a browser session loads the board and holds a live `SubscribeEvents` stream (the browser dials the door directly; no middlebox in the path). @@ -564,8 +564,8 @@ co-edits the compass-server process attr T4 unguards (darwin variance via guard move; both tasks' interfaces acknowledge the co-edit). - **Interfaces:** consumes `podman machine ssh/inspect`, the token file - `${config.devenv.state}/compass/runner.token` (devenv.nix:328,383), the - trust anchor `tls.crt` (devenv.nix:332), and the network door on port + `${config.devenv.state}/compass/runner.token` (devenv.nix:343,383), the + trust anchor `tls.crt` (devenv.nix:347), and the network door on port 50052 at the spike-resolved VM-reachable host address (the exact address and its discovery method are the opening spike's outputs, not assumed here); produces a new `compass-gen-cert` flag `--san` (string, diff --git a/docs/designs/infra/ci/compass-pr-validation/design.md b/docs/designs/infra/ci/compass-pr-validation/design.md index 2936a555d..db7fcafd8 100644 --- a/docs/designs/infra/ci/compass-pr-validation/design.md +++ b/docs/designs/infra/ci/compass-pr-validation/design.md @@ -218,7 +218,7 @@ Interfaces: Test cycle: red — no build input targets a configured TLS door today; green — `bunx vite build` with the env set yields a `dist/` whose `bootConnection` reaches WhoAmI against a TLS door (verified against the dev stack's own -network door, which devenv already binds: `devenv.nix:255-257` +network door, which devenv already binds: `devenv.nix:270-272` `--listen … --tls-cert … --tls-key`). ### B3 — scenario expansion: multi-peer fan-out, provision/enroll hardening diff --git a/docs/designs/infra/runtime/compass-runner-arbitrary-uid/design.md b/docs/designs/infra/runtime/compass-runner-arbitrary-uid/design.md index 8e5fe7b0c..502405088 100644 --- a/docs/designs/infra/runtime/compass-runner-arbitrary-uid/design.md +++ b/docs/designs/infra/runtime/compass-runner-arbitrary-uid/design.md @@ -99,7 +99,7 @@ verified this session). Grounded availability: "not executed here — it needs a rootless-podman host, which GitHub-hosted runners are not"). - GA runtime hosts: **unpinned**. Podman is the external host's, not shipped - by the repo — `devenv.nix:415-418` is explicit that even the dogfood loop + by the repo — `devenv.nix:430-433` is explicit that even the dogfood loop "Uses the host's rootless podman", and `agent-image/` bakes no podman. **Decision: hard floor ≥ 4.3, no `--uidmap`/`--gidmap` fallback — Matt ruled** @@ -107,7 +107,7 @@ verified this session). Grounded availability: the runner's operator-facing docs. Rationale: podman 4.3 shipped in 2022, so the below-floor population is small but not empty — Ubuntu 22.04 LTS (supported into 2027) ships podman 3.4.x, and GA runtime hosts are unpinned -(`devenv.nix:415-418`), so a 22.04 operator is a plausible GA host that this +(`devenv.nix:430-433`), so a 22.04 operator is a plausible GA host that this floor **will** refuse. That is exactly why the OQ-A preflight's legible startup refusal (not a deep-in-create failure) is the right handling, and a `--uidmap` fallback would double the launch-path test matrix to serve it. The error copy @@ -364,7 +364,7 @@ decisions. 2. **OQ-B — podman floor: hard ≥ 4.3, or a `--uidmap` fallback for < 4.3? → HARD FLOOR ≥ 4.3, NO FALLBACK (Matt).** `keep-id:uid=` needs podman ≥ 4.3. Dev box (5.8.4) and CI (4.9.3) clear it; GA runtime hosts are unpinned - (podman is the host's, not shipped — `devenv.nix:415-418`). **Decision: + (podman is the host's, not shipped — `devenv.nix:430-433`). **Decision: hard floor ≥ 4.3**, enforced by the OQ-A preflight and documented for operators; no `--uidmap` fallback. The known below-floor case is Ubuntu 22.04 LTS (podman 3.4.x, supported into 2027), a plausible unpinned GA host diff --git a/docs/designs/repo/compass-drop-proto.md b/docs/designs/repo/compass-drop-proto.md index 456c2195d..291a2aeee 100644 --- a/docs/designs/repo/compass-drop-proto.md +++ b/docs/designs/repo/compass-drop-proto.md @@ -219,7 +219,7 @@ Grounded in the current file, the complete delta set: `bun install --frozen-lockfile` and the `hk install` lines (`:165-171`) stay. - All four `PROTO_HOME`/`PATH` shim exports in the process/task exec blocks: - compass-server (`devenv.nix:237-244`: "Go is proto-managed (.prototools); + compass-server (`devenv.nix:237-259`: "Go is proto-managed (.prototools); make its shim resolvable even when this process is launched outside the enterShell PATH mutation"), compass-runner (`:323-324`), gen-cert (`:357-358`), mint-runner-token (`:377-378`). With go a `packages` entry it diff --git a/docs/designs/repo/compass-renovate-migration.md b/docs/designs/repo/compass-renovate-migration.md index 4bef6e443..56fad201f 100644 --- a/docs/designs/repo/compass-renovate-migration.md +++ b/docs/designs/repo/compass-renovate-migration.md @@ -114,12 +114,12 @@ with that idiom instead of inventing a second toolchain path. `devenv` is the one binary that idiom does NOT provide: compass CI never puts a `devenv` on PATH — its only devenv invocations run the vendored fork's CLI by path (`ci.yml:812` `nix run path:../forks/devenv#devenv -- container copy agent`; -`agent-image/moon.yml:44`; `devenv.nix:469`). The Renovate job therefore +`agent-image/moon.yml:44`; `devenv.nix:484`). The Renovate job therefore builds that same fork (the flake exports the CLI as `packages..devenv`, `forks/devenv/flake.nix:113-115`) and shims it onto PATH. This is FORCED by the frozen fork posture, not a fresh choice: the image pipeline pins to "the vendored fork's own CLI … so it cannot diverge -from the fork source" (`devenv.nix:450-453`; +from the fork source" (`devenv.nix:465-468`; `docs/designs/agent/compass-forks-reversal/design.md:125-134` — "The fork's own CLI is invoked by path everywhere the image is built"). A nixpkgs devenv doing the relock would be a SECOND, divergent devenv — the exact thing @@ -435,7 +435,7 @@ one devenv the fork posture allows; zero new infrastructure. Rejected: a nixpkgs-built devenv doing the relock would be a SECOND devenv, divergent from the vendored fork whose CLI compass pins by path everywhere devenv runs ("pinned to the vendored fork's own CLI … so it cannot diverge -from the fork source", `devenv.nix:450-453`; "The fork's own CLI is invoked +from the fork source", `devenv.nix:465-468`; "The fork's own CLI is invoked by path everywhere the image is built", `docs/designs/agent/compass-forks-reversal/design.md:125-134`) — exactly the divergence the frozen fork posture exists to eliminate. The vendored-fork diff --git a/docs/designs/server/compass-forge-poll-driver/design.md b/docs/designs/server/compass-forge-poll-driver/design.md index 7a858ed38..4c43097cb 100644 --- a/docs/designs/server/compass-forge-poll-driver/design.md +++ b/docs/designs/server/compass-forge-poll-driver/design.md @@ -670,12 +670,12 @@ Mirrors the two established precedents exactly: mutation RPC/admin surface is a named non-goal (Global Constraints). - **Secret resolve:** the driver's `TokenSource` closes over the one `secrets.SpecResolver` built at `serve.go:287`, calls - `Resolve(ctx, "forge poll")` (`secrets/resolver.go:135`), and selects the + `Resolve(ctx, "forge poll")` (`secrets/resolver.go:146`), and selects the configured name from the returned `[]ResolvedSecret` — the same single-resolve-surface DL-052 mandates; the row is declared `server_only` so it never reaches a container (mechanism per the ownership-layer T5, `compass-server-ownership-layer/design.md:1991-1995`). **Resolve is not - cheap** — `resolver.go:135-165` reads the entire declared-secret registry + cheap** — `resolver.go:146-172` reads the entire declared-secret registry from the store, writes a manifest temp file, and drives a full secretspec provider `Load` (potentially an external provider call) per invocation — so the `TokenSource` implementation caches the resolved value behind a short TTL @@ -1193,7 +1193,7 @@ type forgePollStore struct { ``` Consumes: `secrets.SpecResolver.Resolve(ctx, reason) ([]ResolvedSecret, error)` -(`secrets/resolver.go:135`), `secrets.ResolvedSecret{Name, Value, …}` +(`secrets/resolver.go:146`), `secrets.ResolvedSecret{Name, Value, …}` (`secrets/secrets.go:132-135`; Value redacted under all fmt verbs — safe to thread), the `board.NewIssueProjection` instance from `serve.go:259`, `ingest.NewIngester` (`ingest.go:44`), `forge.NewGitHub` (T1), the T2 store 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 e9b310683..77f4beda6 100644 --- a/docs/designs/server/compass-gateway-credentials-at-rest-encryption.md +++ b/docs/designs/server/compass-gateway-credentials-at-rest-encryption.md @@ -628,14 +628,15 @@ declared into a store that does not exist. when this record was written (go/go.mod:22 now reads v0.20.0, bumped by RIG-3320), and at that pin `age://` did not resolve and T2's master-key write-back has no writable target. This gates the self-hosted `age://` - DEFAULT specifically, NOT T0/T2 wholesale: on the current pin the matrix - already marks Write yes for `keyring`, `dotenv`, `pass`, `gopass` + DEFAULT specifically, NOT T0/T2 wholesale: even at v0.15 the matrix + already marked Write yes for `keyring`, `dotenv`, `pass`, `gopass` (0.15+), `awssm`, `akv` and `vault`, so a deployment pointing the - SERVER resolver at a cloud store or HashiCorp Vault needs NO bump (OpenBao, - like `age`, is 0.17+); what - v0.15 lacks is `age` (0.17+), with `pass`/`gopass` the self-hosted - encrypted-at-rest alternatives available today (`age://` is the ruled - default — a single local identity file rather than a GPG keyring). The + SERVER resolver at a cloud store or HashiCorp Vault needed NO bump + (OpenBao, like `age`, is 0.17+); what v0.15 lacked is `age` (0.17+), with + `pass`/`gopass` the self-hosted encrypted-at-rest alternatives available + at that pin (`age://` is the ruled default — a single local identity file + rather than a GPG keyring). RIG-3320 has since taken the pin to v0.20.0, + so `age://` and `openbao://` both resolve and this prerequisite is MET. The bump covers TWO separate closures, since the SDK read path and the CLI write path are distinct binaries: (a) the Go module `secretspec-go` `>= 0.17` for the READ path (`b.Load()`, resolver.go:172); AND (b) the @@ -923,7 +924,9 @@ is declared into it) and T1. master key's provider value unreachable from the user path. The user `SetSecret`/`DeleteSecret` RPC (`authenticatedOpen`, any authenticated account — admin_gate.go:122-125) declares into `secrets` but then calls - `resolver.Set`, which shells `secretspec set --profile default` + `resolver.Set`, which shells + `secretspec --file= --reason= set --provider=

+ --profile=

` (resolver.go:325-331) against the keyspace that is SHARED under the default single-URI wiring (F2) — so absent a guard a user calling `SetSecret` with name `GATEWAY_CREDENTIALS_MASTER_KEY` would @@ -1151,12 +1154,13 @@ RPC exactly as the frozen record already specifies. `r.cli`, resolver.go:273/30), the latter pinned via `WithCLI` (resolver.go:92) into the Server's closure so read and write cannot drift to different provider capabilities. `age` is a 0.17+ build-feature - provider, so on the current pin the `age://` default does not resolve - and T2's master-key write-back has no writable target; a deployment on a - cloud store (`awssm`/`akv`) or HashiCorp Vault — all Write-capable at - the current pin — needs no bump (OpenBao does NOT qualify: `openbao://` is a - 0.17+ provider, with 0.16 routing it through `vault`, so an OpenBao - deployment rides the same bump as `age://`). GATING for T2 ONLY on the `age://` + provider, so at the v0.15 pin this record was written against the + `age://` default did not resolve and T2's master-key write-back had no + writable target; a deployment on a cloud store (`awssm`/`akv`) or + HashiCorp Vault — all Write-capable even at v0.15 — needed no bump + (OpenBao did NOT qualify: `openbao://` is a 0.17+ provider, with 0.16 + routing it through `vault`, so an OpenBao deployment rode the same bump + as `age://`). RIG-3320 landed that bump at v0.20.0, so both resolve now. GATING for T2 ONLY on the `age://` path. Proto delta: two additive `SecretsService` methods, no enum change — the checked-in public gen trees are drift-gated, so this needs the diff --git a/docs/designs/server/compass-linear-agent-responder/design.md b/docs/designs/server/compass-linear-agent-responder/design.md index 6aa4e31ec..da0d1d61b 100644 --- a/docs/designs/server/compass-linear-agent-responder/design.md +++ b/docs/designs/server/compass-linear-agent-responder/design.md @@ -399,9 +399,9 @@ boundary: the names are declared in the store registry and the values live in the human-controlled provider vault, resolved server-side through `secrets.Resolver` ("Resolve reads the whole names registry, generates the manifest, resolves values from the configured provider", -`go/internal/secrets/resolver.go:33-36`; "The resolver process (the Server) is +`go/internal/secrets/resolver.go:40-43`; "The resolver process (the Server) is the only place SecretSpec runs — containers receive resolved values, never -provider access", `resolver.go:53-55`). Matt sets the values once via +provider access", `resolver.go:63-65`). Matt sets the values once via `SetSecret` — user-only by design ("SetSecret … USER-ONLY (record §911-927): an agent-token caller is CodePermissionDenied", `go/server/secrets_service.go:75-77`) — as `SecretKindGeneric` declarations @@ -418,10 +418,10 @@ agent container. **Degraded mode: fail-open to stale, fail-closed only on never-resolved.** `Resolve` is inject-ALL: it "reads the whole registry (inject-all: no per-agent filter in the MVP — a names filter is the future grants seam)" -(`go/internal/secrets/resolver.go:33-36`), every declared name is +(`go/internal/secrets/resolver.go:40-43`), every declared name is `required = true` ("a missing one is a MissingRequiredError at resolve, -surfaced loudly", `resolver.go:123-125`), and one unresolvable name fails -the whole `Load` (`resolver.go:165-170`). So a naive TTL-expiry re-resolve +surfaced loudly", `resolver.go:134-136`), and one unresolvable name fails +the whole `Load` (`resolver.go:172-177`). So a naive TTL-expiry re-resolve would fail the `/webhooks` path the moment Matt declares ANY unrelated secret before providing its value — a 503 streak, which is exactly the failure mode that gets the Linear category auto-disabled (the incident @@ -432,7 +432,7 @@ still lands on the next successful resolve). It fails closed (503, never accept an unverifiable webhook) only when a secret has NEVER successfully resolved. The proper long-term fix is the narrow per-name resolve the resolver doc itself anticipates ("a names filter is the future grants -seam", `resolver.go:35-36`); the stale cache is the scoped decoupling. +seam", `resolver.go:42-43`); the stale cache is the scoped decoupling. ### Part 6 — the public URL is per-deployment (Matt ruled OQ-2) @@ -550,7 +550,7 @@ the Linear loop needs and a bespoke injection would have to rebuild. defaults to `https://compass.rigel.build`, self-host sets its own, dev/tailnet deploys need their own ingress for Linear reachability (Part 6). - Secrets follow the declared-by-name / provided-by-value boundary - (`go/internal/secrets/resolver.go:33-55`); the three Linear secrets are + (`go/internal/secrets/resolver.go:40-65`); the three Linear secrets are server-resolved, never container-delivered. Resolve failures serve the last-known-good cached value (fail-open to stale; fail closed only when never resolved) so an unrelated unprovided declaration can never 503 the @@ -611,7 +611,7 @@ step explicitly. Interfaces: consumes `SecretsService.SetSecret` (`go/server/secrets_service.go:92`); produces three resolvable declarations the responder reads via `secrets.Resolver.Resolve(ctx, reason) ([]ResolvedSecret, error)` -(`go/internal/secrets/resolver.go:33-49`). +(`go/internal/secrets/resolver.go:40-59`). ### T1 — linearagent: webhook envelope types + signature verification @@ -803,7 +803,7 @@ whether stale-drop / duplicate-ack-thought ever trigger, Part 1); (b) confirm the per-deployment webhook URL + deep link resolve correctly for the managed deploy. -Interfaces: consumes T1/T4/T5/T6, `secrets.Resolver` (`resolver.go:37-49`), +Interfaces: consumes T1/T4/T5/T6, `secrets.Resolver` (`resolver.go:44-59`), `netMux` (`network_door.go:270`); produces the live `POST /webhooks` endpoint at `/webhooks`. diff --git a/docs/designs/server/compass-server-ownership-layer/design.md b/docs/designs/server/compass-server-ownership-layer/design.md index a92ef32d3..70c168119 100644 --- a/docs/designs/server/compass-server-ownership-layer/design.md +++ b/docs/designs/server/compass-server-ownership-layer/design.md @@ -829,9 +829,9 @@ registry** and resolved through the existing resolver: encryption-at-rest is the provider's job" (`0002_secrets.sql:10-14`). The Server resolves it on demand through `Resolver.Resolve(ctx, reason)` - (`go/internal/secrets/resolver.go:37-49`), which reads the registry and pulls + (`go/internal/secrets/resolver.go:44-59`), which reads the registry and pulls values from the configured SecretSpec provider - (`resolver.go:135-139`). Values "live only in the provider and this process's + (`resolver.go:146-150`). Values "live only in the provider and this process's memory during a resolve; they are never persisted by Compass and never logged" (`go/internal/secrets/secrets.go:20-22`), and every value-bearing type redacts under `%s`/`%v`/`%#v` (`secrets.go:146-156`). @@ -861,9 +861,9 @@ it quotes name `FetchSecrets` as the seam; grepping `compass` this run returns exactly one hit — the comment itself, `0002_secrets.sql:14`. There is no such function in the Go tree. The actual declarations seam is `declarations interface { DeclaredSecrets(ctx) }` -(`go/internal/secrets/resolver.go:29-31`), consumed by `SpecResolver.Resolve` -(`resolver.go:135-139`), whose own doc says it is "inject-all: the whole store, -no per-agent filter (the future grants seam)" (`resolver.go:130-134`). +(`go/internal/secrets/resolver.go:36-38`), consumed by `SpecResolver.Resolve` +(`resolver.go:146-150`), whose own doc says it is "inject-all: the whole store, +no per-agent filter (the future grants seam)" (`resolver.go:141-145`). The complication that makes this a design question rather than a rename: **that one `Resolve` serves BOTH consumers.** This record needs the Server's own @@ -887,7 +887,7 @@ makes `gh issue create` a one-liner with no header and no Server involvement. **Those two comments describe intent, not shipped code.** Verified this run: `NewSpecResolver` has zero non-test callers in the Go tree (only its definition -at `go/internal/secrets/resolver.go:86`); `ResolvedSecret` appears in zero +at `go/internal/secrets/resolver.go:97`); `ResolvedSecret` appears in zero files outside `go/internal/secrets/`; and the only credential that reaches a container is `Workspace.Credentials` via `CredentialSetupScript`, which writes `$HOME/.git-credentials` and nothing else — no `hosts.yml` anywhere in @@ -1977,9 +1977,9 @@ the filtered set. **Identify and switch the real caller, or this task ships a filter nothing calls.** The declarations seam is `declarations interface { DeclaredSecrets(ctx) }` -(`go/internal/secrets/resolver.go:29-31`), consumed by `SpecResolver.Resolve` -(`resolver.go:135-139`) — documented "inject-all: the whole store, no per-agent -filter (the future grants seam)" (`resolver.go:130-134`). That one `Resolve` +(`go/internal/secrets/resolver.go:36-38`), consumed by `SpecResolver.Resolve` +(`resolver.go:146-150`) — documented "inject-all: the whole store, no per-agent +filter (the future grants seam)" (`resolver.go:141-145`). That one `Resolve` serves **both** the Server's own resolve and container materialization, so adding `ContainerSecrets` is only half the change: the container-materialization caller must be switched to it while the Server's resolve keeps the unfiltered @@ -2639,9 +2639,9 @@ is a config change to something already built. **(i) Which concrete function is the filter point?** The real declarations seam is `declarations interface { DeclaredSecrets(ctx) }` -(`go/internal/secrets/resolver.go:29-31`), consumed by `SpecResolver.Resolve` -(`resolver.go:135-139`), documented "inject-all: the whole store, no per-agent -filter (the future grants seam)" (`resolver.go:130-134`). **That one `Resolve` +(`go/internal/secrets/resolver.go:36-38`), consumed by `SpecResolver.Resolve` +(`resolver.go:146-150`), documented "inject-all: the whole store, no per-agent +filter (the future grants seam)" (`resolver.go:141-145`). **That one `Resolve` serves both** the Server's own resolve and container materialization — this record needs them to diverge. So T5 must name the container-materialization caller and switch it to `ContainerSecrets` while the Server's resolve keeps the @@ -2660,7 +2660,7 @@ live behaviour, a token there makes `gh issue create` a one-liner with no header and no Server involvement. **It is not live behaviour.** Verified: `NewSpecResolver` has zero non-test -callers (definition only, `resolver.go:86`); `ResolvedSecret` appears in zero +callers (definition only, `resolver.go:97`); `ResolvedSecret` appears in zero files outside `go/internal/secrets/`; and the only credential reaching a container is `Workspace.CredentialSetupScript`, which writes `$HOME/.git-credentials` and never `hosts.yml`. **There is no materializer, so diff --git a/docs/designs/ui/compass-gtk4-migration/design.md b/docs/designs/ui/compass-gtk4-migration/design.md index 17f9374f1..102c97056 100644 --- a/docs/designs/ui/compass-gtk4-migration/design.md +++ b/docs/designs/ui/compass-gtk4-migration/design.md @@ -48,7 +48,7 @@ for why it is this small: `webkitgtk_6_0 = 2.52.5` (same WebKit release as the current `webkitgtk_4_1 = 2.52.5`). No nixpkgs bump is required to get the packages. 4. **The closure has ONE definition.** `gtk-closure.nix:3-10`: "ONE definition, - imported by two consumers so they cannot drift" (devenv.nix:251, + imported by two consumers so they cannot drift" (devenv.nix:266, gtk-e2e-env.nix:38) — plus the flake (`flake.nix:101`). Swapping the list swaps every consumer at once; the packaging record predicted exactly this: "a GTK4 flip is a closure-list + tag edit, not a packaging redesign" @@ -252,7 +252,7 @@ after T1 lands. (gtk-e2e-env.nix) and the built app-bundle tarball sizes; record both deltas in the PR body per Global Constraint 5. + **Interfaces:** consumes `gtk-closure.nix:18-32` (the name list); produces - the new list consumed unchanged by `devenv.nix:251` (PKG_CONFIG_PATH), + the new list consumed unchanged by `devenv.nix:266` (PKG_CONFIG_PATH), `gtk-e2e-env.nix:38` (pcClosure), `flake.nix:101` (buildInputs). No nixpkgs pin change (Global Constraint 2: both attrs exist at `c946ff36bf19`). diff --git a/docs/designs/ui/compass-native-app/design.md b/docs/designs/ui/compass-native-app/design.md index 751bb7448..705b6311a 100644 --- a/docs/designs/ui/compass-native-app/design.md +++ b/docs/designs/ui/compass-native-app/design.md @@ -106,7 +106,7 @@ Source facts the design composes with (`RigelBuild/compass` @ main 2624bcb5): cert/key, `devenv.nix:199-205`) → `dogfood:mint-runner-token` (`cmd/compass-mint-runner-token`, writes the enrollment token 0600) → `compass-runner` (dials `https://127.0.0.1:` with `--ca` = the generated - cert, `devenv.nix:277-282`). + cert, `devenv.nix:292-297`). - **The UI resolves its connection once at boot, at one seam.** `apps/ui/src/live/connection.ts:58-79` (`resolveConnection`) requires `VITE_COMPASS_BASE_URL` + `VITE_COMPASS_CALLER_ID` (+ optional token); @@ -225,7 +225,7 @@ env only. The runner spawn MUST carry `--image` (it refuses to boot without one, A0; pulled from GHCR per DL-112) and `--runtime-dir` under `$XDG_RUNTIME_DIR` — the `/run/compass` default is root-owned and a deep state-dir path overflows the 107-byte AF_UNIX `sun_path` cap the runner's per-container sockets live under -(`devenv.nix:255-259`). Spawn-if-absent + attach: a live `GetServerInfo` on the +(`devenv.nix:270-274`). Spawn-if-absent + attach: a live `GetServerInfo` on the socket means attach, never double-spawn; an O_EXCL state-dir lockfile guards the probe→spawn window so two concurrent `up`s (app launch racing a manual CLI `up`) cannot both spawn. @@ -457,7 +457,7 @@ shell (DL-110). https://127.0.0.1: --ca --image --runtime-dir ` with `COMPASS_RUNNER_TOKEN` in env only (`RuntimeDir` under `$XDG_RUNTIME_DIR`, validate its length against the 107-byte `sun_path` cap, - `devenv.nix:255-259`). + `devenv.nix:270-274`). Attach-if-live under an O_EXCL state-dir lockfile: a `GetServerInfo` answering on the socket short-circuits to attach, and the lockfile closes the probe→spawn TOCTOU so two concurrent `up`s cannot both spawn; on attach, diff --git a/docs/designs/ui/compass-native-client-mode/design.md b/docs/designs/ui/compass-native-client-mode/design.md index 560ff6c61..b4b649338 100644 --- a/docs/designs/ui/compass-native-client-mode/design.md +++ b/docs/designs/ui/compass-native-client-mode/design.md @@ -638,9 +638,9 @@ design red-team. Kept here as the decision record. `WithProvider/WithProfile` → `Load()/Report()`), with no `Set`/`Write` primitive — writing a value into a provider is a CLI action, not an SDK one. This is grounded in-repo by the seal-side wrapper of that same SDK: - `go/internal/secrets/resolver.go:80-81` documents `WithCLI` as pinning "the + `go/internal/secrets/resolver.go:91-92` documents `WithCLI` as pinning "the secretspec CLI binary used for the write path", and `resolver.go`'s resolve - path uses only `b.WithProvider`/`b.WithProfile`/`b.Load()` (`resolver.go:162-164`), + path uses only `b.WithProvider`/`b.WithProfile`/`b.Load()` (`resolver.go:169-171`), never a `Set`. And it is a SERVER-side manifest-driven resolve-a-declared-set surface (DL-026, `internal/secrets`), the wrong layer for a client-side single-token write. So T5 takes a direct keyring dep: From c46e9f76e1362f777b61005ddb47d54f655ef19f Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 6 Sep 2026 17:53:15 -0400 Subject: [PATCH 8/9] =?UTF-8?q?docs(secrets):=20finish=20the=20citation=20?= =?UTF-8?q?sweep=20=E2=80=94=20Go=20comments=20and=20joined=20refs=20(RIG-?= =?UTF-8?q?3320)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-6 review. Two gaps in the previous sweep, both places its own stated rule was not actually applied. The commit message said the audit unit is the shifted FILE rather than the edited document, then the sweep narrowed to markdown under docs/ — so eight endpoints in Go source comments were never visited, including the same b.Load() failure mode the last two rounds each called the worst case: serve.go cited resolver.go:135-165 for the Resolve->b.Load() span, which at head is a temp-file-cleanup comment and a defer os.Remove. A citation lives wherever someone wrote it, and .go comments are a place people write them. Fixed in serve.go and internal/stack/{spec,config,deps}.go; all four files are untouched by this PR, so those citations were correct at the merge base and are regressions this stack introduced. The rewriter also only re-pointed the FIRST numeric component of a comma-joined citation, leaving four trailing components at base values — e.g. devenv.nix:328,383, where the 383 half is the --listen line the sentence is actually about. The text-identity check missed it by comparing per-citation rather than per-endpoint, which is the same shape of error: verifying the unit you happened to iterate over instead of the unit that has to be right. Both checks are now per-ENDPOINT and prefix-aware. Prefix-awareness matters because a bare basename match would conflate root devenv.nix with agent-image/devenv.nix, forks/*/default.nix and guest-image/default.nix, which this PR does not touch — I verified the previous round shifted none of those. All 15 endpoints verified by text identity: the line each citation named at the merge base and the line it names at head are byte-identical. Refs RIG-3320 Co-authored-by: Matt Wilkinson --- docs/designs/agent/compass-forks-reversal/design.md | 2 +- docs/designs/infra/ci/compass-local-dev/design.md | 4 ++-- docs/designs/ui/compass-gtk4-migration/design.md | 2 +- go/internal/stack/config.go | 2 +- go/internal/stack/deps.go | 2 +- go/internal/stack/spec.go | 4 ++-- go/server/serve.go | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/docs/designs/agent/compass-forks-reversal/design.md b/docs/designs/agent/compass-forks-reversal/design.md index ae6c06c4f..419578b6d 100644 --- a/docs/designs/agent/compass-forks-reversal/design.md +++ b/docs/designs/agent/compass-forks-reversal/design.md @@ -419,7 +419,7 @@ Interfaces: - Comment sweep (fork-path references in prose): `agent-image/devenv.nix:7`, `agent-image/toolchain.nix:96`, `agent-image/moon.yml:5,38` (the `nix run path:../forks/devenv#devenv` example and the `path:../forks/*` - cwd-rationale comment, adjacent to the repointed CLI), `devenv.nix:110,443`, + cwd-rationale comment, adjacent to the repointed CLI), `devenv.nix:110,458`, `tools/agent-image-env-gate/env-check.ts:8`, `index.ts:15`, and `moon.yml:10,41` (all cite `forks/devenv/...` or `path:../forks/*`), `apps/ui/.env.development:30-32` (cites diff --git a/docs/designs/infra/ci/compass-local-dev/design.md b/docs/designs/infra/ci/compass-local-dev/design.md index 12314803b..8b23a3dfe 100644 --- a/docs/designs/infra/ci/compass-local-dev/design.md +++ b/docs/designs/infra/ci/compass-local-dev/design.md @@ -382,7 +382,7 @@ OQ4, not a task. - **Tool pins (as of this record):** bun 1.3.13 / node 24.18.0 / moon 2.4.2 / go 1.26.5 (.prototools:6-13); biome 2.5.4 (biome.json:2, bun.lock:237); vite port 5173 strictPort (apps/ui/vite.config.ts:11); dev-http door 50051, - TLS network door 50052 (devenv.nix:275,265). + TLS network door 50052 (devenv.nix:275,280). - **Never-heavy-on-up:** nothing added to `devenv up` may pull a heavy closure (the precedent: `dogfood:agent-image` is deliberately opt-in, devenv.nix:412-415). @@ -564,7 +564,7 @@ co-edits the compass-server process attr T4 unguards (darwin variance via guard move; both tasks' interfaces acknowledge the co-edit). - **Interfaces:** consumes `podman machine ssh/inspect`, the token file - `${config.devenv.state}/compass/runner.token` (devenv.nix:343,383), the + `${config.devenv.state}/compass/runner.token` (devenv.nix:343,398), the trust anchor `tls.crt` (devenv.nix:347), and the network door on port 50052 at the spike-resolved VM-reachable host address (the exact address and its discovery method are the opening spike's outputs, not assumed diff --git a/docs/designs/ui/compass-gtk4-migration/design.md b/docs/designs/ui/compass-gtk4-migration/design.md index 102c97056..14339f423 100644 --- a/docs/designs/ui/compass-gtk4-migration/design.md +++ b/docs/designs/ui/compass-gtk4-migration/design.md @@ -346,7 +346,7 @@ after T1 lands. ### T6 — Docs sweep + ledger encode -+ **Do:** sweep remaining `gtk3` prose: `devenv.nix:106,153-160,225-251` ++ **Do:** sweep remaining `gtk3` prose: `devenv.nix:106,153-160,225-266` comments, `gtk-e2e-env.nix:1-27` comments, native-app record's system-libs constraint (`compass-native-app/design.md:384-391` names `webkit2gtk-4.1` — annotate, don't rewrite frozen prose, per the repo's diff --git a/go/internal/stack/config.go b/go/internal/stack/config.go index 4a761ffc0..e6235c98e 100644 --- a/go/internal/stack/config.go +++ b/go/internal/stack/config.go @@ -120,7 +120,7 @@ const sunPathMax = len(syscall.RawSockaddrUnix{}.Path) - 1 // agentSocketTailWidth is the fixed suffix the runner appends to RuntimeDir to // form the widest per-container agent socket path: // /containers/compass-agent-<32-hex account id>/agent.sock. It is 69 bytes -// (devenv.nix:255-263), so on Linux (sunPathMax 107) a RuntimeDir over 38 bytes +// (devenv.nix:270-278), so on Linux (sunPathMax 107) a RuntimeDir over 38 bytes // overflows the cap. Built with the same filepath.Join the runner uses rather // than hand-summed, so it tracks the real path construction. var agentSocketTailWidth = len(filepath.Join( diff --git a/go/internal/stack/deps.go b/go/internal/stack/deps.go index 224b87fed..c2e0efa9c 100644 --- a/go/internal/stack/deps.go +++ b/go/internal/stack/deps.go @@ -27,7 +27,7 @@ type Deps struct { // attach version check. Prober HealthProber // DBProber probes Postgres reachability between starting postgres and - // compass-server, so the store opens on the first try (devenv.nix:224-242). + // compass-server, so the store opens on the first try (devenv.nix:224-257). DBProber DBProber // GroupSignaller signals and liveness-checks a persisted child process // group by pgid for the cross-process teardown (DownDetached). It is the diff --git a/go/internal/stack/spec.go b/go/internal/stack/spec.go index 5082a4176..e9e06ec23 100644 --- a/go/internal/stack/spec.go +++ b/go/internal/stack/spec.go @@ -16,7 +16,7 @@ const tokenEnvVar = "COMPASS_RUNNER_TOKEN" // mode is single-user/single-runner by design (DL-106), so the id is an internal // constant rather than a Config knob; it is cross-checked against the minted // token's subject, so mint and spawn must agree on this one value (mirrors -// devenv's fixed `--runner-id dogfood`, devenv.nix:278). +// devenv's fixed `--runner-id dogfood`, devenv.nix:293). const embeddedRunnerID = "embedded" // serverSpec builds the compass-server child spec from the resolved config and @@ -35,7 +35,7 @@ func serverSpec(cfg Config, cert CertResult) ProcessSpec { } } -// runnerSpec builds the compass-runner child spec (devenv.nix:277-282): it dials +// runnerSpec builds the compass-runner child spec (devenv.nix:292-297): it dials // the server's TLS door over https, trusts the same cert as its --ca anchor, // runs cfg.AgentImage, and mints per-container sockets under cfg.RuntimeDir. The // token rides in Env only. diff --git a/go/server/serve.go b/go/server/serve.go index ab90837d8..206832f7b 100644 --- a/go/server/serve.go +++ b/go/server/serve.go @@ -209,7 +209,7 @@ const ( // resolved webhook signing secret for: /webhooks/{github,linear} resolve the // secret on every request before the HMAC check, and a resolve reads the whole // declared-secret registry, writes a manifest temp file, and drives a full - // secretspec provider Load (resolver.go:135-165), so an uncached resolve would + // secretspec provider Load (resolver.go:146-172), so an uncached resolve would // let a garbage POST force that whole Load ahead of authentication. The cache // bounds the per-request cost to a memcmp; a rotated secret still takes effect // within the TTL. From 57d6c71f152476c2631af2ae23154a0e2a03aae4 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 6 Sep 2026 22:06:03 -0400 Subject: [PATCH 9/9] docs(secrets): re-point citations that were stale before the shift, not just by it (RIG-3320) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 8 verified its rewrites by text identity: the line each citation named at the merge base and the line it names at head are byte-identical. That claim was true, and that is exactly the gap. Text identity proves a rewrite tracked the line shift; it says nothing about whether the citation was correct BEFORE the shift. Seven citations were already stale on main, so tracking the shift faithfully preserved the staleness — and rule://planning-evidence asks for the semantic property, that the cited line names the construct the prose claims. Re-pointed against the construct each one claims, verified by asserting the cited line contains the named flag/attr rather than by comparing base to head: - go/internal/stack/spec.go:19 --runner-id: 293 (a GTK closure comment) -> 498 for the spawn, :547 for the mint side, the two that must agree. - go/internal/stack/spec.go:23 serverSpec: 199-205 (skopeo prose) -> 395-400, the --socket/--database/--listen/--tls-cert/--tls-key block it mirrors. This one round 8 did not touch and no round had flagged. - go/internal/stack/spec.go:39 runnerSpec: 292-297 (pcClosure let-binding) -> 497-502, the exec whose flags the comment enumerates. - go/internal/stack/config.go:123 the 69-byte agent-socket suffix: 270-278 (pkg-config `.pc` walk prose) -> 479-480, the sentence that states the path and the figure. Load-bearing: it grounds a 107-byte sun_path constant. - go/internal/stack/deps.go:30 postgres ordering: 224-257 (cloud-hypervisor and the new secretspec block) -> :362 services.postgres and :386 the compass-server process, the two ends of the ordering. - compass-local-dev/design.md:385 port doors: 275,280 (a `.pc` subdir comment and a bare `#`) -> 403,408, the ports.devhttp/ports.network allocations. - compass-local-dev/design.md:567 runner.token and tls.crt: 343,398 and 347 (RunSessions prose, container teardown prose) -> :548 mints the token and :496 reads it, :529 mints the cert and :500 trusts it. Verified: all 16 endpoints assert-checked against the flag or attribute the prose names, 0 mismatches. gofmt clean, go vet and go build pass, internal/stack tests pass, markdownlint 199 files 0 errors. Refs RIG-3320 Co-authored-by: Matt Wilkinson --- docs/designs/infra/ci/compass-local-dev/design.md | 7 ++++--- go/internal/stack/config.go | 2 +- go/internal/stack/deps.go | 3 ++- go/internal/stack/spec.go | 7 ++++--- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/docs/designs/infra/ci/compass-local-dev/design.md b/docs/designs/infra/ci/compass-local-dev/design.md index 8b23a3dfe..037855745 100644 --- a/docs/designs/infra/ci/compass-local-dev/design.md +++ b/docs/designs/infra/ci/compass-local-dev/design.md @@ -382,7 +382,7 @@ OQ4, not a task. - **Tool pins (as of this record):** bun 1.3.13 / node 24.18.0 / moon 2.4.2 / go 1.26.5 (.prototools:6-13); biome 2.5.4 (biome.json:2, bun.lock:237); vite port 5173 strictPort (apps/ui/vite.config.ts:11); dev-http door 50051, - TLS network door 50052 (devenv.nix:275,280). + TLS network door 50052 (devenv.nix:403,408). - **Never-heavy-on-up:** nothing added to `devenv up` may pull a heavy closure (the precedent: `dogfood:agent-image` is deliberately opt-in, devenv.nix:412-415). @@ -564,8 +564,9 @@ co-edits the compass-server process attr T4 unguards (darwin variance via guard move; both tasks' interfaces acknowledge the co-edit). - **Interfaces:** consumes `podman machine ssh/inspect`, the token file - `${config.devenv.state}/compass/runner.token` (devenv.nix:343,398), the - trust anchor `tls.crt` (devenv.nix:347), and the network door on port + `${config.devenv.state}/compass/runner.token` (devenv.nix:548 mints it, + :496 reads it), the trust anchor `tls.crt` (devenv.nix:529 mints it, :500 + trusts it), and the network door on port 50052 at the spike-resolved VM-reachable host address (the exact address and its discovery method are the opening spike's outputs, not assumed here); produces a new `compass-gen-cert` flag `--san` (string, diff --git a/go/internal/stack/config.go b/go/internal/stack/config.go index e6235c98e..d771d8a4c 100644 --- a/go/internal/stack/config.go +++ b/go/internal/stack/config.go @@ -120,7 +120,7 @@ const sunPathMax = len(syscall.RawSockaddrUnix{}.Path) - 1 // agentSocketTailWidth is the fixed suffix the runner appends to RuntimeDir to // form the widest per-container agent socket path: // /containers/compass-agent-<32-hex account id>/agent.sock. It is 69 bytes -// (devenv.nix:270-278), so on Linux (sunPathMax 107) a RuntimeDir over 38 bytes +// (devenv.nix:479-480), so on Linux (sunPathMax 107) a RuntimeDir over 38 bytes // overflows the cap. Built with the same filepath.Join the runner uses rather // than hand-summed, so it tracks the real path construction. var agentSocketTailWidth = len(filepath.Join( diff --git a/go/internal/stack/deps.go b/go/internal/stack/deps.go index c2e0efa9c..577721d56 100644 --- a/go/internal/stack/deps.go +++ b/go/internal/stack/deps.go @@ -27,7 +27,8 @@ type Deps struct { // attach version check. Prober HealthProber // DBProber probes Postgres reachability between starting postgres and - // compass-server, so the store opens on the first try (devenv.nix:224-257). + // compass-server, so the store opens on the first try (devenv.nix:362 defines + // services.postgres, :386 the compass-server process). DBProber DBProber // GroupSignaller signals and liveness-checks a persisted child process // group by pgid for the cross-process teardown (DownDetached). It is the diff --git a/go/internal/stack/spec.go b/go/internal/stack/spec.go index e9e06ec23..2e562509a 100644 --- a/go/internal/stack/spec.go +++ b/go/internal/stack/spec.go @@ -16,11 +16,12 @@ const tokenEnvVar = "COMPASS_RUNNER_TOKEN" // mode is single-user/single-runner by design (DL-106), so the id is an internal // constant rather than a Config knob; it is cross-checked against the minted // token's subject, so mint and spawn must agree on this one value (mirrors -// devenv's fixed `--runner-id dogfood`, devenv.nix:293). +// devenv's fixed `--runner-id dogfood`, devenv.nix:498 for the spawn and :547 +// for the mint side). const embeddedRunnerID = "embedded" // serverSpec builds the compass-server child spec from the resolved config and -// cert paths, mirroring the devenv dogfood invocation (devenv.nix:199-205): +// cert paths, mirroring the devenv dogfood invocation (devenv.nix:395-400): // --socket / --database / --listen / --tls-cert / --tls-key. func serverSpec(cfg Config, cert CertResult) ProcessSpec { return ProcessSpec{ @@ -35,7 +36,7 @@ func serverSpec(cfg Config, cert CertResult) ProcessSpec { } } -// runnerSpec builds the compass-runner child spec (devenv.nix:292-297): it dials +// runnerSpec builds the compass-runner child spec (devenv.nix:497-502): it dials // the server's TLS door over https, trusts the same cert as its --ca anchor, // runs cfg.AgentImage, and mints per-container sockets under cfg.RuntimeDir. The // token rides in Env only.