Re: RLS Design

On Fri, Aug 29, 2014 at 8:16 PM, Brightwell, Adam
<adam(dot)brightwell(at)crunchydatasolutions(dot)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