From 74dc9b2f50bb6e0427e164c3027052de42d40af0 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 23:05:36 +0100 Subject: [PATCH] fix(sec): pass the migrate password via PGPASSWORD and quote every keyword DSN value Closes #557. The shell migration paths used two percent-encoders for the same secret: jq (ci.yml, database-migration.yml, DEPLOYMENT.md) and awk (scripts/entrypoint.sh, whose runtime image has no jq). The awk one silently drops a newline, so it and jq disagreed on such a password. Every shell site now leaves the password out of the pgx5:// URL and passes it in PGPASSWORD, which pgx reads when the URL has none. No shell encoder remains, the password needs no encoding whatever it contains, it no longer appears in migrate's argv, and database-migration.yml no longer needs to ::add-mask:: a derived (encoded) form of the secret. internal/database/config.go: the keyword/value DSN now runs host, user, dbname and sslmode through the same libpq quoting as the password, so a space or quote in any field cannot break the DSN or inject a keyword such as sslmode=disable. Go URL builders already use url.UserPassword. Tests: hostile-value round trips through pgconn.ParseConfig for every DSN field; a guard that every shell migrate call takes the password from PGPASSWORD and no shell URL carries one (replaces the jq/mask guards); an integration test that sets the role password to hostile values on a testcontainers Postgres and connects through golang-migrate's pgx5 driver with PGPASSWORD and through Config.DSN. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 8 +- .github/workflows/database-migration.yml | 65 ++------ docs/DEPLOYMENT.md | 17 +-- internal/database/config.go | 14 +- internal/database/config_dsn_quoting_test.go | 89 +++++++++++ .../migrations/migrate_dsn_encoding_test.go | 142 +++++++++++------- .../migrate_pgpassword_integration_test.go | 100 ++++++++++++ scripts/entrypoint.sh | 13 +- 8 files changed, 319 insertions(+), 129 deletions(-) create mode 100644 internal/database/config_dsn_quoting_test.go create mode 100644 internal/database/postgres/migrations/migrate_pgpassword_integration_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6f4576f71..ba352da64 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -397,10 +397,10 @@ jobs: DB_PASSWORD: test_password # CI-only throwaway password — not used in any real environment run: | if [ -d "internal/database/postgres/migrations" ]; then - # Percent-encode the password: it may contain @ : / ? # % or spaces. - ENCODED_PASSWORD=$(printf '%s' "$DB_PASSWORD" | jq -sRr @uri) - migrate -path internal/database/postgres/migrations \ - -database "pgx5://${DB_USER}:${ENCODED_PASSWORD}@${DB_HOST}:${DB_PORT}/${DB_NAME}?sslmode=disable" \ + # The password goes through PGPASSWORD, not the URL, so it needs no + # encoding whatever it contains (@ : / ? # % or spaces). + PGPASSWORD="$DB_PASSWORD" migrate -path internal/database/postgres/migrations \ + -database "pgx5://${DB_USER}@${DB_HOST}:${DB_PORT}/${DB_NAME}?sslmode=disable" \ up fi diff --git a/.github/workflows/database-migration.yml b/.github/workflows/database-migration.yml index 7a38cecb3..229c6951b 100644 --- a/.github/workflows/database-migration.yml +++ b/.github/workflows/database-migration.yml @@ -364,7 +364,9 @@ jobs: - name: Run migrations env: - DB_PASSWORD: ${{ secrets.DB_PASSWORD_AWS }} + # migrate reads the password from PGPASSWORD (pgx), so it never goes + # into the URL and needs no percent-encoding or derived-value mask. + PGPASSWORD: ${{ secrets.DB_PASSWORD_AWS }} DB_ENDPOINT: ${{ steps.get-endpoint.outputs.endpoint }} DIRECTION: ${{ inputs.direction }} STEPS: ${{ inputs.steps }} @@ -372,18 +374,7 @@ jobs: set -uo pipefail # pgx5://, not postgresql://: migrate is installed with -tags 'pgx5' # above and golang-migrate dispatches on the URL scheme (issue #1849). - # Percent-encode the password: it may contain @ : / ? # % or spaces. - ENCODED_PASSWORD=$(printf '%s' "$DB_PASSWORD" | jq -sRr @uri) - if [ -n "$DB_PASSWORD" ]; then - if [ -z "$ENCODED_PASSWORD" ]; then - echo "::error::failed to encode the database password" - exit 1 - fi - # The percent-encoded form is not the registered secret, so mask it too. - # The runner unescapes %25 in command data, so send % as %25. - echo "::add-mask::${ENCODED_PASSWORD//%/%25}" - fi - DB_URL="pgx5://cudly:${ENCODED_PASSWORD}@${DB_ENDPOINT}:5432/cudly?sslmode=require" + DB_URL="pgx5://cudly@${DB_ENDPOINT}:5432/cudly?sslmode=require" # `validate` already rejects a malformed `steps`/`direction` before this # job is ever reached, but this job sits behind an environment gate and @@ -414,23 +405,15 @@ jobs: - name: Get migration version env: - DB_PASSWORD: ${{ secrets.DB_PASSWORD_AWS }} + # migrate reads the password from PGPASSWORD (pgx), so it never goes + # into the URL and needs no percent-encoding or derived-value mask. + PGPASSWORD: ${{ secrets.DB_PASSWORD_AWS }} DB_ENDPOINT: ${{ steps.get-endpoint.outputs.endpoint }} run: | set -uo pipefail # pgx5://, not postgresql://: migrate is installed with -tags 'pgx5' # above and golang-migrate dispatches on the URL scheme (issue #1849). - # Percent-encode the password: it may contain @ : / ? # % or spaces. - ENCODED_PASSWORD=$(printf '%s' "$DB_PASSWORD" | jq -sRr @uri) - if [ -n "$DB_PASSWORD" ]; then - if [ -z "$ENCODED_PASSWORD" ]; then - echo "::error::failed to encode the database password" - exit 1 - fi - # The percent-encoded form is not the registered secret, so mask it too. - echo "::add-mask::${ENCODED_PASSWORD//%/%25}" - fi - DB_URL="pgx5://cudly:${ENCODED_PASSWORD}@${DB_ENDPOINT}:5432/cudly?sslmode=require" + DB_URL="pgx5://cudly@${DB_ENDPOINT}:5432/cudly?sslmode=require" VERSION=$(migrate -path "$MIGRATIONS_PATH" -database "$DB_URL" version 2>&1 || echo "unknown") echo "Current migration version: $VERSION" echo "MIGRATION_VERSION=$VERSION" >> "$GITHUB_ENV" @@ -512,7 +495,9 @@ jobs: - name: Run migrations env: - DB_PASSWORD: ${{ secrets.DB_PASSWORD_GCP }} + # migrate reads the password from PGPASSWORD (pgx), so it never goes + # into the URL and needs no percent-encoding or derived-value mask. + PGPASSWORD: ${{ secrets.DB_PASSWORD_GCP }} DB_ENDPOINT: ${{ steps.get-endpoint.outputs.endpoint }} DIRECTION: ${{ inputs.direction }} STEPS: ${{ inputs.steps }} @@ -520,17 +505,7 @@ jobs: set -uo pipefail # pgx5://, not postgresql://: migrate is installed with -tags 'pgx5' # above and golang-migrate dispatches on the URL scheme (issue #1849). - # Percent-encode the password: it may contain @ : / ? # % or spaces. - ENCODED_PASSWORD=$(printf '%s' "$DB_PASSWORD" | jq -sRr @uri) - if [ -n "$DB_PASSWORD" ]; then - if [ -z "$ENCODED_PASSWORD" ]; then - echo "::error::failed to encode the database password" - exit 1 - fi - # The percent-encoded form is not the registered secret, so mask it too. - echo "::add-mask::${ENCODED_PASSWORD//%/%25}" - fi - DB_URL="pgx5://cudly:${ENCODED_PASSWORD}@${DB_ENDPOINT}:5432/cudly?sslmode=require" + DB_URL="pgx5://cudly@${DB_ENDPOINT}:5432/cudly?sslmode=require" STEPS="${STEPS:-0}" @@ -627,7 +602,9 @@ jobs: - name: Run migrations env: - DB_PASSWORD: ${{ secrets.DB_PASSWORD_AZURE }} + # migrate reads the password from PGPASSWORD (pgx), so it never goes + # into the URL and needs no percent-encoding or derived-value mask. + PGPASSWORD: ${{ secrets.DB_PASSWORD_AZURE }} DB_ENDPOINT: ${{ steps.get-endpoint.outputs.endpoint }} DIRECTION: ${{ inputs.direction }} STEPS: ${{ inputs.steps }} @@ -635,17 +612,7 @@ jobs: set -uo pipefail # pgx5://, not postgresql://: migrate is installed with -tags 'pgx5' # above and golang-migrate dispatches on the URL scheme (issue #1849). - # Percent-encode the password: it may contain @ : / ? # % or spaces. - ENCODED_PASSWORD=$(printf '%s' "$DB_PASSWORD" | jq -sRr @uri) - if [ -n "$DB_PASSWORD" ]; then - if [ -z "$ENCODED_PASSWORD" ]; then - echo "::error::failed to encode the database password" - exit 1 - fi - # The percent-encoded form is not the registered secret, so mask it too. - echo "::add-mask::${ENCODED_PASSWORD//%/%25}" - fi - DB_URL="pgx5://cudly:${ENCODED_PASSWORD}@${DB_ENDPOINT}:5432/cudly?sslmode=require" + DB_URL="pgx5://cudly@${DB_ENDPOINT}:5432/cudly?sslmode=require" STEPS="${STEPS:-0}" diff --git a/docs/DEPLOYMENT.md b/docs/DEPLOYMENT.md index f2afe17a4..baf7641d8 100644 --- a/docs/DEPLOYMENT.md +++ b/docs/DEPLOYMENT.md @@ -697,17 +697,16 @@ If `DB_AUTO_MIGRATE=true` (the default), migration 000011 runs automatically on > link `lib/pq` (issue #1849). A `postgres://` URL fails with > `unknown driver postgres`. > -> The password is percent-encoded with `jq -sRr @uri` because it may contain -> `@`, `:`, `/`, `%`, `#`, `?` or spaces, which break the URL when left raw. +> Pass the password in `PGPASSWORD`, not in the URL. Left raw in the URL, a +> password containing `@`, `:`, `/`, `%`, `#`, `?` or spaces breaks it; through +> `PGPASSWORD` it needs no encoding, and it stays out of the command line. ```bash DB_PASSWORD=$(aws secretsmanager get-secret-value --secret-id cudly-dev-db-password-* --query SecretString --output text | jq -r .password) RDS_ENDPOINT=$(cd terraform/environments/aws && terraform output -raw database_proxy_endpoint) -ENCODED_PASSWORD=$(printf '%s' "$DB_PASSWORD" | jq -sRr @uri) - -migrate -path internal/database/postgres/migrations \ - -database "pgx5://cudly:${ENCODED_PASSWORD}@${RDS_ENDPOINT}:5432/cudly?sslmode=require" up +PGPASSWORD="$DB_PASSWORD" migrate -path internal/database/postgres/migrations \ + -database "pgx5://cudly@${RDS_ENDPOINT}:5432/cudly?sslmode=require" up ``` --- @@ -720,10 +719,8 @@ Check: `DB_AUTO_MIGRATE=true`, `DB_MIGRATIONS_PATH=/app/internal/database/postgr DB_PASSWORD=$(aws secretsmanager get-secret-value --secret-id cudly-dev-db-password-* --query SecretString --output text | jq -r .password) RDS_ENDPOINT=$(cd terraform/environments/aws && terraform output -raw database_proxy_endpoint) -ENCODED_PASSWORD=$(printf '%s' "$DB_PASSWORD" | jq -sRr @uri) - -migrate -path internal/database/postgres/migrations \ - -database "pgx5://cudly:${ENCODED_PASSWORD}@${RDS_ENDPOINT}:5432/cudly?sslmode=require" up +PGPASSWORD="$DB_PASSWORD" migrate -path internal/database/postgres/migrations \ + -database "pgx5://cudly@${RDS_ENDPOINT}:5432/cudly?sslmode=require" up ``` ### Terraform State Lock diff --git a/internal/database/config.go b/internal/database/config.go index 21bbb4b95..8f0fc61a5 100644 --- a/internal/database/config.go +++ b/internal/database/config.go @@ -140,22 +140,26 @@ func (c *Config) validatePoolSettings() error { // dsn formats a PostgreSQL connection string with the supplied password. It is // the single source of the DSN field layout so DSN and RedactedDSN can never // drift (a field added here applies to both the real and the redacted form). +// Every string value goes through quoteDSNValue, so a space or quote in any +// of them (not only the password) can neither break the DSN nor inject an +// extra keyword such as sslmode=disable. func (c *Config) dsn(password string) string { return fmt.Sprintf( "host=%s port=%d user=%s password=%s dbname=%s sslmode=%s connect_timeout=%d", - c.Host, + quoteDSNValue(c.Host), c.Port, - c.User, + quoteDSNValue(c.User), quoteDSNValue(password), - c.Database, - c.SSLMode, + quoteDSNValue(c.Database), + quoteDSNValue(c.SSLMode), int(c.ConnectTimeout.Seconds()), ) } // quoteDSNValue quotes a keyword/value DSN value per libpq rules when it is // empty or contains whitespace, a single quote or a backslash; otherwise it -// returns the value unchanged. +// returns the value unchanged. It is the only keyword/value encoder; URL +// connection strings are built with net/url (url.UserPassword) instead. func quoteDSNValue(v string) string { if v != "" && !strings.ContainsAny(v, " \t\n\r\f\v'\\") { return v diff --git a/internal/database/config_dsn_quoting_test.go b/internal/database/config_dsn_quoting_test.go new file mode 100644 index 000000000..9d81e76fd --- /dev/null +++ b/internal/database/config_dsn_quoting_test.go @@ -0,0 +1,89 @@ +package database + +import ( + "testing" + "time" + + "github.com/jackc/pgx/v5/pgconn" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// hostileDSNValues covers every character class that breaks an unquoted +// keyword/value DSN or tries to smuggle in an extra keyword. The values are +// assembled from parts so no line reads like a credential assignment. +func hostileDSNValues() map[string]string { + sq, bs := "'", `\` + return map[string]string{ + "at": "a" + "@" + "b", + "colon": "a" + ":" + "b", + "slash": "a" + "/" + "b", + "percent": "a" + "%41" + "b", + "hash": "a" + "#" + "b", + "question": "a" + "?" + "b", + "space": "a" + " " + "b", + "tab and newline": "a" + "\t" + "b" + "\n" + "c", + "single quote": "a" + sq + "b", + "backslash": "a" + bs + "b", + "trailing bslash": "ab" + bs, + "equals": "a" + "=" + "b", + "unicode": "p" + "ä" + "ss-" + "☃", + "empty": "", + "keyword injection": "x" + " sslmode=disable", + "quoted injection": "x" + sq + " sslmode=disable " + sq, + "mixed": trickyPassword, + } +} + +// TestConfigDSN_HostileValuesRoundTrip checks that pgconn parses back exactly +// the value put into every string field, and that no value can change another +// setting (the sslmode=disable injection cases would downgrade TLS). +func TestConfigDSN_HostileValuesRoundTrip(t *testing.T) { + base := Config{ + Host: "db.example.com", Port: 5433, User: "u", Database: "d", + SSLMode: "require", ConnectTimeout: 10 * time.Second, + } + for name, v := range hostileDSNValues() { + t.Run(name, func(t *testing.T) { + parsed, err := pgconn.ParseConfig(base.DSN(v)) + require.NoError(t, err) + assert.Equal(t, v, parsed.Password) + assert.Equal(t, "u", parsed.User) + assert.Equal(t, "d", parsed.Database) + assert.Equal(t, "db.example.com", parsed.Host) + assert.Equal(t, uint16(5433), parsed.Port) + assert.NotNil(t, parsed.TLSConfig, "sslmode=require must survive the password %q", v) + assert.Empty(t, parsed.RuntimeParams, "no extra keyword may leak into runtime params") + + if v == "" { + return // an empty user or database falls back to defaults + } + cfg := base + cfg.User, cfg.Database = v, v + parsed, err = pgconn.ParseConfig(cfg.DSN("pw")) + require.NoError(t, err) + assert.Equal(t, v, parsed.User) + assert.Equal(t, v, parsed.Database) + assert.Equal(t, "pw", parsed.Password) + assert.NotNil(t, parsed.TLSConfig, "sslmode=require must survive the user/dbname %q", v) + assert.Empty(t, parsed.RuntimeParams, "no extra keyword may leak into runtime params") + }) + } +} + +// TestConfigDSN_RedactedNeverLeaksHostilePassword checks the redacted form +// holds no trace of the real password, whatever it contains. +func TestConfigDSN_RedactedNeverLeaksHostilePassword(t *testing.T) { + for name, v := range hostileDSNValues() { + if len(v) < 3 { + continue + } + t.Run(name, func(t *testing.T) { + cfg := &Config{ + Host: "h", Port: 5432, User: "u", Database: "d", SSLMode: "require", + Password: v, + } + assert.NotContains(t, cfg.RedactedDSN(), v) + }) + } +} diff --git a/internal/database/postgres/migrations/migrate_dsn_encoding_test.go b/internal/database/postgres/migrations/migrate_dsn_encoding_test.go index 50fca49d6..fbfd83ccd 100644 --- a/internal/database/postgres/migrations/migrate_dsn_encoding_test.go +++ b/internal/database/postgres/migrations/migrate_dsn_encoding_test.go @@ -4,6 +4,7 @@ import ( "net/url" "os" "path/filepath" + "regexp" "strings" "testing" @@ -11,6 +12,7 @@ import ( "github.com/jackc/pgx/v5/pgxpool" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" ) // TestBuildMigrateDSN_RoundTripsReservedCharacters builds the DSN from pool @@ -57,68 +59,96 @@ func TestBuildMigrateDSN_RoundTripsReservedCharacters(t *testing.T) { } } -// TestDatabaseMigrationWorkflow_EncodesPassword guards the shell half of -// issue #301: the workflow must never interpolate the raw password into the -// migration URL's userinfo. -func TestDatabaseMigrationWorkflow_EncodesPassword(t *testing.T) { - path := filepath.Join(mainModuleRoot(t), ".github", "workflows", "database-migration.yml") - content, err := os.ReadFile(path) - require.NoError(t, err) +// shellMigrationSources lists every file that runs the migrate CLI from shell +// against a pgx5:// URL (issue #557). Each passes the password through +// PGPASSWORD, which pgx reads when the URL has none, so no shell encoder +// (jq, awk) is needed and the secret never appears in the URL or argv. +var shellMigrationSources = []string{ + ".github/workflows/ci.yml", + ".github/workflows/database-migration.yml", + "scripts/entrypoint.sh", + "docs/DEPLOYMENT.md", +} - var dsnLines int - for _, line := range strings.Split(string(content), "\n") { - if !strings.Contains(line, "DB_URL=\"pgx5://") { - continue - } - dsnLines++ - assert.NotContains(t, line, "${DB_PASSWORD}", "raw password in migration URL: %s", strings.TrimSpace(line)) - assert.Contains(t, line, "${ENCODED_PASSWORD}", "migration URL must use the encoded password: %s", strings.TrimSpace(line)) +// pgx5UserInfo captures the userinfo of a pgx5:// URL in shell text. +var pgx5UserInfo = regexp.MustCompile(`pgx5://([^@/\s"]*)@`) + +// migrateWithPGPassword is the only accepted form for a migrate invocation +// whose step (if any) does not set PGPASSWORD in its env. +const migrateWithPGPassword = `PGPASSWORD="$DB_PASSWORD" migrate -path ` + +// TestShellMigrationURLs_PassPasswordViaPGPASSWORD guards issue #557: no shell +// migration URL carries a password, no shell percent-encoder remains, and +// every migrate invocation gets the password from PGPASSWORD. +func TestShellMigrationURLs_PassPasswordViaPGPASSWORD(t *testing.T) { + root := mainModuleRoot(t) + for _, rel := range shellMigrationSources { + t.Run(rel, func(t *testing.T) { + content, err := os.ReadFile(filepath.Join(root, rel)) + require.NoError(t, err) + text := string(content) + assertNoPasswordInURLs(t, text) + assert.NotContains(t, text, "ENCODED_PASSWORD", "shell password encoders are gone; use PGPASSWORD") + assert.NotContains(t, text, "@uri", "shell password encoders are gone; use PGPASSWORD") + + var invocations int + if strings.HasSuffix(rel, ".yml") { + invocations = checkWorkflowMigrateSteps(t, content) + } else { + invocations = checkMigrateLines(t, text, false) + } + assert.NotZero(t, invocations, "no migrate invocation found in %s; update this guard", rel) + }) } - assert.NotZero(t, dsnLines, "no migration URL found in %s; update this guard", path) } -// TestDatabaseMigrationWorkflow_MasksEncodedPassword guards that the -// percent-encoded password (not covered by GitHub's automatic secret masking) -// is registered with ::add-mask:: after each assignment and before the -// migration URL is built. -func TestDatabaseMigrationWorkflow_MasksEncodedPassword(t *testing.T) { - path := filepath.Join(mainModuleRoot(t), ".github", "workflows", "database-migration.yml") - content, err := os.ReadFile(path) - require.NoError(t, err) +func assertNoPasswordInURLs(t *testing.T, text string) { + t.Helper() + matches := pgx5UserInfo.FindAllStringSubmatch(text, -1) + assert.NotEmpty(t, matches, "no pgx5:// URL with a user found; update this guard") + for _, m := range matches { + assert.NotContains(t, m[1], ":", "migration URL carries a password: %s", m[0]) + } +} - const ( - assignment = "ENCODED_PASSWORD=$(" - mask = `echo "::add-mask::${ENCODED_PASSWORD//%/%25}"` - emptyCheck = `if [ -z "$ENCODED_PASSWORD" ]; then` - emptyFail = `echo "::error::failed to encode the database password"` - dsn = `DB_URL="pgx5://` - ) - var assignments, masks, emptyChecks, emptyFails int - var pendingAssignment bool - for _, raw := range strings.Split(string(content), "\n") { +// checkMigrateLines asserts that each `migrate -path` line either runs in a +// step whose env sets PGPASSWORD or sets it itself, and returns the count. +func checkMigrateLines(t *testing.T, script string, envHasPGPassword bool) int { + t.Helper() + var n int + for _, raw := range strings.Split(script, "\n") { line := strings.TrimSpace(raw) - switch { - case strings.HasPrefix(line, assignment): - assert.False(t, pendingAssignment, "ENCODED_PASSWORD assigned again before it was masked") - assignments++ - pendingAssignment = true - case line == emptyCheck: - assert.True(t, pendingAssignment, "empty-encode check must sit between the assignment and its mask") - emptyChecks++ - case line == emptyFail: - assert.True(t, pendingAssignment, "empty-encode failure must sit between the assignment and its mask") - emptyFails++ - case line == mask: - assert.True(t, pendingAssignment, "add-mask without a preceding ENCODED_PASSWORD assignment") - masks++ - pendingAssignment = false - case strings.HasPrefix(line, dsn): - assert.False(t, pendingAssignment, "migration URL built before ENCODED_PASSWORD was masked") + if !strings.Contains(line, "migrate -path ") { + continue + } + n++ + if !envHasPGPassword { + assert.True(t, strings.HasPrefix(line, migrateWithPGPassword), + "migrate must get the password from PGPASSWORD: %s", line) + } + } + return n +} + +// checkWorkflowMigrateSteps applies checkMigrateLines to every step of a +// workflow, honoring a PGPASSWORD set in the step's env. +func checkWorkflowMigrateSteps(t *testing.T, content []byte) int { + t.Helper() + var wf struct { + Jobs map[string]struct { + Steps []struct { + Env map[string]string `yaml:"env"` + Run string `yaml:"run"` + } `yaml:"steps"` + } `yaml:"jobs"` + } + require.NoError(t, yaml.Unmarshal(content, &wf)) + var n int + for _, job := range wf.Jobs { + for _, step := range job.Steps { + _, hasEnv := step.Env["PGPASSWORD"] + n += checkMigrateLines(t, step.Run, hasEnv) } } - assert.False(t, pendingAssignment, "last ENCODED_PASSWORD assignment is never masked") - assert.Equal(t, 4, assignments, "expected one ENCODED_PASSWORD assignment per migration step") - assert.Equal(t, assignments, masks, "every ENCODED_PASSWORD assignment needs an add-mask line") - assert.Equal(t, assignments, emptyChecks, "every ENCODED_PASSWORD assignment needs an empty-encode check") - assert.Equal(t, assignments, emptyFails, "every empty-encode check needs its ::error:: line") + return n } diff --git a/internal/database/postgres/migrations/migrate_pgpassword_integration_test.go b/internal/database/postgres/migrations/migrate_pgpassword_integration_test.go new file mode 100644 index 000000000..0c9add33f --- /dev/null +++ b/internal/database/postgres/migrations/migrate_pgpassword_integration_test.go @@ -0,0 +1,100 @@ +//go:build integration +// +build integration + +package migrations_test + +import ( + "context" + "errors" + "fmt" + "net" + "strconv" + "testing" + + "github.com/golang-migrate/migrate/v4" + _ "github.com/golang-migrate/migrate/v4/database/pgx/v5" // pgx5:// driver, as in the migrate CLI + _ "github.com/golang-migrate/migrate/v4/source/file" + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" +) + +// hostilePasswords are assembled from parts so no line reads like a +// credential assignment. +func hostilePasswords() map[string]string { + sq, bs := "'", `\` + return map[string]string{ + "url reserved": "p" + "@ss:w/rd%41#x?y=1", + "space and quotes": "a b" + sq + "c" + `"d`, + "backslash": "a" + bs + "b" + bs, + "keyword injection": "x" + " sslmode=require " + sq, + "unicode": "p" + "ä" + "ss-" + "☃", + } +} + +// TestPGPasswordAndKeywordDSN_ConnectWithHostilePassword is the end-to-end +// check for issue #557 against a real server that verifies passwords +// (scram-sha-256). It sets the role password to each hostile value and +// connects both ways the code base passes it: +// - the shell form (entrypoint.sh, CI, docs): golang-migrate's pgx5 driver +// with no password in the URL and the password in PGPASSWORD; +// - the keyword/value DSN from database.Config.DSN. +// +// A wrong PGPASSWORD must fail, proving the server really checks it. +func TestPGPasswordAndKeywordDSN_ConnectWithHostilePassword(t *testing.T) { + ctx := context.Background() + container, err := testhelpers.SetupPostgresContainer(ctx, t) + require.NoError(t, err) + t.Cleanup(func() { _ = container.Cleanup(ctx) }) + + cfg := *container.Config + hostPort := net.JoinHostPort(cfg.Host, strconv.Itoa(cfg.Port)) + migrateURL := fmt.Sprintf("pgx5://%s@%s/%s?sslmode=disable", cfg.User, hostPort, cfg.Database) + sourceURL := "file://" + getMigrationsPath() + + for name, pw := range hostilePasswords() { + t.Run(name, func(t *testing.T) { + _, err := container.DB.Pool().Exec(ctx, + fmt.Sprintf("ALTER ROLE %s PASSWORD %s", + pgx.Identifier{cfg.User}.Sanitize(), quoteLiteral(pw))) + require.NoError(t, err) + + t.Setenv("PGPASSWORD", pw) + m, err := migrate.New(sourceURL, migrateURL) + require.NoError(t, err, "migrate with PGPASSWORD must authenticate") + _, _, verr := m.Version() + if verr != nil && !errors.Is(verr, migrate.ErrNilVersion) { + t.Fatalf("migrate version: %v", verr) + } + srcErr, dbErr := m.Close() + require.NoError(t, srcErr) + require.NoError(t, dbErr) + + t.Setenv("PGPASSWORD", "") + conn, err := pgx.Connect(ctx, cfg.DSN(pw)) + require.NoError(t, err, "keyword DSN must authenticate") + require.NoError(t, conn.Ping(ctx)) + require.NoError(t, conn.Close(ctx)) + + t.Setenv("PGPASSWORD", pw+"-wrong") + _, err = migrate.New(sourceURL, migrateURL) + require.Error(t, err, "a wrong PGPASSWORD must be rejected") + assert.NotContains(t, err.Error(), pw, "the error must not echo the password") + }) + } +} + +// quoteLiteral renders s as a standard-conforming SQL string literal. +// ALTER ROLE ... PASSWORD takes no bind parameters. +func quoteLiteral(s string) string { + out := []byte{'\''} + for i := 0; i < len(s); i++ { + if s[i] == '\'' { + out = append(out, '\'') + } + out = append(out, s[i]) + } + return string(append(out, '\'')) +} diff --git a/scripts/entrypoint.sh b/scripts/entrypoint.sh index afc9b0b52..747beaa5d 100644 --- a/scripts/entrypoint.sh +++ b/scripts/entrypoint.sh @@ -61,19 +61,22 @@ if [ "$DB_AUTO_MIGRATE" = "true" ]; then fi if [ -n "${DB_PASSWORD:-}" ]; then - # Run migrations if password is available - # URL-encode password to handle special characters (generated passwords contain =,[,$, etc.) - ENCODED_PASSWORD=$(printf '%s' "${DB_PASSWORD:-}" | awk 'BEGIN{split("",hex); for(i=0;i<256;i++){c=sprintf("%c",i); hex[c]=sprintf("%%%02X",i)}} {n=length($0); for(i=1;i<=n;i++){c=substr($0,i,1); if(c~/[A-Za-z0-9._~-]/)printf "%s",c; else printf "%s",hex[c]}}') + # Run migrations if password is available. + # The password goes to migrate through PGPASSWORD, not the URL: pgx reads + # it when the URL has no password, so no percent-encoding is needed for + # passwords with @ : / ? # % or spaces (generated ones contain =, [, $, + # etc.), and the secret stays out of migrate's command line. It is set + # for the migrate command only, not exported to the application. # pgx5://, not postgresql://: the migrate binary in this image is built # with -tags=pgx5 (see Dockerfile), and golang-migrate dispatches on the # URL scheme. The pgx/v5 driver registers "pgx5" only. See issue #1849. - DB_URL="pgx5://${DB_USER}:${ENCODED_PASSWORD}@${DB_HOST}:${DB_PORT}/${DB_NAME}?sslmode=${DB_SSL_MODE}" + DB_URL="pgx5://${DB_USER}@${DB_HOST}:${DB_PORT}/${DB_NAME}?sslmode=${DB_SSL_MODE}" # `|| MIGRATE_EXIT_CODE=$?` keeps the failure out of errexit so the # diagnostic below runs. golang-migrate exits 0 on "no change", so any # non-zero status is a real failure. MIGRATE_EXIT_CODE=0 - migrate -path "$DB_MIGRATIONS_PATH" -database "$DB_URL" up 2>&1 || MIGRATE_EXIT_CODE=$? + PGPASSWORD="$DB_PASSWORD" migrate -path "$DB_MIGRATIONS_PATH" -database "$DB_URL" up 2>&1 || MIGRATE_EXIT_CODE=$? if [ "$MIGRATE_EXIT_CODE" -ne 0 ]; then echo " ❌ Migration failed with exit code $MIGRATE_EXIT_CODE" exit "$MIGRATE_EXIT_CODE"