Thread: RLS open items are vague and unactionable

The open items list here:

https://wiki.postgresql.org/wiki/PostgreSQL_9.5_Open_Items

...currently has the following items related to RLS:

CREATE POLICY and RETURNING
Arguable RLS security bug, EvalPlanQual() paranoia
Dean's latest round of RLS refactoring.
more RLS oversights

Those items don't seem to have changed much in the last month, but I
think what I'm actually more concerned about is that I don't really
know what they mean or how serious they are.  Is there a way that we
can rephrase this list into a form such that, if somebody were
motivated to work on these issues, they'd be able to contribute?  And
if they were not motivated to work on them but at least wanted to
understand the situation, that would be easy?

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

On 10 September 2015 at 21:48, Robert Haas <robertmhaas@gmail.com> wrote:
> The open items list here:
>
> https://wiki.postgresql.org/wiki/PostgreSQL_9.5_Open_Items
>
> Dean's latest round of RLS refactoring.

This refactoring patch doesn't fix any behavioural issues. It is all
about trying to make the code simpler and more readable and
maintainable, and also addressing Alvaro's comments here:
http://www.postgresql.org/message-id/20150522180807.GB5885@postgresql.org

However, it has bit-rotted over the last 3 months. In particular it
doesn't take account of this change:
http://git.postgresql.org/pg/commitdiff/dee0200f0276c0f9da930a2c926f90f5615f2d64

I will happily work up a rebased version of the patch, but only if I
get a serious commitment from a committer to review it. Otherwise,
I'll let it drop.

Regards,
Dean

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 10 September 2015 at 21:48, Robert Haas <robertmhaas@gmail.com> wrote:
> > The open items list here:
> >
> > https://wiki.postgresql.org/wiki/PostgreSQL_9.5_Open_Items
> >
> > Dean's latest round of RLS refactoring.
>
> This refactoring patch doesn't fix any behavioural issues. It is all
> about trying to make the code simpler and more readable and
> maintainable, and also addressing Alvaro's comments here:
> http://www.postgresql.org/message-id/20150522180807.GB5885@postgresql.org
>
> However, it has bit-rotted over the last 3 months. In particular it
> doesn't take account of this change:
> http://git.postgresql.org/pg/commitdiff/dee0200f0276c0f9da930a2c926f90f5615f2d64
>
> I will happily work up a rebased version of the patch, but only if I
> get a serious commitment from a committer to review it. Otherwise,
> I'll let it drop.

There's no question about getting it reviewed and moving it forward;
either Joe or myself will certainly be able to handle that if others
don't step up.

I believe there's just a question about if it should be done for 9.5 or
only in master.

That said, addressing Alvaro's comments and avoiding unnecessary code
differences between the back branches and current might be sufficient
justification to move forward with it for 9.5 too.  I thought the
refactoring was a good change in general.

All,

I've updated the page to add more details about the various items,
though the only code changes at this point considered 'open' are this
refactoring and the question regarding the 'row-level-security disabled'
context which I posted a patch for discussion yesterday.

Comments and help on these would certainly be welcome, of course.  We're
working on a set of documentation updates to hopefully finish up in the
next week to add more details about RLS (additional sub-sections,
coverage of the issue Peter raised, additional discussion of RETURNING,
etc).

Thanks!

Stephen

On Fri, Sep 11, 2015 at 7:33 AM, Stephen Frost <sfrost@snowman.net> wrote:
> I've updated the page to add more details about the various items,
> though the only code changes at this point considered 'open' are this
> refactoring and the question regarding the 'row-level-security disabled'
> context which I posted a patch for discussion yesterday.
>
> Comments and help on these would certainly be welcome, of course.  We're
> working on a set of documentation updates to hopefully finish up in the
> next week to add more details about RLS (additional sub-sections,
> coverage of the issue Peter raised, additional discussion of RETURNING,
> etc).

Thanks for the updates.  My thoughts:

On RETURNING, it seems like we've got a fairly fundamental problem
here.  If I understand correctly, the intention of allowing policies
to be filtered by command type is to allow blind updates and deletes,
but this behavior means that they are not really blind.  You can
always use BEGIN/UPDATE-or-DELETE-with-RETURNING/ROLLBACK as a
substitute for SELECT.  So the only possible thing you can do with the
ability to filter by command tag that is coherent from a security
point of view is to make the update and delete predicates *tighter*
than the select predicate.

And if that's where we end up, then haven't we fundamentally
mis-designed the feature?  I mean, without the blind update case, it
just seems kooky to have different commands see different rows.  It
would be better to have all the command see the same rows, and then
have update/delete *error out* if you try to update a row you're not
allowed to touch.

On Dean's refactoring patch, I would tend to favor back-patching
whatever do there to 9.5, but I'm not volunteering to do the work.

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

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Sep 11, 2015 at 7:33 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > I've updated the page to add more details about the various items,
> > though the only code changes at this point considered 'open' are this
> > refactoring and the question regarding the 'row-level-security disabled'
> > context which I posted a patch for discussion yesterday.
> >
> > Comments and help on these would certainly be welcome, of course.  We're
> > working on a set of documentation updates to hopefully finish up in the
> > next week to add more details about RLS (additional sub-sections,
> > coverage of the issue Peter raised, additional discussion of RETURNING,
> > etc).
>
> Thanks for the updates.  My thoughts:

Certainly, happy to help.

> On RETURNING, it seems like we've got a fairly fundamental problem
> here.  If I understand correctly, the intention of allowing policies
> to be filtered by command type is to allow blind updates and deletes,

That's not correct, no, will clarify below.

> but this behavior means that they are not really blind.  You can
> always use BEGIN/UPDATE-or-DELETE-with-RETURNING/ROLLBACK as a
> substitute for SELECT.  So the only possible thing you can do with the
> ability to filter by command tag that is coherent from a security
> point of view is to make the update and delete predicates *tighter*
> than the select predicate.

The original intention and the primary expected use-case is to allow the
predicates to be tighter, yes.  What we're discussing here is if we want
to allow that flexibility at all or force users to have a single
visibility policy for all commands.

It's already possible to configure RLS with one visibility policy for
all commands- simply only create a USING clause for 'ALL' commands and
you're done.

The only reason to avoid providing that flexibility is the concern that
it might be misunderstood and users might misconfigure their system.
Removing the flexibility to have per-command visibility policies and
instead force a single visibility policy doesn't add any capabilities.

Further, from the discussion which Dean and I had over the summer and, I
believe, agreed on is that providing *both* a per-command visibility and
having an "overall" visibiility policy (as proposed nearby, where SELECT
always applies but then the other per-command policies are combined with
that policy) ends up being more difficult to reason about and explain
and simply doesn't add any new functionality.

> And if that's where we end up, then haven't we fundamentally
> mis-designed the feature?  I mean, without the blind update case, it
> just seems kooky to have different commands see different rows.  It
> would be better to have all the command see the same rows, and then
> have update/delete *error out* if you try to update a row you're not
> allowed to touch.

Having an error-out option which is based on the values of the existing
rows, instead of only allowing the error-out case based on the values of
the row being added to the relation, is certainly an interesting idea.

