Re: Additional role attributes && superuser review

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)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?

Thanks!

Stephen