Thread: Add default role 'pg_access_server_files'
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. Thanks! Stephen
Attachment
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, * 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
Attachment
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, * 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
Attachment
Stephen, so far I've read thru your patch and familiarized myself with some of the auth functionality in pg_authid.h andsrc/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 withoutadding 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 insteadof replaced by is_member_of_role() checks like you did elsewhere). I still have limited understanding of the overallcode, 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 andsrc/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 withoutadding 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 insteadof replaced by is_member_of_role() checks like you did elsewhere). I still have limited understanding of the overallcode, 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
Attachment
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 filesin 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 todo 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
psql (9.4.5, server 11devel)
Type "help" for help.
postgres=> select pg_read_file('/Users/postgres/temp');
ERROR: absolute path not allowed
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_filesto 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
Attachment
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 withmy 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 mysuperuser to be able to perform this task, but it seems that for security reasons we presently don't allow access to absolutepath names (except in the data dir and log dir) - not even for a superuser. Is that correct? In that case the securityimplications 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 granularityof 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 https://www.postgresql.org/message-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
Attachment
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 > https://www.postgresql.org/message-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
Attachment
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
Attachment
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
Attachment
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
Attachment
On Wed, Mar 07, 2018 at 07:00:03AM -0500, Stephen Frost wrote: > * Michael Paquier (michael@paquier.xyz) wrote: >> 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. One patch should defend one idea, this makes the git history easier to understand in my opinion. > 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. Hm, OK... >> 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. That's what I did, manually, using the attached SQL script with your patch 1 applied. You can check for the list of functions used by pg_rewind in libpq_fetch.c where you just need to grant execution access to those functions for a login user and you are good to go: pg_ls_dir(text, boolean, boolean) pg_stat_file(text, boolean) pg_read_binary_file(text) pg_read_binary_file(text, bigint, bigint, boolean) So getting this documented properly would be nice. Of course feel free to test this by yourself as you wish. Could you send separate files for each patch by the way? This eases testing, and I don't recall that git am has a way to enforce only a subset of patches to be applied based on one file, though my git-fu is limited in this area. + read or which program is run. In principle regular users could be allowed to change the other options, but that's not supported at present. Well, the parsing of file_fdw complains if "program" or "filename" is not defined, so a user has to be in either pg_read_server_files to use "filename" or in pg_execute_server_program to use "program", so I am not sure that the last sentence of this paragraph makes much sense from the beginning. - Return the contents of a file. + Return the contents of a file. Restricted to superusers by default, but other users can be granted EXECUTE to run the function. </entry> You should make those paragraphs spawn into multiple lines. EXECUTE should use <literal> markup, and I think that you should use "EXECUTE privilege to run the function" on those doc portions. That's all for the nits. Other than that the patch looks in pretty good shape to me. -- Michael
Attachment
On Thu, Mar 08, 2018 at 10:15:11AM +0900, Michael Paquier wrote: > Other than that the patch looks in pretty good shape to me. The regression tests of file_fdw are blowing up because of an error string patch 2 changes. -- Michael
Attachment
Greetings, * Michael Paquier (michael@paquier.xyz) wrote: > On Thu, Mar 08, 2018 at 10:15:11AM +0900, Michael Paquier wrote: > > Other than that the patch looks in pretty good shape to me. > > The regression tests of file_fdw are blowing up because of an error > string patch 2 changes. Fixed in the attached. Does anyone have an opinion regarding the adminpack functions? I was just reviewing the patch and considering if we should adjust the privileges there also and it seems like we should. That'd be a pretty straight-forward change, of course, so unless there's some reason not to then I'll see about providing an updated patch tomorrow which covers those functions as well. Note that it'll be a bit more complicated since we can't just remove the checks from the existing functions- we'll need to have new functions where the checks are removed and a new extension version that updates to the new functions and then REVOKE's access to them. Not a big deal, just pointing out that it's not quite as straight-forward since it's an extension and we need to deal with environments where the server's been upgraded and the .so changed, but the existing functions are still in place with their current public-execute rights. Thanks! Stephen
Attachment
On Sun, Mar 25, 2018 at 09:43:25PM -0400, Stephen Frost wrote: > * Michael Paquier (michael@paquier.xyz) wrote: >> On Thu, Mar 08, 2018 at 10:15:11AM +0900, Michael Paquier wrote: >> > Other than that the patch looks in pretty good shape to me. >> >> The regression tests of file_fdw are blowing up because of an error >> string patch 2 changes. > > Fixed in the attached. Thanks for the updated version. This test is fixed. Patch 2 includes the documentation changes from patch 1, which would matter only if you decide to keep things splitted. As far as my brain sees the patch is logically correct. > Note that it'll be a bit more complicated since we can't just remove the > checks from the existing functions- we'll need to have new functions > where the checks are removed and a new extension version that updates to > the new functions and then REVOKE's access to them. Not a big deal, > just pointing out that it's not quite as straight-forward since it's an > extension and we need to deal with environments where the server's been > upgraded and the .so changed, but the existing functions are still in > place with their current public-execute rights. Yeah, that's basically what you did for pgstattuple in fd321a1. I am not sure that I would have time to double-check what you code and the commit fest ends in 5 days. There are many other patches in need of attention, so I would be incline to just do this portion in the future and keep the proposal as-is. My 2c. -- Michael
Attachment
Greetings, * Michael Paquier (michael@paquier.xyz) wrote: > On Sun, Mar 25, 2018 at 09:43:25PM -0400, Stephen Frost wrote: > > * Michael Paquier (michael@paquier.xyz) wrote: > >> On Thu, Mar 08, 2018 at 10:15:11AM +0900, Michael Paquier wrote: > >> > Other than that the patch looks in pretty good shape to me. > >> > >> The regression tests of file_fdw are blowing up because of an error > >> string patch 2 changes. > > > > Fixed in the attached. > > Thanks for the updated version. This test is fixed. Thanks for checking. Attached is an updated version which also includes the changes for adminpack, done in a similar manner to how pgstattuple was updated, as discussed. Regression tests updated and extended a bit, doc updates also included. If you get a chance to take a look, that'd be great. I'll do my own review of it again also after stepping away for a day or so. Thanks! Stephen
Attachment
On Sun, Apr 01, 2018 at 09:39:02AM -0400, Stephen Frost wrote: > Thanks for checking. Attached is an updated version which also includes > the changes for adminpack, done in a similar manner to how pgstattuple > was updated, as discussed. Regression tests updated and extended a bit, > doc updates also included. > > If you get a chance to take a look, that'd be great. I'll do my own > review of it again also after stepping away for a day or so. I have spotted some issues mainly in patch 3. I am not sure what has happened to your editor, but git diff --check is throwing a dozen of warnings coming from adminpack.c. c0cbe00 has stolen the OIDs your patch is using for the new roles, so patch 2 needs a refresh. @@ -68,6 +77,15 @@ convert_and_check_filename(text *arg, bool logAllowed) [...] + /* + * 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; So... If a user has loaded adminpack v1.0 with Postgres v11, then convert_and_check_filename would actually be able to read paths out of the data folder for a user within pg_read_server_files, while with Postgres v10 then only paths within the data folder were allowed. And that7s actually fine because a superuser check happens before entering in this code path. pg_file_rename(PG_FUNCTION_ARGS) +{ + text *file1; + text *file2; + text *file3; + bool result; + + if (PG_ARGISNULL(0) || PG_ARGISNULL(1)) + PG_RETURN_NULL(); + + file1 = PG_GETARG_TEXT_PP(0); + file2 = PG_GETARG_TEXT_PP(1); + + if (PG_ARGISNULL(2)) + file3 = NULL; + else + file3 = PG_GETARG_TEXT_PP(2); + + requireSuperuser(); Here requireSuperuser() should be called before looking at the argument values. No refactoring for pg_file_unlink and its v1.1? The argument checks are exactly the same for +pg_file_rename and pg_file_rename_v1_1. Why about just passing fcinfo around and simplify the patch? +CREATE OR REPLACE FUNCTION pg_catalog.pg_file_rename(text, text) +RETURNS bool +AS 'SELECT pg_catalog.pg_file_rename($1, $2, NULL::pg_catalog.text);' +LANGUAGE SQL VOLATILE STRICT; You forgot a REVOKE clause for pg_file_rename(text, text). In adminpack.c, convert_and_check_filename is always called with false as second argument. Why not dropping it and use the version in genfile.c instead? As far as I can see, both functions are the same. pg_read_file and pg_read_file_v2 could be refactored as well with an internal routine. Having to support v1 and v2 functions in the backend code is not elegant. Would it actually work to keep the v1 function in adminpack.c and the v2 function in genfile.c even if adminpack 1.0 is loaded? This way you keep in core only one function. What matters is that the function name matches, right? +int64 pg_file_write_internal(text *file, text *data, bool replace); +bool pg_file_rename_internal(text *file1, text *file2, text *file3); +Datum pg_logdir_ls_internal(FunctionCallInfo fcinfo) Those three functions should be static. -- Michael
Attachment
Michael, all, * Michael Paquier (michael@paquier.xyz) wrote: > On Sun, Apr 01, 2018 at 09:39:02AM -0400, Stephen Frost wrote: > > Thanks for checking. Attached is an updated version which also includes > > the changes for adminpack, done in a similar manner to how pgstattuple > > was updated, as discussed. Regression tests updated and extended a bit, > > doc updates also included. > > > > If you get a chance to take a look, that'd be great. I'll do my own > > review of it again also after stepping away for a day or so. > > I have spotted some issues mainly in patch 3. Thanks for taking a look. > I am not sure what has happened to your editor, but git diff --check is > throwing a dozen of warnings coming from adminpack.c. Ahhh, those came from switching over to tmux.. I need to figure out how to get it to copy/paste like I had set up before with screen. Anyhow, they were all in patch 3 and I've fixed them all. > c0cbe00 has stolen the OIDs your patch is using for the new roles, so > patch 2 needs a refresh. Fixed and generally rebased. > @@ -68,6 +77,15 @@ convert_and_check_filename(text *arg, bool logAllowed) > [...] > + /* > + * 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; > So... If a user has loaded adminpack v1.0 with Postgres v11, then > convert_and_check_filename would actually be able to read paths out of > the data folder for a user within pg_read_server_files, while with > Postgres v10 then only paths within the data folder were allowed. And > that7s actually fine because a superuser check happens before entering > in this code path. Yes, all of the adminpack v1.0 code paths still have superuser checks, similar to how the older pgstattuple code paths do. When an upgrade to adminpack v1.1 is done, the new v1_1 functions don't have those superuser checks but the upgrade script REVOKE's execute rights from public, so the right to execute the functions has to be explicitly GRANT'd for non-superusers. > pg_file_rename(PG_FUNCTION_ARGS) > +{ > + text *file1; > + text *file2; > + text *file3; > + bool result; > + > + if (PG_ARGISNULL(0) || PG_ARGISNULL(1)) > + PG_RETURN_NULL(); > + > + file1 = PG_GETARG_TEXT_PP(0); > + file2 = PG_GETARG_TEXT_PP(1); > + > + if (PG_ARGISNULL(2)) > + file3 = NULL; > + else > + file3 = PG_GETARG_TEXT_PP(2); > + > + requireSuperuser(); > Here requireSuperuser() should be called before looking at the > argument values. Moved up. > No refactoring for pg_file_unlink and its v1.1? I considered each function and thought about if it'd make sense to refactor them or if they were simple enough that the additional function wouldn't really be all that useful. I'm open to revisiting that, but just want to make it clear that it was something I thought about and considered. Since pg_file_unlink is basically two function calls, I didn't think it worthwhile to refactor those into their own function. > The argument checks are exactly the same for +pg_file_rename and > pg_file_rename_v1_1. Why about just passing fcinfo around and simplify > the patch? In general, I prefer to keep the PG_FUNCTION_ARGS abstraction when we can. Unfortunately, that does fall apart when wrapping an SRF as in pg_logdir_ls(), but with pg_file_rename we can maintain it and it's really not that much code to do so. As with the refactoring of pg_file_unlink, this is something which could really go either way. > +CREATE OR REPLACE FUNCTION pg_catalog.pg_file_rename(text, text) > +RETURNS bool > +AS 'SELECT pg_catalog.pg_file_rename($1, $2, NULL::pg_catalog.text);' > +LANGUAGE SQL VOLATILE STRICT; > You forgot a REVOKE clause for pg_file_rename(text, text). No, I explicitly didn't include it because that's a security-invoker SQL function that basically doesn't do anything but turn around and call pg_file_rename(), which will handle the privilege check: =*> select pg_file_rename('abc','123'); ERROR: permission denied for function pg_file_rename CONTEXT: SQL function "pg_file_rename" statement 1 I'm not sure how useful it is to REVOKE the rights on the simple SQL function; that would just mean that an admin has to go GRANT the rights on that function as well as the three-argument version. > In adminpack.c, convert_and_check_filename is always called with false > as second argument. Why not dropping it and use the version in > genfile.c instead? As far as I can see, both functions are the same. Hmm. I'm pretty sure that function was actually copied from adminpack into core, so I'm not surprised that they're basically the same, but it was implemented in core as static and I'm not really sure that we want to export it- it wasn't when it was first copied into core, after all. Also, they actually should be different- the functions in core are for reading files while the ones in adminpack are for writing to files, so that should really be checking against the pg_write_server_files role instead, as updated in the patch. > pg_read_file and pg_read_file_v2 could be refactored as well with an > internal routine. Having to support v1 and v2 functions in the backend > code is not elegant. Would it actually work to keep the v1 function in > adminpack.c and the v2 function in genfile.c even if adminpack 1.0 is > loaded? This way you keep in core only one function. What matters is > that the function name matches, right? The function name in core would still have to be different and it's also possible that there are other callers of it beyond those in adminpack, so keeping them in core also avoids breaking those callers. That's not a huge deal in general, of course, but I think it tends to tip the scales towards just having two versions in core, at least for now. Thinking about this a bit more though, those old function names were listed as deprecated and have been such for quite a while. If a user upgrades to adminpack 1.1, which they would have to do pretty much explicitly, maybe they'll be expecting and understanding that they might have to change their user code and instead of trying to support these old names in adminpack for compatibility we should just drop the old function names? Users could still install adminpack 1.0 if they wish to, after all. The more I think about it, the more I like the apporach of just dropping these deprecated "alternative names for things in core" with the new adminpack version. In terms of applications, as I understand it, they aren't used in the latest version of pgAdmin3 and they also aren't used with pgAdmin4, so I don't think we need to be worrying about supporting them in v11. I've dropped those functions from the new patch. We still need to keep the backend functions for adminpack v1.0, of course, but perhaps in a few releases we can decide that adminpack v1.0 is no longer supported and drop it and the functions which are supporting it in core. > +int64 pg_file_write_internal(text *file, text *data, bool replace); > +bool pg_file_rename_internal(text *file1, text *file2, text *file3); > +Datum pg_logdir_ls_internal(FunctionCallInfo fcinfo) > Those three functions should be static. Fixed. I also went through and updated the docs for the new default roles (we have a list of the default roles in the docs with what they're for). In those docs I also pointed out that users with these roles can bypass the in-database permissions checks, etc. I also went back and added similar warnings to other places in the docs to hopefully make sure everyone realizes that these roles are very nearly "superuser" equivilant. Updated patch attached. I still want to do more review of it, but this is defintiely starting to feel better to me. I'm going to spend tomorrow making sure that the patch to update the pg_dump-related regression tests is good to go and hopefully push that, after which I'll come back to this for another review. Thanks! Stephen
Attachment
On Mon, Apr 02, 2018 at 05:09:21PM -0400, Stephen Frost wrote: > * Michael Paquier (michael@paquier.xyz) wrote: >> No refactoring for pg_file_unlink and its v1.1? > > I considered each function and thought about if it'd make sense to > refactor them or if they were simple enough that the additional function > wouldn't really be all that useful. I'm open to revisiting that, but > just want to make it clear that it was something I thought about and > considered. Since pg_file_unlink is basically two function calls, I > didn't think it worthwhile to refactor those into their own function. I don't mind if this is done your way. >> The argument checks are exactly the same for pg_file_rename and >> pg_file_rename_v1_1. Why about just passing fcinfo around and simplify >> the patch? > > In general, I prefer to keep the PG_FUNCTION_ARGS abstraction when we > can. Unfortunately, that does fall apart when wrapping an SRF as in > pg_logdir_ls(), but with pg_file_rename we can maintain it and it's > really not that much code to do so. As with the refactoring of > pg_file_unlink, this is something which could really go either way. Okay... > I'm not sure how useful it is to REVOKE the rights on the simple SQL > function; that would just mean that an admin has to go GRANT the rights > on that function as well as the three-argument version. Indeed, I had a brain fade here. > The more I think about it, the more I like the approach of just dropping > these deprecated "alternative names for things in core" with the new > adminpack version. In terms of applications, as I understand it, they > aren't used in the latest version of pgAdmin3 and they also aren't used > with pgAdmin4, so I don't think we need to be worrying about supporting > them in v11. +1 to simplify the code a bit. -- Michael
Attachment
Michael, * Michael Paquier (michael@paquier.xyz) wrote: > On Mon, Apr 02, 2018 at 05:09:21PM -0400, Stephen Frost wrote: > > * Michael Paquier (michael@paquier.xyz) wrote: > >> No refactoring for pg_file_unlink and its v1.1? > > > > I considered each function and thought about if it'd make sense to > > refactor them or if they were simple enough that the additional function > > wouldn't really be all that useful. I'm open to revisiting that, but > > just want to make it clear that it was something I thought about and > > considered. Since pg_file_unlink is basically two function calls, I > > didn't think it worthwhile to refactor those into their own function. > > I don't mind if this is done your way. > > >> The argument checks are exactly the same for pg_file_rename and > >> pg_file_rename_v1_1. Why about just passing fcinfo around and simplify > >> the patch? > > > > In general, I prefer to keep the PG_FUNCTION_ARGS abstraction when we > > can. Unfortunately, that does fall apart when wrapping an SRF as in > > pg_logdir_ls(), but with pg_file_rename we can maintain it and it's > > really not that much code to do so. As with the refactoring of > > pg_file_unlink, this is something which could really go either way. > > Okay... > > > I'm not sure how useful it is to REVOKE the rights on the simple SQL > > function; that would just mean that an admin has to go GRANT the rights > > on that function as well as the three-argument version. > > Indeed, I had a brain fade here. > > > The more I think about it, the more I like the approach of just dropping > > these deprecated "alternative names for things in core" with the new > > adminpack version. In terms of applications, as I understand it, they > > aren't used in the latest version of pgAdmin3 and they also aren't used > > with pgAdmin4, so I don't think we need to be worrying about supporting > > them in v11. > > +1 to simplify the code a bit. Great, thanks. I'll be doing more review of it myself and see about pushing it later this afternoon. Stephen
Attachment
Greetings, * Stephen Frost (sfrost@snowman.net) wrote: > Great, thanks. I'll be doing more review of it myself and see about > pushing it later this afternoon. Took a bit longer as I wanted to check over a few more things, but I've now pushed this. Thanks much for all of the help with review and commentary, Michael and Magnus! Thanks again! Stephen