diff --git a/go/internal/envelope/envelope.go b/go/internal/envelope/envelope.go index bbefe4c04..bc7dea835 100644 --- a/go/internal/envelope/envelope.go +++ b/go/internal/envelope/envelope.go @@ -1,7 +1,9 @@ // Package envelope is the at-rest crypto seam for user-provided secrets: // AES-256-GCM under a single master key, with the nonce generated internally // per encryption so reuse is structurally impossible. The key bytes are held -// unexported so no reflection-based logger or marshaler can reach them. +// unexported so encoding/json and external packages cannot reach them; fmt and +// slog read unexported fields reflectively, so Key.String/GoString/LogValue are +// what stop those two vectors. package envelope import ( @@ -12,7 +14,9 @@ import ( "errors" "fmt" "io" + "log/slog" "strconv" + "strings" ) // keyLen is the AES-256 key size and the only length NewKey accepts. @@ -25,10 +29,19 @@ const nonceLen = 12 // internals and carries no plaintext or key material. var ErrDecrypt = errors.New("envelope: decrypt failed") -// Key is a 256-bit AES-GCM key. The bytes are unexported so no exported field, -// formatter, or marshaler can render them. +// ErrUnsetKey is returned when a zero-value Key is used to encrypt or decrypt. +// It is deliberately NOT ErrDecrypt: a zero Key is a wiring bug (a Key that +// skipped NewKey), not a tamper or wrong-key failure, and conflating it with +// ErrDecrypt would hide the misuse behind the opaque decrypt path. +var ErrUnsetKey = errors.New("envelope: key is unset (must be built with NewKey)") + +// Key is a 256-bit AES-GCM key. The bytes are unexported so encoding/json and +// external packages cannot reach them; String/GoString/LogValue close the fmt +// and slog reflection vectors. set is true only for a NewKey-built Key, so the +// zero value fails closed instead of acting as an all-zero (publicly known) key. type Key struct { - k [keyLen]byte + k [keyLen]byte + set bool } // NewKey copies raw (which must be exactly 32 bytes) into a Key. The copy lets @@ -40,9 +53,20 @@ func NewKey(raw []byte) (Key, error) { } var k Key copy(k.k[:], raw) + k.set = true return k, nil } +// String redacts the key for fmt %v/%s and any Stringer consumer. +func (k Key) String() string { return "envelope.Key(REDACTED)" } + +// GoString redacts the key for fmt %#v. +func (k Key) GoString() string { return "envelope.Key(REDACTED)" } + +// LogValue redacts the key for slog. This is the vector slog actually honors: +// without a LogValuer, a JSONHandler renders the struct reflectively. +func (k Key) LogValue() slog.Value { return slog.StringValue("REDACTED") } + // Fingerprint returns the salted SHA-256 digest of the key under salt — the // non-secret server_key_state tripwire value. func (k Key) Fingerprint(salt []byte) []byte { @@ -74,6 +98,11 @@ func (k Key) Encrypt(plaintext, aad []byte) (nonce, ciphertext []byte, err error func (k Key) Decrypt(nonce, ciphertext, aad []byte) ([]byte, error) { gcm, err := k.gcm() if err != nil { + // An unset key is a wiring bug, not a decrypt failure: surface it plainly + // rather than folding it into the opaque ErrDecrypt path. + if errors.Is(err, ErrUnsetKey) { + return nil, err + } return nil, ErrDecrypt } if len(nonce) != gcm.NonceSize() { @@ -87,6 +116,9 @@ func (k Key) Decrypt(nonce, ciphertext, aad []byte) ([]byte, error) { } func (k Key) gcm() (cipher.AEAD, error) { + if !k.set { + return nil, ErrUnsetKey + } block, err := aes.NewCipher(k.k[:]) if err != nil { return nil, err @@ -94,6 +126,12 @@ func (k Key) gcm() (cipher.AEAD, error) { return cipher.NewGCM(block) } +// ErrAADField is returned by UserSecretAAD when a bound field contains a \x00. +// A \x00 is the field separator, so an in-field one shifts a boundary and makes +// the encoding non-injective. The error names the field only — a rejected value +// may carry attacker-controlled bytes and must never reach a log or error string. +var ErrAADField = errors.New("envelope: AAD field contains NUL") + // UserSecretAAD builds the canonical user-secret AAD binding a ciphertext to // its scope tuple, name, tenant, and key generation: // @@ -101,10 +139,20 @@ func (k Key) gcm() (cipher.AEAD, error) { // "\x00" + scopeID + "\x00" + name + "\x00" + decimal(keyVersion) // // Every field is bound unconditionally (a tenant-scoped row passes scopeID="") -// so the field count never varies, and the \x00 separators make the encoding -// injective: no two distinct field tuples concatenate to the same bytes. -func UserSecretAAD(tenantID string, scopeKind int16, scopeID, name string, keyVersion int16) []byte { +// so the field count never varies. The \x00 separators make the encoding +// injective over its accepted domain; that domain — no \x00 in tenantID, +// scopeID, or name — is enforced here rather than assumed from caller grammar. +// The name grammar itself lives in secrets.ValidateName; this boundary only +// rejects the separator byte that would break injectivity. +func UserSecretAAD(tenantID string, scopeKind int16, scopeID, name string, keyVersion int16) ([]byte, error) { const sep = "\x00" + for _, f := range []struct { + name, value string + }{{"tenantID", tenantID}, {"scopeID", scopeID}, {"name", name}} { + if strings.IndexByte(f.value, 0) >= 0 { + return nil, fmt.Errorf("%w: field %s", ErrAADField, f.name) + } + } buf := make([]byte, 0, len("compass/user-secret/v1")+len(tenantID)+len(scopeID)+len(name)+16) buf = append(buf, "compass/user-secret/v1"...) buf = append(buf, sep...) @@ -117,5 +165,5 @@ func UserSecretAAD(tenantID string, scopeKind int16, scopeID, name string, keyVe buf = append(buf, name...) buf = append(buf, sep...) buf = append(buf, strconv.FormatInt(int64(keyVersion), 10)...) - return buf + return buf, nil } diff --git a/go/internal/envelope/envelope_test.go b/go/internal/envelope/envelope_test.go index a93736f52..5a8e283ee 100644 --- a/go/internal/envelope/envelope_test.go +++ b/go/internal/envelope/envelope_test.go @@ -5,6 +5,8 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" + "strconv" "strings" "testing" ) @@ -148,34 +150,44 @@ func TestDecryptWrongLengthNonce(t *testing.T) { } func TestKeyDoesNotLeakBytes(t *testing.T) { - secret := repeat(0xAB) + const b = 0xAB + secret := repeat(b) k := mustKey(t, secret) - needle := fmt.Sprintf("%02x", secret[0]) // "ab" - - // %v / %+v / %#v must not render the key bytes. - for _, s := range []string{ - fmt.Sprintf("%v", k), - fmt.Sprintf("%+v", k), - fmt.Sprintf("%#v", k), - fmt.Sprintf("%s", k), - } { - if bytes.Contains(bytes.ToLower([]byte(s)), []byte(needle+needle)) { - t.Fatalf("formatted Key leaks key bytes: %q", s) + + // Needles built from the ACTUAL renderings, not a guessed hex string: + // fmt emits a byte as decimal for %v/%+v and as 0xNN for %#v. A run of two + // catches the [32]byte array without matching incidental single occurrences. + dec := strconv.Itoa(b) // "171" + decRun := dec + " " + dec // "171 171" + hexRun := "0xab, 0xab" // %#v array element form + rawNeedles := [][]byte{[]byte(decRun), []byte(hexRun), secret} + + assertClean := func(label, out string) { + low := bytes.ToLower([]byte(out)) + for _, n := range rawNeedles { + if bytes.Contains(low, bytes.ToLower(n)) { + t.Fatalf("%s leaks key bytes (needle %q): %q", label, n, out) + } } } + assertClean("%v", fmt.Sprintf("%v", k)) + assertClean("%+v", fmt.Sprintf("%+v", k)) + assertClean("%#v", fmt.Sprintf("%#v", k)) + assertClean("%s", k.String()) + j, err := json.Marshal(k) //nolint:staticcheck // marshaling a no-exported-field Key to prove it yields no key bytes IS the test if err != nil { t.Fatalf("json.Marshal: %v", err) } - if bytes.Contains(bytes.ToLower(j), []byte(needle)) { - t.Fatalf("json.Marshal(Key) leaks key bytes: %s", j) - } - // The bytes must not appear as a base64/array either: marshaling an all-0xAB - // key should not embed a run of the raw value in any form. - if bytes.Contains(j, secret) { - t.Fatalf("json.Marshal(Key) embeds raw key: %s", j) - } + assertClean("json.Marshal", string(j)) + + // slog is the vector the type must close: both handlers, key as an attr value. + var textBuf, jsonBuf bytes.Buffer + slog.New(slog.NewTextHandler(&textBuf, nil)).Info("m", "master_key", k) + slog.New(slog.NewJSONHandler(&jsonBuf, nil)).Info("m", "master_key", k) + assertClean("slog TextHandler", textBuf.String()) + assertClean("slog JSONHandler", jsonBuf.String()) } func TestFingerprintStableAndDistinct(t *testing.T) { @@ -202,26 +214,36 @@ func TestFingerprintStableAndDistinct(t *testing.T) { } } +// mustAAD builds an AAD the encoding must accept; a NUL-free tuple never errors. +func mustAAD(t *testing.T, tenantID string, scopeKind int16, scopeID, name string, keyVersion int16) []byte { + t.Helper() + aad, err := UserSecretAAD(tenantID, scopeKind, scopeID, name, keyVersion) + if err != nil { + t.Fatalf("UserSecretAAD(%q,%d,%q,%q,%d): %v", tenantID, scopeKind, scopeID, name, keyVersion, err) + } + return aad +} + func TestUserSecretAADInjective(t *testing.T) { - // Adjacent-field ambiguity: without the \x00 separators, moving the "\x00B" + // Adjacent-field ambiguity: without the \x00 separators, moving the "B" // from the name into the scopeID boundary would collide. - a := UserSecretAAD("tenant", 1, "", "A\x00B", 1) - b := UserSecretAAD("tenant", 1, "B", "A", 1) + a := mustAAD(t, "tenant", 1, "", "AB", 1) + b := mustAAD(t, "tenant", 1, "B", "A", 1) if bytes.Equal(a, b) { t.Fatal("UserSecretAAD not injective across name/scopeID field boundary") } // Numeric run-together: a trailing-digit name plus keyVersion must not // concatenate into the same bytes as a shorter name and a longer version. - c := UserSecretAAD("t", 0, "", "KEY1", 2) - d := UserSecretAAD("t", 0, "", "KEY", 12) + c := mustAAD(t, "t", 0, "", "KEY1", 2) + d := mustAAD(t, "t", 0, "", "KEY", 12) if bytes.Equal(c, d) { t.Fatal("UserSecretAAD not injective across name/keyVersion digit boundary") } // scopeKind is bound: same everything else, different scope kind differs. - e := UserSecretAAD("t", 1, "acct", "N", 1) - f := UserSecretAAD("t", 2, "acct", "N", 1) + e := mustAAD(t, "t", 1, "acct", "N", 1) + f := mustAAD(t, "t", 2, "acct", "N", 1) if bytes.Equal(e, f) { t.Fatal("UserSecretAAD does not bind scopeKind") } @@ -237,8 +259,8 @@ func TestScopeBinding(t *testing.T) { k := mustKey(t, repeat(0x33)) // User scope shadows tenant scope for the same name/tenant. An AAD that // differs in ONLY the scope field must fail to decrypt. - aadUser := UserSecretAAD("tenant-x", 1, "acct-1", "OPENAI_API_KEY", 1) - aadAgent := UserSecretAAD("tenant-x", 2, "acct-1", "OPENAI_API_KEY", 1) + aadUser := mustAAD(t, "tenant-x", 1, "acct-1", "OPENAI_API_KEY", 1) + aadAgent := mustAAD(t, "tenant-x", 2, "acct-1", "OPENAI_API_KEY", 1) nonce, ct, err := k.Encrypt([]byte("sk-live"), aadUser) if err != nil { @@ -252,3 +274,76 @@ func TestScopeBinding(t *testing.T) { t.Fatalf("same-AAD round-trip failed: pt=%q err=%v", pt, err) } } + +func TestZeroValueKeyFailsClosed(t *testing.T) { + var zero Key // never through NewKey: 32 zero bytes would be a publicly-known key + aad := []byte("aad") + + if _, _, err := zero.Encrypt([]byte("secret"), aad); !errors.Is(err, ErrUnsetKey) { + t.Fatalf("zero-value Encrypt: want ErrUnsetKey, got %v", err) + } + // An unset key is a wiring bug, not a tamper: it must NOT masquerade as ErrDecrypt. + if _, err := zero.Decrypt(make([]byte, nonceLen), []byte("ct"), aad); !errors.Is(err, ErrUnsetKey) { + t.Fatalf("zero-value Decrypt: want ErrUnsetKey, got %v", err) + } + if errors.Is(ErrUnsetKey, ErrDecrypt) { + t.Fatal("ErrUnsetKey must be distinct from ErrDecrypt") + } + + // A NewKey-built key still works end to end. + k := mustKey(t, repeat(0x5A)) + nonce, ct, err := k.Encrypt([]byte("secret"), aad) + if err != nil { + t.Fatalf("NewKey Encrypt: %v", err) + } + if pt, err := k.Decrypt(nonce, ct, aad); err != nil || string(pt) != "secret" { + t.Fatalf("NewKey round-trip: pt=%q err=%v", pt, err) + } +} + +func TestUserSecretAADRejectsNUL(t *testing.T) { + // The \x00 separator makes an in-field \x00 a boundary shifter, so the + // boundary must refuse it rather than emit a colliding AAD. + for _, tc := range []struct { + field string + tenantID, scopeID, name string + }{ + {"tenantID", "t\x00x", "s", "N"}, + {"scopeID", "t", "s\x00y", "N"}, + {"name", "t", "s", "N\x00M"}, + } { + aad, err := UserSecretAAD(tc.tenantID, 1, tc.scopeID, tc.name, 1) + if aad != nil { + t.Errorf("%s: want nil AAD on NUL, got %q", tc.field, aad) + } + if !errors.Is(err, ErrAADField) { + t.Fatalf("%s: want ErrAADField, got %v", tc.field, err) + } + if !strings.Contains(err.Error(), tc.field) { + t.Errorf("%s: error must name the field, got %q", tc.field, err) + } + // The rejected value carries attacker-controlled bytes: it must never + // appear in the error string. + for _, v := range []string{tc.tenantID, tc.scopeID, tc.name} { + if strings.Contains(v, "\x00") && strings.Contains(err.Error(), v) { + t.Errorf("%s: error leaked the offending value %q", tc.field, err) + } + } + } + + // The exact reproduced collision pair is now refused rather than equal: + // a NUL in name vs. a NUL in scopeID both error instead of colliding. + if _, err := UserSecretAAD("t", 1, "a", "b\x00c", 1); !errors.Is(err, ErrAADField) { + t.Fatalf("collision pair (name NUL): want ErrAADField, got %v", err) + } + if _, err := UserSecretAAD("t", 1, "a\x00b", "c", 1); !errors.Is(err, ErrAADField) { + t.Fatalf("collision pair (scopeID NUL): want ErrAADField, got %v", err) + } + + // The accepted (NUL-free) domain stays injective: distinct tuples differ. + c := mustAAD(t, "tenant", 1, "a1b2c3", "OPENAI_API_KEY", 1) + d := mustAAD(t, "tenant", 1, "a1b2", "c3OPENAI_API_KEY", 1) + if bytes.Equal(c, d) { + t.Fatal("distinct NUL-free tuples must not collide") + } +} diff --git a/go/internal/store/db/models.go b/go/internal/store/db/models.go index 9f1f2b870..ead7b0858 100644 --- a/go/internal/store/db/models.go +++ b/go/internal/store/db/models.go @@ -270,15 +270,20 @@ type OwedMention struct { } type Secret struct { - Name string - Delivery int16 - Kind int16 - Provider string - Host string - DeclaredBy string - CreatedAt pgtype.Timestamptz - UpdatedAt pgtype.Timestamptz - TenantID string + Name string + ScopeKind int16 + ScopeID string + Delivery int16 + Kind int16 + Provider string + Host string + ValueCiphertext []byte + ValueNonce []byte + KeyVersion int16 + DeclaredBy string + CreatedAt pgtype.Timestamptz + UpdatedAt pgtype.Timestamptz + TenantID string } type ServerKeyState struct { diff --git a/go/internal/store/db/querier.go b/go/internal/store/db/querier.go index 541a68f2f..e9cb2239d 100644 --- a/go/internal/store/db/querier.go +++ b/go/internal/store/db/querier.go @@ -86,6 +86,8 @@ type Querier interface { // row (singleton = TRUE) with a monotonic version supplying the CAS substrate: // a write only lands if the row still holds the version the caller read. CurrentModelRegistry(ctx context.Context) (CurrentModelRegistryRow, error) + // DeclaredSecrets is the names-only view the SERVER SpecResolver's declarations + // interface still consumes (value-free, all scopes). DeclaredSecrets(ctx context.Context) ([]DeclaredSecretsRow, error) DeclaredServerSecrets(ctx context.Context) ([]ServerSecret, error) DeleteAgentConfig(ctx context.Context) error @@ -97,7 +99,9 @@ type Querier interface { DeleteChannelPin(ctx context.Context, arg DeleteChannelPinParams) error DeleteChannelPinReturningPosition(ctx context.Context, arg DeleteChannelPinReturningPositionParams) (int32, error) DeleteModelRegistry(ctx context.Context) error - DeleteSecret(ctx context.Context, name string) (int64, error) + // DeleteSecret addresses one scope coordinate — a name alone no longer + // identifies a row (composite PK). + DeleteSecret(ctx context.Context, arg DeleteSecretParams) (int64, error) DeleteServerSecret(ctx context.Context, name string) (int64, error) DeleteSessionBinding(ctx context.Context, sessionID string) error // The reconnect sweep. Hub.enroll (internal/runnerhub/hub.go:905-957) clears @@ -248,12 +252,17 @@ type Querier interface { // RETURNING) rather than clobbering the winner. The seeded version is 1. InsertModelRegistry(ctx context.Context, registry []byte) (int64, error) InsertOwnerDMGroup(ctx context.Context, arg InsertOwnerDMGroupParams) error - // Secrets-registry queries (sqlc adoption T6, RIG-3034). These replace the inline - // SQL literals in internal/store/secrets.go; the hand-written Store methods keep - // their signatures, the door-side validation (name grammar, kind routing), the - // ErrConflict/ErrInvalidArgument/ErrNotFound mapping, and the RowsAffected branch - // (DeleteSecretDeclaration is :execrows). DeclaredSecrets maps the generated row - // back to the domain SecretDeclaration (delivery/kind ints -> named types). + // Secrets-registry queries (sqlc adoption T6, RIG-3034). These back the + // hand-written Store methods, which keep their signatures, the door-side + // validation (name grammar, kind/routing, A9 scope shape), the + // ErrConflict/ErrInvalidArgument/ErrNotFound mapping, and the RowsAffected + // branch (DeleteSecretDeclaration is :execrows). + // + // InsertSecret/DeclaredSecrets are the retained value-free path (T5 caller); the + // scoped, encrypted path is UpsertSecret + SecretRecordsForAgent (A1/A9). + // InsertSecret writes the value-free declaration at the tenant coordinate + // (scope_kind 0, empty scope_id); the value columns stay NULL. Retained for the T5 + // SetSecret caller, removed with it in T5. InsertSecret(ctx context.Context, arg InsertSecretParams) error // Server-secrets registry queries (design record T0, mechanism C1/D6). The // SERVER-owned half of the names-only secret registry, physically separate from @@ -283,6 +292,10 @@ type Querier interface { InsertUserAccount(ctx context.Context, arg InsertUserAccountParams) error IsAgentAccount(ctx context.Context, accountID string) (bool, error) IsEnabledForgeRepo(ctx context.Context, repo string) (bool, error) + // IsUserAccount reports whether an id names a human account — the user-scope + // (scope_kind 1) referential check the UpsertSecret door runs in lieu of an FK + // (A9). The agent-scope check reuses IsAgentAccount. + IsUserAccount(ctx context.Context, accountID string) (bool, error) LatestCheckpointSeq(ctx context.Context, sessionID string) (int64, error) LatestSessionForAccount(ctx context.Context, agentAccountID string) (string, error) LinearAgentSession(ctx context.Context, linearSessionID string) (LinearAgentSessionRow, error) @@ -485,6 +498,11 @@ type Querier interface { // (tenants, 0001_init.sql), so sqlc compiles it against the real schema. ScaffoldGetTenant(ctx context.Context, id string) (Tenant, error) SearchMessages(ctx context.Context, arg SearchMessagesParams) ([]SearchMessagesRow, error) + // SecretRecordsForAgent collapses the A9 precedence in SQL: DISTINCT ON keeps the + // first row per name under scope_kind DESC (agent 2 > user 1 > tenant 0), the + // user tier reached through agent_accounts.owner_user_id. $1 is the calling + // agent's account id. Ciphertext only — the store never decrypts. + SecretRecordsForAgent(ctx context.Context, accountID string) ([]SecretRecordsForAgentRow, error) SeedChannelDeliveryCursors(ctx context.Context, channelID string) error // Delivery-cursor queries (sqlc adoption T4, RIG-3034). These replace the inline // SQL literals in internal/store/delivery_cursors.go; the hand-written Store @@ -586,6 +604,11 @@ type Querier interface { // generated row (nullable linear_issue_id, created_at timestamp) back to the // domain LinearAgentSessionRow inline. UpsertLinearAgentSession(ctx context.Context, arg UpsertLinearAgentSessionParams) (int64, error) + // UpsertSecret writes declaration+value in one row and, on a re-write of an + // existing (name, scope_kind, scope_id), rewrites value/nonce/key_version and the + // routing metadata. updated_at is maintained by the set_updated_at trigger, which + // fires on the ON CONFLICT DO UPDATE path — never set here. + UpsertSecret(ctx context.Context, arg UpsertSecretParams) error } var _ Querier = (*Queries)(nil) diff --git a/go/internal/store/db/secrets.sql.go b/go/internal/store/db/secrets.sql.go index 1b2326c01..89ce7196d 100644 --- a/go/internal/store/db/secrets.sql.go +++ b/go/internal/store/db/secrets.sql.go @@ -27,6 +27,8 @@ type DeclaredSecretsRow struct { UpdatedAt pgtype.Timestamptz } +// DeclaredSecrets is the names-only view the SERVER SpecResolver's declarations +// interface still consumes (value-free, all scopes). func (q *Queries) DeclaredSecrets(ctx context.Context) ([]DeclaredSecretsRow, error) { rows, err := q.db.Query(ctx, declaredSecrets) if err != nil { @@ -57,11 +59,19 @@ func (q *Queries) DeclaredSecrets(ctx context.Context) ([]DeclaredSecretsRow, er } const deleteSecret = `-- name: DeleteSecret :execrows -DELETE FROM secrets WHERE name = $1 +DELETE FROM secrets WHERE name = $1 AND scope_kind = $2 AND scope_id = $3 ` -func (q *Queries) DeleteSecret(ctx context.Context, name string) (int64, error) { - result, err := q.db.Exec(ctx, deleteSecret, name) +type DeleteSecretParams struct { + Name string + ScopeKind int16 + ScopeID string +} + +// DeleteSecret addresses one scope coordinate — a name alone no longer +// identifies a row (composite PK). +func (q *Queries) DeleteSecret(ctx context.Context, arg DeleteSecretParams) (int64, error) { + result, err := q.db.Exec(ctx, deleteSecret, arg.Name, arg.ScopeKind, arg.ScopeID) if err != nil { return 0, err } @@ -70,8 +80,8 @@ func (q *Queries) DeleteSecret(ctx context.Context, name string) (int64, error) const insertSecret = `-- name: InsertSecret :exec -INSERT INTO secrets (name, delivery, kind, provider, host, declared_by) -VALUES ($1, $2, $3, $4, $5, $6) +INSERT INTO secrets (name, scope_kind, delivery, kind, provider, host, declared_by) +VALUES ($1, 0, $2, $3, $4, $5, $6) ` type InsertSecretParams struct { @@ -83,12 +93,17 @@ type InsertSecretParams struct { DeclaredBy string } -// Secrets-registry queries (sqlc adoption T6, RIG-3034). These replace the inline -// SQL literals in internal/store/secrets.go; the hand-written Store methods keep -// their signatures, the door-side validation (name grammar, kind routing), the -// ErrConflict/ErrInvalidArgument/ErrNotFound mapping, and the RowsAffected branch -// (DeleteSecretDeclaration is :execrows). DeclaredSecrets maps the generated row -// back to the domain SecretDeclaration (delivery/kind ints -> named types). +// Secrets-registry queries (sqlc adoption T6, RIG-3034). These back the +// hand-written Store methods, which keep their signatures, the door-side +// validation (name grammar, kind/routing, A9 scope shape), the +// ErrConflict/ErrInvalidArgument/ErrNotFound mapping, and the RowsAffected +// branch (DeleteSecretDeclaration is :execrows). +// +// InsertSecret/DeclaredSecrets are the retained value-free path (T5 caller); the +// scoped, encrypted path is UpsertSecret + SecretRecordsForAgent (A1/A9). +// InsertSecret writes the value-free declaration at the tenant coordinate +// (scope_kind 0, empty scope_id); the value columns stay NULL. Retained for the T5 +// SetSecret caller, removed with it in T5. func (q *Queries) InsertSecret(ctx context.Context, arg InsertSecretParams) error { _, err := q.db.Exec(ctx, insertSecret, arg.Name, @@ -100,3 +115,132 @@ func (q *Queries) InsertSecret(ctx context.Context, arg InsertSecretParams) erro ) return err } + +const isUserAccount = `-- name: IsUserAccount :one +SELECT EXISTS (SELECT 1 FROM user_accounts WHERE account_id = $1) +` + +// IsUserAccount reports whether an id names a human account — the user-scope +// (scope_kind 1) referential check the UpsertSecret door runs in lieu of an FK +// (A9). The agent-scope check reuses IsAgentAccount. +func (q *Queries) IsUserAccount(ctx context.Context, accountID string) (bool, error) { + row := q.db.QueryRow(ctx, isUserAccount, accountID) + var exists bool + err := row.Scan(&exists) + return exists, err +} + +const secretRecordsForAgent = `-- name: SecretRecordsForAgent :many +SELECT DISTINCT ON (s.name) s.name, s.scope_kind, s.scope_id, s.delivery, s.kind, + s.provider, s.host, s.value_ciphertext, s.value_nonce, s.key_version, + s.declared_by, s.created_at, s.updated_at + FROM secrets s + JOIN agent_accounts a ON a.account_id = $1 + WHERE (s.scope_kind = 0 AND s.scope_id = '') + OR (s.scope_kind = 1 AND s.scope_id = a.owner_user_id) + OR (s.scope_kind = 2 AND s.scope_id = a.account_id) + ORDER BY s.name, s.scope_kind DESC +` + +type SecretRecordsForAgentRow struct { + Name string + ScopeKind int16 + ScopeID string + Delivery int16 + Kind int16 + Provider string + Host string + ValueCiphertext []byte + ValueNonce []byte + KeyVersion int16 + DeclaredBy string + CreatedAt pgtype.Timestamptz + UpdatedAt pgtype.Timestamptz +} + +// SecretRecordsForAgent collapses the A9 precedence in SQL: DISTINCT ON keeps the +// first row per name under scope_kind DESC (agent 2 > user 1 > tenant 0), the +// user tier reached through agent_accounts.owner_user_id. $1 is the calling +// agent's account id. Ciphertext only — the store never decrypts. +func (q *Queries) SecretRecordsForAgent(ctx context.Context, accountID string) ([]SecretRecordsForAgentRow, error) { + rows, err := q.db.Query(ctx, secretRecordsForAgent, accountID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []SecretRecordsForAgentRow + for rows.Next() { + var i SecretRecordsForAgentRow + if err := rows.Scan( + &i.Name, + &i.ScopeKind, + &i.ScopeID, + &i.Delivery, + &i.Kind, + &i.Provider, + &i.Host, + &i.ValueCiphertext, + &i.ValueNonce, + &i.KeyVersion, + &i.DeclaredBy, + &i.CreatedAt, + &i.UpdatedAt, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const upsertSecret = `-- name: UpsertSecret :exec +INSERT INTO secrets (name, scope_kind, scope_id, delivery, kind, provider, host, + value_ciphertext, value_nonce, key_version, declared_by) +VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) +ON CONFLICT (name, scope_kind, scope_id) DO UPDATE SET + value_ciphertext = EXCLUDED.value_ciphertext, + value_nonce = EXCLUDED.value_nonce, + key_version = EXCLUDED.key_version, + delivery = EXCLUDED.delivery, + kind = EXCLUDED.kind, + provider = EXCLUDED.provider, + host = EXCLUDED.host +` + +type UpsertSecretParams struct { + Name string + ScopeKind int16 + ScopeID string + Delivery int16 + Kind int16 + Provider string + Host string + ValueCiphertext []byte + ValueNonce []byte + KeyVersion int16 + DeclaredBy string +} + +// UpsertSecret writes declaration+value in one row and, on a re-write of an +// existing (name, scope_kind, scope_id), rewrites value/nonce/key_version and the +// routing metadata. updated_at is maintained by the set_updated_at trigger, which +// fires on the ON CONFLICT DO UPDATE path — never set here. +func (q *Queries) UpsertSecret(ctx context.Context, arg UpsertSecretParams) error { + _, err := q.db.Exec(ctx, upsertSecret, + arg.Name, + arg.ScopeKind, + arg.ScopeID, + arg.Delivery, + arg.Kind, + arg.Provider, + arg.Host, + arg.ValueCiphertext, + arg.ValueNonce, + arg.KeyVersion, + arg.DeclaredBy, + ) + return err +} diff --git a/go/internal/store/errors.go b/go/internal/store/errors.go index 278db8949..869be8cc9 100644 --- a/go/internal/store/errors.go +++ b/go/internal/store/errors.go @@ -5,10 +5,12 @@ import "errors" // Postgres SQLSTATE codes the store maps to its sentinels (pgErrIs). 23505 is a // unique_violation (duplicate handle / channel name / re-used id → ErrConflict); // 23503 is a foreign_key_violation (an input referencing a row that does not -// exist → ErrInvalidArgument). +// exist → ErrInvalidArgument); 23514 is a check_violation (a row failing a table +// CHECK — the defense-in-depth backstop when a door guard is bypassed). const ( pgUniqueViolation = "23505" pgForeignKeyViolation = "23503" + pgCheckViolation = "23514" ) // The store's sentinel errors. Callers (the comms service, the auth layer) diff --git a/go/internal/store/migrations/0001_init.sql b/go/internal/store/migrations/0001_init.sql index 820614be9..0e2b3d90b 100644 --- a/go/internal/store/migrations/0001_init.sql +++ b/go/internal/store/migrations/0001_init.sql @@ -382,18 +382,24 @@ CREATE TABLE tokens ( CREATE INDEX tokens_subject_idx ON tokens (subject_kind, subject_id); --- ── Secrets names registry ────────────────────────────────────────────────── --- The Server-side secrets NAMES registry (RIG-1327 T3): the DECLARED set of --- secrets — their names and how each is delivered/routed — and NOTHING about --- their values. Values live only in the SecretSpec provider (keyring/1Password/ --- Vault/…); the Server resolves them at fetch time and never persists them. --- Deliberately absent and load-bearing: NO value column (encryption-at-rest is --- the provider's job) and NO per-agent grant column (the MVP injects the whole --- store into every agent; per-agent scoping is a named FUTURE seam). +-- ── Secrets registry (scoped, encrypted at rest) ──────────────────────────── +-- The user-secret store: a declared secret's name + routing AND its value, +-- AES-256-GCM-encrypted at rest (design record compass-user-secret-store, A1). +-- Values were formerly provider-held and this table names-only; ruling D1 moved +-- them here, encrypted, so a DB dump/replica/operator SELECT is not a +-- compromise. Rows are scoped tenant/user/agent with most-specific-wins +-- resolution (A9); a tenant row is a real shared value several users resolve, +-- not a placeholder. CREATE TABLE secrets ( -- The secret's name, validated at the store door against SecretSpec's -- env-var-name grammar (^[A-Za-z_][A-Za-z0-9_]*$) before it can reach a row. - name TEXT PRIMARY KEY, + name TEXT NOT NULL, + -- scope_kind: 0 tenant, 1 user, 2 agent. scope_id is '' for a tenant row, + -- else the owning accounts.id. No FK: a tenant row's '' can never satisfy + -- one, and Postgres has no partial FK (A9) — the store door resolves the + -- account instead. + scope_kind SMALLINT NOT NULL CHECK (scope_kind IN (0, 1, 2)), + scope_id TEXT NOT NULL DEFAULT '', -- delivery: the file-vs-env split that determines how a secret rotates -- (0 file, 1 env). CHECK-pinned so a bad value can never reach a row. delivery SMALLINT NOT NULL CHECK (delivery IN (0, 1)), @@ -405,12 +411,24 @@ CREATE TABLE secrets ( provider TEXT NOT NULL DEFAULT '', -- host: the forge host for a gh secret. Empty for non-gh kinds. host TEXT NOT NULL DEFAULT '', + -- value_ciphertext/value_nonce: the AES-256-GCM ciphertext and its fresh + -- 96-bit nonce. NULLABLE in T2 only — the retained value-free + -- InsertSecret/DeclareSecret path writes no value through T5, which then + -- tightens both to NOT NULL once the upsert is the sole writer (A1). + value_ciphertext BYTEA, + value_nonce BYTEA, + -- key_version: which master-key generation encrypted this row (A3, reserved + -- for the deferred rotation record). + key_version SMALLINT NOT NULL DEFAULT 1, -- declared_by: the account that declared this secret. FK ON DELETE RESTRICT -- so a referenced account cannot be orphaned out from under a declaration. declared_by TEXT NOT NULL REFERENCES accounts (id) ON DELETE RESTRICT, created_at TIMESTAMPTZ NOT NULL DEFAULT now(), updated_at TIMESTAMPTZ NOT NULL DEFAULT now(), tenant_id TEXT NOT NULL DEFAULT current_setting('compass.tenant_id', TRUE), + -- (name, scope_kind, scope_id) is the identity: the same name resolves to a + -- different value at each tier, so the value store cannot key on name alone. + PRIMARY KEY (name, scope_kind, scope_id), -- kind↔provider/host invariant, enforced (not merely documented): a provider -- row (kind=1) carries a non-empty provider and no host; a gh row (kind=2) a -- non-empty host and no provider; a generic row (kind=0) neither. Without @@ -421,6 +439,12 @@ CREATE TABLE secrets ( (kind = 0 AND provider = '' AND host = '') OR (kind = 1 AND provider <> '' AND host = '') OR (kind = 2 AND host <> '' AND provider = '') + ), + -- scope↔id shape, mirrored at the UpsertSecret door so a caller gets + -- ErrInvalidArgument: a tenant row carries no id; a user/agent row must. + CONSTRAINT secrets_scope_shape CHECK ( + (scope_kind = 0 AND scope_id = '') + OR (scope_kind IN (1, 2) AND scope_id <> '') ) ); diff --git a/go/internal/store/queries/secrets.sql b/go/internal/store/queries/secrets.sql index bed66e304..b43ed899e 100644 --- a/go/internal/store/queries/secrets.sql +++ b/go/internal/store/queries/secrets.sql @@ -1,17 +1,64 @@ --- Secrets-registry queries (sqlc adoption T6, RIG-3034). These replace the inline --- SQL literals in internal/store/secrets.go; the hand-written Store methods keep --- their signatures, the door-side validation (name grammar, kind routing), the --- ErrConflict/ErrInvalidArgument/ErrNotFound mapping, and the RowsAffected branch --- (DeleteSecretDeclaration is :execrows). DeclaredSecrets maps the generated row --- back to the domain SecretDeclaration (delivery/kind ints -> named types). +-- Secrets-registry queries (sqlc adoption T6, RIG-3034). These back the +-- hand-written Store methods, which keep their signatures, the door-side +-- validation (name grammar, kind/routing, A9 scope shape), the +-- ErrConflict/ErrInvalidArgument/ErrNotFound mapping, and the RowsAffected +-- branch (DeleteSecretDeclaration is :execrows). +-- +-- InsertSecret/DeclaredSecrets are the retained value-free path (T5 caller); the +-- scoped, encrypted path is UpsertSecret + SecretRecordsForAgent (A1/A9). +-- InsertSecret writes the value-free declaration at the tenant coordinate +-- (scope_kind 0, empty scope_id); the value columns stay NULL. Retained for the T5 +-- SetSecret caller, removed with it in T5. -- name: InsertSecret :exec -INSERT INTO secrets (name, delivery, kind, provider, host, declared_by) -VALUES ($1, $2, $3, $4, $5, $6); +INSERT INTO secrets (name, scope_kind, delivery, kind, provider, host, declared_by) +VALUES ($1, 0, $2, $3, $4, $5, $6); +-- IsUserAccount reports whether an id names a human account — the user-scope +-- (scope_kind 1) referential check the UpsertSecret door runs in lieu of an FK +-- (A9). The agent-scope check reuses IsAgentAccount. +-- name: IsUserAccount :one +SELECT EXISTS (SELECT 1 FROM user_accounts WHERE account_id = $1); + +-- UpsertSecret writes declaration+value in one row and, on a re-write of an +-- existing (name, scope_kind, scope_id), rewrites value/nonce/key_version and the +-- routing metadata. updated_at is maintained by the set_updated_at trigger, which +-- fires on the ON CONFLICT DO UPDATE path — never set here. +-- name: UpsertSecret :exec +INSERT INTO secrets (name, scope_kind, scope_id, delivery, kind, provider, host, + value_ciphertext, value_nonce, key_version, declared_by) +VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) +ON CONFLICT (name, scope_kind, scope_id) DO UPDATE SET + value_ciphertext = EXCLUDED.value_ciphertext, + value_nonce = EXCLUDED.value_nonce, + key_version = EXCLUDED.key_version, + delivery = EXCLUDED.delivery, + kind = EXCLUDED.kind, + provider = EXCLUDED.provider, + host = EXCLUDED.host; + +-- DeleteSecret addresses one scope coordinate — a name alone no longer +-- identifies a row (composite PK). -- name: DeleteSecret :execrows -DELETE FROM secrets WHERE name = $1; +DELETE FROM secrets WHERE name = $1 AND scope_kind = $2 AND scope_id = $3; +-- DeclaredSecrets is the names-only view the SERVER SpecResolver's declarations +-- interface still consumes (value-free, all scopes). -- name: DeclaredSecrets :many SELECT name, delivery, kind, provider, host, declared_by, created_at, updated_at FROM secrets ORDER BY name; + +-- SecretRecordsForAgent collapses the A9 precedence in SQL: DISTINCT ON keeps the +-- first row per name under scope_kind DESC (agent 2 > user 1 > tenant 0), the +-- user tier reached through agent_accounts.owner_user_id. $1 is the calling +-- agent's account id. Ciphertext only — the store never decrypts. +-- name: SecretRecordsForAgent :many +SELECT DISTINCT ON (s.name) s.name, s.scope_kind, s.scope_id, s.delivery, s.kind, + s.provider, s.host, s.value_ciphertext, s.value_nonce, s.key_version, + s.declared_by, s.created_at, s.updated_at + FROM secrets s + JOIN agent_accounts a ON a.account_id = $1 + WHERE (s.scope_kind = 0 AND s.scope_id = '') + OR (s.scope_kind = 1 AND s.scope_id = a.owner_user_id) + OR (s.scope_kind = 2 AND s.scope_id = a.account_id) + ORDER BY s.name, s.scope_kind DESC; diff --git a/go/internal/store/secrets.go b/go/internal/store/secrets.go index 88dd4fab4..71b0492f5 100644 --- a/go/internal/store/secrets.go +++ b/go/internal/store/secrets.go @@ -150,25 +150,23 @@ func validateKindRouting(kind SecretKind, provider, host string) error { return nil } -// DeleteSecretDeclaration removes a names-only registry row. Deleting a name -// that was never declared is ErrNotFound, so a caller learns a bad delete -// target rather than silently succeeding (matching RevokeToken's unknown-target -// posture). The provider-side value deletion is a separate write path -// (internal/secrets Resolver.Delete); this only drops the declaration. +// DeleteSecretDeclaration removes the secret row at (name, scopeKind, scopeID). +// Deleting a coordinate that was never declared is ErrNotFound, so a caller +// learns a bad delete target rather than silently succeeding. Deleting the row +// deletes its value with it (declaration and value are the same row post-A1). // -// The registry is a single global namespace (name is the PRIMARY KEY, inject-all -// MVP — no per-declaration owner, the frozen record's D-decisions), so a row is -// keyed by name alone, not (actor, name): any declared name is a legal delete -// target regardless of who declared it. This is contract-correct only under the -// single-user Server MVP (OQ7, Matt-ruled): the secrets table has no user -// dimension, so no other user's declaration exists for a name-keyed delete to -// cross; per-owner scoping (an owner_user_id column + a scoped delete) is the -// named post-MVP seam, not a gap here. actor is carried for the audit trail and -// so the signature matches DeclareSecret; write authorization is user-only and -// enforced at the T7 RPC edge, not re-litigated per row here. -func (s *Store) DeleteSecretDeclaration(ctx context.Context, actor AccountID, name string) error { - _ = actor // see doc: name-keyed global registry; actor is audit context, not a filter - affected, err := s.q.DeleteSecret(ctx, name) +// The scope pair is required because a name alone no longer identifies a row +// (composite PK, A9): the same name may hold a distinct value at tenant, user, +// and agent scope, so a name-keyed delete would be ambiguous. actor is carried +// for the audit trail and so the signature matches the write door; write +// authorization is enforced at the RPC edge, not re-litigated per row here. +func (s *Store) DeleteSecretDeclaration(ctx context.Context, actor AccountID, name string, scopeKind int16, scopeID string) error { + _ = actor // audit context, not a filter — see doc + affected, err := s.q.DeleteSecret(ctx, db.DeleteSecretParams{ + Name: name, + ScopeKind: scopeKind, + ScopeID: scopeID, + }) if err != nil { return fmt.Errorf("store: delete secret declaration: %w", err) } @@ -202,3 +200,177 @@ func (s *Store) DeclaredSecrets(ctx context.Context) ([]SecretDeclaration, error } return out, nil } + +// Secret scope tiers (A9): a secret resolves most-specific-wins, agent > user > +// tenant. The int16 encoding IS the resolution precedence (SecretRecordsForAgent +// orders by scope_kind DESC), so the values are load-bearing, not arbitrary. +const ( + // SecretScopeTenant is a shared value several users resolve; scope_id is "". + SecretScopeTenant int16 = 0 + // SecretScopeUser is owned by a user; scope_id is that user's account id. + SecretScopeUser int16 = 1 + // SecretScopeAgent is owned by an agent; scope_id is that agent's account id. + SecretScopeAgent int16 = 2 +) + +// SecretRecord is a SecretDeclaration plus the at-rest value columns and the +// scope coordinate. It carries CIPHERTEXT only — the store never sees plaintext +// (crypto lives in the envelope/secrets layer). +type SecretRecord struct { + SecretDeclaration + ScopeKind int16 + ScopeID string + ValueCiphertext []byte + ValueNonce []byte + KeyVersion int16 +} + +// validateScopeShape enforces the A9 scope↔id shape at the store door, mirroring +// the secrets_scope_shape CHECK: a tenant row carries no id; a user/agent row +// must. A caller that violates it gets ErrInvalidArgument here rather than a raw +// constraint violation from the write. +func validateScopeShape(scopeKind int16, scopeID string) error { + switch scopeKind { + case SecretScopeTenant: + if scopeID != "" { + return fmt.Errorf("%w: tenant-scoped secret carries no scope id", ErrInvalidArgument) + } + case SecretScopeUser, SecretScopeAgent: + if scopeID == "" { + return fmt.Errorf("%w: user/agent-scoped secret requires a scope id", ErrInvalidArgument) + } + default: + return fmt.Errorf("%w: unknown secret scope kind %d", ErrInvalidArgument, scopeKind) + } + return nil +} + +// validateDelivery enforces the delivery range at the store door, mirroring the +// secrets.delivery CHECK (0=file, 1=env). Without it an out-of-range delivery +// sails past the door and surfaces as a bare wrapped error at the RPC edge +// (CodeInternal) rather than the ErrInvalidArgument UpsertSecret's doc promises. +func validateDelivery(delivery SecretDelivery) error { + switch delivery { + case SecretDeliveryFile, SecretDeliveryEnv: + return nil + default: + return fmt.Errorf("%w: unknown secret delivery %d", ErrInvalidArgument, delivery) + } +} + +// UpsertSecret validates name grammar, the reserved-prefix partition, kind +// routing, and the A9 scope shape at the door, resolves the scope_id against the +// right account subtype in the writing transaction (no FK exists, A9), then +// transactionally upserts declaration+value at (name, scopeKind, scopeID). A +// fresh coordinate inserts; an existing one is a value rewrite. It carries +// CIPHERTEXT — the caller encrypts before this door. +func (s *Store) UpsertSecret(ctx context.Context, actor AccountID, name string, scopeKind int16, scopeID string, delivery SecretDelivery, kind SecretKind, provider, host string, ciphertext, nonce []byte, keyVersion int16) error { + if !secretNamePattern.MatchString(name) { + return fmt.Errorf("%w: secret name %q must match %s", ErrInvalidArgument, name, secretNamePattern.String()) + } + // F1: the user keyspace rejects reserved server-secret prefixes case-fold + // (ShadowsServerSecretPrefix), the wide reject side of the partition. + if ShadowsServerSecretPrefix(name) { + return fmt.Errorf("%w: secret name %q uses a reserved server-secret prefix", ErrInvalidArgument, name) + } + if actor == "" { + return fmt.Errorf("%w: writing account id is required", ErrInvalidArgument) + } + if err := validateKindRouting(kind, provider, host); err != nil { + return err + } + if err := validateScopeShape(scopeKind, scopeID); err != nil { + return err + } + if err := validateDelivery(delivery); err != nil { + return err + } + + tx, err := s.beginTenantTx(ctx) + if err != nil { + return fmt.Errorf("store: begin upsert secret: %w", err) + } + defer func() { _ = tx.Rollback(ctx) }() // no-op after commit; safe on every non-commit path + qtx := s.q.WithTx(tx) + + // Referential integrity for a user/agent scope_id, in lieu of an FK (A9): the + // scope_id must name a real account of the scope's subtype. Resolved in this + // transaction so a concurrent account delete cannot race the write. + switch scopeKind { + case SecretScopeUser: + ok, err := qtx.IsUserAccount(ctx, scopeID) + if err != nil { + return fmt.Errorf("store: resolve user scope: %w", err) + } + if !ok { + return fmt.Errorf("%w: user scope %q is not a user account", ErrInvalidArgument, scopeID) + } + case SecretScopeAgent: + ok, err := qtx.IsAgentAccount(ctx, scopeID) + if err != nil { + return fmt.Errorf("store: resolve agent scope: %w", err) + } + if !ok { + return fmt.Errorf("%w: agent scope %q is not an agent account", ErrInvalidArgument, scopeID) + } + } + + if err := qtx.UpsertSecret(ctx, db.UpsertSecretParams{ + Name: name, + ScopeKind: scopeKind, + ScopeID: scopeID, + Delivery: int16(delivery), //nolint:gosec // G115: SecretDelivery is a CHECK-constrained 0/1 enum, always within int16 + Kind: int16(kind), //nolint:gosec // G115: SecretKind is a CHECK-constrained 0/1/2 enum, always within int16 + Provider: provider, + Host: host, + ValueCiphertext: ciphertext, + ValueNonce: nonce, + KeyVersion: keyVersion, + DeclaredBy: string(actor), + }); err != nil { + if pgErrIs(err, pgForeignKeyViolation) { + return fmt.Errorf("%w: writing account %q does not exist", ErrInvalidArgument, actor) + } + // Backstop the door checks: a CHECK violation (e.g. a delivery/kind out of + // range) is an invalid argument, not an internal fault. + if pgErrIs(err, pgCheckViolation) { + return fmt.Errorf("%w: secret write violates a table constraint", ErrInvalidArgument) + } + return fmt.Errorf("store: upsert secret: %w", err) + } + if err := tx.Commit(ctx); err != nil { + return fmt.Errorf("store: commit upsert secret: %w", err) + } + return nil +} + +// SecretRecordsForAgent returns the one most-specific row per name visible to +// agent (the A9 DISTINCT ON collapse), name-ordered, ciphertext only — the +// StoreResolver.ResolveFor read. Shadowed rows never leave Postgres. +func (s *Store) SecretRecordsForAgent(ctx context.Context, agent AccountID) ([]SecretRecord, error) { + rows, err := s.q.SecretRecordsForAgent(ctx, string(agent)) + if err != nil { + return nil, fmt.Errorf("store: secret records for agent: %w", err) + } + out := make([]SecretRecord, 0, len(rows)) + for _, r := range rows { + out = append(out, SecretRecord{ + SecretDeclaration: SecretDeclaration{ + Name: r.Name, + Delivery: SecretDelivery(r.Delivery), + Kind: SecretKind(r.Kind), + Provider: r.Provider, + Host: r.Host, + DeclaredBy: AccountID(r.DeclaredBy), + CreatedAt: r.CreatedAt.Time, + UpdatedAt: r.UpdatedAt.Time, + }, + ScopeKind: r.ScopeKind, + ScopeID: r.ScopeID, + ValueCiphertext: r.ValueCiphertext, + ValueNonce: r.ValueNonce, + KeyVersion: r.KeyVersion, + }) + } + return out, nil +} diff --git a/go/internal/store/secrets_scope_pgtest_test.go b/go/internal/store/secrets_scope_pgtest_test.go new file mode 100644 index 000000000..1ae932856 --- /dev/null +++ b/go/internal/store/secrets_scope_pgtest_test.go @@ -0,0 +1,461 @@ +//go:build pgtest && unix + +// T2 of the compass-user-secret-store record: the scoped, encrypted user-secret +// store. These prove the A9 scope model (tenant/user/agent, most-specific-wins) +// and the A1 value columns against real Postgres — the composite-PK resolution +// collapse, cross-scope isolation, tenant sharing, the upsert value-rewrite vs +// second-scope-insert distinction, the door-side scope-shape + reserved-prefix +// validation (and that the CHECK backs the door if bypassed), scope-addressed +// delete, and that a stored ciphertext never contains its plaintext. + +package store + +import ( + "bytes" + "context" + "testing" + + "github.com/RigelBuild/compass/go/internal/envelope" +) + +// testKey is a fixed 32-byte AES key for the ciphertext round-trips. The value +// is irrelevant — only that Encrypt/Decrypt use the same one. +func testKey(t *testing.T) envelope.Key { + t.Helper() + raw := make([]byte, 32) + for i := range raw { + raw[i] = byte(i) + } + k, err := envelope.NewKey(raw) + if err != nil { + t.Fatalf("NewKey: %v", err) + } + return k +} + +// upsertValue encrypts value under the row's AAD and upserts it at the given +// scope. tenantID is "" here: the store resolves the bootstrap tenant for the +// row's tenant_id, and the AAD binds whatever the caller passes — the tests only +// need self-consistency between encrypt and the later decrypt, so they bind "". +func upsertValue(t *testing.T, s *Store, key envelope.Key, actor AccountID, name string, scopeKind int16, scopeID, value string) { + t.Helper() + ctx := context.Background() + aad, err := envelope.UserSecretAAD("", scopeKind, scopeID, name, 1) + if err != nil { + t.Fatalf("AAD %s@%d/%s: %v", name, scopeKind, scopeID, err) + } + nonce, ct, err := key.Encrypt([]byte(value), aad) + if err != nil { + t.Fatalf("encrypt %s@%d/%s: %v", name, scopeKind, scopeID, err) + } + if err := s.UpsertSecret(ctx, actor, name, scopeKind, scopeID, + SecretDeliveryEnv, SecretKindGeneric, "", "", ct, nonce, 1); err != nil { + t.Fatalf("UpsertSecret %s@%d/%s: %v", name, scopeKind, scopeID, err) + } +} + +// recordsByName indexes an agent's resolved records by name, failing on a +// duplicate — the DISTINCT ON contract is exactly one row per name. +func recordsByName(t *testing.T, recs []SecretRecord) map[string]SecretRecord { + t.Helper() + out := map[string]SecretRecord{} + for _, r := range recs { + if _, dup := out[r.Name]; dup { + t.Fatalf("SecretRecordsForAgent returned two rows for name %q — DISTINCT ON broken", r.Name) + } + out[r.Name] = r + } + return out +} + +// TestSecretScopePrecedence proves the A9 most-specific-wins collapse: a name at +// all three tiers resolves to the agent row; at tenant+user to the user row; +// tenant-only to the tenant row — exactly one row per name for the caller. +func TestSecretScopePrecedence(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + key := testKey(t) + owner := mustUser(t, s, "owner") + agent := mustAgent(t, s, owner.ID, "agent") + + // THREE: present at tenant, user, and agent scope. + upsertValue(t, s, key, owner.ID, "THREE", SecretScopeTenant, "", "tenant-three") + upsertValue(t, s, key, owner.ID, "THREE", SecretScopeUser, string(owner.ID), "user-three") + upsertValue(t, s, key, agent.ID, "THREE", SecretScopeAgent, string(agent.ID), "agent-three") + // TWO: tenant + user only. + upsertValue(t, s, key, owner.ID, "TWO", SecretScopeTenant, "", "tenant-two") + upsertValue(t, s, key, owner.ID, "TWO", SecretScopeUser, string(owner.ID), "user-two") + // ONE: tenant only. + upsertValue(t, s, key, owner.ID, "ONE", SecretScopeTenant, "", "tenant-one") + + recs, err := s.SecretRecordsForAgent(ctx, agent.ID) + if err != nil { + t.Fatalf("SecretRecordsForAgent: %v", err) + } + byName := recordsByName(t, recs) + if len(byName) != 3 { + t.Fatalf("resolved %d names, want 3: %+v", len(byName), byName) + } + + // Decrypt each winner under its own scope AAD and check it is the expected + // tier — a wrong tier would either decrypt to the wrong value or fail AAD. + for name, want := range map[string]struct { + scope int16 + id string + value string + }{ + "THREE": {SecretScopeAgent, string(agent.ID), "agent-three"}, + "TWO": {SecretScopeUser, string(owner.ID), "user-two"}, + "ONE": {SecretScopeTenant, "", "tenant-one"}, + } { + r := byName[name] + if r.ScopeKind != want.scope || r.ScopeID != want.id { + t.Errorf("%s: resolved scope (%d,%q), want (%d,%q)", name, r.ScopeKind, r.ScopeID, want.scope, want.id) + } + aad, err := envelope.UserSecretAAD("", r.ScopeKind, r.ScopeID, r.Name, r.KeyVersion) + if err != nil { + t.Fatalf("%s: AAD: %v", name, err) + } + pt, err := key.Decrypt(r.ValueNonce, r.ValueCiphertext, aad) + if err != nil { + t.Fatalf("%s: decrypt winner: %v", name, err) + } + if string(pt) != want.value { + t.Errorf("%s: decrypted %q, want %q", name, pt, want.value) + } + } +} + +// TestSecretScopeIsolation proves a user-scoped row of another user and an +// agent-scoped row of another agent are NOT returned to the calling agent. +func TestSecretScopeIsolation(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + key := testKey(t) + owner := mustUser(t, s, "owner") + other := mustUser(t, s, "other") + agent := mustAgent(t, s, owner.ID, "agent") + otherAgent := mustAgent(t, s, other.ID, "other-agent") + + // Rows the calling agent must NOT see: another user's user row and another + // agent's agent row, both under a name the caller has no own row for. + upsertValue(t, s, key, other.ID, "FOREIGN_USER", SecretScopeUser, string(other.ID), "other-user-val") + upsertValue(t, s, key, otherAgent.ID, "FOREIGN_AGENT", SecretScopeAgent, string(otherAgent.ID), "other-agent-val") + // A tenant row the caller SHOULD see, to prove the query returns anything. + upsertValue(t, s, key, owner.ID, "SHARED", SecretScopeTenant, "", "shared-val") + + recs, err := s.SecretRecordsForAgent(ctx, agent.ID) + if err != nil { + t.Fatalf("SecretRecordsForAgent: %v", err) + } + byName := recordsByName(t, recs) + if _, ok := byName["FOREIGN_USER"]; ok { + t.Error("another user's user-scoped row leaked to the calling agent") + } + if _, ok := byName["FOREIGN_AGENT"]; ok { + t.Error("another agent's agent-scoped row leaked to the calling agent") + } + if _, ok := byName["SHARED"]; !ok { + t.Error("tenant-scoped row not resolved for the calling agent") + } +} + +// TestSecretScopeTenantSharing proves the ruling that motivated the whole scope +// model: ONE tenant-scoped row resolves for two different agents under two +// different owning users. +func TestSecretScopeTenantSharing(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + key := testKey(t) + userA := mustUser(t, s, "user-a") + userB := mustUser(t, s, "user-b") + agentA := mustAgent(t, s, userA.ID, "agent-a") + agentB := mustAgent(t, s, userB.ID, "agent-b") + + upsertValue(t, s, key, userA.ID, "SHARED", SecretScopeTenant, "", "one-shared-value") + + for _, ag := range []Account{agentA, agentB} { + recs, err := s.SecretRecordsForAgent(ctx, ag.ID) + if err != nil { + t.Fatalf("SecretRecordsForAgent(%s): %v", ag.ID, err) + } + byName := recordsByName(t, recs) + r, ok := byName["SHARED"] + if !ok { + t.Fatalf("agent %s did not resolve the shared tenant row", ag.ID) + } + if r.ScopeKind != SecretScopeTenant { + t.Errorf("agent %s resolved SHARED at scope %d, want tenant", ag.ID, r.ScopeKind) + } + aad, err := envelope.UserSecretAAD("", r.ScopeKind, r.ScopeID, r.Name, r.KeyVersion) + if err != nil { + t.Fatalf("agent %s: AAD: %v", ag.ID, err) + } + pt, err := key.Decrypt(r.ValueNonce, r.ValueCiphertext, aad) + if err != nil || string(pt) != "one-shared-value" { + t.Errorf("agent %s: shared value = %q,%v, want one-shared-value", ag.ID, pt, err) + } + } +} + +// TestSecretUpsertRewriteVsSecondScope proves the composite-PK contract: the +// same (name, scope) rewrites its value; the SAME name at a DIFFERENT scope +// inserts a second row rather than overwriting the first. +func TestSecretUpsertRewriteVsSecondScope(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + key := testKey(t) + owner := mustUser(t, s, "owner") + agent := mustAgent(t, s, owner.ID, "agent") + + // Fresh tenant insert, then rewrite the same coordinate. + upsertValue(t, s, key, owner.ID, "DUP", SecretScopeTenant, "", "first") + upsertValue(t, s, key, owner.ID, "DUP", SecretScopeTenant, "", "second") + // Same name at agent scope: a distinct row, not an overwrite. + upsertValue(t, s, key, agent.ID, "DUP", SecretScopeAgent, string(agent.ID), "agent-val") + + // Two physical rows for DUP: tenant + agent. + var count int + if err := s.pool.QueryRow(ctx, + `SELECT count(*) FROM secrets WHERE name = 'DUP'`).Scan(&count); err != nil { + t.Fatalf("count DUP rows: %v", err) + } + if count != 2 { + t.Fatalf("DUP has %d rows, want 2 (tenant rewrite + agent insert)", count) + } + + // The tenant row holds "second" (rewrite landed), the agent row "agent-val". + tenantVal := decryptRow(t, s, key, "DUP", SecretScopeTenant, "") + if tenantVal != "second" { + t.Errorf("tenant DUP = %q, want second (rewrite)", tenantVal) + } + agentVal := decryptRow(t, s, key, "DUP", SecretScopeAgent, string(agent.ID)) + if agentVal != "agent-val" { + t.Errorf("agent DUP = %q, want agent-val", agentVal) + } +} + +// decryptRow reads one row's ciphertext straight from the table and decrypts it +// under its scope AAD — the direct read the value-distinction tests assert on. +func decryptRow(t *testing.T, s *Store, key envelope.Key, name string, scopeKind int16, scopeID string) string { + t.Helper() + var ct, nonce []byte + var kv int16 + if err := s.pool.QueryRow(context.Background(), + `SELECT value_ciphertext, value_nonce, key_version FROM secrets + WHERE name = $1 AND scope_kind = $2 AND scope_id = $3`, + name, scopeKind, scopeID).Scan(&ct, &nonce, &kv); err != nil { + t.Fatalf("read row %s@%d/%s: %v", name, scopeKind, scopeID, err) + } + aad, err := envelope.UserSecretAAD("", scopeKind, scopeID, name, kv) + if err != nil { + t.Fatalf("AAD row %s@%d/%s: %v", name, scopeKind, scopeID, err) + } + pt, err := key.Decrypt(nonce, ct, aad) + if err != nil { + t.Fatalf("decrypt row %s@%d/%s: %v", name, scopeKind, scopeID, err) + } + return string(pt) +} + +// TestSecretUpsertScopeShapeDoorValidation proves the store door rejects +// scope-shape violations with ErrInvalidArgument, not a raw Postgres error. +func TestSecretUpsertScopeShapeDoorValidation(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + owner := mustUser(t, s, "owner") + + // Tenant scope with a non-empty scope_id. + err := s.UpsertSecret(ctx, owner.ID, "BAD", SecretScopeTenant, string(owner.ID), + SecretDeliveryEnv, SecretKindGeneric, "", "", []byte("ct"), []byte("nonce"), 1) + sentinelIs(t, err, ErrInvalidArgument, "tenant scope with non-empty id") + + // User scope with an empty scope_id. + err = s.UpsertSecret(ctx, owner.ID, "BAD", SecretScopeUser, "", + SecretDeliveryEnv, SecretKindGeneric, "", "", []byte("ct"), []byte("nonce"), 1) + sentinelIs(t, err, ErrInvalidArgument, "user scope with empty id") +} + +// TestSecretScopeShapeCheckBacksTheDoor proves the CHECK is real, not merely +// mirrored in Go: a direct bad-shape insert is rejected by Postgres. +func TestSecretScopeShapeCheckBacksTheDoor(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + owner := mustUser(t, s, "owner") + + // A tenant row (scope_kind 0) carrying a non-empty scope_id violates + // secrets_scope_shape. declared_by must be a real account (FK), so use owner. + _, err := s.pool.Exec(ctx, + `INSERT INTO secrets (name, scope_kind, scope_id, delivery, kind, declared_by) + VALUES ('DIRECT_BAD', 0, $1, 1, 0, $1)`, string(owner.ID)) + if err == nil { + t.Fatal("direct bad-shape insert succeeded — secrets_scope_shape CHECK not enforcing") + } +} + +// TestSecretUpsertUnknownScopeAccount proves a user/agent write naming a scope_id +// that is not an account of that subtype is rejected at the door. +func TestSecretUpsertUnknownScopeAccount(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + owner := mustUser(t, s, "owner") + agent := mustAgent(t, s, owner.ID, "agent") + + // User scope naming an id that is not a user account (it is an agent). + err := s.UpsertSecret(ctx, owner.ID, "UNK", SecretScopeUser, string(agent.ID), + SecretDeliveryEnv, SecretKindGeneric, "", "", []byte("ct"), []byte("nonce"), 1) + sentinelIs(t, err, ErrInvalidArgument, "user scope naming a non-user account") + + // Agent scope naming an id that is not an agent account (it is a user). + err = s.UpsertSecret(ctx, owner.ID, "UNK", SecretScopeAgent, string(owner.ID), + SecretDeliveryEnv, SecretKindGeneric, "", "", []byte("ct"), []byte("nonce"), 1) + sentinelIs(t, err, ErrInvalidArgument, "agent scope naming a non-agent account") +} + +// TestSecretUpsertReservedPrefixRejected proves the user door rejects a +// SERVER_-prefixed name AND a case-folded near-miss, exercising the case-folding +// ShadowsServerSecretPrefix predicate distinctly from the byte-exact admit check. +func TestSecretUpsertReservedPrefixRejected(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + owner := mustUser(t, s, "owner") + + // Exact-case reserved prefix. + err := s.UpsertSecret(ctx, owner.ID, "SERVER_THING", SecretScopeTenant, "", + SecretDeliveryEnv, SecretKindGeneric, "", "", []byte("ct"), []byte("nonce"), 1) + sentinelIs(t, err, ErrInvalidArgument, "exact reserved prefix") + + // Case-folded near-miss: byte-exact HasServerSecretPrefix would ADMIT this, + // so this row asserts the case-folding reject predicate is genuinely used. + if HasServerSecretPrefix("server_thing") { + t.Fatal("precondition broken: HasServerSecretPrefix should not match lowercase") + } + err = s.UpsertSecret(ctx, owner.ID, "server_thing", SecretScopeTenant, "", + SecretDeliveryEnv, SecretKindGeneric, "", "", []byte("ct"), []byte("nonce"), 1) + sentinelIs(t, err, ErrInvalidArgument, "case-folded reserved prefix near-miss") +} + +// TestSecretDeleteScopeAddressed proves deleting at one scope leaves a +// same-named row at another scope intact. +func TestSecretDeleteScopeAddressed(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + key := testKey(t) + owner := mustUser(t, s, "owner") + agent := mustAgent(t, s, owner.ID, "agent") + + upsertValue(t, s, key, owner.ID, "KEEP", SecretScopeTenant, "", "tenant-keep") + upsertValue(t, s, key, agent.ID, "KEEP", SecretScopeAgent, string(agent.ID), "agent-gone") + + // Delete only the agent-scoped row. + if err := s.DeleteSecretDeclaration(ctx, owner.ID, "KEEP", SecretScopeAgent, string(agent.ID)); err != nil { + t.Fatalf("delete agent-scoped KEEP: %v", err) + } + + // The tenant row survives; the agent row is gone. + if got := decryptRow(t, s, key, "KEEP", SecretScopeTenant, ""); got != "tenant-keep" { + t.Errorf("tenant KEEP after agent delete = %q, want tenant-keep", got) + } + var count int + if err := s.pool.QueryRow(ctx, + `SELECT count(*) FROM secrets WHERE name = 'KEEP' AND scope_kind = $1 AND scope_id = $2`, + SecretScopeAgent, string(agent.ID)).Scan(&count); err != nil { + t.Fatalf("count agent KEEP: %v", err) + } + if count != 0 { + t.Errorf("agent-scoped KEEP still present after scoped delete (count=%d)", count) + } +} + +// TestSecretCiphertextAtRest proves a stored value_ciphertext never contains the +// plaintext bytes — the whole point of encryption at rest. +func TestSecretCiphertextAtRest(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + key := testKey(t) + owner := mustUser(t, s, "owner") + + const plaintext = "super-secret-database-url-value" + upsertValue(t, s, key, owner.ID, "AT_REST", SecretScopeTenant, "", plaintext) + + var ct []byte + if err := s.pool.QueryRow(ctx, + `SELECT value_ciphertext FROM secrets WHERE name = 'AT_REST' AND scope_kind = 0 AND scope_id = ''`). + Scan(&ct); err != nil { + t.Fatalf("read ciphertext: %v", err) + } + if bytes.Contains(ct, []byte(plaintext)) { + t.Fatal("value_ciphertext contains the plaintext bytes — not encrypted at rest") + } + if len(ct) == 0 { + t.Fatal("value_ciphertext is empty") + } +} + +// TestShadowsServerSecretPrefix is a pure predicate unit test (no Postgres, but +// it rides the pgtest build with its neighbors): the case-folding reject side +// matches every case variant while the byte-exact admit side does not. +func TestShadowsServerSecretPrefix(t *testing.T) { + reject := []string{"SERVER_X", "server_x", "SeRvEr_x", "GATEWAY_CREDENTIALS_K", "gateway_credentials_k"} + for _, n := range reject { + if !ShadowsServerSecretPrefix(n) { + t.Errorf("ShadowsServerSecretPrefix(%q) = false, want true", n) + } + } + admit := []string{"PLAIN", "SERVERX_Y", "server", "GATEWAY_CREDENTIAL"} + for _, n := range admit { + if ShadowsServerSecretPrefix(n) { + t.Errorf("ShadowsServerSecretPrefix(%q) = true, want false", n) + } + } + // The two predicates differ: lowercase is admitted byte-exact, rejected fold. + if HasServerSecretPrefix("server_x") || !ShadowsServerSecretPrefix("server_x") { + t.Error("admit/reject predicates should differ on lowercase server_x") + } +} + +// TestSecretUpsertDeliveryRangeDoorValidation proves the door rejects an +// out-of-range delivery with ErrInvalidArgument rather than letting it reach the +// secrets.delivery CHECK and surface as a bare CodeInternal at the RPC edge. +func TestSecretUpsertDeliveryRangeDoorValidation(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + owner := mustUser(t, s, "owner") + + // delivery 2 is outside the file(0)/env(1) range the CHECK enforces. + err := s.UpsertSecret(ctx, owner.ID, "BADDEL", SecretScopeTenant, "", + SecretDelivery(2), SecretKindGeneric, "", "", []byte("ct"), []byte("nonce"), 1) + sentinelIs(t, err, ErrInvalidArgument, "out-of-range delivery") +} + +// TestSecretRecordsForAgentUnknownPrincipal pins the read-side authz gate: the +// INNER JOIN agent_accounts means a non-agent or unknown principal resolves zero +// rows even against a tenant-scoped secret. A regression to LEFT JOIN would hand +// every tenant secret to an unknown principal; this fails on that. +func TestSecretRecordsForAgentUnknownPrincipal(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + key := testKey(t) + owner := mustUser(t, s, "owner") + + // A tenant-scoped row any real agent would resolve — the bait the gate must + // withhold from a principal that is not an agent account. + upsertValue(t, s, key, owner.ID, "SHARED", SecretScopeTenant, "", "tenant-val") + + // A real account that is not an agent (a user), and a wholly unknown id. + for _, tc := range []struct { + name string + principal AccountID + }{ + {"user account is not an agent", owner.ID}, + {"non-existent account id", AccountID("acc_does_not_exist")}, + } { + recs, err := s.SecretRecordsForAgent(ctx, tc.principal) + if err != nil { + t.Fatalf("%s: SecretRecordsForAgent: %v", tc.name, err) + } + if len(recs) != 0 { + t.Errorf("%s: got %d records, want 0 — read-side authz gate leaked", tc.name, len(recs)) + } + } +} diff --git a/go/internal/store/secrets_test.go b/go/internal/store/secrets_test.go index bb80b9310..4098ceb62 100644 --- a/go/internal/store/secrets_test.go +++ b/go/internal/store/secrets_test.go @@ -118,7 +118,7 @@ func TestDeleteSecretDeclaration(t *testing.T) { if err := s.DeclareSecret(ctx, actor.ID, "API_KEY", SecretDeliveryEnv, SecretKindGeneric, "", ""); err != nil { t.Fatalf("declare: %v", err) } - if err := s.DeleteSecretDeclaration(ctx, actor.ID, "API_KEY"); err != nil { + if err := s.DeleteSecretDeclaration(ctx, actor.ID, "API_KEY", SecretScopeTenant, ""); err != nil { t.Fatalf("delete: %v", err) } @@ -136,7 +136,7 @@ func TestDeleteUnknownSecretNotFound(t *testing.T) { s := newTestStore(t) actor := mustUser(t, s, "declarer") - err := s.DeleteSecretDeclaration(ctx, actor.ID, "NEVER_DECLARED") + err := s.DeleteSecretDeclaration(ctx, actor.ID, "NEVER_DECLARED", SecretScopeTenant, "") sentinelIs(t, err, ErrNotFound, "delete unknown secret") } diff --git a/go/internal/store/server_secrets.go b/go/internal/store/server_secrets.go index 9483e59f2..92f3b579c 100644 --- a/go/internal/store/server_secrets.go +++ b/go/internal/store/server_secrets.go @@ -42,6 +42,23 @@ func HasServerSecretPrefix(name string) bool { return false } +// ShadowsServerSecretPrefix reports whether name case-FOLDS onto a reserved +// server-secret prefix — the wide REJECT predicate at the user-secret write/ +// delete door (A2, ported from held PR #1066). It is deliberately distinct from +// HasServerSecretPrefix, the byte-exact ADMIT check at the server door: the +// server door must admit only the canonical uppercase spelling, while the user +// door must reject any case variant so a near-miss like "server_x" or +// "Gateway_Credentials_x" can never mint a user row that shadows the reserved +// keyspace. +func ShadowsServerSecretPrefix(name string) bool { + for _, p := range serverSecretPrefixes { + if len(name) >= len(p) && strings.EqualFold(name[:len(p)], p) { + return true + } + } + return false +} + // ServerSecretDeclaration is a names-only server-secret registry row. It // carries no value (the value lives in the SecretSpec provider) and no // delivery/kind — a server secret is never container-delivered and never diff --git a/go/server/secrets_service.go b/go/server/secrets_service.go index 932343a76..64667089f 100644 --- a/go/server/secrets_service.go +++ b/go/server/secrets_service.go @@ -157,7 +157,9 @@ func (s *secretsService) SetSecret( // name/cli/stderr, never the value, so logging it server-side is safe; the // client-facing error is value-free. if declErr == nil { - if delErr := s.store.DeleteSecretDeclaration(ctx, callerID, msg.GetName()); delErr != nil { + // Tenant coordinate (scope 0, "") is a T5 placeholder: this handler still + // declares at tenant scope pending the write-surface scope ruling (A9 OQ). + if delErr := s.store.DeleteSecretDeclaration(ctx, callerID, msg.GetName(), 0, ""); delErr != nil { slog.ErrorContext(ctx, "rolling back secret declaration after failed write", "err", delErr) } } @@ -233,7 +235,9 @@ func (s *secretsService) DeleteSecret( if err := s.resolver.Delete(ctx, name); err != nil { return nil, connect.NewError(connect.CodeInvalidArgument, fmt.Errorf("deleting secret value: %w", err)) } - if err := s.store.DeleteSecretDeclaration(ctx, callerID, name); err != nil { + // Tenant coordinate (scope 0, "") is a T5 placeholder: this handler deletes at + // tenant scope pending the write-surface scope ruling (A9 OQ). + if err := s.store.DeleteSecretDeclaration(ctx, callerID, name, 0, ""); err != nil { if errors.Is(err, store.ErrNotFound) { return nil, connect.NewError(connect.CodeNotFound, fmt.Errorf("secret %q", name)) }