Possible typo in create_policy.sgml

On Tue, Jan 6, 2015 at 12:26 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

Following is perhaps a typo:

-   qualifications of queries which are run against the table the policy
is on,
+   qualifications of queries which are run against the table if the
policy is on,

Attached fixes it if so.

I don't think that's a typo, although it's not particularly
well-worded IMHO. I might rewrite the whole paragraph like this:

A policy limits the ability to SELECT, INSERT, UPDATE, or DELETE rows
in a table to those rows which match the relevant policy expression.
Existing table rows are checked against the expression specified via
USING, while new rows that would be created via INSERT or UPDATE are
checked against the expression specified via WITH CHECK. Generally,
the system will enforce filter conditions imposed using security
policies prior to qualifications that appear in the query itself, in
order to the prevent the inadvertent exposure of the protected data to
user-defined functions which might not be trustworthy. However,
functions and operators marked by the system (or the system
administrator) as LEAKPROOF may be evaluated before policy
expressions, as they are assumed to be trustworthy.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert, Amit,

* Robert Haas (robertmhaas@gmail.com) wrote:

I don't think that's a typo, although it's not particularly
well-worded IMHO. I might rewrite the whole paragraph like this:

A policy limits the ability to SELECT, INSERT, UPDATE, or DELETE rows
in a table to those rows which match the relevant policy expression.
Existing table rows are checked against the expression specified via
USING, while new rows that would be created via INSERT or UPDATE are
checked against the expression specified via WITH CHECK. Generally,
the system will enforce filter conditions imposed using security
policies prior to qualifications that appear in the query itself, in
order to the prevent the inadvertent exposure of the protected data to
user-defined functions which might not be trustworthy. However,
functions and operators marked by the system (or the system
administrator) as LEAKPROOF may be evaluated before policy
expressions, as they are assumed to be trustworthy.

Looks reasonable to me. Amit, does this read better for you? If so, I
can handle making the change to the docs.

Thanks!

Stephen

On Tue, Jan 6, 2015 at 11:25 AM, Stephen Frost <sfrost@snowman.net> wrote:

Looks reasonable to me. Amit, does this read better for you? If so, I
can handle making the change to the docs.

The docs also prominently say:

"The security-barrier qualifications will always be evaluated prior to
any user-defined functions or user-provided WHERE clauses, while the
with-check expression will be evaluated against the rows which are
going to be added to the table. By adding policies to a table, a user
can limit the rows which a given user can select, insert, update, or
delete. This capability is also known as Row Level Security or RLS."

I would prefer it if it was clearer based on the syntax description
which qual is which. The security barrier qual "expression" should
have an identifier/name in the syntax description that is more
suggestive of "security barrier qual", emphasizing its distinctness
from "check_expression". For example, I think "barrier_expression"
would be clearer.
--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Jan 6, 2015 at 2:48 PM, Peter Geoghegan <pg@heroku.com> wrote:

On Tue, Jan 6, 2015 at 11:25 AM, Stephen Frost <sfrost@snowman.net> wrote:

Looks reasonable to me. Amit, does this read better for you? If so, I
can handle making the change to the docs.

The docs also prominently say:

"The security-barrier qualifications will always be evaluated prior to
any user-defined functions or user-provided WHERE clauses, while the
with-check expression will be evaluated against the rows which are
going to be added to the table. By adding policies to a table, a user
can limit the rows which a given user can select, insert, update, or
delete. This capability is also known as Row Level Security or RLS."

I would prefer it if it was clearer based on the syntax description
which qual is which. The security barrier qual "expression" should
have an identifier/name in the syntax description that is more
suggestive of "security barrier qual", emphasizing its distinctness
from "check_expression". For example, I think "barrier_expression"
would be clearer.

I thought my rewrite clarified this distinction pretty well. Maybe
I'm wrong? We're talking about the same paragraph.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Jan 6, 2015 at 1:03 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I thought my rewrite clarified this distinction pretty well. Maybe
I'm wrong? We're talking about the same paragraph.

Sorry, I didn't express myself clearly. I think that you did get it
right, but I would like to see that distinction also reflected in the
actual sgml PARAMETER class tag. "expression" is way too generic here.

--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I also don't see this behavior documented (this is from process_policies()):

/*
* If we end up with only USING quals, then use those as
* WITH CHECK quals also.
*/
if (with_check_quals == NIL)
with_check_quals = copyObject(quals);

