ExecutorCheckPerms() hook
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
Attachments:
executor_check_perms.patchapplication/octet-stream; name=executor_check_perms.patchDownload+191-0
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
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
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
(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>
Attachments:
dml_reworks_kaigai.1.patchtext/x-patch; name=dml_reworks_kaigai.1.patchDownload+434-209
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
* 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
(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>
(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>
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;
GRANTAt 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
* 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
(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;
GRANTAt 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>
(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>
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
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
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
(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>
Attachments:
dml_reworks_kaigai.2.patchtext/x-patch; name=dml_reworks_kaigai.2.patchDownload+143-115
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
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
* 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