[PATCH] Add regress test for pg_read_all_stats role
Started by Alexandra Ryzhevichover 7 years ago8 messages
Hello,
I have noticed that there is no regression tests for default monitoring
roles such as pg_read_all_stats.
In this patch the test called defroles is added. It tests permissions of
pg_read_all_stats role to query database size without CONNECT permission on
it, to query tablespace size without CREATE permission on it, and to read
"query" column of pg_stat_activity table without SUPERUSER privilege.
Best regards,
Alexandra Ryzhevich
Attachments:
0001-Add-regress-test-for-pg_read_all_stats-role.patchtext/x-patch; charset=US-ASCII; name=0001-Add-regress-test-for-pg_read_all_stats-role.patchDownload
From 3cf3d1b9e252902e0e8f05436c703d1ec2d90125 Mon Sep 17 00:00:00 2001
From: Alexandra Ryzhevich <aryzhevich@google.com>
Date: Thu, 2 Aug 2018 18:06:13 +0100
Subject: [PATCH 1/1] Add regress test for pg_read_all_stats role
---
src/test/regress/expected/defroles.out | 68 ++++++++++++++++++++++++++
src/test/regress/parallel_schedule | 2 +-
src/test/regress/serial_schedule | 1 +
src/test/regress/sql/defroles.sql | 46 +++++++++++++++++
4 files changed, 116 insertions(+), 1 deletion(-)
create mode 100644 src/test/regress/expected/defroles.out
create mode 100644 src/test/regress/sql/defroles.sql
diff --git a/src/test/regress/expected/defroles.out b/src/test/regress/expected/defroles.out
new file mode 100644
index 0000000000..f3bec0910a
--- /dev/null
+++ b/src/test/regress/expected/defroles.out
@@ -0,0 +1,68 @@
+-- DEFAULT MONITORING ROLES
+DROP ROLE IF EXISTS tmp_role_stats;
+NOTICE: role "tmp_role_stats" does not exist, skipping
+DROP ROLE IF EXISTS tmp_role_no_stats;
+NOTICE: role "tmp_role_no_stats" does not exist, skipping
+DROP DATABASE IF EXISTS tmp_db;
+NOTICE: database "tmp_db" does not exist, skipping
+CREATE ROLE tmp_role_stats;
+CREATE ROLE tmp_role_no_stats;
+-- check pg_read_all_stats default role permissions
+GRANT pg_read_all_stats TO tmp_role_stats;
+CREATE DATABASE tmp_db;
+-- CONNECT is granted by default
+REVOKE CONNECT ON DATABASE tmp_db FROM public;
+SET SESSION AUTHORIZATION tmp_role_stats;
+-- should not fail because tmp_role_stats is member of pg_read_all_stats
+SELECT pg_database_size('tmp_db') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should not fail because tmp_role_stats is member of pg_read_all_stats
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should be true because tmp_role_stats is member of pg_read_all_stats
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" LIKE '<insufficient privilege>';
+ haspriv
+---------
+ t
+(1 row)
+
+SET SESSION AUTHORIZATION tmp_role_no_stats;
+-- should fail because tmp_role_no_stats has not CONNECT permission on this db
+SELECT pg_database_size('tmp_db') > 0 AS canread;
+ERROR: permission denied for database tmp_db
+-- should fail because tmp_role_no_stats has not CREATE permission on this tablespace
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+ERROR: permission denied for tablespace pg_global
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should be false because current session belongs to superuser
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" LIKE '<insufficient privilege>';
+ haspriv
+---------
+ f
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+DROP ROLE tmp_role_stats;
+DROP ROLE tmp_role_no_stats;
+DROP DATABASE tmp_db;
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 16f979c8d9..d6cf7b8226 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -89,7 +89,7 @@ test: brin gin gist spgist privileges init_privs security_label collate matview
# ----------
# Another group of parallel tests
# ----------
-test: alter_generic alter_operator misc psql async dbsize misc_functions sysviews tsrf tidscan stats_ext
+test: alter_generic alter_operator misc psql async dbsize misc_functions sysviews tsrf tidscan stats_ext defroles
# rules cannot run concurrently with any test that creates a view
test: rules psql_crosstab amutils
diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule
index 42632be675..b12aa09904 100644
--- a/src/test/regress/serial_schedule
+++ b/src/test/regress/serial_schedule
@@ -135,6 +135,7 @@ test: sysviews
test: tsrf
test: tidscan
test: stats_ext
+test: defroles
test: rules
test: psql_crosstab
test: select_parallel
diff --git a/src/test/regress/sql/defroles.sql b/src/test/regress/sql/defroles.sql
new file mode 100644
index 0000000000..8c91ee9c95
--- /dev/null
+++ b/src/test/regress/sql/defroles.sql
@@ -0,0 +1,46 @@
+-- DEFAULT MONITORING ROLES
+
+DROP ROLE IF EXISTS tmp_role_stats;
+DROP ROLE IF EXISTS tmp_role_no_stats;
+
+DROP DATABASE IF EXISTS tmp_db;
+
+CREATE ROLE tmp_role_stats;
+CREATE ROLE tmp_role_no_stats;
+
+
+-- check pg_read_all_stats default role permissions
+GRANT pg_read_all_stats TO tmp_role_stats;
+
+CREATE DATABASE tmp_db;
+-- CONNECT is granted by default
+REVOKE CONNECT ON DATABASE tmp_db FROM public;
+
+SET SESSION AUTHORIZATION tmp_role_stats;
+-- should not fail because tmp_role_stats is member of pg_read_all_stats
+SELECT pg_database_size('tmp_db') > 0 AS canread;
+-- should not fail because tmp_role_stats is member of pg_read_all_stats
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+-- should be true because tmp_role_stats is member of pg_read_all_stats
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" LIKE '<insufficient privilege>';
+
+SET SESSION AUTHORIZATION tmp_role_no_stats;
+-- should fail because tmp_role_no_stats has not CONNECT permission on this db
+SELECT pg_database_size('tmp_db') > 0 AS canread;
+-- should fail because tmp_role_no_stats has not CREATE permission on this tablespace
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+-- should be false because current session belongs to superuser
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" LIKE '<insufficient privilege>';
+
+
+RESET SESSION AUTHORIZATION;
+
+DROP ROLE tmp_role_stats;
+DROP ROLE tmp_role_no_stats;
+
+DROP DATABASE tmp_db;
+
--
2.18.0.597.ga71716f1ad-goog
Re: [PATCH] Add regress test for pg_read_all_stats role
On Thu, Aug 02, 2018 at 06:25:14PM +0100, Alexandra Ryzhevich wrote:
I have noticed that there is no regression tests for default monitoring
roles such as pg_read_all_stats.
A bug has been recently fixed for that, see 0c8910a0, so having more
coverage would be welcome, now your patch has a couple of problems.
25fff40 is the original commit which has introduced pg_read_all_stats.
Your patch has a couple of problems by the way:
- database creation is costly, those should not be part of the main
regression test suite.
- Any roles created should use "regress_" as prefix.
- You should look also at negative tests which trigger "must be
superuser or a member of pg_read_all_settings", like say a "SHOW
stats_temp_directory" with a non-privileged user.
--
Michael
Re: [PATCH] Add regress test for pg_read_all_stats role
Thank you for pointing to some problems of the patch. I've attached a
modified version below.
On Thu, Aug 2, 2018 at 8:38 PM Michael Paquier <michael@paquier.xyz> wrote:
Show quoted text
On Thu, Aug 02, 2018 at 06:25:14PM +0100, Alexandra Ryzhevich wrote:
I have noticed that there is no regression tests for default monitoring
roles such as pg_read_all_stats.A bug has been recently fixed for that, see 0c8910a0, so having more
coverage would be welcome, now your patch has a couple of problems.
25fff40 is the original commit which has introduced pg_read_all_stats.Your patch has a couple of problems by the way:
- database creation is costly, those should not be part of the main
regression test suite.
- Any roles created should use "regress_" as prefix.
- You should look also at negative tests which trigger "must be
superuser or a member of pg_read_all_settings", like say a "SHOW
stats_temp_directory" with a non-privileged user.
--
Michael
Attachments:
0001-Add-regress-test-for-pg_read_all_stats-role.patchtext/x-patch; charset=US-ASCII; name=0001-Add-regress-test-for-pg_read_all_stats-role.patchDownload
From 40145d9431bb26b2c172b3529eceb0595703b7e0 Mon Sep 17 00:00:00 2001
From: Alexandra Ryzhevich <aryzhevich@google.com>
Date: Fri, 3 Aug 2018 12:34:53 +0100
Subject: [PATCH 1/1] Add regress test for pg_read_all_stats role
---
src/test/regress/expected/defroles.out | 102 +++++++++++++++++++++++++
src/test/regress/parallel_schedule | 2 +-
src/test/regress/serial_schedule | 1 +
src/test/regress/sql/defroles.sql | 71 +++++++++++++++++
4 files changed, 175 insertions(+), 1 deletion(-)
create mode 100644 src/test/regress/expected/defroles.out
create mode 100644 src/test/regress/sql/defroles.sql
diff --git a/src/test/regress/expected/defroles.out b/src/test/regress/expected/defroles.out
new file mode 100644
index 0000000000..7cb44ae8bb
--- /dev/null
+++ b/src/test/regress/expected/defroles.out
@@ -0,0 +1,102 @@
+-- DEFAULT MONITORING ROLES
+DROP ROLE IF EXISTS regress_role_haspriv;
+NOTICE: role "regress_role_haspriv" does not exist, skipping
+DROP ROLE IF EXISTS regress_role_nopriv;
+NOTICE: role "regress_role_nopriv" does not exist, skipping
+CREATE ROLE regress_role_haspriv;
+CREATE ROLE regress_role_nopriv;
+----
+-- pg_read_all_stats
+----
+REVOKE CONNECT ON DATABASE regression FROM public;
+GRANT pg_read_all_stats TO regress_role_haspriv;
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_stats
+SELECT pg_database_size('regression') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should not fail because regress_role_haspriv is a member of pg_read_all_stats
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should be true because regress_role_haspriv is member of pg_read_all_stats
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" LIKE '<insufficient privilege>';
+ haspriv
+---------
+ t
+(1 row)
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv has not CONNECT permission on this db
+SELECT pg_database_size('regression') > 0 AS canread;
+ERROR: permission denied for database regression
+-- should fail because regress_role_nopriv has not CREATE permission on this tablespace
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+ERROR: permission denied for tablespace pg_global
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should be false because current session belongs to superuser
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" LIKE '<insufficient privilege>';
+ haspriv
+---------
+ f
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+GRANT CONNECT ON DATABASE regression TO public;
+REVOKE pg_read_all_stats FROM regress_role_haspriv;
+----
+-- pg_read_all_settings
+----
+GRANT pg_read_all_settings TO regress_role_haspriv;
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_settings
+SHOW stats_temp_directory;
+ stats_temp_directory
+----------------------
+ pg_stat_tmp
+(1 row)
+
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW archive_timeout;
+ archive_timeout
+-----------------
+ 0
+(1 row)
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv is not superuser and is not a member of pg_read_all_settings
+SHOW stats_temp_directory;
+ERROR: must be superuser or a member of pg_read_all_settings to examine "stats_temp_directory"
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW archive_timeout;
+ archive_timeout
+-----------------
+ 0
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+REVOKE pg_read_all_settings FROM regress_role_haspriv;
+----
+-- Clean up
+----
+DROP ROLE regress_role_haspriv;
+DROP ROLE regress_role_nopriv;
diff --git a/src/test/regress/parallel_schedule b/src/test/regress/parallel_schedule
index 16f979c8d9..d6cf7b8226 100644
--- a/src/test/regress/parallel_schedule
+++ b/src/test/regress/parallel_schedule
@@ -89,7 +89,7 @@ test: brin gin gist spgist privileges init_privs security_label collate matview
# ----------
# Another group of parallel tests
# ----------
-test: alter_generic alter_operator misc psql async dbsize misc_functions sysviews tsrf tidscan stats_ext
+test: alter_generic alter_operator misc psql async dbsize misc_functions sysviews tsrf tidscan stats_ext defroles
# rules cannot run concurrently with any test that creates a view
test: rules psql_crosstab amutils
diff --git a/src/test/regress/serial_schedule b/src/test/regress/serial_schedule
index 42632be675..b12aa09904 100644
--- a/src/test/regress/serial_schedule
+++ b/src/test/regress/serial_schedule
@@ -135,6 +135,7 @@ test: sysviews
test: tsrf
test: tidscan
test: stats_ext
+test: defroles
test: rules
test: psql_crosstab
test: select_parallel
diff --git a/src/test/regress/sql/defroles.sql b/src/test/regress/sql/defroles.sql
new file mode 100644
index 0000000000..d75d097981
--- /dev/null
+++ b/src/test/regress/sql/defroles.sql
@@ -0,0 +1,71 @@
+-- DEFAULT MONITORING ROLES
+
+DROP ROLE IF EXISTS regress_role_haspriv;
+DROP ROLE IF EXISTS regress_role_nopriv;
+
+CREATE ROLE regress_role_haspriv;
+CREATE ROLE regress_role_nopriv;
+
+
+----
+-- pg_read_all_stats
+----
+
+REVOKE CONNECT ON DATABASE regression FROM public;
+
+GRANT pg_read_all_stats TO regress_role_haspriv;
+
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_stats
+SELECT pg_database_size('regression') > 0 AS canread;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_stats
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+-- should be true because regress_role_haspriv is member of pg_read_all_stats
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" LIKE '<insufficient privilege>';
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv has not CONNECT permission on this db
+SELECT pg_database_size('regression') > 0 AS canread;
+-- should fail because regress_role_nopriv has not CREATE permission on this tablespace
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+-- should be false because current session belongs to superuser
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" LIKE '<insufficient privilege>';
+
+RESET SESSION AUTHORIZATION;
+
+GRANT CONNECT ON DATABASE regression TO public;
+
+REVOKE pg_read_all_stats FROM regress_role_haspriv;
+
+----
+-- pg_read_all_settings
+----
+
+GRANT pg_read_all_settings TO regress_role_haspriv;
+
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_settings
+SHOW stats_temp_directory;
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW archive_timeout;
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv is not superuser and is not a member of pg_read_all_settings
+SHOW stats_temp_directory;
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW archive_timeout;
+
+RESET SESSION AUTHORIZATION;
+
+REVOKE pg_read_all_settings FROM regress_role_haspriv;
+
+----
+-- Clean up
+----
+
+DROP ROLE regress_role_haspriv;
+DROP ROLE regress_role_nopriv;
--
2.18.0.597.ga71716f1ad-goog
Re: [PATCH] Add regress test for pg_read_all_stats role
On Fri, Aug 03, 2018 at 02:08:27PM +0100, Alexandra Ryzhevich wrote:
Thank you for pointing to some problems of the patch. I've attached a
modified version below.
Could you avoid top-posting? This is not the style of the Postgres
mailing lists.
I have been looking at your latest patch, and there are some gotchas:
- There is no need for the initial DROP ROLE commands as those already
get dropped at the end of the tests.
- Some installations may use non-default settings, causing installcheck
to fail with your patch once stats_temp_directory is using a different
path than pg_stat_tmp.
- The same applies for archive_timeout.
- There is already rolenames.sql which has a tiny coverage for default
roles, why not just using it?
+-- should fail because regress_role_nopriv has not CONNECT permission
on this db
+SELECT pg_database_size('regression') > 0 AS canread;
+ERROR: permission denied for database regression
+-- should fail because regress_role_nopriv has not CREATE permission on
this tablespace
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+ERROR: permission denied for tablespace pg_global
Why is that part of a test suite for default roles?There are three code paths which could trigger the restrictions we are
looking at for pg_read_all_settings:
- GetConfigOptionResetString, which is only used for datetyle now.
- GetConfigOptionByName, which can be triggered with any SHOW command.
- GetConfigOption, which is used at postmaster startup and when
reloading the configuration file.
2) is easy to be triggered as a negative test (which fails), less as a
positive test. In order to make a positive test failure-proof with
installcheck you would need to have a parameter which can be changed by
a superuser at session level which gets updated to a certain value, and
would fail to show for another user, so you could use one which is
GUC_SUPERUSER_ONLY and of category PGC_SUSET, like
session_preload_libraries or dynamic_preload_libraries. Still that's
pretty restrictive, and would only test one out of the three code paths
available.
1) and 3) have restrictions in place visibly mainly for module
developers.
--
Michael
Re: [PATCH] Add regress test for pg_read_all_stats role
- There is no need for the initial DROP ROLE commands as those already
get dropped at the end of the tests.
Removed.
- There is already rolenames.sql which has a tiny coverage for default
roles, why not just using it?
Moved changes to rolenames.sql.
+-- should fail because regress_role_nopriv has not CONNECT permission on this db +SELECT pg_database_size('regression') > 0 AS canread; +ERROR: permission denied for database regression +-- should fail because regress_role_nopriv has not CREATE permission on this tablespace +SELECT pg_tablespace_size('pg_global') > 0 AS canread; +ERROR: permission denied for tablespace pg_global Why is that part of a test suite for default roles?
Just to check if changes broke something. I haven't find these checks in
other
regress tests. In other way we get only positive tests. If this is not
needed then
should I remove all the checks for regress_role_nopriv role or negative
regress_role_nopriv tests only?
2) is easy to be triggered as a negative test (which fails), less as a
positive test. In order to make a positive test failure-proof with
installcheck you would need to have a parameter which can be changed by
a superuser at session level which gets updated to a certain value, and
would fail to show for another user, so you could use one which is
GUC_SUPERUSER_ONLY and of category PGC_SUSET, like
session_preload_libraries or dynamic_preload_libraries. Still that's
pretty restrictive, and would only test one out of the three code paths
available.
Changed to use session_preload_libraries.
Alexandra
Attachments:
0001-Add-regress-tests-for-default-monitoring-roles.patchtext/x-patch; charset=US-ASCII; name=0001-Add-regress-tests-for-default-monitoring-roles.patchDownload
From 9d16cdb419b8cea547a40bf4f188b0bd744de310 Mon Sep 17 00:00:00 2001
From: Alexandra Ryzhevich <aryzhevich@google.com>
Date: Tue, 21 Aug 2018 16:01:30 +0100
Subject: [PATCH 1/1] Add regress tests for default monitoring roles
---
src/test/regress/expected/rolenames.out | 98 +++++++++++++++++++++++++
src/test/regress/sql/rolenames.sql | 70 ++++++++++++++++++
2 files changed, 168 insertions(+)
diff --git a/src/test/regress/expected/rolenames.out b/src/test/regress/expected/rolenames.out
index 7daba9fc12..67a9cb75a2 100644
--- a/src/test/regress/expected/rolenames.out
+++ b/src/test/regress/expected/rolenames.out
@@ -944,9 +944,107 @@ SELECT proname, proacl FROM pg_proc WHERE proname LIKE 'testagg_';
testagg9 |
(9 rows)
+-- DEFAULT MONITORING ROLES
+CREATE ROLE regress_role_haspriv;
+CREATE ROLE regress_role_nopriv;
+-- pg_read_all_stats
+REVOKE CONNECT ON DATABASE regression FROM public;
+GRANT pg_read_all_stats TO regress_role_haspriv;
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_stats
+SELECT pg_database_size('regression') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should not fail because regress_role_haspriv is a member of pg_read_all_stats
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should be true because regress_role_haspriv is member of pg_read_all_stats
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" = '<insufficient privilege>';
+ haspriv
+---------
+ t
+(1 row)
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv has not CONNECT permission on this db
+SELECT pg_database_size('regression') > 0 AS canread;
+ERROR: permission denied for database regression
+-- should fail because regress_role_nopriv has not CREATE permission on this tablespace
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+ERROR: permission denied for tablespace pg_global
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should be false because current session belongs to superuser
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" = '<insufficient privilege>';
+ haspriv
+---------
+ f
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+GRANT CONNECT ON DATABASE regression TO public;
+REVOKE pg_read_all_stats FROM regress_role_haspriv;
+-- pg_read_all_settings
+GRANT pg_read_all_settings TO regress_role_haspriv;
+BEGIN;
+-- session_preload_libraries is of PGC_SUSET category
+SET LOCAL session_preload_libraries TO '/tmp/';
+-- vacuum_cost_delay is of PGC_USERSET category
+SET LOCAL vacuum_cost_delay TO 40;
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_settings
+SHOW session_preload_libraries;
+ session_preload_libraries
+---------------------------
+ "/tmp/"
+(1 row)
+
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW vacuum_cost_delay;
+ vacuum_cost_delay
+-------------------
+ 40ms
+(1 row)
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW vacuum_cost_delay;
+ vacuum_cost_delay
+-------------------
+ 40ms
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+ROLLBACK;
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv is not superuser and is not a member of pg_read_all_settings
+SHOW session_preload_libraries;
+ERROR: must be superuser or a member of pg_read_all_settings to examine "session_preload_libraries"
+RESET SESSION AUTHORIZATION;
+REVOKE pg_read_all_settings FROM regress_role_haspriv;
-- clean up
\c
DROP SCHEMA test_roles_schema;
DROP OWNED BY regress_testrol0, "Public", "current_user", regress_testrol1, regress_testrol2, regress_testrolx CASCADE;
DROP ROLE regress_testrol0, regress_testrol1, regress_testrol2, regress_testrolx;
DROP ROLE "Public", "None", "current_user", "session_user", "user";
+DROP ROLE regress_role_haspriv, regress_role_nopriv;
diff --git a/src/test/regress/sql/rolenames.sql b/src/test/regress/sql/rolenames.sql
index 5fe8bc8bca..d8bcd24791 100644
--- a/src/test/regress/sql/rolenames.sql
+++ b/src/test/regress/sql/rolenames.sql
@@ -438,6 +438,75 @@ REVOKE ALL PRIVILEGES ON FUNCTION testagg9(int2) FROM "none"; --error
SELECT proname, proacl FROM pg_proc WHERE proname LIKE 'testagg_';
+
+-- DEFAULT MONITORING ROLES
+
+CREATE ROLE regress_role_haspriv;
+CREATE ROLE regress_role_nopriv;
+
+-- pg_read_all_stats
+
+REVOKE CONNECT ON DATABASE regression FROM public;
+
+GRANT pg_read_all_stats TO regress_role_haspriv;
+
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_stats
+SELECT pg_database_size('regression') > 0 AS canread;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_stats
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+-- should be true because regress_role_haspriv is member of pg_read_all_stats
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" = '<insufficient privilege>';
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv has not CONNECT permission on this db
+SELECT pg_database_size('regression') > 0 AS canread;
+-- should fail because regress_role_nopriv has not CREATE permission on this tablespace
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+-- should be false because current session belongs to superuser
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" = '<insufficient privilege>';
+
+RESET SESSION AUTHORIZATION;
+
+GRANT CONNECT ON DATABASE regression TO public;
+
+REVOKE pg_read_all_stats FROM regress_role_haspriv;
+
+-- pg_read_all_settings
+
+GRANT pg_read_all_settings TO regress_role_haspriv;
+
+BEGIN;
+-- session_preload_libraries is of PGC_SUSET category
+SET LOCAL session_preload_libraries TO '/tmp/';
+-- vacuum_cost_delay is of PGC_USERSET category
+SET LOCAL vacuum_cost_delay TO 40;
+
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_settings
+SHOW session_preload_libraries;
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW vacuum_cost_delay;
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW vacuum_cost_delay;
+
+RESET SESSION AUTHORIZATION;
+ROLLBACK;
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv is not superuser and is not a member of pg_read_all_settings
+SHOW session_preload_libraries;
+
+RESET SESSION AUTHORIZATION;
+
+REVOKE pg_read_all_settings FROM regress_role_haspriv;
+
-- clean up
\c
@@ -445,3 +514,4 @@ DROP SCHEMA test_roles_schema;
DROP OWNED BY regress_testrol0, "Public", "current_user", regress_testrol1, regress_testrol2, regress_testrolx CASCADE;
DROP ROLE regress_testrol0, regress_testrol1, regress_testrol2, regress_testrolx;
DROP ROLE "Public", "None", "current_user", "session_user", "user";
+DROP ROLE regress_role_haspriv, regress_role_nopriv;
--
2.18.0.865.gffc8e1a3cd6-goog
Re: [PATCH] Add regress test for pg_read_all_stats role
On Tue, Aug 21, 2018 at 05:48:49PM +0100, Alexandra Ryzhevich wrote:
Just to check if changes broke something. I haven't find these checks in
other regress tests. In other way we get only positive tests. If this
is not needed then should I remove all the checks for
regress_role_nopriv role or negative regress_role_nopriv tests only?
+-- should not fail because regress_role_haspriv is a member of
pg_read_all_stats
+SELECT pg_database_size('regression') > 0 AS canread;
What is really disturbing about the ones testing the size functions is
that they may be costly when running installcheck. By the way, it would
be nice to avoid the system-wide REVOKE, which could impact any tests
running in parallel. You could simply check for some other NULL
values.Changed to use session_preload_libraries.
+-- session_preload_libraries is of PGC_SUSET category
+SET LOCAL session_preload_libraries TO '/tmp/';
+-- vacuum_cost_delay is of PGC_USERSET category
+SET LOCAL vacuum_cost_delay TO 40;This gets better, thanks. I would suggest using a less realistic value
than "/tmp/", which could become a security hole if copy-pasted around...
--
Michael
Re: [PATCH] Add regress test for pg_read_all_stats role
On Thu, Aug 23, 2018 at 9:12 AM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Aug 21, 2018 at 05:48:49PM +0100, Alexandra Ryzhevich wrote:
Just to check if changes broke something. I haven't find these checks in
other regress tests. In other way we get only positive tests. If this
is not needed then should I remove all the checks for
regress_role_nopriv role or negative regress_role_nopriv tests only?+-- should not fail because regress_role_haspriv is a member of pg_read_all_stats +SELECT pg_database_size('regression') > 0 AS canread; What is really disturbing about the ones testing the size functions is that they may be costly when running installcheck. By the way, it would be nice to avoid the system-wide REVOKE, which could impact any tests running in parallel. You could simply check for some other NULL values.
CONNECT ON DATABASE privilege is granted to public by default (so
all users by default can select pg_database_size()) and
REVOKE CONNECT ... FROM <username> command doesn't impact
user's privileges. The only way to revoke connect privilege from user
is to revoke it from public. That's why maybe it will be less harmful just
to
remove pg_database_size tests at all. But will it be better to remove
pg_tablespace_size() tests also? if possible costs are more harmful
than not testing this code path then I'll remove these tests also.
Changed to use session_preload_libraries.
+-- session_preload_libraries is of PGC_SUSET category +SET LOCAL session_preload_libraries TO '/tmp/'; +-- vacuum_cost_delay is of PGC_USERSET category +SET LOCAL vacuum_cost_delay TO 40;This gets better, thanks. I would suggest using a less realistic value
than "/tmp/", which could become a security hole if copy-pasted around...
Changed to unrealistic 'path-to-preload-libraries'.
Alexandra
Attachments:
0001-Add-regress-tests-for-default-monitoring-roles.patchtext/x-patch; charset=US-ASCII; name=0001-Add-regress-tests-for-default-monitoring-roles.patchDownload
From 000a5b728f0d3ec5079d6421aad60c4aae0f44b8 Mon Sep 17 00:00:00 2001
From: Alexandra Ryzhevich <aryzhevich@google.com>
Date: Fri, 24 Aug 2018 15:08:45 +0100
Subject: [PATCH 1/1] Add regress tests for default monitoring roles
---
src/test/regress/expected/rolenames.out | 86 +++++++++++++++++++++++++
src/test/regress/sql/rolenames.sql | 62 ++++++++++++++++++
2 files changed, 148 insertions(+)
diff --git a/src/test/regress/expected/rolenames.out b/src/test/regress/expected/rolenames.out
index 7daba9fc12..37084c1617 100644
--- a/src/test/regress/expected/rolenames.out
+++ b/src/test/regress/expected/rolenames.out
@@ -944,9 +944,95 @@ SELECT proname, proacl FROM pg_proc WHERE proname LIKE 'testagg_';
testagg9 |
(9 rows)
+-- DEFAULT MONITORING ROLES
+CREATE ROLE regress_role_haspriv;
+CREATE ROLE regress_role_nopriv;
+-- pg_read_all_stats
+GRANT pg_read_all_stats TO regress_role_haspriv;
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_stats
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should be true because regress_role_haspriv is member of pg_read_all_stats
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" = '<insufficient privilege>';
+ haspriv
+---------
+ t
+(1 row)
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv has not CREATE permission on this tablespace
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+ERROR: permission denied for tablespace pg_global
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+ canread
+---------
+ t
+(1 row)
+
+-- should be false because current session belongs to superuser
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" = '<insufficient privilege>';
+ haspriv
+---------
+ f
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+REVOKE pg_read_all_stats FROM regress_role_haspriv;
+-- pg_read_all_settings
+GRANT pg_read_all_settings TO regress_role_haspriv;
+BEGIN;
+-- session_preload_libraries is of PGC_SUSET category
+SET LOCAL session_preload_libraries TO 'path-to-preload-libraries';
+-- vacuum_cost_delay is of PGC_USERSET category
+SET LOCAL vacuum_cost_delay TO 40;
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_settings
+SHOW session_preload_libraries;
+ session_preload_libraries
+-----------------------------
+ "path-to-preload-libraries"
+(1 row)
+
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW vacuum_cost_delay;
+ vacuum_cost_delay
+-------------------
+ 40ms
+(1 row)
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW vacuum_cost_delay;
+ vacuum_cost_delay
+-------------------
+ 40ms
+(1 row)
+
+RESET SESSION AUTHORIZATION;
+ROLLBACK;
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv is not superuser and is not a member of pg_read_all_settings
+SHOW session_preload_libraries;
+ERROR: must be superuser or a member of pg_read_all_settings to examine "session_preload_libraries"
+RESET SESSION AUTHORIZATION;
+REVOKE pg_read_all_settings FROM regress_role_haspriv;
-- clean up
\c
DROP SCHEMA test_roles_schema;
DROP OWNED BY regress_testrol0, "Public", "current_user", regress_testrol1, regress_testrol2, regress_testrolx CASCADE;
DROP ROLE regress_testrol0, regress_testrol1, regress_testrol2, regress_testrolx;
DROP ROLE "Public", "None", "current_user", "session_user", "user";
+DROP ROLE regress_role_haspriv, regress_role_nopriv;
diff --git a/src/test/regress/sql/rolenames.sql b/src/test/regress/sql/rolenames.sql
index 5fe8bc8bca..3e4e91632a 100644
--- a/src/test/regress/sql/rolenames.sql
+++ b/src/test/regress/sql/rolenames.sql
@@ -438,6 +438,67 @@ REVOKE ALL PRIVILEGES ON FUNCTION testagg9(int2) FROM "none"; --error
SELECT proname, proacl FROM pg_proc WHERE proname LIKE 'testagg_';
+
+-- DEFAULT MONITORING ROLES
+
+CREATE ROLE regress_role_haspriv;
+CREATE ROLE regress_role_nopriv;
+
+-- pg_read_all_stats
+
+GRANT pg_read_all_stats TO regress_role_haspriv;
+
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_stats
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+-- should be true because regress_role_haspriv is member of pg_read_all_stats
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" = '<insufficient privilege>';
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv has not CREATE permission on this tablespace
+SELECT pg_tablespace_size('pg_global') > 0 AS canread;
+-- should not fail because it is a default tablespace
+SELECT pg_tablespace_size('pg_default') > 0 AS canread;
+-- should be false because current session belongs to superuser
+SELECT COUNT(*) = 0 AS haspriv FROM pg_stat_activity WHERE "query" = '<insufficient privilege>';
+
+RESET SESSION AUTHORIZATION;
+
+REVOKE pg_read_all_stats FROM regress_role_haspriv;
+
+-- pg_read_all_settings
+
+GRANT pg_read_all_settings TO regress_role_haspriv;
+
+BEGIN;
+-- session_preload_libraries is of PGC_SUSET category
+SET LOCAL session_preload_libraries TO 'path-to-preload-libraries';
+-- vacuum_cost_delay is of PGC_USERSET category
+SET LOCAL vacuum_cost_delay TO 40;
+
+SET SESSION AUTHORIZATION regress_role_haspriv;
+-- should not fail because regress_role_haspriv is a member of pg_read_all_settings
+SHOW session_preload_libraries;
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW vacuum_cost_delay;
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should not fail because this config option has not GUC_SUPERUSER_ONLY flag
+SHOW vacuum_cost_delay;
+
+RESET SESSION AUTHORIZATION;
+ROLLBACK;
+
+SET SESSION AUTHORIZATION regress_role_nopriv;
+-- should fail because regress_role_nopriv is not superuser and is not a member of pg_read_all_settings
+SHOW session_preload_libraries;
+
+RESET SESSION AUTHORIZATION;
+
+REVOKE pg_read_all_settings FROM regress_role_haspriv;
+
-- clean up
\c
@@ -445,3 +506,4 @@ DROP SCHEMA test_roles_schema;
DROP OWNED BY regress_testrol0, "Public", "current_user", regress_testrol1, regress_testrol2, regress_testrolx CASCADE;
DROP ROLE regress_testrol0, regress_testrol1, regress_testrol2, regress_testrolx;
DROP ROLE "Public", "None", "current_user", "session_user", "user";
+DROP ROLE regress_role_haspriv, regress_role_nopriv;
--
2.19.0.rc0.228.g281dcd1b4d0-goog
Re: [PATCH] Add regress test for pg_read_all_stats role
On Fri, Aug 24, 2018 at 03:37:17PM +0100, Alexandra Ryzhevich wrote:
CONNECT ON DATABASE privilege is granted to public by default (so
all users by default can select pg_database_size()) and
REVOKE CONNECT ... FROM <username> command doesn't impact
user's privileges. The only way to revoke connect privilege from user
is to revoke it from public. That's why maybe it will be less harmful
just to remove pg_database_size tests at all. But will it be better to
remove pg_tablespace_size() tests also? if possible costs are more
harmful than not testing this code path then I'll remove these tests
also.
Sorry for the delay, Alexandra, and thanks for the new version. I have
removed the tests related to pg_tablespace_size as they are costly and
those for vacuum_cost_delay as we didn't gain more with those tests.
The rest has been committed, which makes a good first cut.
--
Michael