Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.7\))
Subject: Re: Non-superuser subscription owners
From: Mark Dilger <mark.dilger@enterprisedb.com>
In-Reply-To: <0774acfb4e613b9959c4e4995ba163cdadc454ef.camel@j-davis.com>
Date: Wed, 17 Nov 2021 16:17:42 -0800
Cc: Andrew Dunstan <andrew@dunslane.net>,
 PostgreSQL-development <pgsql-hackers@postgresql.org>,
 Robert Haas <robertmhaas@gmail.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <2EA8302B-9805-4FAA-A95D-C4D5CEB82916@enterprisedb.com>
References: <9DFC88D3-1300-4DE8-ACBC-4CEF84399A53@enterprisedb.com>
 <c1c334a2-5d85-687f-f0dc-0ac8c422ca1a@dunslane.net>
 <CD02D159-7260-4247-888F-2CFA7AD3798D@enterprisedb.com>
 <6BB4451E-7B1B-474C-BD1F-DB7531E720C6@enterprisedb.com>
 <aa28945f9f348307c8ac833a937b5fca732668b8.camel@j-davis.com>
 <6A2B0FF6-CC86-48CE-B0D3-5401AA5CFEA9@enterprisedb.com>
 <b6f6ed921b6e219875801857204b14bbc8782e5e.camel@j-davis.com>
 <CF56AC0D-7495-4E8D-A48F-FF38BD8074EB@enterprisedb.com>
 <0774acfb4e613b9959c4e4995ba163cdadc454ef.camel@j-davis.com>
To: Jeff Davis <pgsql@j-davis.com>
Archived-At: 
 <https://www.postgresql.org/message-id/2EA8302B-9805-4FAA-A95D-C4D5CEB82916%40enterprisedb.com>
Precedence: bulk


> On Nov 17, 2021, at 1:06 PM, Jeff Davis <pgsql@j-davis.com> wrote:
>=20
> On Wed, 2021-11-17 at 10:25 -0800, Mark Dilger wrote:
>> We may eventually allow non-superusers to create subscriptions, but
>> there are lots of details to work out.
>=20
> I am setting aside the idea of subscriptions created by =
non-superusers.

Ok, fair enough.  I think eventually we'll want that, but I'm also =
setting that aside for this patch.

> My comments were about your idea for "low-power users" that can still
> do things with subscriptions. And for that, GRANT seems like a better
> fit than ownership.

This patch has basically no value beyond the fact that it allows the =
replication to be *applied* as a user other than superuser.  Throw that =
out, and there isn't any point.  Everything else is window dressing.  =
The real security problem with subscriptions is that they act with =
superuser power. That naturally means that they must be owned and =
operated by superuser, too, otherwise they serve as a privilege =
escalation attack vector.  It really doesn't make any sense to think of =
subscriptions as operating under the permissions of multiple =
non-superusers.  You must choose a single role you want the subscription =
to run under.  What purpose would be served by GRANTing privileges on a =
subscription to more than one non-superuser?  It still operates as just =
the one user.  I agree you *could* give multiple users privileges to =
mess with it, but you'd still need to assign a single role as the one =
whose privileges matter for the purpose of applying replication changes. =
 I'm using "owner" for that purpose, and I think that is consistent with =
how security definer functions work.  They run as the owner, too.  It's =
perfectly well-precedented to use "owner" for this.

I think the longer term plan is that non-superusers who have some =
privileged role will be allowed to create subscriptions, and naturally =
they will own the subscriptions that they create, at least until an =
ALTER SUBSCRIPTION..OWNER TO is successfully executed to transfer =
ownership.  Once that longer term plan is complete, non-superusers will =
be able to create publications of their tables on one database, and =
subscriptions to those publications on another database, all without =
needing the help of a superuser.  This patch doesn't get us all the way =
there, but it heads directly toward that goal.

> With v2-0001, there are several things that seem weird to me:
>=20
>  * Why can there be only one low-power user per subscription?

Because the apply workers run as only one user.  Currently it is always =
superuser.  After this patch, it is always the owner, which amounts to =
the same thing for legacy subscriptions created and owned by superuser =
prior to upgrading to v15, but not necessarily for new ones or ones that =
have ownership transferred after upgrade.

