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

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> 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
securityrole to 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

I tend to agree that it'd be better to clean this up and just use
has_privs_of_role() and not include explicit superuser checks.  I don't
think we need to constantly re-remind ourselves in the code that
superusers are members of all roles.

> 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"/>.

I'm a bit on the fence about the documentation side...  I could be
convinced either way, really, but I generally agree that it'd be good to
pick one and be consistent.  I don't think the places where we do/don't
mention it were done for any particular reason.

> 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.

Agreed.

> 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 agree that installing extensions and event triggers are different
things, and if what we're left with is "create tablespaces" then maybe
we should have that as an explicit permission on its own.

> 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.

When it comes to these- we already have pg_read_server_files and
pg_write_server_files, so I'm not sure I see why it'd make sense to have
another thing that grants filesystem access like this..?

> 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.

Considering it's a role, and roles aren't specific to databases, I don't
think "pg_database_security" is a good name.  To me, at least, it
implies a way to allow a given user the ability to do most everything in
a specific database, but that's not the case at all, and further saying
'security' comes across as suggesting that this would be a role granted
to someone setting up permissions or otherwise modifying the database's
security aspects, not someone who is being given full access to every
object in the system.

I'm also left wondering if this doesn't end up introducing opportunities
for someone with this role to become superuser pretty easily.  Maybe it
does and maybe we're ok with that, but I would think that it'd be really
useful to have a role that can't become superuser easily which can
access/modify most objects in the system.

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

Agreed.

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

Agreed.

> 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.

I don't really see either of those as being filesystem changing things.

> 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.

It's often the case that logging/auditing are handled by a different
group than those who might be creating/modifying objects.  Yet another
group is often the group that actually handles granting access.  Broad
classes being:

- Users
- Auditors (controls what's logged, what is audited, etc)
- Security (controls who has access to what)

Note that 'security' and 'auditors' shouldn't have access to the actual
data either, or have the ability to do things like modify data.  Not
sure all of this quite fits what we're going for here but figured it
might help with sorting out what other buckets we need.

Thanks,

Stephen