MuhammadTahaNaveed commented on code in PR #2585:
URL: https://github.com/apache/age/pull/2585#discussion_r4251363324
##########
src/backend/utils/graph_generation.c:
##########
@@ -20,8 +20,42 @@
#include "postgres.h"
#include "access/genam.h"
+#include "catalog/pg_namespace.h"
+#include "miscadmin.h"
#include "commands/graph_commands.h"
+#include "utils/acl.h"
#include "utils/load/age_load.h"
+#include "utils/rls.h"
+
+/* The generators insert tuples directly, without the executor's ACL/RLS
checks. */
+static void check_generation_insert(Oid relid, int columns)
+{
+ if (pg_class_aclcheck(relid, GetUserId(), ACL_INSERT) != ACLCHECK_OK)
+ {
+ int attnum;
+
+ /* INSERT on every written column is also sufficient in PostgreSQL. */
+ for (attnum = 1; attnum <= columns; attnum++)
+ {
+ AclResult aclresult = pg_attribute_aclcheck(relid, attnum,
+ GetUserId(),
ACL_INSERT);
+
+ if (aclresult != ACLCHECK_OK)
+ {
+ aclcheck_error(aclresult, OBJECT_TABLE, get_rel_name(relid));
+ }
+ }
+ }
+
+ /* These direct writes cannot evaluate INSERT WITH CHECK policies. */
+ if (check_enable_rls(relid, InvalidOid, true) == RLS_ENABLED)
Review Comment:
Please lock both label tables before checking RLS and retain the locks
through the writes. There is no table lock here, so RLS can change while the
generator waits to insert.
Reproduced on PostgreSQL 18.3:
Setup as an administrator:
```sql
CREATE EXTENSION IF NOT EXISTS age;
LOAD 'age';
SET search_path = ag_catalog, public;
SELECT create_graph('rls_repro');
SELECT create_vlabel('rls_repro', 'Node');
SELECT create_elabel('rls_repro', 'LINK');
CREATE ROLE rls_writer NOSUPERUSER NOBYPASSRLS;
GRANT USAGE ON SCHEMA ag_catalog, rls_repro TO rls_writer;
GRANT USAGE ON ALL SEQUENCES IN SCHEMA rls_repro TO rls_writer;
GRANT INSERT ON rls_repro."Node", rls_repro."LINK" TO rls_writer;
```
Session A:
```sql
BEGIN;
LOCK TABLE rls_repro."Node" IN ACCESS EXCLUSIVE MODE;
```
Session B — leave the SELECT waiting:
```sql
LOAD 'age';
SET search_path = ag_catalog, public;
SET ROLE rls_writer;
SELECT create_complete_graph('rls_repro', 2, 'LINK', 'Node');
```
Then in session A:
```sql
ALTER TABLE rls_repro."Node" ENABLE ROW LEVEL SECURITY;
ALTER TABLE rls_repro."Node" FORCE ROW LEVEL SECURITY;
COMMIT;
```
Session B succeeds and commits two vertices and one edge, despite RLS having
no INSERT policy. A normal SQL INSERT under the same lock/RLS sequence
correctly fails. Please cover this with a two-session regression.
##########
regress/sql/graph_generation_security.sql:
##########
@@ -0,0 +1,65 @@
+/* Graph generators must respect table INSERT privileges and RLS. */
Review Comment:
Please move these cases into `regress/sql/graph_generation.sql` and its
expected output. It already covers both generators; there is no need for a
separate test file and Makefile entry.
##########
tools/graph-generation-security/reproduce.py:
##########
@@ -0,0 +1,104 @@
+#!/usr/bin/env python3
Review Comment:
These checks can go in the SQL regression tests using `SET ROLE`. The Python
script isn’t run by CI and only prints results.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
