fix(kms): require explicit env on secret writes (no silent default)

POST /v1/kms/orgs/{org}/secrets defaulted a missing env to "default", committing the write to a bucket that project/env/path readers (the kms-operator, cluster syncs) never resolve. That split landed an IAM z-password in env=default while prod kept serving the stale value. env is a first-class component of the storage key (kms/secrets/{path}/{env}/{name}) and cannot be aliased, so a write with no env now fails loud (400). GET keeps a backward-compatible default (a read can't plant a value another reader trusts; legacy readers that omit env must keep working). The POST handler is extracted to a named func (putSecretHandler) so the contract is unit-testable. Regression tests: write without env -> 400; write env=prod is stored under prod (sha256 round-trip, values never printed) and is not visible in env=default. Claude-Session: https://claude.ai/code/session_01D4FSvT3UfhrFJNQjrctjEj
2026-07-27 03:38:31 +00:00 · 2026-07-08 07:17:47 -07:00
parent ea6d043405
commit 61ec351a1a
2 changed files with 153 additions and 27 deletions
@@ -0,0 +1,107 @@
+// Regression coverage for the one-way env contract on secret writes. env is
+// a first-class component of the storage key (kms/secrets/{path}/{env}/{name});
+// a POST that omits env must fail loud (400) rather than silently landing in a
+// "default" bucket that project/env/path readers (the kms-operator, cluster
+// syncs) never resolve. That split is what let an IAM z-password land in
+// env=default while prod kept serving the stale value.
+//
+// Secret values are never printed — round-trip fidelity is asserted by
+// comparing SHA-256 digests.
+package main
+
+import (
+	"bytes"
+	"crypto/sha256"
+	"encoding/hex"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/luxfi/kms/pkg/store"
+	badger "github.com/luxfi/zapdb"
+)
+
+func newTestSecretStore(t *testing.T) *store.SecretStore {
+	t.Helper()
+	db, err := badger.Open(badger.DefaultOptions(t.TempDir()).WithLogger(nil))
+	if err != nil {
+		t.Fatalf("open zapdb: %v", err)
+	}
+	t.Cleanup(func() { _ = db.Close() })
+	return store.NewSecretStore(db)
+}
+
+func sha256hex(s string) string {
+	sum := sha256.Sum256([]byte(s))
+	return hex.EncodeToString(sum[:])
+}
+
+func postJSON(t *testing.T, url string, body map[string]string) *http.Response {
+	t.Helper()
+	b, _ := json.Marshal(body)
+	resp, err := http.Post(url, "application/json", bytes.NewReader(b))
+	if err != nil {
+		t.Fatalf("post: %v", err)
+	}
+	return resp
+}
+
+// A write that omits env fails loud (400) and lands nowhere.
+func TestPutSecret_WithoutEnv_400(t *testing.T) {
+	secStore := newTestSecretStore(t)
+	srv := httptest.NewServer(putSecretHandler(secStore))
+	defer srv.Close()
+
+	resp := postJSON(t, srv.URL, map[string]string{
+		"path": "iam-passwords", "name": "Z_PASSWORD", "value": "irrelevant",
+	})
+	defer resp.Body.Close()
+	if resp.StatusCode != http.StatusBadRequest {
+		t.Fatalf("omitted-env write: want 400, got %d", resp.StatusCode)
+	}
+	var m map[string]any
+	_ = json.NewDecoder(resp.Body).Decode(&m)
+	if msg, _ := m["message"].(string); !strings.Contains(msg, "env is required") {
+		t.Fatalf("want env-required message, got %q", msg)
+	}
+
+	// The rejected write must not have populated the default bucket.
+	if _, err := secStore.Get("iam-passwords", "Z_PASSWORD", "default"); err == nil {
+		t.Fatal("omitted-env write must not land in env=default, but a record exists there")
+	}
+}
+
+// A write with an explicit env is readable through the exact project/env/path
+// resolution the kms-operator uses, and is not visible in any other bucket.
+func TestPutSecret_WithEnvProd_StoredUnderProd(t *testing.T) {
+	secStore := newTestSecretStore(t)
+	srv := httptest.NewServer(putSecretHandler(secStore))
+	defer srv.Close()
+
+	const secret = "z-password-98f3-do-not-log"
+	want := sha256hex(secret)
+
+	resp := postJSON(t, srv.URL, map[string]string{
+		"path": "iam-passwords", "name": "Z_PASSWORD", "env": "prod", "value": secret,
+	})
+	defer resp.Body.Close()
+	if resp.StatusCode != http.StatusCreated {
+		t.Fatalf("explicit-env write: want 201, got %d", resp.StatusCode)
+	}
+
+	// Operator resolution: env=prod, path=/iam-passwords, name=Z_PASSWORD.
+	got, err := secStore.Get("iam-passwords", "Z_PASSWORD", "prod")
+	if err != nil {
+		t.Fatalf("project/env/path read (prod): %v", err)
+	}
+	if h := sha256hex(string(got.Ciphertext)); h != want {
+		t.Fatalf("round-trip value digest mismatch: got %s want %s", h, want)
+	}
+
+	// No cross-bucket bleed: env=default must not resolve the prod write.
+	if _, err := secStore.Get("iam-passwords", "Z_PASSWORD", "default"); err == nil {
+		t.Fatal("prod write must not be visible in env=default")
+	}
+}
@@ -271,33 +271,10 @@ func main() {
 		})
 	}))

