Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions docs/engine-role.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ requires — nothing higher.
| --- | --- | --- | --- |
| 0 | Connect and resolve the target | `LOGIN`; `CONNECT` on the database; `USAGE` on the target schema (directly or via membership) | `has_database_privilege`, `has_schema_privilege(..., 'USAGE')` |
| 1 | In-place `ALTER TABLE` (the instant and fast native paths) | Inheritable **membership in the owning role** — sufficient on its own | `pg_has_role(current_user, <owner>, 'USAGE')` |
| 2 | Index builds (`CREATE INDEX [CONCURRENTLY]`) | Tier 1 + **`CREATE` on the target schema** — table ownership alone is refused with `permission denied for schema` | `has_schema_privilege(..., 'CREATE')` |
| 3 | Copy-and-swap | Tier 2 + membership usable with `SET ROLE` (for owner-correct shadow objects); for logical-decoding CDC: `rds_replication` membership (Aurora/RDS) or the `REPLICATION` attribute (self-managed) | `pg_has_role(..., 'MEMBER')` (14–15) / `pg_has_role(..., 'SET')` (16+); `pg_has_role(current_user, 'rds_replication', 'MEMBER')` |
| 4 | Planner scratch database (execute-and-introspect) | A pre-provisioned `pg_sprite_scratch` owned by the engine role, **or** `CREATEDB` | `pg_database` ownership or `pg_roles.rolcreatedb` |
| 2 | Index builds: `CREATE INDEX [CONCURRENTLY]`, and the `ALTER TABLE` shapes that build one — `ADD CONSTRAINT UNIQUE` / `PRIMARY KEY` / `EXCLUDE` without `USING INDEX`, or `ADD COLUMN` with an inline `UNIQUE` / `PRIMARY KEY` | Tier 1 + **`CREATE` on the target schema** — table ownership alone is refused with `permission denied for schema` | `has_schema_privilege(..., 'CREATE')` |
| 3 | Copy-and-swap | Tier 2 + membership usable with `SET ROLE` (for owner-correct shadow objects); for logical-decoding CDC: `rds_replication` membership (Aurora/RDS) or the `REPLICATION` attribute (self-managed) | `pg_has_role(..., 'SET')` (16+; on 14–15 the Tier 1 `USAGE` check already proves `SET ROLE` access — membership options arrive in 16); `pg_has_role(current_user, 'rds_replication', 'MEMBER')` |
| 4 | Planner scratch database (execute-and-introspect) | A pre-provisioned `pg_sprite_scratch` owned by the engine role, **or** `CREATEDB` | *Not yet implemented* — a missing scratch database surfaces at scratch creation, not in the preflight |

