Re: Monitoring roles patch

From: Stephen Frost <sfrost(at)snowman(dot)net>
To: Dave Page <dpage(at)pgadmin(dot)org>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Monitoring roles patch
Date: 2017-03-23 12:23:35
Message-ID: 20170323122335.GK9812@tamriel.snowman.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Dave,

* Dave Page (dpage(at)pgadmin(dot)org) wrote:
> On Wed, Mar 22, 2017 at 3:46 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
> > On Wed, Mar 22, 2017 at 1:15 PM, Stephen Frost <sfrost(at)snowman(dot)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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Pavel Stehule 2017-03-23 12:39:16 Re: GSOC'17 project introduction: Parallel COPY execution with errors handling
Previous Message Etsuro Fujita 2017-03-23 12:20:54 Re: postgres_fdw bug in 9.6