Re: API change advice: Passing plan invalidation info from the rewriter into the planner?

On Fri, Jul 18, 2014 at 7:01 PM, Brightwell, Adam
<adam.brightwell@crunchydatasolutions.com> wrote:
>> I think we do want a way to modify policies.  However, we tend to
>> avoid syntax that involves unnatural word order, as this certainly
>> does.  Maybe it's better to follow the example of CREATE RULE and
>> CREATE TRIGGER and do something this instead:
>>
>> CREATE POLICY policy_name ON table_name USING quals;
>> ALTER POLICY policy_name ON table_name USING quals;
>> DROP POLICY policy_name ON table_name;
>>
>> The advantage of this is that you can regard "policy_name ON
>> table_name" as the identifier for the policy throughout the system.
>> You need some kind of identifier of that sort anyway to support
>> COMMENT ON, SECURITY LABEL, and ALTER EXTENSION ADD/DROP for policies.
>
> Sounds good.  I certainly think it makes a lot of sense to include the ALTER
> functionality, if for no other reason than ease of use.
>
> Another item to consider, though I believe it can come later, is per-action
> policies.  Following the above suggested syntax, perhaps that might look
> like the following?
>
> CREATE POLICY policy_name ON table_name FOR action USING quals;
> ALTER POLICY policy_name ON table_name FOR action USING quals;
> DROP POLICY policy_name ON table_name FOR action;

That seems reasonable.  You need to give some thought to what happens
if the user types:

CREATE POLICY pol1 ON tab1 FOR SELECT USING q1;
ALTER POLICY pol1 ON tab1 FOR INSERT USING q2;

I guess you end up with q1 as the SELECT policy and q2 as the INSERT
policy.  Similarly, had you typed:

CREATE POLICY pol1 ON tab1 USING q1;
ALTER POLICY pol1 ON tab1 FOR INSERT USING q2;

...then I guess you end up with q2 for INSERTs and q1 for everything
else.  I'm wondering if it might be better, though, not to allow the
quals to be specified in CREATE POLICY, or else to allow multiple
actions.  Otherwise, getting pg_dump to DTRT might be complicated.

Perhaps:

CREATE POLICY pol1 ON tab1 ( [ [ FOR operation [ OR operation ] ... ]
USING quals ] ... );
where operation = SELECT | INSERT | UPDATE | DELETE

So that you can write things like:

CREATE POLICY pol1 ON tab1 (USING a = 1);
CREATE POLICY pol2 ON tab2 (FOR INSERT USING a = 1, FOR UPDATE USING b
= 1, FOR DELETE USING c = 1);

And then, for ALTER, just allow one change at a time, syntax as you
proposed.  That way each policy can be dumped as a single CREATE
statement.

> I was also giving some thought to the use of "POLICY", perhaps I am wrong,
> but it does seem it could be at risk of becoming ambiguous down the road.  I
> can't think of any specific examples at the moment, but my concern is what
> happens if we wanted to add another "type" of policy, whatever that might
> be, later?  Would it make more sense to go ahead and qualify this a little
> more with "ROW SECURITY POLICY"?

I think that's probably over-engineering.  I'm not aware of anything
else we might add that would be likely to be called a policy, and if
we did add something we could probably call it something else instead.
And long command names are annoying.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Aug 29, 2014 at 8:16 PM, Brightwell, Adam
<adam.brightwell@crunchydatasolutions.com> wrote:
> Attached is a patch for RLS that was create against master at
> 01363beae52700c7425cb2d2452177133dad3e93 and is ready for review.
>
> Overview:
>
> This patch provides the capability to create multiple named row level
> security policies for a table on a per command basis and assign them to be
> applied to specific roles/users.
>
> It contains the following changes:
>
> * Syntax:
>
> CREATE POLICY <name> ON <table>
>     [ FOR { ALL | SELECT | INSERT | UPDATE | DELETE } ]
>     [ TO { PUBLIC | <role> [, <role> ] } ]
>     USING (<condition>)
>
> Creates a RLS policy named <name> on <table>.  Specifying a command is
> optional, but the default is ALL.  Specifying a role is options, but the
> default is PUBLIC.  If PUBLIC and other roles are specified, ONLY PUBLIC is
> applied and a warning is raised.
>
> ALTER POLICY <name> ON <table>
>     [ FOR { ALL | SELECT | INSERT | UPDATE | DELETE } ]
>     [ TO { PUBLIC | <role> [, <role> ] } ]
>     USING (<condition>)
>
> Alter a RLS policy named <name> on <table>.  Specifying a command is
> optional, if provided then the policy's command is changed otherwise it is
> left as-is.  Specifying a role is optional, if provided then the policy's
> role is changed otherwise it is left as-is.  The <condition> must always be
> provided and is therefore always replaced.