We could think about subscriptions that act under multiple roles, =
perhaps taking role information as part of the data-stream from the =
publisher, but that's a pretty complicated proposal, and it is far from =
clear that we want it anyway.  There is a security case to be made for =
*not* allowing the publisher to call all the shots, so such a proposal =
would at best be an alternate mode of operation, not the one and only =
mode.

>  * Why is RENAME a separate capability from CREATE/DROP?

I don't care enough to argue this point.  If you want me to remove =
RENAME privilege from the owner, I can resubmit with it removed.  It =
just doesn't seem like it's dangerous to allow a non-superuser to rename =
their subscriptions, so I saw no compelling reason to disallow it.

CREATE clearly must be disallowed since it gives the creator the ability =
to form network connections, set fsync modes, etc., and there is no =
reason to assume arbitrary non-superusers should be able to do that.

The argument against DROP is a bit weaker.  It doesn't seem like a user =
who cannot bring subscriptions into existence should be able to drop =
them either.  I was expecting to visit that issue in a follow-on patch =
which deals with non-superuser predefined roles that have some power to =
create and drop subscriptions.  What that patch will propose to do is =
not obvious, since some of what you can do with subscriptions is so =
powerful we may not want non-superusers doing it, even with a privileged =
role.  If you can't picture what I mean, consider that you might use a =
connection parameter that connects outside and embeds data into the =
connection string, with a server listening on the other end, not really =
to publish data, but to harvest the secret data that you are embedding =
in the network connection attempt.

>  * What if you want to make the privileges more fine-grained, or make
> changes in the future? Ownership is a single bit, so it requires that
> everyone agree.

We can modify the patch to have the subscription owner have zero =
privileges on the subscription, not even the ability to see how it is =
defined, and just have "owner" mean the role under whose privileges the =
logical replication workers apply changes.  Would that be better?  I =
would expect people to find that odd.

The problem is that we want a setuid/setgid type behavior.  Actual =
setuid/setgid programs act as the user/group of the executable.  There's =
no reason that user/group needs to be one that any real human uses to =
log into the system.  Likewise, we need the subscription to act under a =
role, and we're establishing which role that is by having that role own =
the subscription.  That is like how setuid/setgid programs work by =
executing as the user/group that owns the executable, except that =
postgres doesn't have separate user/group concepts, just roles.  Isn't =
this design pattern completely commonplace?

> Maybe some people want RENAME to be a part of that, and
> others don't.

Fair enough.  Should I remove RENAME from what the patch allows the =
owner to do?  On this particular point, I genuinely don't care.  I think =
it can be reasonably argued either way.

> GRANT seems to provide better answers here.

No, because we don't have infinite privilege bits to burn.

>> Since we're trying to make subscriptions into things that non-
>> superusers can use, we have to deal with some things in those
>> functions.
>=20
> I understand the use case where a superuser isn't required anywhere in
> the process, and some special users can create and own subscriptions. =
I
> also understand that's not what these patches are trying to accomplish
> (though v2-0003 seems like a good step in that direction).

There is a cart-before-the-horse problem here.  If I propose a patch =
with a privileged role for creating and owning subscriptions *before* I =
tighten down how non-superuser-owned subscriptions work, that patch =
would surely be rejected.  So I either propose this first, and only =
if/when it gets accepted, propose the other, or I propose them together. =
 That's a damned-if-you-do--damned-if-you-dont situation, because if I =
propose them together, I'll get arguments that they are clearly =
separable and should be proposed separately, and if I do them one before =
the other, I'll get the argument that you are making now.  I fully =
expect the privileged role proposal to be made (possibly by me), though =
it is unclear if there will be time left to do it in v15.

> I don't understand the use case as well where a non-superuser can
> merely "use" a subscription. I'm sure such use cases exist and I'm =
fine
> to go along with that idea, but I'd like to understand why ownership
> (partial ownership?) is the right way to do this and GRANT is the =
wrong
> way.

