Re: ExecutorCheckPerms() hook

KaiGai,

* KaiGai Kohei (kaigai@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;
>   GRANT
>
> At that time, the 'ymj' has ownership and REFERENCES permissions on
> both of pk_tbl and fk_tbl. In this case, RI_Initial_Check() shall return
> and the fallback-seqscan will run. But,

ymj may be considered an 'owner' on that table, but in this case, it
doesn't have SELECT rights on it.  Now, you might argue that we should
assume that the owner has SELECT rights (since they're granted by
default), even if they've been revoked, but that's a whole separate
issue.

>   postgres=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a);
>   ERROR:  permission denied for relation pk_tbl
>   CONTEXT:  SQL statement "SELECT 1 FROM ONLY "public"."pk_tbl" x WHERE "a" OPERATOR(pg_catalog.=) $1 FOR SHARE OF x"

I think you've got another issue here that's not related.  Perhaps
something wrong with a patch you've applied?  Otherwise, what version of
PG is this?  Using 8.2, 8.3, 8.4 and a recent git checkout, I get:

postgres=# CREATE USER ymj;
CREATE ROLE
postgres=# CREATE TABLE pk_tbl (a int primary key, b text);
NOTICE:  CREATE TABLE / PRIMARY KEY will create implicit index "pk_tbl_pkey" for table "pk_tbl"
CREATE TABLE
postgres=# CREATE TABLE fk_tbl (x int, y text);
CREATE TABLE
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;
GRANT
postgres=# SET ROLE ymj;
SET
postgres=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a);
ALTER TABLE
postgres=>

> 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.
> Is there any other idea?

Yeah, I don't see that going anywhere...

> 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().

No, this is not a service of the executor, putting it in execMain.c does
not make any sense.

> It also suggest us where the checker functions should be deployed on the upcoming
> DDL reworks. In similar way, we will deploy them on src/backend/command/pg_database
> for example?

We'll worry about DDL when we get there.  It won't be any time soon.  I
would strongly recommend that you concentrate on building an SELinux
module using the hook function that Robert wrote or none of this is
going to end up going anywhere.  If and when we find other places which
handle DML and need adjustment, we can identify how to handle them at
that time.

Hopefully by the time we're comfortable with DML, some of the DDL
permissions checking rework will have been done and how to move forward
with allowing external security modules to handle that will be clear.
Thanks,
    Stephen