Re: Monitoring roles patch

Hi

On Thu, Mar 23, 2017 at 12:23 PM, Stephen Frost <sfrost(at)snowman(dot)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