Re: row_security GUC, BYPASSRLS - Mailing list pgsql-hackers

* Noah Misch (noah@leadboat.com) wrote:
> On Mon, Sep 28, 2015 at 05:13:56PM -0400, Stephen Frost wrote:
> > * Noah Misch (noah@leadboat.com) wrote:
> > > In schema reviews, I will raise a red flag for use of this feature; the best
> > > designs will instead use additional roles.  I forecast that PostgreSQL would
> > > fare better with no owner-constrained-by-RLS capability.  Even so, others want
> > > it, and FORCE ROW SECURITY would deliver it with an acceptable risk profile.
> >
> > 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.
>
> This patch has FORCE ROW LEVEL SECURITY take precedence over row_security=off,
> which thwarts pg_dump use of row_security=off to ensure dump completeness.

Fixed.

> Should this be a table-level flag, or should it be a policy-level flag?  A
> policy-level flag is more powerful.  If nobody really anticipates using that
> power, this table-level flag works for me.

table-level seems the right level to me and no one is calling for
policy-level.  Further, policy-level could be added later if there ends
up being significant interest later.

> > > SECURITY_ROW_LEVEL_DISABLED could have been okay.  I removed an incomplete
> > > implementation (e.g. didn't affect CASCADE constraints).  Writing a full one
> > > would be a mammoth job, and for what?  Setting the wrong SELECT policies can
> > > disrupt _any_ application logic; no foreign keys or FORCE ROW SECURITY need be
> > > involved.  Protecting just foreign keys brings some value, but it will not
> > > materially reduce the vigilance demanded of RLS policy authors and reviewers.
> >
> > 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.
>
> If disabling policies has an effect, table owners must be feeding conflicting
> requirements into the system.  Violating policies during referential integrity
> queries is not, in general, less serious than violating referential integrity
> itself.  Rules and triggers pose the same threat, and we let those break
> referential integrity.  I think the ideal in all of these cases is rather to
> detect the conflict and raise an error.  SECURITY_ROW_LEVEL_DISABLED and
> SECURITY_NOFORCE_RLS send policies in a third direction, neither the beaten
> path of rules/triggers nor the ideal.

The agreement between the user and the system with regard to permissions
and referential integrity is that referential integrity takes priority
over permissions.  Prior to FORCE and SECURITY_NOFORCE_RLS that is true
for RLS.  I don't believe it makes sense that adding FORCE would change
that agreement, nor do I agree that the ideal would be for the system to
throw errors when the permissions system would deny access during RI
checks.

While I appreciate that rules and triggers can break RI, the way RLS
works is consistent with the ACL system and FORCE should be consistent
with the ACL system and normal/non-FORCE RLS with regard to referential
integrity.

> > 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.
>
> Yes, that's far safer than SECURITY_ROW_LEVEL_DISABLED.  I assume the final
> design will let table owners completely bypass FORCE ROW LEVEL SECURITY under
> "SET row_security = off".  If so, SECURITY_NOFORCE_RLS poses negligible risk.

I've made that change.

> Functions differing only in s/ = true/ = false/?  ATExecEnableDisableTrigger()
> is a better model for this.

I've changed that to be one function.  As an independent patch, I'll do
the same for ATExecEnable/DisableRowSecurity.

Apologies about the timing, I had intended to get this done yesterday.

Barring further concerns, I'll push the attached later today with the
necessary catversion bump.

Thanks!

Stephen