Re: Add default role 'pg_access_server_files' - Mailing list pgsql-hackers

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