Re: row_security GUC, BYPASSRLS

From: Adam Brightwell <adam(dot)brightwell(at)crunchydatasolutions(dot)com>
To: Stephen Frost <sfrost(at)snowman(dot)net>
Cc: Noah Misch <noah(at)leadboat(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Joe Conway <mail(at)joeconway(dot)com>
Subject: Re: row_security GUC, BYPASSRLS
Date: 2015-09-29 14:56:44
Message-ID: CAKRt6CSt=0d2UGajDSQYPqFTtmZZWBtz+0UAb6edzDDqUQg3Bg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> I've attached a patch to implement it. It's not fully polished but it's
> sufficient for comment, I believe. Additional comments, documentation
> and regression tests are to be added, if we have agreement on the
> grammer and implementation approach.

I have given the first patch a quick review. I think I agree with the
grammar, it makes sense to me. At first I didn't really like NO
FORCE, but I couldn't think of anything better.

One comment:

if (pg_class_ownercheck(relid, user_id))
- return RLS_NONE_ENV;
+ {
+ if (relforcerowsecurity)
+ return RLS_ENABLED;
+ else
+ return RLS_NONE_ENV;
+ }

Is the 'else' even necessary in this block?

Other than that, the approach looks good to me.

The patch applied cleanly against master (758fcfd). As well
'check-world' was successful (obviously understanding that more
related tests are necessary).

> I have a hard time with this. We're not talking about the application
> logic in this case, we're talking about the guarantees which the
> database is making to the user, be it an application or an individual.
>
> I've included a patch (the second in the set attached) which adds a
> SECURITY_NOFORCE_RLS bit which has a much tighter scope- it only applies
> after the regular owner check is done. This reduces the risk of it
> being set mistakenly dramatically, I believe. Further, the cascaded
> case is correctly handled. This also needs more polish and regression
> tests, but I wanted to solicit for comment before moving forward with
> that since we don't have a consensus about if this should be done.

I'm not sure that I understand the previous concerns around this bit
well enough to speak intelligently on this point. However, with that
said, I believe the changes made by this patch make sense.

-Adam

--
Adam Brightwell - adam(dot)brightwell(at)crunchydatasolutions(dot)com
Database Engineer - www.crunchydatasolutions.com

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2015-09-29 14:57:03 Re: PATCH: index-only scans with partial indexes
Previous Message Steve Crawford 2015-09-29 14:55:26 Re: No Issue Tracker - Say it Ain't So!