Received: from malur.postgresql.org ([217.196.149.56])
	by arkaria.postgresql.org with esmtp (Exim 4.84_2)
	(envelope-from
 <pgsql-hackers-owner+M311511=pgsql-hackers-arkari@postgresql.org>)
	id 1cCSWp-000212-Qt
	for pgsql-hackers@arkaria.postgresql.org; Thu, 01 Dec 2016 14:40:03 +0000
Received: from localhost ([127.0.0.1] helo=postgresql.org)
	by malur.postgresql.org with smtp (Exim 4.84_2)
	(envelope-from
 <pgsql-hackers-owner+M311511=pgsql-hackers-arkari@postgresql.org>)
	id 1cCSWp-0005jI-7I
	for pgsql-hackers@arkaria.postgresql.org; Thu, 01 Dec 2016 14:40:03 +0000
Received: from makus.postgresql.org ([2001:4800:1501:1::229])
	by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256)
	(Exim 4.84_2)
	(envelope-from <sfrost@tamriel.snowman.net>)
	id 1cCSWn-0005hV-90
	for pgsql-hackers@postgresql.org; Thu, 01 Dec 2016 14:40:01 +0000
Received: from tamriel.snowman.net ([72.66.115.51])
	by makus.postgresql.org with esmtp (Exim 4.84_2)
	(envelope-from <sfrost@tamriel.snowman.net>)
	id 1cCSWk-0004cL-RA
	for pgsql-hackers@postgresql.org; Thu, 01 Dec 2016 14:40:00 +0000
Received: by tamriel.snowman.net (Postfix, from userid 1000)
	id AD9085F7A8; Thu,  1 Dec 2016 09:38:57 -0500 (EST)
Date: Thu, 1 Dec 2016 09:38:57 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Dean Rasheed <dean.a.rasheed@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
	Jeevan Chalke <jeevan.chalke@enterprisedb.com>,
	Robert Haas <robertmhaas@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	Thom Brown <thom@linux.com>,
	"pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: Add support for restrictive RLS policies
Message-ID: <20161201143857.GH13284@tamriel.snowman.net>
References: 
 <CAM2+6=WaYrso+K4spreugPBxmBGNSwycXaS1B2a-dPieawJLwQ@mail.gmail.com>
 <CAM2+6=UHCuD_wT2kK_H2zL_9k8XabVLxOUaBgWcWqsSvBP1gXg@mail.gmail.com>
 <20160926204331.GA178854@alvherre.pgsql>
 <20160926195554.GA176476@alvherre.pgsql>
 <20160928191746.GZ5148@tamriel.snowman.net>
 <20161130215419.GZ13284@tamriel.snowman.net>
 <CAEZATCURYzb8M_DMsmKQ9LQ+rZUmM=3irATtLgkRxopw=j=yjg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="2unbGIxD3TSJ7c94"
Content-Disposition: inline
In-Reply-To: 
 <CAEZATCURYzb8M_DMsmKQ9LQ+rZUmM=3irATtLgkRxopw=j=yjg@mail.gmail.com>
X-Editor: Vim http://www.vim.org/
X-Info: http://www.snowman.net
X-Operating-System: Linux/3.13.0-91-generic (x86_64)
X-Uptime: 09:32:03 up 145 days, 15:56, 25 users,  load average: 0.13, 0.12,
 0.07
User-Agent: Mutt/1.5.21 (2010-09-15)
X-Pg-Spam-Score: -4.8 (----)
List-Archive: <http://archives.postgresql.org/pgsql-hackers>
List-Help: <mailto:majordomo@postgresql.org?body=help>
List-ID: <pgsql-hackers.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@postgresql.org>
List-Post: <mailto:pgsql-hackers@postgresql.org>
List-Subscribe: <mailto:majordomo@postgresql.org?body=sub%20pgsql-hackers>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
X-Mailing-List: pgsql-hackers
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


--2unbGIxD3TSJ7c94
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 30 November 2016 at 21:54, Stephen Frost <sfrost@snowman.net> wrote:
> > Unless there's further comments, I'll plan to push this sometime
> > tomorrow.
>=20
> Sorry I didn't have time to look at this properly. I was intending to,
> but my day job just keeps getting in the way.

No worries, took me a while to get back to adding the requested
documentation too.

> I do have a couple of
> comments relating to the documentation and one relating to the code:

Thanks!

> -   row-level security policy.
> +   row-level security policy.  Note that only the set of roles which the
> +   policy applies to and the <literal>USING</literal> and
> +   <literal>WITH CHECK</literal> expressions are able to be changed using
> +   <command>ALTER POLICY</command>.  To change other properties of a pol=
icy,
> +   such as the command it is applied for or if it is a permissive or
> +   restrictive policy, the policy must be dropped and recreated.
>=20
> This note reads a little awkwardly to me. I think I would write it as:
>=20
>     Note that <command>ALTER POLICY</command> only allows the set of roles
>     to which the policy applies and the <literal>USING</literal> and
>     <literal>WITH CHECK</literal> expressions to be modified.  To change
>     other properties of a policy, such as the command to which it applies
>     or whether it is permissive or restrictive, the policy must be dropped
>     and recreated.

I agree, that's better, will update.