Now, I do see a reference to it under "Per-Command policies - ALL". It says:

"If an INSERT or UPDATE command attempts to add rows to the table
which do not pass the ALL WITH CHECK (or USING, if no WITH CHECK
expression is defined) expression, the command will error."

But is that really the right place for it? Does it not equally well
apply to FOR UPDATE policies, that can on their own have both barriers
quals and WITH CHECK options? Basically, that seems to me like a
*generic* property of policies (it's a generic thing that the WITH
CHECK options/expressions "shadow" the USING security barrier quals as
check options), and so should be documented as such.

--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 07-01-2015 AM 04:25, Stephen Frost wrote:

Robert, Amit,

* Robert Haas (robertmhaas@gmail.com) wrote:

I don't think that's a typo, although it's not particularly
well-worded IMHO. I might rewrite the whole paragraph like this:

A policy limits the ability to SELECT, INSERT, UPDATE, or DELETE rows
in a table to those rows which match the relevant policy expression.
Existing table rows are checked against the expression specified via
USING, while new rows that would be created via INSERT or UPDATE are
checked against the expression specified via WITH CHECK. Generally,
the system will enforce filter conditions imposed using security
policies prior to qualifications that appear in the query itself, in
order to the prevent the inadvertent exposure of the protected data to
user-defined functions which might not be trustworthy. However,
functions and operators marked by the system (or the system
administrator) as LEAKPROOF may be evaluated before policy
expressions, as they are assumed to be trustworthy.

Looks reasonable to me. Amit, does this read better for you? If so, I
can handle making the change to the docs.

Yes, it looks reasonable to me to.

Thanks,
Amit

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Jan 6, 2015 at 4:07 PM, Peter Geoghegan <pg@heroku.com> wrote:

On Tue, Jan 6, 2015 at 1:03 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I thought my rewrite clarified this distinction pretty well. Maybe
I'm wrong? We're talking about the same paragraph.

Sorry, I didn't express myself clearly. I think that you did get it
right, but I would like to see that distinction also reflected in the
actual sgml PARAMETER class tag. "expression" is way too generic here.

I'm happy to see us change that if it makes sense, but I'm not sure
what that actually does.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Robert Haas (robertmhaas@gmail.com) wrote:

On Tue, Jan 6, 2015 at 4:07 PM, Peter Geoghegan <pg@heroku.com> wrote:

On Tue, Jan 6, 2015 at 1:03 PM, Robert Haas <robertmhaas@gmail.com> wrote:

I thought my rewrite clarified this distinction pretty well. Maybe
I'm wrong? We're talking about the same paragraph.

Sorry, I didn't express myself clearly. I think that you did get it
right, but I would like to see that distinction also reflected in the
actual sgml PARAMETER class tag. "expression" is way too generic here.

I'm happy to see us change that if it makes sense, but I'm not sure
what that actually does.

If I'm following correctly, Peter's specifically talking about:

[ USING ( <replaceable class="parameter">expression</replaceable> ) ]
[ WITH CHECK ( <replaceable class="parameter">check_expression</replaceable> ) ]

Where the USING parameter is 'expression' but the WITH CHECK parameter
is 'check_expression'. He makes a good point, I believe, as
"expression" is overly generic. I don't like the idea of using
"barrier_expression" though as that ends up introducing a new term- how
about "using_expression"?

This would need to be reflected below in the documentation which talks
about "expression" as the parameter to "USING", but I can certainly
handle cleaning that up.

Thanks!

Stephen

On Wed, Jan 7, 2015 at 3:06 PM, Stephen Frost <sfrost@snowman.net> wrote:

If I'm following correctly, Peter's specifically talking about:

[ USING ( <replaceable class="parameter">expression</replaceable> ) ]
[ WITH CHECK ( <replaceable class="parameter">check_expression</replaceable> ) ]

Where the USING parameter is 'expression' but the WITH CHECK parameter
is 'check_expression'. He makes a good point, I believe, as
"expression" is overly generic. I don't like the idea of using
"barrier_expression" though as that ends up introducing a new term- how
about "using_expression"?

Oh. Well, I guess we could change that. I don't think it's a
problem, myself. I thought he was talking about something in the SGML
markup.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Peter Geoghegan (pg@heroku.com) wrote:

I also don't see this behavior documented (this is from process_policies()):

/*
* If we end up with only USING quals, then use those as
* WITH CHECK quals also.
*/
if (with_check_quals == NIL)
with_check_quals = copyObject(quals);

