Greetings Michael,
* Michael Paquier (michael@paquier.xyz) wrote:
> On Tue, Mar 06, 2018 at 10:00:54AM -0500, Stephen Frost wrote:
> > Attached is an updated patch which splits up the permissions as
> > suggested up-thread by Magnus:
> >
> > The default roles added are:
> >
> > * pg_read_server_files
> > * pg_write_server_files
> > * pg_execute_server_program
> >
> > Reviews certainly welcome.
>
> It seems to me that we have two different concepts here in one single
> patch:
> 1) Replace superuser checks by execution ACLs for FS-related functions.
> 2) Introduce new administration roles to control COPY and file_fdw
> First, could it be possible to do a split for 1) and 2)?
Done, because it was less work than arguing about it, but I'm not
convinced that we really need to split out patches to this level of
granularity. Perhaps something to consider for the future.
> + /*
> + * Members of the 'pg_read_server_files' role are allowed to access any
> + * files on the server as the PG user, so no need to do any further checks
> + * here.
> + */
> + if (is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_SERVER_FILES))
> + return filename;
> Second, I don't quite understand what is the link between COPY/file_fdw
> and the possibility to use absolute paths in all the functions of
> genfile.c. Is the use-case the possibility to check for the existence
> of a file using pg_stat_file before doing a copy? This seems rather
> limited because COPY or file_fdw would complain similarly for a missing
> file. So I don't quite get the use-case for authorizing that.
There's absolutely a use-case for being able to work with files outside
of the data directory using the misc file functions, which is what's
being addressed here, while also bringing into line the privileges given
to this new default role. To address the use-case of accessing files
generically through pg_read_file() or pg_read_binary_file() by having to
go through COPY instead would be unnecessairly complex.
Consider a case like postgresql.conf residing outside of the data
directory. For an application to be able to read that with
pg_read_file() is very straight-forward and applications already exist
which do. Forcing that application to go through COPY would require
creating a TEMP table and then coming up with a COPY command that
doesn't actually *do* what COPY is meant to do- that is, parse the file.
By default, you'd get errors from such a COPY command as it would think
there's extra columns defined in the "copy-format" or "csv" or
what-have-you file.
> Could you update the documentation of pg_rewind please? It seems to me
> that with your patch then superuser rights are not necessary mandatory,
This will require actual testing to be done before I'd make such a
change. I'll see if I can do that soon, but I also wouldn't complain if
someone else wanted to actually go through and set that up and test that
it works.
Thanks!
Stephen
