From 5a7a31b95bc51fc8292aaf598039c122ccd88307 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 04:00:45 +0100 Subject: [PATCH 1/2] sec(api): stop /health naming the dev encryption key to anonymous callers The credential_store check on the public /health and /api/health routes answered "dev_key_in_use" when CREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY=1 was set, telling an unauthenticated caller that every stored cloud credential is encrypted with the publicly known all-zero key. Report a generic "credential store degraded" message instead and log the cause server-side, matching how checkConfigStore already handles this public route. Status stays "degraded" so monitoring keeps alerting. Closes #15 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler.go | 5 +++-- internal/api/health.go | 9 ++++++--- internal/api/health_test.go | 34 +++++++++++++++++++++++++++++++++- internal/server/app.go | 3 ++- 4 files changed, 44 insertions(+), 7 deletions(-) diff --git a/internal/api/handler.go b/internal/api/handler.go index 46981775e..79957295d 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -135,8 +135,9 @@ type Handler struct { executeExchangeFn func(ctx context.Context, req exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) // encryptionKeySource is the env var name that resolved the credential - // encryption key. Empty when no credStore is configured. Used by the - // /health endpoint only — never logged outside that one place. + // encryption key. Empty when no credStore is configured. Read only by the + // /health credential_store check, which keeps it out of the public body + // and logs the dev-key case server-side (#15). encryptionKeySource string // Optional Azure revoke client factory injected by tests. When nil, the diff --git a/internal/api/health.go b/internal/api/health.go index 5c3611e34..893b6e9eb 100644 --- a/internal/api/health.go +++ b/internal/api/health.go @@ -83,9 +83,11 @@ func (h *Handler) checkConfigStore(ctx context.Context) HealthCheck { // Returns one of three logical states via Status + Message: // // - "healthy" / "ok": real key loaded from a Secrets Manager / Vault. -// - "degraded" / "dev_key_in_use": CREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY=1 was -// set, so the all-zero key is in use. Acceptable for local dev only; +// - "degraded" / "credential store degraded": CREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY=1 +// was set, so the all-zero key is in use. Acceptable for local dev only; // monitoring should alert on this state in any deployed environment. +// /health is a public route, so the body stays generic and the cause +// (dev key in use) goes to the server log only, like checkConfigStore (#15). // - "unhealthy" / "not configured": no credStore was wired in (config error). // // NOTE: "ok" only confirms the key is valid (LoadKey succeeded + startup guard @@ -99,9 +101,10 @@ func (h *Handler) checkCredentialStore() HealthCheck { } } if h.encryptionKeySource == credentials.EnvAllowDev { + logging.Warnf("health: credential store is using the insecure all-zero dev key (%s=1)", credentials.EnvAllowDev) return HealthCheck{ Status: "degraded", - Message: "dev_key_in_use", + Message: "credential store degraded", } } return HealthCheck{Status: "healthy"} diff --git a/internal/api/health_test.go b/internal/api/health_test.go index d7636c73a..85170f86e 100644 --- a/internal/api/health_test.go +++ b/internal/api/health_test.go @@ -2,7 +2,9 @@ package api import ( "context" + "encoding/json" "errors" + "strings" "testing" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" @@ -58,7 +60,37 @@ func TestHandler_checkCredentialStore_DevKeyInUse(t *testing.T) { } check := h.checkCredentialStore() assert.Equal(t, "degraded", check.Status) - assert.Equal(t, "dev_key_in_use", check.Message) + // /health is a public route: the body must not say which key is in use + // (#15). The detail goes to the server log only. + assert.Equal(t, "credential store degraded", check.Message) +} + +// TestHandler_GetHealth_DevKeyNotDisclosed checks the public /health body +// never names the dev key or the env var that selected it (#15). +func TestHandler_GetHealth_DevKeyNotDisclosed(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) + + handler := &Handler{ + config: mockStore, + auth: new(MockAuthService), + credStore: &stubCredStore{}, + encryptionKeySource: credentials.EnvAllowDev, + } + + response, err := handler.GetHealth(ctx) + require.NoError(t, err) + + // Monitoring still sees the degraded state. + assert.Equal(t, "degraded", response.Status) + assert.Equal(t, "degraded", response.Checks["credential_store"].Status) + + body, err := json.Marshal(response) + require.NoError(t, err) + for _, leak := range []string{"dev_key", "dev key", credentials.EnvAllowDev, "CREDENTIAL_ENCRYPTION"} { + assert.NotContains(t, strings.ToLower(string(body)), strings.ToLower(leak)) + } } func TestHandler_checkCredentialStore_NotConfigured(t *testing.T) { diff --git a/internal/server/app.go b/internal/server/app.go index d7eedbbcd..4a6f23103 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -99,7 +99,8 @@ type Application struct { // encKeySource is the env var name that resolved the credential encryption // key (e.g. "CREDENTIAL_ENCRYPTION_KEY_SECRET_NAME"). Set during - // reinitializeAfterConnect; surfaced via /health. + // reinitializeAfterConnect. /health reports only a generic degraded state + // from it, never the source itself (#15). encKeySource string // State from the most recent migration attempt. Surfaced by /health so From 2659825184c8acf389b8497868bd6a14f2270af2 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 11:45:17 +0100 Subject: [PATCH 2/2] fix(api): log the /health dev-key warning once per process Load balancers poll /health every few seconds, so the warning fired continuously in dev-key environments. The degraded status in the response still keeps monitoring alerting. Addresses review note on #941. Co-Authored-By: Claude Opus 5.5 --- internal/api/health.go | 10 +++++++++- internal/api/health_test.go | 16 ++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/internal/api/health.go b/internal/api/health.go index 893b6e9eb..6e4886ca7 100644 --- a/internal/api/health.go +++ b/internal/api/health.go @@ -2,6 +2,7 @@ package api import ( "context" + "sync" "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" @@ -93,6 +94,11 @@ func (h *Handler) checkConfigStore(ctx context.Context) HealthCheck { // NOTE: "ok" only confirms the key is valid (LoadKey succeeded + startup guard // passed); it does NOT guarantee that all DB rows have been re-keyed. Detect // pre-rekey state by alerting on application-level decrypt-failure ERROR logs. +// devKeyWarnOnce limits the dev-key warning to once per process: /health is +// polled every few seconds by load balancers, and the degraded status in the +// response already keeps monitoring alerting. +var devKeyWarnOnce sync.Once + func (h *Handler) checkCredentialStore() HealthCheck { if h.credStore == nil { return HealthCheck{ @@ -101,7 +107,9 @@ func (h *Handler) checkCredentialStore() HealthCheck { } } if h.encryptionKeySource == credentials.EnvAllowDev { - logging.Warnf("health: credential store is using the insecure all-zero dev key (%s=1)", credentials.EnvAllowDev) + devKeyWarnOnce.Do(func() { + logging.Warnf("health: credential store is using the insecure all-zero dev key (%s=1)", credentials.EnvAllowDev) + }) return HealthCheck{ Status: "degraded", Message: "credential store degraded", diff --git a/internal/api/health_test.go b/internal/api/health_test.go index 85170f86e..603842927 100644 --- a/internal/api/health_test.go +++ b/internal/api/health_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "strings" + "sync" "testing" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" @@ -260,3 +261,18 @@ func TestHealthCheck_Structure(t *testing.T) { assert.Equal(t, "unhealthy", check.Status) assert.Equal(t, "Service unavailable", check.Message) } + +// TestHandler_checkCredentialStore_DevKeyWarnsOnce checks the dev-key warning +// is logged once per process, not on every /health probe. +func TestHandler_checkCredentialStore_DevKeyWarnsOnce(t *testing.T) { + devKeyWarnOnce = sync.Once{} + t.Cleanup(func() { devKeyWarnOnce = sync.Once{} }) + buf := captureDefaultLog(t) + + h := &Handler{credStore: &stubCredStore{}, encryptionKeySource: credentials.EnvAllowDev} + for i := 0; i < 3; i++ { + assert.Equal(t, "degraded", h.checkCredentialStore().Status) + } + + assert.Equal(t, 1, strings.Count(buf.String(), "insecure all-zero dev key"), buf.String()) +}