row_security GUC does not behave as documented

Tom,

On Sunday, January 3, 2016, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The fine manual says that when row_security is set to off, "queries fail
which would otherwise apply at least one policy". However, a look at
check_enable_rls() says that that is a true statement only when the user
is not table owner. If the user *is* table owner, turning off
row_security seems to amount to just silently disabling RLS, even for
tables with FORCE ROW LEVEL SECURITY.

I am not sure if this is a documentation bug or a code bug, but it
sure looks to be one or the other.

The original reason for changing how row_security works was to avoid a
change in behavior based on a GUC changing. As such, I'm thinking that has
to be a code bug, as otherwise it would be a behavior change due to a GUC
being changed in the FORCE RLS case for table owners.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

On Sunday, January 3, 2016, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The fine manual says that when row_security is set to off, "queries fail
which would otherwise apply at least one policy". However, a look at
check_enable_rls() says that that is a true statement only when the user
is not table owner. If the user *is* table owner, turning off
row_security seems to amount to just silently disabling RLS, even for
tables with FORCE ROW LEVEL SECURITY.

I am not sure if this is a documentation bug or a code bug, but it
sure looks to be one or the other.

The original reason for changing how row_security works was to avoid a
change in behavior based on a GUC changing. As such, I'm thinking that has
to be a code bug, as otherwise it would be a behavior change due to a GUC
being changed in the FORCE RLS case for table owners.

Well, I tried changing the code to act the way I gather it should, and
it breaks a whole bunch of regression test cases. See attached.

regards, tom lane

Tom Lane wrote:

Stephen Frost <sfrost@snowman.net> writes:

On Sunday, January 3, 2016, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The fine manual says that when row_security is set to off, "queries fail
which would otherwise apply at least one policy". However, a look at
check_enable_rls() says that that is a true statement only when the user
is not table owner. If the user *is* table owner, turning off
row_security seems to amount to just silently disabling RLS, even for
tables with FORCE ROW LEVEL SECURITY.

I am not sure if this is a documentation bug or a code bug, but it
sure looks to be one or the other.

The original reason for changing how row_security works was to avoid a
change in behavior based on a GUC changing. As such, I'm thinking that has
to be a code bug, as otherwise it would be a behavior change due to a GUC
being changed in the FORCE RLS case for table owners.

Well, I tried changing the code to act the way I gather it should, and
it breaks a whole bunch of regression test cases. See attached.

I think this means we need to postpone 9.5.0 for a week.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Tom Lane wrote:

Well, I tried changing the code to act the way I gather it should, and
it breaks a whole bunch of regression test cases. See attached.

I think this means we need to postpone 9.5.0 for a week.

I think the regression cases are not that big a deal to fix, but
I'd rather that the original authors did it, as I might miss what
they intended to test.

regards, tom lane

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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

On Sunday, January 3, 2016, Tom Lane <tgl@sss.pgh.pa.us> wrote:

The fine manual says that when row_security is set to off, "queries fail
which would otherwise apply at least one policy". However, a look at
check_enable_rls() says that that is a true statement only when the user
is not table owner. If the user *is* table owner, turning off
row_security seems to amount to just silently disabling RLS, even for
tables with FORCE ROW LEVEL SECURITY.

I am not sure if this is a documentation bug or a code bug, but it
sure looks to be one or the other.

The original reason for changing how row_security works was to avoid a
change in behavior based on a GUC changing. As such, I'm thinking that has
to be a code bug, as otherwise it would be a behavior change due to a GUC
being changed in the FORCE RLS case for table owners.

Well, I tried changing the code to act the way I gather it should, and
it breaks a whole bunch of regression test cases. See attached.

Right, I wrote the code that way originally thinking that it didn't make
sense to throw a permission denied error when it's the owner, but I
hadn't been thinking about, at that time, how we don't want the GUC to
result in a behavior change.

As we don't want to end up with the same behavior-change-due-to-GUC that
we had with the original row_security implementation, we should change
the code as your patch does and update the regression tests accordingly.
Perhaps the error code thrown could be tailored a bit when it's the
owner, to indicate that FORCE RLS has been set on the table, but I'm not
sure it's really a big deal either way.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:

As we don't want to end up with the same behavior-change-due-to-GUC that
we had with the original row_security implementation, we should change
the code as your patch does and update the regression tests accordingly.

I think probably the tests need some adjustment rather than just stuffing
in the new results; but I'm unsure what's most appropriate.

Perhaps the error code thrown could be tailored a bit when it's the
owner, to indicate that FORCE RLS has been set on the table, but I'm not
sure it's really a big deal either way.

Yeah, the error message seemed less than apropos to me too; but on the
other hand there's an argument that FORCE RLS means "treat me just like
everybody else".

One idea would be to use the same primary error message either way,
but add a DETAIL or HINT mentioning FORCE RLS if it's the table owner.

regards, tom lane

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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Stephen Frost <sfrost@snowman.net> writes:

As we don't want to end up with the same behavior-change-due-to-GUC that
we had with the original row_security implementation, we should change
the code as your patch does and update the regression tests accordingly.

I think probably the tests need some adjustment rather than just stuffing
in the new results; but I'm unsure what's most appropriate.

Right, the comments, at least, need to be updated to be correct.

Perhaps the error code thrown could be tailored a bit when it's the
owner, to indicate that FORCE RLS has been set on the table, but I'm not
sure it's really a big deal either way.

Yeah, the error message seemed less than apropos to me too; but on the
other hand there's an argument that FORCE RLS means "treat me just like
everybody else".

Agreed.

One idea would be to use the same primary error message either way,
but add a DETAIL or HINT mentioning FORCE RLS if it's the table owner.

Having a detail or hint which indicates that seems like a great approach
to me.

Thanks!

Stephen