As being discussed nearby with Zhaomo (thread 'CREATE POLICY and
RETURNING', which ends up being a pretty bad subject, unfortunately), if
the WITH CHECK option supported referring to 'OLD' and 'NEW' then that
could be supported.  I'm not against having that capability, but it
seems like a new feature rather than an issue with the current
implementation.  That would also imply adding a 'WITH CHECK' clause to
DELETE, to support the "error-instead" option, but that also looks like
a new feature and one which could certainly be added later without any
backwards compatibility concerns, as far as I can see, rather than a
current issue.

Perhaps we would even allow such a 'WITH CHECK' to be applied to SELECT
statements to cause an error to be returned instead, though that would
imply that the visibility policy allows users to query records which
would end up just erroring out due to the 'WITH CHECK' policy and that
might end up providing the user with information about what's in the
relation that they aren't allowed to see due to the 'WITH CHECK' policy.
I can see how it would be useful when it's the intent of the
administrators to produce errors in cases where a user is trying to
access data they are not allowed to, as that could then be audited.  In
such a case, the actual visibility rule might be simply 'true', but an
error is thrown if the rows actually returned do not pass the
'WITH CHECK' policy specified.

> On Dean's refactoring patch, I would tend to favor back-patching
> whatever do there to 9.5, but I'm not volunteering to do the work.

Alright, I'll see about getting that done then.

Thanks!

Stephen

On 11 September 2015 at 13:05, Robert Haas <robertmhaas@gmail.com> wrote:
> Thanks for the updates.  My thoughts:
>
> On RETURNING, it seems like we've got a fairly fundamental problem
> here.  If I understand correctly, the intention of allowing policies
> to be filtered by command type is to allow blind updates and deletes,
> but this behavior means that they are not really blind.  You can
> always use BEGIN/UPDATE-or-DELETE-with-RETURNING/ROLLBACK as a
> substitute for SELECT.  So the only possible thing you can do with the
> ability to filter by command tag that is coherent from a security
> point of view is to make the update and delete predicates *tighter*
> than the select predicate.
>
> And if that's where we end up, then haven't we fundamentally
> mis-designed the feature?  I mean, without the blind update case, it
> just seems kooky to have different commands see different rows.  It
> would be better to have all the command see the same rows, and then
> have update/delete *error out* if you try to update a row you're not
> allowed to touch.
>

I think blind updates are a pretty niche case, and I think that it
wasn't the intention to support them, but more of an unintentional
side effect of the implementation. That said, there are
just-about-plausible use-cases where they might be considered useful,
e.g., allow a password to be nulled out, forcing a reset, but don't
allow the existing value to be read. But then, as you say, RETURNING
blows a hole in the security of that model.

I still think the answer is to make RETURNING subject to SELECT
policies, with an error thrown if you attempt a blind-update-returning
for a row not visible to you, e.g.:

DELETE FROM foo WHERE id=10; -- OK even if row 10 is not visible

DELETE FROM foo WHERE id=10 RETURNING *;
ERROR:  row returned by RETURNING is not visible using row level
security policies for "foo"

Regards,
Dean

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> I think blind updates are a pretty niche case, and I think that it
> wasn't the intention to support them, but more of an unintentional
> side effect of the implementation. That said, there are
> just-about-plausible use-cases where they might be considered useful,
> e.g., allow a password to be nulled out, forcing a reset, but don't
> allow the existing value to be read. But then, as you say, RETURNING
> blows a hole in the security of that model.
>
> I still think the answer is to make RETURNING subject to SELECT
> policies, with an error thrown if you attempt a blind-update-returning
> for a row not visible to you, e.g.:
>
> DELETE FROM foo WHERE id=10; -- OK even if row 10 is not visible
>
> DELETE FROM foo WHERE id=10 RETURNING *;
> ERROR:  row returned by RETURNING is not visible using row level
> security policies for "foo"

For a DELETE, applying the SELECT policy to RETURNING works, but it
doesn't work for UPDATE as the row being compared to the SELECT policy
would be the user-modified row and not the original; or at least, that's
what I recall from our discussion earlier in the summer.

Or are you suggesting that both UPDATE and DELETE apply the SELECT
policy, only when RETURNING is specified, to the original rows from the
table and throw an error if the row wouldn't be allowed per that policy?

That seems like it might be workable and is in-line with the regular
permissions system where we require SELECT rights if you specify
RETURNING but not otherwise (unless a predicate is specified, of
course), though my recollection is that there was disagreement about
having the RETURNING case throw errors rather than simply filtering the
records out (which gets us back to the discussion around applying a
single visibility policy).  Still, perhaps opinions have changed
regarding that.

Of course, it seems like that further limits the use-cases where the
blind updates could be done; though our existing support for similar
such cases (DELETE without a WHERE clause) is similairly limiting, so
perhaps that's not an issue.  This case is, perhaps, a bit different
since the user has the capability to explicitly specify the visibility
for the command either way (by either including or not including the
predicates in the SELECT policy in the UPDATE/DELETE policy), but we
don't currently support a way to alter the policy used based on the
existance of a RETURNING clause.  I had suggested supporting that quite
a while ago, but as I recall it wasn't well received.

Thanks!

Stephen

On 11 September 2015 at 15:20, Stephen Frost <sfrost@snowman.net> wrote:
> * Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
>> I think blind updates are a pretty niche case, and I think that it
>> wasn't the intention to support them, but more of an unintentional
>> side effect of the implementation. That said, there are
>> just-about-plausible use-cases where they might be considered useful,
>> e.g., allow a password to be nulled out, forcing a reset, but don't
>> allow the existing value to be read. But then, as you say, RETURNING
>> blows a hole in the security of that model.
>>
>> I still think the answer is to make RETURNING subject to SELECT
>> policies, with an error thrown if you attempt a blind-update-returning
>> for a row not visible to you, e.g.:
>>
>> DELETE FROM foo WHERE id=10; -- OK even if row 10 is not visible
>>
>> DELETE FROM foo WHERE id=10 RETURNING *;
>> ERROR:  row returned by RETURNING is not visible using row level
>> security policies for "foo"
>
> For a DELETE, applying the SELECT policy to RETURNING works, but it
> doesn't work for UPDATE as the row being compared to the SELECT policy
> would be the user-modified row and not the original; or at least, that's
> what I recall from our discussion earlier in the summer.
>
> Or are you suggesting that both UPDATE and DELETE apply the SELECT
> policy, only when RETURNING is specified, to the original rows from the
> table and throw an error if the row wouldn't be allowed per that policy?
>

Yes, that's what I was suggesting -- for UPDATE and DELETE with
RETURNING, test the OLD row against the table's SELECT policies. For
INSERT (or UPDATE and DELETE without RETURNING), do not check the
SELECT policies, allowing blind updates, but not blind updates with
RETURNING.


> That seems like it might be workable and is in-line with the regular
> permissions system where we require SELECT rights if you specify
> RETURNING but not otherwise (unless a predicate is specified, of
> course), though my recollection is that there was disagreement about
> having the RETURNING case throw errors rather than simply filtering the
> records out (which gets us back to the discussion around applying a
> single visibility policy).  Still, perhaps opinions have changed
> regarding that.
>

Yeah, we had a similar discussion regarding UPDATE USING policies and
ON CONFLICT UPDATE clauses. I think the argument against filtering is
that the rows returned would then be misleading about what was
actually updated.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> Yeah, we had a similar discussion regarding UPDATE USING policies and
> ON CONFLICT UPDATE clauses. I think the argument against filtering is
> that the rows returned would then be misleading about what was
> actually updated.

It seems to me that it would be a horribly bad idea to allow RLS to act
in such a way that rows could be updated and then not shown in RETURNING.

However, I don't see why UPDATE/DELETE with RETURNING couldn't be
restricted according to *both* the UPDATE and SELECT policies,
ie if there's RETURNING then you can't update a row you could not
have selected.  Note this would be a nothing-happens result not a
throw-error result, else you still leak info about the existence of
the row.
        regards, tom lane

On 09/11/2015 04:33 AM, Stephen Frost wrote:
> * Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
>> I will happily work up a rebased version of the patch, but only if I
>> get a serious commitment from a committer to review it. Otherwise,
>> I'll let it drop.
>
> There's no question about getting it reviewed and moving it forward;
> either Joe or myself will certainly be able to handle that if others
> don't step up.

Yes, we'll get it done.

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

On Fri, Sep 11, 2015 at 10:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
>> Yeah, we had a similar discussion regarding UPDATE USING policies and
>> ON CONFLICT UPDATE clauses. I think the argument against filtering is
>> that the rows returned would then be misleading about what was
>> actually updated.
>
> It seems to me that it would be a horribly bad idea to allow RLS to act
> in such a way that rows could be updated and then not shown in RETURNING.
>
> However, I don't see why UPDATE/DELETE with RETURNING couldn't be
> restricted according to *both* the UPDATE and SELECT policies,
> ie if there's RETURNING then you can't update a row you could not
> have selected.  Note this would be a nothing-happens result not a
> throw-error result, else you still leak info about the existence of
> the row.

Yes, this seems like an entirely reasonable way forward.

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

On Fri, Sep 11, 2015 at 9:48 AM, Stephen Frost <sfrost@snowman.net> wrote:
> The only reason to avoid providing that flexibility is the concern that
> it might be misunderstood and users might misconfigure their system.
> Removing the flexibility to have per-command visibility policies and
> instead force a single visibility policy doesn't add any capabilities.

That seems like an extremely weak argument.  If a feature can't be
used for anything useful, the fact that it doesn't actively interfere
with the use of other features that are useful is not a reason to keep
it.  Clearly, something needs to be done about this.  Saying, you can
restrict by something other than ALL but it adds no security and
serves no use cases is, frankly, a ridiculous position.  Tom's
proposal downthread is a reasonable one, and I endorse it: there may
be other approaches as well.  But regardless of the particular
approach, if we're going to have per-command policies, then you need
to do the work to make them useful.

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

On 11 September 2015 at 15:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
>> Yeah, we had a similar discussion regarding UPDATE USING policies and
>> ON CONFLICT UPDATE clauses. I think the argument against filtering is
>> that the rows returned would then be misleading about what was
>> actually updated.
>
> It seems to me that it would be a horribly bad idea to allow RLS to act
> in such a way that rows could be updated and then not shown in RETURNING.
>
> However, I don't see why UPDATE/DELETE with RETURNING couldn't be
> restricted according to *both* the UPDATE and SELECT policies,
> ie if there's RETURNING then you can't update a row you could not
> have selected.  Note this would be a nothing-happens result not a
> throw-error result, else you still leak info about the existence of
> the row.
>

That's what I was suggesting, except I was advocating a throw-error
result rather than a nothing-happens result.

Regarding the possibility of leaking info about the existence of rows,
that's something that already happens with INSERT if there are unique
indexes, and we've effectively decided there is nothing we can do
about it. So I don't buy that as an argument for doing nothing over
throwing an error.

My concern about doing nothing is how confusing it might be that an
UPDATE without RETURNING might update more rows than an UPDATE with
RETURNING and an identical WHERE clause. Throwing an error is much
more explicit about why you can't return those rows.

Ultimately I think this will be an extremely rare case, probably more
likely to happen as a result of accidentally misconfigured policies.
But if that does happen, I'd rather have an error to alert me to the
fact, than to silently do nothing.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On 11 September 2015 at 15:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> However, I don't see why UPDATE/DELETE with RETURNING couldn't be
>> restricted according to *both* the UPDATE and SELECT policies,
>> ie if there's RETURNING then you can't update a row you could not
>> have selected.  Note this would be a nothing-happens result not a
>> throw-error result, else you still leak info about the existence of
>> the row.

> That's what I was suggesting, except I was advocating a throw-error
> result rather than a nothing-happens result.

> Regarding the possibility of leaking info about the existence of rows,
> that's something that already happens with INSERT if there are unique
> indexes, and we've effectively decided there is nothing we can do
> about it. So I don't buy that as an argument for doing nothing over
> throwing an error.

Well, I don't buy your argument either.  The unique-index problem means
that if you have INSERT or UPDATE privilege, you can check for existence
of a row conflicting with a row that RLS would let you insert or update.
It does not mean that DELETE privilege would let you make that check.
Also, the unique-index problem only applies if there's a relevant unique
index, which doesn't necessarily have anything at all to do with the RLS
filter condition.

I think this is coming back to Robert's concern, that it is far from clear
that we understand the interactions of per-command RLS policies well
enough to ship them.  Maybe we should rip that out for 9.5 and only allow
a single RLS policy that that's the same for all commands.  We can always
add the more general facility later.

> Ultimately I think this will be an extremely rare case, probably more
> likely to happen as a result of accidentally misconfigured policies.
> But if that does happen, I'd rather have an error to alert me to the
> fact, than to silently do nothing.

I understand your concern about that, and it's reasonable.  But that
just leads me to the conclusion that maybe our ideas in this area are
not fully baked.
        regards, tom lane

On 11 September 2015 at 17:56, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
>> On 11 September 2015 at 15:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> However, I don't see why UPDATE/DELETE with RETURNING couldn't be
>>> restricted according to *both* the UPDATE and SELECT policies,
>>> ie if there's RETURNING then you can't update a row you could not
>>> have selected.  Note this would be a nothing-happens result not a
>>> throw-error result, else you still leak info about the existence of
>>> the row.
>
>> That's what I was suggesting, except I was advocating a throw-error
>> result rather than a nothing-happens result.
>
>> Regarding the possibility of leaking info about the existence of rows,
>> that's something that already happens with INSERT if there are unique
>> indexes, and we've effectively decided there is nothing we can do
>> about it. So I don't buy that as an argument for doing nothing over
>> throwing an error.
>
> Well, I don't buy your argument either.  The unique-index problem means
> that if you have INSERT or UPDATE privilege, you can check for existence
> of a row conflicting with a row that RLS would let you insert or update.
> It does not mean that DELETE privilege would let you make that check.

OK, the unique-index argument wasn't the best argument. How about
this:- if you have the DELETE privilege you can already check for the
existence of any row that RLS would let you delete just by looking at
the command status. The same is true for UPDATE. So throwing an error
when attempting to use RETURNING to see a row that's not visible to
you doesn't leak any more existence info than before.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

> Ultimately I think this will be an extremely rare case, probably more
> likely to happen as a result of accidentally misconfigured policies.
> But if that does happen, I'd rather have an error to alert me to the
> fact, than to silently do nothing.

I agree with Tom and Robert on this -- if we are going to allow
RETURNING on a DELETE or UPDATE of a table with RLS, the SELECT
policy must filter rows going to the DELETE or UPDATE phase and
silently ignore those which the user is not allowed to read.
Anything else seems crazy to me.

If we can't do that, I think we should prohibit RETURNING on DELETE
or UPDATE if there is RLS affecting the user's SELECTs.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 11 September 2015 at 15:51, Joe Conway <mail@joeconway.com> wrote:
> On 09/11/2015 04:33 AM, Stephen Frost wrote:
>> * Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
>>> I will happily work up a rebased version of the patch, but only if I
>>> get a serious commitment from a committer to review it. Otherwise,
>>> I'll let it drop.
>>
>> There's no question about getting it reviewed and moving it forward;
>> either Joe or myself will certainly be able to handle that if others
>> don't step up.
>
> Yes, we'll get it done.
>

OK, here's a rebased version of the patch.

There are no significant changes from last time this was discussed. I
believe the module regression test changes are harmless --- a result
of a change in the order that SB quals are added (internal policies
are now always added/checked before external ones), which can
influence qual pushdown.

Regards,
Dean

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> OK, here's a rebased version of the patch.

Thanks!

> There are no significant changes from last time this was discussed. I
> believe the module regression test changes are harmless --- a result
> of a change in the order that SB quals are added (internal policies
> are now always added/checked before external ones), which can
> influence qual pushdown.

I've been through this and definitely like it more than what we had
previously, as I had mentioned previously.  I've also added the
RETURNING handling, per the discussion between you, Robert, Tom, and
Kevin.

Would be great to get your feedback both on the relatively minor changes
which I made to your refactoring patch (mostly cosmetic) and how I added
in the handling for the RETURNING case, which was made much simpler and
cleaner by the refactoring.  (Working through adding the RETURNING also
helped my understanding of the refactoring, which I feel comfortable
that I understand well now.)

Thanks again!

Stephen

On 13 September 2015 at 20:59, Stephen Frost <sfrost@snowman.net> wrote:
> I've been through this and definitely like it more than what we had
> previously, as I had mentioned previously.  I've also added the
> RETURNING handling, per the discussion between you, Robert, Tom, and
> Kevin.
>
> Would be great to get your feedback both on the relatively minor changes
> which I made to your refactoring patch (mostly cosmetic) and how I added
> in the handling for the RETURNING case, which was made much simpler and
> cleaner by the refactoring.  (Working through adding the RETURNING also
> helped my understanding of the refactoring, which I feel comfortable
> that I understand well now.)
>

Cool. I haven't looked in detail yet, but I expected the changes
necessary to support RETURNING to be much simpler than that. Isn't it
just a case of adding something like

if (root->returningList &&   commandType == CMD_UPDATE or CMD_DELETE)
{   get_policies_for_relation(rel, CMD_SELECT, ...)   build_security_quals(...)
}

and then something similar with build_with_check_options() for the upsert case?

Then it isn't necessary to modify get_policies_for_relation(),
build_security_quals() and build_with_check_options() to know anything
specific about returning. They're just another set of permissive and
restrictive policies to be fetched and added to the command.

One change I thought about making was renaming build_security_quals()
and build_with_check_options() to add_security_quals() and
add_with_check_options(), because they're adding to lists rather than
returning lists, and in general they are called multiple times to
build up the complete lists.

Regards,
Dean

On 13 September 2015 at 20:59, Stephen Frost <sfrost@snowman.net> wrote:
> I've been through this and definitely like it more than what we had
> previously, as I had mentioned previously.  I've also added the
> RETURNING handling, per the discussion between you, Robert, Tom, and
> Kevin.
>
> Would be great to get your feedback both on the relatively minor changes
> which I made to your refactoring patch (mostly cosmetic) and how I added
> in the handling for the RETURNING case, which was made much simpler and
> cleaner by the refactoring.  (Working through adding the RETURNING also
> helped my understanding of the refactoring, which I feel comfortable
> that I understand well now.)
>

Cool. I haven't looked in detail yet, but I expected the changes
necessary to support RETURNING to be much simpler than that. Isn't it
just a case of adding something like

if (root->returningList &&
    commandType == CMD_UPDATE or CMD_DELETE)
{
    get_policies_for_relation(rel, CMD_SELECT, ...)
    build_security_quals(...)
}

and then something similar with build_with_check_options() for the upsert case?

Then it isn't necessary to modify get_policies_for_relation(),
build_security_quals() and build_with_check_options() to know anything
specific about returning. They're just another set of permissive and
restrictive policies to be fetched and added to the command.

One change I thought about making was renaming build_security_quals()
and build_with_check_options() to add_security_quals() and
add_with_check_options(), because they're adding to lists rather than
returning lists, and in general they are called multiple times to
build up the complete lists.

On 13 September 2015 at 22:54, Stephen Frost <sfrost@snowman.net> wrote:
> Not in front of my laptop and will review it when I get back in more detail,
> but the original approach that I tried was changing
> get_policies_for_relation to try and build everything necessary, which
> didn't work as we need to OR the various ALL/SELECT policies together and
> then AND the result to apply the filtering.
>

Yes that wouldn't work because get_policies_for_relation() isn't the
place for that. It has a clear responsibility which is to build and
return 2 lists of policies (permissive and restrictive) for the
specified relation, command type and user role. It doesn't have any
say in what is done with those policies (how their quals get
combined), it just fetches them.

It shouldn't be necessary to change get_policies_for_relation() at all
to support the RETURNING check. You just need to call it with
CMD_SELECT. BTW, your change to change get_policies_for_relation() has
a bug -- if the policy is for ALL commands it gets added
unconditionally to the list of returning_policies, regardless of
whether it applies to the current user role. That's the risk of
complicating the function by trying to make it do more than it was
designed to do.


> Seems like that might not be an issue with your proposed approach, but
> wouldn't we need to either pass in fresh lists and then append them to the
> existing lists, or change the functions to work with non-empty lists? (I had
> thought about changing that anyway since there's often cases where it's
> useful to be able to call a function which adds to an existing list).
> Further, actually, we'd still have to figure out how to build up the OR'd
> qual from the ALL/SELECT policies and then add that to the restrictive set.
> That didn't appear easy to do from get_policies_for_relation as all the
> ChangeVarNode work is handled in the build_* functions, which have the info
> necessary.
>

No, the lists of policies built and returned by
get_policies_for_relation() should not be appended to any other lists.
That would have the effect of OR'ing the SELECT policy quals with the
UPDATE/DELETE policy quals, which is wrong. The user needs to have
both SELECT and UPDATE/DELETE permissions on each row, so the OR'ed
set of permissive SELECT quals needs to be AND'ed with the OR'ed set
of permissive UPDATE/DELETE quals. The build_* functions do precisely
what is needed for that, and shouldn't need changing either (other
than the rename discussed previously).

