Re: table partitioning and access privileges
On 2020/02/17 17:13, Amit Langote wrote: On Mon, Feb 17, 2020 at 4:59 PM Fujii Masao wrote: On 2020/02/14 10:28, Amit Langote wrote: On Thu, Feb 13, 2020 at 8:39 PM Fujii Masao wrote: We can verify that even "LOCK TABLE ONLY" command works expectedly on the inherited tables by keeping those SQLs in the regression test. So what about not removing these SQLs? Hmm, that test becomes meaningless with the behavior change we are introducing, but I am okay with not removing it. Only this regression test seems to verify LOCK TABLE ONLY command. So if we remove this, I'm afraid that the test coverage would be reduced. Oh, I didn't notice that this is the only instance of testing LOCK TABLE ONLY. I would've expected that the test for: 1. checking that ONLY works correctly with LOCK TABLE, and 2. checking permission works correctly with ONLY are separate. Anyway, we can leave that as is. However, I added a test showing that locking child table directly doesn't work. Attached updated patch. Thanks for updating the patch! Barring any objection, I will commit the patch. Thank you. Pushed. Thanks! Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
Re: table partitioning and access privileges
On Mon, Feb 17, 2020 at 4:59 PM Fujii Masao wrote: > On 2020/02/14 10:28, Amit Langote wrote: > > On Thu, Feb 13, 2020 at 8:39 PM Fujii Masao > > wrote: > >> We can verify that even "LOCK TABLE ONLY" command works > >> expectedly on the inherited tables by keeping those SQLs in the > >> regression test. So what about not removing these SQLs? > > > > Hmm, that test becomes meaningless with the behavior change we are > > introducing, but I am okay with not removing it. > > Only this regression test seems to verify LOCK TABLE ONLY command. > So if we remove this, I'm afraid that the test coverage would be reduced. Oh, I didn't notice that this is the only instance of testing LOCK TABLE ONLY. I would've expected that the test for: 1. checking that ONLY works correctly with LOCK TABLE, and 2. checking permission works correctly with ONLY are separate. Anyway, we can leave that as is. > > However, I added a test showing that locking child table directly doesn't > > work. > > > > Attached updated patch. > > Thanks for updating the patch! > Barring any objection, I will commit the patch. Thank you. Regards, Amit
Re: table partitioning and access privileges
On 2020/02/14 10:28, Amit Langote wrote: On Thu, Feb 13, 2020 at 8:39 PM Fujii Masao wrote: On 2020/02/07 10:39, Amit Langote wrote: On Fri, Feb 7, 2020 at 1:16 AM Fujii Masao wrote: Yes, so I will review your patch getting rid of LOCK TABLE exception. Attached updated patch. Thanks! This patch basically looks good to me except the following minor comment. ROLLBACK; -BEGIN; -LOCK TABLE ONLY lock_tbl1; -ROLLBACK; RESET ROLE; I think that there is no strong reason why these SQLs need to be removed. We can verify that even "LOCK TABLE ONLY" command works expectedly on the inherited tables by keeping those SQLs in the regression test. So what about not removing these SQLs? Hmm, that test becomes meaningless with the behavior change we are introducing, but I am okay with not removing it. Only this regression test seems to verify LOCK TABLE ONLY command. So if we remove this, I'm afraid that the test coverage would be reduced. However, I added a test showing that locking child table directly doesn't work. Attached updated patch. Thanks for updating the patch! Barring any objection, I will commit the patch. Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
Re: table partitioning and access privileges
On Thu, Feb 13, 2020 at 8:39 PM Fujii Masao wrote:
> On 2020/02/07 10:39, Amit Langote wrote:
> > On Fri, Feb 7, 2020 at 1:16 AM Fujii Masao
> > wrote:
> >> Yes, so I will review your patch getting rid of
> >> LOCK TABLE exception.
> >
> > Attached updated patch.
>
> Thanks! This patch basically looks good to me except
> the following minor comment.
>
> ROLLBACK;
> -BEGIN;
> -LOCK TABLE ONLY lock_tbl1;
> -ROLLBACK;
> RESET ROLE;
>
> I think that there is no strong reason why these SQLs need to be
> removed. We can verify that even "LOCK TABLE ONLY" command works
> expectedly on the inherited tables by keeping those SQLs in the
> regression test. So what about not removing these SQLs?
Hmm, that test becomes meaningless with the behavior change we are
introducing, but I am okay with not removing it.
However, I added a test showing that locking child table directly doesn't work.
Attached updated patch.
Thanks,
Amit
From 71a2dcbb00928dc695831ceace5992d729436fcd Mon Sep 17 00:00:00 2001
From: Amit Langote
Date: Wed, 22 Jan 2020 15:43:41 +0900
Subject: [PATCH v3] Don't check child's LOCK privilege when locked recursively
---
src/backend/commands/lockcmds.c | 33 +++-
src/test/regress/expected/lock.out | 10 +++---
src/test/regress/expected/privileges.out | 7 +++
src/test/regress/sql/lock.sql| 8 ++--
src/test/regress/sql/privileges.sql | 6 ++
5 files changed, 37 insertions(+), 27 deletions(-)
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 329ab849c0..d8cafc42bb 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -28,7 +28,7 @@
#include "utils/lsyscache.h"
#include "utils/syscache.h"
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid
userid);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
Oid oldrelid, void *arg);
@@ -59,7 +59,7 @@ LockTableCommand(LockStmt *lockstmt)
if (get_rel_relkind(reloid) == RELKIND_VIEW)
LockViewRecurse(reloid, lockstmt->mode,
lockstmt->nowait, NIL);
else if (recurse)
- LockTableRecurse(reloid, lockstmt->mode,
lockstmt->nowait, GetUserId());
+ LockTableRecurse(reloid, lockstmt->mode,
lockstmt->nowait);
}
}
@@ -108,35 +108,26 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid
relid, Oid oldrelid,
/*
* Apply LOCK TABLE recursively over an inheritance tree
*
- * We use find_inheritance_children not find_all_inheritors to avoid taking
- * locks far in advance of checking privileges. This means we'll visit
- * multiply-inheriting children more than once, but that's no problem.
+ * This doesn't check permission to perform LOCK TABLE on the child tables,
+ * because getting here means that the user has permission to lock the
+ * parent which is enough.
*/
static void
-LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid)
+LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
{
List *children;
ListCell *lc;
- children = find_inheritance_children(reloid, NoLock);
+ children = find_all_inheritors(reloid, NoLock, NULL);
foreach(lc, children)
{
Oid childreloid = lfirst_oid(lc);
- AclResult aclresult;
- /* Check permissions before acquiring the lock. */
- aclresult = LockTableAclCheck(childreloid, lockmode, userid);
- if (aclresult != ACLCHECK_OK)
- {
- char *relname = get_rel_name(childreloid);
-
- if (!relname)
- continue; /* child concurrently
dropped, just skip it */
- aclcheck_error(aclresult,
get_relkind_objtype(get_rel_relkind(childreloid)), relname);
- }
+ /* Parent already locked. */
+ if (childreloid == reloid)
+ continue;
- /* We have enough rights to lock the relation; do so. */
if (!nowait)
LockRelationOid(childreloid, lockmode);
else if (!ConditionalLockRelationOid(childreloid, lockmode))
@@ -162,8 +153,6 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool
nowait, Oid userid)
UnlockRelationOid(childreloid, lockmode);
continue;
}
-
- LockTableRecurse(childreloid, lockmode, nowait, userid);
}
}
@@ -241,7 +230,7 @@ LockViewRecurse_walker(Node *node, LockViewRecurse_context
Re: table partitioning and access privileges
On 2020/02/07 10:39, Amit Langote wrote: On Fri, Feb 7, 2020 at 1:16 AM Fujii Masao wrote: On 2020/02/03 14:26, Amit Langote wrote: On Mon, Feb 3, 2020 at 2:07 PM Fujii Masao wrote: On 2020/02/03 11:05, Amit Langote wrote: Okay. How about the attached? Thanks for the patches! You added the note just after the description about row level security on inherited table, but isn't it better to add it before that? Attached patch does that. Thought? Yeah, that might be a better flow for that paragraph. Pushed! Thanks! Thank you. Maybe, we should also note the LOCK TABLE exception? Yes. Note that, unlike TRUNCATE, LOCK TABLE exception exists in HEAD too, but maybe you're aware of that. Yes, so I will review your patch getting rid of LOCK TABLE exception. Attached updated patch. Thanks! This patch basically looks good to me except the following minor comment. ROLLBACK; -BEGIN; -LOCK TABLE ONLY lock_tbl1; -ROLLBACK; RESET ROLE; I think that there is no strong reason why these SQLs need to be removed. We can verify that even "LOCK TABLE ONLY" command works expectedly on the inherited tables by keeping those SQLs in the regression test. So what about not removing these SQLs? Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
Re: table partitioning and access privileges
On Fri, Feb 7, 2020 at 1:16 AM Fujii Masao wrote:
> On 2020/02/03 14:26, Amit Langote wrote:
> > On Mon, Feb 3, 2020 at 2:07 PM Fujii Masao
> > wrote:
> >> On 2020/02/03 11:05, Amit Langote wrote:
> >>> Okay. How about the attached?
> >>
> >> Thanks for the patches! You added the note just after the description
> >> about row level security on inherited table, but isn't it better to
> >> add it before that? Attached patch does that. Thought?
> >
> > Yeah, that might be a better flow for that paragraph.
>
> Pushed! Thanks!
Thank you.
> >>> Maybe, we should also note the LOCK TABLE exception?
> >>
> >> Yes.
> >
> > Note that, unlike TRUNCATE, LOCK TABLE exception exists in HEAD too,
> > but maybe you're aware of that.
>
> Yes, so I will review your patch getting rid of
> LOCK TABLE exception.
Attached updated patch.
Regards,
Amit
From 77bf8ac02734b75f8a1e7c61bfc84e1b37189893 Mon Sep 17 00:00:00 2001
From: Amit Langote
Date: Wed, 22 Jan 2020 15:43:41 +0900
Subject: [PATCH v2] Don't check child's LOCK privilege when locked recursively
---
src/backend/commands/lockcmds.c | 33 +++-
src/test/regress/expected/lock.out | 8 ++--
src/test/regress/expected/privileges.out | 7 +++
src/test/regress/sql/lock.sql| 7 ++-
src/test/regress/sql/privileges.sql | 6 ++
5 files changed, 28 insertions(+), 33 deletions(-)
diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c
index 329ab849c0..d8cafc42bb 100644
--- a/src/backend/commands/lockcmds.c
+++ b/src/backend/commands/lockcmds.c
@@ -28,7 +28,7 @@
#include "utils/lsyscache.h"
#include "utils/syscache.h"
-static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid
userid);
+static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait);
static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid);
static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid,
Oid oldrelid, void *arg);
@@ -59,7 +59,7 @@ LockTableCommand(LockStmt *lockstmt)
if (get_rel_relkind(reloid) == RELKIND_VIEW)
LockViewRecurse(reloid, lockstmt->mode,
lockstmt->nowait, NIL);
else if (recurse)
- LockTableRecurse(reloid, lockstmt->mode,
lockstmt->nowait, GetUserId());
+ LockTableRecurse(reloid, lockstmt->mode,
lockstmt->nowait);
}
}
@@ -108,35 +108,26 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid
relid, Oid oldrelid,
/*
* Apply LOCK TABLE recursively over an inheritance tree
*
- * We use find_inheritance_children not find_all_inheritors to avoid taking
- * locks far in advance of checking privileges. This means we'll visit
- * multiply-inheriting children more than once, but that's no problem.
+ * This doesn't check permission to perform LOCK TABLE on the child tables,
+ * because getting here means that the user has permission to lock the
+ * parent which is enough.
*/
static void
-LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid)
+LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait)
{
List *children;
ListCell *lc;
- children = find_inheritance_children(reloid, NoLock);
+ children = find_all_inheritors(reloid, NoLock, NULL);
foreach(lc, children)
{
Oid childreloid = lfirst_oid(lc);
- AclResult aclresult;
- /* Check permissions before acquiring the lock. */
- aclresult = LockTableAclCheck(childreloid, lockmode, userid);
- if (aclresult != ACLCHECK_OK)
- {
- char *relname = get_rel_name(childreloid);
-
- if (!relname)
- continue; /* child concurrently
dropped, just skip it */
- aclcheck_error(aclresult,
get_relkind_objtype(get_rel_relkind(childreloid)), relname);
- }
+ /* Parent already locked. */
+ if (childreloid == reloid)
+ continue;
- /* We have enough rights to lock the relation; do so. */
if (!nowait)
LockRelationOid(childreloid, lockmode);
else if (!ConditionalLockRelationOid(childreloid, lockmode))
@@ -162,8 +153,6 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool
nowait, Oid userid)
UnlockRelationOid(childreloid, lockmode);
continue;
}
-
- LockTableRecurse(childreloid, lockmode, nowait, userid);
}
}
@@ -241,7 +230,7 @@ LockViewRecurse_walker(Node *node, LockViewRecurse_context
*context)
if (relkind == RELKIND_VIEW)
Re: table partitioning and access privileges
On 2020/02/03 14:26, Amit Langote wrote: On Mon, Feb 3, 2020 at 2:07 PM Fujii Masao wrote: On 2020/02/03 11:05, Amit Langote wrote: Okay. How about the attached? Thanks for the patches! You added the note just after the description about row level security on inherited table, but isn't it better to add it before that? Attached patch does that. Thought? Yeah, that might be a better flow for that paragraph. Pushed! Thanks! Maybe, we should also note the LOCK TABLE exception? Yes. Note that, unlike TRUNCATE, LOCK TABLE exception exists in HEAD too, but maybe you're aware of that. Yes, so I will review your patch getting rid of LOCK TABLE exception. Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
Re: table partitioning and access privileges
On Mon, Feb 3, 2020 at 2:07 PM Fujii Masao wrote: > On 2020/02/03 11:05, Amit Langote wrote: > > Okay. How about the attached? > > Thanks for the patches! You added the note just after the description > about row level security on inherited table, but isn't it better to > add it before that? Attached patch does that. Thought? Yeah, that might be a better flow for that paragraph. > > Maybe, we should also note the LOCK TABLE exception? > > Yes. Note that, unlike TRUNCATE, LOCK TABLE exception exists in HEAD too, but maybe you're aware of that. Thanks, Amit
Re: table partitioning and access privileges
On 2020/02/03 11:05, Amit Langote wrote:
On Fri, Jan 31, 2020 at 9:39 PM Fujii Masao wrote:
On 2020/01/31 13:38, Amit Langote wrote:
On Fri, Jan 31, 2020 at 1:28 AM Fujii Masao wrote:
Fair enough. I finally did back-patch because the behavior is clearly
documented and I failed to hear the opinions to object the back-patch.
But I should have heard and discussed such risks more.
I'm OK to revert all those back-patch. Instead, probably the document
should be updated in old branches.
I could find only this paragraph in the section on inheritance that
talks about how access permissions work:
9.4:
"Note how table access permissions are handled. Querying a parent
table can automatically access data in child tables without further
access privilege checking. This preserves the appearance that the data
is (also) in the parent table. Accessing the child tables directly is,
however, not automatically allowed and would require further
privileges to be granted."
9.5-12:
"Inherited queries perform access permission checks on the parent
table only. Thus, for example, granting UPDATE permission on the
cities table implies permission to update rows in the capitals table
as well, when they are accessed through cities. This preserves the
appearance that the data is (also) in the parent table. But the
capitals table could not be updated directly without an additional
grant. In a similar way, the parent table's row security policies (see
Section 5.7) are applied to rows coming from child tables during an
inherited query. A child table's policies, if any, are applied only
when it is the table explicitly named in the query; and in that case,
any policies attached to its parent(s) are ignored."
Do you mean that the TRUNCATE exception should be noted here?
Yes, that's what I was thinking.
Okay. How about the attached?
Thanks for the patches! You added the note just after the description
about row level security on inherited table, but isn't it better to
add it before that? Attached patch does that. Thought?
Maybe, we should also note the LOCK TABLE exception?
Yes.
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index d88651df9e..7550d03f27 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3384,7 +3384,15 @@ VALUES ('Albany', NULL, NULL, 'NY');
accessed through cities. This preserves the
appearance
that the data is (also) in the parent table. But
the capitals table could not be updated directly
- without an additional grant. In a similar way, the parent table's row
+ without an additional grant. Two exceptions to this rule are
+ TRUNCATE and LOCK TABLE,
+ where permissions on the child tables are always checked,
+ whether they are processed directly or recursively via those commands
+ performed on the parent table.
+
+
+
+ In a similar way, the parent table's row
security policies (see ) are applied to
rows coming from child tables during an inherited query. A child table's
policies, if any, are applied only when it is the table explicitly named
Re: table partitioning and access privileges
On Fri, Jan 31, 2020 at 9:39 PM Fujii Masao wrote:
> On 2020/01/31 13:38, Amit Langote wrote:
> > On Fri, Jan 31, 2020 at 1:28 AM Fujii Masao
> > wrote:
> >> Fair enough. I finally did back-patch because the behavior is clearly
> >> documented and I failed to hear the opinions to object the back-patch.
> >> But I should have heard and discussed such risks more.
> >>
> >> I'm OK to revert all those back-patch. Instead, probably the document
> >> should be updated in old branches.
> >
> > I could find only this paragraph in the section on inheritance that
> > talks about how access permissions work:
> >
> > 9.4:
> >
> > "Note how table access permissions are handled. Querying a parent
> > table can automatically access data in child tables without further
> > access privilege checking. This preserves the appearance that the data
> > is (also) in the parent table. Accessing the child tables directly is,
> > however, not automatically allowed and would require further
> > privileges to be granted."
> >
> > 9.5-12:
> >
> > "Inherited queries perform access permission checks on the parent
> > table only. Thus, for example, granting UPDATE permission on the
> > cities table implies permission to update rows in the capitals table
> > as well, when they are accessed through cities. This preserves the
> > appearance that the data is (also) in the parent table. But the
> > capitals table could not be updated directly without an additional
> > grant. In a similar way, the parent table's row security policies (see
> > Section 5.7) are applied to rows coming from child tables during an
> > inherited query. A child table's policies, if any, are applied only
> > when it is the table explicitly named in the query; and in that case,
> > any policies attached to its parent(s) are ignored."
> >
> > Do you mean that the TRUNCATE exception should be noted here?
>
> Yes, that's what I was thinking.
Okay. How about the attached?
Maybe, we should also note the LOCK TABLE exception?
Regards,
Amit
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 8d2908c34d..d274d048ec 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2323,7 +2323,10 @@ VALUES ('New York', NULL, NULL, 'NY');
access privilege checking. This preserves the appearance that the
data is (also) in the parent table. Accessing the child tables
directly is, however, not automatically allowed and would require
- further privileges to be granted.
+ further privileges to be granted. One exception to this rule is
+ TRUNCATE command, where permissions on the child tables
+ are always checked, whether they are truncated directly or recursively via
+ TRUNCATE performed on the parent table.
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 6ab37e7354..cf7b4fd891 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -2762,7 +2762,10 @@ VALUES ('Albany', NULL, NULL, 'NY');
rows coming from child tables during an inherited query. A child table's
policies, if any, are applied only when it is the table explicitly named
in the query; and in that case, any policies attached to its parent(s) are
- ignored.
+ ignored. One exception to this rule is TRUNCATE command,
+ where permissions on the child tables are always checked, whether they are
+ truncated directly or recursively via TRUNCATE performed
+ on the parent table.
Re: table partitioning and access privileges
On 2020/01/31 13:38, Amit Langote wrote: On Fri, Jan 31, 2020 at 1:28 AM Fujii Masao wrote: On 2020/01/31 1:02, Tom Lane wrote: Fujii Masao writes: Thanks for updating the patch! Barring any objection, I will commit this fix and backport it to all supported versions. Sorry for not having paid closer attention to this thread, but ... is back-patching this behavioral change really a good idea? It's not that hard to imagine that somebody is expecting the old behavior and will complain that we broke their application's security. So I'd have thought it better to fix only in HEAD, with a compatibility warning in the v13 release notes. I'm afraid it's much more likely that people will complain about making such a change in a minor release than that they will be happy about it. It's particularly risky to be making it in what will be the last 9.4.x release, because we will not have any opportunity to undo it in that branch if there is pushback. Fair enough. I finally did back-patch because the behavior is clearly documented and I failed to hear the opinions to object the back-patch. But I should have heard and discussed such risks more. I'm OK to revert all those back-patch. Instead, probably the document should be updated in old branches. I could find only this paragraph in the section on inheritance that talks about how access permissions work: 9.4: "Note how table access permissions are handled. Querying a parent table can automatically access data in child tables without further access privilege checking. This preserves the appearance that the data is (also) in the parent table. Accessing the child tables directly is, however, not automatically allowed and would require further privileges to be granted." 9.5-12: "Inherited queries perform access permission checks on the parent table only. Thus, for example, granting UPDATE permission on the cities table implies permission to update rows in the capitals table as well, when they are accessed through cities. This preserves the appearance that the data is (also) in the parent table. But the capitals table could not be updated directly without an additional grant. In a similar way, the parent table's row security policies (see Section 5.7) are applied to rows coming from child tables during an inherited query. A child table's policies, if any, are applied only when it is the table explicitly named in the query; and in that case, any policies attached to its parent(s) are ignored." Do you mean that the TRUNCATE exception should be noted here? Yes, that's what I was thinking. Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
Re: table partitioning and access privileges
On Fri, Jan 31, 2020 at 1:28 AM Fujii Masao wrote: > On 2020/01/31 1:02, Tom Lane wrote: > > Fujii Masao writes: > >> Thanks for updating the patch! Barring any objection, > >> I will commit this fix and backport it to all supported versions. > > > > Sorry for not having paid closer attention to this thread, but ... > > is back-patching this behavioral change really a good idea? > > > > It's not that hard to imagine that somebody is expecting the old > > behavior and will complain that we broke their application's security. > > So I'd have thought it better to fix only in HEAD, with a > > compatibility warning in the v13 release notes. > > > > I'm afraid it's much more likely that people will complain about > > making such a change in a minor release than that they will be > > happy about it. It's particularly risky to be making it in what > > will be the last 9.4.x release, because we will not have any > > opportunity to undo it in that branch if there is pushback. > > Fair enough. I finally did back-patch because the behavior is clearly > documented and I failed to hear the opinions to object the back-patch. > But I should have heard and discussed such risks more. > > I'm OK to revert all those back-patch. Instead, probably the document > should be updated in old branches. I could find only this paragraph in the section on inheritance that talks about how access permissions work: 9.4: "Note how table access permissions are handled. Querying a parent table can automatically access data in child tables without further access privilege checking. This preserves the appearance that the data is (also) in the parent table. Accessing the child tables directly is, however, not automatically allowed and would require further privileges to be granted." 9.5-12: "Inherited queries perform access permission checks on the parent table only. Thus, for example, granting UPDATE permission on the cities table implies permission to update rows in the capitals table as well, when they are accessed through cities. This preserves the appearance that the data is (also) in the parent table. But the capitals table could not be updated directly without an additional grant. In a similar way, the parent table's row security policies (see Section 5.7) are applied to rows coming from child tables during an inherited query. A child table's policies, if any, are applied only when it is the table explicitly named in the query; and in that case, any policies attached to its parent(s) are ignored." Do you mean that the TRUNCATE exception should be noted here? Thanks, Amit
Re: table partitioning and access privileges
Fujii Masao writes: > On 2020/01/31 1:28, Fujii Masao wrote: >> On 2020/01/31 1:02, Tom Lane wrote: >>> Sorry for not having paid closer attention to this thread, but ... >>> is back-patching this behavioral change really a good idea? > I'm thinking to wait at least half a day before reverting > the back-patch just in case someone can give opinion > during that period. Sure, other opinions welcome. We still have a week before the back-branch releases. regards, tom lane
Re: table partitioning and access privileges
On 2020/01/31 1:28, Fujii Masao wrote: On 2020/01/31 1:02, Tom Lane wrote: Fujii Masao writes: Thanks for updating the patch! Barring any objection, I will commit this fix and backport it to all supported versions. Sorry for not having paid closer attention to this thread, but ... is back-patching this behavioral change really a good idea? It's not that hard to imagine that somebody is expecting the old behavior and will complain that we broke their application's security. So I'd have thought it better to fix only in HEAD, with a compatibility warning in the v13 release notes. I'm afraid it's much more likely that people will complain about making such a change in a minor release than that they will be happy about it. It's particularly risky to be making it in what will be the last 9.4.x release, because we will not have any opportunity to undo it in that branch if there is pushback. Fair enough. I finally did back-patch because the behavior is clearly documented and I failed to hear the opinions to object the back-patch. But I should have heard and discussed such risks more. I'm OK to revert all those back-patch. Instead, probably the document should be updated in old branches. I'm thinking to wait at least half a day before reverting the back-patch just in case someone can give opinion during that period. Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
Re: table partitioning and access privileges
On 2020/01/31 1:02, Tom Lane wrote: Fujii Masao writes: Thanks for updating the patch! Barring any objection, I will commit this fix and backport it to all supported versions. Sorry for not having paid closer attention to this thread, but ... is back-patching this behavioral change really a good idea? It's not that hard to imagine that somebody is expecting the old behavior and will complain that we broke their application's security. So I'd have thought it better to fix only in HEAD, with a compatibility warning in the v13 release notes. I'm afraid it's much more likely that people will complain about making such a change in a minor release than that they will be happy about it. It's particularly risky to be making it in what will be the last 9.4.x release, because we will not have any opportunity to undo it in that branch if there is pushback. Fair enough. I finally did back-patch because the behavior is clearly documented and I failed to hear the opinions to object the back-patch. But I should have heard and discussed such risks more. I'm OK to revert all those back-patch. Instead, probably the document should be updated in old branches. Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
Re: table partitioning and access privileges
Fujii Masao writes: > Thanks for updating the patch! Barring any objection, > I will commit this fix and backport it to all supported versions. Sorry for not having paid closer attention to this thread, but ... is back-patching this behavioral change really a good idea? It's not that hard to imagine that somebody is expecting the old behavior and will complain that we broke their application's security. So I'd have thought it better to fix only in HEAD, with a compatibility warning in the v13 release notes. I'm afraid it's much more likely that people will complain about making such a change in a minor release than that they will be happy about it. It's particularly risky to be making it in what will be the last 9.4.x release, because we will not have any opportunity to undo it in that branch if there is pushback. regards, tom lane
Re: table partitioning and access privileges
On 2020/01/27 14:02, Amit Langote wrote: Fujii-san, On Mon, Jan 27, 2020 at 11:19 AM Fujii Masao wrote: On 2020/01/23 22:14, Fujii Masao wrote: Thanks for updating the patch! Barring any objection, I will commit this fix and backport it to all supported versions. Attached are the back-port versions of the patches. The patch for master branch separates truncate_check_activity() into two functions, but in v11 or before, truncate_check_activity() didn't exist and its code was in truncate_check_rel(). So I had to write the back-port version of the patch for the previous versions and separate truncate_check_rel() into three functions, i.e., truncate_check_rel(), truncate_check_activity() and truncate_check_perms(). Thank you for creating the back-port versions. I agree with making the code look similar in all supported branches for the ease of future maintenance. Thanks for the check! I pushed the patches. Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
Re: table partitioning and access privileges
Fujii-san, On Mon, Jan 27, 2020 at 11:19 AM Fujii Masao wrote: > On 2020/01/23 22:14, Fujii Masao wrote: > > Thanks for updating the patch! Barring any objection, > > I will commit this fix and backport it to all supported versions. > > Attached are the back-port versions of the patches. > > > The patch for master branch separates truncate_check_activity() into two > functions, but in v11 or before, truncate_check_activity() didn't exist and > its code was in truncate_check_rel(). So I had to write the back-port > version > of the patch for the previous versions and separate truncate_check_rel() > into three functions, i.e., truncate_check_rel(), > truncate_check_activity() and > truncate_check_perms(). Thank you for creating the back-port versions. I agree with making the code look similar in all supported branches for the ease of future maintenance. Thanks, Amit
Re: table partitioning and access privileges
On 2020/01/23 22:14, Fujii Masao wrote:
On 2020/01/22 16:54, Amit Langote wrote:
Fujii-san,
Thanks for taking a look.
On Fri, Jan 10, 2020 at 10:29 AM Fujii Masao
wrote:
On Tue, Jan 7, 2020 at 5:15 PM Amit Langote
wrote:
I tend to agree that TRUNCATE's permission model for inheritance
should be consistent with that for the other commands. How about the
attached patch toward that end?
Thanks for the patch!
The patch basically looks good to me.
+GRANT SELECT (f1, fz), UPDATE (fz) ON atestc TO regress_priv_user2;
+REVOKE TRUNCATE ON atestc FROM regress_priv_user2;
These seem not to be necessary for the test.
You're right. Removed in the attached updated patch.
Thanks for updating the patch! Barring any objection,
I will commit this fix and backport it to all supported versions.
Attached are the back-port versions of the patches.
- patch for master and v12
0001-Don-t-check-child-s-TRUNCATE-privilege-when-truncate-fujii-pg12-13.patch
- patch for v11
0001-Don-t-check-child-s-TRUNCATE-privilege-when-truncate-fujii-pg11.patch
- patch for v10
0001-Don-t-check-child-s-TRUNCATE-privilege-when-truncate-fujii-pg10.patch
- patch for v9.6
0001-Don-t-check-child-s-TRUNCATE-privilege-when-truncate-fujii-pg96.patch
- patch for v9.5 and v9.4
0001-Don-t-check-child-s-TRUNCATE-privilege-when-truncate-fujii-pg94-95.patch
The patch for master branch separates truncate_check_activity() into two
functions, but in v11 or before, truncate_check_activity() didn't exist and
its code was in truncate_check_rel(). So I had to write the back-port
version
of the patch for the previous versions and separate truncate_check_rel()
into three functions, i.e., truncate_check_rel(),
truncate_check_activity() and
truncate_check_perms().
Also the names of users that the regression test for privileges use were
different between PostgreSQL versions. This is another reason
why I had to write several back-port versions of the patches.
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1a005760d8..204d25aeb3 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -291,7 +291,9 @@ struct DropRelationCallbackState
#define child_dependency_type(child_is_partition) \
((child_is_partition) ? DEPENDENCY_AUTO : DEPENDENCY_NORMAL)
-static void truncate_check_rel(Relation rel);
+static void truncate_check_rel(Oid relid, Form_pg_class reltuple);
+static void truncate_check_perms(Oid relid, Form_pg_class reltuple);
+static void truncate_check_activity(Relation rel);
static List *MergeAttributes(List *schema, List *supers, char relpersistence,
bool is_partition, List **supOids, List
**supconstr,
int *supOidCount);
@@ -1242,7 +1244,11 @@ ExecuteTruncate(TruncateStmt *stmt)
heap_close(rel, lockmode);
continue;
}
- truncate_check_rel(rel);
+
+ truncate_check_rel(myrelid, rel->rd_rel);
+ truncate_check_perms(myrelid, rel->rd_rel);
+ truncate_check_activity(rel);
+
rels = lappend(rels, rel);
relids = lappend_oid(relids, myrelid);
@@ -1278,7 +1284,15 @@ ExecuteTruncate(TruncateStmt *stmt)
continue;
}
- truncate_check_rel(rel);
+ /*
+* Inherited TRUNCATE commands perform access
+* permission checks on the parent table only.
+* So we skip checking the children's
permissions
+* and don't call truncate_check_perms() here.
+*/
+ truncate_check_rel(RelationGetRelid(rel),
rel->rd_rel);
+ truncate_check_activity(rel);
+
rels = lappend(rels, rel);
relids = lappend_oid(relids, childrelid);
}
@@ -1317,7 +1331,9 @@ ExecuteTruncate(TruncateStmt *stmt)
ereport(NOTICE,
(errmsg("truncate cascades to
table \"%s\"",
RelationGetRelationName(rel;
- truncate_check_rel(rel);
+ truncate_check_rel(relid, rel->rd_rel);
+ truncate_check_perms(relid, rel->rd_rel);
+ truncate_check_activity(rel);
rels = lappend(rels, rel);
relids = lappend_oid(relids, relid);
Re: table partitioning and access privileges
On 2020/01/22 16:54, Amit Langote wrote: Fujii-san, Thanks for taking a look. On Fri, Jan 10, 2020 at 10:29 AM Fujii Masao wrote: On Tue, Jan 7, 2020 at 5:15 PM Amit Langote wrote: I tend to agree that TRUNCATE's permission model for inheritance should be consistent with that for the other commands. How about the attached patch toward that end? Thanks for the patch! The patch basically looks good to me. +GRANT SELECT (f1, fz), UPDATE (fz) ON atestc TO regress_priv_user2; +REVOKE TRUNCATE ON atestc FROM regress_priv_user2; These seem not to be necessary for the test. You're right. Removed in the attached updated patch. Thanks for updating the patch! Barring any objection, I will commit this fix and backport it to all supported versions. BTW, I found that LOCK TABLE on the parent table checks the permission of its child tables. This also needs to be fixed (as a separate patch)? Commit ac33c7e2c13 and a past discussion ([1], [2], resp.) appear to disagree with that position, but I would like to agree with you because the behavior you suggest would be consistent with other commands. So, I'm attaching a patch for that too, although it would be better to hear more opinions before accepting it. Yes. I'd like to hear more opinion about this. But since the document explains "Inherited queries perform access permission checks on the parent table only." in ddl.sgml, that also seems a bug to fix... Regards, -- Fujii Masao NTT DATA CORPORATION Advanced Platform Technology Group Research and Development Headquarters
Re: table partitioning and access privileges
Fujii-san,
Thanks for taking a look.
On Fri, Jan 10, 2020 at 10:29 AM Fujii Masao wrote:
> On Tue, Jan 7, 2020 at 5:15 PM Amit Langote wrote:
> > I tend to agree that TRUNCATE's permission model for inheritance
> > should be consistent with that for the other commands. How about the
> > attached patch toward that end?
>
> Thanks for the patch!
>
> The patch basically looks good to me.
>
> +GRANT SELECT (f1, fz), UPDATE (fz) ON atestc TO regress_priv_user2;
> +REVOKE TRUNCATE ON atestc FROM regress_priv_user2;
>
> These seem not to be necessary for the test.
You're right. Removed in the attached updated patch.
> BTW, I found that LOCK TABLE on the parent table checks the permission
> of its child tables. This also needs to be fixed (as a separate patch)?
Commit ac33c7e2c13 and a past discussion ([1], [2], resp.) appear to
disagree with that position, but I would like to agree with you
because the behavior you suggest would be consistent with other
commands. So, I'm attaching a patch for that too, although it would
be better to hear more opinions before accepting it.
Thanks,
Amit
[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=ac33c7e2c13
[2]
https://www.postgresql.org/message-id/flat/34d269d40905121340h535ef652kbf8f054811e42e39%40mail.gmail.com
From fd763965648865062cdcb47c5029a50efb645119 Mon Sep 17 00:00:00 2001
From: Amit Langote
Date: Wed, 22 Jan 2020 15:39:39 +0900
Subject: [PATCH 1/2] Don't check child's TRUNCATE privilege when truncated
recursively
---
src/backend/commands/tablecmds.c | 30 +++---
src/test/regress/expected/privileges.out | 21 +
src/test/regress/sql/privileges.sql | 14 ++
3 files changed, 58 insertions(+), 7 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7c23968f2d..9b86692e87 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -304,6 +304,7 @@ struct DropRelationCallbackState
((child_is_partition) ? DEPENDENCY_AUTO : DEPENDENCY_NORMAL)
static void truncate_check_rel(Oid relid, Form_pg_class reltuple);
+static void truncate_check_perms(Oid relid, Form_pg_class reltuple);
static void truncate_check_activity(Relation rel);
static void RangeVarCallbackForTruncate(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
@@ -1616,6 +1617,10 @@ ExecuteTruncate(TruncateStmt *stmt)
}
truncate_check_rel(RelationGetRelid(rel),
rel->rd_rel);
+ /*
+* We skip checking the child's permission,
because we
+* already checked the parent's.
+*/
truncate_check_activity(rel);
rels = lappend(rels, rel);
@@ -1701,6 +1706,7 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids,
List *relids_logged,
(errmsg("truncate cascades to
table \"%s\"",
RelationGetRelationName(rel;
truncate_check_rel(relid, rel->rd_rel);
+ truncate_check_perms(relid, rel->rd_rel);
truncate_check_activity(rel);
rels = lappend(rels, rel);
relids = lappend_oid(relids, relid);
@@ -1951,7 +1957,6 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids,
List *relids_logged,
static void
truncate_check_rel(Oid relid, Form_pg_class reltuple)
{
- AclResult aclresult;
char *relname = NameStr(reltuple->relname);
/*
@@ -1965,12 +1970,6 @@ truncate_check_rel(Oid relid, Form_pg_class reltuple)
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not a table", relname)));
- /* Permissions checks */
- aclresult = pg_class_aclcheck(relid, GetUserId(), ACL_TRUNCATE);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult,
get_relkind_objtype(reltuple->relkind),
- relname);
-
if (!allowSystemTableMods && IsSystemClass(relid, reltuple))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -1980,6 +1979,22 @@ truncate_check_rel(Oid relid, Form_pg_class reltuple)
InvokeObjectTruncateHook(relid);
}
+/*
+ * Check that current user has the permission to truncate given relation.
+ */
+static void
+truncate_check_perms(Oid relid, Form_pg_class reltuple)
+{
+ char *relname = NameStr(reltuple->relname);
+ AclResult aclresult;
+
+ /* Permissions checks
Re: table partitioning and access privileges
On Tue, Jan 7, 2020 at 5:15 PM Amit Langote wrote: > > On Fri, Dec 27, 2019 at 4:26 AM Tom Lane wrote: > > Fujii Masao writes: > > > My customer reported me that the queries through a partitioned table > > > ignore each partition's SELECT, INSERT, UPDATE, and DELETE privileges, > > > on the other hand, only TRUNCATE privilege specified for each partition > > > is applied. I'm not sure if this behavior is expected or not. But anyway > > > is it better to document that? For example, > > > > > Access privileges may be defined and removed separately for each > > > partition. > > > But note that queries through a partitioned table ignore each > > > partition's > > > SELECT, INSERT, UPDATE and DELETE privileges, and apply only TRUNCATE > > > one. > > > > I believe it's intentional that we only check access privileges on > > the table explicitly named in the query. So I'd say SELECT etc > > are doing the right thing, and if TRUNCATE isn't in step with them > > that's a bug to fix, not something to document. > > I tend to agree that TRUNCATE's permission model for inheritance > should be consistent with that for the other commands. How about the > attached patch toward that end? Thanks for the patch! The patch basically looks good to me. +GRANT SELECT (f1, fz), UPDATE (fz) ON atestc TO regress_priv_user2; +REVOKE TRUNCATE ON atestc FROM regress_priv_user2; These seem not to be necessary for the test. BTW, I found that LOCK TABLE on the parent table checks the permission of its child tables. This also needs to be fixed (as a separate patch)? Regards, -- Fujii Masao
Re: table partitioning and access privileges
On Fri, Dec 27, 2019 at 4:26 AM Tom Lane wrote:
> Fujii Masao writes:
> > My customer reported me that the queries through a partitioned table
> > ignore each partition's SELECT, INSERT, UPDATE, and DELETE privileges,
> > on the other hand, only TRUNCATE privilege specified for each partition
> > is applied. I'm not sure if this behavior is expected or not. But anyway
> > is it better to document that? For example,
>
> > Access privileges may be defined and removed separately for each
> > partition.
> > But note that queries through a partitioned table ignore each
> > partition's
> > SELECT, INSERT, UPDATE and DELETE privileges, and apply only TRUNCATE
> > one.
>
> I believe it's intentional that we only check access privileges on
> the table explicitly named in the query. So I'd say SELECT etc
> are doing the right thing, and if TRUNCATE isn't in step with them
> that's a bug to fix, not something to document.
I tend to agree that TRUNCATE's permission model for inheritance
should be consistent with that for the other commands. How about the
attached patch toward that end?
Thanks,
Amit
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 1c4394abea..43377e24c4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -298,6 +298,7 @@ struct DropRelationCallbackState
((child_is_partition) ? DEPENDENCY_AUTO : DEPENDENCY_NORMAL)
static void truncate_check_rel(Oid relid, Form_pg_class reltuple);
+static void truncate_check_perms(Oid relid, Form_pg_class reltuple);
static void truncate_check_activity(Relation rel);
static void RangeVarCallbackForTruncate(const RangeVar *relation,
Oid relId, Oid oldRelId, void *arg);
@@ -1575,6 +1576,10 @@ ExecuteTruncate(TruncateStmt *stmt)
}
truncate_check_rel(RelationGetRelid(rel),
rel->rd_rel);
+ /*
+* We skip checking the child's permission,
because we
+* already checked the parent's.
+*/
truncate_check_activity(rel);
rels = lappend(rels, rel);
@@ -1660,6 +1665,7 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids,
List *relids_logged,
(errmsg("truncate cascades to
table \"%s\"",
RelationGetRelationName(rel;
truncate_check_rel(relid, rel->rd_rel);
+ truncate_check_perms(relid, rel->rd_rel);
truncate_check_activity(rel);
rels = lappend(rels, rel);
relids = lappend_oid(relids, relid);
@@ -1910,7 +1916,6 @@ ExecuteTruncateGuts(List *explicit_rels, List *relids,
List *relids_logged,
static void
truncate_check_rel(Oid relid, Form_pg_class reltuple)
{
- AclResult aclresult;
char *relname = NameStr(reltuple->relname);
/*
@@ -1924,12 +1929,6 @@ truncate_check_rel(Oid relid, Form_pg_class reltuple)
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not a table", relname)));
- /* Permissions checks */
- aclresult = pg_class_aclcheck(relid, GetUserId(), ACL_TRUNCATE);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult,
get_relkind_objtype(reltuple->relkind),
- relname);
-
if (!allowSystemTableMods && IsSystemClass(relid, reltuple))
ereport(ERROR,
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
@@ -1939,6 +1938,22 @@ truncate_check_rel(Oid relid, Form_pg_class reltuple)
InvokeObjectTruncateHook(relid);
}
+/*
+ * Check that current user has the permission to truncate given relation.
+ */
+static void
+truncate_check_perms(Oid relid, Form_pg_class reltuple)
+{
+ char *relname = NameStr(reltuple->relname);
+ AclResult aclresult;
+
+ /* Permissions checks */
+ aclresult = pg_class_aclcheck(relid, GetUserId(), ACL_TRUNCATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult,
get_relkind_objtype(reltuple->relkind),
+ relname);
+}
+
/*
* Set of extra sanity checks to check if a given relation is safe to
* truncate. This is split with truncate_check_rel() as
@@ -14799,6 +14814,7 @@ RangeVarCallbackForTruncate(const RangeVar *relation,
elog(ERROR, "cache lookup failed for relation %u", relId);
truncate_check_rel(relId, (Form_pg_class) GETSTRUCT(tuple));
+ truncate_check_perms(relId,
Re: table partitioning and access privileges
Fujii Masao writes: > My customer reported me that the queries through a partitioned table > ignore each partition's SELECT, INSERT, UPDATE, and DELETE privileges, > on the other hand, only TRUNCATE privilege specified for each partition > is applied. I'm not sure if this behavior is expected or not. But anyway > is it better to document that? For example, > Access privileges may be defined and removed separately for each > partition. > But note that queries through a partitioned table ignore each partition's > SELECT, INSERT, UPDATE and DELETE privileges, and apply only TRUNCATE one. I believe it's intentional that we only check access privileges on the table explicitly named in the query. So I'd say SELECT etc are doing the right thing, and if TRUNCATE isn't in step with them that's a bug to fix, not something to document. regards, tom lane
table partitioning and access privileges
Hi, My customer reported me that the queries through a partitioned table ignore each partition's SELECT, INSERT, UPDATE, and DELETE privileges, on the other hand, only TRUNCATE privilege specified for each partition is applied. I'm not sure if this behavior is expected or not. But anyway is it better to document that? For example, Access privileges may be defined and removed separately for each partition. But note that queries through a partitioned table ignore each partition's SELECT, INSERT, UPDATE and DELETE privileges, and apply only TRUNCATE one. Regards, -- Fujii Masao
