Обсуждение: 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
Tom,
On Sunday, January 3, 2016, Tom Lane <tgl@sss.pgh.pa.us> wrote:
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 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 ! * GUC to get a forced error instead. ! */ 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; *** /home/postgres/pgsql/src/test/regress/expected/rowsecurity.out Sat Dec 19 16:47:11 2015 --- /home/postgres/pgsql/src/test/regress/results/rowsecurity.out Sun Jan 3 21:56:18 2016 *************** *** 3217,3244 **** SET row_security = off; -- Shows all rows TABLE r1; ! a ! ---- ! 10 ! 20 ! (2 rows) ! -- Update all rows UPDATE r1 SET a = 1; TABLE r1; ! a ! --- ! 1 ! 1 ! (2 rows) ! -- Delete all rows DELETE FROM r1; TABLE r1; ! a ! --- ! (0 rows) ! DROP TABLE r1; -- -- FORCE ROW LEVEL SECURITY does not break RI --- 3217,3233 ---- SET row_security = off; -- Shows all rows TABLE r1; ! ERROR: insufficient privilege to bypass row-level security -- Update all rows UPDATE r1 SET a = 1; + ERROR: insufficient privilege to bypass row-level security TABLE r1; ! ERROR: insufficient privilege to bypass row-level security -- Delete all rows DELETE FROM r1; + ERROR: insufficient privilege to bypass row-level security TABLE r1; ! ERROR: insufficient privilege to bypass row-level security DROP TABLE r1; -- -- FORCE ROW LEVEL SECURITY does not break RI *************** *** 3351,3362 **** SET row_security = off; -- Rows shown now TABLE r1; ! a ! ---- ! 10 ! 20 ! (2 rows) ! SET row_security = on; -- Error INSERT INTO r1 VALUES (10), (20) RETURNING *; --- 3340,3346 ---- SET row_security = off; -- Rows shown now TABLE r1; ! ERROR: insufficient privilege to bypass row-level security SET row_security = on; -- Error INSERT INTO r1 VALUES (10), (20) RETURNING *; *************** *** 3379,3402 **** -- Show updated rows SET row_security = off; TABLE r1; ! a ! ---- ! 30 ! (1 row) ! -- reset value in r1 for test with RETURNING UPDATE r1 SET a = 10; -- Verify row reset TABLE r1; ! a ! ---- ! 10 ! (1 row) ! SET row_security = on; -- Error UPDATE r1 SET a = 30 RETURNING *; ! ERROR: new row violates row-level security policy for table "r1" DROP TABLE r1; -- Check dependency handling RESET SESSION AUTHORIZATION; --- 3363,3382 ---- -- Show updated rows SET row_security = off; TABLE r1; ! ERROR: insufficient privilege to bypass row-level security -- reset value in r1 for test with RETURNING UPDATE r1 SET a = 10; + ERROR: insufficient privilege to bypass row-level security -- Verify row reset TABLE r1; ! ERROR: insufficient privilege to bypass row-level security SET row_security = on; -- Error UPDATE r1 SET a = 30 RETURNING *; ! a ! --- ! (0 rows) ! DROP TABLE r1; -- Check dependency handling RESET SESSION AUTHORIZATION; ======================================================================
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
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
* 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
* 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