Re: Add default role 'pg_access_server_files'

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