From bdc79108e9de498d9b4371fa74405bc6bc80e0cc Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Wed, 19 Aug 2026 11:04:21 +1000 Subject: [PATCH] fix: make the DB endpoint the only path to the engine (#830) --- cmd/rds-agent/agent_test.go | 9 + cmd/rds-agent/bootstrap.go | 11 ++ cmd/rds-agent/bootstrap_test.go | 10 ++ cmd/rds-agent/config.go | 6 - cmd/rds-agent/config_test.go | 4 +- cmd/rds-agent/engine_postgres.go | 7 +- cmd/rds-agent/health_test.go | 35 +++- docs/COMMANDS.md | 2 + .../rds-quickstart/README.md | 2 + scripts/images/rds-mariadb/rds-agent.initd | 2 +- scripts/images/rds-mariadb/rds-init | 72 +++++++- scripts/images/rds-mariadb/rds-init_test.sh | 69 +++++++- scripts/images/rds-postgres/rds-agent.initd | 2 +- scripts/images/rds-postgres/rds-init | 101 ++++++++++- scripts/images/rds-postgres/rds-init_test.sh | 118 ++++++++++++- spinifex/handlers/rds/agent.go | 52 ++++++ spinifex/handlers/rds/agent_test.go | 73 +++++++- spinifex/handlers/rds/create.go | 2 + spinifex/handlers/rds/create_test.go | 27 ++- spinifex/handlers/rds/launch.go | 25 ++- spinifex/handlers/rds/launch_test.go | 113 +++++++++++- spinifex/handlers/rds/lifecycle_test.go | 14 +- spinifex/handlers/rds/network.go | 35 +++- spinifex/handlers/rds/reconciler.go | 106 +++++++++++- spinifex/handlers/rds/reconciler_test.go | 121 +++++++++++++ spinifex/handlers/rds/records.go | 12 +- spinifex/handlers/rds/replace.go | 2 + spinifex/handlers/rds/vpc.go | 93 +++++++++- tests/e2e/harness/rds.go | 87 ++++++++++ tests/e2e/rds/isolation_test.go | 163 ++++++++++++++++++ 30 files changed, 1317 insertions(+), 58 deletions(-) create mode 100644 tests/e2e/rds/isolation_test.go diff --git a/cmd/rds-agent/agent_test.go b/cmd/rds-agent/agent_test.go index 7b9f2944b..410bd74db 100644 --- a/cmd/rds-agent/agent_test.go +++ b/cmd/rds-agent/agent_test.go @@ -207,6 +207,8 @@ func bootstrapOutput(password *string) *handlers_rds.GetDBBootstrapConfigOutput MasterUsername: "master", MasterUserPassword: password, Port: 5432, + ListenAddress: testListenAddress, + ClientCIDR: testClientCIDR, DataVolumeID: "vol-data-01", DataVolumeSerial: "voldata01", VMGeneration: 1, @@ -217,6 +219,13 @@ func bootstrapOutput(password *string) *handlers_rds.GetDBBootstrapConfigOutput const testPayloadID = "bp-0123456789abcdef" +// The customer ENI address the engine binds, and the VPC range its client +// authentication admits. writeHandoff refuses a config without either. +const ( + testListenAddress = "10.0.1.20" + testClientCIDR = "10.0.0.0/16" +) + // Writes the completion receipt rds-init leaves behind, so the acknowledgement // step finds what a finished initialization would have produced. func writeReceipt(t *testing.T, a *Agent, payloadID, dbInstanceIdentifier string) { diff --git a/cmd/rds-agent/bootstrap.go b/cmd/rds-agent/bootstrap.go index 2502768de..d2368741e 100644 --- a/cmd/rds-agent/bootstrap.go +++ b/cmd/rds-agent/bootstrap.go @@ -96,6 +96,15 @@ func writeHandoff(dir string, cfg *handlers_rds.GetDBBootstrapConfigOutput) erro if cfg.MasterUsername == "" { return fmt.Errorf("bootstrap config carries no master username") } + // Half a handoff would leave rds-init deciding what to bind and who to admit, + // and the only answers it could reach on its own are the wildcards this + // contract exists to remove. + if cfg.ListenAddress == "" { + return fmt.Errorf("bootstrap config carries no listen address for the engine to bind") + } + if cfg.ClientCIDR == "" { + return fmt.Errorf("bootstrap config carries no client CIDR to scope client authentication to") + } if err := os.MkdirAll(dir, handoffDirMode); err != nil { return fmt.Errorf("create handoff dir %s: %w", dir, err) } @@ -134,6 +143,8 @@ func renderBootstrapEnv(cfg *handlers_rds.GetDBBootstrapConfigOutput) string { writeEnvLine(&b, "RDS_MODE", cfg.Mode) writeEnvLine(&b, "RDS_DB_INSTANCE_IDENTIFIER", cfg.DBInstanceIdentifier) writeEnvLine(&b, "RDS_MASTER_USERNAME", cfg.MasterUsername) + writeEnvLine(&b, "RDS_LISTEN_ADDRESS", cfg.ListenAddress) + writeEnvLine(&b, "RDS_CLIENT_CIDR", cfg.ClientCIDR) writeEnvLine(&b, "RDS_DATA_VOLUME_ID", cfg.DataVolumeID) writeEnvLine(&b, "RDS_DATA_VOLUME_SERIAL", cfg.DataVolumeSerial) writeEnvLine(&b, "RDS_VM_GENERATION", strconv.FormatInt(cfg.VMGeneration, 10)) diff --git a/cmd/rds-agent/bootstrap_test.go b/cmd/rds-agent/bootstrap_test.go index 523e6f11c..6544700a3 100644 --- a/cmd/rds-agent/bootstrap_test.go +++ b/cmd/rds-agent/bootstrap_test.go @@ -18,6 +18,8 @@ func TestWriteHandoff_WritesEveryFileRootOnly(t *testing.T) { Mode: handlers_rds.BootstrapModeInitialize, Engine: "postgres", MasterUsername: "master", + ListenAddress: "10.0.1.20", + ClientCIDR: "10.0.0.0/16", MasterUserPassword: &password, DBName: "appdb", Port: 5433, @@ -85,6 +87,8 @@ func TestWriteHandoff_AttachOmitsPassword(t *testing.T) { Mode: handlers_rds.BootstrapModeAttach, Engine: "postgres", MasterUsername: "master", + ListenAddress: "10.0.1.20", + ClientCIDR: "10.0.0.0/16", Port: 5432, }); err != nil { t.Fatalf("writeHandoff: %v", err) @@ -116,6 +120,8 @@ func TestWriteHandoff_QuotesShellMetacharacters(t *testing.T) { Mode: handlers_rds.BootstrapModeInitialize, Engine: "postgres", MasterUsername: "master", + ListenAddress: "10.0.1.20", + ClientCIDR: "10.0.0.0/16", MasterUserPassword: &password, Port: 5432, }); err != nil { @@ -137,6 +143,8 @@ func TestWriteHandoff_SkipsHalfATLSPair(t *testing.T) { Mode: handlers_rds.BootstrapModeAttach, Engine: "postgres", MasterUsername: "master", + ListenAddress: "10.0.1.20", + ClientCIDR: "10.0.0.0/16", Port: 5432, ServingCertificate: "CERT", }); err != nil { @@ -177,6 +185,8 @@ func TestWriteHandoff_TightensExistingDirectory(t *testing.T) { Mode: handlers_rds.BootstrapModeAttach, Engine: "postgres", MasterUsername: "master", + ListenAddress: "10.0.1.20", + ClientCIDR: "10.0.0.0/16", Port: 5432, }); err != nil { t.Fatalf("writeHandoff: %v", err) diff --git a/cmd/rds-agent/config.go b/cmd/rds-agent/config.go index 3473cc841..b4cda5c08 100644 --- a/cmd/rds-agent/config.go +++ b/cmd/rds-agent/config.go @@ -13,7 +13,6 @@ const ( defaultEnvFile = "/etc/spinifex-rds/agent.env" defaultGatewayCA = "/etc/spinifex-rds/gateway-ca.pem" defaultHandoffDir = "/run/spinifex-rds" - defaultEngineHost = "127.0.0.1" // Where setup.sh stamps the engine the image bakes. The agent builds its // engine implementation from this file rather than from anything delivered. defaultEngineFile = "/etc/spinifex-rds/engine" @@ -38,7 +37,6 @@ type config struct { // version alone rather than clearing it. EngineVersion string HandoffDir string - EngineHost string EnginePort int PollWait time.Duration @@ -89,7 +87,6 @@ func loadConfig(envFile string) config { EngineFile: get("RDS_ENGINE_FILE"), EngineVersion: get("RDS_ENGINE_VERSION"), HandoffDir: get("RDS_HANDOFF_DIR"), - EngineHost: get("RDS_ENGINE_HOST"), EngineBinDir: get("RDS_ENGINE_BIN"), EngineDataDir: get("RDS_ENGINE_DATA"), SocketDir: get("RDS_SOCKET_DIR"), @@ -108,9 +105,6 @@ func loadConfig(envFile string) config { if cfg.HandoffDir == "" { cfg.HandoffDir = defaultHandoffDir } - if cfg.EngineHost == "" { - cfg.EngineHost = defaultEngineHost - } if cfg.EngineFile == "" { cfg.EngineFile = defaultEngineFile } diff --git a/cmd/rds-agent/config_test.go b/cmd/rds-agent/config_test.go index be43f70dc..e6414e0d8 100644 --- a/cmd/rds-agent/config_test.go +++ b/cmd/rds-agent/config_test.go @@ -26,8 +26,8 @@ func TestLoadConfig_Defaults(t *testing.T) { if cfg.GatewayCA != defaultGatewayCA || cfg.HandoffDir != defaultHandoffDir { t.Errorf("CA/handoff = %q/%q, want the built-in defaults", cfg.GatewayCA, cfg.HandoffDir) } - if cfg.EngineHost != defaultEngineHost || cfg.PollWait != defaultPollWait { - t.Errorf("engine host/pollWait = %q/%v, want the built-in defaults", cfg.EngineHost, cfg.PollWait) + if cfg.PollWait != defaultPollWait { + t.Errorf("pollWait = %v, want the built-in default", cfg.PollWait) } if cfg.EngineProbeTimeout != defaultEngineProbeTimeout { t.Errorf("engine probe timeout = %v, want %v", cfg.EngineProbeTimeout, defaultEngineProbeTimeout) diff --git a/cmd/rds-agent/engine_postgres.go b/cmd/rds-agent/engine_postgres.go index 172d5df5b..ca7de963b 100644 --- a/cmd/rds-agent/engine_postgres.go +++ b/cmd/rds-agent/engine_postgres.go @@ -41,8 +41,13 @@ var _ engine = (*postgresEngine)(nil) // is not there. Resolved on PATH, where the client package puts it. const postgresProbeBinary = "pg_isready" +// The socket directory rather than loopback: the engine binds the customer ENI +// and nothing else, so there is no TCP path for the probe to take. It also keeps +// liveness independent of the network path, of the pg_hba scope and of the TLS +// enforcement rule — libpq reads a host beginning with / as a socket directory, +// and the generated `local ... peer` line covers it. func newPostgresProbe(cfg config, run probeRunner) *engineProbe { - return newEngineProbe(cfg.EnginePort, postgresProbeState(cfg.EngineHost, run)) + return newEngineProbe(cfg.EnginePort, postgresProbeState(cfg.SocketDir, run)) } func postgresProbeState(host string, run probeRunner) probeStateFn { diff --git a/cmd/rds-agent/health_test.go b/cmd/rds-agent/health_test.go index 25c92db1f..d76b129d7 100644 --- a/cmd/rds-agent/health_test.go +++ b/cmd/rds-agent/health_test.go @@ -110,6 +110,37 @@ func TestExecProbeRunner_CapturesTheClientsStderr(t *testing.T) { } } -func testProbeConfig() config { - return config{EngineHost: defaultEngineHost, EnginePort: engineLayouts[enginePostgres].port} +// The engine binds the customer ENI and nothing else, so a loopback probe would +// report a serving engine as absent. libpq reads a host beginning with / as a +// socket directory, which is also what keeps liveness off the network path. +func TestPostgresProbe_ProbesTheUnixSocket(t *testing.T) { + cfg := testProbeConfig() + var probed []string + probe := newPostgresProbe(cfg, func(_ context.Context, name string, args ...string) (int, string, error) { + probed = append([]string{name}, args...) + return 0, "", nil + }) + + if state, _ := probe.state(context.Background()); state != engineServing { + t.Fatalf("probe state = %v, want engineServing", state) + } + host := "" + for i, arg := range probed { + if arg == "-h" && i+1 < len(probed) { + host = probed[i+1] + } + } + if host != cfg.SocketDir { + t.Errorf("probe host = %q, want the socket directory %q", host, cfg.SocketDir) + } + if !strings.HasPrefix(host, "/") { + t.Errorf("probe host = %q, which libpq reads as a TCP host rather than a socket directory", host) + } +} + +func testProbeConfig() config { + return config{ + SocketDir: engineLayouts[enginePostgres].socketDir, + EnginePort: engineLayouts[enginePostgres].port, + } } diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 8e6fe6287..84f888f5d 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -902,6 +902,8 @@ Spinifex both stores externally-issued certificates (`import-certificate`) and i Each DB instance is one dedicated system-owned VM running the engine directly, launched from the `spinifex-rds-postgres` or `spinifex-rds-mariadb` AMI, tagged `spinifex:managed-by=rds` and therefore hidden from the customer's EC2 API. The engine is reached over a customer-account ENI injected into a subnet of the DB subnet group, so **the endpoint is private — reachable from inside the VPC only**, from any subnet of it rather than the endpoint ENI's own. `Endpoint.Address` is `{db-instance-identifier}.{account-id}.{region}.rds.{base-domain}` where northstar is configured, and the endpoint ENI's private IP where it is not; the IP is stable across VM replacement either way. +**The endpoint is the only path to the engine.** The DB VM has two other NICs that no customer security group governs — a system NIC in a shared RDS system VPC, which is how the in-guest agent reaches the control plane, and a management NIC on the host's management bridge — and the engine binds neither. It binds the endpoint ENI's address alone, so the port is not open on them at all rather than open and gated. The system NIC additionally carries a dedicated region-wide security group that authorizes no ingress; agent traffic is outbound and its replies are matched by connection state. PostgreSQL's generated `pg_hba.conf` admits the customer VPC's address range and nothing wider, so a client outside the VPC is refused authentication even if it somehow reached the socket. MariaDB has no equivalent file: its analogue is the host pattern on a grant, which lives in the data volume and would follow a restore into another VPC, so for that engine the bind is the whole boundary. + Everything below applies to both engines unless it names one. `Engine` is fixed at create — there is no in-place engine change, no cross-engine snapshot restore and no migration between the two. | | `postgres` | `mariadb` | diff --git a/docs/terraform-workbooks/rds-quickstart/README.md b/docs/terraform-workbooks/rds-quickstart/README.md index c51a64821..83daefed6 100644 --- a/docs/terraform-workbooks/rds-quickstart/README.md +++ b/docs/terraform-workbooks/rds-quickstart/README.md @@ -53,6 +53,8 @@ The client subnet is public in that it has an internet-gateway route, which the The database security group admits `5432` from the client security group and nothing else, which compiles to an ACL on the endpoint ENI itself — the port is deny-by-default for everything else in the VPC. +That holds for the instance, not just for the endpoint ENI. The DB VM has two further NICs the platform uses to manage it, and no customer security group governs either; the engine binds the endpoint ENI's address alone, so it is not listening on them. PostgreSQL's client authentication rules are scoped to this VPC's own range as well, so the endpoint really is the whole of the reachable surface. + ## Prerequisites - **Spinifex running**, with the AWS CLI configured for the `spinifex` profile (see [Installing Spinifex](/docs/install)) and OpenTofu (or Terraform) installed. diff --git a/scripts/images/rds-mariadb/rds-agent.initd b/scripts/images/rds-mariadb/rds-agent.initd index a01ff9d41..d3d4cee33 100644 --- a/scripts/images/rds-mariadb/rds-agent.initd +++ b/scripts/images/rds-mariadb/rds-agent.initd @@ -35,7 +35,7 @@ start_pre() { # shellcheck disable=SC1090 . "${AGENT_ENV}" export RDS_GATEWAY_URL RDS_GATEWAY_CA RDS_REGION RDS_DB_INSTANCE_IDENTIFIER \ - RDS_ENGINE RDS_ENGINE_VERSION RDS_HANDOFF_DIR RDS_ENGINE_HOST RDS_ENGINE_PORT \ + RDS_ENGINE RDS_ENGINE_VERSION RDS_HANDOFF_DIR RDS_ENGINE_PORT \ RDS_POLL_WAIT RDS_DATA_MOUNT fi } diff --git a/scripts/images/rds-mariadb/rds-init b/scripts/images/rds-mariadb/rds-init index b5af000cc..35695b1c1 100644 --- a/scripts/images/rds-mariadb/rds-init +++ b/scripts/images/rds-mariadb/rds-init @@ -14,6 +14,7 @@ # RDS_MASTER_PASSWORD= # RDS_DB_NAME= # RDS_PORT= +# RDS_LISTEN_ADDRESS= # RDS_DATA_VOLUME_ID / RDS_DATA_VOLUME_SERIAL= # RDS_VM_GENERATION / RDS_FORMAT_AUTHORIZED= # RDS_BOOTSTRAP_PENDING / RDS_PAYLOAD_ID=&2; exit 1; } +# A dotted-quad IPv4 literal. bind_address would also take a hostname, but the +# control plane sends an address, and accepting a name would reintroduce the +# resolution step the bind exists to remove. +is_ipv4() { + case "$1" in + '' | *[!0-9.]*) return 1 ;; + esac + _rest="$1" + _octets=0 + while [ -n "${_rest}" ]; do + _octet="${_rest%%.*}" + case "${_rest}" in + *.*) _rest="${_rest#*.}" ;; + *) _rest="" ;; + esac + # A leading zero reads as octal to some parsers and decimal to others, + # so the two would disagree about which address was bound. + case "${_octet}" in + 0 | [1-9] | [1-9][0-9] | [1-9][0-9][0-9]) ;; + *) return 1 ;; + esac + [ "${_octet}" -le 255 ] || return 1 + _octets=$((_octets + 1)) + done + [ "${_octets}" = 4 ] +} + +# The engine binds one address and refuses to start if it is not there yet, and +# an engine that will not start for an unexplained reason is the worst way to +# learn that DHCP lost its race. Bounded, then fatal with the address named. +wait_for_listen_address() { + _deadline=$(($(date +%s) + LISTEN_ADDRESS_TIMEOUT)) + while :; do + if ip -4 -o addr show 2>/dev/null | + awk -v want="${RDS_LISTEN_ADDRESS}" \ + '{ split($4, addr, "/"); if (addr[1] == want) found = 1 } END { exit !found }'; then + return 0 + fi + [ "$(date +%s)" -lt "${_deadline}" ] || break + sleep 1 + done + fail "the endpoint address ${RDS_LISTEN_ADDRESS} did not appear on any interface within ${LISTEN_ADDRESS_TIMEOUT}s; the engine binds it and nothing else, so refusing to start" +} + # Between these two, the datadir carries a mysql system database but no usable # master user. _server_pid is the private bootstrap server, if one is running. _created_datadir=0 @@ -199,6 +249,7 @@ load_handoff() { RDS_MASTER_PASSWORD="${RDS_MASTER_PASSWORD:-}" RDS_DB_NAME="${RDS_DB_NAME:-}" RDS_PORT="${RDS_PORT:-3306}" + RDS_LISTEN_ADDRESS="${RDS_LISTEN_ADDRESS:-}" RDS_VM_GENERATION="${RDS_VM_GENERATION:-}" # Absent from an agent that predates the receipt protocol, which reads as # "nothing is pending" and leaves every path below unchanged. @@ -252,6 +303,14 @@ load_handoff() { '' | *[!0-9]*) fail "bootstrap config has a non-numeric RDS_PORT: ${RDS_PORT}" ;; esac + # No fallback to a wildcard. The engine is reachable over the customer ENI + # and nowhere else, and a handoff that cannot say which address that is has + # no safe reading — the DB VM's other two NICs carry no security group the + # customer can see, let alone set. + [ -n "${RDS_LISTEN_ADDRESS}" ] || fail "bootstrap config has no RDS_LISTEN_ADDRESS; refusing to start an engine that would answer on every interface" + is_ipv4 "${RDS_LISTEN_ADDRESS}" || + fail "bootstrap config has a non-IPv4 RDS_LISTEN_ADDRESS: ${RDS_LISTEN_ADDRESS}" + # 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 @@ -537,7 +596,8 @@ EOF # The 90- prefix sorts last within the include directory and MariaDB takes the # last occurrence of a setting, so a resolved parameter can never move the -# endpoint's port, redirect the datadir, turn off TLS or lower the protocol floor. +# endpoint's port, widen the bind, redirect the datadir, turn off TLS or lower +# the protocol floor. # # innodb_buffer_pool_chunk_size and innodb_buffer_pool_instances are deliberately # absent although they are platform-owned: 11.8 removed the second outright, so @@ -554,7 +614,14 @@ write_platform_config() { port = ${RDS_PORT} datadir = ${DATADIR} socket = ${SOCKET} -bind_address = 0.0.0.0 + +# The customer ENI and nothing else. The DB VM's other two NICs sit on the +# shared RDS system VPC and the host's management bridge, neither of which any +# customer security group governs, so a socket bound there is reachable across +# accounts. MariaDB's analogue of a scoped pg_hba is the host pattern on a +# grant, which lives in mysql.user on the data volume and would survive a +# restore into another VPC — so for this engine the bind is the boundary. +bind_address = ${RDS_LISTEN_ADDRESS} # InnoDB is the only storage engine the snapshot guarantee holds for: it keeps a # redo log and recovers from a crash-consistent snapshot, and Aria and MyISAM do @@ -963,6 +1030,7 @@ consume_secrets() { } load_handoff +wait_for_listen_address require_data_volume require_engine_stamp diff --git a/scripts/images/rds-mariadb/rds-init_test.sh b/scripts/images/rds-mariadb/rds-init_test.sh index e9c3cd047..88725bfd2 100755 --- a/scripts/images/rds-mariadb/rds-init_test.sh +++ b/scripts/images/rds-mariadb/rds-init_test.sh @@ -201,6 +201,18 @@ EOF # an installed receipt and an armed sweep coexist. Matched on the receipt # directory by name: the engine stamp syncs its directory too, long before the # traps are armed, and killing there would leave the window untested. +# ip stub: stands in for `ip -4 -o addr show`, printing one line per address in +# IP_ADDR_LIST in the format the bind-address wait parses. +cat > "${STUBBIN}/ip" <<'EOF' +#!/bin/sh +n=1 +for cidr in ${IP_ADDR_LIST:-}; do + echo "${n}: eth$((n - 1)) inet ${cidr} scope global eth$((n - 1))" + n=$((n + 1)) +done +exit 0 +EOF + cat > "${STUBBIN}/sync" <<'EOF' #!/bin/sh if [ "${SYNC_KILL_PARENT:-0}" = "1" ]; then @@ -236,6 +248,9 @@ SECURE_FILE_DIR="${WORK}/mysql-files" LOG_DIR="${WORK}/log" ENGINE_LOG="${LOG_DIR}/error.log" MOUNTS="${WORK}/mounts" + +# The customer ENI address the engine binds, as the control plane delivers it. +LISTEN_ADDRESS_DEFAULT="10.30.1.15" KILL_PID_FILE="${WORK}/rds-init.pid" MARIADBD_TEST_PID_FILE="${WORK}/mariadbd.pid" WORKDIR="${WORK}" @@ -266,6 +281,13 @@ write_handoff() { [ -n "$3" ] && echo "RDS_DB_NAME='$3'" [ -n "${PENDING:-}" ] && echo "RDS_BOOTSTRAP_PENDING=${PENDING}" [ -n "${PAYLOAD_ID:-}" ] && echo "RDS_PAYLOAD_ID=${PAYLOAD_ID}" + # Written unset rather than defaulted when the case is about a handoff + # that omits it, which rds-init has to refuse rather than fill in. + [ "${OMIT_LISTEN_ADDRESS:-0}" = "1" ] || + echo "RDS_LISTEN_ADDRESS=${LISTEN_ADDRESS:-${LISTEN_ADDRESS_DEFAULT}}" + # Delivered but unread here: MariaDB's analogue of a scoped pg_hba is a + # grant host pattern on the data volume, which a restore would carry. + echo "RDS_CLIENT_CIDR=10.30.0.0/16" # Last, and unconditional: an optional line failing its test would make # the whole group's status non-zero and `set -e` would end the run here. echo "RDS_PORT=6543" @@ -327,6 +349,10 @@ reset_state() { unset PENDING PAYLOAD_ID DB_ID RECEIPT_DIR_OVERRIDE CLIENT_STATUS_CORRUPT STAMP_OVERRIDE || true unset MARIADBD_START_FAIL MARIADBD_KILL_PARENT MARIADBD_IGNORE_TERM CLIENT_KILL_PARENT || true unset ADMIN_SHUTDOWN_FAIL SYNC_KILL_PARENT || true + unset LISTEN_ADDRESS OMIT_LISTEN_ADDRESS || true + # The customer ENI carries its address by the time rds-init runs, which is + # the ordinary case; the cases about a late lease drop it themselves. + IP_ADDR_LIST="${LISTEN_ADDRESS_DEFAULT}/24 10.99.0.7/24" rm -f "${MARIADBD_TEST_PID_FILE}" write_tls } @@ -346,6 +372,8 @@ run_env() { RDS_RECEIPT_DIR="${RECEIPT_DIR_OVERRIDE:-${RECEIPT_DIR}}" \ RDS_ENGINE_STAMP="${STAMP_OVERRIDE:-${STAMP}}" \ RDS_ALLOW_LOCAL_DATADIR="${RDS_ALLOW_LOCAL_DATADIR:-0}" \ + RDS_LISTEN_ADDRESS_TIMEOUT="${LISTEN_TIMEOUT:-0}" \ + IP_ADDR_LIST="${IP_ADDR_LIST:-}" \ RDS_BOOTSTRAP_POLL=0.05 \ RDS_BOOTSTRAP_PROBES=200 \ RDS_BOOTSTRAP_STOP_GRACE=0.1 \ @@ -493,9 +521,12 @@ if run_ok "initialize"; then && pass "initialize: delivered port applied" || fail "initialize: port not applied" grep -q "^datadir = ${DATADIR}" "${PLATFORM_FILE}" \ && pass "initialize: datadir pinned by the platform" || fail "initialize: no datadir" - grep -q "^bind_address = 0.0.0.0" "${PLATFORM_FILE}" \ - && pass "initialize: the engine listens for the customer ENI" \ - || fail "initialize: no bind_address" + grep -q "^bind_address = ${LISTEN_ADDRESS_DEFAULT}$" "${PLATFORM_FILE}" \ + && pass "initialize: the engine binds the customer ENI" \ + || fail "initialize: the engine does not bind the delivered address" + grep -qE '^bind_address = (0\.0\.0\.0|\*|::)$' "${PLATFORM_FILE}" \ + && fail "initialize: the engine still answers on every interface" \ + || pass "initialize: no wildcard bind" grep -q "^default_storage_engine = InnoDB" "${PLATFORM_FILE}" \ && pass "initialize: tables land in the engine snapshots recover" \ || fail "initialize: default_storage_engine not pinned" @@ -1233,6 +1264,38 @@ grep -q 'engine stamp' "${WORK}/out" \ && pass "stamp-unwritable: refusal names the stamp" || fail "stamp-unwritable: no refusal message" unset STAMP_OVERRIDE +# --- Case 13: a handoff that cannot say what to bind is refused --- +# The refusal is before the one-shot password is spent, so the create is still +# retryable. The only reading rds-init could reach on its own is the wildcard +# the bind exists to remove. +for _case in omit-listen bad-listen leading-zero short-quad; do + reset_state + case "${_case}" in + omit-listen) OMIT_LISTEN_ADDRESS=1 ;; + bad-listen) LISTEN_ADDRESS="db.internal" ;; + leading-zero) LISTEN_ADDRESS="010.30.1.15" ;; + short-quad) LISTEN_ADDRESS="10.30.1" ;; + esac + write_handoff initialize 's3cr3t' appdb + run_fails "${_case}" + [ -s "${INSTALLDB_CALLS}" ] \ + && fail "${_case}: initialised on a handoff that could not be read" \ + || pass "${_case}: refused before the datadir was created" +done + +# --- Case 13a: the bind address must be on an interface before the engine starts --- +# An engine asked to bind an address that is not there refuses to start, which is +# the worst way to learn that DHCP lost its race. +reset_state +IP_ADDR_LIST="10.99.0.7/24" +write_handoff initialize 's3cr3t' appdb +run_fails "bind-wait" +grep -q "${LISTEN_ADDRESS_DEFAULT}" "${WORK}/out" \ + && pass "bind-wait: the refusal names the address" || fail "bind-wait: the refusal does not name the address" +[ -s "${INSTALLDB_CALLS}" ] \ + && fail "bind-wait: initialised before the address existed" \ + || pass "bind-wait: refused before the datadir was created" + if [ "${FAILS}" -eq 0 ]; then echo "PASS: all rds-init cases" exit 0 diff --git a/scripts/images/rds-postgres/rds-agent.initd b/scripts/images/rds-postgres/rds-agent.initd index 406d1549e..34011febd 100644 --- a/scripts/images/rds-postgres/rds-agent.initd +++ b/scripts/images/rds-postgres/rds-agent.initd @@ -35,7 +35,7 @@ start_pre() { # shellcheck disable=SC1090 . "${AGENT_ENV}" export RDS_GATEWAY_URL RDS_GATEWAY_CA RDS_REGION RDS_DB_INSTANCE_IDENTIFIER \ - RDS_ENGINE RDS_ENGINE_VERSION RDS_HANDOFF_DIR RDS_ENGINE_HOST RDS_ENGINE_PORT \ + RDS_ENGINE RDS_ENGINE_VERSION RDS_HANDOFF_DIR RDS_ENGINE_PORT \ RDS_POLL_WAIT RDS_DATA_MOUNT fi } diff --git a/scripts/images/rds-postgres/rds-init b/scripts/images/rds-postgres/rds-init index dec7dc237..3c9711774 100644 --- a/scripts/images/rds-postgres/rds-init +++ b/scripts/images/rds-postgres/rds-init @@ -14,6 +14,8 @@ # RDS_MASTER_PASSWORD= # RDS_DB_NAME= # RDS_PORT= +# RDS_LISTEN_ADDRESS= +# RDS_CLIENT_CIDR= # RDS_DATA_VOLUME_ID / RDS_DATA_VOLUME_SERIAL= # RDS_VM_GENERATION / RDS_FORMAT_AUTHORIZED= # RDS_BOOTSTRAP_PENDING / RDS_PAYLOAD_ID=/dev/null | + awk -v want="${RDS_LISTEN_ADDRESS}" \ + '{ split($4, addr, "/"); if (addr[1] == want) found = 1 } END { exit !found }'; then + return 0 + fi + [ "$(date +%s)" -lt "${_deadline}" ] || break + sleep 1 + done + fail "the endpoint address ${RDS_LISTEN_ADDRESS} did not appear on any interface within ${LISTEN_ADDRESS_TIMEOUT}s; the engine binds it and nothing else, so refusing to start" +} + load_handoff() { [ -f "${ENV_FILE}" ] || fail "no bootstrap handoff at ${ENV_FILE} — rds-agent has not delivered the bootstrap config" @@ -146,6 +210,8 @@ load_handoff() { RDS_MASTER_PASSWORD="${RDS_MASTER_PASSWORD:-}" RDS_DB_NAME="${RDS_DB_NAME:-}" RDS_PORT="${RDS_PORT:-5432}" + RDS_LISTEN_ADDRESS="${RDS_LISTEN_ADDRESS:-}" + RDS_CLIENT_CIDR="${RDS_CLIENT_CIDR:-}" RDS_VM_GENERATION="${RDS_VM_GENERATION:-}" # Absent from an agent that predates the receipt protocol, which reads as # "nothing is pending" and leaves every path below unchanged. @@ -192,6 +258,17 @@ load_handoff() { '' | *[!0-9]*) fail "bootstrap config has a non-numeric RDS_PORT: ${RDS_PORT}" ;; esac + # Neither falls back to a wildcard. The engine is reachable over the customer + # ENI and nowhere else, and a handoff that cannot say which address that is + # has no safe reading — the DB VM's other two NICs carry no security group + # the customer can see, let alone set. + [ -n "${RDS_LISTEN_ADDRESS}" ] || fail "bootstrap config has no RDS_LISTEN_ADDRESS; refusing to start an engine that would answer on every interface" + is_ipv4 "${RDS_LISTEN_ADDRESS}" || + fail "bootstrap config has a non-IPv4 RDS_LISTEN_ADDRESS: ${RDS_LISTEN_ADDRESS}" + [ -n "${RDS_CLIENT_CIDR}" ] || fail "bootstrap config has no RDS_CLIENT_CIDR; refusing to start an engine that would accept clients from anywhere" + is_ipv4_cidr "${RDS_CLIENT_CIDR}" || + fail "bootstrap config has a non-IPv4 RDS_CLIENT_CIDR: ${RDS_CLIENT_CIDR}" + # 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 @@ -443,13 +520,15 @@ write_pg_hba() { # Managed by rds-init — regenerated on every boot. Edits here are lost. # # Clients reach the instance over its customer ENI, gated by the DB security -# group as on AWS RDS, and authenticate with scram-sha-256. The catch-alls cover -# loopback too, so initdb's own 127.0.0.1 and ::1 lines are not carried over. -# The local socket is how rds-init and rds-agent run privileged SQL. +# group as on AWS RDS, and authenticate with scram-sha-256. The host rule is +# scoped to the customer VPC rather than the DB subnet: an app subnet reaching a +# DB subnet arrives carrying its own address, with no NAT in between. The engine +# does not bind loopback, so initdb's own 127.0.0.1 and ::1 lines govern nothing +# and are not carried over. The local socket is how rds-init and rds-agent run +# privileged SQL. local all all peer include_if_exists "${HBA_FORCE_SSL_INCLUDE}" -host all all 0.0.0.0/0 scram-sha-256 -host all all ::/0 scram-sha-256 +host all all ${RDS_CLIENT_CIDR} scram-sha-256 EOF chown "${PG_USER}:${PG_USER}" "${_tmp}" chmod 0600 "${_tmp}" @@ -466,8 +545,13 @@ EOF } # The 90- prefix sorts last within include_dir and the last setting wins, so a -# resolved parameter can never move the endpoint's port, turn off TLS or lower -# the protocol floor. +# resolved parameter can never move the endpoint's port, widen the bind, turn off +# TLS or lower the protocol floor. +# +# listen_addresses is the customer ENI and nothing else — not the wildcard, and +# not loopback either. The DB VM's other two NICs sit on the shared RDS system +# VPC and the host's management bridge, neither of which any customer security +# group governs, so a socket bound there is reachable across accounts. # # The floor is written whether or not a cert was delivered: it costs nothing with # ssl off, and making it conditional would mean a boot without a cert leaves the @@ -478,7 +562,7 @@ write_rds_conf() { cat > "${_tmp}" < "${STUBBIN}/ls" <<'EOF' exec "${REAL_LS}" "$@" EOF +# ip stub: stands in for `ip -4 -o addr show`, printing one line per address in +# IP_ADDR_LIST in the format the bind-address wait parses. +cat > "${STUBBIN}/ip" <<'EOF' +#!/bin/sh +n=1 +for cidr in ${IP_ADDR_LIST:-}; do + echo "${n}: eth$((n - 1)) inet ${cidr} scope global eth$((n - 1))" + n=$((n + 1)) +done +exit 0 +EOF + # su stub: `su postgres -c "cmd"` runs cmd in the harness user's shell. cat > "${STUBBIN}/su" <<'EOF' #!/bin/sh @@ -205,6 +217,11 @@ RECEIPT="${RECEIPT_DIR}/receipt.env" STAMP="${DATA_MOUNT}/.spinifex-rds/engine" HANDOFF="${WORK}/run/spinifex-rds" MOUNTS="${WORK}/mounts" + +# The customer ENI address the engine binds and the VPC range its client +# authentication admits, as the control plane delivers them. +LISTEN_ADDRESS_DEFAULT="10.30.1.15" +CLIENT_CIDR_DEFAULT="10.30.0.0/16" KILL_PID_FILE="${WORK}/rds-init.pid" export KILL_PID_FILE @@ -234,6 +251,12 @@ write_handoff() { [ -n "$3" ] && echo "RDS_DB_NAME='$3'" [ -n "${PENDING:-}" ] && echo "RDS_BOOTSTRAP_PENDING=${PENDING}" [ -n "${PAYLOAD_ID:-}" ] && echo "RDS_PAYLOAD_ID=${PAYLOAD_ID}" + # Written unset rather than defaulted when the case is about a handoff + # that omits one, which rds-init has to refuse rather than fill in. + [ "${OMIT_LISTEN_ADDRESS:-0}" = "1" ] || + echo "RDS_LISTEN_ADDRESS=${LISTEN_ADDRESS:-${LISTEN_ADDRESS_DEFAULT}}" + [ "${OMIT_CLIENT_CIDR:-0}" = "1" ] || + echo "RDS_CLIENT_CIDR=${CLIENT_CIDR:-${CLIENT_CIDR_DEFAULT}}" # Last, and unconditional: an optional line failing its test would make # the whole group's status non-zero and `set -e` would end the run here. echo "RDS_PORT=6543" @@ -270,6 +293,10 @@ reset_state() { : > "${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 PSQL_STATUS_CORRUPT STAMP_OVERRIDE || true + unset LISTEN_ADDRESS CLIENT_CIDR OMIT_LISTEN_ADDRESS OMIT_CLIENT_CIDR || true + # The customer ENI carries its address by the time rds-init runs, which is + # the ordinary case; the cases about a late lease drop it themselves. + IP_ADDR_LIST="${LISTEN_ADDRESS_DEFAULT}/24 10.99.0.7/24" write_tls } @@ -293,6 +320,8 @@ run_env() { RDS_RECEIPT_DIR="${RECEIPT_DIR_OVERRIDE:-${RECEIPT_DIR}}" \ RDS_ENGINE_STAMP="${STAMP_OVERRIDE:-${STAMP}}" \ RDS_ALLOW_LOCAL_DATADIR="${RDS_ALLOW_LOCAL_DATADIR:-0}" \ + RDS_LISTEN_ADDRESS_TIMEOUT="${LISTEN_TIMEOUT:-0}" \ + IP_ADDR_LIST="${IP_ADDR_LIST:-}" \ INITDB_FAIL="${INITDB_FAIL:-0}" \ PG_HBA_AS_DIR="${PG_HBA_AS_DIR:-0}" \ PSQL_FAIL="${PSQL_FAIL:-0}" \ @@ -328,14 +357,24 @@ if run_ok "initialize"; then grep -q 'initdb' "${INITDB_CALLS}" && pass "initialize: initdb ran" || fail "initialize: no initdb" grep -q -- '--data-checksums' "${INITDB_CALLS}" \ && pass "initialize: data checksums on" || fail "initialize: no --data-checksums" - grep -q 'host all all 0.0.0.0/0 scram-sha-256' "${PGDATA}/pg_hba.conf" \ - && pass "initialize: remote scram hba rule written" || fail "initialize: no hba rule" + grep -q "host all all ${CLIENT_CIDR_DEFAULT} scram-sha-256" "${PGDATA}/pg_hba.conf" \ + && pass "initialize: the scram hba rule is scoped to the customer VPC" || fail "initialize: no hba rule" + + # The scope is the whole boundary this file carries: a catch-all here admits + # the system VPC and the management bridge, which no customer security group + # governs. IPv6 never appears — the platform's VPCs are IPv4-only. + grep -qE 'host .*(0\.0\.0\.0/0|::/0)' "${PGDATA}/pg_hba.conf" \ + && fail "initialize: a catch-all host rule survived the VPC scoping" \ + || pass "initialize: no catch-all host rule" + [ "$(grep -c '^host ' "${PGDATA}/pg_hba.conf")" = 1 ] \ + && pass "initialize: exactly one host rule" \ + || fail "initialize: more than one host rule, so the scope is not the boundary" # pg_hba is first-match-wins, so initdb's loopback rules sorting above the - # catch-all would shadow anything the platform appends below them. rds-init - # owns the whole file; the catch-alls cover loopback themselves. + # platform's would shadow it. rds-init owns the whole file, and the engine + # does not bind loopback at all. grep -qE '127\.0\.0\.1/32|::1/128' "${PGDATA}/pg_hba.conf" \ - && fail "initialize: initdb's loopback TCP rules survived above the catch-all" \ + && fail "initialize: initdb's loopback TCP rules survived" \ || pass "initialize: initdb's loopback TCP rules are gone" grep -q 'replication' "${PGDATA}/pg_hba.conf" \ && fail "initialize: initdb's replication rules survived" \ @@ -358,6 +397,12 @@ if run_ok "initialize"; then && pass "initialize: include_dir hooked" || fail "initialize: no include_dir" grep -q '^port = 6543' "${PGDATA}/conf.d/90-rds-init.conf" \ && pass "initialize: delivered port applied" || fail "initialize: port not applied" + grep -q "^listen_addresses = '${LISTEN_ADDRESS_DEFAULT}'$" "${PGDATA}/conf.d/90-rds-init.conf" \ + && pass "initialize: the engine binds the customer ENI" \ + || fail "initialize: the engine does not bind the delivered address" + grep -q "^listen_addresses = '\*'" "${PGDATA}/conf.d/90-rds-init.conf" \ + && fail "initialize: the engine still answers on every interface" \ + || pass "initialize: no wildcard bind" grep -q '^ssl = on' "${PGDATA}/conf.d/90-rds-init.conf" \ && pass "initialize: ssl enabled" || fail "initialize: ssl not enabled" grep -q "^ssl_min_protocol_version = 'TLSv1.3'$" "${PGDATA}/conf.d/90-rds-init.conf" \ @@ -1159,6 +1204,69 @@ grep -q 'engine stamp' "${WORK}/out" \ && pass "stamp-unwritable: refusal names the stamp" || fail "stamp-unwritable: no refusal message" unset STAMP_OVERRIDE +# --- Case 12: a handoff that cannot say what to bind is refused before initdb --- +# The refusal is before the one-shot password is spent, so the create is still +# retryable. The only readings rds-init could reach on its own are the wildcards +# the bind exists to remove. +for _case in omit-listen bad-listen omit-cidr bad-cidr; do + reset_state + case "${_case}" in + omit-listen) OMIT_LISTEN_ADDRESS=1 ;; + bad-listen) LISTEN_ADDRESS="db.internal" ;; + omit-cidr) OMIT_CLIENT_CIDR=1 ;; + bad-cidr) CLIENT_CIDR="10.30.0.0/33" ;; + esac + write_handoff initialize 's3cr3t' appdb + run_fails "${_case}" + [ -s "${INITDB_CALLS}" ] \ + && fail "${_case}: initdb ran on a handoff that could not be read" \ + || pass "${_case}: refused before initdb" +done + +# --- Case 12a: an octet-shaped address that is not one is refused --- +# 10.30.1.256 and 010.30.1.15 both parse as addresses to a permissive check, and +# the second binds a different host to anything reading the leading zero as octal. +for _addr in 10.30.1.256 010.30.1.15 10.30.1 10.30.1.15.1; do + reset_state + LISTEN_ADDRESS="${_addr}" + write_handoff initialize 's3cr3t' appdb + run_fails "listen-${_addr}" +done + +# --- Case 13: the bind address must be on an interface before the engine starts --- +# An engine asked to bind an address that is not there exits with `could not +# bind`, which is the worst way to learn that DHCP lost its race. +reset_state +IP_ADDR_LIST="10.99.0.7/24" +write_handoff initialize 's3cr3t' appdb +run_fails "bind-wait" +grep -q "${LISTEN_ADDRESS_DEFAULT}" "${WORK}/out" \ + && pass "bind-wait: the refusal names the address" || fail "bind-wait: the refusal does not name the address" +[ -s "${INITDB_CALLS}" ] \ + && fail "bind-wait: initdb ran before the address existed" \ + || pass "bind-wait: refused before initdb" + +# --- Case 13a: a restored volume takes the destination VPC's scope --- +# A restore attaches a data volume carrying the source instance's pg_hba into a +# possibly different VPC. Regenerating the whole file every boot is what stops +# the restored instance locking out its own VPC's clients. +reset_state +write_handoff initialize 's3cr3t' appdb +write_parameters +if run_ok "restore-scope-setup"; then + CLIENT_CIDR="172.31.0.0/16" + write_handoff attach '' appdb + write_parameters + if run_ok "restore-scope"; then + grep -q 'host all all 172.31.0.0/16 scram-sha-256' "${PGDATA}/pg_hba.conf" \ + && pass "restore-scope: the destination VPC's range is admitted" \ + || fail "restore-scope: the restored instance did not take its own VPC's scope" + grep -q "${CLIENT_CIDR_DEFAULT}" "${PGDATA}/pg_hba.conf" \ + && fail "restore-scope: the source VPC's range survived the restore" \ + || pass "restore-scope: the source VPC's range is gone" + fi +fi + if [ "${FAILS}" -eq 0 ]; then echo "PASS: all rds-init cases" exit 0 diff --git a/spinifex/handlers/rds/agent.go b/spinifex/handlers/rds/agent.go index 9ba4962b0..e2914f65a 100644 --- a/spinifex/handlers/rds/agent.go +++ b/spinifex/handlers/rds/agent.go @@ -78,6 +78,13 @@ type GetDBBootstrapConfigOutput struct { MasterUserPassword *string `json:"masterUserPassword,omitempty" locationName:"MasterUserPassword"` Port int64 `json:"port" locationName:"Port"` + // The customer ENI address the engine binds, and the range its client + // authentication rules admit. Together they make the endpoint the only path + // to the engine: the management NICs no customer security group governs are + // not bound at all. + ListenAddress string `json:"listenAddress" locationName:"ListenAddress"` + ClientCIDR string `json:"clientCidr" locationName:"ClientCIDR"` + DataVolumeID string `json:"dataVolumeId,omitempty" locationName:"DataVolumeId" xml:"DataVolumeId"` DataVolumeSerial string `json:"dataVolumeSerial,omitempty" locationName:"DataVolumeSerial" xml:"DataVolumeSerial"` VMGeneration int64 `json:"vmGeneration,omitempty" locationName:"VMGeneration" xml:"VMGeneration"` @@ -348,6 +355,18 @@ func (s *Service) GetDBBootstrapConfig(ctx context.Context, input *GetDBBootstra return nil, errors.New(awserrors.ErrorAccessDenied) } + // Both fail the fetch rather than falling back to a wildcard. The agent + // retries on a backoff, so an unresolvable one presents as a stuck bootstrap + // with the reason on the guest console rather than as an engine that came up + // reachable from every network it is attached to. + if rec.ENIPrivateIP == "" { + return nil, errors.New("rds bootstrap: the DB instance record carries no endpoint ENI address for the engine to bind") + } + clientCIDR, err := s.resolveClientCIDR(ctx, kv, &rec) + if err != nil { + return nil, err + } + out := &GetDBBootstrapConfigOutput{ Mode: BootstrapModeAttach, DBInstanceIdentifier: rec.DBInstanceIdentifier, @@ -356,6 +375,8 @@ func (s *Service) GetDBBootstrapConfig(ctx context.Context, input *GetDBBootstra DBName: rec.DBName, MasterUsername: rec.MasterUsername, Port: rec.Port, + ListenAddress: rec.ENIPrivateIP, + ClientCIDR: clientCIDR, DataVolumeID: rec.DataVolumeID, DataVolumeSerial: serial, VMGeneration: rec.VMGeneration, @@ -380,6 +401,37 @@ func (s *Service) GetDBBootstrapConfig(ctx context.Context, input *GetDBBootstra return out, nil } +// The range the guest's client authentication rules admit. A record predating +// the field is backfilled from its VPC and persisted, so the describe is paid +// once rather than on every boot of every instance. +// +// A failure to persist the backfill is not a failure to serve it: the value is +// re-derivable and the next fetch records it, whereas failing here would hold a +// bootable instance down for a bookkeeping write. +func (s *Service) resolveClientCIDR(ctx context.Context, kv jetstream.KeyValue, rec *DBInstanceRecord) (string, error) { + if rec.VpcCIDR != "" { + return rec.VpcCIDR, nil + } + if rec.VpcID == "" { + return "", fmt.Errorf("rds bootstrap: DB instance %s records no VPC to scope its client authentication to", + rec.DBInstanceIdentifier) + } + if s.deps.Network == nil { + return "", errors.New("rds bootstrap: RDS networking is not wired on this node") + } + cidr, err := s.vpcCIDR(ctx, rec.AccountID, rec.VpcID) + if err != nil { + return "", err + } + if err := s.updateInstance(ctx, kv, rec.DBInstanceIdentifier, func(stored *DBInstanceRecord) { + stored.VpcCIDR = cidr + }); err != nil { + slog.ErrorContext(ctx, "rds bootstrap: the VPC address range could not be recorded; serving it regardless", + "dbInstance", rec.DBInstanceIdentifier, "vpcId", rec.VpcID, "err", err) + } + return cidr, nil +} + // Fills in the initialize half of the response when a payload is staged for this // generation, and leaves the attach response untouched otherwise. // diff --git a/spinifex/handlers/rds/agent_test.go b/spinifex/handlers/rds/agent_test.go index c722b308c..c6be90570 100644 --- a/spinifex/handlers/rds/agent_test.go +++ b/spinifex/handlers/rds/agent_test.go @@ -25,6 +25,8 @@ import ( const ( testDBID = "orders-db" testInstance = "i-0abc123" + testVpcID = "vpc-0aaa111" + testVpcCIDR = "10.20.0.0/16" ) // Minted once per package rather than per harness. An RSA-2048 keygen costs @@ -93,6 +95,8 @@ func defaultRecord() DBInstanceRecord { InstanceID: testInstance, VMGeneration: firstVMGeneration, ENIPrivateIP: "10.20.30.40", + VpcID: testVpcID, + VpcCIDR: testVpcCIDR, DNSName: "orders-db.123456789012.ap-southeast-2.rds.example.internal", Bootstrap: BootstrapState{ State: BootstrapStatePending, @@ -271,7 +275,11 @@ func TestGetDBBootstrapConfig_NoCAStillServesConfig(t *testing.T) { assert.Equal(t, int64(5432), out.Port) } -func TestGetDBBootstrapConfig_ConfiguredCARequiresENIPrivateIP(t *testing.T) { +// The address the engine binds, so a record without one fails the fetch on +// every deployment rather than only where TLS needs it as a SAN. The agent +// retries, so this is a stuck bootstrap with a reason rather than an engine +// that came up on the wildcard. +func TestGetDBBootstrapConfig_RequiresENIPrivateIP(t *testing.T) { t.Parallel() svc := newTestService(t) rec := defaultRecord() @@ -280,7 +288,68 @@ func TestGetDBBootstrapConfig_ConfiguredCARequiresENIPrivateIP(t *testing.T) { _, err := svc.GetDBBootstrapConfig(t.Context(), bootstrapInput(), testAccountID) require.Error(t, err) - assert.Contains(t, err.Error(), "configured TLS requires an ENI private IP") + assert.Contains(t, err.Error(), "no endpoint ENI address") + + envelope, _ := storedPayload(t, svc, testDBID) + assert.NotNil(t, envelope, "a failed fetch must leave the payload replayable") +} + +// The endpoint address and the VPC range reach the guest, which is what makes +// the engine bind one NIC and admit one range. +func TestGetDBBootstrapConfig_CarriesListenAddressAndClientCIDR(t *testing.T) { + t.Parallel() + svc := newTestService(t) + rec := defaultRecord() + seedInstance(t, svc, rec) + + out, err := svc.GetDBBootstrapConfig(t.Context(), + &GetDBBootstrapConfigInput{DBInstanceIdentifier: testDBID, InstanceID: testInstance}, testAccountID) + require.NoError(t, err) + assert.Equal(t, rec.ENIPrivateIP, out.ListenAddress) + assert.Equal(t, testVpcCIDR, out.ClientCIDR) +} + +// A record predating the field resolves its VPC once and keeps the answer, so +// the describe is not paid on every boot of every instance. +func TestGetDBBootstrapConfig_BackfillsVpcCIDR(t *testing.T) { + t.Parallel() + _, nc, _ := testutil.StartTestJetStream(t) + network := newFakeNetwork() + svc := NewService(nc, testRegion).WithDeps(Deps{ + LoadCA: newTestCA(t), MasterKey: testMasterKey, Network: network, + }) + rec := defaultRecord() + rec.VpcID = testDefaultVPC + rec.VpcCIDR = "" + seedInstance(t, svc, rec) + + in := &GetDBBootstrapConfigInput{DBInstanceIdentifier: testDBID, InstanceID: testInstance} + out, err := svc.GetDBBootstrapConfig(t.Context(), in, testAccountID) + require.NoError(t, err) + assert.Equal(t, testDefaultVPCCIDR, out.ClientCIDR) + + stored, _ := readRecord(t, svc) + assert.Equal(t, testDefaultVPCCIDR, stored.VpcCIDR, "the backfill must be persisted") + + describes := len(network.accts) + _, err = svc.GetDBBootstrapConfig(t.Context(), in, testAccountID) + require.NoError(t, err) + assert.Len(t, network.accts, describes, "a backfilled record must not describe its VPC again") +} + +// Without a VPC on the record there is nothing to scope client authentication +// to, and the permissive reading is the one this plan exists to remove. +func TestGetDBBootstrapConfig_UnresolvableClientCIDRFailsClosed(t *testing.T) { + t.Parallel() + svc := newTestService(t) + rec := defaultRecord() + rec.VpcID = "" + rec.VpcCIDR = "" + seedPending(t, svc, rec) + + _, err := svc.GetDBBootstrapConfig(t.Context(), bootstrapInput(), testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), "records no VPC") envelope, _ := storedPayload(t, svc, testDBID) assert.NotNil(t, envelope, "a failed fetch must leave the payload replayable") diff --git a/spinifex/handlers/rds/create.go b/spinifex/handlers/rds/create.go index d19e09947..88fc20e9c 100644 --- a/spinifex/handlers/rds/create.go +++ b/spinifex/handlers/rds/create.go @@ -166,6 +166,7 @@ func newDBInstanceRecord(accountID string, req *validatedCreate, placement *endp Port: req.Port, SubnetID: placement.SubnetID, VpcID: placement.VpcID, + VpcCIDR: placement.VpcCIDR, VpcSecurityGroupIDs: placement.SecurityGroupIDs, DBSubnetGroupName: req.DBSubnetGroupName, DBParameterGroupName: req.DBParameterGroupName, @@ -214,6 +215,7 @@ func (s *Service) recordLaunch(ctx context.Context, kv jetstream.KeyValue, key, rec.InstanceID = launched.InstanceID rec.VMGeneration = firstVMGeneration rec.SystemENIID = launched.SystemENIID + rec.SystemSGID = launched.SystemSGID rec.ENIID = launched.CustomerENIID rec.ENIPrivateIP = launched.CustomerENIIP rec.DataVolumeID = launched.DataVolumeID diff --git a/spinifex/handlers/rds/create_test.go b/spinifex/handlers/rds/create_test.go index c706482b2..4112ba421 100644 --- a/spinifex/handlers/rds/create_test.go +++ b/spinifex/handlers/rds/create_test.go @@ -24,10 +24,11 @@ import ( ) const ( - testDefaultVPC = "vpc-default01" - testDefaultSG = "sg-default01" - testBaseDomain = "spx3.net" - testDBInstanceID = "orders-db" + testDefaultVPC = "vpc-default01" + testDefaultVPCCIDR = "10.0.0.0/16" + testDefaultSG = "sg-default01" + testBaseDomain = "spx3.net" + testDBInstanceID = "orders-db" // The one zone this platform exposes, which every subnet reports. testZone = "spinifexz1" ) @@ -55,7 +56,11 @@ var _ networkResolver = (*fakeNetwork)(nil) func newFakeNetwork() *fakeNetwork { return &fakeNetwork{ - vpcs: []*ec2.Vpc{{VpcId: aws.String(testDefaultVPC), IsDefault: aws.Bool(true)}}, + vpcs: []*ec2.Vpc{{ + VpcId: aws.String(testDefaultVPC), + IsDefault: aws.Bool(true), + CidrBlock: aws.String(testDefaultVPCCIDR), + }}, // Deliberately out of order: the resolver sorts, so the placement is the // same on every repeat regardless of how the describe happened to order. subnets: []*ec2.Subnet{ @@ -85,7 +90,17 @@ func (f *fakeNetwork) DescribeVpcs(_ context.Context, input *ec2.DescribeVpcsInp if f.vpcErr != nil { return nil, f.vpcErr } - return &ec2.DescribeVpcsOutput{Vpcs: f.vpcs}, nil + if input == nil || len(input.VpcIds) == 0 { + return &ec2.DescribeVpcsOutput{Vpcs: f.vpcs}, nil + } + wanted := aws.StringValueSlice(input.VpcIds) + var matched []*ec2.Vpc + for _, vpc := range f.vpcs { + if slices.Contains(wanted, aws.StringValue(vpc.VpcId)) { + matched = append(matched, vpc) + } + } + return &ec2.DescribeVpcsOutput{Vpcs: matched}, nil } // Honours SubnetIds the way EC2 does — a named subnet that is not in this diff --git a/spinifex/handlers/rds/launch.go b/spinifex/handlers/rds/launch.go index ac799c014..1eb1f3740 100644 --- a/spinifex/handlers/rds/launch.go +++ b/spinifex/handlers/rds/launch.go @@ -62,6 +62,10 @@ type launchVPCProvisioner interface { // Re-associates the customer ENI's security groups in place, which is what // makes a VpcSecurityGroupIds modify need no VM replace and no new IP. ModifyNetworkInterfaceAttribute(ctx context.Context, input *ec2.ModifyNetworkInterfaceAttributeInput, accountID string) (*ec2.ModifyNetworkInterfaceAttributeOutput, error) + // The system NIC's own group, ensured per launch rather than at daemon + // start, so a deployment whose system VPC was rebuilt still gets one. + CreateSecurityGroup(ctx context.Context, input *ec2.CreateSecurityGroupInput, accountID string) (*ec2.CreateSecurityGroupOutput, error) + DescribeSecurityGroups(ctx context.Context, input *ec2.DescribeSecurityGroupsInput, accountID string) (*ec2.DescribeSecurityGroupsOutput, error) } // System-managed VMs get a mgmt-bridge NIC alongside their VPC NICs, which is @@ -124,7 +128,10 @@ type LaunchOutput struct { InstanceID string // The system ENI is disposable — a replace makes a new one. The customer // ENI is the stable endpoint: its IP is the DNS target and survives. - SystemENIID string + SystemENIID string + // The region's RDS system group, re-derived on every launch so a record + // written before the group existed is corrected by the next replace. + SystemSGID string CustomerENIID string CustomerENIIP string DataVolumeID string @@ -160,6 +167,11 @@ func LaunchDBInstanceVM(ctx context.Context, deps LaunchDeps, in LaunchInput) (o } systemSubnetID := sysRefs.PrivateSubnetIDs[0] + systemSGID, err := EnsureSystemSecurityGroup(ctx, deps.VPC, utils.GlobalAccountID, region, sysRefs.VpcID) + if err != nil { + return nil, err + } + // Unwind in reverse creation order on any failure below. Each step appends // its own undo as soon as the resource exists. var rollback []func(context.Context) @@ -187,7 +199,7 @@ func LaunchDBInstanceVM(ctx context.Context, deps LaunchDeps, in LaunchInput) (o } }() - systemENI, err := createLaunchENI(ctx, deps.VPC, utils.GlobalAccountID, systemSubnetID, nil, + systemENI, err := createLaunchENI(ctx, deps.VPC, utils.GlobalAccountID, systemSubnetID, []string{systemSGID}, "RDS management NIC for "+in.DBInstanceIdentifier, in.DBInstanceIdentifier) if err != nil { return nil, err @@ -294,12 +306,14 @@ func LaunchDBInstanceVM(ctx context.Context, deps LaunchDeps, in LaunchInput) (o slog.InfoContext(ctx, "rds: DB VM launched", "dbInstance", in.DBInstanceIdentifier, "instanceId", instanceID, "ami", amiID, - "systemEni", systemENI.id, "customerEni", customerENI.id, "customerEniIp", customerENI.ip, + "systemEni", systemENI.id, "systemSg", systemSGID, + "customerEni", customerENI.id, "customerEniIp", customerENI.ip, "dataVolume", volumeID, "device", device) return &LaunchOutput{ InstanceID: instanceID, SystemENIID: systemENI.id, + SystemSGID: systemSGID, CustomerENIID: customerENI.id, CustomerENIIP: customerENI.ip, DataVolumeID: volumeID, @@ -332,8 +346,9 @@ type launchENI struct { mac string } -// groups is nil for the system NIC, which is unreachable from any customer VPC -// and so needs no security group of its own. +// groups must be non-empty for either NIC: an ENI created with none is placed in +// its VPC's default group, whose sole ingress rule admits every other member of +// itself — which for the shared system VPC is every DB VM in the deployment. func createLaunchENI(ctx context.Context, vpcSvc launchVPCProvisioner, accountID, subnetID string, groups []string, description, dbInstanceID string) (*launchENI, error) { var groupIDs []*string if len(groups) > 0 { diff --git a/spinifex/handlers/rds/launch_test.go b/spinifex/handlers/rds/launch_test.go index e68580973..682742104 100644 --- a/spinifex/handlers/rds/launch_test.go +++ b/spinifex/handlers/rds/launch_test.go @@ -32,6 +32,10 @@ const ( // target and its MAC is what the replacement VM's NIC has to come up with. testEndpointIP = "10.20.30.40" testEndpointMAC = "02:00:00:00:aa:01" + + // The RDS system group the system NIC carries, in place of the system VPC's + // mutually-permissive default group. + testSystemSG = "sg-rdssystem" ) // --- Fakes --- @@ -193,6 +197,14 @@ type fakeENIs struct { // replace must refuse to mint a replacement address for. describeMissing bool modifyErr error + + // The region's RDS system group, ensured per launch. Recording both halves of + // the lookup-or-create is what makes its idempotence observable. + sgCreated []*ec2.CreateSecurityGroupInput + sgDescribed []*ec2.DescribeSecurityGroupsInput + sgExisting string + sgExistingGroups []*ec2.SecurityGroup + sgCreateErr error } var _ launchVPCProvisioner = (*fakeENIs)(nil) @@ -254,6 +266,30 @@ func (f *fakeENIs) ModifyNetworkInterfaceAttribute(_ context.Context, in *ec2.Mo return &ec2.ModifyNetworkInterfaceAttributeOutput{}, nil } +func (f *fakeENIs) DescribeSecurityGroups(_ context.Context, in *ec2.DescribeSecurityGroupsInput, _ string) (*ec2.DescribeSecurityGroupsOutput, error) { + f.sgDescribed = append(f.sgDescribed, in) + if f.sgExistingGroups != nil { + return &ec2.DescribeSecurityGroupsOutput{SecurityGroups: f.sgExistingGroups}, nil + } + if f.sgExisting == "" { + return &ec2.DescribeSecurityGroupsOutput{}, nil + } + return &ec2.DescribeSecurityGroupsOutput{ + SecurityGroups: []*ec2.SecurityGroup{{GroupId: aws.String(f.sgExisting)}}, + }, nil +} + +// A created group is found by the next describe, as the real API would have it: +// that is what a second launch's idempotence rests on. +func (f *fakeENIs) CreateSecurityGroup(_ context.Context, in *ec2.CreateSecurityGroupInput, _ string) (*ec2.CreateSecurityGroupOutput, error) { + if f.sgCreateErr != nil { + return nil, f.sgCreateErr + } + f.sgCreated = append(f.sgCreated, in) + f.sgExisting = testSystemSG + return &ec2.CreateSecurityGroupOutput{GroupId: aws.String(f.sgExisting)}, nil +} + // fakeLauncher stands in for the system-instance launcher. type fakeLauncher struct { input *sysinstance.SystemInstanceInput @@ -528,7 +564,10 @@ func TestLaunchDBInstanceVMWiresBothNICs(t *testing.T) { assert.Equal(t, utils.GlobalAccountID, h.enis.accts[0]) assert.True(t, strings.HasPrefix(aws.StringValue(sysENI.SubnetId), "subnet-rdssys"), "the primary NIC must land in the RDS system VPC, got %s", aws.StringValue(sysENI.SubnetId)) - assert.Empty(t, sysENI.Groups, "the system NIC is unreachable from any customer VPC and needs no security group") + // Its own ingress-free group, not the system VPC's default one — whose sole + // rule admits every other member of itself, which is every DB VM there is. + assert.Equal(t, []string{testSystemSG}, aws.StringValueSlice(sysENI.Groups)) + assert.Equal(t, testSystemSG, out.SystemSGID) // The customer-facing ENI is created in the customer's account and subnet, // with the customer's security groups: it is the only ingress path. @@ -566,6 +605,78 @@ func TestLaunchDBInstanceVMWiresBothNICs(t *testing.T) { assert.Equal(t, "i-rds0001", out.InstanceID) } +// The group is per region and shared by every DB VM, so a second launch has to +// find the first one's rather than accumulate a group per instance. +func TestLaunchDBInstanceVMReusesTheSystemSecurityGroup(t *testing.T) { + t.Parallel() + h := newLaunchHarness() + + first, err := LaunchDBInstanceVM(t.Context(), h.deps(), testLaunchInput()) + require.NoError(t, err) + second, err := LaunchDBInstanceVM(t.Context(), h.deps(), testLaunchInput()) + require.NoError(t, err) + + assert.Equal(t, first.SystemSGID, second.SystemSGID) + require.Len(t, h.enis.sgCreated, 1, "the region's system group is shared, not minted per DB instance") + + created := h.enis.sgCreated[0] + assert.Equal(t, SystemSecurityGroupName("ap-southeast-2"), aws.StringValue(created.GroupName)) + assert.Equal(t, tags.ManagedByRDS, tagOf(created.TagSpecifications, tags.ManagedByKey)) + + // The name and the VPC together are the lookup key: a group of that name in + // some customer VPC must not be adopted for the system NIC. + require.NotEmpty(t, h.enis.sgDescribed) + filters := map[string]string{} + for _, f := range h.enis.sgDescribed[0].Filters { + filters[aws.StringValue(f.Name)] = aws.StringValue(f.Values[0]) + } + assert.Equal(t, SystemSecurityGroupName("ap-southeast-2"), filters["group-name"]) + assert.NotEmpty(t, filters["vpc-id"]) +} + +// There is no fallback to nil groups, because that fallback is the bug: an ENI +// created with none lands in the VPC's default group. +func TestLaunchDBInstanceVMFailsWhenTheSystemSecurityGroupCannotBeEnsured(t *testing.T) { + t.Parallel() + h := newLaunchHarness() + h.enis.sgCreateErr = errors.New("vpcd unavailable") + + _, err := LaunchDBInstanceVM(t.Context(), h.deps(), testLaunchInput()) + require.Error(t, err) + assert.Empty(t, h.enis.created, "no NIC may be created before its security group exists") + assert.Nil(t, h.launcher.input) +} + +func TestLaunchDBInstanceVMRejectsAnExistingSystemSecurityGroupWithIngress(t *testing.T) { + t.Parallel() + h := newLaunchHarness() + h.enis.sgExistingGroups = []*ec2.SecurityGroup{{ + GroupId: aws.String(testSystemSG), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("-1"), + }}, + }} + + _, err := LaunchDBInstanceVM(t.Context(), h.deps(), testLaunchInput()) + require.Error(t, err) + assert.Contains(t, err.Error(), "has ingress rules") + assert.Empty(t, h.enis.created, "an unvalidated group must never reach a system NIC") +} + +func TestLaunchDBInstanceVMRejectsMultipleExistingSystemSecurityGroups(t *testing.T) { + t.Parallel() + h := newLaunchHarness() + h.enis.sgExistingGroups = []*ec2.SecurityGroup{ + {GroupId: aws.String(testSystemSG)}, + {GroupId: aws.String("sg-rdssystem02")}, + } + + _, err := LaunchDBInstanceVM(t.Context(), h.deps(), testLaunchInput()) + require.Error(t, err) + assert.Contains(t, err.Error(), "found 2 groups") + assert.Empty(t, h.enis.created, "an ambiguous group must never reach a system NIC") +} + func TestLaunchDBInstanceVMAttachesTheDataVolume(t *testing.T) { t.Parallel() h := newLaunchHarness() diff --git a/spinifex/handlers/rds/lifecycle_test.go b/spinifex/handlers/rds/lifecycle_test.go index e360976db..bd04075a6 100644 --- a/spinifex/handlers/rds/lifecycle_test.go +++ b/spinifex/handlers/rds/lifecycle_test.go @@ -15,6 +15,7 @@ import ( "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/rds" "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/mulgadc/spinifex/spinifex/config" "github.com/mulgadc/spinifex/spinifex/testutil" "github.com/nats-io/nats.go" "github.com/stretchr/testify/assert" @@ -231,6 +232,7 @@ type lifecycleHarness struct { cmdr *fakeInstanceCommander snaps *fakeSnapshots enis *fakeENIs + sysvpc *fakeSystemVPC launcher *fakeLauncher volumes *fakeVolumes agent *stubAgent @@ -251,6 +253,7 @@ func newLifecycleHarness(t *testing.T, agentFails bool) *lifecycleHarness { cmdr: &fakeInstanceCommander{vm: vmState}, snaps: &fakeSnapshots{}, enis: &fakeENIs{}, + sysvpc: &fakeSystemVPC{}, launcher: &fakeLauncher{}, volumes: &fakeVolumes{}, vmState: vmState, @@ -264,9 +267,11 @@ func newLifecycleHarness(t *testing.T, agentFails bool) *lifecycleHarness { InstanceState: h.vmState, VMStopTimeout: testVMStopTimeout, Launch: LaunchDeps{ - VPC: h.enis, - Instance: h.launcher, - Volume: h.volumes, + Config: &config.Config{Region: testRegion}, + SystemVPC: h.sysvpc.deps(), + VPC: h.enis, + Instance: h.launcher, + Volume: h.volumes, }, }) return h @@ -278,6 +283,9 @@ func availableRecord() DBInstanceRecord { rec.Status = StatusAvailable rec.ENIID = "eni-cust01" rec.SystemENIID = "eni-sys01" + // Already on the region's system group, as a current launch records it, so a + // lifecycle case is not answering the retrofit sweep instead of its own step. + rec.SystemSGID = testSystemSG rec.DataVolumeID = "vol-rdsdata01" rec.CreatedAt = time.Now().UTC().Add(-time.Hour) return rec diff --git a/spinifex/handlers/rds/network.go b/spinifex/handlers/rds/network.go index c02ab8b13..f825531b7 100644 --- a/spinifex/handlers/rds/network.go +++ b/spinifex/handlers/rds/network.go @@ -21,7 +21,10 @@ type networkResolver interface { // Where the customer-facing ENI lands. type endpointPlacement struct { - VpcID string + VpcID string + // The VPC's own address range, which becomes the scope of the guest's client + // authentication rules. + VpcCIDR string SubnetID string SecurityGroupIDs []string } @@ -60,7 +63,35 @@ func (s *Service) resolvePlacement(ctx context.Context, kv jetstream.KeyValue, a if err != nil { return nil, err } - return &endpointPlacement{VpcID: vpcID, SubnetID: subnetID, SecurityGroupIDs: groupIDs}, nil + cidr, err := s.vpcCIDR(ctx, accountID, vpcID) + if err != nil { + return nil, err + } + return &endpointPlacement{VpcID: vpcID, VpcCIDR: cidr, SubnetID: subnetID, SecurityGroupIDs: groupIDs}, nil +} + +// The VPC's address range. Resolved rather than derived from the subnet: the +// guest's client authentication rules are scoped to the whole VPC, because an +// app subnet reaching a DB subnet arrives carrying its own address. +func (s *Service) vpcCIDR(ctx context.Context, accountID, vpcID string) (string, error) { + out, err := s.deps.Network.DescribeVpcs(ctx, &ec2.DescribeVpcsInput{ + VpcIds: aws.StringSlice([]string{vpcID}), + }, accountID) + if err != nil { + return "", fmt.Errorf("rds: describe VPC %s: %w", vpcID, err) + } + if out != nil { + for _, vpc := range out.Vpcs { + if aws.StringValue(vpc.VpcId) != vpcID { + continue + } + if cidr := aws.StringValue(vpc.CidrBlock); cidr != "" { + return cidr, nil + } + } + } + return "", awserrors.Errorf(awserrors.ErrorDBInvalidVPCNetworkState, + "VPC %s has no address range to scope the DB instance's client authentication to", vpcID) } // The subnet a group places an instance in. Sorted rather than first-listed, so diff --git a/spinifex/handlers/rds/reconciler.go b/spinifex/handlers/rds/reconciler.go index 0187c230c..5c070a344 100644 --- a/spinifex/handlers/rds/reconciler.go +++ b/spinifex/handlers/rds/reconciler.go @@ -84,6 +84,12 @@ type Reconciler struct { // every sweep and crowd the bounded ring. reportedMu sync.Mutex reportedPending map[string]string + + // The region's RDS system security group, resolved once. Ensuring it per + // instance per pass would be a VPC and group describe for every DB VM every + // 15s; the group is regional and nothing deletes it per instance. + systemSGMu sync.Mutex + systemSGID string } // holder identifies this daemon in the lease. @@ -94,19 +100,27 @@ func NewReconciler(svc *Service, holder string) *Reconciler { // Drives the leadership and reconcile loop until ctx is cancelled. Intended as // a daemon-boot goroutine; panics are the caller's recover concern. func (r *Reconciler) Run(ctx context.Context) { - leaseTicker := time.NewTicker(leaseRefresh) + leadershipCtx, cancelLeadership := context.WithCancel(ctx) + leadershipDone := make(chan struct{}) + go func() { + defer close(leadershipDone) + r.maintainLeadership(leadershipCtx, leaseRefresh) + }() + defer func() { + cancelLeadership() + <-leadershipDone + }() + reconcileTicker := time.NewTicker(reconcileInterval) - defer leaseTicker.Stop() defer reconcileTicker.Stop() - r.evaluateLeadership(ctx) for { select { case <-ctx.Done(): + cancelLeadership() + <-leadershipDone r.relinquish() return - case <-leaseTicker.C: - r.evaluateLeadership(ctx) case <-reconcileTicker.C: if !r.isLeader() { continue @@ -118,6 +132,23 @@ func (r *Reconciler) Run(ctx context.Context) { } } +// Leadership refresh runs independently because a fleet pass can contain +// blocking network I/O and must not outlive its lease. +func (r *Reconciler) maintainLeadership(ctx context.Context, refresh time.Duration) { + leaseTicker := time.NewTicker(refresh) + defer leaseTicker.Stop() + + r.evaluateLeadership(ctx) + for { + select { + case <-ctx.Done(): + return + case <-leaseTicker.C: + r.evaluateLeadership(ctx) + } + } +} + // The shared GC backstop's cluster-wide gate. The reconciler's lease is already // cluster-singular and held continuously rather than claimed per sweep, so // holding it is the whole answer and there is nothing for the caller to release. @@ -251,6 +282,10 @@ func (r *Reconciler) reconcileInstance(ctx context.Context, kv jetstream.KeyValu if err := r.reportStalePendingBootstrap(ctx, kv, accountID, &rec); err != nil { return err } + remediated, err := r.remediateSystemENISG(ctx, kv, rev, &rec) + if err != nil || remediated { + return err + } switch rec.Status { case StatusCreating: return r.reconcileCreating(ctx, kv, rev, accountID, &rec) @@ -271,6 +306,67 @@ func (r *Reconciler) reconcileInstance(ctx context.Context, kv jetstream.KeyValu } } +// Moves a system NIC launched before the RDS system security group existed onto +// it, once. Reports whether it acted, so the caller yields this pass and the +// status handler runs on the next tick against a fresh revision. +// +// vpcd applies the requested group list declaratively, so this removes the ENI +// from the system VPC's default group rather than merely adding the new one +// alongside it — and that default group is the whole of the exposure. +func (r *Reconciler) remediateSystemENISG(ctx context.Context, kv jetstream.KeyValue, + rev uint64, rec *DBInstanceRecord) (bool, error) { + if rec.SystemENIID == "" || rec.Status == StatusDeleting { + return false, nil + } + sgID, err := r.ensuredSystemSG(ctx) + if err != nil { + return false, err + } + if rec.SystemSGID == sgID { + return false, nil + } + + if _, err := r.svc.deps.Launch.VPC.ModifyNetworkInterfaceAttribute(ctx, &ec2.ModifyNetworkInterfaceAttributeInput{ + NetworkInterfaceId: aws.String(rec.SystemENIID), + Groups: aws.StringSlice([]string{sgID}), + }, utils.GlobalAccountID); err != nil { + return false, fmt.Errorf("rds: move the system NIC of %s onto %s: %w", rec.DBInstanceIdentifier, sgID, err) + } + // Recorded only once vpcd has accepted the change, so a failure above is + // retried next pass rather than remembered as done. + rec.SystemSGID = sgID + if _, err := updateJSONRevision(ctx, kv, DBInstanceKey(rec.DBInstanceIdentifier), rev, rec); err != nil { + return false, err + } + slog.InfoContext(ctx, "rds: system NIC moved onto the RDS system security group", + "dbInstance", rec.DBInstanceIdentifier, "eniId", rec.SystemENIID, "groupId", sgID) + return true, nil +} + +// The region's system security group, ensured on first use and then cached for +// the life of the process. +func (r *Reconciler) ensuredSystemSG(ctx context.Context) (string, error) { + r.systemSGMu.Lock() + defer r.systemSGMu.Unlock() + if r.systemSGID != "" { + return r.systemSGID, nil + } + deps := r.svc.deps.Launch + if deps.VPC == nil || deps.Config == nil { + return "", errors.New("rds reconciler: no VPC path is configured to place system NICs on their own security group") + } + refs, err := EnsureSystemVPC(ctx, deps.SystemVPC, &deps.Config.RDS, utils.GlobalAccountID, deps.Config.Region) + if err != nil { + return "", err + } + sgID, err := EnsureSystemSecurityGroup(ctx, deps.VPC, utils.GlobalAccountID, deps.Config.Region, refs.VpcID) + if err != nil { + return "", err + } + r.systemSGID = sgID + return sgID, nil +} + // An available instance whose payload is still staged bootstrapped against an // agent too old to acknowledge, so the ciphertext will sit there until an // operator acts. Only reported: deleting the payload on the grounds that a diff --git a/spinifex/handlers/rds/reconciler_test.go b/spinifex/handlers/rds/reconciler_test.go index bc35cc8d6..d624b5b81 100644 --- a/spinifex/handlers/rds/reconciler_test.go +++ b/spinifex/handlers/rds/reconciler_test.go @@ -8,6 +8,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/rds" + "github.com/mulgadc/spinifex/spinifex/config" "github.com/mulgadc/spinifex/spinifex/testutil" "github.com/nats-io/nats.go" "github.com/stretchr/testify/assert" @@ -270,6 +271,95 @@ func TestReconciler_FallsBackToTheHeartbeatWhenVMStateIsUnwired(t *testing.T) { assert.Equal(t, StatusAvailable, status) } +// The launch surface the retrofit sweep needs: a system VPC to ensure the group +// in, and the ENI surface to move the NIC with. +func withSystemNICPath(enis *fakeENIs) func(*Deps) { + return func(d *Deps) { + d.Launch = LaunchDeps{ + Config: &config.Config{Region: testRegion}, + SystemVPC: (&fakeSystemVPC{}).deps(), + VPC: enis, + } + } +} + +func (h *reconcileHarness) systemSGOf(t *testing.T, id string) string { + t.Helper() + kv, err := h.svc.bucket(t.Context(), testAccountID) + require.NoError(t, err) + var rec DBInstanceRecord + found, err := getJSON(t.Context(), kv, DBInstanceKey(id), &rec) + require.NoError(t, err) + require.True(t, found) + return rec.SystemSGID +} + +// A DB VM launched before the RDS system group existed sits in the system VPC's +// default group, whose sole ingress rule admits every other member of itself. +func TestReconciler_MovesAnExistingSystemNICOntoTheRDSSystemGroup(t *testing.T) { + t.Parallel() + enis := &fakeENIs{} + h := newReconcileHarness(t, withSystemNICPath(enis)) + rec := healthyRecord() + rec.SystemENIID = "eni-sys01" + seedInstance(t, h.svc, rec) + + require.NoError(t, h.rec.reconcileOnce(t.Context())) + + require.Len(t, enis.modified, 1) + assert.Equal(t, "eni-sys01", aws.StringValue(enis.modified[0].NetworkInterfaceId)) + assert.Equal(t, []string{testSystemSG}, aws.StringValueSlice(enis.modified[0].Groups)) + assert.Equal(t, testSystemSG, h.systemSGOf(t, testDBID)) + + // The remediation yields its pass, so the record it wrote is not transitioned + // on a revision the status handler read before the move. + status, _ := h.statusOf(t, testDBID) + assert.Equal(t, StatusCreating, status) + + require.NoError(t, h.rec.reconcileOnce(t.Context())) + assert.Len(t, enis.modified, 1, "once the group is recorded the sweep is a string compare") + status, _ = h.statusOf(t, testDBID) + assert.Equal(t, StatusAvailable, status) +} + +// The group is regional, so the sweep must not describe and create one per DB +// instance per pass. +func TestReconciler_EnsuresTheSystemGroupOncePerProcess(t *testing.T) { + t.Parallel() + enis := &fakeENIs{} + h := newReconcileHarness(t, withSystemNICPath(enis)) + rec := healthyRecord() + rec.SystemENIID = "eni-sys01" + seedInstance(t, h.svc, rec) + + require.NoError(t, h.rec.reconcileOnce(t.Context())) + require.NoError(t, h.rec.reconcileOnce(t.Context())) + + assert.Len(t, enis.sgDescribed, 1) + assert.Len(t, enis.sgCreated, 1) +} + +// Recording the move before vpcd accepted it would leave the ENI in the default +// group with the record claiming otherwise, which no later pass would revisit. +func TestReconciler_RetriesTheSystemNICMoveAfterAFailure(t *testing.T) { + t.Parallel() + enis := &fakeENIs{modifyErr: errors.New("vpcd is not answering")} + h := newReconcileHarness(t, withSystemNICPath(enis)) + rec := healthyRecord() + rec.SystemENIID = "eni-sys01" + seedInstance(t, h.svc, rec) + + err := h.rec.reconcileOnce(t.Context()) + require.Error(t, err) + assert.Contains(t, err.Error(), "vpcd is not answering") + assert.Empty(t, h.systemSGOf(t, testDBID)) + + enis.modifyErr = nil + require.NoError(t, h.rec.reconcileOnce(t.Context())) + assert.Len(t, enis.modified, 1) + assert.Equal(t, testSystemSG, h.systemSGOf(t, testDBID)) +} + // One node does the control work; the rest keep serving the API. A second // claimant must not also believe it is leader, or two nodes would transition // the same records. @@ -290,6 +380,37 @@ func TestReconciler_ElectsASingleLeader(t *testing.T) { assert.False(t, h.rec.acquireOrRefresh(t.Context())) } +func TestReconciler_RefreshesLeadershipOnAnIndependentLoop(t *testing.T) { + t.Parallel() + h := newReconcileHarness(t) + ctx, cancel := context.WithCancel(t.Context()) + done := make(chan struct{}) + go func() { + defer close(done) + h.rec.maintainLeadership(ctx, 5*time.Millisecond) + }() + t.Cleanup(func() { + cancel() + select { + case <-done: + case <-time.After(time.Second): + t.Error("leadership refresh did not stop after cancellation") + } + }) + + require.Eventually(t, h.rec.isLeader, time.Second, 5*time.Millisecond) + kv, err := h.rec.leaderBucket(t.Context()) + require.NoError(t, err) + entry, err := kv.Get(t.Context(), reconcilerLeaderKey) + require.NoError(t, err) + firstRevision := entry.Revision() + + require.Eventually(t, func() bool { + entry, err := kv.Get(t.Context(), reconcilerLeaderKey) + return err == nil && entry.Revision() > firstRevision + }, time.Second, 5*time.Millisecond, "the lease revision must advance without the reconcile loop driving it") +} + // A node that never won the lease must not delete the holder's key on shutdown. func TestReconciler_RelinquishOnlyReleasesItsOwnLease(t *testing.T) { t.Parallel() diff --git a/spinifex/handlers/rds/records.go b/spinifex/handlers/rds/records.go index 5c060d78c..c8e46776f 100644 --- a/spinifex/handlers/rds/records.go +++ b/spinifex/handlers/rds/records.go @@ -72,8 +72,12 @@ type DBInstanceRecord struct { // Where the customer ENI was placed, so a replace lands the new VM's ENI in // the same subnet and security groups without re-deriving them. - SubnetID string `json:"subnetId,omitempty"` - VpcID string `json:"vpcId,omitempty"` + SubnetID string `json:"subnetId,omitempty"` + VpcID string `json:"vpcId,omitempty"` + // The customer VPC's address range, which is the scope the guest's client + // authentication rules are generated against. Captured at create rather than + // resolved per boot, so a bootstrap fetch costs no VPC describe. + VpcCIDR string `json:"vpcCidr,omitempty"` VpcSecurityGroupIDs []string `json:"vpcSecurityGroupIds,omitempty"` DBSubnetGroupName string `json:"dbSubnetGroupName,omitempty"` DBParameterGroupName string `json:"dbParameterGroupName,omitempty"` @@ -92,6 +96,10 @@ type DBInstanceRecord struct { ENIID string `json:"eniId,omitempty"` // Disposable: a replace mints a new one, unlike the customer ENI. SystemENIID string `json:"systemEniId,omitempty"` + // The RDS system security group the system ENI carries. Recorded so the + // reconciler's one-time remediation is a string compare per pass rather than + // an ENI describe, and re-derived from the launch on every replace. + SystemSGID string `json:"systemSgId,omitempty"` // Stable across VM replace, so it serves as both the fallback endpoint and // a durable IP SAN on the serving cert. ENIPrivateIP string `json:"eniPrivateIp,omitempty"` diff --git a/spinifex/handlers/rds/replace.go b/spinifex/handlers/rds/replace.go index c4aa9dd2e..ac90cf8b5 100644 --- a/spinifex/handlers/rds/replace.go +++ b/spinifex/handlers/rds/replace.go @@ -132,6 +132,7 @@ func (s *Service) replaceInstanceVM(ctx context.Context, kv jetstream.KeyValue, if err := s.updateInstance(ctx, kv, rec.DBInstanceIdentifier, func(stored *DBInstanceRecord) { stored.InstanceID = launched.InstanceID stored.SystemENIID = launched.SystemENIID + stored.SystemSGID = launched.SystemSGID stored.DataVolumeSerial = launched.DataVolumeSerial stored.VMGeneration++ stored.FormatAuthorized = false @@ -145,6 +146,7 @@ func (s *Service) replaceInstanceVM(ctx context.Context, kv jetstream.KeyValue, // VM's identity on top of this one. rec.InstanceID = launched.InstanceID rec.SystemENIID = launched.SystemENIID + rec.SystemSGID = launched.SystemSGID rec.DataVolumeSerial = launched.DataVolumeSerial rec.VMGeneration++ rec.FormatAuthorized = false diff --git a/spinifex/handlers/rds/vpc.go b/spinifex/handlers/rds/vpc.go index 99a2a1afd..72261221b 100644 --- a/spinifex/handlers/rds/vpc.go +++ b/spinifex/handlers/rds/vpc.go @@ -5,11 +5,20 @@ import ( "errors" "fmt" + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" "github.com/mulgadc/spinifex/spinifex/config" handlers_systemvpc "github.com/mulgadc/spinifex/spinifex/handlers/systemvpc" "github.com/mulgadc/spinifex/spinifex/tags" ) +// The security-group surface the system ENI's group is ensured through. Narrow +// on purpose: this never authorizes a rule, so it holds no authorize verb. +type systemSecurityGroupProvisioner interface { + CreateSecurityGroup(ctx context.Context, input *ec2.CreateSecurityGroupInput, accountID string) (*ec2.CreateSecurityGroupOutput, error) + DescribeSecurityGroups(ctx context.Context, input *ec2.DescribeSecurityGroupsInput, accountID string) (*ec2.DescribeSecurityGroupsOutput, error) +} + // One shared RDS system VPC per region holds every DB VM's primary NIC. DB // instances are isolated by their own customer ENI and security group, so a VPC // per instance would add a NAT gateway and EIP without adding isolation. @@ -28,6 +37,12 @@ func SystemVPCName(region string) string { return "rds-system-" + region } +// One group per region, shared by every DB VM's system ENI. The name is the +// lookup key, so it must stay stable across releases too. +func SystemSecurityGroupName(region string) string { + return "rds-system-" + region +} + // cfg supplies the operator-overridable address space and subnet count; a nil // or unset cfg falls back to the defaults. func SystemVPCSpec(cfg *config.RDSConfig, region string) handlers_systemvpc.Spec { @@ -56,7 +71,10 @@ func SystemVPCSpec(cfg *config.RDSConfig, region string) handlers_systemvpc.Spec } // Idempotent. The private subnet the DB VMs sit in routes 0.0.0.0/0 to the -// VPC's NAT gateway, which is the agent's only egress to the gateway. +// VPC's NAT gateway, which is the agent's egress to the gateway wherever no +// management bridge exists. On a formed deployment one always does, and the +// agent reaches the gateway over it instead — so this path carries almost no +// required traffic. func EnsureSystemVPC(ctx context.Context, deps handlers_systemvpc.Deps, cfg *config.RDSConfig, accountID, region string) (*handlers_systemvpc.Refs, error) { if region == "" { return nil, errors.New("rds: EnsureSystemVPC empty region") @@ -70,3 +88,76 @@ func EnsureSystemVPC(ctx context.Context, deps handlers_systemvpc.Deps, cfg *con } return refs, nil } + +// The security group every DB VM's system ENI carries, found by name or created. +// Idempotent, and shared across the region: nothing deletes it per instance. +// +// No ingress rules are authorized, and none are needed. Nothing in the platform +// connects to a DB VM's system ENI — the agent long-polls outbound, the control +// plane never dials the engine, and the gateway is reached over the management +// bridge. The ACLs are allow-related, so an agent-initiated connection's return +// traffic is matched by conntrack rather than by an ingress rule. +// +// The whole of the fix is not landing in the VPC's default group, whose one +// ingress rule admits every other member of itself — which is every DB VM in +// the deployment, across every account. +func EnsureSystemSecurityGroup(ctx context.Context, sgs systemSecurityGroupProvisioner, accountID, region, vpcID string) (string, error) { + if region == "" { + return "", errors.New("rds: EnsureSystemSecurityGroup empty region") + } + if vpcID == "" { + return "", errors.New("rds: EnsureSystemSecurityGroup empty vpc id") + } + name := SystemSecurityGroupName(region) + + out, err := sgs.DescribeSecurityGroups(ctx, &ec2.DescribeSecurityGroupsInput{ + Filters: []*ec2.Filter{ + {Name: aws.String("group-name"), Values: aws.StringSlice([]string{name})}, + {Name: aws.String("vpc-id"), Values: aws.StringSlice([]string{vpcID})}, + }, + }, accountID) + if err != nil { + return "", fmt.Errorf("rds: describe the system security group %s: %w", name, err) + } + var matches []*ec2.SecurityGroup + if out != nil { + for _, group := range out.SecurityGroups { + if group == nil || aws.StringValue(group.GroupId) == "" { + return "", fmt.Errorf("rds: describe the system security group %s: group has no id", name) + } + matches = append(matches, group) + } + } + if len(matches) > 1 { + return "", fmt.Errorf("rds: describe the system security group %s: found %d groups, want exactly one", name, len(matches)) + } + if len(matches) == 1 { + group := matches[0] + id := aws.StringValue(group.GroupId) + if len(group.IpPermissions) != 0 { + return "", fmt.Errorf("rds: system security group %s (%s) has ingress rules; refusing to attach DB system NICs", name, id) + } + return id, nil + } + + created, err := sgs.CreateSecurityGroup(ctx, &ec2.CreateSecurityGroupInput{ + GroupName: aws.String(name), + Description: aws.String("RDS management NIC security group; no ingress"), + VpcId: aws.String(vpcID), + TagSpecifications: []*ec2.TagSpecification{{ + ResourceType: aws.String("security-group"), + Tags: []*ec2.Tag{ + {Key: aws.String(tags.ManagedByKey), Value: aws.String(tags.ManagedByRDS)}, + {Key: aws.String(rdsSystemVPCTagKey), Value: aws.String(SystemVPCName(region))}, + {Key: aws.String(rdsSystemVPCRoleTagKey), Value: aws.String(rdsSystemVPCRolePrefix)}, + }, + }}, + }, accountID) + if err != nil { + return "", fmt.Errorf("rds: create the system security group %s: %w", name, err) + } + if created == nil || aws.StringValue(created.GroupId) == "" { + return "", fmt.Errorf("rds: create the system security group %s: no group id returned", name) + } + return aws.StringValue(created.GroupId), nil +} diff --git a/tests/e2e/harness/rds.go b/tests/e2e/harness/rds.go index daafe5d9d..7034f359d 100644 --- a/tests/e2e/harness/rds.go +++ b/tests/e2e/harness/rds.go @@ -5,12 +5,14 @@ package harness import ( "encoding/json" "fmt" + "strings" "testing" "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" "github.com/aws/aws-sdk-go/service/rds" + "github.com/mulgadc/spinifex/spinifex/daemon" handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" "github.com/mulgadc/spinifex/spinifex/tags" "github.com/mulgadc/spinifex/spinifex/utils" @@ -316,6 +318,91 @@ func dbInstanceVolumes(system *AWSClient, id string) ([]*ec2.Volume, error) { return out.Volumes, nil } +// DBSystemENI returns the DB VM's management NIC in the shared RDS system VPC, +// read with the system credentials that own it. +// +// This is the NIC no customer security group governs and no customer can see, +// which is exactly why an isolation assertion has to look at it: every DB VM in +// the deployment, across every account, has one in the same VPC. +func DBSystemENI(t *testing.T, system *AWSClient, id string) *ec2.NetworkInterface { + t.Helper() + out, err := system.EC2.DescribeNetworkInterfaces(&ec2.DescribeNetworkInterfacesInput{ + Filters: []*ec2.Filter{{ + Name: aws.String("description"), + Values: []*string{aws.String("RDS management NIC for " + id)}, + }}, + }) + if err != nil { + t.Fatalf("DBSystemENI %s: describe-network-interfaces: %v", id, err) + } + if len(out.NetworkInterfaces) != 1 { + t.Fatalf("DBSystemENI %s: found %d matching ENIs, want 1", id, len(out.NetworkInterfaces)) + } + return out.NetworkInterfaces[0] +} + +// The prefix daemon namespaces its management-IP allocations under in the +// cluster-state bucket. Duplicated rather than imported because the daemon keeps +// the key unexported. +const mgmtIPAMKeyPrefix = "mgmt-ipam." + +// DBInstanceMgmtIP resolves a DB VM's br-mgmt address from the cluster's +// management IPAM record. +// +// No API exposes it: the management NIC is host-local plumbing rather than a VPC +// interface. The runner sits on the same bridge, which makes this the one path +// from which a DB VM's engine port can be dialled without a customer VPC — and +// so the one place the bind can be proven from outside the guest. +func DBInstanceMgmtIP(t *testing.T, env *Env, system *AWSClient, id string) string { + t.Helper() + vm := DBInstanceVM(t, system, id) + instanceID := aws.StringValue(vm.InstanceId) + + host, token, ca := natsConn(t, env) + nc, err := utils.ConnectNATS(host, token, ca) + if err != nil { + t.Fatalf("DBInstanceMgmtIP %s: connect NATS %s: %v", id, host, err) + } + defer nc.Close() + + js, err := jetstream.New(nc) + if err != nil { + t.Fatalf("DBInstanceMgmtIP %s: jetstream context: %v", id, err) + } + kv, err := js.KeyValue(t.Context(), daemon.ClusterStateBucket) + if err != nil { + t.Fatalf("DBInstanceMgmtIP %s: open %s: %v", id, daemon.ClusterStateBucket, err) + } + keys, err := kv.Keys(t.Context()) + if err != nil { + t.Fatalf("DBInstanceMgmtIP %s: list %s: %v", id, daemon.ClusterStateBucket, err) + } + + // br-mgmt can be more than one /24 across a cluster, so every allocation + // record is scanned rather than the local node's. + for _, key := range keys { + if !strings.HasPrefix(key, mgmtIPAMKeyPrefix) { + continue + } + entry, err := kv.Get(t.Context(), key) + if err != nil { + t.Fatalf("DBInstanceMgmtIP %s: read %s: %v", id, key, err) + } + var rec daemon.MgmtIPRecord + if err := json.Unmarshal(entry.Value(), &rec); err != nil { + t.Fatalf("DBInstanceMgmtIP %s: decode %s: %v", id, key, err) + } + for _, alloc := range rec.Allocated { + if alloc.InstanceID == instanceID && alloc.IP != "" { + t.Logf("DB instance %s VM %s has mgmt address %s", id, instanceID, alloc.IP) + return alloc.IP + } + } + } + t.Fatalf("DBInstanceMgmtIP %s: VM %s holds no management IP allocation", id, instanceID) + return "" +} + // dbEndpointENIs finds a DB instance's endpoint ENI in the tenant account. ENIs // ignore tag filters, so the description the control plane sets at launch is the // only discriminator available. diff --git a/tests/e2e/rds/isolation_test.go b/tests/e2e/rds/isolation_test.go new file mode 100644 index 000000000..b4337816e --- /dev/null +++ b/tests/e2e/rds/isolation_test.go @@ -0,0 +1,163 @@ +//go:build e2e + +package rds + +import ( + "errors" + "fmt" + "net" + "syscall" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + handlers_rds "github.com/mulgadc/spinifex/spinifex/handlers/rds" + "github.com/mulgadc/spinifex/tests/e2e/harness" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Long enough that a slow datapath is not read as a refusal, short enough that +// four probes do not dominate the test. +const isolationDialTimeout = 5 * time.Second + +// TestDBVMIsolation asserts that a DB instance's endpoint is the only path to +// its engine. +// +// A DB VM has three NICs. Only one of them — the customer ENI the endpoint +// resolves to — is governed by a customer security group. The other two are the +// system NIC in the RDS system VPC every DB VM in the deployment shares, and a +// management NIC on the host's br-mgmt bridge, which is plain L2 with no ACLs at +// all. An engine bound to the wildcard is therefore reachable on both, from any +// account, and the customer's security group is not consulted. +// +// Two DB instances in two tenants, because one instance can only show that a +// path is closed to the runner; two show it is closed between tenants, which is +// what the exposure actually was. +func TestDBVMIsolation(t *testing.T) { + f := requireRDSFixture(t) + t.Parallel() + reserveDBVMs(t, dbClass, dbClass) + + suffix := time.Now().Unix() + mine := fmt.Sprintf("%s-iso-a-%d", dbInstancePfx, suffix) + theirs := fmt.Sprintf("%s-iso-b-%d", dbInstancePfx, suffix) + + harness.Phase(t, "Creating DB instance %q in this tenant", mine) + createDBInstance(t, f, mine) + + harness.Phase(t, "Creating DB instance %q in a second tenant", theirs) + other := isolationSecondTenant(t, f, suffix) + _, err := other.RDS.CreateDBInstance(validCreateInput(theirs)) //nolint:staticcheck // e2e:allow-create — the second tenant's own instance + require.NoError(t, err, "create-db-instance %s in the second tenant", theirs) + t.Cleanup(func() { deleteInstanceAs(t, other, theirs) }) + + instance := waitForAvailable(t, f, mine) + harness.WaitForDBInstanceAvailable(t, other, theirs) + enginePort := aws.Int64Value(instance.Endpoint.Port) + require.NotZero(t, enginePort, "an available instance must publish an endpoint port") + + system := f.SystemAWS(t) + + // The direct proof, and the only leg that covers the co-location path: the + // runner sits on br-mgmt and can address any DB VM's management NIC on it, + // with no security group and no VPC in the way. A wildcard bind answers here. + t.Run("TheEngineIsNotBoundOnTheManagementBridge", func(t *testing.T) { + for _, id := range []string{mine, theirs} { + mgmtIP := harness.DBInstanceMgmtIP(t, f.Env, system, id) + requireRunnerOnMgmtBridge(t, mgmtIP) + + conn, err := net.DialTimeout("tcp", net.JoinHostPort(mgmtIP, fmt.Sprint(enginePort)), isolationDialTimeout) + if err == nil { + _ = conn.Close() + t.Errorf("%s: the engine answered on the management bridge at %s:%d; "+ + "that NIC is governed by no security group, so this is reachable from every DB VM on the host", + id, mgmtIP, enginePort) + continue + } + if !errors.Is(err, syscall.ECONNREFUSED) { + t.Errorf("%s: management bridge %s:%d could not be tested: want connection refused, got %v", + id, mgmtIP, enginePort, err) + continue + } + t.Logf("%s: management bridge %s:%d refused as expected", id, mgmtIP, enginePort) + } + }) + + // The second layer, asserted from the control plane: even a wildcard bind must + // not be admitted on the system NIC. Both instances' system NICs live in the + // same shared VPC, so the group they carry is what separates them. + t.Run("TheSystemNICCarriesAnIngressFreeGroup", func(t *testing.T) { + want := handlers_rds.SystemSecurityGroupName(f.Region) + + var vpcIDs []string + for _, id := range []string{mine, theirs} { + eni := harness.DBSystemENI(t, system, id) + vpcIDs = append(vpcIDs, aws.StringValue(eni.VpcId)) + + require.Len(t, eni.Groups, 1, "%s: the system NIC must carry exactly one group", id) + group := isolationDescribeSG(t, system, aws.StringValue(eni.Groups[0].GroupId)) + + // Landing in the VPC's default group is the whole of the exposure: its + // one ingress rule admits every other member of itself, which in this + // VPC is every DB VM in the deployment, across every account. + assert.Equal(t, want, aws.StringValue(group.GroupName), + "%s: the system NIC must carry the RDS system group, not the VPC default", id) + assert.Empty(t, group.IpPermissions, + "%s: nothing dials a DB VM's system NIC, so its group must authorize no ingress", id) + } + + require.Len(t, vpcIDs, 2) + assert.Equal(t, vpcIDs[0], vpcIDs[1], + "both tenants' system NICs share one VPC, which is why the group has to be the boundary") + }) + + // The regression half: closing those paths must not have closed the one the + // customer actually uses. + t.Run("TheEndpointStillReachesTheEngine", func(t *testing.T) { + client := rdsClient(t, f) + out := harness.PSQL(t, client, harness.PSQLConnFor(t, instance, dbMasterUser, dbMasterPassword, dbName), + "SELECT 1;") + assert.Contains(t, out, "1", "the endpoint must still serve the customer it is bound for") + }) +} + +// A second tenant with its own account, so the two DB VMs share nothing but the +// platform underneath them. +func isolationSecondTenant(t *testing.T, f *Fixture, suffix int64) *harness.AWSClient { + t.Helper() + carousel := harness.NewAccountCarousel() + tenant := carousel.Add(t, f.Env, "rds-e2e-isolation", + harness.SpxAdminAccountCreate(t, fmt.Sprintf("RDS E2E Isolation %d", suffix), "")) + return tenant.Client +} + +// A refused connection proves nothing if the runner has no route to br-mgmt at +// all, so the probe's premise is checked rather than assumed: the bridge is one +// flat segment across the cluster, and the runner holds an address on it. +func requireRunnerOnMgmtBridge(t *testing.T, mgmtIP string) { + t.Helper() + target := net.ParseIP(mgmtIP) + require.NotNil(t, target, "management address %q is not an IP", mgmtIP) + + addrs, err := net.InterfaceAddrs() + require.NoError(t, err, "read the runner's own interface addresses") + for _, addr := range addrs { + if ipnet, ok := addr.(*net.IPNet); ok && ipnet.Contains(target) { + return + } + } + t.Fatalf("the runner holds no address on the segment carrying %s, so a refused connection "+ + "would prove a missing route rather than an unbound engine", mgmtIP) +} + +func isolationDescribeSG(t *testing.T, system *harness.AWSClient, groupID string) *ec2.SecurityGroup { + t.Helper() + out, err := system.EC2.DescribeSecurityGroups(&ec2.DescribeSecurityGroupsInput{ + GroupIds: []*string{aws.String(groupID)}, + }) + require.NoError(t, err, "describe-security-groups %s", groupID) + require.Len(t, out.SecurityGroups, 1, "describe-security-groups %s returned no group", groupID) + return out.SecurityGroups[0] +}