Re: [HACKERS] ExecutorCheckPerms() hook

2010-07-21 Thread Robert Haas
2010/5/24 KaiGai Kohei kai...@ak.jp.nec.com: (2010/05/24 22:18), Robert Haas wrote: 2010/5/24 KaiGai Koheikai...@ak.jp.nec.com: BTW, I guess the reason why permissions on attributes are not checked here is that we missed it at v8.4 development. That's a little worrying.  Can you construct

Re: [HACKERS] ExecutorCheckPerms() hook

2010-07-21 Thread KaiGai Kohei
(2010/07/22 8:45), Robert Haas wrote: 2010/5/24 KaiGai Koheikai...@ak.jp.nec.com: (2010/05/24 22:18), Robert Haas wrote: 2010/5/24 KaiGai Koheikai...@ak.jp.nec.com: BTW, I guess the reason why permissions on attributes are not checked here is that we missed it at v8.4 development. That's

Re: [HACKERS] ExecutorCheckPerms() hook

2010-07-21 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote: I think the relevant case might be where ymj owns fk_tbl but not pk_tbl, and has REFERENCES but not SELECT on pk_tbl. Come to think of it, I wonder if REFERENCES on fk_tbl ought to be sufficient to create a foreign key. Currently, it requires

Re: [HACKERS] ExecutorCheckPerms() hook

2010-07-21 Thread Stephen Frost
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote: We can find out a similar case in CreateTrigger(). If I was granted TRIGGER privilege on a certain table, I can create a new trigger on the table without its ownership. More commonly, it allows us to modify a certain property of the table without

Re: [HACKERS] ExecutorCheckPerms() hook

2010-07-21 Thread Robert Haas
On Wed, Jul 21, 2010 at 9:02 PM, Stephen Frost sfr...@snowman.net wrote: * Robert Haas (robertmh...@gmail.com) wrote: I think the relevant case might be where ymj owns fk_tbl but not pk_tbl, and has REFERENCES but not SELECT on pk_tbl. Come to think of it, I wonder if REFERENCES on fk_tbl

Re: [HACKERS] ExecutorCheckPerms() hook

2010-07-21 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote: On Wed, Jul 21, 2010 at 9:02 PM, Stephen Frost sfr...@snowman.net wrote: Errr, no.  If I grant you REFERENCES on my table, it means you can create a FK to it from some other table. Well, in that case, we should fix the fine documentation:

Re: [HACKERS] ExecutorCheckPerms() hook

2010-07-21 Thread KaiGai Kohei
(2010/07/22 10:04), Stephen Frost wrote: * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: We can find out a similar case in CreateTrigger(). If I was granted TRIGGER privilege on a certain table, I can create a new trigger on the table without its ownership. More commonly, it allows us to modify

Re: [HACKERS] ExecutorCheckPerms() hook

2010-07-09 Thread Robert Haas
On Thu, May 20, 2010 at 1:33 PM, Tom Lane t...@sss.pgh.pa.us wrote: As for committing it, I would definitely like to commit the actual hook.  If we want the hook without the contrib module that's OK with me, although I generally feel it's useful to have examples of how hooks can be used, which

Re: [HACKERS] ExecutorCheckPerms() hook

2010-06-14 Thread KaiGai Kohei
I attached three patches for the effort. Each patch tries to tackle one theme, so it is not unreasonable. But the ESP security hook patch (quite tiny) depends on the DML permission refactoring patch (relatively larger). So, Robert suggested me to reconsider the dependency of these patches. The

Re: [HACKERS] ExecutorCheckPerms() hook

2010-06-14 Thread Robert Haas
2010/6/14 KaiGai Kohei kai...@ak.jp.nec.com: I attached three patches for the effort. Each patch tries to tackle one theme, so it is not unreasonable. But the ESP security hook patch (quite tiny) depends on the DML permission refactoring patch (relatively larger). So, Robert suggested me to

Re: [HACKERS] ExecutorCheckPerms() hook

2010-06-14 Thread Stephen Frost
* Robert Haas (robertmh...@gmail.com) wrote: This is essentially the same patch that I wrote and posted several weeks ago, with changes to the comments and renaming of the identifiers. Are you trying to represent it as your own work? Ehh, I doubt it. He had included your patch in another

Re: [HACKERS] ExecutorCheckPerms() hook

