Re: RLS Design

All,

Attached is a updated patch taking into account the recommendations
provided.

This patch created against master
at ad5d46a4494b0b480a3af246bb4227d9bdadca37

The following items have been addressed:

* Add ALTER TABLE <name> { ENABLE | DISABLE } ROW LEVEL SECURITY - set flag
on table to allow for a default-deny capability. If RLS is enabled on a
table and has no policies, then a default-deny policy is automatically
applied. If RLS is disabled on table and the table still has policies on
it then then an error is raised. Though if DISABLE is accompanied with
CASCADE, then all policies will be removed and no error is raised.

* Update CREATE POLICY to include WITH CHECK ( <expression> ). Therefore,
the syntax is now as follows:
CREATE POLICY <name> ON <table>
[ FOR { ALL | SELECT | INSERT | UPDATE | DELETE } ]
[ USING ( <expression> ) ]
[ WITH CHECK ( <expression> ) ]

A WITH CHECK expression is required for creating an INSERT policy and is
optional on UPDATE and ALL. The intended purpose is to provide a VIEW-like
WITH CHECK OPTION functionality to RLS.

* Add ALTER POLICY <name> ON <table> RENAME TO <new_name> - renames a
policy.

* Updated GUC row_security to allow ON | OFF | FORCE. Each option breaks
down as follows:
- ON - RLS is appled to all roles except the table owner and superusers.
- OFF - RLS can be bypassed, but only by roles with BYPASSRLS. If the
roles does not have BYPASSRLS, then an error is raised.
- FORCE - RLS is applied to all roles, regardless of ownership,
superuser or BYPASSRLS.

* Removed SET ROW SECURITY { ON | OFF } as requested.

* Removed all GetConfigOption for "row_security" GUC.

* Removed setting row_security GUC to OFF in SET SESSION/SET ROLE for
superuser.

* Add psql \dp support. Displays RLS information in new column "Policies".

* Updated documentation.

* Other cleanup and improvements.

There are still some minor issues being worked through, however, it is
expected that those will be resolved soon. However, any feedback, comments
or suggestions on the above and in general would be greatly appreciated.

Thanks,
Adam

On Wed, Sep 3, 2014 at 10:17 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> 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
>

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