-	// POST /v1/kms/orgs/{org}/secrets — create a secret.
-	mux.HandleFunc("POST /v1/kms/orgs/{org}/secrets", auth.requireOrgJWT(func(w http.ResponseWriter, r *http.Request) {
-		var req struct {
-			Path  string `json:"path"`
-			Name  string `json:"name"`
-			Env   string `json:"env"`
-			Value string `json:"value"`
-		}
-		if err := json.NewDecoder(r.Body).Decode(&req); err != nil || req.Name == "" {
-			writeJSON(w, http.StatusBadRequest, map[string]any{"message": "name and value required"})
-			return
-		}
-		if req.Env == "" {
-			req.Env = "default"
-		}
-		sec := &store.Secret{
-			Name:       req.Name,
-			Path:       req.Path,
-			Env:        req.Env,
-			Ciphertext: []byte(req.Value),
-		}
-		if err := secStore.Put(sec); err != nil {
-			writeJSON(w, http.StatusInternalServerError, map[string]any{"message": err.Error()})
-			return
-		}
-		writeJSON(w, http.StatusCreated, map[string]any{"ok": true})
-	}))
+	// POST /v1/kms/orgs/{org}/secrets — create a secret. The handler is a
+	// named func (putSecretHandler) so the env-required contract is unit
+	// testable without standing up the full server.
+	mux.HandleFunc("POST /v1/kms/orgs/{org}/secrets", auth.requireOrgJWT(putSecretHandler(secStore)))

 	// DELETE /v1/kms/orgs/{org}/secrets/{rest...}/{name}
 	mux.HandleFunc("DELETE /v1/kms/orgs/{org}/secrets/{rest...}", auth.requireOrgJWT(func(w http.ResponseWriter, r *http.Request) {
@@ -502,6 +479,48 @@ func main() {
 	srv.Shutdown(ctx)
 }

+// putSecretHandler serves POST /v1/kms/orgs/{org}/secrets (create/upsert).
+//
+// env is a first-class component of the storage key
+// (kms/secrets/{path}/{env}/{name}); it can never be aliased. A silent
+// "default" would commit the write to a bucket that project/env/path readers
+// (the kms-operator, cluster syncs) never resolve — the exact split that let
+// an IAM z-password land in env=default while prod kept serving the stale
+// value. So a write with no env fails loud (400). Reads (GET) keep a
+// backward-compatible default: a read cannot plant a value another reader
+// later trusts, and legacy readers that omit env must keep working.
+func putSecretHandler(secStore *store.SecretStore) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		var req struct {
+			Path  string `json:"path"`
+			Name  string `json:"name"`
+			Env   string `json:"env"`
+			Value string `json:"value"`
+		}
+		if err := json.NewDecoder(r.Body).Decode(&req); err != nil || req.Name == "" {
+			writeJSON(w, http.StatusBadRequest, map[string]any{"message": "name and value required"})
+			return
+		}
+		if strings.TrimSpace(req.Env) == "" {
+			writeJSON(w, http.StatusBadRequest, map[string]any{
+				"message": `env is required — set "env" in the request body; there is no default. A silent default would split this write from the project/env/path record readers resolve.`,
+			})
+			return
+		}
+		sec := &store.Secret{
+			Name:       req.Name,
+			Path:       req.Path,
+			Env:        req.Env,
+			Ciphertext: []byte(req.Value),
+		}
+		if err := secStore.Put(sec); err != nil {
+			writeJSON(w, http.StatusInternalServerError, map[string]any{"message": err.Error()})
+			return
+		}
+		writeJSON(w, http.StatusCreated, map[string]any{"ok": true})
+	}
+}
+
 // registerStubKMSRoutes installs 503-only handlers for every /v1/kms/keys/*
 // route. Used when the boot-time MPC client could not be constructed at
 // all (e.g. dial timed out, ZAP node init failed). Without this, GET