From 84cb156295b70d205257beb5be7ba4c417bc1b2d Mon Sep 17 00:00:00 2001 From: mintaka Date: Sat, 12 Sep 2026 15:39:21 -0400 Subject: [PATCH 1/2] docs(design): rule the user-secret scope selector and correct D8 (RIG-3655) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit D9 closes the record's wire-surface open question: SetSecretRequest and DeleteSecretRequest gain a SecretScope selector defaulting to USER scope, so a credential a user sets is not resolved by another user's agents. Tenant scope stays admin-gated per D8's matrix, checked at the RPC edge where requireUser already holds the caller's role. Also corrects a factual error in D8: it claimed the two tenant-coordinate writes were already admin-gated at the door. They are not — classifyProcedure returns authenticatedOpen for both verbs and requireUser never reads UserRole. Left uncorrected, D8 plus the tenant-pinned verb would have made SetSecret reachable only by the bootstrap admin, with no role-promotion path to grant it back. Refs RIG-3655 Co-authored-by: Matt Wilkinson --- docs/designs/DECISIONS.md | 1 + .../server/compass-user-secret-store.md | 116 +++++++++++++----- 2 files changed, 89 insertions(+), 28 deletions(-) diff --git a/docs/designs/DECISIONS.md b/docs/designs/DECISIONS.md index da2a1b25..634c623a 100644 --- a/docs/designs/DECISIONS.md +++ b/docs/designs/DECISIONS.md @@ -134,6 +134,7 @@ check enforces the mechanical half. Full rationale: | DL-362 | The canonical user-secret AAD is the five-field tuple `"compass/user-secret/v1\x00" + tenantID + "\x00" + decimal(scopeKind) + "\x00" + scopeID + "\x00" + name + "\x00" + decimal(keyVersion)` (Go: `UserSecretAAD(tenantID string, scopeKind int16, scopeID, name string, keyVersion int16) []byte`; SMALLINTs rendered `strconv.FormatInt(int64(v), 10)`), every field bound unconditionally (a tenant row binds scopeID as the empty string) with `\x00` separators keeping the encoding injective. Fixed BEFORE any migration ships because the AAD is baked into every ciphertext — a scope field added later would force a re-encrypt of every row. Refines DL-351's four-field AAD clause; DL-351's other rulings stand | Active (Matt, 2026-09-11) | [user-secret store §A9](server/compass-user-secret-store.md#a9--scope-model-tenant--user--agent-most-specific-wins) | | DL-363 | Writing a tenant-scoped user-secret row requires an admin (`store.UserRoleAdmin`, `go/internal/store/types.go`), reusing the existing role elevation rather than introducing a permission concept: tenant (0) admin-only, user (1) and agent (2) writable by the owning user or an admin. The check lands in the store door inside the same writing transaction as DL-360's FK-substitute referential checks, so one place enforces both. READS are deliberately asymmetric — a plain user's agent resolves tenant rows, which is the point of a shared tenant value under DL-361; reading a shared secret is the feature, writing one is the privileged act. The wire surface for a scoped write stays undecided (a scope selector on `SetSecretRequest` is a public-proto fork) | Active (Matt, 2026-09-12) | [user-secret store §D8](server/compass-user-secret-store.md#resolved-decisions) | | DL-366 | The `compass-agent` image publishes as a multi-arch OCI image index carrying exactly `linux/amd64` + `linux/arm64` under the existing `:git-`/`:latest`/`:vX.Y.Z` tags, each arch built NATIVELY on its own runner (`ubuntu-latest` + the free public-repo `ubuntu-24.04-arm`, never QEMU) and composed by member digest in a serialized manifest job. Consumer-facing tags stay bare, so every puller relies on engine platform negotiation and no consumer changes; the per-arch `:git--` tags are internal immutable building blocks. Every single-digest guard is carried forward at manifest-list level rather than dropped — immutability, two-tag coherence and the semver re-tag compare list digests, and the `linux/amd64` platform tripwire becomes an exact platform-SET assertion plus per-member arch/os checks, a stricter contract than the one it replaces. Supersedes the `linux/amd64` single-arch platform contract of the agent-image publish record; that record's tag names, immutability posture, auth pin and build-once mechanism all stay live | Active (Matt, 2026-09-12) | [arm64 agent image §Decision A](infra/ci/compass-agent-arm64-image/design.md#decision-a--consumer-facing-tags-are-a-manifest-list-per-arch-tags-are-internal) | +| DL-368 | `SetSecretRequest`/`DeleteSecretRequest` gain a `SecretScope scope` selector, and the default is USER scope — an unspecified scope writes `(scope_kind=1, scope_id=caller)`, so a client that omits the field gets the private-by-default coordinate rather than a tenant-wide value every other user's agents resolve. Tenant scope is explicit and requires `store.UserRoleAdmin`, checked at the RPC edge (where `requireUser`'s existing `GetAccount` already holds the role) rather than the store door, which keeps DL-360's scope-shape and referential checks. Agent scope gets no wire surface: agents hold no write door, so an agent-scoped write has no authenticated writer to authorize. SUPERSEDES DL-361's "pinned to the tenant coordinate" clause and DL-363's enforcement point, keeping DL-363's authorization matrix. Corrects a factual error in D8: no admin check existed on the user-secret write path — `classifyProcedure` returns `authenticatedOpen` for both verbs — so T5 adds the gate rather than documenting one. Behavior change stated not silent: today's tenant-wide rows become per-user on re-set | Active (Matt, 2026-09-12) | [user-secret store](server/compass-user-secret-store.md#resolved-decisions) | > Note (2026-07-31, RIG-1570 R5): DL-065's retired internal `ResumeContext > resume = 12` envelope field is NOT silently reintroduced — the collapsed diff --git a/docs/designs/server/compass-user-secret-store.md b/docs/designs/server/compass-user-secret-store.md index edbdda20..389a8ac0 100644 --- a/docs/designs/server/compass-user-secret-store.md +++ b/docs/designs/server/compass-user-secret-store.md @@ -855,15 +855,20 @@ The fail-closed assertions sit AFTER the pgtest fixture (A8 ordering). ### T5 — Service cutover: SetSecret/DeleteSecret on the DB -Rewrite the user handlers in `go/server/secrets_service.go` onto -`StoreResolver` (A5): the `SetSecret` handler calls `StoreResolver.Upsert` at -the tenant coordinate (`scope_kind = 0`, `scope_id = ''` — today's -inject-all behavior preserved; the user/agent write surface is an open -question) with the rollback machinery deleted; the `DeleteSecret` handler -calls `StoreResolver.Remove` at the same tenant coordinate, with the -reserved-prefix reject (`ShadowsServerSecretPrefix` → CodeInvalidArgument) -ahead of any store call. `secretsService.resolver` field becomes -`*secrets.StoreResolver`; `serverResolver` stays `secrets.Resolver`. +Add a `SecretScope` enum and a `scope` field to +`SetSecretRequest`/`DeleteSecretRequest` (`proto/compass/v1/compass.proto`), +regenerating the Go connect and both TypeScript surfaces. Rewrite the user +handlers in `go/server/secrets_service.go` onto `StoreResolver` (A5) with the +D9 coordinate resolution: unspecified/user scope writes +`(scope_kind=1, scope_id=caller)`; tenant scope writes `(0, "")` and requires +`UserRoleAdmin`, else `CodePermissionDenied`. `requireUser` already fetches the +caller's account, so it returns the role rather than issuing a second +`GetAccount`. `SetSecret` calls `StoreResolver.Upsert` with the rollback +machinery deleted; `DeleteSecret` calls `StoreResolver.Remove` at the same +resolved coordinate, with the reserved-prefix reject +(`ShadowsServerSecretPrefix` → CodeInvalidArgument) ahead of any store call. +`secretsService.resolver` becomes `*secrets.StoreResolver`; `serverResolver` +stays `secrets.Resolver`. Once `SetSecret` no longer calls `s.store.DeclareSecret`, remove the now-unused `DeclareSecret` store method and its `InsertSecret` query (kept live through @@ -889,7 +894,12 @@ StoreResolver, A8): SetSecret persists an encrypted value a follow-up FetchSecrets-path ResolveFor returns; re-set rewrites and bumps the version signal; DeleteSecret removes it (NotFound after); agent-token callers still PermissionDenied; reserved-prefix set AND delete rejected case-insensitively; -empty value rejected before any row exists. +empty value rejected before any row exists. D9 adds: an omitted scope lands at +the CALLER's user coordinate, not `(0, "")`; a non-admin requesting tenant +scope is PermissionDenied on both set and delete; an admin requesting tenant +scope succeeds; and the isolation property that motivated D9 — user A's +user-scoped secret is NOT resolved by user B's agent, while a tenant row IS +resolved by both. ### T6 — D4 removal: server-secret write path @@ -984,21 +994,22 @@ ledger rows (below) to `docs/designs/DECISIONS.md` in the design PR itself - `secrets.Version` stays SHA-256-of-value, computed at resolve time, never stored. - Store layer sees ciphertext only; crypto lives in `envelope`/`secrets`. -- pgtest suites run under `COMPASS_TEST_USE_CONTAINER=1`; fail-closed guards - ordered after the store fixture (A8). -- `rule://red-green-testing`, `rule://no-inert-gating` (T6 removes the write - path in the same PR that makes it unreachable — no dormant flag), - `rule://go-no-fmt-print-logging`. - Public proto changes were ask-first; Matt ruled deletion (D4), so the two server-secret RPCs and their four messages are removed in T6. ## Ledger delta -`Ledger-impact: adds DL-350..DL-362 to docs/designs/DECISIONS.md (Server & +`Ledger-impact: adds DL-350..DL-356, DL-360..DL-363 and DL-368 to docs/designs/DECISIONS.md (Server & store section); FLIPS the Status cell of DL-328 to Superseded by DL-355.` Highest existing id verified this session: DL-356 (DL-350..DL-356 landed with -the original record; the A9 scope amendment adds DL-360..DL-362). +the original record; the A9 scope amendment adds DL-360..DL-362; D8 adds +DL-363; D9 adds DL-368 (364..367 were taken on main while this was in review). + +DL-368 supersedes a clause of two rows this same record adds, so those clauses +are amended in place rather than the rows flipped: DL-361's "stay pinned to the +tenant coordinate" and DL-363's store-door enforcement point. Both stay Active +— only those clauses are replaced; the rest of each decision stands. DL-328's `Status` becomes `Superseded by DL-355 (Matt, 2026-09-11)` — it is the only existing row this record touches. The full-supersession form loses no @@ -1022,6 +1033,7 @@ to `Active (key-custody clause superseded by the user-secret store record)`. | DL-361 | Secret resolution is most-specific-wins — `agent > user > tenant`, ONE value per name in the injected environment — collapsed in SQL (`DISTINCT ON` ordered by `scope_kind DESC`, the numeric encoding being the precedence) so shadowed rows never leave Postgres or get decrypted; `FetchSecrets` resolves per agent account using the identity the runnerhub authz maps (`sessionAccounts`/`containerAccounts`) already hold and previously discarded, the user tier reached through the single `agent_accounts.owner_user_id` FK hop. Scope is an ADDITIONAL filter inside a tenant — RLS tenant isolation stays the outer boundary, never replaced. The existing `SetSecret`/`DeleteSecret` verbs stay pinned to the tenant coordinate, preserving inject-all behavior until a scope wire surface is ruled | Active (Matt, 2026-09-11) | [user-secret store §A9](compass-user-secret-store.md#a9--scope-model-tenant--user--agent-most-specific-wins) | | DL-362 | The canonical user-secret AAD is the five-field tuple `"compass/user-secret/v1\x00" + tenantID + "\x00" + decimal(scopeKind) + "\x00" + scopeID + "\x00" + name + "\x00" + decimal(keyVersion)` (Go: `UserSecretAAD(tenantID string, scopeKind int16, scopeID, name string, keyVersion int16) []byte`; SMALLINTs rendered `strconv.FormatInt(int64(v), 10)`), every field bound unconditionally (a tenant row binds scopeID as the empty string) with `\x00` separators keeping the encoding injective. Fixed BEFORE any migration ships because the AAD is baked into every ciphertext — a scope field added later would force a re-encrypt of every row. Refines DL-351's four-field AAD clause; DL-351's other rulings stand | Active (Matt, 2026-09-11) | [user-secret store §A9](compass-user-secret-store.md#a9--scope-model-tenant--user--agent-most-specific-wins) | | DL-363 | Writing a tenant-scoped user-secret row requires an admin (`store.UserRoleAdmin`, `go/internal/store/types.go`), reusing the existing role elevation rather than introducing a permission concept: tenant (0) admin-only, user (1) and agent (2) writable by the owning user or an admin. The check lands in the store door inside the same writing transaction as DL-360's FK-substitute referential checks, so one place enforces both. READS are deliberately asymmetric — a plain user's agent resolves tenant rows, which is the point of a shared tenant value under DL-361; reading a shared secret is the feature, writing one is the privileged act. The wire surface for a scoped write stays undecided (a scope selector on `SetSecretRequest` is a public-proto fork) | Active (Matt, 2026-09-12) | [user-secret store §D8](compass-user-secret-store.md#resolved-decisions) | +| DL-368 | `SetSecretRequest`/`DeleteSecretRequest` gain a `SecretScope scope` selector, and the default is USER scope — an unspecified scope writes `(scope_kind=1, scope_id=caller)`, so a client that omits the field gets the private-by-default coordinate rather than a tenant-wide value every other user's agents resolve. Tenant scope is explicit and requires `store.UserRoleAdmin`, checked at the RPC edge (where `requireUser`'s existing `GetAccount` already holds the role) rather than the store door, which keeps DL-360's scope-shape and referential checks. Agent scope gets no wire surface: agents hold no write door, so an agent-scoped write has no authenticated writer to authorize. SUPERSEDES DL-361's "pinned to the tenant coordinate" clause and DL-363's enforcement point, keeping DL-363's authorization matrix. Corrects a factual error in D8: no admin check existed on the user-secret write path — `classifyProcedure` returns `authenticatedOpen` for both verbs — so T5 adds the gate rather than documenting one. Behavior change stated not silent: today's tenant-wide rows become per-user on re-set | Active (Matt, 2026-09-12) | [user-secret store](compass-user-secret-store.md#resolved-decisions) | ## Resolved decisions @@ -1122,10 +1134,61 @@ the draft argued for, and because D1 supersedes part of a frozen record. checks, so one place enforces both. READS stay deliberately asymmetric: a plain user's agent resolves tenant rows, which is the entire point of a shared tenant value under DL-361 — reading a shared secret is the feature, - writing one is the privileged act. This also answers T2's two `(0, "")` - placeholders in `go/server/secrets_service.go`: they are tenant-scoped - writes and are already admin-gated at the door, so T5 verifies and - documents that gate rather than adding one. Ledger: DL-363. + writing one is the privileged act. D9 supersedes D8's ENFORCEMENT POINT (the + role check lands at the RPC edge, not the store door) while keeping its + matrix intact. This also bears on T2's two `(0, "")` placeholders in + `go/server/secrets_service.go`. D8 originally claimed they were "already + admin-gated at the door" — that is FALSE, corrected by D9: no admin check + exists on the user-secret write path. T5 adds one. Ledger: DL-363. +- **D9 — `SetSecret`/`DeleteSecret` carry an explicit scope selector; the + default is USER scope (Matt, 2026-09-12, closes OQ "what wire surface + carries a scoped write").** Two corrections to D8 land with this ruling. + + First, a **factual error in D8**: it asserts the two `(0, "")` coordinates in + `go/server/secrets_service.go` "are already admin-gated at the door, so T5 + verifies and documents that gate rather than adding one." They are NOT. + `classifyProcedure` (`go/internal/auth/admin_gate.go`) returns + `authenticatedOpen{}` for `SetSecretProcedure` and `DeleteSecretProcedure`, + and `requireUser` checks only that the caller is not an agent — it never + reads `UserRole`. No admin check exists anywhere on the user-secret write + path. T5 ADDS the gate; it does not document an existing one. + + Second, D8 + the pinned tenant coordinate would have made `SetSecret` + **admin-only in practice**, silently removing a shipped user-facing verb from + every ordinary user: `CreateUser` always seeds `UserRoleMember` + (`go/internal/store/accounts.go`), the only `UserRoleAdmin` account is the + bootstrap one (`adminByHandle`), and NO role-promotion path exists — the + proto calls elevation "a separate admin-authorized path (deferred)" + (`proto/compass/v1/comms.proto`, `CreateUser`). So an admin-only tenant write + plus a tenant-pinned verb equals a verb no user can reach and no admin can + grant. + + The ruling: **per-user credentials are the intended behavior** — a + user-scoped credential must NOT be visible to another user's agents. + `SetSecretRequest`/`DeleteSecretRequest` gain a `SecretScope scope` field + (proto enum mirroring `store.SecretScope*`), and the handler resolves the + coordinate from it: + + - `SECRET_SCOPE_UNSPECIFIED` (0) and `SECRET_SCOPE_USER` both write + `(scope_kind=1, scope_id=caller)`. The unspecified default is USER, not + tenant, so an old client that omits the field gets the private-by-default + coordinate rather than silently writing a value every other user's agents + resolve. This is the one deliberate behavior change: today's rows are + tenant-wide, and a pre-existing tenant row keeps resolving (it still wins + nothing — user scope outranks it under DL-361) until re-set. + - `SECRET_SCOPE_TENANT` writes `(0, "")` and **requires `UserRoleAdmin`**, + per D8's matrix. A non-admin requesting tenant scope is + `CodePermissionDenied`. + + The admin check lives at the RPC edge (it needs the caller's role, which the + handler already fetches in `requireUser` via `GetAccount`), while the store + door keeps D8's scope-shape and referential checks. This splits D8's "one + place enforces both" — the role is an identity property known at the edge, + not a row property, and re-reading the account inside the write transaction + would add a query to every write to re-derive what the caller already holds. + `SECRET_SCOPE_AGENT` gets no wire surface here: agents hold no write door + (`requireUser` rejects agent tokens), so an agent-scoped write has no + authenticated writer to authorize. Ledger: DL-368. ## Open questions @@ -1140,13 +1203,10 @@ needs a Matt ruling; none is silently decided by this amendment. provenance (whoever wrote the row), or does it carry authorization weight (only the declarer may rewrite/delete)? This record treats it as provenance only. -- **What wire surface carries a scoped write.** The authorization matrix is - now ruled (D8), but the surface is not. T5 keeps the existing - `SetSecret`/`DeleteSecret` verbs pinned to the tenant coordinate — today's - observable behavior — so per-user/per-agent writes have NO surface yet: - `SetSecretRequest`/`DeleteSecretRequest` would need a scope selector, and - adding one is a public-proto change (an ask-first fork), so it is - deliberately not decided here. +- ~~**What wire surface carries a scoped write.**~~ RESOLVED by D9 + (2026-09-12): a `SecretScope scope` field on + `SetSecretRequest`/`DeleteSecretRequest`, defaulting to USER scope, with + tenant scope admin-gated. - **Lifecycle of scoped rows when their account goes away.** `scope_id` carries no FK (A9), so deleting an agent account neither cascades nor RESTRICTs its agent-scoped secret rows — they linger as unreachable From 95db14c12c7fe1e522475cfd94ad1be187e53abf Mon Sep 17 00:00:00 2001 From: mintaka Date: Sat, 12 Sep 2026 16:22:59 -0400 Subject: [PATCH 2/2] docs(design): propagate D9's supersession through the record (RIG-3655) Review found the amendment did not carry its own ruling into the normative text. The ledger-delta prose claimed DL-361 and DL-363 were amended in place when the diff left both rows untouched, so the ledger asserted the pinned tenant coordinate and D9's scope selector at once. Both clauses are now edited and marked superseded in part, in the record table and DECISIONS.md alike. A5's SetSecret and DeleteSecret bullets and the T5 task index still instructed the tenant-pinned handler D9 replaces; an implementer reading them would have built the wrong thing. The dependent DeleteSecret open question is resolved. Corrects a wrong reassurance about existing rows: a re-set writes a new user-scoped row and does NOT retire the tenant row, which keeps resolving for every other user until an admin deletes it. A regression test pins that. Restores three Global Constraints bullets an earlier edit in this branch dropped by accident. Refs RIG-3655 Co-authored-by: Matt Wilkinson --- docs/designs/DECISIONS.md | 4 +- .../server/compass-user-secret-store.md | 99 +++++++++++++------ 2 files changed, 73 insertions(+), 30 deletions(-) diff --git a/docs/designs/DECISIONS.md b/docs/designs/DECISIONS.md index 634c623a..187fd061 100644 --- a/docs/designs/DECISIONS.md +++ b/docs/designs/DECISIONS.md @@ -130,9 +130,9 @@ check enforces the mechanical half. Full rationale: | DL-355 | Master-key custody is operator-seeded, not compass-written: DL-328's zero-human-step auto-provision into a WRITABLE provider is superseded, because making boot secrets read-only removes the write path it needs (and it was never implemented — `go/internal/envelope` was absent and `server_key_state` had no non-test readers). First-run generation is a standalone `compass` CLI verb that mints a 256-bit key and seeds the configured provider; the nix/devenv seed script invokes that same verb, so a standard deploy needs no explicit step and a hand-rolled one needs a single documented command. Boot only reads, failing closed naming the verb. The key is renamed `GATEWAY_CREDENTIALS_MASTER_KEY` → `COMPASS_MASTER_KEY` and shared with the future `gateway_credentials` store, which MUST adopt its own distinct AAD domain label; a new `COMPASS_` reserved prefix joins the existing two in the CHECK constraint and both prefix predicates, without which the renamed key is undeclarable and therefore unresolvable | Active (Matt, 2026-09-11) | [user-secret store](server/compass-user-secret-store.md#resolved-decisions) | | DL-356 | `SetServerSecret`/`DeleteServerSecret` and their four request/response messages are DELETED from `proto/compass/v1/compass.proto`, together with the `compass server-secret set` verb, the two admin-gate procedures, and the generated Go connect + both TypeScript surfaces — a clean public-API cutover, not a CodeUnimplemented stub. Grounded on no consumer needing a runtime WRITE: server secret VALUES are in fact read at runtime (`newDeclaredSecretResolver` returns a per-call `Resolve` closure; the webhook secret is resolved on every unauthenticated `POST /webhooks/github` before the HMAC check, TTL-cached at `forgeTokenTTL`), which is precisely why provider-side rotation with the secretspec CLI suffices without any RPC. `ListServerSecrets`/`Statuses` survive; the unrelated store-layer `DeleteServerSecret` sqlc query (declaration removal) is untouched | Active (Matt, 2026-09-11) | [user-secret store](server/compass-user-secret-store.md#resolved-decisions) | | DL-360 | User secrets are scoped at THREE levels — tenant (0), user (1), agent (2) — via `scope_kind SMALLINT` + `scope_id TEXT` (empty string for a tenant row, the owning `accounts.id` for user/agent rows) with the `secrets` PK widened to `(name, scope_kind, scope_id)`; a tenant row is a real shared VALUE several users resolve, not a declaration placeholder. The scope↔id shape is CHECK-enforced (`secrets_scope_shape`); `scope_id` carries NO FK to `accounts` — a tenant row's `''` can never satisfy one and Postgres has no conditional FK — so user/agent referential integrity is enforced at the store door in the writing transaction | Active (Matt, 2026-09-11) | [user-secret store §A9](server/compass-user-secret-store.md#a9--scope-model-tenant--user--agent-most-specific-wins) | -| DL-361 | Secret resolution is most-specific-wins — `agent > user > tenant`, ONE value per name in the injected environment — collapsed in SQL (`DISTINCT ON` ordered by `scope_kind DESC`, the numeric encoding being the precedence) so shadowed rows never leave Postgres or get decrypted; `FetchSecrets` resolves per agent account using the identity the runnerhub authz maps (`sessionAccounts`/`containerAccounts`) already hold and previously discarded, the user tier reached through the single `agent_accounts.owner_user_id` FK hop. Scope is an ADDITIONAL filter inside a tenant — RLS tenant isolation stays the outer boundary, never replaced. The existing `SetSecret`/`DeleteSecret` verbs stay pinned to the tenant coordinate, preserving inject-all behavior until a scope wire surface is ruled | Active (Matt, 2026-09-11) | [user-secret store §A9](server/compass-user-secret-store.md#a9--scope-model-tenant--user--agent-most-specific-wins) | +| DL-361 | Secret resolution is most-specific-wins — `agent > user > tenant`, ONE value per name in the injected environment — collapsed in SQL (`DISTINCT ON` ordered by `scope_kind DESC`, the numeric encoding being the precedence) so shadowed rows never leave Postgres or get decrypted; `FetchSecrets` resolves per agent account using the identity the runnerhub authz maps (`sessionAccounts`/`containerAccounts`) already hold and previously discarded, the user tier reached through the single `agent_accounts.owner_user_id` FK hop. Scope is an ADDITIONAL filter inside a tenant — RLS tenant isolation stays the outer boundary, never replaced. SUPERSEDED IN PART by DL-368: the `SetSecret`/`DeleteSecret` verbs no longer pin to the tenant coordinate — they carry an explicit scope selector defaulting to user scope | Active (Matt, 2026-09-11) | [user-secret store §A9](server/compass-user-secret-store.md#a9--scope-model-tenant--user--agent-most-specific-wins) | | DL-362 | The canonical user-secret AAD is the five-field tuple `"compass/user-secret/v1\x00" + tenantID + "\x00" + decimal(scopeKind) + "\x00" + scopeID + "\x00" + name + "\x00" + decimal(keyVersion)` (Go: `UserSecretAAD(tenantID string, scopeKind int16, scopeID, name string, keyVersion int16) []byte`; SMALLINTs rendered `strconv.FormatInt(int64(v), 10)`), every field bound unconditionally (a tenant row binds scopeID as the empty string) with `\x00` separators keeping the encoding injective. Fixed BEFORE any migration ships because the AAD is baked into every ciphertext — a scope field added later would force a re-encrypt of every row. Refines DL-351's four-field AAD clause; DL-351's other rulings stand | Active (Matt, 2026-09-11) | [user-secret store §A9](server/compass-user-secret-store.md#a9--scope-model-tenant--user--agent-most-specific-wins) | -| DL-363 | Writing a tenant-scoped user-secret row requires an admin (`store.UserRoleAdmin`, `go/internal/store/types.go`), reusing the existing role elevation rather than introducing a permission concept: tenant (0) admin-only, user (1) and agent (2) writable by the owning user or an admin. The check lands in the store door inside the same writing transaction as DL-360's FK-substitute referential checks, so one place enforces both. READS are deliberately asymmetric — a plain user's agent resolves tenant rows, which is the point of a shared tenant value under DL-361; reading a shared secret is the feature, writing one is the privileged act. The wire surface for a scoped write stays undecided (a scope selector on `SetSecretRequest` is a public-proto fork) | Active (Matt, 2026-09-12) | [user-secret store §D8](server/compass-user-secret-store.md#resolved-decisions) | +| DL-363 | Writing a tenant-scoped user-secret row requires an admin (`store.UserRoleAdmin`, `go/internal/store/types.go`), reusing the existing role elevation rather than introducing a permission concept: tenant (0) admin-only, user (1) and agent (2) writable by the owning user or an admin. SUPERSEDED IN PART by DL-368: the role check lands at the RPC edge, not the store door, which keeps DL-360's scope-shape and referential checks. READS are deliberately asymmetric — a plain user's agent resolves tenant rows, which is the point of a shared tenant value under DL-361; reading a shared secret is the feature, writing one is the privileged act. The wire surface is now ruled by DL-368 (a `SecretScope` selector on `SetSecretRequest`/`DeleteSecretRequest`) | Active (Matt, 2026-09-12) | [user-secret store §D8](server/compass-user-secret-store.md#resolved-decisions) | | DL-366 | The `compass-agent` image publishes as a multi-arch OCI image index carrying exactly `linux/amd64` + `linux/arm64` under the existing `:git-`/`:latest`/`:vX.Y.Z` tags, each arch built NATIVELY on its own runner (`ubuntu-latest` + the free public-repo `ubuntu-24.04-arm`, never QEMU) and composed by member digest in a serialized manifest job. Consumer-facing tags stay bare, so every puller relies on engine platform negotiation and no consumer changes; the per-arch `:git--` tags are internal immutable building blocks. Every single-digest guard is carried forward at manifest-list level rather than dropped — immutability, two-tag coherence and the semver re-tag compare list digests, and the `linux/amd64` platform tripwire becomes an exact platform-SET assertion plus per-member arch/os checks, a stricter contract than the one it replaces. Supersedes the `linux/amd64` single-arch platform contract of the agent-image publish record; that record's tag names, immutability posture, auth pin and build-once mechanism all stay live | Active (Matt, 2026-09-12) | [arm64 agent image §Decision A](infra/ci/compass-agent-arm64-image/design.md#decision-a--consumer-facing-tags-are-a-manifest-list-per-arch-tags-are-internal) | | DL-368 | `SetSecretRequest`/`DeleteSecretRequest` gain a `SecretScope scope` selector, and the default is USER scope — an unspecified scope writes `(scope_kind=1, scope_id=caller)`, so a client that omits the field gets the private-by-default coordinate rather than a tenant-wide value every other user's agents resolve. Tenant scope is explicit and requires `store.UserRoleAdmin`, checked at the RPC edge (where `requireUser`'s existing `GetAccount` already holds the role) rather than the store door, which keeps DL-360's scope-shape and referential checks. Agent scope gets no wire surface: agents hold no write door, so an agent-scoped write has no authenticated writer to authorize. SUPERSEDES DL-361's "pinned to the tenant coordinate" clause and DL-363's enforcement point, keeping DL-363's authorization matrix. Corrects a factual error in D8: no admin check existed on the user-secret write path — `classifyProcedure` returns `authenticatedOpen` for both verbs — so T5 adds the gate rather than documenting one. Behavior change stated not silent: today's tenant-wide rows become per-user on re-set | Active (Matt, 2026-09-12) | [user-secret store](server/compass-user-secret-store.md#resolved-decisions) | diff --git a/docs/designs/server/compass-user-secret-store.md b/docs/designs/server/compass-user-secret-store.md index 389a8ac0..9d7fcfca 100644 --- a/docs/designs/server/compass-user-secret-store.md +++ b/docs/designs/server/compass-user-secret-store.md @@ -362,17 +362,16 @@ semantics are unchanged: a same-value re-set still hashes identically. two-system flow (`s.store.DeclareSecret` + `s.resolver.Set`) with its ErrConflict re-set branch and rollback-on-fresh-write-failure collapses into one `StoreResolver.Upsert` upsert transaction. Re-set of an existing name - stays a value rewrite (UPSERT). Scope on this verb: `SetSecret` writes the - TENANT row (`scope_kind = 0`, `scope_id = ''`), preserving today's - observable inject-all behavior — a wire surface for writing user- or - agent-scoped rows, and who may write each tier, is an open question - (`## Open questions`), deliberately not smuggled into this amendment. The + stays a value rewrite (UPSERT). Scope on this verb (D9): the request carries + a `SecretScope` selector; an unspecified or user scope writes + `(scope_kind = 1, scope_id = caller)`, and an explicit tenant scope writes + `(0, '')` and requires `UserRoleAdmin`, else `CodePermissionDenied`. The user-only gate (`requireUser`), empty value rejection, `secretRoutingFromProto`, and `bumpSecretsVersion` are unchanged. - **`DeleteSecret`** (`go/server/secrets_service.go`): one transaction - deleting the row (declaration and value are the same row), addressed at - the tenant coordinate (`scope_kind = 0`, `scope_id = ''`) until the scope - wire surface exists, matching `SetSecret`. PR #1066's + deleting the row (declaration and value are the same row), addressed at the + coordinate its own `SecretScope` selector resolves to (D9), the same + resolution `SetSecret` uses. PR #1066's reviewed semantics fold in as follows — carried forward: the guard that a reserved-prefix name never reaches the user delete door, split as `HasServerSecretPrefix` (byte-exact ADMIT check at the server door, @@ -870,6 +869,30 @@ resolved coordinate, with the reserved-prefix reject `secretsService.resolver` becomes `*secrets.StoreResolver`; `serverResolver` stays `secrets.Resolver`. +The enum names the same tiers as `store.SecretScope*` but NOT the same numbers, +because proto reserves 0 for unspecified: + +```protobuf +enum SecretScope { + SECRET_SCOPE_UNSPECIFIED = 0; // treated as USER — private by default + SECRET_SCOPE_USER = 1; + SECRET_SCOPE_TENANT = 2; // admin-only +} +``` + +`SECRET_SCOPE_UNSPECIFIED` and `SECRET_SCOPE_USER` are distinct wire values but +the SAME coordinate, so an old client and a new explicit one land identically. +There is no `SECRET_SCOPE_AGENT`: agents hold no write door, so it would name a +coordinate no caller can write. The handler MAPS the wire value to the store +value explicitly rather than casting — tenant is 0 in the store and 2 here, so +a cast would turn an omitted field into a tenant write, the exact failure this +default exists to prevent. + +The CLI carries the selector too: `compass secret set` gains a `--scope` +string flag (`user` default, `tenant` explicit) parsed the way `--delivery` and +`--kind` already are (`go/cmd/compass/secret.go`), and `compass secret delete` +gains the same flag. Without it the ruling has no operator-reachable surface. + Once `SetSecret` no longer calls `s.store.DeclareSecret`, remove the now-unused `DeclareSecret` store method and its `InsertSecret` query (kept live through T2–T4 per the F2 re-sequence). With the value-free insert gone and the upsert — @@ -899,7 +922,10 @@ the CALLER's user coordinate, not `(0, "")`; a non-admin requesting tenant scope is PermissionDenied on both set and delete; an admin requesting tenant scope succeeds; and the isolation property that motivated D9 — user A's user-scoped secret is NOT resolved by user B's agent, while a tenant row IS -resolved by both. +resolved by both. One more pins the surviving-row behavior above: after user A +re-sets an existing tenant-scoped NAME at the default user scope, user B's +agent still resolves the OLD tenant value for NAME. That test fails if anyone +later "helpfully" makes a user-scope write delete the tenant row. ### T6 — D4 removal: server-secret write path @@ -967,8 +993,10 @@ ledger rows (below) to `docs/designs/DECISIONS.md` in the design PR itself rename machinery (CHECK, prefix set, `masterKeyName`/`masterKeyCLIName`, strip list) that makes `COMPASS_MASTER_KEY` constructible and resolvable (F1). -- [ ] T5 — Service cutover: `SetSecret`/`DeleteSecret` on the DB at the - tenant coordinate, PR #1066 delete semantics folded; +- [ ] T5 — Service cutover: `SetSecret`/`DeleteSecret` on the DB with the D9 + scope selector (`SecretScope` proto enum + `scope` field + `--scope` CLI + flag; unspecified/user → `(1, caller)`, tenant → `(0, '')` admin-gated at + the RPC edge), PR #1066 delete semantics folded; `DeclareSecret`/`InsertSecret` removed with their caller; `value_ciphertext`/`value_nonce` tightened to NOT NULL now the upsert is the sole writer. @@ -996,6 +1024,11 @@ ledger rows (below) to `docs/designs/DECISIONS.md` in the design PR itself - Store layer sees ciphertext only; crypto lives in `envelope`/`secrets`. - Public proto changes were ask-first; Matt ruled deletion (D4), so the two server-secret RPCs and their four messages are removed in T6. +- pgtest suites run under `COMPASS_TEST_USE_CONTAINER=1`; fail-closed guards + ordered after the store fixture (A8). +- `rule://red-green-testing`, `rule://no-inert-gating` (T6 removes the write + path in the same PR that makes it unreachable — no dormant flag), + `rule://go-no-fmt-print-logging`. ## Ledger delta @@ -1006,10 +1039,14 @@ Highest existing id verified this session: DL-356 (DL-350..DL-356 landed with the original record; the A9 scope amendment adds DL-360..DL-362; D8 adds DL-363; D9 adds DL-368 (364..367 were taken on main while this was in review). -DL-368 supersedes a clause of two rows this same record adds, so those clauses -are amended in place rather than the rows flipped: DL-361's "stay pinned to the -tenant coordinate" and DL-363's store-door enforcement point. Both stay Active -— only those clauses are replaced; the rest of each decision stands. +DL-368 supersedes a clause of two rows this same record adds. Those clauses are +edited in place and marked SUPERSEDED IN PART, in both this table and +`DECISIONS.md`: DL-361's "stay pinned to the tenant coordinate" and DL-363's +store-door enforcement point. Both rows stay Active — only those clauses are +replaced, and the rest of each decision stands. Editing them in place rather +than appending a contradicting row matters here because an implementer reads +the ledger literally; two live rows disagreeing about where the admin check +lands is worse than either answer. DL-328's `Status` becomes `Superseded by DL-355 (Matt, 2026-09-11)` — it is the only existing row this record touches. The full-supersession form loses no @@ -1030,10 +1067,10 @@ to `Active (key-custody clause superseded by the user-secret store record)`. | DL-355 | Master-key custody is operator-seeded, not compass-written: DL-328's zero-human-step auto-provision into a WRITABLE provider is superseded, because making boot secrets read-only removes the write path it needs (and it was never implemented — `go/internal/envelope` was absent and `server_key_state` had no non-test readers). First-run generation is a standalone `compass` CLI verb that mints a 256-bit key and seeds the configured provider; the nix/devenv seed script invokes that same verb, so a standard deploy needs no explicit step and a hand-rolled one needs a single documented command. Boot only reads, failing closed naming the verb. The key is renamed `GATEWAY_CREDENTIALS_MASTER_KEY` → `COMPASS_MASTER_KEY` and shared with the future `gateway_credentials` store, which MUST adopt its own distinct AAD domain label; a new `COMPASS_` reserved prefix joins the existing two in the CHECK constraint and both prefix predicates, without which the renamed key is undeclarable and therefore unresolvable | Active (Matt, 2026-09-11) | [user-secret store](compass-user-secret-store.md#resolved-decisions) | | DL-356 | `SetServerSecret`/`DeleteServerSecret` and their four request/response messages are DELETED from `proto/compass/v1/compass.proto`, together with the `compass server-secret set` verb, the two admin-gate procedures, and the generated Go connect + both TypeScript surfaces — a clean public-API cutover, not a CodeUnimplemented stub. Grounded on no consumer needing a runtime WRITE: server secret VALUES are in fact read at runtime (`newDeclaredSecretResolver` returns a per-call `Resolve` closure; the webhook secret is resolved on every unauthenticated `POST /webhooks/github` before the HMAC check, TTL-cached at `forgeTokenTTL`), which is precisely why provider-side rotation with the secretspec CLI suffices without any RPC. `ListServerSecrets`/`Statuses` survive; the unrelated store-layer `DeleteServerSecret` sqlc query (declaration removal) is untouched | Active (Matt, 2026-09-11) | [user-secret store](compass-user-secret-store.md#resolved-decisions) | | DL-360 | User secrets are scoped at THREE levels — tenant (0), user (1), agent (2) — via `scope_kind SMALLINT` + `scope_id TEXT` (empty string for a tenant row, the owning `accounts.id` for user/agent rows) with the `secrets` PK widened to `(name, scope_kind, scope_id)`; a tenant row is a real shared VALUE several users resolve, not a declaration placeholder. The scope↔id shape is CHECK-enforced (`secrets_scope_shape`); `scope_id` carries NO FK to `accounts` — a tenant row's `''` can never satisfy one and Postgres has no conditional FK — so user/agent referential integrity is enforced at the store door in the writing transaction | Active (Matt, 2026-09-11) | [user-secret store §A9](compass-user-secret-store.md#a9--scope-model-tenant--user--agent-most-specific-wins) | -| DL-361 | Secret resolution is most-specific-wins — `agent > user > tenant`, ONE value per name in the injected environment — collapsed in SQL (`DISTINCT ON` ordered by `scope_kind DESC`, the numeric encoding being the precedence) so shadowed rows never leave Postgres or get decrypted; `FetchSecrets` resolves per agent account using the identity the runnerhub authz maps (`sessionAccounts`/`containerAccounts`) already hold and previously discarded, the user tier reached through the single `agent_accounts.owner_user_id` FK hop. Scope is an ADDITIONAL filter inside a tenant — RLS tenant isolation stays the outer boundary, never replaced. The existing `SetSecret`/`DeleteSecret` verbs stay pinned to the tenant coordinate, preserving inject-all behavior until a scope wire surface is ruled | Active (Matt, 2026-09-11) | [user-secret store §A9](compass-user-secret-store.md#a9--scope-model-tenant--user--agent-most-specific-wins) | +| DL-361 | Secret resolution is most-specific-wins — `agent > user > tenant`, ONE value per name in the injected environment — collapsed in SQL (`DISTINCT ON` ordered by `scope_kind DESC`, the numeric encoding being the precedence) so shadowed rows never leave Postgres or get decrypted; `FetchSecrets` resolves per agent account using the identity the runnerhub authz maps (`sessionAccounts`/`containerAccounts`) already hold and previously discarded, the user tier reached through the single `agent_accounts.owner_user_id` FK hop. Scope is an ADDITIONAL filter inside a tenant — RLS tenant isolation stays the outer boundary, never replaced. SUPERSEDED IN PART by DL-368: the `SetSecret`/`DeleteSecret` verbs no longer pin to the tenant coordinate — they carry an explicit scope selector defaulting to user scope | Active (Matt, 2026-09-11) | [user-secret store §A9](compass-user-secret-store.md#a9--scope-model-tenant--user--agent-most-specific-wins) | | DL-362 | The canonical user-secret AAD is the five-field tuple `"compass/user-secret/v1\x00" + tenantID + "\x00" + decimal(scopeKind) + "\x00" + scopeID + "\x00" + name + "\x00" + decimal(keyVersion)` (Go: `UserSecretAAD(tenantID string, scopeKind int16, scopeID, name string, keyVersion int16) []byte`; SMALLINTs rendered `strconv.FormatInt(int64(v), 10)`), every field bound unconditionally (a tenant row binds scopeID as the empty string) with `\x00` separators keeping the encoding injective. Fixed BEFORE any migration ships because the AAD is baked into every ciphertext — a scope field added later would force a re-encrypt of every row. Refines DL-351's four-field AAD clause; DL-351's other rulings stand | Active (Matt, 2026-09-11) | [user-secret store §A9](compass-user-secret-store.md#a9--scope-model-tenant--user--agent-most-specific-wins) | -| DL-363 | Writing a tenant-scoped user-secret row requires an admin (`store.UserRoleAdmin`, `go/internal/store/types.go`), reusing the existing role elevation rather than introducing a permission concept: tenant (0) admin-only, user (1) and agent (2) writable by the owning user or an admin. The check lands in the store door inside the same writing transaction as DL-360's FK-substitute referential checks, so one place enforces both. READS are deliberately asymmetric — a plain user's agent resolves tenant rows, which is the point of a shared tenant value under DL-361; reading a shared secret is the feature, writing one is the privileged act. The wire surface for a scoped write stays undecided (a scope selector on `SetSecretRequest` is a public-proto fork) | Active (Matt, 2026-09-12) | [user-secret store §D8](compass-user-secret-store.md#resolved-decisions) | -| DL-368 | `SetSecretRequest`/`DeleteSecretRequest` gain a `SecretScope scope` selector, and the default is USER scope — an unspecified scope writes `(scope_kind=1, scope_id=caller)`, so a client that omits the field gets the private-by-default coordinate rather than a tenant-wide value every other user's agents resolve. Tenant scope is explicit and requires `store.UserRoleAdmin`, checked at the RPC edge (where `requireUser`'s existing `GetAccount` already holds the role) rather than the store door, which keeps DL-360's scope-shape and referential checks. Agent scope gets no wire surface: agents hold no write door, so an agent-scoped write has no authenticated writer to authorize. SUPERSEDES DL-361's "pinned to the tenant coordinate" clause and DL-363's enforcement point, keeping DL-363's authorization matrix. Corrects a factual error in D8: no admin check existed on the user-secret write path — `classifyProcedure` returns `authenticatedOpen` for both verbs — so T5 adds the gate rather than documenting one. Behavior change stated not silent: today's tenant-wide rows become per-user on re-set | Active (Matt, 2026-09-12) | [user-secret store](compass-user-secret-store.md#resolved-decisions) | +| DL-363 | Writing a tenant-scoped user-secret row requires an admin (`store.UserRoleAdmin`, `go/internal/store/types.go`), reusing the existing role elevation rather than introducing a permission concept: tenant (0) admin-only, user (1) and agent (2) writable by the owning user or an admin. SUPERSEDED IN PART by DL-368: the role check lands at the RPC edge, not the store door, which keeps DL-360's scope-shape and referential checks. READS are deliberately asymmetric — a plain user's agent resolves tenant rows, which is the point of a shared tenant value under DL-361; reading a shared secret is the feature, writing one is the privileged act. The wire surface is now ruled by DL-368 (a `SecretScope` selector on `SetSecretRequest`/`DeleteSecretRequest`) | Active (Matt, 2026-09-12) | [user-secret store §D8](compass-user-secret-store.md#resolved-decisions) | +| DL-368 | `SetSecretRequest`/`DeleteSecretRequest` gain a `SecretScope scope` selector, and the default is USER scope — an unspecified scope writes `(scope_kind=1, scope_id=caller)`, so a client that omits the field gets the private-by-default coordinate rather than a tenant-wide value every other user's agents resolve. Tenant scope is explicit and requires `store.UserRoleAdmin`, checked at the RPC edge (where `requireUser`'s existing `GetAccount` already holds the role) rather than the store door, which keeps DL-360's scope-shape and referential checks. Agent scope gets no wire surface: agents hold no write door, so an agent-scoped write has no authenticated writer to authorize. SUPERSEDES DL-361's "pinned to the tenant coordinate" clause and DL-363's enforcement point, keeping DL-363's authorization matrix. Corrects a factual error in D8: no admin check existed on the user-secret write path — `classifyProcedure` returns `authenticatedOpen` for both verbs — so T5 adds the gate rather than documenting one. Behavior change stated not silent: a re-set writes a NEW user-scoped row and does NOT retire the pre-existing tenant row, which keeps resolving for every other user until an admin deletes it at explicit tenant scope | Active (Matt, 2026-09-12) | [user-secret store §D9](compass-user-secret-store.md#resolved-decisions) | ## Resolved decisions @@ -1157,7 +1194,7 @@ the draft argued for, and because D1 supersedes part of a frozen record. **admin-only in practice**, silently removing a shipped user-facing verb from every ordinary user: `CreateUser` always seeds `UserRoleMember` (`go/internal/store/accounts.go`), the only `UserRoleAdmin` account is the - bootstrap one (`adminByHandle`), and NO role-promotion path exists — the + bootstrap one (`BootstrapAdmin`), and NO role-promotion path exists — the proto calls elevation "a separate admin-authorized path (deferred)" (`proto/compass/v1/comms.proto`, `CreateUser`). So an admin-only tenant write plus a tenant-pinned verb equals a verb no user can reach and no admin can @@ -1166,16 +1203,22 @@ the draft argued for, and because D1 supersedes part of a frozen record. The ruling: **per-user credentials are the intended behavior** — a user-scoped credential must NOT be visible to another user's agents. `SetSecretRequest`/`DeleteSecretRequest` gain a `SecretScope scope` field - (proto enum mirroring `store.SecretScope*`), and the handler resolves the - coordinate from it: + (a proto enum naming the same tiers as `store.SecretScope*`, though not the + same numbers — proto reserves 0 for unspecified), and the handler resolves + the coordinate from it: - `SECRET_SCOPE_UNSPECIFIED` (0) and `SECRET_SCOPE_USER` both write `(scope_kind=1, scope_id=caller)`. The unspecified default is USER, not tenant, so an old client that omits the field gets the private-by-default coordinate rather than silently writing a value every other user's agents - resolve. This is the one deliberate behavior change: today's rows are - tenant-wide, and a pre-existing tenant row keeps resolving (it still wins - nothing — user scope outranks it under DL-361) until re-set. + resolve. **A re-set does NOT retire the old shared value.** It writes a new + row at a different primary key, so the pre-existing `(0, '')` tenant row + survives: the re-setting user's own agents now see their private value + (user scope outranks tenant under DL-361), but every OTHER user in the + tenant keeps resolving the old shared one. Retiring it takes an admin + `DeleteSecret` at explicit tenant scope. Stated because the intuition runs + the other way — a user who re-sets a secret may believe they have + privatized it while the old value keeps reaching other users' agents. - `SECRET_SCOPE_TENANT` writes `(0, "")` and **requires `UserRoleAdmin`**, per D8's matrix. A non-admin requesting tenant scope is `CodePermissionDenied`. @@ -1215,7 +1258,7 @@ needs a Matt ruling; none is silently decided by this amendment. question holds for user deletion and user-scoped rows (`declared_by`'s ON DELETE RESTRICT already blocks deleting an account that DECLARED rows, which may mask this in practice). -- **Whether `DeleteSecret` needs scope addressing before the write surface - lands.** With writes pinned to the tenant coordinate the pinned delete is - consistent; but the moment any user/agent row exists (e.g. operator-seeded), - the name-only wire delete cannot address it. +- ~~**Whether `DeleteSecret` needs scope addressing before the write surface + lands.**~~ RESOLVED by D9 (2026-09-12): `DeleteSecret` carries the same + `SecretScope` selector as `SetSecret` and resolves to the caller's user + coordinate by default, so a name is addressable at every tier it can hold.