This is not a full review of this patch; as we're mid-CommitFest, I
assume this will get added to the next CommitFest.

In earlier discussions, it was proposed (and I thought the proposal
was viewed favorably) that when enabling row-level security for a
table (i.e. before doing CREATE POLICY), you'd have to first flip the
table to a default-deny mode:

ALTER TABLE <name> ENABLE ROW LEVEL SECURITY;

In this design, I'm not sure what happens when there are policies for
some but not all users or some but not all actions.  Does creating a
INSERT policy for one particular user cause a default-deny policy to
be turned on for all other users and all other operations?  That might
be OK, but at the very least it should be documented more clearly.
Does dropping the very last policy then instantaneously flip the table
back to default-allow?

As far as I can tell from the patch, and that's not too far since I've
only looked at briefly, there's a default-deny policy only if there is
at least 1 policy that applies to your user ID for this operation.  As
far as making it easy to create a watertight combination of policies,
that seems like a bad plan.

+         elog(ERROR, "Table \"%s\" already has a policy named \"%s\"."
+             " Use a different name for the policy or to modify this policy"
+             " use ALTER POLICY %s ON %s USING (qual)",
+             RelationGetRelationName(target_table), stmt->policy_name,
+             RelationGetRelationName(target_table), stmt->policy_name);
+

That needs to be an ereport, be capitalized properly, and the hint, if
it's to be included at all, needs to go into errhint().

