Thread

ExecutorCheckPerms() hook

Robert Haas <robertmhaas@gmail.com> — 2010-05-20T16:14:44Z

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, SE-PostgreSQL would need to get control in just one place:
ExecCheckRTPerms.  This morning, Stephen Frost and I worked up a quick
patch showing how we could add a hook here to let a hypothetical
SE-PostgreSQL module get control in the relevant place.  The attached
patch also includes a toy contrib module showing how it could be used
to enforce arbitrary security policy.

I don't think that this by itself would be quite enough framework for
a minimal SE-PostgreSQL implementation - for that, you'd probably need
an object-labeling facility in core which SE-PostgreSQL could leverage
- or else some other way to determine which the label associated with
a given object - but I think that plus this would be enough.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: ExecutorCheckPerms() hook

Tom Lane <tgl@sss.pgh.pa.us> — 2010-05-20T16:32:06Z

Robert Haas <robertmhaas@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 users, while being much simpler.
>  In fact, SE-PostgreSQL would need to get control in just one place:
> ExecCheckRTPerms.  This morning, Stephen Frost and I worked up a quick
> patch showing how we could add a hook here to let a hypothetical
> SE-PostgreSQL module get control in the relevant place.  The attached
> patch also includes a toy contrib module showing how it could be used
> to enforce arbitrary security policy.

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

			regards, tom lane

Re: ExecutorCheckPerms() hook

Robert Haas <robertmhaas@gmail.com> — 2010-05-20T16:37:07Z

On Thu, May 20, 2010 at 12:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@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 users, while being much simpler.
>>  In fact, SE-PostgreSQL would need to get control in just one place:
>> ExecCheckRTPerms.  This morning, Stephen Frost and I worked up a quick
>> patch showing how we could add a hook here to let a hypothetical
>> SE-PostgreSQL module get control in the relevant place.  The attached
>> patch also includes a toy contrib module showing how it could be used
>> to enforce arbitrary security policy.
>
> 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.)

Well, that's an easy change - just out of curiosity, how do we end up
with RT entries with no requiredPerm bits set?

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 is why I took the time to produce a working
example.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: ExecutorCheckPerms() hook

Tom Lane <tgl@sss.pgh.pa.us> — 2010-05-20T17:33:55Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, May 20, 2010 at 12:32 PM, Tom Lane <tgl@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.)

> Well, that's an easy change - just out of curiosity, how do we end up
> with RT entries with no requiredPerm bits set?

Inheritance child tables look like that now, per the discussion
awhile back that a SELECT on the parent shouldn't require any
particular permission on the individual child tables.  IIRC there
are some other cases involving views too, but those are probably
just optimizations (ie not do duplicate permissions checks) rather
than something that would result in a user-visible behavioral issue.

> 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 is why I took the time to produce a working
> example.

+1 on committing the hook.  As for the contrib module, it doesn't strike
me that there's much of a use-case for it as is.  I think it's enough
that it's available in the -hackers archives.

			regards, tom lane

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-05-24T06:48:55Z