Now, I do see a reference to it under "Per-Command policies - ALL". It says:

"If an INSERT or UPDATE command attempts to add rows to the table
which do not pass the ALL WITH CHECK (or USING, if no WITH CHECK
expression is defined) expression, the command will error."

But is that really the right place for it? Does it not equally well
apply to FOR UPDATE policies, that can on their own have both barriers
quals and WITH CHECK options? Basically, that seems to me like a
*generic* property of policies (it's a generic thing that the WITH
CHECK options/expressions "shadow" the USING security barrier quals as
check options), and so should be documented as such.

Ah, yes, good point, I can add more documentation around that.

Thanks!

Stephen

On Wed, Jan 7, 2015 at 12:06 PM, Stephen Frost <sfrost@snowman.net> wrote:

Where the USING parameter is 'expression' but the WITH CHECK parameter
is 'check_expression'. He makes a good point, I believe, as
"expression" is overly generic. I don't like the idea of using
"barrier_expression" though as that ends up introducing a new term- how
about "using_expression"?

Yes, I think "using_expression" works best.

--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 6 January 2015 at 19:25, Stephen Frost <sfrost@snowman.net> wrote:

Robert, Amit,

* Robert Haas (robertmhaas@gmail.com) wrote:

I don't think that's a typo, although it's not particularly
well-worded IMHO. I might rewrite the whole paragraph like this:

A policy limits the ability to SELECT, INSERT, UPDATE, or DELETE rows
in a table to those rows which match the relevant policy expression.
Existing table rows are checked against the expression specified via
USING, while new rows that would be created via INSERT or UPDATE are
checked against the expression specified via WITH CHECK. Generally,
the system will enforce filter conditions imposed using security
policies prior to qualifications that appear in the query itself, in
order to the prevent the inadvertent exposure of the protected data to
user-defined functions which might not be trustworthy. However,
functions and operators marked by the system (or the system
administrator) as LEAKPROOF may be evaluated before policy
expressions, as they are assumed to be trustworthy.

Looks reasonable to me. Amit, does this read better for you? If so, I
can handle making the change to the docs.

I have a wider concern about the wording on this page - both the
rewritten paragraph and elsewhere talk about policies in terms of
limiting access to or filtering out rows.

However, since policy expressions are OR'ed together and there is a
default-deny policy when RLS is enabled, I think it should be talking
about policies in terms of permitting access to tables that have row
security enabled.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 8 January 2015 at 08:30, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

I have a wider concern about the wording on this page - both the
rewritten paragraph and elsewhere talk about policies in terms of
limiting access to or filtering out rows.

However, since policy expressions are OR'ed together and there is a
default-deny policy when RLS is enabled, I think it should be talking
about policies in terms of permitting access to tables that have row
security enabled.

