Re: Commitfest 2021-11 Patch Triage - Part 3
On Tue, 2021-12-14 at 07:41 -0800, Mark Dilger wrote: > I went a different direction with this. The need is to prevent non- > superuser subscription owners from *circumventing* RLS. For this > patch set, I'm just checking whether RLS should be enforced against > the subscription owner, and if so, raising an ERROR, same as for a > privilege violation. This should be sufficient in practice, as the > table owner, roles with bypassrls privilege, and superusers should > still be able to replicate into an RLS enabled table. I agree with this. Let's consider subscription targets involving RLS a separate feature that is just blocked for now (except for bypassrls roles and superusers). > As far as completeness and a clean implementation (but not > performance) are concerned, using ExecInsert is quite appealing. I > think that would require reworking the logical replication protocol > to send more than one row at a time, so that the overhead of such a > choice is not paid *per row*. That seems quite out of scope for this > release cycle, though. Are you sure overhead is a problem? INSERT INTO ... SELECT goes through that path. And the existing logical replication insert path does some extra stuff, like opening the table for each row, so it's not clear to me that logical replication is much faster. Also, the current patch does per-row ACL checks. Did you consider making the checks a part of logicalrep_rel_open()? That already has a path to consider relcache invalidation, so it would also be a good place to check if the table has RLS enabled. Regards, Jeff Davis
Re: Commitfest 2021-11 Patch Triage - Part 3
> On Dec 13, 2021, at 10:18 PM, Jeff Davis wrote: > >>This is implemented but I still need to update the documentation >> before >>posting. > > We also discussed how much of the insert path we want to include in the > apply worker. The immediate need is for the patch to support RLS, but > it raised the question: "why not just use the entire ExecInsert() > path?". I went a different direction with this. The need is to prevent non-superuser subscription owners from *circumventing* RLS. For this patch set, I'm just checking whether RLS should be enforced against the subscription owner, and if so, raising an ERROR, same as for a privilege violation. This should be sufficient in practice, as the table owner, roles with bypassrls privilege, and superusers should still be able to replicate into an RLS enabled table. The problem with actually *supporting* RLS is that the current logical replication implementation is a mix of different practical (rather than theoretical) design choices. After working to get RLS working as part of that, I became concerned that there may be subtle differences between how I was making RLS work in logical replication vs. how it works for regular inserts/updates/deletes. Rather than create a mess that would need to be supported going forward, I bailed out and just forbade it. As far as completeness and a clean implementation (but not performance) are concerned, using ExecInsert is quite appealing. I think that would require reworking the logical replication protocol to send more than one row at a time, so that the overhead of such a choice is not paid *per row*. That seems quite out of scope for this release cycle, though. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Commitfest 2021-11 Patch Triage - Part 3
On Mon, 2021-12-13 at 11:20 -0800, Mark Dilger wrote: > - The logical replication subscription patch needs to consider RLS. > (Jeff Davis) > > This is implemented but I still need to update the documentation > before > posting. We also discussed how much of the insert path we want to include in the apply worker. The immediate need is for the patch to support RLS, but it raised the question: "why not just use the entire ExecInsert() path?". That's not a blocking issue for this patch, but something to consider that will avoid problems if we want to support updatable views or foreign tables as a target of a subscription. Regards, Jeff Davis
Re: Commitfest 2021-11 Patch Triage - Part 3
> On Nov 11, 2021, at 2:12 PM, Daniel Gustafsson wrote: > > 3223: Delegating superuser tasks to new security roles > == > There is a lot of ongoing discussion on this one, but skimming the thread it's > not really clear (to me) where it leaves the patch in question. This set of patches got a fair amount of in-person off-list discussion at the PGConf NYC with folks who had concerns about some or all parts of the patch. Most of the known contentious design details were resolved, and I've been updating the patches to repost this week. The subscriptions patch and guc patch should be close to ready. The CREATEROLE patch may take a bit longer. Conversation Highlights: - The logical replication subscription patch needs to consider RLS. (Jeff Davis) This is implemented but I still need to update the documentation before posting. - The patch for granting SET VALUE and/or ALTER SYSTEM on guc settings should be extended to allow revoking privilege to SET VALUE on userset gucs. (Joshua Brindle, Stephen Frost). This is implemented for core by means of converting PGC_USERSET variables to PGC_SUSET, with a corresponding "GRANT SET VALUE ON .. TO public" added to the new catalog/setting_privileges.sql file. By making it an explicit grant, it is revokable without the need for any special logic. This would allow, for example, REVOKE SET VALUE ON work_mem FROM public; followed by explicit grants to whichever roles you want to allow to set work_mem. I still need to adjust gucs defined in some contrib modules, update documentation, and verify pg_upgrade from older versions before posting. External projects are not harmed by this implementation. The only issue for them is that SET VALUE cannot be revoked from any PGC_USERSET gucs they define until they update their module to use PGC_SUSET plus a GRANT somewhere in their extension creation or upgrade sql script. - The CREATEROLE patch can go forward with roles having owners, but other spec compliant behavior for GRANT/REVOKE should be implemented. (Stephen Frost) The bug wherein the grantor field in pg_auth_members may point to a dropped role should be fixed. Probably this field should simply be dropped. We don't use it anywhere, and on upgrade we can't know if existing values are correct or bogus. Values that do not refer to any existing role are obviously bogus, but if a role Oid was reassigned, a grantor entry may be pointing wrongly at the new role, and there is no way to know that. Stephen indicated that, for all privileges, only the role which GRANTed a privilege should be able to REVOKE it. This differs from the current implementation and is likely controversial. Robert Haas, at least, described that idea as a non-starter. I don't plan to include that in this patch, as it doesn't seem necessary to combine the two features even if we eventually intend to do both. (And certainly unhelpful to include it if the idea is rejected.) Whether the owner of a role is implicitly a member (or admin) of the owned role is still disputed. Stephen Frost would rather the owner has the right to grant itself into the role, or grant itself ADMIN on the role, but not be a member nor an admin by default. Robert Haas preferred that the owner is both a member and an admin by default, making role ownership analogous to how superuser works. I lean slightly in Stephen's favor, as any role "foo" with CREATEROLE could always run: CREATE ROLE bar [ ROLE foo | ADMIN foo]; and thereby give themselves the privilege they want in the same command that creates the role. Going with Robert's proposal, a creator who does not want to have privileges in the role would need to run: CREATE ROLE bar; REVOKE [ADMIN OPTION FOR ] bar FROM foo; which requires two separate commands, and creates a short window inbetween where a (possibly undesireable) configuration exists. Jeff Davis and Andrew Dunstan have volunteered as committers. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
