Re: Non-superuser subscription owners

> On Nov 24, 2021, at 4:30 PM, Jeff Davis <pgsql(at)j-davis(dot)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 agree this is a problem for subscriptions owned by non-superusers.

The implementation of the table sync worker uses COPY FROM, which makes this problem hard to fix, because COPY FROM does not support row level security. We could do some work to honor the RLS policies during the apply workers' INSERT statements, but then some data would circumvent RLS during table sync and other data would honor RLS during worker apply, which would make the implementation not only wrong but inconsistently so.

I think a more sensible approach for v15 is to raise an ERROR if a non-superuser owned subscription is trying to replicate into a table which has RLS enabled. We might try to be more clever and check whether the RLS policies could possibly reject the operation (by comparing the TO and FOR clauses of the policies against the role and operation type) but that seems like a partial re-implementation of RLS. It would be simpler and more likely correct if we just unconditionally reject replicating into tables which have RLS enabled.

What do you think?

> A couple other points:
>
> * We shouldn't refer to the behavior of previous versions in the docs
> unless there's a compelling reason

Fair enough.

> * 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 expect the 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.

> * 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 logical replication 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 target table 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?

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