On Thu, Jun 8, 2023 at 7:29 PM Amit Kapila wrote:
>
> On Thu, Jun 8, 2023 at 6:32 AM Masahiko Sawada wrote:
> >
> > On Mon, Jun 5, 2023 at 3:15 AM Amit Kapila wrote:
> > >
> > > On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada
> > > wrote:
> > > >
> > > > On Thu, May 25, 2023 at 5:41 PM Amit Ka
On Thu, Jun 8, 2023 at 6:32 AM Masahiko Sawada wrote:
>
> On Mon, Jun 5, 2023 at 3:15 AM Amit Kapila wrote:
> >
> > On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada
> > wrote:
> > >
> > > On Thu, May 25, 2023 at 5:41 PM Amit Kapila
> > > wrote:
> > >
> > > I've attached the updated patch. Plea
On Mon, Jun 5, 2023 at 3:15 AM Amit Kapila wrote:
>
> On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada wrote:
> >
> > On Thu, May 25, 2023 at 5:41 PM Amit Kapila wrote:
> >
> > I've attached the updated patch. Please review it.
> >
>
> Few comments:
> 1.
> + /* get the owner for ACL and RLS check
On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada wrote:
>
> On Thu, May 25, 2023 at 5:41 PM Amit Kapila 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_
On Thu, May 25, 2023 at 5:41 PM Amit Kapila wrote:
>
> On Thu, May 25, 2023 at 12:33 PM Masahiko Sawada
> wrote:
> >
> > On Tue, May 23, 2023 at 8:21 PM Amit Kapila wrote:
> > >
> > > On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada
> > > wrote:
> > > >
> > > > Thank you for updating the patch
On Thu, May 25, 2023 at 12:33 PM Masahiko Sawada wrote:
>
> On Tue, May 23, 2023 at 8:21 PM Amit Kapila wrote:
> >
> > On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada
> > wrote:
> > >
> > > Thank you for updating the patch! Here are review comments:
> > >
> > > + /*
> > > +* Make
On Tue, May 23, 2023 at 8:21 PM Amit Kapila wrote:
>
> On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada wrote:
> >
> > Thank you for updating the patch! Here are review comments:
> >
> > + /*
> > +* Make sure that the copy command runs as the table owner, unless
> > +* the us
On Mon, May 22, 2023 at 10:36 PM Masahiko Sawada wrote:
>
> On Wed, May 17, 2023 at 10:10 AM Ajin Cherian wrote:
> >
> > On Mon, May 15, 2023 at 10:47 PM Masahiko Sawada
> > wrote:
> > >
> > > On Mon, May 15, 2023 at 5:44 PM Ajin Cherian wrote:
> > > >
> > > > On Fri, May 12, 2023 at 9:55 PM A
On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada 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_o
On Wed, May 17, 2023 at 10:10 AM Ajin Cherian wrote:
>
> On Mon, May 15, 2023 at 10:47 PM Masahiko Sawada
> wrote:
> >
> > On Mon, May 15, 2023 at 5:44 PM Ajin Cherian wrote:
> > >
> > > On Fri, May 12, 2023 at 9:55 PM Ajin Cherian wrote:
> > > >
> > > > If nobody else is working on this, I ca
On Mon, May 15, 2023 at 10:47 PM Masahiko Sawada wrote:
>
> On Mon, May 15, 2023 at 5:44 PM Ajin Cherian wrote:
> >
> > On Fri, May 12, 2023 at 9:55 PM Ajin Cherian wrote:
> > >
> > > If nobody else is working on this, I can come up with a patch to fix this
> > >
> >
> > Attaching a patch which
On Tue, May 16, 2023 at 2:39 AM Amit Kapila 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 a
On Mon, May 15, 2023 at 7:24 PM Jelte Fennema wrote:
>
> On Mon, 15 May 2023 at 14:47, Masahiko Sawada 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 wit
On Fri, May 12, 2023 at 5:25 PM Ajin Cherian wrote:
>
> On Fri, May 12, 2023 at 1:49 PM Amit Kapila wrote:
> >
>
> I tried the following test:
>
>
> Repeat On the publisher and subscriber:
> /* Create role regress_alice with NOSUPERUSER on
>publisher and subscriber and
On Fri, 24 Mar 2023 at 19:37, Robert Haas 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
On Mon, 15 May 2023 at 14:47, Masahiko Sawada 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 wrote:
> CREATE ROLE regress_alic
On Mon, May 15, 2023 at 5:44 PM Ajin Cherian wrote:
>
> On Fri, May 12, 2023 at 9:55 PM Ajin Cherian 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
On Fri, May 12, 2023 at 9:55 PM Ajin Cherian 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
v1-0001-Fix-issue-where-the-copy-command-does-not-adhere-.patch
Description:
On Fri, May 12, 2023 at 1:49 PM Amit Kapila wrote:
>
> On Fri, May 12, 2023 at 9:10 AM Masahiko Sawada wrote:
> >
> > On Fri, May 12, 2023 at 1:12 AM Robert Haas wrote:
> > >
> > > On Thu, May 11, 2023 at 7:38 AM Amit Kapila
> > > wrote:
> > > > Do we want the initial sync to also respect 'run
On Fri, May 12, 2023 at 9:10 AM Masahiko Sawada wrote:
>
> On Fri, May 12, 2023 at 1:12 AM Robert Haas wrote:
> >
> > On Thu, May 11, 2023 at 7:38 AM Amit Kapila wrote:
> > > Do we want the initial sync to also respect 'run_as_owner' option? I
> > > might be missing something but I don't see any
On Fri, May 12, 2023 at 1:12 AM Robert Haas wrote:
>
> On Thu, May 11, 2023 at 7:38 AM Amit Kapila 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 optio
On Thu, May 11, 2023 at 7:38 AM Amit Kapila 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
On Tue, Apr 4, 2023 at 9:40 PM Robert Haas wrote:
>
> On Mon, Apr 3, 2023 at 10:09 PM Noah Misch 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
On Mon, Apr 10, 2023 at 10:09 PM Shinoda, Noriyoshi (PN Japan FSIP)
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=482675987bcdffb390ae735cfd
-Original Message-
From: Robert Haas
Sent: Wednesday, April 5, 2023 1:10 AM
To: Noah Misch
Cc: Jeff Davis ; Jelte Fennema ;
pgsql-hack...@postgresql.org; Andres Freund
Subject: Re: running logical replication as the subscription owner
On Mon, Apr 3, 2023 at 10:09 PM Noah Misch wrote:
> I gat
On Mon, Apr 3, 2023 at 10:09 PM Noah Misch 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 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 ROL
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
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, u
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 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 th
On Fri, Mar 31, 2023 at 6:46 PM Jeff Davis 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 sp
On Thu, Mar 30, 2023 at 7:12 PM Robert Haas 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
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 ma
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_owne
On Fri, Mar 31, 2023 at 3:36 AM Jeff Davis 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=+fo8ehk5edk...@mail.gmail.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 vu
On Thu, Mar 30, 2023 at 2:52 PM Jeff Davis 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
On Thu, 2023-03-30 at 09:41 -0400, Robert Haas wrote:
> On Thu, Mar 30, 2023 at 1:19 AM Jeff Davis 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
On Thu, Mar 30, 2023 at 11:11 AM Jelte Fennema 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
On Thu, 30 Mar 2023 at 15:42, Robert Haas wrote:
> On Thu, Mar 30, 2023 at 1:19 AM Jeff Davis 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 0
On Thu, Mar 30, 2023 at 1:19 AM Jeff Davis 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 imp
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 use
On Tue, Mar 28, 2023 at 6:48 PM Jeff Davis 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 tha
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 whe
On Tue, 28 Mar 2023 at 18:13, Robert Haas wrote:
>
> On Mon, Mar 27, 2023 at 6:08 PM Jelte Fennema 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
> > it
On Tue, Mar 28, 2023 at 1:38 AM Jeff Davis 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 easi
On Mon, Mar 27, 2023 at 6:08 PM Jelte Fennema 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 **cann
On Tue, 28 Mar 2023 at 11:15, Jelte Fennema 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 currentl
On Mon, 27 Mar 2023 at 22:47, Robert Haas 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.
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
pfun
> 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
On Mon, Mar 27, 2023 at 3:04 PM Jeff Davis 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_catalo
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.
On Sat, Mar 25, 2023 at 5:24 AM Jelte Fennema 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. T
On Sat, Mar 25, 2023 at 12:01 PM Noah Misch 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
On Fri, Mar 24, 2023 at 8:02 PM Jeff Davis 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?
Possi
On Fri, Mar 24, 2023 at 4:11 PM Mark Dilger
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.
On Fri, Mar 24, 2023 at 10:00:36AM -0400, Robert Haas wrote:
> On Fri, Mar 24, 2023 at 3:59 AM Jeff Davis 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
> 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
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 filt
On Fri, 2023-03-24 at 10:00 -0400, Robert Haas wrote:
> On Fri, Mar 24, 2023 at 3:59 AM Jeff Davis 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 ema
> On Mar 24, 2023, at 11:35 AM, Robert Haas 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 p
On Fri, Mar 24, 2023 at 2:14 PM Jelte Fennema 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 both behaviours. I mainly meant that if
> we actually think the new behaviour is better (which honestly I'm not
> convinced
On Fri, Mar 24, 2023 at 12:58 PM Mark Dilger
wrote:
> I also think the subscription owner should be a low-privileged user, owing to
> the risk of the publisher injecting malicious content into the publication.
> I think you are focused on all the bad actors on the subscription-side
> database
> 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
> On Mar 24, 2023, at 7:00 AM, Robert Haas 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 probl
On Fri, Mar 24, 2023 at 12:17 PM Jelte Fennema 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_OPER
> 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 sa
On Fri, Mar 24, 2023 at 3:59 AM Jeff Davis 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 do
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?
> Howev
> 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
On Fri, Mar 3, 2023 at 6:57 PM Jelte Fennema 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 subscr
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
73 matches
Mail list logo