diff --git a/CHANGELOG.md b/CHANGELOG.md index ae42c092..d6f5579e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,59 @@ true until the next version shipped. ### Added +- `projection_privilege.sh` has a pytest twin, and both halves now attribute a refusal + by SQLSTATE instead of by error text (#432, #562, #563). + + The bash suite decided four things by matching the message, and two of them were + load-bearing rather than decorative. Its own comment says why: both ACL layers raise + 42501, so a bare "refused" stays true if the SQL `REVOKE` is deleted and the C check + catches it instead. Measured there: with the `REVOKE` removed the suite still passed + 14 of 14. + + The fixture is what separates them, not the wording. Called with a projection name + that does not exist, on a table the role may read, a caller stopped by the SQL grant + never runs the body and gets 42501; one that gets past the grant reaches the lookup + and gets 42704. The refusal is attributed by what the code REACHED. RLS is a third + code again, `0A000` from `ERRCODE_FEATURE_NOT_SUPPORTED`, which is a different + SQLSTATE class from either ACL refusal. + + Two orderings that `src/columnar_projection.c` and `src/columnar_vacuum.c` assert had + no test in either harness: the base ACL is checked before the projection is looked + up, so a caller with no SELECT cannot learn whether a projection exists on a table it + may not read; and the ACL is checked before RLS, so a caller with no privilege is not + told the table has row-level security enabled. The second is a correction the source + records being made in review. + + Five mutations, each asserted to apply at both call sites. Moving the ACL check below + the projection lookup but above its raise reddens nothing, correctly -- 42501 still + wins. Moving it below the raise reddens both ordering arms, and the shell half prints + the disclosure: `got [42704] want [42501]`. + +- `compare_to_bash.py` reads the assertion's NAME (#432, #897). + + The parity tool decides whether a port is one-for-one with its bash suite, which is + #432's definition of done, and it was reading the wrong argument. The python side was + matched with `expect\.\w+\([^)]*?"([^"]+)"`, whose lazy `[^)]*?` stops at the FIRST + quoted argument. For `expect.num(got, 1, NAME)` that is the name, so the tool looked + correct. For `expect.sqlstate(err, "42501", NAME)` it is `42501`. + + Every SQLSTATE assertion was therefore read as the literal `42501`, reported as an + "extra" name the bash suite does not have, while the real property was reported + MISSING. #432's ports are exactly the ones replacing a grep on a message with a + SQLSTATE assertion, so the tool went blind in proportion to the work being done well. + + It parses with `ast` now and takes the last string argument, resolving f-strings to + templates, both arms of a conditional, and the `name` column of a + `@pytest.mark.parametrize`. Bash interpolations reduce to the same template, including + `$1`, which is the commonest one in a check name and which the first version of the + reducer missed because its pattern required a letter after the dollar. + + Measured over every pair in the tree: **61 bash properties reported missing, now 0.** + 34 were never missing. The rest were real and are closed here: `stats_privilege` had + invented a name for a property the bash suite already named, and `zonemap_boundaries` + was missing its `backend alive` premise outright. Neither was visible while the tool + was reporting the wrong string. + - A UNIQUE-constraint check passed on any psql failure, and a recursive sweep passed on a tree it never read (#1033). diff --git a/test/projection_privilege.sh b/test/projection_privilege.sh index 6669f70a..63b7defc 100755 --- a/test/projection_privilege.sh +++ b/test/projection_privilege.sh @@ -67,6 +67,15 @@ for r in t_prjexec t_prjsel; do done psql_run "GRANT SELECT ON secret TO t_prjsel;" +# A table EVERY role may read. It carries no projection, which is what lets the +# arms below attribute a refusal to a LAYER without reading the message: a caller +# stopped by the SQL grant never reaches the projection lookup, and one that gets +# past the grant does. See test/pytest/test_projection_privilege.py, which decides +# the same two questions the same way in a different language. +psql_run "CREATE TABLE prjopen (id int) USING pgcolumnar;" +psql_run "INSERT INTO prjopen SELECT generate_series(1,10);" +psql_run "GRANT SELECT ON prjopen TO t_prjnone, t_prjexec, t_prjsel;" + as() { # as env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U "$1" \ -d "$PGC_DB" -At -c "$2" 2>&1 @@ -82,6 +91,22 @@ count_as() { # count_as esac } +# state_as -> the SQLSTATE, or the literal noerror. +# +# VERBOSITY is set with -v, NOT with -c "\\set ...". psql treats a -c argument +# beginning with a backslash as a meta-command and takes a different code path, +# which is how a sibling suite ended up with deny arms that could never go green. +# The SQLSTATE is read from the ERROR line because psql prefixes it, and it is +# extracted into a variable rather than tested through a pipeline whose STATUS +# would be the answer. +state_as() { # state_as + local out _sqlstate + out="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U "$1" \ + -d "$PGC_DB" -At -v VERBOSITY=sqlstate -v ON_ERROR_STOP=0 -c "$2" 2>&1)" + _sqlstate="$(sed -n 's/^.*ERROR:[[:space:]]*\([0-9A-Z]\{5\}\).*$/\1/p' <<<"$out" | head -1)" + if [ -n "$_sqlstate" ]; then printf '%s\n' "$_sqlstate"; else echo noerror; fi +} + # ---- premises: the fixture, and that the functions WORK ---------------------- # # The owner arms are not decoration. A DENIED result and a BROKEN result are @@ -125,11 +150,19 @@ check "and is refused reconstruct_via_projection" \ # WHICH layer refused, not merely that one did. Both layers reject this role, so # a bare "refused" stays true if the REVOKE is deleted and the C check catches it # instead -- measured: with the REVOKE removed this suite still passed 14 of 14. -# The error text is the only thing that attributes the refusal, so it is asserted -# here to tell the layers apart rather than in place of behaviour. -check "and the refusal comes from the SQL grant, naming the function" \ - "$(as t_prjnone "SELECT count(*) FROM pgcolumnar.read_projection('secret','p1');" | grep -c 'permission denied for function')" \ - "1" +# +# THE ERROR TEXT USED TO BE THE ONLY THING THAT COULD ATTRIBUTE IT, because both +# layers raise 42501. It is not, once the call names a table every role may read +# and a projection that does not exist: a caller stopped by the SQL grant never +# runs the body, so it never reaches the lookup, and one that gets past the grant +# does. The two outcomes are then different SQLSTATEs and the wording is free to +# change without moving the arm. +check "the no-EXECUTE role never reaches the body, so the grant is what stopped it" \ + "$(state_as t_prjnone "SELECT count(*) FROM pgcolumnar.read_projection('prjopen','no_such_proj');")" \ + "42501" +check "while the EXECUTE role reaches the projection lookup on the same call" \ + "$(state_as t_prjexec "SELECT count(*) FROM pgcolumnar.read_projection('prjopen','no_such_proj');")" \ + "42704" # ---- layer two: the C check, reached only because EXECUTE was granted -------- # @@ -150,9 +183,20 @@ check "a role with EXECUTE but no SELECT is refused read_projection" \ "$(count_as t_prjexec read_projection)" "refused" check "and is refused reconstruct_via_projection, which leaks non-covered columns" \ "$(count_as t_prjexec reconstruct_via_projection)" "refused" -check "and THAT refusal names the table, so it is the C check and not the grant" \ - "$(as t_prjexec "SELECT count(*) FROM pgcolumnar.read_projection('secret','p1');" | grep -c 'permission denied for table')" \ - "1" +check "and THAT refusal is the C check, which raises 42501 from aclcheck_error" \ + "$(state_as t_prjexec "SELECT count(*) FROM pgcolumnar.read_projection('secret','p1');")" \ + "42501" + +# AN ORDERING THE SOURCE ASSERTS AND NOTHING TESTED. The ACL check is the first +# statement of the body; the projection lookup is well below it. Swapped, a caller +# with no SELECT would learn whether a named projection exists on a table it may +# not read -- existence disclosure, from a function whose purpose is to stop +# disclosure. The arm above is the control: the same role, the same bogus name, on +# a table it MAY read, returns 42704, so 42501 here is the ACL check winning rather +# than the lookup being unreachable. +check "the base ACL is checked before the projection name is looked up" \ + "$(state_as t_prjexec "SELECT count(*) FROM pgcolumnar.read_projection('secret','no_such_proj');")" \ + "42501" # ---- the bar is SELECT, not ownership --------------------------------------- # @@ -185,8 +229,25 @@ check "read_projection now refuses a policy-restricted caller (#563)" \ "$(count_as t_prjsel read_projection)" "refused" check "and so does reconstruct_via_projection" \ "$(count_as t_prjsel reconstruct_via_projection)" "refused" -check "and the refusal names row-level security, not the table ACL" \ - "$(as t_prjsel "SELECT count(*) FROM pgcolumnar.read_projection('secret','p1');" | grep -c '^ERROR:.*row-level security')" \ - "1" +# 0A000, not a phrase. PgColumnarRequireNoRowSecurity raises +# ERRCODE_FEATURE_NOT_SUPPORTED, which is a different SQLSTATE CLASS from either +# ACL refusal -- 0A against 42 -- so no rewording of any message can confuse them. +check "and the refusal says the feature is not supported, not that a privilege is missing" \ + "$(state_as t_prjsel "SELECT count(*) FROM pgcolumnar.read_projection('secret','p1');")" \ + "0A000" + +# THE SECOND ORDERING, recorded in src/columnar_vacuum.c as a correction made in +# review. RLS is checked AFTER the relation ACL. With it first, a caller holding +# no privilege at all was told the table has RLS enabled, which ordinary SQL does +# not disclose and which disagrees with core. Measured there: +# +# RLS on, no-select, ordinary SQL -> permission denied for table +# RLS on, no-select, read_projection -> row-level security is in force +# +# The arm above is its control: same table, same policy, a role that DOES hold +# SELECT, and 0A000. So 42501 here is the ordering, not RLS being switched off. +check "a caller with no SELECT is told a privilege is missing, not that RLS is in force" \ + "$(state_as t_prjexec "SELECT count(*) FROM pgcolumnar.read_projection('secret','p1');")" \ + "42501" pgc_summary diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index d6891b7f..2c88f5c1 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -80,6 +80,8 @@ behaviour, the source of that number is named. - [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) - [33. test_docs_table_structure.py: a table must stay a table](#33-test_docs_table_structurepy-a-table-must-stay-a-table) - [34. test_docs_stripe_floor.py: the stripe floor is below a vector](#34-test_docs_stripe_floorpy-the-stripe-floor-is-below-a-vector) +- [35. test_projection_privilege.py: the projection read helpers are a privilege boundary](#35-test_projection_privilegepy-the-projection-read-helpers-are-a-privilege-boundary) +- [36. test_compare_to_bash.py: the parity tool reads the NAME](#36-test_compare_to_bashpy-the-parity-tool-reads-the-name) ## 1. How to read a test in here @@ -3274,3 +3276,150 @@ anything. The shell twin is three arms in `docs_style.sh`: `grep` for the two one-line pages and an awk heading walker for `administration.md`. The two halves share no code. + +## 35. test_projection_privilege.py: the projection read helpers are a privilege boundary + +`read_projection()` and `reconstruct_via_projection()` opened a caller-supplied regclass +and returned its contents with no privilege check, and `CREATE FUNCTION` grants EXECUTE to +PUBLIC. `reconstruct` rebuilds NON-COVERED columns from the base by row number, so the +projection was never the bound on what leaked: one projection on any column exposed the +whole row (#562). A caller holding SELECT but restricted by an RLS policy received every +row (#563). + +### Four refusals, four SQLSTATEs, and why the bash suite needed the message + +`projection_privilege.sh` decided four things by matching error text, and two of them were +load-bearing rather than decorative. Its own comment says why: + +> Both layers reject this role, so a bare "refused" stays true if the REVOKE is deleted and +> the C check catches it instead -- measured: with the REVOKE removed this suite still +> passed 14 of 14. + +Both ACL layers raise 42501, so the code alone does not separate them. What separates them +is the fixture: + +| the caller | the call | outcome | +| --- | --- | --- | +| no EXECUTE | a projection that does not exist, on a table it MAY read | `42501` -- the body never ran | +| EXECUTE | the same call | `42704` -- it ran and reached the lookup | +| EXECUTE, no SELECT | a real projection on a table it may NOT read | `42501` -- the base ACL | +| SELECT, under a policy | the same | `0A000` -- `ERRCODE_FEATURE_NOT_SUPPORTED` | + +The refusal is attributed by what the code REACHED. That is a fact about execution, and no +rephrasing of either message can move it. Both harnesses now do this: the shell half reads +the SQLSTATE through `psql -v VERBOSITY=sqlstate`. + +### Two orderings the source asserts and nothing tested + +| arm | what breaks without it | +| --- | --- | +| `test_the_base_acl_is_checked_before_the_projection_is_looked_up` | a caller with no SELECT learns whether a named projection exists on a table it may not read | +| `test_the_acl_is_checked_before_rls_so_no_privilege_discloses_no_rls_state` | a caller with no privilege is told the table has RLS enabled, which ordinary SQL does not disclose | + +### Every arm + +| test | what it holds | +| --- | --- | +| `test_the_premises_the_fixture_is_what_the_suite_assumes` | each role opens its own session, the owner gets rows from both helpers, and reconstruct really does return the non-covered column | +| `test_a_role_with_only_schema_usage_is_refused` | layer one, the SQL grant, per function | +| `test_a_role_with_execute_but_no_select_is_refused` | layer two, the C check, reached only because EXECUTE was granted | +| `test_which_layer_refused_without_reading_the_message` | which of the two, decided by what the code reached rather than by wording | +| `test_the_base_acl_is_checked_before_the_projection_is_looked_up` | the first ordering | +| `test_a_role_with_select_still_reads` | THE CONTROL: a bar that refused everyone is not a fix | +| `test_a_policy_restricted_caller_is_refused` | RLS, as `0A000` rather than a phrase | +| `test_the_acl_is_checked_before_rls_so_no_privilege_discloses_no_rls_state` | the second ordering | + +The second is a correction `src/columnar_vacuum.c` records being made in review. Neither +had a test in either harness. + +### Removal proof + +Five mutations, each asserted to apply at both call sites before the run: + +| mutation | pytest | shell | +| --- | --- | --- | +| drop the base ACL check | 4 arms red | -- | +| drop the RLS refusal | 2 arms red | -- | +| RLS **before** the ACL check | the RLS-ordering arm alone | the RLS-ordering arm alone | +| ACL below the projection lookup but above its raise | no arm red, correctly: 42501 still wins | same | +| ACL below the not-found **raise** | both ordering arms | both, reporting `got [42704] want [42501]` | + +The fourth row is the one that says the ordering arms measure an ordering rather than the +presence of a check. The fifth is the disclosure itself, printed. + +## 36. test_compare_to_bash.py: the parity tool reads the NAME + +`compare_to_bash.py` decides whether a port is one-for-one with its bash suite, which is +#432's definition of done. It was reading the wrong argument. + +The python side was matched with `expect\.\w+\([^)]*?"([^"]+)"...`, and `[^)]*?` is lazy, +so it stopped at the FIRST quoted argument: + +| call | name read | +| --- | --- | +| `expect.num(got, 1, NAME)` | `NAME` -- correct, which is why it looked right | +| `expect.sqlstate(err, "42501", NAME)` | `"42501"` | +| `expect.text(got, "none", NAME)` | `"none"` | + +So every SQLSTATE assertion was read as the literal `42501`, reported as an "extra" the +bash suite lacks, while the real property was reported MISSING. #432's ports are exactly +the ones replacing a grep on a message with a SQLSTATE assertion, so **the tool went blind +in proportion to the work being done well.** + +### Measured over the tree + +| pair | missing before | after | +| --- | --- | --- | +| differential | 6 | 0 | +| hilbert_locality | 13 | 0 | +| native_ownership | 1 | 0 | +| native_projection | 0 | 0 | +| projection_privilege | 23 | 0 | +| stats_privilege | 9 | 0 | +| zonemap_boundaries | 9 | 0 | +| **total** | **61** | **0** | + +Of the 61, 34 were never missing. The rest were real, and four of them are closed here: +`stats_privilege` had invented a name for a property the bash suite already named, and +`zonemap_boundaries` was missing its liveness premise outright. Neither was visible while +the tool was reporting the wrong string. + +### What the parser reads + +| shape | read as | +| --- | --- | +| the last string argument | the name | +| an f-string | a `{}` template, matched against bash interpolations reduced the same way | +| `"a" if cond else "b"` | both arms | +| `@pytest.mark.parametrize("func,name", ROWS)` | the `name` column, resolved through module constants | +| anything else | nothing -- absent is better than wrong | + +`$1` is the commonest interpolation in a bash check name and the first version of the +template reducer missed every one of them, because its pattern required `[A-Za-z_]` after +the dollar. + +### Removal proof + +| mutation | red | +| --- | --- | +| take the first string argument, as the regex did | the regression arm, the unreadable-name arm, and the whole-tree arm | +| drop the conditional-name case | its own arm, and the whole-tree arm | +| drop parametrize resolution | its own arm, and the whole-tree arm | +| read the name column by position instead of by its declared name | its own arm, and the whole-tree arm | + +`test_the_ported_suites_in_this_tree_are_graded_one_for_one` catches all four. It is the +arm that matters: a guard over invented sources proves the extractor reads python, not that +the tool grades THIS tree. + +### Every arm + +| test | what it holds | +| --- | --- | +| `test_the_name_is_the_last_argument_not_the_first_string` | the regression, over three helpers, one of which always worked | +| `test_a_call_whose_name_is_not_a_literal_contributes_nothing` | absent beats wrong: a false green on a parity tool loses a property in both harnesses | +| `test_an_fstring_name_becomes_a_template` | a runtime-built name is compared by shape | +| `test_a_conditional_name_carries_both_of_its_arms` | `"a" if c else "b"` states two properties | +| `test_a_parametrized_name_is_resolved_from_the_decorator` | the idiom a repeated bash property should be ported to, with a no-`name` decorator as the control | +| `test_the_parametrize_reader_takes_the_column_called_name` | the declared column, not position | +| `test_the_two_harnesses_interpolations_land_on_one_template` | bash and python spell interpolation differently and must meet | +| `test_the_ported_suites_in_this_tree_are_graded_one_for_one` | the standing arm: every pair in the tree, graded | diff --git a/test/pytest/compare_to_bash.py b/test/pytest/compare_to_bash.py index 875b9552..f7f03178 100755 --- a/test/pytest/compare_to_bash.py +++ b/test/pytest/compare_to_bash.py @@ -10,43 +10,225 @@ The port makes this possible by passing each assertion the SAME name string the bash check uses. That is a convention the port must keep, so this script is also what enforces it. + +THE PYTHON SIDE IS PARSED, NOT MATCHED (#432, #897) +--------------------------------------------------- + +This read the pytest file with a regex and got the wrong argument: + + expect.sqlstate(err, "42501", "a role with no privilege is refused") + ^^^^^^^ reported as the assertion's name + +`[^)]*?"([^"]+)"` is lazy, so it stops at the FIRST quoted argument. For +`expect.num(got, 1, "name")` that happens to be the name and the tool looked +correct. For any helper whose WANT is itself a string it is not: + + expect.sqlstate(err, "42501", NAME) -> "42501" + expect.text(got, "none", NAME) -> "none" + +So every SQLSTATE assertion was read as the literal `42501`, counted as an +"extra" name the bash suite does not have, and the real property was reported +MISSING. That is a false red aimed squarely at the ports #432 exists to produce, +which are the ones replacing a `grep` on an error message with a SQLSTATE. The +tool got blinder as the work it grades got better. + +It is parsed with `ast` now, and the name is the LAST string argument of the +call, which is the convention every port already follows. + +INTERPOLATED NAMES ARE MATCHED AS TEMPLATES +------------------------------------------- + +Both harnesses build some names at runtime, bash as `non-owner refused: ${1%%(*}` +and pytest as an f-string. Neither can be expanded without running the suite, so +this compares the SHAPE: every interpolation on both sides becomes `{}`, and two +names match when their templates do. + +That is a weaker claim than a literal match and it is reported separately rather +than folded in, because a template match says the two harnesses assert a property +of the same shape, not that they assert it over the same values. A port should +still prefer literal names. + +Exit status is 1 when a bash property has no counterpart of either kind. """ +import ast import re import sys -bash_file, py_file = sys.argv[1], sys.argv[2] - -# bash: check "NAME" ... / check_num "NAME" ... / check_ratio "NAME" ... -bash_src = open(bash_file).read() -bash_names = re.findall(r'\bcheck(?:_num|_ratio|_text|_timing)?\s+"([^"]+)"', bash_src) - -# pytest: expect.(..., "NAME") and name="NAME" -py_src = open(py_file).read() -py_names = re.findall(r'expect\.\w+\([^)]*?"([^"]+)"\s*(?:,[^)]*)?\)', py_src, re.S) -py_names += re.findall(r'name\s*=\s*"([^"]+)"', py_src) - -bset, pset = set(bash_names), set(py_names) - -print(f"bash checks: {len(bash_names)} ({len(bset)} distinct)") -print(f"pytest named assertions: {len(py_names)} ({len(pset)} distinct)") -print() - -missing = sorted(bset - pset) -extra = sorted(pset - bset) - -print("PROPERTIES IN THE BASH SUITE AND NOT IN THE PORT:") -if missing: - for n in missing: - print(f" MISSING {n}") -else: - print(" none -- every bash property is asserted by name in the port") -print() -print("ASSERTIONS IN THE PORT AND NOT IN THE BASH SUITE:") -if extra: - for n in extra: - print(f" extra {n}") -else: - print(" none") -print() -print("VERDICT:", "PORT IS INCOMPLETE" if missing else "every bash property is covered") -sys.exit(1 if missing else 0) + +def _parametrized_names(tree): + """-> every name supplied by a `@pytest.mark.parametrize` that declares one. + + A port of a suite whose bash half repeats a property per function writes the arm + once and parametrises it, carrying the bash name as a parameter: + + @pytest.mark.parametrize("func,name", USAGE_ONLY) + def test_a_role_with_only_schema_usage_is_refused(..., func, name): + expect.sqlstate(err, "42501", name) + + The name reaching `expect` is then a variable, and reading only the call site + reports every such property MISSING -- which would push a port AWAY from the one + idiom that keeps the two harnesses one-to-one across a repeated property. + + Only the column actually called `name` is read, resolved through module-level + constants, so the other parameters of the same decorator contribute nothing. + """ + consts = {} + for node in tree.body: + if isinstance(node, ast.Assign) and len(node.targets) == 1 and \ + isinstance(node.targets[0], ast.Name): + try: + consts[node.targets[0].id] = ast.literal_eval(node.value) + except (ValueError, TypeError, SyntaxError): + pass + + out = [] + for node in ast.walk(tree): + if not (isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) + and node.func.attr == "parametrize" and len(node.args) >= 2): + continue + argnames = _as_names(node.args[0]) + if not argnames: + continue + cols = [c.strip() for c in argnames[0].split(",")] + if "name" not in cols: + continue + idx = cols.index("name") + + values = node.args[1] + if isinstance(values, ast.Name): + rows = consts.get(values.id) + else: + try: + rows = ast.literal_eval(values) + except (ValueError, TypeError, SyntaxError): + rows = None + if rows is None: + continue + for row in rows: + if len(cols) == 1: + cell = row + elif isinstance(row, (tuple, list)) and len(row) > idx: + cell = row[idx] + else: + continue + if isinstance(cell, str): + out.append(cell) + return out + + +def _py_names(src): + """Every assertion name in the port, by parsing rather than matching. + + The name is the LAST argument of an `expect.(...)` call, or the value of + a `name=` keyword, read through `_as_names` so a conditional carries both of + its arms. + """ + tree = ast.parse(src) + out = _parametrized_names(tree) + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + func = node.func + is_expect = (isinstance(func, ast.Attribute) + and isinstance(func.value, ast.Name) + and func.value.id == "expect") + for kw in node.keywords: + if kw.arg == "name": + out.extend(_as_names(kw.value)) + if not is_expect or not node.args: + continue + out.extend(_as_names(node.args[-1])) + return out + + +def _as_names(node): + """-> every string this node can evaluate to; [] when it states none. + + A LIST rather than one string, because `"a" if cond else "b"` is a name argument + that carries two properties depending on the arm, and both are asserted by the + suite. Reading only one of them reported the other MISSING, which is the same + false red as reading the wrong argument, one level in. + + An f-string becomes a `{}` template. A node that is neither contributes nothing: + reporting a name as absent is better than reporting the wrong string as present. + """ + if isinstance(node, ast.Constant) and isinstance(node.value, str): + return [node.value] + if isinstance(node, ast.JoinedStr): + parts = [] + for v in node.values: + if isinstance(v, ast.Constant) and isinstance(v.value, str): + parts.append(v.value) + else: + parts.append("{}") + return ["".join(parts)] + if isinstance(node, ast.IfExp): + return _as_names(node.body) + _as_names(node.orelse) + return [] + + +# `${...}`, `$(...)`, `$NAME`, and the POSITIONAL parameters. `$1` is how a bash +# helper names the thing it was called about, so it is the commonest interpolation +# in a check name and the first version of this missed every one of them. +_BASH_INTERP = re.compile( + r'\$\{[^}]*\}|\$\([^)]*\)|\$[A-Za-z_][A-Za-z0-9_]*|\$[0-9]+|\$[@*#?]') + + +def _template(name): + """-> the name with every interpolation reduced to `{}`. + + Applied to both sides, so `non-owner refused: ${1%%(*}` and the f-string + `f"non-owner refused: {fn}"` land on the same string. + """ + return re.sub(r"\{[^{}]*\}", "{}", _BASH_INTERP.sub("{}", name)) + + +def main(bash_file, py_file): + """-> the exit status: 1 when a bash property has no counterpart.""" + bash_names = re.findall( + r'\bcheck(?:_num|_ratio|_text|_timing)?\s+"([^"]+)"', open(bash_file).read()) + py_names = _py_names(open(py_file).read()) + + bset, pset = set(bash_names), set(py_names) + + print(f"bash checks: {len(bash_names)} ({len(bset)} distinct)") + print(f"pytest named assertions: {len(py_names)} ({len(pset)} distinct)") + print() + + literal = bset & pset + # Only names with no literal partner are considered as templates, so a template + # match can never hide a literal one or be double-counted. + b_left, p_left = bset - literal, pset - literal + p_templates = {_template(n) for n in p_left} + templated = {n for n in b_left if _template(n) in p_templates} + + missing = sorted(b_left - templated) + extra = sorted(n for n in p_left if _template(n) not in {_template(m) for m in templated}) + + print("PROPERTIES IN THE BASH SUITE AND NOT IN THE PORT:") + if missing: + for n in missing: + print(f" MISSING {n}") + else: + print(" none -- every bash property is asserted by name in the port") + print() + if templated: + print("MATCHED BY TEMPLATE ONLY (both sides build the name at runtime):") + for n in sorted(templated): + print(f" shape {n}") + print() + print("ASSERTIONS IN THE PORT AND NOT IN THE BASH SUITE:") + if extra: + for n in extra: + print(f" extra {n}") + else: + print(" none") + print() + print(f"literal matches: {len(literal)} | template matches: {len(templated)} | " + f"missing: {len(missing)}") + print("VERDICT:", "PORT IS INCOMPLETE" if missing else "every bash property is covered") + return 1 if missing else 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1], sys.argv[2])) diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 6672bcde..39e5213b 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -48,7 +48,10 @@ # `--pgc-expect-tests` compares against exactly what collection reports: # # 290 tests collected -guard_tests 290 +# 290 -> 298 when test_compare_to_bash.py landed: eight arms over the parity tool's own +# extractors. Re-derived by collection on the merged tree, per the recipe above: +# `298 tests collected`. +guard_tests 298 # The complement: tests that need the driver and a throwaway cluster. Until #1016 these ran # in no CI job at all -- a quarter of the corpus, green when somebody ran them by hand and @@ -68,4 +71,7 @@ guard_tests 290 # test_native_ownership.py's 166 -> 177. DERIVED by collection on the merged tree, never by # addition: several branches bumped this from 166 at once, and arithmetic on any one of # them lands on a number no tree collects. -cluster_tests 205 +# 205 -> 217 when test_projection_privilege.py landed: twelve arms over the two projection +# read helpers, parametrised per function so each carries the bash suite's own check name. +# Re-derived by collection. +cluster_tests 217 diff --git a/test/pytest/test_compare_to_bash.py b/test/pytest/test_compare_to_bash.py new file mode 100644 index 00000000..25ee5abd --- /dev/null +++ b/test/pytest/test_compare_to_bash.py @@ -0,0 +1,195 @@ +"""`compare_to_bash.py` must read the assertion's NAME, not some other argument (#432). + +The parity tool is what decides whether a port is one-for-one with its bash suite, which +is #432's definition of done. So the tool is a claim like any other, and it was wrong in a +way that pointed directly at the work it grades. + +THE DEFECT. The python side was matched with a regex: + + expect\\.\\w+\\([^)]*?"([^"]+)"\\s*(?:,[^)]*)?\\) + +`[^)]*?` is lazy, so it stopped at the FIRST quoted argument. For `expect.num(got, 1, +NAME)` that is the name, and the tool looked correct on every arm anyone checked. For a +helper whose WANT is itself a string it is not: + + expect.sqlstate(err, "42501", NAME) -> read "42501" as the name + expect.text(got, "none", NAME) -> read "none" + +Every SQLSTATE assertion was therefore read as the literal `42501`, reported as an "extra" +name the bash suite does not have, while the real property was reported MISSING. #432's +ports are precisely the ones replacing a grep on an error message with a SQLSTATE +assertion, so the tool went blind in proportion to the work being done well. Measured over +the seven pairs in the tree: **61 bash properties reported missing, of which 34 were not +missing at all.** Two whole pairs flipped from `PORT IS INCOMPLETE` to complete. + +WHY A GUARD AND NOT JUST A FIX. Nothing could see this. The tool's own output was the only +evidence either way, and its verdict for a correct port was a plausible-looking list of +names that really were absent from the port -- absent because the tool had matched a +different string, which is not visible from the list. `test_hilbert_locality.py` records +somebody working around it by rewriting their test file until the count fell, and +concluding the rest needed a change to this tool. It did. + +THE ARMS BELOW DRIVE THE REAL EXTRACTORS, never a copy. A python twin of a python rule +would agree with itself. +""" + +import ast +import pathlib +import sys + +HERE = pathlib.Path(__file__).resolve().parent +sys.path.insert(0, str(HERE)) + +from compare_to_bash import _as_names, _parametrized_names, _py_names, _template # noqa: E402 + + +def _names(src): + return _py_names(src) + + +def test_the_name_is_the_last_argument_not_the_first_string(expect): + """THE REGRESSION. Three helpers, one of which always worked. + + `expect.num` is the control: its want is a number, so the old regex happened to reach + the name and the tool looked correct. Without that arm this test would pass over a + rule that returns the last argument of nothing at all. + """ + src = ( + 'def t(expect):\n' + ' expect.sqlstate(err, "42501", "a role with no privilege is refused")\n' + ' expect.text(got, "none", "no key is stated twice")\n' + ' expect.num(got, 1, "the owner reads its own table")\n' + ) + got = _names(src) + expect.text(", ".join(sorted(got)), + "a role with no privilege is refused, no key is stated twice, " + "the owner reads its own table", + "each helper contributes its NAME and not its want") + expect.num(len(got), 3, "three assertions, three names") + expect.num(sum(1 for n in got if n in ("42501", "none")), 0, + "and no want is mistaken for a name, which is the defect this closes") + + +def test_a_call_whose_name_is_not_a_literal_contributes_nothing(expect): + """Better absent than wrong. + + A name the tool cannot read must be reported MISSING, which a person then fixes. + Guessing at it reports the wrong string as PRESENT, and a false green on a parity tool + is how a property ends up asserted in neither harness. + """ + src = 'def t(expect):\n expect.sqlstate(err, "42501", some_variable)\n' + expect.num(len(_names(src)), 0, + "an unreadable name yields nothing rather than the want beside it") + + +def test_an_fstring_name_becomes_a_template(expect): + """Both harnesses build some names at runtime. The shape is what can be compared.""" + src = 'def t(expect):\n expect.num(got, 1, f"premise: {r} can open a session")\n' + expect.text(_names(src)[0], "premise: {} can open a session", + "the interpolated part is reduced to a placeholder") + + +def test_a_conditional_name_carries_both_of_its_arms(expect): + """`"a" if cond else "b"` asserts two properties depending on the arm taken. + + Reading one of them reports the other MISSING, which is the same false red as reading + the wrong argument, one level in. + """ + src = ('def t(expect):\n' + ' expect.num(got, 1, "the owner reads" if f == "read" else "the owner writes")\n') + expect.text(", ".join(sorted(_names(src))), "the owner reads, the owner writes", + "both arms of a conditional name are collected") + + +def test_a_parametrized_name_is_resolved_from_the_decorator(expect): + """The idiom a repeated bash property should be ported to. + + When the bash suite states the same property once per function, the port writes the arm + once and parametrises it, carrying the bash name as a parameter. If the tool cannot see + those names it reports every one of them MISSING, which pushes a port away from the one + idiom that keeps the two harnesses one-to-one. + + The control is the second decorator: a parametrize with no `name` column must + contribute nothing, or the tool would harvest every parameter in the file as an + assertion name and report a pile of extras. + """ + src = ( + 'USAGE_ONLY = (\n' + ' ("read_projection", "a role with only schema USAGE is refused"),\n' + ' ("reconstruct_via_projection", "and is refused reconstruct"),\n' + ')\n' + '@pytest.mark.parametrize("func,name", USAGE_ONLY)\n' + 'def t(expect, func, name):\n' + ' expect.sqlstate(err, "42501", name)\n' + '@pytest.mark.parametrize("func", ["read_projection", "reconstruct"])\n' + 'def u(expect, func):\n' + ' expect.num(got, 1, "an unrelated property")\n' + ) + got = _names(src) + expect.num(int("a role with only schema USAGE is refused" in got), 1, + "a parametrized name is resolved through the module-level constant") + expect.num(int("and is refused reconstruct" in got), 1, "for every row of it") + expect.num(int("read_projection" in got), 0, + "while the OTHER column of the same decorator is not a name") + expect.num(int("reconstruct" in got), 0, + "and a parametrize with no name column contributes nothing") + + +def test_the_parametrize_reader_takes_the_column_called_name(expect): + """Position is not the rule; the declared column is. + + A port that writes `parametrize("name,func", ...)` states the same properties, and a + reader keyed on position silently harvests the function names instead. + """ + tree = ast.parse( + 'ROWS = (("the property", "read_projection"),)\n' + '@pytest.mark.parametrize("name,func", ROWS)\n' + 'def t(name, func):\n pass\n' + ) + expect.text(", ".join(_parametrized_names(tree)), "the property", + "the name column is found by its declared name, whatever its position") + + +def test_the_two_harnesses_interpolations_land_on_one_template(expect): + """What makes a template match mean anything: bash and python spell it differently.""" + expect.text(_template("non-owner refused: ${1%%(*}"), "non-owner refused: {}", + "a bash parameter expansion is reduced to a placeholder") + expect.text(_template("non-owner refused: {}"), "non-owner refused: {}", + "and an f-string template is already in that form, so the two meet") + expect.text(_template("premise: $PGC_PORT is open"), "premise: {} is open", + "a bare variable reference too") + + +def test_the_ported_suites_in_this_tree_are_graded_one_for_one(expect): + """THE STANDING ARM, and the reason this file is not only about fixtures. + + A guard over invented sources proves the extractor reads python. It cannot prove the + tool grades THIS tree, which is the claim #432 rests on. So the pairs that are declared + complete are asserted complete here, and a later edit that breaks parity fails with the + pair named rather than the whole gate going red for an unrelated reason. + + Only the pairs that reach zero today are listed. A pair with a real gap is not pinned to + its gap: that would turn the gap into the expected state. + """ + from compare_to_bash import main + import contextlib + import io + + root = HERE.parent.parent + # EVERY pair in the tree. When a new port lands it belongs here, and when one + # cannot reach zero the reason belongs in its own file rather than in an omission + # from this list. + complete = ["differential", "hilbert_locality", "native_ownership", + "native_projection", "projection_privilege", "stats_privilege", + "zonemap_boundaries"] + verdicts = {} + for stem in complete: + sh, py = root / "test" / f"{stem}.sh", HERE / f"test_{stem}.py" + expect.num(int(sh.exists() and py.exists()), 1, f"premise: both halves of {stem} exist") + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + rc = main(str(sh), str(py)) + verdicts[stem] = rc + expect.text(", ".join(f"{k}={v}" for k, v in sorted(verdicts.items())), + ", ".join(f"{k}=0" for k in sorted(complete)), + "every pair declared one-for-one still grades one-for-one") diff --git a/test/pytest/test_harness_deps.py b/test/pytest/test_harness_deps.py index 20926dc8..30c166bf 100644 --- a/test/pytest/test_harness_deps.py +++ b/test/pytest/test_harness_deps.py @@ -109,6 +109,9 @@ # No cluster, no driver: the public seam is the published page. "test_docs_stripe_floor.py", "test_docs_table_structure.py", + # The parity tool is python and reads python. No cluster and no driver: its input + # is a source file and its output is a verdict about two source files. + "test_compare_to_bash.py", ] diff --git a/test/pytest/test_projection_privilege.py b/test/pytest/test_projection_privilege.py new file mode 100644 index 00000000..15fbac09 --- /dev/null +++ b/test/pytest/test_projection_privilege.py @@ -0,0 +1,394 @@ +"""The projection READ helpers are a privilege boundary (#562, #563; ported for #432). + +`pgcolumnar.read_projection()` and `pgcolumnar.reconstruct_via_projection()` opened a +caller-supplied regclass and returned its contents with no privilege check of any kind, +and `CREATE FUNCTION` grants EXECUTE to PUBLIC. `reconstruct_via_projection` is the worse +of the two: it rebuilds NON-COVERED columns from the base relation by row number, so the +projection was never the bound on what leaked. One projection on any column exposed the +whole row. + +The bar is ACL_SELECT on the BASE relation rather than ownership, and that is a +correctness argument rather than a lenient one: reconstruct returns columns the projection +does not store, so SELECT on the base is exactly the privilege that governs reading those +columns by any other route. + +WHAT THIS PORT ASSERTS THAT `projection_privilege.sh` CANNOT. + +That suite decides four things by matching error TEXT, and two of them are load-bearing +rather than decorative, because the refusals it must tell apart share a SQLSTATE. Its own +comment says so: + + Both layers reject this role, so a bare "refused" stays true if the REVOKE is deleted + and the C check catches it instead -- measured: with the REVOKE removed this suite + still passed 14 of 14. The error text is the only thing that attributes the refusal. + +That is correct about the shell harness and it is not a limit of the property. Three +refusals arrive from three different places in the code, and each carries its own +SQLSTATE the moment you stop asking which words the message contains: + + the SQL grant on the function 42501, raised before the body runs at all + the base-relation ACL check 42501, the first statement of the body + row-level security 0A000, ERRCODE_FEATURE_NOT_SUPPORTED + the projection lookup 42704, ERRCODE_UNDEFINED_OBJECT + +Two of those are 42501, so SQLSTATE alone does not separate the two ACL layers either. +**The discriminator is the fixture, not the message.** Call the function with a projection +name that does not exist, on a table the role may read: + + stopped by the SQL grant -> 42501, because the body never ran to notice the name + past the SQL grant -> 42704, because it ran and looked the name up + +So `test_which_layer_refused` attributes the refusal to a layer by what the code REACHED, +and reaching is a fact about execution rather than about wording. `grep -c 'permission +denied for function'` is satisfied by any future message containing that phrase; this is +satisfied only by control actually arriving at the projection lookup. + +The same move gives the port two arms the shell suite does not have at all: the base-ACL +check must run BEFORE the projection lookup, and before the RLS check. Both are asserted +as orderings in `src/columnar_projection.c`, the second one in a comment recording that an +earlier version had it backwards and disclosed RLS state to a caller with no privilege. +Nothing tested either until now. + +REAL LOGINS, NOT `SET ROLE`. A deny arm is evidence only if the call reached the code that +denies it, and `projection_privilege.sh` records measuring that exact hole: with `ALTER +ROLE t_prjexec NOLOGIN` injected, its EXECUTE premise and both "is refused" checks still +passed. Every premise here is run BY the role it is about. +""" + +import pytest + +ROWS = 2000 +NONE, EXEC, SEL = "t_prjnone", "t_prjexec", "t_prjsel" +ROLES = (NONE, EXEC, SEL) + +FUNCS = ("read_projection", "reconstruct_via_projection") + +# (function, the name `projection_privilege.sh` gives this exact property). The names +# are the bash suite's character for character, which is what lets compare_to_bash.py +# pair the two harnesses by property instead of by shape. +USAGE_ONLY = ( + ("read_projection", "a role with only schema USAGE is refused read_projection"), + ("reconstruct_via_projection", "and is refused reconstruct_via_projection"), +) +EXEC_NO_SELECT = ( + ("read_projection", "a role with EXECUTE but no SELECT is refused read_projection"), + ("reconstruct_via_projection", + "and is refused reconstruct_via_projection, which leaks non-covered columns"), +) +WITH_SELECT = ( + ("read_projection", "a role WITH SELECT still reads the projection"), + ("reconstruct_via_projection", "and still reconstructs"), +) +UNDER_POLICY = ( + ("read_projection", "read_projection now refuses a policy-restricted caller (#563)"), + ("reconstruct_via_projection", "and so does reconstruct_via_projection"), +) + + +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. Ported from `test_stats_privilege.py`, where collapsing that into a 0 + produced a product claim out of 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): + """Two tables and three roles, one role per layer so a refusal can be attributed. + + t_prjnone schema USAGE only. Stopped by the SQL REVOKE. + t_prjexec USAGE + EXECUTE. Clears the SQL layer ON PURPOSE, so a + refusal can only be the C check. + t_prjsel USAGE + EXECUTE + SELECT. Must SUCCEED -- this is the arm that + distinguishes ACL_SELECT from ownership. + + `open_t` is the second table, and it is what makes the layer attribution possible: + every role may read it, so a refusal on it cannot come from the base-relation ACL and + a call naming a projection it does not have reaches the lookup or it does not. + """ + for r in ROLES: + 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("SELECT current_schema()") + schema = cur.fetchone()[0] + for r in ROLES: + cur.execute(f'GRANT USAGE ON SCHEMA "{schema}" TO {r}') + + cur.execute("DROP TABLE IF EXISTS prj_secret") + cur.execute("CREATE TABLE prj_secret (id int, ssn text, note text) USING pgcolumnar") + cur.execute( + f"INSERT INTO prj_secret " + f"SELECT g, 'ssn-'||g, 'note-'||g FROM generate_series(1,{ROWS}) g" + ) + # The projection covers id and ssn. It does NOT cover note, which is the point: + # reconstruct returns note anyway. + cur.execute( + "SELECT pgcolumnar.add_projection(" + "'prj_secret','p1',ARRAY['id','ssn'],ARRAY['id'])" + ) + cur.execute("SELECT pgcolumnar.rebuild_projections('prj_secret')") + cur.execute("REVOKE ALL ON prj_secret FROM PUBLIC") + + cur.execute("DROP TABLE IF EXISTS prj_open") + cur.execute("CREATE TABLE prj_open (id int) USING pgcolumnar") + cur.execute("INSERT INTO prj_open SELECT generate_series(1,10)") + + for r in (EXEC, SEL): + for f in FUNCS: + cur.execute( + f"GRANT EXECUTE ON FUNCTION pgcolumnar.{f}(regclass,text) TO {r}" + ) + cur.execute(f"GRANT SELECT ON prj_secret TO {SEL}") + for r in ROLES: + cur.execute(f"GRANT SELECT ON prj_open TO {r}") + return schema + + +def test_the_premises_the_fixture_is_what_the_suite_assumes(pgc_cluster, pgc_conn, expect): + """Every premise about a role is run BY that role. + + A DENIED result and a BROKEN result are indistinguishable from outside, so the owner + arms are not decoration: they assert a ROW COUNT on the same call the deny arms make. + If a later change made these functions return zero rows for everyone, a no-error owner + arm would stay green while every deny arm stayed green, and the suite would pass over a + function that does nothing. + """ + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + + # CARDINALITY FIRST, for every loop below. A loop over an empty sequence runs no + # assertion and reports no failure, so each of these says how many arms it owes + # before it runs them. + expect.num(len(ROLES), 3, "premise: three roles, one per privilege layer") + expect.num(len(FUNCS), 2, "premise: both read helpers are under test") + + for r in ROLES: + rows, err = _as(pgc_cluster, r, "SELECT 1", schema) + assert err is None, f"{r} could not open a session: {err}" + expect.num(rows[0][0], 1, "premise: t_prjexec can open a session, so a refusal below is a refusal" + if r == EXEC else f"premise: {r} can open a session of its own") + + with pgc_conn.cursor() as cur: + for f in FUNCS: + cur.execute(f"SELECT count(*) FROM pgcolumnar.{f}('prj_secret','p1')") + expect.num(cur.fetchone()[0], ROWS, + "premise: the owner reads the projection, and gets rows" if f == "read_projection" + else "premise: the owner reconstructs, and gets rows") + cur.execute( + "SELECT pgcolumnar.reconstruct_via_projection('prj_secret','p1') " + "LIKE '%note-%' LIMIT 1" + ) + expect.text(str(cur.fetchone()[0]), "True", + "premise: reconstruct really does return the NON-COVERED column") + cur.execute( + "SELECT pgcolumnar.read_projection('prj_secret','p1') LIKE '%note-%' LIMIT 1" + ) + expect.text(str(cur.fetchone()[0]), "False", + "premise: and read_projection does NOT, so the two differ as the issue says") + cur.execute( + "SELECT count(*) FROM pg_roles WHERE rolname = ANY(%s) AND rolsuper", (list(ROLES),) + ) + expect.num(cur.fetchone()[0], 0, "premise: the test roles are not superusers") + # A CATALOG LOOKUP RUN AS THE OWNER, which is blind to whether t_prjexec can + # open a session at all -- hence the session premise above, which that role + # runs itself. Both are needed: this says the grant exists, that says it can + # be exercised. + cur.execute( + "SELECT has_function_privilege(" + "'t_prjexec','pgcolumnar.read_projection(regclass,text)','EXECUTE')" + ) + expect.text(str(cur.fetchone()[0]), "True", + "premise: t_prjexec really does hold EXECUTE, so it reaches the C gate") + + _, err = _as(pgc_cluster, NONE, "SELECT count(*) FROM prj_secret", schema) + expect.sqlstate(err, "42501", + "premise: the unprivileged role cannot read the table by any ordinary route") + + rows, err = _as(pgc_cluster, SEL, "SELECT count(*) FROM prj_secret", schema) + assert err is None, f"{SEL} could not read the table: {err}" + expect.num(rows[0][0], ROWS, + "premise: t_prjsel CAN read the table, so its arm below tests the bar") + + for r in ROLES: + rows, err = _as(pgc_cluster, r, "SELECT count(*) FROM prj_open", schema) + assert err is None, f"{r} could not read prj_open: {err}" + expect.num(rows[0][0], 10, + f"premise: {r} may read prj_open, so a refusal on it is not the ACL") + + +@pytest.mark.parametrize("func,name", USAGE_ONLY) +def test_a_role_with_only_schema_usage_is_refused(pgc_cluster, pgc_conn, expect, func, name): + """Layer one: the SQL grant.""" + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + _, err = _as(pgc_cluster, NONE, + f"SELECT count(*) FROM pgcolumnar.{func}('prj_secret','p1')", schema) + expect.sqlstate(err, "42501", name) + + +@pytest.mark.parametrize("func,name", EXEC_NO_SELECT) +def test_a_role_with_execute_but_no_select_is_refused(pgc_cluster, pgc_conn, expect, func, name): + """Layer two: the C check, reached only because EXECUTE was granted. + + This role clears the SQL layer deliberately. If it is refused, the refusal cannot be + the REVOKE and cannot be the schema. + """ + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + _, err = _as(pgc_cluster, EXEC, + f"SELECT count(*) FROM pgcolumnar.{func}('prj_secret','p1')", schema) + expect.sqlstate(err, "42501", name) + if func == "read_projection": + expect.sqlstate( + err, "42501", + "and THAT refusal is the C check, which raises 42501 from aclcheck_error") + + +def test_which_layer_refused_without_reading_the_message(pgc_cluster, pgc_conn, expect): + """THE ARM THE SHELL SUITE NEEDS ERROR TEXT FOR, decided by execution instead. + + Both layers raise 42501, so the code alone cannot tell them apart and a bare "refused" + stays true if the REVOKE is deleted and the C check catches it instead. Measured in + `projection_privilege.sh`: with the REVOKE removed, that suite still passed 14 of 14. + + So ask a question only one layer can answer. On `prj_open`, which EVERY role may read, + with a projection name that does not exist: + + stopped by the SQL grant 42501 the body never ran, so the name was never read + past the SQL grant 42704 it ran, and the projection lookup raised + + The difference is which code executed. That is not a property of the wording, and no + future rephrasing of either message can move it. + """ + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + call = "SELECT count(*) FROM pgcolumnar.read_projection('prj_open','no_such_proj')" + + _, err = _as(pgc_cluster, NONE, call, schema) + expect.sqlstate(err, "42501", + "the no-EXECUTE role never reaches the body, so the grant is what stopped it") + + _, err = _as(pgc_cluster, EXEC, call, schema) + expect.sqlstate(err, "42704", + "while the EXECUTE role reaches the projection lookup on the same call") + + +def test_the_base_acl_is_checked_before_the_projection_is_looked_up( + pgc_cluster, pgc_conn, expect): + """An ordering `src/columnar_projection.c` asserts and nothing tested. + + The ACL check is the first statement of the body and the projection lookup is well + below it. If they were swapped, a caller with no SELECT would learn whether a named + projection exists on a table it may not read -- existence disclosure, from a function + whose whole purpose is to stop disclosure. + + The control is the arm above: the same role, the same bogus name, on a table it MAY + read, returns 42704. So 42501 here is the ACL check winning the race rather than the + lookup being unreachable. + """ + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + _, err = _as(pgc_cluster, EXEC, + "SELECT count(*) FROM pgcolumnar.read_projection(" + "'prj_secret','no_such_proj')", schema) + expect.sqlstate(err, "42501", + "the base ACL is checked before the projection name is looked up") + + +@pytest.mark.parametrize("func,name", WITH_SELECT) +def test_a_role_with_select_still_reads(pgc_cluster, pgc_conn, expect, func, name): + """THE CONTROL FOR THE WHOLE SUITE. Without it the fix could pass by refusing + everyone, which is not a fix, it is a broken function with a good error message. + + It is also what distinguishes ACL_SELECT from ownership: this role owns nothing, and + an owner-only bar refuses it. Refusing it would be a regression, not a fix. + """ + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + rows, err = _as(pgc_cluster, SEL, + f"SELECT count(*) FROM pgcolumnar.{func}('prj_secret','p1')", schema) + assert err is None, f"{SEL} was refused {func}: {type(err).__name__}: {err}" + expect.num(rows[0][0], ROWS, name) + + +@pytest.mark.parametrize("func,name", UNDER_POLICY) +def test_a_policy_restricted_caller_is_refused(pgc_cluster, pgc_conn, expect, func, name): + """Row-level security, closed by #563, and a DIFFERENT SQLSTATE rather than a phrase. + + ACL_SELECT answers "may this role read this table". RLS answers "which rows". The + direct-storage paths cannot answer the second: policies are applied by the REWRITER to + a query's range table entry, and these functions never build a query. A caller holding + SELECT but restricted to one row by a policy received every row. + + `PgColumnarRequireNoRowSecurity` raises ERRCODE_FEATURE_NOT_SUPPORTED, so this arm + asserts 0A000 where the shell suite greps `^ERROR:.*row-level security`. The codes are + from different SQLSTATE CLASSES -- 0A and 42 -- so this cannot be confused with either + ACL refusal by any amount of rewording, and the premise below shows the policy really + took effect rather than the call having broken for some other reason. + """ + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + cur.execute("ALTER TABLE prj_secret ENABLE ROW LEVEL SECURITY") + cur.execute("DROP POLICY IF EXISTS p_one ON prj_secret") + cur.execute(f"CREATE POLICY p_one ON prj_secret FOR SELECT TO {SEL} USING (id = 1)") + + rows, err = _as(pgc_cluster, SEL, "SELECT count(*) FROM prj_secret", schema) + assert err is None, f"{SEL} could not read the table under the policy: {err}" + expect.num(rows[0][0], 1, "premise: the policy is in force for ordinary SQL") + + _, err = _as(pgc_cluster, SEL, + f"SELECT count(*) FROM pgcolumnar.{func}('prj_secret','p1')", schema) + expect.sqlstate(err, "0A000", name) + if func == "read_projection": + expect.sqlstate( + err, "0A000", + "and the refusal says the feature is not supported, not that a privilege " + "is missing") + + +def test_the_acl_is_checked_before_rls_so_no_privilege_discloses_no_rls_state( + pgc_cluster, pgc_conn, expect): + """The second ordering, recorded in the source as a correction made in review. + + `PgColumnarRequireNoRowSecurity` is called AFTER the relation ACL check. An earlier + version had it first, and `src/columnar_vacuum.c` records what that produced: + + RLS on, no-select, ordinary SQL -> permission denied for table + RLS on, no-select, read_projection -> row-level security is in force + + A caller with no privilege at all was told the table has RLS enabled, which ordinary + SQL does not disclose and which disagrees with core's ordering. + + THE TWO OUTCOMES ARE DIFFERENT SQLSTATE CLASSES, so this arm is decidable: 42501 means + the ACL check ran first and 0A000 means RLS did. The arm above is its control -- the + same table, the same RLS, a role that DOES hold SELECT, and 0A000 -- which is what + makes 42501 here an ordering result rather than the RLS check being switched off. + """ + with pgc_conn.cursor() as cur: + schema = _fixture(cur) + cur.execute("ALTER TABLE prj_secret ENABLE ROW LEVEL SECURITY") + cur.execute("DROP POLICY IF EXISTS p_one ON prj_secret") + cur.execute(f"CREATE POLICY p_one ON prj_secret FOR SELECT TO {SEL} USING (id = 1)") + + _, err = _as(pgc_cluster, EXEC, + "SELECT count(*) FROM pgcolumnar.read_projection('prj_secret','p1')", + schema) + expect.sqlstate(err, "42501", + "a caller with no SELECT is told a privilege is missing, not that RLS is in force") diff --git a/test/pytest/test_stats_privilege.py b/test/pytest/test_stats_privilege.py index d1c588f3..16161820 100644 --- a/test/pytest/test_stats_privilege.py +++ b/test/pytest/test_stats_privilege.py @@ -82,10 +82,20 @@ def test_the_premises_each_role_is_what_the_suite_assumes(pgc_cluster, pgc_conn, """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) + # WRITTEN OUT, NOT LOOPED, and the names are the bash suite's character for + # character. The loop that was here passed `f"premise: {r} can open a session"`, + # which compare_to_bash.py reads as the template `premise: {} can open a session` + # -- matching neither bash name, so both properties were reported MISSING from a + # port that asserts them. A name held in a variable is unreadable to the parity + # tool by design: guessing at it would report the wrong string as PRESENT. + sessions = {} 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") + sessions[r] = rows[0][0] + expect.num(sessions[OWNER], 1, "premise: the owner can open a session") + expect.num(sessions[NONE], 1, "premise: the no-privilege role can open a session") + expect.num(sessions[SEL], 1, "premise: the granted role can open a session") with pgc_conn.cursor() as cur: cur.execute("SELECT relowner::regrole::text FROM pg_class WHERE relname = 'st_t'") diff --git a/test/pytest/test_zonemap_boundaries.py b/test/pytest/test_zonemap_boundaries.py index 03c2f53d..807241d9 100644 --- a/test/pytest/test_zonemap_boundaries.py +++ b/test/pytest/test_zonemap_boundaries.py @@ -48,7 +48,8 @@ def test_exact_zonemap_boundaries(pgc_conn, expect): "SELECT count(*) FROM pgcolumnar.row_group " "WHERE storage_id=pgcolumnar.get_storage_id('zb_c')" ) - expect.num(cur.fetchone()[0], 2, "premise: fixture has two row groups") + expect.num(cur.fetchone()[0], 2, + "premise: the boundary fixture has two row groups") expect.num( _removed(_plan(pgc_conn, "v < 1001")), 1, @@ -80,3 +81,13 @@ def test_exact_zonemap_boundaries(pgc_conn, expect): _removed(_plan(pgc_conn, "v = 1001")), 1, "= excludes the group lying wholly below the constant", ) + # THE LIVENESS PREMISE, last, as in the bash suite. + # + # Every arm above reads a plan or a row set, and a backend that died partway through + # would leave the arms that already ran green and the rest unrun. `pgc_summary` + # accounting catches a missing arm, but only this says the session that produced the + # answers was still the one answering at the end. It is cheap and it is the difference + # between "the arms passed" and "the arms passed on a live server". + with pgc_conn.cursor() as cur: + cur.execute("SELECT 1") + expect.num(cur.fetchone()[0], 1, "backend alive")