Re: RLS Design

Robert,

Alright, I can't help it so I'll try and reply from my phone for a couple
of these. :)

On Wednesday, September 3, 2014, 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 <javascript:;>> 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.

As per usual, the expectation is that the patch is reviewed and updated
during the commitfest. Given that the commitfest isn't even over according
to the calendar it seems a bit premature to talk about the next one, but
certainly if it's not up to a commitable level before the end of this
commitfest then it'll be submitted for the next.

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

I do recall that (now that you remind me- clearly it had been lost during
the subsequent discussion, from my point of view at least) and agree that
it'd be useful. I don't believe it'll be difficult to address.

> ALTER TABLE <name> ENABLE ROW LEVEL SECURITY;

Sounds reasonable to me.

> + 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().

Already addressed.

> + errhint("all roles are considered members
> of public")));
>
> Wrong message style for a hint. Also, not sure that's actually
> appropriate for a hint.

Fair enough. Will address.

> + case EXPR_KIND_ROW_SECURITY:
> + return "ROW SECURITY";
>
> This is quite simply bizarre. That's not the SQL syntax of anything.

Will address.

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

Agreed. Seemed like a nice idea but it's not necessary.

> /*
> + * 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.

Right- holdover from an earlier attempt to make it more complicated but now
we've simplified it and so it should just be a bool.

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

I was struggling with the right way to address this and welcome
suggestions. The primary issue is that I really want to support a superuser
turning it on, so we can't simply have it disabled for all superusers all
the time. The requirement that it not be enabled by default for superusers
makes sense, but how far does that extend and how do we address upgrades?
In particular, can we simply set row_security=off as a custom GUC setting
when superusers are created or roles altered to be made superusers? Would
we do that in pg_upgrade?

Thanks!

Stephen