(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, while being much simpler.
>   In fact, SE-PostgreSQL would need to get control in just one place:
> ExecCheckRTPerms.  This morning, Stephen Frost and I worked up a quick
> patch showing how we could add a hook here to let a hypothetical
> SE-PostgreSQL module get control in the relevant place.  The attached
> patch also includes a toy contrib module showing how it could be used
> to enforce arbitrary security policy.
> 
> I don't think that this by itself would be quite enough framework for
> a minimal SE-PostgreSQL implementation - for that, you'd probably need
> an object-labeling facility in core which SE-PostgreSQL could leverage
> - or else some other way to determine which the label associated with
> a given object - but I think that plus this would be enough.

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 performs
as DML statement, rather than DDL. It check ACL_SELECT or ACL_INSERT on
the copied table or attributes, similar to what ExecCheckRTEPerms() doing.

* RI_Initial_Check()

RI_Initial_Check() is a function called on ALTER TABLE command to add FK
constraints between two relations. The permission to execute this ALTER TABLE
command itself is checked on ATPrepCmd() and ATAddForeignKeyConstraint(),
so it does not affect anything on the DML permission reworks.

When we add a new FK constraint, both of the existing FK and PK relations have
to satify the new constraint. So, RI_Initial_Check() tries to check whether the
PK relation has corresponding tuples to FK relation, or not.
Then, it tries to execute a secondary query using SPI_*() functions, if no
access violations are expected. Otherwise, it scans the FK relation with
per tuple checks sequentionally (see, validateForeignKeyConstraint()), but slow.

If we have an external security provider which will deny accesses on the FK/PK
relation, but the default PG checks allows it, the RI_Initial_Check() tries to
execute secondary SELECT statement, then it raises an access violation error,
although we are already allowed to execute ALTER TABLE statement.

Therefore, we also need to check DML permissions at RI_Initial_Check() to avoid
unexpected access violation error, prior to the secondary query.

BTW, I guess the reason why permissions on attributes are not checked here is
that we missed it at v8.4 development.

The attached patch provides a common checker function of DML, and modifies
ExecCheckRTPerms(), CopyTo() and RI_Initial_Check() to call the checker
function instead of individual ACL checks.

The most part of the checker function is cut & paste from ExecCheckRTEPerms(),
but its arguments are modified for easy invocation from other functions.

  extern bool check_dml_permissions(Oid relOid,
                                    Oid userId,
                                    AclMode requiredPerms,
                                    Bitmapset *selCols,
                                    Bitmapset *modCols,
                                    bool abort);

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: ExecutorCheckPerms() hook

Robert Haas <robertmhaas@gmail.com> — 2010-05-24T13:18:40Z

2010/5/24 KaiGai Kohei <kaigai@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 attached patch provides a common checker function of DML, and modifies
> ExecCheckRTPerms(), CopyTo() and RI_Initial_Check() to call the checker
> function instead of individual ACL checks.

This looks pretty sane to me, although I have not done a full review.
I am disinclined to create a whole new directory for it.   I think the
new function should go in src/backend/catalog/aclchk.c and be declared
in src/include/utils/acl.h.  If that sounds reasonable to you, please
revise and post an updated patch.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-05-24T19:11:11Z

* KaiGai Kohei (kaigai@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 performs
> as DML statement, rather than DDL. It check ACL_SELECT or ACL_INSERT on
> the copied table or attributes, similar to what ExecCheckRTEPerms() doing.

Rather than construct a complicated API for this DML activity, why don't
we just make ExecCheckRTPerms available for DoCopy to use?  It seems
like we could move ExecCheckRTPerms() to acl.c without too much trouble.
acl.h already includes parsenodes.h and has knowledge of RangeVar's.
Once DoCopy is using that, this issue resolves itself with the hook that
Robert already wrote up.

> * RI_Initial_Check()
> 
> RI_Initial_Check() is a function called on ALTER TABLE command to add FK
> constraints between two relations. The permission to execute this ALTER TABLE
> command itself is checked on ATPrepCmd() and ATAddForeignKeyConstraint(),
> so it does not affect anything on the DML permission reworks.

I'm not really thrilled with how RI_Initial_Check() does it's own
permissions checking and then calls SPI expecting things to 'just work'.
Not sure if there's some way we could handle failure from SPI, or, if it
was changed to call ExecCheckRTPerms() instead, how it would handle
failure cases from there.  One possible solution would be to have an
additional option to ExecCheckRTPerms() which asks it to just check and
return false if there's a problem, rather than unconditionally calling
aclcheck_error() whenever it finds a problem.

Using the same function for both the initial check in RI_Initial_Check()
and then from SPI would eliminate issues where those two checks disagree
for some reason, which would be good in the general case.

> BTW, I guess the reason why permissions on attributes are not checked here is
> that we missed it at v8.4 development.

Indeed, but at the same time, this looks to be a 'fail-safe' situation.
Basically, this is checking table-level permissions, which, if you have,
gives you sufficient rights to SELECT against the table (any column).
What this isn't doing is allowing the option of column-level permissions
to be sufficient for this requirement.  That's certainly an oversight in
the column-level permissions handling (sorry about that), but it's not
horrible- there's a workaround if RI_Initial_Check returns false already
anyway.

Basically, if you are using column-level privs, and you have necessary
rights to do this w/ those permissions (but don't have table-level
rights), it's not going to be as fast as it could be.

> The most part of the checker function is cut & paste from ExecCheckRTEPerms(),
> but its arguments are modified for easy invocation from other functions.

As mentioned above, it seems like this would be better the other way-
have the callers build RangeTbl's and then call ExecCheckRTPerms().  It
feels like that approach might be more 'future-proof' as well.

	Thanks,

		Stephen

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-05-25T00:37:01Z

(2010/05/24 22:18), Robert Haas wrote:
> 2010/5/24 KaiGai Kohei<kaigai@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?

Sorry, after more detailed consideration, it seems to me the permission
checks in RI_Initial_Check() and its fallback mechanism are nonsense.

See the following commands.

  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

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,

  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"

From more careful observation of the code, the validateForeignKeyConstraint()
also calls RI_FKey_check_ins() for each scanned tuples, but it also execute
SELECT statement using SPI_*() interface internally.

In other words, both of execution paths entirely require SELECT permission
to validate new FK constraint.

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?

>> The attached patch provides a common checker function of DML, and modifies
>> ExecCheckRTPerms(), CopyTo() and RI_Initial_Check() to call the checker
>> function instead of individual ACL checks.
> 
> This looks pretty sane to me, although I have not done a full review.
> I am disinclined to create a whole new directory for it.   I think the
> new function should go in src/backend/catalog/aclchk.c and be declared
> in src/include/utils/acl.h.  If that sounds reasonable to you, please
> revise and post an updated patch.
> 

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

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?

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-05-25T01:08:38Z

(2010/05/25 4:11), Stephen Frost wrote:
> * KaiGai Kohei (kaigai@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 performs
>> as DML statement, rather than DDL. It check ACL_SELECT or ACL_INSERT on
>> the copied table or attributes, similar to what ExecCheckRTEPerms() doing.
> 
> Rather than construct a complicated API for this DML activity, why don't
> we just make ExecCheckRTPerms available for DoCopy to use?  It seems
> like we could move ExecCheckRTPerms() to acl.c without too much trouble.
> acl.h already includes parsenodes.h and has knowledge of RangeVar's.
> Once DoCopy is using that, this issue resolves itself with the hook that
> Robert already wrote up.

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.

As I replied in the earlier message, it may be an idea to rename and change
the definition of ExecCheckRTEPerms() without moving it anywhere.

>> * RI_Initial_Check()
>>
>> RI_Initial_Check() is a function called on ALTER TABLE command to add FK
>> constraints between two relations. The permission to execute this ALTER TABLE
>> command itself is checked on ATPrepCmd() and ATAddForeignKeyConstraint(),
>> so it does not affect anything on the DML permission reworks.
> 
> I'm not really thrilled with how RI_Initial_Check() does it's own
> permissions checking and then calls SPI expecting things to 'just work'.
> Not sure if there's some way we could handle failure from SPI, or, if it
> was changed to call ExecCheckRTPerms() instead, how it would handle
> failure cases from there.  One possible solution would be to have an
> additional option to ExecCheckRTPerms() which asks it to just check and
> return false if there's a problem, rather than unconditionally calling
> aclcheck_error() whenever it finds a problem.
> 
> Using the same function for both the initial check in RI_Initial_Check()
> and then from SPI would eliminate issues where those two checks disagree
> for some reason, which would be good in the general case.

Sorry, I missed the fallback path also needs SELECT permissions because
validateForeignKeyConstraint() calls RI_FKey_check_ins() which entirely
tries to execute SELECT statement using SPI_*() interface.
But, it is a separate issue from the DML permission reworks.

It seems to me the permission checks in RI_Initial_Check() is a bit ad-hoc.
What we really want to do here is validation of the new FK constraints.
So, the validateForeignKeyConstraint() intends to provide a fall-back code
when table-level permission is denied, doesn't 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.

>> BTW, I guess the reason why permissions on attributes are not checked here is
>> that we missed it at v8.4 development.
> 
> Indeed, but at the same time, this looks to be a 'fail-safe' situation.
> Basically, this is checking table-level permissions, which, if you have,
> gives you sufficient rights to SELECT against the table (any column).
> What this isn't doing is allowing the option of column-level permissions
> to be sufficient for this requirement.  That's certainly an oversight in
> the column-level permissions handling (sorry about that), but it's not
> horrible- there's a workaround if RI_Initial_Check returns false already
> anyway.

Yes, it is harmless expect for performances in a corner-case.
If user have table-level permissions, it does not need to check column-
level permissions, even if it is implemented.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-05-25T01:13:07Z

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

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-05-25T01:27:45Z

* KaiGai Kohei (kaigai@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.

> As I replied in the earlier message, it may be an idea to rename and change
> the definition of ExecCheckRTEPerms() without moving it anywhere.

And, again, I don't see that as a good idea at all.

> >> * 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.

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

	Thanks,

		Stephen

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-05-25T02:56:19Z

(2010/05/25 10:13), Stephen Frost wrote:
> 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.

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.

>>    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=>

Sorry, I missed to copy & paste INSERT statement just after CREATE TABLE.

The secondary RI_FKey_check_ins() is invoked during the while() loop using
heap_getnext(), so it is not called for empty table.

For correctness,

  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=# INSERT INTO pk_tbl VALUES (1,'aaa'), (2,'bbb'), (3,'ccc');
| INSERT 0 3
| postgres=# INSERT INTO fk_tbl VALUES (1,'xxx'), (2,'yyy'), (3,'zzz');
| INSERT 0 3
  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);
  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 could reproduce it on the 8.4.4, but didn't try on the prior releases.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-05-25T02:56:40Z

(2010/05/25 10:27), Stephen Frost wrote:
> * KaiGai Kohei (kaigai@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@ak.jp.nec.com>

Re: ExecutorCheckPerms() hook

Robert Haas <robertmhaas@gmail.com> — 2010-05-25T03:16:39Z

2010/5/24 KaiGai Kohei <kaigai@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

Re: ExecutorCheckPerms() hook

Robert Haas <robertmhaas@gmail.com> — 2010-05-25T03:19:06Z

On Mon, May 24, 2010 at 9:27 PM, Stephen Frost <sfrost@snowman.net> wrote:
> * KaiGai Kohei (kaigai@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

Re: ExecutorCheckPerms() hook

Tom Lane <tgl@sss.pgh.pa.us> — 2010-05-25T03:25:58Z

Stephen Frost <sfrost@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

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-05-25T09:10:03Z

(2010/05/25 12:19), Robert Haas wrote:
> On Mon, May 24, 2010 at 9:27 PM, Stephen Frost<sfrost@snowman.net>  wrote:
>> * KaiGai Kohei (kaigai@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@ak.jp.nec.com>

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-05-25T10:28:49Z

KaiGai,

* KaiGai Kohei (kaigai@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 implemented would have access to
essentially everything we know the query is going to use all at once
(instead of on a per-range-table basis).  That could be particularly
useful if you wanted to, say, enforce a constraint that says "no two
tables of different labels shall ever be used in the same query at the
same time" (perhaps with some caveats on that, etc).

Could you change this patch to use ExecCheckRTPerms() instead?

> * 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.

Does this change fix the issue you had in RI_Initial_Check()?

	Thanks,

		Stephen

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-05-25T12:44:20Z

KaiGai,

* KaiGai Kohei (kaigai@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 moved some code
around such that some of the previous comments don't make sense.  You
have got to update comments when you're writing a patch.  Indeed, the
places I see a changes in comments are when you've removed what appears
to still be valid and appropriate comments, or places where you've added
comments which are just blatently wrong with the submitted patch.

#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.

#3: You didn't move ExecCheckRTPerms() and ExecCheckRTEPerms() to
utils/acl and instead added executor/executor.h to rt_triggers.c.
I don't particularly like that.  I admit that DoCopy() already knew
about the executor, and if that were the only case outside of the
executor where ExecCheckRTPerms() was getting called it'd probably be
alright, but we already have another place that wants to use it, so
let's move it to a more appropriate place.

#4: As mentioned previously, the hook (which should be added in a
separate patch anyway) makes more sense to me to be in
ExecCheckRTPerms(), not ExecCheckRTEPerms().  This also means that we
need to be calling ExecCheckRTPerms() from DoCopy and
RI_Initial_Check(), to make sure that the hook gets called.  To that
end, I wouldn't even expose ExecCheckRTEPerms() outside of acl.c.  Also,
there should be a big comment about not using or calling
ExecCheckRTEPerms() directly outside of ExecCheckRTPerms() since the
hook would then be skipped.

#5: In DoCopy, you can remove relPerms and remainingPerms, but I'd
probably leave required_access up near the top and then just use it to
set rte->required_access directly rather than moving that bit deep down
into the function.

#6: I havn't checked yet, but if there are other things in an RTE which
would make sense in the DoCopy case, beyond just what's needed for the
permissions checking, and which wouldn't be 'correct' with a NULL'd
value, I would set those.  Yes, we're building the RTE to check
permissions, but we don't want someone downstream to be suprised when
they make a change to something in the permissions checking and discover
that a value in RTE they expected to be there wasn't valid.  Even more
so, if there are function helpers which can be used to build an RTE, we
should be using them.  The same goes for RI_Initial_Check().

#7: I'd move the conditional if (is_from) into the foreach which is
building the columnsSet and eliminate the need for columnsSet; I don't
see that it's really adding much here.

#8: When moving ExecCheckRTPerms(), you should rename it to be more like
the other function calls in acl.h  Perhaps pg_rangetbl_aclcheck()?
Also, it should return an actual AclResult instead of just true/false.

	Thanks,

		Stephen

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-05-25T13:59:14Z

* KaiGai Kohei (kaigai@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 for in
RI_Initial_Check(), right?  KaiGai, your patch should be addressing that
in a similar fashion..

	Thanks,

		Stephen

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-05-26T00:34:38Z

(2010/05/25 21:44), Stephen Frost wrote:
> KaiGai,
> 
> * KaiGai Kohei (kaigai@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 moved some code
> around such that some of the previous comments don't make sense.  You
> have got to update comments when you're writing a patch.  Indeed, the
> places I see a changes in comments are when you've removed what appears
> to still be valid and appropriate comments, or places where you've added
> comments which are just blatently wrong with the submitted patch.

Hmm. I'll revise/add the comment around the patched code.

> #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 it?

> #3: You didn't move ExecCheckRTPerms() and ExecCheckRTEPerms() to
> utils/acl and instead added executor/executor.h to rt_triggers.c.
> I don't particularly like that.  I admit that DoCopy() already knew
> about the executor, and if that were the only case outside of the
> executor where ExecCheckRTPerms() was getting called it'd probably be
> alright, but we already have another place that wants to use it, so
> let's move it to a more appropriate place.

Sorry, I'm a bit confused.
It seemed to me you suggested to utilize ExecCheckRTPerms() rather than
moving its logic anywhere, so I kept it here. (Was it misunderstand?)

If so, but, I doubt utils/acl is the best placeholder of the moved
ExecCheckRTPerms(), because the checker function calls both of the
default acl functions and a optional external security function.
It means the ExecCheckRTPerms() is caller of acl functions, not acl
function itself, isn't it?
In other words, I wonder we should categorize a function X which calls
A and (optionally) B as a part of A.

I agreed the checker function is not a part of executor, but it is
also not a part of acl functions in my opinion.

If it is disinclined to create a new directory to deploy the checker
function, my preference is src/backend/utils/adt/security.c and
src/include/utils/security.h .

> #4: As mentioned previously, the hook (which should be added in a
> separate patch anyway) makes more sense to me to be in
> ExecCheckRTPerms(), not ExecCheckRTEPerms().  This also means that we
> need to be calling ExecCheckRTPerms() from DoCopy and
> RI_Initial_Check(), to make sure that the hook gets called.  To that
> end, I wouldn't even expose ExecCheckRTEPerms() outside of acl.c.  Also,
> there should be a big comment about not using or calling
> ExecCheckRTEPerms() directly outside of ExecCheckRTPerms() since the
> hook would then be skipped.

I don't have any differences in preference between ExecCheckRTPerms()
and ExecCheckRTEPerms(), except for DoCopy() and RI_Initial_Check()
have to call the checker function with list_make1(&rte), instead of &rte.

> #5: In DoCopy, you can remove relPerms and remainingPerms, but I'd
> probably leave required_access up near the top and then just use it to
> set rte->required_access directly rather than moving that bit deep down
> into the function.

OK,

> #6: I havn't checked yet, but if there are other things in an RTE which
> would make sense in the DoCopy case, beyond just what's needed for the
> permissions checking, and which wouldn't be 'correct' with a NULL'd
> value, I would set those.  Yes, we're building the RTE to check
> permissions, but we don't want someone downstream to be suprised when
> they make a change to something in the permissions checking and discover
> that a value in RTE they expected to be there wasn't valid.  Even more
> so, if there are function helpers which can be used to build an RTE, we
> should be using them.  The same goes for RI_Initial_Check().

Are you saying something like makeFuncExpr()?
I basically agree. However, should it be done in this patch?

> #7: I'd move the conditional if (is_from) into the foreach which is
> building the columnsSet and eliminate the need for columnsSet; I don't
> see that it's really adding much here.

OK,

> #8: When moving ExecCheckRTPerms(), you should rename it to be more like
> the other function calls in acl.h  Perhaps pg_rangetbl_aclcheck()?
> Also, it should return an actual AclResult instead of just true/false.

See the comments in #3.
And, if the caller has to handle aclcheck_error(), user cannot distinguish
access violation errors between the default PG permission and any other
external security stuff, isn't it?

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-05-26T00:52:13Z

(2010/05/25 22:59), Stephen Frost wrote:
> * KaiGai Kohei (kaigai@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 for in
> RI_Initial_Check(), right?  KaiGai, your patch should be addressing that
> in a similar fashion..

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 RI_Initial_Check() now calls pg_class_aclcheck() is
to try to avoid unexpected access violation error because of SPI_execute().
However, the fallback path also calls SPI_execute() entirely, so I concluded
the permission checks in RI_Initial_Check() is nonsense.

However, it is an independent issue right now, so I kept it as is.

The origin of the matter is that we applies unnecessary permission checks,
although it is purely internal use and user was already checked to execute
whole of ALTER TABLE statement. Right?

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-05-26T02:12:46Z

* KaiGai Kohei (kaigai@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 it?

That really depends on if this is actually fixing a bug in the existing
code or not.  I'm on the fence about that at the moment, to be honest.
I was trying to find if we expliitly say that SELECT rights are needed
to reference a column but wasn't able to.  If every code path is
expecting that, then perhaps we should just document it that way and
move on.  In that case, all these changes would be for 9.1.  If we
decide the current behavior is a bug, it might be something which could
be fixed in 9.0 and maybe back-patched.

In *either* case, given that one is a 'clean-up' patch and the other is
'new functionality', they should be independent *anyway*.  Small
incremental changes that don't break things when applied is what we're
shooting for here.

> > #3: You didn't move ExecCheckRTPerms() and ExecCheckRTEPerms() to
> > utils/acl and instead added executor/executor.h to rt_triggers.c.
> > I don't particularly like that.  I admit that DoCopy() already knew
> > about the executor, and if that were the only case outside of the
> > executor where ExecCheckRTPerms() was getting called it'd probably be
> > alright, but we already have another place that wants to use it, so
> > let's move it to a more appropriate place.
> 
> Sorry, I'm a bit confused.
> It seemed to me you suggested to utilize ExecCheckRTPerms() rather than
> moving its logic anywhere, so I kept it here. (Was it misunderstand?)

I'm talking about moving the whole function (all 3 lines of it) to
somewhere else and then reworking the function to be more appropriate
based on it's new location (including renaming and changing arguments
and return values, as appropriate).

> If so, but, I doubt utils/acl is the best placeholder of the moved
> ExecCheckRTPerms(), because the checker function calls both of the
> default acl functions and a optional external security function.

Can you explain why you think that having a function in utils/acl (eg:
include/utils/acl.h and backend/utils/aclchk.c) which calls default acl
functions and an allows for an external hook would be a bad idea?

> It means the ExecCheckRTPerms() is caller of acl functions, not acl
> function itself, isn't it?

It's providing a higher-level service, sure, but there's nothing
particularly interesting or special about what it's doing in this case,
and, we need it in multiple places.  Why duplicate it?

> I agreed the checker function is not a part of executor, but it is
> also not a part of acl functions in my opinion.
> 
> If it is disinclined to create a new directory to deploy the checker
> function, my preference is src/backend/utils/adt/security.c and
> src/include/utils/security.h .

We don't need a new directory or file for one function, as Robert
already pointed out.

> > #6: I havn't checked yet, but if there are other things in an RTE which
> > would make sense in the DoCopy case, beyond just what's needed for the
> > permissions checking, and which wouldn't be 'correct' with a NULL'd
> > value, I would set those.  Yes, we're building the RTE to check
> > permissions, but we don't want someone downstream to be suprised when
> > they make a change to something in the permissions checking and discover
> > that a value in RTE they expected to be there wasn't valid.  Even more
> > so, if there are function helpers which can be used to build an RTE, we
> > should be using them.  The same goes for RI_Initial_Check().
> 
> Are you saying something like makeFuncExpr()?
> I basically agree. However, should it be done in this patch?

Actually, I mean looking for, and using, things like
markRTEForSelectPriv() and addRangeTableEntry() or
addRangeTableEntryForRelation().

> > #8: When moving ExecCheckRTPerms(), you should rename it to be more like
> > the other function calls in acl.h  Perhaps pg_rangetbl_aclcheck()?
> > Also, it should return an actual AclResult instead of just true/false.
> 
> See the comments in #3.
> And, if the caller has to handle aclcheck_error(), user cannot distinguish
> access violation errors between the default PG permission and any other
> external security stuff, isn't it?

I'm not suggesting that the caller handle aclcheck_error()..
ExecCheckRTPerms() could just as easily have a flag which indicates if
it will call aclcheck_error() or not, and if not, to return an
AclResult to the caller.  That flag could then be passed to
ExecCheckRTEPerms() as you have it now.

	Thanks,

		Stephen

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-05-26T02:16:43Z

* KaiGai Kohei (kaigai@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 RI_Initial_Check() now calls pg_class_aclcheck() is
> to try to avoid unexpected access violation error because of SPI_execute().
> However, the fallback path also calls SPI_execute() entirely, so I concluded
> the permission checks in RI_Initial_Check() is nonsense.

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 primary table?  If so, it's
still a bug, but at that point it's a bug in our documentation where we
don't mention that SELECT rights are also needed.

Anyone know what the SQL spec says about this (if anything...)?

> However, it is an independent issue right now, so I kept it as is.

Uh, I don't really see it as independent..  If we have a bug there that
we need to fix, and it's because we have two different bits of code
trying to do the same checking, we should fix it be eliminating the
duplicate checking, imv.

> The origin of the matter is that we applies unnecessary permission checks,
> although it is purely internal use and user was already checked to execute
> whole of ALTER TABLE statement. Right?

That's certainly a nice thought, but given the complexity in ALTER
TABLE, in particular with regard to permissions checking, I have no idea
if what it's doing is intentional or wrong.

		Thanks,

			Stephen

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-05-26T03:04:46Z

(2010/05/26 11:12), Stephen Frost wrote:
> * KaiGai Kohei (kaigai@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 it?
> 
> That really depends on if this is actually fixing a bug in the existing
> code or not.  I'm on the fence about that at the moment, to be honest.
> I was trying to find if we expliitly say that SELECT rights are needed
> to reference a column but wasn't able to.  If every code path is
> expecting that, then perhaps we should just document it that way and
> move on.  In that case, all these changes would be for 9.1.  If we
> decide the current behavior is a bug, it might be something which could
> be fixed in 9.0 and maybe back-patched.

Ahh, because I found out an independent problem during the discussion,
it made us confused. Please make clear this patch does not intend to
fix the bug.

If we decide it is an actual bug to be fixed/informed, I also agree
it should be worked in a separated patch.

Well, rest of discussion should be haven in different thread.

> In *either* case, given that one is a 'clean-up' patch and the other is
> 'new functionality', they should be independent *anyway*.  Small
> incremental changes that don't break things when applied is what we're
> shooting for here.

Agreed.

>>> #3: You didn't move ExecCheckRTPerms() and ExecCheckRTEPerms() to
>>> utils/acl and instead added executor/executor.h to rt_triggers.c.
>>> I don't particularly like that.  I admit that DoCopy() already knew
>>> about the executor, and if that were the only case outside of the
>>> executor where ExecCheckRTPerms() was getting called it'd probably be
>>> alright, but we already have another place that wants to use it, so
>>> let's move it to a more appropriate place.
>>
>> Sorry, I'm a bit confused.
>> It seemed to me you suggested to utilize ExecCheckRTPerms() rather than
>> moving its logic anywhere, so I kept it here. (Was it misunderstand?)
> 
> I'm talking about moving the whole function (all 3 lines of it) to
> somewhere else and then reworking the function to be more appropriate
> based on it's new location (including renaming and changing arguments
> and return values, as appropriate).

OK, I agreed.

>> If so, but, I doubt utils/acl is the best placeholder of the moved
>> ExecCheckRTPerms(), because the checker function calls both of the
>> default acl functions and a optional external security function.
> 
> Can you explain why you think that having a function in utils/acl (eg:
> include/utils/acl.h and backend/utils/aclchk.c) which calls default acl
> functions and an allows for an external hook would be a bad idea?
>
>> It means the ExecCheckRTPerms() is caller of acl functions, not acl
>> function itself, isn't it?
>
> It's providing a higher-level service, sure, but there's nothing
> particularly interesting or special about what it's doing in this case,
> and, we need it in multiple places.  Why duplicate it?

If number of the checker functions is only a reason why we move
ExecCheckRTPerms() into the backend/utils/aclchk.c right now, I
don't have any opposition.
When it reaches to a dozen, we can consider new location. Right?

Sorry, the name of pg_rangetbl_aclcheck() was misleading for me.

>> I agreed the checker function is not a part of executor, but it is
>> also not a part of acl functions in my opinion.
>>
>> If it is disinclined to create a new directory to deploy the checker
>> function, my preference is src/backend/utils/adt/security.c and
>> src/include/utils/security.h .
> 
> We don't need a new directory or file for one function, as Robert
> already pointed out.

OK, let's consider when aclchk.c holds a dozen of checker functions.

>>> #6: I havn't checked yet, but if there are other things in an RTE which
>>> would make sense in the DoCopy case, beyond just what's needed for the
>>> permissions checking, and which wouldn't be 'correct' with a NULL'd
>>> value, I would set those.  Yes, we're building the RTE to check
>>> permissions, but we don't want someone downstream to be suprised when
>>> they make a change to something in the permissions checking and discover
>>> that a value in RTE they expected to be there wasn't valid.  Even more
>>> so, if there are function helpers which can be used to build an RTE, we
>>> should be using them.  The same goes for RI_Initial_Check().
>>
>> Are you saying something like makeFuncExpr()?
>> I basically agree. However, should it be done in this patch?
> 
> Actually, I mean looking for, and using, things like
> markRTEForSelectPriv() and addRangeTableEntry() or
> addRangeTableEntryForRelation().

OK, I'll make it in separated patch.

>>> #8: When moving ExecCheckRTPerms(), you should rename it to be more like
>>> the other function calls in acl.h  Perhaps pg_rangetbl_aclcheck()?
>>> Also, it should return an actual AclResult instead of just true/false.
>>
>> See the comments in #3.
>> And, if the caller has to handle aclcheck_error(), user cannot distinguish
>> access violation errors between the default PG permission and any other
>> external security stuff, isn't it?
> 
> I'm not suggesting that the caller handle aclcheck_error()..
> ExecCheckRTPerms() could just as easily have a flag which indicates if
> it will call aclcheck_error() or not, and if not, to return an
> AclResult to the caller.  That flag could then be passed to
> ExecCheckRTEPerms() as you have it now.

Sorry, the name of pg_rangetbl_aclcheck() is also misleading for me.
It makes me an impression that it always returns AclResult and caller handles
it appropriately, like pg_class_aclcheck().

What you explained seems to me same as what I plan now.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: ExecutorCheckPerms() hook

Tom Lane <tgl@sss.pgh.pa.us> — 2010-05-26T03:17:13Z

Stephen Frost <sfrost@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 primary table?  If so, it's
> still a bug, but at that point it's a bug in our documentation where we
> don't mention that SELECT rights are also needed.

Having an FK to another table certainly allows at least an indirect
form of SELECT, because you can determine whether any given key
exists in the PK table by seeing if you're allowed to insert a
referencing row.  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.

In any case, RI_Initial_Check isn't broken, because if it can't do
the SELECTs it just falls back to a slower method.  It's arguable
that the FK triggers themselves are assuming more than they should
about permissions, but I don't think that RI_Initial_Check can be
claimed to be buggy.

			regards, tom lane

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-05-26T03:39:07Z

(2010/05/26 12:17), Tom Lane wrote:
> Stephen Frost<sfrost@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 primary table?  If so, it's
>> still a bug, but at that point it's a bug in our documentation where we
>> don't mention that SELECT rights are also needed.
> 
> Having an FK to another table certainly allows at least an indirect
> form of SELECT, because you can determine whether any given key
> exists in the PK table by seeing if you're allowed to insert a
> referencing row.  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.
> 
> In any case, RI_Initial_Check isn't broken, because if it can't do
> the SELECTs it just falls back to a slower method.  It's arguable
> that the FK triggers themselves are assuming more than they should
> about permissions, but I don't think that RI_Initial_Check can be
> claimed to be buggy.

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 is unclear for me why we need to provide a slower fallback.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-05-26T06:50:56Z

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 argument (bool abort) that is a hint of behavior
  on access violations.
- It also returns AclResult, instead of bool.
- I assumed RI_Initial_Check() is not broken, right now.
  So, this patch just reworks DML permission checks without any bugfixes.
- The ESP hook were moved to ExecCheckRTPerms() from ExecCheckRTEPerms().
- At DoCopy() and RI_Initial_Check() call the checker function with
  list_make1(&rte), instead of &rte.
- In DoCopy(), required_access is used to store either ACL_SELECT or
  ACL_INSERT; initialized at head of the function.
- In DoCopy(), it initialize selectedCols or modifiedCol of RTE depending
  on if (is_from), instead of columnsSet.

ToDo:
- makeRangeTblEntry() stuff to allocate a RTE node with given parameter
  is not yet.

Thanks,

(2010/05/26 12:04), KaiGai Kohei wrote:
> (2010/05/26 11:12), Stephen Frost wrote:
>> * KaiGai Kohei (kaigai@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 it?
>>
>> That really depends on if this is actually fixing a bug in the existing
>> code or not.  I'm on the fence about that at the moment, to be honest.
>> I was trying to find if we expliitly say that SELECT rights are needed
>> to reference a column but wasn't able to.  If every code path is
>> expecting that, then perhaps we should just document it that way and
>> move on.  In that case, all these changes would be for 9.1.  If we
>> decide the current behavior is a bug, it might be something which could
>> be fixed in 9.0 and maybe back-patched.
> 
> Ahh, because I found out an independent problem during the discussion,
> it made us confused. Please make clear this patch does not intend to
> fix the bug.
> 
> If we decide it is an actual bug to be fixed/informed, I also agree
> it should be worked in a separated patch.
> 
> Well, rest of discussion should be haven in different thread.
> 
>> In *either* case, given that one is a 'clean-up' patch and the other is
>> 'new functionality', they should be independent *anyway*.  Small
>> incremental changes that don't break things when applied is what we're
>> shooting for here.
> 
> Agreed.
> 
>>>> #3: You didn't move ExecCheckRTPerms() and ExecCheckRTEPerms() to
>>>> utils/acl and instead added executor/executor.h to rt_triggers.c.
>>>> I don't particularly like that.  I admit that DoCopy() already knew
>>>> about the executor, and if that were the only case outside of the
>>>> executor where ExecCheckRTPerms() was getting called it'd probably be
>>>> alright, but we already have another place that wants to use it, so
>>>> let's move it to a more appropriate place.
>>>
>>> Sorry, I'm a bit confused.
>>> It seemed to me you suggested to utilize ExecCheckRTPerms() rather than
>>> moving its logic anywhere, so I kept it here. (Was it misunderstand?)
>>
>> I'm talking about moving the whole function (all 3 lines of it) to
>> somewhere else and then reworking the function to be more appropriate
>> based on it's new location (including renaming and changing arguments
>> and return values, as appropriate).
> 
> OK, I agreed.
> 
>>> If so, but, I doubt utils/acl is the best placeholder of the moved
>>> ExecCheckRTPerms(), because the checker function calls both of the
>>> default acl functions and a optional external security function.
>>
>> Can you explain why you think that having a function in utils/acl (eg:
>> include/utils/acl.h and backend/utils/aclchk.c) which calls default acl
>> functions and an allows for an external hook would be a bad idea?
>>
>>> It means the ExecCheckRTPerms() is caller of acl functions, not acl
>>> function itself, isn't it?
>>
>> It's providing a higher-level service, sure, but there's nothing
>> particularly interesting or special about what it's doing in this case,
>> and, we need it in multiple places.  Why duplicate it?
> 
> If number of the checker functions is only a reason why we move
> ExecCheckRTPerms() into the backend/utils/aclchk.c right now, I
> don't have any opposition.
> When it reaches to a dozen, we can consider new location. Right?
> 
> Sorry, the name of pg_rangetbl_aclcheck() was misleading for me.
> 
>>> I agreed the checker function is not a part of executor, but it is
>>> also not a part of acl functions in my opinion.
>>>
>>> If it is disinclined to create a new directory to deploy the checker
>>> function, my preference is src/backend/utils/adt/security.c and
>>> src/include/utils/security.h .
>>
>> We don't need a new directory or file for one function, as Robert
>> already pointed out.
> 
> OK, let's consider when aclchk.c holds a dozen of checker functions.
> 
>>>> #6: I havn't checked yet, but if there are other things in an RTE which
>>>> would make sense in the DoCopy case, beyond just what's needed for the
>>>> permissions checking, and which wouldn't be 'correct' with a NULL'd
>>>> value, I would set those.  Yes, we're building the RTE to check
>>>> permissions, but we don't want someone downstream to be suprised when
>>>> they make a change to something in the permissions checking and discover
>>>> that a value in RTE they expected to be there wasn't valid.  Even more
>>>> so, if there are function helpers which can be used to build an RTE, we
>>>> should be using them.  The same goes for RI_Initial_Check().
>>>
>>> Are you saying something like makeFuncExpr()?
>>> I basically agree. However, should it be done in this patch?
>>
>> Actually, I mean looking for, and using, things like
>> markRTEForSelectPriv() and addRangeTableEntry() or
>> addRangeTableEntryForRelation().
> 
> OK, I'll make it in separated patch.
> 
>>>> #8: When moving ExecCheckRTPerms(), you should rename it to be more like
>>>> the other function calls in acl.h  Perhaps pg_rangetbl_aclcheck()?
>>>> Also, it should return an actual AclResult instead of just true/false.
>>>
>>> See the comments in #3.
>>> And, if the caller has to handle aclcheck_error(), user cannot distinguish
>>> access violation errors between the default PG permission and any other
>>> external security stuff, isn't it?
>>
>> I'm not suggesting that the caller handle aclcheck_error()..
>> ExecCheckRTPerms() could just as easily have a flag which indicates if
>> it will call aclcheck_error() or not, and if not, to return an
>> AclResult to the caller.  That flag could then be passed to
>> ExecCheckRTEPerms() as you have it now.
> 
> Sorry, the name of pg_rangetbl_aclcheck() is also misleading for me.
> It makes me an impression that it always returns AclResult and caller handles
> it appropriately, like pg_class_aclcheck().
> 
> What you explained seems to me same as what I plan now.
> 
> Thanks,


-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-05-26T10:54:18Z

Tom,

* Tom Lane (tgl@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 then I
think the question is: when is the path in RI_Initial_Check not able
to be used (hence, why do we need a fall-back)?  My guess would be:

role X has:

Primary table: SELECT, REFERENCES
Foreign table: REFERENCES

This doesn't make much sense either though, because X has to own the
foreign table.

postgres=> alter table fk_tbl add foreign key (x) references pk_tbl;
ERROR:  must be owner of relation fk_tbl

So, the only situation, it seems, where the fall-back method has to be
used is when X owns the table but doesn't have SELECT rights on it.
Maybe it's just me, but that seems pretty silly.

If we require:

Primary table: SELECT, REFERENCES
Foreign table: OWNER, SELECT, REFERENCES

Then it seems like we should be able to eliminate the fall-back method
and just use the RI_Initial_Check approach.  What am I missing here? :/

> In any case, RI_Initial_Check isn't broken, because if it can't do
> the SELECTs it just falls back to a slower method.  It's arguable
> that the FK triggers themselves are assuming more than they should
> about permissions, but I don't think that RI_Initial_Check can be
> claimed to be buggy.

RI_Initial_Check is at least missing an optimization to support
column-level priviledges.  If we say that REFERENCES alone is allowed to
create a FK, then the fall-back method is broken because it depends on
SELECT rights on the primary.

To be honest, I'm guessing that the reason there's so much confusion
around this is that the spec probably says you don't need SELECT rights
(I havn't checked though), and at some point in the distant past we
handled that correctly with the fall-back method and that has since been
broken by other changes (possibly to plug the hole the fall-back method
was using).

I'll try to go deipher the spec so we can at least have something more
interesting to discuss (if we agree with doing it how the spec says or
not :).

	Thanks,

		Stephen

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-05-26T13:42:02Z

KaiGai,

* KaiGai Kohei (kaigai@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.  So, wrt the standard, you do NOT need SELECT rights on
a table to create an FK against it.  You only need references.  PG
handles this correctly.

This error:

> >>    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"

Is due to the *owner* of pk_tbl not having SELECT rights- a situation
we're not generally expecting to see.  What's happening is that prior to
the above being run, we've switched user over to the owner of the table
to perform this check.

This script illustrates what I'm talking about:

CREATE USER pk_owner;
CREATE USER fk_owner;
GRANT pk_owner TO sfrost;
GRANT fk_owner TO sfrost;

SET ROLE pk_owner;
CREATE TABLE pk_tbl (a int primary key, b text);
INSERT INTO pk_tbl VALUES (1,'aaa'), (2,'bbb'), (3,'ccc');
GRANT REFERENCES ON pk_tbl TO fk_owner;

SET ROLE fk_owner;
CREATE TABLE fk_tbl (x int, y text);
INSERT INTO fk_tbl VALUES (1,'xxx'), (2,'yyy'), (3,'zzz');

ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a);

ALTER TABLE fk_tbl DROP CONSTRAINT fk_tbl_x_fkey;

SET ROLE pk_owner;
REVOKE ALL ON pk_tbl FROM pk_owner;

SET ROLE fk_owner;
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"

This does make me think (as I've thought in the past..) that we really
should say *who* doesn't have that permission.  We run into the same
problem with views- they're run as the owner of the view, so you can get
a permission denied error trying to select from the view when you
clearly have select rights on the view itself.

As it turns out, the check in RI_Initial_Check() to provide the speed-up
is if the current role can just SELECT against the PK table- in which
case, you can run the check as the FK user and not have to change user.
We can't just switch to the PK user and run the same query though,
because that user might not have any rights on the FK table.  So, we end
up taking the slow path, which fires off the FK trigger that's been set
up on the fk_tbl but which runs as the owner of the pk_tbl.

So, long-and-short, I don't see that we have a bug in any of this.  I do
think we should allow RI_Initial_Check() to run if it has the necessary
column-level permissions, and we should remove the duplicate
permissions-checking code in favor of using the same code the executor
will, which happens to also be where the new hook we're talking about
is.

	Thanks,

		Stephen

Re: ExecutorCheckPerms() hook

Tom Lane <tgl@sss.pgh.pa.us> — 2010-05-26T13:50:02Z

KaiGai Kohei <kaigai@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 is unclear for me why we need to provide a slower fallback.

Because the queries inside the triggers are done with a different
current userid.

			regards, tom lane

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-05-26T15:09:10Z

KaiGai,

* KaiGai Kohei (kaigai@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 understand why it can't
be if that turns out to be the case.

A couple of other comments, all pretty minor things:

- I'd still rather see the hook itself in another patch, but given that
  we've determined that none of this is going to go into 9.0, it's not
  as big a deal.

- The hook definition in aclchk.c should really be at the top of that
  file.  We've been pretty consistant about putting hooks at the top of
  files instead of deep down in the file, this should also follow that
  scheme.

- Some of the comments at the top of chkpriv_rte_perms probably make
  sense to move up to where it's called from execMain.c.  Specifically,
  the comments about the other RTE types (function, join, subquery).
  I'd probably change the comment in chkpriv_rte_perms to be simpler-
  "This is only used for checking plain relation permissions, nothing
  else is checked here", and also have that same comment around
  chkpriv_relation_perms, both in aclchk.c and in acl.h.

- I'd move chkpriv_relation_perms above chkpriv_rte_perms, it's what we
  expect people to use, after all.

- Don't particularly like the function names.  How about
  relation_privilege_check?  Or rangetbl_privilege_check?  We don't use
  'perms' much (uh, at all?) in function names, and even if we did, it'd
  be redundant and not really help someone understand what the function
  is doing.

- I don't really like having 'abort' as the variable name for the 2nd
  argument.  I'm not finding an obvious convention right now, but maybe
  something like "error_on_failure" instead?

- In DoCopy, some comments about what you're doing there to set up for
  calling chkpriv_relation_perms would be good (like the comment you
  removed- /* We don't have table permissions, check per-column
  permissions */, updated to for something like "build an RTE with the
  columns referenced marked to check for necessary privileges").  
  Additionally, it might be worth considering if having an RTE built
  farther up in DoCopy would make sense and would then be usable for
  other bits in DoCopy.

- In RI_Initial_Check, why not build up an actual list of RTEs and just
  call chkpriv_relation_perms once?  Also, you should add comments
  there, again, about what you're doing and why.  If you can use another
  function to build the actual RTE, this will probably fall out more
  sensibly too.

- Have you checked if there are any bad side-effects from calling
  ri_FetchConstraintInfo before doing the permissions checking?

- The hook in acl.h should be separated out and brought to the top and
  documented independently as to exactly where the hook is and what it
  can be used for, along with what the arguments mean, etc.  Similairly,
  chkpriv_relation_perms should really have a short comment for it about
  what it's for.  Something more than 'security checker function'.

All pretty minor things that I'd probably just fix myself if I was going
to be committing it (not that I have that option ;).

	Thanks,

		Stephen

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-05-26T15:10:34Z

* Tom Lane (tgl@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

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-05-27T01:45:10Z

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 (kaigai@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 understand why it can't
> be if that turns out to be the case.

The patch-A tries to implement makeRangeTblEntry() which takes only rtekind
as argument right now.
Other fields are initialized to zero, using makeNode().

> A couple of other comments, all pretty minor things:
> 
> - I'd still rather see the hook itself in another patch, but given that
>    we've determined that none of this is going to go into 9.0, it's not
>    as big a deal.

OK, I divided the ESP hook part into the patch-C.

> - The hook definition in aclchk.c should really be at the top of that
>    file.  We've been pretty consistant about putting hooks at the top of
>    files instead of deep down in the file, this should also follow that
>    scheme.

OK, I moved it.

> - Some of the comments at the top of chkpriv_rte_perms probably make
>    sense to move up to where it's called from execMain.c.  Specifically,
>    the comments about the other RTE types (function, join, subquery).
>    I'd probably change the comment in chkpriv_rte_perms to be simpler-
>    "This is only used for checking plain relation permissions, nothing
>    else is checked here", and also have that same comment around
>    chkpriv_relation_perms, both in aclchk.c and in acl.h.

OK, I edited the comment as follows:

|   /*
|    * Do permissions checks. The check_relation_privileges() checks access
|    * permissions for all relations listed in a range table, but does not
|    * check anything for other RTE types (function, join, subquery, ...).
|    * Function RTEs are checked by init_fcache when the function is prepared
|    * for execution. Join, subquery, and special RTEs need no checks.
|    */

> - I'd move chkpriv_relation_perms above chkpriv_rte_perms, it's what we
>    expect people to use, after all.

OK, I reordered it.

> - Don't particularly like the function names.  How about
>    relation_privilege_check?  Or rangetbl_privilege_check?  We don't use
>    'perms' much (uh, at all?) in function names, and even if we did, it'd
>    be redundant and not really help someone understand what the function
>    is doing.

IIRC, Robert suggested that a verb should lead the function name.
So, I renamed it into check_relation_privileges() and check_rte_privileges().

> - I don't really like having 'abort' as the variable name for the 2nd
>    argument.  I'm not finding an obvious convention right now, but maybe
>    something like "error_on_failure" instead?

The 'failure' may make an impression of generic errors not only permission denied.
How about 'error_on_violation'?

> - In DoCopy, some comments about what you're doing there to set up for
>    calling chkpriv_relation_perms would be good (like the comment you
>    removed- /* We don't have table permissions, check per-column
>    permissions */, updated to for something like "build an RTE with the
>    columns referenced marked to check for necessary privileges").
>    Additionally, it might be worth considering if having an RTE built
>    farther up in DoCopy would make sense and would then be usable for
>    other bits in DoCopy.

I edited the comments as follows:

| /*
|  * Check relation permissions.
|  * We built an RTE with the relation and columns to be accessed
|  * to check for necessary privileges in the common way.
|  */

> - In RI_Initial_Check, why not build up an actual list of RTEs and just
>    call chkpriv_relation_perms once?  Also, you should add comments
>    there, again, about what you're doing and why.  If you can use another
>    function to build the actual RTE, this will probably fall out more
>    sensibly too.

Good catch! I fixed the invocation of checker function with list_make2().

And, I edited the comments as follows:

| /*
|  * We built a pair of RTEs of FK/PK relations and columns referenced
|  * in the test query to check necessary permission in the common way.
|  */

> - Have you checked if there are any bad side-effects from calling
>    ri_FetchConstraintInfo before doing the permissions checking?

The ri_FetchConstraintInfo() only references SysCaches to set up given
local variable without any other locks except for ones acquired by syscache.c.

> - The hook in acl.h should be separated out and brought to the top and
>    documented independently as to exactly where the hook is and what it
>    can be used for, along with what the arguments mean, etc.  Similairly,
>    chkpriv_relation_perms should really have a short comment for it about
>    what it's for.  Something more than 'security checker function'.

OK, at the patch-C, I moved the definition of the hook into the first half
of acl.h, but it needs to be declared after the AclResult definition.

BTW, I wonder whether acl.h is a correct place to explain about the hook,
although I added comments for the hook.
I think we should add a series of explanation about ESP hooks in the internal
section of the documentation, when the number of hooks reaches a dozen for
example.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-05-27T02:15:12Z

KaiGai,

* KaiGai Kohei (kaigai@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 anything more appropriate already.
Still, I don't really have a problem with your makeRangeTblEntry() and
you certainly found quite a few places to use it.

>  B: major reworks of DML permission checks

No serious issues with this that I saw either.  I definitely think it's
cleaner using makeRangeTblEntry() and check_relation_privileges().

>  C: add an ESP hook on the DML permission checks

This also looks good to me, though I don't know that you really need the
additional comment in execMain.c about the hook.  I would make sure that
you have a comment around check_rte_privileges() which says not to call
it directly because you'll bypass the hook (and potentially cause a
security leak by doing so).  Don't recall seeing that, apologies if it
was there.

> IIRC, Robert suggested that a verb should lead the function name.
> So, I renamed it into check_relation_privileges() and check_rte_privileges().

Yeah, that's alright.  I'm on the fence about using 'relation' or using
'rangetbl' there, but certainly whomever commits this could trivially
change it to whatever they prefer.

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

> > - Have you checked if there are any bad side-effects from calling
> >    ri_FetchConstraintInfo before doing the permissions checking?
> 
> The ri_FetchConstraintInfo() only references SysCaches to set up given
> local variable without any other locks except for ones acquired by syscache.c.

Ok.

> > - The hook in acl.h should be separated out and brought to the top and
> >    documented independently as to exactly where the hook is and what it
> >    can be used for, along with what the arguments mean, etc.  Similairly,
> >    chkpriv_relation_perms should really have a short comment for it about
> >    what it's for.  Something more than 'security checker function'.
> 
> OK, at the patch-C, I moved the definition of the hook into the first half
> of acl.h, but it needs to be declared after the AclResult definition.

Fair enough.

> BTW, I wonder whether acl.h is a correct place to explain about the hook,
> although I added comments for the hook.

Guess I don't really see a problem putting the comments there.  By the
way, have we got a place where we actually document the hooks we support
somewhere in the official documentation..?  If so, that should certainly
be updated too..

> I think we should add a series of explanation about ESP hooks in the internal
> section of the documentation, when the number of hooks reaches a dozen for
> example.

I believe the goal will be to avoid reaching a dozen hooks for this.

All-in-all, I'm pretty happy with these.  Couple minor places which
could use some copy editing, but that's about it.

Next, we need to get the security label catalog and the grammar to
support it implemented and then from that an SELinux module should
be pretty easy to implement.  Based on the discussions at PGCon, Robert
is working on the security label catalog and grammar.  The current plan
is to have a catalog similar to pg_depend, to minimize impact to the
rest of the backend and to those who aren't interested in using security
labels.  Of course, there will also need to be hooks there for an
external module to enforce restrictions associated with changing labels
on various objects in the system.

	Thanks,

		Stephen

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-05-27T03:00:56Z

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.

I have no preference between 'error_on_violation' and 'ereport_on_violation'.
OK, I fixed it.

>> BTW, I wonder whether acl.h is a correct place to explain about the hook,
>> although I added comments for the hook.
> 
> Guess I don't really see a problem putting the comments there.  By the
> way, have we got a place where we actually document the hooks we support
> somewhere in the official documentation..?  If so, that should certainly
> be updated too..

I could not find Executor hooks from doc/src/sgml using grep.
If so, it might be worth to list them on the wikipage.

>> I think we should add a series of explanation about ESP hooks in the internal
>> section of the documentation, when the number of hooks reaches a dozen for
>> example.
> 
> I believe the goal will be to avoid reaching a dozen hooks for this.

Maybe, all we need to hook on DML permissions is only this one.

> All-in-all, I'm pretty happy with these.  Couple minor places which
> could use some copy editing, but that's about it.
> 
> Next, we need to get the security label catalog and the grammar to
> support it implemented and then from that an SELinux module should
> be pretty easy to implement.  Based on the discussions at PGCon, Robert
> is working on the security label catalog and grammar.  The current plan
> is to have a catalog similar to pg_depend, to minimize impact to the
> rest of the backend and to those who aren't interested in using security
> labels.

Pg_depend? not pg_description/pg_shdescription?

I basically agree with the idea that minimizes damages to the existing schema
of system catalogs, but I cannot imagine something like pg_depend well.

I'd like to post a new thread to discuss the security label support. OK?

> Of course, there will also need to be hooks there for an
> external module to enforce restrictions associated with changing labels
> on various objects in the system.

Yes, the user given has to be validated by ESP.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-06-14T08:27:11Z

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 attached patch shall be applied on the head of the git repository.
It just adds a security hook on ExecCheckRTPerms() as Robert suggested
at first.
Of course, it does not allow to acquire the control on COPY TO/FROM and
RI_Initial_Check(). It will be refactored in the following patches.

Thanks,

(2010/05/27 12:00), KaiGai Kohei wrote:
> 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.
> 
> I have no preference between 'error_on_violation' and 'ereport_on_violation'.
> OK, I fixed it.
> 
>>> BTW, I wonder whether acl.h is a correct place to explain about the hook,
>>> although I added comments for the hook.
>>
>> Guess I don't really see a problem putting the comments there.  By the
>> way, have we got a place where we actually document the hooks we support
>> somewhere in the official documentation..?  If so, that should certainly
>> be updated too..
> 
> I could not find Executor hooks from doc/src/sgml using grep.
> If so, it might be worth to list them on the wikipage.
> 
>>> I think we should add a series of explanation about ESP hooks in the internal
>>> section of the documentation, when the number of hooks reaches a dozen for
>>> example.
>>
>> I believe the goal will be to avoid reaching a dozen hooks for this.
> 
> Maybe, all we need to hook on DML permissions is only this one.
> 
>> All-in-all, I'm pretty happy with these.  Couple minor places which
>> could use some copy editing, but that's about it.
>>
>> Next, we need to get the security label catalog and the grammar to
>> support it implemented and then from that an SELinux module should
>> be pretty easy to implement.  Based on the discussions at PGCon, Robert
>> is working on the security label catalog and grammar.  The current plan
>> is to have a catalog similar to pg_depend, to minimize impact to the
>> rest of the backend and to those who aren't interested in using security
>> labels.
> 
> Pg_depend? not pg_description/pg_shdescription?
> 
> I basically agree with the idea that minimizes damages to the existing schema
> of system catalogs, but I cannot imagine something like pg_depend well.
> 
> I'd like to post a new thread to discuss the security label support. OK?
> 
>> Of course, there will also need to be hooks there for an
>> external module to enforce restrictions associated with changing labels
>> on various objects in the system.
> 
> Yes, the user given has to be validated by ESP.
> 
> Thanks,

-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

[v9.1] add makeRangeTblEntry() into makefuncs.c

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-06-14T08:34:21Z

The attached patch was a part of DML refactoring and security hook patches.

It adds makeRangeTblEntry() into makefuncs.c to keep the code more
clean. It shall be also used for the upcoming DML refactor patch.
In this refactoring, a common DML permission checker function take
a list of RangeTblEntry, so the caller has to set up the object.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Reworks of DML permission checks

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-06-14T08:46:12Z

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.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

[v9.1] Add security hook on initialization of instance

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-06-14T09:03:25Z

The attached patch tries to add one more security hook on the
initialization of PostgreSQL instance (InitPostgres()).

It gives the external security module a chance to initialize itself,
and acquire credential of the client.

I assumed the best place to initialize the module is just after the
initialize_acl() invocation, if ESP is available.
We have not discussed about this hook yet. So, I'd like to see any
comments.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: [v9.1] Add security hook on initialization of instance

Stephen Frost <sfrost@snowman.net> — 2010-06-14T11:01:29Z

* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
> The attached patch tries to add one more security hook on the
> initialization of PostgreSQL instance (InitPostgres()).
> 
> It gives the external security module a chance to initialize itself,
> and acquire credential of the client.
> 
> I assumed the best place to initialize the module is just after the
> initialize_acl() invocation, if ESP is available.
> We have not discussed about this hook yet. So, I'd like to see any
> comments.

Aren't modules given a __PG_Init or something similar that they can
define which will be called when the module is loaded..?

	Stephen

Re: [v9.1] Add security hook on initialization of instance

Kohei KaiGai <kaigai@kaigai.gr.jp> — 2010-06-14T11:12:40Z

(2010/06/14 20:01), Stephen Frost wrote:
> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>> The attached patch tries to add one more security hook on the
>> initialization of PostgreSQL instance (InitPostgres()).
>>
>> It gives the external security module a chance to initialize itself,
>> and acquire credential of the client.
>>
>> I assumed the best place to initialize the module is just after the
>> initialize_acl() invocation, if ESP is available.
>> We have not discussed about this hook yet. So, I'd like to see any
>> comments.
> 
> Aren't modules given a __PG_Init or something similar that they can
> define which will be called when the module is loaded..?
> 
I assume the security module shall be loaded within 'shared_preload_libraries',
because we can overwrite 'local_preload_libraries' (PGC_BACKEND) setting using
connection string, so it allows users to bypass security features, doesn't it?

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>

Re: ExecutorCheckPerms() hook

Robert Haas <robertmhaas@gmail.com> — 2010-06-14T11:54:17Z

2010/6/14 KaiGai Kohei <kaigai@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 reconsider
> the dependency of these patches.
>
> The attached patch shall be applied on the head of the git repository.
> It just adds a security hook on ExecCheckRTPerms() as Robert suggested
> at first.
> Of course, it does not allow to acquire the control on COPY TO/FROM and
> RI_Initial_Check(). It will be refactored in the following patches.

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?

With all due respect, I intend to imply my own version.  Please make
your other proposed patches apply on top of that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: [v9.1] add makeRangeTblEntry() into makefuncs.c

Robert Haas <robertmhaas@gmail.com> — 2010-06-14T12:06:04Z

2010/6/14 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> The attached patch was a part of DML refactoring and security hook patches.
>
> It adds makeRangeTblEntry() into makefuncs.c to keep the code more
> clean. It shall be also used for the upcoming DML refactor patch.
> In this refactoring, a common DML permission checker function take
> a list of RangeTblEntry, so the caller has to set up the object.

I think this is the epitome of pointless.  It looks to me like this
just makes the code harder to read and very slightly slower without
actually accomplishing any useful abstraction.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: [v9.1] Add security hook on initialization of instance

Robert Haas <robertmhaas@gmail.com> — 2010-06-14T12:15:12Z

2010/6/14 KaiGai Kohei <kaigai@kaigai.gr.jp>:
> (2010/06/14 20:01), Stephen Frost wrote:
>> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>>> The attached patch tries to add one more security hook on the
>>> initialization of PostgreSQL instance (InitPostgres()).
>>>
>>> It gives the external security module a chance to initialize itself,
>>> and acquire credential of the client.
>>>
>>> I assumed the best place to initialize the module is just after the
>>> initialize_acl() invocation, if ESP is available.
>>> We have not discussed about this hook yet. So, I'd like to see any
>>> comments.
>>
>> Aren't modules given a __PG_Init or something similar that they can
>> define which will be called when the module is loaded..?
>>
> I assume the security module shall be loaded within 'shared_preload_libraries',
> because we can overwrite 'local_preload_libraries' (PGC_BACKEND) setting using
> connection string, so it allows users to bypass security features, doesn't it?

Yeah, but so what?  Stephen's point is still valid.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-06-14T12:35:06Z

* Robert Haas (robertmhaas@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 patch that he
was working, which I then reviewed and asked him to update/change, and
I think part of that was me asking that he keep the hook patch split
out.  He then split it out of his patch rather than just going back to
yours.

> With all due respect, I intend to imply my own version.  Please make
> your other proposed patches apply on top of that.

This strikes me as a case of "gee, won't git help here?".  Perhaps we
can use this as an opportunity to show how git can help.  Then again,
it's not exactly a huge patch. :)

	Thanks,

		Stephen
		(who won't mention the impetus for the hook being put here in
		the first place.. ;)

Re: [v9.1] add makeRangeTblEntry() into makefuncs.c

Stephen Frost <sfrost@snowman.net> — 2010-06-14T12:46:14Z

* Robert Haas (robertmhaas@gmail.com) wrote:
> 2010/6/14 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> > It adds makeRangeTblEntry() into makefuncs.c to keep the code more
> > clean. It shall be also used for the upcoming DML refactor patch.
> > In this refactoring, a common DML permission checker function take
> > a list of RangeTblEntry, so the caller has to set up the object.
> 
> I think this is the epitome of pointless.  It looks to me like this
> just makes the code harder to read and very slightly slower without
> actually accomplishing any useful abstraction.

I had suggested to KaiGai that he check if there was an existing
function for creating an RTE rather than just malloc'ing it- he took
that to mean he should add one if he couldn't find one.  Wasn't my
intent, but by the same token I didn't see it as a terribly bad thing
either.  Perhaps it should be improved or maybe we should just rip it
out, but I rather prefer some kind of abstraction for that given it's
use in a number of places.  Of course, I may just be overly thinking it.

	Thanks,

		Stephen

Re: [v9.1] add makeRangeTblEntry() into makefuncs.c

Robert Haas <robertmhaas@gmail.com> — 2010-06-14T13:11:02Z

On Mon, Jun 14, 2010 at 8:46 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Robert Haas (robertmhaas@gmail.com) wrote:
>> 2010/6/14 KaiGai Kohei <kaigai@ak.jp.nec.com>:
>> > It adds makeRangeTblEntry() into makefuncs.c to keep the code more
>> > clean. It shall be also used for the upcoming DML refactor patch.
>> > In this refactoring, a common DML permission checker function take
>> > a list of RangeTblEntry, so the caller has to set up the object.
>>
>> I think this is the epitome of pointless.  It looks to me like this
>> just makes the code harder to read and very slightly slower without
>> actually accomplishing any useful abstraction.
>
> I had suggested to KaiGai that he check if there was an existing
> function for creating an RTE rather than just malloc'ing it- he took
> that to mean he should add one if he couldn't find one.  Wasn't my
> intent, but by the same token I didn't see it as a terribly bad thing
> either.  Perhaps it should be improved or maybe we should just rip it
> out, but I rather prefer some kind of abstraction for that given it's
> use in a number of places.  Of course, I may just be overly thinking it.

Well, there's not much point in having a function that initializes ONE
member of a 20+ member structure and leaves the initialization of all
the rest to the caller.  The structure effectively functions like a
disjoint union, so maybe there'd be some value in having a function
for "build a relation RTE", which would set the rtekind to
RTE_RELATION and take arguments for the other fields that pertain to
that case.  But the generic function proposed here doesn't really
provide any meaningful abstraction.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: [v9.1] Add security hook on initialization of instance

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-06-14T23:54:05Z

(2010/06/14 21:15), Robert Haas wrote:
> 2010/6/14 KaiGai Kohei<kaigai@kaigai.gr.jp>:
>> (2010/06/14 20:01), Stephen Frost wrote:
>>> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>>>> The attached patch tries to add one more security hook on the
>>>> initialization of PostgreSQL instance (InitPostgres()).
>>>>
>>>> It gives the external security module a chance to initialize itself,
>>>> and acquire credential of the client.
>>>>
>>>> I assumed the best place to initialize the module is just after the
>>>> initialize_acl() invocation, if ESP is available.
>>>> We have not discussed about this hook yet. So, I'd like to see any
>>>> comments.
>>>
>>> Aren't modules given a __PG_Init or something similar that they can
>>> define which will be called when the module is loaded..?
>>>
>> I assume the security module shall be loaded within 'shared_preload_libraries',
>> because we can overwrite 'local_preload_libraries' (PGC_BACKEND) setting using
>> connection string, so it allows users to bypass security features, doesn't it?
> 
> Yeah, but so what?  Stephen's point is still valid.
> 
On the hook, I'd like to obtain security context of the client process
which connected to the PostgreSQL instance. It is not available at the
_PG_init() phase, because clients don't connect yet.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: [v9.1] add makeRangeTblEntry() into makefuncs.c

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-06-14T23:54:53Z

(2010/06/14 22:11), Robert Haas wrote:
> On Mon, Jun 14, 2010 at 8:46 AM, Stephen Frost<sfrost@snowman.net>  wrote:
>> * Robert Haas (robertmhaas@gmail.com) wrote:
>>> 2010/6/14 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>>>> It adds makeRangeTblEntry() into makefuncs.c to keep the code more
>>>> clean. It shall be also used for the upcoming DML refactor patch.
>>>> In this refactoring, a common DML permission checker function take
>>>> a list of RangeTblEntry, so the caller has to set up the object.
>>>
>>> I think this is the epitome of pointless.  It looks to me like this
>>> just makes the code harder to read and very slightly slower without
>>> actually accomplishing any useful abstraction.
>>
>> I had suggested to KaiGai that he check if there was an existing
>> function for creating an RTE rather than just malloc'ing it- he took
>> that to mean he should add one if he couldn't find one.  Wasn't my
>> intent, but by the same token I didn't see it as a terribly bad thing
>> either.  Perhaps it should be improved or maybe we should just rip it
>> out, but I rather prefer some kind of abstraction for that given it's
>> use in a number of places.  Of course, I may just be overly thinking it.
> 
> Well, there's not much point in having a function that initializes ONE
> member of a 20+ member structure and leaves the initialization of all
> the rest to the caller.  The structure effectively functions like a
> disjoint union, so maybe there'd be some value in having a function
> for "build a relation RTE", which would set the rtekind to
> RTE_RELATION and take arguments for the other fields that pertain to
> that case.  But the generic function proposed here doesn't really
> provide any meaningful abstraction.
> 
Yes, it is fact that I hesitated about what fields should be initialized
at the makeRangeTblEntry(), because it has more than 20 members but
most of them are meaningful only when rtekind has a certain code.

OK, at least, priority of the patch is not higher than others for me.
I'll cancel the patch submission.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-06-15T00:09:51Z

(2010/06/14 21:35), Stephen Frost wrote:
> * Robert Haas (robertmhaas@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 patch that he
> was working, which I then reviewed and asked him to update/change, and
> I think part of that was me asking that he keep the hook patch split
> out.  He then split it out of his patch rather than just going back to
> yours.
> 
>> With all due respect, I intend to imply my own version.  Please make
>> your other proposed patches apply on top of that.
> 
> This strikes me as a case of "gee, won't git help here?".  Perhaps we
> can use this as an opportunity to show how git can help.  Then again,
> it's not exactly a huge patch. :)
> 
The patch provides the same functionality with what you wrote and posted
several weeks ago, but different from identifiers and comments.
During the discussion, I was suggested that 'ExecutorCheckPerms_hook' is
not an appropriate naming on the refactored DML permission check routine,
because it is not still a part of the executor.
So, I changed your original proposition.

When ExecCheckRTPerms() was refactored to a common DML permission checker
function, is the hook also renamed to more appropriately?
If so, I don't have any opposition to go back to the original one.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: [v9.1] Add security hook on initialization of instance

Robert Haas <robertmhaas@gmail.com> — 2010-06-15T00:22:12Z

2010/6/14 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> On the hook, I'd like to obtain security context of the client process
> which connected to the PostgreSQL instance. It is not available at the
> _PG_init() phase, because clients don't connect yet.

Can't you just call getpeercon() the first time you need the context
and cache it in a backend-local variable?  Then you don't need a hook
at all.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: [v9.1] Add security hook on initialization of instance

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-06-15T00:41:32Z

(2010/06/15 9:22), Robert Haas wrote:
> 2010/6/14 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>> On the hook, I'd like to obtain security context of the client process
>> which connected to the PostgreSQL instance. It is not available at the
>> _PG_init() phase, because clients don't connect yet.
> 
> Can't you just call getpeercon() the first time you need the context
> and cache it in a backend-local variable?  Then you don't need a hook
> at all.
> 
I've tried to implement my earlier version in this idea.
As long as getpeercon() performs correctly, it will work well.
But, if it returns an error due to the system configuration,
the security module cannot continue to make access control
decision anymore, although client can open the connection already.

I think this kind of initialization should be also done at
the initialization of backend, then it disconnect immediately
if something troubled.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: [v9.1] Add security hook on initialization of instance

Robert Haas <robertmhaas@gmail.com> — 2010-06-15T01:12:39Z

2010/6/14 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> (2010/06/15 9:22), Robert Haas wrote:
>> 2010/6/14 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>>> On the hook, I'd like to obtain security context of the client process
>>> which connected to the PostgreSQL instance. It is not available at the
>>> _PG_init() phase, because clients don't connect yet.
>>
>> Can't you just call getpeercon() the first time you need the context
>> and cache it in a backend-local variable?  Then you don't need a hook
>> at all.
>>
> I've tried to implement my earlier version in this idea.
> As long as getpeercon() performs correctly, it will work well.
> But, if it returns an error due to the system configuration,
> the security module cannot continue to make access control
> decision anymore, although client can open the connection already.
>
> I think this kind of initialization should be also done at
> the initialization of backend, then it disconnect immediately
> if something troubled.

I think if getpeercon() fails you can just throw ERROR or FATAL at
that point.  Until the user does something that requires a valid
security context, there's no reason to terminate the session if they
don't have one.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: [v9.1] Add security hook on initialization of instance

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-06-15T02:26:30Z

(2010/06/15 10:12), Robert Haas wrote:
> 2010/6/14 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>> (2010/06/15 9:22), Robert Haas wrote:
>>> 2010/6/14 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>>>> On the hook, I'd like to obtain security context of the client process
>>>> which connected to the PostgreSQL instance. It is not available at the
>>>> _PG_init() phase, because clients don't connect yet.
>>>
>>> Can't you just call getpeercon() the first time you need the context
>>> and cache it in a backend-local variable?  Then you don't need a hook
>>> at all.
>>>
>> I've tried to implement my earlier version in this idea.
>> As long as getpeercon() performs correctly, it will work well.
>> But, if it returns an error due to the system configuration,
>> the security module cannot continue to make access control
>> decision anymore, although client can open the connection already.
>>
>> I think this kind of initialization should be also done at
>> the initialization of backend, then it disconnect immediately
>> if something troubled.
> 
> I think if getpeercon() fails you can just throw ERROR or FATAL at
> that point.  Until the user does something that requires a valid
> security context, there's no reason to terminate the session if they
> don't have one.
> 
The 'initialization hook' might be misleading.
During authentication steps, the backend raises an error and close
the connection immediately, if the backend could not identify the
client in the configured way.
Because I think security context is a part of identifiers of the user,
not only database user id, I proposed a hook on the initialization
stage. (Perhaps, it might be placed on just after ClientAuthentication?)

I can agree that we can implement the module with this idea,
but delayed getpeercon() seemed to me strange.
# Of course, it is reasonable if we discuss it later.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: [v9.1] Add security hook on initialization of instance

Tom Lane <tgl@sss.pgh.pa.us> — 2010-06-15T03:28:11Z

KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
>>>> The attached patch tries to add one more security hook on the
>>>> initialization of PostgreSQL instance (InitPostgres()).

>> Yeah, but so what?  Stephen's point is still valid.

> On the hook, I'd like to obtain security context of the client process
> which connected to the PostgreSQL instance. It is not available at the
> _PG_init() phase, because clients don't connect yet.

InitPostgres is called by a number of process types that don't *have*
clients.  I concur with the other opinions that this hook is badly
thought out.

			regards, tom lane

Re: [v9.1] Add security hook on initialization of instance

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-06-15T03:47:48Z

(2010/06/15 12:28), Tom Lane wrote:
> KaiGai Kohei<kaigai@ak.jp.nec.com>  writes:
>>>>> The attached patch tries to add one more security hook on the
>>>>> initialization of PostgreSQL instance (InitPostgres()).
> 
>>> Yeah, but so what?  Stephen's point is still valid.
> 
>> On the hook, I'd like to obtain security context of the client process
>> which connected to the PostgreSQL instance. It is not available at the
>> _PG_init() phase, because clients don't connect yet.
> 
> InitPostgres is called by a number of process types that don't *have*
> clients.  I concur with the other opinions that this hook is badly
> thought out.
> 
I intended to skip it when InitPostgres() is called without clients.

For example, the hook might be better to put on PerformAuthentication()
for more clarification of the purpose.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: [v9.1] Add security hook on initialization of instance

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-06-15T07:08:18Z

(2010/06/15 12:47), KaiGai Kohei wrote:
> (2010/06/15 12:28), Tom Lane wrote:
>> KaiGai Kohei<kaigai@ak.jp.nec.com>   writes:
>>>>>> The attached patch tries to add one more security hook on the
>>>>>> initialization of PostgreSQL instance (InitPostgres()).
>>
>>>> Yeah, but so what?  Stephen's point is still valid.
>>
>>> On the hook, I'd like to obtain security context of the client process
>>> which connected to the PostgreSQL instance. It is not available at the
>>> _PG_init() phase, because clients don't connect yet.
>>
>> InitPostgres is called by a number of process types that don't *have*
>> clients.  I concur with the other opinions that this hook is badly
>> thought out.
>>
> I intended to skip it when InitPostgres() is called without clients.
> 
> For example, the hook might be better to put on PerformAuthentication()
> for more clarification of the purpose.
> 

In the attached patch, the security hook was moved to ClientAuthentication()
from InitPostgres(), for more clarification of the purpose.
What I want to do is to assign additional properties to identify the client
(such as security label) for each authenticated session.

Its purpose is similar to "session" module of PAM in operating system.
It allows to assign additional session properties more than user-id.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: [v9.1] Add security hook on initialization of instance

Stephen Frost <sfrost@snowman.net> — 2010-06-15T12:37:50Z

KaiGai,

* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
> In the attached patch, the security hook was moved to ClientAuthentication()
> from InitPostgres(), for more clarification of the purpose.
> What I want to do is to assign additional properties to identify the client
> (such as security label) for each authenticated session.
> 
> Its purpose is similar to "session" module of PAM in operating system.
> It allows to assign additional session properties more than user-id.

That's all fine- but let's work within the confines of the *existing*
hook that's been discussed to get something working first before we go
adding hooks in other places.  I think it's important that we put
together at least a proof of concept that an SELinux module or other
external auth module can sensible use the DML hook.

After that, we can discuss what other hooks are needed.  KaiGai, please,
before sending in patches of any kind, propose at a high-level what the
problem is and what the security module needs in general terms.  If you
have a recommendation, that's fine, but let's talk about it before
implementing anything.

	Thanks,

		Stephen

Re: [v9.1] Add security hook on initialization of instance

Robert Haas <robertmhaas@gmail.com> — 2010-06-15T14:24:03Z

On Tue, Jun 15, 2010 at 8:37 AM, Stephen Frost <sfrost@snowman.net> wrote:
> KaiGai,
>
> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>> In the attached patch, the security hook was moved to ClientAuthentication()
>> from InitPostgres(), for more clarification of the purpose.
>> What I want to do is to assign additional properties to identify the client
>> (such as security label) for each authenticated session.
>>
>> Its purpose is similar to "session" module of PAM in operating system.
>> It allows to assign additional session properties more than user-id.
>
> That's all fine- but let's work within the confines of the *existing*
> hook that's been discussed to get something working first before we go
> adding hooks in other places.  I think it's important that we put
> together at least a proof of concept that an SELinux module or other
> external auth module can sensible use the DML hook.

+1.

> After that, we can discuss what other hooks are needed.  KaiGai, please,
> before sending in patches of any kind, propose at a high-level what the
> problem is and what the security module needs in general terms.  If you
> have a recommendation, that's fine, but let's talk about it before
> implementing anything.

+2.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: [v9.1] Add security hook on initialization of instance

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-06-16T04:54:31Z

(2010/06/15 21:37), Stephen Frost wrote:
> KaiGai,
> 
> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>> In the attached patch, the security hook was moved to ClientAuthentication()
>> from InitPostgres(), for more clarification of the purpose.
>> What I want to do is to assign additional properties to identify the client
>> (such as security label) for each authenticated session.
>>
>> Its purpose is similar to "session" module of PAM in operating system.
>> It allows to assign additional session properties more than user-id.
> 
> That's all fine- but let's work within the confines of the *existing*
> hook that's been discussed to get something working first before we go
> adding hooks in other places.  I think it's important that we put
> together at least a proof of concept that an SELinux module or other
> external auth module can sensible use the DML hook.
> 
Yes, I'd like to introduce the reason and purpose of the hook.

At first, please assume any external security modules should be loaded
as shared preload libraries, because 'local_preload_libraries' setting
can be overwritten using connection string.
So, _PG_init() shall be invoked at the starting-up of the postmaster
daemon process at once, not per connection.

On the other hand, a security feature have to identify the client and
assign an appropriate set of privileges on the session prior to it being
available for users.
E,g. The default PG security assigns a certain database user-id on
the current session, then it will be used for access control decision.
In a similar way, an external security also wants a chance to identify,
authenticate and authorize the client. (SELinux uses getpeercon() to
obtain security label of the peer process, and applies it as privilege
of the current session.)

However, here is no hooks available for the purpose. In this case,
_PG_init() is not called for each connections, because the module is
a shared preload library, so we have to call getpeercon() in another
point.

One idea is, as Robert suggested, that we can invoke getpeercon() at
the first call of SELinux module and store it on the local variable.
It will work well as long as getpeercon() does not cause an error.

Robert pointed out we can always raise ERROR or FATAL using ereport(),
if it is troubled.
However, it seems to me a trouble of getpeercon() is fundamentally
an error within the step of client authentication.
In the case of database user-id, it immediately raises an error, and
close the connection. I think we should follow the manner of similar
features, so I proposed the new security hook at the authentication.

Thanks,

> After that, we can discuss what other hooks are needed.  KaiGai, please,
> before sending in patches of any kind, propose at a high-level what the
> problem is and what the security module needs in general terms.  If you
> have a recommendation, that's fine, but let's talk about it before
> implementing anything.
> 
> 	Thanks,
> 
> 		Stephen

-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: [v9.1] Add security hook on initialization of instance

Stephen Frost <sfrost@snowman.net> — 2010-06-16T12:37:09Z

KaiGai,

* KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
> On the other hand, a security feature have to identify the client and
> assign an appropriate set of privileges on the session prior to it being
> available for users.
[...]
> However, here is no hooks available for the purpose.

I believe we understand the issue now, my point was that in the future
let's have this discussion first.

> One idea is, as Robert suggested, that we can invoke getpeercon() at
> the first call of SELinux module and store it on the local variable.
> It will work well as long as getpeercon() does not cause an error.

Let's work with this approach to build a proof-of-concept that at least
the DML hook will work as advertised.  We've got alot of time till 9.1
and I think that if we can show that a module exists that implements
SELinux using the DML hook, and that a few other hooks are needed to
address short-comings in that module, adding them won't be a huge issue.

	Thanks,

		Stephen

Re: [v9.1] Add security hook on initialization of instance

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-06-16T23:33:06Z

(2010/06/16 21:37), Stephen Frost wrote:
> KaiGai,
> 
> * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote:
>> On the other hand, a security feature have to identify the client and
>> assign an appropriate set of privileges on the session prior to it being
>> available for users.
> [...]
>> However, here is no hooks available for the purpose.
> 
> I believe we understand the issue now, my point was that in the future
> let's have this discussion first.
> 
>> One idea is, as Robert suggested, that we can invoke getpeercon() at
>> the first call of SELinux module and store it on the local variable.
>> It will work well as long as getpeercon() does not cause an error.
> 
> Let's work with this approach to build a proof-of-concept that at least
> the DML hook will work as advertised.  We've got alot of time till 9.1
> and I think that if we can show that a module exists that implements
> SELinux using the DML hook, and that a few other hooks are needed to
> address short-comings in that module, adding them won't be a huge issue.
> 
OK, fair enough. Please wait for a few days.
I'll introduce the proof-of-concept module until this week.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: [v9.1] Add security hook on initialization of instance

Robert Haas <robertmhaas@gmail.com> — 2010-07-08T13:14:38Z

2010/6/16 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> OK, fair enough. Please wait for a few days.
> I'll introduce the proof-of-concept module until this week.

I think we have decided not to pursue this, at least for now.  If that
is the case, the CommitFest entry should be updated to Returned with
Feedback.

https://commitfest.postgresql.org/action/patch_view?id=323

FWIW, I am still of the opinion that we shouldn't have a hook here
anyway, because there is no reason to complain about lack of a
security context until the user performs an action which requires them
to have a security context.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: [v9.1] Add security hook on initialization of instance

Stephen Frost <sfrost@snowman.net> — 2010-07-08T13:37:05Z

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> 2010/6/16 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> > OK, fair enough. Please wait for a few days.
> > I'll introduce the proof-of-concept module until this week.
> 
> I think we have decided not to pursue this, at least for now.  If that
> is the case, the CommitFest entry should be updated to Returned with
> Feedback.

I think RwF is fine (since I think we're still waiting on another patch
anyway) for this commitfest.  I don't want to shut the door entirely on
this for 9.1, but a new/updated patch could be done in a later
commitfest.

> FWIW, I am still of the opinion that we shouldn't have a hook here
> anyway, because there is no reason to complain about lack of a
> security context until the user performs an action which requires them
> to have a security context.

I don't agree with this, in general.  It may be a difficult problem to
solve though.  From my perspective the above is similar to saying we
don't need a pg_hba.conf or that we should open a database before
checking the user's credentials.  I'd like to give a security module the
ability to be involved in the initial connection authorization, but we
run into an issue there if that module then needs access to the catalog.
Perhaps it doesn't, but it seems like it would, to use to make a
decision.

	Thanks,

		Stephen

Re: [v9.1] Add security hook on initialization of instance

Tom Lane <tgl@sss.pgh.pa.us> — 2010-07-08T14:40:35Z

Stephen Frost <sfrost@snowman.net> writes:
> * Robert Haas (robertmhaas@gmail.com) wrote:
>> FWIW, I am still of the opinion that we shouldn't have a hook here
>> anyway, because there is no reason to complain about lack of a
>> security context until the user performs an action which requires them
>> to have a security context.

> I don't agree with this, in general.  It may be a difficult problem to
> solve though.  From my perspective the above is similar to saying we
> don't need a pg_hba.conf or that we should open a database before
> checking the user's credentials.  I'd like to give a security module the
> ability to be involved in the initial connection authorization, but we
> run into an issue there if that module then needs access to the catalog.

Maybe so, but the proposed hook placement doesn't actually allow a
plugin module to be "involved" in the authorization --- we've already
decided the authorization is OK.  All it can do there is some additional
initialization, which could equally well be done on first use (if any)
of the additional information.

There might be some value in letting a plugin actually have some control
over the authentication process, but I'm not sure offhand what a
reasonable hook design would be.

			regards, tom lane

Re: [v9.1] Add security hook on initialization of instance

Robert Haas <robertmhaas@gmail.com> — 2010-07-08T14:42:03Z

On Thu, Jul 8, 2010 at 9:37 AM, Stephen Frost <sfrost@snowman.net> wrote:
>> FWIW, I am still of the opinion that we shouldn't have a hook here
>> anyway, because there is no reason to complain about lack of a
>> security context until the user performs an action which requires them
>> to have a security context.
>
> I don't agree with this, in general.  It may be a difficult problem to
> solve though.  From my perspective the above is similar to saying we
> don't need a pg_hba.conf or that we should open a database before
> checking the user's credentials.  I'd like to give a security module the
> ability to be involved in the initial connection authorization, but we
> run into an issue there if that module then needs access to the catalog.
> Perhaps it doesn't, but it seems like it would, to use to make a
> decision.

Well, perhaps I'll revise my opinion here a bit.  If we're actually
going to do something with the user's security context at connection
time, like validate that they have rights to connect to the database
they've selected, then it would make sense to have a hook somewhere in
the authentication process.

I think we have to assume that whatever actions a pluggable security
provider might take at authentication time are going to be based on
information from outside the database.  It would be nice to have an
infrastructure that would support making an access control decision
based on data from within the database, but as of today any catalogs
consulted during authentication must be (a) shared and (b) nailed, and
there's certainly no provision for third-party modules to add shared
or nailed system tables (or even, ordinary system tables).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: [v9.1] Add security hook on initialization of instance

Stephen Frost <sfrost@snowman.net> — 2010-07-08T14:42:51Z

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Maybe so, but the proposed hook placement doesn't actually allow a
> plugin module to be "involved" in the authorization --- we've already
> decided the authorization is OK.  All it can do there is some additional
> initialization, which could equally well be done on first use (if any)
> of the additional information.

Right, I agree that the existing patch isn't what should be done here.

> There might be some value in letting a plugin actually have some control
> over the authentication process, but I'm not sure offhand what a
> reasonable hook design would be.

Definitely needs more thought, but that's the direction that I think
makes more sense.

	Thanks!

		Stephen

Re: [v9.1] Add security hook on initialization of instance

Stephen Frost <sfrost@snowman.net> — 2010-07-08T14:48:26Z

* Robert Haas (robertmhaas@gmail.com) wrote:
> I think we have to assume that whatever actions a pluggable security
> provider might take at authentication time are going to be based on
> information from outside the database.

I feel that would be perfect for 9.1 and supporting access to the
general catalog is something that, if we figure out a sane way to
do it, we could always add later (if there's demand, etc).

For those bits of the catalog which *do* meet the requirements you
mention, I hope it'll be possible for the security module to access
them?  Does make me wonder if we might consider adding a field to those
to support a label rather than trying to figure out a way for a third
party to provide a shared/nailed relation.

	Thanks,

		Stephen

Re: [v9.1] Add security hook on initialization of instance

Robert Haas <robertmhaas@gmail.com> — 2010-07-08T14:54:46Z

On Thu, Jul 8, 2010 at 10:48 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Robert Haas (robertmhaas@gmail.com) wrote:
>> I think we have to assume that whatever actions a pluggable security
>> provider might take at authentication time are going to be based on
>> information from outside the database.
>
> I feel that would be perfect for 9.1 and supporting access to the
> general catalog is something that, if we figure out a sane way to
> do it, we could always add later (if there's demand, etc).
>
> For those bits of the catalog which *do* meet the requirements you
> mention, I hope it'll be possible for the security module to access
> them?  Does make me wonder if we might consider adding a field to those
> to support a label rather than trying to figure out a way for a third
> party to provide a shared/nailed relation.

I'm not sure what the best thing to do about this is.  I think it
might be a good idea to start with some discussion of what problems
people are trying to solve (hopefully N > 1?) and then try to figure
out what a good solution might look like.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: [v9.1] Add security hook on initialization of instance

Stephen Frost <sfrost@snowman.net> — 2010-07-08T14:58:09Z

* Robert Haas (robertmhaas@gmail.com) wrote:
> I'm not sure what the best thing to do about this is.  I think it
> might be a good idea to start with some discussion of what problems
> people are trying to solve (hopefully N > 1?) and then try to figure
> out what a good solution might look like.

Guess my first thought was that you'd have a database-level label that
would be used by SELinux to validate a connection.  A second thought is
labels for roles.  KaiGai, can you provide your thoughts on this
discussion/approach/problems?  I realize it's come a bit far-afield from
your original proposal.

	Thanks,

		Stephen

Re: [v9.1] Add security hook on initialization of instance

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-07-09T00:20:17Z

(2010/07/08 23:58), Stephen Frost wrote:
> * Robert Haas (robertmhaas@gmail.com) wrote:
>> I'm not sure what the best thing to do about this is.  I think it
>> might be a good idea to start with some discussion of what problems
>> people are trying to solve (hopefully N>  1?) and then try to figure
>> out what a good solution might look like.
> 
> Guess my first thought was that you'd have a database-level label that
> would be used by SELinux to validate a connection.  A second thought is
> labels for roles.  KaiGai, can you provide your thoughts on this
> discussion/approach/problems?  I realize it's come a bit far-afield from
> your original proposal.
> 

Let's sort out the point at issues from perspective of the models.

Please remind the authentication (in broad-sense) is consist of a few steps.

1) Identification
In this step, system tries to obtain the identifier of client.
Typically, it is a username provided using connection string in pgsql.

2) Authentication (in narrow-sense)
In this step, system tries to verify whether the given identifier is
correct, or not. Typically, it checks the password corresponding to
the user.

3) Authorization
In this step, system tries to assign a set of privileges on the new
session. Typically, it is user identifier itself, because DAC makes
access control decision based on the combination of user identifier
and access control list of the objects.
But all the security modules also intend to utilize the user identifier
for its own access control decision. E.g, SELinux uses a security label
of the client process. If we would have something like Oracle Label
Security, client's label may be associated with a certain database role
that is already authorized.

The purpose of my patch was to provide an external security provider
a chance to (3) authorize the new session based on or not-based on
the (1) identification and (2) authentication.

If we try to allow security providers to get control all of the 1-3,
it might be hard to find the best place to put the hook right now.
But, at least, SELinux does not have a plan to interpose identification
and authentication. All I want to do here is to authorize the client
just after authentication process.

I'd like to suggest that we focus on the (3) authorization process
for functionality of the patch. It may be also worth to control
identification and authentication using security provider, but we don't
have actual guest module right now.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: ExecutorCheckPerms() hook

Robert Haas <robertmhaas@gmail.com> — 2010-07-09T14:07:33Z

On Thu, May 20, 2010 at 1:33 PM, Tom Lane <tgl@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 is why I took the time to produce a working
>> example.
>
> +1 on committing the hook.  As for the contrib module, it doesn't strike
> me that there's much of a use-case for it as is.  I think it's enough
> that it's available in the -hackers archives.

OK, done that way.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: [v9.1] Add security hook on initialization of instance

Stephen Frost <sfrost@snowman.net> — 2010-07-09T14:52:22Z

* Stephen Frost (sfrost@snowman.net) wrote:
> Guess my first thought was that you'd have a database-level label that
> would be used by SELinux to validate a connection.  A second thought is
> labels for roles.  KaiGai, can you provide your thoughts on this
> discussion/approach/problems?  I realize it's come a bit far-afield from
> your original proposal.

Something else which has come up but is related is the ability to
support a "pam_tally"-like function in PG.  Basically, the ability to
lock users out if they've had too many failed login attempts.  I wonder
if we could add this hook (or maybe have more than one if necessary) in
a way to support a contrib module for that.

	Thanks,

		Stephen

Re: [v9.1] Add security hook on initialization of instance

Robert Haas <robertmhaas@gmail.com> — 2010-07-09T15:09:38Z

On Fri, Jul 9, 2010 at 10:52 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Stephen Frost (sfrost@snowman.net) wrote:
>> Guess my first thought was that you'd have a database-level label that
>> would be used by SELinux to validate a connection.  A second thought is
>> labels for roles.  KaiGai, can you provide your thoughts on this
>> discussion/approach/problems?  I realize it's come a bit far-afield from
>> your original proposal.
>
> Something else which has come up but is related is the ability to
> support a "pam_tally"-like function in PG.  Basically, the ability to
> lock users out if they've had too many failed login attempts.  I wonder
> if we could add this hook (or maybe have more than one if necessary) in
> a way to support a contrib module for that.

Seems like the hard part would be figuring out where to store the
bookkeeping information.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: [v9.1] Add security hook on initialization of instance

Stephen Frost <sfrost@snowman.net> — 2010-07-09T15:19:17Z

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Jul 9, 2010 at 10:52 AM, Stephen Frost <sfrost@snowman.net> wrote:
> > Something else which has come up but is related is the ability to
> > support a "pam_tally"-like function in PG.  Basically, the ability to
> > lock users out if they've had too many failed login attempts.  I wonder
> > if we could add this hook (or maybe have more than one if necessary) in
> > a way to support a contrib module for that.
> 
> Seems like the hard part would be figuring out where to store the
> bookkeeping information.

pam_tally does it in a flat file on-disk.  It's not perfect, but it
works.  It'd be good enough for me if there was a hook in the right
place that I could write the contrib module.

Of course, it'd be even nicer to have support for this in-core, since
it's a FISMA requirement and not everyone is going to like having to
install a contrib module to handle something like this.  Of course, so
is stuff like remembering old passwords, password aging, etc...  I do
really wish there was a way PAM could be used by applications
*independently* of the system auth (eg: /etc/passwd) to handle all of
this.  If anyone's aware of a way, I'm all ears..

	Thanks,

		Stephen

Re: [v9.1] Add security hook on initialization of instance

Robert Haas <robertmhaas@gmail.com> — 2010-07-09T15:21:29Z

On Fri, Jul 9, 2010 at 11:19 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Robert Haas (robertmhaas@gmail.com) wrote:
>> On Fri, Jul 9, 2010 at 10:52 AM, Stephen Frost <sfrost@snowman.net> wrote:
>> > Something else which has come up but is related is the ability to
>> > support a "pam_tally"-like function in PG.  Basically, the ability to
>> > lock users out if they've had too many failed login attempts.  I wonder
>> > if we could add this hook (or maybe have more than one if necessary) in
>> > a way to support a contrib module for that.
>>
>> Seems like the hard part would be figuring out where to store the
>> bookkeeping information.
>
> pam_tally does it in a flat file on-disk.  It's not perfect, but it
> works.  It'd be good enough for me if there was a hook in the right
> place that I could write the contrib module.
>
> Of course, it'd be even nicer to have support for this in-core, since
> it's a FISMA requirement and not everyone is going to like having to
> install a contrib module to handle something like this.

Feel free to propose a patch...!

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: Reworks of DML permission checks

Robert Haas <robertmhaas@gmail.com> — 2010-07-09T20:53:56Z

2010/6/14 KaiGai Kohei <kaigai@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

Re: Reworks of DML permission checks

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-07-12T04:09:08Z

(2010/07/10 5:53), Robert Haas wrote:
> 2010/6/14 KaiGai Kohei<kaigai@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@ak.jp.nec.com>

Re: [v9.1] Add security hook on initialization of instance

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-07-12T04:58:12Z

(2010/07/09 23:52), Stephen Frost wrote:
> * Stephen Frost (sfrost@snowman.net) wrote:
>> Guess my first thought was that you'd have a database-level label that
>> would be used by SELinux to validate a connection.  A second thought is
>> labels for roles.  KaiGai, can you provide your thoughts on this
>> discussion/approach/problems?  I realize it's come a bit far-afield from
>> your original proposal.
> 
> Something else which has come up but is related is the ability to
> support a "pam_tally"-like function in PG.  Basically, the ability to
> lock users out if they've had too many failed login attempts.  I wonder
> if we could add this hook (or maybe have more than one if necessary) in
> a way to support a contrib module for that.
> 
It seems to me a good idea.

BTW, where do you intend to apply this "pam_tally" like functionality?
If it tries to lock users out on the identification stage; like the
pam_tally.so on operating systems, the hook should be placed on the
top-half of ClientAuthentication().

On the other hand, when we tries to set up properties of a certain user's
session, it needs to be placed on the authorization stage.
In the PG code, InitializeSessionUserId() just performs the role to assign
the authenticated user's identifier on the current session. It seems to me
it is a candidate where we put a hook on the authorization stage.

Of course, these are not exclusive. We can provide two hooks to provide
a chance to get control on identification and authorization stages.

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: Reworks of DML permission checks

Robert Haas <robertmhaas@gmail.com> — 2010-07-19T18:13:18Z

2010/7/12 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> (2010/07/10 5:53), Robert Haas wrote:
>> 2010/6/14 KaiGai Kohei<kaigai@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

Re: Reworks of DML permission checks

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-07-20T00:24:47Z

(2010/07/20 3:13), Robert Haas wrote:
> 2010/7/12 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>> (2010/07/10 5:53), Robert Haas wrote:
>>> 2010/6/14 KaiGai Kohei<kaigai@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@ak.jp.nec.com>

Re: Reworks of DML permission checks

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-07-20T02:54:08Z

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@ak.jp.nec.com>:
>>> (2010/07/10 5:53), Robert Haas wrote:
>>>> 2010/6/14 KaiGai Kohei<kaigai@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@ak.jp.nec.com>

Re: ExecutorCheckPerms() hook

Robert Haas <robertmhaas@gmail.com> — 2010-07-21T23:45:04Z

2010/5/24 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> (2010/05/24 22:18), Robert Haas wrote:
>> 2010/5/24 KaiGai Kohei<kaigai@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?
>
> Sorry, after more detailed consideration, it seems to me the permission
> checks in RI_Initial_Check() and its fallback mechanism are nonsense.
>
> See the following commands.
>
>  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
>
> 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,
>
>  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"
>
> From more careful observation of the code, the validateForeignKeyConstraint()
> also calls RI_FKey_check_ins() for each scanned tuples, but it also execute
> SELECT statement using SPI_*() interface internally.
>
> In other words, both of execution paths entirely require SELECT permission
> to validate new FK constraint.

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 ownership:

rhaas=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a);
ERROR:  must be owner of relation fk_tbl

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-07-22T00:13:30Z

(2010/07/22 8:45), Robert Haas wrote:
> 2010/5/24 KaiGai Kohei<kaigai@ak.jp.nec.com>:
>> (2010/05/24 22:18), Robert Haas wrote:
>>> 2010/5/24 KaiGai Kohei<kaigai@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?
>>
>> Sorry, after more detailed consideration, it seems to me the permission
>> checks in RI_Initial_Check() and its fallback mechanism are nonsense.
>>
>> See the following commands.
>>
>>   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
>>
>> 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,
>>
>>   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"
>>
>>  From more careful observation of the code, the validateForeignKeyConstraint()
>> also calls RI_FKey_check_ins() for each scanned tuples, but it also execute
>> SELECT statement using SPI_*() interface internally.
>>
>> In other words, both of execution paths entirely require SELECT permission
>> to validate new FK constraint.
> 
> 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 ownership:
> 
> rhaas=>  ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a);
> ERROR:  must be owner of relation fk_tbl
> 
+1.

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 its ownership.

Perhaps, if SQL permission would be more fine-grained, for example,
"RENAME" permission might control RENAME TO statement, rather than
its ownership.

What is the reason why we check its ownership here, although we already
have REFERENCES permission to control ADD FOREIGN KEY?

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>

Re: Reworks of DML permission checks

Robert Haas <robertmhaas@gmail.com> — 2010-07-22T00:48:59Z

2010/7/19 KaiGai Kohei <kaigai@ak.jp.nec.com>:
> 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.

Committed with some changes to the comments.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-07-22T01:02:15Z

* Robert Haas (robertmhaas@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 ownership:
> 
> rhaas=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a);
> ERROR:  must be owner of relation fk_tbl

Errr, no.  If I grant you REFERENCES on my table, it means you can
create a FK to it from some other table.  That's very different from
saying you can create a FK *on* my table.  Put another way- you can
prevent me from deleting data in my table if you have a FK to it, but
you can prevent me from *inserting* data into my table if you can create
a FK on it.  Those are two distinct and different things and I
definitely don't believe we should have 1 permission be used for both.

Also, REFERENCES is in the spec, and I don't believe you could
interprete it to letting people create FKs on tables they have
REFERENCES on, afaik.  I don't believe it's how other RDBMS' are either,
but I have to admit to not having tested yet.

Let's not add things to an SQL-defined priviledge or we'll end up
seriously suprising people coming from standard-conforming databases,
and in a security way.

All that being said, having more fine-grained control over what can be
done through an ALTER TABLE command is a neat idea, but it's also a
pretty huge can of worms.  I'd rather spend time figuring out the
somewhat smaller set of things which are superuser only right now, and
creating a way to have just non-superuser roles which can do those
things (where it makes sense, anyway).

	Thanks,

		Stephen

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-07-22T01:04:41Z

* KaiGai Kohei (kaigai@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 its ownership.

TRIGGER is hardly the same as REFERENCES.  If we invented a new priv, it
would be more like 'FK_CREATE'.

> Perhaps, if SQL permission would be more fine-grained, for example,
> "RENAME" permission might control RENAME TO statement, rather than
> its ownership.

This wouldn't actually be any more fine-grained, it'd just be adding
rights on to an existing priv, which I think is a wholly *bad* idea.

> What is the reason why we check its ownership here, although we already
> have REFERENCES permission to control ADD FOREIGN KEY?

REFERENCES is needed on the REFERENCED table, ownership is needed on the
REFERENCING table.  They're not the same..

We only allow owners of objects to change the structure of those
objects.  Adding a FK to another table doesn't really change the
structure of the referenced table.  Adding a FK does though, imv.

	Thanks,

		Stephen

Re: ExecutorCheckPerms() hook

Robert Haas <robertmhaas@gmail.com> — 2010-07-22T01:08:23Z

On Wed, Jul 21, 2010 at 9:02 PM, Stephen Frost <sfrost@snowman.net> wrote:
> * Robert Haas (robertmhaas@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 ownership:
>>
>> rhaas=> ALTER TABLE fk_tbl ADD FOREIGN KEY (x) REFERENCES pk_tbl (a);
>> ERROR:  must be owner of relation fk_tbl
>
> 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:

       To create a foreign key constraint, it is
       necessary to have this privilege on both the referencing and
       referenced columns.  The privilege may be granted for all columns
       of a table, or just specific columns.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Re: ExecutorCheckPerms() hook

Stephen Frost <sfrost@snowman.net> — 2010-07-22T01:15:07Z

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Wed, Jul 21, 2010 at 9:02 PM, Stephen Frost <sfrost@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:
> 
>        To create a foreign key constraint, it is
>        necessary to have this privilege on both the referencing and
>        referenced columns.  The privilege may be granted for all columns
>        of a table, or just specific columns.

Technically that's true..  You just *also* have to own the referencing
table. :)  I agree though, if my claims are correct (which I'd like to
think they are, but perusing the SQL spec just now didn't make it as
abundently clear as I would have hoped...), and it's how PG acts today
anyway, we should definitely fix the docs.

Also, we do document that to use ALTER TABLE you have to own the table
you're calling ALTER TABLE on, and obviously if you're calling CREATE
TABLE you're "owner" of the object..  Have we got another way to add a
FK to an existing table?  If so, we should make sure they're all
consistant in any case.

	Thanks,

		Stephen

Re: ExecutorCheckPerms() hook

Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-07-22T01:18:50Z

(2010/07/22 10:04), Stephen Frost wrote:
> * KaiGai Kohei (kaigai@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 its ownership.
> 
> TRIGGER is hardly the same as REFERENCES.  If we invented a new priv, it
> would be more like 'FK_CREATE'.
> 
>> Perhaps, if SQL permission would be more fine-grained, for example,
>> "RENAME" permission might control RENAME TO statement, rather than
>> its ownership.
> 
> This wouldn't actually be any more fine-grained, it'd just be adding
> rights on to an existing priv, which I think is a wholly *bad* idea.
> 
>> What is the reason why we check its ownership here, although we already
>> have REFERENCES permission to control ADD FOREIGN KEY?
> 
> REFERENCES is needed on the REFERENCED table, ownership is needed on the
> REFERENCING table.  They're not the same..
> 
> We only allow owners of objects to change the structure of those
> objects.  Adding a FK to another table doesn't really change the
> structure of the referenced table.  Adding a FK does though, imv.
> 
However, existing ATAddForeignKeyConstraint() checks REFERENCES
permission on both of the referencing and referenced table/columns.
Is it unexpected behavior???

It is an agreeable interpretation that we need ownership on the
referencing table because creating a new FK equals to change
a certain property of the referencing table.

If so, why REFERENCES permissions are necessary on the referencing
side, not only referenced side?

Thanks,
-- 
KaiGai Kohei <kaigai@ak.jp.nec.com>