Dean, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: > Attached is a patch to make RLS checks run before attempting to > insert/update any data rather than afterwards.
Excellent, this I really like and it's a pretty straight-forward change.
I wonder if there are some documentation updates which need to be done
for this also? I'm planning to look as I vauguely recall mentioning the
ordering of operations somewhere along the way.
I also addressed the bitrot from the column-priv leak patch. Would be
great to have you take a look at the latest and let me know if you have
any further comments or suggestions. I'm definitely looking forward to
getting these changes in soon.
Thanks!
Stephen
From 9fb54d9119d83b00cc7839efff573d313c6fa8e5 Mon Sep 17 00:00:00 2001
From: Stephen Frost <sfr...@snowman.net>
Date: Wed, 25 Feb 2015 23:58:22 -0500
Subject: [PATCH] Run RLS checks before attempting to insert/update
The initial approach with RLS leveraged the existing WITH CHECK
OPTION capability, which meant that the tuple was essentially fully
inserted and only after that was done was it checked for policies. WITH
CHECK OPTION is required to do that per the SQL spec, but RLS would
ideally be done earlier.
This changes the RLS policies to be done prior to attempting to insert
or update the tuple, which means that an RLS violation will be thrown
(if the policy is violated) instead of, say, a primary key constraint
violation. This approach still leverages a great deal of the WITH CHECK
OPTION infrastructure, but modifies it to understand when it's operating
with RLS policies or WITH CHECK options, which allows it to produce
better error messages.
Dean Rasheed
---
src/backend/executor/execMain.c | 34 ++++++++++++++++++++++++-------
src/backend/executor/nodeModifyTable.c | 32 +++++++++++++++++++++--------
src/backend/nodes/copyfuncs.c | 3 ++-
src/backend/nodes/equalfuncs.c | 3 ++-
src/backend/nodes/outfuncs.c | 3 ++-
src/backend/nodes/readfuncs.c | 3 ++-
src/backend/rewrite/rewriteHandler.c | 3 ++-
src/backend/rewrite/rowsecurity.c | 8 ++++++--
src/include/executor/executor.h | 2 +-
src/include/nodes/execnodes.h | 2 +-
src/include/nodes/parsenodes.h | 15 +++++++++++---
src/test/regress/expected/rowsecurity.out | 9 ++++++--
src/test/regress/sql/rowsecurity.sql | 4 ++++
13 files changed, 91 insertions(+), 30 deletions(-)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 33b172b..5c0ae39 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1667,9 +1667,10 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
/*
* ExecWithCheckOptions -- check that tuple satisfies any WITH CHECK OPTIONs
+ * of the specified kind.
*/
void
-ExecWithCheckOptions(ResultRelInfo *resultRelInfo,
+ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate)
{
Relation rel = resultRelInfo->ri_RelationDesc;
@@ -1694,6 +1695,9 @@ ExecWithCheckOptions(ResultRelInfo *resultRelInfo,
WithCheckOption *wco = (WithCheckOption *) lfirst(l1);
ExprState *wcoExpr = (ExprState *) lfirst(l2);
+ if (wco->kind != kind)
+ continue;
+
/*
* WITH CHECK OPTION checks are intended to ensure that the new tuple
* is visible (in the case of a view) or that it passes the
@@ -1715,12 +1719,28 @@ ExecWithCheckOptions(ResultRelInfo *resultRelInfo,
modifiedCols,
64);
- ereport(ERROR,
- (errcode(ERRCODE_WITH_CHECK_OPTION_VIOLATION),
- errmsg("new row violates WITH CHECK OPTION for \"%s\"",
- wco->viewname),
- val_desc ? errdetail("Failing row contains %s.", val_desc) :
- 0));
+ switch (wco->kind)
+ {
+ case WCO_VIEW_CHECK:
+ ereport(ERROR,
+ (errcode(ERRCODE_WITH_CHECK_OPTION_VIOLATION),
+ errmsg("new row violates WITH CHECK OPTION for \"%s\"",
+ wco->relname),
+ val_desc ? errdetail("Failing row contains %s.",
+ val_desc) : 0));
+ case WCO_RLS_INSERT_CHECK:
+ case WCO_RLS_UPDATE_CHECK:
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("new row violates row level security policy for \"%s\"",
+ wco->relname),
+ val_desc ? errdetail("Failing row contains %s.",
+ val_desc) : 0));
+ break;
+ default:
+ elog(ERROR, "unrecognized WCO kind: %u", wco->kind);
+ break;
+ }
}
}
}
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index f96fb24..be879a4 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -253,6 +253,13 @@ ExecInsert(TupleTableSlot *slot,
tuple->t_tableOid = RelationGetRelid(resultRelationDesc);
/*
+ * Check any RLS INSERT WITH CHECK policies
+ */
+ if (resultRelInfo->ri_WithCheckOptions != NIL)
+ ExecWithCheckOptions(WCO_RLS_INSERT_CHECK,
+ resultRelInfo, slot, estate);
+
+ /*
* Check the constraints of the tuple
*/
if (resultRelationDesc->rd_att->constr)
@@ -287,9 +294,9 @@ ExecInsert(TupleTableSlot *slot,
list_free(recheckIndexes);
- /* Check any WITH CHECK OPTION constraints */
+ /* Check any WITH CHECK OPTION constraints from parent views */
if (resultRelInfo->ri_WithCheckOptions != NIL)
- ExecWithCheckOptions(resultRelInfo, slot, estate);
+ ExecWithCheckOptions(WCO_VIEW_CHECK, resultRelInfo, slot, estate);
/* Process RETURNING if present */
if (resultRelInfo->ri_projectReturning)
@@ -653,15 +660,22 @@ ExecUpdate(ItemPointer tupleid,
tuple->t_tableOid = RelationGetRelid(resultRelationDesc);
/*
- * Check the constraints of the tuple
+ * Check any RLS UPDATE WITH CHECK policies
*
* If we generate a new candidate tuple after EvalPlanQual testing, we
- * must loop back here and recheck constraints. (We don't need to
- * redo triggers, however. If there are any BEFORE triggers then
- * trigger.c will have done heap_lock_tuple to lock the correct tuple,
- * so there's no need to do them again.)
+ * must loop back here and recheck any RLS policies and constraints.
+ * (We don't need to redo triggers, however. If there are any BEFORE
+ * triggers then trigger.c will have done heap_lock_tuple to lock the
+ * correct tuple, so there's no need to do them again.)
*/
lreplace:;
+ if (resultRelInfo->ri_WithCheckOptions != NIL)
+ ExecWithCheckOptions(WCO_RLS_UPDATE_CHECK,
+ resultRelInfo, slot, estate);
+
+ /*
+ * Check the constraints of the tuple
+ */
if (resultRelationDesc->rd_att->constr)
ExecConstraints(resultRelInfo, slot, estate);
@@ -780,9 +794,9 @@ lreplace:;
list_free(recheckIndexes);
- /* Check any WITH CHECK OPTION constraints */
+ /* Check any WITH CHECK OPTION constraints from parent views */
if (resultRelInfo->ri_WithCheckOptions != NIL)
- ExecWithCheckOptions(resultRelInfo, slot, estate);
+ ExecWithCheckOptions(WCO_VIEW_CHECK, resultRelInfo, slot, estate);
/* Process RETURNING if present */
if (resultRelInfo->ri_projectReturning)
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 9fe8008..d49585a 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2059,7 +2059,8 @@ _copyWithCheckOption(const WithCheckOption *from)
{
WithCheckOption *newnode = makeNode(WithCheckOption);
- COPY_STRING_FIELD(viewname);
+ COPY_SCALAR_FIELD(kind);
+ COPY_STRING_FIELD(relname);
COPY_NODE_FIELD(qual);
COPY_SCALAR_FIELD(cascaded);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index fe509b0..2cf1899 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2371,7 +2371,8 @@ _equalRangeTblFunction(const RangeTblFunction *a, const RangeTblFunction *b)
static bool
_equalWithCheckOption(const WithCheckOption *a, const WithCheckOption *b)
{
- COMPARE_STRING_FIELD(viewname);
+ COMPARE_SCALAR_FIELD(kind);
+ COMPARE_STRING_FIELD(relname);
COMPARE_NODE_FIELD(qual);
COMPARE_SCALAR_FIELD(cascaded);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 775f482..f86086c 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2326,7 +2326,8 @@ _outWithCheckOption(StringInfo str, const WithCheckOption *node)
{
WRITE_NODE_TYPE("WITHCHECKOPTION");
- WRITE_STRING_FIELD(viewname);
+ WRITE_ENUM_FIELD(kind, WCOKind);
+ WRITE_STRING_FIELD(relname);
WRITE_NODE_FIELD(qual);
WRITE_BOOL_FIELD(cascaded);
}
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 563209c..b0cd95d 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -266,7 +266,8 @@ _readWithCheckOption(void)
{
READ_LOCALS(WithCheckOption);
- READ_STRING_FIELD(viewname);
+ READ_ENUM_FIELD(kind, WCOKind);
+ READ_STRING_FIELD(relname);
READ_NODE_FIELD(qual);
READ_BOOL_FIELD(cascaded);
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 9d2c280..9ea770b 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -2910,7 +2910,8 @@ rewriteTargetView(Query *parsetree, Relation view)
WithCheckOption *wco;
wco = makeNode(WithCheckOption);
- wco->viewname = pstrdup(RelationGetRelationName(view));
+ wco->kind = WCO_VIEW_CHECK;
+ wco->relname = pstrdup(RelationGetRelationName(view));
wco->qual = NULL;
wco->cascaded = cascaded;
diff --git a/src/backend/rewrite/rowsecurity.c b/src/backend/rewrite/rowsecurity.c
index 7669130..687c7f6 100644
--- a/src/backend/rewrite/rowsecurity.c
+++ b/src/backend/rewrite/rowsecurity.c
@@ -227,7 +227,9 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index)
WithCheckOption *wco;
wco = (WithCheckOption *) makeNode(WithCheckOption);
- wco->viewname = RelationGetRelationName(rel);
+ wco->kind = root->commandType == CMD_INSERT ?
+ WCO_RLS_INSERT_CHECK : WCO_RLS_UPDATE_CHECK;
+ wco->relname = RelationGetRelationName(rel);
wco->qual = (Node *) rowsec_with_check_expr;
wco->cascaded = false;
root->withCheckOptions = lcons(wco, root->withCheckOptions);
@@ -241,7 +243,9 @@ prepend_row_security_policies(Query* root, RangeTblEntry* rte, int rt_index)
WithCheckOption *wco;
wco = (WithCheckOption *) makeNode(WithCheckOption);
- wco->viewname = RelationGetRelationName(rel);
+ wco->kind = root->commandType == CMD_INSERT ?
+ WCO_RLS_INSERT_CHECK : WCO_RLS_UPDATE_CHECK;
+ wco->relname = RelationGetRelationName(rel);
wco->qual = (Node *) hook_with_check_expr;
wco->cascaded = false;
root->withCheckOptions = lcons(wco, root->withCheckOptions);
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 40fde83..cb4f158 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -194,7 +194,7 @@ extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
extern bool ExecContextForcesOids(PlanState *planstate, bool *hasoids);
extern void ExecConstraints(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate);
-extern void ExecWithCheckOptions(ResultRelInfo *resultRelInfo,
+extern void ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate);
extern ExecRowMark *ExecFindRowMark(EState *estate, Index rti);
extern ExecAuxRowMark *ExecBuildAuxRowMark(ExecRowMark *erm, List *targetlist);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 41288ed..791343c 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -303,7 +303,7 @@ typedef struct JunkFilter
* TrigInstrument optional runtime measurements for triggers
* FdwRoutine FDW callback functions, if foreign table
* FdwState available to save private state of FDW
- * WithCheckOptions list of WithCheckOption's for views
+ * WithCheckOptions list of WithCheckOption's to be checked
* WithCheckOptionExprs list of WithCheckOption expr states
* ConstraintExprs array of constraint-checking expr states
* junkFilter for removing junk attributes from tuples
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index ac13302..2c3bbce 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -861,14 +861,23 @@ typedef struct RangeTblFunction
/*
* WithCheckOption -
* representation of WITH CHECK OPTION checks to be applied to new tuples
- * when inserting/updating an auto-updatable view.
+ * when inserting/updating an auto-updatable view, or RLS WITH CHECK
+ * policies to be applied when inserting/updating a relation with RLS.
*/
+typedef enum WCOKind
+{
+ WCO_VIEW_CHECK, /* WCO on an auto-updatable view */
+ WCO_RLS_INSERT_CHECK, /* RLS INSERT WITH CHECK policy */
+ WCO_RLS_UPDATE_CHECK /* RLS UPDATE WITH CHECK policy */
+} WCOKind;
+
typedef struct WithCheckOption
{
NodeTag type;
- char *viewname; /* name of view that specified the WCO */
+ WCOKind kind; /* kind of WCO */
+ char *relname; /* name of relation that specified the WCO */
Node *qual; /* constraint qual to check */
- bool cascaded; /* true = WITH CASCADED CHECK OPTION */
+ bool cascaded; /* true for a cascaded WCO on a view */
} WithCheckOption;
/*
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index f41bef1..d318bf9 100644
--- a/src/test/regress/expected/rowsecurity.out
+++ b/src/test/regress/expected/rowsecurity.out
@@ -300,6 +300,11 @@ SELECT * FROM document WHERE did = 8; -- and confirm we can't see it
-----+-----+--------+---------+--------
(0 rows)
+-- RLS policies are checked before constraints
+INSERT INTO document VALUES (8, 44, 1, 'rls_regress_user2', 'my third manga'); -- Should fail with RLS check violation, not duplicate key violation
+ERROR: new row violates row level security policy for "document"
+UPDATE document SET did = 8, dauthor = 'rls_regress_user2' WHERE did = 5; -- Should fail with RLS check violation, not duplicate key violation
+ERROR: new row violates row level security policy for "document"
-- database superuser does bypass RLS policy when enabled
RESET SESSION AUTHORIZATION;
SET row_security TO ON;
@@ -1689,7 +1694,7 @@ EXPLAIN (COSTS OFF) WITH cte1 AS (SELECT * FROM t1 WHERE f_leak(b)) SELECT * FRO
(6 rows)
WITH cte1 AS (UPDATE t1 SET a = a + 1 RETURNING *) SELECT * FROM cte1; --fail
-ERROR: new row violates WITH CHECK OPTION for "t1"
+ERROR: new row violates row level security policy for "t1"
WITH cte1 AS (UPDATE t1 SET a = a RETURNING *) SELECT * FROM cte1; --ok
a | b
----+----------------------------------
@@ -1707,7 +1712,7 @@ WITH cte1 AS (UPDATE t1 SET a = a RETURNING *) SELECT * FROM cte1; --ok
(11 rows)
WITH cte1 AS (INSERT INTO t1 VALUES (21, 'Fail') RETURNING *) SELECT * FROM cte1; --fail
-ERROR: new row violates WITH CHECK OPTION for "t1"
+ERROR: new row violates row level security policy for "t1"
WITH cte1 AS (INSERT INTO t1 VALUES (20, 'Success') RETURNING *) SELECT * FROM cte1; --ok
a | b
----+---------
diff --git a/src/test/regress/sql/rowsecurity.sql b/src/test/regress/sql/rowsecurity.sql
index ed7adbf..a9f4b83 100644
--- a/src/test/regress/sql/rowsecurity.sql
+++ b/src/test/regress/sql/rowsecurity.sql
@@ -146,6 +146,10 @@ SET SESSION AUTHORIZATION rls_regress_user1;
INSERT INTO document VALUES (8, 44, 1, 'rls_regress_user1', 'my third manga'); -- Must fail with unique violation, revealing presence of did we can't see
SELECT * FROM document WHERE did = 8; -- and confirm we can't see it
+-- RLS policies are checked before constraints
+INSERT INTO document VALUES (8, 44, 1, 'rls_regress_user2', 'my third manga'); -- Should fail with RLS check violation, not duplicate key violation
+UPDATE document SET did = 8, dauthor = 'rls_regress_user2' WHERE did = 5; -- Should fail with RLS check violation, not duplicate key violation
+
-- database superuser does bypass RLS policy when enabled
RESET SESSION AUTHORIZATION;
SET row_security TO ON;
--
1.9.1
signature.asc
Description: Digital signature
