Add default role 'pg_access_server_files'

On Sun, Dec 31, 2017 at 8:19 PM, Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

This patch adds a new default role called 'pg_access_server_files' which
allows an administrator to GRANT to a non-superuser role the ability to
access server-side files through PostgreSQL (as the user the database is
running as). By itself, having this role allows a non-superuser to use
server-side COPY and to use file_fdw (if installed by a superuser and
GRANT'd USAGE on it).

Further, this patch moves the privilege check for the remaining misc
file functions from explicit superuser checks to the GRANT system,
similar to what's done for pg_ls_logdir() and others. Lastly, these
functions are changed to allow a user with the 'pg_access_server_files'
role to be able to access files outside of the PG data directory.

This follows on and continues what was recently done with the
lo_import/export functions. There's other superuser checks to replace
with grant'able default roles, but those probably make more sense as
independent patches. I continue to be of the opinion that it'd be nice
to have more fine-grained control over these functions to limit the
access granted, but nothing here prevents that from being done and this
at least allows some movement away from having to have roles with
superuser access.

Would it make sense to separate out:
* write from read. E.g. a pg_write_server_files/pg_read_server_files? ISTM
that will turn into a pretty common request...
* execute from read/write, so COPY FROM PROGRAM etc would be a separate
role?

I realize we don't want to go overboard with the number of roles here, but
at least separating read from write seems useful.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

Magnus,

* Magnus Hagander (magnus@hagander.net) wrote:

On Sun, Dec 31, 2017 at 8:19 PM, Stephen Frost <sfrost@snowman.net> wrote:

This patch adds a new default role called 'pg_access_server_files' which
allows an administrator to GRANT to a non-superuser role the ability to
access server-side files through PostgreSQL (as the user the database is
running as). By itself, having this role allows a non-superuser to use
server-side COPY and to use file_fdw (if installed by a superuser and
GRANT'd USAGE on it).

Further, this patch moves the privilege check for the remaining misc
file functions from explicit superuser checks to the GRANT system,
similar to what's done for pg_ls_logdir() and others. Lastly, these
functions are changed to allow a user with the 'pg_access_server_files'
role to be able to access files outside of the PG data directory.

This follows on and continues what was recently done with the
lo_import/export functions. There's other superuser checks to replace
with grant'able default roles, but those probably make more sense as
independent patches. I continue to be of the opinion that it'd be nice
to have more fine-grained control over these functions to limit the
access granted, but nothing here prevents that from being done and this
at least allows some movement away from having to have roles with
superuser access.

Would it make sense to separate out:
* write from read. E.g. a pg_write_server_files/pg_read_server_files? ISTM
that will turn into a pretty common request...

Ok.

* execute from read/write, so COPY FROM PROGRAM etc would be a separate
role?

Suggestions on a name for this..? pg_server_copy_program?

I realize we don't want to go overboard with the number of roles here, but
at least separating read from write seems useful.

Yeah, these are certainly good suggestions for the COPY case. I had set
out thihking about pg_read/write_file and we have the read/write
seperation there through the GRANT rights on the functions themselves,
but we don't have that for COPY without different roles.

I'll add those in and publish a new patch soon.

Thanks!

Stephen

On Tue, Jan 2, 2018 at 1:08 PM, Stephen Frost <sfrost@snowman.net> wrote:

Magnus,

* Magnus Hagander (magnus@hagander.net) wrote:

On Sun, Dec 31, 2017 at 8:19 PM, Stephen Frost <sfrost@snowman.net>

wrote:

This patch adds a new default role called 'pg_access_server_files'

which

allows an administrator to GRANT to a non-superuser role the ability to
access server-side files through PostgreSQL (as the user the database

is

running as). By itself, having this role allows a non-superuser to use
server-side COPY and to use file_fdw (if installed by a superuser and
GRANT'd USAGE on it).

Further, this patch moves the privilege check for the remaining misc
file functions from explicit superuser checks to the GRANT system,
similar to what's done for pg_ls_logdir() and others. Lastly, these
functions are changed to allow a user with the 'pg_access_server_files'
role to be able to access files outside of the PG data directory.

This follows on and continues what was recently done with the
lo_import/export functions. There's other superuser checks to replace
with grant'able default roles, but those probably make more sense as
independent patches. I continue to be of the opinion that it'd be nice
to have more fine-grained control over these functions to limit the
access granted, but nothing here prevents that from being done and this
at least allows some movement away from having to have roles with
superuser access.

Would it make sense to separate out:
* write from read. E.g. a pg_write_server_files/pg_read_server_files?

ISTM

that will turn into a pretty common request...

Ok.

* execute from read/write, so COPY FROM PROGRAM etc would be a separate
role?

Suggestions on a name for this..? pg_server_copy_program?

Presumably it would also be used in postgres_fdw, so that seems like a bad
name. Maybe pg_exec_server_command?

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

Magnus,

* Magnus Hagander (magnus@hagander.net) wrote:

On Tue, Jan 2, 2018 at 1:08 PM, Stephen Frost <sfrost@snowman.net> wrote:

* Magnus Hagander (magnus@hagander.net) wrote:

On Sun, Dec 31, 2017 at 8:19 PM, Stephen Frost <sfrost@snowman.net>

wrote:

This patch adds a new default role called 'pg_access_server_files'

which

allows an administrator to GRANT to a non-superuser role the ability to
access server-side files through PostgreSQL (as the user the database

is

running as). By itself, having this role allows a non-superuser to use
server-side COPY and to use file_fdw (if installed by a superuser and
GRANT'd USAGE on it).

Further, this patch moves the privilege check for the remaining misc
file functions from explicit superuser checks to the GRANT system,
similar to what's done for pg_ls_logdir() and others. Lastly, these
functions are changed to allow a user with the 'pg_access_server_files'
role to be able to access files outside of the PG data directory.

This follows on and continues what was recently done with the
lo_import/export functions. There's other superuser checks to replace
with grant'able default roles, but those probably make more sense as
independent patches. I continue to be of the opinion that it'd be nice
to have more fine-grained control over these functions to limit the
access granted, but nothing here prevents that from being done and this
at least allows some movement away from having to have roles with
superuser access.

Would it make sense to separate out:
* write from read. E.g. a pg_write_server_files/pg_read_server_files?

ISTM

that will turn into a pretty common request...

Ok.

* execute from read/write, so COPY FROM PROGRAM etc would be a separate
role?

Suggestions on a name for this..? pg_server_copy_program?

Presumably it would also be used in postgres_fdw, so that seems like a bad
name. Maybe pg_exec_server_command?

I'm guessing you mean file_fdw.. :)

That name sounds alright to me though.

Thanks!

Stephen

Stephen, so far I've read thru your patch and familiarized myself with some of the auth functionality in pg_authid.h and src/backend/utils/adt/acl.c

The only question I have so far about your patch is the last several hunks of the diff, which remove superuser checks without adding anything immediately obvious in their place:

...
@@ -195,11 +205,6 @@ pg_read_file(PG_FUNCTION_ARGS)
char *filename;
text *result;

-   if (!superuser())
-       ereport(ERROR,
-               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                (errmsg("must be superuser to read files"))));
-
    /* handle optional arguments */
    if (PG_NARGS() >= 3)
    {
@@ -236,11 +241,6 @@ pg_read_binary_file(PG_FUNCTION_ARGS)
    char       *filename;
    bytea      *result;

-   if (!superuser())
-       ereport(ERROR,
-               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                (errmsg("must be superuser to read files"))));
-
    /* handle optional arguments */
    if (PG_NARGS() >= 3)
    {
@@ -313,11 +313,6 @@ pg_stat_file(PG_FUNCTION_ARGS)
    TupleDesc   tupdesc;
    bool        missing_ok = false;

- if (!superuser())
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- (errmsg("must be superuser to get file information"))));
-
/* check the optional argument */
if (PG_NARGS() == 2)
missing_ok = PG_GETARG_BOOL(1);
...

I wanted to ask if you have reason to believe that these checks were not necessary (and therefore can be deleted instead of replaced by is_member_of_role() checks like you did elsewhere). I still have limited understanding of the overall code, so really just asking because it's the first thing that jumped out.

Best,
Ryan

Greetings Ryan!

* Ryan Murphy (ryanfmurphy@gmail.com) wrote:

Stephen, so far I've read thru your patch and familiarized myself with some of the auth functionality in pg_authid.h and src/backend/utils/adt/acl.c

The only question I have so far about your patch is the last several hunks of the diff, which remove superuser checks without adding anything immediately obvious in their place:

Ah, I realize it's not immediately obvious, but they *are* replaced by
something else- REVOKE statements in the "system_views.sql" file in
src/backend/catalog:

REVOKE EXECUTE ON FUNCTION pg_read_file(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_read_file(text,bigint,bigint) FROM public;
REVOKE EXECUTE ON FUNCTION pg_read_file(text,bigint,bigint,boolean) FROM public;

REVOKE EXECUTE ON FUNCTION pg_read_binary_file(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_read_binary_file(text,bigint,bigint) FROM public;
REVOKE EXECUTE ON FUNCTION pg_read_binary_file(text,bigint,bigint,boolean) FROM public;

REVOKE EXECUTE ON FUNCTION pg_stat_file(text) FROM public;
REVOKE EXECUTE ON FUNCTION pg_stat_file(text,boolean) FROM public;

That script is run as part of 'initdb' to set things up in the system.
By issueing those REVOKE statements, no one but the cluster owner (a
superuser) is able to run those functions- unless a superuser decides
that it's ok for others to run them, in which case they would run:

GRANT EXECUTE ON FUNCTION pg_read_file(text) TO myuser;

I wanted to ask if you have reason to believe that these checks were not necessary (and therefore can be deleted instead of replaced by is_member_of_role() checks like you did elsewhere). I still have limited understanding of the overall code, so really just asking because it's the first thing that jumped out.

The places where is_member_of_role() is being used are cases where it's
not possible to use the GRANT system. For example, we don't have a way
to say "GRANT read-files-outside-the-data-directory TO role1;" in the
normal GRANT system, and so a default role is added to allow that
specific right to be GRANT'd to non-superuser.

One would need to have both the default role AND EXECUTE rights on the
function to be able to, say, run:

SELECT pg_read_file('/data/load_area/load_file');

With just EXECUTE on the function, they could use pg_read_file() to read
files under the data directory but not elsewhere on the system, which
may be overly limiting for some use-cases.

Of course, all of these functions allow a great deal of access to the
system and that's why they started out being superuser-only.
Administrators will need to carefully consider who, if anyone, should
have the level of access which these functions and default roles
provide.

Thanks!

Stephen

Hi Stephen,

I have another question then based on what you said earlier today, and some testing I did using your patch.

TLDR: I created a role "tester" and was (as expected) not able to perform pg_read_file() on files outside the data directory.
But then I granted EXECUTE on that function for that role, and then I was able to (which is not what I expected).

Here's what I did (I apologize if this is too verbose):

* I successfully applied your patch to HEAD, and built Postgres from source:

make clean
configure (with options including a specific --prefix)
make
make install

* Then I went into the "install_dir/bin" and did the following to setup a data directory:

$ ./initdb ~/sql-data/2018-01-06
The files belonging to this database system will be owned by user "postgres".
This user must also own the server process.

The database cluster will be initialized with locale "en_US.UTF-8".
The default database encoding has accordingly been set to "UTF8".
The default text search configuration will be set to "english".

Data page checksums are disabled.

creating directory /Users/postgres/sql-data/2018-01-06 ... ok
creating subdirectories ... ok
selecting default max_connections ... 100
selecting default shared_buffers ... 128MB
selecting dynamic shared memory implementation ... posix
creating configuration files ... ok
running bootstrap script ... ok
performing post-bootstrap initialization ... ok
syncing data to disk ... ok

WARNING: enabling "trust" authentication for local connections
You can change this by editing pg_hba.conf or using the option -A, or
--auth-local and --auth-host, the next time you run initdb.

Success. You can now start the database server using:

./pg_ctl -D /Users/postgres/sql-data/2018-01-06 -l logfile start

* Then I started the database:

$ ./pg_ctl -D /Users/postgres/sql-data/2018-01-06 -l logfile start
waiting for server to start.... done
server started

* I went into the database and tried a pg_read_file:

$ psql postgres
psql (9.4.5, server 11devel)
Type "help" for help.

postgres=# select pg_read_file('/Users/postgres/temp');
pg_read_file
-----------------------------------------------------------
here is the file content

(1 row)

* Of course that worked as superuser, so created a new role:

postgres=# create role tester;
CREATE ROLE
postgres=# \q
postgres=# alter role tester with login;
ALTER ROLE
postgres=# \q

$ psql postgres tester
psql (9.4.5, server 11devel)
Type "help" for help.

postgres=> select pg_read_file('/Users/postgres/temp');
ERROR: permission denied for function pg_read_file
postgres=> \q

* My current understanding at this point is that EXECUTE permissions would only allow "tester" to pg_read_file() on files in the data directory. I try GRANTing EXECUTE:

$ psql postgres
psql (9.4.5, server 11devel)
Type "help" for help.

postgres=# grant execute on function pg_read_file(text) to tester;
GRANT
postgres=# select pg_read_file('/Users/postgres/temp');
pg_read_file
-----------------------------------------------------------
here is the file content

(1 row)

Is this expected behavior? I thought I would need to GRANT that new "pg_access_server_files" role to "tester" in order to do this. I may have misunderstood how your new feature works, just doublechecking.

Regards,
Ryan

Oops! I made a mistake, which clearly showed up in my last email: I forgot
to psql back in as "tester".

Now I get the right behavior:

$ psql postgres tester
psql (9.4.5, server 11devel)
Type "help" for help.

postgres=> select pg_read_file('/Users/postgres/temp');
ERROR: absolute path not allowed

Thanks for bearing with me. So far so good on this feature, going to run
the tests too.

Best,
Ryan

(Duplicated to make sure it's in the commitfest Thread, didn't seem to get in there when I replied to the email)

Oops! I made a mistake, which clearly showed up in my last email: I forgot to psql back in as "tester".

Now I get the right behavior:

$ psql postgres tester
postgres=> select pg_read_file('/Users/postgres/temp');
ERROR: absolute path not allowed

Thanks for bearing with me. So far so good on this feature, going to run the tests too.

Best,
Ryan

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: tested, passed

I ran make installcheck-world and all tests passed except one that is a known issue with the way I have my environment setup (ecpg tests unrelated to this patch).

Manual tests I ran to see if it Implements the Feature:

1) confirmed that superuser can call pg_read_file() to read files in or out of data directory
2) confirmed that "tester" can call pg_read_file() only if given EXECUTE privilege
3) confirmed that "tester" can only call pg_read_file() on a file OUTSIDE of the data directory iff I "grant pg_access_server_files to tester;"

Documentation seems reasonable.

I believe this patch to be Ready for Committer.

The new status of this patch is: Ready for Committer

On Mon, Jan 1, 2018 at 8:19 AM, Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

This patch adds a new default role called 'pg_access_server_files' which
allows an administrator to GRANT to a non-superuser role the ability to
access server-side files through PostgreSQL (as the user the database is
running as). By itself, having this role allows a non-superuser to use
server-side COPY and to use file_fdw (if installed by a superuser and
GRANT'd USAGE on it).

Hi Stephen,

Not sure if you are aware of this failure?

test file_fdw ... FAILED

Because:

! ERROR: only superuser can change options of a file_fdw foreign table
...
! ERROR: only superuser or a member of the pg_access_server_files role
can change options of a file_fdw foreign table

--
Thomas Munro
http://www.enterprisedb.com

Thomas,

* Thomas Munro (thomas.munro@enterprisedb.com) wrote:

On Mon, Jan 1, 2018 at 8:19 AM, Stephen Frost <sfrost@snowman.net> wrote:

This patch adds a new default role called 'pg_access_server_files' which
allows an administrator to GRANT to a non-superuser role the ability to
access server-side files through PostgreSQL (as the user the database is
running as). By itself, having this role allows a non-superuser to use
server-side COPY and to use file_fdw (if installed by a superuser and
GRANT'd USAGE on it).

Not sure if you are aware of this failure?

test file_fdw ... FAILED

Thanks, I did do a make check-world, but I tend to do them in parallel
and evidently missed this. The patch needs to be reworked based on
discussion with Magnus anyhow, which I hope to do this weekend.
Currently trying to push other patches along. :)

Thanks!

Stephen

Just circling back on this.

I did have a question that came up about the behavior of the server as it is without the patch. I logged into psql with my superuser "postgres":

postgres=# select pg_read_file('/Users/postgres/temp');
ERROR: absolute path not allowed

I had not tried this before with my unpatched build of postgres. (In retrospect of course I should have). I expected my superuser to be able to perform this task, but it seems that for security reasons we presently don't allow access to absolute path names (except in the data dir and log dir) - not even for a superuser. Is that correct? In that case the security implications of this patch would need more consideration.

Stephen, looking at the patch, I see that in src/backend/utils/adt/genfile.c you've made some changes to the function convert_and_check_filename(). These changes, I believe, loosen the security checks in ways other than just adding the granularity of a new role which can be GRANTed to non superusers:

    +   /*
    +    * Members of the 'pg_access_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_ACCESS_SERVER_FILES))
    +       return filename;
    +
    +   /* User isn't a member of the default role, so check if it's allowable */
        if (is_absolute_path(filename))
        {

As you can see, you've added a short-circuit "return" statement for anyone who has the DEFAULT_ROLE_ACCESS_SERVER_FILES. Prior to this patch, even a Superuser would be subject to the following is_absolute_path() logic. But with it, the return statement short-circuits the is_absolute_path() check.

Is this an intended behavior of the patch - to allow file access to absolute paths where previously it was impossible? Or, was the intention just to add granularity via the new role? I had assumed the latter.

Best regards,
Ryan

The new status of this patch is: Waiting on Author

On Thu, Jan 18, 2018 at 02:04:45PM +0000, Ryan Murphy wrote:

I had not tried this before with my unpatched build of postgres. (In
retrospect of course I should have). I expected my superuser to be
able to perform this task, but it seems that for security reasons we
presently don't allow access to absolute path names (except in the
data dir and log dir) - not even for a superuser. Is that correct?

Correct.

In that case the security implications of this patch would need more
consideration.

Stephen, looking at the patch, I see that in
src/backend/utils/adt/genfile.c you've made some changes to the
function convert_and_check_filename(). These changes, I believe,
loosen the security checks in ways other than just adding the
granularity of a new role which can be GRANTed to non superusers:

+   /*
+    * Members of the 'pg_access_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_ACCESS_SERVER_FILES))
+       return filename;
+
+   /* User isn't a member of the default role, so check if it's allowable */
if (is_absolute_path(filename))
{

It seems to me that this is the intention behind the patch as the
comment points out and as Stephen has stated in
/messages/by-id/20171231191939.GR2416@tamriel.snowman.net.

As you can see, you've added a short-circuit "return" statement for
anyone who has the DEFAULT_ROLE_ACCESS_SERVER_FILES. Prior to this
patch, even a Superuser would be subject to the following
is_absolute_path() logic. But with it, the return statement
short-circuits the is_absolute_path() check.

I agree that it is a strange concept to loosen the access while even
superusers cannot do that. By concept superusers are assumed to be able
to do anything on the server by the way.
--
Michael

Michael, all,

* Michael Paquier (michael.paquier@gmail.com) wrote:

On Thu, Jan 18, 2018 at 02:04:45PM +0000, Ryan Murphy wrote:

I had not tried this before with my unpatched build of postgres. (In
retrospect of course I should have). I expected my superuser to be
able to perform this task, but it seems that for security reasons we
presently don't allow access to absolute path names (except in the
data dir and log dir) - not even for a superuser. Is that correct?

Correct.

That's how it currently is, yes, though that doesn't really prevent a
superuser from accessing files outside of the data dir, they would just
have to use another mechanism to do so than this (but it's not hard).

In that case the security implications of this patch would need more
consideration.

Stephen, looking at the patch, I see that in
src/backend/utils/adt/genfile.c you've made some changes to the
function convert_and_check_filename(). These changes, I believe,
loosen the security checks in ways other than just adding the
granularity of a new role which can be GRANTed to non superusers:

+   /*
+    * Members of the 'pg_access_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_ACCESS_SERVER_FILES))
+       return filename;
+
+   /* User isn't a member of the default role, so check if it's allowable */
if (is_absolute_path(filename))
{

It seems to me that this is the intention behind the patch as the
comment points out and as Stephen has stated in
/messages/by-id/20171231191939.GR2416@tamriel.snowman.net.

Yes, this change is intentional. Note that superusers are members of
all roles explicitly (see the check in is_member_of_role()).

As you can see, you've added a short-circuit "return" statement for
anyone who has the DEFAULT_ROLE_ACCESS_SERVER_FILES. Prior to this
patch, even a Superuser would be subject to the following
is_absolute_path() logic. But with it, the return statement
short-circuits the is_absolute_path() check.

I agree that it is a strange concept to loosen the access while even
superusers cannot do that. By concept superusers are assumed to be able
to do anything on the server by the way.

As best as I can tell, the checks in these functions weren't because of
security concerns but simply because the original justification for them
was to be able to read files in the data directory and so they were
written specifically for that purpose. There's no such check in
lo_import(), for example, and it is, as Michael says, assumed that
superusers are equivilant to having full access to the server as the
user the database is running as.

This patch still needs updating for Magnus' comments, of course, and
I'm still planning to make that happen, so Waiting on Author is the
right status currently.

Thanks!

Stephen

Ok great. Thanks Michael and Stephen for the explanations.

Hi,

On 2018-01-19 09:28:33 -0500, Stephen Frost wrote:

This patch still needs updating for Magnus' comments, of course, and
I'm still planning to make that happen, so Waiting on Author is the
right status currently.

Given that this hasn't happened, and that the next CF has started, ISTM,
the patch should be marked as returned with feedback. If not, it at the
very least should be updated soon...

Greetings,

Andres Freund

Magnus, all,

* Magnus Hagander (magnus@hagander.net) wrote:

On Tue, Jan 2, 2018 at 1:08 PM, Stephen Frost <sfrost@snowman.net> wrote:

Suggestions on a name for this..? pg_server_copy_program?

Presumably it would also be used in postgres_fdw, so that seems like a bad
name. Maybe pg_exec_server_command?

I went with 'pg_execute_server_program', since 'program' is what we use
in the COPY syntax, et al.

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.

Thanks!

Stephen

On Tue, Mar 06, 2018 at 10:00:54AM -0500, Stephen Frost wrote:

* Magnus Hagander (magnus@hagander.net) wrote:

On Tue, Jan 2, 2018 at 1:08 PM, Stephen Frost <sfrost@snowman.net> wrote:

Suggestions on a name for this..? pg_server_copy_program?

Presumably it would also be used in postgres_fdw, so that seems like a bad
name. Maybe pg_exec_server_command?

I went with 'pg_execute_server_program', since 'program' is what we use
in the COPY syntax, et al.

Okay.

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)?

+   /*
+    * 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.

Could you update the documentation of pg_rewind please? It seems to me
that with your patch then superuser rights are not necessary mandatory,
as the role connecting to the source server does not need to have
superuser right, and needs just execution rights to pg_ls_dir,
pg_read_binary_files and pg_stat_file. Such a user does not need to be
granted access to pg_read_server_files either as no absolute paths are
involved in pg_rewind. Listing the functions directly would be also
nice. Personally I care a lot about 1), way less about 2).
--
Michael