+                          errhint("all roles are considered members
of public")));

Wrong message style for a hint.  Also, not sure that's actually
appropriate for a hint.

+         case EXPR_KIND_ROW_SECURITY:
+             return "ROW SECURITY";

This is quite simply bizarre.  That's not the SQL syntax of anything.

+             | ROW SECURITY row_security_option
+                 {
+                     VariableSetStmt *n = makeNode(VariableSetStmt);
+                     n->kind = VAR_SET_VALUE;
+                     n->name = "row_security";
+                     n->args = list_make1(makeStringConst($3, @3));
+                     $$ = n;
+                 }

I object to this.  There's no reason that we should bloat the parser
to allow SET ROW SECURITY in lieu of SET row_security unless this is a
standard-mandated syntax with standard-mandated semantics, which I bet
it isn't.

  /*
+  * Although only "on" and"off" are documented, we accept all likely
variants of
+  * "on" and "off".
+  */
+ static const struct config_enum_entry row_security_options[] = {
+     {"off", ROW_SECURITY_OFF, false},
+     {"on", ROW_SECURITY_ON, false},
+     {"true", ROW_SECURITY_ON, true},
+     {"false", ROW_SECURITY_OFF, true},
+     {"yes", ROW_SECURITY_ON, true},
+     {"no", ROW_SECURITY_OFF, true},
+     {"1", ROW_SECURITY_ON, true},
+     {"0", ROW_SECURITY_OFF, true},
+     {NULL, 0, false}
+ };

Just make it a bool and you get all this for free.

+ /*
+  * is_rls_enabled -
+  *   determines if row-security is enabled by checking the value of the system
+  *   configuration "row_security".
+  */
+ bool
+ is_rls_enabled()
+ {
+     char const *rls_option;
+
+     rls_option = GetConfigOption("row_security", true, false);
+
+     return (strcmp(rls_option, "on") == 0);
+ }

Words fail me.

+     if (AuthenticatedUserIsSuperuser)
+         SetConfigOption("row_security", "off", PGC_INTERNAL, PGC_S_OVERRIDE);

Injecting this kind of magic into InitializeSessionUserId(),
SetSessionAuthorization(), and SetCurrentRoleId() seems 100%
unacceptable to me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

All,

Attached is a patch for RLS that incorporates the following changes:

* Syntax:

  - CREATE POLICY <policy_name> ON <table_name> FOR <command> USING ( <qual> )

  - ALTER POLICY <policy_name> ON <table_name> FOR <command> USING ( <qual> )

  - DROP POLICY <policy_name> ON <table_name> FOR <command>

* "row_security" GUC Setting - enable/disable row level security.

* BYPASSRLS and NOBYPASSRLS role attribute - allows user to bypass RLS if row_security GUC is set to OFF.

There are still some remaining issues but we hope to have those resolved soon.

Any comments or suggestions would be greatly appreciated.

Thanks,

Adam

On Mon, Jul 21, 2014 at 11:38 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Fri, Jul 18, 2014 at 7:01 PM, Brightwell, Adam
<adam.brightwell@crunchydatasolutions.com> wrote:
>> I think we do want a way to modify policies.  However, we tend to
>> avoid syntax that involves unnatural word order, as this certainly
>> does.  Maybe it's better to follow the example of CREATE RULE and
>> CREATE TRIGGER and do something this instead:
>>
>> CREATE POLICY policy_name ON table_name USING quals;
>> ALTER POLICY policy_name ON table_name USING quals;
>> DROP POLICY policy_name ON table_name;
>>
>> The advantage of this is that you can regard "policy_name ON
>> table_name" as the identifier for the policy throughout the system.
>> You need some kind of identifier of that sort anyway to support
>> COMMENT ON, SECURITY LABEL, and ALTER EXTENSION ADD/DROP for policies.
>
> Sounds good.  I certainly think it makes a lot of sense to include the ALTER
> functionality, if for no other reason than ease of use.
>
> Another item to consider, though I believe it can come later, is per-action
> policies.  Following the above suggested syntax, perhaps that might look
> like the following?
>
> CREATE POLICY policy_name ON table_name FOR action USING quals;
> ALTER POLICY policy_name ON table_name FOR action USING quals;
> DROP POLICY policy_name ON table_name FOR action;

That seems reasonable.  You need to give some thought to what happens
if the user types:

CREATE POLICY pol1 ON tab1 FOR SELECT USING q1;
ALTER POLICY pol1 ON tab1 FOR INSERT USING q2;

I guess you end up with q1 as the SELECT policy and q2 as the INSERT
policy.  Similarly, had you typed:

CREATE POLICY pol1 ON tab1 USING q1;
ALTER POLICY pol1 ON tab1 FOR INSERT USING q2;

...then I guess you end up with q2 for INSERTs and q1 for everything
else.  I'm wondering if it might be better, though, not to allow the
quals to be specified in CREATE POLICY, or else to allow multiple
actions.  Otherwise, getting pg_dump to DTRT might be complicated.

Perhaps:

CREATE POLICY pol1 ON tab1 ( [ [ FOR operation [ OR operation ] ... ]
USING quals ] ... );
where operation = SELECT | INSERT | UPDATE | DELETE

So that you can write things like:

CREATE POLICY pol1 ON tab1 (USING a = 1);
CREATE POLICY pol2 ON tab2 (FOR INSERT USING a = 1, FOR UPDATE USING b
= 1, FOR DELETE USING c = 1);

And then, for ALTER, just allow one change at a time, syntax as you
proposed.  That way each policy can be dumped as a single CREATE
statement.

> I was also giving some thought to the use of "POLICY", perhaps I am wrong,
> but it does seem it could be at risk of becoming ambiguous down the road.  I
> can't think of any specific examples at the moment, but my concern is what
> happens if we wanted to add another "type" of policy, whatever that might
> be, later?  Would it make more sense to go ahead and qualify this a little
> more with "ROW SECURITY POLICY"?

I think that's probably over-engineering.  I'm not aware of anything
else we might add that would be likely to be called a policy, and if
we did add something we could probably call it something else instead.
And long command names are annoying.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--

Adam Brightwell - adam.brightwell@crunchydatasolutions.com

Database Engineer - www.crunchydatasolutions.com

On 6/11/14, 10:26 AM, Robert Haas wrote:

Now, as soon as we introduce the concept that selecting from a table might not really mean "read from the table" but "read from the table after applying this owner-specified qual", we're opening up a whole new set of attack surfaces. Every pg_dump is an opportunity to hack somebody else's account, or at least audit their activity.

I'm in full agreement we should clearly communicate the issues around pg_dump in particular, because they can't necessarily be eliminated altogether without some major work that's going to take a while to finish.  And if the work-around is some sort of GUC for killing RLS altogether, that's ugly but not unacceptable to me as a short-term fix.