From 53b31cb76ca12f8a48288132bee97d30a8348156 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Mon, 10 Aug 2026 10:16:53 +1000 Subject: [PATCH] fix: keep the RDS master password out of the server and console logs (#754) --- cmd/rds-agent/engine.go | 8 +- cmd/rds-agent/engine_test.go | 34 ++++ scripts/images/rds-postgres/rds-init | 90 +++++++++- scripts/images/rds-postgres/rds-init_test.sh | 163 ++++++++++++++++++- spinifex/handlers/rds/engine.go | 12 +- spinifex/handlers/rds/engine_test.go | 5 + 6 files changed, 295 insertions(+), 17 deletions(-) diff --git a/cmd/rds-agent/engine.go b/cmd/rds-agent/engine.go index 263ecbcb9..d5979a728 100644 --- a/cmd/rds-agent/engine.go +++ b/cmd/rds-agent/engine.go @@ -187,7 +187,13 @@ func (e *postgresEngine) SetPassword(ctx context.Context, username, password str if err := handlers_rds.EnginePostgres().ValidateUsernameNotReserved(username); err != nil { return fmt.Errorf("refusing to set the password of a role the engine reserves: %w", err) } - const sql = `\getenv master RDS_MASTER_USERNAME + // psql interpolates the password into the ALTER ROLE before the server sees + // it, and these three are what would write it to the log — the last on any + // failure at its own default. SUSET, so the parameter group cannot win. + const sql = `SET log_statement = 'none'; +SET log_min_duration_statement = -1; +SET log_min_error_statement = 'panic'; +\getenv master RDS_MASTER_USERNAME \getenv password RDS_MASTER_PASSWORD ALTER ROLE :"master" WITH LOGIN PASSWORD :'password'; ` diff --git a/cmd/rds-agent/engine_test.go b/cmd/rds-agent/engine_test.go index 4b5022708..6b226c18f 100644 --- a/cmd/rds-agent/engine_test.go +++ b/cmd/rds-agent/engine_test.go @@ -185,6 +185,40 @@ func TestPostgresEngine_SetPasswordKeepsTheSecretOutOfArgv(t *testing.T) { } } +// psql interpolates the password into the ALTER ROLE before the server sees it, +// so a parameter group that turns statement logging on would write the rotated +// password into a postmaster.log every snapshot then carries. +func TestPostgresEngine_SetPasswordSilencesStatementLogging(t *testing.T) { + runner := &recordingRunner{} + engine := newTestEngine(t, runner.run) + + if err := engine.SetPassword(context.Background(), "mulgamaster", "n3w-pw"); err != nil { + t.Fatalf("SetPassword: %v", err) + } + if len(runner.calls) != 1 { + t.Fatalf("ran %d commands, want 1", len(runner.calls)) + } + sql := runner.calls[0].Stdin + alter := strings.Index(sql, "ALTER ROLE") + if alter < 0 { + t.Fatalf("SQL %q does not alter the role", sql) + } + for _, guard := range []string{ + "SET log_statement = 'none';", + "SET log_min_duration_statement = -1;", + "SET log_min_error_statement = 'panic';", + } { + at := strings.Index(sql, guard) + if at < 0 { + t.Errorf("SQL %q is missing %q", sql, guard) + continue + } + if at > alter { + t.Errorf("SQL %q sets %q only after the ALTER ROLE has already been logged", sql, guard) + } + } +} + // psql echoes the failing statement, which here would carry the new password // back into a reply the control plane records. func TestPostgresEngine_SetPasswordRedactsTheSecretFromAFailure(t *testing.T) { diff --git a/scripts/images/rds-postgres/rds-init b/scripts/images/rds-postgres/rds-init index e4b869491..eca6defc0 100644 --- a/scripts/images/rds-postgres/rds-init +++ b/scripts/images/rds-postgres/rds-init @@ -138,6 +138,15 @@ load_handoff() { '' | *[!0-9]*) fail "bootstrap config has a non-numeric RDS_PORT: ${RDS_PORT}" ;; esac + # redact_stream is line-oriented, so a password spanning a line boundary + # would match neither half and reach /dev/console in the clear. The control + # plane rejects one; refusing here too keeps that an asserted property of + # the handoff rather than a silent dependency on another service. + case "${RDS_MASTER_PASSWORD}" in + *' +'*) fail "the master password in the bootstrap config spans more than one line; the control plane should have rejected it" ;; + esac + log "bootstrap config loaded (mode=${RDS_MODE:-unset}, port=${RDS_PORT})" } @@ -322,16 +331,81 @@ stop_bootstrap_server() { as_pg "'${PG_BIN}/pg_ctl' --pgdata='${PGDATA}' --wait --timeout=60 --mode=fast stop" } +# Prepended to every bootstrap script. psql interpolates the password into the +# ALTER ROLE before the server sees it, and these three are what would then write +# it to the log: log_min_error_statement does so on any failure at its own +# default. All are SUSET and set for this session only, so nothing the customer +# puts in the parameter group overrides them. +BOOTSTRAP_LOG_GUARD="SET log_statement = 'none'; +SET log_min_duration_statement = -1; +SET log_min_error_statement = 'panic';" + +# Replaces every occurrence of the secret with a placeholder. Parameter +# expansion rather than sed, whose script would carry the secret in an argv any +# process on the guest can read. +redact_stream() { + _secret="$1" + while IFS= read -r _line || [ -n "${_line}" ]; do + _out="" + while [ -n "${_secret}" ]; do + case "${_line}" in + *"${_secret}"*) + _out="${_out}${_line%%"${_secret}"*}[REDACTED]" + _line="${_line#*"${_secret}"}" + ;; + *) break ;; + esac + done + printf '%s\n' "${_out}${_line}" + done +} + # Identifiers and the password ride the environment and are re-quoted by psql, -# never reaching a shell word or argv. +# never reaching a shell word or argv. Both streams are redacted before they +# reach the caller: psql echoes the statement it failed on, and the initd sends +# everything here to /dev/console, which the host captures off ttyS0. psql_bootstrap() { - RDS_MASTER_USERNAME="${RDS_MASTER_USERNAME}" \ - RDS_MASTER_PASSWORD="${RDS_MASTER_PASSWORD}" \ - RDS_DB_NAME="${RDS_DB_NAME}" \ - RDS_GROUP_ROLE="${MASTER_GROUP_ROLE}" \ - "${PG_BIN}/psql" --no-psqlrc --quiet --no-align --tuples-only \ - -v ON_ERROR_STOP=1 \ - -h "${BOOTSTRAP_DIR}" -p "${RDS_PORT}" -U "${PG_USER}" -d postgres -f - + # A pipeline's status is its last command's and POSIX sh has no pipefail, so + # psql's own status leaves the subshell through the filesystem. The guard + # shifts psql's line numbers by the number of lines it adds. + _status_file="${BOOTSTRAP_DIR}/psql.status" + rm -f "${_status_file}" + { + _psql_rc=0 + { printf '%s\n' "${BOOTSTRAP_LOG_GUARD}"; cat; } | + RDS_MASTER_USERNAME="${RDS_MASTER_USERNAME}" \ + RDS_MASTER_PASSWORD="${RDS_MASTER_PASSWORD}" \ + RDS_DB_NAME="${RDS_DB_NAME}" \ + RDS_GROUP_ROLE="${MASTER_GROUP_ROLE}" \ + "${PG_BIN}/psql" --no-psqlrc --quiet --no-align --tuples-only \ + -v ON_ERROR_STOP=1 \ + -h "${BOOTSTRAP_DIR}" -p "${RDS_PORT}" -U "${PG_USER}" -d postgres -f - || + _psql_rc=$? + printf '%s\n' "${_psql_rc}" > "${_status_file}" + } 2>&1 | redact_stream "${RDS_MASTER_PASSWORD}" + + # Every way of not recovering psql's status is a failed bootstrap rather + # than a silent success, and each says so in its own words: blaming psql for + # a status file that could not be written would send an operator looking at + # a PostgreSQL that did exactly what it was asked. + # + # A torn write leaves the file created but empty, so the value is checked + # before it is returned: `return ""` is fatal in the guest's ash, and would + # kill rds-init here — past the sweep, before the diagnostics. + _psql_status=1 + if [ -f "${_status_file}" ]; then + _psql_status=$(cat "${_status_file}" 2>/dev/null) || _psql_status="" + case "${_psql_status}" in + '' | *[!0-9]*) + echo "[rds-init] ERROR: psql ran but its exit status could not be read back from ${_status_file} (got '${_psql_status}'); treating the bootstrap as failed" >&2 + _psql_status=1 + ;; + esac + rm -f "${_status_file}" + else + echo "[rds-init] ERROR: psql ran but its exit status was never recorded at ${_status_file}; treating the bootstrap as failed" >&2 + fi + return "${_psql_status}" } # Runs on a freshly initialised cluster only. diff --git a/scripts/images/rds-postgres/rds-init_test.sh b/scripts/images/rds-postgres/rds-init_test.sh index d2b8f5cc0..227060b03 100644 --- a/scripts/images/rds-postgres/rds-init_test.sh +++ b/scripts/images/rds-postgres/rds-init_test.sh @@ -15,7 +15,8 @@ WORK=$(mktemp -d) trap 'rm -rf "${WORK}"' EXIT REAL_LS=$(command -v ls) -export REAL_LS +REAL_CAT=$(command -v cat) +export REAL_LS REAL_CAT STUBBIN="${WORK}/bin" mkdir -p "${STUBBIN}" @@ -122,11 +123,41 @@ cat > "${STUBBIN}/psql" <<'EOF' cat } >> "${PSQL_CALLS}" # ON_ERROR_STOP makes a real psql exit non-zero on any SQL failure; the stub -# reproduces that so the master-bootstrap failure path is exercisable. -[ "${PSQL_FAIL:-0}" = "1" ] && exit 3 +# reproduces that, and with it the echo of the failing statement — password and +# all — that the redaction filter exists to catch. +if [ "${PSQL_FAIL:-0}" = "1" ]; then + echo "psql::7: ERROR: syntax error at or near \"${RDS_MASTER_PASSWORD:-}\"" >&2 + echo "psql::7: STATEMENT: ALTER ROLE \"m\" WITH LOGIN PASSWORD '${RDS_MASTER_PASSWORD:-}';" >&2 + exit 3 +fi +# PSQL_STATUS_CORRUPT=absent: psql succeeds, but rds-init cannot create the +# status file it recovers psql's exit code through — the shape a read-only or +# full /run gives it. -h is the bootstrap directory the status file lives in. +if [ "${PSQL_STATUS_CORRUPT:-}" = "absent" ]; then + while [ $# -gt 0 ]; do + [ "$1" = "-h" ] && mkdir -p "$2/psql.status" && break + shift + done +fi exit 0 EOF +# cat stub: delegates normally, but lets the read-back of psql's exit status +# return what a torn write or an I/O error would. rds-init's other use of cat +# takes no argument (it reads the bootstrap script from stdin) and is untouched. +cat > "${STUBBIN}/cat" <<'EOF' +#!/bin/sh +case "${1:-}" in + *psql.status) + case "${PSQL_STATUS_CORRUPT:-}" in + empty) exit 0 ;; + garbage) echo "not-a-number"; exit 0 ;; + esac + ;; +esac +exec "${REAL_CAT}" "$@" +EOF + # sync stub: a no-op, except that SYNC_KILL_PARENT signals rds-init from the # directory sync that follows the receipt rename. That is the one window where # an installed receipt and an armed sweep coexist. @@ -172,7 +203,10 @@ write_handoff() { echo "RDS_MODE=$1" echo "RDS_DB_INSTANCE_IDENTIFIER=${DB_ID:-db1}" echo "RDS_MASTER_USERNAME=${MASTER_USER:-mulgamaster}" - [ -n "$2" ] && echo "RDS_MASTER_PASSWORD=$2" + # Single-quoted as rds-agent's shellQuote writes it, which is what lets a + # password carrying shell metacharacters — or a newline — survive the + # handoff intact and reach the checks that exist for it. + [ -n "$2" ] && echo "RDS_MASTER_PASSWORD='$2'" [ -n "$3" ] && echo "RDS_DB_NAME=$3" [ -n "${PENDING:-}" ] && echo "RDS_BOOTSTRAP_PENDING=${PENDING}" [ -n "${PAYLOAD_ID:-}" ] && echo "RDS_PAYLOAD_ID=${PAYLOAD_ID}" @@ -189,9 +223,10 @@ write_tls() { echo "KEY" > "${HANDOFF}/server.key" } +# write_parameters [setting]: the resolved parameter group rds-agent hands over. write_parameters() { mkdir -p "${HANDOFF}" - echo "shared_buffers = 128MB" > "${HANDOFF}/parameters.conf" + echo "${1:-shared_buffers = 128MB}" > "${HANDOFF}/parameters.conf" } # reset_state clears everything the previous case left behind: a fresh datadir, @@ -204,7 +239,7 @@ reset_state() { : > "${PGCTL_CALLS}" : > "${PSQL_CALLS}" unset INITDB_FAIL PG_HBA_AS_DIR PSQL_FAIL LS_FAIL RDS_ALLOW_LOCAL_DATADIR MASTER_USER || true - unset PENDING PAYLOAD_ID DB_ID RECEIPT_DIR_OVERRIDE || true + unset PENDING PAYLOAD_ID DB_ID RECEIPT_DIR_OVERRIDE PSQL_STATUS_CORRUPT || true } # run_env: invoke a command with every path knob pointed into the temp dir. @@ -325,6 +360,122 @@ if run_ok "attach"; then && pass "attach: TLS survives a handoff without a new cert" || fail "attach: TLS turned off" fi +# --- Case 3a: a parameter group cannot log the master password --- +# The customer's parameters are installed before the master role is applied and +# the bootstrap postmaster does not override config_file, so log_statement = all +# is live for the ALTER ROLE carrying the password. The guard is session-local +# and SUSET, so it holds whatever the parameter group says. +reset_state +write_handoff initialize 's3cr3t' appdb +write_parameters "log_statement = all" +if run_ok "log-guard"; then + grep -q '^log_statement = all' "${PGDATA}/conf.d/10-rds-parameters.conf" \ + && pass "log-guard: the customer's logging parameter is installed" \ + || fail "log-guard: the parameter group was not installed, so this is not the reported case" + for setting in "SET log_statement = 'none';" \ + "SET log_min_duration_statement = -1;" \ + "SET log_min_error_statement = 'panic';"; do + grep -qF "${setting}" "${PSQL_CALLS}" \ + && pass "log-guard: ${setting}" || fail "log-guard: psql never received ${setting}" + done + # Every bootstrap session carries the guard, so a statement added to the + # bootstrap later cannot end up outside it. + # `grep -c` exits non-zero on no match, which `set -e` would turn into an + # aborted run in place of a reported failure. + guard_count=$(grep -cF "SET log_statement = 'none';" "${PSQL_CALLS}" || true) + psql_count=$(grep -c '^--- psql ' "${PSQL_CALLS}" || true) + [ "${guard_count}" = "${psql_count}" ] \ + && pass "log-guard: all ${psql_count} bootstrap sessions guarded" \ + || fail "log-guard: ${guard_count} of ${psql_count} bootstrap sessions guarded" + guard_line=$(grep -nF "SET log_statement = 'none';" "${PSQL_CALLS}" | head -n 1 | cut -d: -f1) + alter_line=$(grep -n 'ALTER ROLE' "${PSQL_CALLS}" | head -n 1 | cut -d: -f1) + { [ -n "${alter_line}" ] && [ "${guard_line}" -lt "${alter_line}" ]; } \ + && pass "log-guard: the guard precedes the ALTER ROLE" \ + || fail "log-guard: the guard does not precede the ALTER ROLE" +fi + +# --- Case 3b: a failing psql must not echo the password to the console --- +# rds-init.initd sends this script's output to /dev/console, which the host +# captures off ttyS0, and psql under ON_ERROR_STOP echoes the statement it +# failed on. The redaction must not swallow the failure: the datadir is cleared +# exactly as in case 6d. +reset_state +write_handoff initialize 's3cr3t' appdb +write_parameters +PSQL_FAIL=1 +export PSQL_FAIL +run_fails "console-redaction" +grep -q 's3cr3t' "${WORK}/out" \ + && fail "console-redaction: the console capture carries the master password" \ + || pass "console-redaction: no password on the console" +grep -q '\[REDACTED\]' "${WORK}/out" \ + && pass "console-redaction: the redaction is marked" \ + || fail "console-redaction: psql's stderr never reached the console" +grep -q 'ERROR: syntax error' "${WORK}/out" \ + && pass "console-redaction: the diagnostic itself survives" \ + || fail "console-redaction: the redaction swallowed the diagnostic" +[ -e "${PGDATA}/PG_VERSION" ] \ + && fail "console-redaction: datadir kept after a failed master bootstrap" \ + || pass "console-redaction: the failure still propagates" +unset PSQL_FAIL + +# --- Case 3c: psql's status must cross the redaction pipeline or fail closed --- +# The status file is how psql's exit code leaves the subshell, POSIX sh having +# no pipefail. A torn write leaves it created but empty, and `return ""` is +# fatal in the guest's ash: rds-init would die past the datadir sweep and before +# any diagnostic, leaving a trust-authenticated postmaster on a deleted datadir. +for corruption in empty garbage; do + reset_state + write_handoff initialize 's3cr3t' appdb + PSQL_STATUS_CORRUPT="${corruption}" + export PSQL_STATUS_CORRUPT + run_fails "psql-status-${corruption}" + grep -q 'exit status could not be read back' "${WORK}/out" \ + && pass "psql-status-${corruption}: names the unreadable status rather than blaming psql" \ + || fail "psql-status-${corruption}: no diagnostic naming the status file" + grep -q 'Illegal number\|numeric argument required' "${WORK}/out" \ + && fail "psql-status-${corruption}: an unvalidated status reached return" \ + || pass "psql-status-${corruption}: nothing unvalidated reached return" + [ -e "${PGDATA}/PG_VERSION" ] \ + && fail "psql-status-${corruption}: datadir kept after an unrecoverable status" \ + || pass "psql-status-${corruption}: datadir cleared" + grep -q 'stop' "${PGCTL_CALLS}" \ + && pass "psql-status-${corruption}: bootstrap server stopped" \ + || fail "psql-status-${corruption}: bootstrap server left holding the datadir" + unset PSQL_STATUS_CORRUPT +done + +# --- Case 3d: a status file that could never be written is not psql's fault --- +# The write can fail outright — a read-only or full /run — while psql itself +# succeeded. Failing closed is right, but the operator must not be sent to a +# PostgreSQL that did exactly what it was asked. +reset_state +write_handoff initialize 's3cr3t' appdb +PSQL_STATUS_CORRUPT=absent +export PSQL_STATUS_CORRUPT +run_fails "psql-status-absent" +grep -q 'exit status was never recorded' "${WORK}/out" \ + && pass "psql-status-absent: names the missing status file" \ + || fail "psql-status-absent: no diagnostic distinguishing it from a psql failure" +[ -e "${PGDATA}/PG_VERSION" ] \ + && fail "psql-status-absent: datadir kept" || pass "psql-status-absent: datadir cleared" +unset PSQL_STATUS_CORRUPT + +# --- Case 3e: a multi-line master password is refused before it is spent --- +# redact_stream is line-oriented, so a password spanning a line boundary matches +# neither half and reaches /dev/console in the clear. The control plane rejects +# one; this is the guest asserting that rather than trusting it. +reset_state +write_handoff initialize 'top +secret' appdb +run_fails "multiline-password" +[ -s "${INITDB_CALLS}" ] \ + && fail "multiline-password: initdb ran before the password was refused" \ + || pass "multiline-password: refused before initdb spent it" +grep -q 'spans more than one line' "${WORK}/out" \ + && pass "multiline-password: refusal names the reason" \ + || fail "multiline-password: no refusal message" + # --- Case 4: an empty datadir in attach mode means the data volume is missing --- reset_state write_handoff attach '' appdb diff --git a/spinifex/handlers/rds/engine.go b/spinifex/handlers/rds/engine.go index aea3ef814..1870c6e44 100644 --- a/spinifex/handlers/rds/engine.go +++ b/spinifex/handlers/rds/engine.go @@ -112,8 +112,9 @@ func (e Engine) ValidateUsernameNotReserved(username string) error { return nil } -// Bounds only. The password is never inspected beyond this and never stored in -// cleartext past the first bootstrap fetch. +// Bounds and the printable-ASCII range AWS accepts. The password is never +// inspected beyond this and never stored in cleartext past the first bootstrap +// fetch. func ValidateMasterUserPassword(password string) error { switch { case password == "": @@ -123,6 +124,13 @@ func ValidateMasterUserPassword(password string) error { "MasterUserPassword must be between %d and %d characters", minMasterPasswordLen, maxMasterPasswordLen) } for _, r := range password { + // A control character would also survive the bootstrap handoff and defeat + // the line-oriented redaction that keeps the password off the guest + // console, so the range is refused here rather than sanitised there. + if r < 0x20 || r > 0x7e { + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "MasterUserPassword may only contain printable ASCII characters") + } if r == '/' || r == '"' || r == '@' || r == ' ' { return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "MasterUserPassword may not contain '/', '\"', '@' or spaces") diff --git a/spinifex/handlers/rds/engine_test.go b/spinifex/handlers/rds/engine_test.go index 48cf73f1b..3a8f899ae 100644 --- a/spinifex/handlers/rds/engine_test.go +++ b/spinifex/handlers/rds/engine_test.go @@ -86,6 +86,11 @@ func TestValidateMasterUserPassword(t *testing.T) { // The characters AWS excludes, which would otherwise break a connection // string or the engine's own role syntax. "pass/word1", `pass"word1`, "pass@word1", "pass word1", + // Outside printable ASCII. A newline survives the bootstrap handoff and + // defeats the guest's line-oriented redaction, putting the password on + // the host serial console; the rest are refused as the same class. + "pass\nword1", "pass\rword1", "pass\tword1", "pass\x00word1", "pass\x7fword1", + "pässword1", "passwordé", "pass\u00a0word1", } { err := ValidateMasterUserPassword(password) require.Error(t, err, "password %q should be rejected", password)