Uh oh!
There was an error while loading. Please reload this page.
Add RLS support and fix permission checks - #2309
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive Row-Level Security (RLS) support and fixes permission checking for the Apache AGE extension. Previously, AGE only checked SELECT and INSERT permissions, bypassing PostgreSQL's privilege system for UPDATE and DELETE operations. Additionally, RLS policies were not enforced because AGE transforms all Cypher queries to CMD_SELECT, preventing PostgreSQL's rewriter from applying RLS policies.
Changes:
- Added ACL_DELETE and ACL_UPDATE permission checks for DELETE, SET, and REMOVE clause operations
- Implemented executor-level RLS enforcement for INSERT/UPDATE/DELETE operations with WITH CHECK and USING policies
- Added permission and RLS checks to startNode/endNode functions
- Comprehensive regression test suite covering both permission checks and RLS scenarios
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/include/executor/cypher_utils.h | Added SecurityQualContext struct and RLS function declarations |
| src/backend/parser/cypher_clause.c | Added permission tracking functions and integrated ACL checks into DELETE/SET/REMOVE clauses |
| src/backend/executor/cypher_utils.c | Core RLS implementation with policy setup and checking functions |
| src/backend/executor/cypher_set.c | Integrated RLS WITH CHECK and USING policies for UPDATE operations |
| src/backend/executor/cypher_merge.c | Added RLS WITH CHECK policies for MERGE operations |
| src/backend/executor/cypher_delete.c | Integrated RLS USING policies for DELETE and DETACH DELETE operations |
| src/backend/executor/cypher_create.c | Added RLS WITH CHECK policies for CREATE operations |
| src/backend/utils/adt/agtype.c | Added permission and RLS checks for startNode/endNode functions |
| regress/sql/security.sql | Comprehensive test suite for permissions and RLS |
| regress/expected/security.out | Expected output for security tests |
| Makefile | Added security test to regression test suite |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
480c1f2 to
d8d7be5Compare- Previously, age only set ACL_SELECT and ACL_INSERT in RTEPermissionInfo, bypassing pg's privilege checking for DELETE and UPDATE operations. - Additionally, RLS policies were not enforced because AGE uses CMD_SELECT for all Cypher queries, causing the rewriter to skip RLS policy application. Permission fixes: - Add ACL_DELETE permission flag for DELETE clause operations - Add ACL_UPDATE permission flag for SET/REMOVE clause operations - Recursively search RTEs including subqueries for permission info RLS support: - Implemented at executor level because age transforms all cypher queries to CMD_SELECT, so pg's rewriter never adds RLS policies for INSERT/UPDATE/DELETE operations. There isnt an appropriate rewriter hook to modify this behavior, so we do it in executor instead. - Add setup_wcos() to apply WITH CHECK policies at execution time for CREATE, SET, and MERGE operations - Add setup_security_quals() and check_security_quals() to apply USING policies for UPDATE and DELETE operations - USING policies silently filter rows (matching pg behavior) - WITH CHECK policies raise errors on violation - DETACH DELETE raises error if edge RLS blocks deletion to prevent dangling edges - Add permission checks and rls in startnode/endnode functions - Add regression tests Assisted-by AI idk
d8d7be5 to
617581bCompareUh oh!
There was an error while loading. Please reload this page.
- Previously, age only set ACL_SELECT and ACL_INSERT in RTEPermissionInfo, bypassing pg's privilege checking for DELETE and UPDATE operations. - Additionally, RLS policies were not enforced because AGE uses CMD_SELECT for all Cypher queries, causing the rewriter to skip RLS policy application. Permission fixes: - Add ACL_DELETE permission flag for DELETE clause operations - Add ACL_UPDATE permission flag for SET/REMOVE clause operations - Recursively search RTEs including subqueries for permission info RLS support: - Implemented at executor level because age transforms all cypher queries to CMD_SELECT, so pg's rewriter never adds RLS policies for INSERT/UPDATE/DELETE operations. There isnt an appropriate rewriter hook to modify this behavior, so we do it in executor instead. - Add setup_wcos() to apply WITH CHECK policies at execution time for CREATE, SET, and MERGE operations - Add setup_security_quals() and check_security_quals() to apply USING policies for UPDATE and DELETE operations - USING policies silently filter rows (matching pg behavior) - WITH CHECK policies raise errors on violation - DETACH DELETE raises error if edge RLS blocks deletion to prevent dangling edges - Add permission checks and rls in startnode/endnode functions - Add regression tests Assisted-by AI Resolved Conflicts: src/backend/executor/cypher_create.c src/backend/executor/cypher_delete.c src/backend/executor/cypher_merge.c src/backend/executor/cypher_set.c src/backend/executor/cypher_utils.c
- Previously, age only set ACL_SELECT and ACL_INSERT in RTEPermissionInfo, bypassing pg's privilege checking for DELETE and UPDATE operations. - Additionally, RLS policies were not enforced because AGE uses CMD_SELECT for all Cypher queries, causing the rewriter to skip RLS policy application. Permission fixes: - Add ACL_DELETE permission flag for DELETE clause operations - Add ACL_UPDATE permission flag for SET/REMOVE clause operations - Recursively search RTEs including subqueries for permission info RLS support: - Implemented at executor level because age transforms all cypher queries to CMD_SELECT, so pg's rewriter never adds RLS policies for INSERT/UPDATE/DELETE operations. There isnt an appropriate rewriter hook to modify this behavior, so we do it in executor instead. - Add setup_wcos() to apply WITH CHECK policies at execution time for CREATE, SET, and MERGE operations - Add setup_security_quals() and check_security_quals() to apply USING policies for UPDATE and DELETE operations - USING policies silently filter rows (matching pg behavior) - WITH CHECK policies raise errors on violation - DETACH DELETE raises error if edge RLS blocks deletion to prevent dangling edges - Add permission checks and rls in startnode/endnode functions - Add regression tests Assisted-by AI Resolved Conflicts: src/backend/executor/cypher_create.c src/backend/executor/cypher_delete.c src/backend/executor/cypher_merge.c src/backend/executor/cypher_set.c src/backend/executor/cypher_utils.c
- Previously, age only set ACL_SELECT and ACL_INSERT in RTEPermissionInfo, bypassing pg's privilege checking for DELETE and UPDATE operations. - Additionally, RLS policies were not enforced because AGE uses CMD_SELECT for all Cypher queries, causing the rewriter to skip RLS policy application. Permission fixes: - Add ACL_DELETE permission flag for DELETE clause operations - Add ACL_UPDATE permission flag for SET/REMOVE clause operations - Recursively search RTEs including subqueries for permission info RLS support: - Implemented at executor level because age transforms all cypher queries to CMD_SELECT, so pg's rewriter never adds RLS policies for INSERT/UPDATE/DELETE operations. There isnt an appropriate rewriter hook to modify this behavior, so we do it in executor instead. - Add setup_wcos() to apply WITH CHECK policies at execution time for CREATE, SET, and MERGE operations - Add setup_security_quals() and check_security_quals() to apply USING policies for UPDATE and DELETE operations - USING policies silently filter rows (matching pg behavior) - WITH CHECK policies raise errors on violation - DETACH DELETE raises error if edge RLS blocks deletion to prevent dangling edges - Add permission checks and rls in startnode/endnode functions - Add regression tests Assisted-by AI
- Previously, age only set ACL_SELECT and ACL_INSERT in RTEPermissionInfo, bypassing pg's privilege checking for DELETE and UPDATE operations. - Additionally, RLS policies were not enforced because AGE uses CMD_SELECT for all Cypher queries, causing the rewriter to skip RLS policy application. Permission fixes: - Add ACL_DELETE permission flag for DELETE clause operations - Add ACL_UPDATE permission flag for SET/REMOVE clause operations - Recursively search RTEs including subqueries for permission info RLS support: - Implemented at executor level because age transforms all cypher queries to CMD_SELECT, so pg's rewriter never adds RLS policies for INSERT/UPDATE/DELETE operations. There isnt an appropriate rewriter hook to modify this behavior, so we do it in executor instead. - Add setup_wcos() to apply WITH CHECK policies at execution time for CREATE, SET, and MERGE operations - Add setup_security_quals() and check_security_quals() to apply USING policies for UPDATE and DELETE operations - USING policies silently filter rows (matching pg behavior) - WITH CHECK policies raise errors on violation - DETACH DELETE raises error if edge RLS blocks deletion to prevent dangling edges - Add permission checks and rls in startnode/endnode functions - Add regression tests Assisted-by AI
- Previously, age only set ACL_SELECT and ACL_INSERT in RTEPermissionInfo, bypassing pg's privilege checking for DELETE and UPDATE operations. - Additionally, RLS policies were not enforced because AGE uses CMD_SELECT for all Cypher queries, causing the rewriter to skip RLS policy application. Permission fixes: - Add ACL_DELETE permission flag for DELETE clause operations - Add ACL_UPDATE permission flag for SET/REMOVE clause operations - Recursively search RTEs including subqueries for permission info RLS support: - Implemented at executor level because age transforms all cypher queries to CMD_SELECT, so pg's rewriter never adds RLS policies for INSERT/UPDATE/DELETE operations. There isnt an appropriate rewriter hook to modify this behavior, so we do it in executor instead. - Add setup_wcos() to apply WITH CHECK policies at execution time for CREATE, SET, and MERGE operations - Add setup_security_quals() and check_security_quals() to apply USING policies for UPDATE and DELETE operations - USING policies silently filter rows (matching pg behavior) - WITH CHECK policies raise errors on violation - DETACH DELETE raises error if edge RLS blocks deletion to prevent dangling edges - Add permission checks and rls in startnode/endnode functions - Add regression tests Assisted-by AI
- Previously, age only set ACL_SELECT and ACL_INSERT in RTEPermissionInfo, bypassing pg's privilege checking for DELETE and UPDATE operations. - Additionally, RLS policies were not enforced because AGE uses CMD_SELECT for all Cypher queries, causing the rewriter to skip RLS policy application. Permission fixes: - Add ACL_DELETE permission flag for DELETE clause operations - Add ACL_UPDATE permission flag for SET/REMOVE clause operations - Recursively search RTEs including subqueries for permission info RLS support: - Implemented at executor level because age transforms all cypher queries to CMD_SELECT, so pg's rewriter never adds RLS policies for INSERT/UPDATE/DELETE operations. There isnt an appropriate rewriter hook to modify this behavior, so we do it in executor instead. - Add setup_wcos() to apply WITH CHECK policies at execution time for CREATE, SET, and MERGE operations - Add setup_security_quals() and check_security_quals() to apply USING policies for UPDATE and DELETE operations - USING policies silently filter rows (matching pg behavior) - WITH CHECK policies raise errors on violation - DETACH DELETE raises error if edge RLS blocks deletion to prevent dangling edges - Add permission checks and rls in startnode/endnode functions - Add regression tests Assisted-by AI
…licy (#2475) A non-superuser subject to row-level security crashed the backend with SIGSEGV when running DETACH DELETE on a vertex that has a connected edge, if the edge label carried an RLS policy whose USING/WITH CHECK qual invokes a function (e.g. a STABLE tenant/owner accessor). Reported in issue #2474; reproduces on 1.7.0 and master. Mechanism: AGE deletes a vertex's connected edges in check_for_connected_edges(), which is called from end_cypher_delete() -- i.e. during executor shutdown (ExecEndPlan, reached via PortalCleanup -> ExecutorEnd when the portal is dropped). By that point the portal's active snapshot has already been popped, so the active-snapshot stack is empty. RLS for Cypher DELETE is enforced at the executor level (added in #2309): check_for_connected_edges() compiles the edge label's security quals and evaluates them per candidate edge with check_security_quals() -> ExecQual(). When the qual calls a SQL-language function, ExecQual() dispatches into fmgr_sql() -> postquel_start(), which runs the function's query and reads the current snapshot with GetActiveSnapshot(). With no snapshot on the stack that is a NULL-pointer dereference -> SIGSEGV (an assert build trips the Assert(ActiveSnapshotSet()) in postquel_start first). This is why the narrowing in the report holds: the vertex path (process_delete_list) runs during normal execution while a snapshot is active; edge-only DELETE and node-only DETACH DELETE never evaluate an edge qual at teardown; a superuser bypasses RLS; and a constant-only edge policy never enters fmgr_sql, so only DETACH DELETE of an edge-connected vertex under a function-bearing edge policy trips it. Fix: Ensure an active snapshot for the duration of the connected-edge scan in check_for_connected_edges(). es_snapshot is still valid there (the EState is not torn down until this returns) and is the correct snapshot for reading the edges, so push it if none is active and pop it before return. The scans themselves already pass es_snapshot explicitly, so this only affects RLS qual evaluation; non-RLS paths are unchanged. On the error path (e.g. an RLS denial raised mid-scan) transaction abort resets the active-snapshot stack, so the unpaired push is cleaned up. Test: Adds a regression to the security suite (PART 11b): an edge label policy whose qual calls a STABLE SQL function, then DETACH DELETE of an edge-connected vertex as the RLS-bound role. It must delete the vertex and its edge (leaving the other endpoint) instead of crashing. The existing PART 11 uses a constant-only edge policy, which does not exercise the snapshot-dependent function path.
Previously, age only set ACL_SELECT and ACL_INSERT in RTEPermissionInfo, bypassing pg's privilege checking for DELETE and UPDATE operations. Additionally, RLS policies were not enforced because AGE uses CMD_SELECT for all Cypher queries, causing the rewriter to skip RLS policy application.
Permission fixes:
RLS support:
Assisted-by AI