diff --git a/cmd/rds-agent/engine.go b/cmd/rds-agent/engine.go index ae2565a6d..d136ec984 100644 --- a/cmd/rds-agent/engine.go +++ b/cmd/rds-agent/engine.go @@ -155,6 +155,12 @@ func (e *postgresEngine) SetPassword(ctx context.Context, username, password str return fmt.Errorf("set-password requires both %s and %s", handlers_rds.CommandParamMasterUsername, handlers_rds.CommandParamMasterUserPassword) } + // The statement below runs as the cluster superuser over the socket under + // peer auth, so a reserved name in the command payload would hand the + // customer the bootstrap superuser rather than rotate their own role. + 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 \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 d82ea52b1..d209cb694 100644 --- a/cmd/rds-agent/engine_test.go +++ b/cmd/rds-agent/engine_test.go @@ -61,7 +61,7 @@ func TestCommandRegistry_SetPasswordCarriesBothParameters(t *testing.T) { CommandID: "cmd-1", Type: handlers_rds.CommandSetPassword, Parameters: []handlers_rds.Parameter{ - {Name: handlers_rds.CommandParamMasterUsername, Value: "postgres"}, + {Name: handlers_rds.CommandParamMasterUsername, Value: "mulgamaster"}, {Name: handlers_rds.CommandParamMasterUserPassword, Value: "n3w-pw"}, }, }) @@ -69,8 +69,8 @@ func TestCommandRegistry_SetPasswordCarriesBothParameters(t *testing.T) { if reply.Status != handlers_rds.CommandStatusSucceeded { t.Fatalf("reply = %+v, want succeeded", reply) } - if engine.username != "postgres" || engine.password != "n3w-pw" { - t.Errorf("engine got %q/%q, want postgres/n3w-pw", engine.username, engine.password) + if engine.username != "mulgamaster" || engine.password != "n3w-pw" { + t.Errorf("engine got %q/%q, want mulgamaster/n3w-pw", engine.username, engine.password) } // The reply is recorded by the control plane; the new password must not be // in it. @@ -162,7 +162,7 @@ func TestPostgresEngine_SetPasswordKeepsTheSecretOutOfArgv(t *testing.T) { runner := &recordingRunner{} engine := newTestEngine(t, runner.run) - if err := engine.SetPassword(context.Background(), "postgres", "n3w-pw"); err != nil { + if err := engine.SetPassword(context.Background(), "mulgamaster", "n3w-pw"); err != nil { t.Fatalf("SetPassword: %v", err) } if len(runner.calls) != 1 { @@ -191,7 +191,7 @@ func TestPostgresEngine_SetPasswordRedactsTheSecretFromAFailure(t *testing.T) { runner := &recordingRunner{err: errors.New(`ERROR: syntax error at or near "n3w-pw"`)} engine := newTestEngine(t, runner.run) - err := engine.SetPassword(context.Background(), "postgres", "n3w-pw") + err := engine.SetPassword(context.Background(), "mulgamaster", "n3w-pw") if err == nil { t.Fatal("SetPassword succeeded against a failing psql") } @@ -207,7 +207,7 @@ func TestPostgresEngine_SetPasswordRejectsAnIncompleteCommand(t *testing.T) { runner := &recordingRunner{} engine := newTestEngine(t, runner.run) - if err := engine.SetPassword(context.Background(), "postgres", ""); err == nil { + if err := engine.SetPassword(context.Background(), "mulgamaster", ""); err == nil { t.Error("SetPassword accepted an empty password") } if len(runner.calls) != 0 { @@ -215,6 +215,26 @@ func TestPostgresEngine_SetPasswordRejectsAnIncompleteCommand(t *testing.T) { } } +// D21: the apply runs as the cluster superuser under peer auth, so a reserved +// name arriving in a command payload would rotate the bootstrap superuser's +// password rather than the customer's own role. +func TestPostgresEngine_SetPasswordRefusesReservedRoles(t *testing.T) { + for _, username := range []string{"postgres", "rds_superuser", "rdsadmin", "pg_toast_owner", "PostGres"} { + t.Run(username, func(t *testing.T) { + runner := &recordingRunner{} + engine := newTestEngine(t, runner.run) + + err := engine.SetPassword(context.Background(), username, "n3w-pw") + if err == nil { + t.Fatalf("SetPassword accepted the reserved role %q", username) + } + if len(runner.calls) != 0 { + t.Errorf("ran %d commands for a reserved role, want 0", len(runner.calls)) + } + }) + } +} + // The parameters land where rds-init installs them, so the next boot overwrites // the file rather than shadowing it with a second copy. func TestPostgresEngine_ApplyParametersInstallsAndReloads(t *testing.T) { diff --git a/scripts/images/rds-postgres/rds-init b/scripts/images/rds-postgres/rds-init index 5b2502875..ab8701b62 100644 --- a/scripts/images/rds-postgres/rds-init +++ b/scripts/images/rds-postgres/rds-init @@ -98,9 +98,14 @@ load_handoff() { # The master role is created NOSUPERUSER, so a master named after the # bootstrap superuser would leave the cluster without one and strip rds-agent # of the privileged SQL it reaches over peer auth — unrepairable, since the - # datadir bootstraps once. The control plane reserves both names already. - case "${RDS_MASTER_USERNAME}" in - "${PG_USER}" | "${MASTER_GROUP_ROLE}") + # datadir bootstraps once. + # + # The same set the control plane reserves, matched the same way: anything it + # refuses must be refused here too, or the name reaches initdb, fails at role + # creation and spends the one-shot password on the way. + _master_lower=$(printf '%s' "${RDS_MASTER_USERNAME}" | tr '[:upper:]' '[:lower:]') + case "${_master_lower}" in + "${PG_USER}" | "${MASTER_GROUP_ROLE}" | rdsadmin | pg_*) fail "RDS_MASTER_USERNAME ${RDS_MASTER_USERNAME} is reserved by the platform; the control plane should have rejected it" ;; esac diff --git a/scripts/images/rds-postgres/rds-init_test.sh b/scripts/images/rds-postgres/rds-init_test.sh index 7b0626335..53433c6e3 100644 --- a/scripts/images/rds-postgres/rds-init_test.sh +++ b/scripts/images/rds-postgres/rds-init_test.sh @@ -508,7 +508,9 @@ run_fails "no-handoff" # The master role is created NOSUPERUSER, so one named after the bootstrap # superuser would leave the cluster without a superuser at all and strip # rds-agent of the privileged SQL it needs — on a datadir that bootstraps once. -for reserved in postgres rds_superuser; do +# The set and the case-insensitive matching are the control plane's: a name it +# refuses must not reach initdb here and spend the one-shot password. +for reserved in postgres rds_superuser rdsadmin pg_toast_owner PostGres; do reset_state MASTER_USER="${reserved}" write_handoff initialize 's3cr3t' appdb diff --git a/spinifex/handlers/rds/engine.go b/spinifex/handlers/rds/engine.go index 3f02e40f3..ddc0269d0 100644 --- a/spinifex/handlers/rds/engine.go +++ b/spinifex/handlers/rds/engine.go @@ -96,7 +96,14 @@ func (e Engine) ValidateMasterUsername(username string) error { "MasterUsername may contain only letters, digits and underscores") } } - lower := strings.ToLower(username) + return e.ValidateUsernameNotReserved(username) +} + +// The reserved-role half of the check on its own, exported for the in-guest +// agent: its live password apply runs as the cluster superuser, so it re-checks +// the name it is handed rather than trusting the control plane to have done it. +func (e Engine) ValidateUsernameNotReserved(username string) error { + lower := strings.ToLower(strings.TrimSpace(username)) if slices.Contains(e.reservedUsernames, lower) { return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "MasterUsername %q is reserved by %s", username, e.Name) diff --git a/spinifex/handlers/rds/engine_postgres.go b/spinifex/handlers/rds/engine_postgres.go index 147716d2f..cf43309e3 100644 --- a/spinifex/handlers/rds/engine_postgres.go +++ b/spinifex/handlers/rds/engine_postgres.go @@ -13,3 +13,8 @@ var enginePostgres = Engine{ reservedUsernames: []string{"rdsadmin", "postgres", "rds_superuser"}, reservedUsernamePrefixes: []string{"pg_"}, } + +// The pinned engine as a value, for the in-guest agent: it enforces the same +// reserved-role set before altering a role as the cluster superuser, and a +// lookup by name there could only fail silently into an empty set. +func EnginePostgres() Engine { return enginePostgres }