Date: Tue, 28 Mar 2017 14:22:46 -0400
From: Stephen Frost <sfrost@snowman.net>
To: Dave Page <dpage@pgadmin.org>
Cc: Robert Haas <robertmhaas@gmail.com>,
	Mark Dilger <hornschnorter@gmail.com>,
	Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Monitoring roles patch
Message-ID: <20170328182246.GG9812@tamriel.snowman.net>
References: 
 <CA+OCxozGTGd-x+cx03N=KJVK=soJwOpTWkq_=zac3P04aBCU4Q@mail.gmail.com>
 <20170323122335.GK9812@tamriel.snowman.net>
 <CA+OCxozZxJouNjY4+-heYSUk=qagoytQXnSjMyO5-R_a_mddKA@mail.gmail.com>
 <c1d3db24-85f0-96bb-0ec3-aeddd9d35941@2ndquadrant.com>
 <CA+OCxox8HW6obNKY5atVA6pX=Z=wQw6rTGcggy4N05k=mLkaAQ@mail.gmail.com>
 <FEE8D3A7-FDBF-432F-BEEF-F28B7D3AD2F4@gmail.com>
 <CA+OCxoy7rqEZ65byS+3E_0dsTKYD8NcUzQB1SiGknARmbxBTDg@mail.gmail.com>
 <CA+TgmoZA-gM+Un0E65seyVT45axpaH8q-QRDs8L+nSUrynkSMA@mail.gmail.com>
 <20170328173721.GF9812@tamriel.snowman.net>
 <CA+OCxozVLKLNNDDJQYc01z58GpSpxGitihG4+_V9V2VgU-fEyg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="/zZuIhN9RW1uCJRx"
Content-Disposition: inline
In-Reply-To: 
 <CA+OCxozVLKLNNDDJQYc01z58GpSpxGitihG4+_V9V2VgU-fEyg@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


--/zZuIhN9RW1uCJRx
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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:
>=20
> 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:
>=20
> - 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 '=3D=3D' 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
=66rom 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

--/zZuIhN9RW1uCJRx
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJY2qn1AAoJEO1sijiDR2RVZ4QQAI/ovHX3HtObElbd/g4RHd3o
xQ5yVWm91nqRWFZld9nLbbQgAHCyuEJRE0BpjSI/jnFPwrdlNHs9ivqbWt47qPFL
u+1txO3wPwDhdFYVCg8KBpqjjkCIBZaCMSWsJNUHQnD1Sf40lL1TPK811BL/gkly
0K+57ZrzT5OTQkIKhRGkfyOZ3CiwibgHqw1Rmb05c4pHU8OE4F+khbMHqGtAvfei
eeMEIWX8roC/TDxZz7rSZtNB9V1G9Ka10oXAHg05AM4r4/q7ypfR/jDRi+X9P9xY
8BMjDblyChh6gESZU0dXkrt1sYUCiUpwI5EQv4OHzxBqCLeR3Lqkt9IuOmOY2+kt
EVzgxHdEEZMdyrT4ubsCHNoS/+1CbxB362h3uz9KOjEACVQ3zW1Mr1mCkuAFPJ5d
XkpBw7ycWqxffo+N1+HUexAqd4TgX0iPChZ+x+CgRTSp/C2c/quG0SpQyE1Skg57
DB3c0U7NC0ycU7lZMeGKrc+6UbTMTtU6SOWytIEVRGiGbssOa9e/fwELDglbUvwe
15E7NjgLiQNuPEbiLdilZiHNvQjQYtrdcE55uDionAdnzKCn3OWwss/44iNTxe4W
8HEuB0Pw1Woku3pCd80HhB1sF/ETrN0m2DTclT+f2S26Sg8W/npzh4q93fh+kWWb
ZBHnholCkT0zpgyj9VGb
=Dskz
-----END PGP SIGNATURE-----

--/zZuIhN9RW1uCJRx--