So for example, build^H^H^Hadd_security_quals() takes 2 lists of
policies (permissive and restrictive), combines them in the right way
using OR and AND, does the necessary varno manipulation, and adds the
resulting quals to the passed-in list of security quals (thereby
implicitly AND'ing the new quals with any pre-existing ones), which is
precisely what's needed to support RETURNING.


>> Then it isn't necessary to modify get_policies_for_relation(),
>> build_security_quals() and build_with_check_options() to know anything
>> specific about returning. They're just another set of permissive and
>> restrictive policies to be fetched and added to the command.
>
>
> The ALL/SELECT policies need to be combined with OR's (just like all
> permissive sets of policies) and then added to the restrictive set of quals,
> to ensure that they are evaluated as a restriction and not just another set
> of permissive policies, which wouldn't provide the required filtering.
>

Right, and that's what the add_* functions do. The separation of
concerns between get_policies_for_relation() and the add_* functions
was intended to make just this kind of change trivial at a higher
level.

I think this should actually be 2 separate commits, since the
refactoring and the support for RETURNING are entirely different
things. It just happens that after the refactoring, the RETURNING
patch becomes trivial (4 new executable lines of code wrapped in a
couple of if statements, to fetch and then apply the new policies in
the necessary cases). At least that's the theory :-)

