Re: [PATCH] Simplify permission checking logic in user.c

On Wed, Dec 30, 2020 at 12:28 PM Andrey Borodin <x4mmm(at)yandex-team(dot)ru> wrote:
> I think that sharing "various small changes to permission checks" is a really good idea.
>
> > 30 дек. 2020 г., в 20:39, Stephen Frost <sfrost(at)snowman(dot)net> написал(а):
> > In other words, I suspect people would be happier if we
> > provided a way for non-superusers a way to create replication roles and
> > bypassrls roles.
> +1 again. I hope we will return to the topic soon.

On Wed, Dec 30, 2020 at 9:26 AM Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> While I do appreciate that it'd be nice if we made all of the code in
> the backend easier for folks to maintain their own patch sets against
> core, it'd also mean that we're now being asked to provide some level of
> commitment that we aren't going to change these things later because
> then we'd break $whomever's patches. That's certainly not something
> that is really reasonable for us to agree to.
>
> I'd strongly suggest that, instead, you consider proposing changes which
> would address the actual use cases you have and work with the community
> to have those included in core, which would further have the added
> property that everyone would then benefit from those improvements.

On Wed, Dec 30, 2020 at 9:39 AM Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
> That really strikes me as pretty darn unlikely to happen, it's not like
> this code is really spread out across very many lines or that it's hard
> to see the pretty clear pattern.
>
> In thinking about these role attributes and the restrictions we have
> about what attributes don't require superuser to set and what do, I do
> feel like we're probably missing a bet regarding replication and
> bypassrls.. In other words, I suspect people would be happier if we
> provided a way for non-superusers a way to create replication roles and
> bypassrls roles. Exactly how we do that is a bit tricky to figure out
> but that's certainly a much more productive direction to be going in.

Thanks everyone for taking a look!

You've identified exactly the problem we're running into -- we want to
allow customers, who aren't superusers, to create replication roles.

In Google Cloud SQL we create a role for customers, cloudsqlsuperuser,
which, confusingly, is not a SUPERUSER, but does have some extra
permissions. We've modified a lot of "if (!superuser())" checks to
"if (!superuser() && !cloudsqlsuperuser())".

That was actually what I initially tried to do when modifying this bit of
code:

if (issuperuser)
if (!superuser()) ereport(...);
elsif (isreplication)
- if (!superuser()) ereport(...);
+ if (!superuser() && !cloudsqlsuperuser()) ereport(...);
elsif (bypassrls)
if (!superuser()) ereport(...);
else
if (!have_createrole_privilege()) ereport()

But this inadvertently allowed cloudsqlsuperuser to _also_ create users
with the BYPASSRLS attribute by creating a user with both REPLICATION
and BYPASSRLS, which I only realized when a couple of tests failed.
Properly modifying the required permissions required separating the
if/else branches, which led to this patch.

From my understanding, AWS RDS Postgres takes a slightly different approach
and allows the REPLICATION attribute to be inherited through membership in
a special rds_replication role.

We haven't seriously considered what some sort of general functionality
would look like to support our managed Postgres use-cases, but here's a
rough sketch of what we're trying to accomplish with roles and permissions:

- We, ideally, want to give our customers access to as much of Postgres'
functionality as possible, including SUPERUSER capabilities

- But we do not want customers to have access to the underlying VM or
filesystem

- There are certain parts of the system (i.e., certain databases, tables,
individual rows in catalog tables, etc.) that we want to manage and we
can't allow customers to modify these at all. Examples include:
- Our own SUPERUSER role that we use to connect to the cluster; customers
shouldn't be able to modify this role at all
- Replication slots used for managed replication shouldn't be able to be
modified by customers (even if they have the REPLICATION attribute)

- We want to prevent customers from depending on any data we choose to store
in other database, as that limits our flexibility to make future changes
- Notably this means we only can support logical replication, and not
physical replication.

I suppose this could be roughly summarized as "90% of a superuser, but also
still obeys SQL privileges for objects created by someone else".

We would definitely be happy to explore what sort of functionality like this
would be useful for others!