* Noah Misch (n...@leadboat.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
signature.asc
Description: Digital signature
