ExecutorCheckPerms() hook
in [PgSql]
|
From: KaiGai Kohei on 24 May 2010 22:56 (2010/05/25 10:27), Stephen Frost wrote: > * KaiGai Kohei (kaigai(a)ak.jp.nec.com) wrote: >> We have two options; If the checker function takes the list of RangeTblEntry, >> it will be comfortable to ExecCheckRTPerms(), but not DoCopy(). Inversely, >> if the checker function takes arguments in my patch, it will be comfortable >> to DoCopy(), but ExecCheckRTPerms(). >> >> In my patch, it takes 6 arguments, but we can reference all of them from >> the given RangeTblEntry. On the other hand, if DoCopy() has to set up >> a pseudo RangeTblEntry to call checker function, it entirely needs to set >> up similar or a bit large number of variables. > > I don't know that it's really all that difficult to set up an RT in > DoCopy or RI_Initial_Check(). In my opinion, those are the strange or > corner cases- not the Executor code, through which all 'regular' DML is > done. It makes me wonder if COPY shouldn't have been implemented using > the Executor instead, but that's, again, a completely separate topic. > It wasn't, but it wants to play like it operates in the same kind of way > as INSERT, so it needs to pick up the slack. > Yes, it is not difficult to set up. The reason why I prefer the checker function takes 6 arguments are that DoCopy() / RI_Initial_Check() has to set up RangeTblEntry in addition to Bitmap set, but we don't have any other significant reason. OK, let's add a hook in the ExecCheckRTPerms(). >>>> * RI_Initial_Check() >> >> It seems to me the permission checks in RI_Initial_Check() is a bit ad-hoc. > > I agree with this- my proposal would address this in a way whih would be > guaranteed to be consistant: by using the same code path to do both > checks. I'm still not thrilled with how RI_Initial_Check() works, but > rewriting that isn't part of this. I agree to ignore the problem right now. It implicitly assume the owner has SELECT privilege on the FK/PK tables, so the minimum SELinux module also implicitly assume the client has similar permissions on it. >> In this case, we should execute the secondary query without permission checks, >> because the permissions of ALTER TABLE is already checked, and we can trust >> the given query is harmless. > > I dislike the idea of providing a new SPI interfance (on the face of > it), and also dislike the idea of having a "skip all permissions > checking" option for anything which resembles SPI. I would rather ask > the question of if it really makes sense to use SPI to check FKs as > they're being added, but we're not going to solve that issue here. Apart from the topic of this thread, I guess it allows us to utilize query optimization and cascaded triggers to implement FK constraints with minimum code size. 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: Robert Haas on 24 May 2010 23:16 2010/5/24 KaiGai Kohei <kaigai(a)ak.jp.nec.com>: > I think we need a new SPI_*() interface which allows to run the given query > without any permission checks, because these queries are purely internal stuff, > so we can trust the query is harmless. [...] > I'm afraid of that the src/backend/catalog/aclchk.c will become overcrowding > in the future. If it is ugly to deploy checker functions in separated dir/files, > I think it is an idea to put it on the execMain.c, instead of ExecCheckRTEPerms(). Both of these are bad ideas, for reasons Stephen Frost has articulated well. -- 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: Robert Haas on 24 May 2010 23:19 On Mon, May 24, 2010 at 9:27 PM, Stephen Frost <sfrost(a)snowman.net> wrote: > * KaiGai Kohei (kaigai(a)ak.jp.nec.com) wrote: >> We have two options; If the checker function takes the list of RangeTblEntry, >> it will be comfortable to ExecCheckRTPerms(), but not DoCopy(). Inversely, >> if the checker function takes arguments in my patch, it will be comfortable >> to DoCopy(), but ExecCheckRTPerms(). >> >> In my patch, it takes 6 arguments, but we can reference all of them from >> the given RangeTblEntry. On the other hand, if DoCopy() has to set up >> a pseudo RangeTblEntry to call checker function, it entirely needs to set >> up similar or a bit large number of variables. > > I don't know that it's really all that difficult to set up an RT in > DoCopy or RI_Initial_Check(). In my opinion, those are the strange or > corner cases- not the Executor code, through which all 'regular' DML is > done. It makes me wonder if COPY shouldn't have been implemented using > the Executor instead, but that's, again, a completely separate topic. > It wasn't, but it wants to play like it operates in the same kind of way > as INSERT, so it needs to pick up the slack. I think this approach is definitely worth investigating. KaiGai, can you please work up what the patch would look like if we do it this way? Thanks, -- 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: Tom Lane on 24 May 2010 23:25 Stephen Frost <sfrost(a)snowman.net> writes: > ... It makes me wonder if COPY shouldn't have been implemented using > the Executor instead, but that's, again, a completely separate topic. > It wasn't, but it wants to play like it operates in the same kind of way > as INSERT, so it needs to pick up the slack. FWIW, we've shifted COPY more towards using executor support over the years. I'm pretty sure that it didn't originally use the executor's index-entry-insertion infrastructure, for instance. Building an RT entry seems like a perfectly sane thing to do in order to make it use the executor's permissions infrastructure. regards, tom lane -- 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 25 May 2010 05:10
(2010/05/25 12:19), Robert Haas wrote: > On Mon, May 24, 2010 at 9:27 PM, Stephen Frost<sfrost(a)snowman.net> wrote: >> * KaiGai Kohei (kaigai(a)ak.jp.nec.com) wrote: >>> We have two options; If the checker function takes the list of RangeTblEntry, >>> it will be comfortable to ExecCheckRTPerms(), but not DoCopy(). Inversely, >>> if the checker function takes arguments in my patch, it will be comfortable >>> to DoCopy(), but ExecCheckRTPerms(). >>> >>> In my patch, it takes 6 arguments, but we can reference all of them from >>> the given RangeTblEntry. On the other hand, if DoCopy() has to set up >>> a pseudo RangeTblEntry to call checker function, it entirely needs to set >>> up similar or a bit large number of variables. >> >> I don't know that it's really all that difficult to set up an RT in >> DoCopy or RI_Initial_Check(). In my opinion, those are the strange or >> corner cases- not the Executor code, through which all 'regular' DML is >> done. It makes me wonder if COPY shouldn't have been implemented using >> the Executor instead, but that's, again, a completely separate topic. >> It wasn't, but it wants to play like it operates in the same kind of way >> as INSERT, so it needs to pick up the slack. > > I think this approach is definitely worth investigating. KaiGai, can > you please work up what the patch would look like if we do it this > way? OK, the attached patch reworks it according to the way. * ExecCheckRTEPerms() becomes to take 2nd argument the caller to suggest behavior on access violation. The 'abort' argument is true, it raises an error using aclcheck_error() or ereport(). Otherwise, it returns false immediately without rest of checks. * DoCopy() and RI_Initial_Check() were reworked to call ExecCheckRTEPerms() with locally built RangeTblEntry. Thanks, -- KaiGai Kohei <kaigai(a)ak.jp.nec.com> |