On Thu, Mar 4, 2021 at 5:15 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> I took a quick look at this.
Thanks a lot for the review.
> I guess I'm disturbed by the idea
> that we'd totally replace the implementation technology for only one
> variant of foreign key checks. That means that there'll be a lot
> of minor details that don't act the same depending on context. One
> point I was just reminded of by [1] is that the SPI approach enforces
> permissions checks on the table access, which I do not see being done
> anywhere in your patch. Now, maybe it's fine not to have such checks,
> on the grounds that the existence of the RI constraint is sufficient
> permission (the creator had to have REFERENCES permission to make it).
> But I'm not sure about that. Should we add SELECT permissions checks
> to this code path to make it less different?
>
> In the same vein, the existing code actually runs the query as the
> table owner (cf. SetUserIdAndSecContext in ri_PerformCheck), another
> nicety you haven't bothered with. Maybe that is invisible for a
> pure SELECT query but I'm not sure I would bet on it. At the very
> least you're betting that the index-related operators you invoke
> aren't going to care, and that nobody is going to try to use this
> difference to create a security exploit via a trojan-horse index.
How about we do at the top of ri_ReferencedKeyExists() what
ri_PerformCheck() always does before executing a query, which is this:
/* Switch to proper UID to perform check as */
GetUserIdAndSecContext(&save_userid, &save_sec_context);
SetUserIdAndSecContext(RelationGetForm(query_rel)->relowner,
save_sec_context | SECURITY_LOCAL_USERID_CHANGE |
SECURITY_NOFORCE_RLS);
And then also check the permissions of the switched user on the scan
target relation's schema (ACL_USAGE) and the relation itself
(ACL_SELECT).
IOW, this:
+ Oid save_userid;
+ int save_sec_context;
+ AclResult aclresult;
+
+ /* Switch to proper UID to perform check as */
+ GetUserIdAndSecContext(&save_userid, &save_sec_context);
+ SetUserIdAndSecContext(RelationGetForm(pk_rel)->relowner,
+ save_sec_context | SECURITY_LOCAL_USERID_CHANGE |
+ SECURITY_NOFORCE_RLS);
+
+ /* Check namespace permissions. */
+ aclresult = pg_namespace_aclcheck(RelationGetNamespace(pk_rel),
+ GetUserId(), ACL_USAGE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, OBJECT_SCHEMA,
+ get_namespace_name(RelationGetNamespace(pk_rel)));
+ /* Check the user has SELECT permissions on the referenced relation. */
+ aclresult = pg_class_aclcheck(RelationGetRelid(pk_rel), GetUserId(),
+ ACL_SELECT);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, OBJECT_TABLE,
+ RelationGetRelationName(pk_rel));
/*
* Extract the unique key from the provided slot and choose the equality
@@ -414,6 +436,9 @@ ri_ReferencedKeyExists(Relation pk_rel, Relation fk_rel,
index_endscan(scan);
ExecDropSingleTupleTableSlot(outslot);
+ /* Restore UID and security context */
+ SetUserIdAndSecContext(save_userid, save_sec_context);
+
/* Don't release lock until commit. */
index_close(idxrel, NoLock);
> Shall we mention RLS restrictions? If we don't worry about that,
> I think REFERENCES privilege becomes a full bypass of RLS, at
> least for unique-key columns.
Seeing what check_enable_rls() does when running under the security
context set by ri_PerformCheck(), it indeed seems that RLS is bypassed
when executing these RI queries. The following comment in
check_enable_rls() seems to say so:
* InNoForceRLSOperation indicates that we should not apply RLS even
* if the table has FORCE RLS set - IF the current user is the owner.
* This is specifically to ensure that referential integrity checks
* are able to still run correctly.
> I wonder also what happens if the referenced table isn't a plain
> heap with a plain btree index. Maybe you're accessing it at the
> right level of abstraction so things will just work with some
> other access methods, but I'm not sure about that.
I believe that I've made ri_ReferencedKeyExists() use the appropriate
APIs to scan the index, lock the returned table tuple, etc., but do
you think we might be better served by introducing a new set of APIs
for this use case?
> (Anybody
> want to try this with a partitioned table some of whose partitions
> are foreign tables?)
Partitioned tables with foreign table partitions cannot be referenced
in a foreign key, so cannot appear in this function. That's because
unique constraints are not allowed when there are foreign table
partitions.
> Lastly, ri_PerformCheck is pretty careful about not only which
> snapshot it uses, but which *pair* of snapshots it uses, because
> sometimes it needs to worry about data changes since the start
> of the transaction. You've ignored all of that complexity AFAICS.
> That's okay (I think) for RI_FKey_check which was passing
> detectNewRows = false, but for sure it's not okay for
> ri_Check_Pk_Match. (I kind of thought we had isolation tests
> that would catch that, but apparently not.)
Okay, let me closely check the ri_Check_Pk_Match() case and see if
there's any live bug.
> So, this is a cute idea, and the speedup is pretty impressive,
> but I don't think it's anywhere near committable. I also wonder
> whether we really want ri_triggers.c having its own copy of
> low-level stuff like the tuple-locking code you copied. Seems
> like a likely maintenance hazard, so maybe some more refactoring
> is needed.
Okay, I will see if there's a way to avoid copying too much code.
--
Amit Langote
EDB: http://www.enterprisedb.com