Two cluster-level *facts* — settings, not grants — accompany Tier 3 and are checked in the
same preflight: `wal_level = logical` (`rds.logical_replication = 1` on Aurora/RDS, a
Expand All @@ -70,11 +70,18 @@ GRANT USAGE, CREATE ON SCHEMA app TO app_owner; -- if the owner lacks it
GRANT rds_replication TO pgsprite_engine;
```

The contract covers the target table's own access. A `FOREIGN KEY` that references a
table owned by a *different* role additionally needs `REFERENCES` on the referenced
table (`GRANT REFERENCES ON <referenced> TO <owning role>`), which the preflight does
not check — where every table shares one owning role, the owner already holds it.

One membership grant per owning role: a database where every schema is owned by one
application role needs exactly one `GRANT`. On PostgreSQL 16+ the membership defaults
include `SET TRUE` and `INHERIT TRUE`, which this contract relies on; grants issued with
`WITH SET FALSE` or to a `NOINHERIT` engine role break Tiers 1 and 3 and are caught by the
preflight checks above.
application role needs exactly one `GRANT`. This contract relies on the membership being
inheritable and `SET ROLE`-capable; grants issued `WITH SET FALSE` or to a `NOINHERIT`
engine role break Tiers 1 and 3, and the preflight refuses them naming the statement that
actually repairs the option — `GRANT ... WITH INHERIT TRUE` / `WITH SET TRUE` on
PostgreSQL 16+, `ALTER ROLE ... INHERIT` on 14–15, where inheritance is a role attribute
that `GRANT` cannot change.

## What the engine role must not have

Expand Down
8 changes: 8 additions & 0 deletions docs/testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,14 @@ generated CA for verify-full tests. The harness has its own tests proving
the version selected by `PG_VERSION` is the version actually running, and
that throwaway schemas are isolated.

The privilege-ladder tests additionally create throwaway **cluster-level
roles** (`NewRole`), so the role behind an external `PG_DSN` needs
`CREATEROLE` — a step up from "a database you can create schemas in".
The compose database and per-test containers connect as superuser, so
this only matters when pointing `PG_DSN` at a shared server; tests whose
requirements go further (replication attributes) skip themselves when the
server refuses.

## Aurora-shaped environments: three tiers, each proving what it can

CI runs the matrix against **vanilla PostgreSQL 14 → 18 images** — the floor
Expand Down
37 changes: 36 additions & 1 deletion internal/cli/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,29 @@ func (c *MigrateCmd) auditForce(st statement.Statement, rs router.Statement) {
// same brief budgets. Blind attempts of the submitted form — including
// forced ones — are size-guarded; substituted sequences and planner-proven
// online idioms are not — long work on large tables is their purpose, and
// every brief step is still budget-bounded.
// every brief step is still budget-bounded. Before anything runs, the
// connected role is checked at the tier the routed steps actually need
// (engine-role contract), so a role that would die mid-change is refused
// with the exact provisioning statement instead.
func (c *MigrateCmd) execute(ctx context.Context, out io.Writer, pool *pgxpool.Pool,
st statement.Statement, execSQL []string, plan planner.Plan,
substituted, forced bool, logger *slog.Logger) error {
tier, err := preflight.RequiredTier(execSQL)
if err != nil {
return err
}
priv, err := preflight.CheckPrivileges(ctx, pool, st.Schema(), st.Table(),
preflight.Requirement{Tier: tier})
var privErr *preflight.PrivilegeError
if errors.As(err, &privErr) {
return c.emit(out, privilegeVerdict(st, privErr, forced))
}
if err != nil {
return err
}
logger.Debug("privilege preflight passed",
"role", priv.Role(), "owner", priv.Owner(), "tier", tier.String())

limit := int64(c.MaxTableSize)
if !sizeGuardApplies(plan, substituted) {
limit = preflight.NoSizeLimit
Expand Down Expand Up @@ -436,6 +455,22 @@ func indexAdvice(st statement.Statement) (detail, saferIdiom string) {
}
}

// privilegeVerdict is the refusal for a connected role that lacks the access
// the routed change needs. The error already names the failed catalog check
// and the exact provisioning statement, so it is the detail verbatim. A
// refused forced attempt still records the override: the operator asked for
// the submitted form and the role could not run it.
func privilegeVerdict(st statement.Statement, privErr *preflight.PrivilegeError, forced bool) verdict.Verdict {
return verdict.Verdict{
Outcome: verdict.OutcomeRefused,
Reason: verdict.ReasonInsufficientPrivileges,
Statement: st.SQL(),
Table: qualified(st),
Forced: forced,
Detail: privErr.Error(),
}
}

// rewriteRequiredVerdict is the refusal for a statement whose submitted
// form blocks but for which the planner could not construct the safer
// native sequence — a multi-operation statement, or a pattern it cannot
Expand Down
119 changes: 119 additions & 0 deletions internal/cli/migrate_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,125 @@ func TestMigrateSizeGuardSkipsAttempt(t *testing.T) {
assert.Zero(t, n, "the size guard must skip the attempt entirely")
}

// A connected role without membership in the owning role is refused up
// front with a typed privilege verdict — instead of the server's mid-change
// "must be owner" error — and the same change executes once the exact
// membership the refusal names is granted.
func TestMigrateRefusesInsufficientPrivileges(t *testing.T) {
serverURL := testutil.StartPostgres(t)
admin, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: serverURL})
require.NoError(t, err)
defer admin.Close()

owner := testutil.NewRole(t, admin, "NOLOGIN")
const password = "engine-test-password"
engine := testutil.NewRole(t, admin, "LOGIN PASSWORD '"+password+"'")
schema := testutil.NewSchema(t, admin)
_, err = admin.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.t (id int PRIMARY KEY)", schema))
require.NoError(t, err)
_, err = admin.Exec(t.Context(), fmt.Sprintf("ALTER TABLE %s.t OWNER TO %s",
schema, pgx.Identifier{owner}.Sanitize()))
require.NoError(t, err)
_, err = admin.Exec(t.Context(), fmt.Sprintf("GRANT USAGE ON SCHEMA %s TO %s",
schema, pgx.Identifier{engine}.Sanitize()))
require.NoError(t, err)

u, err := neturl.Parse(serverURL)
require.NoError(t, err)
u.User = neturl.UserPassword(engine, password)
cmd := newMigrateCmd(u.String(), fmt.Sprintf("ALTER TABLE %s.t ADD COLUMN age int", schema))
cmd.JSON = true

var out strings.Builder
err = cmd.run(t.Context(), &out)
require.ErrorIs(t, err, verdict.ErrRefused)
var v verdict.Verdict
require.NoError(t, json.Unmarshal([]byte(out.String()), &v))
assert.Equal(t, verdict.OutcomeRefused, v.Outcome)
assert.Equal(t, verdict.ReasonInsufficientPrivileges, v.Reason)
assert.Equal(t, schema+".t", v.Table)

// The detail carries the exact provisioning statement, and the fix
// below executes that same statement — the closed loop the verdict
// promises an operator. The expected grant is version-dependent: 16+
// membership grants carry the INHERIT option explicitly.
grant := fmt.Sprintf("GRANT %s TO %s",
pgx.Identifier{owner}.Sanitize(), pgx.Identifier{engine}.Sanitize())
var versionNum int
require.NoError(t, admin.QueryRow(t.Context(),
"SELECT current_setting('server_version_num')::int").Scan(&versionNum))
if versionNum >= 160000 {
grant += " WITH INHERIT TRUE"
}
assert.Contains(t, v.Detail, grant, "the refusal detail must name the exact remediation")

_, err = admin.Exec(t.Context(), grant)
require.NoError(t, err)

out.Reset()
require.NoError(t, cmd.run(t.Context(), &out))
require.NoError(t, json.Unmarshal([]byte(out.String()), &v))
assert.Equal(t, verdict.OutcomeExecuted, v.Outcome)
}

// The privilege requirement follows the routed steps: a concurrent index
// build needs CREATE on the schema (Tier 2 of the engine-role contract), so
// a role that clears the owner-membership rung but whose owner lacks schema
// CREATE is refused before the build starts — not killed mid-build by the
// server. Applying the refusal's own Grant statement verbatim unlocks the
// build — the closed loop the verdict promises an operator.
func TestMigrateChecksPrivilegesAtRoutedTier(t *testing.T) {
serverURL := testutil.StartPostgres(t)
admin, err := dbconn.NewPool(t.Context(), dbconn.Config{URL: serverURL})
require.NoError(t, err)
defer admin.Close()

owner := testutil.NewRole(t, admin, "NOLOGIN")
const password = "engine-test-password"
engine := testutil.NewRole(t, admin, "LOGIN PASSWORD '"+password+"'")
schema := testutil.NewSchema(t, admin)
_, err = admin.Exec(t.Context(), fmt.Sprintf("CREATE TABLE %s.t (id int PRIMARY KEY, c int)", schema))
require.NoError(t, err)
_, err = admin.Exec(t.Context(), fmt.Sprintf("ALTER TABLE %s.t OWNER TO %s",
schema, pgx.Identifier{owner}.Sanitize()))
require.NoError(t, err)
_, err = admin.Exec(t.Context(), fmt.Sprintf("GRANT USAGE ON SCHEMA %s TO %s",
schema, pgx.Identifier{engine}.Sanitize()))
require.NoError(t, err)
// Tier 1 is satisfied up front: the engine is a member of the owning
// role. Only the schema CREATE rung is missing.
_, err = admin.Exec(t.Context(), fmt.Sprintf("GRANT %s TO %s",
pgx.Identifier{owner}.Sanitize(), pgx.Identifier{engine}.Sanitize()))
require.NoError(t, err)

u, err := neturl.Parse(serverURL)
require.NoError(t, err)
u.User = neturl.UserPassword(engine, password)
cmd := newMigrateCmd(u.String(), fmt.Sprintf("CREATE INDEX CONCURRENTLY t_c_idx ON %s.t (c)", schema))
cmd.JSON = true

var out strings.Builder
err = cmd.run(t.Context(), &out)
require.ErrorIs(t, err, verdict.ErrRefused)
var v verdict.Verdict
require.NoError(t, json.Unmarshal([]byte(out.String()), &v))
assert.Equal(t, verdict.OutcomeRefused, v.Outcome)
assert.Equal(t, verdict.ReasonInsufficientPrivileges, v.Reason)
assert.Equal(t, schema+".t", v.Table)

grant := fmt.Sprintf("GRANT CREATE ON SCHEMA %s TO %s",
pgx.Identifier{schema}.Sanitize(), pgx.Identifier{owner}.Sanitize())
assert.Contains(t, v.Detail, grant, "the refusal detail must name the exact Tier-2 remediation")

_, err = admin.Exec(t.Context(), grant)
require.NoError(t, err)

out.Reset()
require.NoError(t, cmd.run(t.Context(), &out))
require.NoError(t, json.Unmarshal([]byte(out.String()), &v))
assert.Equal(t, verdict.OutcomeExecuted, v.Outcome)
}

// Acceptance (iv): non-ALTER TABLE statements are refused with the safe-idiom
// pointer and never executed. The gate needs no database at all.
func TestMigrateGateRefusesWithoutDatabase(t *testing.T) {
Expand Down
19 changes: 19 additions & 0 deletions internal/testutil/postgres.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,25 @@ func NewSchema(t *testing.T, pool *pgxpool.Pool) string {
return name
}

// NewRole creates a throwaway cluster-level role with the given options and
// registers its drop. Roles are cluster-scoped, so names are unique per
// process the same way throwaway schemas are.
func NewRole(t *testing.T, pool *pgxpool.Pool, options string) string {
t.Helper()
name := fmt.Sprintf("r_%d_%d", os.Getpid(), schemaSeq.Add(1))
_, err := pool.Exec(t.Context(), fmt.Sprintf("CREATE ROLE %s %s", pgx.Identifier{name}.Sanitize(), options))
require.NoError(t, err, "create throwaway role")
t.Cleanup(func() {
// t.Context is cancelled by cleanup time; strip the cancellation.
_, err := pool.Exec(context.WithoutCancel(t.Context()),
"DROP ROLE IF EXISTS "+pgx.Identifier{name}.Sanitize())
if err != nil {
t.Logf("drop throwaway role %s: %v", name, err)
}
})
return name
}

// NewPublicTable creates a uniquely named throwaway table in the public
// schema — for tests that exercise unqualified-statement resolution, where
// a dedicated schema would defeat the point — and returns its name. The
Expand Down
4 changes: 4 additions & 0 deletions pkg/executor/sequence.go
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,10 @@ type SequenceStepError struct {
// committed prefix, because "what already happened" is the first triage
// question a partial sequence raises.
func (e *SequenceStepError) Error() string {
if e.Step == 1 {
return fmt.Sprintf("sequence step 1 of %d (%s) failed; no earlier steps had committed: %v",
e.Total, e.Kind, e.Err)
}
return fmt.Sprintf("sequence step %d of %d (%s) failed; steps before it committed and their state remains: %v",
e.Step, e.Total, e.Kind, e.Err)
}
Expand Down
12 changes: 12 additions & 0 deletions pkg/executor/sequence_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,3 +77,15 @@ func TestRunSequenceRefusesEmptySequence(t *testing.T) {
executor.DefaultRetryPolicy())
require.ErrorIs(t, err, executor.ErrEmptySequence)
}

// The renderer's own unit test: a step-1 failure must not claim earlier
// steps committed, because none did.
func TestSequenceStepErrorNamesTheCommittedPrefix(t *testing.T) {
cause := errors.New("boom")
first := &executor.SequenceStepError{Step: 1, Total: 3, Kind: executor.StepBrief, Err: cause}
assert.Contains(t, first.Error(), "no earlier steps had committed")
assert.NotContains(t, first.Error(), "steps before it committed")

later := &executor.SequenceStepError{Step: 2, Total: 3, Kind: executor.StepBrief, Err: cause}
assert.Contains(t, later.Error(), "steps before it committed and their state remains")
}
Loading
Loading