Reworks of DML permission checks
in [PgSql]
|
Prev: - GSoC - snapshot materialized view (work-in-progress) patch
Next: gSoC - ADD MERGE COMMAND - code patch submission
From: Robert Haas on 9 Jul 2010 16:53 2010/6/14 KaiGai Kohei <kaigai(a)ak.jp.nec.com>: > The attached patch tries to rework DML permission checks. > > It was mainly checked at the ExecCheckRTEPerms(), but same logic was > implemented in COPY TO/FROM statement and RI_Initial_Check(). > > This patch tries to consolidate these permission checks into a common > function to make access control decision on DML permissions. It enables > to eliminate the code duplication, and improve consistency of access > controls. This patch is listed on the CommitFest page, but I'm not sure if it represents the latest work on this topic. At a minimum, it needs to be rebased. I am not excited about moving ExecCheckRT[E]Perms to some other place in the code. It seems to me that will complicate back-patching with no corresponding advantage. I'd suggest we not do that. The COPY and RI code can call ExecCheckRTPerms() where it is. Maybe at some point we will have a grand master plan for how this should all be laid out, but right now I'd prefer localized changes. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: KaiGai Kohei on 12 Jul 2010 00:09 (2010/07/10 5:53), Robert Haas wrote: > 2010/6/14 KaiGai Kohei<kaigai(a)ak.jp.nec.com>: >> The attached patch tries to rework DML permission checks. >> >> It was mainly checked at the ExecCheckRTEPerms(), but same logic was >> implemented in COPY TO/FROM statement and RI_Initial_Check(). >> >> This patch tries to consolidate these permission checks into a common >> function to make access control decision on DML permissions. It enables >> to eliminate the code duplication, and improve consistency of access >> controls. > > This patch is listed on the CommitFest page, but I'm not sure if it > represents the latest work on this topic. At a minimum, it needs to > be rebased. > > I am not excited about moving ExecCheckRT[E]Perms to some other place > in the code. It seems to me that will complicate back-patching with > no corresponding advantage. I'd suggest we not do that. The COPY > and RI code can call ExecCheckRTPerms() where it is. Maybe at some > point we will have a grand master plan for how this should all be laid > out, but right now I'd prefer localized changes. > OK, I rebased and revised the patch not to move ExecCheckRTPerms() from executor/execMain.c. In the attached patch, DoCopy() and RI_Initial_Check() calls that function to consolidate dml access control logic. Thanks, -- KaiGai Kohei <kaigai(a)ak.jp.nec.com>
From: Robert Haas on 19 Jul 2010 14:13 2010/7/12 KaiGai Kohei <kaigai(a)ak.jp.nec.com>: > (2010/07/10 5:53), Robert Haas wrote: >> 2010/6/14 KaiGai Kohei<kaigai(a)ak.jp.nec.com>: >>> The attached patch tries to rework DML permission checks. >>> >>> It was mainly checked at the ExecCheckRTEPerms(), but same logic was >>> implemented in COPY TO/FROM statement and RI_Initial_Check(). >>> >>> This patch tries to consolidate these permission checks into a common >>> function to make access control decision on DML permissions. It enables >>> to eliminate the code duplication, and improve consistency of access >>> controls. >> >> This patch is listed on the CommitFest page, but I'm not sure if it >> represents the latest work on this topic. �At a minimum, it needs to >> be rebased. >> >> I am not excited about moving ExecCheckRT[E]Perms to some other place >> in the code. �It seems to me that will complicate back-patching with >> no corresponding advantage. �I'd suggest we not do that. � �The COPY >> and RI code can call ExecCheckRTPerms() where it is. Maybe at some >> point we will have a grand master plan for how this should all be laid >> out, but right now I'd prefer localized changes. >> > > OK, I rebased and revised the patch not to move ExecCheckRTPerms() > from executor/execMain.c. > In the attached patch, DoCopy() and RI_Initial_Check() calls that > function to consolidate dml access control logic. This patch contains a number of copies of the following code: + { + if (ereport_on_violation) + aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS, + get_rel_name(relOid)); + return false; + } What if we don't pass ereport_on_violation down to ExecCheckRTEPerms(), and just have it return a boolean? Then ExecCheckRTPerms() can throw the error if ereport_on_violation is true, and return false otherwise. With this patch, ri_triggers.c emits a compiler warning, apparently due to a missing include. Otherwise, the changes look pretty sensible, though I haven't tested them yet. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: KaiGai Kohei on 19 Jul 2010 20:24 (2010/07/20 3:13), Robert Haas wrote: > 2010/7/12 KaiGai Kohei<kaigai(a)ak.jp.nec.com>: >> (2010/07/10 5:53), Robert Haas wrote: >>> 2010/6/14 KaiGai Kohei<kaigai(a)ak.jp.nec.com>: >>>> The attached patch tries to rework DML permission checks. >>>> >>>> It was mainly checked at the ExecCheckRTEPerms(), but same logic was >>>> implemented in COPY TO/FROM statement and RI_Initial_Check(). >>>> >>>> This patch tries to consolidate these permission checks into a common >>>> function to make access control decision on DML permissions. It enables >>>> to eliminate the code duplication, and improve consistency of access >>>> controls. >>> >>> This patch is listed on the CommitFest page, but I'm not sure if it >>> represents the latest work on this topic. At a minimum, it needs to >>> be rebased. >>> >>> I am not excited about moving ExecCheckRT[E]Perms to some other place >>> in the code. It seems to me that will complicate back-patching with >>> no corresponding advantage. I'd suggest we not do that. The COPY >>> and RI code can call ExecCheckRTPerms() where it is. Maybe at some >>> point we will have a grand master plan for how this should all be laid >>> out, but right now I'd prefer localized changes. >>> >> >> OK, I rebased and revised the patch not to move ExecCheckRTPerms() >> from executor/execMain.c. >> In the attached patch, DoCopy() and RI_Initial_Check() calls that >> function to consolidate dml access control logic. > > This patch contains a number of copies of the following code: > > + { > + if (ereport_on_violation) > + aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS, > + > get_rel_name(relOid)); > + return false; > + } > > What if we don't pass ereport_on_violation down to > ExecCheckRTEPerms(), and just have it return a boolean? Then > ExecCheckRTPerms() can throw the error if ereport_on_violation is > true, and return false otherwise. > All the error messages are indeed same, so it seems to me fair enough. As long as we don't need to report the error using aclcheck_error_col(), instead of aclcheck_error(), this change will keep the code simple. If it is preferable to show users the column-name in access violations, we need to raise an error from ExecCheckRTEPerms(). > With this patch, ri_triggers.c emits a compiler warning, apparently > due to a missing include. > Oh, sorry, I'll fix it soon. Thanks, -- KaiGai Kohei <kaigai(a)ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: KaiGai Kohei on 19 Jul 2010 22:54
The attached patch is the revised one. * It was rebased to the latest git HEAD. * Prototype of ExecCheckRTEPerms() was changed; it become to return a bool value to inform the caller its access control decision, and its 'ereport_on_violation' argument has gone. * ExecCheckRTPerms() calls aclcheck_error() when ExecCheckRTEPerms() returned false, and 'ereport_on_violation' is true. * Add '#include "executor/executor.h"' on the ri_triggers.c. Thanks, (2010/07/20 9:24), KaiGai Kohei wrote: > (2010/07/20 3:13), Robert Haas wrote: >> 2010/7/12 KaiGai Kohei<kaigai(a)ak.jp.nec.com>: >>> (2010/07/10 5:53), Robert Haas wrote: >>>> 2010/6/14 KaiGai Kohei<kaigai(a)ak.jp.nec.com>: >>>>> The attached patch tries to rework DML permission checks. >>>>> >>>>> It was mainly checked at the ExecCheckRTEPerms(), but same logic was >>>>> implemented in COPY TO/FROM statement and RI_Initial_Check(). >>>>> >>>>> This patch tries to consolidate these permission checks into a common >>>>> function to make access control decision on DML permissions. It enables >>>>> to eliminate the code duplication, and improve consistency of access >>>>> controls. >>>> >>>> This patch is listed on the CommitFest page, but I'm not sure if it >>>> represents the latest work on this topic. At a minimum, it needs to >>>> be rebased. >>>> >>>> I am not excited about moving ExecCheckRT[E]Perms to some other place >>>> in the code. It seems to me that will complicate back-patching with >>>> no corresponding advantage. I'd suggest we not do that. The COPY >>>> and RI code can call ExecCheckRTPerms() where it is. Maybe at some >>>> point we will have a grand master plan for how this should all be laid >>>> out, but right now I'd prefer localized changes. >>>> >>> >>> OK, I rebased and revised the patch not to move ExecCheckRTPerms() >>> from executor/execMain.c. >>> In the attached patch, DoCopy() and RI_Initial_Check() calls that >>> function to consolidate dml access control logic. >> >> This patch contains a number of copies of the following code: >> >> + { >> + if (ereport_on_violation) >> + aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS, >> + >> get_rel_name(relOid)); >> + return false; >> + } >> >> What if we don't pass ereport_on_violation down to >> ExecCheckRTEPerms(), and just have it return a boolean? Then >> ExecCheckRTPerms() can throw the error if ereport_on_violation is >> true, and return false otherwise. >> > All the error messages are indeed same, so it seems to me fair enough. > > As long as we don't need to report the error using aclcheck_error_col(), > instead of aclcheck_error(), this change will keep the code simple. > If it is preferable to show users the column-name in access violations, > we need to raise an error from ExecCheckRTEPerms(). > >> With this patch, ri_triggers.c emits a compiler warning, apparently >> due to a missing include. >> > Oh, sorry, I'll fix it soon. > > Thanks, -- KaiGai Kohei <kaigai(a)ak.jp.nec.com> |