[HACKERS] row_security GUC does not behave as documented
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
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
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. However, the user may turn off the row_security
Re: [HACKERS] row_security GUC does not behave as documented
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
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
* 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
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
Re: [HACKERS] row_security GUC does not behave as documented
* 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