Even if we had the privilege bits to burn, no spelling of that GRANT =
idea sounds all that great:

	GRANT RUN AS ON subscription TO role;
	GRANT RUN AS ON role TO subscription;
	GRANT SUDO ON subscription TO role;
	GRANT SETUID ON role TO subscription;
	...

I just don't see how that really works.  I'm not inclined to spend time =
being more clever, since I already know that privilege bits are in short =
supply, but if you want to propose something, go ahead.  Elsewhere you =
proposed GRANT REFRESH or something, not looking at that email just now, =
but that's not the same thing as GRANT RUN AS, and burns another =
privilege bit, and still doesn't get us all the way there, because you =
presumably also want GRANT RENAME, GRANT ALTER CONNECTION SETTING, GRANT =
ALTER FSYNC SETTING, ..., and we're out of privilege bits before we're =
done.

>> For example, ALTER SUBSCRIPTION can change the database connection
>> parameter, or the publication subscribed, or whether
>> synchronous_commit is used.  I don't see that a subscription owner
>> should necessarily be allowed to mess with that, at least not without
>> some other privilege checks beyond mere ownership.
>=20
> That violates my expectations of what "ownership" means.

I think that's because you're thinking of these settings as properties =
of the subscription.  You may *own* the subscription, but the =
subscription doesn't *own* the right to make connections to arbitrary =
databases, nor *own* the right to change buffer cache settings, nor =
*own* the right to bring data from a publication on some other server =
which, if it existed on the local server, would violate site policy and =
possibly constitute a civil or criminal violation of data privacy laws.  =
I may own my house, and the land it sits on, and my driveway, but that =
doesn't mean I own the ability to make my driveway go across my =
neighbor's field, down through town, and to the waterfront.  But that's =
the kind of ownership definition you seem to be defending.

Some of what I perceive as the screwiness of your argument I must admin =
is not your fault.  The properties of subscriptions are defined in ways =
that don't make sense to me.  It would be far more sensible if =
connection strings were objects in their own right, and you could grant =
USAGE on a connection string to a role, and USAGE on a subscription to a =
role, and only if the subscription worker's role had privileges on the =
connection string could they use it as part of fulfilling their task of =
replicating the data, and otherwise they'd error out in the attempt.  =
Likewise, fsync modes could be proper objects, and only if the =
subscription's role had privileges on the fsync mode they wanted to use =
would they be able to use it.  But we don't have these things as proper =
objects, with acl lists on them, so we're stuck trying to design around =
that.  To my mind, that means subscription owners *do not own* =
properties associated with the subscription.  To your mind, that's not =
what "ownership" means.  What to do?

>> I think this is pretty analogous to how security definer functions
>> work.
>=20
> The analogy to SECURITY DEFINER functions seems to support my
> suggestion for GRANT at least as much as your modified definition of
> ownership.

I don't see how.  Can you please explain?

>>> This would not address the weirdness of the existing code where a
>>> superuser loses their superuser privileges but still owns a
>>> subscription. But perhaps we can solve that a different way, like
>>> just
>>> performing a check when someone loses their superuser privileges
>>> that
>>> they don't own any subscriptions.
>>=20
>> I gave that a slight amount of thought during the design of this
>> patch, but didn't think we could refuse to revoke superuser on such a
>> basis,
>=20
> I don't necessarily see a problem there, but I could be missing
> something.

Close your eyes and imagine that I have superuser on your database... =
really picture it in your mind.  Now, do you want the revoke command you =
are issuing to work?

>> and didn't see what we should do with the subscription other than
>> have it continue to be owned by the recently-non-superuser.  If you
>> have a better idea, we can discuss it, but to some degree I think
>> that is also orthogonal to the purpose of this patch.  The only sense
>> in which this patch depends on that issue is that this patch proposes
>> that non-superuser subscription owners are already an issue, and
>> therefore that this patch isn't creating a new issue, but rather
>> making more sane something that already can happen.
>=20
> By introducing and documenting a way to get non-superusers to own a
> subscription, it makes it more likely that people will do it, and
> harder for us to change. That means the standard should be "this is
> what we really want", rather than just "more sane than before".

Ok, I'll wait to hear back from you on the points above.

=E2=80=94
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company