Re: pg_ls_dir & friends still have a hard-coded superuser check

Robert,

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Wed, Jan 25, 2017 at 3:17 PM, Stephen Frost <sfrost(at)snowman(dot)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