diff --git a/sql/test_factory--0.5.0.sql b/sql/test_factory--0.5.0.sql index 340e33f..75c4f90 100644 --- a/sql/test_factory--0.5.0.sql +++ b/sql/test_factory--0.5.0.sql @@ -15,6 +15,23 @@ EXCEPTION END $body$; +/* + * As of PG16, CREATE ROLE no longer grants the creating role a SET-enabled + * membership in the new role, so SET ROLE test_factory__owner below fails + * unless the current role is a superuser (which bypasses the check). Grant it + * explicitly WITH SET so a non-superuser install works too. Runs + * unconditionally, even when the role already existed and CREATE ROLE was a + * no-op. Gated on PG16+, where the WITH SET syntax exists; pre-16 GRANT ... TO + * already confers the ability to SET ROLE. + */ +DO $body$ +BEGIN + IF current_setting('server_version_num')::int >= 160000 THEN + EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); + END IF; +END +$body$; + CREATE SCHEMA tf AUTHORIZATION test_factory__owner; COMMENT ON SCHEMA tf IS $$Test factory. Tools for maintaining test data.$$; GRANT USAGE ON SCHEMA tf TO public; diff --git a/sql/test_factory.sql b/sql/test_factory.sql index c44b076..baf8a5f 100644 --- a/sql/test_factory.sql +++ b/sql/test_factory.sql @@ -14,6 +14,23 @@ EXCEPTION END $body$; +/* + * As of PG16, CREATE ROLE no longer grants the creating role a SET-enabled + * membership in the new role, so SET ROLE test_factory__owner below fails + * unless the current role is a superuser (which bypasses the check). Grant it + * explicitly WITH SET so a non-superuser install works too. Runs + * unconditionally, even when the role already existed and CREATE ROLE was a + * no-op. Gated on PG16+, where the WITH SET syntax exists; pre-16 GRANT ... TO + * already confers the ability to SET ROLE. + */ +DO $body$ +BEGIN + IF current_setting('server_version_num')::int >= 160000 THEN + EXECUTE format('GRANT test_factory__owner TO %I WITH SET TRUE', current_user); + END IF; +END +$body$; + CREATE SCHEMA tf AUTHORIZATION test_factory__owner; COMMENT ON SCHEMA tf IS $$Test factory. Tools for maintaining test data.$$; GRANT USAGE ON SCHEMA tf TO public; diff --git a/test/expected/base.out b/test/expected/base.out index 57f115a..de59c65 100644 --- a/test/expected/base.out +++ b/test/expected/base.out @@ -1,24 +1,25 @@ \set ECHO none Creating extension test_factory -ok 1 - Register test customers -ok 2 - Create function customer__add -ok 3 - Register test invoices -ok 4 - Ensure original_role temp table was dropped -ok 5 - Ensure role is put back after install -ok 6 - Security definer function _tf.schema__getsert has search_path=pg_catalog -ok 7 - Security definer function _tf.test_factory__get has search_path=pg_catalog -ok 8 - Security definer function _tf.test_factory__set has search_path=pg_catalog -ok 9 - Security definer function _tf.table_create has search_path=pg_catalog -ok 10 - Security definer function _tf.get has search_path=pg_catalog -ok 11 - customer table is empty -ok 12 - invoice table is empty -ok 13 - invoice factory output -ok 14 - invoice table content -ok 15 - customer table content -ok 16 - invoice factory second call -ok 17 - invoice table content stayed constant -ok 18 - customer table content stayed constant -ok 19 - Test function factory -ok 20 - customer table has new row -ok 21 - truncate invoice -ok 22 - invoice factory get remains the same after truncate +ok 1 - Installing role has SET-enabled membership in test_factory__owner (issue #14) +ok 2 - Register test customers +ok 3 - Create function customer__add +ok 4 - Register test invoices +ok 5 - Ensure original_role temp table was dropped +ok 6 - Ensure role is put back after install +ok 7 - Security definer function _tf.schema__getsert has search_path=pg_catalog +ok 8 - Security definer function _tf.test_factory__get has search_path=pg_catalog +ok 9 - Security definer function _tf.test_factory__set has search_path=pg_catalog +ok 10 - Security definer function _tf.table_create has search_path=pg_catalog +ok 11 - Security definer function _tf.get has search_path=pg_catalog +ok 12 - customer table is empty +ok 13 - invoice table is empty +ok 14 - invoice factory output +ok 15 - invoice table content +ok 16 - customer table content +ok 17 - invoice factory second call +ok 18 - invoice table content stayed constant +ok 19 - customer table content stayed constant +ok 20 - Test function factory +ok 21 - customer table has new row +ok 22 - truncate invoice +ok 23 - invoice factory get remains the same after truncate diff --git a/test/expected/install.out b/test/expected/install.out index ae61611..9c7abeb 100644 --- a/test/expected/install.out +++ b/test/expected/install.out @@ -3,7 +3,7 @@ ok 1 - drop extension test_factory_pgtap ok 2 - drop extension test_factory ok 3 - Extension test_factory should not exist ok 4 - Extension test_factory_pgtap should not exist -ok 5 - create extension +ok 5 - create extension as a non-superuser role (issue #14) ok 6 - Function tf.tap(text, text) should exist ok 7 - clean-up test_factory_pgtap ok 8 - clean-up test_factory diff --git a/test/expected/security.out b/test/expected/security.out new file mode 100644 index 0000000..7313ce6 --- /dev/null +++ b/test/expected/security.out @@ -0,0 +1,6 @@ +\set ECHO none +Creating extension test_factory +ok 1 - Bare, unprivileged role can register test data with zero extra grants +ok 2 - Bare, unprivileged role can create+fetch test data with zero extra grants +ok 3 - Bare, unprivileged role gets the cached row on a second call +ok 4 - Bare role cannot SET ROLE into the extension owner role diff --git a/test/helpers/create.sql b/test/helpers/create.sql index 9897f75..eae4f54 100644 --- a/test/helpers/create.sql +++ b/test/helpers/create.sql @@ -7,7 +7,15 @@ GRANT USAGE ON SCHEMA tap TO test_role; */ CREATE SCHEMA test AUTHORIZATION test_role; -SET ROLE = test_role; +/* + * SET SESSION AUTHORIZATION (not SET ROLE): it changes session_user too, not + * just current_user. Permission checks for a *further* SET ROLE (like the one + * test_factory's install does, and like issue #14's bug) are based on + * session_user's superuser status, not current_user's -- so a plain SET ROLE + * here would leave that one class of check silently bypassed for the rest of + * this file, since pg_regress always connects as a superuser. + */ +SET SESSION AUTHORIZATION test_role; SET search_path = test, tap; CREATE TABLE customer( diff --git a/test/sql/base.sql b/test/sql/base.sql index f52bf4a..700342c 100644 --- a/test/sql/base.sql +++ b/test/sql/base.sql @@ -4,6 +4,36 @@ \set extension_name test_factory \i test/helpers/create_extension.sql +/* + * Regression test for issue #14. On PostgreSQL 16+, CREATE ROLE no longer + * grants the creating role a SET-enabled membership in the new role, so the + * install must GRANT test_factory__owner ... WITH SET TRUE or the SET ROLE + * performed during install fails for non-superuser installs (RDS/Aurora). A + * real superuser bypasses the SET ROLE check, so a plain install here cannot + * reproduce the failure; instead assert the SET-enabled membership the fix + * establishes. pg_auth_members.set_option only exists on PG16+, so the check is + * skipped (with identical TAP output) on older versions, where a plain + * GRANT ... TO already confers the ability to SET ROLE. + */ +SELECT (current_setting('server_version_num')::int >= 160000) AS pg16plus \gset +\if :pg16plus +SELECT ok( + EXISTS( + SELECT 1 + FROM pg_auth_members + WHERE roleid = 'test_factory__owner'::regrole + AND member = current_user::regrole + AND set_option + ) + , 'Installing role has SET-enabled membership in test_factory__owner (issue #14)' +); +\else +SELECT ok( + true + , 'Installing role has SET-enabled membership in test_factory__owner (issue #14)' +); +\endif + -- NOTE: This runs some tests itself \i test/helpers/create.sql diff --git a/test/sql/install.sql b/test/sql/install.sql index ae58bf3..57aa986 100644 --- a/test/sql/install.sql +++ b/test/sql/install.sql @@ -9,11 +9,41 @@ SET client_min_messages = WARNING; */ SELECT lives_ok($$DROP EXTENSION IF EXISTS test_factory_pgtap$$, 'drop extension test_factory_pgtap'); SELECT lives_ok($$DROP EXTENSION IF EXISTS test_factory$$, 'drop extension test_factory'); +/* + * test_factory__owner is deliberately left behind by DROP EXTENSION (the + * install script tolerates it already existing, so a real install/uninstall + * cycle by the same installer keeps working). But this test creates a fresh, + * disposable test_factory_installer role below, and an orphaned owner role + * from a previous run of *this file* would belong to an installer that no + * longer exists -- drop it defensively so repeated local `make installcheck` + * runs against the same cluster don't flake. + */ +DROP ROLE IF EXISTS test_factory__owner; SELECT hasnt_extension( 'test_factory' ); SELECT hasnt_extension( 'test_factory_pgtap' ); -SELECT lives_ok($$CREATE EXTENSION test_factory_pgtap CASCADE$$, 'create extension'); +/* + * Install as a genuine non-superuser role (NOSUPERUSER + CREATEROLE mirrors + * what a real RDS/Aurora master user has), now that both control files are + * marked `superuser = false`. Before the issue #14 fix this fails with + * "must be able to SET ROLE test_factory__owner"; after the fix it succeeds. + */ +CREATE ROLE test_factory_installer NOSUPERUSER CREATEROLE; +-- USAGE on tap is a pgtap test-harness necessity (to call lives_ok() etc. +-- below), not one of the grants under test here. +GRANT USAGE ON SCHEMA tap TO test_factory_installer; +-- CREATE ON DATABASE is never granted to PUBLIC by default (only CONNECT/TEMP +-- are) -- a real RDS/Aurora master user gets this explicitly via rds_superuser, +-- so grant it here to mirror that setup. +DO $body$ +BEGIN + EXECUTE format('GRANT CREATE ON DATABASE %I TO test_factory_installer', current_database()); +END +$body$; +SET SESSION AUTHORIZATION test_factory_installer; +SELECT lives_ok($$CREATE EXTENSION test_factory_pgtap CASCADE$$, 'create extension as a non-superuser role (issue #14)'); +RESET SESSION AUTHORIZATION; COMMIT; SELECT has_function('tf', 'tap', array['text','text']); @@ -21,6 +51,12 @@ SELECT has_function('tf', 'tap', array['text','text']); -- Cleanup SELECT lives_ok($$DROP EXTENSION IF EXISTS test_factory_pgtap$$, 'clean-up test_factory_pgtap'); SELECT lives_ok($$DROP EXTENSION IF EXISTS test_factory$$, 'clean-up test_factory'); +-- DROP ROLE alone fails while the GRANT USAGE ON SCHEMA tap above still holds; +-- DROP OWNED clears any privileges/ownership left in this database first. +DROP OWNED BY test_factory_installer; +DROP ROLE IF EXISTS test_factory_installer; +-- See the comment above the earlier DROP ROLE IF EXISTS test_factory__owner. +DROP ROLE IF EXISTS test_factory__owner; /* * Arguably we should cleanup pgtap and the tap schema... diff --git a/test/sql/security.sql b/test/sql/security.sql new file mode 100644 index 0000000..3ba5047 --- /dev/null +++ b/test/sql/security.sql @@ -0,0 +1,69 @@ +\set ECHO none +\i test/helpers/setup.sql + +\set extension_name test_factory +\i test/helpers/create_extension.sql + +/* + * Prove the public tf.* API needs nothing beyond what a freshly-created, + * unprivileged login role already gets by default: no owned schema, no + * explicit GRANTs, and (deliberately) no membership in test_factory__owner. + * Everything it uses here (tf/_tf schema USAGE, EXECUTE on tf.* functions, + * CREATE TEMP TABLE) comes from either Postgres' own defaults or the GRANTs + * test_factory's install script makes to PUBLIC. + */ +SET ROLE = DEFAULT; +CREATE ROLE test_factory_bare_user; +-- USAGE on tap is a pgtap test-harness necessity (to call lives_ok() etc. +-- below), not one of the grants under test here. +GRANT USAGE ON SCHEMA tap TO test_factory_bare_user; +/* + * SET SESSION AUTHORIZATION, not SET ROLE: it changes session_user too, which + * is what a further SET ROLE's permission check actually looks at. A plain + * SET ROLE here would leave this session able to SET ROLE into anything + * (including test_factory__owner below) regardless of grants, since + * pg_regress always connects as a superuser. + */ +SET SESSION AUTHORIZATION test_factory_bare_user; + +CREATE TEMP TABLE widget( + widget_id serial PRIMARY KEY + , name text NOT NULL +); + +SELECT lives_ok( +$lives_ok$SELECT tf.register( + 'widget' + , array[ + row( + 'base' + , $$INSERT INTO widget VALUES (DEFAULT, 'gadget') RETURNING *$$ + )::tf.test_set + ] +);$lives_ok$ + , 'Bare, unprivileged role can register test data with zero extra grants' +); + +SELECT results_eq( + $$SELECT * FROM tf.get( NULL::widget, 'base' )$$ + , $$VALUES( 1, 'gadget' )$$ + , 'Bare, unprivileged role can create+fetch test data with zero extra grants' +); + +SELECT results_eq( + $$SELECT * FROM tf.get( NULL::widget, 'base' )$$ + , $$VALUES( 1, 'gadget' )$$ + , 'Bare, unprivileged role gets the cached row on a second call' +); + +-- Confirm role isolation still holds for a role that otherwise works fine +SELECT throws_ok( + $$SET ROLE test_factory__owner$$ + , '42501' + , NULL + , 'Bare role cannot SET ROLE into the extension owner role' +); + +ROLLBACK; + +-- vi: expandtab ts=2 sw=2 diff --git a/test_factory.control b/test_factory.control index a18e438..8faf2d4 100644 --- a/test_factory.control +++ b/test_factory.control @@ -1,3 +1,10 @@ comment = 'A framework for managing test data' default_version = '0.5.0' relocatable = false +# Not a security boundary weakening: test_factory__owner is a locked-down, +# dedicated owner role and every privileged function is SECURITY DEFINER with +# search_path=pg_catalog specifically so this is safe to install without a +# real superuser (e.g. RDS/Aurora's non-superuser master user). Installing +# still requires CREATEROLE (for test_factory__owner) plus CREATE on the +# target database. +superuser = false diff --git a/test_factory_pgtap.control b/test_factory_pgtap.control index 8e4a75f..0711698 100644 --- a/test_factory_pgtap.control +++ b/test_factory_pgtap.control @@ -2,3 +2,5 @@ comment = 'A framework for managing test data' default_version = '0.1.0' relocatable = false requires = 'pgtap, test_factory' +# See test_factory.control -- same rationale. +superuser = false