On Thu, Apr 2, 2015 at 12:53 AM, Stephen Frost <sfr...@snowman.net> wrote: > * Tom Lane (t...@sss.pgh.pa.us) wrote: >> Stephen Frost <sfr...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
