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

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.

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.

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

> 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.
Even if not, SECURITY_NOFORCE_RLS poses low risk.

> @@ -3667,6 +3673,12 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
>          case AT_DisableRowSecurity:
>              ATExecDisableRowSecurity(rel);
>              break;
> +        case AT_ForceRowSecurity:
> +            ATExecForceRowSecurity(rel);
> +            break;
> +        case AT_NoForceRowSecurity:
> +            ATExecNoForceRowSecurity(rel);
> +            break;

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

nm