Re: Non-superuser subscription owners

On Sat, Nov 27, 2021 at 11:37 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
>
>
>
> > On Nov 24, 2021, at 4:30 PM, Jeff Davis <pgsql@j-davis.com> wrote:
> >
> > We need to do permission checking for WITH CHECK OPTION and RLS. The
> > patch right now allows the subscription to write data that an RLS
> > policy forbids.
>
> Thanks for reviewing and for this observation!  I can verify that RLS is not being honored on the subscriber side.  I
agreethis is a problem for subscriptions owned by non-superusers. 
>
...
>
> > A couple other points:
> >
>
> > * Do we need to be smarter about partitioned tables, where an insert
> > can turn into an update?
>
> Do you mean an INSERT statement with an ON CONFLICT DO UPDATE clause that is running against a partitioned table?  If
so,I don't think we need to handle that on the subscriber side under the current logical replication design.  I would
expectthe plain INSERT or UPDATE that ultimately executes on the publisher to be what gets replicated to the
subscriber,and not the original INSERT .. ON CONFLICT DO UPDATE statement. 
>

Yeah, that is correct but I think the update case is more relevant
here. In ExecUpdate(), we convert Update to DELETE+INSERT when the
partition constraint is failed whereas, on the subscriber-side, it
will simply fail in this case. It is not clear to me how that is
directly related to this patch but surely it will be a good
improvement on its own and might help if that requires us to change
some infrastructure here like hooking into executor at a higher level.

> > * Should we refactor to borrow logic from ExecInsert so that it's less
> > likely that we miss something in the future?
>
> Hooking into the executor at a higher level, possibly ExecInsert or ExecModifyTable would do a lot more than what
logicalreplication currently does.  If we also always used INSERT/UPDATE/DELETE statements and never COPY FROM
statements,we might solve several problems at once, including honoring RLS policies and honoring rules defined for the
targettable on the subscriber side. 
>
> Doing this would clearly be a major design change, and possibly one we do not want.  Can we consider this out of
scope?
>

I agree that if we want to do all of this then that would require a
lot of changes. However, giving an error for RLS-enabled tables might
also be too restrictive. The few alternatives could be that (a) we
allow subscription owners to be either have "bypassrls" attribute or
they could be superusers. (b) don't allow initial table_sync for rls
enabled tables. (c) evaluate/analyze what is required to allow Copy
From to start respecting RLS policies. (d) reject replicating any
changes to tables that have RLS enabled.

I see that you are favoring (d) which clearly has merits like lesser
code/design change but not sure if that is the best way forward or we
can do something better than that either by following one of (a), (b),
(c), or something less restrictive than (d).

--
With Regards,
Amit Kapila.