Dave, * Dave Page (dp...@pgadmin.org) wrote: > On Wed, Feb 22, 2017 at 4:52 PM, Stephen Frost <sfr...@snowman.net> wrote: > >> > > And what about the diagnostic tools such as pageinspect and > >> > > pgstattuple? > >> > > >> > I think external/contrib modules should not be included. To install > >> > them you need admin privileges anyway, so you can easily grant > >> > whatever usage privileges you want at that time. > >> > >> I'll start by saying "why not cover contrib"? > > > > +1. > > I'm not convinced we should include it, for the reason I gave above. > However, I don't feel that strongly. > > What modules should be included?
On a quick review of all of the modules, excluding those that are just testing or examples or which can already be used by non-superusers by default, and excluding those which can be used to trivially gain superuser access (adminpack and pageinspect), I came up with: pg_buffercache pg_freespacemap pgrowlocks pg_stat_statements pgstattuple pg_visibility Reviewing this list, they all seem like things a monitoring user could have a use for and none of them allow direct access to table data from what I could tell on a quick review. Obviously, a more detailed review of each should be done to make sure I didn't miss something. One interesting thing that comes up from this list is that there's a number of things which are "look at something about a row" or "look at something about a block" (pg_freespacemap, pgrowlocks, pgstattuple, pg_visibility all fall into those, and to some extent pg_buffercache too). I'm tempted to suggest that we have a role which covers that theme (and is then GRANT'd to pg_monitor). > >> pgstattuple can be discussed. It doesn't leak anything dangerous. But it > >> does have views that are quite expensive. > > I don't think expense should be a concern. It's not like a regular > user cannot run something expensive already, so why stop a user > specifically setup to monitor something? I tend to agree with this. > > I do see two issues to be addressed with such a role: > > > > #1- We shouldn't just shove everything into one role. Where > > functionality can't be GRANT'd independently of the role, we should have > > another default role. For example, "Read all GUCs" is not something > > that can currently be GRANT'd. I'm sure there are cases where $admin > > wants a given role to be able to read all GUCs, but not execute > > pg_ls_logdir(), for example. If we start writing code that refers > > explicitly to pg_monitor then we will end up in an "all-or-nothing" kind > > of situation (not unlike the superuser case) instead of allowing users a > > fine-grained set of options. > > I'm fine with having pg_read_all_gucs - it's a trivial change. I > wouldn't want us to go too far and end up with separate roles for > everything under the sun though. I agree with you there- having too many default roles would lead to things getting messy, without there really being a need for it. Users can always create their own roles for the specific set of capabilities that they want to provide. The main thing I want to avoid is having a situation where a user *can't* create a role that has only a subset of what "pg_monitor" has because there's some code somewhere that explicitly allows the "pg_monitor" role to do something. > > #2- We need to define very carefully, up-front, how we will deal with > > new privileges/capabilities/features down the road. A very specific > > default role like 'pg_read_all_gucs' is quite clear about what's allowed > > by it and I don't think we'd get any push-back from adding new GUCs that > > such a default role could read, but some new view pg_stat_X view that > > would be really useful to monitoring tools might also allow more access > > than the pg_monitor has or that some admins would be comfortable with- > > how do we handle such a case? I see a few options: > > > > - Define up-front that pg_monitor has rights on all pg_stat_X views, > > which then requires we provide a definition and clarity on what > > "pg_stat_X" *is* and provides. We can then later add such views and > > GRANT access to them to pg_monitor. > > > > - Create new versions of pg_monitor in the future that cover ever > > increasing sets of privileges ("pg_monitor_with_pg_stat_X" or > > "pg_monitor_v11" for PG11 or something). > > I prefer the first option. In my experience, users don't much care > about the rights their monitoring user has, as long as it's not full > superuser. The only case where I think there are legitimate concerns > are where you can read arbitrary data (I do not consider query strings > to be in that class for the record). That said, if we ever do add > something like that then there's nothing stopping us from explicitly > documenting that it's excluded from pg_monitor for that reason, and if > desired the user can grant on it as needed. > > Using a scheme like that would also mean that the user is more likely > to need to manually update the role their monitoring system uses > following an upgrade. I prefer the first option too, but that means you have work to do to define what "pg_stat_X" means/covers in a way that, hopefully, future hackers will remember and adhere to. ;) I'd rather we try to avoid having exceptions as it can lead to confusion and adds complexity that security folks would almost certainly prefer to not have to deal with. > > It seems at least unlikely that we'll never have another pg_stat_X view > > that we want to give pg_monitor access to, so I don't really see "Fix > > what pg_monitor can read forever based on what's in PG10" as being a > > solution. > > No, but similarly I don't see any reason why we cannot add new views > by default, and exclude by exception when there's a compelling reason. I'd rather simply name whatever that exception is 'pg_somethingelse'. There would have to be a really, really good reason to have a 'pg_stat_X' that isn't something that pg_monitor can have access to. Now, perhaps what that means is that we should really name these things something else or have aliases to them that is what pg_monitor is given, so we get away from the issue that "statistics" stuff might have something sensitive included that we don't want pg_monitor to have access to (eg: pg_mon_statements, pg_mon_activity, pg_mon_whatever ...). I guess that ends up depending on what we can come up with to define what "pg_stat_X" is and if everyone can more-or-less agree to that (and, subsequently, agree that pg_monitor should have access to all of that). Thanks! Stephen
Description: Digital signature