> +    <term><replaceable class=3D"parameter">PERMISSIVE</replaceable></ter=
m>
> +    <listitem>
> +     <para>
> +      Specify that the policy is to be created as a "permissive" policy.
> +      All "permissive" policies which are applicable to a given query wi=
ll
> +      be combined together using the boolean "OR" operator.  By creating
> +      "permissive" policies, administrators can add to the set of records
> +      which can be accessed.  Policies are PERMISSIVE by default.
> +     </para>
> +    </listitem>
> +   </varlistentry>
> +
> +   <varlistentry>
> +    <term><replaceable class=3D"parameter">RESTRICTIVE</replaceable></te=
rm>
> +    <listitem>
> +     <para>
> +      Specify that the policy is to be created as a "restrictive" policy.
> +      All "restrictive" policies which are applicable to a given query w=
ill
> +      be combined together using the boolean "AND" operator.  By creating
> +      "restrictive" policies, administrators can reduce the set of recor=
ds
> +      which can be accessed as all "restrictive" policies must be passed=
 for
> +      each record.
> +     </para>
> +    </listitem>
> +   </varlistentry>
>=20
> I don't think this or anywhere else makes it entirely clear that the
> user needs to have at least one permissive policy in addition to any
> restrictive policies for this to be useful. I think this section is
> probably a good place to mention that, since it's probably the first
> place people will read about restrictive policies. I think it also
> needs to be spelled out exactly how a mix of permissive and
> restrictive policies are combined, because there is more than one way
> to combine a set of quals with ANDs and ORs (although only one way
> really makes sense in this context). So perhaps an additional note
> along the lines of:
>=20
>     Note that there needs to be at least one permissive policy to grant
>     access to records before restrictive policies can be usefully used to
>     reduce that access. If only restrictive policies exist, then no recor=
ds
>     will be accessible. When a mix of permissive and restrictive policies
>     are present, a record is only accessible if at least one of the
>     permissive policies passes, in addition to all the restrictive
>     policies.
>=20
> Also, I don't think it's necessary to keep quoting "restrictive" and
> "permissive" here.

Works for me, I'll add that, and remove the quotes around restrictive
and permissive.

> In get_policies_for_relation(), after the loop that does this:
>=20
> -            *permissive_policies =3D lappend(*permissive_policies, polic=
y);
> +        {
> +            if (policy->permissive)
> +                *permissive_policies =3D lappend(*permissive_policies, p=
olicy);
> +            else
> +                *restrictive_policies =3D lappend(*restrictive_policies,=
 policy);
> +        }
>=20
> I think it should sort the restrictive policies by name, for the same
> reason that hook-restrictive policies are sorted -- so that the WITH
> CHECK expressions are checked in a well-defined order (which was
> chosen to be consistent with the order of checking of other similar
> things, like CHECK constraints and triggers). I also think that this
> should be a separate sort step from the sort for hook policies,
> inserted just after the loop above, so that the order is all internal
> policies sorted by name, followed by all hook policies sorted by name,
> to be consistent with the existing principle that hook policies are
> applied after internal policies.

Hmmm, is it really the case that the quals will always end up being
evaluated in that order though?  Isn't order_qual_clauses() going to end
up changing the order based on the relative cost?  If the cost is the
same it should maintain the order, but even that could change in the
future based on the comments, no?  In short, I'm not entirely sure that
we actually want to be required to always evaluate the quals in order of
policy name and we might get complaints if we happen to make that work
today and it ends up being changed later.

Thanks!

Stephen

--2unbGIxD3TSJ7c94
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJYQDYBAAoJEO1sijiDR2RVi20P+QHx9w+YgVG5VFlkc52T8fb4
SMEN2kXTpZKMHtQQNo6aNo/TL0yfkI9iaUUCgekFQCPnRacPbEqIiZh4YfqdiRDN
qtZ8Y2jKEWuaDL9v4H3rEuFQ1KWHz33vAIRLLhOZbJAr3FvNO8k5IUo5DyZIHBb6
7doMMhjCFogjzI+/KA4VjjiNmJeD3allrx55vRgAwmGCJUA+qNspXbxBntgIi6yx
5OABTvjcoMGvJyCBgiYHosZ9/P+/lowTn9m7oE3EyfH0Bj8JfmkgbFFdg1rorpxR
txlWWEV4DCOKgHLM7kptqoCk9gyfazk7avUUxsa51v5SSTMgJ0T7SPazznHsHk3P
qibLXLgS4EAnVPMfWaLttRZ3H8dDyV/PYme3nDfDOKEdIxEUTFOo0d1/zrOEr96c
e9MTnImR2OBV5kxJzy8pjQMeDtT5yVfIxfFgHlNUIkRc3UmTld71pVVlRwktZC9e
3kOH0CRdzyC4dYbhenunk8h0X3BzZ3ThP0aFpKxC9K+U199mr+ZxKa/n5bh+BBJr
Uzt6kDYHQIJV3eIncnX085YOvpGJ8LjRmbl4QabTc9r1tsAezL7H92JJuFtB+BaA
IGKxAamrU+cgHo+yUwADe/m3sIXsQk+RTVAdj/zZ9xqe2s6RL3Iu2XaNZQSm6YGG
fnj7cWkVRK73OAuU6n7v
=sUE+
-----END PGP SIGNATURE-----

--2unbGIxD3TSJ7c94--