[PATCH] ACE Framework - Database, Schema
Stephen,
The attached two patches are the first pieces of split out from
the previous large access control reworks patch.
The pgsql-ace-01-database-8.5devel-r2475.patch contains nigh
security hooks related to global initialization and databases.
The pgsql-ace-02-schema-8.5devel-r2475.patch contains the six
security hooks related to schema objects.
Note that these are not simple replacement for pg_xxx_aclcheck()
and pg_xxx_ownercheck(). For example, DefineRelation() calls
pg_namespace_aclcheck() with ACL_CREATE. This check shall be
abstracted in the pgsql-ace-0x-relation patch, so I don't touch
them yet.
Also note that these patches don't support any security label.
So, ace_xxx_create() is declared as void function, although it
has to return a security label to be assigned.
But these hooks are deployed on where we can easily support
security label management, so later patch will fix it.
The previous patch is too large to review.
Is this scale confortable to review?
$ diffstat pgsql-ace-01-database-8.5devel-r2475.patch
backend/Makefile | 2
backend/catalog/aclchk.c | 68 +++++++!
backend/commands/comment.c | 5
backend/commands/dbcommands.c | 154 +--------!!!!!!!!!
backend/commands/indexcmds.c | 6
backend/security/Makefile | 10 +
backend/security/ace/Makefile | 11 +
backend/security/ace/ace_database.c | 285 ++++++++++++++++++++++++++++++++++++
backend/security/ace/ace_misc.c | 23 ++
backend/utils/adt/dbsize.c | 9
backend/utils/init/postinit.c | 17 !!
include/security/ace.h | 39 ++++
12 files changed, 445 insertions(+), 63 deletions(-), 121 modifications(!)
$ diffstat pgsql-ace-02-schema-8.5devel-r2475.patch
backend/catalog/aclchk.c | 15 +!
backend/catalog/namespace.c | 42 ++---!!
backend/commands/comment.c | 4
backend/commands/schemacmds.c | 57 -!!!!!!!!!
backend/security/ace/Makefile | 2
backend/security/ace/ace_schema.c | 200 ++++++++++++++++++++++++++++++++++++++
backend/tcop/fastpath.c | 6 !
include/security/ace.h | 14 ++
8 files changed, 234 insertions(+), 25 deletions(-), 81 modifications(!)
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
Attachments:
pgsql-ace-01-database-8.5devel-r2475.patchtext/x-patch; name=pgsql-ace-01-database-8.5devel-r2475.patchDownload
diff -Nrpc base/src/backend/Makefile ace_database/src/backend/Makefile
*** base/src/backend/Makefile Fri Sep 11 15:31:36 2009
--- ace_database/src/backend/Makefile Sun Dec 13 14:27:22 2009
*************** include $(top_builddir)/src/Makefile.glo
*** 16,22 ****
SUBDIRS = access bootstrap catalog parser commands executor foreign lib libpq \
main nodes optimizer port postmaster regex rewrite \
! storage tcop tsearch utils $(top_builddir)/src/timezone
include $(srcdir)/common.mk
--- 16,22 ----
SUBDIRS = access bootstrap catalog parser commands executor foreign lib libpq \
main nodes optimizer port postmaster regex rewrite \
! security storage tcop tsearch utils $(top_builddir)/src/timezone
include $(srcdir)/common.mk
diff -Nrpc base/src/backend/catalog/aclchk.c ace_database/src/backend/catalog/aclchk.c
*** base/src/backend/catalog/aclchk.c Fri Dec 11 14:58:31 2009
--- ace_database/src/backend/catalog/aclchk.c Sun Dec 13 14:27:22 2009
***************
*** 46,51 ****
--- 46,52 ----
#include "foreign/foreign.h"
#include "miscadmin.h"
#include "parser/parse_func.h"
+ #include "security/ace.h"
#include "utils/acl.h"
#include "utils/fmgroids.h"
#include "utils/lsyscache.h"
*************** static void expand_all_col_privileges(Oi
*** 125,130 ****
--- 126,134 ----
int num_col_privileges);
static AclMode string_to_privilege(const char *privname);
static const char *privilege_to_string(AclMode privilege);
+ static AclMode restrict_grant(bool is_grant, AclMode avail_goptions,
+ bool all_privs, AclMode privileges,
+ const char *objname);
static AclMode restrict_and_check_grant(bool is_grant, AclMode avail_goptions,
bool all_privs, AclMode privileges,
Oid objectId, Oid grantorId,
*************** merge_acl_with_grant(Acl *old_acl, bool
*** 221,230 ****
--- 225,281 ----
return new_acl;
}
+
/*
* Restrict the privileges to what we can actually grant, and emit
* the standards-mandated warning and error messages.
*/
+ static AclMode restrict_grant(bool is_grant, AclMode avail_goptions,
+ bool all_privs, AclMode privileges,
+ const char *objname)
+ {
+ AclMode this_privileges;
+
+ /*
+ * Restrict the operation to what we can actually grant or revoke, and
+ * issue a warning if appropriate. (For REVOKE this isn't quite what the
+ * spec says to do: the spec seems to want a warning only if no privilege
+ * bits actually change in the ACL. In practice that behavior seems much
+ * too noisy, as well as inconsistent with the GRANT case.)
+ */
+ this_privileges = privileges & ACL_OPTION_TO_PRIVS(avail_goptions);
+ if (is_grant)
+ {
+ if (this_privileges == 0)
+ ereport(WARNING,
+ (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
+ errmsg("no privileges were granted for \"%s\"", objname)));
+ else if (!all_privs && this_privileges != privileges)
+ ereport(WARNING,
+ (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_GRANTED),
+ errmsg("not all privileges were granted for \"%s\"", objname)));
+ }
+ else
+ {
+ if (this_privileges == 0)
+ ereport(WARNING,
+ (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
+ errmsg("no privileges could be revoked for \"%s\"", objname)));
+ else if (!all_privs && this_privileges != privileges)
+ ereport(WARNING,
+ (errcode(ERRCODE_WARNING_PRIVILEGE_NOT_REVOKED),
+ errmsg("not all privileges could be revoked for \"%s\"", objname)));
+ }
+
+ return this_privileges;
+ }
+
+ /*
+ * Restrict the privileges to what we can actually grant, and emit
+ * the standards-mandated warning and error messages.
+ * (when ace_xxx_grant() is given for all the object classes, this
+ * function shall be removed.)
+ */
static AclMode
restrict_and_check_grant(bool is_grant, AclMode avail_goptions, bool all_privs,
AclMode privileges, Oid objectId, Oid grantorId,
*************** ExecGrant_Database(InternalGrant *istmt)
*** 1965,1979 ****
&grantorId, &avail_goptions);
/*
* Restrict the privileges to what we can actually grant, and emit the
* standards-mandated warning and error messages.
*/
this_privileges =
! restrict_and_check_grant(istmt->is_grant, avail_goptions,
! istmt->all_privs, istmt->privileges,
! datId, grantorId, ACL_KIND_DATABASE,
! NameStr(pg_database_tuple->datname),
! 0, NULL);
/*
* Generate new ACL.
--- 2016,2037 ----
&grantorId, &avail_goptions);
/*
+ * Permission check to grant/revoke permissions.
+ *
+ * If we found no grant options, consider whether to issue a hard
+ * error. Per spec, having any privilege at all on the object will
+ * get you by here.
+ */
+ ace_database_grant(datId, grantorId, avail_goptions);
+
+ /*
* Restrict the privileges to what we can actually grant, and emit the
* standards-mandated warning and error messages.
*/
this_privileges =
! restrict_grant(istmt->is_grant, avail_goptions,
! istmt->all_privs, istmt->privileges,
! NameStr(pg_database_tuple->datname));
/*
* Generate new ACL.
diff -Nrpc base/src/backend/commands/comment.c ace_database/src/backend/commands/comment.c
*** base/src/backend/commands/comment.c Fri Dec 11 14:58:31 2009
--- ace_database/src/backend/commands/comment.c Sun Dec 13 14:27:22 2009
***************
*** 49,54 ****
--- 49,55 ----
#include "parser/parse_func.h"
#include "parser/parse_oper.h"
#include "parser/parse_type.h"
+ #include "security/ace.h"
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
*************** CommentDatabase(List *qualname, char *co
*** 685,693 ****
}
/* Check object security */
! if (!pg_database_ownercheck(oid, GetUserId()))
! aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
! database);
/* Call CreateSharedComments() to create/drop the comments */
CreateSharedComments(oid, DatabaseRelationId, comment);
--- 686,692 ----
}
/* Check object security */
! ace_database_comment(oid);
/* Call CreateSharedComments() to create/drop the comments */
CreateSharedComments(oid, DatabaseRelationId, comment);
diff -Nrpc base/src/backend/commands/dbcommands.c ace_database/src/backend/commands/dbcommands.c
*** base/src/backend/commands/dbcommands.c Thu Nov 12 23:28:17 2009
--- ace_database/src/backend/commands/dbcommands.c Sun Dec 13 14:27:22 2009
***************
*** 42,47 ****
--- 42,48 ----
#include "miscadmin.h"
#include "pgstat.h"
#include "postmaster/bgwriter.h"
+ #include "security/ace.h"
#include "storage/bufmgr.h"
#include "storage/fd.h"
#include "storage/lmgr.h"
*************** static bool get_db_info(const char *name
*** 79,85 ****
int *encodingP, bool *dbIsTemplateP, bool *dbAllowConnP,
Oid *dbLastSysOidP, TransactionId *dbFrozenXidP,
Oid *dbTablespace, char **dbCollate, char **dbCtype);
- static bool have_createdb_privilege(void);
static void remove_dbtablespaces(Oid db_id);
static bool check_db_file_conflict(Oid db_id);
static int errdetail_busy_db(int notherbackends, int npreparedxacts);
--- 80,85 ----
*************** createdb(const CreatedbStmt *stmt)
*** 103,109 ****
Oid src_lastsysoid;
TransactionId src_frozenxid;
Oid src_deftablespace;
! volatile Oid dst_deftablespace;
Relation pg_database_rel;
HeapTuple tuple;
Datum new_record[Natts_pg_database];
--- 103,109 ----
Oid src_lastsysoid;
TransactionId src_frozenxid;
Oid src_deftablespace;
! volatile Oid dst_deftablespace = InvalidOid;
Relation pg_database_rel;
HeapTuple tuple;
Datum new_record[Natts_pg_database];
*************** createdb(const CreatedbStmt *stmt)
*** 250,255 ****
--- 250,267 ----
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid connection limit: %d", dbconnlimit)));
}
+ if (dtablespacename && dtablespacename->arg)
+ {
+ char *tablespacename;
+
+ tablespacename = strVal(dtablespacename->arg);
+ dst_deftablespace = get_tablespace_oid(tablespacename);
+ if (!OidIsValid(dst_deftablespace))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("tablespace \"%s\" does not exist",
+ tablespacename)));
+ }
/* obtain OID of proposed owner */
if (dbowner)
*************** createdb(const CreatedbStmt *stmt)
*** 258,277 ****
datdba = GetUserId();
/*
- * To create a database, must have createdb privilege and must be able to
- * become the target role (this does not imply that the target role itself
- * must have createdb privilege). The latter provision guards against
- * "giveaway" attacks. Note that a superuser will always have both of
- * these privileges a fortiori.
- */
- if (!have_createdb_privilege())
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to create database")));
-
- check_is_member_of_role(GetUserId(), datdba);
-
- /*
* Lookup database (template) to be cloned, and obtain share lock on it.
* ShareLock allows two CREATE DATABASEs to work from the same template
* concurrently, while ensuring no one is busy dropping it in parallel
--- 270,275 ----
*************** createdb(const CreatedbStmt *stmt)
*** 293,310 ****
errmsg("template database \"%s\" does not exist",
dbtemplate)));
! /*
! * Permission check: to copy a DB that's not marked datistemplate, you
! * must be superuser or the owner thereof.
! */
! if (!src_istemplate)
! {
! if (!pg_database_ownercheck(src_dboid, GetUserId()))
! ereport(ERROR,
! (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("permission denied to copy database \"%s\"",
! dbtemplate)));
! }
/* If encoding or locales are defaulted, use source's setting */
if (encoding < 0)
--- 291,299 ----
errmsg("template database \"%s\" does not exist",
dbtemplate)));
! /* Permission check to create a new database */
! ace_database_create(dbname, src_dboid, src_istemplate,
! datdba, dst_deftablespace);
/* If encoding or locales are defaulted, use source's setting */
if (encoding < 0)
*************** createdb(const CreatedbStmt *stmt)
*** 421,445 ****
}
/* Resolve default tablespace for new database */
! if (dtablespacename && dtablespacename->arg)
{
- char *tablespacename;
- AclResult aclresult;
-
- tablespacename = strVal(dtablespacename->arg);
- dst_deftablespace = get_tablespace_oid(tablespacename);
- if (!OidIsValid(dst_deftablespace))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("tablespace \"%s\" does not exist",
- tablespacename)));
- /* check permissions */
- aclresult = pg_tablespace_aclcheck(dst_deftablespace, GetUserId(),
- ACL_CREATE);
- if (aclresult != ACLCHECK_OK)
- aclcheck_error(aclresult, ACL_KIND_TABLESPACE,
- tablespacename);
-
/* pg_global must never be the default tablespace */
if (dst_deftablespace == GLOBALTABLESPACE_OID)
ereport(ERROR,
--- 410,417 ----
}
/* Resolve default tablespace for new database */
! if (OidIsValid(dst_deftablespace))
{
/* pg_global must never be the default tablespace */
if (dst_deftablespace == GLOBALTABLESPACE_OID)
ereport(ERROR,
*************** createdb(const CreatedbStmt *stmt)
*** 471,477 ****
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot assign new default tablespace \"%s\"",
! tablespacename),
errdetail("There is a conflict because database \"%s\" already has some tables in this tablespace.",
dbtemplate)));
pfree(srcpath);
--- 443,449 ----
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot assign new default tablespace \"%s\"",
! get_tablespace_name(dst_deftablespace)),
errdetail("There is a conflict because database \"%s\" already has some tables in this tablespace.",
dbtemplate)));
pfree(srcpath);
*************** dropdb(const char *dbname, bool missing_
*** 771,779 ****
/*
* Permission checks
*/
! if (!pg_database_ownercheck(db_id, GetUserId()))
! aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
! dbname);
/*
* Disallow dropping a DB that is marked istemplate. This is just to
--- 743,749 ----
/*
* Permission checks
*/
! ace_database_drop(db_id, false);
/*
* Disallow dropping a DB that is marked istemplate. This is just to
*************** RenameDatabase(const char *oldname, cons
*** 904,919 ****
(errcode(ERRCODE_UNDEFINED_DATABASE),
errmsg("database \"%s\" does not exist", oldname)));
! /* must be owner */
! if (!pg_database_ownercheck(db_id, GetUserId()))
! aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
! oldname);
!
! /* must have createdb rights */
! if (!have_createdb_privilege())
! ereport(ERROR,
! (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("permission denied to rename database")));
/*
* Make sure the new name doesn't exist. See notes for same error in
--- 874,881 ----
(errcode(ERRCODE_UNDEFINED_DATABASE),
errmsg("database \"%s\" does not exist", oldname)));
! /* Permission check to rename the database */
! ace_database_alter(db_id, newname, InvalidOid, InvalidOid);
/*
* Make sure the new name doesn't exist. See notes for same error in
*************** movedb(const char *dbname, const char *t
*** 984,990 ****
bool new_record_repl[Natts_pg_database];
ScanKeyData scankey;
SysScanDesc sysscan;
- AclResult aclresult;
char *src_dbpath;
char *dst_dbpath;
DIR *dstdir;
--- 946,951 ----
*************** movedb(const char *dbname, const char *t
*** 1015,1027 ****
AccessExclusiveLock);
/*
- * Permission checks
- */
- if (!pg_database_ownercheck(db_id, GetUserId()))
- aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
- dbname);
-
- /*
* Obviously can't move the tables of my own database
*/
if (db_id == MyDatabaseId)
--- 976,981 ----
*************** movedb(const char *dbname, const char *t
*** 1041,1051 ****
/*
* Permission checks
*/
! aclresult = pg_tablespace_aclcheck(dst_tblspcoid, GetUserId(),
! ACL_CREATE);
! if (aclresult != ACLCHECK_OK)
! aclcheck_error(aclresult, ACL_KIND_TABLESPACE,
! tblspcname);
/*
* pg_global must never be the default tablespace
--- 995,1001 ----
/*
* Permission checks
*/
! ace_database_alter(db_id, NULL, dst_tblspcoid, InvalidOid);
/*
* pg_global must never be the default tablespace
*************** AlterDatabase(AlterDatabaseStmt *stmt, b
*** 1367,1375 ****
(errcode(ERRCODE_UNDEFINED_DATABASE),
errmsg("database \"%s\" does not exist", stmt->dbname)));
! if (!pg_database_ownercheck(HeapTupleGetOid(tuple), GetUserId()))
! aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
! stmt->dbname);
/*
* Build an updated tuple, perusing the information just obtained
--- 1317,1326 ----
(errcode(ERRCODE_UNDEFINED_DATABASE),
errmsg("database \"%s\" does not exist", stmt->dbname)));
! /*
! * Permission checks
! */
! ace_database_alter(HeapTupleGetOid(tuple), NULL, InvalidOid, InvalidOid);
/*
* Build an updated tuple, perusing the information just obtained
*************** AlterDatabaseSet(AlterDatabaseSetStmt *s
*** 1417,1425 ****
*/
shdepLockAndCheckObject(DatabaseRelationId, datid);
! if (!pg_database_ownercheck(datid, GetUserId()))
! aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
! stmt->dbname);
AlterSetting(datid, InvalidOid, stmt->setstmt);
--- 1368,1375 ----
*/
shdepLockAndCheckObject(DatabaseRelationId, datid);
! /* Permission checks */
! ace_database_alter(datid, NULL, InvalidOid, InvalidOid);
AlterSetting(datid, InvalidOid, stmt->setstmt);
*************** AlterDatabaseOwner(const char *dbname, O
*** 1474,1500 ****
bool isNull;
HeapTuple newtuple;
! /* Otherwise, must be owner of the existing object */
! if (!pg_database_ownercheck(HeapTupleGetOid(tuple), GetUserId()))
! aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
! dbname);
!
! /* Must be able to become new owner */
! check_is_member_of_role(GetUserId(), newOwnerId);
!
! /*
! * must have createdb rights
! *
! * NOTE: This is different from other alter-owner checks in that the
! * current user is checked for createdb privileges instead of the
! * destination owner. This is consistent with the CREATE case for
! * databases. Because superusers will always have this right, we need
! * no special case for them.
! */
! if (!have_createdb_privilege())
! ereport(ERROR,
! (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("permission denied to change owner of database")));
memset(repl_null, false, sizeof(repl_null));
memset(repl_repl, false, sizeof(repl_repl));
--- 1424,1432 ----
bool isNull;
HeapTuple newtuple;
! /* Permission checks */
! ace_database_alter(HeapTupleGetOid(tuple),
! NULL, InvalidOid, newOwnerId);
memset(repl_null, false, sizeof(repl_null));
memset(repl_repl, false, sizeof(repl_repl));
*************** get_db_info(const char *name, LOCKMODE l
*** 1664,1691 ****
return result;
}
- /* Check if current user has createdb privileges */
- static bool
- have_createdb_privilege(void)
- {
- bool result = false;
- HeapTuple utup;
-
- /* Superusers can always do everything */
- if (superuser())
- return true;
-
- utup = SearchSysCache(AUTHOID,
- ObjectIdGetDatum(GetUserId()),
- 0, 0, 0);
- if (HeapTupleIsValid(utup))
- {
- result = ((Form_pg_authid) GETSTRUCT(utup))->rolcreatedb;
- ReleaseSysCache(utup);
- }
- return result;
- }
-
/*
* Remove tablespace directories
*
--- 1596,1601 ----
diff -Nrpc base/src/backend/commands/indexcmds.c ace_database/src/backend/commands/indexcmds.c
*** base/src/backend/commands/indexcmds.c Fri Dec 11 14:58:31 2009
--- ace_database/src/backend/commands/indexcmds.c Sun Dec 13 14:27:22 2009
***************
*** 39,44 ****
--- 39,45 ----
#include "parser/parse_func.h"
#include "parser/parse_oper.h"
#include "parser/parsetree.h"
+ #include "security/ace.h"
#include "storage/lmgr.h"
#include "storage/proc.h"
#include "storage/procarray.h"
*************** ReindexDatabase(const char *databaseName
*** 1534,1542 ****
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("can only reindex the currently open database")));
! if (!pg_database_ownercheck(MyDatabaseId, GetUserId()))
! aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
! databaseName);
/*
* Create a memory context that will survive forced transaction commits we
--- 1535,1542 ----
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("can only reindex the currently open database")));
! /* Permission checks */
! ace_database_reindex(MyDatabaseId);
/*
* Create a memory context that will survive forced transaction commits we
diff -Nrpc base/src/backend/security/Makefile ace_database/src/backend/security/Makefile
*** base/src/backend/security/Makefile Thu Jan 1 09:00:00 1970
--- ace_database/src/backend/security/Makefile Sun Dec 13 14:27:22 2009
***************
*** 0 ****
--- 1,10 ----
+ #
+ # Makefile for security features
+ #
+ subdir = src/backend/security
+ top_builddir = ../../..
+ include $(top_builddir)/src/Makefile.global
+
+ SUBDIRS = ace
+
+ include $(top_srcdir)/src/backend/common.mk
diff -Nrpc base/src/backend/security/ace/Makefile ace_database/src/backend/security/ace/Makefile
*** base/src/backend/security/ace/Makefile Thu Jan 1 09:00:00 1970
--- ace_database/src/backend/security/ace/Makefile Sun Dec 13 14:27:22 2009
***************
*** 0 ****
--- 1,11 ----
+ #
+ # Makefile for ACE framework
+ #
+ subdir = src/backend/security/ace
+ top_builddir = ../../../..
+ include $(top_builddir)/src/Makefile.global
+
+ OBJS = ace_misc.o ace_database.o
+
+ include $(top_srcdir)/src/backend/common.mk
+
diff -Nrpc base/src/backend/security/ace/ace_database.c ace_database/src/backend/security/ace/ace_database.c
*** base/src/backend/security/ace/ace_database.c Thu Jan 1 09:00:00 1970
--- ace_database/src/backend/security/ace/ace_database.c Sun Dec 13 14:27:22 2009
***************
*** 0 ****
--- 1,285 ----
+ /*
+ * ace_database.c
+ *
+ * security hooks related to database object class.
+ *
+ * Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ */
+ #include "postgres.h"
+
+ #include "catalog/pg_authid.h"
+ #include "catalog/pg_database.h"
+ #include "commands/dbcommands.h"
+ #include "commands/tablespace.h"
+ #include "miscadmin.h"
+ #include "security/ace.h"
+ #include "utils/syscache.h"
+
+ /* Check if current user has createdb privileges */
+ static bool
+ have_createdb_privilege(void)
+ {
+ bool result = false;
+ HeapTuple utup;
+
+ /* Superusers can always do everything */
+ if (superuser())
+ return true;
+
+ utup = SearchSysCache(AUTHOID,
+ ObjectIdGetDatum(GetUserId()),
+ 0, 0, 0);
+ if (HeapTupleIsValid(utup))
+ {
+ result = ((Form_pg_authid) GETSTRUCT(utup))->rolcreatedb;
+ ReleaseSysCache(utup);
+ }
+ return result;
+ }
+
+ /*
+ * ace_database_create
+ *
+ * It enables security providers to apply permission checks to create
+ * a new database.
+ *
+ * datName : Name of the new database
+ * srcDatOid : OID of the source database
+ * srcIsTemplate : True, if the source database is marked as template
+ * datOwner : OID of the new database owner
+ * datTblspc : OID of the default tablespace of the new database,
+ * if explicitly given. Otherwise, InvalidOid.
+ */
+ void
+ ace_database_create(const char *datName, Oid srcDatOid, bool srcIsTemplate,
+ Oid datOwner, Oid datTblspc)
+ {
+ AclResult aclresult;
+
+ /*
+ * To create a database, must have createdb privilege and must be able to
+ * become the target role (this does not imply that the target role itself
+ * must have createdb privilege). The latter provision guards against
+ * "giveaway" attacks. Note that a superuser will always have both of
+ * these privileges a fortiori.
+ */
+ if (!have_createdb_privilege())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to create database")));
+
+ check_is_member_of_role(GetUserId(), datOwner);
+
+ /*
+ * Permission check: to copy a DB that's not marked datistemplate, you
+ * must be superuser or the owner thereof.
+ */
+ if (!srcIsTemplate)
+ {
+ if (!pg_database_ownercheck(srcDatOid, GetUserId()))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to copy database \"%s\"",
+ get_database_name(srcDatOid))));
+ }
+
+ /*
+ * Check permissions to use a certain tablespace as the default
+ * tablespace in the new database
+ */
+ if (OidIsValid(datTblspc))
+ {
+ aclresult = pg_tablespace_aclcheck(datTblspc, GetUserId(),
+ ACL_CREATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_TABLESPACE,
+ get_tablespace_name(datTblspc));
+ }
+ }
+
+ /*
+ * ace_database_alter
+ *
+ * It enables security provides to check permission to alter properties
+ * of a certain database.
+ *
+ * datOid : OID of the database to be altered.
+ * newName : New name of the database, if given. Or, NULL.
+ * newTblspc : OID of the new defatult tablespace, if given. Or, InvalidOid.
+ * newOwner : OID of the new database owner, if given. Or, InvalidOid.
+ */
+ void
+ ace_database_alter(Oid datOid, const char *newName,
+ Oid newTblspc, Oid newOwner)
+ {
+ AclResult aclresult;
+
+ /* Must be owner for all the ALTER DATABASE options */
+ if (!pg_database_ownercheck(datOid, GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
+ get_database_name(datOid));
+
+ /* ALTER DATABASE ... RENAME TO */
+ if (newName)
+ {
+ /* Must have createdb right for renaming */
+ if (!have_createdb_privilege())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to rename database")));
+ }
+
+ /* ALTER DATABASE ... SET TABLESPACE */
+ if (OidIsValid(newTblspc))
+ {
+ /* Must have ACL_CREATE for the new default tablespace */
+ aclresult = pg_tablespace_aclcheck(newTblspc, GetUserId(),
+ ACL_CREATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_TABLESPACE,
+ get_tablespace_name(newTblspc));
+ }
+
+ /* ALTER DATABASE ... OWNER TO */
+ if (OidIsValid(newOwner))
+ {
+ /* Must be able to become new owner */
+ check_is_member_of_role(GetUserId(), newOwner);
+
+ /*
+ * must have createdb rights
+ *
+ * NOTE: This is different from other alter-owner checks in that the
+ * current user is checked for createdb privileges instead of the
+ * destination owner. This is consistent with the CREATE case for
+ * databases. Because superusers will always have this right, we need
+ * no special case for them.
+ */
+ if (!have_createdb_privilege())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied to change owner of database")));
+ }
+ }
+
+ /*
+ * ace_database_drop
+ *
+ * It enables security providers to check permission to drop a certain
+ * database.
+ *
+ * datOid : OID of the database to be dropped
+ * cascade : True, if cascaded deletion. Currently, it should never happen.
+ */
+ void
+ ace_database_drop(Oid datOid, bool cascade)
+ {
+ if (!cascade &&
+ !pg_database_ownercheck(datOid, GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
+ get_database_name(datOid));
+ }
+
+ /*
+ * ace_database_grant
+ *
+ * It enables security provides to check permission to grant/revoke
+ * privileges in the default PG model.
+ *
+ * datOid : OID of the database to be granted/revoked
+ * grantor : OID of the grantor role
+ * goptions : Available AclMask available to grant others
+ */
+ void
+ ace_database_grant(Oid datOid, Oid grantor, AclMode goptions)
+ {
+ /*
+ * If we found no grant options, consider whether to issue a hard
+ * error. Per spec, having any privilege at all on the object will
+ * get you by here.
+ */
+ if (goptions == ACL_NO_RIGHTS)
+ {
+ AclMode whole_mask = ACL_ALL_RIGHTS_DATABASE;
+
+ if (pg_database_aclmask(datOid, grantor,
+ whole_mask | ACL_GRANT_OPTION_FOR(whole_mask),
+ ACLMASK_ANY) == ACL_NO_RIGHTS)
+ aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_DATABASE,
+ get_database_name(datOid));
+ }
+ }
+
+ /*
+ * ace_database_comment
+ *
+ * It enables security provides to check permission to comment on
+ * the given database.
+ *
+ * datOid : OID of the database to be commented
+ */
+ void
+ ace_database_comment(Oid datOid)
+ {
+ if (!pg_database_ownercheck(datOid, GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
+ get_database_name(datOid));
+ }
+
+ /*
+ * ace_database_connect
+ *
+ * It enables security providers to check permission to connect to
+ * the given database on CheckMyDatabase()
+ *
+ * datOid : OID of the database to be connected
+ */
+ void
+ ace_database_connect(Oid datOid)
+ {
+ if (pg_database_aclcheck(datOid, GetUserId(),
+ ACL_CONNECT) != ACLCHECK_OK)
+ ereport(FATAL,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied for database \"%s\"",
+ get_database_name(datOid)),
+ errdetail("User does not have CONNECT privilege.")));
+ }
+
+ /*
+ * ace_database_reindex
+ *
+ * It enables security providers to check permissions to reindex
+ * all the tables withing the database
+ *
+ * datOid : OID of the database to be reindexed
+ */
+ void
+ ace_database_reindex(Oid datOid)
+ {
+ /* Must have ownership of the database */
+ if (!pg_database_ownercheck(datOid, GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
+ get_database_name(datOid));
+ }
+
+ /*
+ * ace_database_calculate_size
+ *
+ * It enables security providers to check permission to calculate
+ * a certain database size.
+ *
+ * datOid : OID of the database to be referenced
+ */
+ void
+ ace_database_calculate_size(Oid datOid)
+ {
+ AclResult aclresult;
+
+ /* User must have connect privilege for target database */
+ aclresult = pg_database_aclcheck(datOid, GetUserId(), ACL_CONNECT);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_DATABASE,
+ get_database_name(datOid));
+ }
diff -Nrpc base/src/backend/security/ace/ace_misc.c ace_database/src/backend/security/ace/ace_misc.c
*** base/src/backend/security/ace/ace_misc.c Thu Jan 1 09:00:00 1970
--- ace_database/src/backend/security/ace/ace_misc.c Sun Dec 13 14:27:22 2009
***************
*** 0 ****
--- 1,23 ----
+ /*
+ * ace_misc.c
+ *
+ * miscellaneous security hook routines
+ *
+ * Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ */
+ #include "postgres.h"
+
+ #include "security/ace.h"
+
+ /*
+ * ace_provider_initialize
+ *
+ * This hook allows security providers to initialize itself.
+ */
+ void
+ ace_provider_initialize(void)
+ {
+ /* initialize the default PG privs */
+ initialize_acl();
+ }
diff -Nrpc base/src/backend/utils/adt/dbsize.c ace_database/src/backend/utils/adt/dbsize.c
*** base/src/backend/utils/adt/dbsize.c Thu Jun 18 10:20:52 2009
--- ace_database/src/backend/utils/adt/dbsize.c Sun Dec 13 14:27:22 2009
***************
*** 21,26 ****
--- 21,27 ----
#include "commands/dbcommands.h"
#include "commands/tablespace.h"
#include "miscadmin.h"
+ #include "security/ace.h"
#include "storage/fd.h"
#include "utils/acl.h"
#include "utils/builtins.h"
*************** calculate_database_size(Oid dbOid)
*** 79,91 ****
struct dirent *direntry;
char dirpath[MAXPGPATH];
char pathname[MAXPGPATH];
- AclResult aclresult;
! /* User must have connect privilege for target database */
! aclresult = pg_database_aclcheck(dbOid, GetUserId(), ACL_CONNECT);
! if (aclresult != ACLCHECK_OK)
! aclcheck_error(aclresult, ACL_KIND_DATABASE,
! get_database_name(dbOid));
/* Shared storage in pg_global is not counted */
--- 80,88 ----
struct dirent *direntry;
char dirpath[MAXPGPATH];
char pathname[MAXPGPATH];
! /* Permission checks */
! ace_database_calculate_size(dbOid);
/* Shared storage in pg_global is not counted */
diff -Nrpc base/src/backend/utils/init/postinit.c ace_database/src/backend/utils/init/postinit.c
*** base/src/backend/utils/init/postinit.c Tue Oct 13 08:36:15 2009
--- ace_database/src/backend/utils/init/postinit.c Sun Dec 13 14:27:22 2009
***************
*** 36,41 ****
--- 36,42 ----
#include "pgstat.h"
#include "postmaster/autovacuum.h"
#include "postmaster/postmaster.h"
+ #include "security/ace.h"
#include "storage/bufmgr.h"
#include "storage/fd.h"
#include "storage/ipc.h"
*************** CheckMyDatabase(const char *name, bool a
*** 276,292 ****
name)));
/*
! * Check privilege to connect to the database. (The am_superuser test
! * is redundant, but since we have the flag, might as well check it
! * and save a few cycles.)
*/
! if (!am_superuser &&
! pg_database_aclcheck(MyDatabaseId, GetUserId(),
! ACL_CONNECT) != ACLCHECK_OK)
! ereport(FATAL,
! (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! errmsg("permission denied for database \"%s\"", name),
! errdetail("User does not have CONNECT privilege.")));
/*
* Check connection limit for this database.
--- 277,285 ----
name)));
/*
! * Check privilege to connect to the database.
*/
! ace_database_connect(MyDatabaseId);
/*
* Check connection limit for this database.
*************** InitPostgres(const char *in_dbname, Oid
*** 715,722 ****
am_superuser = superuser();
}
! /* set up ACL framework (so CheckMyDatabase can check permissions) */
! initialize_acl();
/* Process pg_db_role_setting options */
process_settings(MyDatabaseId, GetSessionUserId());
--- 708,715 ----
am_superuser = superuser();
}
! /* Set up ACE security providers */
! ace_provider_initialize();
/* Process pg_db_role_setting options */
process_settings(MyDatabaseId, GetSessionUserId());
diff -Nrpc base/src/include/security/ace.h ace_database/src/include/security/ace.h
*** base/src/include/security/ace.h Thu Jan 1 09:00:00 1970
--- ace_database/src/include/security/ace.h Sun Dec 13 14:27:22 2009
***************
*** 0 ****
--- 1,39 ----
+ /*
+ * security/ace.h
+ *
+ * Header file of the ACE framework
+ *
+ * Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ */
+ #ifndef SECURITY_ACE_H
+ #define SECURITY_ACE_H
+
+ #include "utils/acl.h"
+
+ /* ace_misc.c */
+ extern void
+ ace_provider_initialize(void);
+
+ /* ace_database.c */
+ extern void
+ ace_database_create(const char *datName,
+ Oid srcDatOid, bool srcIsTemplate,
+ Oid datOwner, Oid datTblspc);
+ extern void
+ ace_database_alter(Oid datOid, const char *newName,
+ Oid newTblspc, Oid newOwner);
+ extern void
+ ace_database_drop(Oid datOid, bool cascade);
+ extern void
+ ace_database_grant(Oid datOid, Oid grantor, AclMode goptions);
+ extern void
+ ace_database_comment(Oid datOid);
+ extern void
+ ace_database_connect(Oid datOid);
+ extern void
+ ace_database_reindex(Oid datOid);
+ extern void
+ ace_database_calculate_size(Oid datOid);
+
+ #endif /* SECURITY_ACE_H */
pgsql-ace-02-schema-8.5devel-r2475.patchtext/x-patch; name=pgsql-ace-02-schema-8.5devel-r2475.patchDownload
diff -Nrpc ace_database/src/backend/catalog/aclchk.c ace_schema/src/backend/catalog/aclchk.c
*** ace_database/src/backend/catalog/aclchk.c Sun Dec 13 14:27:22 2009
--- ace_schema/src/backend/catalog/aclchk.c Sun Dec 13 15:51:45 2009
*************** ExecGrant_Namespace(InternalGrant *istmt
*** 2752,2766 ****
&grantorId, &avail_goptions);
/*
* Restrict the privileges to what we can actually grant, and emit the
* standards-mandated warning and error messages.
*/
this_privileges =
! restrict_and_check_grant(istmt->is_grant, avail_goptions,
! istmt->all_privs, istmt->privileges,
! nspid, grantorId, ACL_KIND_NAMESPACE,
! NameStr(pg_namespace_tuple->nspname),
! 0, NULL);
/*
* Generate new ACL.
--- 2752,2771 ----
&grantorId, &avail_goptions);
/*
+ * If we found no grant options, consider whether to issue a hard
+ * error. Per spec, having any privilege at all on the object will
+ * get you by here.
+ */
+ ace_schema_grant(nspid, grantorId, avail_goptions);
+
+ /*
* Restrict the privileges to what we can actually grant, and emit the
* standards-mandated warning and error messages.
*/
this_privileges =
! restrict_grant(istmt->is_grant, avail_goptions,
! istmt->all_privs, istmt->privileges,
! NameStr(pg_namespace_tuple->nspname));
/*
* Generate new ACL.
diff -Nrpc ace_database/src/backend/catalog/namespace.c ace_schema/src/backend/catalog/namespace.c
*** ace_database/src/backend/catalog/namespace.c Mon Nov 9 18:42:41 2009
--- ace_schema/src/backend/catalog/namespace.c Sun Dec 13 15:51:45 2009
***************
*** 40,48 ****
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "parser/parse_func.h"
#include "storage/backendid.h"
#include "storage/ipc.h"
- #include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/guc.h"
#include "utils/inval.h"
--- 40,48 ----
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "parser/parse_func.h"
+ #include "security/ace.h"
#include "storage/backendid.h"
#include "storage/ipc.h"
#include "utils/builtins.h"
#include "utils/guc.h"
#include "utils/inval.h"
*************** Oid
*** 2332,2338 ****
LookupExplicitNamespace(const char *nspname)
{
Oid namespaceId;
- AclResult aclresult;
/* check for pg_temp alias */
if (strcmp(nspname, "pg_temp") == 0)
--- 2332,2337 ----
*************** LookupExplicitNamespace(const char *nspn
*** 2356,2365 ****
(errcode(ERRCODE_UNDEFINED_SCHEMA),
errmsg("schema \"%s\" does not exist", nspname)));
! aclresult = pg_namespace_aclcheck(namespaceId, GetUserId(), ACL_USAGE);
! if (aclresult != ACLCHECK_OK)
! aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
! nspname);
return namespaceId;
}
--- 2355,2362 ----
(errcode(ERRCODE_UNDEFINED_SCHEMA),
errmsg("schema \"%s\" does not exist", nspname)));
! /* Permission checks */
! ace_schema_search(namespaceId, true);
return namespaceId;
}
*************** LookupCreationNamespace(const char *nspn
*** 2397,2402 ****
--- 2394,2406 ----
(errcode(ERRCODE_UNDEFINED_SCHEMA),
errmsg("schema \"%s\" does not exist", nspname)));
+ /*
+ * XXX This permission check shall be moved to the caller side
+ * in the later patch which support tables, procs and types.
+ * In other word, ace_relation_alter(), ace_proc_alter() and
+ * ace_type_alter() handle this permission checks when these
+ * objects are altered.
+ */
aclresult = pg_namespace_aclcheck(namespaceId, GetUserId(), ACL_CREATE);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
*************** recomputeNamespacePath(void)
*** 2954,2961 ****
ReleaseSysCache(tuple);
if (OidIsValid(namespaceId) &&
!list_member_oid(oidlist, namespaceId) &&
! pg_namespace_aclcheck(namespaceId, roleid,
! ACL_USAGE) == ACLCHECK_OK)
oidlist = lappend_oid(oidlist, namespaceId);
}
}
--- 2958,2964 ----
ReleaseSysCache(tuple);
if (OidIsValid(namespaceId) &&
!list_member_oid(oidlist, namespaceId) &&
! ace_schema_search(namespaceId, false))
oidlist = lappend_oid(oidlist, namespaceId);
}
}
*************** recomputeNamespacePath(void)
*** 2982,2989 ****
0, 0, 0);
if (OidIsValid(namespaceId) &&
!list_member_oid(oidlist, namespaceId) &&
! pg_namespace_aclcheck(namespaceId, roleid,
! ACL_USAGE) == ACLCHECK_OK)
oidlist = lappend_oid(oidlist, namespaceId);
}
}
--- 2985,2991 ----
0, 0, 0);
if (OidIsValid(namespaceId) &&
!list_member_oid(oidlist, namespaceId) &&
! ace_schema_search(namespaceId, false))
oidlist = lappend_oid(oidlist, namespaceId);
}
}
*************** InitTempTableNamespace(void)
*** 3052,3076 ****
Assert(!OidIsValid(myTempNamespace));
- /*
- * First, do permission check to see if we are authorized to make temp
- * tables. We use a nonstandard error message here since "databasename:
- * permission denied" might be a tad cryptic.
- *
- * Note that ACL_CREATE_TEMP rights are rechecked in pg_namespace_aclmask;
- * that's necessary since current user ID could change during the session.
- * But there's no need to make the namespace in the first place until a
- * temp table creation request is made by someone with appropriate rights.
- */
- if (pg_database_aclcheck(MyDatabaseId, GetUserId(),
- ACL_CREATE_TEMP) != ACLCHECK_OK)
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to create temporary tables in database \"%s\"",
- get_database_name(MyDatabaseId))));
-
snprintf(namespaceName, sizeof(namespaceName), "pg_temp_%d", MyBackendId);
namespaceId = GetSysCacheOid(NAMESPACENAME,
CStringGetDatum(namespaceName),
0, 0, 0);
--- 3054,3064 ----
Assert(!OidIsValid(myTempNamespace));
snprintf(namespaceName, sizeof(namespaceName), "pg_temp_%d", MyBackendId);
+ /* Permission check to create a temporary schema */
+ ace_schema_create(namespaceName, BOOTSTRAP_SUPERUSERID, true);
+
namespaceId = GetSysCacheOid(NAMESPACENAME,
CStringGetDatum(namespaceName),
0, 0, 0);
diff -Nrpc ace_database/src/backend/commands/comment.c ace_schema/src/backend/commands/comment.c
*** ace_database/src/backend/commands/comment.c Sun Dec 13 14:27:22 2009
--- ace_schema/src/backend/commands/comment.c Sun Dec 13 15:51:45 2009
*************** CommentNamespace(List *qualname, char *c
*** 792,800 ****
errmsg("schema \"%s\" does not exist", namespace)));
/* Check object security */
! if (!pg_namespace_ownercheck(oid, GetUserId()))
! aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_NAMESPACE,
! namespace);
/* Call CreateComments() to create/drop the comments */
CreateComments(oid, NamespaceRelationId, 0, comment);
--- 792,798 ----
errmsg("schema \"%s\" does not exist", namespace)));
/* Check object security */
! ace_schema_comment(oid);
/* Call CreateComments() to create/drop the comments */
CreateComments(oid, NamespaceRelationId, 0, comment);
diff -Nrpc ace_database/src/backend/commands/schemacmds.c ace_schema/src/backend/commands/schemacmds.c
*** ace_database/src/backend/commands/schemacmds.c Fri Dec 11 14:58:31 2009
--- ace_schema/src/backend/commands/schemacmds.c Sun Dec 13 15:51:45 2009
***************
*** 25,30 ****
--- 25,31 ----
#include "commands/schemacmds.h"
#include "miscadmin.h"
#include "parser/parse_utilcmd.h"
+ #include "security/ace.h"
#include "tcop/utility.h"
#include "utils/acl.h"
#include "utils/builtins.h"
*************** CreateSchemaCommand(CreateSchemaStmt *st
*** 61,79 ****
else
owner_uid = saved_uid;
! /*
! * To create a schema, must have schema-create privilege on the current
! * database and must be able to become the target role (this does not
! * imply that the target role itself must have create-schema privilege).
! * The latter provision guards against "giveaway" attacks. Note that a
! * superuser will always have both of these privileges a fortiori.
! */
! aclresult = pg_database_aclcheck(MyDatabaseId, saved_uid, ACL_CREATE);
! if (aclresult != ACLCHECK_OK)
! aclcheck_error(aclresult, ACL_KIND_DATABASE,
! get_database_name(MyDatabaseId));
!
! check_is_member_of_role(saved_uid, owner_uid);
/* Additional check to protect reserved schema names */
if (!allowSystemTableMods && IsReservedName(schemaName))
--- 62,69 ----
else
owner_uid = saved_uid;
! /* Permission check to create a new schema */
! ace_schema_create(schemaName, owner_uid, false);
/* Additional check to protect reserved schema names */
if (!allowSystemTableMods && IsReservedName(schemaName))
*************** RemoveSchemas(DropStmt *drop)
*** 201,209 ****
}
/* Permission check */
! if (!pg_namespace_ownercheck(namespaceId, GetUserId()))
! aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_NAMESPACE,
! namespaceName);
object.classId = NamespaceRelationId;
object.objectId = namespaceId;
--- 191,197 ----
}
/* Permission check */
! ace_schema_drop(namespaceId, false);
object.classId = NamespaceRelationId;
object.objectId = namespaceId;
*************** RenameSchema(const char *oldname, const
*** 276,291 ****
(errcode(ERRCODE_DUPLICATE_SCHEMA),
errmsg("schema \"%s\" already exists", newname)));
! /* must be owner */
! if (!pg_namespace_ownercheck(HeapTupleGetOid(tup), GetUserId()))
! aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_NAMESPACE,
! oldname);
!
! /* must have CREATE privilege on database */
! aclresult = pg_database_aclcheck(MyDatabaseId, GetUserId(), ACL_CREATE);
! if (aclresult != ACLCHECK_OK)
! aclcheck_error(aclresult, ACL_KIND_DATABASE,
! get_database_name(MyDatabaseId));
if (!allowSystemTableMods && IsReservedName(newname))
ereport(ERROR,
--- 264,271 ----
(errcode(ERRCODE_DUPLICATE_SCHEMA),
errmsg("schema \"%s\" already exists", newname)));
! /* permission check to rename the schema */
! ace_schema_alter(HeapTupleGetOid(tup), newname, InvalidOid);
if (!allowSystemTableMods && IsReservedName(newname))
ereport(ERROR,
*************** AlterSchemaOwner_internal(HeapTuple tup,
*** 373,402 ****
Datum aclDatum;
bool isNull;
HeapTuple newtuple;
- AclResult aclresult;
-
- /* Otherwise, must be owner of the existing object */
- if (!pg_namespace_ownercheck(HeapTupleGetOid(tup), GetUserId()))
- aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_NAMESPACE,
- NameStr(nspForm->nspname));
! /* Must be able to become new owner */
! check_is_member_of_role(GetUserId(), newOwnerId);
!
! /*
! * must have create-schema rights
! *
! * NOTE: This is different from other alter-owner checks in that the
! * current user is checked for create privileges instead of the
! * destination owner. This is consistent with the CREATE case for
! * schemas. Because superusers will always have this right, we need
! * no special case for them.
! */
! aclresult = pg_database_aclcheck(MyDatabaseId, GetUserId(),
! ACL_CREATE);
! if (aclresult != ACLCHECK_OK)
! aclcheck_error(aclresult, ACL_KIND_DATABASE,
! get_database_name(MyDatabaseId));
memset(repl_null, false, sizeof(repl_null));
memset(repl_repl, false, sizeof(repl_repl));
--- 353,361 ----
Datum aclDatum;
bool isNull;
HeapTuple newtuple;
! /* Permission check to alter owner of the schema */
! ace_schema_alter(HeapTupleGetOid(tup), NULL, newOwnerId);
memset(repl_null, false, sizeof(repl_null));
memset(repl_repl, false, sizeof(repl_repl));
diff -Nrpc ace_database/src/backend/security/ace/Makefile ace_schema/src/backend/security/ace/Makefile
*** ace_database/src/backend/security/ace/Makefile Sun Dec 13 14:27:22 2009
--- ace_schema/src/backend/security/ace/Makefile Sun Dec 13 15:51:45 2009
*************** subdir = src/backend/security/ace
*** 5,11 ****
top_builddir = ../../../..
include $(top_builddir)/src/Makefile.global
! OBJS = ace_misc.o ace_database.o
include $(top_srcdir)/src/backend/common.mk
--- 5,11 ----
top_builddir = ../../../..
include $(top_builddir)/src/Makefile.global
! OBJS = ace_misc.o ace_database.o ace_schema.o
include $(top_srcdir)/src/backend/common.mk
diff -Nrpc ace_database/src/backend/security/ace/ace_schema.c ace_schema/src/backend/security/ace/ace_schema.c
*** ace_database/src/backend/security/ace/ace_schema.c Thu Jan 1 09:00:00 1970
--- ace_schema/src/backend/security/ace/ace_schema.c Sun Dec 13 19:10:51 2009
***************
*** 0 ****
--- 1,200 ----
+ /*
+ * ace_schema.c
+ *
+ * security hooks related to schema object class.
+ *
+ * Portions Copyright (c) 1996-2010, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ */
+ #include "postgres.h"
+
+ #include "miscadmin.h"
+ #include "security/ace.h"
+ #include "utils/lsyscache.h"
+ #include "utils/syscache.h"
+
+ /*
+ * ace_schema_create
+ *
+ * It enables security providers to apply permission checks to create
+ * a new schema object.
+ *
+ * nspName : Name of the new schema object
+ * nspOwner : OID of the new schema owner
+ * isTemp : True, if it is a temporary schema.
+ */
+ void
+ ace_schema_create(const char *nspName, Oid nspOwner, bool isTemp)
+ {
+ AclResult aclresult;
+
+ /*
+ * To create a schema, must have (temporary) schema-create privilege
+ * on the current database and must be able to become the target role
+ * (this does not imply that the target role itself must have create-schema
+ * privilege), if not temporary schema.
+ * The latter provision guards against "giveaway" attacks. Note that a
+ * superuser will always have both of these privileges a fortiori.
+ */
+ aclresult = pg_database_aclcheck(MyDatabaseId, GetUserId(),
+ isTemp ? ACL_CREATE_TEMP : ACL_CREATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_DATABASE,
+ get_database_name(MyDatabaseId));
+
+ if (!isTemp)
+ check_is_member_of_role(GetUserId(), nspOwner);
+ }
+
+ /*
+ * ace_schema_alter
+ *
+ * It enables security providers to apply permission checks to alter
+ * properties of a certain schema object.
+ *
+ * nspOid : OID of the schema to be altered
+ * newName : New name of the schema, if given. Or, NULL.
+ * newOwner : OID of the new owner, if given, Or, InvalidOid.
+ */
+ void
+ ace_schema_alter(Oid nspOid, const char *newName, Oid newOwner)
+ {
+ AclResult aclresult;
+
+ /* Must be owner for all the ALTER SCHEMA options */
+ if (!pg_namespace_ownercheck(nspOid, GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_NAMESPACE,
+ get_namespace_name(nspOid));
+
+ /* ALTER SCHEMA ... RENAME TO */
+ if (newName)
+ {
+ /* must have CREATE privilege on database to rename */
+ aclresult = pg_database_aclcheck(MyDatabaseId, GetUserId(),
+ ACL_CREATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_DATABASE,
+ get_database_name(MyDatabaseId));
+ }
+
+ /* ALTER SCHEMA ... OWNER TO */
+ if (OidIsValid(newOwner))
+ {
+ /* Must be able to become new owner */
+ check_is_member_of_role(GetUserId(), newOwner);
+
+ /*
+ * must have create-schema rights
+ *
+ * NOTE: This is different from other alter-owner checks in that the
+ * current user is checked for create privileges instead of the
+ * destination owner. This is consistent with the CREATE case for
+ * schemas. Because superusers will always have this right, we need
+ * no special case for them.
+ */
+ aclresult = pg_database_aclcheck(MyDatabaseId, GetUserId(),
+ ACL_CREATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult, ACL_KIND_DATABASE,
+ get_database_name(MyDatabaseId));
+ }
+ }
+
+ /*
+ * ace_schema_drop
+ *
+ * It enables security providers to apply permission checks to drop
+ * a certain schema obejct.
+ *
+ * nspOid : OID of the schema to be dropped
+ * cascade : True, if cascaded deletion.
+ */
+ void
+ ace_schema_drop(Oid nspOid, bool cascade)
+ {
+ if (!cascade &&
+ !pg_namespace_ownercheck(nspOid, GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_NAMESPACE,
+ get_namespace_name(nspOid));
+ }
+
+ /*
+ * ace_schema_grant
+ *
+ * It enables security provides to check permission to grant/revoke
+ * privileges in the default PG model.
+ *
+ * nspOid : OID of the schema to be granted/revoked
+ * grantor : OID of the grantor role
+ * goptions : Available AclMask available to grant others
+ */
+ void
+ ace_schema_grant(Oid nspOid, Oid grantor, AclMode goptions)
+ {
+ if (goptions == ACL_NO_RIGHTS)
+ {
+ /*
+ * If we found no grant options, consider whether to issue a hard
+ * error. Per spec, having any privilege at all on the object will
+ * get you by here.
+ */
+ AclMode whole_mask = ACL_ALL_RIGHTS_NAMESPACE;
+
+ if (pg_namespace_aclmask(nspOid, grantor,
+ whole_mask | ACL_GRANT_OPTION_FOR(whole_mask),
+ ACLMASK_ANY) == ACL_NO_RIGHTS)
+ aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_DATABASE,
+ get_namespace_name(nspOid));
+ }
+ }
+
+ /*
+ * ace_schema_search
+ *
+ * It enables security provides to check permission to search database
+ * objects under a certain schema.
+ *
+ * Note that we handles "pg_temp" schema as an exception.
+ * It is indeed a schema in fact, and in implementation. but it is an
+ * internal details from the perspective of users.
+ * Any security providers launched from this hook shall always return
+ * 'true' on the temporary schema. Even if it tries to apply access
+ * controls on temporary schema, this hook is not called when the schema
+ * is obviously temporary one.
+ *
+ * nspOid : OID of the schema to be searched
+ * abort : True, if the caller want to raise an error on violation.
+ */
+ bool
+ ace_schema_search(Oid nspOid, bool abort)
+ {
+ AclResult aclresult;
+
+ aclresult = pg_namespace_aclcheck(nspOid, GetUserId(),
+ ACL_USAGE);
+ if (aclresult != ACLCHECK_OK)
+ {
+ if (abort)
+ aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
+ get_namespace_name(nspOid));
+ return false;
+ }
+
+ return true;
+ }
+
+ /*
+ * ace_schema_comment
+ *
+ * It enables security provides to check permission to comment on
+ * a certain schema object.
+ *
+ * nspOid : OID of the schema to be commented
+ */
+ void
+ ace_schema_comment(Oid nspOid)
+ {
+ if (!pg_namespace_ownercheck(nspOid, GetUserId()))
+ aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_NAMESPACE,
+ get_namespace_name(nspOid));
+ }
diff -Nrpc ace_database/src/backend/tcop/fastpath.c ace_schema/src/backend/tcop/fastpath.c
*** ace_database/src/backend/tcop/fastpath.c Sat Jan 3 13:01:35 2009
--- ace_schema/src/backend/tcop/fastpath.c Sun Dec 13 15:51:45 2009
***************
*** 26,31 ****
--- 26,32 ----
#include "libpq/pqformat.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
+ #include "security/ace.h"
#include "tcop/fastpath.h"
#include "tcop/tcopprot.h"
#include "utils/acl.h"
*************** HandleFunctionRequest(StringInfo msgBuf)
*** 339,348 ****
* Check permission to access and call function. Since we didn't go
* through a normal name lookup, we need to check schema usage too.
*/
! aclresult = pg_namespace_aclcheck(fip->namespace, GetUserId(), ACL_USAGE);
! if (aclresult != ACLCHECK_OK)
! aclcheck_error(aclresult, ACL_KIND_NAMESPACE,
! get_namespace_name(fip->namespace));
aclresult = pg_proc_aclcheck(fid, GetUserId(), ACL_EXECUTE);
if (aclresult != ACLCHECK_OK)
--- 340,346 ----
* Check permission to access and call function. Since we didn't go
* through a normal name lookup, we need to check schema usage too.
*/
! ace_schema_search(fip->namespace, true);
aclresult = pg_proc_aclcheck(fid, GetUserId(), ACL_EXECUTE);
if (aclresult != ACLCHECK_OK)
diff -Nrpc ace_database/src/include/security/ace.h ace_schema/src/include/security/ace.h
*** ace_database/src/include/security/ace.h Sun Dec 13 14:27:22 2009
--- ace_schema/src/include/security/ace.h Sun Dec 13 15:51:45 2009
*************** ace_database_reindex(Oid datOid);
*** 36,39 ****
--- 36,53 ----
extern void
ace_database_calculate_size(Oid datOid);
+ /* ace_schema.c */
+ extern void
+ ace_schema_create(const char *nspName, Oid nspOwner, bool isTemp);
+ extern void
+ ace_schema_alter(Oid nspOid, const char *newName, Oid newOwner);
+ extern void
+ ace_schema_drop(Oid nspOid, bool cascade);
+ extern void
+ ace_schema_grant(Oid nspOid, Oid grantor, AclMode goptions);
+ extern bool
+ ace_schema_search(Oid nspOid, bool abort);
+ extern void
+ ace_schema_comment(Oid nspOid);
+
#endif /* SECURITY_ACE_H */
2009/12/13 KaiGai Kohei <kaigai@kaigai.gr.jp>:
The previous patch is too large to review.
Is this scale confortable to review?
The scale is fine. But the content is not. So I am faced with a bit
of a dilemma. If I start enumerating specific reasons why it's not
OK, then it's going to take more time than I really want to put into
this project. If I don't, then I may be accused of hiding the ball.
What I'm hoping is that there are other interested people on this
mailing list (or not on this mailing list, maybe in the security
community) who have the time and the ability to help you fix some of
the issues here so that we can then have a serious design discussion.
Just to name a few really obvious problems (I only looked at the
01-database patch):
1. We have been talking for several days about the need to make the
initial patch in this area strictly a code cleanup patch. Is this
cleaner than the code that it is replacing? Is it even making an
attempt to conform to that mandate?
2. What will happen when someone runs pgindent against this?
Perhaps you only intended this to be a starting point for discussion,
in which case that's fine, but I don't think I can really contribute
anything useful until it gets a little further along.
Thanks,
...Robert
I read through the database patch a little more. Here are some
further thoughts.
ace_database_create() calls have_createdb_privilege(), but of course
what ace_database_create() is supposed to be checking is whether you
have privileges to create the DB. This is extremely confusing. The
calling sequence for ace_database_create() seems to indicate a failure
of abstraction. The srcIsTemplate argument seems quite problematic.
It is one of many values that are returned by get_db_info(), and it's
passed to ace_database_create() because the current PG model happens
to care. It seems like a near-certainty that future security models
will care about something else. (Incidentally, the interface to the
existing get_db_info function seems to me to be rather poor. By the
time we get up to 10 out parameters, I think we ought to be using a
structure. This might be material for a standalone patch.)
I don't understand the modifications to restrict_and_check_grant().
It seems to me that this is going in the wrong direction, although
since I don't understand the motivation for the change, I might be
wrong. If we have a piece of code that centralizes permissions
checking now, splitting that up into a bunch of ace_*_grant()
functions and duplicating it doesn't seem like an improvement.
ace_database_alter() appears to never be called with more than one
argument that is non-NULL/InvalidOid, which is usually a sign that the
interface is wrong. There are two calls to this function where,
apparently, we're checking alter database permissions without
specifying any details whatsoever about exactly what's going to get
altered. That sounds like we don't really know what things we want to
pass across the interface layer, so we're just passing the ones that
we happen to care about at the moment. It certainly looks like we
need separate functions for alter, rename, and alter-set. In some
cases it might make sense to pass the parsenode as an argument rather
than trying to decide which bits of data contained within it are
sufficiently interesting to be worth sending over.
Tom objected to ace_database_calculate_size() before. It seems to me
that in order to keep this managable we have to settle on a finite set
of permissions and stick with it. For example, in this case, we might
decide that in EVERY security model, calculating the database size
requires the same permissions as connect. The individual security
models might want to make different decisions about how to check that
permission, but they all have to treat it the same way they would
treat an actual connection attempt. It seems to me that if every
security model is allowed to introduce not only new logic for making
checks but also new kinds of checks, we might as well give up on
designing an interface layer.
(Similarly, skipping back to ace_database_create() for a minute ...
since I asked for a patch that only deals with one object type, I
won't now complain about having gotten one. But I will note that I
think where ace_database_create() calls pg_tablespace_aclchk() it
probably eventually needs to call some ace_tablespace_...() function.
Although frankly I'm not sure which one. Would
ace_tablespace_create() mean permission to create a tablespace or
permission to create tables within that tablespace?)
security/ace.h, like a good number of other things in this patch, does
not conform to project indentation style. All of the parameter blocks
(parameter : description) don't either; unless I'm mistaken, pgindent
will do something awful to those. The comments generally need some
editing help from a native English speaker, and I spotted at least two
typos (defatult for default, provides for providers). They also make
reference to multiple security providers, which is not within the
purview of this patch.
I am not very happy with the ace_<object-type>_<operation> naming
convention. Most PG functions are named a little more descriptively
than that. Like I can pretty much guess that AlterDatabase() is going
to alter a database, and get_db_info() is going to get info about a
DB, and ExecHashJoin() is going to execute a hash join. It's not
possible to look at ace_database_create() and have any idea what the
function does, unless you have the preexisting knowledge that ACE
stands for "access control extensions", and then you still don't know
what the function does because "access control extensions database
create" is a sentence with no verb. Granted, we have some poorly
named functions in the system already, but I'd like to not increase
the number. Apart from all of the above, I still believe that for a
first phase of this we should just be looking to centralize access
control decisions without promising to ever do anything more, and
"access control extensions" doesn't send that message. I don't know
exactly what the best way to structure this is but I think putting the
word "check" somewhere in there would be a good start (or maybe the
already-used-elsewhere abbreviation "chk").
Again, I want to provide what help I can here, but this is a massive
project, so help is really needed.
...Robert
Robert, Thanks for your detailed comments.
Just to name a few really obvious problems (I only looked at the
01-database patch):1. We have been talking for several days about the need to make the
initial patch in this area strictly a code cleanup patch. Is this
cleaner than the code that it is replacing? Is it even making an
attempt to conform to that mandate?
Even if it is unclear whether the current form is more clear than the
current inlined pg_xxx_aclcheck() form, or not, it will obviously
provide a set of common entry points for upcoming enhanced security
providers.
Eventually, it is more clear than enumeration of #ifdef ... #endif
blocks for SELinux, Smack, Solaris-TX and others.
2. What will happen when someone runs pgindent against this?
Perhaps you only intended this to be a starting point for discussion,
in which case that's fine, but I don't think I can really contribute
anything useful until it gets a little further along.
If someone runs pgindent, I'll have to fix up a series of patches
mechanically to resolve conflictions, if necessary.
Indeed, this framework does not provide any additional user visible
features by itself, but necessary to accept upcoming anything useful.
Robert Haas wrote:
I read through the database patch a little more. Here are some
further thoughts.ace_database_create() calls have_createdb_privilege(), but of course
what ace_database_create() is supposed to be checking is whether you
have privileges to create the DB. This is extremely confusing. The
calling sequence for ace_database_create() seems to indicate a failure
of abstraction. The srcIsTemplate argument seems quite problematic.
It is one of many values that are returned by get_db_info(), and it's
passed to ace_database_create() because the current PG model happens
to care. It seems like a near-certainty that future security models
will care about something else. (Incidentally, the interface to the
existing get_db_info function seems to me to be rather poor. By the
time we get up to 10 out parameters, I think we ought to be using a
structure. This might be material for a standalone patch.)
The have_createdb_privilege() is a copy & paste from the dbcommand.c.
It lookups AUTHOID syscache and returns pg_authid.rolcreatedb, but
indeed, its name is confusable. I'll consider another name.
The srcIsTemplate might be pulled out from DATABASEOID syscache
using the given srcDatOid. I guess the reason why get_db_info()
is it tries to resolve dbname -> db_oid and fetch its properties
in same time. But OID of the source database is already given,
so security provider (including the default PG) can lookup this
as neccessity.
I don't understand the modifications to restrict_and_check_grant().
It seems to me that this is going in the wrong direction, although
since I don't understand the motivation for the change, I might be
wrong. If we have a piece of code that centralizes permissions
checking now, splitting that up into a bunch of ace_*_grant()
functions and duplicating it doesn't seem like an improvement.
From perspective of the enhanced security providers, GRANT/REVOKE is
a kind of modification of properties on a certain database object.
So, it has a motivation to check GRANT/REVOKE using its access control
rules.
If we don't modify restrict_and_check_grant(), the caller (aclchk.c)
correctly handles the default PG checks, so rest of enhanced security
providers will apply additional checks in the ace_*_grant().
In this case, we keep ace_*_grant() empty at the moment, and it shall
be a special purpose hook for enhanced security providers.
I don't have any preference on either of them.
ace_database_alter() appears to never be called with more than one
argument that is non-NULL/InvalidOid, which is usually a sign that the
interface is wrong. There are two calls to this function where,
apparently, we're checking alter database permissions without
specifying any details whatsoever about exactly what's going to get
altered. That sounds like we don't really know what things we want to
pass across the interface layer, so we're just passing the ones that
we happen to care about at the moment. It certainly looks like we
need separate functions for alter, rename, and alter-set. In some
cases it might make sense to pass the parsenode as an argument rather
than trying to decide which bits of data contained within it are
sufficiently interesting to be worth sending over.
It makes sense for me. I'll separate it as follows:
- ace_database_alter_rename() ... ALTER DATABASE xxx RENAME TO
- ace_database_alter_owner() ... ALTER DATABASE xxx OWNER TO
- ace_database_alter() ... ALTER DATABASE with other options
When we support security label on database, it will also necessary:
- ace_database_alter_relabel()
Tom objected to ace_database_calculate_size() before. It seems to me
that in order to keep this managable we have to settle on a finite set
of permissions and stick with it. For example, in this case, we might
decide that in EVERY security model, calculating the database size
requires the same permissions as connect. The individual security
models might want to make different decisions about how to check that
permission, but they all have to treat it the same way they would
treat an actual connection attempt. It seems to me that if every
security model is allowed to introduce not only new logic for making
checks but also new kinds of checks, we might as well give up on
designing an interface layer.
It againsts to the purpose of the framework. It provides a common set
of access controls for multiple security providers, but it does not
enforce the way to make its access control decision.
It seems to me ace_database_calculate_size() might be a bad name
because of too specific naming for a certin functionality.
We can also consider this hook checks permission to obtain attributes
of a certain database.
So, we can also generalize it as follows:
void ace_database_getattr(Oid datOid);
It checks permission to reference attributes (not contents) of a certain
database. The database size is a part of attribute, and the default PG
model checks ACL_CONNECT permission for the purpose.
Any other upcoming enhanced security provider may make its decision based
on different basis. But there is no matter.
(Similarly, skipping back to ace_database_create() for a minute ...
since I asked for a patch that only deals with one object type, I
won't now complain about having gotten one. But I will note that I
think where ace_database_create() calls pg_tablespace_aclchk() it
probably eventually needs to call some ace_tablespace_...() function.
Although frankly I'm not sure which one. Would
ace_tablespace_create() mean permission to create a tablespace or
permission to create tables within that tablespace?)
ace_tablespace_create() means permission to create a tablespace itself.
It shall be checked on CREATE TABLESPACE statement, not an alternative
of pg_tablespace_aclcheck(..., ACL_CREATE).
In the default PG model, it makes its access control decision based on
the following rules:
- Current database user must has pg_authid.rolsuper or pg_authid.rolcreatedb
- If not superuser, current database user must have membership of the
owner of the new database.
- If source database is not template, the current database user must
have ownership of the new database.
- If an explicit tablespace is given, the current database user must
have CREATE privilege on the target tablespace.
It is the way to make access control decision in the default PG model.
However, any other enhanced security providers can apply thier rules.
E.g, SE-PgSQL tries to assign a default security label on a new database,
and checks permission to create a new database (db_database:{create})
between the client and the database with this label.
This framework provides a set of common entrypoints for various kind of
security providers, but does not enforce a certain security model.
security/ace.h, like a good number of other things in this patch, does
not conform to project indentation style. All of the parameter blocks
(parameter : description) don't either; unless I'm mistaken, pgindent
will do something awful to those. The comments generally need some
editing help from a native English speaker, and I spotted at least two
typos (defatult for default, provides for providers). They also make
reference to multiple security providers, which is not within the
purview of this patch.
I'll pay my efforts as possible as I can.
I am not very happy with the ace_<object-type>_<operation> naming
convention. Most PG functions are named a little more descriptively
than that. Like I can pretty much guess that AlterDatabase() is going
to alter a database, and get_db_info() is going to get info about a
DB, and ExecHashJoin() is going to execute a hash join. It's not
possible to look at ace_database_create() and have any idea what the
function does, unless you have the preexisting knowledge that ACE
stands for "access control extensions", and then you still don't know
what the function does because "access control extensions database
create" is a sentence with no verb. Granted, we have some poorly
named functions in the system already, but I'd like to not increase
the number. Apart from all of the above, I still believe that for a
first phase of this we should just be looking to centralize access
control decisions without promising to ever do anything more, and
"access control extensions" doesn't send that message. I don't know
exactly what the best way to structure this is but I think putting the
word "check" somewhere in there would be a good start (or maybe the
already-used-elsewhere abbreviation "chk").
OK. Indeed, it is not a verb.
However, the "check" does not means by itself what should be checked.
It may be permission, integrity, syntax or others.
What about "acechk_*" to mean access control extension checks <something>
to be <action>'ed? For example, acechk_database_create() means "access
control extension checks <database> to be <created>".
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
2009/12/13 KaiGai Kohei <kaigai@ak.jp.nec.com>:
Just to name a few really obvious problems (I only looked at the
01-database patch):1. We have been talking for several days about the need to make the
initial patch in this area strictly a code cleanup patch. Is this
cleaner than the code that it is replacing? Is it even making an
attempt to conform to that mandate?Even if it is unclear whether the current form is more clear than the
current inlined pg_xxx_aclcheck() form, or not, it will obviously
provide a set of common entry points for upcoming enhanced security
providers.
Eventually, it is more clear than enumeration of #ifdef ... #endif
blocks for SELinux, Smack, Solaris-TX and others.
Right, but it will also not get committed. :-(
You seem to still be failing to understand the approach that Stephen
and I have been discussing for the last several days... but at any
rate, I think that it is possible to do this in a way that is more
clear (or at least, AS clear) as the current method. Stephen is also
working on a concept patch that I'm eager to see, and I would like to
give him a little time to get that together before we spend too much
more time working on this.
I don't understand the modifications to restrict_and_check_grant().
It seems to me that this is going in the wrong direction, although
since I don't understand the motivation for the change, I might be
wrong. If we have a piece of code that centralizes permissions
checking now, splitting that up into a bunch of ace_*_grant()
functions and duplicating it doesn't seem like an improvement.From perspective of the enhanced security providers, GRANT/REVOKE is
a kind of modification of properties on a certain database object.
So, it has a motivation to check GRANT/REVOKE using its access control
rules.
If we don't modify restrict_and_check_grant(), the caller (aclchk.c)
correctly handles the default PG checks, so rest of enhanced security
providers will apply additional checks in the ace_*_grant().
In this case, we keep ace_*_grant() empty at the moment, and it shall
be a special purpose hook for enhanced security providers.
I don't have any preference on either of them.
[...]
When we support security label on database, it will also necessary:
- ace_database_alter_relabel()
This seems like bad design to me. I don't see why SE-Linux would get
a vote on whether a user is allowed to change the DAC permissions, any
more than DAC gets a vote on whether an SE-Linux relabel operation can
go through. If it does, then this attempt to create a framework is
not going to succeed, because every new provider will require hooks to
let every exist provider muck with what it does, and they'll all have
to have complete knowledge of each other's internals.
It seems to me ace_database_calculate_size() might be a bad name
because of too specific naming for a certin functionality.
That's exactly the problem. I'm not sure if I believe in the
particular solution you've offered here, but it's certainly better
than the way it is now. I think this needs more thought from more
people.
What about "acechk_*" to mean access control extension checks <something>
to be <action>'ed? For example, acechk_database_create() means "access
control extension checks <database> to be <created>".
That fails to address all of my comments, so, -1 from me.
...Robert
Robert Haas wrote:
2009/12/13 KaiGai Kohei <kaigai@ak.jp.nec.com>:
Just to name a few really obvious problems (I only looked at the
01-database patch):1. We have been talking for several days about the need to make the
initial patch in this area strictly a code cleanup patch. Is this
cleaner than the code that it is replacing? Is it even making an
attempt to conform to that mandate?Even if it is unclear whether the current form is more clear than the
current inlined pg_xxx_aclcheck() form, or not, it will obviously
provide a set of common entry points for upcoming enhanced security
providers.
Eventually, it is more clear than enumeration of #ifdef ... #endif
blocks for SELinux, Smack, Solaris-TX and others.Right, but it will also not get committed. :-(
The framework will be necessary to get them committed.
Which is an egg, and which is a chicken? :-(
You seem to still be failing to understand the approach that Stephen
and I have been discussing for the last several days... but at any
rate, I think that it is possible to do this in a way that is more
clear (or at least, AS clear) as the current method. Stephen is also
working on a concept patch that I'm eager to see, and I would like to
give him a little time to get that together before we spend too much
more time working on this.
Hmm... In my hope, I'd like to work on these patches during the Christmas
vacation terms for US people. So, it seems to me we need to decide the
direction in this week.
Stephen, sorry for the prod.
I don't understand the modifications to restrict_and_check_grant().
It seems to me that this is going in the wrong direction, although
since I don't understand the motivation for the change, I might be
wrong. If we have a piece of code that centralizes permissions
checking now, splitting that up into a bunch of ace_*_grant()
functions and duplicating it doesn't seem like an improvement.From perspective of the enhanced security providers, GRANT/REVOKE is
a kind of modification of properties on a certain database object.
So, it has a motivation to check GRANT/REVOKE using its access control
rules.
If we don't modify restrict_and_check_grant(), the caller (aclchk.c)
correctly handles the default PG checks, so rest of enhanced security
providers will apply additional checks in the ace_*_grant().
In this case, we keep ace_*_grant() empty at the moment, and it shall
be a special purpose hook for enhanced security providers.
I don't have any preference on either of them.[...]
When we support security label on database, it will also necessary:
- ace_database_alter_relabel()
This seems like bad design to me. I don't see why SE-Linux would get
a vote on whether a user is allowed to change the DAC permissions, any
more than DAC gets a vote on whether an SE-Linux relabel operation can
go through. If it does, then this attempt to create a framework is
not going to succeed, because every new provider will require hooks to
let every exist provider muck with what it does, and they'll all have
to have complete knowledge of each other's internals.
From the DAC perspective, a relabel operation is setting an attribute
of a certain database, such as ALTER DATABASE ... SET xxx. So, it is
quite natural to check ownership of the target database like other
ALTER DATABASE stuff, not only SELinux relabeling checks.
I intend to use the *_alter_relabel() hook for any other label-based
security providers, not only SELinux. The caller, syntax support and
the default PG checks don't need to know details in any other security
providers. (It is also reason why I prefer one enhanced provider at
the same time, because they can share syntax support such as SECURITY
LABEL ('...') option.)
In my image, it shall be implemented as follows:
ALTER DATABASE <datname> SECURITY LABEL ( '<new label>' );
|
V
Value *
ace_database_alter_relabel(Oid datOid, Value *newLabel)
{
if (pg_database_ownercheck(datOid, GetUserId())
aclcheck_error(...);
switch (ace_provider)
{
#if HAVE_SELINUX
case ACE_PROVIDER_SELINUX:
if (sepgsql_is_enabled())
return sepgsql_database_alter_relabel(...);
break;
#endif
#if HAVE_SMACK
case ACE_PROVIDER_SMACK:
if (smack_is_enabled())
return smack_database_alter_relabel(...);
break;
#endif
default:
break;
}
ereport(ERROR, "No label-based MAC is now enabled");
return NULL;
}
|
V
The caller update pg_database system catalog with the returned
security label.
However, it is not necessary to conclude at the moment.
We can add it later.
It seems to me ace_database_calculate_size() might be a bad name
because of too specific naming for a certin functionality.That's exactly the problem. I'm not sure if I believe in the
particular solution you've offered here, but it's certainly better
than the way it is now. I think this needs more thought from more
people.
From my previous large patch, the following functions might have similar
arguments:
* ac_relation_get_transaction_id()
The currtid_byreloid() and currtid_byrelname() are the only caller, and
checks ACL_SELECT permission on the given relation.
We can consider its purpose is to check references to attribute of
a certain relation.
* ac_relation_copy_definition()
The transformInhRelation() is the only caller (name is confusable because
it handles LIKE clause in CREATE TABLE statement), and checks ACL_SELECT
permission on the given relation.
We can consider its purpose is to check references to attribute of
a certain relation.
-> These two checks apply identical permissions for similar purpose from
more generalized perspective.
It seems to me these can be replaced by *_relation_getattr().
* ac_database_calculate_size()
The calculate_database_size() is the only caller, and checks ACL_CONNECT
permission on the given database.
We can consider its purpose is to return users attribute of a certain
database.
-> ditto, can be replaced by *_database_getattr()
* ac_tablespace_calculate_size()
The calculate_tablespace_size() is the only caller, and checks ACL_CREAET
permission on the given tablespace.
We can consider its purpose is to return users attribute of a certain
tablespace.
-> ditto, can be replaced by *_tablespace_getattr()
What about "acechk_*" to mean access control extension checks <something>
to be <action>'ed? For example, acechk_database_create() means "access
control extension checks <database> to be <created>".That fails to address all of my comments, so, -1 from me.
OK, should be check_<object class>_<action>()?
Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai@ak.jp.nec.com>
2009/12/14 KaiGai Kohei <kaigai@ak.jp.nec.com>:
Robert Haas wrote:
2009/12/13 KaiGai Kohei <kaigai@ak.jp.nec.com>:
Just to name a few really obvious problems (I only looked at the
01-database patch):1. We have been talking for several days about the need to make the
initial patch in this area strictly a code cleanup patch. Is this
cleaner than the code that it is replacing? Is it even making an
attempt to conform to that mandate?Even if it is unclear whether the current form is more clear than the
current inlined pg_xxx_aclcheck() form, or not, it will obviously
provide a set of common entry points for upcoming enhanced security
providers.
Eventually, it is more clear than enumeration of #ifdef ... #endif
blocks for SELinux, Smack, Solaris-TX and others.Right, but it will also not get committed. :-(
The framework will be necessary to get them committed.
Which is an egg, and which is a chicken? :-(
We've been around that path a few times, but that's not my point here.
Doing the framework first makes a lot of sense; the problem is that
we just had a design discussion regarding that framework and you've
chosen to do something other than what was discussed. Did you not
read that discussion? Did you not understand it?
Unfortunately, what this project has turned into is a very long series
of patch submissions that all basically have the same problems. The
code is messy. It doesn't conform to project style. It embeds
undiscussed design assumptions that the community does not endorse.
It has poorly factored interfaces that may serve SE-PostgreSQL
adequately for the moment but are likely to require constant
rejiggering as new problems arise. It is filled with shims that don't
accomplish anything useful and other places where useful shims are
left out.
Now, it may be that even if you respond to all of the comments and fix
all of the issues that people are concerned about, the patch still
won't get committed. As you know, Tom is very skeptical that the
user-base for this feature is large enough to warrant the work that it
will take. But my point is that you seem to be very deliberately not
fixing those problems, and I don't see how we're going to make any
progress that way. Many of the problems that I found in my review of
this patch were covered in design discussions that happened this week,
and yet the patch shows almost no evidence of those design
discussions.
Frankly, I find that kind of depressing.
...Robert
(2009/12/14 20:48), Robert Haas wrote:
2009/12/14 KaiGai Kohei<kaigai@ak.jp.nec.com>:
Robert Haas wrote:
2009/12/13 KaiGai Kohei<kaigai@ak.jp.nec.com>:
Just to name a few really obvious problems (I only looked at the
01-database patch):1. We have been talking for several days about the need to make the
initial patch in this area strictly a code cleanup patch. Is this
cleaner than the code that it is replacing? Is it even making an
attempt to conform to that mandate?Even if it is unclear whether the current form is more clear than the
current inlined pg_xxx_aclcheck() form, or not, it will obviously
provide a set of common entry points for upcoming enhanced security
providers.
Eventually, it is more clear than enumeration of #ifdef ... #endif
blocks for SELinux, Smack, Solaris-TX and others.Right, but it will also not get committed. :-(
The framework will be necessary to get them committed.
Which is an egg, and which is a chicken? :-(We've been around that path a few times, but that's not my point here.
Doing the framework first makes a lot of sense; the problem is that
we just had a design discussion regarding that framework and you've
chosen to do something other than what was discussed. Did you not
read that discussion? Did you not understand it?
Please point out, if my understanding is incorrect from the discussion
in a few days.
* As a draft of the discussion, I have to split out the access control
reworks patch in the 2nd CF per object classes.
* This framework supports only the default PG privileges at the moment.
* The way to host enhanced security providers are not decided.
(Maybe #ifdef ... #endif block, Maybe function pointer)
* It is not decided how many security labels are assigned on a database
object. (Maybe 1:1, Maybe 1:n)
I don't intend to go to something undecided, but, might understand
something incorrectly or not be able to follow the discussion enough.
Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
On Mon, Dec 14, 2009 at 7:30 AM, KaiGai Kohei <kaigai@kaigai.gr.jp> wrote:
(2009/12/14 20:48), Robert Haas wrote:
2009/12/14 KaiGai Kohei<kaigai@ak.jp.nec.com>:
Robert Haas wrote:
2009/12/13 KaiGai Kohei<kaigai@ak.jp.nec.com>:
Just to name a few really obvious problems (I only looked at the
01-database patch):1. We have been talking for several days about the need to make the
initial patch in this area strictly a code cleanup patch. Is this
cleaner than the code that it is replacing? Is it even making an
attempt to conform to that mandate?Even if it is unclear whether the current form is more clear than the
current inlined pg_xxx_aclcheck() form, or not, it will obviously
provide a set of common entry points for upcoming enhanced security
providers.
Eventually, it is more clear than enumeration of #ifdef ... #endif
blocks for SELinux, Smack, Solaris-TX and others.Right, but it will also not get committed. :-(
The framework will be necessary to get them committed.
Which is an egg, and which is a chicken? :-(We've been around that path a few times, but that's not my point here.
Doing the framework first makes a lot of sense; the problem is that
we just had a design discussion regarding that framework and you've
chosen to do something other than what was discussed. Did you not
read that discussion? Did you not understand it?Please point out, if my understanding is incorrect from the discussion
in a few days.* As a draft of the discussion, I have to split out the access control
reworks patch in the 2nd CF per object classes.
* This framework supports only the default PG privileges at the moment.
* The way to host enhanced security providers are not decided.
(Maybe #ifdef ... #endif block, Maybe function pointer)
* It is not decided how many security labels are assigned on a database
object. (Maybe 1:1, Maybe 1:n)I don't intend to go to something undecided, but, might understand
something incorrectly or not be able to follow the discussion enough.
Hmm... all of those things are true, but it seems to leave quite a bit out.
...Robert
(2009/12/14 21:34), Robert Haas wrote:
On Mon, Dec 14, 2009 at 7:30 AM, KaiGai Kohei<kaigai@kaigai.gr.jp> wrote:
(2009/12/14 20:48), Robert Haas wrote:
2009/12/14 KaiGai Kohei<kaigai@ak.jp.nec.com>:
Robert Haas wrote:
2009/12/13 KaiGai Kohei<kaigai@ak.jp.nec.com>:
Just to name a few really obvious problems (I only looked at the
01-database patch):1. We have been talking for several days about the need to make the
initial patch in this area strictly a code cleanup patch. Is this
cleaner than the code that it is replacing? Is it even making an
attempt to conform to that mandate?Even if it is unclear whether the current form is more clear than the
current inlined pg_xxx_aclcheck() form, or not, it will obviously
provide a set of common entry points for upcoming enhanced security
providers.
Eventually, it is more clear than enumeration of #ifdef ... #endif
blocks for SELinux, Smack, Solaris-TX and others.Right, but it will also not get committed. :-(
The framework will be necessary to get them committed.
Which is an egg, and which is a chicken? :-(We've been around that path a few times, but that's not my point here.
Doing the framework first makes a lot of sense; the problem is that
we just had a design discussion regarding that framework and you've
chosen to do something other than what was discussed. Did you not
read that discussion? Did you not understand it?Please point out, if my understanding is incorrect from the discussion
in a few days.* As a draft of the discussion, I have to split out the access control
reworks patch in the 2nd CF per object classes.
* This framework supports only the default PG privileges at the moment.
* The way to host enhanced security providers are not decided.
(Maybe #ifdef ... #endif block, Maybe function pointer)
* It is not decided how many security labels are assigned on a database
object. (Maybe 1:1, Maybe 1:n)I don't intend to go to something undecided, but, might understand
something incorrectly or not be able to follow the discussion enough.Hmm... all of those things are true, but it seems to leave quite a bit out.
Since I had to look many messages in a day, my concentration for each
messages might not be enough. I'll try to check it again.
At least, I don't intend to ignore our discussion.
--
KaiGai Kohei <kaigai@kaigai.gr.jp>