Обсуждение: running logical replication as the subscription owner

Hi,

Here's a patch against master for $SUBJECT. It lacks documentation
changes and might have bugs, so please review if you're concerned
about this issue.

To recap, under CVE-2020-14349, Noah documented that untrusted users
shouldn't own tables into which the system is performing logical
replication. Otherwise, the users could hook up triggers or default
expressions or whatever to those tables and they would execute with
the subscription owner's privileges, which would allow the table
owners to escalate to superuser. However, I'm unsatisfied with just
documenting the hazard, because I feel like almost everyone who uses
logical replication wants to do the exact thing that this
documentation says you shouldn't do. If you don't use logical
replication or run everything as the superuser or just don't care
about security, well then you don't have any problem here, but
otherwise you probably do.

The proposed fix is to perform logical replication actions (SELECT,
INSERT, UPDATE, DELETE, and TRUNCATE) as the user who owns the table
rather than as the owner of the subscription. The session still runs
as the subscription owner, but the active user ID is switched to the
table owner for the duration of each operation. To prevent table
owners from doing tricky things to attack the subscription owner, we
impose SECURITY_RESTRICTED_OPERATION while running as the table owner.
To avoid inconveniencing users when this restriction adds no
meaningful security, we refrain from imposing
SECURITY_RESTRICTED_OPERATION when the table owner can SET ROLE to the
subscription owner anyway. Such a user need not use logical
replication to break into the subscription owner's account: they have
access to it anyway.

There is also a possibility of an attack in the other direction. Maybe
the subscription owner would like to obtain the table owner's
permissions, or at the very least, use logical replication as a
vehicle to perform operations they can't perform directly. A malicious
subscription owner could hook up logical replication to a table into
which the table owner doesn't want replication to occur. To block such
attacks, the patch requires that the subscription owner have the
ability to SET ROLE to each table owner. If the subscription owner is
a superuser, which is usual, this will be automatic. Otherwise, the
superuser will need to grant to the subscription owner the roles that
own relevant tables. This can usefully serve as a kind of access
control to make sure that the subscription doesn't touch any tables
other than the ones it's supposed to be touching: just make those
tables owned by a different user and don't grant them to the
subscription owner. Previously, we provided no way at all of
controlling the tables that replication can target.

This fix interacts in an interesting way with Mark Dilger's work,
committed by Jeff Davis, to make logical replication respect table
permissions. I initially thought that with this change, that commit
would end up just being reverted, with the permissions scheme
described above replacing the existing one. However, I then realized
that it's still good to perform those checks. Normally, a table owner
can do any DML operation on a table they own, so those checks will
never fail. However, if a table owner has revoked their ability to,
say, INSERT into one of their own tables, then logical replication
shouldn't bypass that and perform the INSERT anyway. So I now think
that the checks added by that commit complement the ones added by this
proposed patch, rather than competing with them.

It is unclear to me whether we should try to back-port this. It's
definitely a behavior change, and changing the behavior in the back
branches is not a nice thing to do. On the other hand, at least in my
opinion, the security consequences of the status quo are pretty dire.
I tend to suspect that a really high percentage of people who are
using logical replication at all are vulnerable to this, and lots of
people having a way to escalate to superuser isn't good.

Comments?

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

I'm definitely in favor of making it easier to use logical replication
in a safe manner. In Citus we need to logically replicate and we're
currently using quite some nasty and undocumented hacks to do so:
We're creating a subscription per table owner, where each subscription
is owned by a temporary user that has the same permissions as the
table owner. These temporary users were originally superusers, because
otherwise we cannot make them subscription owners, but once assigning
a subscription to them we take away the superuser permissions from
them[1]. And we also need to hook into ALTER/DELETE subscription
commands to make sure that these temporary owners cannot edit their
own subscription[2].

Getting this right was not easy. And even it has the serious downside
that we need multiple subscriptions/replication slots which causes
extra complexity in various ways and it eats much more aggressively
into the replication slot limits than we'd like. Having one
subscription that could apply into tables that were owned by multiple
users in a safe way would make this sooo much easier.

[1]:
https://github.com/citusdata/citus/blob/main/src/backend/distributed/replication/multi_logical_replication.c#L1487-L1573
[2]: https://github.com/citusdata/citus/blob/main/src/backend/distributed/commands/utility_hook.c#L455-L487

On Fri, Mar 3, 2023 at 6:57 PM Jelte Fennema <postgres@jeltef.nl> wrote:
> I'm definitely in favor of making it easier to use logical replication
> in a safe manner.

Cool.

> In Citus we need to logically replicate and we're
> currently using quite some nasty and undocumented hacks to do so:
> We're creating a subscription per table owner, where each subscription
> is owned by a temporary user that has the same permissions as the
> table owner. These temporary users were originally superusers, because
> otherwise we cannot make them subscription owners, but once assigning
> a subscription to them we take away the superuser permissions from
> them[1]. And we also need to hook into ALTER/DELETE subscription
> commands to make sure that these temporary owners cannot edit their
> own subscription[2].
>
> Getting this right was not easy. And even it has the serious downside
> that we need multiple subscriptions/replication slots which causes
> extra complexity in various ways and it eats much more aggressively
> into the replication slot limits than we'd like. Having one
> subscription that could apply into tables that were owned by multiple
> users in a safe way would make this sooo much easier.

Yeah. As Andres pointed out somewhere or other, that also means you're
decoding the WAL once per user instead of just once. I'm surprised
that hasn't been cost-prohibitive.

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

> Yeah. As Andres pointed out somewhere or other, that also means you're
> decoding the WAL once per user instead of just once. I'm surprised
> that hasn't been cost-prohibitive.

We'd definitely prefer to have one subscription and do the decoding
only once. But we haven't run into big perf issues with the current
setup so far. We use it for non-blocking copying of shards (regular PG
tables under the hood). Most of the time is usually spent in the
initial copy phase, not the catchup. And also in practice our users
often only have one table owning user (and more than 5 table owning
users is extremely rare).

On Fri, 2023-03-03 at 11:02 -0500, Robert Haas wrote:
> Hi,
>
> Here's a patch against master for $SUBJECT. It lacks documentation
> changes and might have bugs, so please review if you're concerned
> about this issue.

I think the subject has a typo, you mean "as the table owner", right?

> However, I'm unsatisfied with just
> documenting the hazard, because I feel like almost everyone who uses
> logical replication wants to do the exact thing that this
> documentation says you shouldn't do.

+1

> The proposed fix is to perform logical replication actions (SELECT,
> INSERT, UPDATE, DELETE, and TRUNCATE) as the user who owns the table
> rather than as the owner of the subscription. The session still runs
> as the subscription owner, but the active user ID is switched to the
> table owner for the duration of each operation. To prevent table
> owners from doing tricky things to attack the subscription owner, we
> impose SECURITY_RESTRICTED_OPERATION while running as the table
> owner.

+1

> To avoid inconveniencing users when this restriction adds no
> meaningful security, we refrain from imposing
> SECURITY_RESTRICTED_OPERATION when the table owner can SET ROLE to
> the
> subscription owner anyway.

That's a little confusing, why not just always use the
SECURITY_RESTRICTED_OPERATION? Is there a use case I'm missing?

> There is also a possibility of an attack in the other direction.
> Maybe
> the subscription owner would like to obtain the table owner's
> permissions, or at the very least, use logical replication as a
> vehicle to perform operations they can't perform directly. A
> malicious
> subscription owner could hook up logical replication to a table into
> which the table owner doesn't want replication to occur. To block
> such
> attacks, the patch requires that the subscription owner have the
> ability to SET ROLE to each table owner.

It would be really nice if this could be done with some kind of
ordinary privilege rather than requiring SET ROLE. A user might expect
that INSERT/UPDATE/DELETE/TRUNCATE privileges are enough. Or
pg_write_all_data.

I can see theoretically that a table owner might write some dangerous
code and attach it to their table. But I don't quite understand why
they would do that. If the code was vulnerable to attack, would that
mean that they couldn't even insert into their own table safely?

Requiring SET ROLE seems like it makes the subscription role into
something very close to a superuser. And that takes away some of the
benefits of delegating to non-superusers.

>  If the subscription owner is
> a superuser, which is usual, this will be automatic. Otherwise, the
> superuser will need to grant to the subscription owner the roles that
> own relevant tables. This can usefully serve as a kind of access
> control to make sure that the subscription doesn't touch any tables
> other than the ones it's supposed to be touching: just make those
> tables owned by a different user and don't grant them to the
> subscription owner. Previously, we provided no way at all of
> controlling the tables that replication can target.

We check for ordinary access privileges, which I think is your next
point, so the above paragraph confuses me a bit.

> This fix interacts in an interesting way with Mark Dilger's work,
> committed by Jeff Davis, to make logical replication respect table
> permissions. I initially thought that with this change, that commit
> would end up just being reverted, with the permissions scheme
> described above replacing the existing one. However, I then realized
> that it's still good to perform those checks. Normally, a table owner
> can do any DML operation on a table they own, so those checks will
> never fail. However, if a table owner has revoked their ability to,
> say, INSERT into one of their own tables, then logical replication
> shouldn't bypass that and perform the INSERT anyway. So I now think
> that the checks added by that commit complement the ones added by
> this
> proposed patch, rather than competing with them.

That's an interesting case.

> It is unclear to me whether we should try to back-port this. It's
> definitely a behavior change, and changing the behavior in the back
> branches is not a nice thing to do. On the other hand, at least in my
> opinion, the security consequences of the status quo are pretty dire.
> I tend to suspect that a really high percentage of people who are
> using logical replication at all are vulnerable to this, and lots of
> people having a way to escalate to superuser isn't good.

It's worth considering given that most subscription owners are
superusers anyway. What plausible cases might it break?

Regards,
    Jeff Davis

On Fri, Mar 24, 2023 at 3:59 AM Jeff Davis <pgsql@j-davis.com> wrote:
> That's a little confusing, why not just always use the
> SECURITY_RESTRICTED_OPERATION? Is there a use case I'm missing?

Some concern was expressed -- not sure exactly where the email is
exactly, and it might've been on pgsql-security -- that doing that
categorically might break things that are currently working. The
people who were concerned included Andres and I forget who else. My
gut reaction was the same as yours, just do it always and don't worry
about it. But if people think that users are likely to run afoul of
the SECURITY_RESTRICTED_OPERATION restrictions in practice, then this
is better, and the implementation complexity isn't high. We could even
think of extending this kind of logic to other places where
SECURITY_RESTRICTED_OPERATION is enforced, if desired.

> It would be really nice if this could be done with some kind of
> ordinary privilege rather than requiring SET ROLE. A user might expect
> that INSERT/UPDATE/DELETE/TRUNCATE privileges are enough. Or
> pg_write_all_data.
>
> I can see theoretically that a table owner might write some dangerous
> code and attach it to their table. But I don't quite understand why
> they would do that. If the code was vulnerable to attack, would that
> mean that they couldn't even insert into their own table safely?
>
> Requiring SET ROLE seems like it makes the subscription role into
> something very close to a superuser. And that takes away some of the
> benefits of delegating to non-superusers.

I am not thrilled with this either, but if you can arrange to run code
as a certain user without the ability to SET ROLE to that user then
there is, by definition, a potential privilege escalation. I don't
think we can just dismiss that as a non-issue. If the ability of one
user to potentially become some other user weren't a problem, we
wouldn't need any patch at all. Imagine for example that the table
owner has a trigger which doesn't sanitize search_path. The
subscription owner can potentially leverage that to get the table
owner's privileges.

More generally, Stephen Frost has elsewhere argued that we should want
the subscription owner to be a very low-privilege user, so that if
their privileges get stolen, it's no big deal. I disagree with that. I
think it's always a problem if one user can get unauthorized access to
another user's account, regardless of exactly what those accounts can
do. I think our goal should be to make it safe for the subscription
owner to be a very high-privilege user, because you're going to need
to be a very high-privilege user to set up replication. And if you do
have that level of privilege, it's more convenient and simpler if you
can just own the subscription yourself, rather than having to make a
dummy account to own it. To put that another way, I think that what
people are going to want to do in a lot of cases is have the superuser
own the subscription, so I think we need to make that case safe,
whatever it takes. In cases where the subscription owner isn't the
superuser, I think the next most likely possibility is that the
subscription owner is some kind of almost-super-user, like a
CREATEROLE user or someone running with rds_superuser or similar on
some PG fork. So that needs to be safe too, and I think this does
that. Having the subscription owner be some random user that doesn't
have a lot of privileges doesn't seem particularly useful to me. If it
were unproblematic to allow that, sure, but considering how easy it
would be for that low-privilege user to steal table owner privileges,
I don't think it makes sense.

> > It is unclear to me whether we should try to back-port this. It's
> > definitely a behavior change, and changing the behavior in the back
> > branches is not a nice thing to do. On the other hand, at least in my
> > opinion, the security consequences of the status quo are pretty dire.
> > I tend to suspect that a really high percentage of people who are
> > using logical replication at all are vulnerable to this, and lots of
> > people having a way to escalate to superuser isn't good.
>
> It's worth considering given that most subscription owners are
> superusers anyway. What plausible cases might it break?

AFAIU, the main concern is about the SECURITY_RESTRICTED_OPERATION
flag interacting badly with things people are already doing. Other
problems seem possible, e.g. if you're doing something that gets the
current user name and does something with it, the answer's going to
change, and you might like the new answer more or less than the old
one. It's a little hard to predict who will be inconvenienced in what
ways when you change behavior, but problems are certainly possible.

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

> Some concern was expressed -- not sure exactly where the email is
> exactly, and it might've been on pgsql-security -- that doing that
> categorically might break things that are currently working. The
> people who were concerned included Andres and I forget who else. My
> gut reaction was the same as yours, just do it always and don't worry
> about it. But if people think that users are likely to run afoul of
> the SECURITY_RESTRICTED_OPERATION restrictions in practice, then this
> is better, and the implementation complexity isn't high. We could even
> think of extending this kind of logic to other places where
> SECURITY_RESTRICTED_OPERATION is enforced, if desired.

I personally cannot think of a reasonable example that would be
broken, but I agree the code is simple enough. I do think that if
there is an actual reason to do this, we'd probably want it in other
places where SECURITY_RESTRICTED_OPERATION is enforced too.

I think there's some important tests missing related to this:
1. Ensuring that SECURITY_RESTRICTED_OPERATION things are enforced
when the user **does not** have SET ROLE permissions to the
subscription owner, e.g. don't allow SET ROLE from a trigger.
2. Ensuring that SECURITY_RESTRICTED_OPERATION things are not enforced
when the user **does** have SET ROLE permissions to the subscription
owner, e.g. allows SET ROLE from trigger.


Finally a small typo in the one of the comments:

> + * If we created a new GUC nest level, also role back any changes that were
s/role/roll/

On Fri, Mar 24, 2023 at 12:17 PM Jelte Fennema <postgres@jeltef.nl> wrote:
> I personally cannot think of a reasonable example that would be
> broken, but I agree the code is simple enough. I do think that if
> there is an actual reason to do this, we'd probably want it in other
> places where SECURITY_RESTRICTED_OPERATION is enforced too.

I don't think it makes sense for this patch to run around and try to
adjust all of those other pages. We have to pick between doing it this
way (and thus being inconsistent with what we do elsewhere) or taking
that logic out (and taking our chances that something will break for
some users). I'm OK with either of those, but I'm not OK with going
and changing the way this works in all of the other cases first and
only then coming back to this problem. This problem is WAY more
important than fiddling with the details of how
SECURITY_RESTRICTED_OPERATION is applied.

> I think there's some important tests missing related to this:
> 1. Ensuring that SECURITY_RESTRICTED_OPERATION things are enforced
> when the user **does not** have SET ROLE permissions to the
> subscription owner, e.g. don't allow SET ROLE from a trigger.
> 2. Ensuring that SECURITY_RESTRICTED_OPERATION things are not enforced
> when the user **does** have SET ROLE permissions to the subscription
> owner, e.g. allows SET ROLE from trigger.

Yeah, if we stick with the current approach we should probably add
tests for that stuff.

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

