gsadmin/spinifex
1
0
Fork 0
mirror of https://github.com/mulgadc/spinifex.git synced 2026-10-15 09:01:31 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: keep the RDS master password out of the server and console logs (#754)

Browse Source
This commit is contained in:
Julian Sommer
2026-08-10 10:16:53 +10:00
committed by GitHub
parent 7b33219617
commit 53b31cb76c
6 changed files with 295 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
cmd/rds-agent/engine.go
+7 -1
View File
@@ -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';
`
cmd/rds-agent/engine_test.go
+34
View File
@@ -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) {
scripts/images/rds-postgres/rds-init
+82 -8
View File
@@ -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 <stdin> 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.
scripts/images/rds-postgres/rds-init_test.sh
+157 -6
View File
@@ -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:<stdin>:7: ERROR: syntax error at or near \"${RDS_MASTER_PASSWORD:-}\"" >&2
echo "psql:<stdin>: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
spinifex/handlers/rds/engine.go
+10 -2
View File
@@ -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")
spinifex/handlers/rds/engine_test.go
+5
View File
@@ -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)