From 5021e001368bd737d6d42c5a178ffd5fa832fa52 Mon Sep 17 00:00:00 2001 From: Manas Srivastava Date: Fri, 29 May 2026 23:34:09 +0530 Subject: [PATCH 1/2] sec(jobs): pass pg_dump password via PGPASSWORD env, not argv MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes SEC-WORKER FINDING-1 (CWE-214, P1) + FINDING-2 (CWE-214, P1). Both pg_dump call-sites embedded the connection password in the URL on argv: - platform_db_backup.go:631 — daily 02:00 UTC platform DB backup (leaks the doadmin password) - customer_backup_runner.go:108 — hourly Pro/Team customer backup (leaks the customer's DB password, decrypted from AES-GCM ciphertext) argv is world-readable via /proc//cmdline for the entire backup window (multi-minute on the platform DB). Any sidecar / debug shell / log-shipper / kube-exec process — and any crash-dump archived by `kubectl describe` — captures the secret. Fix: tiny helper `splitPGPassword(url) → (urlWithoutPW, password, err)` strips the password from the URL userinfo. Both call-sites set `PGPASSWORD=` on cmd.Env (alongside the parent env) so libpq picks it up out-of-band. URL on argv no longer contains the password. Fail-open posture: if URL parse fails (malformed connection_url), fall back to the original URL on argv — better than wedging every customer's backup ladder over one operator typo. Today's URLs are constructed by the provisioner from validated identifiers so this path is purely defensive. Production LOC delta: 62 (helper 58 + 4 imports/edits per site). Tests: - TestSplitPGPassword (6 subcases: userpass, user-only, no-userinfo, empty, percent-encoded password, malformed-URL-fail-open) - TestSplitPGPassword_NoLeak: literal password substring MUST NOT appear in returned URL (THE regression guard for this fix) Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/jobs/customer_backup_runner.go | 17 ++++- internal/jobs/pgpw.go | 59 ++++++++++++++++ internal/jobs/pgpw_test.go | 92 +++++++++++++++++++++++++ internal/jobs/platform_db_backup.go | 23 ++++++- 4 files changed, 189 insertions(+), 2 deletions(-) create mode 100644 internal/jobs/pgpw.go create mode 100644 internal/jobs/pgpw_test.go diff --git a/internal/jobs/customer_backup_runner.go b/internal/jobs/customer_backup_runner.go index 26b1bce..e85525b 100644 --- a/internal/jobs/customer_backup_runner.go +++ b/internal/jobs/customer_backup_runner.go @@ -50,6 +50,7 @@ import ( "io" "log/slog" "net/http" + "os" "os/exec" "strings" "time" @@ -105,11 +106,25 @@ type pgDumpRunner interface { type realPgDumpRunner struct{} func (realPgDumpRunner) Run(ctx context.Context, connURL string, w io.Writer) error { + // SEC-WORKER FINDING-2 (2026-05-29): split the customer's DB password + // out of the URL into PGPASSWORD env so it does NOT sit in argv (and + // therefore /proc//cmdline + `ps aux` + kubectl describe crash + // archive) for the entire hourly backup window. Fail-open on parse + // error to avoid a single malformed connection_url stalling every + // customer's backup ladder. + dsn, pw, splitErr := splitPGPassword(connURL) + if splitErr != nil { + dsn = connURL + pw = "" + } cmd := exec.CommandContext(ctx, "pg_dump", "--no-owner", "--no-acl", "--format=custom", - "-d", connURL, + "-d", dsn, ) + if pw != "" { + cmd.Env = append(os.Environ(), "PGPASSWORD="+pw) + } cmd.Stdout = w // Stderr goes to slog at the call site by buffering — we don't want a // noisy pg_dump banner ("dumping contents of table ...") to flood diff --git a/internal/jobs/pgpw.go b/internal/jobs/pgpw.go new file mode 100644 index 0000000..1d25b3f --- /dev/null +++ b/internal/jobs/pgpw.go @@ -0,0 +1,59 @@ +package jobs + +// pgpw.go — small helper used by every pg_dump call-site to pass the +// Postgres password out-of-band (via PGPASSWORD env) instead of inside the +// process-args connection URI. +// +// SEC-WORKER FINDING-1 + FINDING-2 (2026-05-29): +// - platform_db_backup.go ran `pg_dump ` for the +// daily 02:00 UTC platform-DB backup. The DSN with embedded +// doadmin password was visible in `ps aux` / /proc//cmdline for +// the entire multi-minute backup window — any sidecar / debug shell / +// log-shipper / `kubectl describe` crash dump could read it. +// - customer_backup_runner.go ran `pg_dump -d ` for +// every per-customer hourly Pro/Team backup. Same surface, but the +// leaked secret is the customer's DB password (decrypted from +// resources.connection_url AES-GCM ciphertext). +// +// libpq honors PGPASSWORD via env. We strip the password from the URI +// userinfo and set PGPASSWORD on the cmd.Env before exec — the password +// no longer appears in cmdline. +// +// Conservative: if parsing fails, returns the original URL and "" — the +// caller falls back to old behavior (no regression). Callers that want +// hard-fail on parse can check the returned error. + +import ( + "fmt" + "net/url" +) + +// splitPGPassword returns the Postgres URL with the userinfo password +// removed, plus the extracted password. If u has no password (e.g. SSL +// cert auth) the returned password is "" and the URL is returned +// unchanged. If u cannot be parsed as a URL, the input is returned +// unchanged along with the parse error. +// +// Examples: +// +// "postgres://u:p@h:5432/db?sslmode=require" +// → ("postgres://u@h:5432/db?sslmode=require", "p", nil) +// +// "postgres://u@h/db" → ("postgres://u@h/db", "", nil) +// "postgres://h/db" → ("postgres://h/db", "", nil) +func splitPGPassword(rawURL string) (string, string, error) { + u, err := url.Parse(rawURL) + if err != nil { + return rawURL, "", fmt.Errorf("parse pg url: %w", err) + } + if u.User == nil { + return rawURL, "", nil + } + pw, hasPW := u.User.Password() + if !hasPW { + return rawURL, "", nil + } + // Reconstruct userinfo with only the username. + u.User = url.User(u.User.Username()) + return u.String(), pw, nil +} diff --git a/internal/jobs/pgpw_test.go b/internal/jobs/pgpw_test.go new file mode 100644 index 0000000..6ed1cae --- /dev/null +++ b/internal/jobs/pgpw_test.go @@ -0,0 +1,92 @@ +package jobs + +import ( + "strings" + "testing" +) + +// TestSplitPGPassword pins the SEC-WORKER FINDING-1 + FINDING-2 fix: +// pg_dump call sites move the Postgres password from process argv into +// PGPASSWORD env. The helper must: +// 1. Strip the password from a typical userinfo URL. +// 2. Pass through unchanged when there is no password (cert auth, +// no user, malformed URL with fail-open). +// 3. Never leak the literal password in the returned URL. +func TestSplitPGPassword(t *testing.T) { + cases := []struct { + name string + in string + wantURL string + wantPW string + wantErr bool + }{ + { + name: "userpass", + in: "postgres://doadmin:abc123@host:25060/db?sslmode=require", + wantURL: "postgres://doadmin@host:25060/db?sslmode=require", + wantPW: "abc123", + }, + { + name: "user_only_no_password", + in: "postgres://doadmin@host:25060/db?sslmode=require", + wantURL: "postgres://doadmin@host:25060/db?sslmode=require", + wantPW: "", + }, + { + name: "no_userinfo", + in: "postgres://host:25060/db", + wantURL: "postgres://host:25060/db", + wantPW: "", + }, + { + name: "empty", + in: "", + wantURL: "", + wantPW: "", + }, + { + name: "percent_encoded_password", + in: "postgres://u:p%40ss%40word@h:5432/db", + wantURL: "postgres://u@h:5432/db", + wantPW: "p@ss@word", // url.Userinfo.Password() decodes + }, + { + name: "malformed_url_fail_open", + in: "::::not a url", + wantURL: "::::not a url", + wantPW: "", + wantErr: true, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + gotURL, gotPW, err := splitPGPassword(c.in) + if (err != nil) != c.wantErr { + t.Fatalf("err = %v, wantErr = %v", err, c.wantErr) + } + if gotURL != c.wantURL { + t.Errorf("URL\n got: %q\n want: %q", gotURL, c.wantURL) + } + if gotPW != c.wantPW { + t.Errorf("password\n got: %q\n want: %q", gotPW, c.wantPW) + } + }) + } +} + +// TestSplitPGPassword_NoLeak: the literal password substring must never +// appear in the returned URL (this is THE point of the fix). +func TestSplitPGPassword_NoLeak(t *testing.T) { + const secret = "ZZZ_NEVER_IN_URL_ZZZ" + in := "postgres://admin:" + secret + "@host:5432/db?sslmode=require" + gotURL, gotPW, err := splitPGPassword(in) + if err != nil { + t.Fatalf("splitPGPassword(%q) error: %v", in, err) + } + if strings.Contains(gotURL, secret) { + t.Errorf("returned URL %q still contains the password %q", gotURL, secret) + } + if gotPW != secret { + t.Errorf("password\n got: %q\n want: %q", gotPW, secret) + } +} diff --git a/internal/jobs/platform_db_backup.go b/internal/jobs/platform_db_backup.go index 97df79a..0a18d82 100644 --- a/internal/jobs/platform_db_backup.go +++ b/internal/jobs/platform_db_backup.go @@ -623,18 +623,39 @@ func durationSeconds(d time.Duration) float64 { type defaultPgDumpExec struct{} // Dump runs pg_dump and streams its stdout to w. +// +// SEC-WORKER FINDING-1 (2026-05-29): the connection password is passed +// to pg_dump out-of-band via PGPASSWORD env, NOT embedded in the URL on +// argv. argv is world-readable via /proc//cmdline for any sidecar / +// debug shell / log-shipper / kube-exec process for the entire backup +// window. func (defaultPgDumpExec) Dump(ctx context.Context, databaseURL string, w io.Writer) (int64, error) { bin := os.Getenv("PG_DUMP_BIN") if bin == "" { bin = "pg_dump" } + // Split password out of the URL → into PGPASSWORD env. If parse fails + // we fall back to the original URL (no regression): the same code path + // it has always run. The downside of fail-open is that a malformed + // URL would still leak; the alternative is hard-fail on every backup + // run because of one operator typo — caller chose the safer default. + dsn, pw, splitErr := splitPGPassword(databaseURL) + if splitErr != nil { + // non-fatal — fall back to original URL on argv + dsn = databaseURL + pw = "" + } cmd := exec.CommandContext(ctx, bin, "--no-owner", "--no-acl", "--format=custom", "--compress=9", - databaseURL, + dsn, ) + if pw != "" { + // Inherit parent env so pg_dump still sees PATH, HOME, etc. + cmd.Env = append(os.Environ(), "PGPASSWORD="+pw) + } // Capture stderr to a small buffer so a pg_dump failure produces a // useful error message. stdout streams straight to w. var stderr strings.Builder From 1fe55e96892e67ee4409924afb3031ed14e876f8 Mon Sep 17 00:00:00 2001 From: Manas Srivastava Date: Sat, 30 May 2026 19:45:50 +0530 Subject: [PATCH 2/2] test(jobs): cover PGPASSWORD env-injection branches (100% patch) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add direct tests for realPgDumpRunner.Run and defaultPgDumpExec.Dump that exercise the SEC-WORKER FINDING-1 + FINDING-2 fix paths added in this PR: - pw != "" → cmd.Env carries PGPASSWORD, argv carries the stripped DSN - splitErr != nil → fail-open passthrough (no env, original URL on argv) Spawns a shell-script fake pg_dump that records argv + PGPASSWORD env to files in a TempDir, then asserts no literal password leak in argv and the secret IS in PGPASSWORD env. Closes the diff-cover gap on lines 117-119, 126-127 (customer_backup_runner.go) and 644-647, 656-658 (platform_db_backup.go). --- internal/jobs/customer_backup_runner_test.go | 150 +++++++++++++++++++ internal/jobs/platform_db_backup_test.go | 122 +++++++++++++++ 2 files changed, 272 insertions(+) diff --git a/internal/jobs/customer_backup_runner_test.go b/internal/jobs/customer_backup_runner_test.go index 6e97721..71e6263 100644 --- a/internal/jobs/customer_backup_runner_test.go +++ b/internal/jobs/customer_backup_runner_test.go @@ -5,6 +5,9 @@ import ( "context" "errors" "io" + "os" + "path/filepath" + "runtime" "sync" "testing" "time" @@ -550,3 +553,150 @@ func TestRetentionCutoff_PositiveDaysIsBackInTime(t *testing.T) { t.Errorf("pro 30d: cutoff = %v, want %v", got, want) } } + +// installFakePgDump writes a shell-script "pg_dump" into a TempDir, prepends +// it to PATH for the test's lifetime, and returns the script path so the +// test can read back the recorded argv + env after invocation. The fake +// prints argv to /argv.txt and env's PGPASSWORD value to +// /pgpassword.txt, then exits 0 (success path) or 1 if the caller +// passes failExitCode=true. +// +// Used by TestRealPgDumpRunner_* and TestDefaultPgDumpExec_*: those tests +// exercise the SEC-WORKER FINDING-1 + FINDING-2 PGPASSWORD-env branches +// in customer_backup_runner.go + platform_db_backup.go which require +// actually spawning a pg_dump-named process. +func installFakePgDump(t *testing.T, failExitCode bool) (dir string) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("fake pg_dump script is shell-based; worker runs on linux/darwin only") + } + dir = t.TempDir() + exitCode := "0" + if failExitCode { + exitCode = "1" + } + // The script: + // 1. Writes every argv element (one per line) to argv.txt + // 2. Writes PGPASSWORD (or empty string) to pgpassword.txt + // 3. Writes a tiny stdout payload so callers that pipe stdout see bytes + // 4. Exits 0 (success) or 1 (caller-controlled failure) + script := "#!/bin/sh\n" + + "printf '%s\\n' \"$@\" > \"" + dir + "/argv.txt\"\n" + + "printf '%s' \"${PGPASSWORD:-}\" > \"" + dir + "/pgpassword.txt\"\n" + + "printf 'fakepgdumpbody'\n" + + "exit " + exitCode + "\n" + path := filepath.Join(dir, "pg_dump") + if err := os.WriteFile(path, []byte(script), 0o755); err != nil { + t.Fatalf("write fake pg_dump: %v", err) + } + oldPATH := os.Getenv("PATH") + t.Setenv("PATH", dir+string(os.PathListSeparator)+oldPATH) + return dir +} + +func readFakePgDumpRecord(t *testing.T, dir string) (argv []string, pgpassword string) { + t.Helper() + argvBytes, err := os.ReadFile(filepath.Join(dir, "argv.txt")) + if err != nil { + t.Fatalf("read argv.txt: %v", err) + } + // Strip the trailing newline before splitting so the last entry isn't "". + argvStr := string(bytes.TrimRight(argvBytes, "\n")) + pgpassword = mustReadString(t, filepath.Join(dir, "pgpassword.txt")) + if argvStr == "" { + return nil, pgpassword + } + parts := bytes.Split([]byte(argvStr), []byte("\n")) + argv = make([]string, len(parts)) + for i, b := range parts { + argv[i] = string(b) + } + return argv, pgpassword +} + +func mustReadString(t *testing.T, path string) string { + t.Helper() + b, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + return string(b) +} + +// TestRealPgDumpRunner_Run_PasswordMovesToEnv pins SEC-WORKER FINDING-2: +// realPgDumpRunner must strip the password out of connURL and pass it via +// PGPASSWORD env, NOT inside argv. This covers customer_backup_runner.go +// lines 125-127 (the `if pw != ""` env-setting branch). +func TestRealPgDumpRunner_Run_PasswordMovesToEnv(t *testing.T) { + dir := installFakePgDump(t, false) + + const secret = "super-secret-pw-ZZZ" + connURL := "postgres://doadmin:" + secret + "@db.example.com:25060/app?sslmode=require" + + var out bytes.Buffer + if err := (realPgDumpRunner{}).Run(context.Background(), connURL, &out); err != nil { + t.Fatalf("Run: %v", err) + } + if out.String() != "fakepgdumpbody" { + t.Errorf("stdout payload: got %q, want %q", out.String(), "fakepgdumpbody") + } + + argv, pgpassword := readFakePgDumpRecord(t, dir) + + // PGPASSWORD env must carry the secret. + if pgpassword != secret { + t.Errorf("PGPASSWORD env: got %q, want %q", pgpassword, secret) + } + // argv must NOT contain the literal password anywhere — this is THE + // security promise the PR is shipping. + for _, a := range argv { + if bytes.Contains([]byte(a), []byte(secret)) { + t.Errorf("argv leaks password: %q (full argv: %q)", a, argv) + } + } + // argv MUST still carry the stripped DSN (with userinfo password removed). + found := false + for _, a := range argv { + if a == "postgres://doadmin@db.example.com:25060/app?sslmode=require" { + found = true + } + } + if !found { + t.Errorf("argv missing stripped DSN; got: %q", argv) + } +} + +// TestRealPgDumpRunner_Run_MalformedURLFailOpen pins the fail-open branch: +// if splitPGPassword returns an error, the runner falls back to the original +// connURL with no PGPASSWORD env. Covers customer_backup_runner.go lines +// 116-119 (the `if splitErr != nil { dsn = connURL; pw = "" }` branch). +func TestRealPgDumpRunner_Run_MalformedURLFailOpen(t *testing.T) { + dir := installFakePgDump(t, false) + + // Same shape that splitPGPassword's TestSplitPGPassword malformed_url_fail_open + // case proves returns an error from url.Parse. + const malformed = "::::not a url" + + var out bytes.Buffer + if err := (realPgDumpRunner{}).Run(context.Background(), malformed, &out); err != nil { + t.Fatalf("Run on malformed URL: %v", err) + } + + argv, pgpassword := readFakePgDumpRecord(t, dir) + + // Fail-open: no PGPASSWORD env is set because pw == "". + if pgpassword != "" { + t.Errorf("PGPASSWORD on fail-open: got %q, want empty", pgpassword) + } + // The original malformed URL is passed through to pg_dump argv unchanged + // (this is the "no regression" promise — same code path it has always run). + found := false + for _, a := range argv { + if a == malformed { + found = true + } + } + if !found { + t.Errorf("argv missing fail-open passthrough URL %q; got: %q", malformed, argv) + } +} diff --git a/internal/jobs/platform_db_backup_test.go b/internal/jobs/platform_db_backup_test.go index 5125b15..2eee9a9 100644 --- a/internal/jobs/platform_db_backup_test.go +++ b/internal/jobs/platform_db_backup_test.go @@ -37,6 +37,9 @@ import ( "database/sql" "errors" "io" + "os" + "path/filepath" + "runtime" "strings" "sync" "testing" @@ -590,3 +593,122 @@ func keysOf(m map[string][]byte) []string { } return out } + +// installFakePgDumpBin writes a fake pg_dump script and points PG_DUMP_BIN +// at its absolute path. Unlike installFakePgDump (PATH-prepend), this targets +// the defaultPgDumpExec.Dump path which honors PG_DUMP_BIN env override. +// The script records argv + PGPASSWORD env for the test to inspect. +func installFakePgDumpBin(t *testing.T) (dir string) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("fake pg_dump script is shell-based; worker runs on linux/darwin only") + } + dir = t.TempDir() + script := "#!/bin/sh\n" + + "printf '%s\\n' \"$@\" > \"" + dir + "/argv.txt\"\n" + + "printf '%s' \"${PGPASSWORD:-}\" > \"" + dir + "/pgpassword.txt\"\n" + + "printf 'fakedump'\n" + + "exit 0\n" + path := filepath.Join(dir, "pg_dump_fake") + if err := os.WriteFile(path, []byte(script), 0o755); err != nil { + t.Fatalf("write fake pg_dump: %v", err) + } + t.Setenv("PG_DUMP_BIN", path) + return dir +} + +func readBinRecord(t *testing.T, dir string) (argv []string, pgpassword string) { + t.Helper() + argvBytes, err := os.ReadFile(filepath.Join(dir, "argv.txt")) + if err != nil { + t.Fatalf("read argv.txt: %v", err) + } + pwBytes, err := os.ReadFile(filepath.Join(dir, "pgpassword.txt")) + if err != nil { + t.Fatalf("read pgpassword.txt: %v", err) + } + pgpassword = string(pwBytes) + argvStr := string(bytes.TrimRight(argvBytes, "\n")) + if argvStr == "" { + return nil, pgpassword + } + parts := strings.Split(argvStr, "\n") + return parts, pgpassword +} + +// TestDefaultPgDumpExec_Dump_PasswordMovesToEnv pins SEC-WORKER FINDING-1: +// defaultPgDumpExec must strip the platform-DB doadmin password from the URL +// and pass it via PGPASSWORD env, NOT inside argv. Covers +// platform_db_backup.go lines 655-658 (the `if pw != ""` env-setting branch). +func TestDefaultPgDumpExec_Dump_PasswordMovesToEnv(t *testing.T) { + dir := installFakePgDumpBin(t) + + const secret = "doadmin-platform-pw-XYZ" + databaseURL := "postgres://doadmin:" + secret + "@platform.example.com:25060/instant_platform?sslmode=require" + + var out bytes.Buffer + n, err := (defaultPgDumpExec{}).Dump(context.Background(), databaseURL, &out) + if err != nil { + t.Fatalf("Dump: %v", err) + } + if n == 0 { + t.Errorf("Dump returned 0 bytes written; want >0") + } + if out.String() != "fakedump" { + t.Errorf("stdout payload: got %q, want %q", out.String(), "fakedump") + } + + argv, pgpassword := readBinRecord(t, dir) + + if pgpassword != secret { + t.Errorf("PGPASSWORD env: got %q, want %q", pgpassword, secret) + } + // The literal password must not appear anywhere in argv. + for _, a := range argv { + if strings.Contains(a, secret) { + t.Errorf("argv leaks password: %q (full argv: %q)", a, argv) + } + } + // And the stripped DSN must be present. + stripped := "postgres://doadmin@platform.example.com:25060/instant_platform?sslmode=require" + found := false + for _, a := range argv { + if a == stripped { + found = true + } + } + if !found { + t.Errorf("argv missing stripped DSN %q; got: %q", stripped, argv) + } +} + +// TestDefaultPgDumpExec_Dump_MalformedURLFailOpen pins the fail-open branch: +// if splitPGPassword errors, Dump falls back to the original URL on argv +// with no PGPASSWORD env (no regression vs pre-fix behavior). Covers +// platform_db_backup.go lines 643-647. +func TestDefaultPgDumpExec_Dump_MalformedURLFailOpen(t *testing.T) { + dir := installFakePgDumpBin(t) + + // Matches pgpw_test.go's malformed_url_fail_open case — url.Parse errors. + const malformed = "::::not a url" + + var out bytes.Buffer + if _, err := (defaultPgDumpExec{}).Dump(context.Background(), malformed, &out); err != nil { + t.Fatalf("Dump on malformed URL: %v", err) + } + + argv, pgpassword := readBinRecord(t, dir) + + if pgpassword != "" { + t.Errorf("PGPASSWORD on fail-open: got %q, want empty", pgpassword) + } + found := false + for _, a := range argv { + if a == malformed { + found = true + } + } + if !found { + t.Errorf("argv missing fail-open passthrough URL %q; got: %q", malformed, argv) + } +}