Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/expected/install.out
Original file line number Diff line number Diff line change
Expand Up @@ -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
6 changes: 6 additions & 0 deletions test/expected/security.out
Original file line number Diff line number Diff line change
@@ -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
10 changes: 9 additions & 1 deletion test/helpers/create.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
38 changes: 37 additions & 1 deletion test/sql/install.sql
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,54 @@ 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']);

-- 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...
Expand Down
69 changes: 69 additions & 0 deletions test/sql/security.sql
Original file line number Diff line number Diff line change
@@ -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
7 changes: 7 additions & 0 deletions test_factory.control
Original file line number Diff line number Diff line change
@@ -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
2 changes: 2 additions & 0 deletions test_factory_pgtap.control
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading