Thread: [PATCH] Reworks for Access Control facilities (r2251)

[PATCH] Reworks for Access Control facilities (r2251)

From
KaiGai Kohei
Date:
The attached patch reworks access control facilities in PostgreSQL.

The current implementation does not have well separation in what
to be controled and how to be controled. For example, when we create
a new table, it requires users ACL_CREATE on the namespace and
ACL_CREATE on the tablespace if necessary. These checks are methods
to control whether he can create a new table, or not.

This patch provides an abstraction layer of access controls to
separate what to be controlsed and how to be controled.
The abstraction layer is a set of functions to implement what
to be controled.
For example, ac_relation_create() checks user's privilege to
create a new table. It internally calls pg_namespace_aclcheck()
and pg_tablespace_aclcheck() to make its access control decision
based on the security model in database ACLs.

This abstraction layer functions have the following naming convension.

  ac_<object type>_<action>(args, ...)

e.g)  void ac_proc_execute(Oid proOid, Oid roleOid)
        It checks privilege to execute a certain procedure with
        the given database role. The caller gives all the necessary
        informations to make its decision.

It replaces all the pg_xxx_aclcheck() and pg_xxx_ownercheck() invocations
from the backend implementations, except for security/access_control.c.
In this patch, these are used as helper functions to implement access
control logic (in other word, how to be controled), invoked from the
access control functions.

These ac_xxx_xxx() routines will be entrypoints to invoke additional
security checks (SE-PostgreSQL), rather than sepgsqlXXXX() hooks around
the backend implementation.

Thanks,

[kaigai@saba pgsec]$ diffstat sepgsql-01-base-8.5devel-r2251.patch.gz
 backend/Makefile                  |    2
 backend/catalog/aclchk.c          |  218 !
 backend/catalog/namespace.c       |   53
 backend/catalog/pg_aggregate.c    |   12
 backend/catalog/pg_conversion.c   |   33
 backend/catalog/pg_operator.c     |   42
 backend/catalog/pg_proc.c         |   15
 backend/catalog/pg_shdepend.c     |    8
 backend/catalog/pg_type.c         |   25
 backend/commands/aggregatecmds.c  |   42
 backend/commands/alter.c          |   66
 backend/commands/analyze.c        |    5
 backend/commands/cluster.c        |    9
 backend/commands/comment.c        |  120
 backend/commands/conversioncmds.c |   71
 backend/commands/copy.c           |   40
 backend/commands/dbcommands.c     |  160 !
 backend/commands/foreigncmds.c    |  144
 backend/commands/functioncmds.c   |  123
 backend/commands/indexcmds.c      |  120
 backend/commands/lockcmds.c       |   17
 backend/commands/opclasscmds.c    |  223 !
 backend/commands/operatorcmds.c   |   70
 backend/commands/proclang.c       |   56
 backend/commands/schemacmds.c     |   60
 backend/commands/sequence.c       |   38
 backend/commands/tablecmds.c      |  427 -!
 backend/commands/tablespace.c     |   46
 backend/commands/trigger.c        |   41
 backend/commands/tsearchcmds.c    |  176 !
 backend/commands/typecmds.c       |  136 !
 backend/commands/vacuum.c         |    3
 backend/commands/view.c           |    7
 backend/executor/execMain.c       |  203 !
 backend/executor/execQual.c       |   16
 backend/executor/nodeAgg.c        |   24
 backend/executor/nodeMergejoin.c  |    8
 backend/executor/nodeWindowAgg.c  |   24
 backend/optimizer/util/clauses.c  |    6
 backend/parser/parse_utilcmd.c    |   13
 backend/rewrite/rewriteDefine.c   |   10
 backend/rewrite/rewriteRemove.c   |    6
 backend/security/Makefile         |   10
 backend/security/access_control.c | 4290 ++++++++++++++++++++++++++++++++++++++
 backend/tcop/fastpath.c           |   15
 backend/tcop/utility.c            |   74
 backend/utils/adt/dbsize.c        |   25
 backend/utils/adt/ri_triggers.c   |   24
 backend/utils/adt/tid.c           |   18
 backend/utils/init/postinit.c     |   14
 include/catalog/pg_proc_fn.h      |    1
 include/commands/defrem.h         |    1
 include/utils/security.h          |  337 ++
 53 files changed, 5027 insertions(+), 924 deletions(-), 1776 modifications(!)

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

Attachment

[PATCH] Reworks for Access Control facilities (r2277)

From
KaiGai Kohei
Date:
The attached patch is updated one for reworks of access control
facilities in PostgreSQL.

List of updates:
- Base tree was updated to the latest CVS HEAD
- ac_role_xxx() routines were added, which was forgotten to include.
- Code cleanups. A few compile time warnings were eliminated.
- Add source code comments to introduce why the security checks are
  modified at some points.

BTW, IIRC, I remember you mentioned as follows:
| We're not afraid of making large scale changes to core.  It just needs
| to be done incrementally and done *first*, before adding features and
| code which depends on it.  We have to fix PG *first*, in this regard,
| before we can even start to look at SELinux hooks, etc.

This patch is indeed pure PostgreSQL feature, but a bit large.
|  54 files changed, 5313 insertions(+), 937 deletions(-), 1875 modifications(!)

