Re: Additional role attributes && superuser review

On Thu, Apr 2, 2015 at 12:53 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> Stephen Frost <sfrost@snowman.net> writes:
>> > REVOKE'ing access *without* removing the permissions checks would defeat
>> > the intent of these changes, which is to allow an administrator to grant
>> > the ability for a certain set of users to cancel and/or terminate
>> > backends started by other users, without also granting those users
>> > superuser rights.
>>
>> I see: we have two different use-cases and no way for GRANT/REVOKE
>> to manage both cases using permissions on a single object.  Carry
>> on then.
>
> Alright, after going about this three or four different ways, I've
> settled on the approach proposed in the attached patch.  Most of it is
> pretty straight-forward: drop the hard-coded check in the function
> itself and REVOKE EXECUTE on the function from PUBLIC.  The interesting
> bits are those pieces which are more complex than the "all-or-nothing"
> cases.
>
> This adds a few new SQL-level functions which return unfiltered results,
> if you're allowed to call them based on the GRANT system (and EXECUTE
> privileges for them are REVOKE'd from PUBLIC, of course).  These are:
> pg_stat_get_activity_all, pg_stat_get_wal_senders_all, and
> pg_signal_backend (which is similar but not the same- that allows you to
> send signals to other backends as a regular user, if you are allowed to
> call that function and the other backend wasn't started by a superuser).
>
> There are two new views added, which simply sit on top of the new
> functions: pg_stat_activity_all and pg_stat_replication_all.
>
> Minimizing the amount of code duplication wasn't too hard with the
> pg_stat_get_wal_senders case; as it was already returning a tuplestore
> and so I simply moved most of the logic into a "helper" function which
> handled populating the tuplestore and then each call path was relatively
> small and mostly boilerplate.  pg_stat_get_activity required a bit more
> in the way of changes, but they were essentially to modify it to return
> a tuplestore like pg_stat_get_wal_senders, and then add in the extra
> function and boilerplate for the non-filtered path.
>
> I had considered (and spent a good bit of time implementing...) a number
> of alternatives, mostly around trying to do more at the SQL level when
> it came to filtering, but that got very ugly very quickly.  There's no
> simple way to find out "what was the effective role of the caller of
> this function" from inside of a security definer function (which would
> be required for the filtering, as it would need to be able to call the
> function underneath).  This is necessary, of course, because functions
> in views run as the caller of the view, not as the view owner (that's
> useful in some cases, less useful in others).  I looked at exposing the
> information about the effective role which was calling a function, but
> that got pretty ugly too.  The GUC system has a stack, but we don't
> actually update the 'role' GUC when we are in security definer
> functions.  There's the notion of an "outer" role ID, but that doesn't
> account for intermediate security definer functions and so, while it's
> fine for what it does, it wasn't really helpful in this case.
>
> I do still think it'd be nice to provide that information and perhaps we
> can do so with fmgr_security_definer(), but it's beyond what's really
> needed here and I don't think it'd end up being particularly cleaner to
> do it all in SQL now that I've refactored pg_stat_get_activity.
>
> I'd further like to see the ability to declare on either a function call
> by function call basis (inside a view defintion), of even at the view
> level (as that would allow me to build up multiple views with different
> parameters) "who to run this function as", where the options would be
> "view owner" or "view querier", very similar to our SECURITY DEFINER vs.
> SECURITY INVOKER options for functions today.  This is interesting
> because, more and more, we're building functions which are actually
> returning data sets, not individual values, and we want to filter those
> sets sometimes and not other times.  In any case, those are really
> thoughts for the future and get away from what this is all about, which
> is reducing the need for monitoring and/or "regular" admins to have the
> "superuser" bit when they don't really need it.
>
> Clearly, further testing and documentation is required and I'll be
> getting to that over the next couple of days, but it's pretty darn late
> and I'm currently getting libpq undefined reference errors, probably
> because I need to set LD_LIBRARY_PATH, but I'm just too tired to now. :)
>
> Thoughts?

The tricky part of this seems to me to be the pg_dump changes.  The
new catalog flag seems a little sketchy to me; wouldn't it be better
to make the dump flag into an enum, DUMP_EVERYTHING, DUMP_ACL,
DUMP_NONE?

Is this creating a user-visible API break, where pg_stat_activity and
pg_stat_replication now always show only the publicly-visible
information, and privileged users must explicitly decide to query the
_all versions?  If so, -1 from me on that part of this.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company