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"