diff --git a/AUTHZ.md b/AUTHZ.md index 23ed22965..4a0bef185 100644 --- a/AUTHZ.md +++ b/AUTHZ.md @@ -281,6 +281,15 @@ when the setting is missing. So: | `app.bypass = '1'` | All rows visible (workers + platform admin). | | Neither set | **Zero rows** — fail-closed. | +For the length of the scope-setting compatibility window (store migration 66) +the predicate reads +`COALESCE(NULLIF(current_setting('codefly.current_tenant_id', true), ''), current_setting('app.current_org_id', true))`: +a pod that binds either generation of the name is admitted to its own tenant, +and "neither set" is still zero rows, because an empty live binding does not +fall through to a value — both arms empty is a false predicate. +[module/services/store/migrations/README.md](./module/services/store/migrations/README.md) +carries the window and the release that closes it. + **What this catches:** - A SQL injection that bypassed `WHERE org_id = $1` parsing. - A bug that uses the wrong orgID variable (`getWebhook(id, attackerOrg)` reading another tenant's row). diff --git a/CLAIM_INVENTORY.md b/CLAIM_INVENTORY.md index 6f6fec9f2..ca1eedc38 100644 --- a/CLAIM_INVENTORY.md +++ b/CLAIM_INVENTORY.md @@ -48,7 +48,8 @@ must not be inferred from a row here. See | The daily audit archive is "sealed" | AUTHZ.md § The daily archive, `pkg/business/audit_archive.go` | the object is written create-if-absent and its SHA-256 digest is receipted in the audit log, so a replacement is **detectable**; the object is **not immutable** — that needs an object-lock/retention policy on the bucket, which belongs to the infrastructure that provisions it and does not exist yet. Read "sealed" as tamper-evident, never as WORM | **configured**, with the immutability half **planned** elsewhere | accounts | | Scope-node role grants and installation identity ship | PLATFORM_REFERENCE.md §1.2 | `98_layered_access`, `112_installations`, `pkg/business/installations.go` | implemented | accounts | | RLS table→migration adoption table | AUTHZ.md § RLS coverage | names `audit_export_configs`, dropped by store migration 102 | historical (labelled) | #541 | -| The tenant and user are bound and read under the platform Postgres library's own setting names, `codefly.current_tenant_id` / `codefly.current_user_id` | MODULE.md, AUTHZ.md, module/DATABASE_AUTHORITY.md, module/JOBS.md, module/services/accounts/AUTHZ.md | store migration `65_library_scope_settings`; `pkg/infra.TenantScopeSetting` / `UserScopeSetting`; `TestEveryScopedPolicyReadsTheLibrarySettings` fails on any policy or function still reading `app.current_*`; `rls-migration-gate.mjs` accepts only these two | implemented | #1293 | +| The tenant and user are bound and read under the platform Postgres library's own setting names, `codefly.current_tenant_id` / `codefly.current_user_id` | MODULE.md, AUTHZ.md, module/DATABASE_AUTHORITY.md, module/JOBS.md, module/services/accounts/AUTHZ.md | store migration `65_library_scope_settings`; `pkg/infra.TenantScopeSetting` / `UserScopeSetting`, which `WithScopeSettings` also hands the library's scoped pools; `TestEveryScopedPolicyReadsTheLibrarySettings` fails on any policy or function reading `app.current_*` outside the compatibility read; `rls-migration-gate.mjs` accepts only these two as the live name | implemented | #1293 | +| The rename is deployable: for one release every predicate reads the library's name with the host's retired one behind it, the host binds both generations, and a catalog that reads only the retired names is refused rather than served empty | module/services/store/migrations/README.md § The compatibility window, module/services/accounts/AGENTS.md, module/DATABASE_AUTHORITY.md, module/module.package.codefly.yaml breaking-changes | store migration `66_scope_settings_transition` = `module/tools/rls-scope-settings-transition.sql` (`TestMigration66IsTheShippedScopeSettingsRecipe`); `TestMigration66OpensTheScopeSettingCompatibilityWindow` admits a transaction binding either generation on a real cluster and refuses an unbound one; `requireCatalogReadsScopeSettings` with `TestScopeSettingRefusalReadsTheCatalogsVersion` and `TestCatalogScopeSettingCheckReadsTheLiveCatalog` | implemented, **window open** — the migration that closes it is `66_..._transition.down.sql`, uncut | #1293 | | ~~`app.current_org_id` / `app.current_user_id` as the host's RLS settings~~ | RLS_PLAN.md, APPROVALS_DESIGN.md, module/docs/adr/0003, docs/authorization/2-proposals/0001, docs/authorization/3-implementation/spikes/ltree-record-shares.md | renamed by store migration 65; these five keep the old names because each records a design or a decision as it was written (and each already names the long-gone `app.bypass`) | historical (not rewritten) | #1293 | ## Gateway hot path diff --git a/RLS_PLAN.md b/RLS_PLAN.md index 48bd0b146..7462173a7 100644 --- a/RLS_PLAN.md +++ b/RLS_PLAN.md @@ -7,6 +7,16 @@ in [module/DATABASE_AUTHORITY.md](./module/DATABASE_AUTHORITY.md). Relations named here have since been renamed or dropped (`audit_export_configs` went in store migration 102), and the foundation has changed shape — the application-settable bypass is gone, replaced by the `app_control_plane` role. +The two settings every policy below reads are also renamed: a tenant-scoped +policy reads `codefly.current_tenant_id` and a user-scoped one +`codefly.current_user_id` (store migration 65), with the +`app.current_org_id` / `app.current_user_id` spellings kept in this file as the +record of what was written. A consumer writing a policy today uses the two +library names and nothing else — +[module/services/store/migrations/README.md](./module/services/store/migrations/README.md) +is the current convention, including the compatibility window a rename runs +through and `module/tools/rls-scope-settings-transition.sql`, which rewrites +policies already installed on the retired names. > Defense-in-depth for tenant isolation at the database row level. > Layered on top of: diff --git a/module/DATABASE_AUTHORITY.md b/module/DATABASE_AUTHORITY.md index 3bfafa7b1..a96d44230 100644 --- a/module/DATABASE_AUTHORITY.md +++ b/module/DATABASE_AUTHORITY.md @@ -37,8 +37,10 @@ constructor. That primitive owns the separate reader/writer pools, startup pings failure cleanup and repeated-close safety, and it also owns the two transaction-local settings a scope is bound to: its defaults, `codefly.current_tenant_id` and `codefly.current_user_id`, are what the policies -read, so accounts passes no `WithScopeSettings` override and supplies only its -verified identity adapter. Its current request adapter issues Readers only; +read, and accounts passes them back through `WithScopeSettings` from the same +two constants its own hand-written SQL names, so the library's unexported +default cannot drift away from the policies without a refusal. It supplies its +verified identity adapter and nothing else. Its current request adapter issues Readers only; authentication alone does not grant a Writer. External-identity deployments with separate logins configure @@ -88,6 +90,20 @@ transaction-locally, the half it does not carry as the empty string, so a session-level binding is never visible inside one: an org transaction cannot read a previous user's `sessions` or `mfa_devices`. +For the length of the scope-setting compatibility window it binds four: the +host's retired `app.current_org_id` and `app.current_user_id` carry the same two +values, and store migration 66's predicates read the library's name with the +retired one behind it. That is what makes a catalog and an image deployable +independently — a rename applied in one step is zero rows on every scoped +SELECT for whichever side lands first, silently. The retired half is bound to +the same value as the live one precisely so clearing one half clears both. +Startup refuses a database whose policies read only the retired names +(`requireCatalogReadsScopeSettings`): the two pools `service-postgres` owns bind +the library's names alone, so no binding here could make such a catalog serve, +and a started process would report healthy and answer nothing through them. +`module/services/store/migrations/README.md` carries the window and how it +closes. + Every new request connection, the first one at startup included, is judged before the pool hands it out. It is refused if its login can reach any role other than `app_tenant` — through any chain of memberships, whatever their @@ -184,8 +200,9 @@ importers to `pkg/infra` and `storetx` and refuses an exported `pkg/infra` function that takes a `pgx.Tx` and returns a context — a binder handed on. Code holding the raw transaction could still rebind it in SQL, under a binding that claims the old scope; `TestNoTenantRebindingOutsideInfra` fails on a -string literal containing `set_config(`, `SET ROLE`, `RESET ROLE` or -`app.current_` in any non-test file outside `pkg/infra`. Both are checks on +string literal containing `set_config(`, `SET ROLE`, `RESET ROLE` or either +generation of the scope setting names (`app.current_`, `codefly.current_`) in +any non-test file outside `pkg/infra`. Both are checks on the source, not capabilities. The store pins published Postgres agent 0.0.140, which provisions its declared diff --git a/module/module.package.codefly.yaml b/module/module.package.codefly.yaml index f9a7ff511..1cd871287 100644 --- a/module/module.package.codefly.yaml +++ b/module/module.package.codefly.yaml @@ -100,10 +100,14 @@ migrations: - id: composition-v2 from: ">=0.0.32 <0.1.0" breaking: true + - id: scope-settings-transition + from: ">=0.1.0 <0.2.0" + breaking: true breaking-changes: - Consumers resolve the immutable package into a disposable projection instead of editing Starter-owned files. - "Audit event types are namespaced under `saas.*` (`auth.login` -> `saas.auth.login`). The wire value of `audit_events.event_type`, the `X-Webhook-Event` header, and webhook routing keys all change; migration 116 rewrites stored history and existing subscriptions in place, so a live subscriber keeps firing, but a consumer that re-creates a subscription from a hardcoded pre-namespace name, or emits one through `ModuleEmitAuditEvent`, is now rejected. No alias layer." - "The telemetry service and its dependency contract are removed. SDK Init exports traces when OTEL_EXPORTER_OTLP_ENDPOINT is delivered; no endpoint means no tracing, including native local runs. Exporter initialization failures are logged without stopping the service. Accounts and auth-gateway push metrics to the same endpoint and no longer serve /metrics. Telemetry uses standard process variables, not the removed observability configuration group. Deliver standard OTEL variables to every service through the environment telemetry declaration using a compatible CLI. Existing consumers of the telemetry service must migrate to the HTTPS operations intake." + - "The two transaction-local settings an RLS policy reads are now the platform Postgres library's own, `codefly.current_tenant_id` and `codefly.current_user_id`, not the host's `app.current_org_id` and `app.current_user_id` (store migration 65). A consumer whose own migrations created tenant policies against the retired names must rewrite them: nothing binds those names as the only read any more, so each such table reads as EMPTY for every caller and every INSERT fails its WITH CHECK, with no error raised anywhere. Run `tools/rls-scope-settings-transition.sql` against the database — it is catalog-driven, idempotent, and reaches a consumer's own policies and routine bodies as well as this module's — or rewrite the predicates onto the two library names. Store migration 66 is that same file applied to this module's ledger: for the length of the compatibility window every predicate reads the library's name with the retired one behind it, and the host binds both generations, so the catalog and the fleet can move independently. A later release closes the window by deleting the retired arm; a policy still reading a retired name alone at that point goes silently empty, which accounts refuses to start against and `tools/rls-migration-gate.mjs` refuses to build." reserved-namespaces: - "@codefly/saas-" - "@codefly-dev/saas-" diff --git a/module/services/accounts/AGENTS.md b/module/services/accounts/AGENTS.md index 8d139eaac..d4d465088 100644 --- a/module/services/accounts/AGENTS.md +++ b/module/services/accounts/AGENTS.md @@ -28,15 +28,38 @@ is read by the capability checks and does not create synthetic directory rows. **The two scope settings are named once.** A transaction's tenant and user are bound to `codefly.current_tenant_id` and `codefly.current_user_id` — the defaults of the platform's Postgres library, which installs them for any -transaction opened through the scoped factory. Nothing here passes -`WithScopeSettings`, and hand-written SQL names them from +transaction opened through the scoped factory. Both the scoped factory and the +hand-written SQL in `pkg/infra` take the two names from `pkg/infra.TenantScopeSetting` / `UserScopeSetting` rather than spelling the -literal again. Only `pkg/infra` may bind them at all -(`TestNoTenantRebindingOutsidePkgInfra`), and a policy or function body left on +literal again, the factory through `WithScopeSettings`: the library's default is +unexported, so passing it is what ties the pools to the same constant the +policies and the request binding read. Only `pkg/infra` may bind them at all +(`TestNoTenantRebindingOutsideInfra`), and a policy or function body left on some other setting does not fail loudly: `current_setting(…, true)` returns NULL, the predicate is false, and the relation reads as empty for every caller (`TestEveryScopedPolicyReadsTheLibrarySettings` is what catches that). +**A scope-setting rename is a two-release move, and this is the window.** A +one-step rename is undeployable: the catalog and the image ship separately, and +whichever loses the race reads an unset setting, which is zero rows on every +scoped SELECT and a WITH CHECK failure on every INSERT, with nothing raised. So +store migration 66 leaves every predicate reading the library's name with the +host's retired `app.current_org_id` / `app.current_user_id` behind it +(`COALESCE(NULLIF(, ''), )`), `bindRequestScopeSQL` binds all +four names, and one image is correct against a catalog from before the rename, +after it, or inside the window. The retired names are +`pkg/infra.LegacyTenantScopeSetting` / `LegacyUserScopeSetting`, and nothing but +that binding and the migration may name them. + +What the window does not cover is the two pools `service-postgres` owns: those +bind the library's two names and take no more, so against a catalog that reads +only the retired ones every read through them is silently empty and no binding +here can fix it. `requireCatalogReadsScopeSettings` refuses to start against +such a database rather than serving it — it asks the catalog what the policies +judge by, not the ledger what version it claims. The window closes in a later +release, whose migration is `66_scope_settings_transition.down.sql`: it deletes +the retired arm, and the two legacy constants and this section go with it. + ## Forwarded Work Contexts on the organization directory `DirectoryService` on the Connect endpoint accepts a person-owned Work Context diff --git a/module/services/accounts/code/pkg/infra/control_plane_call_sites_test.go b/module/services/accounts/code/pkg/infra/control_plane_call_sites_test.go index 6db3c0959..aa3327769 100644 --- a/module/services/accounts/code/pkg/infra/control_plane_call_sites_test.go +++ b/module/services/accounts/code/pkg/infra/control_plane_call_sites_test.go @@ -93,8 +93,9 @@ func read(ctx context.Context, store Store) { // TestNoTenantRebindingOutsideInfra fails on a string literal, in a non-test Go // file outside pkg/infra, that binds or reads a transaction's request scope or -// role: set_config(, SET [LOCAL|SESSION] ROLE, RESET ROLE or codefly.current_, -// matched without regard to case or spacing. pkg/business, pkg/auth/pg and +// role: set_config(, SET [LOCAL|SESSION] ROLE, RESET ROLE or either generation +// of the scope setting names (app.current_, codefly.current_), matched without +// regard to case or spacing. pkg/business, pkg/auth/pg and // pkg/billing/pg hold the raw transaction through storetx.Tx, and one such // statement on it would rebind the tenant — or leave app_tenant — under a // binding that still claims the org, user and role pkg/infra set, which is what @@ -150,6 +151,7 @@ func escalate(ctx context.Context, tx pgx.Tx) { _, _ = tx.Exec(ctx, "reset role") _, _ = tx.Exec(ctx, "SELECT pg_catalog.set_config ('role', 'app_control_plane', true)") _, _ = tx.Exec(ctx, "SELECT current_setting('codefly.current_user_id', true)") + _, _ = tx.Exec(ctx, "SELECT set_config('app.current_org_id', $1, true)") _, _ = tx.Exec(ctx, "SET LOCAL " + "ROLE " + role) _, _ = tx.Exec(ctx, "UPDATE organization_members SET a = 1; SET role = 'app_control_plane'") } @@ -169,13 +171,15 @@ func harmless(ctx context.Context, tx pgx.Tx) { } require.ElementsMatch(t, []string{ "(package level): set_config(", - "(package level): codefly.current_", + "(package level): app|codefly.current_", "escalate: SET ROLE", "escalate: SET ROLE", "escalate: SET ROLE", "escalate: RESET ROLE", "escalate: set_config(", - "escalate: codefly.current_", + "escalate: app|codefly.current_", + "escalate: set_config(", + "escalate: app|codefly.current_", "escalate: SET ROLE", "escalate: SET ROLE", }, got) @@ -190,7 +194,12 @@ var tenantRebinding = []struct { {"set_config(", regexp.MustCompile(`(?i)\bset_config\s*\(`)}, {"SET ROLE", regexp.MustCompile(`(?i)\bSET\s+(?:LOCAL\s+|SESSION\s+)?ROLE\b`)}, {"RESET ROLE", regexp.MustCompile(`(?i)\bRESET\s+ROLE\b`)}, - {"codefly.current_", regexp.MustCompile(`(?i)\bcodefly\s*\.\s*current_`)}, + // Both generations of the two names. The retired spelling has to be refused + // as well as the live one: it is readable again for the length of migration + // 66's compatibility window, so a statement outside pkg/infra that binds or + // reads it rebinds a transaction's scope exactly as the live name would — + // and a scan that matched only the live one would have reported nothing. + {"app|codefly.current_", regexp.MustCompile(`(?i)\b(?:app|codefly)\s*\.\s*current_`)}, } var updateStatement = regexp.MustCompile(`(?i)\bUPDATE\b`) diff --git a/module/services/accounts/code/pkg/infra/export_test.go b/module/services/accounts/code/pkg/infra/export_test.go index dca902d22..a566878ab 100644 --- a/module/services/accounts/code/pkg/infra/export_test.go +++ b/module/services/accounts/code/pkg/infra/export_test.go @@ -284,3 +284,28 @@ func ProjectedTokenForTest(lifetime time.Duration) string { encode := base64.RawURLEncoding.EncodeToString return encode([]byte(`{"alg":"RS256"}`)) + "." + encode(claims) + "." + encode([]byte("signature")) } + +// ScopeSettingPolicy is one policy as the startup catalog check reads it. +type ScopeSettingPolicy = scopeSettingPolicy + +// ScopeSettingRefusal is the decision requireCatalogReadsScopeSettings makes +// over what it read, so the suite can state a catalog — notably the one from +// before migration 65, every policy reading the retired names alone — without +// standing a second database to hold it. +func ScopeSettingRefusal(judged []ScopeSettingPolicy) error { return scopeSettingRefusal(judged) } + +// RequireCatalogReadsScopeSettings is the refusal startup applies, query and +// decision together, against whatever connection the caller hands it. +func RequireCatalogReadsScopeSettings(ctx context.Context, q interface { + Query(context.Context, string, ...any) (pgx.Rows, error) +}) error { + return requireCatalogReadsScopeSettings(ctx, q) +} + +// ReturnsToTenantRole is the request pool's AfterRelease judgement: it runs the +// reset a released connection runs and returns the verdict the pool acts on — +// true to put the connection back, false to destroy it. The suite drives it on +// a connection it poisoned itself, so "the connection is reused, cleaned" is a +// decision this returns rather than a backend reappearing in a shared pool +// within some number of seconds. +func ReturnsToTenantRole(conn *pgx.Conn) bool { return returnsToTenantRole(conn) } diff --git a/module/services/accounts/code/pkg/infra/postgres.go b/module/services/accounts/code/pkg/infra/postgres.go index b48250a75..887ba1e9c 100644 --- a/module/services/accounts/code/pkg/infra/postgres.go +++ b/module/services/accounts/code/pkg/infra/postgres.go @@ -174,6 +174,16 @@ func NewPostgresStoreWithCapabilities(ctx context.Context, readOnlyConnection, r requestPool.Close() return nil, err } + // And the database judges by the settings this process can bind. A catalog + // and a binary that disagree about the two names do not fail: every scoped + // read is empty and every scoped write fails its WITH CHECK, silently. The + // control-plane pool asks, because it is the one already open and the + // question is about pg_catalog, not about a tenant. + if err := requireCatalogReadsScopeSettings(ctx, controlPlanePool); err != nil { + controlPlanePool.Close() + requestPool.Close() + return nil, err + } database, closeDatabase, err := openScopedBoundary(ctx, readOnlyConnection, readWriteConnection, provider) if err != nil { @@ -302,9 +312,17 @@ const requestReleaseTimeout = 5 * time.Second // next borrower's unqualified table names, and RESET ALL does not touch it. // Unlike DISCARD ALL it may run inside the implicit transaction a multi-statement // message opens. The four statements travel in one simple-protocol message. +// +// It reads back all four names, not just the two the library binds: inside +// migration 66's compatibility window a policy falls through to the retired +// name when the live one is empty, so a session-level binding left on either +// generation hands the next borrower a scope. The reset clears both; this is +// what refuses a connection on which it did not. const sessionReset = `RESET ROLE; RESET ALL; DISCARD TEMP; SELECT current_user::text, - coalesce(current_setting('` + TenantScopeSetting + `', true), ''), - coalesce(current_setting('` + UserScopeSetting + `', true), '')` + coalesce(current_setting('` + TenantScopeSetting + `', true), '') + || coalesce(current_setting('` + LegacyTenantScopeSetting + `', true), ''), + coalesce(current_setting('` + UserScopeSetting + `', true), '') + || coalesce(current_setting('` + LegacyUserScopeSetting + `', true), '')` // resetSession runs sessionReset on a released connection. It returns the role // the session came back to, and false if the reset failed or left a request @@ -323,7 +341,7 @@ func resetSession(conn *pgx.Conn) (string, bool) { // returnsToTenantRole is the request pool's AfterRelease. A session-level change // a borrower leaves behind would reach the next borrower until the connection is // recycled: `SET ROLE NONE` hands it the bare login, and a session-level -// codefly.current_tenant_id or codefly.current_user_id hands it the previous +// binding of either generation of the two scope settings hands it the previous // caller's // scope, which every un-wrapped read runs under. The reset clears both; a // connection that does not come back as app_tenant with no binding is @@ -767,10 +785,17 @@ func openScopedBoundaryAs(ctx context.Context, readOnlyConnection, readWriteConn return nil, nil, errors.New("reader and writer require distinct private identity sockets") } options := []scopedpostgres.Option{ - // No WithScopeSettings: the library's own defaults are - // codefly.current_tenant_id and codefly.current_user_id, which is what - // the policies read (migration 65). An override here would be a second - // place to keep the two names in step. + // The two names these pools bind, named from the same two constants the + // hand-written SQL in this package names them from. They are also the + // library's own defaults, so passing them changes no behaviour today — + // what it changes is what happens when one side moves. The default is + // unexported, its doc comment in the published library still says + // `app.current_tenant_id`, and nothing links it to the constant the + // policies, the request binding and the startup catalog check all read. + // Passing it makes the host's two constants the single place the names + // live, and a library default that drifts a refusal here rather than a + // pool that silently binds a name no policy reads. + scopedpostgres.WithScopeSettings(TenantScopeSetting, UserScopeSetting), scopedpostgres.WithOperationTimeout(scopedBoundaryOperationTimeout), // service-postgres judges both scoped pools on every new physical // connection and again on every checkout: it refuses a session whose diff --git a/module/services/accounts/code/pkg/infra/postgres_effect_receipts.go b/module/services/accounts/code/pkg/infra/postgres_effect_receipts.go index 66c80f3cf..d179bdfe1 100644 --- a/module/services/accounts/code/pkg/infra/postgres_effect_receipts.go +++ b/module/services/accounts/code/pkg/infra/postgres_effect_receipts.go @@ -25,7 +25,10 @@ func (s *PostgresStore) NewEffectReceipts() (*receipts.PostgresStore, func(), er db := stdlib.OpenDBFromPool(s.pool) db.SetMaxOpenConns(int(s.pool.Config().MaxConns / 2)) store, err := receipts.NewPostgresStore(db, receipts.WithTenantScope(func(ctx context.Context, tx *sql.Tx, tenant string) error { - _, err := tx.ExecContext(ctx, `SELECT set_config('`+TenantScopeSetting+`', $1, true), set_config('`+UserScopeSetting+`', '', true)`, tenant) + // The same binding every request transaction uses, both generations of + // both names, so this path cannot be the one left behind when the + // compatibility window closes or the names move again. + _, err := tx.ExecContext(ctx, bindRequestScopeSQL, tenant, "") return err })) if err != nil { diff --git a/module/services/accounts/code/pkg/infra/postgres_scope_settings_test.go b/module/services/accounts/code/pkg/infra/postgres_scope_settings_test.go index 15725aeab..5c5e380a4 100644 --- a/module/services/accounts/code/pkg/infra/postgres_scope_settings_test.go +++ b/module/services/accounts/code/pkg/infra/postgres_scope_settings_test.go @@ -3,6 +3,7 @@ package infra_test import ( + "strings" "testing" codefly "github.com/codefly-dev/sdk-go" @@ -12,14 +13,22 @@ import ( "accounts/pkg/infra" ) -// The database must read the caller's scope under exactly the two settings the -// platform's Postgres library installs, and under no others. A policy or a -// SECURITY DEFINER body left on a setting nothing binds any more does not fail: -// current_setting(..., true) returns NULL, the predicate is false, and the -// relation reads as empty — so the break is a silent loss of access on one -// table, or, with the comparison the other way round, a silent loss of -// isolation. Nothing else in the suite would see it: a test that sets the old -// name and reads the old policy passes on both halves of the mistake. +// The database must read the caller's scope under the two settings the +// platform's Postgres library installs — and may read the host's two retired +// names only behind them, as the second arm of migration 66's compatibility +// read. A policy or a SECURITY DEFINER body left on a setting nothing binds +// does not fail: current_setting(..., true) returns NULL, the predicate is +// false, and the relation reads as empty — so the break is a silent loss of +// access on one table, or, with the comparison the other way round, a silent +// loss of isolation. Nothing else in the suite would see it: a test that sets +// the old name and reads the old policy passes on both halves of the mistake. +// +// The window is why "and under no others" is now "and only inside the +// compatibility read". A retired name read ALONE is still the silent break, and +// a retired name read beside the library's is the one deployable state during a +// rename — so the assertion is the shape, not the absence: every read of a +// retired setting must be the fallback arm of a read of its live counterpart, +// which is what counting the two forms per body says. // // This asks the catalog rather than the migration text, which is the only place // that knows the whole set — 75 policies over 70 relations, 6 more on partitions @@ -27,9 +36,15 @@ import ( // reads every object kind an expression can hide in, not just pg_policies, and // the counts below refuse a vacuous pass: a query that matched nothing would // otherwise read exactly like a clean database. +// +// When the window closes, the later migration deletes the compatibility arm and +// the two retired constants with it, and this test becomes the stricter one it +// was: no body reads a retired name at all. func TestEveryScopedPolicyReadsTheLibrarySettings(t *testing.T) { require.Equal(t, "codefly.current_tenant_id", infra.TenantScopeSetting) require.Equal(t, "codefly.current_user_id", infra.UserScopeSetting) + require.Equal(t, "app.current_org_id", infra.LegacyTenantScopeSetting) + require.Equal(t, "app.current_user_id", infra.LegacyUserScopeSetting) dsn, err := codefly.For(testCtx).Service("store").Secret("postgres", "owner-connection") require.NoError(t, err) @@ -77,10 +92,28 @@ WITH source AS ( SELECT 'rule ' || r.ev_class::regclass::text || '.' || r.rulename, pg_get_ruledef(r.oid) FROM pg_catalog.pg_rewrite r WHERE r.rulename <> '_RETURN' ) -SELECT what, body FROM source WHERE body ~ $1 ORDER BY what` +SELECT what, body FROM source` + + // One read of one setting, in both spellings the ledger and the deparser + // produce, and the compatibility read that migration 66 wraps it in. + read := func(setting string) string { + return `(?:pg_catalog\.)?current_setting\('` + strings.ReplaceAll(setting, ".", `\.`) + `'(?:::text)?, true\)` + } + window := func(live, retired string) string { + return `COALESCE\(NULLIF\(` + read(live) + `, ''::text\), ` + read(retired) + `\)` + } + // A read of a retired setting that is not the fallback arm of a read of its + // live counterpart: the body reads a name nothing will bind once the window + // closes, and reads as empty for every caller the moment it does. var stale []string - rows, err := conn.Query(testCtx, sources, `\mapp\.current_(org|user)_id\M`) + rows, err := conn.Query(testCtx, ` +SELECT what, body FROM (`+sources+`) c + WHERE regexp_count(body, $1) <> regexp_count(body, $2) + OR regexp_count(body, $3) <> regexp_count(body, $4) + ORDER BY what`, + read(infra.LegacyTenantScopeSetting), window(infra.TenantScopeSetting, infra.LegacyTenantScopeSetting), + read(infra.LegacyUserScopeSetting), window(infra.UserScopeSetting, infra.LegacyUserScopeSetting)) require.NoError(t, err) for rows.Next() { var what, body string @@ -89,13 +122,13 @@ SELECT what, body FROM source WHERE body ~ $1 ORDER BY what` } require.NoError(t, rows.Err()) require.Empty(t, stale, - "these objects still read the host's old scope settings, which nothing binds any more, so each one reads as empty") + "these objects read one of the host's retired scope settings outside migration 66's compatibility read, so each one reads as empty for every caller once the window closes") // And the rename did land, rather than the probe above finding nothing // because the database is empty or the query is wrong. count := func(pattern string) int { var n int - require.NoError(t, conn.QueryRow(testCtx, `SELECT count(*) FROM (`+sources+`) c`, pattern).Scan(&n)) + require.NoError(t, conn.QueryRow(testCtx, `SELECT count(*) FROM (`+sources+`) c WHERE body ~ $1`, pattern).Scan(&n)) return n } tenantReaders := count(`\m` + infra.TenantScopeSetting + `\M`) @@ -103,6 +136,20 @@ SELECT what, body FROM source WHERE body ~ $1 ORDER BY what` require.Greater(t, tenantReaders, 70, "the tenant setting is read by far more than this; a low count means the probe is wrong") require.Greater(t, userReaders, 25, "the user setting is read by far more than this; a low count means the probe is wrong") + // And the window is open on every one of those reads, rather than the stale + // probe above passing because nothing reads a retired name at all — the + // state this database reaches only after the window closes, and which no + // pre-rename image can serve. Counted over calls rather than mentions: the + // floors above count a body that names the setting in a comment too. + tenantReads := count(read(infra.TenantScopeSetting)) + userReads := count(read(infra.UserScopeSetting)) + require.Greater(t, tenantReads, 50, "the tenant setting is read by far more than this; a low count means the probe is wrong") + require.Greater(t, userReads, 25, "the user setting is read by far more than this; a low count means the probe is wrong") + require.Equal(t, tenantReads, count(window(infra.TenantScopeSetting, infra.LegacyTenantScopeSetting)), + "every read of the tenant setting must be migration 66's compatibility read, so a pod that binds either generation is admitted") + require.Equal(t, userReads, count(window(infra.UserScopeSetting, infra.LegacyUserScopeSetting)), + "every read of the user setting must be migration 66's compatibility read") + // Each partition of audit_events carries its own copy of the parent's // policies, and a query that names the partition is judged by those alone. var outOfStep []string diff --git a/module/services/accounts/code/pkg/infra/postgres_users_rls_test.go b/module/services/accounts/code/pkg/infra/postgres_users_rls_test.go index 227c89e8a..324a4ba43 100644 --- a/module/services/accounts/code/pkg/infra/postgres_users_rls_test.go +++ b/module/services/accounts/code/pkg/infra/postgres_users_rls_test.go @@ -7,6 +7,7 @@ import ( "testing" "accounts/pkg/business" + "accounts/pkg/infra" "accounts/pkg/infra/storetx" "github.com/stretchr/testify/require" @@ -103,8 +104,16 @@ func TestOrganizationMemberEmailFunctionHasPinnedAuthority(t *testing.T) { require.True(t, securityDefiner) require.True(t, tenantCanExecute) require.False(t, publicCanExecute) + // Pinned as the catalog renders it, second arm and all: inside migration + // 66's compatibility window users_select reads the library's user + // setting and falls through to the host's retired one, so a pod built + // on either side of the rename sees its own user's row and no other. + // When the window closes this returns to the single read it pins + // above the fallback — which is the point of pinning the text rather + // than asserting that it mentions a setting. require.Equal(t, - "((uuid)::text = current_setting('codefly.current_user_id'::text, true))", + "((uuid)::text = COALESCE(NULLIF(current_setting('"+infra.UserScopeSetting+ + "'::text, true), ''::text), current_setting('"+infra.LegacyUserScopeSetting+"'::text, true)))", policyExpression, ) return nil diff --git a/module/services/accounts/code/pkg/infra/scope_settings_catalog.go b/module/services/accounts/code/pkg/infra/scope_settings_catalog.go new file mode 100644 index 000000000..ea6f6716a --- /dev/null +++ b/module/services/accounts/code/pkg/infra/scope_settings_catalog.go @@ -0,0 +1,147 @@ +package infra + +import ( + "context" + "fmt" + "strings" + + "github.com/jackc/pgx/v5" +) + +// scopeSettingReader is the one capability the catalog check needs. A pool and a +// transaction both satisfy it, so the check runs at startup on the control-plane +// pool and, in the suite, inside a transaction that stages a catalog to refuse. +type scopeSettingReader interface { + Query(context.Context, string, ...any) (pgx.Rows, error) +} + +// scopeSettingPolicyQuery is every policy in the schema, with whether each half +// of its judgement reads the library's setting names and whether either reads +// the host's retired ones. A setting name is matched as a whole word with its +// dot escaped — unescaped it is a wildcard, and `app.current_org_id` would then +// also match a name that merely resembles it. +const scopeSettingPolicyQuery = ` +WITH policy AS ( + SELECT p.polrelid::regclass::text || '.' || p.polname AS what, + coalesce(pg_get_expr(p.polqual, p.polrelid), '') || ' ' || + coalesce(pg_get_expr(p.polwithcheck, p.polrelid), '') AS body + FROM pg_catalog.pg_policy p + JOIN pg_catalog.pg_class c ON c.oid = p.polrelid + JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = 'public' +), setting AS ( + SELECT '\m' || replace($1, '.', '\.') || '\M' AS tenant, + '\m' || replace($2, '.', '\.') || '\M' AS "user", + '\m' || replace($3, '.', '\.') || '\M' AS retired_tenant, + '\m' || replace($4, '.', '\.') || '\M' AS retired_user +) +SELECT what, + body ~ s.tenant OR body ~ s."user" AS reads_library, + body ~ s.retired_tenant OR body ~ s.retired_user AS reads_retired + FROM policy, setting s + ORDER BY what` + +// requireCatalogReadsScopeSettings refuses to serve against a database whose +// policies do not read the two settings this process can bind. +// +// The predicate a scoped policy carries is +// `col::text = current_setting(, true)`, and `current_setting(…, true)` +// returns NULL for a setting nothing bound: the predicate is false, the +// relation reads as empty for every caller, and no error is raised anywhere. +// So a binary and a catalog that disagree about the name do not fail — the +// product goes blank. Rolling back the accounts image with migration 65 applied +// is that state for as long as the rollback lasts, and the reverse (an image +// that binds only the library's names against a catalog from before the rename) +// is it too. +// +// bindRequestScopeSQL binds both generations, so this process survives either +// catalog on its own request pool. What it cannot do is widen the two scoped +// pools service-postgres owns: those bind the library's names and nothing else +// — the library takes two names, not four — so against a catalog that reads +// only the retired ones every read through them is silently empty. That is the +// reason this is a refusal to start rather than a warning: there is no binding +// the host can add to make such a catalog serve, and a started process would +// report healthy while answering nothing. +// +// It judges policies rather than the ledger's version row: the question is what +// the database will actually evaluate, and a version row is a claim about how +// it got there. A policy inside the compatibility window reads both names and +// is admitted by the first half of the rule. +// +// It runs where the served process is assembled, NewPostgresStoreWithCapabilities, +// and not in the single-credential tooling store: that one opens no scoped +// pool, so the reason this is fatal rather than advisory does not apply to it, +// and a migration tool that refused to open a database it was about to move +// forward would be refusing its own job. +func requireCatalogReadsScopeSettings(ctx context.Context, q scopeSettingReader) error { + rows, err := q.Query(ctx, scopeSettingPolicyQuery, + TenantScopeSetting, UserScopeSetting, LegacyTenantScopeSetting, LegacyUserScopeSetting) + if err != nil { + return fmt.Errorf("read the scope settings this database's policies judge by: %w", err) + } + defer rows.Close() + var judged []scopeSettingPolicy + for rows.Next() { + var policy scopeSettingPolicy + if err := rows.Scan(&policy.What, &policy.ReadsLibrary, &policy.ReadsRetired); err != nil { + return fmt.Errorf("read the scope settings this database's policies judge by: %w", err) + } + judged = append(judged, policy) + } + if err := rows.Err(); err != nil { + return fmt.Errorf("read the scope settings this database's policies judge by: %w", err) + } + return scopeSettingRefusal(judged) +} + +// scopeSettingPolicy is one policy as the query above reports it: which +// generation of the two setting names its judgement reads. +type scopeSettingPolicy struct { + What string + ReadsLibrary bool + ReadsRetired bool +} + +// scopeSettingRefusal is the decision requireCatalogReadsScopeSettings makes, +// separated from the query that feeds it: a catalog this process cannot serve +// is a property of a list of policies, and stating it this way is what lets the +// pre-rename catalog — every policy reading the retired names alone — be a test +// case rather than a second database. +func scopeSettingRefusal(judged []scopeSettingPolicy) error { + var stale []string + var library int + for _, policy := range judged { + switch { + case policy.ReadsLibrary: + library++ + case policy.ReadsRetired: + stale = append(stale, policy.What) + } + } + if len(stale) > 0 { + return fmt.Errorf("this database judges %d row-security polic%s by the retired scope settings alone (%s), which the scoped pools cannot bind: every read through them would be silently empty. Apply store migration 66_scope_settings_transition, or tools/rls-scope-settings-transition.sql for a policy this module does not own", + len(stale), policyPlural(len(stale)), strings.Join(sampleScopeSettingPolicies(stale, 5), ", ")) + } + if library == 0 { + return fmt.Errorf("no row-security policy in this database reads %s or %s, so nothing isolates a tenant from the request pools: this is not a database the store's ledger installed", + TenantScopeSetting, UserScopeSetting) + } + return nil +} + +// sampleScopeSettingPolicies is the first `most` of list, with a count of what +// it leaves out, so a startup refusal names the offenders without printing +// seventy of them. +func sampleScopeSettingPolicies(list []string, most int) []string { + if len(list) <= most { + return list + } + return append(list[:most:most], fmt.Sprintf("and %d more", len(list)-most)) +} + +func policyPlural(n int) string { + if n == 1 { + return "y" + } + return "ies" +} diff --git a/module/services/accounts/code/pkg/infra/scope_settings_catalog_db_test.go b/module/services/accounts/code/pkg/infra/scope_settings_catalog_db_test.go new file mode 100644 index 000000000..322108f3a --- /dev/null +++ b/module/services/accounts/code/pkg/infra/scope_settings_catalog_db_test.go @@ -0,0 +1,65 @@ +//go:build !pure + +package infra_test + +import ( + "testing" + + codefly "github.com/codefly-dev/sdk-go" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/stretchr/testify/require" + + "accounts/pkg/infra" +) + +// Startup refuses a database whose policies judge by a scope setting this +// process cannot bind. The decision is covered by its own table of catalogs; +// what this adds is the half that table cannot state — that the query behind it +// reads a real catalog correctly, both when the answer is yes and when it is no. +// +// The no case is staged rather than installed. A policy reading the retired +// names alone IS version 64's shape, one relation's worth of it, and the rule +// is per policy: the pre-rename catalog is seventy-five of these. Staging it in +// a transaction that rolls back proves the query sees it, over the same catalog +// the live one is in, without a second cluster to install 64 into — and without +// leaving anything behind for the next test in the package. +func TestCatalogScopeSettingCheckReadsTheLiveCatalog(t *testing.T) { + dsn, err := codefly.For(testCtx).Service("store").Secret("postgres", "owner-connection") + require.NoError(t, err) + pool, err := pgxpool.New(testCtx, dsn) + require.NoError(t, err) + defer pool.Close() + + // This database is at the ledger's frontier, inside the compatibility + // window, and must serve. + require.NoError(t, infra.RequireCatalogReadsScopeSettings(testCtx, pool)) + + tx, err := pool.Begin(testCtx) + require.NoError(t, err) + defer func() { _ = tx.Rollback(testCtx) }() + + _, err = tx.Exec(testCtx, ` +CREATE TABLE public.scope_setting_version_probe (id uuid PRIMARY KEY, org_id uuid NOT NULL); +ALTER TABLE public.scope_setting_version_probe ENABLE ROW LEVEL SECURITY; +ALTER TABLE public.scope_setting_version_probe FORCE ROW LEVEL SECURITY; +CREATE POLICY scope_setting_version_probe_tenant ON public.scope_setting_version_probe + FOR ALL TO PUBLIC + USING ((org_id)::text = current_setting('`+infra.LegacyTenantScopeSetting+`', true)) + WITH CHECK ((org_id)::text = current_setting('`+infra.LegacyTenantScopeSetting+`', true));`) + require.NoError(t, err) + + err = infra.RequireCatalogReadsScopeSettings(testCtx, tx) + require.Error(t, err, "a policy judging by the retired tenant setting alone must refuse startup, not read as an empty table") + require.Contains(t, err.Error(), "scope_setting_version_probe_tenant") + require.Contains(t, err.Error(), "66_scope_settings_transition") + + // And the compatibility window over the same policy is admitted, so what + // the check refuses is the retired name read ALONE rather than the retired + // name at all — which is the state every policy in this database is in. + _, err = tx.Exec(testCtx, ` +ALTER POLICY scope_setting_version_probe_tenant ON public.scope_setting_version_probe + USING ((org_id)::text = COALESCE(NULLIF(current_setting('`+infra.TenantScopeSetting+`', true), ''), current_setting('`+infra.LegacyTenantScopeSetting+`', true))) + WITH CHECK ((org_id)::text = COALESCE(NULLIF(current_setting('`+infra.TenantScopeSetting+`', true), ''), current_setting('`+infra.LegacyTenantScopeSetting+`', true)));`) + require.NoError(t, err) + require.NoError(t, infra.RequireCatalogReadsScopeSettings(testCtx, tx)) +} diff --git a/module/services/accounts/code/pkg/infra/scope_settings_catalog_decision_test.go b/module/services/accounts/code/pkg/infra/scope_settings_catalog_decision_test.go new file mode 100644 index 000000000..5c3bbb3c1 --- /dev/null +++ b/module/services/accounts/code/pkg/infra/scope_settings_catalog_decision_test.go @@ -0,0 +1,100 @@ +package infra_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "accounts/pkg/infra" +) + +// The catalog a database presents, as the startup check reads it, at each +// version of the ledger that can be under a running pod — and what the check +// must do with each. +// +// The one that matters is version 64: every policy reads the host's retired +// names and none reads the library's. An image built after migration 65 binds +// both generations, so its own request pool would serve that catalog correctly; +// the two pools service-postgres owns would not, because the library binds its +// two names and takes no more. A process that started there would report +// healthy and answer nothing through them, which is the failure mode this +// refusal exists to convert into a refusal to start. +func TestScopeSettingRefusalReadsTheCatalogsVersion(t *testing.T) { + policy := func(what string, library, retired bool) infra.ScopeSettingPolicy { + return infra.ScopeSettingPolicy{What: what, ReadsLibrary: library, ReadsRetired: retired} + } + for _, c := range []struct { + name string + judged []infra.ScopeSettingPolicy + refused string + }{{ + name: "version 64: the retired names alone, which the scoped pools cannot bind", + judged: []infra.ScopeSettingPolicy{ + policy("public.organizations.organizations_self", false, true), + policy("public.api_keys.api_keys_tenant", false, true), + }, + refused: "retired scope settings alone", + }, { + name: "version 65: the library's names alone", + judged: []infra.ScopeSettingPolicy{policy("public.organizations.organizations_self", true, false)}, + }, { + name: "version 66: both, which is the compatibility window", + judged: []infra.ScopeSettingPolicy{policy("public.organizations.organizations_self", true, true)}, + }, { + name: "a half-applied rewrite: one policy left behind is still a silent empty relation", + judged: []infra.ScopeSettingPolicy{ + policy("public.organizations.organizations_self", true, true), + policy("public.api_keys.api_keys_tenant", false, true), + }, + refused: "public.api_keys.api_keys_tenant", + }, { + name: "a database with no scoped policy at all is not one this ledger installed", + judged: nil, + refused: "no row-security policy in this database reads", + }, { + name: "a policy that reads neither setting is some other boundary's, and not this check's business", + judged: []infra.ScopeSettingPolicy{ + policy("public.organizations.organizations_self", true, false), + policy("public.audit_events.audit_events_worker_write", false, false), + }, + }} { + t.Run(c.name, func(t *testing.T) { + err := infra.ScopeSettingRefusal(c.judged) + if c.refused == "" { + require.NoError(t, err) + return + } + require.Error(t, err) + require.Contains(t, err.Error(), c.refused) + }) + } +} + +// The refusal has to tell an operator what to do about it, which for a policy +// this module owns is one migration and for a consumer's own policy is the +// recipe that migration embeds. A refusal that only reports the fact leaves +// them reading the catalog. +func TestScopeSettingRefusalNamesTheRemedy(t *testing.T) { + err := infra.ScopeSettingRefusal([]infra.ScopeSettingPolicy{ + {What: "public.organizations.organizations_self", ReadsRetired: true}, + }) + require.Error(t, err) + require.Contains(t, err.Error(), "66_scope_settings_transition") + require.Contains(t, err.Error(), "tools/rls-scope-settings-transition.sql") +} + +// Seventy refused policies is not a startup log line anyone reads, and a +// refusal that prints all of them buries the one fact that matters. Five and a +// count. +func TestScopeSettingRefusalSamplesALongList(t *testing.T) { + var judged []infra.ScopeSettingPolicy + for _, what := range []string{"a", "b", "c", "d", "e", "f", "g"} { + judged = append(judged, infra.ScopeSettingPolicy{What: "public.t." + what, ReadsRetired: true}) + } + err := infra.ScopeSettingRefusal(judged) + require.Error(t, err) + require.Contains(t, err.Error(), "judges 7 row-security policies") + require.Contains(t, err.Error(), "public.t.e") + require.NotContains(t, err.Error(), "public.t.f") + require.Contains(t, err.Error(), "and 2 more") +} diff --git a/module/services/accounts/code/pkg/infra/tenant_tx.go b/module/services/accounts/code/pkg/infra/tenant_tx.go index b749da8a1..5cfd08888 100644 --- a/module/services/accounts/code/pkg/infra/tenant_tx.go +++ b/module/services/accounts/code/pkg/infra/tenant_tx.go @@ -28,21 +28,58 @@ const ( // and the SQL this package writes by hand names them from here. Every policy, // and every SECURITY DEFINER operation that judges a caller's scope, reads // exactly these two; migration 65 renamed the host's own app.current_org_id and -// app.current_user_id onto them. +// app.current_user_id onto them, and migration 66 left those two readable +// behind them for the length of the compatibility window below. These are the +// names the library's own scoped pools bind, and the only ones they can bind, +// which is why a catalog that does not read them is refused at startup +// (requireCatalogReadsScopeSettings) rather than served empty. const ( TenantScopeSetting = "codefly.current_tenant_id" UserScopeSetting = "codefly.current_user_id" ) +// The two names the host carried before migration 65, which migration 66's +// policies read behind the library's two for the length of the compatibility +// window. +// +// A rename of an RLS setting cannot be deployed in one step. The policy and the +// binary that binds it ship separately, and the predicate is +// `col::text = current_setting(, true)`: the half that loses the race +// reads an unset setting, which is zero rows on every scoped SELECT and a WITH +// CHECK failure on every INSERT, with no error anywhere. A rolling update puts +// pre-rename pods against a renamed catalog; rolling the image back with the +// migration applied leaves the fleet there until someone notices that the +// product is empty. +// +// So this process binds all four names, and migration 66's predicates read the +// library's name first and the retired one behind it. That makes one image +// correct against a catalog from before the rename, one from after it, and one +// in the window — which is what lets the catalog and the fleet move +// independently. Both halves go when the window closes; nothing but +// bindRequestScopeSQL and migration 66 may name them. +const ( + LegacyTenantScopeSetting = "app.current_org_id" + LegacyUserScopeSetting = "app.current_user_id" +) + // bindRequestScopeSQL binds both halves of a request scope for the current -// transaction only. Every request transaction binds both, the empty string for -// the half it does not carry, so a session-level binding left on the connection -// is never visible inside it: an org transaction cannot read a previous user's -// sessions or mfa_devices, nor a user transaction a previous tenant's rows. The -// policies compare the settings as text and the SECURITY DEFINER operations +// transaction only, under both generations of the two setting names. Every +// request transaction binds all four, the empty string for the half it does not +// carry, so a session-level binding left on the connection is never visible +// inside it: an org transaction cannot read a previous user's sessions or +// mfa_devices, nor a user transaction a previous tenant's rows. Binding the +// retired names to the same values is what keeps that true inside the window — +// a transaction that cleared only the library's half would otherwise fall +// through to whatever the retired one still held. +// +// The policies compare the settings as text and the SECURITY DEFINER operations // map an empty setting to NULL through NULLIF, so an empty binding matches -// nothing, as an unset one does. -const bindRequestScopeSQL = "SELECT set_config('" + TenantScopeSetting + "', $1, true), set_config('" + UserScopeSetting + "', $2, true)" +// nothing, as an unset one does. Under migration 66 the comparison reads the +// library's name through NULLIF and falls through to the retired one, which +// preserves that exactly: both arms empty is a false predicate, not an absent +// one. +const bindRequestScopeSQL = "SELECT set_config('" + TenantScopeSetting + "', $1, true), set_config('" + UserScopeSetting + "', $2, true), " + + "set_config('" + LegacyTenantScopeSetting + "', $1, true), set_config('" + LegacyUserScopeSetting + "', $2, true)" // bindRequestScope binds orgID and userID, either possibly empty, for tx. func bindRequestScope(ctx context.Context, tx pgx.Tx, orgID, userID string) error { diff --git a/module/services/accounts/code/pkg/infra/tenant_tx_pooling_test.go b/module/services/accounts/code/pkg/infra/tenant_tx_pooling_test.go index eda1df0b7..263831955 100644 --- a/module/services/accounts/code/pkg/infra/tenant_tx_pooling_test.go +++ b/module/services/accounts/code/pkg/infra/tenant_tx_pooling_test.go @@ -6,7 +6,6 @@ import ( "context" "strings" "testing" - "time" "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgxpool" @@ -194,10 +193,22 @@ func TestRequestTransactionsClearTheBindingTheyDoNotCarry(t *testing.T) { } // A request connection comes back to the pool carrying no request binding, -// whatever its borrower left at session level. RESET ROLE alone would return -// the role and keep codefly.current_tenant_id and codefly.current_user_id, so the next -// borrower's un-wrapped reads — and any transaction that binds only half the -// scope — would run as the previous caller. +// whatever its borrower left at session level, and comes back rather than being +// thrown away. RESET ROLE alone would return the role and keep the scope +// settings, so the next borrower's un-wrapped reads — and any transaction that +// binds only half the scope — would run as the previous caller. +// +// Both halves are asserted on the connection the reset actually ran on, by +// driving the pool's own AfterRelease judgement. The earlier shape released the +// connection to the shared pool and then waited up to five seconds for that +// backend's PID to reappear among the idle ones, which is two conditions at +// once: it needed the reset to have cleaned the session AND nothing else in the +// package to be holding every request connection for the length of the wait. +// The second is not a property of this code — a background dispatcher is enough +// to fail it — and a failure could not say which half it was. The judgement +// returns the verdict the pool acts on: true reuses the connection, false +// destroys it. Asking it directly is the same property with the timing removed, +// and names the failure when it breaks. func TestRequestConnectionCarriesNoBindingAfterRelease(t *testing.T) { const ( staleOrg = "12121212-1212-1212-1212-121212121212" @@ -205,25 +216,44 @@ func TestRequestConnectionCarriesNoBindingAfterRelease(t *testing.T) { ) conn, err := testPool.Acquire(testCtx) require.NoError(t, err) - pid := conn.Conn().PgConn().PID() + defer conn.Release() + + // What a borrower that used set_config(..., false) or SET, rather than a + // transaction-local binding, hands back — under both generations of both + // names, because inside migration 66's compatibility window a predicate + // falls through to the retired name when the live one is empty, so a + // session-level binding on either is the previous caller's scope. _, err = conn.Exec(testCtx, - "SELECT set_config('codefly.current_tenant_id', $1, false), set_config('codefly.current_user_id', $2, false)", staleOrg, staleUser) + `SELECT set_config($1, $3, false), set_config($2, $4, false), + set_config($5, $3, false), set_config($6, $4, false)`, + infra.TenantScopeSetting, infra.UserScopeSetting, staleOrg, staleUser, + infra.LegacyTenantScopeSetting, infra.LegacyUserScopeSetting) require.NoError(t, err) - conn.Release() - - require.Eventually(t, func() bool { - idle := testPool.AcquireAllIdle(testCtx) - defer func() { - for _, c := range idle { - c.Release() - } - }() - found := false - for _, c := range idle { - require.Empty(t, currentOrgID(t, c), "an idle request connection carries an org binding") - require.Empty(t, currentUserID(t, c), "an idle request connection carries a user binding") - found = found || c.Conn().PgConn().PID() == pid - } - return found - }, 5*time.Second, 50*time.Millisecond, "the released connection returns to the pool without its bindings") + require.Equal(t, staleOrg, currentOrgID(t, conn), "the session binding really did survive its statement") + require.Equal(t, staleUser, currentUserID(t, conn)) + + require.True(t, infra.ReturnsToTenantRole(conn.Conn()), + "the release judgement refused the connection it had just reset, so every request connection would be destroyed on release instead of reused") + + // And this is the session the next borrower would be handed. + for _, setting := range []string{ + infra.TenantScopeSetting, infra.UserScopeSetting, + infra.LegacyTenantScopeSetting, infra.LegacyUserScopeSetting, + } { + require.Empty(t, sessionSetting(t, conn, setting), + "a request connection returned to the pool still carries %s", setting) + } + var role string + require.NoError(t, conn.QueryRow(testCtx, "SELECT current_user::text").Scan(&role)) + require.Equal(t, "app_tenant", role, "a released request connection must come back as app_tenant") +} + +// sessionSetting is one scope setting's value on q, the empty string when +// nothing has bound it. +func sessionSetting(t *testing.T, q orgIDQuerier, setting string) string { + t.Helper() + var got string + require.NoError(t, q.QueryRow(testCtx, + "SELECT coalesce(current_setting($1, true), '')", setting).Scan(&got)) + return got } diff --git a/module/services/store/code/migration_66_scope_settings_transition_test.go b/module/services/store/code/migration_66_scope_settings_transition_test.go new file mode 100644 index 000000000..ca6503631 --- /dev/null +++ b/module/services/store/code/migration_66_scope_settings_transition_test.go @@ -0,0 +1,255 @@ +package main + +import ( + "database/sql" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + + "github.com/golang-migrate/migrate/v4" +) + +// A read of one scope setting, in every spelling the ledger and the deparser +// produce: optionally schema-qualified, with or without the ::text cast. Group 1 +// is the qualification and group 2 the cast, which the rewrite keeps. +func scopeReadPattern(setting string) *regexp.Regexp { + return regexp.MustCompile(`((?:pg_catalog\.)?)current_setting\('` + + regexp.QuoteMeta(setting) + `'(::text)?, true\)`) +} + +// compatibilityWindow is what migration 66 must produce, re-derived here rather +// than read from the migration: every read of a library setting becomes a read +// of it with the host's retired name behind it. Written as a substitution over +// the whole catalog fingerprint, so it carries policy predicates and routine +// bodies alike. +func compatibilityWindow(catalog string) string { + for _, pair := range [][2]string{ + {libraryTenantSetting, hostTenantSetting}, + {libraryUserSetting, hostUserSetting}, + } { + catalog = scopeReadPattern(pair[0]).ReplaceAllString(catalog, + `COALESCE(NULLIF(${1}current_setting('`+pair[0]+`'${2}, true), ''::text), `+ + `${1}current_setting('`+pair[1]+`'${2}, true))`) + } + return catalog +} + +// Migration 65 renamed the two scope settings in one step, and a one-step +// rename of an RLS setting is not deployable. The policy is +// `col::text = current_setting(, true)`, so a binary that binds the other +// name reads NULL: every scoped SELECT returns zero rows with no error and +// every INSERT fails its WITH CHECK. The store's bootstrap job applies the +// migration before the new accounts ReplicaSet is ready, so a rolling update +// puts old pods against the renamed catalog; rolling the image back with the +// migration applied leaves the whole fleet there indefinitely. Nothing raises. +// +// Migration 66 is the transition that was missing: every predicate reads both +// names, the library's first. This stands a database at 65, applies 66, and +// requires three things of it — that the catalog changed in exactly that one +// way and nothing else, that a transaction binding EITHER name is admitted to +// its own tenant and no other, and that binding neither is still empty. +// +// The admission half is what a reading of the SQL cannot give. A predicate that +// reads both names and admits the wrong tenant, or every tenant, is a correct +// looking COALESCE away. +func TestMigration66OpensTheScopeSettingCompatibilityWindow(t *testing.T) { + db, databaseURL := throwawayPostgres(t) + if err := migrateStoreFrom("file://"+ledgerUpTo(t, 65), databaseURL); err != nil { + t.Fatal(err) + } + + before := rowSecurityCatalog(t, db) + if !strings.Contains(before, libraryTenantSetting) || !strings.Contains(before, libraryUserSetting) { + t.Fatal("version 65 is expected to read the library's two settings; the fingerprint query found neither, so this proves nothing") + } + if strings.Contains(before, hostTenantSetting) || strings.Contains(before, hostUserSetting) { + t.Fatal("version 65 is expected to read the library's settings alone; something already reads a retired name") + } + + // Two organisations, staged while the superuser still bypasses RLS. + orgA, _, _, _ := stageInstallationParents(t, db, "jane@example.com", "acme") + orgB, _, _, _ := stageInstallationParents(t, db, "john@example.com", "example-two") + + // The skew, at version 65: a pod that binds the retired name — any image + // built before the rename — is admitted to nothing at all. This is the + // failure 66 exists to remove, and asserting it here is what keeps the + // assertion after the migration from passing for some other reason. + if seen := organizationsVisibleWith(t, db, hostTenantSetting, orgA); seen != "" { + t.Fatalf("at version 65 the retired %s admitted %q; the skew this migration addresses does not exist as described", hostTenantSetting, seen) + } + + ledger := ledgerUpTo(t, 66) + if err := migrateStoreFrom("file://"+ledger, databaseURL); err != nil { + t.Fatal(err) + } + + after := rowSecurityCatalog(t, db) + if want := compatibilityWindow(before); after != want { + t.Fatalf("migration 66 is not exactly the compatibility window over version 65's catalog:\n%s", firstDifference(want, after)) + } + // The substitution above would also be satisfied by a catalog that reads + // neither name, so require the window to be there in its own right. + if !strings.Contains(after, hostTenantSetting) || !strings.Contains(after, libraryTenantSetting) { + t.Fatal("the rewritten catalog does not read both tenant settings") + } + + // A pod on either side of the rename reads its own tenant's rows, and only + // those. A count would prove nothing about WHICH row was admitted. + if seen := organizationsVisibleWith(t, db, libraryTenantSetting, orgA); seen != orgA { + t.Fatalf("a transaction binding %s alone saw %q, want only %q", libraryTenantSetting, seen, orgA) + } + if seen := organizationsVisibleWith(t, db, hostTenantSetting, orgB); seen != orgB { + t.Fatalf("a transaction binding the retired %s alone saw %q, want only %q", hostTenantSetting, seen, orgB) + } + // The host binds both halves of both names. The library's name wins, so a + // stale value under the retired one cannot widen or redirect a request. + if seen := organizationsVisibleWith(t, db, + libraryTenantSetting, orgA, hostTenantSetting, orgB); seen != orgA { + t.Fatalf("with %s=%s and %s=%s the transaction saw %q, want only %q — the library's name must win", + libraryTenantSetting, "A", hostTenantSetting, "B", seen, orgA) + } + // Fail-closed, in both the ways a request transaction reaches it: nothing + // bound at all, and both halves bound empty, which is what WithOrgTx does + // to the user half and WithUserTx to the tenant half. + if seen := organizationsVisibleWith(t, db); seen != "" { + t.Fatalf("an unbound transaction saw %q, want nothing", seen) + } + if seen := organizationsVisibleWith(t, db, + libraryTenantSetting, "", hostTenantSetting, ""); seen != "" { + t.Fatalf("a transaction with both tenant settings bound empty saw %q, want nothing", seen) + } + + // And the WITH CHECK half, which a SELECT never exercises: a write under + // the retired binding alone is admitted for its own tenant and refused for + // the other. Under forced RLS a refused UPDATE is not an error — it matches + // zero rows — so the row count is the assertion. + if affected := organizationsRenamedWith(t, db, orgA, hostTenantSetting, orgA); affected != 1 { + t.Fatalf("an UPDATE bound to the retired %s changed %d rows of its own organisation, want 1", hostTenantSetting, affected) + } + if affected := organizationsRenamedWith(t, db, orgB, hostTenantSetting, orgA); affected != 0 { + t.Fatalf("an UPDATE bound to the retired %s changed %d rows of ANOTHER organisation, want 0", hostTenantSetting, affected) + } + + // The down half closes the window, restoring the catalog it found. That + // text is also the later migration that closes it for good, so this is the + // only place that proves it before it is cut. + abs, err := filepath.Abs(ledger) + if err != nil { + t.Fatal(err) + } + runner, err := migrate.New("file://"+abs, databaseURL) + if err != nil { + t.Fatal(err) + } + defer func() { _, _ = runner.Close() }() + if err := runner.Steps(-1); err != nil { + t.Fatal(err) + } + if closed := rowSecurityCatalog(t, db); closed != before { + t.Fatalf("the down half did not restore version 65's catalog:\n%s", firstDifference(before, closed)) + } + if err := runner.Steps(1); err != nil { + t.Fatal(err) + } + if replayed := rowSecurityCatalog(t, db); replayed != after { + t.Fatalf("replaying the up half did not reproduce the window:\n%s", firstDifference(after, replayed)) + } + // Applied twice over, it is the same catalog: the rewrite recognises a + // predicate already in the window instead of nesting a second COALESCE + // around it. A migration that is not idempotent here cannot be re-run + // against a database whose version row was lost. + if err := applyScopeSettingsTransitionAgain(t, databaseURL); err != nil { + t.Fatal(err) + } + if again := rowSecurityCatalog(t, db); again != after { + t.Fatalf("applying the up half a second time changed the catalog:\n%s", firstDifference(after, again)) + } +} + +// organizationsVisibleWith is every organization id a request transaction sees +// with each named setting bound to the value after it: as app_tenant, which +// holds no BYPASSRLS, in a transaction of its own so no binding outlives it. +// +// It takes a list rather than one setting (scopedOrganizationIDs, which the +// migration-65 test uses) because the question here is what a transaction +// carrying BOTH names sees, which is what the host now binds. +func organizationsVisibleWith(t *testing.T, db *sql.DB, bindings ...string) string { + t.Helper() + if len(bindings)%2 != 0 { + t.Fatalf("bindings must be setting/value pairs, got %d values", len(bindings)) + } + tx, err := db.Begin() + if err != nil { + t.Fatal(err) + } + defer func() { _ = tx.Rollback() }() + if _, err := tx.Exec(`SET LOCAL ROLE app_tenant`); err != nil { + t.Fatal(err) + } + for i := 0; i < len(bindings); i += 2 { + if _, err := tx.Exec(`SELECT pg_catalog.set_config($1, $2, true)`, bindings[i], bindings[i+1]); err != nil { + t.Fatal(err) + } + } + var seen string + if err := tx.QueryRow(`SELECT coalesce(string_agg(id::text, ',' ORDER BY id::text), '') FROM public.organizations`).Scan(&seen); err != nil { + t.Fatal(err) + } + return seen +} + +// organizationsRenamedWith is how many rows an UPDATE of organization `target` +// changes under those bindings, as app_tenant, rolled back. The FOR ALL policy's +// WITH CHECK judges the new row, which no SELECT reaches. +func organizationsRenamedWith(t *testing.T, db *sql.DB, target string, bindings ...string) int64 { + t.Helper() + tx, err := db.Begin() + if err != nil { + t.Fatal(err) + } + defer func() { _ = tx.Rollback() }() + if _, err := tx.Exec(`SET LOCAL ROLE app_tenant`); err != nil { + t.Fatal(err) + } + for i := 0; i < len(bindings); i += 2 { + if _, err := tx.Exec(`SELECT pg_catalog.set_config($1, $2, true)`, bindings[i], bindings[i+1]); err != nil { + t.Fatal(err) + } + } + result, err := tx.Exec(`UPDATE public.organizations SET name = name || '-renamed' WHERE id = $1::uuid`, target) + if err != nil { + t.Fatal(err) + } + affected, err := result.RowsAffected() + if err != nil { + t.Fatal(err) + } + return affected +} + +// applyScopeSettingsTransitionAgain runs migration 66's up half once more, +// outside the runner, in one transaction — the shape an operator gets from +// `psql --single-transaction -f`, and the shape a consumer runs the recipe in. +func applyScopeSettingsTransitionAgain(t *testing.T, databaseURL string) error { + t.Helper() + body, err := os.ReadFile(filepath.Join("..", "migrations", "66_scope_settings_transition.up.sql")) + if err != nil { + return err + } + db, err := sql.Open("postgres", databaseURL) + if err != nil { + return err + } + defer func() { _ = db.Close() }() + tx, err := db.Begin() + if err != nil { + return err + } + defer func() { _ = tx.Rollback() }() + if _, err := tx.Exec(string(body)); err != nil { + return err + } + return tx.Commit() +} diff --git a/module/services/store/migrations/66_scope_settings_transition.down.sql b/module/services/store/migrations/66_scope_settings_transition.down.sql new file mode 100644 index 000000000..5f9aeede4 --- /dev/null +++ b/module/services/store/migrations/66_scope_settings_transition.down.sql @@ -0,0 +1,120 @@ +-- Close the compatibility window: every policy and routine body that reads both +-- scope settings goes back to reading the platform Postgres library's name +-- alone, in the spelling it had before 66 wrapped it. +-- +-- This is the exact reverse of the up half, which is what makes it the text of +-- the later migration that closes the window for good — the one cut once no +-- deployable image binds the retired names. It is the same catalog-driven walk, +-- the same ALTER POLICY (so no policy's kind, verb or role list is restated), +-- and the same replace-as-owner idiom for a routine the baseline reassigned. +-- +-- Running it as a rollback puts the database back where 65 left it, which is +-- also where the version skew it exists to survive bites: a pod that binds the +-- retired name reads no rows. Roll the image back too, or forward past it. + +SET LOCAL lock_timeout = '15s'; + +CREATE FUNCTION pg_temp.scope_settings_single_read(body text) RETURNS text + LANGUAGE plpgsql IMMUTABLE AS $single$ +DECLARE + pair record; + live text; + legacy text; + compat_re text; +BEGIN + FOR pair IN + SELECT * + FROM (VALUES ('codefly.current_tenant_id', 'app.current_org_id'), + ('codefly.current_user_id', 'app.current_user_id')) + AS p(live, legacy) + LOOP + live := replace(pair.live, '.', '\.'); + legacy := replace(pair.legacy, '.', '\.'); + -- The compatibility read, with the qualification and the cast of its + -- FIRST arm captured: the up half wrote both arms from one match, so + -- restoring the first arm's spelling restores the text it replaced. + compat_re := 'COALESCE\(NULLIF\(((?:pg_catalog\.)?)current_setting\(''' || live + || '''(::text)?, true\), ''''::text\), (?:pg_catalog\.)?current_setting\(''' || legacy + || '''(?:::text)?, true\)\)'; + body := regexp_replace(body, compat_re, + '\1current_setting(''' || pair.live || '''\2, true)', 'g'); + END LOOP; + RETURN body; +END +$single$; + +DO $transition$ +DECLARE + pol record; + routine record; + new_qual text; + new_check text; + new_def text; + rewritten int := 0; +BEGIN + FOR pol IN + SELECT p.polname, + p.polrelid::regclass AS relation, + pg_get_expr(p.polqual, p.polrelid) AS qual, + pg_get_expr(p.polwithcheck, p.polrelid) AS with_check + FROM pg_catalog.pg_policy p + JOIN pg_catalog.pg_class c ON c.oid = p.polrelid + JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = 'public' + ORDER BY p.polrelid::regclass::text, p.polname + LOOP + new_qual := pg_temp.scope_settings_single_read(COALESCE(pol.qual, '')); + new_check := pg_temp.scope_settings_single_read(COALESCE(pol.with_check, '')); + IF new_qual = COALESCE(pol.qual, '') AND new_check = COALESCE(pol.with_check, '') THEN + CONTINUE; + END IF; + IF pol.qual IS NOT NULL AND pol.with_check IS NOT NULL THEN + EXECUTE format('ALTER POLICY %I ON %s USING (%s) WITH CHECK (%s)', + pol.polname, pol.relation, new_qual, new_check); + ELSIF pol.qual IS NOT NULL THEN + EXECUTE format('ALTER POLICY %I ON %s USING (%s)', pol.polname, pol.relation, new_qual); + ELSE + EXECUTE format('ALTER POLICY %I ON %s WITH CHECK (%s)', pol.polname, pol.relation, new_check); + END IF; + rewritten := rewritten + 1; + END LOOP; + + FOR routine IN + SELECT pg_get_functiondef(p.oid) AS definition, + pg_get_userbyid(p.proowner) AS owner, + p.oid::regprocedure::text AS signature + FROM pg_catalog.pg_proc p + JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace + WHERE n.nspname = 'public' AND p.prokind IN ('f', 'p') + ORDER BY p.oid::regprocedure::text + LOOP + new_def := pg_temp.scope_settings_single_read(routine.definition); + IF new_def = routine.definition THEN + CONTINUE; + END IF; + IF routine.owner = current_user THEN + EXECUTE new_def; + ELSE + EXECUTE format('GRANT CREATE ON SCHEMA public TO %I', routine.owner); + EXECUTE format('SET LOCAL ROLE %I', routine.owner); + EXECUTE new_def; + EXECUTE 'RESET ROLE'; + EXECUTE format('REVOKE CREATE ON SCHEMA public FROM %I', routine.owner); + END IF; + rewritten := rewritten + 1; + END LOOP; + + IF rewritten = 0 AND NOT EXISTS ( + SELECT 1 FROM pg_catalog.pg_policy p + WHERE COALESCE(pg_get_expr(p.polqual, p.polrelid), '') + || COALESCE(pg_get_expr(p.polwithcheck, p.polrelid), '') + ~ 'current_setting\(''codefly\.current_tenant_id' + ) THEN + RAISE EXCEPTION 'no policy or routine in this database reads either scope setting, so there is no compatibility window to close' + USING HINT = 'check the connection: this is not a database whose tenant isolation reads codefly.current_tenant_id'; + END IF; + RAISE NOTICE 'scope settings: % policies and routines now read the library name alone', rewritten; +END +$transition$; + +DROP FUNCTION pg_temp.scope_settings_single_read(text); diff --git a/module/services/store/migrations/66_scope_settings_transition.up.sql b/module/services/store/migrations/66_scope_settings_transition.up.sql new file mode 100644 index 000000000..12db9f26e --- /dev/null +++ b/module/services/store/migrations/66_scope_settings_transition.up.sql @@ -0,0 +1,248 @@ +-- Open the compatibility window on the two scope settings: every policy and +-- routine body that reads one reads both, the platform Postgres library's name +-- first and the host's retired one second. +-- +-- Migration 65 renamed app.current_org_id / app.current_user_id onto +-- codefly.current_tenant_id / codefly.current_user_id in one step, and a +-- one-step rename of an RLS setting is not deployable: the catalog and the +-- binary that binds it ship separately, and the half that loses the race reads +-- an unset setting, which is zero rows on every scoped SELECT and a WITH CHECK +-- failure on every INSERT, silently. This migration is the transition 65 needed +-- — paired with a host that binds both names, so a pod serves correctly against +-- a catalog from before the rename, after it, or in this window. +-- +-- The body below is ../../../tools/rls-scope-settings-transition.sql, byte for +-- byte: a consumer whose own migrations wrote tenant policies against the +-- convention this repository documented before 65 runs that file against its +-- own database and lands in the same state, and +-- tools/rls_scope_settings_recipe_test.go holds the two texts in step. It is +-- catalog-driven rather than generated, so it also reaches a policy whose name +-- depends on the month (the partitions of public.audit_events) and a consumer's +-- own policies, which no generated file here could name. +-- +-- The window closes in a later migration: the same catalog-driven walk, +-- rewriting the compatibility read back to a single read of the library's name. +-- That text is 66_scope_settings_transition.down.sql, which the replay test +-- already proves, renumbered above the frontier and cut once no deployable +-- image binds the retired names. It is a release boundary, not a cleanup, and +-- it must not ship beside this one -- together they are the one-step rename +-- again. + +-- rls-scope-settings-transition — open the compatibility window on the two +-- scope settings, over whatever this database's policies and routines read now. +-- +-- A tenant-scoped policy reads `codefly.current_tenant_id` and a user-scoped +-- one `codefly.current_user_id` — the defaults of the platform's Postgres +-- library, which installs them for every transaction opened through its scoped +-- factory. The host carried names of its own, `app.current_org_id` and +-- `app.current_user_id`, until store migration 65 renamed every policy and +-- routine body onto the library's two. +-- +-- A rename is not deployable on its own. The policy and the binary that binds +-- it ship separately, and the predicate is `col::text = current_setting(, +-- true)`: a binary that binds the other name reads NULL, so every scoped SELECT +-- returns zero rows with no error and every INSERT fails its WITH CHECK. A +-- rolling update applies the migration before the new pods are ready; a +-- rollback of the image leaves the whole fleet there indefinitely. Nothing +-- raises — the loss of access is silent and total. +-- +-- This rewrites every such predicate to read BOTH names, the library's first: +-- +-- COALESCE(NULLIF(current_setting('codefly.current_tenant_id', true), ''), +-- current_setting('app.current_org_id', true)) +-- +-- so one database serves a binary that binds either name, and a host that binds +-- both is correct against a catalog from before the rename, after it, or in this +-- window. Fail-closed is preserved exactly: a transaction that binds neither +-- half reads '' through both arms, the predicate is false, and the relation is +-- empty rather than open. An absent setting and an empty one stay the same +-- thing, which is what NULLIF is for. +-- +-- It is catalog-driven on purpose. The whole set — 75 policies over 70 +-- relations, 6 more on partitions of public.audit_events, and 9 routine bodies +-- when migration 65 renamed them — is known only to the catalog, and a +-- consumer's own set is known only to the consumer's. So this reads what is +-- there rather than naming it, and reaches the same end state from either side +-- of the rename: +-- +-- * a predicate already reading both names is left exactly as it is, so the +-- rewrite is idempotent; +-- * a predicate reading the library's name is wrapped; +-- * a predicate still reading the host's retired name — a consumer that +-- wrote its policies against the convention this repository documented +-- before migration 65 — is wrapped to the same text, which is also that +-- consumer's rename. +-- +-- A policy's kind, verb and role list are not reassigned: this is ALTER POLICY, +-- which cannot change them, rather than DROP and CREATE, which restates them +-- all and can get one wrong. A routine is replaced as its own owner, because +-- the baseline hands many of them to an application role (see the "Replacing a +-- function the baseline reassigned" idiom in +-- services/store/migrations/README.md); CREATE OR REPLACE keeps the owner and +-- the grants. +-- +-- Running it by hand, against a consumer's own database: +-- +-- psql "$DATABASE_URL" --single-transaction \ +-- -f modules//tools/rls-scope-settings-transition.sql +-- +-- It must run in a transaction — every statement below belongs to one change — +-- and as the principal that installs that database's migrations, which owns the +-- tables and the schema. It is also store migration 66 of this module's own +-- ledger, byte for byte, and a test holds the two in step. +-- +-- The window closes in a later migration, which rewrites these predicates back +-- to a single read of the library's name once no deployable binary binds the +-- retired one. Until then a policy reading the retired name ALONE is still a +-- silent empty relation, which is what the host's startup check and +-- tools/rls-migration-gate.mjs refuse. + +-- An exclusive lock on every scoped relation, taken in one transaction. Bound +-- the wait: unbounded, this queues behind one long-running query and then +-- blocks every statement on 70 relations for as long as that query runs, which +-- is an outage with a deploy's name on it. Bounded, it fails and is retried. +SET LOCAL lock_timeout = '15s'; + +-- The rewrite, as one text-to-text function: the same substitution has to reach +-- a policy's USING, its WITH CHECK, and a routine's whole definition, and the +-- three are separately executed statements. +CREATE FUNCTION pg_temp.scope_settings_compatibility(body text) RETURNS text + LANGUAGE plpgsql IMMUTABLE AS $compatibility$ +DECLARE + pair record; + live text; -- the library's name, dots escaped for the pattern + legacy text; -- the host's retired name, likewise + live_re text; + legacy_re text; + compat_re text; + compat text; +BEGIN + FOR pair IN + SELECT * + FROM (VALUES ('codefly.current_tenant_id', 'app.current_org_id'), + ('codefly.current_user_id', 'app.current_user_id')) + AS p(live, legacy) + LOOP + live := replace(pair.live, '.', '\.'); + legacy := replace(pair.legacy, '.', '\.'); + -- One read of one setting, in both spellings that occur: optionally + -- schema-qualified, with or without the ::text cast the parser adds and + -- the deparser prints. \1 is the qualification and \2 the cast, so the + -- rewrite keeps whichever the source had — that is what makes the + -- window's own reversal exact, and it keeps a SECURITY DEFINER body's + -- pg_catalog. qualification rather than quietly dropping it. + live_re := '((?:pg_catalog\.)?)current_setting\(''' || live || '''(::text)?, true\)'; + legacy_re := '((?:pg_catalog\.)?)current_setting\(''' || legacy || '''(::text)?, true\)'; + compat := 'COALESCE(NULLIF(\1current_setting(''' || pair.live || '''\2, true), ''''::text), ' + || '\1current_setting(''' || pair.legacy || '''\2, true))'; + compat_re := 'COALESCE\(NULLIF\((?:pg_catalog\.)?current_setting\(''' || live + || '''(?:::text)?, true\), ''''::text\), (?:pg_catalog\.)?current_setting\(''' || legacy + || '''(?:::text)?, true\)\)'; + + IF body ~ compat_re THEN + -- Already inside the window. Wrapping it again would nest a second + -- COALESCE around the first and still read correctly, which is the + -- worst kind of no-op: it would make replay produce a different + -- catalog every time. + CONTINUE; + END IF; + IF body ~ live_re THEN + body := regexp_replace(body, live_re, compat, 'g'); + CONTINUE; + END IF; + IF body ~ legacy_re THEN + -- Both patterns capture the qualification and the cast in the same + -- two groups, so one replacement text serves either direction. + body := regexp_replace(body, legacy_re, compat, 'g'); + END IF; + END LOOP; + RETURN body; +END +$compatibility$; + +DO $transition$ +DECLARE + pol record; + routine record; + new_qual text; + new_check text; + new_def text; + rewritten int := 0; +BEGIN + FOR pol IN + SELECT p.polname, + p.polrelid::regclass AS relation, + pg_get_expr(p.polqual, p.polrelid) AS qual, + pg_get_expr(p.polwithcheck, p.polrelid) AS with_check + FROM pg_catalog.pg_policy p + JOIN pg_catalog.pg_class c ON c.oid = p.polrelid + JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = 'public' + ORDER BY p.polrelid::regclass::text, p.polname + LOOP + -- Every partition of a partitioned table carries its own copy of the + -- parent's policies, and a query that names the partition is judged by + -- those alone. They are rows of pg_policy like any other, so this loop + -- reaches them without knowing a partition's name — which depends on + -- the month. + new_qual := pg_temp.scope_settings_compatibility(COALESCE(pol.qual, '')); + new_check := pg_temp.scope_settings_compatibility(COALESCE(pol.with_check, '')); + IF new_qual = COALESCE(pol.qual, '') AND new_check = COALESCE(pol.with_check, '') THEN + CONTINUE; + END IF; + IF pol.qual IS NOT NULL AND pol.with_check IS NOT NULL THEN + EXECUTE format('ALTER POLICY %I ON %s USING (%s) WITH CHECK (%s)', + pol.polname, pol.relation, new_qual, new_check); + ELSIF pol.qual IS NOT NULL THEN + EXECUTE format('ALTER POLICY %I ON %s USING (%s)', pol.polname, pol.relation, new_qual); + ELSE + EXECUTE format('ALTER POLICY %I ON %s WITH CHECK (%s)', pol.polname, pol.relation, new_check); + END IF; + rewritten := rewritten + 1; + END LOOP; + + FOR routine IN + SELECT pg_get_functiondef(p.oid) AS definition, + pg_get_userbyid(p.proowner) AS owner, + p.oid::regprocedure::text AS signature + FROM pg_catalog.pg_proc p + JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace + WHERE n.nspname = 'public' AND p.prokind IN ('f', 'p') + ORDER BY p.oid::regprocedure::text + LOOP + new_def := pg_temp.scope_settings_compatibility(routine.definition); + IF new_def = routine.definition THEN + CONTINUE; + END IF; + IF routine.owner = current_user THEN + EXECUTE new_def; + ELSE + -- The owner holds CREATE on the schema for this statement only. + EXECUTE format('GRANT CREATE ON SCHEMA public TO %I', routine.owner); + EXECUTE format('SET LOCAL ROLE %I', routine.owner); + EXECUTE new_def; + EXECUTE 'RESET ROLE'; + EXECUTE format('REVOKE CREATE ON SCHEMA public FROM %I', routine.owner); + END IF; + rewritten := rewritten + 1; + END LOOP; + + -- A rewrite that found nothing is indistinguishable from a clean one, and + -- the thing it is most likely to mean is that this is not the database + -- anyone meant: the wrong connection, an empty schema, a ledger that never + -- installed. Re-running it on a database already inside the window is + -- legitimate and stays quiet. + IF rewritten = 0 AND NOT EXISTS ( + SELECT 1 FROM pg_catalog.pg_policy p + WHERE COALESCE(pg_get_expr(p.polqual, p.polrelid), '') + || COALESCE(pg_get_expr(p.polwithcheck, p.polrelid), '') + ~ 'current_setting\(''codefly\.current_tenant_id' + ) THEN + RAISE EXCEPTION 'no policy or routine in this database reads either scope setting, so there is nothing to put in the compatibility window' + USING HINT = 'check the connection: this is not a database whose tenant isolation reads codefly.current_tenant_id or app.current_org_id'; + END IF; + RAISE NOTICE 'scope settings: % policies and routines now read the library name and the retired one', rewritten; +END +$transition$; + +DROP FUNCTION pg_temp.scope_settings_compatibility(text); diff --git a/module/services/store/migrations/README.md b/module/services/store/migrations/README.md index 95279cf1f..88c39710e 100644 --- a/module/services/store/migrations/README.md +++ b/module/services/store/migrations/README.md @@ -13,7 +13,21 @@ the runner refuses one rather than leave it behind (`code/main.go`, Add a forward migration above the baseline, as a matched `N_name.up.sql` / `N_name.down.sql` pair with `N` above the current frontier. A shipped file is -immutable; corrections are new forward migrations. When the ledger has grown +immutable; corrections are new forward migrations — including a correction to +how a shipped migration takes its locks, which is why the bound below lives in +66 and not in 65. + +A migration that touches many relations runs in one implicit transaction and +holds every lock it takes until the end of it. `ALTER TABLE`, `CREATE POLICY` +and `ALTER POLICY` each take `AccessExclusiveLock`, so a migration over the +whole scoped set blocks every statement on seventy relations for as long as it +waits — and with no bound it waits for whatever long-running query it queued +behind, turning a deploy into an outage. Open such a migration with `SET LOCAL +lock_timeout` (`66_scope_settings_transition` uses `'15s'`): a bounded wait +fails and is retried, where an unbounded one takes the database with it. Prefer +`ALTER POLICY` to a `DROP`/`CREATE` pair for the same reason it is preferred in +66 — it cannot restate a policy's kind, verb or role list, so it cannot get one +of them wrong. When the ledger has grown enough to be worth reading as one schema again, fold it: ```sh @@ -78,7 +92,41 @@ A consumer's own migration must use the same two names. A policy on any other setting fails **quietly**: nothing binds it, `current_setting(…, true)` returns NULL, the predicate is false, and the relation reads as empty for every caller rather than raising. `../../../tools/rls-migration-gate.mjs` is what refuses -that, and it accepts only these two. +that, and it accepts only these two — a predicate on a retired name alone is +refused with the remedy named. + +### The compatibility window, and how to close it + +A rename of a scope setting cannot be deployed in one step. The policy and the +binary that binds it ship separately: the bootstrap job applies the migration +before the new ReplicaSet is ready, and rolling the image back leaves the old +one against the new catalog indefinitely. Either way the half that loses the +race reads an unset setting, and the product goes blank without an error. + +`66_scope_settings_transition` is the transition 65 needed. Every predicate and +routine body reads the library's name with the host's retired one behind it: + +```sql +COALESCE(NULLIF(current_setting('codefly.current_tenant_id', true), ''), + current_setting('app.current_org_id', true)) +``` + +Fail-closed is unchanged — both arms empty is a false predicate, not an absent +one — and accounts binds all four names, so one image serves a catalog from +either side of the rename. The migration is +`../../../tools/rls-scope-settings-transition.sql` byte for byte +(`module/tools/rls_scope_settings_recipe_test.go` holds the two in step), which +is the same file a consumer runs against its own database: it is catalog-driven +and idempotent, and reaches a policy whose name depends on the month and a +consumer's own policies alike. + +**Closing the window is a release of its own.** Its migration is the text of +`66_scope_settings_transition.down.sql` — the same walk, rewriting the +compatibility read back to a single read of the library's name — renumbered +above the frontier, cut only once no deployable image binds the retired names. +It must not ship beside 66: together they are the one-step rename again. Once it +lands, `pkg/infra`'s two `Legacy*ScopeSetting` constants, the retired arm in +`bindRequestScopeSQL` and the host's half of the gate's remedy go with it. ## What CI proves diff --git a/module/tools/rls-migration-gate.mjs b/module/tools/rls-migration-gate.mjs index 2d709be56..4ac73d479 100644 --- a/module/tools/rls-migration-gate.mjs +++ b/module/tools/rls-migration-gate.mjs @@ -51,7 +51,14 @@ const TENANT_COLUMNS = new Set(["org_id", "organization_id", "tenant_id"]); // — codefly-dev/service-postgres installs exactly these for a transaction opened // through its scoped factory — so a policy on any other setting reads something // nothing sets. The host carried names of its own (app.current_org_id, -// app.current_user_id) until migration 65 renamed them onto these. +// app.current_user_id) until migration 65 renamed them onto these, and +// migration 66 left those two readable BEHIND the library's for the length of +// the compatibility window: a predicate of the form +// COALESCE(NULLIF(current_setting('codefly.current_tenant_id', true), ''), +// current_setting('app.current_org_id', true)) reads the library's name and is +// admitted here, which is what lets one database serve a pod from either side +// of a rename. A predicate on a retired name ALONE is not admitted — that is +// the silent-empty state, and the error below names the remedy. const SCOPING_SETTING = /current_setting\(\s*'codefly\.current_(?:tenant|user)_id'/; const MUTATION_VERBS = ["UPDATE", "DELETE"]; @@ -484,7 +491,10 @@ export function analyzeSql(sql) { exactRolePredicate(expr) === `current_user = '${p.roles[0]}'` )) { errors.push( - `${name}: policy ${p.policy} has a ${clause} predicate that never references codefly.current_tenant_id/codefly.current_user_id — it may be accidentally unconditional`, + `${name}: policy ${p.policy} has a ${clause} predicate that never references codefly.current_tenant_id/codefly.current_user_id — it may be accidentally unconditional.` + + " A predicate on the host's retired app.current_org_id/app.current_user_id reads a name nothing binds any more:" + + " rewrite it onto the two names above, or run tools/rls-scope-settings-transition.sql over the database to put every" + + " predicate it already installed into the compatibility window (store migration 66 is that file applied to this module's own ledger)", ); } } diff --git a/module/tools/rls-migration-gate.test.mjs b/module/tools/rls-migration-gate.test.mjs index c0a6c1146..8cad72551 100644 --- a/module/tools/rls-migration-gate.test.mjs +++ b/module/tools/rls-migration-gate.test.mjs @@ -112,6 +112,32 @@ test("an unconditional policy predicate is rejected", () => { assert.ok(errors.every((e) => /may be accidentally unconditional/.test(e))); }); +test("a predicate on the host's retired setting is refused, and the error names the remedy", () => { + // The state a consumer that wrote its policies against the convention this + // repository documented before migration 65 is in. Nothing binds the retired + // name any more, so the relation reads as empty for every caller — and the + // refusal has to say what to do about it, because the predicate looks exactly + // like a correct one. + const sql = + tenantSetup + + "CREATE POLICY p ON t USING (org_id::text = current_setting('app.current_org_id', true));"; + const errors = analyzeSql(sql); + assert.equal(errors.length, 1); + assert.match(errors[0], /app\.current_org_id/); + assert.match(errors[0], /rls-scope-settings-transition\.sql/); + assert.match(errors[0], /migration 66/); +}); + +test("the compatibility window reads the library's name and is admitted", () => { + // One database serving a pod from either side of the rename: the library's + // name first, the retired one behind it. The gate must not read this as a + // policy on a setting nothing binds. + const sql = + tenantSetup + + "CREATE POLICY p ON t USING (org_id::text = COALESCE(NULLIF(current_setting('codefly.current_tenant_id', true), ''), current_setting('app.current_org_id', true)));"; + assert.deepEqual(analyzeSql(sql), []); +}); + test("a user-scoped predicate counts as tenant-scoped", () => { const sql = ` CREATE TABLE t (org_id UUID, user_id UUID); diff --git a/module/tools/rls-scope-settings-transition.sql b/module/tools/rls-scope-settings-transition.sql new file mode 100644 index 000000000..cdb1e71e6 --- /dev/null +++ b/module/tools/rls-scope-settings-transition.sql @@ -0,0 +1,218 @@ +-- rls-scope-settings-transition — open the compatibility window on the two +-- scope settings, over whatever this database's policies and routines read now. +-- +-- A tenant-scoped policy reads `codefly.current_tenant_id` and a user-scoped +-- one `codefly.current_user_id` — the defaults of the platform's Postgres +-- library, which installs them for every transaction opened through its scoped +-- factory. The host carried names of its own, `app.current_org_id` and +-- `app.current_user_id`, until store migration 65 renamed every policy and +-- routine body onto the library's two. +-- +-- A rename is not deployable on its own. The policy and the binary that binds +-- it ship separately, and the predicate is `col::text = current_setting(, +-- true)`: a binary that binds the other name reads NULL, so every scoped SELECT +-- returns zero rows with no error and every INSERT fails its WITH CHECK. A +-- rolling update applies the migration before the new pods are ready; a +-- rollback of the image leaves the whole fleet there indefinitely. Nothing +-- raises — the loss of access is silent and total. +-- +-- This rewrites every such predicate to read BOTH names, the library's first: +-- +-- COALESCE(NULLIF(current_setting('codefly.current_tenant_id', true), ''), +-- current_setting('app.current_org_id', true)) +-- +-- so one database serves a binary that binds either name, and a host that binds +-- both is correct against a catalog from before the rename, after it, or in this +-- window. Fail-closed is preserved exactly: a transaction that binds neither +-- half reads '' through both arms, the predicate is false, and the relation is +-- empty rather than open. An absent setting and an empty one stay the same +-- thing, which is what NULLIF is for. +-- +-- It is catalog-driven on purpose. The whole set — 75 policies over 70 +-- relations, 6 more on partitions of public.audit_events, and 9 routine bodies +-- when migration 65 renamed them — is known only to the catalog, and a +-- consumer's own set is known only to the consumer's. So this reads what is +-- there rather than naming it, and reaches the same end state from either side +-- of the rename: +-- +-- * a predicate already reading both names is left exactly as it is, so the +-- rewrite is idempotent; +-- * a predicate reading the library's name is wrapped; +-- * a predicate still reading the host's retired name — a consumer that +-- wrote its policies against the convention this repository documented +-- before migration 65 — is wrapped to the same text, which is also that +-- consumer's rename. +-- +-- A policy's kind, verb and role list are not reassigned: this is ALTER POLICY, +-- which cannot change them, rather than DROP and CREATE, which restates them +-- all and can get one wrong. A routine is replaced as its own owner, because +-- the baseline hands many of them to an application role (see the "Replacing a +-- function the baseline reassigned" idiom in +-- services/store/migrations/README.md); CREATE OR REPLACE keeps the owner and +-- the grants. +-- +-- Running it by hand, against a consumer's own database: +-- +-- psql "$DATABASE_URL" --single-transaction \ +-- -f modules//tools/rls-scope-settings-transition.sql +-- +-- It must run in a transaction — every statement below belongs to one change — +-- and as the principal that installs that database's migrations, which owns the +-- tables and the schema. It is also store migration 66 of this module's own +-- ledger, byte for byte, and a test holds the two in step. +-- +-- The window closes in a later migration, which rewrites these predicates back +-- to a single read of the library's name once no deployable binary binds the +-- retired one. Until then a policy reading the retired name ALONE is still a +-- silent empty relation, which is what the host's startup check and +-- tools/rls-migration-gate.mjs refuse. + +-- An exclusive lock on every scoped relation, taken in one transaction. Bound +-- the wait: unbounded, this queues behind one long-running query and then +-- blocks every statement on 70 relations for as long as that query runs, which +-- is an outage with a deploy's name on it. Bounded, it fails and is retried. +SET LOCAL lock_timeout = '15s'; + +-- The rewrite, as one text-to-text function: the same substitution has to reach +-- a policy's USING, its WITH CHECK, and a routine's whole definition, and the +-- three are separately executed statements. +CREATE FUNCTION pg_temp.scope_settings_compatibility(body text) RETURNS text + LANGUAGE plpgsql IMMUTABLE AS $compatibility$ +DECLARE + pair record; + live text; -- the library's name, dots escaped for the pattern + legacy text; -- the host's retired name, likewise + live_re text; + legacy_re text; + compat_re text; + compat text; +BEGIN + FOR pair IN + SELECT * + FROM (VALUES ('codefly.current_tenant_id', 'app.current_org_id'), + ('codefly.current_user_id', 'app.current_user_id')) + AS p(live, legacy) + LOOP + live := replace(pair.live, '.', '\.'); + legacy := replace(pair.legacy, '.', '\.'); + -- One read of one setting, in both spellings that occur: optionally + -- schema-qualified, with or without the ::text cast the parser adds and + -- the deparser prints. \1 is the qualification and \2 the cast, so the + -- rewrite keeps whichever the source had — that is what makes the + -- window's own reversal exact, and it keeps a SECURITY DEFINER body's + -- pg_catalog. qualification rather than quietly dropping it. + live_re := '((?:pg_catalog\.)?)current_setting\(''' || live || '''(::text)?, true\)'; + legacy_re := '((?:pg_catalog\.)?)current_setting\(''' || legacy || '''(::text)?, true\)'; + compat := 'COALESCE(NULLIF(\1current_setting(''' || pair.live || '''\2, true), ''''::text), ' + || '\1current_setting(''' || pair.legacy || '''\2, true))'; + compat_re := 'COALESCE\(NULLIF\((?:pg_catalog\.)?current_setting\(''' || live + || '''(?:::text)?, true\), ''''::text\), (?:pg_catalog\.)?current_setting\(''' || legacy + || '''(?:::text)?, true\)\)'; + + IF body ~ compat_re THEN + -- Already inside the window. Wrapping it again would nest a second + -- COALESCE around the first and still read correctly, which is the + -- worst kind of no-op: it would make replay produce a different + -- catalog every time. + CONTINUE; + END IF; + IF body ~ live_re THEN + body := regexp_replace(body, live_re, compat, 'g'); + CONTINUE; + END IF; + IF body ~ legacy_re THEN + -- Both patterns capture the qualification and the cast in the same + -- two groups, so one replacement text serves either direction. + body := regexp_replace(body, legacy_re, compat, 'g'); + END IF; + END LOOP; + RETURN body; +END +$compatibility$; + +DO $transition$ +DECLARE + pol record; + routine record; + new_qual text; + new_check text; + new_def text; + rewritten int := 0; +BEGIN + FOR pol IN + SELECT p.polname, + p.polrelid::regclass AS relation, + pg_get_expr(p.polqual, p.polrelid) AS qual, + pg_get_expr(p.polwithcheck, p.polrelid) AS with_check + FROM pg_catalog.pg_policy p + JOIN pg_catalog.pg_class c ON c.oid = p.polrelid + JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = 'public' + ORDER BY p.polrelid::regclass::text, p.polname + LOOP + -- Every partition of a partitioned table carries its own copy of the + -- parent's policies, and a query that names the partition is judged by + -- those alone. They are rows of pg_policy like any other, so this loop + -- reaches them without knowing a partition's name — which depends on + -- the month. + new_qual := pg_temp.scope_settings_compatibility(COALESCE(pol.qual, '')); + new_check := pg_temp.scope_settings_compatibility(COALESCE(pol.with_check, '')); + IF new_qual = COALESCE(pol.qual, '') AND new_check = COALESCE(pol.with_check, '') THEN + CONTINUE; + END IF; + IF pol.qual IS NOT NULL AND pol.with_check IS NOT NULL THEN + EXECUTE format('ALTER POLICY %I ON %s USING (%s) WITH CHECK (%s)', + pol.polname, pol.relation, new_qual, new_check); + ELSIF pol.qual IS NOT NULL THEN + EXECUTE format('ALTER POLICY %I ON %s USING (%s)', pol.polname, pol.relation, new_qual); + ELSE + EXECUTE format('ALTER POLICY %I ON %s WITH CHECK (%s)', pol.polname, pol.relation, new_check); + END IF; + rewritten := rewritten + 1; + END LOOP; + + FOR routine IN + SELECT pg_get_functiondef(p.oid) AS definition, + pg_get_userbyid(p.proowner) AS owner, + p.oid::regprocedure::text AS signature + FROM pg_catalog.pg_proc p + JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace + WHERE n.nspname = 'public' AND p.prokind IN ('f', 'p') + ORDER BY p.oid::regprocedure::text + LOOP + new_def := pg_temp.scope_settings_compatibility(routine.definition); + IF new_def = routine.definition THEN + CONTINUE; + END IF; + IF routine.owner = current_user THEN + EXECUTE new_def; + ELSE + -- The owner holds CREATE on the schema for this statement only. + EXECUTE format('GRANT CREATE ON SCHEMA public TO %I', routine.owner); + EXECUTE format('SET LOCAL ROLE %I', routine.owner); + EXECUTE new_def; + EXECUTE 'RESET ROLE'; + EXECUTE format('REVOKE CREATE ON SCHEMA public FROM %I', routine.owner); + END IF; + rewritten := rewritten + 1; + END LOOP; + + -- A rewrite that found nothing is indistinguishable from a clean one, and + -- the thing it is most likely to mean is that this is not the database + -- anyone meant: the wrong connection, an empty schema, a ledger that never + -- installed. Re-running it on a database already inside the window is + -- legitimate and stays quiet. + IF rewritten = 0 AND NOT EXISTS ( + SELECT 1 FROM pg_catalog.pg_policy p + WHERE COALESCE(pg_get_expr(p.polqual, p.polrelid), '') + || COALESCE(pg_get_expr(p.polwithcheck, p.polrelid), '') + ~ 'current_setting\(''codefly\.current_tenant_id' + ) THEN + RAISE EXCEPTION 'no policy or routine in this database reads either scope setting, so there is nothing to put in the compatibility window' + USING HINT = 'check the connection: this is not a database whose tenant isolation reads codefly.current_tenant_id or app.current_org_id'; + END IF; + RAISE NOTICE 'scope settings: % policies and routines now read the library name and the retired one', rewritten; +END +$transition$; + +DROP FUNCTION pg_temp.scope_settings_compatibility(text); diff --git a/module/tools/rls_scope_settings_recipe_test.go b/module/tools/rls_scope_settings_recipe_test.go new file mode 100644 index 000000000..53e9b1528 --- /dev/null +++ b/module/tools/rls_scope_settings_recipe_test.go @@ -0,0 +1,96 @@ +package tools + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +const ( + scopeSettingsRecipe = "rls-scope-settings-transition.sql" + scopeSettingsMigration = "66_scope_settings_transition.up.sql" +) + +// The recipe a consumer runs against its own database and the migration this +// module runs against its own are one text. A consumer's tenant policies are +// its own migrations' work, and until migration 65 the convention this +// repository documented was the host's `app.current_org_id`: upgrading the host +// without rewriting them leaves every one of its tables reading as empty, with +// no error. So the rewrite ships as a file a consumer can run, and the module's +// migration 66 embeds that same file rather than restating it. +// +// Two copies of a text-rewriting rule are two rules. This is what makes the one +// the migration applies the one the recipe hands out — and what the replay test +// in services/store/code proves, since it applies the migration. +func TestMigration66IsTheShippedScopeSettingsRecipe(t *testing.T) { + recipe := readFile(t, filepath.Join("..", "tools", scopeSettingsRecipe)) + migration := readFile(t, filepath.Join("..", "services", "store", "migrations", scopeSettingsMigration)) + + if !strings.Contains(migration, recipe) { + t.Fatalf("%s does not contain %s byte for byte.\nThe migration is the recipe with a header comment: regenerate it with\n"+ + " { printf '%%s\\n' \"
\"; cat module/tools/%s; } > module/services/store/migrations/%s", + scopeSettingsMigration, scopeSettingsRecipe, scopeSettingsRecipe, scopeSettingsMigration) + } + // The migration adds a header and nothing else. A body appended AFTER the + // recipe would still contain it, and would be a second rule in the one + // place nobody would look for it. + if tail := strings.TrimSpace(migration[strings.Index(migration, recipe)+len(recipe):]); tail != "" { + t.Fatalf("%s has %d bytes of its own after the recipe; the migration may only add a header before it:\n%s", + scopeSettingsMigration, len(tail), tail) + } + if head := strings.TrimSpace(migration[:strings.Index(migration, recipe)]); !strings.HasPrefix(head, "--") { + t.Fatalf("%s must open with a comment header; it opens with %q", scopeSettingsMigration, firstLine(head)) + } +} + +// The window is only a window if both halves of both names survive the rewrite, +// and if what it produces is still fail-closed. The replay test proves the +// behaviour against a real cluster; this is the cheap guard that the text has +// not lost one of the four names — a rewrite that silently stopped handling the +// user half would leave notifications, sessions and mfa_devices reading as +// empty for every caller on a pre-rename pod, which no SELECT in this +// repository's suite would notice. +func TestScopeSettingsRecipeCarriesBothGenerationsOfBothNames(t *testing.T) { + recipe := readFile(t, filepath.Join("..", "tools", scopeSettingsRecipe)) + for _, setting := range []string{ + "codefly.current_tenant_id", "codefly.current_user_id", + "app.current_org_id", "app.current_user_id", + } { + if !strings.Contains(recipe, setting) { + t.Errorf("%s never names %s, so a predicate reading it is left outside the window", scopeSettingsRecipe, setting) + } + } + // NULLIF is what keeps an empty binding from falling through to the other + // generation's value: without it, a transaction that cleared the library's + // half would be judged by whatever the retired half still held. + if !strings.Contains(recipe, "NULLIF") || !strings.Contains(recipe, "COALESCE") { + t.Errorf("%s must wrap the live read in COALESCE(NULLIF(...)): without NULLIF an empty binding falls through to the other name", scopeSettingsRecipe) + } + // An unbounded exclusive lock on every scoped relation is an outage with a + // deploy's name on it. + if !strings.Contains(recipe, "lock_timeout") { + t.Errorf("%s takes an exclusive lock on every scoped relation and must bound the wait with lock_timeout", scopeSettingsRecipe) + } + // ALTER POLICY cannot restate a policy's kind, verb or role list, which is + // the point: DROP and CREATE can get one of them wrong. + if strings.Contains(recipe, "DROP POLICY") || !strings.Contains(recipe, "ALTER POLICY") { + t.Errorf("%s must rewrite a predicate with ALTER POLICY, which cannot reassign the kind, verb or roles a DROP/CREATE pair restates", scopeSettingsRecipe) + } +} + +func readFile(t *testing.T, path string) string { + t.Helper() + body, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + return string(body) +} + +func firstLine(s string) string { + if i := strings.IndexByte(s, '\n'); i >= 0 { + return s[:i] + } + return s +}