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
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.
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
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 =
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
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
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
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.
> +*/
> +
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
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
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
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.
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
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
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
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
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
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.
>
>
-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
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,
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
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
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
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
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
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
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
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
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
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
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
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
> >
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
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
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
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
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
> 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
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
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.
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
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?
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
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
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
> 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
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
> 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
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
> 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
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
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?
>
> 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
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
73 matches
Mail list logo
