diff --git a/CHANGELOG.md b/CHANGELOG.md index d59b999b..ef68c24d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,47 @@ true until the next version shipped. `compare_to_bash.py` reports `PORT IS INCOMPLETE` for a complete port. Measured: 81 of 253 suites carry at least one interpolated check name. That bounds how much of #432's parity the tool can certify, and it is a false red rather than a false green. +- `stats_privilege.sh` has a pytest twin, and it asserts the SQLSTATE (#432). + + The bash suite decides the refusal with a grep on the message. `CLAUDE.md` names + the rule: 42501 comes only from `aclcheck_error`, while a grep for "permission + denied" is also satisfied by other refusals. + + WHICH ARMS ARE LOOSE, MEASURED RATHER THAN ASSERTED, because two rounds of review + narrowed this twice. The defect arm at `:82` uses `permission denied for table`, + which the schema message does NOT match -- so that arm is not confusable, and an + earlier version of this entry claiming otherwise was wrong. The two BARE greps are + at `:68` and `:70`, and both are premises: + + :68 premise: the no-privilege role cannot read it by ordinary SQL + :70 premise: the catalog tables are NOT readable by these roles + + So the risk is a premise satisfied for the wrong reason, which weakens what the + suite rests on, rather than a defect slipping through. The port asserts 42501 on + the premises and on the refusal, and that the refusal names the table. + + Real logins rather than `SET ROLE`, because session-opening is a property this + suite tests and `SET ROLE` would assert it away. + + SCOPE, MEASURED AND THEN CORRECTED. Across the corpus, 50 suites already assert a + refusal by SQLSTATE and 3 assert both. FIVE assert by text with no SQLSTATE + anywhere: `native_ownership`, `stats_privilege`, `projection_privilege`, + `rls_direct_storage` and `import_export_privilege`. Two are now ported; the class + closes at five, not at the whole corpus. + + My first sweep said four. It missed `import_export_privilege.sh:76` because the + flag class `grep -[qic]*i?` does not cover the `E` in `grep -qiE`, so the line + never matched and a fifth member stayed invisible while the output looked complete. + Found by @OffgridwithJD. Four sweeps in one day across two sessions have now failed + this way, each keyed on how something was NAMED or SPELLED rather than on content: + key on content, and when a sweep returns a tidy number, grep for one known-present + member and check the sweep found it. + + A HELPER TURNED A DRIVER DETAIL INTO A PRODUCT CLAIM. psycopg3 returns the FIRST + statement's result for a multi-statement execute, so `SET search_path ...; SELECT` + hands back the SET's empty result. The first version collapsed that into a 0 and + the arm reported "the OWNER cannot read its stats". Every call site now asserts the + error is None rather than folding it into a value. - The mutation ledger records WHICH MAJORS each check exists on (#1010). diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 3e15d33d..279e9317 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -77,6 +77,7 @@ behaviour, the source of that number is named. - [29. test_join_vector_agg.py: ungrouped fold over a unique-key join](#29-test_join_vector_aggpy-ungrouped-fold-over-a-unique-key-join) - [31. test_native_ownership.py: every maintenance function is owner-only](#31-test_native_ownershippy-every-maintenance-function-is-owner-only) - [30. test_differential.py: the heap oracle, all seven parts](#30-test_differentialpy-the-heap-oracle-all-seven-parts) +- [32. test_stats_privilege.py: stats is readable only by a caller who may read the table](#32-test_stats_privilegepy-stats-is-readable-only-by-a-caller-who-may-read-the-table) ## 1. How to read a test in here @@ -3116,3 +3117,42 @@ count diverge -- which is the arithmetic the metadata path has to get right and gets right for free. Run both ways through `enable_vectorization`, which is what distinguishes "the fast path is correct" from "the fast path was not taken". `SET` on the connection rather than the bash suite's `ALTER DATABASE`, which exists because each psql there is a new session. +## 32. test_stats_privilege.py: stats is readable only by a caller who may read the table + +Port of `test/stats_privilege.sh` (#560, ported for #432). + +`stats()` is SECURITY DEFINER and does its own privilege check, because GRANTing +SELECT on `pgcolumnar.zone_map` to PUBLIC would publish per-column minimum, maximum +and sum for every columnar table. + +**The trap the suite exists to catch.** Inside a SECURITY DEFINER function the +effective user is the function owner, so `pg_class_aclcheck(relid, GetUserId(), ...)` +checks the superuser who installed the extension and returns `ACLCHECK_OK` for every +relation. It looks like a correct check and refuses nobody. + +### What the port asserts that the bash suite cannot + +`stats_privilege.sh` decides the refusal with `grep -c 'permission denied for table'`. +`CLAUDE.md` names the rule: 42501 comes only from `aclcheck_error`, while a grep for +"permission denied" is also satisfied by other refusals. **`permission denied for +schema` matches it too**, which is not hypothetical — it is the confusion measured +while porting `native_ownership`. This port asserts 42501 **and** that the message +names the table, so neither half carries the arm alone. + +**Real logins, not `SET ROLE`.** Unlike the ownership port, session-opening is a +property this suite tests, so `SET ROLE` would assert it away. Each role connects. + +| test | what it pins | +| --- | --- | +| `test_the_premises_each_role_is_what_the_suite_assumes` | each premise run BY the role it is about | +| `test_who_may_read_the_stats` | owner, GRANTed reader and superuser all succeed | +| `test_a_role_with_no_privilege_is_refused` | the bar: 42501 **and** the table named | + +### A helper that turned a driver detail into a product claim + +psycopg3 returns the **first** statement's result for a multi-statement execute, so +`SET search_path ...; SELECT ...` hands back the SET's empty result. The first +version collapsed that into a 0 and the arm reported *"the OWNER cannot read its +stats"* — a product failure, from a driver behaviour. The helper now issues the SET +as its own execute, and every call site asserts the error is `None` rather than +folding it into a value. diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 8b5116f2..fda3537e 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -52,7 +52,11 @@ guard_tests 277 # which is the argument for gating it rather than a detail about it. # 166 -> 177 when test_native_ownership.py landed: nine parametrized refusal arms, # the owner control, and the check-ordering arm. -cluster_tests 202 +cluster_tests 205 +# 177 -> 180 when test_stats_privilege.py landed, on top of +# test_native_ownership.py's 166 -> 177. DERIVED by collection on the merged +# tree, not 169 + 11: both branches bumped this from 166 and the union left two +# lines, which is exactly the staleness #1018's enforcement exists to catch. # The rate is the point. The ungated half grew by 67 tests in the time #1016 took to review, # which was the argument for gating it. It is gated now, and 166 -> 182 is the first move made # with the gate actually watching: this number and the tests land in one commit. diff --git a/test/pytest/test_stats_privilege.py b/test/pytest/test_stats_privilege.py new file mode 100644 index 00000000..d1c588f3 --- /dev/null +++ b/test/pytest/test_stats_privilege.py @@ -0,0 +1,147 @@ +"""`pgcolumnar.stats` is readable by the table's owner, and only by a caller who +may read the table (#560, ported for #432). + +`stats()` is SECURITY DEFINER and does its own privilege check, because the +alternative -- GRANTing SELECT on `pgcolumnar.zone_map` to PUBLIC -- would publish +per-column minimum, maximum and sum for every columnar table. That is a larger +disclosure than the usability bug it fixes. + +THE TRAP THIS SUITE EXISTS TO CATCH. Inside a SECURITY DEFINER function the +effective user is the function OWNER, so `pg_class_aclcheck(relid, GetUserId(), ...)` +checks the superuser who installed the extension and returns ACLCHECK_OK for every +relation in the database. It looks like a correct check and refuses nobody. The +refusal arm below is what fails if `GetOuterUserId()` is ever changed back. + +WHAT THIS PORT ASSERTS THAT THE BASH SUITE CANNOT. `stats_privilege.sh` decides the +refusal with `grep -c 'permission denied for table'`. `CLAUDE.md` names the rule: + + Assert SQLSTATE, not error text: 42501 comes only from aclcheck_error, while a + grep for "permission denied" is also satisfied by a login FATAL, a missing + function (42883), a bad argument (22023), or a transaction-block refusal (25001). + +`permission denied for schema` also matches `permission denied`, which is not +hypothetical: it is the exact confusion measured while porting `native_ownership`, +where a missing schema grant produced a refusal that looked like the one under test. +This port asserts 42501 AND that the message names the table, so neither half can +carry the arm alone. + +REAL LOGINS, NOT `SET ROLE`. Unlike the ownership port, session-opening is a property +this suite tests -- "the owner can open a session" is one of its premises -- and +`SET ROLE` would assert it away. Each role connects. +""" + +import pytest + +ROWS = 500 +OWNER, NONE, SEL = "t_stowner", "t_stnone", "t_stsel" + + +def _as(cluster, role, sql, schema=None): + """Run one statement on a fresh connection as `role`, returning (rows, error). + + THE SET IS ITS OWN EXECUTE. psycopg3 returns the FIRST statement's result for a + multi-statement execute, so `SET search_path ...; SELECT ...` hands back the + SET's empty result and raises "the last operation didn't produce records". The + first version of this helper collapsed that into a 0 and the arm reported + "the OWNER cannot read its stats" -- a product claim, from a driver detail. + """ + import psycopg + + dsn = f"host=127.0.0.1 port={cluster.port} user={role} dbname=postgres" + try: + with psycopg.connect(dsn, autocommit=True) as conn: + with conn.cursor() as cur: + if schema: + cur.execute(f'SET search_path TO "{schema}", pgcolumnar, public') + cur.execute(sql) + return cur.fetchall(), None + except psycopg.Error as exc: + return None, exc + + +def _fixture(cur): + for r in (OWNER, NONE, SEL): + cur.execute(f"SELECT 1 FROM pg_roles WHERE rolname = '{r}'") + if cur.fetchone() is None: + cur.execute(f"CREATE ROLE {r} NOSUPERUSER LOGIN") + cur.execute(f"GRANT USAGE ON SCHEMA pgcolumnar TO {r}") + cur.execute("DROP TABLE IF EXISTS st_t") + cur.execute("CREATE TABLE st_t (id int, v text) USING pgcolumnar") + cur.execute(f"INSERT INTO st_t SELECT g, 'v'||g FROM generate_series(1,{ROWS}) g") + cur.execute("SELECT current_schema()") + schema = cur.fetchone()[0] + for r in (OWNER, NONE, SEL): + cur.execute(f'GRANT USAGE ON SCHEMA "{schema}" TO {r}') + cur.execute(f"ALTER TABLE st_t OWNER TO {OWNER}") + cur.execute("REVOKE ALL ON st_t FROM PUBLIC") + cur.execute(f"GRANT SELECT ON st_t TO {SEL}") + return schema + + +def test_the_premises_each_role_is_what_the_suite_assumes(pgc_cluster, pgc_conn, expect): + """Each premise is run BY the role it is about, which is why real logins matter.""" + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + for r in (OWNER, NONE, SEL): + rows, err = _as(pgc_cluster, r, 'SELECT 1', schema) + assert err is None, f"{r} could not connect: {err}" + expect.num(rows[0][0], 1, f"premise: {r} can open a session") + + with pgc_conn.cursor() as cur: + cur.execute("SELECT relowner::regrole::text FROM pg_class WHERE relname = 'st_t'") + expect.text(cur.fetchone()[0], OWNER, "premise: t_stowner really owns the table") + cur.execute( + "SELECT count(*) FROM pg_roles WHERE rolname = ANY(%s) AND rolsuper", + ([OWNER, NONE, SEL],), + ) + expect.num(cur.fetchone()[0], 0, "premise: none of these roles is a superuser") + + rows, err = _as(pgc_cluster, OWNER, 'SELECT count(*) FROM st_t', schema) + assert err is None, f"owner read failed: {err}" + expect.num(rows[0][0], ROWS, + "premise: the owner can read its own table by ordinary SQL") + + _, err = _as(pgc_cluster, NONE, 'SELECT count(*) FROM st_t', schema) + expect.sqlstate(err, "42501", + "premise: the no-privilege role cannot read it by ordinary SQL") + + _, err = _as(pgc_cluster, SEL, "SELECT count(*) FROM pgcolumnar.row_group") + expect.sqlstate(err, "42501", + "premise: the catalog tables are NOT readable by these roles, " + "so a GRANT is not the fix") + + +def test_who_may_read_the_stats(pgc_cluster, pgc_conn, expect): + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + q = "SELECT count(*) FROM pgcolumnar.stats('st_t')" + + rows, err = _as(pgc_cluster, OWNER, q, schema) + assert err is None, f"owner call failed: {type(err).__name__}: {err}" + expect.num(rows[0][0], 1, "the OWNER of the table can read its stats") + + rows, err = _as(pgc_cluster, SEL, q, schema) + assert err is None, f"granted role failed: {err}" + expect.num(rows[0][0], 1, "a role merely GRANTed select can read them too") + + with pgc_conn.cursor() as cur: + cur.execute("SELECT count(*) FROM pgcolumnar.stats('st_t')") + expect.num(cur.fetchone()[0], 1, "a superuser still reads stats") + + +def test_a_role_with_no_privilege_is_refused(pgc_cluster, pgc_conn, expect): + """THE BAR. With GetUserId() the check tests the superuser who owns the function, + passes, and this role reads the stats of a table it cannot read. + + Two assertions, because neither carries it alone: `42501` says a privilege check + refused rather than something else failing, and the table name says it was THIS + table's check rather than a catalog table the caller never asked about. The bash + suite has only the second, as text. + """ + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + _, err = _as(pgc_cluster, NONE, + "SELECT count(*) FROM pgcolumnar.stats('st_t')", schema) + expect.sqlstate(err, "42501", "a role with no privilege on the table is refused") + expect.num(int("st_t" in str(err)), 1, + "and the refusal names the TABLE, not a catalog table it never asked about")