> On Mar 24, 2023, at 7:00 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> More generally, Stephen Frost has elsewhere argued that we should want
> the subscription owner to be a very low-privilege user, so that if
> their privileges get stolen, it's no big deal. I disagree with that. I
> think it's always a problem if one user can get unauthorized access to
> another user's account, regardless of exactly what those accounts can
> do. I think our goal should be to make it safe for the subscription
> owner to be a very high-privilege user, because you're going to need
> to be a very high-privilege user to set up replication. And if you do
> have that level of privilege, it's more convenient and simpler if you
> can just own the subscription yourself, rather than having to make a
> dummy account to own it. To put that another way, I think that what
> people are going to want to do in a lot of cases is have the superuser
> own the subscription, so I think we need to make that case safe,
> whatever it takes.

I also think the subscription owner should be a low-privileged user, owing to the risk of the publisher injecting
maliciouscontent into the publication.  I think you are focused on all the bad actors on the subscription-side database
andwhat they can do to each other.  That's also valid, but I get the impression that you're losing sight of the risk
posedby malicious publishers.  Or maybe you aren't, and can explain? 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Mar 24, 2023 at 12:58 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> I also think the subscription owner should be a low-privileged user, owing to the risk of the publisher injecting
maliciouscontent into the publication.  I think you are focused on all the bad actors on the subscription-side database
andwhat they can do to each other.  That's also valid, but I get the impression that you're losing sight of the risk
posedby malicious publishers.  Or maybe you aren't, and can explain? 

You have a point.

As things stand today, if you think somebody's going to send you
changes to tables other than the ones you intend to replicate, you
could handle that by making sure that the user that owns the
subscription only has permission to write to the tables that are
expected to receive replicated data. It's a bit awkward to set up
because you have to initially make the subscription owner a superuser
and then later remove the superuser bit, so I think this is another
argument for the pg_create_subscription patch posted elsewhere, but if
you're a superuser yourself, you can do it. However, with this patch,
that wouldn't work any more, because the permissions checks don't
happen until after we've switched to the target role. You could
alternatively set up a user to own the subscription who has the
ability to SET ROLE to some users and not others, but that only lets
you restrict replication based on which user owns the tables, rather
than which specific tables are getting data replicated into them. That
actually wouldn't work today, and with the patch it would start
working, so basically the effect of the patch on the problem that you
mention would be to remove the ability to filter by specific table and
add the ability to filter by owning role.

I don't know how bad that sounds to you, and if it does sound bad, I
don't immediately see how to mitigate it. As I said to Jeff, if you
can replicate into a table that has a casually-written SECURITY
INVOKER trigger on it, you can probably hack into the table owner's
account. So I think that if we allow user A to replicate into user B's
table with fewer privileges than A-can-set-role-to-B, we're building a
privilege-escalation attack into the system. But if we do require
A-can-set-role-to-B, then things change as described above.

I suppose in theory we could check both A-can-set-role-to-B and
A-can-modify-this-table-as-A, but that feels pretty unprincipled. I
can't particularly see why A should need permission to perform an
action that is actually going to be performed as B; what makes sense
is to check that A can become B, and that B has permission to perform
the action, which is the state that this patch would create. I guess
there are other things we could do, too, like add replication
restriction or filtering capabilities specifically to address this
problem, or devise some other kind of new kind of permission system
around this, but I don't have a specific idea what that would look
like.

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

On Fri, Mar 24, 2023 at 2:14 PM Jelte Fennema <postgres@jeltef.nl> wrote:
> Yes, I totally agree. I now realise that wasn't clear at all from the wording in my previous email. I'm fine with
bothbehaviours. I mainly meant that if we actually think the new behaviour is better (which honestly I'm not convinced
ofyet), then some follow up patch would probably be good. I definitely don't want to block this patch on any of that
though.Both behaviors would be vastly better than the current one in my opinion. So if others wanted the behaviour in
yourpatch, I'm completely fine with that. 

Makes sense. I hope a few more people will comment on what they think
we should do here, especially Andres and Noah.

> > Yeah, if we stick with the current approach we should probably add
> > tests for that stuff.
>
> Even if we don't, we should still have tests showing that the security restrictions that we intend to put in place
actuallydo their job. 

Yeah, I just don't want to write the tests and then decide to change
the behavior and then have to write them over again. It's not so much
fun that I'm yearning to do it twice.

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

> On Mar 24, 2023, at 11:35 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> I don't know how bad that sounds to you, and if it does sound bad, I
> don't immediately see how to mitigate it. As I said to Jeff, if you
> can replicate into a table that has a casually-written SECURITY
> INVOKER trigger on it, you can probably hack into the table owner's
> account.

I assume you mean this bit:

> > Imagine for example that the table
> > owner has a trigger which doesn't sanitize search_path. The
> > subscription owner can potentially leverage that to get the table
> > owner's privileges.


I don't find that terribly convincing.  First, there's no reason a subscription owner should be an ordinary user able
tovolitionally do anything.  The subscription owner should just be a role that the subscription runs under, as a means
ofsuperuser dropping privileges before applying changes.  So the only real problem would be that the changes coming
fromthe publisher might, upon application, hack the table owner.  But if that's the case, the table owner's
vulnerabilityon the subscription-database side is equal to their vulnerability on the publication-database side
(assumingequal schemas on both).  Flagging this vulnerability as being logical replication related seems a category
error. Instead, it's a schema vulnerability. 

> So I think that if we allow user A to replicate into user B's
> table with fewer privileges than A-can-set-role-to-B, we're building a
> privilege-escalation attack into the system. But if we do require
> A-can-set-role-to-B, then things change as described above.

I don't understand the direction this patch is going.  I'm emphatically not objecting to it, merely expressing my
confusionabout it. 

I had imagined the solution to the replication security problem was to stop running the replication as superuser, and
insteadas a trivial user.  Imagine that superuser creates roles "deadhead_bob" and "deadhead_alice" which cannot log
in,are not members of any groups nor have any other roles as members of themselves, and have no privileges beyond begin
ableto replicate into bob's and alice's tables, respectively.  The superuser sets up the subscriptions disabled,
transfersownership to deadhead_bob and deadhead_alice, and only then enables the subscriptions. 

Since deadhead_bob and deadhead_alice cannot log in, and nobody can set role to them, I don't see what the
vulnerabilityis.  Sure, maybe alice can attack deadhead_alice, or bob can attack deadhead_bob, but that's more of a
privilegedeescalation than a privilege escalation, so where's the risk?  That's not a rhetorical question.  Is there a
riskhere?  Or are we just concerned that most users will set up replication with superuser or some other high-privilege
user,and we're trying to protect them from the consequences of that choice? 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, 2023-03-24 at 10:00 -0400, Robert Haas wrote:
> On Fri, Mar 24, 2023 at 3:59 AM Jeff Davis <pgsql@j-davis.com> wrote:
> > That's a little confusing, why not just always use the
> > SECURITY_RESTRICTED_OPERATION? Is there a use case I'm missing?
>
> Some concern was expressed -- not sure exactly where the email is
> exactly, and it might've been on pgsql-security -- that doing that
> categorically might break things that are currently working. The
> people who were concerned included Andres and I forget who else. My
> gut reaction was the same as yours, just do it always and don't worry
> about it. But if people think that users are likely to run afoul of
> the SECURITY_RESTRICTED_OPERATION restrictions in practice, then this
> is better, and the implementation complexity isn't high. We could
> even
> think of extending this kind of logic to other places where
> SECURITY_RESTRICTED_OPERATION is enforced, if desired.

Without a reasonable example, we should probably be on some kind of
path to disallowing crazy stuff in triggers that poses only risks and
no benefits. Not the job of this patch, but perhaps it can be seen as a
step in that direction?

>
> I am not thrilled with this either, but if you can arrange to run
> code
> as a certain user without the ability to SET ROLE to that user then
> there is, by definition, a potential privilege escalation.

I don't think that's "by definition".

The code is being executed with the privileges of the person who wrote
it. I don't see anything inherently insecure about that. There could be
incidental or practical risks, but it's a pretty reasonable thing to do
at a high level.

>  I don't
> think we can just dismiss that as a non-issue. If the ability of one
> user to potentially become some other user weren't a problem, we
> wouldn't need any patch at all. Imagine for example that the table
> owner has a trigger which doesn't sanitize search_path. The
> subscription owner can potentially leverage that to get the table
> owner's privileges.

Can you explain? Couldn't we control the subscription process's search
path?

> In cases where the subscription owner isn't the
> superuser, I think the next most likely possibility is that the
> subscription owner is some kind of almost-super-user

The benefit of delegating to a non-superuser is to contain the risk if
that account is compromised. Allowing SET ROLE on tons of accounts
diminishes that benefit, a lot.

Regards,
    Jeff Davis

On Fri, 2023-03-24 at 14:35 -0400, Robert Haas wrote:
> That
> actually wouldn't work today, and with the patch it would start
> working, so basically the effect of the patch on the problem that you
> mention would be to remove the ability to filter by specific table
> and
> add the ability to filter by owning role.

That's certainly a loss. It makes a lot of sense to want to limit a
subscription to only write to certain tables.

If you want to filter by role, you can do that today by granting the
role, or some role that has the necessary privileges.

It takes me a while to re-learn the problems of poorly-written trigger
functions, malicious trigger functions, search paths, etc., each time I
start working in this area again. Can you include an example of such a
trigger function that we're worried about? Can the subscription owner
change the search path in the subscription process, and if so, why?

The doc here:

https://www.postgresql.org/docs/devel/sql-createfunction.html#SQL-CREATEFUNCTION-SECURITY

mentions search_path, but other hazards don't really seem applicable.
(Is the trigger creating new roles?)

Regards,
    Jeff Davis

> As things stand today, if you think somebody's going to send you
> changes to tables other than the ones you intend to replicate, you
> could handle that by making sure that the user that owns the
> subscription only has permission to write to the tables that are
> expected to receive replicated data. It's a bit awkward to set up
> because you have to initially make the subscription owner a superuser
> and then later remove the superuser bit, so I think this is another
> argument for the pg_create_subscription patch posted elsewhere, but if
> you're a superuser yourself, you can do it. However, with this patch,
> that wouldn't work any more, because the permissions checks don't
> happen until after we've switched to the target role.

For my purposes I always trust the publisher, what I don't trust is
the table owners. But I can indeed imagine scenarios where that's the
other way around, and indeed you can protect against that currently,
but not with your new patch. That seems fairly easily solvable though.

+   if (!member_can_set_role(context->save_userid, userid))
+       ereport(ERROR,
+               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                errmsg("role \"%s\" cannot SET ROLE to \"%s\"",
+                       GetUserNameFromId(context->save_userid, false),
+                       GetUserNameFromId(userid, false))));

If we don't throw an error here, but instead simply return, then the
current behaviour is preserved and people can manually configure
permissions to protect against an untrusted publisher. This would
still mean that the table owners can escalate privileges to the
subscription owner, but if that subscription owner actually has fewer
privileges than the table owner then you don't have that issue.

On Fri, Mar 24, 2023 at 10:00:36AM -0400, Robert Haas wrote:
> On Fri, Mar 24, 2023 at 3:59 AM Jeff Davis <pgsql@j-davis.com> wrote:
> > That's a little confusing, why not just always use the
> > SECURITY_RESTRICTED_OPERATION? Is there a use case I'm missing?
> 
> Some concern was expressed -- not sure exactly where the email is
> exactly, and it might've been on pgsql-security -- that doing that
> categorically might break things that are currently working. The
> people who were concerned included Andres and I forget who else. My

SECURITY_RESTRICTED_OPERATION blocks deferred triggers and CREATE TEMP TABLE.
If you create a DEFERRABLE INITIALLY DEFERRED fk constraint and replicate to
the constraint's table in a SECURITY_RESTRICTED_OPERATION, I expect an error.

> gut reaction was the same as yours, just do it always and don't worry
> about it. But if people think that users are likely to run afoul of

Hard to know.  It's the sort of thing that I model as creating months of work
for epsilon users to adapt their applications.  Epsilon could be zero.  Most
users don't notice.

> the SECURITY_RESTRICTED_OPERATION restrictions in practice, then this
> is better, and the implementation complexity isn't high. We could even
> think of extending this kind of logic to other places where
> SECURITY_RESTRICTED_OPERATION is enforced, if desired.

Firing a trigger in an index expression or materialized view query is not
reasonable, so today's uses of SECURITY_RESTRICTED_OPERATION would not benefit
from this approach.  Being able to create temp tables in those places has some
value, but I would allow temp tables in SECURITY_RESTRICTED_OPERATION instead
of proliferating the check on the ability to SET ROLE.  (One might allow temp
tables by introducing NewTempSchemaNestLevel(), called whenever we call
NewGUCNestLevel().  The transaction would then proceed as though it has no
temp schema, allocating an additional schema if creating a temp object.)

On Fri, Mar 24, 2023 at 4:11 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> > > Imagine for example that the table
> > > owner has a trigger which doesn't sanitize search_path. The
> > > subscription owner can potentially leverage that to get the table
> > > owner's privileges.
>
> I don't find that terribly convincing.  First, there's no reason a subscription owner should be an ordinary user able
tovolitionally do anything.  The subscription owner should just be a role that the subscription runs under, as a means
ofsuperuser dropping privileges before applying changes.  So the only real problem would be that the changes coming
fromthe publisher might, upon application, hack the table owner.  But if that's the case, the table owner's
vulnerabilityon the subscription-database side is equal to their vulnerability on the publication-database side
(assumingequal schemas on both).  Flagging this vulnerability as being logical replication related seems a category
error. Instead, it's a schema vulnerability. 

I think there are actually a number of reasons why the subscription
owner should be a regular user account rather than a special
low-privilege account. First, it's only barely possible to do anything
else. As of today, you can't create a subscription owned by a
non-superuser except by making the subscription first and then
removing superuser from the account. You can't even do this:

rhaas=# alter subscription s1 owner to alice;
ERROR:  permission denied to change owner of subscription "s1"
HINT:  The owner of a subscription must be a superuser.

That hint is actually a lie because of the loophole mentioned above,
but even if making the subscription owner a low-privilege account were
the right model (which I don't believe) we've got error messages in
the current source code saying that you're not allowed to even do it.

Second, even if this kind of setup were fully supported and all the
stuff worked as you expect, it's not very convenient. It requires you
to create this extra dummy account that doesn't otherwise need to
exist. I don't see a good reason to impose that requirement on
everybody. Given that subscriptions initially could only be owned by
superusers, and that's still mostly true, it seems to me that the
feature was intended to be used with the superuser as the subscription
owner, and I think that's what most people must be doing now and will
probably want to continue to do, and we should try to make it safe
instead of back-pedaling and saying, hey, do it this totally other way
instead.

More discussion of problems below.

> > So I think that if we allow user A to replicate into user B's
> > table with fewer privileges than A-can-set-role-to-B, we're building a
> > privilege-escalation attack into the system. But if we do require
> > A-can-set-role-to-B, then things change as described above.
>
> I don't understand the direction this patch is going.  I'm emphatically not objecting to it, merely expressing my
confusionabout it. 
>
> I had imagined the solution to the replication security problem was to stop running the replication as superuser, and
insteadas a trivial user.  Imagine that superuser creates roles "deadhead_bob" and "deadhead_alice" which cannot log
in,are not members of any groups nor have any other roles as members of themselves, and have no privileges beyond begin
ableto replicate into bob's and alice's tables, respectively.  The superuser sets up the subscriptions disabled,
transfersownership to deadhead_bob and deadhead_alice, and only then enables the subscriptions. 
>
> Since deadhead_bob and deadhead_alice cannot log in, and nobody can set role to them, I don't see what the
vulnerabilityis.  Sure, maybe alice can attack deadhead_alice, or bob can attack deadhead_bob, but that's more of a
privilegedeescalation than a privilege escalation, so where's the risk?  That's not a rhetorical question.  Is there a
riskhere?  Or are we just concerned that most users will set up replication with superuser or some other high-privilege
user,and we're trying to protect them from the consequences of that choice? 

Having a separate subscription for each different table owner is
extremely undesirable from a performance perspective, because it means
that the WAL on the origin server has to be decoded once per table
owner. That's not very much fun even if the number of table owners is
only two, as in your example, and there could be many more than two
users who own tables. In addition, it breaks the transactional
consistency of replication. If there's any single transaction that
modifies both a table owned by alice and a table owned by bob, we lose
transactional consistency on the subscriber side unless both of those
transactions are replicated in a single transaction, which means they
also need to be part of a single subscription.

I admit that hacking into deadhead_alice or deadhead_bob is probably
only minimally interesting. There are, perhaps, things you could do
with that, like creating objects owned by that user, and maybe your
own account is subject to some kind of restrictions separate from
what's in place on the deadhead account, but most likely in most
circumstances you can't get very far by breaking into a deadhead
account whose only purpose is to replicate changes on tables you
already own. However, because of the problems with having a
subscription per table owner, you're probably really going to need a
shared deadhead account that is used to replicate across all users who
own tables, and now breaking into that account gets a lot more
interesting. Two users that aren't supposed to be able to talk to each
other could use the shared deadhead account to pass data back and
forth, e.g. to exfiltrate data from a top secret account to one with a
lower security classification. Or maybe a malicious user can try to
steal data as it's being replicated, or just mess with the system.
They can create objects owned by the deadhead account in any
publicly-writable schemas, and they can mess with the GUC settings
that apply to the deadhead account, at a minimum.

I basically don't think there's such a thing as an account that is so
low-privilege that we don't care about someone hacking into it. Now,
you can go way down the rabbit hole here and say "well, what if we
invented a SUPER low privilege account that ever create or own objects
and can't use even those privileges that are granted to public and
<insert other restrictions here>." And I admit that could be done, but
that sounds like a lot of work and I see no point. If we just make it
safe for superusers to own subscriptions, then the job's done. And I
see nothing preventing us from doing that. That's the point of this
patch, in fact.

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

On Fri, Mar 24, 2023 at 8:02 PM Jeff Davis <pgsql@j-davis.com> wrote:
> Without a reasonable example, we should probably be on some kind of
> path to disallowing crazy stuff in triggers that poses only risks and
> no benefits. Not the job of this patch, but perhaps it can be seen as a
> step in that direction?

Possibly, but it's a little harder to say what's crazy in a trigger
than in some other contexts. I feel like it would be fine to say that
your index expression should probably not be doing "ALTER USER
somebody SUPERUSER" really ever, but it's not quite so clear in case
of a trigger. And the stuff that's prevented by
SECURITY_RESTRICTED_OPERATION isn't that sort of thing, but has rather
to do with stuff that messes with the session state, maybe leaving
booby-traps behind for the next user. For instance, if user A runs
some code as user B and user B closes a cursor opened by A and opens a
new one with the same name, that has a rather good chance of making A
do something they didn't intend to do. SECURITY_RESTRICTED_OPERATION
is aimed at preventing that sort of attack.

> > I am not thrilled with this either, but if you can arrange to run
> > code
> > as a certain user without the ability to SET ROLE to that user then
> > there is, by definition, a potential privilege escalation.
>
> I don't think that's "by definition".
>
> The code is being executed with the privileges of the person who wrote
> it. I don't see anything inherently insecure about that. There could be
> incidental or practical risks, but it's a pretty reasonable thing to do
> at a high level.

Not really. My home directory on my laptop is full of Perl and sh
scripts that I wouldn't want someone else to execute as me. They don't
have any defenses against malicious use because I don't expect anyone
else has access to run them, especially as me. If someone got access
to run them as me, they'd compromise my laptop account in no time at
all. I don't see any reason to believe the situation is any different
inside of a database. People have no reason to harden code unless
someone else is going to have access to run it.

> >  I don't
> > think we can just dismiss that as a non-issue. If the ability of one
> > user to potentially become some other user weren't a problem, we
> > wouldn't need any patch at all. Imagine for example that the table
> > owner has a trigger which doesn't sanitize search_path. The
> > subscription owner can potentially leverage that to get the table
> > owner's privileges.
>
> Can you explain? Couldn't we control the subscription process's search
> path?

There's no place in the code right now where when we switch user
identities we also change the search_path. There is nothing to prevent
us from writing such code, but we have no place from which to obtain a
search_path that will cause the called code to behave properly. We
don't have access to the search_path that would prevail at the time
the target user logged in, and even if we did, we don't know that that
search_path is secure. We do know that an empty search_path is secure,
but it's probably also going to cause any code we run to fail, unless
that code schema-qualifies all references outside of pg_catalog, or
unless it sets search_path itself. search_path also isn't necessarily
the only problem. As a hypothetical example, suppose I create a table
with one text column, revoke all public access to that table, and then
create an on-insert trigger that executes as an SQL command any text
value inserted into the table. This is perfectly secure as long as I'm
the only one who can access the table, but if someone else gets access
to insert things into that table using my credentials then they can
easily break into my account. Real examples aren't necessarily that
dramatically bad, but that doesn't mean they don't exist.

> The benefit of delegating to a non-superuser is to contain the risk if
> that account is compromised. Allowing SET ROLE on tons of accounts
> diminishes that benefit, a lot.

Well, I continue to feel that if you can compromise the subscription
owner's account, we haven't really fixed anything yet. I mean, it used
to be that autovacuum could compromise the superuser's account, and we
fixed that. If we find more ways for that same thing to happen, we
would presumably fix those too. We would never accept a situation
where autovacuum can compromise the superuser's account. And we
shouldn't accept a situation where either the table owner can
compromise the subscription owner's account, either. And similarly
nobody ever proposed that that issue should be fixed by running the
autovacuum worker process as some kind of low-privileged user that we
created specially for that purpose. We just ... fixed it so that no
compromise was possible. And I think that's also the right approach
here.

I do agree with you that allowing SET ROLE on tons of accounts is not
great, though. I don't really think it matters very much today,
because basically all subscriptions today are owned by superusers and
can do everything anyway. But if you imagine that a lot of
subscriptions are going to be owned by less-privileged users, then
it's a lot less nice. I think it has the strength of at least being
honest about what the problem is. It makes it clear right on the tin
that the subscription owner is going to be able to get into the table
owner's accounts, in a way that people shouldn't really miss noticing.
And if they notice it, then they're at least aware of it, which is
something. It would be better still if we could prevent the
subscription owner from hacking into the table owner's account. Then,
we could very sensibly remove the SET ROLE requirement and check
something weaker instead, and that would be fantastic.

Since I didn't see how to engineer that, I did this.

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

On Sat, Mar 25, 2023 at 12:01 PM Noah Misch <noah@leadboat.com> wrote:
> (One might allow temp
> tables by introducing NewTempSchemaNestLevel(), called whenever we call
> NewGUCNestLevel().  The transaction would then proceed as though it has no
> temp schema, allocating an additional schema if creating a temp object.)

Neat idea. That would require some adjustments, I believe, because of
the way that temp schemas are named. But it sounds doable.

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

On Sat, Mar 25, 2023 at 5:24 AM Jelte Fennema <postgres@jeltef.nl> wrote:
> For my purposes I always trust the publisher, what I don't trust is
> the table owners. But I can indeed imagine scenarios where that's the
> other way around, and indeed you can protect against that currently,
> but not with your new patch. That seems fairly easily solvable though.
>
> +   if (!member_can_set_role(context->save_userid, userid))
> +       ereport(ERROR,
> +               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> +                errmsg("role \"%s\" cannot SET ROLE to \"%s\"",
> +                       GetUserNameFromId(context->save_userid, false),
> +                       GetUserNameFromId(userid, false))));
>
> If we don't throw an error here, but instead simply return, then the
> current behaviour is preserved and people can manually configure
> permissions to protect against an untrusted publisher. This would
> still mean that the table owners can escalate privileges to the
> subscription owner, but if that subscription owner actually has fewer
> privileges than the table owner then you don't have that issue.

I don't get it. If we just return, that would result in skipping
changes rather than erroring out on changes, but it wouldn't preserve
the current behavior, because we'd still care about the table owner's
permissions rather than, as now, the subscription owner's permissions.

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

On Mon, 2023-03-27 at 12:53 -0400, Robert Haas wrote:
> We do know that an empty search_path is secure,
> but it's probably also going to cause any code we run to fail, unless
> that code schema-qualifies all references outside of pg_catalog, or
> unless it sets search_path itself.

I am confused.

If the trigger function requires schema "xyz" to be in the search_path,
and the function itself doesn't set it, how will it ever get set in the
subscription process? Won't such a function be simply broken for all
logical subscriptions (in current code or with any of the proposals
active right now)?

And if the trigger function requires object "abc" (regardless of
schema) to be somehow accessible without qualification, and if it
doesn't set the search path itself, and it's not in pg_catalog; then
again I think that function would be broken today.

It feels like we are reaching to say that a trigger function might be
broken based on some contrived cases, but we can already contrive cases
that are broken for logical replication today. It might not be exactly
the same set, but unless I'm missing something it would be a very
similar set.

>  search_path also isn't necessarily
> the only problem. As a hypothetical example, suppose I create a table
> with one text column, revoke all public access to that table, and
> then
> create an on-insert trigger that executes as an SQL command any text
> value inserted into the table. This is perfectly secure as long as
> I'm
> the only one who can access the table, but if someone else gets
> access
> to insert things into that table using my credentials then they can
> easily break into my account. Real examples aren't necessarily that
> dramatically bad, but that doesn't mean they don't exist.

Are you suggesting that we require the subscription owner to have SET
ROLE on everybody so that everyone is equally insecure and there's no
way to ever fix it even if you don't have any triggers on your tables
at all?

And all of this is to protect a user that writes a trigger function
that executes arbitrary SQL based on the input data?

>
> Well, I continue to feel that if you can compromise the subscription
> owner's account, we haven't really fixed anything yet.

I'm trying to reconcile the following two points:

  A. That you are proposing a patch to allow non-superuser
subscriptions; and
  B. That you are arguing that the subscription owner basically needs
to be superuser and we should just accept that.

Granted, there's some nuance here and I won't say that those two are
mutually exclusive. But I think it would be helpful to explain how
those two ideas fit together.

> We just ... fixed it so that no
> compromise was possible. And I think that's also the right approach
> here.

If you are saying that we fix the subscription process so that it can't
easily be compromised by the table owner, then that appears to be the
entire purpose of this patch and I agree.

But I think you're saying something slightly different about the
subscription process compromising the table owner, and assuming that
problem exists, your patch doesn't do anything to stop it. In fact,
your patch requires the subscription owner can SET ROLE to each of the
table owners, guaranteeing that the table owners can never do anything
at all to protect themselves from the subscription owner.

> I do agree with you that allowing SET ROLE on tons of accounts is not
> great, though.

Regardless of security concerns, the UI is bad.


Regards,
    Jeff Davis

On Mon, Mar 27, 2023 at 3:04 PM Jeff Davis <pgsql@j-davis.com> wrote:
> On Mon, 2023-03-27 at 12:53 -0400, Robert Haas wrote:
> > We do know that an empty search_path is secure,
> > but it's probably also going to cause any code we run to fail, unless
> > that code schema-qualifies all references outside of pg_catalog, or
> > unless it sets search_path itself.
>
> I am confused.
>
> If the trigger function requires schema "xyz" to be in the search_path,
> and the function itself doesn't set it, how will it ever get set in the
> subscription process? Won't such a function be simply broken for all
> logical subscriptions (in current code or with any of the proposals
> active right now)?
>
> And if the trigger function requires object "abc" (regardless of
> schema) to be somehow accessible without qualification, and if it
> doesn't set the search path itself, and it's not in pg_catalog; then
> again I think that function would be broken today.

No, not really. It's pretty common for a lot of things to be in the
public schema, and the public schema is likely to be in the search
path of every user involved.

> It feels like we are reaching to say that a trigger function might be
> broken based on some contrived cases, but we can already contrive cases
> that are broken for logical replication today. It might not be exactly
> the same set, but unless I'm missing something it would be a very
> similar set.

No, it's not a contrived case, and it's not the same set of cases, not
even close. Running functions with a different search path than the
author intended is a really common cause of all kinds of things
breaking. See for example commit
582edc369cdbd348d68441fc50fa26a84afd0c1a, which certainly did break
things for some users.

> Are you suggesting that we require the subscription owner to have SET
> ROLE on everybody so that everyone is equally insecure and there's no
> way to ever fix it even if you don't have any triggers on your tables
> at all?

I certainly am not. I don't even know how to respond to this. I want
to make it secure, not insecure. But we don't gain any security by
pretending that certain exploits don't exist or aren't problems when
they do and are. Quite the contrary.

> And all of this is to protect a user that writes a trigger function
> that executes arbitrary SQL based on the input data?

Or is insecure in any other way, and there are quite a few ways. If
you don't think that this is a problem in reality, I really don't know
how to carry this conversation forward. The idea that the average
trigger function is safe if it can be unexpectedly called by someone
other than the table owner with arguments and GUC settings chosen by
the caller doesn't seem remotely correct to me. It matches no part of
my experience with user-defined functions, either written by me or by
EDB customers. Every database I've ever seen that used triggers at all
would be vulnerable to the subscription owner compromising the table
owner's account.

> > Well, I continue to feel that if you can compromise the subscription
> > owner's account, we haven't really fixed anything yet.
>
> I'm trying to reconcile the following two points:
>
>   A. That you are proposing a patch to allow non-superuser
> subscriptions; and
>   B. That you are arguing that the subscription owner basically needs
> to be superuser and we should just accept that.
>
> Granted, there's some nuance here and I won't say that those two are
> mutually exclusive. But I think it would be helpful to explain how
> those two ideas fit together.

I do think that what this patch does is tantamount to B, because you
can have SET ROLE to some users without having SET ROLE to all users.
That's a big part of the point of the CREATEROLE and
createrole_self_grant work.

> But I think you're saying something slightly different about the
> subscription process compromising the table owner, and assuming that
> problem exists, your patch doesn't do anything to stop it. In fact,
> your patch requires the subscription owner can SET ROLE to each of the
> table owners, guaranteeing that the table owners can never do anything
> at all to protect themselves from the subscription owner.

Yeah, that's true, and like I said, if there's a way to avoid that,
great. But wishing it were so is not that way.

Let's back up a minute here. Suppose someone were to request a new
command, ALTER TABLE <name> DO NOT LET THE SUPERUSER ACCESS THIS. We
would reject that proposal. The reason we would reject it is because
it wouldn't actually work as documented. We know that the superuser
has the power to access that account and reverse that command, either
by SET ROLE or by changing the account password or by changing
pg_hba.conf or by shelling out to the filesystem and doing whatever.
The feature purports to do something that it actually cannot do. No
one can defend themselves against the superuser. Not even another
superuser can defend themselves against a superuser. Pretending
otherwise would be confusing and have no security benefits.

Now let's think about this case. Can a table owner defend themselves
against a subscription owner who wants to usurp their privileges? If
they cannot, then I think that what the patch does is correct for the
same reason that I think we would correctly reject the hypothetical
command proposed above. If they can, then the patch has got the wrong
idea, perhaps. But what is the actual answer to the question? My
answer is that it's *often* impossible but not *always* impossible. If
the table owner has no executable code at all attached to the table --
not just triggers, but also expression indexes and default expressions
and so forth -- then they can. If they do have those things then in
theory they might be able to protect themselves, but in practice they
are unlikely to be careful enough. I judge that practically every
installation where table owners use triggers would be easily
compromised. Only the most security-conscious of table owners are
going to get this right.

So that's a grey area, at least IMHO. The patch could be revised in
some way, and the permissions requirements downgraded. However, if we
do that, I think we're going to have to document that although in
theory table owners can make themselves against subscription owners,
in practice they probably won't be. That approach has some advantages,
and I don't think it's insane. However, I am not convinced that it is
the best idea, either, and I had the impression based on
pgsql-security discussion that Andres and Noah thought this way was
better. I might have misinterpreted their position, and they might
have changed their minds, and they might have been wrong. But that's
how we got here.

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

> I don't get it. If we just return, that would result in skipping
> changes rather than erroring out on changes, but it wouldn't preserve
> the current behavior, because we'd still care about the table owner's
> permissions rather than, as now, the subscription owner's permissions.

Attached is an updated version of your patch with what I had in mind
(admittedly it needed one more line than "just" the return to make it
work). But as you can see all previous tests for a lowly privileged
subscription owner that **cannot** SET ROLE to the table owner
continue to work as they did before. While still downgrading to the
table owners role when the subscription owner **can** SET ROLE to the
table owner.

Obviously this needs some comments explaining what's going on and
probably some code refactoring and/or variable renaming, but I hope
it's clear what I meant now: For high privileged subscription owners,
we downgrade to the permissions of the table owner, but for low
privileged ones we care about permissions of the subscription owner
itself.

On Mon, 2023-03-27 at 16:47 -0400, Robert Haas wrote:
> No, not really. It's pretty common for a lot of things to be in the
> public schema, and the public schema is likely to be in the search
> path of every user involved.

Consider this trigger function which uses an unqualified reference to
pfunc(), therefore implicitly depending on the public schema:

  CREATE FUNCTION public.pfunc() RETURNS INT4 LANGUAGE plpgsql AS
    $$ BEGIN RETURN 42; END; $$;
  CREATE FUNCTION foo.tfunc() RETURNS TRIGGER LANGUAGE plpgsql AS
    $$ BEGIN
       NEW.i = pfunc();
       RETURN NEW;
       END; $$;
  CREATE TABLE foo.a(i INT);
  CREATE TRIGGER a_trigger BEFORE INSERT ON foo.a
    FOR EACH ROW EXECUTE PROCEDURE foo.tfunc();
  ALTER TABLE foo.a ENABLE ALWAYS TRIGGER a_trigger;

But that trigger function is broken today for logical replication --
pfunc() isn't found by the subscription process, which has an empty
search_path.

> Let's back up a minute here. Suppose someone were to request a new
> command, ALTER TABLE <name> DO NOT LET THE SUPERUSER ACCESS THIS. We
> would reject that proposal. The reason we would reject it is because
> it wouldn't actually work as documented. We know that the superuser
> has the power to access that account and reverse that command, either
> by SET ROLE or by changing the account password or by changing
> pg_hba.conf or by shelling out to the filesystem and doing whatever.
> The feature purports to do something that it actually cannot do. No
> one can defend themselves against the superuser. Not even another
> superuser can defend themselves against a superuser. Pretending
> otherwise would be confusing and have no security benefits.

Good framing. With you so far...

> Now let's think about this case. Can a table owner defend themselves
> against a subscription owner who wants to usurp their privileges? If
> they cannot, then I think that what the patch does is correct for the
> same reason that I think we would correctly reject the hypothetical
> command proposed above.

Agreed...

[ Aside: A user foo who accesses a table owned by bar has no means to
defend themselves against SECURITY INVOKER code that usurps their
privileges. Applying your logic above, foo should have to grant SET
ROLE privileges to bar before accessing the table. ]

> If
> the table owner has no executable code at all attached to the table -
> -
> not just triggers, but also expression indexes and default
> expressions
> and so forth -- then they can.

Right...

>  If they do have those things then in
> theory they might be able to protect themselves, but in practice they
> are unlikely to be careful enough. I judge that practically every
> installation where table owners use triggers would be easily
> compromised. Only the most security-conscious of table owners are
> going to get this right.

This is the interesting part.

Commit 11da97024ab set the subscription process's search path as empty.
It seems it was done to protect the subscription owner against users
writing into the public schema. But after your apply-as-table-owner
patch, it seems to also offer some protection for table owners against
a malicious subscription owner, too.

But I must be missing something obvious here -- if the subscription
process has an empty search_path, what can an attacker do to trick it?

Regards,
    Jeff Davis

On Mon, 27 Mar 2023 at 22:47, Robert Haas <robertmhaas@gmail.com> wrote:
> Can a table owner defend themselves
> against a subscription owner who wants to usurp their privileges?

I have a hard time following the discussion. And I think it's because
there's lots of different possible privilege escalations to think
about. Below is the list of escalations I've gathered and how I think
they interact with the different patches:
1. Table owner escalating to a high-privileged subscription owner.
i.e. the subscription owner is superuser, or has SET ROLE privileges
for all owners of tables in the subscription.
2. Table owner escalating to a low-privileged subscription owner. e.g.
the subscription owner can only insert into the tables in its
subscription
    a. The subscription owner only has insert permissions for a tables
owned by a single other user
    b. The subscription owner has insert permissions for tables owned
by multiple other users (but still does not have SET ROLE, or possibly
even select/update/delete)
3. Publisher applying into tables that the subscriber side doesn't want it to
4. Subscription-owner escalating to table owner
    a. The subscription owner is highly privileged (allows SET ROLE to
table owner)
    b. The subscription owner is lowly privileged

Which can currently only be addressed by having 1
subscription/publication pair for every table owner. This has the big
issue that WAL needs to be decoded multiple times on the publisher.
This patch would make escalation 1 impossible without having to do
anything special when setting up the subscription. With Citus we only
run into this escalation issue with logical replication at the moment.
We want to replicate lots of different tables, possibly owned by
multiple users from one node to another. We trust the publisher and we
trust the subscription owner, but we don't trust the table owners at
all. This is why I'm very much in favor of a version of this patch.

2.a seems a non-issue, because in this case the table owner can easily
give itself the same permissions as the subscription owner (if it
didn't have them yet). So by "escalating" to the subscription owner
the table owner only gets fewer permissions. 2.b is actually
interesting from a security perspective, because by escalating to the
subscription owner, the table owner might be able to insert into
tables that it normally can't. Patch v1 would "solve" both these
issues by simply not supporting these scenarios. My patch v2 keeps the
existing behaviour, where both "escalations" are possible and who-ever
sets up the replication should create a dedicated subscriber for each
table owner to make sure that only 2.a ever occurs and 2.b does not.

3 is something I have not run into. But I can easily imagine a case
where a subscriber connects to a (somewhat) untrusted publisher for
the purpose of replicating changes from a single table, e.g. some
events table. But you don't want to allow replication into any other
tables, even if the publisher tells you to. Patch v1 would force you
to have SET ROLE privileges on the target events table its owner. So
if that owner owns other tables too, then effectively you'd allow the
publisher to write into those tables too. The current behaviour
(without any patch) supports protecting against this escalation, by
giving only INSERT permissions on a single table without the need for
SET ROLE. My v2 patch preserves that ability.

4.a again seems like an obvious non-issue to me because the
subscription owner can already "escalate" to table owner using SET
ROLE. 4.b seems like it's pretty much the same as 3, afaict all the
same arguments apply. And I honestly can't think of a real situation
where you would not trust the subscription owner (as opposed to the
publisher). If any of you have an example of such a situation I'd love
to understand this one better.

All in all, I think patch v2 is the right direction here, because it
covers all these escalations to some extent.

On Tue, 28 Mar 2023 at 11:15, Jelte Fennema <postgres@jeltef.nl> wrote:
> Which can currently only be addressed by having 1
> subscription/publication pair for every table owner.

Oops, moving sentences around in my email made me not explicitly
reference escalation 1 anymore. The above should have been:
1 can currently only be addressed by having 1 subscription/publication
pair for every table owner.

On Mon, Mar 27, 2023 at 6:08 PM Jelte Fennema <postgres@jeltef.nl> wrote:
> Attached is an updated version of your patch with what I had in mind
> (admittedly it needed one more line than "just" the return to make it
> work). But as you can see all previous tests for a lowly privileged
> subscription owner that **cannot** SET ROLE to the table owner
> continue to work as they did before. While still downgrading to the
> table owners role when the subscription owner **can** SET ROLE to the
> table owner.
>
> Obviously this needs some comments explaining what's going on and
> probably some code refactoring and/or variable renaming, but I hope
> it's clear what I meant now: For high privileged subscription owners,
> we downgrade to the permissions of the table owner, but for low
> privileged ones we care about permissions of the subscription owner
> itself.

Hmm. This is an interesting idea. A variant on this theme could be:
what if we made this an explicit configuration option?

I'm worried that if we just try to switch users and silently fall back
to not doing so when we don't have enough permissions, the resulting
behavior is going to be difficult to understand and troubleshoot. I'm
thinking that maybe if you let people pick the behavior they want that
becomes more comprehensible. It's also a nice insurance policy: say
for the sake of argument we make switch-to-table-owner the new
default. If that new behavior causes something to happen to somebody
that they don't like, they can always turn it off, even if they are a
highly privileged user who doesn't "need" to turn it off.

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

On Tue, Mar 28, 2023 at 1:38 AM Jeff Davis <pgsql@j-davis.com> wrote:
> >  If they do have those things then in
> > theory they might be able to protect themselves, but in practice they
> > are unlikely to be careful enough. I judge that practically every
> > installation where table owners use triggers would be easily
> > compromised. Only the most security-conscious of table owners are
> > going to get this right.
>
> This is the interesting part.
>
> Commit 11da97024ab set the subscription process's search path as empty.
> It seems it was done to protect the subscription owner against users
> writing into the public schema. But after your apply-as-table-owner
> patch, it seems to also offer some protection for table owners against
> a malicious subscription owner, too.
>
> But I must be missing something obvious here -- if the subscription
> process has an empty search_path, what can an attacker do to trick it?

Oh, interesting. I hadn't realized we were doing that, and I do think
that narrows the vulnerability quite a bit.

But I still feel pretty uncomfortable with the idea of requiring only
INSERT/UPDATE/DELETE permissions on the target table. Let's suppose
that you create a table and attach a trigger to it which is SECURITY
INVOKER. Absent logical replication, you don't really need to worry
about whether that function is written securely -- it will run with
privileges of the person performing the DML, and not impact your
account at all. They have reason to be scared of you, but not the
reverse. However, if the people doing DML on the table can arrange to
perform that DML as you, then any security vulnerabilities in your
function potentially allow them to compromise your account. Now, there
might not be any, but there also might be some, depending on exactly
what your function does. And if logical replication into a table
requires only I/U/D permission then it's basically a back-door way to
run functions that someone could otherwise execute only as themselves
as the table owner, and that's scary.

Now, I don't know how much to worry about that. As you just pointed
out to me in some out-of-band discussion, this is only going to affect
a table owner who writes a trigger, makes it insecure in some way
other than failure to sanitize the search_path, and sets it ENABLE
ALWAYS TRIGGER or ENABLE REPLICA TRIGGER. And maybe we could say that
if you do that last part, you kind of need to think about the
consequences for logical replication. If so, we could document that
problem away. It would also affect someone who uses a default
expression or other means of associating executable code with the
table, and something like a default expression doesn't require any
explicit setting to make it apply in case of replication, so maybe the
risk of someone messing up is a bit higher.

But this definitely makes it more of a judgment call than I thought
initially. Functions that are vulnerable to search_path exploits are a
dime a dozen; other kinds of exploits are going to be less common, and
more dependent on exactly what the function is doing.

I'm still inclined to leave the patch checking for SET ROLE, because
after all, we're thinking of switching user identities to the table
owner, and checking whether we can SET ROLE is the most natural way of
doing that, and definitely doesn't leave room to escalate to any
privilege you don't already have. However, there might be an argument
that we ought to do something else, like have a REPLICATE privilege on
the table that the owner can grant to users that they trust to
replicate into that table. Because time is short, I'd like to leave
that idea for a future release. What I would like to change in the
patch in this release is to add a new subscription property along the
lines of what I proposed to Jelte in my earlier email: let's provide
an escape hatch that turns off the user-switching behavior. If we do
that, then anyone who feels themselves worse off after this patch can
switch back to the old behavior. Most people will be better off, I
believe, and the opportunity to make things still better in the future
is not foreclosed.

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

On Tue, 28 Mar 2023 at 18:13, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, Mar 27, 2023 at 6:08 PM Jelte Fennema <postgres@jeltef.nl> wrote:
> > For high privileged subscription owners,
> > we downgrade to the permissions of the table owner, but for low
> > privileged ones we care about permissions of the subscription owner
> > itself.
>
> Hmm. This is an interesting idea. A variant on this theme could be:
> what if we made this an explicit configuration option?

Sounds perfect to me. Let's do that. As long as the old no-superuser
tests continue to pass when disabling the new switch-to-table-owner
behaviour, that sounds totally fine to me. I think it's probably
easiest if you use the tests from my v2 patch when you add that
option, since that was by far the thing I spent most time on to get
right in the v2 patch.

On Tue, 28 Mar 2023 at 18:13, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, Mar 27, 2023 at 6:08 PM Jelte Fennema <postgres@jeltef.nl> wrote:
> > Attached is an updated version of your patch with what I had in mind
> > (admittedly it needed one more line than "just" the return to make it
> > work). But as you can see all previous tests for a lowly privileged
> > subscription owner that **cannot** SET ROLE to the table owner
> > continue to work as they did before. While still downgrading to the
> > table owners role when the subscription owner **can** SET ROLE to the
> > table owner.
> >
> > Obviously this needs some comments explaining what's going on and
> > probably some code refactoring and/or variable renaming, but I hope
> > it's clear what I meant now: For high privileged subscription owners,
> > we downgrade to the permissions of the table owner, but for low
> > privileged ones we care about permissions of the subscription owner
> > itself.
>
> Hmm. This is an interesting idea. A variant on this theme could be:
> what if we made this an explicit configuration option?
>
> I'm worried that if we just try to switch users and silently fall back
> to not doing so when we don't have enough permissions, the resulting
> behavior is going to be difficult to understand and troubleshoot. I'm
> thinking that maybe if you let people pick the behavior they want that
> becomes more comprehensible. It's also a nice insurance policy: say
> for the sake of argument we make switch-to-table-owner the new
> default. If that new behavior causes something to happen to somebody
> that they don't like, they can always turn it off, even if they are a
> highly privileged user who doesn't "need" to turn it off.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com

On Tue, 2023-03-28 at 13:55 -0400, Robert Haas wrote:
> Oh, interesting. I hadn't realized we were doing that, and I do think
> that narrows the vulnerability quite a bit.

It's good to know I wasn't missing something obvious.

> bsent logical replication, you don't really need to worry
> about whether that function is written securely -- it will run with
> privileges of the person performing the DML, and not impact your
> account at all.

That's not strictly true. See the example at the bottom of this email.

The second trigger is SECURITY INVOKER, and it captures the leaked
secret number that was stored in the tuple by an earlier SECURITY
DEFINER trigger.

Perhaps I'm being pedantic, but my point is that SECURITY INVOKER is
not a magical shield that protects users from themselves without bound.
It protects users against some kinds of attacks, sure, but there's a
limit to what it has to offer.

>  They have reason to be scared of you, but not the
> reverse. However, if the people doing DML on the table can arrange to
> perform that DML as you, then any security vulnerabilities in your
> function potentially allow them to compromise your account.

OK, but I'd like to be clear that you've moved from your prior
statement:

  "in theory they might be able to protect themselves, but in practice
they are unlikely to be careful enough"

To something very different, where it seems that we can't think of a
concrete problem even for not-so-careful users.

The dangerous cases seem to be something along the lines of a security-
invoker trigger function that builds and executes arbirary SQL based on
the row contents. And then the table owner would then still need to set
ENABLE ALWAYS TRIGGER.

Do we really want to take that case on as our security responsibility?

> It would also affect someone who uses a default
> expression or other means of associating executable code with the
> table, and something like a default expression doesn't require any
> explicit setting to make it apply in case of replication, so maybe
> the
> risk of someone messing up is a bit higher.

Perhaps, but I still don't understand that case. Unless I'm missing
something, the table owner would have to write a pretty weird default
expression or check constraint or whatever to end up with something
dangerous.

> But this definitely makes it more of a judgment call than I thought
> initially.

And I'm fine if the judgement is that we just require SET ROLE to be
conservative and make sure we don't over-promise in 16. That's a
totally reasonable thing: it's easier to loosen restrictions later than
to tighten them. Furthermore, just because you and I can't think of
exploitable problems doesn't mean they don't exist.

I have found this discussion enlightening, so thank you for going
through these details with me.

> I'm still inclined to leave the patch checking for SET ROLE

+0.5. I want the patch to go in either way, and can carry on the
discussion later and improve things more for 17.

But I want to say generally that I believe we spend too much effort
trying to protect unreasonable users from themselves, which interferes
with our ability to protect reasonable users and solve real use cases.

> However, there might be an argument
> that we ought to do something else, like have a REPLICATE privilege

Sounds cool. Not sure I have an opinion yet, but probably 17 material.

> What I would like to change in the
> patch in this release is to add a new subscription property along the
> lines of what I proposed to Jelte in my earlier email: let's provide
> an escape hatch that turns off the user-switching behavior.

I believe switching to the table owner (as done in your patch) is the
right best practice, and should be the default.

I'm fine with an escape hatch here to ease migrations or whatever. But
please do it in an unobtrusive way such that we (as hackers) can mostly
forget that the non-switching behavior exists. At least for me, our
system is already pretty complex without two kinds of subscription
security models.


Regards,
    Jeff Davis


----- example follows ------

  -- user foo
  CREATE TABLE secret(I INT);
  INSERT INTO secret VALUES(42);
  CREATE TABLE r(i INT, secret INT);
  CREATE OR REPLACE FUNCTION a_func() RETURNS TRIGGER
    SECURITY DEFINER LANGUAGE plpgsql AS
  $$
    BEGIN
       SELECT i INTO NEW.secret FROM secret;
       RETURN NEW;
    END;
  $$;
  CREATE OR REPLACE FUNCTION z_func() RETURNS TRIGGER
    SECURITY INVOKER LANGUAGE plpgsql AS
  $$
    BEGIN
      IF NEW.secret <> abs(NEW.secret) THEN
        RAISE EXCEPTION 'no negative secret numbers allowed';
      END IF;
      RETURN NEW;
    END;
  $$;
  CREATE TRIGGER a_trigger BEFORE INSERT ON r
    FOR EACH ROW EXECUTE PROCEDURE a_func();
  CREATE TRIGGER z_trigger BEFORE INSERT ON r
    FOR EACH ROW EXECUTE PROCEDURE z_func();
  GRANT INSERT ON r TO bar;
  GRANT USAGE ON SCHEMA foo TO bar;

  -- user bar
  CREATE OR REPLACE FUNCTION bar.abs(i INT4) RETURNS INT4
    LANGUAGE plpgsql AS
  $$
    BEGIN
      INSERT INTO secret_copy VALUES(i);
      RETURN pg_catalog.abs(i);
    END;
  $$;
  CREATE TABLE secret_copy(secret int);
  SET search_path = "$user", pg_catalog;
  INSERT INTO foo.r VALUES(1);

On Tue, Mar 28, 2023 at 6:48 PM Jeff Davis <pgsql@j-davis.com> wrote:
> That's not strictly true. See the example at the bottom of this email.

Yeah, we're very casual about repeated user switching in all kinds of
contexts, and that's really pretty scary. I don't know how to fix that
without removing capabilities that people rely on.

> The dangerous cases seem to be something along the lines of a security-
> invoker trigger function that builds and executes arbirary SQL based on
> the row contents. And then the table owner would then still need to set
> ENABLE ALWAYS TRIGGER.
>
> Do we really want to take that case on as our security responsibility?

That's something about which I would like to get more opinions.

> I believe switching to the table owner (as done in your patch) is the
> right best practice, and should be the default.
>
> I'm fine with an escape hatch here to ease migrations or whatever. But
> please do it in an unobtrusive way such that we (as hackers) can mostly
> forget that the non-switching behavior exists. At least for me, our
> system is already pretty complex without two kinds of subscription
> security models.

Here's a new patch set to show how this would work. 0001 is as before.
0002 adds a run_as_owner option to subscriptions. This doesn't make
the updated regression tests fail, because they don't use it. If you
revert the changes to 027_nosuperuser.pl then you get failures (as one
would hope) and if you then add WITH (run_as_owner = true) to the
CREATE SUBSCRIPTION command in 027_nosuperuser.pl then it passes
again. I need to spend some more time thinking about what the tests
actually ought to look like if we go this route -- I haven't looked
through what Jelte proposed yet -- and also the documentation would
need a bunch of updating.

Now, backing up a minute to address your other comments here, I'm not
particularly dedicated to this new option. But the problem is that you
can't please all the people all the time. When I proposed to
unconditionally change the behavior, people said "what if
SECURITY_RESTRICTED_OPERATION hoses people, or they just don't like
the new behavior?" and "what if I don't trust the publisher and want
to use table permissions on the subscriber side to minimize my
exposure?". Well, this option provides a way out of those problems by
allowing you to get the current behavior, but that gives rise to the
complaint you raise here: it's nicer to have one behavior than two. To
be honest, I'm pretty sympathetic to that complaint: i think the
problems if we don't add this switch are probably not going to affect
many people at all, and the new switch will therefore be of very
little value but will be something we continue to maintain forever.
However, I could be wrong and there's certainly something to be said
for having escape hatches that let people un-break stuff that core
patches break.

But you know we can't have it both ways. We either add an escape hatch
to make sure that people have a way out if they run into trouble and
incur the risk and complexity of carrying it probably forever, OR we
suck it up and make it a hard behavior change and tell people who are
upset "too bad, we changed it." And either way, at least PostgreSQL
hacker who has taken the time to write about this topic is going to
think we've made a poor call, or at best a dubious one, and either
way, there will probably be at least one user who cusses the
PostgreSQL development process out under their breath. I don't know
what to do about that. There's not a fix for this gaping security
problem that doesn't involve the potential of some people being
inconvenienced.

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

On Wed, 2023-03-29 at 16:00 -0400, Robert Haas wrote:
> Here's a new patch set to show how this would work. 0001 is as
> before.

The following special case ("unless they can SET ROLE..."):

 /*
  * Try to prevent the user to which we're switching from assuming the
  * privileges of the current user, unless they can SET ROLE to that
  * user anyway.
  */

which turns SwitchToUntrustedUser() into a no-op, is conceptually
redundant with patch 0002. The only difference is that the former (in
0001) is implicit and the latter (in 0002) is declared.

I say just take the special case out of 0001. If the trigger doesn't
work as a SECURITY_RESTRICTED_OPERATION, and is also ENABLED ALWAYS,
then the user can just use the new option in 0002 to get the old
behavior. I don't see a reason to implicitly give them the old
behavior, as 0001 does.

> 0002 adds a run_as_owner option to subscriptions. This doesn't make
> the updated regression tests fail, because they don't use it. If you
> revert the changes to 027_nosuperuser.pl then you get failures (as
> one
> would hope) and if you then add WITH (run_as_owner = true) to the
> CREATE SUBSCRIPTION command in 027_nosuperuser.pl then it passes
> again. I need to spend some more time thinking about what the tests
> actually ought to look like if we go this route -- I haven't looked
> through what Jelte proposed yet -- and also the documentation would
> need a bunch of updating.

"run_as_owner" is ambiguous -- subscription owner or table owner?

I would prefer something like "trust_table_owners". That would
communicate that the user shouldn't choose it unless they know what
they're doing.



> And either way, at least PostgreSQL
> hacker who has taken the time to write about this topic is going to
> think we've made a poor call, or at best a dubious one, and either
> way,

If you are worried that *I* think 0002 would be a poor call, then no, I
don't. Initially I didn't like the idea of supporting two behaviors
(and who would?), but we probably can't avoid it at least for right
now.

Regards,
    Jeff Davis

On Thu, Mar 30, 2023 at 1:19 AM Jeff Davis <pgsql@j-davis.com> wrote:
> I say just take the special case out of 0001. If the trigger doesn't
> work as a SECURITY_RESTRICTED_OPERATION, and is also ENABLED ALWAYS,
> then the user can just use the new option in 0002 to get the old
> behavior. I don't see a reason to implicitly give them the old
> behavior, as 0001 does.

Mmm, I don't agree. Suppose A can SET ROLE to B or C, and B can SET
ROLE to A. With the patch as written, actions on B's tables are not
confined by the SECURITY_RESTRICTED_OPERATION flag, but actions on C's
tables are.

I think we want to do everything possible to avoid people feeling like
they need to turn on this new option. I'm not sure we'll ever be able
to get rid of it, but we certainly should avoid doing things that make
it more likely that it will be needed.

> > 0002 adds a run_as_owner option to subscriptions. This doesn't make
> > the updated regression tests fail, because they don't use it. If you
> > revert the changes to 027_nosuperuser.pl then you get failures (as
> > one
> > would hope) and if you then add WITH (run_as_owner = true) to the
> > CREATE SUBSCRIPTION command in 027_nosuperuser.pl then it passes
> > again. I need to spend some more time thinking about what the tests
> > actually ought to look like if we go this route -- I haven't looked
> > through what Jelte proposed yet -- and also the documentation would
> > need a bunch of updating.
>
> "run_as_owner" is ambiguous -- subscription owner or table owner?
>
> I would prefer something like "trust_table_owners". That would
> communicate that the user shouldn't choose it unless they know what
> they're doing.

I agree that the naming is somewhat problematic, but I don't like
trust_table_owners. It's not clear enough about what actually happens.
I want the name to describe behavior, not sentiment.

run_as_subscription_owner removes the ambiguity, but is long.

run_as_table_owner is a bit shorter, and we could do that with the
sense reversed. Is that equally clear? I'm not sure.

I can think of other alternatives, like user_switching or
switch_to_table_owner or no_user_switching or various other things,
but none of them seem very good to me.

Another idea could be to make the option non-Boolean. This is
comically long and I can't seriously recommend it, but just to
illustrate the point, if you type CREATE SUBSCRIPTION ... WITH
(execute_code_as_owner_of_which_object = subscription) then you
certainly should know what you've signed up for! If there were a
shorter version that were still clear, I could go for that, but I'm
having trouble coming up with exact wording.

I don't think run_as_owner is terrible, despite the ambiguity. It's
talking about the owner of the object on which the property is being
set. Isn't that the most natural interpretation? I'd be pretty
surprised if I set a property called run_as_owner on object A and it
ran as the owner of some other object B. I think our notion of how
ambiguous this is may be somewhat inflated by the fact that we've just
had a huge conversation about whether it should be the table owner or
the subscription owner, so those possibilities are etched in our mind
in a way that maybe they won't be for people coming at this fresh. But
it's hard to be sure what other people will think about something, and
I don't want to be too optimistic about the name I picked, either.

> If you are worried that *I* think 0002 would be a poor call, then no, I
> don't. Initially I didn't like the idea of supporting two behaviors
> (and who would?), but we probably can't avoid it at least for right
> now.

OK, good. Then we have a way forward that nobody's too upset about.

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

On Thu, 30 Mar 2023 at 15:42, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Mar 30, 2023 at 1:19 AM Jeff Davis <pgsql@j-davis.com> wrote:
> > I say just take the special case out of 0001. If the trigger doesn't
> > work as a SECURITY_RESTRICTED_OPERATION, and is also ENABLED ALWAYS,
> > then the user can just use the new option in 0002 to get the old
> > behavior. I don't see a reason to implicitly give them the old
> > behavior, as 0001 does.
>
> Mmm, I don't agree. Suppose A can SET ROLE to B or C, and B can SET
> ROLE to A. With the patch as written, actions on B's tables are not
> confined by the SECURITY_RESTRICTED_OPERATION flag, but actions on C's
> tables are.

I think that's fair. There's no need to set
SECURITY_RESTRICTED_OPERATION if it doesn't protect you anyway, and
indeed it might break things. To be clear I do think it's important to
still switch to the table owner, simply for consistency. But that's
done in the patch so that's fine.

> I agree that the naming is somewhat problematic, but I don't like
> trust_table_owners. It's not clear enough about what actually happens.
> I want the name to describe behavior, not sentiment.

For security related things, I think sentiment is often just as
important as the actual behaviour. It's not without reason that newer
javascript frameworks have things like dangerouslySetInnerHTML, to
scare people away from using it unless they know what they are doing.

> I don't think run_as_owner is terrible, despite the ambiguity. It's
> talking about the owner of the object on which the property is being
> set. Isn't that the most natural interpretation? I'd be pretty
> surprised if I set a property called run_as_owner on object A and it
> ran as the owner of some other object B.

I think that's fair and I'd be happy with run_as_owner. If someone
doesn't understand which owner, they should probably check the
documentation anyways to understand the implications.

Regarding the actual patch. I think the code looks good. Mainly the
tests and docs are lacking for the new option. Like I said for the
tests you can borrow the tests I updated for my v2 patch, I think
those should work fine for the new option.

On Thu, 30 Mar 2023 at 15:42, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Mar 30, 2023 at 1:19 AM Jeff Davis <pgsql@j-davis.com> wrote:
> > I say just take the special case out of 0001. If the trigger doesn't
> > work as a SECURITY_RESTRICTED_OPERATION, and is also ENABLED ALWAYS,
> > then the user can just use the new option in 0002 to get the old
> > behavior. I don't see a reason to implicitly give them the old
> > behavior, as 0001 does.
>
> Mmm, I don't agree. Suppose A can SET ROLE to B or C, and B can SET
> ROLE to A. With the patch as written, actions on B's tables are not
> confined by the SECURITY_RESTRICTED_OPERATION flag, but actions on C's
> tables are.
>
> I think we want to do everything possible to avoid people feeling like
> they need to turn on this new option. I'm not sure we'll ever be able
> to get rid of it, but we certainly should avoid doing things that make
> it more likely that it will be needed.
>
> > > 0002 adds a run_as_owner option to subscriptions. This doesn't make
> > > the updated regression tests fail, because they don't use it. If you
> > > revert the changes to 027_nosuperuser.pl then you get failures (as
> > > one
> > > would hope) and if you then add WITH (run_as_owner = true) to the
> > > CREATE SUBSCRIPTION command in 027_nosuperuser.pl then it passes
> > > again. I need to spend some more time thinking about what the tests
> > > actually ought to look like if we go this route -- I haven't looked
> > > through what Jelte proposed yet -- and also the documentation would
> > > need a bunch of updating.
> >
> > "run_as_owner" is ambiguous -- subscription owner or table owner?
> >
> > I would prefer something like "trust_table_owners". That would
> > communicate that the user shouldn't choose it unless they know what
> > they're doing.
>
> I agree that the naming is somewhat problematic, but I don't like
> trust_table_owners. It's not clear enough about what actually happens.
> I want the name to describe behavior, not sentiment.
>
> run_as_subscription_owner removes the ambiguity, but is long.
>
> run_as_table_owner is a bit shorter, and we could do that with the
> sense reversed. Is that equally clear? I'm not sure.
>
> I can think of other alternatives, like user_switching or
> switch_to_table_owner or no_user_switching or various other things,
> but none of them seem very good to me.
>
> Another idea could be to make the option non-Boolean. This is
> comically long and I can't seriously recommend it, but just to
> illustrate the point, if you type CREATE SUBSCRIPTION ... WITH
> (execute_code_as_owner_of_which_object = subscription) then you
> certainly should know what you've signed up for! If there were a
> shorter version that were still clear, I could go for that, but I'm
> having trouble coming up with exact wording.
>
> I don't think run_as_owner is terrible, despite the ambiguity. It's
> talking about the owner of the object on which the property is being
> set. Isn't that the most natural interpretation? I'd be pretty
> surprised if I set a property called run_as_owner on object A and it
> ran as the owner of some other object B. I think our notion of how
> ambiguous this is may be somewhat inflated by the fact that we've just
> had a huge conversation about whether it should be the table owner or
> the subscription owner, so those possibilities are etched in our mind
> in a way that maybe they won't be for people coming at this fresh. But
> it's hard to be sure what other people will think about something, and
> I don't want to be too optimistic about the name I picked, either.
>
> > If you are worried that *I* think 0002 would be a poor call, then no, I
> > don't. Initially I didn't like the idea of supporting two behaviors
> > (and who would?), but we probably can't avoid it at least for right
> > now.
>
> OK, good. Then we have a way forward that nobody's too upset about.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com
>
>

On Thu, Mar 30, 2023 at 11:11 AM Jelte Fennema <postgres@jeltef.nl> wrote:
> Regarding the actual patch. I think the code looks good. Mainly the
> tests and docs are lacking for the new option. Like I said for the
> tests you can borrow the tests I updated for my v2 patch, I think
> those should work fine for the new option.

I took a look at that but I didn't really feel like that was quite the
direction I wanted to go. I'd actually like to separate the tests of
the new option out into their own file, so that if for some reason we
decide we want to remove it in the future, it's easier to nuke all the
associated tests. Also, quite frankly, I think we've gone way
overboard in terms of loading too many tests into a single file, with
the result that it's very hard to understand exactly what and all the
file is actually testing and what it's intended to be testing. So the
attached 0002 does it that way. I've also amended 0001 and 0002 with
documentation changes that I hope are appropriate.

I noticed along the way that my earlier commits had missed one place
that needed to be updated by the pg_create_subscription patch I
created earlier. A fix for that is included in 0001, but it can be
broken out and committed separately if somebody feels strongly about
it. I personally don't think it's worth it.

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

On Thu, 2023-03-30 at 09:41 -0400, Robert Haas wrote:
> On Thu, Mar 30, 2023 at 1:19 AM Jeff Davis <pgsql@j-davis.com> wrote:
> > I say just take the special case out of 0001. If the trigger
> > doesn't
> > work as a SECURITY_RESTRICTED_OPERATION, and is also ENABLED
> > ALWAYS,
> > then the user can just use the new option in 0002 to get the old
> > behavior. I don't see a reason to implicitly give them the old
> > behavior, as 0001 does.
>
> Mmm, I don't agree. Suppose A can SET ROLE to B or C, and B can SET
> ROLE to A. With the patch as written, actions on B's tables are not
> confined by the SECURITY_RESTRICTED_OPERATION flag, but actions on
> C's
> tables are.

It's interesting that it's not transitive, but I'm not sure whether
that's an argument for or against the current approach, or where it
fits (or doesn't fit) with my suggestion. Why do you consider it
important that C's actions are SECURITY_RESTRICTED_OPERATIONs?

> I think we want to do everything possible to avoid people feeling
> like
> they need to turn on this new option. I'm not sure we'll ever be able
> to get rid of it, but we certainly should avoid doing things that
> make
> it more likely that it will be needed.

I don't think it helps much, though. While I previously said that the
special-case behavior is implicit (which is true), it still almost
certainly requires a manual step:

  GRANT subscription_owner TO table_owner WITH SET;

In version 16, the subscription owner is almost certainly a superuser,
and the table owner almost certainly is not, so there's little chance
that it just happens that the table owner has that privilege already.

I don't think we want to encourage such grants to proliferate any more
than we want the option you introduce in 0002 to proliferate. Arguably,
it's worse.

And let's say a user says "I upgraded and my trigger broke logical
replication with message about a security-restricted operation... how
do I get up and running again?". With the patches as-written, we will
have two answers to that question:

  * GRANT subscription_owner TO table_owner WITH SET TRUE
  * ALTER SUBSCRIPTION ... ($awesome_option_name=false)

Under what circumstances would we recommend the former vs. the latter?

> >
> I agree that the naming is somewhat problematic, but I don't like
> trust_table_owners. It's not clear enough about what actually
> happens.
> I want the name to describe behavior, not sentiment.

On reflection, I agree here. We want it to communicate something about
the behavior or mechanism.

> run_as_subscription_owner removes the ambiguity, but is long.

Then fewer people will use it, which might be a good thing.

> I can think of other alternatives, like user_switching or
> switch_to_table_owner or no_user_switching or various other things,
> but none of them seem very good to me.

I like the idea of using "switch" (or some synonym) because it's
technically more correct. The subscription always runs as the
subscription owner; we are just switching temporarily while applying a
change.

> Another idea could be to make the option non-Boolean. This is
> comically long and I can't seriously recommend it, but just to
> illustrate the point, if you type CREATE SUBSCRIPTION ... WITH
> (execute_code_as_owner_of_which_object = subscription) then you
> certainly should know what you've signed up for! If there were a
> shorter version that were still clear, I could go for that, but I'm
> having trouble coming up with exact wording.

I don't care for that -- it communicates the options as co-equal and
maybe something that would live forever (or even have more options in
the future). I'd prefer that nobody uses the non-switching behavior
except for migration purposes or weird use cases we don't really
understand.

> I don't think run_as_owner is terrible, despite the ambiguity.

I won't object but I'm not thrilled.

>
Regards,
    Jeff Davis

On Thu, Mar 30, 2023 at 2:52 PM Jeff Davis <pgsql@j-davis.com> wrote:
> > Mmm, I don't agree. Suppose A can SET ROLE to B or C, and B can SET
> > ROLE to A. With the patch as written, actions on B's tables are not
> > confined by the SECURITY_RESTRICTED_OPERATION flag, but actions on
> > C's
> > tables are.
>
> It's interesting that it's not transitive, but I'm not sure whether
> that's an argument for or against the current approach, or where it
> fits (or doesn't fit) with my suggestion. Why do you consider it
> important that C's actions are SECURITY_RESTRICTED_OPERATIONs?

So that C can't try to hack into A's account.

I mean the point here is that B already has permissions to get into
A's account whenever they like, without any hacking. So we don't need
to impose SECURITY_RESTRICTED_OPERATION when running as B, because the
only purpose of SECURITY_RESTRICTED_OPERATION is to prevent the role
to which we're switching from attacking the role from which we're
switching. And that's how the patch is currently coded. You proposed
removing that behavior, on the theory that if the
SECURITY_RESTRICTED_OPERATION restrictions were a problem, someone
could activate the run_as_owner option (or whatever we end up calling
it). But the run_as_owner option applies to the entire subscription.
If A activates that option, then B's hypothetical triggers that run
afoul of the SECURITY_RESTRICTED_OPERATION restrictions start working
again (woohoo!) but they're now vulnerable to attacks from C. With the
patch as coded, A doesn't need to use run_as_owner, everything still
just works for B, and A is still protected against C.

> In version 16, the subscription owner is almost certainly a superuser,
> and the table owner almost certainly is not, so there's little chance
> that it just happens that the table owner has that privilege already.
>
> I don't think we want to encourage such grants to proliferate any more
> than we want the option you introduce in 0002 to proliferate. Arguably,
> it's worse.

I don't necessarily find those role grants to be a problem. Obviously
it depends on the use case. If you're hoping to be able to set up an
account whose only purpose in life is to own subscriptions and which
should have as few permissions as possible, then those role grants
suck, and a hypothetical future feature where you can GRANT
REPLICATION ON TABLE t1 TO subscription_owning_user will be far
better. But I imagine CREATE SUBSCRIPTION being used either by
superusers or by people who already have those role grants anyway,
because I imagine replication as something that a highly privileged
user configures on behalf of everyone who uses the system. And in that
case those role grants aren't something new that you do specifically
for logical replication - they're already there because you need them
to administer stuff. Or you're the superuser and don't need them
anyway.

> And let's say a user says "I upgraded and my trigger broke logical
> replication with message about a security-restricted operation... how
> do I get up and running again?". With the patches as-written, we will
> have two answers to that question:
>
>   * GRANT subscription_owner TO table_owner WITH SET TRUE
>   * ALTER SUBSCRIPTION ... ($awesome_option_name=false)
>
> Under what circumstances would we recommend the former vs. the latter?

Well, the latter is clearly better because it has such an awesome
option name, right?

More seriously, my theory is that there's very little use case for
having a replication trigger, default expression, etc. that is
performing a security restricted operation.  And if someone does have
a use case, and it's between users that can't already SET ROLE back
and forth, then the setup is pretty dubious from a security
perspective and maybe the user ought to rethink it. And if they don't
want to rethink it, then they need to throw security out the window,
and I don't really care which of those commands they use to do it, but
the second one would probably break less other stuff for them, so I'd
likely recommend that one.

> > I don't think run_as_owner is terrible, despite the ambiguity.
>
> I won't object but I'm not thrilled.

Let's see if anyone else weighs in.

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

On Thu, 2023-03-30 at 16:08 -0400, Robert Haas wrote:
>  But the run_as_owner option applies to the entire subscription.
> If A activates that option, then B's hypothetical triggers that run
> afoul of the SECURITY_RESTRICTED_OPERATION restrictions start working
> again (woohoo!) but they're now vulnerable to attacks from C. With
> the
> patch as coded, A doesn't need to use run_as_owner, everything still
> just works for B, and A is still protected against C.

That's moving the goalposts a little, though:

"Some concern was expressed ... might break things that are currently
working..."

https://www.postgresql.org/message-id/CA+TgmoaE35kKS3-zSvGiZszXP9Tb9rNfYzT=+fO8Ehk5EDKrag@mail.gmail.com

If the original use case was "don't break stuff", I think patch 0002
solves that, and we don't need this special case in 0001. Would you
agree with that statement?

Hypothetically, if 0001 (without the special case) along with 0002 were
already in 16, and then there was some hypothetical 0003 that
introduced the special case to solve the problem described above with
the bidirectional trust relationship, I'm not sure I'd be sold on 0003.
First, the problem seems fairly minor to me, at least in comparison to
the main problem you are solving in this thread. Second, it seems like
you could work around it by having two subscriptions. Third, it's a bit
unintuitive at least to me: if you introduce a new user Z that can SET
ROLE to any of A, B, or C, and then Z reassigns the subscription to
themselves, then B's trigger will break because B can't SET ROLE to Z.

Others seem to like it, so don't take that as a hard objection.

>
> But I imagine CREATE SUBSCRIPTION being used either by
> superusers or by people who already have those role grants anyway,
> because I imagine replication as something that a highly privileged
> user configures on behalf of everyone who uses the system. And in
> that
> case those role grants aren't something new that you do specifically
> for logical replication - they're already there because you need them
> to administer stuff. Or you're the superuser and don't need them
> anyway.

Did the discussion drift back towards the SET ROLE in the other
direction? I thought we had settled that in v16 we would require that
the subscription owner can SET ROLE to the table owner (as in your
current 0001), but that we could revisit it later.

Regards,
    Jeff Davis

On Fri, Mar 31, 2023 at 3:36 AM Jeff Davis <pgsql@j-davis.com> wrote:
> That's moving the goalposts a little, though:
>
> "Some concern was expressed ... might break things that are currently
> working..."
>
> https://www.postgresql.org/message-id/CA+TgmoaE35kKS3-zSvGiZszXP9Tb9rNfYzT=+fO8Ehk5EDKrag@mail.gmail.com
>
> If the original use case was "don't break stuff", I think patch 0002
> solves that, and we don't need this special case in 0001. Would you
> agree with that statement?

I think that's too Boolean. The special case in 0001 is a better
solution for the cases where it works. It's both more granular and
more convenient. The fact that you might be able to get by with 0002
doesn't negate that.

> > But I imagine CREATE SUBSCRIPTION being used either by
> > superusers or by people who already have those role grants anyway,
> > because I imagine replication as something that a highly privileged
> > user configures on behalf of everyone who uses the system. And in
> > that
> > case those role grants aren't something new that you do specifically
> > for logical replication - they're already there because you need them
> > to administer stuff. Or you're the superuser and don't need them
> > anyway.
>
> Did the discussion drift back towards the SET ROLE in the other
> direction? I thought we had settled that in v16 we would require that
> the subscription owner can SET ROLE to the table owner (as in your
> current 0001), but that we could revisit it later.

Yeah, I think that's what we agreed. I'm just saying that I'm not as
concerned about that design as you are, and encouraging you to maybe
not be quite so dismayed by it.

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

On Fri, 2023-03-31 at 15:17 -0400, Robert Haas wrote:
> I think that's too Boolean. The special case in 0001 is a better
> solution for the cases where it works. It's both more granular and
> more convenient.

I guess the "more convenient" is where I'm confused, because the "grant
subscription_owner to table owner with set role true" is not likely to
be conveniently already present; it would need to be issued manually to
take advantage of this special case.

Do you have any concern about the weirdness where assigning the
subscription to a higher-privilege user Z would cause B's trigger to
fail?

Regards,
    Jeff Davis

On Mon, Mar 27, 2023 at 04:47:22PM -0400, Robert Haas wrote:
> So that's a grey area, at least IMHO. The patch could be revised in
> some way, and the permissions requirements downgraded. However, if we
> do that, I think we're going to have to document that although in
> theory table owners can make themselves against subscription owners,
> in practice they probably won't be. That approach has some advantages,
> and I don't think it's insane. However, I am not convinced that it is
> the best idea, either, and I had the impression based on
> pgsql-security discussion that Andres and Noah thought this way was
> better. I might have misinterpreted their position, and they might
> have changed their minds, and they might have been wrong. But that's
> how we got here.

["this way" = requirement for SET ROLE]

On Wed, Mar 29, 2023 at 04:00:45PM -0400, Robert Haas wrote:
> > The dangerous cases seem to be something along the lines of a security-
> > invoker trigger function that builds and executes arbirary SQL based on
> > the row contents. And then the table owner would then still need to set
> > ENABLE ALWAYS TRIGGER.
> >
> > Do we really want to take that case on as our security responsibility?
> 
> That's something about which I would like to get more opinions.

The most-plausible-to-me attack involves an ENABLE ALWAYS trigger that logs
CURRENT_USER to an audit table.  The "SQL based on the row contents" scenario
feels remote.  Another remotely-possible attack involves a trigger that
internally queries some other table having RLS.  (Switching to the table owner
can change the rows visible to that query.)

If having INSERT/UPDATE privileges on the table were enough to make a
subscription that impersonates the table owner, then relatively-unprivileged
roles could make a subscription to bypass the aforementioned auditing.  Commit
c3afe8c has imposed weighty requirements beyond I/U privileges, namely holding
the pg_create_subscription role and database-level CREATE privilege.  Since
database-level CREATE is already powerful, it would be plausible to drop the
SET ROLE requirement and add this audit bypass to its powers.  The SET ROLE
requirement is nice for keeping the powers disentangled.  One drawback is
making people do GRANTs regardless of whether a relevant audit trigger exists.
Another drawback is the subscription role having more privileges than ideally
needed.  I do like keeping strong privileges orthogonal, so I lean toward
keeping the SET ROLE requirement.

On Thu, Mar 30, 2023 at 02:17:31PM -0400, Robert Haas wrote:
> --- a/doc/src/sgml/logical-replication.sgml
> +++ b/doc/src/sgml/logical-replication.sgml
> @@ -1774,6 +1774,23 @@ CONTEXT:  processing remote data for replication origin "pg_16395" during "INSER
>     <literal>SET ROLE</literal> to each role that owns a replicated table.
>    </para>
>  
> +  <para>
> +   If the subscription has been configued with

Typo.

> Subject: [PATCH v3 1/2] Perform logical replication actions as the table
>  owner.

> Since this involves switching the active user frequently within
> a session that is authenticated as the subscription user, also
> impose SECURITY_RESTRICTED_OPEATION restrictions on logical

s/OPEATION/OPERATION/

> replication code. As an exception, if the table owner can SET
> ROLE to the subscription owner, these restrictions have no
> security value, so don't impose them in that case.
> 
> Subscription owners are now required to have the ability to
> SET ROLE to every role that owns a table that the subscription
> is replicating. If they don't, replication will fail. Superusers,
> who normally own subscriptions, satisfy this property by default.
> Non-superusers users who own subscriptions will needed to be
> granted the roles that own relevant tables.

s/will needed/will need/

(I did not read the patches in their entirety.)

On Thu, Mar 30, 2023 at 7:12 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> I don't think run_as_owner is terrible, despite the ambiguity. It's
> talking about the owner of the object on which the property is being
> set.
>

I find this justification quite reasonable to keep the option name as
run_as_owner. So, +1 to use the new option name as run_as_owner.

--
With Regards,
Amit Kapila.

On Fri, Mar 31, 2023 at 6:46 PM Jeff Davis <pgsql@j-davis.com> wrote:
> I guess the "more convenient" is where I'm confused, because the "grant
> subscription_owner to table owner with set role true" is not likely to
> be conveniently already present; it would need to be issued manually to
> take advantage of this special case.

You and I disagree about the likelihood of that, but I could well be wrong.

> Do you have any concern about the weirdness where assigning the
> subscription to a higher-privilege user Z would cause B's trigger to
> fail?

Not very much. I think the biggest risk is user confusion, but I don't
think that's a huge risk because I don't think this scenario will come
up very often. Also, it's kind of hard to imagine that there's a
security model here which never does anything potentially surprising.

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

Thank you for this email. It's very helpful to get your opinion on this.

On Sun, Apr 2, 2023 at 11:21 PM Noah Misch <noah@leadboat.com> wrote:
> On Wed, Mar 29, 2023 at 04:00:45PM -0400, Robert Haas wrote:
> > > The dangerous cases seem to be something along the lines of a security-
> > > invoker trigger function that builds and executes arbirary SQL based on
> > > the row contents. And then the table owner would then still need to set
> > > ENABLE ALWAYS TRIGGER.
> > >
> > > Do we really want to take that case on as our security responsibility?
> >
> > That's something about which I would like to get more opinions.
>
> The most-plausible-to-me attack involves an ENABLE ALWAYS trigger that logs
> CURRENT_USER to an audit table.  The "SQL based on the row contents" scenario
> feels remote.  Another remotely-possible attack involves a trigger that
> internally queries some other table having RLS.  (Switching to the table owner
> can change the rows visible to that query.)

I had thought of the first of these cases, but not the second one.

> If having INSERT/UPDATE privileges on the table were enough to make a
> subscription that impersonates the table owner, then relatively-unprivileged
> roles could make a subscription to bypass the aforementioned auditing.  Commit
> c3afe8c has imposed weighty requirements beyond I/U privileges, namely holding
> the pg_create_subscription role and database-level CREATE privilege.  Since
> database-level CREATE is already powerful, it would be plausible to drop the
> SET ROLE requirement and add this audit bypass to its powers.  The SET ROLE
> requirement is nice for keeping the powers disentangled.  One drawback is
> making people do GRANTs regardless of whether a relevant audit trigger exists.
> Another drawback is the subscription role having more privileges than ideally
> needed.  I do like keeping strong privileges orthogonal, so I lean toward
> keeping the SET ROLE requirement.

The orthogonality argument weighs extremely heavily with me in this
case. As I said to Jeff, I would not mind having a more granular way
to control which tables a user can replicate into; e.g. a grantable
REPLICAT{E,ION} privilege, or we want something global we could have a
predefined role for it, e.g. pg_replicate_into_any_table. But I think
any such thing should definitely be separate from
pg_create_subscription.

I'll fix the typos. Thanks for reporting them.

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

On Sun, 2023-04-02 at 20:21 -0700, Noah Misch wrote:
> The most-plausible-to-me attack involves an ENABLE ALWAYS trigger
> that logs
> CURRENT_USER to an audit table.

How does requiring that the subscription owner have SET ROLE privileges
on the table owner help that case? As Robert pointed out, users coming
from v15 will have superuser subscription owners anyway, so the change
will be silent for them.

We need support to apply changes as the table owner, and we need that
to be the default; and this patch provides those things, and almost all
users of logical replication will be better off after this is
committed.

The small number of users for whom this new model is not good still
need the right documentation in front of them to understand the
consequences, so that they can opt out one way or another (as 0002
offers). Release notes are probably the most powerful tool we have for
notifying users, unfortunately. Requiring SET ROLE for users that are
almost certainly superusers doesn't give an opportunity to educate
people about the change in behavior.

As I said before, I'm fine with requiring that the subscription owner
can SET ROLE to the table owner for v16. It's the most conservative
choice and the most "correct" (in that no lesser privilege we have
today is a perfect match).

But I feel like we can do better in version 17 when we have time to
actually work through common use cases and the exceptional cases and
weight them appropriately. Like, how common is it to want to get the
user from a trigger on the subscriber side? Should that trigger be
using SESSION_USER instead of CURRENT_USER? Security is best when it
takes into account what people actually want to do and makes it easy to
do that securely.

Regards,
    Jeff Davis

On Mon, 2023-04-03 at 10:26 -0400, Robert Haas wrote:
> Not very much. I think the biggest risk is user confusion, but I
> don't
> think that's a huge risk because I don't think this scenario will
> come
> up very often. Also, it's kind of hard to imagine that there's a
> security model here which never does anything potentially surprising.

Alright, let's just proceed as-is then. I believe these patches are a
major improvement to the usability of logical replication and will put
up with the weirdness. I wanted to understand better why it's there,
and I'm not sure I fully do, but we'll have more time to discuss later.

Regards,
    Jeff Davis

On Mon, Apr 03, 2023 at 12:05:29PM -0700, Jeff Davis wrote:
> On Sun, 2023-04-02 at 20:21 -0700, Noah Misch wrote:
> > The most-plausible-to-me attack involves an ENABLE ALWAYS trigger
> > that logs
> > CURRENT_USER to an audit table.
> 
> How does requiring that the subscription owner have SET ROLE privileges
> on the table owner help that case? As Robert pointed out, users coming
> from v15 will have superuser subscription owners anyway, so the change
> will be silent for them.

For subscriptions upgraded from v15, it doesn't matter.  Requiring SET ROLE
prevents this sequence:

- Make a table with such an audit trigger.  Grant INSERT and UPDATE to Alice.
- Upgrade to v15.
- Grant pg_create_subscription and database-level CREATE to Alice.
- Alice creates a subscription as a tool to impersonate the table owner,
  bypassing audit.

To put it another way, the benefit of the SET ROLE requirement is not really
making subscriptions more secure.  The benefit of the requirement is
pg_create_subscription not becoming a tool for bypassing audit.

I gather we agree on what to do for v16, which is good.

> But I feel like we can do better in version 17 when we have time to
> actually work through common use cases and the exceptional cases and
> weight them appropriately. Like, how common is it to want to get the
> user from a trigger on the subscriber side?

Fair.  I don't think the community has arrived at a usable approach for
answering questions like that.  It would be valuable to have an approach.

> Should that trigger be
> using SESSION_USER instead of CURRENT_USER?

Apart from evaluating the argument of SET ROLE, I've not heard of a valid use
case for SESSION_USER.

On Mon, Apr 3, 2023 at 10:09 PM Noah Misch <noah@leadboat.com> wrote:
> I gather we agree on what to do for v16, which is good.

I have committed the patches.

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

Hi hackers,
Thank you for developing a great feature. 
The following commit added a column to the pg_subscription catalog.
 https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=482675987bcdffb390ae735cfd5f34b485ae97c6
 https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c3afe8cf5a1e465bd71e48e4bc717f5bfdc7a7d6

I found that the documentation of the pg_subscription catalog was missing an explanation of the columns subrunasowner
andsubpasswordrequired, so I attached a patch. Please fix the patch if you have a better explanation.
 

Regards,
Noriyoshi Shinoda

-----Original Message-----
From: Robert Haas <robertmhaas@gmail.com> 
Sent: Wednesday, April 5, 2023 1:10 AM
To: Noah Misch <noah@leadboat.com>
Cc: Jeff Davis <pgsql@j-davis.com>; Jelte Fennema <postgres@jeltef.nl>; pgsql-hackers@postgresql.org; Andres Freund
<andres@anarazel.de>
Subject: Re: running logical replication as the subscription owner

On Mon, Apr 3, 2023 at 10:09 PM Noah Misch <noah@leadboat.com> wrote:
> I gather we agree on what to do for v16, which is good.

I have committed the patches.

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

On Mon, Apr 10, 2023 at 10:09 PM Shinoda, Noriyoshi (PN Japan FSIP)
<noriyoshi.shinoda@hpe.com> wrote:
> Hi hackers,
> Thank you for developing a great feature.
> The following commit added a column to the pg_subscription catalog.
>  https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=482675987bcdffb390ae735cfd5f34b485ae97c6
>  https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c3afe8cf5a1e465bd71e48e4bc717f5bfdc7a7d6
>
> I found that the documentation of the pg_subscription catalog was missing an explanation of the columns subrunasowner
andsubpasswordrequired, so I attached a patch. Please fix the patch if you have a better explanation. 

Thank you. Committed.

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

On Tue, Apr 4, 2023 at 9:40 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, Apr 3, 2023 at 10:09 PM Noah Misch <noah@leadboat.com> wrote:
> > I gather we agree on what to do for v16, which is good.
>
> I have committed the patches.
>

Do we want the initial sync to also respect 'run_as_owner' option? I
might be missing something but I don't see anything in the docs about
initial sync interaction with this option. In the commit a2ab9c06ea,
we did the permission checking during the initial sync so I thought we
should do it here as well.

--
With Regards,
Amit Kapila.

On Thu, May 11, 2023 at 7:38 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> Do we want the initial sync to also respect 'run_as_owner' option? I
> might be missing something but I don't see anything in the docs about
> initial sync interaction with this option. In the commit a2ab9c06ea,
> we did the permission checking during the initial sync so I thought we
> should do it here as well.

It definitely should work that way. lf it doesn't, that's a bug.

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

On Fri, May 12, 2023 at 1:12 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, May 11, 2023 at 7:38 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > Do we want the initial sync to also respect 'run_as_owner' option? I
> > might be missing something but I don't see anything in the docs about
> > initial sync interaction with this option. In the commit a2ab9c06ea,
> > we did the permission checking during the initial sync so I thought we
> > should do it here as well.
>
> It definitely should work that way. lf it doesn't, that's a bug.

After some tests, it seems that the initial sync worker respects
'run_as_owner' during catching up but not during COPYing.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Fri, May 12, 2023 at 9:10 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, May 12, 2023 at 1:12 AM Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > On Thu, May 11, 2023 at 7:38 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > Do we want the initial sync to also respect 'run_as_owner' option? I
> > > might be missing something but I don't see anything in the docs about
> > > initial sync interaction with this option. In the commit a2ab9c06ea,
> > > we did the permission checking during the initial sync so I thought we
> > > should do it here as well.
> >
> > It definitely should work that way. lf it doesn't, that's a bug.
>
> After some tests, it seems that the initial sync worker respects
> 'run_as_owner' during catching up but not during COPYing.
>

Yeah, I was worried during copy phase only. During catchup, the code
is common with apply worker code, so it will work.

--
With Regards,
Amit Kapila.

On Fri, May 12, 2023 at 1:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, May 12, 2023 at 9:10 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Fri, May 12, 2023 at 1:12 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > >
> > > On Thu, May 11, 2023 at 7:38 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > Do we want the initial sync to also respect 'run_as_owner' option? I
> > > > might be missing something but I don't see anything in the docs about
> > > > initial sync interaction with this option. In the commit a2ab9c06ea,
> > > > we did the permission checking during the initial sync so I thought we
> > > > should do it here as well.
> > >
> > > It definitely should work that way. lf it doesn't, that's a bug.
> >
> > After some tests, it seems that the initial sync worker respects
> > 'run_as_owner' during catching up but not during COPYing.
> >
>
> Yeah, I was worried during copy phase only. During catchup, the code
> is common with apply worker code, so it will work.
>

I tried the following test:

====================
Repeat On the publisher and subscriber:
 /* Create role regress_alice with  NOSUPERUSER on
   publisher and subscriber and a table for replication */

CREATE ROLE regress_alice NOSUPERUSER LOGIN;
CREATE ROLE regress_admin SUPERUSER LOGIN;
GRANT CREATE ON DATABASE postgres TO regress_alice;
SET SESSION AUTHORIZATION regress_alice;
CREATE SCHEMA alice;
GRANT USAGE ON SCHEMA alice TO regress_admin;
CREATE TABLE alice.test (i INTEGER);
ALTER TABLE alice.test REPLICA IDENTITY FULL;

On the publisher:
postgres=> insert into alice.test values(1);
postgres=> insert into alice.test values(2);
postgres=> insert into alice.test values(3);
postgres=> CREATE PUBLICATION alice FOR TABLE alice.test
WITH (publish_via_partition_root = true);

On the subscriber: /* create table admin_audit which regress_alice
does not have access to */
SET SESSION AUTHORIZATION regress_admin;
create table admin_audit (i integer);

On the subscriber: /* Create a trigger for table alice.test which
inserts on table admin_audit which the table owner of alice.test does
not have access to */
SET SESSION AUTHORIZATION regress_alice;
CREATE OR REPLACE FUNCTION alice.alice_audit()
RETURNS trigger AS
$$
BEGIN
insert into public.admin_audit values(2);
RETURN NEW;
END;
$$
LANGUAGE 'plpgsql';
create trigger test_alice after insert on alice.test for each row
execute procedure alice.alice_audit();
alter table alice.test enable always trigger test_alice;

On the subscriber: /* Create a subscription with run_as_owner = false */
CREATE SUBSCRIPTION admin_sub CONNECTION 'dbname=postgres
host=localhost port=6972' PUBLICATION alice WITH (run_as_owner =
false);
===============

What I see is that as part of tablesync, the trigger invokes an
updates admin_audit which it shouldn't, as the table owner
of alice.test should not have access to the
table admin_audit. This means the table copy is being invoked as the
subscription owner and not the table owner.

However, I see subsequent inserts fail on replication with
permission denied error, so the apply worker correctly
applies the inserts as the table owner.

If nobody else is working on this, I can come up with a patch to fix this

regards,
Ajin Cherian
Fujitsu Australia

On Fri, May 12, 2023 at 9:55 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> If nobody else is working on this, I can come up with a patch to fix this
>

Attaching a patch which attempts to fix this.

regards,
Ajin Cherian
Fujitsu Australia

On Mon, May 15, 2023 at 5:44 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Fri, May 12, 2023 at 9:55 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > If nobody else is working on this, I can come up with a patch to fix this
> >
>
> Attaching a patch which attempts to fix this.
>

Thank you for the patch! I think we might want to have tests for it.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Mon, 15 May 2023 at 14:47, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> Thank you for the patch! I think we might want to have tests for it.

Yes, code looks good. But indeed some tests would be great. It seems
we forgot to actually do:

On Fri, 12 May 2023 at 13:55, Ajin Cherian <itsajin@gmail.com> wrote:
> CREATE ROLE regress_alice NOSUPERUSER LOGIN;
> CREATE ROLE regress_admin SUPERUSER LOGIN;
> ...
>
> What I see is that as part of tablesync, the trigger invokes an
> updates admin_audit which it shouldn't, as the table owner
> of alice.test should not have access to the
> table admin_audit. This means the table copy is being invoked as the
> subscription owner and not the table owner.

I think having this as a tap/regress test would be very useful.

On Fri, 24 Mar 2023 at 19:37, Robert Haas <robertmhaas@gmail.com> wrote:
> > > > I think there's some important tests missing related to this:
> > > > 1. Ensuring that SECURITY_RESTRICTED_OPERATION things are enforced
> > > > when the user **does not** have SET ROLE permissions to the
> > > > subscription owner, e.g. don't allow SET ROLE from a trigger.
> > > > 2. Ensuring that SECURITY_RESTRICTED_OPERATION things are not enforced
> > > > when the user **does** have SET ROLE permissions to the subscription
> > > > owner, e.g. allows SET ROLE from trigger.
> > > Yeah, if we stick with the current approach we should probably add
> > > tests for that stuff.
> >
> > Even if we don't, we should still have tests showing that the security restrictions that we intend to put in place
actuallydo their job.
 
>
> Yeah, I just don't want to write the tests and then decide to change
> the behavior and then have to write them over again. It's not so much
> fun that I'm yearning to do it twice.

I forgot to follow up on this before, but based on the bug found by
Amit. I think it would be good to still add these tests.

On Fri, May 12, 2023 at 5:25 PM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Fri, May 12, 2023 at 1:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
>
> I tried the following test:
>
> ====================
> Repeat On the publisher and subscriber:
>  /* Create role regress_alice with  NOSUPERUSER on
>    publisher and subscriber and a table for replication */
>
> CREATE ROLE regress_alice NOSUPERUSER LOGIN;
> CREATE ROLE regress_admin SUPERUSER LOGIN;
> GRANT CREATE ON DATABASE postgres TO regress_alice;
> SET SESSION AUTHORIZATION regress_alice;
> CREATE SCHEMA alice;
> GRANT USAGE ON SCHEMA alice TO regress_admin;
> CREATE TABLE alice.test (i INTEGER);
> ALTER TABLE alice.test REPLICA IDENTITY FULL;
>

Why do we need a schema and following grant statement for this test?

> On the publisher:
> postgres=> insert into alice.test values(1);
> postgres=> insert into alice.test values(2);
> postgres=> insert into alice.test values(3);
> postgres=> CREATE PUBLICATION alice FOR TABLE alice.test
> WITH (publish_via_partition_root = true);
>

Again, 'publish_via_partition_root' doesn't seem to be required. Let's
try to write a minimal test for the initial sync behaviour.

--
With Regards,
Amit Kapila.

On Mon, May 15, 2023 at 7:24 PM Jelte Fennema <postgres@jeltef.nl> wrote:
>
> On Mon, 15 May 2023 at 14:47, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > Thank you for the patch! I think we might want to have tests for it.
>
> Yes, code looks good. But indeed some tests would be great. It seems
> we forgot to actually do:
>

Agreed with you and Sawada-San about having a test. BTW, shall we
slightly tweak the documentation [1]: "The subscription apply process
will, at a session level, run with the privileges of the subscription
owner. However, when performing an insert, update, delete, or truncate
operation on a particular table, it will switch roles to the table
owner and perform the operation with the table owner's privileges." to
be bit more specific about initial sync process as well?

[1] - https://www.postgresql.org/docs/devel/logical-replication-security.html

--
With Regards,
Amit Kapila.

On Tue, May 16, 2023 at 2:39 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> Agreed with you and Sawada-San about having a test. BTW, shall we
> slightly tweak the documentation [1]: "The subscription apply process
> will, at a session level, run with the privileges of the subscription
> owner. However, when performing an insert, update, delete, or truncate
> operation on a particular table, it will switch roles to the table
> owner and perform the operation with the table owner's privileges." to
> be bit more specific about initial sync process as well?

It doesn't seem entirely necessary to me because the initial sync is
in effect a bunch of inserts.

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

On Mon, May 15, 2023 at 10:47 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, May 15, 2023 at 5:44 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > On Fri, May 12, 2023 at 9:55 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > If nobody else is working on this, I can come up with a patch to fix this
> > >
> >
> > Attaching a patch which attempts to fix this.
> >
>
> Thank you for the patch! I think we might want to have tests for it.
>
I have updated the patch with a test case as well.

regards,
Ajin Cherian
Fujitsu Australia

On Wed, May 17, 2023 at 10:10 AM Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Mon, May 15, 2023 at 10:47 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, May 15, 2023 at 5:44 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > >
> > > On Fri, May 12, 2023 at 9:55 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > > >
> > > > If nobody else is working on this, I can come up with a patch to fix this
> > > >
> > >
> > > Attaching a patch which attempts to fix this.
> > >
> >
> > Thank you for the patch! I think we might want to have tests for it.
> >
> I have updated the patch with a test case as well.

Thank you for updating the patch! Here are review comments:

+       /*
+        * Make sure that the copy command runs as the table owner, unless
+        * the user has opted out of that behaviour.
+        */
+       run_as_owner = MySubscription->runasowner;
+       if (!run_as_owner)
+               SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt);
+
        /* Now do the initial data copy */
        PushActiveSnapshot(GetTransactionSnapshot());

I think we should switch users before the acl check in
LogicalRepSyncTableStart().

---
+# Create a trigger on table alice.unpartitioned that writes
+# to a table that regress_alice does not have permission.
+$node_subscriber->safe_psql(
+               'postgres', qq(
+SET SESSION AUTHORIZATION regress_alice;
+CREATE OR REPLACE FUNCTION alice.alice_audit()
+RETURNS trigger AS
+\$\$
+       BEGIN
+       insert into public.admin_audit values(2);
+       RETURN NEW;
+       END;
+\$\$
+LANGUAGE 'plpgsql';
+CREATE TRIGGER ALICE_TRIGGER AFTER INSERT ON alice.unpartitioned FOR EACH ROW
+EXECUTE PROCEDURE alice.alice_audit();
+ALTER TABLE alice.unpartitioned ENABLE ALWAYS TRIGGER ALICE_TRIGGER;
+));

While this approach works, I'm not sure we really need a trigger for
this test. I've attached a patch for discussion that doesn't use
triggers for the regression tests. We create a new subscription owned
by a user who doesn't have the permission to the target table. The
test passes only if run_as_owner = true works.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Thank you for updating the patch! Here are review comments:
>
> +       /*
> +        * Make sure that the copy command runs as the table owner, unless
> +        * the user has opted out of that behaviour.
> +        */
> +       run_as_owner = MySubscription->runasowner;
> +       if (!run_as_owner)
> +               SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt);
> +
>         /* Now do the initial data copy */
>         PushActiveSnapshot(GetTransactionSnapshot());
>
> I think we should switch users before the acl check in
> LogicalRepSyncTableStart().
>

Agreed, we should check acl with the user that is going to perform
operations on the target table. BTW, is it okay to perform an
operation on the system table with the changed user as that would be
possible with your suggestion (see replorigin_create())?

>
> While this approach works, I'm not sure we really need a trigger for
> this test. I've attached a patch for discussion that doesn't use
> triggers for the regression tests. We create a new subscription owned
> by a user who doesn't have the permission to the target table. The
> test passes only if run_as_owner = true works.
>

Why in the test do you need to give additional permissions to
regress_admin2 when the actual operation has to be performed by the
table owner?

+# Because the initial data sync is working as the table owner, all
+# dat should be copied.

Typo. /dat/data

--
With Regards,
Amit Kapila.

On Mon, May 22, 2023 at 10:36 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, May 17, 2023 at 10:10 AM Ajin Cherian <itsajin@gmail.com> wrote:
> >
> > On Mon, May 15, 2023 at 10:47 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Mon, May 15, 2023 at 5:44 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > > >
> > > > On Fri, May 12, 2023 at 9:55 PM Ajin Cherian <itsajin@gmail.com> wrote:
> > > > >
> > > > > If nobody else is working on this, I can come up with a patch to fix this
> > > > >
> > > >
> > > > Attaching a patch which attempts to fix this.
> > > >
> > >
> > > Thank you for the patch! I think we might want to have tests for it.
> > >
> > I have updated the patch with a test case as well.
>
> Thank you for updating the patch! Here are review comments:
>
> +       /*
> +        * Make sure that the copy command runs as the table owner, unless
> +        * the user has opted out of that behaviour.
> +        */
> +       run_as_owner = MySubscription->runasowner;
> +       if (!run_as_owner)
> +               SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt);
> +
>         /* Now do the initial data copy */
>         PushActiveSnapshot(GetTransactionSnapshot());
>
> I think we should switch users before the acl check in
> LogicalRepSyncTableStart().
>
> ---
> +# Create a trigger on table alice.unpartitioned that writes
> +# to a table that regress_alice does not have permission.
> +$node_subscriber->safe_psql(
> +               'postgres', qq(
> +SET SESSION AUTHORIZATION regress_alice;
> +CREATE OR REPLACE FUNCTION alice.alice_audit()
> +RETURNS trigger AS
> +\$\$
> +       BEGIN
> +       insert into public.admin_audit values(2);
> +       RETURN NEW;
> +       END;
> +\$\$
> +LANGUAGE 'plpgsql';
> +CREATE TRIGGER ALICE_TRIGGER AFTER INSERT ON alice.unpartitioned FOR EACH ROW
> +EXECUTE PROCEDURE alice.alice_audit();
> +ALTER TABLE alice.unpartitioned ENABLE ALWAYS TRIGGER ALICE_TRIGGER;
> +));
>
> While this approach works, I'm not sure we really need a trigger for
> this test. I've attached a patch for discussion that doesn't use
> triggers for the regression tests. We create a new subscription owned
> by a user who doesn't have the permission to the target table. The
> test passes only if run_as_owner = true works.
>
this is better, thanks. Since you are testing run_as_owner = false behaviour
during table copy phase, you might as well add a test case that it
correctly behaves during insert replication as well.

regards,
Ajin Cherian
Fujitsu Australia

On Tue, May 23, 2023 at 8:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > Thank you for updating the patch! Here are review comments:
> >
> > +       /*
> > +        * Make sure that the copy command runs as the table owner, unless
> > +        * the user has opted out of that behaviour.
> > +        */
> > +       run_as_owner = MySubscription->runasowner;
> > +       if (!run_as_owner)
> > +               SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt);
> > +
> >         /* Now do the initial data copy */
> >         PushActiveSnapshot(GetTransactionSnapshot());
> >
> > I think we should switch users before the acl check in
> > LogicalRepSyncTableStart().
> >
>
> Agreed, we should check acl with the user that is going to perform
> operations on the target table. BTW, is it okay to perform an
> operation on the system table with the changed user as that would be
> possible with your suggestion (see replorigin_create())?

Do you see any problem in particular?

As per the documentation, pg_replication_origin_create() is only
allowed to the superuser by default, but in CreateSubscription() a
non-superuser (who has pg_create_subscription privilege) can call
replorigin_create(). OTOH, we don't necessarily need to switch to the
table owner user for checking ACL and RLS. We can just pass either
table owner OID or subscription owner OID to pg_class_aclcheck() and
check_enable_rls() without actually switching the user.

>
> >
> > While this approach works, I'm not sure we really need a trigger for
> > this test. I've attached a patch for discussion that doesn't use
> > triggers for the regression tests. We create a new subscription owned
> > by a user who doesn't have the permission to the target table. The
> > test passes only if run_as_owner = true works.
> >
>
> Why in the test do you need to give additional permissions to
> regress_admin2 when the actual operation has to be performed by the
> table owner?

Good point. We need to give the ability to SET ROLE to regress_admin2
but other permissions are unnecessary.

>
> +# Because the initial data sync is working as the table owner, all
> +# dat should be copied.
>
> Typo. /dat/data

Will fix.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Thu, May 25, 2023 at 12:33 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, May 23, 2023 at 8:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > Thank you for updating the patch! Here are review comments:
> > >
> > > +       /*
> > > +        * Make sure that the copy command runs as the table owner, unless
> > > +        * the user has opted out of that behaviour.
> > > +        */
> > > +       run_as_owner = MySubscription->runasowner;
> > > +       if (!run_as_owner)
> > > +               SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt);
> > > +
> > >         /* Now do the initial data copy */
> > >         PushActiveSnapshot(GetTransactionSnapshot());
> > >
> > > I think we should switch users before the acl check in
> > > LogicalRepSyncTableStart().
> > >
> >
> > Agreed, we should check acl with the user that is going to perform
> > operations on the target table. BTW, is it okay to perform an
> > operation on the system table with the changed user as that would be
> > possible with your suggestion (see replorigin_create())?
>
> Do you see any problem in particular?
>
> As per the documentation, pg_replication_origin_create() is only
> allowed to the superuser by default, but in CreateSubscription() a
> non-superuser (who has pg_create_subscription privilege) can call
> replorigin_create().

Nothing in particular but it seems a bit odd to perform operations on
catalog tables with some other user table owners when that was not the
actual intent of this option.

> OTOH, we don't necessarily need to switch to the
> table owner user for checking ACL and RLS. We can just pass either
> table owner OID or subscription owner OID to pg_class_aclcheck() and
> check_enable_rls() without actually switching the user.
>

I think that would be better.

--
With Regards,
Amit Kapila.

On Thu, May 25, 2023 at 5:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, May 25, 2023 at 12:33 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Tue, May 23, 2023 at 8:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > Thank you for updating the patch! Here are review comments:
> > > >
> > > > +       /*
> > > > +        * Make sure that the copy command runs as the table owner, unless
> > > > +        * the user has opted out of that behaviour.
> > > > +        */
> > > > +       run_as_owner = MySubscription->runasowner;
> > > > +       if (!run_as_owner)
> > > > +               SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt);
> > > > +
> > > >         /* Now do the initial data copy */
> > > >         PushActiveSnapshot(GetTransactionSnapshot());
> > > >
> > > > I think we should switch users before the acl check in
> > > > LogicalRepSyncTableStart().
> > > >
> > >
> > > Agreed, we should check acl with the user that is going to perform
> > > operations on the target table. BTW, is it okay to perform an
> > > operation on the system table with the changed user as that would be
> > > possible with your suggestion (see replorigin_create())?
> >
> > Do you see any problem in particular?
> >
> > As per the documentation, pg_replication_origin_create() is only
> > allowed to the superuser by default, but in CreateSubscription() a
> > non-superuser (who has pg_create_subscription privilege) can call
> > replorigin_create().
>
> Nothing in particular but it seems a bit odd to perform operations on
> catalog tables with some other user table owners when that was not the
> actual intent of this option.
>
> > OTOH, we don't necessarily need to switch to the
> > table owner user for checking ACL and RLS. We can just pass either
> > table owner OID or subscription owner OID to pg_class_aclcheck() and
> > check_enable_rls() without actually switching the user.
> >
>
> I think that would be better.

Agreed.

I've attached the updated patch. Please review it.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, May 25, 2023 at 5:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> I've attached the updated patch. Please review it.
>

Few comments:
1.
+ /* get the owner for ACL and RLS checks */
+ run_as_owner = MySubscription->runasowner;
+ checkowner = run_as_owner ? MySubscription->owner : rel->rd_rel->relowner;
+
  /*
  * Check that our table sync worker has permission to insert into the
  * target table.
  */
- aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
+ aclresult = pg_class_aclcheck(RelationGetRelid(rel), checkowner,

One thing that slightly worries me about this change is that we
started to check the permission for relowner before even ensuring that
we can switch to relowner. See checks in SwitchToUntrustedUser(). If
we want to first ensure that we can switch to relowner then I think we
should move this permission-checking code before we try to copy the
table.

2. In the commit message, the link for discussion
"https://postgr.es/m/CAA4eK1KfZcRq7hUqQ7WknP+u=08+6MevVm+2W5RrAb+DTxrdww@mail.gmail.com"
is slightly misleading. Can we instead use
"https://www.postgresql.org/message-id/CAA4eK1L%3DqzRHPEn%2BqeMoKQGFBzqGoLBzt_ov0A89iFFiut%2BppA%40mail.gmail.com"?

--
With Regards,
Amit Kapila.

On Mon, Jun 5, 2023 at 3:15 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Thu, May 25, 2023 at 5:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > I've attached the updated patch. Please review it.
> >
>
> Few comments:
> 1.
> + /* get the owner for ACL and RLS checks */
> + run_as_owner = MySubscription->runasowner;
> + checkowner = run_as_owner ? MySubscription->owner : rel->rd_rel->relowner;
> +
>   /*
>   * Check that our table sync worker has permission to insert into the
>   * target table.
>   */
> - aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
> + aclresult = pg_class_aclcheck(RelationGetRelid(rel), checkowner,
>
> One thing that slightly worries me about this change is that we
> started to check the permission for relowner before even ensuring that
> we can switch to relowner. See checks in SwitchToUntrustedUser(). If
> we want to first ensure that we can switch to relowner then I think we
> should move this permission-checking code before we try to copy the
> table.

Agreed. I thought it's better to do ACL and RLS checks before creating
the replication slot but it's not important. Rather checking them
after switching user would make sense since we do the same in
worker.c.

>
> 2. In the commit message, the link for discussion
> "https://postgr.es/m/CAA4eK1KfZcRq7hUqQ7WknP+u=08+6MevVm+2W5RrAb+DTxrdww@mail.gmail.com"
> is slightly misleading. Can we instead use
> "https://www.postgresql.org/message-id/CAA4eK1L%3DqzRHPEn%2BqeMoKQGFBzqGoLBzt_ov0A89iFFiut%2BppA%40mail.gmail.com"?

Agreed.

I've attached the updated patch.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Thu, Jun 8, 2023 at 6:32 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, Jun 5, 2023 at 3:15 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Thu, May 25, 2023 at 5:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > I've attached the updated patch. Please review it.
> > >
> >
> > Few comments:
> > 1.
> > + /* get the owner for ACL and RLS checks */
> > + run_as_owner = MySubscription->runasowner;
> > + checkowner = run_as_owner ? MySubscription->owner : rel->rd_rel->relowner;
> > +
> >   /*
> >   * Check that our table sync worker has permission to insert into the
> >   * target table.
> >   */
> > - aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
> > + aclresult = pg_class_aclcheck(RelationGetRelid(rel), checkowner,
> >
> > One thing that slightly worries me about this change is that we
> > started to check the permission for relowner before even ensuring that
> > we can switch to relowner. See checks in SwitchToUntrustedUser(). If
> > we want to first ensure that we can switch to relowner then I think we
> > should move this permission-checking code before we try to copy the
> > table.
>
> Agreed. I thought it's better to do ACL and RLS checks before creating
> the replication slot but it's not important. Rather checking them
> after switching user would make sense since we do the same in
> worker.c.
>

LGTM.

--
With Regards,
Amit Kapila.

On Thu, Jun 8, 2023 at 7:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Jun 8, 2023 at 6:32 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Mon, Jun 5, 2023 at 3:15 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Thu, May 25, 2023 at 5:41 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > I've attached the updated patch. Please review it.
> > > >
> > >
> > > Few comments:
> > > 1.
> > > + /* get the owner for ACL and RLS checks */
> > > + run_as_owner = MySubscription->runasowner;
> > > + checkowner = run_as_owner ? MySubscription->owner : rel->rd_rel->relowner;
> > > +
> > >   /*
> > >   * Check that our table sync worker has permission to insert into the
> > >   * target table.
> > >   */
> > > - aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
> > > + aclresult = pg_class_aclcheck(RelationGetRelid(rel), checkowner,
> > >
> > > One thing that slightly worries me about this change is that we
> > > started to check the permission for relowner before even ensuring that
> > > we can switch to relowner. See checks in SwitchToUntrustedUser(). If
> > > we want to first ensure that we can switch to relowner then I think we
> > > should move this permission-checking code before we try to copy the
> > > table.
> >
> > Agreed. I thought it's better to do ACL and RLS checks before creating
> > the replication slot but it's not important. Rather checking them
> > after switching user would make sense since we do the same in
> > worker.c.
> >
>
> LGTM.

Thanks, pushed.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com