Stephen, >> The 'failure' may make an impression of generic errors not only permission >> denied. >> How about 'error_on_violation'? > > Maybe 'ereport_on_violation'? I dunno, guess one isn't really better > than the other. You need to go back and fix the comment though- you > still say 'abort' there.
I have no preference between 'error_on_violation' and 'ereport_on_violation'. OK, I fixed it. >> BTW, I wonder whether acl.h is a correct place to explain about the hook, >> although I added comments for the hook. > > Guess I don't really see a problem putting the comments there. By the > way, have we got a place where we actually document the hooks we support > somewhere in the official documentation..? If so, that should certainly > be updated too.. I could not find Executor hooks from doc/src/sgml using grep. If so, it might be worth to list them on the wikipage. >> I think we should add a series of explanation about ESP hooks in the internal >> section of the documentation, when the number of hooks reaches a dozen for >> example. > > I believe the goal will be to avoid reaching a dozen hooks for this. Maybe, all we need to hook on DML permissions is only this one. > All-in-all, I'm pretty happy with these. Couple minor places which > could use some copy editing, but that's about it. > > Next, we need to get the security label catalog and the grammar to > support it implemented and then from that an SELinux module should > be pretty easy to implement. Based on the discussions at PGCon, Robert > is working on the security label catalog and grammar. The current plan > is to have a catalog similar to pg_depend, to minimize impact to the > rest of the backend and to those who aren't interested in using security > labels. Pg_depend? not pg_description/pg_shdescription? I basically agree with the idea that minimizes damages to the existing schema of system catalogs, but I cannot imagine something like pg_depend well. I'd like to post a new thread to discuss the security label support. OK? > Of course, there will also need to be hooks there for an > external module to enforce restrictions associated with changing labels > on various objects in the system. Yes, the user given has to be validated by ESP. Thanks, -- KaiGai Kohei <kai...@ak.jp.nec.com>
*** a/src/backend/catalog/aclchk.c --- b/src/backend/catalog/aclchk.c *************** *** 135,140 **** static AclMode pg_aclmask(AclObjectKind objkind, Oid table_oid, AttrNumber attnu --- 135,144 ---- static AclResult check_rte_privileges(RangeTblEntry *rte, bool ereport_on_violation); + /* + * External security provider hooks + */ + check_relation_privileges_hook_type check_relation_privileges_hook = NULL; #ifdef ACLDEBUG static void *************** *** 4730,4735 **** get_user_default_acl(GrantObjectType objtype, Oid ownerId, Oid nsp_oid) --- 4734,4742 ---- * It checks access permissions for all relations listed in a range table. * If violated, it raises an error or returns false depending on the 'abort' * argument. + * It also invokes an external security provide to check the permissions. + * If it is available, both of the default PG checks and external checks + * have to allow the required accesses for the relations. */ AclResult check_relation_privileges(List *rangeTable, bool ereport_on_violation) *************** *** 4746,4751 **** check_relation_privileges(List *rangeTable, bool ereport_on_violation) --- 4753,4763 ---- if (retval != ACLCHECK_OK) return retval; } + + /* External security provider invocation */ + if (check_relation_privileges_hook) + retval = (*check_relation_privileges_hook)(rangeTable, + ereport_on_violation); return retval; } *** a/src/backend/executor/execMain.c --- b/src/backend/executor/execMain.c *************** *** 466,471 **** InitPlan(QueryDesc *queryDesc, int eflags) --- 466,475 ---- * check anything for other RTE types (function, join, subquery, ...). * Function RTEs are checked by init_fcache when the function is prepared * for execution. Join, subquery, and special RTEs need no checks. + * + * If available, it also invokes an external security provider. In this + * case, both of the default PG checks and the external checks have to + * allow the required accesses on the relation */ check_relation_privileges(rangeTable, true); *** a/src/include/utils/acl.h --- b/src/include/utils/acl.h *************** *** 196,201 **** typedef enum AclObjectKind --- 196,216 ---- MAX_ACL_KIND /* MUST BE LAST */ } AclObjectKind; + /* + * External security provider hooks + */ + + /* + * check_relation_privileges_hook + * It allows an ESP to get control at check_relation_privileges(). + * A list of RangeTblEntry to be referenced and a flag to inform preferable + * bahavior on access violations. + * The ESP shall return ACLCHECK_OK if it allows the required access. + * Elsewhere, it raises an error or return other AclResult status depending + * on the given flag. + */ + typedef AclResult (*check_relation_privileges_hook_type)(List *, bool); + extern PGDLLIMPORT check_relation_privileges_hook_type check_relation_privileges_hook; /* * routines used internally
*** a/src/backend/catalog/aclchk.c --- b/src/backend/catalog/aclchk.c *************** *** 133,138 **** static AclMode restrict_and_check_grant(bool is_grant, AclMode avail_goptions, --- 133,140 ---- static AclMode pg_aclmask(AclObjectKind objkind, Oid table_oid, AttrNumber attnum, Oid roleid, AclMode mask, AclMaskHow how); + static AclResult check_rte_privileges(RangeTblEntry *rte, bool ereport_on_violation); + #ifdef ACLDEBUG static void *************** *** 4722,4724 **** get_user_default_acl(GrantObjectType objtype, Oid ownerId, Oid nsp_oid) --- 4724,4923 ---- return result; } + + /* + * check_relation_privileges + * It checks access permissions for all relations listed in a range table. + * If violated, it raises an error or returns false depending on the 'abort' + * argument. + */ + AclResult + check_relation_privileges(List *rangeTable, bool ereport_on_violation) + { + ListCell *l; + RangeTblEntry *rte; + AclResult retval = ACLCHECK_OK; + + foreach(l, rangeTable) + { + rte = (RangeTblEntry *) lfirst(l); + + retval = check_rte_privileges(rte, ereport_on_violation); + if (retval != ACLCHECK_OK) + return retval; + } + return retval; + } + + /* + * check_rte_privileges + * This is only used for checking plain relation permissions, by + * check_relation_privileges(), nothing else shall be checked here. + */ + static AclResult + check_rte_privileges(RangeTblEntry *rte, bool ereport_on_violation) + { + AclMode requiredPerms; + AclMode relPerms; + AclMode remainingPerms; + Oid relOid; + Oid userid; + Bitmapset *tmpset; + int col; + + /* + * Only plain-relation RTEs need to be checked here. + */ + if (rte->rtekind != RTE_RELATION) + return ACLCHECK_OK; + + /* + * No work if requiredPerms is empty. + */ + requiredPerms = rte->requiredPerms; + if (requiredPerms == 0) + return ACLCHECK_OK; + + relOid = rte->relid; + + /* + * userid to check as: current user unless we have a setuid indication. + * + * Note: GetUserId() is presently fast enough that there's no harm in + * calling it separately for each RTE. If that stops being true, we could + * call it once in chkpriv_relation_perms and pass the userid down from + * there. But for now, no need for the extra clutter. + */ + userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); + + /* + * We must have *all* the requiredPerms bits, but some of the bits can be + * satisfied from column-level rather than relation-level permissions. + * First, remove any bits that are satisfied by relation permissions. + */ + relPerms = pg_class_aclmask(relOid, userid, requiredPerms, ACLMASK_ALL); + remainingPerms = requiredPerms & ~relPerms; + if (remainingPerms != 0) + { + /* + * If we lack any permissions that exist only as relation permissions, + * we can fail straight away. + */ + if (remainingPerms & ~(ACL_SELECT | ACL_INSERT | ACL_UPDATE)) + { + if (ereport_on_violation) + aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS, + get_rel_name(relOid)); + return ACLCHECK_NO_PRIV; + } + + /* + * Check to see if we have the needed privileges at column level. + * + * Note: failures just report a table-level error; it would be nicer + * to report a column-level error if we have some but not all of the + * column privileges. + */ + if (remainingPerms & ACL_SELECT) + { + /* + * When the query doesn't explicitly reference any columns (for + * example, SELECT COUNT(*) FROM table), allow the query if we + * have SELECT on any column of the rel, as per SQL spec. + */ + if (bms_is_empty(rte->selectedCols)) + { + if (pg_attribute_aclcheck_all(relOid, userid, ACL_SELECT, + ACLMASK_ANY) != ACLCHECK_OK) + { + if (ereport_on_violation) + aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS, + get_rel_name(relOid)); + return ACLCHECK_NO_PRIV; + } + } + + tmpset = bms_copy(rte->selectedCols); + while ((col = bms_first_member(tmpset)) >= 0) + { + /* remove the column number offset */ + col += FirstLowInvalidHeapAttributeNumber; + if (col == InvalidAttrNumber) + { + /* Whole-row reference, must have priv on all cols */ + if (pg_attribute_aclcheck_all(relOid, userid, ACL_SELECT, + ACLMASK_ALL) != ACLCHECK_OK) + { + if (ereport_on_violation) + aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS, + get_rel_name(relOid)); + return ACLCHECK_NO_PRIV; + } + } + else + { + if (pg_attribute_aclcheck(relOid, col, userid, + ACL_SELECT) != ACLCHECK_OK) + { + if (ereport_on_violation) + aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS, + get_rel_name(relOid)); + return ACLCHECK_NO_PRIV; + } + } + } + bms_free(tmpset); + } + + /* + * Basically the same for the mod columns, with either INSERT or + * UPDATE privilege as specified by remainingPerms. + */ + remainingPerms &= ~ACL_SELECT; + if (remainingPerms != 0) + { + /* + * When the query doesn't explicitly change any columns, allow the + * query if we have permission on any column of the rel. This is + * to handle SELECT FOR UPDATE as well as possible corner cases in + * INSERT and UPDATE. + */ + if (bms_is_empty(rte->modifiedCols)) + { + if (pg_attribute_aclcheck_all(relOid, userid, remainingPerms, + ACLMASK_ANY) != ACLCHECK_OK) + { + if (ereport_on_violation) + aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS, + get_rel_name(relOid)); + return ACLCHECK_NO_PRIV; + } + } + + tmpset = bms_copy(rte->modifiedCols); + while ((col = bms_first_member(tmpset)) >= 0) + { + /* remove the column number offset */ + col += FirstLowInvalidHeapAttributeNumber; + if (col == InvalidAttrNumber) + { + /* whole-row reference can't happen here */ + elog(ERROR, "whole-row update is not implemented"); + } + else + { + if (pg_attribute_aclcheck(relOid, col, userid, + remainingPerms) != ACLCHECK_OK) + { + if (ereport_on_violation) + aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS, + get_rel_name(relOid)); + return ACLCHECK_NO_PRIV; + } + } + } + bms_free(tmpset); + } + } + return ACLCHECK_OK; + } *** a/src/backend/commands/copy.c --- b/src/backend/commands/copy.c *************** *** 21,26 **** --- 21,27 ---- #include <arpa/inet.h> #include "access/heapam.h" + #include "access/sysattr.h" #include "access/xact.h" #include "catalog/namespace.h" #include "catalog/pg_type.h" *************** *** 726,733 **** DoCopy(const CopyStmt *stmt, const char *queryString) bool force_quote_all = false; bool format_specified = false; AclMode required_access = (is_from ? ACL_INSERT : ACL_SELECT); - AclMode relPerms; - AclMode remainingPerms; ListCell *option; TupleDesc tupDesc; int num_phys_attrs; --- 727,732 ---- *************** *** 988,993 **** DoCopy(const CopyStmt *stmt, const char *queryString) --- 987,996 ---- if (stmt->relation) { + RangeTblEntry *rte; + List *attnums; + ListCell *cur; + Assert(!stmt->query); cstate->queryDesc = NULL; *************** *** 997,1025 **** DoCopy(const CopyStmt *stmt, const char *queryString) tupDesc = RelationGetDescr(cstate->rel); ! /* Check relation permissions. */ ! relPerms = pg_class_aclmask(RelationGetRelid(cstate->rel), GetUserId(), ! required_access, ACLMASK_ALL); ! remainingPerms = required_access & ~relPerms; ! if (remainingPerms != 0) ! { ! /* We don't have table permissions, check per-column permissions */ ! List *attnums; ! ListCell *cur; ! attnums = CopyGetAttnums(tupDesc, cstate->rel, attnamelist); ! foreach(cur, attnums) ! { ! int attnum = lfirst_int(cur); ! if (pg_attribute_aclcheck(RelationGetRelid(cstate->rel), ! attnum, ! GetUserId(), ! remainingPerms) != ACLCHECK_OK) ! aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS, ! RelationGetRelationName(cstate->rel)); ! } } /* check read-only transaction */ if (XactReadOnly && is_from && !cstate->rel->rd_islocaltemp) --- 1000,1025 ---- tupDesc = RelationGetDescr(cstate->rel); ! /* ! * Check relation permissions. ! * We built an RTE with the relation and columns to be accessed ! * to check for necessary privileges in the common way. ! */ ! rte = makeRangeTblEntry(RTE_RELATION); ! rte->relid = RelationGetRelid(cstate->rel); ! rte->requiredPerms = required_access; ! attnums = CopyGetAttnums(tupDesc, cstate->rel, attnamelist); ! foreach (cur, attnums) ! { ! int attnum = lfirst_int(cur) - FirstLowInvalidHeapAttributeNumber; ! if (is_from) ! rte->modifiedCols = bms_add_member(rte->modifiedCols, attnum); ! else ! rte->selectedCols = bms_add_member(rte->selectedCols, attnum); } + check_relation_privileges(list_make1(rte), true); /* check read-only transaction */ if (XactReadOnly && is_from && !cstate->rel->rd_islocaltemp) *** a/src/backend/executor/execMain.c --- b/src/backend/executor/execMain.c *************** *** 72,79 **** static void ExecutePlan(EState *estate, PlanState *planstate, long numberTuples, ScanDirection direction, DestReceiver *dest); - static void ExecCheckRTPerms(List *rangeTable); - static void ExecCheckRTEPerms(RangeTblEntry *rte); static void ExecCheckXactReadOnly(PlannedStmt *plannedstmt); static void EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree); --- 72,77 ---- *************** *** 402,572 **** ExecutorRewind(QueryDesc *queryDesc) MemoryContextSwitchTo(oldcontext); } - - /* - * ExecCheckRTPerms - * Check access permissions for all relations listed in a range table. - */ - static void - ExecCheckRTPerms(List *rangeTable) - { - ListCell *l; - - foreach(l, rangeTable) - { - ExecCheckRTEPerms((RangeTblEntry *) lfirst(l)); - } - } - - /* - * ExecCheckRTEPerms - * Check access permissions for a single RTE. - */ - static void - ExecCheckRTEPerms(RangeTblEntry *rte) - { - AclMode requiredPerms; - AclMode relPerms; - AclMode remainingPerms; - Oid relOid; - Oid userid; - Bitmapset *tmpset; - int col; - - /* - * Only plain-relation RTEs need to be checked here. Function RTEs are - * checked by init_fcache when the function is prepared for execution. - * Join, subquery, and special RTEs need no checks. - */ - if (rte->rtekind != RTE_RELATION) - return; - - /* - * No work if requiredPerms is empty. - */ - requiredPerms = rte->requiredPerms; - if (requiredPerms == 0) - return; - - relOid = rte->relid; - - /* - * userid to check as: current user unless we have a setuid indication. - * - * Note: GetUserId() is presently fast enough that there's no harm in - * calling it separately for each RTE. If that stops being true, we could - * call it once in ExecCheckRTPerms and pass the userid down from there. - * But for now, no need for the extra clutter. - */ - userid = rte->checkAsUser ? rte->checkAsUser : GetUserId(); - - /* - * We must have *all* the requiredPerms bits, but some of the bits can be - * satisfied from column-level rather than relation-level permissions. - * First, remove any bits that are satisfied by relation permissions. - */ - relPerms = pg_class_aclmask(relOid, userid, requiredPerms, ACLMASK_ALL); - remainingPerms = requiredPerms & ~relPerms; - if (remainingPerms != 0) - { - /* - * If we lack any permissions that exist only as relation permissions, - * we can fail straight away. - */ - if (remainingPerms & ~(ACL_SELECT | ACL_INSERT | ACL_UPDATE)) - aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS, - get_rel_name(relOid)); - - /* - * Check to see if we have the needed privileges at column level. - * - * Note: failures just report a table-level error; it would be nicer - * to report a column-level error if we have some but not all of the - * column privileges. - */ - if (remainingPerms & ACL_SELECT) - { - /* - * When the query doesn't explicitly reference any columns (for - * example, SELECT COUNT(*) FROM table), allow the query if we - * have SELECT on any column of the rel, as per SQL spec. - */ - if (bms_is_empty(rte->selectedCols)) - { - if (pg_attribute_aclcheck_all(relOid, userid, ACL_SELECT, - ACLMASK_ANY) != ACLCHECK_OK) - aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS, - get_rel_name(relOid)); - } - - tmpset = bms_copy(rte->selectedCols); - while ((col = bms_first_member(tmpset)) >= 0) - { - /* remove the column number offset */ - col += FirstLowInvalidHeapAttributeNumber; - if (col == InvalidAttrNumber) - { - /* Whole-row reference, must have priv on all cols */ - if (pg_attribute_aclcheck_all(relOid, userid, ACL_SELECT, - ACLMASK_ALL) != ACLCHECK_OK) - aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS, - get_rel_name(relOid)); - } - else - { - if (pg_attribute_aclcheck(relOid, col, userid, ACL_SELECT) - != ACLCHECK_OK) - aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS, - get_rel_name(relOid)); - } - } - bms_free(tmpset); - } - - /* - * Basically the same for the mod columns, with either INSERT or - * UPDATE privilege as specified by remainingPerms. - */ - remainingPerms &= ~ACL_SELECT; - if (remainingPerms != 0) - { - /* - * When the query doesn't explicitly change any columns, allow the - * query if we have permission on any column of the rel. This is - * to handle SELECT FOR UPDATE as well as possible corner cases in - * INSERT and UPDATE. - */ - if (bms_is_empty(rte->modifiedCols)) - { - if (pg_attribute_aclcheck_all(relOid, userid, remainingPerms, - ACLMASK_ANY) != ACLCHECK_OK) - aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS, - get_rel_name(relOid)); - } - - tmpset = bms_copy(rte->modifiedCols); - while ((col = bms_first_member(tmpset)) >= 0) - { - /* remove the column number offset */ - col += FirstLowInvalidHeapAttributeNumber; - if (col == InvalidAttrNumber) - { - /* whole-row reference can't happen here */ - elog(ERROR, "whole-row update is not implemented"); - } - else - { - if (pg_attribute_aclcheck(relOid, col, userid, remainingPerms) - != ACLCHECK_OK) - aclcheck_error(ACLCHECK_NO_PRIV, ACL_KIND_CLASS, - get_rel_name(relOid)); - } - } - bms_free(tmpset); - } - } - } - /* * Check that the query does not imply any writes to non-temp tables. * --- 400,405 ---- *************** *** 628,636 **** InitPlan(QueryDesc *queryDesc, int eflags) int i; /* ! * Do permissions checks */ ! ExecCheckRTPerms(rangeTable); /* * initialize the node's execution state --- 461,473 ---- int i; /* ! * Do permissions checks. The check_relation_privileges() checks access ! * permissions for all relations listed in a range table, but does not ! * check anything for other RTE types (function, join, subquery, ...). ! * Function RTEs are checked by init_fcache when the function is prepared ! * for execution. Join, subquery, and special RTEs need no checks. */ ! check_relation_privileges(rangeTable, true); /* * initialize the node's execution state *** a/src/backend/utils/adt/ri_triggers.c --- b/src/backend/utils/adt/ri_triggers.c *************** *** 30,35 **** --- 30,36 ---- #include "postgres.h" + #include "access/sysattr.h" #include "access/xact.h" #include "catalog/pg_constraint.h" #include "catalog/pg_operator.h" *************** *** 39,44 **** --- 40,46 ---- #include "parser/parse_coerce.h" #include "parser/parse_relation.h" #include "miscadmin.h" + #include "nodes/makefuncs.h" #include "utils/acl.h" #include "utils/builtins.h" #include "utils/fmgroids.h" *************** *** 2624,2629 **** RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) --- 2626,2633 ---- char fkrelname[MAX_QUOTED_REL_NAME_LEN]; char pkattname[MAX_QUOTED_NAME_LEN + 3]; char fkattname[MAX_QUOTED_NAME_LEN + 3]; + RangeTblEntry *pkrte; + RangeTblEntry *fkrte; const char *sep; int i; int old_work_mem; *************** *** 2632,2649 **** RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel) SPIPlanPtr qplan; /* * Check to make sure current user has enough permissions to do the test * query. (If not, caller can fall back to the trigger method, which * works because it changes user IDs on the fly.) * * XXX are there any other show-stopper conditions to check? */ - if (pg_class_aclcheck(RelationGetRelid(fk_rel), GetUserId(), ACL_SELECT) != ACLCHECK_OK) - return false; - if (pg_class_aclcheck(RelationGetRelid(pk_rel), GetUserId(), ACL_SELECT) != ACLCHECK_OK) - return false; ! ri_FetchConstraintInfo(&riinfo, trigger, fk_rel, false); /*---------- * The query string built is: --- 2636,2678 ---- SPIPlanPtr qplan; /* + * Set up RI_ConstraintInfo + */ + ri_FetchConstraintInfo(&riinfo, trigger, fk_rel, false); + + /* * Check to make sure current user has enough permissions to do the test * query. (If not, caller can fall back to the trigger method, which * works because it changes user IDs on the fly.) * * XXX are there any other show-stopper conditions to check? */ ! /* ! * We built a pair of RTEs of FK/PK relations and columns referenced ! * in the test query to check necessary permission in the common way. ! */ ! pkrte = makeRangeTblEntry(RTE_RELATION); ! pkrte->relid = RelationGetRelid(pk_rel); ! pkrte->requiredPerms = ACL_SELECT; ! ! fkrte = makeRangeTblEntry(RTE_RELATION); ! fkrte->relid = RelationGetRelid(fk_rel); ! fkrte->requiredPerms = ACL_SELECT; ! ! for (i = 0; i < riinfo.nkeys; i++) ! { ! int attno; ! ! attno = riinfo.pk_attnums[i] - FirstLowInvalidHeapAttributeNumber; ! pkrte->selectedCols = bms_add_member(pkrte->selectedCols, attno); ! ! attno = riinfo.fk_attnums[i] - FirstLowInvalidHeapAttributeNumber; ! fkrte->selectedCols = bms_add_member(fkrte->selectedCols, attno); ! } ! ! if (check_relation_privileges(list_make2(fkrte, pkrte), false) != ACLCHECK_OK) ! return false; /*---------- * The query string built is: *** a/src/include/utils/acl.h --- b/src/include/utils/acl.h *************** *** 311,314 **** extern bool pg_ts_dict_ownercheck(Oid dict_oid, Oid roleid); --- 311,319 ---- extern bool pg_ts_config_ownercheck(Oid cfg_oid, Oid roleid); extern bool pg_foreign_server_ownercheck(Oid srv_oid, Oid roleid); + /* + * security checker functions + */ + extern AclResult check_relation_privileges(List *rangeTable, bool ereport_on_violation); + #endif /* ACL_H */
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers