Re: Delegating superuser tasks to new security roles (Was: Granting control of SUSET gucs to non-superusers) - Mailing list pgsql-hackers

On Thu, Jul 1, 2021 at 11:59 AM Mark Dilger <mark.dilger@enterprisedb.com> wrote
> > On Jun 29, 2021, at 6:25 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> > Please find attached a new set of patches.
>
> And again, this time attaching a fifth patch which includes the work to allow users who belong to the right security
roleto SET and ALTER SYSTEM SET variables without being a superuser.
 

In general, I don't like this kind of coding:

-       /* Superusers bypass all permission checking. */
-       if (superuser_arg(roleid))
+       /*
+        * Superusers and members of the pg_host_security role bypass all
+        * permission checking.
+        */
+       if (superuser_arg(roleid) ||
+               has_privs_of_role(roleid, ROLE_PG_HOST_SECURITY))
                return true;

From a theoretical point of view, the superuser has the privileges of
every role, so this is redundant. From a coding point of view,
has_privs_of_role() does a superuser_arg() test already, and it seems
likely to be a loser to do it twice. Arguably, we should take the same
approach in code comments and documentation, rather than saying "You
must be superuser or a member of role XYZ" everywhere, but there seems
to be some existing precedent for mentioning superuser explicitly in
those cases, so maybe it's fine. I think it's kind of weird though,
because in other places we don't do it, e.g.:

      unique or primary key constraint in the referenced table.  The user
      must have <literal>REFERENCES</literal> permission on the referenced table
      (either the whole table, or the specific referenced columns).  The

We could have said "or be superuser" there, but we didn't. It doesn't
seem efficient to say "or be superuser" every time we mention a
required permission, rather than just taking it as a given that the
superuser has all permissions. Yet, again, there's some precedent for
your approach:

   To create a database, you must be a superuser or have the special
   <literal>CREATEDB</literal> privilege.
   See <xref linkend="sql-createrole"/>.

So I don't know. At the very least I think we should not do it as an
"or" in the code; what we want to do in comments and documentation I'm
less sure.

I think 0002 needs more explanation of the theory behind the specific
permissions granted. It enumerates what they are in both the commit
message and the documentation, but no theory is offered explaining why
these permissions are included and not others. I think my idea was
that "host" security would encompass everything that touches the
filesystem on the server where the database is running. I agree that
this naturally includes the ability to create a tablespace and
probably, at least for symmetry, the ability to drop it. But, you
can't ALTER a tablespace's location, so I see no reason why that
should be tied to this permission. I think it's arguable whether it
includes creating and dropping extensions, but I would argue that it
shouldn't. True, the extensions include SQL files installed in the
filesystem, and shared libraries also installed on the filesystem, but
ultimately everything you ever do involves files in some way, so I
don't see that as a very compelling argument. These operations on
extensions don't let you modify the filesystem in any way, and they
only let you read from carefully sandboxed things that are designed
for precisely that purpose, so the system administrator really already
has good control. The sorts of things I'd include in this category are
things like server-side COPY FROM or COPY TO. When we come to the
third thing the patch includes in this category, creating and dropping
event triggers, I *really* don't understand why that one is considered
host security. That one isn't touching the filesystem even to the
extent that the extension stuff is; it seems to me to be purely
internal to the database. Yeah, OK, that could involve writing files
because we make catalog entries, but so could any DDL. Now, maybe
there's a theory of operation that you have in mind that makes this
all make more sense the way you have it, but if so, it seems not to be
spelled out anywhere in the patch itself or the commit message you
wrote for it, so I'm in the dark.

I also tend to think that functions like pg_read_file() ought to come
with execute permission pre-granted, with grant option, to
pg_host_security, and perhaps similarly for adminpack.

From the department of nitpicking, all four of your commit messages
begin with the word "Reducing" which I think should be just "Reduce".
Otherwise, at least to me, it doesn't look like a proper sentence.

0004 has this kind of thing all over the place:

-       /* Superusers bypass all permission checking. */
-       if (superuser_arg(roleid))
+       /*
+        * Superusers and members of the pg_database_security role bypass all
+        * permission checking.
+        */

If that were true, the pg_database_security role would be equivalent
to superuser, which isn't the intent, so I think the comment needs
more thought. Also, I think that somewhere in the patch, either in
code comments or at the very least in the commit message, there needs
to be some justification of why the approach taken here is correct.
Like, the idea here is that if you have pg_database_security, you can
do whatever you want to objects within the database as long as you
don't try to touch the network or the host filesystem. So that would
imply that you can do anything you like to databases. So it sorta
makes sense to me that the patch goes about that by changing
pg_database_aclmask(). But I would feel better if there were some
explanation somewhere of why such a change is expecting to allow
precisely DDL-related database commands and nothing else. I think
that's true if pg_database_aclmask() is used for that purpose and not
for any other purpose, which may well be true, but I think it would be
best to be more explicit about the assumptions. I'm sure we don't want
a lengthy comment about this in every pg_*_aclmask() function, but I
think we should have a general explanation of it somewhere.

I also think that in this case, as in 0002 and 0003, we really need
some documentation of what this new role is all about. The
documentation changes in 0004 are really extremely minimal. Users need
to understand what they can expect to happen if they grant this new
role to someone, and hackers need to understand how to update the code
the next time they're patching something that interacts with this, and
I do not think that what you've got here now is going to be sufficient
to meet the needs of either group. (I realize that you may have been
planning to wait until there was more consensus to flesh this out, but
because the definitional issues here are so tricky, I don't think it
can wait in this case.)

In 0005, I do not think the function name role_has_privileges() is
sufficiently specific. Maybe role_can_change_guc()? Also, I think here
again you should draft some documentation changes. We're going to need
to indicate a category for every GUC somehow, and I'm not quite sure
how we're going to do that. If you want to just do a few examples for
now and also provide some general documentation on how the system is
intended to work, we can wait to do every GUC until we settle on how
to categorize everything, but I think we need to see what the general
plan looks like there. Consider the way that we currently indicate (a)
the GUC's data type and (b) when the GUC can be changed. The former is
shown in fixed point type in parentheses after the GUC name. The
latter is indicated by adding a sentence such as "This parameter can
only be set at server start." to everything that is PGC_POSTMASTER (I
think). What are we going to do with this new categorization?

I see that you've categorized things like restart_after_crash and
zero_damaged_pages as GUC_HOST_SECURITY. I think I like that, but it
again begs the definitional question. If host security basically means
touching the server filesystem, well, restart_after_crash doesn't. It
can be justified from the perspective that restart_after_crash is a
property of the host system, not something strictly internal to the
database. So when you go to write definitions of what these categories
are actually supposed to mean, they've got to be written in such a way
that these categorizations end up looking correct. Or else these have
got to be recategorized somehow. Anyway the point is that it "feels
good" but as you have it without the definitions it's hard to really
know.

The categorization of the logging GUCs looks haphazard to me. Why is
log_duration GUC_HOST_SECURITY but debug_print_parse is
GUC_DATABASE_SECURITY, for example? Again, we need clear definitions,
but I'm inclined to think this doesn't look great. I even less
understand why autovacuum is classified as GUC_HOST_SECURITY. That
seems like it's probably database security, while db_user_namespace
feels to me like network security. Another oddity is the replication
settings, which seem to be mostly classified as GUC_HOST_SECURITY. I
can see why you don't want to make them GUC_DATABASE_SECURITY, but eh,
what do they have to do with host security? It's similarly odd to me
that hash_mem_multiplier is GUC_DATABASE_SECURITY while work_mem, for
which it is a multiplier, is GUC_HOST_SECURITY.

Perhaps we need to break this up into a few more buckets to make sense
of it; I'm not really sure. For example, we could add buckets for
controlling what goes to the server log, resource utilization, system
integrity, and split inbound and outbound network security. Well, now
I just turned your three predefined roles into seven, which maybe is a
bad idea, but perhaps it's worth it if it gets us to a place where we
can clearly categorize everything. On the other hand, maybe if we did
that there'd just be a new set of things that look a little ambiguous.
I don't know. I guess trying to write a good set of definitions might
be job one.

--
Robert Haas
EDB: http://www.enterprisedb.com