Re: revamp row-security tracking

On Fri, Nov 29, 2024 at 10:01:41AM +0000, Dean Rasheed wrote:
> On Thu, 21 Nov 2024 at 18:00, Nathan Bossart <nathandbossart(at)gmail(dot)com> wrote:
>> The attached patch accomplishes this by establishing a global queue of
>> row-security "nest levels" that the aforementioned higher-level callers can
>> use.
>
> I'm not convinced that this is an improvement.

Thanks for reviewing.

> The code in check_sql_fn_retval() is building a Query struct from
> scratch, so it seems perfectly natural for it to be responsible for
> setting all the required fields, based on the information it has
> available. With this patch, check_sql_fn_retval() is returning a
> potentially incorrectly marked Query at the end of the querytree list,
> which the caller is responsible for fixing up, which doesn't seem
> ideal.

While it is indeed natural for the code that builds a Query to be
responsible for setting it correctly, unfortunately there's no backstop if
someone forgets to do so (as was the case in the recent CVE). I don't
think my v1 patch would necessarily prevent all such problems, but I do
think it would help prevent some.

> There is exactly one place where RLS policies are applied, and it
> seems much more natural for it to have responsibility for setting this
> flag. I think that a slightly neater way for it to handle that would
> be to modify fireRIRrules(), adding an extra parameter "bool
> *hasRowSecurity" that it would set to true if RLS is enabled for the
> query it is rewriting. Doing that forces all callers to think about
> whether or not that affects some outer query. For example,
> ApplyRetrieveRule() would then do:
>
> rule_action = fireRIRrules(rule_action, activeRIRs,
> &parsetree->hasRowSecurity);
>
> rather than having a separate second step to update the flag on
> "parsetree", and similarly for fireRIRrules()'s recursive calls to
> itself. If, in the future, it becomes necessary to invoke
> fireRIRrules() on more parts of a Query, it's then much more likely
> that the new code won't forget to update the parent query's flag.

I've attempted this in the attached v2 patch. I do think this is an
improvement over the status quo, but I worry that it doesn't go far enough.

--
nathan