Regards,
Dean

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> It shouldn't be necessary to change get_policies_for_relation() at all
> to support the RETURNING check. You just need to call it with
> CMD_SELECT.

Yup, that works well.

> BTW, your change to change get_policies_for_relation() has
> a bug -- if the policy is for ALL commands it gets added
> unconditionally to the list of returning_policies, regardless of
> whether it applies to the current user role. That's the risk of
> complicating the function by trying to make it do more than it was
> designed to do.

Yeah, I had added the lappend's directly under the individual commands
at first, thinking back to when we were doing the per-role check there
instead of later and when I realized how you changed it to happen later
I went back and updated them, but apparently missed the 'all' case.

> So for example, build^H^H^Hadd_security_quals() takes 2 lists of
> policies (permissive and restrictive), combines them in the right way
> using OR and AND, does the necessary varno manipulation, and adds the
> resulting quals to the passed-in list of security quals (thereby
> implicitly AND'ing the new quals with any pre-existing ones), which is
> precisely what's needed to support RETURNING.

Right, it just needs to be called twice to have that happen correctly.

> I think this should actually be 2 separate commits, since the
> refactoring and the support for RETURNING are entirely different
> things. It just happens that after the refactoring, the RETURNING
> patch becomes trivial (4 new executable lines of code wrapped in a
> couple of if statements, to fetch and then apply the new policies in
> the necessary cases). At least that's the theory :-)

I had been planning to do them as independent commits in the end, but
thought it easier to review one patch, particularly when the differences
were larger.  I've now reworked adding the RETURNING handling without
changing the other functions, per discussion.

Attached is a git format-patch built series which includes both commits,
now broken out, for review.

Thanks!

Stephen

On 14 September 2015 at 14:47, Stephen Frost <sfrost@snowman.net> wrote:
> Attached is a git format-patch built series which includes both commits,
> now broken out, for review.
>

That looks OK to me.

A minor point -- this comment isn't quite right:
   /*    * For the target relation, when there is a returning list, we need to    * collect up CMD_SELECT policies to
addvia add_security_quals and    * add_with_check_options.  This is because, for the RETURNING case, we    * have to
filterany records which are not visible through an ALL or SELECT    * USING policy.    *    * We don't need to worry
aboutthe non-target relation case because we are    * checking the ALL and SELECT policies for those relations anyway
(see   * above).    */
 

because the policies that are fetched there are only used for
add_security_quals(), not for add_with_check_options(). It might be
cleaner if the 'if' statement that follows were merged with the
identical one a few lines down, and then those returning policies
could be local to that block, with the 2 pieces of RETURNING handling
done together. Similarly for the upsert block.

Actually, it isn't necessary to test that rt_index ==
root->resultRelation, because for all other relations commandType is
set to CMD_SELECT higher up, so the 'returning' bool variable could
just be replaced with 'root->returningList != NIL' throughout.

Regards,
Dean

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 14 September 2015 at 14:47, Stephen Frost <sfrost@snowman.net> wrote:
> > Attached is a git format-patch built series which includes both commits,
> > now broken out, for review.
>
> That looks OK to me.

Excellent.

> A minor point -- this comment isn't quite right:
>
>     /*
>      * For the target relation, when there is a returning list, we need to
>      * collect up CMD_SELECT policies to add via add_security_quals and
>      * add_with_check_options.  This is because, for the RETURNING case, we
>      * have to filter any records which are not visible through an ALL or SELECT
>      * USING policy.
>      *
>      * We don't need to worry about the non-target relation case because we are
>      * checking the ALL and SELECT policies for those relations anyway (see
>      * above).
>      */
>
> because the policies that are fetched there are only used for
> add_security_quals(), not for add_with_check_options(). It might be
> cleaner if the 'if' statement that follows were merged with the
> identical one a few lines down, and then those returning policies
> could be local to that block, with the 2 pieces of RETURNING handling
> done together. Similarly for the upsert block.

Hmm, ok, will take a look at doing that.

> Actually, it isn't necessary to test that rt_index ==
> root->resultRelation, because for all other relations commandType is
> set to CMD_SELECT higher up, so the 'returning' bool variable could
> just be replaced with 'root->returningList != NIL' throughout.

I had thought something similar originally and ran into a case where
that didn't quite work.  That was a few revisions ago though, so perhaps
there was something else going on.  I'll take a look at making this
change also (which was actually how I had implemented it initially).

I'll be offline for a few hours as I'm about to fly to Dallas, but I'll
get to this tomorrow morning, at the latest.

Thanks!

Stephen

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> A minor point -- this comment isn't quite right:

Fixed.

> because the policies that are fetched there are only used for
> add_security_quals(), not for add_with_check_options(). It might be
> cleaner if the 'if' statement that follows were merged with the
> identical one a few lines down, and then those returning policies
> could be local to that block, with the 2 pieces of RETURNING handling
> done together. Similarly for the upsert block.

Done.

> Actually, it isn't necessary to test that rt_index ==
> root->resultRelation, because for all other relations commandType is
> set to CMD_SELECT higher up, so the 'returning' bool variable could
> just be replaced with 'root->returningList != NIL' throughout.

Done.

Updated patch attached for review.

Unless there are other concerns or issues raised, I'll push this later
today.

Thanks!

Stephen

On 15 September 2015 at 15:22, Stephen Frost <sfrost@snowman.net> wrote:
> Updated patch attached for review.
>
> Unless there are other concerns or issues raised, I'll push this later
> today.
>

Looks good to me.
Thanks for doing all this.

Regards,
Dean

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 15 September 2015 at 15:22, Stephen Frost <sfrost@snowman.net> wrote:
> > Updated patch attached for review.
> >
> > Unless there are other concerns or issues raised, I'll push this later
> > today.
>
> Looks good to me.

Excellent, pushed!

> Thanks for doing all this.

Thank *you* for your continued work in this area and with PG in general.

Stephen

On Tue, Sep 15, 2015 at 10:22 AM, Stephen Frost <sfrost@snowman.net> wrote:
> Dean,
>
> * Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
>> A minor point -- this comment isn't quite right:
>
> Fixed.
>
>> because the policies that are fetched there are only used for
>> add_security_quals(), not for add_with_check_options(). It might be
>> cleaner if the 'if' statement that follows were merged with the
>> identical one a few lines down, and then those returning policies
>> could be local to that block, with the 2 pieces of RETURNING handling
>> done together. Similarly for the upsert block.
>
> Done.
>
>> Actually, it isn't necessary to test that rt_index ==
>> root->resultRelation, because for all other relations commandType is
>> set to CMD_SELECT higher up, so the 'returning' bool variable could
>> just be replaced with 'root->returningList != NIL' throughout.
>
> Done.
>
> Updated patch attached for review.
>
> Unless there are other concerns or issues raised, I'll push this later
> today.

So does this mean that the first RLS open item is addressed?  If so,
can it be moved to the "resolved after 9.5alpha2" section?  Based on
commit 4f3b2a8883c47b6710152a8e157f8a02656d0e68 I *think* yes but...

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

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Tue, Sep 15, 2015 at 10:22 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > Unless there are other concerns or issues raised, I'll push this later
> > today.
>
> So does this mean that the first RLS open item is addressed?  If so,
> can it be moved to the "resolved after 9.5alpha2" section?  Based on
> commit 4f3b2a8883c47b6710152a8e157f8a02656d0e68 I *think* yes but...

I hadn't moved it because there was ongoing discussion and I had an open
item (see: 20150923185403.GC3685@tamriel.snowman.net and the thread
leading up to it).

Attached is a patch to address exactly that issue.  This is all in the
commit message, of course, but the gist of it is:

If SELECT rights are required then apply the SELECT policies, even if
the actual command is an UPDATE or DELETE.  This covers the RETURNING
case which was discussed previously, so we don't need the explicit check
for that, and further addresses the concern raised by Zhaomo about
someone abusing the WHERE clause in an UPDATE or DELETE.

Further, if UPDATE rights are required then apply the UPDATE policies,
even if the actual command is a SELECT.  This addresses the concern that
a user might be able to lock rows they're not actually allowed to UPDATE
through the UPDATE policies.

Comments welcome, of course.  Barring concerns, I'll get this pushed
tomorrow.

Thanks!

Stephen

On Mon, Sep 28, 2015 at 03:03:51PM -0400, Stephen Frost wrote:
> If SELECT rights are required then apply the SELECT policies, even if
> the actual command is an UPDATE or DELETE.  This covers the RETURNING
> case which was discussed previously, so we don't need the explicit check
> for that, and further addresses the concern raised by Zhaomo about
> someone abusing the WHERE clause in an UPDATE or DELETE.
> 
> Further, if UPDATE rights are required then apply the UPDATE policies,
> even if the actual command is a SELECT.  This addresses the concern that
> a user might be able to lock rows they're not actually allowed to UPDATE
> through the UPDATE policies.
> 
> Comments welcome, of course.  Barring concerns, I'll get this pushed
> tomorrow.

The CREATE POLICY reference page continues to describe the behavior this patch
replaced, not today's behavior.

Noah,

* Noah Misch (noah@leadboat.com) wrote:
> On Mon, Sep 28, 2015 at 03:03:51PM -0400, Stephen Frost wrote:
> > If SELECT rights are required then apply the SELECT policies, even if
> > the actual command is an UPDATE or DELETE.  This covers the RETURNING
> > case which was discussed previously, so we don't need the explicit check
> > for that, and further addresses the concern raised by Zhaomo about
> > someone abusing the WHERE clause in an UPDATE or DELETE.
> >
> > Further, if UPDATE rights are required then apply the UPDATE policies,
> > even if the actual command is a SELECT.  This addresses the concern that
> > a user might be able to lock rows they're not actually allowed to UPDATE
> > through the UPDATE policies.
> >
> > Comments welcome, of course.  Barring concerns, I'll get this pushed
> > tomorrow.
>
> The CREATE POLICY reference page continues to describe the behavior this patch
> replaced, not today's behavior.

Just to be clear, I'm not ignoring this, I've been working to try and
rework the RLS documentation to add more information to the main RLS
section and to better segregate out the general RLS documentation out
from what should really be on the CREATE POLICY page.

This update will be incorporated into that and I'll be posting the whole
thing to -docs soon for comment.

Thanks!

Stephen