feat(store): scope and encrypt user secrets at rest (RIG-3655 T2)

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,24 +116,43 @@ 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
 	}
 	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:
 //
 //	"compass/user-secret/v1\x00" + tenantID + "\x00" + decimal(scopeKind) +
 //	  "\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
 }

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")
+	}
+}