Thread: [HACKERS] Monitoring roles patch
Per the discussion at https://www.postgresql.org/message-id/CA%2BOCxoyYxO%2BJmzv2Micj4uAaQdAi6nq0w25BPQgLLxsrvTmREw%40mail.gmail.com, attached is a patch that implements the following: - Adds a default role called pg_monitor - Gives members of the pg_monitor role full access to: pg_ls_logdir() and pg_ls_waldir() pg_stat_* views and functions pg_tablespace_size() and pg_database_size() Contrib modules: pg_buffercache, pg_freespacemap, pgrowlocks, pg_stat_statements, pgstattuple and pg_visibility (but NOT pg_truncate_visibility_map() ) - Adds a default role called pg_read_all_gucs - Allows members of pg_read_all_gucs to, well, read all GUCs - Grants pg_read_all_gucs to pg_monitor Note that updates to contrib modules followed the strategy recently used in changes to pgstattuple following discussion here, in which the installation SQL script is left at the prior version, and an update script is added and default version number bumped to match that of the upgrade script. Patch includes doc updates, and is dependent on my pg_ls_logdir() and pg_ls_waldir() patch (https://www.postgresql.org/message-id/CA+OCxow-X=D2fWdKy+HP+vQ1LtrgbsYQ=CshzZBqyFT5jOYrFw@mail.gmail.com). -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Hi Dave, The patch failed applied... patch -p1 < /home/vagrant/pg_monitor.diff patching file contrib/pg_buffercache/Makefile patching file contrib/pg_buffercache/pg_buffercache--1.2--1.3.sql patching file contrib/pg_buffercache/pg_buffercache.control patching file contrib/pg_freespacemap/Makefile patching file contrib/pg_freespacemap/pg_freespacemap--1.1--1.2.sql patching file contrib/pg_freespacemap/pg_freespacemap.control patching file contrib/pg_stat_statements/Makefile patching file contrib/pg_stat_statements/pg_stat_statements--1.4--1.5.sql patching file contrib/pg_stat_statements/pg_stat_statements.c patching file contrib/pg_stat_statements/pg_stat_statements.control patching file contrib/pg_visibility/Makefile Hunk #1 succeeded at 4 with fuzz 1. patching file contrib/pg_visibility/pg_visibility--1.1--1.2.sql patching file contrib/pg_visibility/pg_visibility.control patching file contrib/pgrowlocks/pgrowlocks.c patching file contrib/pgstattuple/pgstattuple--1.4--1.5.sql patching file doc/src/sgml/catalogs.sgml Hunk #1 succeeded at 10027 (offset 11 lines). patching file doc/src/sgml/func.sgml Hunk #1 succeeded at 19364 (offset 311 lines). Hunk #2 succeeded at 19648 (offset 311 lines). patching file doc/src/sgml/pgbuffercache.sgml patching file doc/src/sgml/pgfreespacemap.sgml patching file doc/src/sgml/pgrowlocks.sgml patching file doc/src/sgml/pgstatstatements.sgml patching file doc/src/sgml/pgstattuple.sgml patching file doc/src/sgml/pgvisibility.sgml patching file doc/src/sgml/user-manag.sgml patching file src/backend/catalog/system_views.sql Hunk #1 FAILED at 1099. 1 out of 1 hunk FAILED -- saving rejects to file src/backend/catalog/system_views.sql.rej patching file src/backend/replication/walreceiver.c patching file src/backend/utils/adt/dbsize.c Hunk #1 succeeded at 17 (offset -1 lines). Hunk #2 succeeded at 89 (offset -1 lines). Hunk #3 succeeded at 179 (offset -1 lines). patching file src/backend/utils/adt/pgstatfuncs.c patching file src/backend/utils/misc/guc.c Hunk #2 succeeded at 6678 (offset 10 lines). Hunk #3 succeeded at 6728 (offset 10 lines). Hunk #4 succeeded at 8021 (offset 10 lines). Hunk #5 succeeded at 8053 (offset 10 lines). patching file src/include/catalog/pg_authid.h Reject file contents... cat src/backend/catalog/system_views.sql.rej --- src/backend/catalog/system_views.sql +++ src/backend/catalog/system_views.sql @@ -1099,3 +1099,7 @@ REVOKE EXECUTE ON FUNCTION pg_ls_logdir() FROM public;REVOKE EXECUTE ON FUNCTION pg_ls_waldir() FROM public; +GRANT EXECUTE ON FUNCTION pg_ls_logdir() TO pg_monitor; +GRANT EXECUTE ON FUNCTION pg_ls_waldir() TO pg_monitor; + +GRANT pg_read_all_gucs TO pg_monitor; The new status of this patch is: Waiting on Author
Hi On Thu, Mar 16, 2017 at 7:04 PM, Denish Patel <denish.j.patel@gmail.com> wrote: > Hi Dave, > > The patch failed applied... > > patch -p1 < /home/vagrant/pg_monitor.diff > patching file contrib/pg_buffercache/Makefile > patching file contrib/pg_buffercache/pg_buffercache--1.2--1.3.sql > patching file contrib/pg_buffercache/pg_buffercache.control > patching file contrib/pg_freespacemap/Makefile > patching file contrib/pg_freespacemap/pg_freespacemap--1.1--1.2.sql > patching file contrib/pg_freespacemap/pg_freespacemap.control > patching file contrib/pg_stat_statements/Makefile > patching file contrib/pg_stat_statements/pg_stat_statements--1.4--1.5.sql > patching file contrib/pg_stat_statements/pg_stat_statements.c > patching file contrib/pg_stat_statements/pg_stat_statements.control > patching file contrib/pg_visibility/Makefile > Hunk #1 succeeded at 4 with fuzz 1. > patching file contrib/pg_visibility/pg_visibility--1.1--1.2.sql > patching file contrib/pg_visibility/pg_visibility.control > patching file contrib/pgrowlocks/pgrowlocks.c > patching file contrib/pgstattuple/pgstattuple--1.4--1.5.sql > patching file doc/src/sgml/catalogs.sgml > Hunk #1 succeeded at 10027 (offset 11 lines). > patching file doc/src/sgml/func.sgml > Hunk #1 succeeded at 19364 (offset 311 lines). > Hunk #2 succeeded at 19648 (offset 311 lines). > patching file doc/src/sgml/pgbuffercache.sgml > patching file doc/src/sgml/pgfreespacemap.sgml > patching file doc/src/sgml/pgrowlocks.sgml > patching file doc/src/sgml/pgstatstatements.sgml > patching file doc/src/sgml/pgstattuple.sgml > patching file doc/src/sgml/pgvisibility.sgml > patching file doc/src/sgml/user-manag.sgml > patching file src/backend/catalog/system_views.sql > Hunk #1 FAILED at 1099. > 1 out of 1 hunk FAILED -- saving rejects to file src/backend/catalog/system_views.sql.rej > patching file src/backend/replication/walreceiver.c > patching file src/backend/utils/adt/dbsize.c > Hunk #1 succeeded at 17 (offset -1 lines). > Hunk #2 succeeded at 89 (offset -1 lines). > Hunk #3 succeeded at 179 (offset -1 lines). > patching file src/backend/utils/adt/pgstatfuncs.c > patching file src/backend/utils/misc/guc.c > Hunk #2 succeeded at 6678 (offset 10 lines). > Hunk #3 succeeded at 6728 (offset 10 lines). > Hunk #4 succeeded at 8021 (offset 10 lines). > Hunk #5 succeeded at 8053 (offset 10 lines). > patching file src/include/catalog/pg_authid.h > > Reject file contents... > > cat src/backend/catalog/system_views.sql.rej > --- src/backend/catalog/system_views.sql > +++ src/backend/catalog/system_views.sql > @@ -1099,3 +1099,7 @@ > > REVOKE EXECUTE ON FUNCTION pg_ls_logdir() FROM public; > REVOKE EXECUTE ON FUNCTION pg_ls_waldir() FROM public; > +GRANT EXECUTE ON FUNCTION pg_ls_logdir() TO pg_monitor; > +GRANT EXECUTE ON FUNCTION pg_ls_waldir() TO pg_monitor; > + > +GRANT pg_read_all_gucs TO pg_monitor; > > The new status of this patch is: Waiting on Author Odd - I get very different rejects than you. Perhaps you didn't apply my pg_ls_logdir() patch first? In any case, that was committed last night. Here's a rebased patch. Thanks! -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: tested, passed Looks good.
On 2/24/17 05:14, Dave Page wrote: > - Adds a default role called pg_read_all_gucs > - Allows members of pg_read_all_gucs to, well, read all GUCs > - Grants pg_read_all_gucs to pg_monitor Maybe pg_read_all_settings? Consistent with pg_settings. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Feb 24, 2017 at 5:14 AM, Dave Page <dpage@pgadmin.org> wrote: > - Adds a default role called pg_monitor > - Gives members of the pg_monitor role full access to: > pg_ls_logdir() and pg_ls_waldir() > pg_stat_* views and functions > pg_tablespace_size() and pg_database_size() > Contrib modules: > pg_buffercache, > pg_freespacemap, > pgrowlocks, > pg_stat_statements, > pgstattuple and > pg_visibility (but NOT pg_truncate_visibility_map() ) > - Adds a default role called pg_read_all_gucs > - Allows members of pg_read_all_gucs to, well, read all GUCs > - Grants pg_read_all_gucs to pg_monitor I like the pg_read_all_gucs role, which I agree with Peter should be called pg_read_all_settings. I'd be inclined to skip the rest of this. If an individual user wants to grant that bundle of privileges to a role, they can do it with or without pg_monitor. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Mar 22, 2017 at 11:32 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, Feb 24, 2017 at 5:14 AM, Dave Page <dpage@pgadmin.org> wrote: >> - Adds a default role called pg_monitor >> - Gives members of the pg_monitor role full access to: >> pg_ls_logdir() and pg_ls_waldir() >> pg_stat_* views and functions >> pg_tablespace_size() and pg_database_size() >> Contrib modules: >> pg_buffercache, >> pg_freespacemap, >> pgrowlocks, >> pg_stat_statements, >> pgstattuple and >> pg_visibility (but NOT pg_truncate_visibility_map() ) >> - Adds a default role called pg_read_all_gucs >> - Allows members of pg_read_all_gucs to, well, read all GUCs >> - Grants pg_read_all_gucs to pg_monitor > > I like the pg_read_all_gucs role, which I agree with Peter should be > called pg_read_all_settings. No objection to that change. > I'd be inclined to skip the rest of > this. If an individual user wants to grant that bundle of privileges > to a role, they can do it with or without pg_monitor. GRANT cannot be used in all cases, as some of the functions changed have hard-coded superuser checks. In those cases, I've added pg_monitor membership to the permission checks in the C code. The reason for having the role is to minimise the amount of work required by the user to setup a role for the purposes of monitoring the server. This is a practice which is seen in other DBMSs for usability. With the patch, complex monitoring systems can easily be setup with something like: CREATE ROLE monitoring_user LOGIN; GRANT pg_monitor TO monitoring_role; Without, the users setting up their monitoring system will have to run a much more complex set of GRANTs, almost certainly requiring scripting. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Mar 22, 2017 at 7:48 AM, Dave Page <dpage@pgadmin.org> wrote: >> I'd be inclined to skip the rest of >> this. If an individual user wants to grant that bundle of privileges >> to a role, they can do it with or without pg_monitor. > > GRANT cannot be used in all cases, as some of the functions changed > have hard-coded superuser checks. In those cases, I've added > pg_monitor membership to the permission checks in the C code. Oh. Well, why not just control access using the permissions checking mechanism entirely, without hardcoding any OID? > The reason for having the role is to minimise the amount of work > required by the user to setup a role for the purposes of monitoring > the server. This is a practice which is seen in other DBMSs for > usability. > > With the patch, complex monitoring systems can easily be setup with > something like: > > CREATE ROLE monitoring_user LOGIN; > GRANT pg_monitor TO monitoring_role; > > Without, the users setting up their monitoring system will have to run > a much more complex set of GRANTs, almost certainly requiring > scripting. But that script is easy to provide, probably not very long, and could be bundled in an extension if it's helpful. I'm wary of committing ourselves to a specific decision about what pg_monitor includes; that seems like it could result in endless future litigation every time somebody wants to make a change. In contrast, nobody's going to have any question at all about the remit of pg_read_all_settings. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 3/22/17 07:48, Dave Page wrote: > With the patch, complex monitoring systems can easily be setup with > something like: > > CREATE ROLE monitoring_user LOGIN; > GRANT pg_monitor TO monitoring_role; That assumes that we have thought of all the ways in which people might want to monitor things. If we do it via GRANTs instead, then users can easily extend it. If we instead change the hardcoded superuser checks to hardcoded some-other-role checks, then the whole system instantly becomes unusable the moment someone wants to monitor something we haven't thought of. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
* Robert Haas (robertmhaas@gmail.com) wrote: > On Wed, Mar 22, 2017 at 7:48 AM, Dave Page <dpage@pgadmin.org> wrote: > >> I'd be inclined to skip the rest of > >> this. If an individual user wants to grant that bundle of privileges > >> to a role, they can do it with or without pg_monitor. > > > > GRANT cannot be used in all cases, as some of the functions changed > > have hard-coded superuser checks. In those cases, I've added > > pg_monitor membership to the permission checks in the C code. > > Oh. Well, why not just control access using the permissions checking > mechanism entirely, without hardcoding any OID? I've not had the chance yet to review this (though I have been planning to), but at least in most cases where we have hard-coded checks it's because the GRANT system doesn't allow the kind of fine-grained access we've implemented at the C level. pg_stat_activity is a great example of this where, if you aren't a superuser or a member of the other role, you can still query the table but you can't see the 'query' column for those other connections. I did specifically ask for explicit roles to be made to enable such capability and that the pg_monitor role be GRANT'd those roles instead of hacking the pg_monitor OID into those checks, but it seems like that's not been done yet. > > The reason for having the role is to minimise the amount of work > > required by the user to setup a role for the purposes of monitoring > > the server. This is a practice which is seen in other DBMSs for > > usability. > > > > With the patch, complex monitoring systems can easily be setup with > > something like: > > > > CREATE ROLE monitoring_user LOGIN; > > GRANT pg_monitor TO monitoring_role; > > > > Without, the users setting up their monitoring system will have to run > > a much more complex set of GRANTs, almost certainly requiring > > scripting. > > But that script is easy to provide, probably not very long, and could > be bundled in an extension if it's helpful. I'm wary of committing > ourselves to a specific decision about what pg_monitor includes; that > seems like it could result in endless future litigation every time > somebody wants to make a change. In contrast, nobody's going to have > any question at all about the remit of pg_read_all_settings. I tend to agree with Dave on this. While I understand this concern and risk of litigation, I believe that comes down to, really, making sure that we spell out exactly what pg_monitor is intended for and that the privileges it has will change over time. If users are not comfortable with that, then they can use provided script to create their own 'monitor' role- that is specifically why I want to have nothing hard-coded to pg_monitor though, because otherwise you couldn't have such a script able to GRANT out a subset of what pg_monitor allows. Thanks! Stephen
Peter, * Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote: > On 3/22/17 07:48, Dave Page wrote: > > With the patch, complex monitoring systems can easily be setup with > > something like: > > > > CREATE ROLE monitoring_user LOGIN; > > GRANT pg_monitor TO monitoring_role; > > That assumes that we have thought of all the ways in which people might > want to monitor things. I disagree. The entire point of the pg_monitor role is to cover those rights which we feel should be available to monitoring solutions, and that *will* change over time. > If we do it via GRANTs instead, then users can easily extend it. The intent here is that users will *also* be able to do it via GRANTs if they wish to. > If we instead change the hardcoded superuser checks to hardcoded > some-other-role checks, then the whole system instantly becomes unusable > the moment someone wants to monitor something we haven't thought of. Right, that's why we need specific roles for the cases where we have to have a C-level check and the pg_monitor role should only be GRANT'd those other roles or GRANTs on specific functions, all of which a DBA/superuser could do themselves with their own role, if they wished to do so, instead of using pg_monitor. Thanks! Stephen
On Wed, Mar 22, 2017 at 12:55 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 3/22/17 07:48, Dave Page wrote: >> With the patch, complex monitoring systems can easily be setup with >> something like: >> >> CREATE ROLE monitoring_user LOGIN; >> GRANT pg_monitor TO monitoring_role; > > That assumes that we have thought of all the ways in which people might > want to monitor things. Right - it was discussed here, and at other meetings. We may not have everything but either users can GRANT anything we need that we missed later, or we can add them in a future release. > If we do it via GRANTs instead, then users can easily extend it. They can do that anyway. > If we instead change the hardcoded superuser checks to hardcoded > some-other-role checks, then the whole system instantly becomes unusable > the moment someone wants to monitor something we haven't thought of. I haven't replaced the checks, I've made them superuser || pg_monitor. Nothing is going to break if we haven't thought of something. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Mar 22, 2017 at 1:15 PM, Stephen Frost <sfrost@snowman.net> wrote: > > I did specifically ask for explicit roles to be made to enable such > capability and that the pg_monitor role be GRANT'd those roles instead > of hacking the pg_monitor OID into those checks, but it seems like > that's not been done yet. Yeah, sorry - I missed that for pg_stat_activity. I'll update the patch. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Mar 22, 2017 at 3:46 PM, Dave Page <dpage@pgadmin.org> wrote: > On Wed, Mar 22, 2017 at 1:15 PM, Stephen Frost <sfrost@snowman.net> wrote: >> >> I did specifically ask for explicit roles to be made to enable such >> capability and that the pg_monitor role be GRANT'd those roles instead >> of hacking the pg_monitor OID into those checks, but it seems like >> that's not been done yet. > > Yeah, sorry - I missed that for pg_stat_activity. I'll update the patch. Updated patch attached. This changes pg_read_all_gucs to pg_read_all_settings, and adds pg_read_all_stats which it grants to pg_monitor. pg_read_all_stats has full access to the pg_stat_ views (as pg_monitor did previously), and is used in the various contrib modules in place of pg_monitor. Thanks. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Dave,
* Dave Page (dpage@pgadmin.org) wrote:
> On Wed, Mar 22, 2017 at 3:46 PM, Dave Page <dpage@pgadmin.org> wrote:
> > On Wed, Mar 22, 2017 at 1:15 PM, Stephen Frost <sfrost@snowman.net> wrote:
> >> I did specifically ask for explicit roles to be made to enable such
> >> capability and that the pg_monitor role be GRANT'd those roles instead
> >> of hacking the pg_monitor OID into those checks, but it seems like
> >> that's not been done yet.
> >
> > Yeah, sorry - I missed that for pg_stat_activity. I'll update the patch.
>
> Updated patch attached. This changes pg_read_all_gucs to
> pg_read_all_settings, and adds pg_read_all_stats which it grants to
> pg_monitor. pg_read_all_stats has full access to the pg_stat_ views
> (as pg_monitor did previously), and is used in the various contrib
> modules in place of pg_monitor.
Thanks! I've started looking through it.
> diff --git a/contrib/pg_buffercache/Makefile b/contrib/pg_buffercache/Makefile
> index 497dbeb229..18f7a87452 100644
> --- a/contrib/pg_buffercache/Makefile
> +++ b/contrib/pg_buffercache/Makefile
> @@ -4,8 +4,9 @@ MODULE_big = pg_buffercache
> OBJS = pg_buffercache_pages.o $(WIN32RES)
>
> EXTENSION = pg_buffercache
> -DATA = pg_buffercache--1.2.sql pg_buffercache--1.1--1.2.sql \
> - pg_buffercache--1.0--1.1.sql pg_buffercache--unpackaged--1.0.sql
> +DATA = pg_buffercache--1.2.sql pg_buffercache--1.2--1.3.sql \
> + pg_buffercache--1.1--1.2.sql pg_buffercache--1.0--1.1.sql \
> + pg_buffercache--unpackaged--1.0.sql
> PGFILEDESC = "pg_buffercache - monitoring of shared buffer cache in real-time"
>
> ifdef USE_PGXS
Did you forget to 'add' the new files..? I don't see the
pg_buffercache--1.2--1.3.sql file.
> diff --git a/contrib/pg_freespacemap/Makefile b/contrib/pg_freespacemap/Makefile
> index 7bc0e9555d..0a2f000ec6 100644
> --- a/contrib/pg_freespacemap/Makefile
> +++ b/contrib/pg_freespacemap/Makefile
> @@ -4,8 +4,8 @@ MODULE_big = pg_freespacemap
> OBJS = pg_freespacemap.o $(WIN32RES)
>
> EXTENSION = pg_freespacemap
> -DATA = pg_freespacemap--1.1.sql pg_freespacemap--1.0--1.1.sql \
> - pg_freespacemap--unpackaged--1.0.sql
> +DATA = pg_freespacemap--1.1.sql pg_freespacemap--1.1--1.2.sql \
> + pg_freespacemap--1.0--1.1.sql pg_freespacemap--unpackaged--1.0.sql
> PGFILEDESC = "pg_freespacemap - monitoring of free space map"
>
> ifdef USE_PGXS
Same here.
> diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
> index 298951a5f5..39b368b70e 100644
> --- a/contrib/pg_stat_statements/Makefile
> +++ b/contrib/pg_stat_statements/Makefile
> @@ -4,9 +4,10 @@ MODULE_big = pg_stat_statements
> OBJS = pg_stat_statements.o $(WIN32RES)
>
> EXTENSION = pg_stat_statements
> -DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.3--1.4.sql \
> - pg_stat_statements--1.2--1.3.sql pg_stat_statements--1.1--1.2.sql \
> - pg_stat_statements--1.0--1.1.sql pg_stat_statements--unpackaged--1.0.sql
> +DATA = pg_stat_statements--1.4.sql pg_stat_statements--1.4--1.5.sql \
> + pg_stat_statements--1.3--1.4.sql pg_stat_statements--1.2--1.3.sql \
> + pg_stat_statements--1.1--1.2.sql pg_stat_statements--1.0--1.1.sql \
> + pg_stat_statements--unpackaged--1.0.sql
> PGFILEDESC = "pg_stat_statements - execution statistics of SQL statements"
>
> LDFLAGS_SL += $(filter -lm, $(LIBS))
And here.
> diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
> index 221ac98d4a..cec94d5896 100644
> --- a/contrib/pg_stat_statements/pg_stat_statements.c
> +++ b/contrib/pg_stat_statements/pg_stat_statements.c
> @@ -1386,7 +1387,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
> MemoryContext per_query_ctx;
> MemoryContext oldcontext;
> Oid userid = GetUserId();
> - bool is_superuser = superuser();
> + bool is_superuser = false;
> char *qbuffer = NULL;
> Size qbuffer_size = 0;
> Size extent = 0;
> @@ -1394,6 +1395,9 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
> HASH_SEQ_STATUS hash_seq;
> pgssEntry *entry;
>
> + /* For the purposes of this module, we consider pg_read_all_stats members to be superusers */
> + is_superuser = (superuser() || is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS));
Whoahhhh, let's *not* set an 'is_superuser' flag for a case where the
user is not, actually, a superuser. This should be 'allowed_role' or
something along those lines, I'm thinking.
> diff --git a/contrib/pg_visibility/Makefile b/contrib/pg_visibility/Makefile
> index bc42944426..21d787ddf7 100644
> --- a/contrib/pg_visibility/Makefile
> +++ b/contrib/pg_visibility/Makefile
> @@ -4,7 +4,8 @@ MODULE_big = pg_visibility
> OBJS = pg_visibility.o $(WIN32RES)
>
> EXTENSION = pg_visibility
> -DATA = pg_visibility--1.1.sql pg_visibility--1.0--1.1.sql
> +DATA = pg_visibility--1.1.sql pg_visibility--1.1--1.2.sql \
> + pg_visibility--1.0--1.1.sql
> PGFILEDESC = "pg_visibility - page visibility information"
>
> REGRESS = pg_visibility
Missing file here also.
> diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c
> index db9e0349a0..c9194d8c0d 100644
> --- a/contrib/pgrowlocks/pgrowlocks.c
> +++ b/contrib/pgrowlocks/pgrowlocks.c
> @@ -28,6 +28,7 @@
> #include "access/relscan.h"
> #include "access/xact.h"
> #include "catalog/namespace.h"
> +#include "catalog/pg_authid.h"
> #include "funcapi.h"
> #include "miscadmin.h"
> #include "storage/bufmgr.h"
> @@ -98,9 +99,11 @@ pgrowlocks(PG_FUNCTION_ARGS)
> relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
> rel = heap_openrv(relrv, AccessShareLock);
>
> - /* check permissions: must have SELECT on table */
> - aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
> - ACL_SELECT);
> + /* check permissions: must have SELECT on table or be in pg_read_all_stats */
> + aclresult = (pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
> + ACL_SELECT) ||
> + is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS));
> +
> if (aclresult != ACLCHECK_OK)
> aclcheck_error(aclresult, ACL_KIND_CLASS,
> RelationGetRelationName(rel));
Doesn't this go beyond just pg_stat_X, but actually allows running
pgrowlocks against any relation? That seems like it should be
independent, though it actually fits the "global-read-metadata" role
case that has been discussed previously as being what pg_visibility,
pgstattuple, and other similar contrib modules need. I don't have a
great name for that role, but it seems different to me from
'pg_read_all_stats', though I don't hold that position very strongly as
I can see an argument that these kind of are stats.
I see you did change pgstattuple too, and perhaps these are the changes
you intended for pg_visibility too? In any case, if so, we need to make
it clear in the docs that pg_read_all_stats grants access to more than
just pg_stat_X.
> diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
> index df0435c3f0..5472ff76a7 100644
> --- a/doc/src/sgml/catalogs.sgml
> +++ b/doc/src/sgml/catalogs.sgml
> @@ -10027,15 +10027,17 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
> <entry><type>text</type></entry>
> <entry>Configuration file the current value was set in (null for
> values set from sources other than configuration files, or when
> - examined by a non-superuser);
> - helpful when using <literal>include</> directives in configuration files</entry>
> + examined by a user who is neither a superuser or a member of
> + <literal>pg_read_all_settings</literal>); helpful when using
> + <literal>include</> directives in configuration files</entry>
> </row>
> <row>
> <entry><structfield>sourceline</structfield></entry>
> <entry><type>integer</type></entry>
> <entry>Line number within the configuration file the current value was
> set at (null for values set from sources other than configuration files,
> - or when examined by a non-superuser)
> + or when examined by a user who is neither a superuser or a member of
> + <literal>pg_read_all_settings</literal>).
> </entry>
> </row>
> <row>
We should really figure out a way to link back to the default roles
section when we refer to roles from other places. Not sure what the
best way to do that off-hand is though.
> diff --git a/doc/src/sgml/pgbuffercache.sgml b/doc/src/sgml/pgbuffercache.sgml
> index b261a4dbe0..9d1faefdd6 100644
> --- a/doc/src/sgml/pgbuffercache.sgml
> +++ b/doc/src/sgml/pgbuffercache.sgml
> @@ -24,8 +24,9 @@
> </para>
>
> <para>
> - By default public access is revoked from both of these, just in case there
> - are security issues lurking.
> + By default use is restricted to superusers and members of the
> + <literal>pg_read_all_stats</literal> role, just in case there are security
> + issues lurking.
> </para>
>
> <sect2>
> diff --git a/doc/src/sgml/pgfreespacemap.sgml b/doc/src/sgml/pgfreespacemap.sgml
> index f2f99d571e..f4f0e08473 100644
> --- a/doc/src/sgml/pgfreespacemap.sgml
> +++ b/doc/src/sgml/pgfreespacemap.sgml
> @@ -16,8 +16,9 @@
> </para>
>
> <para>
> - By default public access is revoked from the functions, just in case
> - there are security issues lurking.
> + By default use is restricted to superusers and members of the
> + <literal>pg_read_all_stats</literal> role, just in case there are security
> + issues lurking.
> </para>
>
> <sect2>
These also look like cases where we might want to think about if we
should have a dedicated role which, essentially, allows locking and
scanning of all relations.
In particular, I can certainly imagine admins who wish to GRANT out the
rights to view other queries in pg_stat_activity to another admin and
maybe even allow them to run pg_terminate_backend, or pg_cancel_query,
but not allow them to scan every relation in the database.
> diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
> index 31c567b37e..8e5c8c507c 100644
> --- a/src/backend/replication/walreceiver.c
> +++ b/src/backend/replication/walreceiver.c
> @@ -50,6 +50,7 @@
> #include "access/timeline.h"
> #include "access/transam.h"
> #include "access/xlog_internal.h"
> +#include "catalog/pg_authid.h"
> #include "catalog/pg_type.h"
> #include "funcapi.h"
> #include "libpq/pqformat.h"
> @@ -1396,7 +1397,7 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
> /* Fetch values */
> values[0] = Int32GetDatum(walrcv->pid);
>
> - if (!superuser())
> + if (!superuser() && !is_member_of_role(GetUserId(), DEFAULT_ROLE_MONITOR))
> {
> /*
> * Only superusers can see details. Other users only get the pid value
Shouldn't that be DEFAULT_ROLE_READ_ALL_STATS, not MONITOR?
> diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
> index 4feb26aa7a..d1da580c9c 100644
> --- a/src/backend/utils/misc/guc.c
> +++ b/src/backend/utils/misc/guc.c
> @@ -34,6 +34,7 @@
> #include "access/xact.h"
> #include "access/xlog_internal.h"
> #include "catalog/namespace.h"
> +#include "catalog/pg_authid.h"
> #include "commands/async.h"
> #include "commands/prepare.h"
> #include "commands/user.h"
> @@ -6677,10 +6678,12 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_superuser)
> }
> if (restrict_superuser &&
> (record->flags & GUC_SUPERUSER_ONLY) &&
> - !superuser())
> + !superuser() &&
> + !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS))
Seems like having a variable to indicate if the caller should be able to
read all these settings or not might be nice to have, so we have one
place that figures out the privileges question.
A few comments where that variable is set wouldn't be bad either.
Also, I'm thinking the pg_monitor documentation really needs to be
expanded and made very clear. I'll think a bit more on just how to try
and phrase that, but the description included certainly seemed
inadequate.
That's it for a quick review. If we can get these changes done and
there aren't any more questions or concerns, I'm happy to continue
working to move this forward. I definitely see the usefulness of these
changes and it'd be great to have an answer to the oft-asked question of
how to do proper monitoring with a non-superuser role.
Thanks!
Stephen
Hi
On Thu, Mar 23, 2017 at 12:23 PM, Stephen Frost <sfrost@snowman.net> wrote:
>
>> diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
>> index 221ac98d4a..cec94d5896 100644
>> --- a/contrib/pg_stat_statements/pg_stat_statements.c
>> +++ b/contrib/pg_stat_statements/pg_stat_statements.c
>> @@ -1386,7 +1387,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
>> MemoryContext per_query_ctx;
>> MemoryContext oldcontext;
>> Oid userid = GetUserId();
>> - bool is_superuser = superuser();
>> + bool is_superuser = false;
>> char *qbuffer = NULL;
>> Size qbuffer_size = 0;
>> Size extent = 0;
>> @@ -1394,6 +1395,9 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
>> HASH_SEQ_STATUS hash_seq;
>> pgssEntry *entry;
>>
>> + /* For the purposes of this module, we consider pg_read_all_stats members to be superusers */
>> + is_superuser = (superuser() || is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS));
>
> Whoahhhh, let's *not* set an 'is_superuser' flag for a case where the
> user is not, actually, a superuser. This should be 'allowed_role' or
> something along those lines, I'm thinking.
Fixed.
>> diff --git a/contrib/pg_visibility/Makefile b/contrib/pg_visibility/Makefile
>> index bc42944426..21d787ddf7 100644
>> --- a/contrib/pg_visibility/Makefile
>> +++ b/contrib/pg_visibility/Makefile
>> @@ -4,7 +4,8 @@ MODULE_big = pg_visibility
>> OBJS = pg_visibility.o $(WIN32RES)
>>
>> EXTENSION = pg_visibility
>> -DATA = pg_visibility--1.1.sql pg_visibility--1.0--1.1.sql
>> +DATA = pg_visibility--1.1.sql pg_visibility--1.1--1.2.sql \
>> + pg_visibility--1.0--1.1.sql
>> PGFILEDESC = "pg_visibility - page visibility information"
>>
>> REGRESS = pg_visibility
>
> Missing file here also.
Yeah, I forgot to add this and the others. Sorry about - fixed now.
>> diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c
>> index db9e0349a0..c9194d8c0d 100644
>> --- a/contrib/pgrowlocks/pgrowlocks.c
>> +++ b/contrib/pgrowlocks/pgrowlocks.c
>> @@ -28,6 +28,7 @@
>> #include "access/relscan.h"
>> #include "access/xact.h"
>> #include "catalog/namespace.h"
>> +#include "catalog/pg_authid.h"
>> #include "funcapi.h"
>> #include "miscadmin.h"
>> #include "storage/bufmgr.h"
>> @@ -98,9 +99,11 @@ pgrowlocks(PG_FUNCTION_ARGS)
>> relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
>> rel = heap_openrv(relrv, AccessShareLock);
>>
>> - /* check permissions: must have SELECT on table */
>> - aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
>> - ACL_SELECT);
>> + /* check permissions: must have SELECT on table or be in pg_read_all_stats */
>> + aclresult = (pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
>> + ACL_SELECT) ||
>> + is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS));
>> +
>> if (aclresult != ACLCHECK_OK)
>> aclcheck_error(aclresult, ACL_KIND_CLASS,
>> RelationGetRelationName(rel));
>
> Doesn't this go beyond just pg_stat_X, but actually allows running
> pgrowlocks against any relation? That seems like it should be
> independent, though it actually fits the "global-read-metadata" role
> case that has been discussed previously as being what pg_visibility,
> pgstattuple, and other similar contrib modules need. I don't have a
> great name for that role, but it seems different to me from
> 'pg_read_all_stats', though I don't hold that position very strongly as
> I can see an argument that these kind of are stats.
That was the way I was leaning, on the grounds that the distinction
between two separate roles might end up being confusing for users.
It's simple enough to change if that's the consencus, and we can come
up with a suitable name.
> I see you did change pgstattuple too, and perhaps these are the changes
> you intended for pg_visibility too? In any case, if so, we need to make
> it clear in the docs that pg_read_all_stats grants access to more than
> just pg_stat_X.
The pg_read_all_stats role docs have:
Read all pg_stat_* views and use various statistics related
extensions, even those normally visible only to superusers.
And then I updated the docs for each contrib module to note where
pg_read_all_stats can use functionality.
>> diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
>> index df0435c3f0..5472ff76a7 100644
>> --- a/doc/src/sgml/catalogs.sgml
>> +++ b/doc/src/sgml/catalogs.sgml
>> @@ -10027,15 +10027,17 @@ SELECT * FROM pg_locks pl LEFT JOIN pg_prepared_xacts ppx
>> <entry><type>text</type></entry>
>> <entry>Configuration file the current value was set in (null for
>> values set from sources other than configuration files, or when
>> - examined by a non-superuser);
>> - helpful when using <literal>include</> directives in configuration files</entry>
>> + examined by a user who is neither a superuser or a member of
>> + <literal>pg_read_all_settings</literal>); helpful when using
>> + <literal>include</> directives in configuration files</entry>
>> </row>
>> <row>
>> <entry><structfield>sourceline</structfield></entry>
>> <entry><type>integer</type></entry>
>> <entry>Line number within the configuration file the current value was
>> set at (null for values set from sources other than configuration files,
>> - or when examined by a non-superuser)
>> + or when examined by a user who is neither a superuser or a member of
>> + <literal>pg_read_all_settings</literal>).
>> </entry>
>> </row>
>> <row>
>
> We should really figure out a way to link back to the default roles
> section when we refer to roles from other places. Not sure what the
> best way to do that off-hand is though.
Yeah, I have no idea about that.
>> diff --git a/doc/src/sgml/pgbuffercache.sgml b/doc/src/sgml/pgbuffercache.sgml
>> index b261a4dbe0..9d1faefdd6 100644
>> --- a/doc/src/sgml/pgbuffercache.sgml
>> +++ b/doc/src/sgml/pgbuffercache.sgml
>> @@ -24,8 +24,9 @@
>> </para>
>>
>> <para>
>> - By default public access is revoked from both of these, just in case there
>> - are security issues lurking.
>> + By default use is restricted to superusers and members of the
>> + <literal>pg_read_all_stats</literal> role, just in case there are security
>> + issues lurking.
>> </para>
>>
>> <sect2>
>> diff --git a/doc/src/sgml/pgfreespacemap.sgml b/doc/src/sgml/pgfreespacemap.sgml
>> index f2f99d571e..f4f0e08473 100644
>> --- a/doc/src/sgml/pgfreespacemap.sgml
>> +++ b/doc/src/sgml/pgfreespacemap.sgml
>> @@ -16,8 +16,9 @@
>> </para>
>>
>> <para>
>> - By default public access is revoked from the functions, just in case
>> - there are security issues lurking.
>> + By default use is restricted to superusers and members of the
>> + <literal>pg_read_all_stats</literal> role, just in case there are security
>> + issues lurking.
>> </para>
>>
>> <sect2>
>
> These also look like cases where we might want to think about if we
> should have a dedicated role which, essentially, allows locking and
> scanning of all relations.
>
> In particular, I can certainly imagine admins who wish to GRANT out the
> rights to view other queries in pg_stat_activity to another admin and
> maybe even allow them to run pg_terminate_backend, or pg_cancel_query,
> but not allow them to scan every relation in the database.
Easy enough to add - thoughts on the name though? I'm struggling to
come up with anything sane.
>> diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
>> index 31c567b37e..8e5c8c507c 100644
>> --- a/src/backend/replication/walreceiver.c
>> +++ b/src/backend/replication/walreceiver.c
>> @@ -50,6 +50,7 @@
>> #include "access/timeline.h"
>> #include "access/transam.h"
>> #include "access/xlog_internal.h"
>> +#include "catalog/pg_authid.h"
>> #include "catalog/pg_type.h"
>> #include "funcapi.h"
>> #include "libpq/pqformat.h"
>> @@ -1396,7 +1397,7 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
>> /* Fetch values */
>> values[0] = Int32GetDatum(walrcv->pid);
>>
>> - if (!superuser())
>> + if (!superuser() && !is_member_of_role(GetUserId(), DEFAULT_ROLE_MONITOR))
>> {
>> /*
>> * Only superusers can see details. Other users only get the pid value
>
> Shouldn't that be DEFAULT_ROLE_READ_ALL_STATS, not MONITOR?
Ooops, fixed.
>> diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
>> index 4feb26aa7a..d1da580c9c 100644
>> --- a/src/backend/utils/misc/guc.c
>> +++ b/src/backend/utils/misc/guc.c
>> @@ -34,6 +34,7 @@
>> #include "access/xact.h"
>> #include "access/xlog_internal.h"
>> #include "catalog/namespace.h"
>> +#include "catalog/pg_authid.h"
>> #include "commands/async.h"
>> #include "commands/prepare.h"
>> #include "commands/user.h"
>> @@ -6677,10 +6678,12 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_superuser)
>> }
>> if (restrict_superuser &&
>> (record->flags & GUC_SUPERUSER_ONLY) &&
>> - !superuser())
>> + !superuser() &&
>> + !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS))
>
> Seems like having a variable to indicate if the caller should be able to
> read all these settings or not might be nice to have, so we have one
> place that figures out the privileges question.
Those checks are not all identical, and they're in different functions
so a variable wouldn't work (I assume you're not suggesting I add a
global :-p ). I could push part of each check out into a separate
function, but it doesn't seem like it would really add to the
readability to me.
> A few comments where that variable is set wouldn't be bad either.
>
> Also, I'm thinking the pg_monitor documentation really needs to be
> expanded and made very clear. I'll think a bit more on just how to try
> and phrase that, but the description included certainly seemed
> inadequate.
I've added some additional text below the table.
> That's it for a quick review. If we can get these changes done and
> there aren't any more questions or concerns, I'm happy to continue
> working to move this forward. I definitely see the usefulness of these
> changes and it'd be great to have an answer to the oft-asked question of
> how to do proper monitoring with a non-superuser role.
Thanks - updated patch attached.
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 3/22/17 09:17, Stephen Frost wrote: >> If we do it via GRANTs instead, then users can easily extend it. > The intent here is that users will *also* be able to do it via GRANTs if > they wish to. But why not do it with GRANTs in the first place then? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter, * Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote: > On 3/22/17 09:17, Stephen Frost wrote: > >> If we do it via GRANTs instead, then users can easily extend it. > > The intent here is that users will *also* be able to do it via GRANTs if > > they wish to. > > But why not do it with GRANTs in the first place then? This is akin to asking why do we need GRANT ALL and ALTER DEFAULT PRIVs. Would it be technically possible to make users jump through hoops every time they set up a new system to create their own monitor role that would then have the right set of privileges for *that* version of PG? Yes, but it's not exactly friendly. The whole idea here is that the tool authors will be able to tell the users that they just need to GRANT this one role, not a script of 20 GRANT statements, oh, and that it won't break when doing upgrades. If we make the users run all the statements individually then they'll also have to get an updated script for the next version of PG too because we will have added things that the tools will want access to. Further, they'll have to make sure to install all the contrib modules they want to use before running the GRANT script which is provided, or deal with GRANT'ing the rights out after installing some extension. With the approach that Dave and I are advocating, we can avoid all of that. Contrib modules can bake-in GRANTs to the appropriate roles, upgrades can be handled smoothly even when we add new capabilities which are appropriate, users have a simple and straight-forward way to set up good monitoring, and tool authors will know what permissions are available and can finally have a better answer than "well, just make the monior user superuser if you want to avoid all these complexities." Thanks! Stephen
On Fri, Mar 24, 2017 at 8:30 AM, Stephen Frost <sfrost@snowman.net> wrote: >> But why not do it with GRANTs in the first place then? > > This is akin to asking why do we need GRANT ALL and ALTER DEFAULT PRIVs. Not really. ALTER DEFAULT PRIVILEGES affects what happens for future objects, which isn't necessary here at all. The monitoring tool only needs to be granted the minimum set of privileges that it currently needs, not future privileges which it or other monitoring tools may need in future versions. GRANT ALL is closer, but GRANT .. ON ALL TABLES IN SCHEMA really is just a convenience function. You would still have the capability to do the exact same thing without that function; it would just be more work. And the same is true here. I understand why you and Dave want to make this a single command: that's easier. But, like GRANT ON ALL TABLES, it's also less flexible. If you want to grant on all tables except one, you can't use that magic syntax any more, and so here. pg_monitor will either target one particular monitoring solution (hey, maybe it'll be the one my employer produces!) or the judgement of one particular person (whatever Stephen Frost thinks it SHOULD do in a given release, independent of what tools on the ground do) and those aren't good definitions. > If we make the users run all the statements individually then they'll > also have to get an updated script for the next version of PG too > because we will have added things that the tools will want access to. That's possible, but it really depends on the tool, not on core PostgreSQL. The tool should be the one providing the script, because the tool is what knows its own permissions requirements. Users who care about security won't want to grant every privilege given by a pg_monitor role to a tool that only needs a subset of those privileges. > Further, they'll have to make sure to install all the contrib modules > they want to use before running the GRANT script which is provided, or > deal with GRANT'ing the rights out after installing some extension. That doesn't sound very hard. > With the approach that Dave and I are advocating, we can avoid all of > that. Contrib modules can bake-in GRANTs to the appropriate roles, > upgrades can be handled smoothly even when we add new capabilities which > are appropriate, users have a simple and straight-forward way to set up > good monitoring, and tool authors will know what permissions are > available and can finally have a better answer than "well, just make the > monior user superuser if you want to avoid all these complexities." They can have that anyway. They just have to run a script provided by the tool rather than one provided by the core project as a one-size-fits-all solution for every tool. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Mar 24, 2017 at 4:24 PM, Robert Haas <robertmhaas@gmail.com> wrote: > >> If we make the users run all the statements individually then they'll >> also have to get an updated script for the next version of PG too >> because we will have added things that the tools will want access to. > > That's possible, but it really depends on the tool, not on core > PostgreSQL. The tool should be the one providing the script, because > the tool is what knows its own permissions requirements. Users who > care about security won't want to grant every privilege given by a > pg_monitor role to a tool that only needs a subset of those > privileges. The upshot of this would be that every time there's a database server upgrade that changes the permissions required somehow, the user has to login to every server they have and run a script. It is no longer a one-time thing, which makes it vastly more painful to deal with upgrades. >> With the approach that Dave and I are advocating, we can avoid all of >> that. Contrib modules can bake-in GRANTs to the appropriate roles, >> upgrades can be handled smoothly even when we add new capabilities which >> are appropriate, users have a simple and straight-forward way to set up >> good monitoring, and tool authors will know what permissions are >> available and can finally have a better answer than "well, just make the >> monior user superuser if you want to avoid all these complexities." > > They can have that anyway. They just have to run a script provided by > the tool rather than one provided by the core project as a > one-size-fits-all solution for every tool. Do you object to having individual default roles specifically for cases where there are superuser-only checks at the moment that prevent GRANT? e.g. pg_show_all_settings for SHOW, pg_show_all_stats for pg_tablespace_size and friends, pg_stat_statements for, well, pg_stat_statements and so on? It would be an inferior solution in my opinion, given that it would demonstrably cause users more work, but at least we could do something. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Mar 24, 2017 at 12:46 PM, Dave Page <dpage@pgadmin.org> wrote: > On Fri, Mar 24, 2017 at 4:24 PM, Robert Haas <robertmhaas@gmail.com> wrote: >>> If we make the users run all the statements individually then they'll >>> also have to get an updated script for the next version of PG too >>> because we will have added things that the tools will want access to. >> >> That's possible, but it really depends on the tool, not on core >> PostgreSQL. The tool should be the one providing the script, because >> the tool is what knows its own permissions requirements. Users who >> care about security won't want to grant every privilege given by a >> pg_monitor role to a tool that only needs a subset of those >> privileges. > > The upshot of this would be that every time there's a database server > upgrade that changes the permissions required somehow, the user has to > login to every server they have and run a script. It is no longer a > one-time thing, which makes it vastly more painful to deal with > upgrades. So, I might be all wet here, but I would have expected that changes on the TOOL side would be vastly more frequent. I mean, we do not change what a certain builtin permission does very often. If we add pg_read_all_settings, what is the likelihood that the remit of that role is ever going to change? I would judge that was is vastly more likely is that a new version of some tool would start needing that privilege (or some other) where it didn't before. If that happens, and the script is on the tool side, then you just add a new line to the script. If the script is built into initdb, then you've got to wait for the next major release before you can update the definition of pg_monitor - and maybe argue with other tool authors with different requirements. >>> With the approach that Dave and I are advocating, we can avoid all of >>> that. Contrib modules can bake-in GRANTs to the appropriate roles, >>> upgrades can be handled smoothly even when we add new capabilities which >>> are appropriate, users have a simple and straight-forward way to set up >>> good monitoring, and tool authors will know what permissions are >>> available and can finally have a better answer than "well, just make the >>> monior user superuser if you want to avoid all these complexities." >> >> They can have that anyway. They just have to run a script provided by >> the tool rather than one provided by the core project as a >> one-size-fits-all solution for every tool. > > Do you object to having individual default roles specifically for > cases where there are superuser-only checks at the moment that prevent > GRANT? e.g. pg_show_all_settings for SHOW, pg_show_all_stats for > pg_tablespace_size and friends, pg_stat_statements for, well, > pg_stat_statements and so on? It would be an inferior solution in my > opinion, given that it would demonstrably cause users more work, but > at least we could do something. I think pg_show_all_settings is brilliant. It's narrowly targeted and in no way redundant with what can anyway be done with GRANT otherwise. As far as the others, I think that we should just let people GRANT privileges one by one whenever that's possible, and use built-in roles where it isn't. So I'm fine with this: - if (tblspcOid != MyDatabaseTableSpace) + if (tblspcOid != MyDatabaseTableSpace && + !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS)) But I don't like this: +GRANT EXECUTE ON FUNCTION pgstattuple(text) TO pg_read_all_stats; My position is that execute permission on a function is already grantable, so granting it by default to a built-in role is just removing flexibility which would otherwise be available to the user. I do understand that in some cases that may result in a long list of GRANT commands to make a particular monitoring tool work, but I think the right solution is to package those commands in an extension or script distributed with that tool. When there's a permission that is reasonably necessary for monitoring tools (or some other reasonable purpose) and we can't arrange for that permission to be given via a GRANT EXECUTE ON FUNCTION, then I support adding built-in roles for those things. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Fri, Mar 24, 2017 at 12:46 PM, Dave Page <dpage@pgadmin.org> wrote: > > On Fri, Mar 24, 2017 at 4:24 PM, Robert Haas <robertmhaas@gmail.com> wrote: > >> That's possible, but it really depends on the tool, not on core > >> PostgreSQL. The tool should be the one providing the script, because > >> the tool is what knows its own permissions requirements. Users who > >> care about security won't want to grant every privilege given by a > >> pg_monitor role to a tool that only needs a subset of those > >> privileges. > > > > The upshot of this would be that every time there's a database server > > upgrade that changes the permissions required somehow, the user has to > > login to every server they have and run a script. It is no longer a > > one-time thing, which makes it vastly more painful to deal with > > upgrades. > > So, I might be all wet here, but I would have expected that changes on > the TOOL side would be vastly more frequent. I mean, we do not change > what a certain builtin permission does very often. If we add > pg_read_all_settings, what is the likelihood that the remit of that > role is ever going to change? I would judge that was is vastly more > likely is that a new version of some tool would start needing that > privilege (or some other) where it didn't before. If that happens, > and the script is on the tool side, then you just add a new line to > the script. If the script is built into initdb, then you've got to > wait for the next major release before you can update the definition > of pg_monitor - and maybe argue with other tool authors with different > requirements. The expectation here is that the pg_monitor role will include all of the reasonable privileges in a given release that a monitor tool should be using. While it's likely that monitoring tools will not all be able to work with every function they would then have access to on day 1, I'm sure the goal for all of them will be to reach the point where they are using all of the functions and tables they then have access to. Moving forward, the privileges being added in future versions of PG (the ones you point out as happening at initdb-time) would be only those which are associated with new features in those later versions of PG. Those new features aren't going to be back-patched and therefore it wouldn't be possible for the tool to provide a script to GRANT access to those new features which would work on older versions of PG, meaning that the script you suggest would necessairly be PG-version specific. In other words, I don't believe this is a valid concern. > So I'm fine with this: > > - if (tblspcOid != MyDatabaseTableSpace) > + if (tblspcOid != MyDatabaseTableSpace && > + !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS)) > > But I don't like this: > > +GRANT EXECUTE ON FUNCTION pgstattuple(text) TO pg_read_all_stats; I believe we understood that to be the case. I don't object to the clarification, of course. > My position is that execute permission on a function is already > grantable, so granting it by default to a built-in role is just > removing flexibility which would otherwise be available to the user. To remove flexibility would require that we remove the ability to independently GRANT that right. We are not doing that. Nor are we taking anything away from the user by added a new default role- we already claimed the pg_ namespace for roles in 9.6. Thanks! Stephen
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Fri, Mar 24, 2017 at 8:30 AM, Stephen Frost <sfrost@snowman.net> wrote: > >> But why not do it with GRANTs in the first place then? > > > > This is akin to asking why do we need GRANT ALL and ALTER DEFAULT PRIVs. I wasn't very clear with my thinking as to why I felt it was akin to those other commands, please let me explain it a bit more clearly. Consider if all the needs of a monitoring role existed in a 'monitor' schema, and ignore the fact that pg_upgrade ignores default privileges on upgrade and instead think of this from the perspective of developing an application in PG. In such a case, users could create a 'monitor' role, GRANT ALL to that user on the functions and tables and views in the 'monitor' schema, and set up DEFAULT PRIVs for the monitor role to have access to such future functions/tables/etc as might be created in the future in that schema. There's no way for users to do that today with those pieces of the system which are associated with monitoring. We are proposing given a way for users to do that, with this proposed default role. > Not really. ALTER DEFAULT PRIVILEGES affects what happens for future > objects, which isn't necessary here at all. The monitoring tool only > needs to be granted the minimum set of privileges that it currently > needs, not future privileges which it or other monitoring tools may > need in future versions. GRANT ALL is closer, but GRANT .. ON ALL > TABLES IN SCHEMA really is just a convenience function. You would > still have the capability to do the exact same thing without that > function; it would just be more work. And the same is true here. I > understand why you and Dave want to make this a single command: that's > easier. But, like GRANT ON ALL TABLES, it's also less flexible. If > you want to grant on all tables except one, you can't use that magic > syntax any more, and so here. pg_monitor will either target one > particular monitoring solution (hey, maybe it'll be the one my > employer produces!) or the judgement of one particular person > (whatever Stephen Frost thinks it SHOULD do in a given release, > independent of what tools on the ground do) and those aren't good > definitions. I don't believe the assertion that when we added GRANT ON ALL TABLES that we reduced flexibility of the system. I do agree that it's a convenience function- a valuable enough one that we added it. It is certainly possible to have exceptions to GRANT ON ALL TABLES- you simply remove those privileges which were GRANT'd, in the same transaction. You are correct that it wouldn't be possible to remove pg_monitor's rights and users would instead need to GRANT just those privileges they wish their monitor user to have, but that is the exception to the rule. As it relates to tools, I might be wrong, but it certainly seems like Dave and I, from two independent consulting organizations, are on the same page when it comes to what this pg_monitor role should have access to, with, frankly, very little discussion needed. The point about least-privilege is certainly one I understand, but I don't believe it's really properly applied here. While a given monitoring tool might not use function X today and therefore from a strict least-privilege standpoint it shouldn't have access to it, but what is the risk of GRANT'ing that access to the monitor user? Is the expectation that a future version of that tool will need access to that function? If the risk is low and the expectation is that the tool will need access in the future, then GRANT'ing the access is entirely reasonable. Consider that an accountant might not need access to the employee/manager relationships today because they are processing only invoices. Tomorrow, however, they will need access because they are reviewing Purchase Requests. Does it make sense to only GRANT the access they will actually use at the start of each day? These are all trade-offs to consider between the risk of granting access and not. There will undoubtably be environments where the tool will actually support using function X, but the user doesn't feel comfortable with giving the monitoring user even that access and, therefore, tools will need to at least fail gracefully in such cases, but those are the exceptions, and will be happy to carefully craft their privileges based on their own determination of the trade-offs. For the vast majority of users, allowing the monitor user access to basically everything, as long as it can't become super-user or directly view/modify the data, will cover the attack vectors they are most concerned with, just like the GRANT ON ALL and ALTER DEFAULT PRIVS satisfy very similar use-cases for a lot of users. > > With the approach that Dave and I are advocating, we can avoid all of > > that. Contrib modules can bake-in GRANTs to the appropriate roles, > > upgrades can be handled smoothly even when we add new capabilities which > > are appropriate, users have a simple and straight-forward way to set up > > good monitoring, and tool authors will know what permissions are > > available and can finally have a better answer than "well, just make the > > monior user superuser if you want to avoid all these complexities." > > They can have that anyway. They just have to run a script provided by > the tool rather than one provided by the core project as a > one-size-fits-all solution for every tool. The one-size-fits-all solution actually works perfectly because it's "all the privileges needed to monitor PG for version X". We're not asking for some set of privileges which is tailored to Dave's monitoring solution, or which is tailored to Stephen's monitoring solution, or to check_postgres, or to some other specific monitoring tool. In the general sense, it isn't actually very difficult to define what capabilities PG has to assist monitoring of the system, nor to define what privileges monitoring tools should have on those capabilities. Maybe that will become an issue some day, but I tend to doubt it- certainly the other systems to which PG is regularly compared have solved this, even in cases where their monitoring capabilities are far beyond what we support today. Thanks! Stephen
Hi > On 25 Mar 2017, at 15:55, Stephen Frost <sfrost@snowman.net> wrote: > > Robert, > > * Robert Haas (robertmhaas@gmail.com) wrote: >>> On Fri, Mar 24, 2017 at 12:46 PM, Dave Page <dpage@pgadmin.org> wrote: >>>> On Fri, Mar 24, 2017 at 4:24 PM, Robert Haas <robertmhaas@gmail.com> wrote: >>>> That's possible, but it really depends on the tool, not on core >>>> PostgreSQL. The tool should be the one providing the script, because >>>> the tool is what knows its own permissions requirements. Users who >>>> care about security won't want to grant every privilege given by a >>>> pg_monitor role to a tool that only needs a subset of those >>>> privileges. >>> >>> The upshot of this would be that every time there's a database server >>> upgrade that changes the permissions required somehow, the user has to >>> login to every server they have and run a script. It is no longer a >>> one-time thing, which makes it vastly more painful to deal with >>> upgrades. >> >> So, I might be all wet here, but I would have expected that changes on >> the TOOL side would be vastly more frequent. I mean, we do not change >> what a certain builtin permission does very often. If we add >> pg_read_all_settings, what is the likelihood that the remit of that >> role is ever going to change? I would judge that was is vastly more >> likely is that a new version of some tool would start needing that >> privilege (or some other) where it didn't before. If that happens, >> and the script is on the tool side, then you just add a new line to >> the script. If the script is built into initdb, then you've got to >> wait for the next major release before you can update the definition >> of pg_monitor - and maybe argue with other tool authors with different >> requirements. > > The expectation here is that the pg_monitor role will include all of the > reasonable privileges in a given release that a monitor tool should be > using. Exactly. > > While it's likely that monitoring tools will not all be able to work > with every function they would then have access to on day 1, I'm sure > the goal for all of them will be to reach the point where they are using > all of the functions and tables they then have access to. Right - and we have discussed here, in at least one developer meeting, and with the tools team at EDB what should be included,so I'm confident we have a solid starting point that would be generally useful. > > Moving forward, the privileges being added in future versions of PG (the > ones you point out as happening at initdb-time) would be only those > which are associated with new features in those later versions of PG. > Those new features aren't going to be back-patched and therefore it > wouldn't be possible for the tool to provide a script to GRANT access to > those new features which would work on older versions of PG, meaning > that the script you suggest would necessairly be PG-version specific. Indeed. Another issue with having the admin run a script is that any new databases created from template0 or the default template1,may all need the script to be run at that point. Admin tools that can automatically and immediately start monitoringnew databases may need manual admin/dbowner intervention which could be extremely painful in large environments. I believe this and other reasons we've described are exactly why other DBMS' do what we're proposing. > > In other words, I don't believe this is a valid concern. > >> So I'm fine with this: >> >> - if (tblspcOid != MyDatabaseTableSpace) >> + if (tblspcOid != MyDatabaseTableSpace && >> + !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_STATS)) >> >> But I don't like this: >> >> +GRANT EXECUTE ON FUNCTION pgstattuple(text) TO pg_read_all_stats; > > I believe we understood that to be the case. I don't object to the > clarification, of course. > >> My position is that execute permission on a function is already >> grantable, so granting it by default to a built-in role is just >> removing flexibility which would otherwise be available to the user. > > To remove flexibility would require that we remove the ability to > independently GRANT that right. We are not doing that. Nor are we > taking anything away from the user by added a new default role- we > already claimed the pg_ namespace for roles in 9.6. Right - and if a user doesn't like it, they can easily just not use the default role and create their own alternative instead. From a usability perspective, I cannot see any downsides to the default roles as proposed. They take nothing away from theusers that don't want/need them, and add significant convenience for those that do.
On 25 March 2017 at 16:30, Dave Page <dpage@pgadmin.org> wrote: > I believe this and other reasons we've described are exactly why other DBMS' do what we're proposing. It would help review if you could show some links and give a commentary on what you think others do, what they get right and what they get wrong, so we can be sure we are providing something people actually want and/or expect. POLA needed. I don't want to be reading various blogs about what those numpties on the Postgres project did in v10. Thanks -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 23 March 2017 at 13:16, Dave Page <dpage@pgadmin.org> wrote: > Thanks - updated patch attached. No problems with the patch so far. I'd like some tests though... -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Mar 27, 2017 at 3:51 AM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 25 March 2017 at 16:30, Dave Page <dpage@pgadmin.org> wrote: > >> I believe this and other reasons we've described are exactly why other DBMS' do what we're proposing. > > It would help review if you could show some links and give a > commentary on what you think others do, what they get right and what > they get wrong, so we can be sure we are providing something people > actually want and/or expect. POLA needed. I don't want to be reading > various blogs about what those numpties on the Postgres project did in > v10. Thanks Most other DBMSs seem to provide either capabilities (or privileges, whatever they may be called by the vendor) that can be assigned to roles, or pre-defined roles with capabilities, or some combination of the two. SQL Server provides a number of server and database level roles that are pre-configured for specific tasks, with set of capabilities. See https://msdn.microsoft.com/en-us/library/ms189612.aspx for example. DB2 appears to provide capabilities that can be assigned to roles. See https://www.ibm.com/support/knowledgecenter/SSEPGG_10.5.0/com.ibm.db2.luw.admin.sec.doc/doc/c0050531.html Oracle has something of a mix or roles and capabilities, eg. the DBA role and SYSOPER privileges, e.g. https://docs.oracle.com/cd/B28359_01/server.111/b28310/dba005.htm#ADMIN11040 What is being proposed here is a similar system, but focussing on pre-defined roles. These make it easy to grant privileges for specific purposes en-masse, without requiring the user to use them, i.e. they're free to ignore them if they wish. As they are roles, they also have the freedom to extend or restrict them in cases where privileges are acquired through GRANT. I believe this offers both the greatest flexibility and the most straightforward and easy to use interface for the end user - the ability to customise is maximised, whilst the default roles will be both safe to use and should work out of the box for the majority of monitoring scenarios. The most important thing is that we'll be able to stop users having to grant superuser privileges to their monitoring roles. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sat, Mar 25, 2017 at 12:30 PM, Dave Page <dpage@pgadmin.org> wrote: > Right - and if a user doesn't like it, they can easily just not use the default role and create their own alternative instead. OK, I see the points that both of you are making and I admit that they've got some validity to them. Let me make a modest proposal. Suppose we define the pg_monitor role as strictly a bundle of privileges that could be granted individually if desired, and similarly define pg_read_all_settings and pg_read_all_stats as roles that are strictly recognized by the code, without any grants. So instead of this: GRANT EXECUTE ON FUNCTION pgstatindex(regclass) TO pg_read_all_stats; We'd instead do this: GRANT EXECUTE ON FUNCTION pgstatindex(regclass) TO pg_monitor; (and similarly for all other GRANT statements that appear in the patch) So, if you want, you can give somebody pg_read_all_stats, enhancing their access to functions to which they already have access, but deny them execute access on some of the individual functions. The permissions with which pg_monitor ends up are unchanged, but somebody creating their own role has a bit more freedom to customize what it can do. I still don't have much confidence in the theory that every monitoring solution ever will want the same privileges, but if we unbundle things as much as we reasonably can, then the worst thing that happens if pg_monitor turns out to be not what everyone wants is that some people do it a different way instead. Which as you say is not that bad. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 23 March 2017 at 13:16, Dave Page <dpage@pgadmin.org> wrote: > Thanks - updated patch attached. I've edited the comments and docs on this patch, so attach a new version with similar name. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On 27 March 2017 at 13:54, Robert Haas <robertmhaas@gmail.com> wrote: > On Sat, Mar 25, 2017 at 12:30 PM, Dave Page <dpage@pgadmin.org> wrote: >> Right - and if a user doesn't like it, they can easily just not use the default role and create their own alternativeinstead. > > OK, I see the points that both of you are making and I admit that > they've got some validity to them. They do. There are clearly some points we agree on and some we don't. If we do not agree, we will have missed an opportunity in this release. > Let me make a modest proposal. +1 I aim to commit something today. If you have other viewpoints, so say now. There aren't many days left to express views and do anything about them, so lets do it. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/28/17 10:24, Simon Riggs wrote: >> Let me make a modest proposal. > > +1 > > I aim to commit something today. If you have other viewpoints, so say > now. There aren't many days left to express views and do anything > about them, so lets do it. I don't even see an actual proposed patch here, so what are we supposed to comment on? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
This patch touches the pg_buffercache and pg_freespacemap extensions, but there appear to be some files missing. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Mar 28, 2017 at 11:31 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > This patch touches the pg_buffercache and pg_freespacemap extensions, > but there appear to be some files missing. Are you looking at an old version? There was one where I forgot to add some files, but that was fixed within an hour or so in a new version. Right now I'm waiting for discussion to conclude before updating the patch again. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 3/27/17 08:54, Robert Haas wrote: > OK, I see the points that both of you are making and I admit that > they've got some validity to them. Let me make a modest proposal. > Suppose we define the pg_monitor role as strictly a bundle of > privileges that could be granted individually if desired, and > similarly define pg_read_all_settings and pg_read_all_stats as roles > that are strictly recognized by the code, without any grants. I think this is very important. We can't have roles that do both. That will just cause confusion. Security confusion. I'm confused about the definition of the pg_read_all_stats role. It seems to give read access to just about everything that rhymes with "stats" as well as a random collection of other things such as table sizes. Except actual table statistics, which was my first guess. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/28/17 11:34, Dave Page wrote: > On Tue, Mar 28, 2017 at 11:31 AM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> This patch touches the pg_buffercache and pg_freespacemap extensions, >> but there appear to be some files missing. > > Are you looking at an old version? There was one where I forgot to add > some files, but that was fixed within an hour or so in a new version. What is the name and date or your latest patch? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On Mar 28, 2017, at 8:34 AM, Dave Page <dpage@pgadmin.org> wrote: > > On Tue, Mar 28, 2017 at 11:31 AM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> This patch touches the pg_buffercache and pg_freespacemap extensions, >> but there appear to be some files missing. > > Are you looking at an old version? There was one where I forgot to add > some files, but that was fixed within an hour or so in a new version. > > Right now I'm waiting for discussion to conclude before updating the > patch again. There does not seem to be a new patch since Robert made his "modest proposal", so I guess I just have to ask questions about how this would work. I don't see any precedent in the code for having a hardcoded role, other than superuser, and allowing privileges based on a hardcoded test for membership in that role. I'm struggling to think of all the security implications of that. If I have even one table in my database which is security sensitive, such that I cannot allow users to see the size of the table, nor whether the table has unvacuumed rows (owing to the fact that would give away that it has been changed since the last vacuum time), then I can't use pg_real_all_stats for anything, right? And I would need to exercise some due diligence to make certain it does not get granted to anybody? What happens if I execute: REVOKE ALL ON TABLE mysecuretable FROM pg_read_all_stats? Does it work? Does it silently fail? Does it raise an exception? Does pg_read_all_stats still have access to stats for mysecuretable? mark
On Tue, Mar 28, 2017 at 11:39 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 3/28/17 11:34, Dave Page wrote: >> On Tue, Mar 28, 2017 at 11:31 AM, Peter Eisentraut >> <peter.eisentraut@2ndquadrant.com> wrote: >>> This patch touches the pg_buffercache and pg_freespacemap extensions, >>> but there appear to be some files missing. >> >> Are you looking at an old version? There was one where I forgot to add >> some files, but that was fixed within an hour or so in a new version. > > What is the name and date or your latest patch? pg_monitor_v4.diff, 23rd March. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Mar 28, 2017 at 12:04 PM, Mark Dilger <hornschnorter@gmail.com> wrote: > >> On Mar 28, 2017, at 8:34 AM, Dave Page <dpage@pgadmin.org> wrote: >> >> On Tue, Mar 28, 2017 at 11:31 AM, Peter Eisentraut >> <peter.eisentraut@2ndquadrant.com> wrote: >>> This patch touches the pg_buffercache and pg_freespacemap extensions, >>> but there appear to be some files missing. >> >> Are you looking at an old version? There was one where I forgot to add >> some files, but that was fixed within an hour or so in a new version. >> >> Right now I'm waiting for discussion to conclude before updating the >> patch again. > > There does not seem to be a new patch since Robert made his "modest proposal", > so I guess I just have to ask questions about how this would work. There hasn't been one yet. > I don't see any precedent in the code for having a hardcoded role, other than > superuser, and allowing privileges based on a hardcoded test for membership > in that role. I'm struggling to think of all the security implications of that. This would be the first. > If I have even one table in my database which is security sensitive, such that > I cannot allow users to see the size of the table, nor whether the table has > unvacuumed rows (owing to the fact that would give away that it has been > changed since the last vacuum time), then I can't use pg_real_all_stats for > anything, right? And I would need to exercise some due diligence to make > certain it does not get granted to anybody? Right. > What happens if I execute: > > REVOKE ALL ON TABLE mysecuretable FROM pg_read_all_stats? > > Does it work? Yes, for the documented use of GRANT/REVOKE on a table. > Does it silently fail? Does it raise an exception? No and no. > Does > pg_read_all_stats still have access to stats for mysecuretable? Yes, because the ACL on the table controls reading/writing the data in the table. It doesn't have any bearing on any kind of table metadata. A user who has no privileges on a table can already look at (for example) pg_stat_all_tables and see the sort of info you're talking about. This patch would just allow members of a specific role get the on-disk size as well, *if* you choose to use it. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Mar 28, 2017 at 12:47 PM, Dave Page <dpage@pgadmin.org> wrote: >> I don't see any precedent in the code for having a hardcoded role, other than >> superuser, and allowing privileges based on a hardcoded test for membership >> in that role. I'm struggling to think of all the security implications of that. > > This would be the first. Isn't pg_signal_backend an existing precedent? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Mar 28, 2017 at 12:55 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Tue, Mar 28, 2017 at 12:47 PM, Dave Page <dpage@pgadmin.org> wrote: >>> I don't see any precedent in the code for having a hardcoded role, other than >>> superuser, and allowing privileges based on a hardcoded test for membership >>> in that role. I'm struggling to think of all the security implications of that. >> >> This would be the first. > > Isn't pg_signal_backend an existing precedent? Good point. Clearly time for some caffeine. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Greetings, * Robert Haas (robertmhaas@gmail.com) wrote: > On Tue, Mar 28, 2017 at 12:47 PM, Dave Page <dpage@pgadmin.org> wrote: > >> I don't see any precedent in the code for having a hardcoded role, other than > >> superuser, and allowing privileges based on a hardcoded test for membership > >> in that role. I'm struggling to think of all the security implications of that. > > > > This would be the first. > > Isn't pg_signal_backend an existing precedent? Yes, it is. Thanks! Stephen
OK, so before I start hacking again, here's a proposal based on my understanding of folks comments, and so open questions. If I can get agreement and answers, I'll be able to break out vi again without (hopefully) too many more revisions: pg_read_all_stats: Will have C-coded access to pg_stats views and pg_*_size that are currently hard-coded to superuser pg_read_all_settings: Will have C-coded access to read GUCs that are currently hard-coded to the superuser pg_monitor: Will include pg_read_all_stats and pg_read_all_settings, and all explicitly GRANTable rights, e.g. in contrib modules. Patch to be rebased on Simon's updated version. Questions: - pg_stat_statements has a hard-coded superuser check. Shall I remove that, and include REVOKE ALL FROM ALL and then GRANT to pg_monitor? - pgrowlocks has hard-coded access to superuser and users with SELECT on the table being examined. How should this be handled? - Stephen suggested a separate role for functions that can lock tables. Is this still desired, or shall we just grant access to pg_monitor (I think the latter is fine)? - Based on Peter's concerns, is pg_read_all_stats the right name? Maybe pg_read_monitoring_stats? Thanks! On Tue, Mar 28, 2017 at 1:37 PM, Stephen Frost <sfrost@snowman.net> wrote: > Greetings, > > * Robert Haas (robertmhaas@gmail.com) wrote: >> On Tue, Mar 28, 2017 at 12:47 PM, Dave Page <dpage@pgadmin.org> wrote: >> >> I don't see any precedent in the code for having a hardcoded role, other than >> >> superuser, and allowing privileges based on a hardcoded test for membership >> >> in that role. I'm struggling to think of all the security implications of that. >> > >> > This would be the first. >> >> Isn't pg_signal_backend an existing precedent? > > Yes, it is. > > Thanks! > > Stephen -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> On Mar 28, 2017, at 9:55 AM, Robert Haas <robertmhaas@gmail.com> wrote: > > On Tue, Mar 28, 2017 at 12:47 PM, Dave Page <dpage@pgadmin.org> wrote: >>> I don't see any precedent in the code for having a hardcoded role, other than >>> superuser, and allowing privileges based on a hardcoded test for membership >>> in that role. I'm struggling to think of all the security implications of that. >> >> This would be the first. > > Isn't pg_signal_backend an existing precedent? Sorry, I meant to say that there is no precedent for allowing access to data based on a hardcoded test for membership in a role other than superuser. All the locations that use pg_signal_backend are checking for something other than data access privileges. That distinction was clear to me in the context of what I was saying, but I obviously didn't phrase it right in my email. mark
On Tue, Mar 28, 2017 at 1:52 PM, Mark Dilger <hornschnorter@gmail.com> wrote: > >> On Mar 28, 2017, at 9:55 AM, Robert Haas <robertmhaas@gmail.com> wrote: >> >> On Tue, Mar 28, 2017 at 12:47 PM, Dave Page <dpage@pgadmin.org> wrote: >>>> I don't see any precedent in the code for having a hardcoded role, other than >>>> superuser, and allowing privileges based on a hardcoded test for membership >>>> in that role. I'm struggling to think of all the security implications of that. >>> >>> This would be the first. >> >> Isn't pg_signal_backend an existing precedent? > > Sorry, I meant to say that there is no precedent for allowing access to data based > on a hardcoded test for membership in a role other than superuser. This doesn't allow access to data, except through monitoring of queries that are executed (e.g. full access to pg_stat_activity) - which you can avoid by not using the role if that's your choice. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> On Mar 28, 2017, at 9:47 AM, Dave Page <dpage@pgadmin.org> wrote: > >> Does >> pg_read_all_stats still have access to stats for mysecuretable? > > Yes, because the ACL on the table controls reading/writing the data in > the table. It doesn't have any bearing on any kind of table metadata. > A user who has no privileges on a table can already look at (for > example) pg_stat_all_tables and see the sort of info you're talking > about. This patch would just allow members of a specific role get the > on-disk size as well, *if* you choose to use it. I don't entirely agree here. Security conscious database users may well restrict access to that view. I added a check to the regression tests to verify that it works: + SET ROLE regress_locktable_user; + SELECT COUNT(*) FROM pg_stat_all_tables; + count + ------- + 290 + (1 row) + + RESET ROLE; + REVOKE ALL PRIVILEGES ON pg_stat_all_tables FROM PUBLIC; + SET ROLE regress_locktable_user; + SELECT COUNT(*) FROM pg_stat_all_tables; + ERROR: permission denied for relation pg_stat_all_tables + RESET ROLE; + SELECT COUNT(*) FROM pg_stat_all_tables; + count + ------- + 292 + (1 row) + The inability to revoke access to this sort of information being proposed makes me a bit uneasy. Mostly, I think, I'm bothered because there may be people who have revoked privileges on a lot of things, thereby restricting access to superuser, who won't necessarily notice this new feature in postgres 10. That could be a source of security holes that we get blamed for. Please note that I'm not specifically opposed to this work, and see a lot of merit here. I'm just thinking about unintended consequences. mark
Dave, * Dave Page (dpage@pgadmin.org) wrote: > OK, so before I start hacking again, here's a proposal based on my > understanding of folks comments, and so open questions. If I can get > agreement and answers, I'll be able to break out vi again without > (hopefully) too many more revisions: > > pg_read_all_stats: Will have C-coded access to pg_stats views and > pg_*_size that are currently hard-coded to superuser Not quite sure what you mean here by 'C-coded access to pg_stats *views*', but I'm guessing that isn't exactly what you meant since I'm sure we aren't going to change how view permissions are done in this patch. I take it you mean access to the functions under the views? If so, I believe this is correct. > pg_read_all_settings: Will have C-coded access to read GUCs that are > currently hard-coded to the superuser Right. > pg_monitor: Will include pg_read_all_stats and pg_read_all_settings, > and all explicitly GRANTable rights, e.g. in contrib modules. Right. > Patch to be rebased on Simon's updated version. Right. > Questions: > > - pg_stat_statements has a hard-coded superuser check. Shall I remove > that, and include REVOKE ALL FROM ALL and then GRANT to pg_monitor? pg_stat_statements shouldn't have ever had that superuser check and it shouldn't have ever used '==' for the user check, it should have been using 'has_privs_of_role()' from the start, which means that the superuser check isn't necessary. I don't think we should remove that check, nor should we REVOKE EXECUTE from public for the function. We *should* add a hard-coded role check to allow another role which isn't a member of the role whose query it is to view the query. That shouldn't be pg_monitor, of course (as discussed). I don't think pg_read_all_stats or pg_read_all_settings really covers this case either- this is more like pg_read_all_queries and should also be used for pg_stat_activity. That would then be granted to pg_monitor. > - pgrowlocks has hard-coded access to superuser and users with SELECT > on the table being examined. How should this be handled? I don't see any hard-coded superuser check? There is a pg_class_aclcheck() for SELECT rights on the table. I like the idea of having another way to grant access to run this function on a table besides giving SELECT rights on the entire table to the user. This would fall under the mandate of the role described in your next bullet, in my view. > - Stephen suggested a separate role for functions that can lock > tables. Is this still desired, or shall we just grant access to > pg_monitor (I think the latter is fine)? Right, I was thinking something like pg_stat_all_tables or pg_stat_scan_tables or similar. We would add that (perhaps also with a SELECT check like pgrowlocks has) for the other functions like pgstattuple and pg_freespacemap and pg_visibility. > - Based on Peter's concerns, is pg_read_all_stats the right name? > Maybe pg_read_monitoring_stats? I'd rather not get too wrapped up in the naming.. The documentation is the important part here, imv. Thanks! Stephen
Greetings, * Mark Dilger (hornschnorter@gmail.com) wrote: > The inability to revoke access to this sort of information being proposed > makes me a bit uneasy. What data are you concerned about, specifically? > Mostly, I think, I'm bothered because there may > be people who have revoked privileges on a lot of things, thereby restricting > access to superuser, who won't necessarily notice this new feature in > postgres 10. That could be a source of security holes that we get blamed > for. There is no access granted by adding this role without an admin granting access to this role to some other user. If they make incorrect assumptions about what granting access to this role means then I'm afraid that's their issue, not ours. > Please note that I'm not specifically opposed to this work, and see a lot > of merit here. I'm just thinking about unintended consequences. Certainly, good to think of, but I don't believe there's a concern here. Thanks! Stephen
> On Mar 28, 2017, at 11:06 AM, Mark Dilger <hornschnorter@gmail.com> wrote: > > >> On Mar 28, 2017, at 9:47 AM, Dave Page <dpage@pgadmin.org> wrote: >> >>> Does >>> pg_read_all_stats still have access to stats for mysecuretable? >> >> Yes, because the ACL on the table controls reading/writing the data in >> the table. It doesn't have any bearing on any kind of table metadata. >> A user who has no privileges on a table can already look at (for >> example) pg_stat_all_tables and see the sort of info you're talking >> about. This patch would just allow members of a specific role get the >> on-disk size as well, *if* you choose to use it. > > I don't entirely agree here. Security conscious database users may well > restrict access to that view. I added a check to the regression tests to > verify that it works: > > + SET ROLE regress_locktable_user; > + SELECT COUNT(*) FROM pg_stat_all_tables; > + count > + ------- > + 290 > + (1 row) > + > + RESET ROLE; > + REVOKE ALL PRIVILEGES ON pg_stat_all_tables FROM PUBLIC; > + SET ROLE regress_locktable_user; > + SELECT COUNT(*) FROM pg_stat_all_tables; > + ERROR: permission denied for relation pg_stat_all_tables > + RESET ROLE; > + SELECT COUNT(*) FROM pg_stat_all_tables; > + count > + ------- > + 292 > + (1 row) > + > > The inability to revoke access to this sort of information being proposed > makes me a bit uneasy. Mostly, I think, I'm bothered because there may > be people who have revoked privileges on a lot of things, thereby restricting > access to superuser, who won't necessarily notice this new feature in > postgres 10. That could be a source of security holes that we get blamed > for. After a bit of introspection, I think what is really bothering me is not the inability to revoke permissions, since as you say I can choose to not assign the role to anybody. What bothers me is that this feature implicitly redefines what is meant by the keyword PUBLIC. If I go around the database revoking privileges on everything from PUBLIC, I should end up with a database that is inaccessible to anyone but superuser, right? All views, functions, tables, etc., would all be locked down. But after this proposed change, IIUC, there would still be a bit of access available to this/these proposed non-superuser role(s) which have hardcoded permissions. That's quite a significant change to the security model of the database, and I don't think it is something users would expect from the release notes if the release notes for this feature say something like: "added database monitoring functionality" To be fair, I have not tried to revoke everything from everybody on a database to verify that their aren't already problems of this sort. Perhaps the cows have already left the barn. mark
Greetings, * Mark Dilger (hornschnorter@gmail.com) wrote: > After a bit of introspection, I think what is really bothering me is not the > inability to revoke permissions, since as you say I can choose to not assign > the role to anybody. What bothers me is that this feature implicitly redefines > what is meant by the keyword PUBLIC. If I go around the database > revoking privileges on everything from PUBLIC, I should end up with > a database that is inaccessible to anyone but superuser, right? While I accept your concern and confusion, this isn't accurate. REVOKE'ing rights from PUBLIC only removes those rights GRANT'd to PUBLIC, it doesn't remove rights which have been GRANT'd to other users. In a degenerate system where the only roles which exist are the superuser role and the pseudo-role 'public', you would be correct with your statement above, but this patch doesn't change that because you will not have GRANT'd the new role to anyone. > All views, > functions, tables, etc., would all be locked down. But after this proposed > change, IIUC, there would still be a bit of access available to this/these > proposed non-superuser role(s) which have hardcoded permissions. These non-superuser roles can't be logged into and if they are not GRANT'd to anyone then it's essentially the same as if they don't exist. > That's quite a significant change to the security model of the database, > and I don't think it is something users would expect from the release notes > if the release notes for this feature say something like: > > "added database monitoring functionality" Having more in the release notes and in the documentation is certainly important, but this is not changing the security model of the database in any significant way. > To be fair, I have not tried to revoke everything from everybody on a > database to verify that their aren't already problems of this sort. Perhaps > the cows have already left the barn. Right, which means that, in addition to the points made above, this isn't a use-case which is actually even all that interesting to consider. Thanks! Stephen
Mark Dilger <hornschnorter@gmail.com> writes:
> After a bit of introspection, I think what is really bothering me is not the
> inability to revoke permissions, since as you say I can choose to not assign
> the role to anybody. What bothers me is that this feature implicitly redefines
> what is meant by the keyword PUBLIC. If I go around the database
> revoking privileges on everything from PUBLIC, I should end up with
> a database that is inaccessible to anyone but superuser, right?
Ummm ... not if you've granted specific permissions to some users.
If you did "GRANT SELECT ON TABLE x TO joe", no amount of public-privilege
revocation makes that go away. This isn't so much different.
It's fair to object that the problem with this approach is that the
permissions available to these special roles aren't visible in the
privilege-related system catalog columns. But we've been around on that
discussion and agreed that it's impractical to have a separate GRANT bit
for every little bit of privilege that someone might want. So the plan is
to have these special roles that are known at the C-code level, and then
granting one of these roles to user X effectively grants user X certain
fine-grained privileges. But you can't see anything except the role grant
in the catalogs. You just have to read the documentation to find out what
that really means.
> But after this proposed
> change, IIUC, there would still be a bit of access available to this/these
> proposed non-superuser role(s) which have hardcoded permissions.
Right, but they are roles not users, ie they do not have the LOGIN bit.
So the only way to use them is via GRANT.
I think there is a fair question about how to document this clearly, but
the design seems reasonable.
regards, tom lane
> On Mar 28, 2017, at 11:52 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Mark Dilger <hornschnorter@gmail.com> writes: >> After a bit of introspection, I think what is really bothering me is not the >> inability to revoke permissions, since as you say I can choose to not assign >> the role to anybody. What bothers me is that this feature implicitly redefines >> what is meant by the keyword PUBLIC. If I go around the database >> revoking privileges on everything from PUBLIC, I should end up with >> a database that is inaccessible to anyone but superuser, right? > > Ummm ... not if you've granted specific permissions to some users. > If you did "GRANT SELECT ON TABLE x TO joe", no amount of public-privilege > revocation makes that go away. This isn't so much different. I think it is pretty different. It is not unreasonable to lock down a freshly installed database by revoking privileges on various objects from PUBLIC prior to creating any users. Before the proposed change, that would have different consequences than after the proposed change. > It's fair to object that the problem with this approach is that the > permissions available to these special roles aren't visible in the > privilege-related system catalog columns. But we've been around on that > discussion and agreed that it's impractical to have a separate GRANT bit > for every little bit of privilege that someone might want. So the plan is > to have these special roles that are known at the C-code level, and then > granting one of these roles to user X effectively grants user X certain > fine-grained privileges. I'm still struggling to understand the motivation for hardcoded permissions checks. I don't see anything wrong with adding roles in pg_authid.h with a #define'd Oid. That's actually pretty helpful for anyone writing code against the database, as they don't have to look up the Oid of the role. But why not then grant privileges to that role in information_schema.sql? That would avoid all the special case logic in the code, and would mean that people could add and remove privileges from the role to suit their own security policies. For somebody configuring security policies on a database prior to creating any users, these changes could be made at that time. No need to write up an explanation. I'm guessing you have a link to a discussion where this was all discussed to death. > But you can't see anything except the role grant > in the catalogs. You just have to read the documentation to find out what > that really means. > >> But after this proposed >> change, IIUC, there would still be a bit of access available to this/these >> proposed non-superuser role(s) which have hardcoded permissions. > > Right, but they are roles not users, ie they do not have the LOGIN bit. > So the only way to use them is via GRANT. > > I think there is a fair question about how to document this clearly, but > the design seems reasonable. > > regards, tom lane
Hi On Tue, Mar 28, 2017 at 2:22 PM, Stephen Frost <sfrost@snowman.net> wrote: > Dave, > > * Dave Page (dpage@pgadmin.org) wrote: >> OK, so before I start hacking again, here's a proposal based on my >> understanding of folks comments, and so open questions. If I can get >> agreement and answers, I'll be able to break out vi again without >> (hopefully) too many more revisions: >> >> pg_read_all_stats: Will have C-coded access to pg_stats views and >> pg_*_size that are currently hard-coded to superuser > > Not quite sure what you mean here by 'C-coded access to pg_stats > *views*', but I'm guessing that isn't exactly what you meant since I'm > sure we aren't going to change how view permissions are done in this > patch. I take it you mean access to the functions under the views? If > so, I believe this is correct. Right, sorry I wasn't clear. >> pg_read_all_settings: Will have C-coded access to read GUCs that are >> currently hard-coded to the superuser > > Right. > >> pg_monitor: Will include pg_read_all_stats and pg_read_all_settings, >> and all explicitly GRANTable rights, e.g. in contrib modules. > > Right. > >> Patch to be rebased on Simon's updated version. > > Right. > >> Questions: >> >> - pg_stat_statements has a hard-coded superuser check. Shall I remove >> that, and include REVOKE ALL FROM ALL and then GRANT to pg_monitor? > > pg_stat_statements shouldn't have ever had that superuser check and it > shouldn't have ever used '==' for the user check, it should have been > using 'has_privs_of_role()' from the start, which means that the > superuser check isn't necessary. > > I don't think we should remove that check, nor should we REVOKE EXECUTE > from public for the function. We *should* add a hard-coded role check > to allow another role which isn't a member of the role whose query it is > to view the query. That shouldn't be pg_monitor, of course (as > discussed). I don't think pg_read_all_stats or pg_read_all_settings > really covers this case either- this is more like pg_read_all_queries > and should also be used for pg_stat_activity. OK, so essentially what I did, except s/pg_read_all_stats/pg_read_all_queries ? > That would then be granted to pg_monitor. > >> - pgrowlocks has hard-coded access to superuser and users with SELECT >> on the table being examined. How should this be handled? > > I don't see any hard-coded superuser check? There is a > pg_class_aclcheck() for SELECT rights on the table. I like the idea of > having another way to grant access to run this function on a table > besides giving SELECT rights on the entire table to the user. This > would fall under the mandate of the role described in your next bullet, > in my view. > >> - Stephen suggested a separate role for functions that can lock >> tables. Is this still desired, or shall we just grant access to >> pg_monitor (I think the latter is fine)? > > Right, I was thinking something like pg_stat_all_tables or > pg_stat_scan_tables or similar. We would add that (perhaps also with a > SELECT check like pgrowlocks has) for the other functions like > pgstattuple and pg_freespacemap and pg_visibility. So pgstattuple, pg_sfreespacemap, pg_visibility and pgrowlocks to be allowed access from members of pg_stat_scan_tables, which in turn is granted to pg_monitor? Thanks! -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Mark Dilger <hornschnorter@gmail.com> writes:
> I don't see anything wrong with adding roles in pg_authid.h with a #define'd
> Oid. That's actually pretty helpful for anyone writing code against the database,
> as they don't have to look up the Oid of the role.
> But why not then grant privileges to that role in information_schema.sql?
To the extent that the desired privileges can be represented at the SQL
level, I agree that that's a better solution than hard-wiring checks in C
code. The problem comes in with cases where that's not fine-grained
enough. Consider a policy like "anybody can select from pg_stat_activity,
but unless you have privilege X, the query column should read as nulls
except for sessions belonging to you". That behavior can't realistically
be represented as a separate SQL privilege. Right now we have "privilege
X" for this purpose hard-coded as "is superuser", but it would be much
better if it were associated with a grantable role.
regards, tom lane
> On Mar 28, 2017, at 1:17 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Mark Dilger <hornschnorter@gmail.com> writes: >> I don't see anything wrong with adding roles in pg_authid.h with a #define'd >> Oid. That's actually pretty helpful for anyone writing code against the database, >> as they don't have to look up the Oid of the role. > >> But why not then grant privileges to that role in information_schema.sql? > > To the extent that the desired privileges can be represented at the SQL > level, I agree that that's a better solution than hard-wiring checks in C > code. The problem comes in with cases where that's not fine-grained > enough. Consider a policy like "anybody can select from pg_stat_activity, > but unless you have privilege X, the query column should read as nulls > except for sessions belonging to you". That behavior can't realistically > be represented as a separate SQL privilege. Right now we have "privilege > X" for this purpose hard-coded as "is superuser", but it would be much > better if it were associated with a grantable role. Many thanks for all the explanation. I now understand better what the patch is trying to do, and have (with some experimentation) seen flaws in what I was saying upthread. I find the notion of a role not being a group of privileges but instead actually being a privilege confusing, and it made it hard to think about the security implications of the proposal. I'm accustomed to the idea of being able to revoke a privilege from a role, and that doesn't work if the "role" is in some sense a privilege, not a role. I might find it all easier to think about if we named these things privileges and not roles, like pg_read_stats_privilege instead of pg_read_stats_role, and then we could grant pg_read_stats_privilege to roles. But I recall you were saying upthread that you did not want to have privilege bits for each of these types of things. I hope this feature is worth the confusion it causes. mark
Dave, * Dave Page (dpage@pgadmin.org) wrote: > OK, so essentially what I did, except s/pg_read_all_stats/pg_read_all_queries ? Yup. > So pgstattuple, pg_sfreespacemap, pg_visibility and pgrowlocks to be > allowed access from members of pg_stat_scan_tables, which in turn is > granted to pg_monitor? Yes. Thanks! Stephen
On 3/28/17 12:19, Dave Page wrote: > On Tue, Mar 28, 2017 at 11:39 AM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> On 3/28/17 11:34, Dave Page wrote: >>> On Tue, Mar 28, 2017 at 11:31 AM, Peter Eisentraut >>> <peter.eisentraut@2ndquadrant.com> wrote: >>>> This patch touches the pg_buffercache and pg_freespacemap extensions, >>>> but there appear to be some files missing. >>> >>> Are you looking at an old version? There was one where I forgot to add >>> some files, but that was fixed within an hour or so in a new version. >> >> What is the name and date or your latest patch? > > pg_monitor_v4.diff, 23rd March. That is the patch I was looking at, and it's the one that is missing files. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote: > On 3/28/17 12:19, Dave Page wrote: > > On Tue, Mar 28, 2017 at 11:39 AM, Peter Eisentraut > > <peter.eisentraut@2ndquadrant.com> wrote: > >> On 3/28/17 11:34, Dave Page wrote: > >>> On Tue, Mar 28, 2017 at 11:31 AM, Peter Eisentraut > >>> <peter.eisentraut@2ndquadrant.com> wrote: > >>>> This patch touches the pg_buffercache and pg_freespacemap extensions, > >>>> but there appear to be some files missing. > >>> > >>> Are you looking at an old version? There was one where I forgot to add > >>> some files, but that was fixed within an hour or so in a new version. > >> > >> What is the name and date or your latest patch? > > > > pg_monitor_v4.diff, 23rd March. > > That is the patch I was looking at, and it's the one that is missing files. Dave's currently hacking on a new patch based on our discussion, so I'd suggest waiting another hour or so anyway until he's done. Might be a bit longer as he's trying to do it in a hallway at PGConf.US... Thanks! Stephen
On Wed, Mar 29, 2017 at 2:51 PM, Stephen Frost <sfrost@snowman.net> wrote: > > Dave's currently hacking on a new patch based on our discussion, so I'd > suggest waiting another hour or so anyway until he's done. > > Might be a bit longer as he's trying to do it in a hallway at > PGConf.US... Thanks Stephen. Here's an updated patch, and description of the changes. Simon, Stephen and Robert have looked at the description and are all happy with it \o/. Thank you to them for taking the time out of the conference to go through it with me. Here's what it does: 1) Creates the following default roles: - pg_monitor - Top-level role that is GRANTed all of the following roles by default. Also GRANTed access to some additional functions. - pg_read_all_settings - A role that can read all GUCs. - pg_read_all_stats - A role that can read un-redacted pg_stat_* views via the functions supporting them, as well as pg_database_size/pg_tablespace_size. - pg_stat_scan_tables - A role that can execute monitoring functions that may lock tables. 2) pg_database_size and pg_tablespace_size have hard-coded permission checks updated to allow execution by pg_read_all_stats. 3) GUC read permission checks for superuser have been replaced with checks for membership in pg_read_all_settings. 4) pg_buffercache functions have GRANTed execute permissions to pg_monitor. 5) pg_freespacemap functions have GRANTed execute permissions to pg_stat_scan_tables. 6) pg_stat_statements has its hard-coded permission check updated to allow execution by pg_read_all_stats, and the same role is GRANTed permission to execute pg_stat_statements_reset(). 7) pg_visibility functions have GRANTed executed permissions to pg_stat_scan_tables. 8) pgrowlocks has it's hard-coded permission check updated to allow execution by pg_stat_scan_tables, 9) pgstattuple functions have GRANTed executed permissions to pg_stat_scan_tables. 10) pg_stat_get_wal_receiver has its hard-coded permission check updated to allow execution by pg_read_all_stats 11) pg_ls_logdir and pg_ls_waldir have execute permissions GRANTed to pg_monitor 12) Un-redacted use of the functions underpinning the pg_stat_* views is available to pg_read_all_stats. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On 29 March 2017 at 21:42, Dave Page <dpage@pgadmin.org> wrote: > On Wed, Mar 29, 2017 at 2:51 PM, Stephen Frost <sfrost@snowman.net> wrote: >> >> Dave's currently hacking on a new patch based on our discussion, so I'd >> suggest waiting another hour or so anyway until he's done. >> >> Might be a bit longer as he's trying to do it in a hallway at >> PGConf.US... > > Thanks Stephen. > > Here's an updated patch, and description of the changes. Simon, > Stephen and Robert have looked at the description and are all happy > with it \o/. Thank you to them for taking the time out of the > conference to go through it with me. Moving to commit this over the next hour. Last chance... -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 30 March 2017 at 18:29, Simon Riggs <simon@2ndquadrant.com> wrote: > Moving to commit this over the next hour. Last chance... Done. Great work Dave, thanks everybody. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Mar 30, 2017 at 2:24 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On 30 March 2017 at 18:29, Simon Riggs <simon@2ndquadrant.com> wrote: > >> Moving to commit this over the next hour. Last chance... > > Done. Great work Dave, thanks everybody. Thanks Simon. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company