Re: row_security GUC, BYPASSRLS

* Noah Misch (noah(at)leadboat(dot)com) wrote:
> Right now, if a BYPASSRLS user creates a SECURITY DEFINER function, any caller
> can change that function's behavior by toggling the GUC. Users won't test
> accordingly; better to have just one success-case behavior.

I agree that's not good, though the function definer could set the
row_security GUC on the function, no? Similar to how we encourage
setting of search_path, we should be encouraging a similar approach to
anything which might be security relevant.

> On Wed, Jul 29, 2015 at 09:09:27AM -0400, Stephen Frost wrote:
> > For superuser (the only similar precedent that we have, I believe), we
> > go based on the view owner, but that isn't quite the same as BYPASSRLS.
> >
> > The reason this doesn't hold is that you have to use a combination of
> > BYPASSRLS and row_security=off to actually bypass RLS, unlike the
> > superuser role attribute which is just always "on" if you've got it. If
> > having BYPASSRLS simply always meant "don't do any RLS" then we could
> > use the superuser precedent to use what the view owner has, but at least
> > for my part, I'm a lot happier with BYPASSRLS and row_security than with
> > superuser and would rather we continue in that direction, where the user
> > has the choice of if they want their role attribute to be in effect or
> > not.
>
> If I make BYPASSRLS GUC-independent, I should then also make it take effect
> when the BYPASSRLS role owns a view. Barring objections, I will change both.

I agree that if it's GUC-independent then it should operate the same as
superuser does for views and security definer functions.

On the one hand, I don't like that BYPASSRLS roles will now behave
differently from non-BYPASSRLS roles, but on the other hand, the above
isn't good and having BYPASSRLS always enabled may make individuals shy
away from giving it out except when strictly necessary and treat it more
similar to superuser, which would be a good thing.

> I do share your wish for an ability to suppress privileges temporarily. I
> have no specific design in mind, but privilege activation and suppression
> should be subject to the approval of roles affected. GUCs probably can't
> serve here; apart from the grandfathered search_path, functions can ignore
> them. GUCs are mostly a property of the whole session.

Perhaps GUCs won't work, but they own a pretty handy namespace
(SET X = Y) and we are able to attach specific GUC settings to
functions already. I don't like the idea that we'd invent a whole new
syntax or bits of grammar to do the same for whatever approach we come
up to for suppressing privileges temporarily (such as in SECURITY
DEFINER functions). The odd case here is really views, since they
operate somewhere inbetween regular queries and security definer
functions, regarding permissions.

> By the way, is there a reason for RI_Initial_Check() to hard-code the rules
> for RLS enablement instead of calling check_enable_rls(..., InvalidOid, true)
> twice? I refer to this code:

I don't see a reason for it now, though I recall one existing when the
code was originally written. That might have simply been a bit of extra
(though unnecessary) paranoia though, as returning 'false' is a safe
route.

Are you planning to handle the ALTER TABLE .. FORCE ROW SECURITY (#3) as
well? I have no complaints if so; just want to make sure we aren't
doing double-work during this crunch time and didn't see your name
listed next to it, but the nearby thread seemed to imply you were
looking at it.

One item which wasn't discussed, that I recall, is just how it will work
without SECURITY_ROW_LEVEL_DISABLED, or something similar, to
differentiate when internal referencial integrity queries are being run,
which should still bypass RLS (even in the FORCE ROW SECURITY case), and
when regular or SECURITY DEFINER originated queries are being run.

The concensus, as I understood it, was that removing the ability to do
SET ROW_SECURITY = force is good, but if done, we need to support
ALTER TABLE .. FORCE ROW SECURITY. I'm trying to figure out if that
means we end up not actually addressing the original concern you raised
regarding SECURITY_ROW_LEVEL_DISABLED.

Thanks!

Stephen