I'm afraid of the patch is ignored due to the scale of changeset again.
But we also need to get succeeded to merge the access control reworks
at the next commit fest, because we have only three commit fest remained
and the last one automatically reject large patches, so I have to submit
SE-PostgreSQL/lite patch on the CF-Nov.

Is there any good idea to help reviewers?
Or, do you think it is enough to review in the CF-Sep?

One possible idea is to separate the current single patch into the several
shorter frames. For example, the first patch reworks access controls
corresponding to relations into ac_relation_xxxx(), the second patch
reworks access controls corresponding to procedures into ac_proc_xxx(),
and so on....
But, I'm not sure whether it is really helpful to review the patch.
(Needless to say, if we can conclude it is helpfull, I'll rework for
the patch prior to the beggining of the CF-Sep.)

Thanks,

KaiGai Kohei wrote:
> The attached patch reworks access control facilities in PostgreSQL.
>
> The current implementation does not have well separation in what
> to be controled and how to be controled. For example, when we create
> a new table, it requires users ACL_CREATE on the namespace and
> ACL_CREATE on the tablespace if necessary. These checks are methods
> to control whether he can create a new table, or not.
>
> This patch provides an abstraction layer of access controls to
> separate what to be controlsed and how to be controled.
> The abstraction layer is a set of functions to implement what
> to be controled.
> For example, ac_relation_create() checks user's privilege to
> create a new table. It internally calls pg_namespace_aclcheck()
> and pg_tablespace_aclcheck() to make its access control decision
> based on the security model in database ACLs.
>
> This abstraction layer functions have the following naming convension.
>
>   ac_<object type>_<action>(args, ...)
>
> e.g)  void ac_proc_execute(Oid proOid, Oid roleOid)
>         It checks privilege to execute a certain procedure with
>         the given database role. The caller gives all the necessary
>         informations to make its decision.
>
> It replaces all the pg_xxx_aclcheck() and pg_xxx_ownercheck() invocations
> from the backend implementations, except for security/access_control.c.
> In this patch, these are used as helper functions to implement access
> control logic (in other word, how to be controled), invoked from the
> access control functions.
>
> These ac_xxx_xxx() routines will be entrypoints to invoke additional
> security checks (SE-PostgreSQL), rather than sepgsqlXXXX() hooks around
> the backend implementation.
>
> Thanks,

[kaigai@saba trunk]$ diffstat sepgsql-01-base-8.5devel-r2277.patch.gz
 backend/Makefile                  |    2
 backend/catalog/aclchk.c          |  220 !
 backend/catalog/namespace.c       |   66
 backend/catalog/pg_aggregate.c    |   12
 backend/catalog/pg_conversion.c   |   33
 backend/catalog/pg_operator.c     |   42
 backend/catalog/pg_proc.c         |   22
 backend/catalog/pg_shdepend.c     |   18
 backend/catalog/pg_type.c         |   25
 backend/commands/aggregatecmds.c  |   42
 backend/commands/alter.c          |   72
 backend/commands/analyze.c        |    5
 backend/commands/cluster.c        |    9
 backend/commands/comment.c        |  125
 backend/commands/conversioncmds.c |   71
 backend/commands/copy.c           |   40
 backend/commands/dbcommands.c     |  160 !
 backend/commands/foreigncmds.c    |  144
 backend/commands/functioncmds.c   |  127
 backend/commands/indexcmds.c      |  120
 backend/commands/lockcmds.c       |   17
 backend/commands/opclasscmds.c    |  223 !
 backend/commands/operatorcmds.c   |   70
 backend/commands/proclang.c       |   56
 backend/commands/schemacmds.c     |   60
 backend/commands/sequence.c       |   38
 backend/commands/tablecmds.c      |  355 -
 backend/commands/tablespace.c     |   46
 backend/commands/trigger.c        |   41
 backend/commands/tsearchcmds.c    |  176 !
 backend/commands/typecmds.c       |  137 !
 backend/commands/user.c           |  183 !
 backend/commands/vacuum.c         |    5
 backend/commands/view.c           |    7
 backend/executor/execMain.c       |  203 !
 backend/executor/execQual.c       |   16
 backend/executor/nodeAgg.c        |   24
 backend/executor/nodeMergejoin.c  |    8
 backend/executor/nodeWindowAgg.c  |   24
 backend/optimizer/util/clauses.c  |    6
 backend/parser/parse_utilcmd.c    |   13
 backend/rewrite/rewriteDefine.c   |   10
 backend/rewrite/rewriteRemove.c   |    6
 backend/security/Makefile         |   10
 backend/security/access_control.c | 4513 ++++++++++++++++++++++++++++++++++++++
 backend/tcop/fastpath.c           |   15
 backend/tcop/utility.c            |   74
 backend/utils/adt/dbsize.c        |   25
 backend/utils/adt/ri_triggers.c   |   24
 backend/utils/adt/tid.c           |   18
 backend/utils/init/postinit.c     |   16
 include/catalog/pg_proc_fn.h      |    1
 include/commands/defrem.h         |    1
 include/utils/security.h          |  349 ++
 54 files changed, 5313 insertions(+), 937 deletions(-), 1875 modifications(!)

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>

Attachment