fix(kms): delete unwrapped /v1/kms/secrets/{name} env-leak route (HIGH/CRITICAL)

The route was registered on the bare mux with NO auth middleware and did os.Getenv(name), returning the raw value. A caller reaching :8080 past the network boundary (NetworkPolicy gap, port-forward, pod compromise, SSRF) could `curl .../v1/kms/secrets/KMS_MASTER_KEY_B64` with no Authorization header and exfiltrate the root REK protecting every per-secret DEK — plus MPC_TOKEN, KMS_ENCRYPTION_KEY_B64, and (k8s/ variant) the S3 backup keys. On the live lux-kms-go/kms statefulset KMS_MASTER_KEY_B64 is populated, so this was live master-key exposure gated only by the network → CRITICAL. Deleted, not gated: zero callers (kms-operator + kms client read via the org-scoped path/ZAP; process env is never a secret source). The three org-scoped secret routes are extracted into registerSecretRoutes so the exact exposed route set is testable in isolation. Secrets now flow ONLY through /v1/kms/orgs/{org}/secrets/* (requireOrgJWT) and the ZAP wire. Regression TestSecretRoutes_NoEnvVarLeak: unauth -> 404, admin -> 404, the process-env canary is never in the body. Corrected the stale kms.go doc comment that referenced the route. Full cmd/kms suite green (CGO_ENABLED=0). Co-authored-by: Hanzo Dev <dev@hanzo.ai>
2026-07-27 03:38:31 +00:00 · 2026-07-21 09:30:20 -07:00
parent 41d2b14198
commit 7116f60fc6
5 changed files with 261 additions and 69 deletions
@@ -3,6 +3,29 @@
 **Project**: Lux Key Management Service (KMS)
 **Organization**: Lux Network

+## v1.12.5 — deleted the env-var fetch route (secrets are never process env)
+
+- **Hole (HIGH, CRITICAL on live `lux-kms-go`):** `GET /v1/kms/secrets/{name}` was
+  registered **unwrapped** — no auth middleware — and did `os.Getenv(name)`, returning
+  the raw value. Any caller reaching `:8080` past the network boundary (NetworkPolicy
+  gap, port-forward, pod compromise, SSRF) could `curl .../v1/kms/secrets/KMS_MASTER_KEY_B64`
+  with **no Authorization header** and read the root REK that protects every per-secret
+  DEK — plus `MPC_TOKEN`, `KMS_ENCRYPTION_KEY_B64`, and (in the `k8s/` variant) the S3
+  backup keys. On the live `lux-kms-go/kms` statefulset `KMS_MASTER_KEY_B64` IS populated
+  (verified: 32-byte key present in `kms-secrets`), so the leak was **live master-key
+  exposure gated only by the network** → CRITICAL.
+- **Fix: deleted the route** (it had **zero callers** — the kms-operator reads via the
+  org-scoped path/ZAP, the `kms` Go client uses ZAP; process env was never a secret
+  source). The three org-scoped secret routes were extracted into `registerSecretRoutes`
+  (mirrors `registerKMSRoutes`/`registerOIDCRoutes`/`registerWebUI`) so the exact exposed
+  route set is testable. Secrets now flow **only** through `/v1/kms/orgs/{org}/secrets/*`
+  (JWT-gated, `requireOrgJWT`) and the ZAP wire. Regression `TestSecretRoutes_NoEnvVarLeak`
+  fails if any route ever echoes process env again (unauth → 404, admin → 404, canary
+  value never in body). Stale `kms.go` doc comment referencing the route corrected.
+- Contrast: the Hanzo white-label fork (`hanzo/kms`) KEPT its `/v1/kms/secrets/{name}`
+  because it has real SDK consumers, and gated it (admin-only + `safeEnvName` + audit,
+  test `TestRed2_EnvVarReadRequiresAdmin`). Lux has no such consumers → delete, don't gate.
+
 ## 2026-07-15 — KMS↔MPC wire fix (v1.12.3) + authorizer-coupling caveat

 - **v1.12.3** (wire-fix commit `7105376`) realigns the KMS↔MPC ZAP signing wire to the mpcd
@@ -1 +1 @@
-1.11.0
+1.12.5
@@ -0,0 +1,154 @@
+// Regression coverage for the deleted env-var fetch route.
+//
+// A prior `GET /v1/kms/secrets/{name}` handler read os.Getenv(name) and
+// returned it UNWRAPPED — registered with no auth middleware — so anyone who
+// reached :8080 past the network boundary (NetworkPolicy gap, port-forward,
+// pod compromise, SSRF) could exfiltrate the KMS process environment: the
+// KMS_MASTER_KEY_B64 root REK that protects every per-secret DEK, MPC_TOKEN,
+// the S3 backup keys, IAM/OIDC client secrets. On the live lux-kms-go pod
+// KMS_MASTER_KEY_B64 is populated, so the leak was CRITICAL, gated only by the
+// network.
+//
+// The route is gone. Secrets are served ONLY through the org-scoped,
+// JWT-gated routes registerSecretRoutes installs (and the ZAP wire). These
+// tests pin that invariant against the REAL production registrar: no path
+// echoes process env, with or without a valid admin bearer. If anyone
+// re-introduces such a route, the leak assertion fails.
+package main
+
+import (
+	"encoding/json"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/luxfi/kms/pkg/store"
+)
+
+// A sentinel that must never appear in any HTTP response body. Installed into
+// the process env under the exact name the deleted route would have echoed.
+const leakSentinel = "MASTER-KEY-LEAK-CANARY-c1a9f0e7-do-not-return"
+
+// TestSecretRoutes_NoEnvVarLeak drives the production secret surface and proves
+// the vestigial env-fetch path leaks nothing — the fix for the HIGH/CRITICAL
+// os.Getenv hole. Fail-closed: unauthenticated AND authenticated-admin both get
+// no env echo.
+func TestSecretRoutes_NoEnvVarLeak(t *testing.T) {
+	// Populate the crown-jewel env var the deleted route echoed. registerSecretRoutes
+	// reads no env at all; the value is here only so a canary would surface if it did.
+	t.Setenv("KMS_MASTER_KEY_B64", leakSentinel)
+
+	auth, bearer, cleanup := newTestKeyAuth(t, roleKMSAdmin)
+	defer cleanup()
+	secStore := newTestSecretStore(t)
+
+	// The SAME registrar main() calls — this is the real route set, not a mock.
+	mux := http.NewServeMux()
+	registerSecretRoutes(mux, auth, secStore)
+
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	// The vestigial env-fetch path, probed the way an attacker on :8080 would.
+	url := srv.URL + "/v1/kms/secrets/KMS_MASTER_KEY_B64"
+
+	cases := []struct {
+		name   string
+		bearer string
+	}{
+		{"unauthenticated", ""},
+		{"authenticated kms-admin", bearer}, // even a valid admin gets no env echo
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			req, err := http.NewRequest(http.MethodGet, url, nil)
+			if err != nil {
+				t.Fatalf("build req: %v", err)
+			}
+			if c.bearer != "" {
+				req.Header.Set("Authorization", "Bearer "+c.bearer)
+			}
+			resp, err := http.DefaultClient.Do(req)
+			if err != nil {
+				t.Fatalf("do: %v", err)
+			}
+			defer resp.Body.Close()
+			body, _ := io.ReadAll(resp.Body)
+
+			// Load-bearing assertion: the process-env value is never returned,
+			// regardless of status code.
+			if strings.Contains(string(body), leakSentinel) {
+				t.Fatalf("LEAK: response body contained the process-env master key (status=%d)", resp.StatusCode)
+			}
+
+			// No handler serves this path anymore → 404. A 200 would mean an
+			// env-echoing route came back.
+			if resp.StatusCode != http.StatusNotFound {
+				t.Fatalf("deleted env route: got status %d, want 404 (no handler)", resp.StatusCode)
+			}
+		})
+	}
+}
+
+// TestSecretRoutes_OrgScopedReadStillWorks proves the extraction into
+// registerSecretRoutes didn't break the real read path, and that a read returns
+// the STORED value — never process env. Guards against a "delete broke the good
+// path" regression.
+func TestSecretRoutes_OrgScopedReadStillWorks(t *testing.T) {
+	auth, bearer, cleanup := newTestKeyAuth(t, roleKMSAdmin)
+	defer cleanup()
+	secStore := newTestSecretStore(t)
+
+	if err := secStore.Put(&store.Secret{
+		Name: "API_KEY", Path: "svc", Env: "prod", Ciphertext: []byte("stored-value-ok"),
+	}); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+
+	mux := http.NewServeMux()
+	registerSecretRoutes(mux, auth, secStore)
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	req, _ := http.NewRequest(http.MethodGet, srv.URL+"/v1/kms/orgs/operator-org/secrets/svc/API_KEY?env=prod", nil)
+	req.Header.Set("Authorization", "Bearer "+bearer)
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		t.Fatalf("do: %v", err)
+	}
+	defer resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("org read: got %d want 200", resp.StatusCode)
+	}
+	var m map[string]map[string]string
+	if err := json.NewDecoder(resp.Body).Decode(&m); err != nil {
+		t.Fatalf("decode: %v", err)
+	}
+	if got := m["secret"]["value"]; got != "stored-value-ok" {
+		t.Fatalf("org read value: got %q want stored-value-ok", got)
+	}
+}
+
+// TestSecretRoutes_UnauthOrgReadRejected confirms the surviving org-scoped read
+// still fails closed without a bearer — the extraction preserved requireOrgJWT.
+func TestSecretRoutes_UnauthOrgReadRejected(t *testing.T) {
+	auth, _, cleanup := newTestKeyAuth(t, roleKMSAdmin)
+	defer cleanup()
+	secStore := newTestSecretStore(t)
+
+	mux := http.NewServeMux()
+	registerSecretRoutes(mux, auth, secStore)
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	resp, err := http.Get(srv.URL + "/v1/kms/orgs/operator-org/secrets/svc/API_KEY?env=prod")
+	if err != nil {
+		t.Fatalf("get: %v", err)
+	}
+	defer resp.Body.Close()
+	if resp.StatusCode != http.StatusUnauthorized {
+		t.Fatalf("unauth org read: got %d want 401", resp.StatusCode)
+	}
+}
@@ -247,71 +247,14 @@ func main() {
 	expectedIss := envOr("KMS_EXPECTED_ISSUER", iamEndpoint)
 	auth := newOrgJWTAuth(jwksFrom, expectedIss)

-	// GET /v1/kms/orgs/{org}/secrets/{path...}/{name}
-	// Matches the ATS kmsclient.Get() URL pattern.
-	mux.HandleFunc("GET /v1/kms/orgs/{org}/secrets/{rest...}", auth.requireOrgJWT(func(w http.ResponseWriter, r *http.Request) {
-		rest := r.PathValue("rest")
-		idx := strings.LastIndex(rest, "/")
-		if idx < 0 {
-			writeJSON(w, http.StatusBadRequest, map[string]any{"message": "path and name required"})
-			return
-		}
-		path, name := rest[:idx], rest[idx+1:]
-		env := r.URL.Query().Get("env")
-		if env == "" {
-			env = "default"
-		}
-		sec, err := secStore.Get(path, name, env)
-		if err != nil {
-			writeJSON(w, http.StatusNotFound, map[string]any{"message": "not found"})
-			return
-		}
-		writeJSON(w, http.StatusOK, map[string]any{
-			"secret": map[string]any{"value": string(sec.Ciphertext)},
-		})
-	}))
-
-	// 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) {
-		rest := r.PathValue("rest")
-		idx := strings.LastIndex(rest, "/")
-		if idx < 0 {
-			writeJSON(w, http.StatusBadRequest, map[string]any{"message": "path and name required"})
-			return
-		}
-		path, name := rest[:idx], rest[idx+1:]
-		env := r.URL.Query().Get("env")
-		if env == "" {
-			env = "default"
-		}
-		if err := secStore.Delete(path, name, env); err != nil {
-			writeJSON(w, http.StatusNotFound, map[string]any{"message": "not found"})
-			return
-		}
-		writeJSON(w, http.StatusOK, map[string]any{"ok": true})
-	}))
-
-	// Legacy: env-backed secret fetch.
-	mux.HandleFunc("GET /v1/kms/secrets/{name}", func(w http.ResponseWriter, r *http.Request) {
-		name := r.PathValue("name")
-		if name == "" {
-			writeJSON(w, http.StatusBadRequest, map[string]any{"message": "secret name required"})
-			return
-		}
-		val := os.Getenv(name)
-		if val == "" {
-			writeJSON(w, http.StatusNotFound, map[string]any{"message": "not found"})
-			return
-		}
-		writeJSON(w, http.StatusOK, map[string]any{
-			"secret": map[string]any{"secretKey": name, "secretValue": val},
-		})
-	})
+	// Secret CRUD surface — org-scoped, JWT-gated, ZapDB-backed. Extracted
+	// into one named registrar (mirrors registerKMSRoutes / registerOIDCRoutes
+	// / registerWebUI) so the exact route set the server exposes is testable
+	// in isolation. There is deliberately NO env-var fetch route: the KMS
+	// process environment (KMS_MASTER_KEY_B64 root REK, MPC_TOKEN, S3 keys) is
+	// never reachable over HTTP. Secrets flow ONLY through these org-scoped
+	// routes and the ZAP wire. Regression: TestSecretRoutes_NoEnvVarLeak.
+	registerSecretRoutes(mux, auth, secStore)

 	// MPC key management (only when MPC_VAULT_ID is set).
 	//
@@ -479,6 +422,78 @@ func main() {
 	srv.Shutdown(ctx)
 }

+// registerSecretRoutes installs the org-scoped secret CRUD surface — the ONLY
+// HTTP way to read or write secrets:
+//
+//	GET    /v1/kms/orgs/{org}/secrets/{path...}/{name}   read a secret value
+//	POST   /v1/kms/orgs/{org}/secrets                    create/upsert (env required)
+//	DELETE /v1/kms/orgs/{org}/secrets/{path...}/{name}   delete a secret
+//
+// Every route is wrapped in auth.requireOrgJWT: an IAM-signed bearer whose
+// owner/name/tag resolves to {org} (or carries the kms-admin role) is
+// mandatory, verified against IAM's JWKS. The values live in ZapDB (encrypted
+// at rest) and are also reachable over the ZAP wire.
+//
+// There is intentionally no `GET /v1/kms/secrets/{name}` env-var route. Such a
+// route once read os.Getenv of an attacker-supplied name and returned it
+// UNWRAPPED — no auth middleware — leaking the KMS process environment
+// (KMS_MASTER_KEY_B64 root REK, MPC_TOKEN, S3 backup keys) to anyone who
+// reached :8080 past the network boundary (NetworkPolicy gap, port-forward,
+// pod compromise, SSRF). It was deleted, not gated: it had zero callers (the
+// kms-operator and in-cluster clients read via the org-scoped path or ZAP) and
+// process env is never a secret-fetch source. Regression that keeps it gone:
+// TestSecretRoutes_NoEnvVarLeak.
+func registerSecretRoutes(mux *http.ServeMux, auth *orgJWTAuth, secStore *store.SecretStore) {
+	// GET /v1/kms/orgs/{org}/secrets/{path...}/{name}
+	// Matches the ATS kmsclient.Get() URL pattern.
+	mux.HandleFunc("GET /v1/kms/orgs/{org}/secrets/{rest...}", auth.requireOrgJWT(func(w http.ResponseWriter, r *http.Request) {
+		rest := r.PathValue("rest")
+		idx := strings.LastIndex(rest, "/")
+		if idx < 0 {
+			writeJSON(w, http.StatusBadRequest, map[string]any{"message": "path and name required"})
+			return
+		}
+		path, name := rest[:idx], rest[idx+1:]
+		env := r.URL.Query().Get("env")
+		if env == "" {
+			env = "default"
+		}
+		sec, err := secStore.Get(path, name, env)
+		if err != nil {
+			writeJSON(w, http.StatusNotFound, map[string]any{"message": "not found"})
+			return
+		}
+		writeJSON(w, http.StatusOK, map[string]any{
+			"secret": map[string]any{"value": string(sec.Ciphertext)},
+		})
+	}))
+
+	// 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) {
+		rest := r.PathValue("rest")
+		idx := strings.LastIndex(rest, "/")
+		if idx < 0 {
+			writeJSON(w, http.StatusBadRequest, map[string]any{"message": "path and name required"})
+			return
+		}
+		path, name := rest[:idx], rest[idx+1:]
+		env := r.URL.Query().Get("env")
+		if env == "" {
+			env = "default"
+		}
+		if err := secStore.Delete(path, name, env); err != nil {
+			writeJSON(w, http.StatusNotFound, map[string]any{"message": "not found"})
+			return
+		}
+		writeJSON(w, http.StatusOK, map[string]any{"ok": true})
+	}))
+}
+
 // putSecretHandler serves POST /v1/kms/orgs/{org}/secrets (create/upsert).
 //
 // env is a first-class component of the storage key
@@ -1,9 +1,9 @@
 // Package kms is the canonical Go client for KMS.
 //
 // Transport: native luxfi/zap binary protocol on port 9999. There is no
-// HTTP fallback in this package — services that need cross-cluster reach
-// can shell out to /v1/kms/secrets/{name} via curl, but in-cluster Go is
-// always ZAP.
+// HTTP fallback in this package — services that need cross-cluster reach use
+// the org-scoped HTTP surface (GET /v1/kms/orgs/{org}/secrets/{path}/{name}
+// with an IAM bearer), but in-cluster Go is always ZAP.
 //
 // Defaults read from environment:
 //
@@ -1 +1 @@
 .11.0
 .12.5