From 4f5cde03bb19dfc5d5b5412fac472f1dbe8d0655 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Tue, 11 Aug 2026 11:48:29 +1000 Subject: [PATCH] feat: validate the initial database name at create and in the guest DBName was passed through unvalidated, so a name such as my-db was accepted at CreateDBInstance and only quoted into safety by psql inside the guest. Engine gains a validateDBName rule and ValidateDBName, and rds-init re-checks the same rule in load_handoff before initdb, where a failure would clear the datadir and spend the one-shot master password. PostgreSQL caps the name at NAMEDATALEN-1. --- docs/COMMANDS.md | 2 +- scripts/images/rds-postgres/rds-init | 18 ++++++++++ scripts/images/rds-postgres/rds-init_test.sh | 30 +++++++++++++++- spinifex/handlers/rds/create_test.go | 4 +++ spinifex/handlers/rds/engine.go | 38 ++++++++++++++++++++ spinifex/handlers/rds/engine_postgres.go | 3 +- spinifex/handlers/rds/engine_test.go | 33 +++++++++++++++++ spinifex/handlers/rds/enginefamily_test.go | 1 + spinifex/handlers/rds/validate.go | 6 +++- 9 files changed, 131 insertions(+), 4 deletions(-) diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 5a979adac..28cceb62a 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -863,7 +863,7 @@ Statuses: `creating`, `available`, `modifying`, `backing-up`, `rebooting`, `stop | Command | Implemented Flags | Missing Flags | Status | |---------|-------------------|---------------|--------| -| `create-db-instance` | `--db-instance-identifier`, `--engine` (postgres), `--engine-version` (18), `--db-instance-class`, `--allocated-storage` (20–65536 GiB), `--storage-type` (gp3), `--master-username`, `--master-user-password`, `--db-name`, `--port` (1150–65535), `--db-subnet-group-name` (unnamed → the account's default VPC subnet), `--vpc-security-group-ids`, `--db-parameter-group-name`, `--backup-retention-period` (0–7, default 7), `--preferred-backup-window`, `--preferred-maintenance-window` (unnamed → assigned from the identifier), `--deletion-protection`, `--tags`, `--storage-encrypted` (true only) | `--auto-minor-version-upgrade`, `--copy-tags-to-snapshot`, Performance Insights / Enhanced Monitoring flags (all accepted, no-op); see "Rejected Parameters" for those that fail loudly | **DONE** | +| `create-db-instance` | `--db-instance-identifier`, `--engine` (postgres), `--engine-version` (18), `--db-instance-class`, `--allocated-storage` (20–65536 GiB), `--storage-type` (gp3), `--master-username`, `--master-user-password`, `--db-name` (letters, digits and underscores, beginning with a letter, max 63), `--port` (1150–65535), `--db-subnet-group-name` (unnamed → the account's default VPC subnet), `--vpc-security-group-ids`, `--db-parameter-group-name`, `--backup-retention-period` (0–7, default 7), `--preferred-backup-window`, `--preferred-maintenance-window` (unnamed → assigned from the identifier), `--deletion-protection`, `--tags`, `--storage-encrypted` (true only) | `--auto-minor-version-upgrade`, `--copy-tags-to-snapshot`, Performance Insights / Enhanced Monitoring flags (all accepted, no-op); see "Rejected Parameters" for those that fail loudly | **DONE** | | `describe-db-instances` | `--db-instance-identifier` | `--filters`, `--max-records`, `--marker` (parsed, not applied) | **DONE** | | `modify-db-instance` | `--db-instance-identifier`, `--master-user-password`, `--allocated-storage` (grow only, stop/start), `--db-instance-class` (VM replace), `--db-parameter-group-name`, `--vpc-security-group-ids`, `--deletion-protection`, `--backup-retention-period`, `--preferred-backup-window`, `--preferred-maintenance-window`, `--apply-immediately` | `--new-db-instance-identifier`, `--engine-version`, `--db-port-number`, `--db-subnet-group-name`, `--max-allocated-storage`, `--ca-certificate-identifier` (all rejected, not ignored) | **DONE** | | `delete-db-instance` | `--db-instance-identifier`, `--skip-final-snapshot`, `--final-db-snapshot-identifier` (exactly one is required) | `--delete-automated-backups` (no-op — automated snapshots are always purged with the instance) | **DONE** — `DeletionProtection` blocks the call; a final snapshot **retains** the data volume until that snapshot is deleted | diff --git a/scripts/images/rds-postgres/rds-init b/scripts/images/rds-postgres/rds-init index eca6defc0..84a135fee 100644 --- a/scripts/images/rds-postgres/rds-init +++ b/scripts/images/rds-postgres/rds-init @@ -71,6 +71,10 @@ MOUNTS_FILE="${RDS_MOUNTS_FILE:-/proc/mounts}" # AWS, so a customer's `GRANT rds_superuser TO ...` works here too. MASTER_GROUP_ROLE="rds_superuser" +# NAMEDATALEN-1: the server truncates a longer identifier, so the database +# created would not be the one the customer asked for. +MAX_DB_NAME_LEN=63 + log() { echo "[rds-init] $*"; } fail() { echo "[rds-init] ERROR: $*" >&2; exit 1; } @@ -134,6 +138,20 @@ load_handoff() { ;; esac + # The same rule the control plane applies, for the same reason the reserved + # name is re-checked: an unusable name reaches initdb, fails at CREATE + # DATABASE and spends the one-shot password on the way. psql quotes the + # identifier, so here this is defence in depth rather than the barrier. + case "${RDS_DB_NAME}" in + '') ;; + [!A-Za-z]* | *[!A-Za-z0-9_]*) + fail "RDS_DB_NAME ${RDS_DB_NAME} must begin with a letter and hold only letters, digits and underscores; the control plane should have rejected it" + ;; + esac + if [ "${#RDS_DB_NAME}" -gt "${MAX_DB_NAME_LEN}" ]; then + fail "RDS_DB_NAME is longer than the ${MAX_DB_NAME_LEN} characters PostgreSQL allows; the control plane should have rejected it" + fi + case "${RDS_PORT}" in '' | *[!0-9]*) fail "bootstrap config has a non-numeric RDS_PORT: ${RDS_PORT}" ;; esac diff --git a/scripts/images/rds-postgres/rds-init_test.sh b/scripts/images/rds-postgres/rds-init_test.sh index 227060b03..a93bda521 100644 --- a/scripts/images/rds-postgres/rds-init_test.sh +++ b/scripts/images/rds-postgres/rds-init_test.sh @@ -207,7 +207,10 @@ write_handoff() { # 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" + # Quoted as rds-agent's shellQuote writes it, so a name carrying a space + # or a shell metacharacter survives the handoff and reaches the check + # that exists for it rather than being mangled on the way. + [ -n "$3" ] && echo "RDS_DB_NAME='$3'" [ -n "${PENDING:-}" ] && echo "RDS_BOOTSTRAP_PENDING=${PENDING}" [ -n "${PAYLOAD_ID:-}" ] && echo "RDS_PAYLOAD_ID=${PAYLOAD_ID}" # Last, and unconditional: an optional line failing its test would make @@ -811,6 +814,31 @@ for reserved in postgres rds_superuser rdsadmin pg_toast_owner PostGres; do unset MASTER_USER done +# --- Case 10: an initial database name the control plane would have refused --- +# The name is interpolated into a CREATE DATABASE, and a failure there is a +# failure after initdb: the datadir is cleared and the one-shot password is +# gone. Refusing before initdb is what keeps the create retryable. +for badname in 'my-db' 'my db' 'my/db' 'my.db' '1db' \ + 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; do + reset_state + write_handoff initialize 's3cr3t' "${badname}" + run_fails "bad-dbname-${badname}" + [ -s "${INITDB_CALLS}" ] \ + && fail "bad-dbname-${badname}: initdb ran before the name was refused" \ + || pass "bad-dbname-${badname}: refused before initdb" +done + +# The rule is a rejection of malformed names, not of every name: the boundary +# length and an underscore have to keep working. +reset_state +write_handoff initialize 's3cr3t' \ + 'a_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa' +write_parameters +if run_ok "dbname-at-the-limit"; then + grep -q 'CREATE DATABASE' "${PSQL_CALLS}" \ + && pass "dbname-at-the-limit: accepted" || fail "dbname-at-the-limit: no CREATE DATABASE" +fi + if [ "${FAILS}" -eq 0 ]; then echo "PASS: all rds-init cases" exit 0 diff --git a/spinifex/handlers/rds/create_test.go b/spinifex/handlers/rds/create_test.go index 3f71b3545..d97cf71c2 100644 --- a/spinifex/handlers/rds/create_test.go +++ b/spinifex/handlers/rds/create_test.go @@ -594,6 +594,10 @@ func TestValidateCreateRequest_RejectsMalformedRequests(t *testing.T) { {"PortTooLow", func(in *rds.CreateDBInstanceInput) { in.Port = aws.Int64(80) }, awserrors.ErrorInvalidParameterValue}, {"ReservedUsername", func(in *rds.CreateDBInstanceInput) { in.MasterUsername = aws.String("rdsadmin") }, awserrors.ErrorInvalidParameterValue}, {"ShortPassword", func(in *rds.CreateDBInstanceInput) { in.MasterUserPassword = aws.String("short") }, awserrors.ErrorInvalidParameterValue}, + {"DBNameHyphen", func(in *rds.CreateDBInstanceInput) { in.DBName = aws.String("my-db") }, awserrors.ErrorInvalidParameterValue}, + {"DBNameTooLong", func(in *rds.CreateDBInstanceInput) { + in.DBName = aws.String(strings.Repeat("a", 64)) + }, awserrors.ErrorInvalidParameterValue}, } for _, tc := range cases { diff --git a/spinifex/handlers/rds/engine.go b/spinifex/handlers/rds/engine.go index 41bbfa857..fd1d50a67 100644 --- a/spinifex/handlers/rds/engine.go +++ b/spinifex/handlers/rds/engine.go @@ -26,6 +26,9 @@ type Engine struct { // The engine's own identifier limit for a role name. The character rule is // shared across engines; the length is not. maxUsernameLen int + // The engine's rule for an initial database name, which the guest + // interpolates into a CREATE DATABASE. + validateDBName func(string) error // The engine's parameter table, keyed by parameter name. The generic spec // machinery is shared; only the table and its formulas are per-engine. @@ -47,6 +50,11 @@ var enginesByFamily = indexEnginesByFamily() func indexEnginesByFamily() map[string]Engine { out := make(map[string]Engine, len(engines)) for _, engine := range engines { + // An engine registered without its own name rule would panic on the first + // create that names a database, rather than here where every build sees it. + if engine.validateDBName == nil { + panic("rds: engine " + engine.Name + " registers no DBName rule") + } family := engine.ParameterGroupFamily() if _, exists := out[family]; exists { panic("rds: parameter group family " + family + " is claimed by two engines") @@ -160,6 +168,36 @@ func (e Engine) ValidateUsernameNotReserved(username string) error { return nil } +// The initial database, which AWS leaves optional: an empty name creates no +// database at all rather than one named by default. +func (e Engine) ValidateDBName(name string) error { + return e.validateDBName(name) +} + +// The shared rule, taking each engine's own identifier limit. The character set +// is narrower than either engine accepts, because it is also what makes the name +// safe to interpolate into the CREATE DATABASE rds-init builds inside the guest. +func dbNameRule(maxLen int) func(string) error { + return func(name string) error { + switch { + case name == "": + return nil + case len(name) > maxLen: + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DBName must be at most %d characters", maxLen) + case !isLetter(rune(name[0])): + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "DBName must begin with a letter") + } + for _, r := range name { + if !isLetter(r) && !isDigit(r) && r != '_' { + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "DBName may contain only letters, digits and underscores") + } + } + return nil + } +} + // 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. diff --git a/spinifex/handlers/rds/engine_postgres.go b/spinifex/handlers/rds/engine_postgres.go index 94e6e3de9..885b612c7 100644 --- a/spinifex/handlers/rds/engine_postgres.go +++ b/spinifex/handlers/rds/engine_postgres.go @@ -20,8 +20,9 @@ var enginePostgres = Engine{ reservedUsernames: []string{"rdsadmin", "postgres", "rds_superuser"}, reservedUsernamePrefixes: []string{"pg_"}, // NAMEDATALEN-1, where the engine's own limit and AWS's documented one - // coincide. + // coincide. A database name is the same identifier limit. maxUsernameLen: 63, + validateDBName: dbNameRule(63), catalog: postgresParameterCatalog, validateCombinations: validatePostgresParameterCombinations, } diff --git a/spinifex/handlers/rds/engine_test.go b/spinifex/handlers/rds/engine_test.go index dd4580681..04b86bc23 100644 --- a/spinifex/handlers/rds/engine_test.go +++ b/spinifex/handlers/rds/engine_test.go @@ -75,6 +75,39 @@ func TestEngineValidateMasterUsername(t *testing.T) { } } +// The name is interpolated into a CREATE DATABASE in the guest, so the rule is +// narrower than the engine's own: it is what makes the interpolation safe. +func TestEngineValidateDBName(t *testing.T) { + engine, err := LookupEngine("postgres") + require.NoError(t, err) + + // Empty is not a rejection: AWS creates no initial database when none is named. + for _, name := range []string{"", "orders", "order_db1", "Orders"} { + assert.NoError(t, engine.ValidateDBName(name), "DBName %q", name) + } + + cases := map[string]string{ + "hyphen": "my-db", + "leading digit": "1db", + "space": "my db", + "dot": "my.db", + // MariaDB maps a database name onto a directory name, so these are + // structurally dangerous there rather than merely awkward. + "slash": "my/db", + "backslash": `my\db`, + "trailing space": "orders ", + "quote": "orders'", + "too long": strings.Repeat("a", 64), + } + for name, dbName := range cases { + t.Run(name, func(t *testing.T) { + err := engine.ValidateDBName(dbName) + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorInvalidParameterValue) + }) + } +} + func TestValidateMasterUserPassword(t *testing.T) { assert.NoError(t, ValidateMasterUserPassword("Sup3rSecret!")) assert.NoError(t, ValidateMasterUserPassword(strings.Repeat("x", maxMasterPasswordLen))) diff --git a/spinifex/handlers/rds/enginefamily_test.go b/spinifex/handlers/rds/enginefamily_test.go index c616ede7b..19a58bff7 100644 --- a/spinifex/handlers/rds/enginefamily_test.go +++ b/spinifex/handlers/rds/enginefamily_test.go @@ -27,6 +27,7 @@ var engineUnderTest = Engine{ reservedUsernames: []string{"root"}, reservedUsernamePrefixes: []string{"testdb_"}, maxUsernameLen: 80, + validateDBName: dbNameRule(64), catalog: buildParameterCatalog( ParameterSpec{ Name: "buffer_size", DataType: paramTypeInteger, ApplyType: ApplyTypeStatic, diff --git a/spinifex/handlers/rds/validate.go b/spinifex/handlers/rds/validate.go index 22da76132..1d9cd9872 100644 --- a/spinifex/handlers/rds/validate.go +++ b/spinifex/handlers/rds/validate.go @@ -117,6 +117,10 @@ func (s *Service) validateCreateRequest(input *rds.CreateDBInstanceInput) (*vali if err := ValidateMasterUserPassword(masterPassword); err != nil { return nil, err } + dbName := aws.StringValue(input.DBName) + if err := engine.ValidateDBName(dbName); err != nil { + return nil, err + } port := engine.DefaultPort if input.Port != nil { @@ -168,7 +172,7 @@ func (s *Service) validateCreateRequest(input *rds.CreateDBInstanceInput) (*vali Port: port, MasterUsername: masterUsername, MasterPassword: masterPassword, - DBName: aws.StringValue(input.DBName), + DBName: dbName, SecurityGroupIDs: aws.StringValueSlice(input.VpcSecurityGroupIds), DBSubnetGroupName: aws.StringValue(input.DBSubnetGroupName), DBParameterGroupName: paramGroup,