[HACKERS] row_security GUC does not behave as documented

[Tom Lane]

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.

Meanwhile, there's a statement about row_security in ddl.sgml that is so
vague as to be nearly meaningless, but it doesn't seem to quite match
either of those interpretations.  I'm in the midst of copy-editing that
section and will make it match what the code actually does at the moment,
but we'll have to change it again if this is deemed a code bug.

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

Re: [HACKERS] row_security GUC does not behave as documented

[Alvaro Herrera]

Tom Lane wrote:
> Stephen Frost  writes:
> > On Sunday, January 3, 2016, Tom Lane  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 Herrerahttp://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

Re: [HACKERS] row_security GUC does not behave as documented

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > On Sunday, January 3, 2016, Tom Lane  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


signature.asc
Description: Digital signature

Re: [HACKERS] row_security GUC does not behave as documented

[Tom Lane]

Alvaro Herrera  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

Re: [HACKERS] row_security GUC does not behave as documented

[Stephen Frost]

Tom,

On Sunday, January 3, 2016, Tom Lane  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

Re: [HACKERS] row_security GUC does not behave as documented

[Tom Lane]

Stephen Frost  writes:
> On Sunday, January 3, 2016, Tom Lane  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

diff --git a/src/backend/utils/misc/rls.c b/src/backend/utils/misc/rls.c
index b6c1d75..4e7b4d1 100644
*** a/src/backend/utils/misc/rls.c
--- b/src/backend/utils/misc/rls.c
*** check_enable_rls(Oid relid, Oid checkAsU
*** 59,67 
  	Oid			user_id = checkAsUser ? checkAsUser : GetUserId();
  
  	/* Nothing to do for built-in relations */
! 	if (relid < FirstNormalObjectId)
  		return RLS_NONE;
  
  	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
  	if (!HeapTupleIsValid(tuple))
  		return RLS_NONE;
--- 59,68 
  	Oid			user_id = checkAsUser ? checkAsUser : GetUserId();
  
  	/* Nothing to do for built-in relations */
! 	if (relid < (Oid) FirstNormalObjectId)
  		return RLS_NONE;
  
+ 	/* Fetch relation's relrowsecurity and relforcerowsecurity flags */
  	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
  	if (!HeapTupleIsValid(tuple))
  		return RLS_NONE;
*** check_enable_rls(Oid relid, Oid checkAsU
*** 88,96 
  		return RLS_NONE_ENV;
  
  	/*
! 	 * Table owners generally bypass RLS, except if row_security=true and the
! 	 * table has been set (by an owner) to FORCE ROW SECURITY, and this is not
! 	 * a referential integrity check.
  	 *
  	 * Return RLS_NONE_ENV to indicate that this decision depends on the
  	 * environment (in this case, the user_id).
--- 89,97 
  		return RLS_NONE_ENV;
  
  	/*
! 	 * Table owners generally bypass RLS, except if the table has been set (by
! 	 * an owner) to FORCE ROW SECURITY, and this is not a referential
! 	 * integrity check.
  	 *
  	 * Return RLS_NONE_ENV to indicate that this decision depends on the
  	 * environment (in this case, the user_id).
*** check_enable_rls(Oid relid, Oid checkAsU
*** 98,128 
  	if (pg_class_ownercheck(relid, user_id))
  	{
  		/*
! 		 * If row_security=true and FORCE ROW LEVEL SECURITY has been set on
! 		 * the relation then we return RLS_ENABLED to indicate that RLS should
! 		 * still be applied.  If we are in a SECURITY_NOFORCE_RLS context or if
! 		 * row_security=false then we return RLS_NONE_ENV.
  		 *
! 		 * The SECURITY_NOFORCE_RLS indicates that we should not apply RLS even
! 		 * if the table has FORCE RLS set- IF the current user is the owner.
! 		 * This is specifically to ensure that referential integrity checks are
! 		 * able to still run correctly.
  		 *
  		 * This is intentionally only done after we have checked that the user
  		 * is the table owner, which should always be the case for referential
  		 * integrity checks.
  		 */
! 		if (row_security && relforcerowsecurity && !InNoForceRLSOperation())
! 			return RLS_ENABLED;
! 		else
  			return RLS_NONE_ENV;
  	}
  
! 	/* row_security GUC says to bypass RLS, but user lacks permission */
  	if (!row_security && !noError)
  		ereport(ERROR,
  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
!  errmsg("insufficient privilege to bypass row-level security")));
  
  	/* RLS should be fully enabled for this relation. */
  	return RLS_ENABLED;
--- 99,130 
  	if (pg_class_ownercheck(relid, user_id))
  	{
  		/*
! 		 * If FORCE ROW LEVEL SECURITY has been set on the relation then we
! 		 * should return RLS_ENABLED to indicate that RLS should be applied.
! 		 * If not, or if we are in an InNoForceRLSOperation context, we return
! 		 * RLS_NONE_ENV.
  		 *
! 		 * InNoForceRLSOperation indicates that we should not apply RLS even
! 		 * if the table has FORCE RLS set - IF the current user is the owner.
! 		 * This is specifically to ensure that referential integrity checks
! 		 * are able to still run correctly.
  		 *
  		 * This is intentionally only done after we have checked that the user
  		 * is the table owner, which should always be the case for referential
  		 * integrity checks.
  		 */
! 		if (!relforcerowsecurity || InNoForceRLSOperation())
  			return RLS_NONE_ENV;
  	}
  
! 	/*
! 	 * We should apply RLS.  

Re: [HACKERS] row_security GUC does not behave as documented

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  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


signature.asc
Description: Digital signature

Re: [HACKERS] row_security GUC does not behave as documented

[Tom Lane]

Stephen Frost  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