feat(store): scope and encrypt user secrets at rest (RIG-3655 T2) - #1118
Merged
trunk-io[bot] merged 2 commits intoSep 12, 2026
Merged
trunk-io[bot] merged 2 commits into
trunk-io[bot] merged 2 commits into
Conversation
This was referenced Sep 11, 2026
rigel-mintaka
added this pull request to stack #1112
September 11, 2026 20:53
|
Compass engineering docs preview: https://compass-server-rig-3655-t2-s.compass-eng-docs.pages.dev Deployed from |
T2 of the user-secret store: the `secrets` table stops being a names-only registry and becomes the store of record for scoped, encrypted values. Scope is three tiers -- 0 tenant, 1 user, 2 agent -- so the primary key becomes `(name, scope_kind, scope_id)`. A tenant row is a real shared value, and a user or agent row of the same name shadows it. `secrets_scope_shape` enforces that a tenant row carries an empty `scope_id` and a user/agent row does not, so a malformed coordinate cannot reach a row. `scope_id` carries no foreign key: a tenant row's empty string could not satisfy one. The referential check moves to the store door, which resolves the id against `user_accounts` or `agent_accounts` inside the write transaction. `SecretRecordsForAgent` collapses the precedence in SQL -- `DISTINCT ON (name)` under `ORDER BY name, scope_kind DESC`, with the user tier reached through `agent_accounts.owner_user_id`. Doing it here rather than in Go means a shadowed row never leaves Postgres, so the resolver never decrypts a value it would discard, and the precedence lives in one ORDER BY instead of a merge a later writer can reorder. The value columns land nullable. T2 keeps the value-free `InsertSecret` path alive for its T5 caller, and that path writes no value, so an unconditional NOT NULL here would fail every live `SetSecret` at runtime. T5 tightens them with the cutover. `secrets` stays in the `tenant_tables` RLS array. Scope is an additional filter inside a tenant, never a replacement for row-level security -- the catalog-driven RLS test still reports it enabled and forced. Tests are red-first and mutation-checked: inverting the ORDER BY direction, swapping the case-folding predicate for a byte-exact one, and stubbing out the door validator each turn the suite red, so the precedence, near-miss rejection, and door-validation assertions can all genuinely fail. The battery also covers cross-user and cross-agent isolation, one tenant row resolving for two agents under different owners, and a same-name-different-scope upsert inserting a second row rather than overwriting the first. The two `DeleteSecretDeclaration` callsites in `go/server/secrets_service.go` take the tenant coordinate as a marked T5 placeholder. Whether a user may write a tenant-scoped row is an open authorization question for Matt; T5 owns the write surface once it is ruled. Refs RIG-3655 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…655)
Review findings across T1 and T2. They ship as one commit because the
UserSecretAAD signature change and the T2 callsites that must follow it cannot
be separated without leaving a commit that does not compile.
The master key leaked through fmt and slog. The type's claim that unexported
bytes made it unloggable was false: fmt reads unexported fields reflectively, so
`%v` rendered the key as decimal bytes and slog's TextHandler logged the whole
32-byte array. Measured before the fix: slog emitted
`masterKey="{k:[171 171 ...]}"`. Key now carries String, GoString and LogValue,
and every vector renders REDACTED. The doc comment now names the real mechanism
-- unexported fields stop encoding/json and external access, the three methods
stop fmt and slog.
The test that was supposed to prove this passed vacuously. It searched for the
HEX needle "abab" while fmt emits decimal for %v, so it matched nothing it was
meant to catch. The needles are now built from the actual renderings and the
matrix covers both slog handlers. Removing the three methods reddens it.
The AAD's injectivity claim was false for a field containing a NUL: an in-field
NUL is indistinguishable from a separator, so
`("t",1,"a","b\x00c",1)` and `("t",1,"a\x00b","c",1)` produced identical bytes.
Matt ruled reject over re-encoding, matching every secret store surveyed -- none
permits a NUL in a name, and POSIX env names cannot contain one at all because
they are C strings. UserSecretAAD now returns an error naming the offending
field, never its value. The byte format is unchanged, which matters because the
AAD is bound into every ciphertext and a format change after rows exist would
force a re-encrypt.
The zero-value Key was a usable all-zero AES key, so a wiring bug that skipped
NewKey would have encrypted every secret under a publicly known key. It now
fails closed with ErrUnsetKey, kept distinct from ErrDecrypt because a missing
key is a wiring fault, not a tamper.
The read-side authorization gate had no test. SecretRecordsForAgent decides
which secrets an agent receives, and its gate for an unknown principal is
entirely the INNER JOIN on agent_accounts -- but every existing test passed a
real agent id, so a regression to LEFT JOIN would have handed every
tenant-scoped secret to an unknown caller with the suite still green. A test now
asserts zero rows for a user account and for a non-existent id; switching the
join to LEFT reddens it.
pgCheckViolation was declared but never referenced, while its comment called it
the backstop for a bypassed door guard. The door did not validate the delivery
range, so an out-of-range value reached the CHECK and surfaced as CodeInternal
instead of the ErrInvalidArgument the method documents. The door now mirrors the
CHECK and the SQLSTATE maps to ErrInvalidArgument. DeclareSecret has the same
gap; it is pre-existing and left alone.
Refs RIG-3655
Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
force-pushed
the
compass-server/rig-3655-t2-store
branch
from
September 12, 2026 02:50
018158d to
3b5c78f
Compare
mattwilkinsonn
approved these changes
Sep 12, 2026
|
This pull request was merged into |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is part of a stack containing 7 PRs:
mainT2 of the user-secret store: the
secretstable stops being a names-onlyregistry and becomes the store of record for scoped, encrypted values.
Scope is three tiers -- 0 tenant, 1 user, 2 agent -- so the primary key becomes
(name, scope_kind, scope_id). A tenant row is a real shared value, and a useror agent row of the same name shadows it.
secrets_scope_shapeenforces that atenant row carries an empty
scope_idand a user/agent row does not, so amalformed coordinate cannot reach a row.
scope_idcarries no foreign key: a tenant row's empty string could not satisfyone. The referential check moves to the store door, which resolves the id
against
user_accountsoragent_accountsinside the write transaction.SecretRecordsForAgentcollapses the precedence in SQL --DISTINCT ON (name)under
ORDER BY name, scope_kind DESC, with the user tier reached throughagent_accounts.owner_user_id. Doing it here rather than in Go means a shadowedrow never leaves Postgres, so the resolver never decrypts a value it would
discard, and the precedence lives in one ORDER BY instead of a merge a later
writer can reorder.
The value columns land nullable. T2 keeps the value-free
InsertSecretpathalive for its T5 caller, and that path writes no value, so an unconditional NOT
NULL here would fail every live
SetSecretat runtime. T5 tightens them withthe cutover.
secretsstays in thetenant_tablesRLS array. Scope is an additional filterinside a tenant, never a replacement for row-level security -- the catalog-driven
RLS test still reports it enabled and forced.
Tests are red-first and mutation-checked: inverting the ORDER BY direction,
swapping the case-folding predicate for a byte-exact one, and stubbing out the
door validator each turn the suite red, so the precedence, near-miss rejection,
and door-validation assertions can all genuinely fail. The battery also covers
cross-user and cross-agent isolation, one tenant row resolving for two agents
under different owners, and a same-name-different-scope upsert inserting a
second row rather than overwriting the first.
The two
DeleteSecretDeclarationcallsites ingo/server/secrets_service.gotake the tenant coordinate as a marked T5 placeholder. Whether a user may write
a tenant-scoped row is an open authorization question for Matt; T5 owns the
write surface once it is ruled.
Refs RIG-3655
Co-authored-by: Matt Wilkinson matt@rigel.build