2010-06-14 Thread KaiGai Kohei
(2010/06/14 21:35), Stephen Frost wrote: * Robert Haas (robertmh...@gmail.com) wrote: This is essentially the same patch that I wrote and posted several weeks ago, with changes to the comments and renaming of the identifiers. Are you trying to represent it as your own work? Ehh, I doubt

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-26 Thread KaiGai Kohei
The attached patch is a revised one for DML permission checks. List of updates: - Source code comments in the patched functions were revised. - ExecCheckRTPerms() and ExecCheckRTEPerms() were moved to aclchk.c, and renamed to chkpriv_relation_perms() and chkpriv_rte_perms(). - It took the 2nd

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-26 Thread Stephen Frost
Tom, * Tom Lane (t...@sss.pgh.pa.us) wrote: I haven't dug in the SQL spec to see if that addresses the point, but it wouldn't bother me in the least to insist that both REFERENCES and SELECT privilege are required to create an FK. Ok. If we require REFERENCES and SELECT privs to create an FK

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-26 Thread Stephen Frost
KaiGai, * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: Yes, it is entirely separate issue. I don't intend to argue whether we can assume the default PG permission allows owner to SELECT on the table, or not. This actually isn't a separate issue. It's the whole crux of it, as a matter of fact.

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-26 Thread Tom Lane
KaiGai Kohei kai...@ak.jp.nec.com writes: Hmm. If both REFERENCES and SELECT privilege are required to create a new FK constraint, why RI_Initial_Check() need to check SELECT permission prior to SPI_execute()? It eventually checks SELECT privilege during execution of the secondary query. It

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-26 Thread Stephen Frost
KaiGai, * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: The attached patch is a revised one for DML permission checks. This is certainly alot better. ToDo: - makeRangeTblEntry() stuff to allocate a RTE node with given parameter is not yet. I'd certainly like to see the above done, or to

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-26 Thread Stephen Frost
* Tom Lane (t...@sss.pgh.pa.us) wrote: Because the queries inside the triggers are done with a different current userid. Indeed, I figured that out eventually too. Sorry it took so long. :/ Thanks, Stephen signature.asc Description: Digital signature

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-26 Thread KaiGai Kohei
Stephen, thanks for comments. The attached three patches are the revised and divided ones. A: add makeRangeTblEntry() B: major reworks of DML permission checks C: add an ESP hook on the DML permission checks (2010/05/27 0:09), Stephen Frost wrote: KaiGai, * KaiGai Kohei

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-26 Thread Stephen Frost
KaiGai, * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: Stephen, thanks for comments. The attached three patches are the revised and divided ones. A: add makeRangeTblEntry() Ok, didn't actually expect that. Guess my suggestion would have been to just use makeNode() since there wasn't

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-26 Thread KaiGai Kohei
Stephen, The 'failure' may make an impression of generic errors not only permission denied. How about 'error_on_violation'? Maybe 'ereport_on_violation'? I dunno, guess one isn't really better than the other. You need to go back and fix the comment though- you still say 'abort' there.

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-25 Thread KaiGai Kohei
(2010/05/25 12:19), Robert Haas wrote: On Mon, May 24, 2010 at 9:27 PM, Stephen Frostsfr...@snowman.net wrote: * KaiGai Kohei (kai...@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

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-25 Thread Stephen Frost
KaiGai, * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: OK, the attached patch reworks it according to the way. I havn't looked at it yet, but the hook was added to ExecCheckRTPerms(), not RTE. This was for two main reasons- it seemed simpler to us and it meant that any security module

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-25 Thread Stephen Frost
KaiGai, * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: OK, the attached patch reworks it according to the way. Reviewing this patch, there are a whole slew of problems. #1: REALLY BIG ISSUE- Insufficient comment updates. You've changed function definitions in a pretty serious way as well as

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-25 Thread Stephen Frost
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote: * DoCopy() and RI_Initial_Check() were reworked to call ExecCheckRTEPerms() with locally built RangeTblEntry. Maybe I missed it somewhere, but we still need to address the case where the user doesn't have those SELECT permissions that we're looking

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-25 Thread KaiGai Kohei
(2010/05/25 21:44), Stephen Frost wrote: KaiGai, * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: OK, the attached patch reworks it according to the way. Reviewing this patch, there are a whole slew of problems. #1: REALLY BIG ISSUE- Insufficient comment updates. You've changed function

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-25 Thread KaiGai Kohei
(2010/05/25 22:59), Stephen Frost wrote: * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: * DoCopy() and RI_Initial_Check() were reworked to call ExecCheckRTEPerms() with locally built RangeTblEntry. Maybe I missed it somewhere, but we still need to address the case where the user doesn't

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-25 Thread Stephen Frost
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote: #2: REALLY BIG ISSUE- You've added ExecutorCheckPerms_hook as part of this patch- don't, we're in feature-freeze right now and should not be adding hooks at this time. The patch is intended to submit for the v9.1 development, not v9.0, isn't

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-25 Thread Stephen Frost
* KaiGai Kohei (kai...@ak.jp.nec.com) wrote: The reason why user must have SELECT privileges on the PK/FK tables is the validateForeignKeyConstraint() entirely calls SPI_execute() to verify FK constraints can be established between two tables (even if fallback path). And, the reason why

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-25 Thread KaiGai Kohei
(2010/05/26 11:12), Stephen Frost wrote: * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: #2: REALLY BIG ISSUE- You've added ExecutorCheckPerms_hook as part of this patch- don't, we're in feature-freeze right now and should not be adding hooks at this time. The patch is intended to submit for

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-25 Thread Tom Lane
Stephen Frost sfr...@snowman.net writes: That may be the case. I'm certainly more concerned with a bug in the existing code than any new code that we're working on. The question is- is this actually a user-visible bug? Or do we require that a user creating an FK needs SELECT rights on the

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-25 Thread KaiGai Kohei
(2010/05/26 12:17), Tom Lane wrote: Stephen Frostsfr...@snowman.net writes: That may be the case. I'm certainly more concerned with a bug in the existing code than any new code that we're working on. The question is- is this actually a user-visible bug? Or do we require that a user

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-24 Thread KaiGai Kohei
(2010/05/21 1:14), Robert Haas wrote: In yesterday's development meeting, we talked about the possibility of a basic SE-PostgreSQL implementation that checks permissions only for DML. Greg Smith offered the opinion that this could provide much of the benefit of SE-PostgreSQL for many users,

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-24 Thread Robert Haas
2010/5/24 KaiGai Kohei kai...@ak.jp.nec.com: BTW, I guess the reason why permissions on attributes are not checked here is that we missed it at v8.4 development. That's a little worrying. Can you construct and post a test case where this results in a user-visible failure in CVS HEAD? The

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-24 Thread KaiGai Kohei
(2010/05/24 22:18), Robert Haas wrote: 2010/5/24 KaiGai Koheikai...@ak.jp.nec.com: BTW, I guess the reason why permissions on attributes are not checked here is that we missed it at v8.4 development. That's a little worrying. Can you construct and post a test case where this results in a

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-24 Thread KaiGai Kohei
(2010/05/25 4:11), Stephen Frost wrote: * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: I'd like to point out two more points are necessary to be considered for DML permission checks in addition to ExecCheckRTPerms(). * DoCopy() Although DoCopy() is called from standard_ProcessUtility(), it

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-24 Thread Stephen Frost
KaiGai, * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: postgres=# ALTER TABLE pk_tbl OWNER TO ymj; ALTER TABLE postgres=# ALTER TABLE fk_tbl OWNER TO ymj; ALTER TABLE postgres=# REVOKE ALL ON pk_tbl, fk_tbl FROM ymj; REVOKE postgres=# GRANT REFERENCES ON pk_tbl, fk_tbl TO ymj;

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-24 Thread Stephen Frost
* KaiGai Kohei (kai...@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

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-24 Thread KaiGai Kohei
(2010/05/25 10:13), Stephen Frost wrote: KaiGai, * KaiGai Kohei (kai...@ak.jp.nec.com) wrote: postgres=# ALTER TABLE pk_tbl OWNER TO ymj; ALTER TABLE postgres=# ALTER TABLE fk_tbl OWNER TO ymj; ALTER TABLE postgres=# REVOKE ALL ON pk_tbl, fk_tbl FROM ymj; REVOKE

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-24 Thread KaiGai Kohei
(2010/05/25 10:27), Stephen Frost wrote: * KaiGai Kohei (kai...@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

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-24 Thread Robert Haas
2010/5/24 KaiGai Kohei kai...@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

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-24 Thread Robert Haas
On Mon, May 24, 2010 at 9:27 PM, Stephen Frost sfr...@snowman.net wrote: * KaiGai Kohei (kai...@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

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-24 Thread Tom Lane
Stephen Frost sfr...@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

[HACKERS] ExecutorCheckPerms() hook

2010-05-20 Thread Robert Haas
In yesterday's development meeting, we talked about the possibility of a basic SE-PostgreSQL implementation that checks permissions only for DML. Greg Smith offered the opinion that this could provide much of the benefit of SE-PostgreSQL for many users, while being much simpler. In fact,

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-20 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes: In yesterday's development meeting, we talked about the possibility of a basic SE-PostgreSQL implementation that checks permissions only for DML. Greg Smith offered the opinion that this could provide much of the benefit of SE-PostgreSQL for many

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-20 Thread Robert Haas
On Thu, May 20, 2010 at 12:32 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: In yesterday's development meeting, we talked about the possibility of a basic SE-PostgreSQL implementation that checks permissions only for DML.  Greg Smith offered the opinion that

Re: [HACKERS] ExecutorCheckPerms() hook

2010-05-20 Thread Tom Lane
Robert Haas robertmh...@gmail.com writes: On Thu, May 20, 2010 at 12:32 PM, Tom Lane t...@sss.pgh.pa.us wrote: Hm, I think you need to ignore RT entries that have no requiredPerms bits set.  (Not that it matters too much, unless you were proposing to actually commit this contrib module.)