feat: User isolation - SQL executes with submitter's privileges - #27
feat: User isolation - SQL executes with submitter's privileges#27Pino de Candia (pinodeca) wants to merge 10 commits into
Conversation
- Explains that functions execute with submitter's privileges - Documents how identity is captured (login_role + submitted_by) - Covers group roles and SET ROLE behavior - Explains dropped role failure mode - Documents current limitations (shared df.vars, HTTP not isolated, cross-instance visibility) - Provides security best practices and minimal permission grants - Updates table of contents Addresses one of the key recommendations from the implementation review.
Verifies that GetOuterUserId() captures caller identity, not definer. Test 6a: Alice calls SECURITY DEFINER wrapper to query her table (succeeds) Test 6b: Alice calls SECURITY DEFINER wrapper to query superuser table (fails) This proves durable functions run as the caller even when df.start() is called inside a SECURITY DEFINER function. Addresses the critical blocking issue identified in the implementation review.
Test 7: Dropped role - verifies clear failure when role dropped mid-execution Addresses high-priority user-isolation review recommendation.
484fdd9 to
39c46a6
Compare
- Add explicit DROP TABLE github_commits in cleanup - Add --clean flag to CI E2E tests to clear cached data directories Fixes ownership error "must be owner of table github_commits" that occurred due to persistent tables in cached ~/.pgrx data directories.
39c46a6 to
fc2dd4f
Compare
Explains why identity uses REGROLE columns (security-critical, needs type validation) while execution context like search_path might use JSONB (supplementary, needs flexibility) when eventually implemented.
There was a problem hiding this comment.
Pull request overview
This PR implements user privilege isolation for pg_durable: SQL in durable functions now executes with the submitting user's privileges rather than the background worker's superuser credentials.
Changes:
- Core implementation: captures user identity (
GetSessionUserId/GetOuterUserId) atdf.start()time, stores it insubmitted_by/login_rolecolumns, and opens per-user connections via a newconnect_as_user()function - E2E test infrastructure: most tests now run as non-privileged
df_e2e_user; two new test files cover superuser scenarios (26) and comprehensive user isolation (27) - Documentation: new
docs/user-isolation.mddesign document,docs/user-isolation-review.mdreview notes, and updatedUSER_GUIDE.mdwith a "User Isolation & Privileges" section
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib.rs |
Adds submitted_by REGROLE and login_role REGROLE columns to df.nodes and df.instances DDL |
src/dsl.rs |
Captures OIDs in df.start() and propagates them to instance and node rows |
src/types.rs |
Adds connect_as_user() helper and identity fields to FunctionNode |
src/activities/execute_sql.rs |
Switches from shared pool to per-user connection; parses ExecuteSqlInput JSON |
src/activities/load_function_graph.rs |
Adds submitted_by/login_role to the node SELECT query |
src/orchestrations/execute_function_graph.rs |
Packages query + identity as JSON before scheduling execute_sql |
src/registry.rs |
Updates activity registration signature from query to input_json |
src/worker.rs |
Suppresses noisy pgpass warnings from sqlx |
tests/e2e/sql/00_setup_playground.sql |
Creates df_e2e_user role and transfers ownership of playground objects |
tests/e2e/sql/26_superuser_scenarios.sql |
New test: verifies superuser durable SQL works |
tests/e2e/sql/27_user_isolation.sql |
New test: comprehensive user isolation coverage (7 scenarios) |
tests/e2e/sql/22_cross_connection.sql |
Removes now-redundant CREATE EXTENSION dblink (moved to setup) |
tests/e2e/sql/23_transactions.sql |
Same dblink cleanup |
tests/e2e/sql/19_github_api.sql |
Adds missing DROP TABLE github_commits cleanup |
scripts/test-e2e-local.sh |
Runs setup once as superuser; dispatches individual tests as df_e2e_user or superuser as appropriate |
docs/user-isolation.md |
Full design and architecture document |
docs/user-isolation-review.md |
Self-review document tracking findings and resolutions |
USER_GUIDE.md |
Adds "User Isolation & Privileges" section |
.github/workflows/ci.yml |
Adds --clean flag to E2E test steps |
| sqlx::query(&format!( | ||
| "SET ROLE \"{}\"", | ||
| effective_role.replace('"', "\"\"") | ||
| )) |
There was a problem hiding this comment.
The login_role string is compared directly to effective_role, but login_role itself is not double-quote escaped. If a role name happens to contain double-quote characters, the comparison would still match but the SET ROLE query path for unequal names does escape correctly. The real issue is the opposite case: when the roles are equal but the raw string contains a double-quote, the SET ROLE is skipped — which is fine. However, the login_role used as the username() in PgConnectOptions on line 92 is passed unescaped, which is correct for the libpq API. The inconsistency is in the equality check: the comparison should be done on the unescaped names (which it already is), so this is actually fine. The real concern is that the login_role value originates from REGROLE::text in PostgreSQL (which returns the canonical role name, not double-quoted), but the comparison on line 112 uses the raw string. If login_role == effective_role after both are resolved from REGROLE::text, the SET ROLE is skipped — this is correct behavior. No change needed here, but the get_host() function on line 97 checks !host.is_empty() which is always true since unwrap_or_else already returns a non-empty default. The condition is redundant and slightly misleading.
| if !host.is_empty() { | ||
| options = options.host(&host); | ||
| } |
There was a problem hiding this comment.
get_host() always returns a non-empty string: its unwrap_or_else fallback is "127.0.0.1".to_string(). The !host.is_empty() guard can never be false, so the options.host() call is always executed. If the intent is to allow Unix socket connections by omitting the host (e.g. when PGHOST is unset), the fallback should be an empty string rather than "127.0.0.1", or the condition should check PGHOST more directly. As written, the worker will always connect via TCP to 127.0.0.1, making Unix-socket-based peer auth impossible even if configured.
| if !host.is_empty() { | |
| options = options.host(&host); | |
| } | |
| options = options.host(&host); |
| if pgdata.contains(".pgrx") { | ||
| "28817".to_string() | ||
| } else { | ||
| "5432".to_string() | ||
| } | ||
| } else { | ||
| "28817".to_string() |
There was a problem hiding this comment.
When PGPORT is unset and PGDATA is also unset, the default port falls back to 28817 (the pgrx development port). In a standard PostgreSQL deployment where neither variable is set, this will cause connection failures because the worker will try port 28817 instead of the standard 5432. The safer fallback when PGDATA is absent should be 5432, not 28817.
| -- Wait for the instance to complete (should fail when trying to execute SQL node) | ||
| LOOP | ||
| SELECT status INTO final_status FROM df.instances WHERE id = inst_id; | ||
| EXIT WHEN lower(final_status) IN ('failed', 'completed') OR attempts > 100; |
There was a problem hiding this comment.
The loop polls up to 100 times with 0.1-second sleeps (10 seconds total), but Test 7 involves waiting for a 3-second df.sleep() node to start plus time for the SQL node to fail after the role is dropped. The same 10-second budget is shared across both the "wait for sleep node to start" loop (lines 375–384) and this "wait for instance to fail" loop. In a slow CI environment, the cumulative wait could exceed 10 seconds and the test would exit with final_status still in a running/pending state, hitting the final_status IS NULL branch (line 416) or causing a false failure. The outer wait for the node to start already consumes up to 10 seconds; the inner wait for completion should use a longer timeout (e.g. 300 attempts, matching the pattern in 26_superuser_scenarios.sql).
| EXIT WHEN lower(final_status) IN ('failed', 'completed') OR attempts > 100; | |
| EXIT WHEN lower(final_status) IN ('failed', 'completed') OR attempts > 300; |
|
This change was merged with #30 |
Summary
Implements user isolation for pg_durable: SQL in durable functions now executes with the privileges of the user who submitted them, not the background worker's superuser privileges.
Changes
Core Implementation (commit a819959)
submitted_byandlogin_rolecolumns (REGROLE type) todf.nodesanddf.instancesdf.start()capturesGetSessionUserId()(login_role) andGetOuterUserId()(submitted_by)connect_as_user()- connects as login_role, then SET ROLE to submitted_byexecute_sqlactivity receives JSON input with query + identity and creates isolated connectionE2E Testing (commit e9dc534)
df_e2e_userroleCode Review (commit 91a70e5)
Follow-up Improvements
Test Results
✅ All unit tests passing
✅ All E2E tests passing (44 tests)
Design Document
See docs/user-isolation.md for complete architecture and design rationale.
Security Considerations
What this protects:
Known limitations (acknowledged as future work):
df.varsshared across all usersdf.instances/df.nodes(cross-instance visibility)Review Findings
See docs/user-isolation-review.md for detailed analysis.
All blocking and strongly recommended items have been addressed:
Status: Ready for merge
Related
Part of the pre-release security hardening effort.