Re: simplifying foreign key/RI checks

On Mon, Mar 8, 2021 at 11:41 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Thu, Mar 4, 2021 at 5:15 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > 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);

I've included these changes in the updated patch.

> > 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've added a comment to note that the new way of "selecting" the
referenced tuple effectively bypasses RLS, as is the case when
selecting via SPI.

> > 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?

I concur that by using the interfaces defined in genam.h and
tableam.h, patch accounts for cases involving other access methods.

That said, I had overlooked one bit in the new code that is specific
to btree AM, which is the hard-coding of BTEqualStrategyNumber in the
following:

/* Initialize the scankey. */
ScanKeyInit(&skey[i],
pkattno,
BTEqualStrategyNumber,
regop,
pk_vals[i]);

In the updated patch, I've added code to look up the index-specific
strategy number to pass here.

> > 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.

As mentioned in my earlier reply, there doesn't seem to be a need for
ri_Check_Pk_Match() to set the crosscheck snapshot as it is basically
unused.

> > 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.

I thought sharing the tuple-locking code with ExecLockRows(), which
seemed closest in semantics to what the new code is doing, might not
be such a bad idea, but not sure I came up with a great interface for
the shared function. Actually, there are other places having their
own copies of tuple-locking logic, but they deal with the locking
result in their own unique ways, so I didn't get excited about finding
a way to make the new function accommodate their needs. I also admit
that I may have totally misunderstood what refactoring you were
referring to in your comment.

Updated patches attached. Sorry about the delay.

--
Amit Langote
EDB: http://www.enterprisedb.com