Skip to content

Commit df11c0a

Browse files
sec(jobs): pass pg_dump password via PGPASSWORD env, not argv
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/<pid>/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=<pw>` 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) <noreply@anthropic.com>
1 parent 9a5a12e commit df11c0a

4 files changed

Lines changed: 189 additions & 2 deletions

File tree

internal/jobs/customer_backup_runner.go

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ import (
5050
"io"
5151
"log/slog"
5252
"net/http"
53+
"os"
5354
"os/exec"
5455
"strings"
5556
"time"
@@ -105,11 +106,25 @@ type pgDumpRunner interface {
105106
type realPgDumpRunner struct{}
106107

107108
func (realPgDumpRunner) Run(ctx context.Context, connURL string, w io.Writer) error {
109+
// SEC-WORKER FINDING-2 (2026-05-29): split the customer's DB password
110+
// out of the URL into PGPASSWORD env so it does NOT sit in argv (and
111+
// therefore /proc/<pid>/cmdline + `ps aux` + kubectl describe crash
112+
// archive) for the entire hourly backup window. Fail-open on parse
113+
// error to avoid a single malformed connection_url stalling every
114+
// customer's backup ladder.
115+
dsn, pw, splitErr := splitPGPassword(connURL)
116+
if splitErr != nil {
117+
dsn = connURL
118+
pw = ""
119+
}
108120
cmd := exec.CommandContext(ctx, "pg_dump",
109121
"--no-owner", "--no-acl",
110122
"--format=custom",
111-
"-d", connURL,
123+
"-d", dsn,
112124
)
125+
if pw != "" {
126+
cmd.Env = append(os.Environ(), "PGPASSWORD="+pw)
127+
}
113128
cmd.Stdout = w
114129
// Stderr goes to slog at the call site by buffering — we don't want a
115130
// noisy pg_dump banner ("dumping contents of table ...") to flood

internal/jobs/pgpw.go

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
package jobs
2+
3+
// pgpw.go — small helper used by every pg_dump call-site to pass the
4+
// Postgres password out-of-band (via PGPASSWORD env) instead of inside the
5+
// process-args connection URI.
6+
//
7+
// SEC-WORKER FINDING-1 + FINDING-2 (2026-05-29):
8+
// - platform_db_backup.go ran `pg_dump <DSN-with-password>` for the
9+
// daily 02:00 UTC platform-DB backup. The DSN with embedded
10+
// doadmin password was visible in `ps aux` / /proc/<pid>/cmdline for
11+
// the entire multi-minute backup window — any sidecar / debug shell /
12+
// log-shipper / `kubectl describe` crash dump could read it.
13+
// - customer_backup_runner.go ran `pg_dump -d <DSN-with-password>` for
14+
// every per-customer hourly Pro/Team backup. Same surface, but the
15+
// leaked secret is the customer's DB password (decrypted from
16+
// resources.connection_url AES-GCM ciphertext).
17+
//
18+
// libpq honors PGPASSWORD via env. We strip the password from the URI
19+
// userinfo and set PGPASSWORD on the cmd.Env before exec — the password
20+
// no longer appears in cmdline.
21+
//
22+
// Conservative: if parsing fails, returns the original URL and "" — the
23+
// caller falls back to old behavior (no regression). Callers that want
24+
// hard-fail on parse can check the returned error.
25+
26+
import (
27+
"fmt"
28+
"net/url"
29+
)
30+
31+
// splitPGPassword returns the Postgres URL with the userinfo password
32+
// removed, plus the extracted password. If u has no password (e.g. SSL
33+
// cert auth) the returned password is "" and the URL is returned
34+
// unchanged. If u cannot be parsed as a URL, the input is returned
35+
// unchanged along with the parse error.
36+
//
37+
// Examples:
38+
//
39+
// "postgres://u:p@h:5432/db?sslmode=require"
40+
// → ("postgres://u@h:5432/db?sslmode=require", "p", nil)
41+
//
42+
// "postgres://u@h/db" → ("postgres://u@h/db", "", nil)
43+
// "postgres://h/db" → ("postgres://h/db", "", nil)
44+
func splitPGPassword(rawURL string) (string, string, error) {
45+
u, err := url.Parse(rawURL)
46+
if err != nil {
47+
return rawURL, "", fmt.Errorf("parse pg url: %w", err)
48+
}
49+
if u.User == nil {
50+
return rawURL, "", nil
51+
}
52+
pw, hasPW := u.User.Password()
53+
if !hasPW {
54+
return rawURL, "", nil
55+
}
56+
// Reconstruct userinfo with only the username.
57+
u.User = url.User(u.User.Username())
58+
return u.String(), pw, nil
59+
}

internal/jobs/pgpw_test.go

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
package jobs
2+
3+
import (
4+
"strings"
5+
"testing"
6+
)
7+
8+
// TestSplitPGPassword pins the SEC-WORKER FINDING-1 + FINDING-2 fix:
9+
// pg_dump call sites move the Postgres password from process argv into
10+
// PGPASSWORD env. The helper must:
11+
// 1. Strip the password from a typical userinfo URL.
12+
// 2. Pass through unchanged when there is no password (cert auth,
13+
// no user, malformed URL with fail-open).
14+
// 3. Never leak the literal password in the returned URL.
15+
func TestSplitPGPassword(t *testing.T) {
16+
cases := []struct {
17+
name string
18+
in string
19+
wantURL string
20+
wantPW string
21+
wantErr bool
22+
}{
23+
{
24+
name: "userpass",
25+
in: "postgres://doadmin:abc123@host:25060/db?sslmode=require",
26+
wantURL: "postgres://doadmin@host:25060/db?sslmode=require",
27+
wantPW: "abc123",
28+
},
29+
{
30+
name: "user_only_no_password",
31+
in: "postgres://doadmin@host:25060/db?sslmode=require",
32+
wantURL: "postgres://doadmin@host:25060/db?sslmode=require",
33+
wantPW: "",
34+
},
35+
{
36+
name: "no_userinfo",
37+
in: "postgres://host:25060/db",
38+
wantURL: "postgres://host:25060/db",
39+
wantPW: "",
40+
},
41+
{
42+
name: "empty",
43+
in: "",
44+
wantURL: "",
45+
wantPW: "",
46+
},
47+
{
48+
name: "percent_encoded_password",
49+
in: "postgres://u:p%40ss%40word@h:5432/db",
50+
wantURL: "postgres://u@h:5432/db",
51+
wantPW: "p@ss@word", // url.Userinfo.Password() decodes
52+
},
53+
{
54+
name: "malformed_url_fail_open",
55+
in: "::::not a url",
56+
wantURL: "::::not a url",
57+
wantPW: "",
58+
wantErr: true,
59+
},
60+
}
61+
for _, c := range cases {
62+
t.Run(c.name, func(t *testing.T) {
63+
gotURL, gotPW, err := splitPGPassword(c.in)
64+
if (err != nil) != c.wantErr {
65+
t.Fatalf("err = %v, wantErr = %v", err, c.wantErr)
66+
}
67+
if gotURL != c.wantURL {
68+
t.Errorf("URL\n got: %q\n want: %q", gotURL, c.wantURL)
69+
}
70+
if gotPW != c.wantPW {
71+
t.Errorf("password\n got: %q\n want: %q", gotPW, c.wantPW)
72+
}
73+
})
74+
}
75+
}
76+
77+
// TestSplitPGPassword_NoLeak: the literal password substring must never
78+
// appear in the returned URL (this is THE point of the fix).
79+
func TestSplitPGPassword_NoLeak(t *testing.T) {
80+
const secret = "ZZZ_NEVER_IN_URL_ZZZ"
81+
in := "postgres://admin:" + secret + "@host:5432/db?sslmode=require"
82+
gotURL, gotPW, err := splitPGPassword(in)
83+
if err != nil {
84+
t.Fatalf("splitPGPassword(%q) error: %v", in, err)
85+
}
86+
if strings.Contains(gotURL, secret) {
87+
t.Errorf("returned URL %q still contains the password %q", gotURL, secret)
88+
}
89+
if gotPW != secret {
90+
t.Errorf("password\n got: %q\n want: %q", gotPW, secret)
91+
}
92+
}

internal/jobs/platform_db_backup.go

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -623,18 +623,39 @@ func durationSeconds(d time.Duration) float64 {
623623
type defaultPgDumpExec struct{}
624624

625625
// Dump runs pg_dump and streams its stdout to w.
626+
//
627+
// SEC-WORKER FINDING-1 (2026-05-29): the connection password is passed
628+
// to pg_dump out-of-band via PGPASSWORD env, NOT embedded in the URL on
629+
// argv. argv is world-readable via /proc/<pid>/cmdline for any sidecar /
630+
// debug shell / log-shipper / kube-exec process for the entire backup
631+
// window.
626632
func (defaultPgDumpExec) Dump(ctx context.Context, databaseURL string, w io.Writer) (int64, error) {
627633
bin := os.Getenv("PG_DUMP_BIN")
628634
if bin == "" {
629635
bin = "pg_dump"
630636
}
637+
// Split password out of the URL → into PGPASSWORD env. If parse fails
638+
// we fall back to the original URL (no regression): the same code path
639+
// it has always run. The downside of fail-open is that a malformed
640+
// URL would still leak; the alternative is hard-fail on every backup
641+
// run because of one operator typo — caller chose the safer default.
642+
dsn, pw, splitErr := splitPGPassword(databaseURL)
643+
if splitErr != nil {
644+
// non-fatal — fall back to original URL on argv
645+
dsn = databaseURL
646+
pw = ""
647+
}
631648
cmd := exec.CommandContext(ctx, bin,
632649
"--no-owner",
633650
"--no-acl",
634651
"--format=custom",
635652
"--compress=9",
636-
databaseURL,
653+
dsn,
637654
)
655+
if pw != "" {
656+
// Inherit parent env so pg_dump still sees PATH, HOME, etc.
657+
cmd.Env = append(os.Environ(), "PGPASSWORD="+pw)
658+
}
638659
// Capture stderr to a small buffer so a pg_dump failure produces a
639660
// useful error message. stdout streams straight to w.
640661
var stderr strings.Builder

0 commit comments

Comments
 (0)