Robert, * Robert Haas (robertmh...@gmail.com) wrote: > On Wed, Jan 25, 2017 at 3:17 PM, Stephen Frost <sfr...@snowman.net> wrote: > > Your email is 'pg_ls_dir & friends', which I took to imply at *least* > > pg_read_file() and pg_read_binary_file(), and it's not unreasonable to > > think you may have meant everything in adminpack, which also includes > > pg_file_write(), pg_file_rename() and pg_file_unlink(). > > Well, I was talking about genfile.c, which doesn't contain those > functions, but for the record I'm in favor of extirpating every single > hard-coded superuser check from the backend without exception.
Then it was correct for me to assume that's what you meant, and means my reaction and response are on-point. > Preventing people from calling functions by denying the ability to > meaningfully GRANT EXECUTE on those functions doesn't actually stop > them from delegating those functions to non-superusers. It either (a) > causes them to give superuser privileges to accounts that otherwise > would have had lesser privileges or (b) forces them to use wrapper > functions to grant access rather than doing it directly or (c) some > other dirty hack. If they pick (a), security is worse; if they pick > (b) or (c), you haven't prevented them from doing what they wanted to > do anyway. You've just made it annoying and inconvenient. The notion that security is 'worse' under (a) is flawed- it's no different. With regard to 'b', if their wrapper function is sufficiently careful to ensure that the caller isn't able to do anything which would increase the caller's level to that of superuser, then security is improved. If the wrapper simply turns around can calls the underlying function, then it's no different from '(a)'. I am suggesting that we shouldn't make it look like there are distinctions when there is actually no difference. That is a good thing for our users because it keeps them informed about what they're actually doing when they grant access. > Both EnterpriseDB's PEM and check_postgres.pl currently have a bunch > of things that don't work unless you run as superuser. I think we > should be trying to provide ways of reducing those lists. If I can't > get agreement on method (a), I'm going to go look for ways of doing > (b) or (c), which is more work and uglier but if I can't get consensus > here then oh well. I've commented on here and spoken numerous times about exactly that goal of reducing the checks in check_postgres.pl which require superuser. You're not actually doing that and nothing you've outlined in here so far makes me believe you see how having pg_write_file() access is equivilant to giving someone superuser, and that concerns me. As someone who has contributed code and committed code back to check_postgres.pl, I would be against making changes there which install security definer functions to give the monitoring user superuser-level access, and I believe Greg S-M would feel the same way considering that he and I have discussed exactly that in the past. I don't mean to speak for him and perhaps his opinion has changed, but it seems unlikely to me. If the DBA wants to give the monitoring user superuser-level access to run the superuser-requiring checks that check_postgres.pl has, they're welcome to do so, but they'll be making an informed decision that they have weighed the risk of their monitoring user being compromised against the value of that additional monitoring, which is an entirely appropriate and reasonable decision for them to make. > I do not accept your authority to determine what monitoring tools need > to or should do. Monitoring tools that use pg_ls_dir are facts on the > ground, and your disapprobation doesn't change that at all. It just > obstructs moving to a saner permissions framework. Allowing GRANT to be used to give access to pg_write_file and friends is not a 'permissions framework'. Further, I am not claiming authority over what monitoring tools should need to do or not do, and a great many people run their monitoring tools as superuser. I am not trying to take away their ability to do so. The way to make progress here is not, however, to just decide that all those superuser() checks we put in place were silly and should be removed, it's to provide better ways to monitor PG which provide exactly the monitoring information needed in a useful and sensible way. I understand the allure of just removing a few lines of code to make things "easier" or "faster" or "better", but I don't think it's a good idea to remove these superuser checks, nor do I think it's a good idea to remove our WAL CRCs even if it'd help some of my clients, nor do I think it's a good idea to have checksums disabled by default, or to rip out all of WAL as being "unnecessary" because we have ZFS and it should handle all things data and we could just fsync all of the heap files on commit and be done with it. Admittedly, you're not argueing for half of what I just mentioned, but I'm not arguing that DBAs shouldn't be able to have their monitoring users be superusers either, I'm pointing out that removing those checks makes any user who is GRANT access to the functions the equivilant of a superuser, potentially without the DBA realizing it, and that is an all-together bad thing. If a DBA sees the reasoning behind not making a monitoring user a superuser, we shouldn't be trying to pull the wool over their eyes or sprinkling a little GRANT dust over in the corner that turns the monitoring user into an account with superuser-level access, we should accept that their decision is to not have the monitoring user have superuser rights and ensure that the database permission system doesn't give it to them. > > I don't think it's nannyism to keep things like pg_read_file and > > pg_file_write as superuser-only. > > I entirely disagree. It is for the DBA to decide to whom and for what > purpose he wishes to give permissions. And if somebody writes a tool > -- monitoring or otherwise -- that needs those permissions, the DBA > should be empowered to give those permissions and no more, and should > be empowered to do that in a nice, clean way without a lot of hassle. The DBA is well within their rights and abilities to do so with a simple comamnd: ALTER ROLE. I am not suggesting that we take their right or ability to do so away from them. > Why you'd rather have people > running check_postgres.pl's wal_files check under an actual superuser > account rather than an account that only has rights to do pg_ls_dir is > beyond me. I don't want people to run the wal_files check as a superuser, as I've said before, even on this thread, there should be a better answer. I do not view removing the superuser() check, while appealing and appearing to be simple, to be that 'better answer'. > Your response will no doubt be that there should otherwise be some > other way to write that check that doesn't use pg_ls_dir. Maybe. But > the thing is that pg_ls_dir is a general tool. You can use it to do a > lot of things, and you can write a monitoring probe that does those > things without having to wait for a new release of PostgreSQL. With pg_write_file(), pg_read_file(), and friends, there's a whole lot of stuff we could punt on and say "user, you figure out how to make these work for you, we don't feel like doing anything more." > So > your argument boils down to saying that when somebody wants to monitor > something that could be checked using pg_ls_dir(), they shouldn't do > write a query using pg_ls_dir(). No, I'm saying that we should provide something better than pg_ls_dir() for them to use. If they want to use pg_ls_dir(), then they can go for it, but I really do *not* think the answer to "what's the best way to see how much WAL space PG is using?" is "oh, just use pg_ls_dir()." Consdier if we had such a neat feature today to return the number of WAL files- assuming we named it correctly, we wouldn't have people complaining about the fact that we're about to change pg_xlog to pg_wal. Generally speaking, I'd really like external tools, monitoring systems, etc, to know less, not more, about our on-disk data layout and structures, where that's possible. > Instead, they should submit a new > patch to core that adds a special-purpose function implementing the > exact functionality they need, get consensus on it, get somebody to > commit it, wait for a new PostgreSQL release to come out, wait for the > users who might want the probe to upgrade to that new release, and > THEN they can do the monitoring they want to do. I sure do think that we should add new monitoring capabilities to PG and that we absolutely should *not* start telling people who want to implement new monitoring features that "oh, well, users can just use pg_read_binary_file() directly to get that info from pg_controldata, why would we need to write a function for that?" From my perspective, at least, that's about the same level as what you're asking to do with pg_ls_dir(). If your goal is to give users the ability to query how many WAL files are in pg_wal, I don't doubt that you could get it implemented and into PG10 with a lot more ease than arguing about pg_ls_dir() with me, or anyone else who wants to chime in on this, and users would have it at just the same time as they would a hack to pg_ls_dir(), and they wouldn't be giving their monitoring user the ability to look at what files the DBA has his home directory on the database server. > Now, I say that's > ridiculous. What's actually going to happen is that the tool author > is going to say "this probe requires superuser" and move on. Sure, and that's a fair response. > The fact > that check_postgres.pl has done exactly that for about 5 different > probes -- not all because of pg_ls_dir, but one of them is for that > reason -- suggests that this will in fact be a typical reaction > whenever a fine-grained privilege is not available to be granted. We > can decide to alleviate that pain or we can decide against it, but > telling people "don't do that" is not going to work. I am all in favor of alleviating that pain, by providing a proper solution which is actually well thought out and designed to answer the question. Removing the check from pg_ls_dir() isn't any of that. Thanks! Stephen
signature.asc
Description: Digital signature
