Re: simplifying foreign key/RI checks

On Thu, Mar 4, 2021 at 5:15 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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