[There's also a typo further down -- "filter out the records which are
visible", should be "not visible"]

What do you think of the attached rewording?

Regards,
Dean

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

[There's also a typo further down -- "filter out the records which are
visible", should be "not visible"]

What do you think of the attached rewording?

Rewording it this way is a great idea. Hopefully that will help address
the confusion which we've seen. The only comment I have offhand is:
should we should add a sentence to this paragraph about the default-deny
policy? I feel like that would help explain why the policies are
allowing access to rows.

Thanks!

Stephen

On 8 January 2015 at 18:57, Stephen Frost <sfrost@snowman.net> wrote:

What do you think of the attached rewording?

Rewording it this way is a great idea. Hopefully that will help address
the confusion which we've seen. The only comment I have offhand is:
should we should add a sentence to this paragraph about the default-deny
policy?

Yes, good idea, although I think perhaps that sentence should be added
to the preceding paragraph, after noting that RLS has to be enabled on
the table for the policies to be applied:

The <command>CREATE POLICY</command> command defines a new policy for a
table. Note that row level security must also be enabled on the table using
<command>ALTER TABLE</command> in order for created policies to be applied.
Once row level security has been enabled, a default-deny policy is
used and no rows
in the table are visible unless permitted by a specific policy.

A policy permits SELECT, INSERT, UPDATE or DELETE commands to access rows
in a table that has row level security enabled. Access to existing table
rows is granted if they match a policy expression specified via USING,
while new rows that would be created via INSERT or UPDATE are checked
against policy expressions specified via WITH CHECK. For policy
expressions specified via USING which grant access to existing rows, the
system will generally test the policy expressions prior to any
qualifications that appear in the query itself, in order to the prevent the
inadvertent exposure of the protected data to user-defined functions which
might not be trustworthy. However, functions and operators marked by the
system (or the system administrator) as LEAKPROOF may be evaluated before
policy expressions, as they are assumed to be trustworthy.

Also, perhaps the "ALTER TABLE" in the first paragraph should be
turned into a link.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:

On 8 January 2015 at 18:57, Stephen Frost <sfrost@snowman.net> wrote:

What do you think of the attached rewording?

Rewording it this way is a great idea. Hopefully that will help address
the confusion which we've seen. The only comment I have offhand is:
should we should add a sentence to this paragraph about the default-deny
policy?

Yes, good idea, although I think perhaps that sentence should be added
to the preceding paragraph, after noting that RLS has to be enabled on
the table for the policies to be applied:

I'm a bit on the fence about these ending up as different paragraphs
then, but ignoring that for the moment, I'd suggest we further clarify
with:

The <command>CREATE POLICY</command> command defines a new policy for a
table. Note that row level security must also be enabled on the table using
<command>ALTER TABLE</command> in order for created policies to be applied.
Once row level security has been enabled, a default-deny policy is used and
no rows in the table are visible, except to the table owner or
superuser, unless permitted by a specific policy.

A policy permits SELECT, INSERT, UPDATE or DELETE commands to access rows
in a table that has row level security enabled. Access to existing table
rows is granted if they match a policy expression specified via USING,
while new rows that would be created via INSERT or UPDATE are checked
against policy expressions specified via WITH CHECK. For policy
expressions specified via USING which grant access to existing rows, the
system will generally test the policy expressions prior to any
qualifications that appear in the query itself, in order to the prevent the
inadvertent exposure of the protected data to user-defined functions which
might not be trustworthy. However, functions and operators marked by the
system (or the system administrator) as LEAKPROOF may be evaluated before
policy expressions, as they are assumed to be trustworthy.

Also, perhaps the "ALTER TABLE" in the first paragraph should be
turned into a link.

Ah, yes, agreed.

Thanks!

Stephen

On 9 January 2015 at 20:46, Stephen Frost <sfrost@snowman.net> wrote:

I'd suggest we further clarify
with:

The <command>CREATE POLICY</command> command defines a new policy for a
table. Note that row level security must also be enabled on the table using
<command>ALTER TABLE</command> in order for created policies to be applied.
Once row level security has been enabled, a default-deny policy is used and
no rows in the table are visible, except to the table owner or
superuser, unless permitted by a specific policy.

A policy permits SELECT, INSERT, UPDATE or DELETE commands to access rows
in a table that has row level security enabled. Access to existing table
rows is granted if they match a policy expression specified via USING,
while new rows that would be created via INSERT or UPDATE are checked
against policy expressions specified via WITH CHECK. For policy
expressions specified via USING which grant access to existing rows, the
system will generally test the policy expressions prior to any
qualifications that appear in the query itself, in order to the prevent the
inadvertent exposure of the protected data to user-defined functions which
might not be trustworthy. However, functions and operators marked by the
system (or the system administrator) as LEAKPROOF may be evaluated before
policy expressions, as they are assumed to be trustworthy.

Looks good to me.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jan 9, 2015 at 3:46 PM, Stephen Frost <sfrost@snowman.net> wrote:

A policy permits SELECT, INSERT, UPDATE or DELETE commands to access rows
in a table that has row level security enabled. Access to existing table
rows is granted if they match a policy expression specified via USING,
while new rows that would be created via INSERT or UPDATE are checked
against policy expressions specified via WITH CHECK. For policy
expressions specified via USING which grant access to existing rows, the
system will generally test the policy expressions prior to any
qualifications that appear in the query itself, in order to the prevent the
inadvertent exposure of the protected data to user-defined functions which
might not be trustworthy. However, functions and operators marked by the
system (or the system administrator) as LEAKPROOF may be evaluated before
policy expressions, as they are assumed to be trustworthy.

I think that sticking "while new rows that would be created via INSERT
or UPDATE are checked against policy expressions specified via WITH
CHECK" into the middle of this is horribly confusing, as it's a
completely separate mechanism from the rest of what's being discussed
here. I think there needs to be some initial language that clarifies
that USING expressions apply to old rows and WITH CHECK expressions to
new rows, and then you can go into more detail. But mentioning WITH
CHECK parenthetically in the middle of the rest of this I think will
not lead to clarity.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers