Horton

Thread

Commits

  1. Add basic regression tests for default monitoring roles

  1. [PATCH] Add regress test for pg_read_all_stats role

    Alexandra Ryzhevich <aryzhevich@google.com> — 2018-08-02T17:25:14Z

    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

  2. Re: [PATCH] Add regress test for pg_read_all_stats role

    Michael Paquier <michael@paquier.xyz> — 2018-08-02T19:38:11Z

    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

  3. Re: [PATCH] Add regress test for pg_read_all_stats role

    Alexandra Ryzhevich <aryzhevich@google.com> — 2018-08-03T13:08:27Z

    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:

> 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
>

  4. Re: [PATCH] Add regress test for pg_read_all_stats role

    Michael Paquier <michael@paquier.xyz> — 2018-08-20T00:50:01Z

    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

  5. Re: [PATCH] Add regress test for pg_read_all_stats role

    Alexandra Ryzhevich <aryzhevich@google.com> — 2018-08-21T16:48:49Z

    >
> - 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

  6. Re: [PATCH] Add regress test for pg_read_all_stats role

    Michael Paquier <michael@paquier.xyz> — 2018-08-23T08:12:33Z

    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

  7. Re: [PATCH] Add regress test for pg_read_all_stats role

    Alexandra Ryzhevich <aryzhevich@google.com> — 2018-08-24T14:37:17Z

    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

  8. Re: [PATCH] Add regress test for pg_read_all_stats role

    Michael Paquier <michael@paquier.xyz> — 2018-09-26T06:29:16Z

    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