From b01969d1d822fcee4b1b7cadef13dbef438555a5 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Sat, 29 Aug 2026 14:31:43 -0700 Subject: [PATCH] fix(backup): reconcile restored credentials Physical backups retain the source role hash while a target host owns a different managed client password. Verify and, when required, reconcile the staged role before cutover. Closes #137 --- e2e/server_test.go | 25 ++++- internal/engine/backup_restore.go | 90 +++++++++++++++++ internal/engine/backup_restore_client_test.go | 98 +++++++++++++++++++ 3 files changed, 212 insertions(+), 1 deletion(-) create mode 100644 internal/engine/backup_restore_client_test.go diff --git a/e2e/server_test.go b/e2e/server_test.go index 1808dc1..877fee8 100644 --- a/e2e/server_test.go +++ b/e2e/server_test.go @@ -94,6 +94,21 @@ func (s *server) psql(t *testing.T, query string) string { `docker exec `+container+` sh -c 'echo `+encoded+` | base64 -d | psql -U "$POSTGRES_USER" -d "$POSTGRES_DB" -tA'`)) } +// psqlClient proves the connection contract workloads receive rather than the +// local administrative socket used by psql above. +func (s *server) psqlClient(t *testing.T, query string) string { + t.Helper() + container := strings.TrimSpace(s.run(t, + `docker ps -q --filter label=com.docker.compose.service=postgres | head -1`)) + if container == "" { + t.Fatal("no postgres container is running") + } + encoded := base64.StdEncoding.EncodeToString([]byte(query)) + return strings.TrimSpace(s.run(t, + `docker exec `+container+` sh -c 'set -- $(hostname -i); export PGPASSWORD="$POSTGRES_PASSWORD"; echo `+encoded+ + ` | base64 -d | psql -X -w -h "$1" -U "$POSTGRES_USER" -d "$POSTGRES_DB" -tA'`)) +} + // rotateWAL closes the current segment so there is something to archive. // // pg_switch_wal() alone is not enough: PostgreSQL skips the switch when @@ -398,10 +413,18 @@ HTTPServer(("127.0.0.1", 18080), Handler).handle_request() s.psql(t, "create table if not exists survivors(note text)") s.psql(t, "insert into survivors values (concat('written', ' ', 'before'))") s.mustOb(t, dir, "backup", "create", "postgres") - s.mustOb(t, dir, "backup", "restore", "postgres", "--confirm", "postgres") + generation := s.psql(t, "select system_identifier::text from pg_control_system()") + // A new host owns a different generated credential while the physical + // generation still carries the source role hash. Rotating only the target + // file reproduces that boundary without requiring a second test server. + s.run(t, "printf 'POSTGRES_PASSWORD=%s\\n' 0123456789abcdef0123456789abcdef0123456789abcdef > /var/lib/ob/observer/services/postgres.secret.env") + s.mustOb(t, dir, "backup", "restore", "postgres", "--generation", generation, "--confirm", "postgres") if note := s.psql(t, "select note from survivors limit 1"); note != "written before" { t.Fatalf("the recovered cluster does not hold the row: %q", note) } + if note := s.psqlClient(t, "select note from survivors limit 1"); note != "written before" { + t.Fatalf("the recovered cluster rejects the target-managed client credential: %q", note) + } }) t.Run("backup prune", func(t *testing.T) { diff --git a/internal/engine/backup_restore.go b/internal/engine/backup_restore.go index be56c08..13e049a 100644 --- a/internal/engine/backup_restore.go +++ b/internal/engine/backup_restore.go @@ -169,6 +169,19 @@ func (e *Engine) RecoverService(ctx context.Context, service, targetTime string, } st(nil) + // A physical backup carries the source cluster's role password hashes. The + // target-side client credential is deliberately not part of that backup, so a + // generation recovered on another host can start and answer local queries while + // refusing every application connection. Prove the managed client contract on + // the staged cluster, and reconcile it there when necessary, before promotion + // is allowed to touch the live volume. + st = e.ui.Step("recovery: verify managed client authentication", false) + if err := e.ensureRecoveredClientCredential(ctx, container); err != nil { + st(err) + return outcome, err + } + st(nil) + if !promote { return outcome, nil } @@ -240,6 +253,10 @@ func (e *Engine) startRecoveryContainer(ctx context.Context, container, staging, "--entrypoint", "sleep", "-v", q(staging + ":/var/lib/postgresql/data"), "-v", q(n.BackupRuntimeDir(service) + ":" + app.WalgMountPath + ":ro"), + // The target-managed PostgreSQL credential is needed only inside the + // recovery container. Passing the file by name keeps its value out of this + // command, transport logs, recovery evidence, and process arguments. + "--env-file", q(n.ServiceSecretFile(service)), "--env-file", q(n.BackupCredentialFile(service, credentialTarget)), } for _, key := range sortedEnvKeys(environment) { @@ -544,6 +561,79 @@ func (e *Engine) probeRecoveredCluster(ctx context.Context, container, service, return recovered, tables + " tables in public schema", nil } +// ensureRecoveredClientCredential proves that the recovered cluster accepts the +// target host's managed application credential. Physical recovery restores role +// password hashes from the source cluster, while the target credential file is +// host-owned and intentionally survives outside the data volume. +// +// A same-host restore normally succeeds on the first probe and leaves pg_authid +// byte-for-byte as recovered. A cross-host restore reconciles only the staged +// cluster, then has to pass the same TCP/password query before cutover can begin. +func (e *Engine) ensureRecoveredClientCredential(ctx context.Context, container string) error { + ok, detail, err := e.recoveredClientAnswers(ctx, container) + if err != nil { + return err + } + if ok { + return nil + } + + // psql reads POSTGRES_PASSWORD from the recovery container's environment and + // quotes it as a SQL literal. Neither the command nor stdin contains the + // credential value, so transport logging and test captures remain safe. + command := "docker exec -i -u postgres " + q(container) + + " psql -X -v ON_ERROR_STOP=1 -U " + q(app.PgSuperuser) + " -d postgres" + script := "\\getenv ob_managed_password POSTGRES_PASSWORD\n" + + "ALTER ROLE \"" + app.PgSuperuser + "\" PASSWORD :'ob_managed_password';\n" + res, err := e.T.RunInput(ctx, command, script) + if err != nil { + return err + } + if res.ExitCode != 0 { + return fmt.Errorf("the recovered cluster refused managed client credential reconciliation after %s: %s", + detail, lastLines(res.Stderr+res.Stdout, 3)) + } + + ok, detail, err = e.recoveredClientAnswers(ctx, container) + if err != nil { + return err + } + if !ok { + return fmt.Errorf("the recovered cluster does not accept the target-managed client credential after reconciliation: %s", detail) + } + return nil +} + +// recoveredClientAnswers uses TCP deliberately. The administrative recovery +// checks use a local socket and can succeed without proving password +// authentication; applications use the generated client credential over the +// service network. PostgreSQL's image trusts loopback in pg_hba.conf, so the +// probe uses the staging container's own non-loopback address; that reaches the +// staged server without colliding with the still-live service's network alias +// and exercises the catch-all password rule applications use. +func (e *Engine) recoveredClientAnswers(ctx context.Context, container string) (bool, string, error) { + inner := "test -n \"${POSTGRES_PASSWORD:-}\" || { echo 'managed PostgreSQL credential is missing' >&2; exit 1; }; " + + "set -- $(hostname -i); test \"$#\" -gt 0 || { echo 'recovery container has no network address' >&2; exit 1; }; host=$1; " + + "export PGPASSWORD=\"$POSTGRES_PASSWORD\" PGCONNECT_TIMEOUT=5; " + + "exec psql -X -w -h \"$host\" -U " + q(app.PgSuperuser) + " -d " + q(e.Spec.Spec.Name) + " -Atc " + q("SELECT 1;") + command := "docker exec -u postgres " + q(container) + " sh -ceu " + q(inner) + res, err := e.T.Run(ctx, command) + if err != nil { + return false, "", err + } + if res.ExitCode != 0 { + detail := lastLines(res.Stderr+res.Stdout, 3) + if detail == "" { + detail = fmt.Sprintf("client probe exited %d", res.ExitCode) + } + return false, detail, nil + } + if strings.TrimSpace(res.Stdout) != "1" { + return false, fmt.Sprintf("client probe returned %q instead of 1", strings.TrimSpace(res.Stdout)), nil + } + return true, "", nil +} + // promoteRecoveredVolume puts the recovered data in front of the application. // // The previous volume is renamed, never deleted by restore. A restore is the diff --git a/internal/engine/backup_restore_client_test.go b/internal/engine/backup_restore_client_test.go new file mode 100644 index 0000000..bdb1bba --- /dev/null +++ b/internal/engine/backup_restore_client_test.go @@ -0,0 +1,98 @@ +package engine + +import ( + "context" + "strings" + "testing" + + "github.com/labstack/onebox/internal/transport" +) + +func TestRecoveredClientCredentialAlreadyMatchesWithoutReconciliation(t *testing.T) { + fake := &transport.Fake{Dynamic: func(command string) (transport.Result, bool) { + if strings.Contains(command, "PGCONNECT_TIMEOUT=5") { + return transport.Result{Stdout: "1\n"}, true + } + return transport.Result{}, false + }} + engine := backupLockTestEngine(fake) + + if err := engine.ensureRecoveredClientCredential(context.Background(), "restore"); err != nil { + t.Fatal(err) + } + if len(fake.Inputs) != 0 { + t.Fatalf("matching credential changed the recovered role: %#v", fake.Inputs) + } + probe := strings.Join(fake.Commands, "\n") + if !strings.Contains(probe, "hostname -i") || !strings.Contains(probe, `-h "$host"`) || !strings.Contains(probe, "PGPASSWORD") { + t.Fatalf("managed client probe did not force TCP password authentication:\n%s", probe) + } + if strings.Contains(probe, "-h 127.0.0.1") { + t.Fatalf("managed client probe used PostgreSQL's trusted loopback rule:\n%s", probe) + } +} + +func TestRecoveredClientCredentialIsReconciledAndVerified(t *testing.T) { + probes := 0 + fake := &transport.Fake{Dynamic: func(command string) (transport.Result, bool) { + if strings.Contains(command, "PGCONNECT_TIMEOUT=5") { + probes++ + if probes == 1 { + return transport.Result{ExitCode: 2, Stderr: "password authentication failed"}, true + } + return transport.Result{Stdout: "1\n"}, true + } + return transport.Result{}, false + }} + engine := backupLockTestEngine(fake) + + if err := engine.ensureRecoveredClientCredential(context.Background(), "restore"); err != nil { + t.Fatal(err) + } + if probes != 2 { + t.Fatalf("managed client probes = %d, want initial failure and post-reconciliation proof", probes) + } + if len(fake.Inputs) != 1 || !strings.Contains(fake.Inputs[0], `ALTER ROLE "onebox"`) { + t.Fatalf("reconciliation input = %#v", fake.Inputs) + } + if !strings.Contains(fake.Inputs[0], `\getenv ob_managed_password POSTGRES_PASSWORD`) { + t.Fatalf("reconciliation did not read the credential inside psql: %q", fake.Inputs[0]) + } +} + +func TestRecoveredClientCredentialFailureStopsAfterVerification(t *testing.T) { + probes := 0 + fake := &transport.Fake{Dynamic: func(command string) (transport.Result, bool) { + if strings.Contains(command, "PGCONNECT_TIMEOUT=5") { + probes++ + return transport.Result{ExitCode: 2, Stderr: "password authentication failed"}, true + } + return transport.Result{}, false + }} + engine := backupLockTestEngine(fake) + + err := engine.ensureRecoveredClientCredential(context.Background(), "restore") + if err == nil || !strings.Contains(err.Error(), "does not accept the target-managed client credential") { + t.Fatalf("verification error = %v", err) + } + if probes != 2 || len(fake.Inputs) != 1 { + t.Fatalf("probes/updates = %d/%d, want 2/1", probes, len(fake.Inputs)) + } +} + +func TestRecoveryContainerReceivesTargetServiceCredentialByFile(t *testing.T) { + fake := &transport.Fake{} + engine := backupLockTestEngine(fake) + + err := engine.startRecoveryContainer(context.Background(), "restore", "stage", "postgres:18", nil, "postgres", "offsite") + if err != nil { + t.Fatal(err) + } + command := fake.Commands[0] + secret := engine.names().ServiceSecretFile("postgres") + backup := engine.names().BackupCredentialFile("postgres", "offsite") + secretAt, backupAt := strings.Index(command, secret), strings.Index(command, backup) + if secretAt < 0 || backupAt < 0 || secretAt >= backupAt { + t.Fatalf("recovery env files do not contain service then backup credentials:\n%s", command) + } +}