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

On Fri, Jan 27, 2017 at 12:32 PM, Stephen Frost <sfrost@snowman.net> wrote:
> You're completely ignoring the use-cases for which these are being done.
>
> I've outlined the precise use-case for pgstattuple()'s usage across the
> entire database for which the admin has granted the EXECUTE access in.
> I've not yet seen a use-case for access to pg_ls_dir() across all
> directories pg_ls_dir() can access.  My recollection is that you brought
> up pg_wal, but that's hardly every directory, and some hypothetical tool
> which could intelligently figure out what orphaned files exist.  For the
> former, I would recommend a function for exactly that (or perhaps
> something better which provides more than just a count of files), for
> the latter, that's something that I would be very worried that someone
> trying to implement outside of core would get wrong or which could be
> version-dependent.  We've already got some code in core to find files
> left over from a crash and deal with them and perhaps that could be
> expanded to deal with other cases.

I agree that those things could be done other ways, but I don't think
that's a valid argument against what I'm proposing.  Sure, for any
given use case, you could come up with a way that the use case could
be satisfied without access to pg_ls_dir().  No question about it.
What I don't understand is why we should particularly want to go to
that trouble.  pg_ls_dir() has been serving authors of monitoring
tools well for many years, it presents minimal security risks, and it
could be useful for other purposes.  A new thing won't be immediately
adopted, has no real advantage on the security front because
pg_ls_dir() isn't actually dangerous, and is by design single-purpose.

I am completely flummoxed by the idea that giving out pg_ls_dir()
access to all directories it can access -- namely the data and log
directories -- is some kind of massive security problem, whereas
giving out superuser access for the same purpose apparently is no
problem at all.  And that is CLEARLY what is happening.  The
documentation for check_postgres.pl recommends it!

> With an appropriate use-case for it, I wouldn't be against it.  I linked
> to both use-cases and a provided patch for finer-grained access to
> pg_ls_dir() and friends three years ago, which got shot down.  I'm not
> against it if the community wishes to reconsider that decision and
> support having filesystem-like access where there's an appropriate
> use-case for it, and with fine-grained access control provided to meet
> that use-case.

I hope you're not saying that you plan to hold this patch hostage
until I agree to something you want to do, because that would be
uncool.  At any rate, I don't even think that patch really got shot
down; the major complaint about that patch was that it arrived out of
nowhere, fully formed, with no prior discussion of the design, and
several people - including me - were concerned that you might commit
it before consensus had been reached, as you had recently done with
another large patch that I was in the middle of reviewing.  Aside from
the "please don't commit this because it isn't agreed" objection,
there were comments like (1) DIRALIAS is not a word, but all other SQL
objects have names which are words or phrases and (2) hey, maybe we
could use a couple of GUCs for this instead of inventing a whole new
SQL object type and (3) what about adding some syntax for COPY that
uses these directory alias things, none of which qualify as trying to
stop the project dead in its tracks.  The only person who was dead set
against the whole concept was Tom, and several people including me
expressed at least some doubt about whether the problems with the idea
were so intractable as he suggested.

For the record, I'm not as keen on the GUC idea as I was when we had
this previous discussion.  This is really a privilege and probably
deserves to be handled as part of the privileges system rather than
crammed into a GUC mechanism which isn't intended for that purpose.  I
am also somewhat more in favor of the idea overall than I was back
then; as we work toward giving users as little privilege as possible,
it's certainly useful to be able to let a monitoring user read the
logs but not the data directory, or the toplevel files (i.e. config
files) in the data directory but not files in subdirectories.   But I
really can't see a lick of need for that kind of fine-grained control
for pg_ls_dir() or pg_stat_files(), which are six kinds of harmless,
and I'm not sure that pg_read_file() permissions really need to be as
granular as allowing each directory on the filesystem to have
individual permissions.  There are a couple of broad categories that
might need to be distinguished: log file, config file, data file, file
I want to COPY, but there might be ways to handle those cases that are
simpler than a full-blown new SQL object type.

> Further, as it relates to goal-posts, if your goal is to let an admin
> grant out the ability to see how many files are in pg_wal, you're
> spending a great deal more effort than that would require.  I wouldn't
> object (and would actually appreciate) a function which is able to do
> exactly that, and I'd go work with Greg S-M right away to make sure that
> check_postgres.pl knows about that function and uses it in PG10 and
> above.

Well, that is true, or would be if that were really what my goal was,
but I didn't know that when I wrote the original email.  Now that
you've done all of the work to allow privileges on default objects to
survive pg_dump, I really see no benefit to having hard-coded
superuser() checks.  I assumed that the fact that we'd left a few in
was because they'd been overlooked or that nobody had gotten around to
removing them yet, and I'm pretty gobsmacked by the way this
discussion has gone.  I was kind of expecting a response along the
lines of "oh, yeah, thanks for taking care of that".

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company