From b721596213241511d85f6e037d4674c6f3987d25 Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Sat, 16 May 2026 04:30:53 +0000 Subject: [PATCH] =?UTF-8?q?fix(config):=20DEPL-004=20=E2=80=94=20expand=20?= =?UTF-8?q?$(POSTGRES=5FPASSWORD)=20placeholder=20in=20CERTCTL=5FDATABASE?= =?UTF-8?q?=5FURL?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sprint 3 unified-master-audit closure. The Helm chart's _helpers.tpl (line 133) renders the bundled-Postgres URL with a literal '$(POSTGRES_PASSWORD)' placeholder: postgres://certctl:$(POSTGRES_PASSWORD)@db:5432/certctl?sslmode=disable Kubernetes' '$(VAR)' env-substitution syntax ONLY expands when the value is a string literal in the Pod spec. Values sourced from 'valueFrom.secretKeyRef' (which is how the chart wires CERTCTL_DATABASE_URL) are NOT expanded — the literal makes it all the way to the server, which tries to dial Postgres with '$(POSTGRES_PASSWORD)' as the password, fails with auth error, and leaks the placeholder into application error logs. Fix: in-process expansion at internal/config/config.expandDatabaseURL. strings.ReplaceAll of the literal '$(POSTGRES_PASSWORD)' token with os.Getenv('POSTGRES_PASSWORD') when both the token is present AND the env var is set. Conservative — no os.ExpandEnv (which would expand any $VAR), no Docker entrypoint shim, no Helm-template-time password injection that would inline the secret into a second Kubernetes resource. External-Postgres deploys whose URL embeds the real password pass through untouched because the placeholder doesn't match. Regression coverage in internal/config/config_test.go pins: - happy-path placeholder substitution - non-placeholder URL passes through unchanged - placeholder + empty POSTGRES_PASSWORD leaves the URL alone - multi-occurrence safety via ReplaceAll Closes DEPL-004. --- internal/config/config.go | 49 +++++++++++++++++++++++++++++++- internal/config/config_test.go | 51 ++++++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/internal/config/config.go b/internal/config/config.go index 35a6938..6222ff0 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -333,7 +333,23 @@ func Load() (*Config, error) { AuditFlushTimeoutSeconds: getEnvInt("CERTCTL_AUDIT_FLUSH_TIMEOUT_SECONDS", 30), }, Database: DatabaseConfig{ - URL: getEnv("CERTCTL_DATABASE_URL", "postgres://localhost/certctl"), + // DEPL-004 closure (Sprint 3, 2026-05-16). The Helm chart's + // _helpers.tpl renders the bundled-Postgres URL with a literal + // `$(POSTGRES_PASSWORD)` placeholder (see + // deploy/helm/certctl/templates/_helpers.tpl line 133). The + // Kubernetes env-substitution `$(VAR)` syntax ONLY expands + // when the value is a string literal in `env:` — values + // sourced from Secrets (via `valueFrom.secretKeyRef`) are + // passed through verbatim with no expansion. Pre-fix the + // server received the literal "postgres://user:$(POSTGRES_PASSWORD)@..." + // string and tried to dial Postgres with that as the password, + // failing with auth error and leaking the placeholder into + // error logs. expandDatabaseURL substitutes the placeholder + // with os.Getenv("POSTGRES_PASSWORD") when present; external- + // Postgres deploys that bake the password directly into the + // URL string are unaffected because there is no placeholder + // to match. + URL: expandDatabaseURL(getEnv("CERTCTL_DATABASE_URL", "postgres://localhost/certctl")), // Phase 6 SCALE-M1 closure (2026-05-14): bumped default from // 25 → 50 to relieve pool-saturation pressure on 1K+ agent / // 10K+ cert fleets. Postgres default max_connections is 100 @@ -1269,6 +1285,37 @@ func getEnv(key, defaultValue string) string { return defaultValue } +// expandDatabaseURL substitutes the literal "$(POSTGRES_PASSWORD)" +// placeholder in a database URL with the value of the POSTGRES_PASSWORD +// environment variable. DEPL-004 closure (Sprint 3, 2026-05-16). +// +// Kubernetes ONLY expands `$(VAR)` syntax when the env value is a +// string literal in the Pod spec. Values sourced from +// `valueFrom.secretKeyRef` (which is how the Helm chart wires +// CERTCTL_DATABASE_URL) are NOT expanded — the literal makes it all +// the way to the application. This helper does the expansion in-process +// so the bundled-Postgres flow Just Works without a per-pod entrypoint +// shim. +// +// Conservative: a strings.Replace on exactly one well-known token +// (the chart's `_helpers.tpl` produces `$(POSTGRES_PASSWORD)` and +// nothing else). External-Postgres deploys whose URL embeds the +// real password don't match the placeholder and pass through untouched. +// When POSTGRES_PASSWORD is unset, the URL is left as-is so the +// downstream connection failure is the same as before (and a missing +// password is the operator's mis-config, not our regression). +func expandDatabaseURL(url string) string { + const placeholder = "$(POSTGRES_PASSWORD)" + if !strings.Contains(url, placeholder) { + return url + } + pw := os.Getenv("POSTGRES_PASSWORD") + if pw == "" { + return url + } + return strings.ReplaceAll(url, placeholder, pw) +} + // getEnvInt reads an integer environment variable with the given key and default value. func getEnvInt(key string, defaultValue int) int { if value := os.Getenv(key); value != "" { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 51ae913..04af06f 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1918,3 +1918,54 @@ func TestValidate_Bundle2_CORSConcreteAllowlist_Accepted(t *testing.T) { t.Errorf("Validate() returned %v; want nil for concrete CORS allowlist", err) } } + +// ============================================================================= +// DEPL-004 closure (Sprint 3, 2026-05-16). The Helm chart renders the +// bundled-Postgres URL with a literal "$(POSTGRES_PASSWORD)" +// placeholder. Kubernetes does NOT expand `$(VAR)` syntax when the env +// is sourced from a Secret (valueFrom.secretKeyRef), so the server +// receives the placeholder verbatim. expandDatabaseURL substitutes the +// token with os.Getenv("POSTGRES_PASSWORD") at Load() time. +// ============================================================================= + +func TestExpandDatabaseURL_SubstitutesPlaceholder(t *testing.T) { + t.Setenv("POSTGRES_PASSWORD", "s3cret!") + in := "postgres://certctl:$(POSTGRES_PASSWORD)@db:5432/certctl?sslmode=disable" + got := expandDatabaseURL(in) + want := "postgres://certctl:s3cret!@db:5432/certctl?sslmode=disable" + if got != want { + t.Errorf("expandDatabaseURL = %q; want %q", got, want) + } +} + +func TestExpandDatabaseURL_NoPlaceholderPassesThrough(t *testing.T) { + // External-Postgres deploys bake the password into the URL string + // — the helper must not touch URLs that don't carry the placeholder. + t.Setenv("POSTGRES_PASSWORD", "ignored") + in := "postgres://user:realpw@external:5432/db?sslmode=require" + if got := expandDatabaseURL(in); got != in { + t.Errorf("expandDatabaseURL on non-placeholder URL = %q; want %q (no-op)", got, in) + } +} + +func TestExpandDatabaseURL_PlaceholderButNoEnvLeftAlone(t *testing.T) { + // When POSTGRES_PASSWORD is unset, leave the URL alone so the + // downstream connection failure is the same as before (misconfig + // is the operator's, not our regression). + t.Setenv("POSTGRES_PASSWORD", "") + in := "postgres://certctl:$(POSTGRES_PASSWORD)@db:5432/certctl?sslmode=disable" + if got := expandDatabaseURL(in); got != in { + t.Errorf("expandDatabaseURL with no POSTGRES_PASSWORD = %q; want unchanged %q", got, in) + } +} + +func TestExpandDatabaseURL_MultipleOccurrences(t *testing.T) { + // Defensive: belt-and-suspenders. The chart only emits one + // placeholder today but ReplaceAll guards against future drift. + t.Setenv("POSTGRES_PASSWORD", "X") + in := "$(POSTGRES_PASSWORD)/$(POSTGRES_PASSWORD)" + want := "X/X" + if got := expandDatabaseURL(in); got != want { + t.Errorf("expandDatabaseURL = %q; want %q", got, want) + } +}