OK thanks, I will change the comment then but keep it since it was not at all clear to me...
> -----Original Message----- > From: Anders Bjornerstedt [mailto:[email protected]] > Sent: den 26 september 2014 11:28 > To: Hans Feldt > Cc: [email protected] > Subject: Re: [devel] [PATCH 1 of 1] immnd: allow service object modifications > for same group [#1136] > > Ack with comment inlined below. > Only tested for non-interference. > Not tested for effectiveness. > > Hans Feldt wrote: > > osaf/services/saf/immsv/immnd/immnd_evt.c | 67 > > ++++++++++++++++++++++-------- > > 1 files changed, 48 insertions(+), 19 deletions(-) > > > > > > SMF running as non root cannot change saImmRepositoryInit which is a normal > > upgrade use case. Same for changing longDnsAllowed but which is one shot > > change. > > > > Reason is IMM now only allows root (uid=0) to modify service objects. > > > > This is changed so that members of the same group as immnd, for example smfd > > is allowed to change the mentioned attributes. Change of access control > > related > > attributes is still restricted to root. > > > > diff --git a/osaf/services/saf/immsv/immnd/immnd_evt.c > > b/osaf/services/saf/immsv/immnd/immnd_evt.c > > --- a/osaf/services/saf/immsv/immnd/immnd_evt.c > > +++ b/osaf/services/saf/immsv/immnd/immnd_evt.c > > @@ -38,7 +38,7 @@ > > #define IMMND_SEARCH_BUNDLE_SIZE ((MDS_DIRECT_BUF_MAXSIZE / 100) * 90) > > #define IMMND_MAX_SEARCH_RESULT (IMMND_SEARCH_BUNDLE_SIZE / 300) > > > > -static SaAisErrorT immnd_fevs_local_checks(IMMND_CB *cb, IMMSV_FEVS > > *fevsReq, uid_t uid); > > +static SaAisErrorT immnd_fevs_local_checks(IMMND_CB *cb, IMMSV_FEVS > > *fevsReq, const IMMSV_SEND_INFO *sinfo); > > static uint32_t immnd_evt_proc_cb_dump(IMMND_CB *cb); > > static uint32_t immnd_evt_proc_imm_init(IMMND_CB *cb, IMMND_EVT *evt, > > IMMSV_SEND_INFO *sinfo, SaBoolT isOm); > > static uint32_t immnd_evt_proc_imm_finalize(IMMND_CB *cb, IMMND_EVT *evt, > > IMMSV_SEND_INFO *sinfo, SaBoolT isOm); > > @@ -2835,7 +2835,7 @@ static uint32_t immnd_evt_proc_fevs_forw > > } > > > > if(newMsg) { > > - error = immnd_fevs_local_checks(cb, &(evt->info.fevsReq), > > (sinfo)?(sinfo->uid):0); > > + error = immnd_fevs_local_checks(cb, &(evt->info.fevsReq), > > sinfo); > > if(error != SA_AIS_OK) { > > /*Fevs request will NOT be forwarded to IMMD. > > Return directly with error or OK for idempotent > > requests. > > @@ -3048,7 +3048,8 @@ static uint32_t immnd_evt_proc_fevs_forw > > nodes and not propagated over fevs, because sync clients may not yet > > have synced the implementer setting and thus reject the idempotent case. > > */ > > -static SaAisErrorT immnd_fevs_local_checks(IMMND_CB *cb, IMMSV_FEVS > > *fevsReq, uid_t uid) > > +static SaAisErrorT immnd_fevs_local_checks(IMMND_CB *cb, IMMSV_FEVS > > *fevsReq, > > + const IMMSV_SEND_INFO *sinfo) > > { > > SaAisErrorT error = SA_AIS_OK; > > osafassert(fevsReq); > > @@ -3101,27 +3102,55 @@ static SaAisErrorT immnd_fevs_local_chec > > goto unpack_failure; > > } > > > > + /* Some checks below use sinfo. On the node where the request > > originated, > > + * the sinfo pointer is valid. On other nodes that receives the same > > through > > + * FEVS, sinfo is NULL. So the value of sinfo can be used if checks are > > to > > + * be done only on the originating node. > > + */ > > > The fevs_local_checks (this function) is only invoked at the node where > the request originated. > So the part in the above comment about "done only on the originating > node" does not really make sense. > The problem is rather that asynchronous requests dont have sinfo. > So access control can not be done for asynchronous requests. > There only relevant one I can think of is the asynchronous > admin-operation.-invoke. > But we dont perform access control for the synchronous variant of > admin-op (yet) so it should not > be a problem (yet). > > /AndersBj > > > + > > switch (frwrd_evt.info.immnd.type) { > > > > case IMMND_EVT_A2ND_OBJ_MODIFY: > > - if((strcmp(frwrd_evt.info.immnd.info.objModify.objectName.buf, > > OPENSAF_IMM_OBJECT_DN) == 0) || > > - > > (strcmp(frwrd_evt.info.immnd.info.objModify.objectName.buf, > "safRdn=immManagement,safApp=safImmService") == 0)) > > - { > > - /* Modifications to: > > - opensafImm=opensafImm,safApp=safImmService > > - or: > > - safRdn=immManagement,safApp=safImmService > > - are only allowed for root users. > > + if ((sinfo != NULL) && > > + > > ((strcmp(frwrd_evt.info.immnd.info.objModify.objectName.buf, > > + OPENSAF_IMM_OBJECT_DN) == 0) || > > + > > (strcmp(frwrd_evt.info.immnd.info.objModify.objectName.buf, > > + "safRdn=immManagement,safApp=safImmService") == > > 0))) { > > + /* Modifications to IMM service objects are only > > allowed for root > > + * users and same group as me. Except for access > > control settings > > + * which are only allowed by root. > > */ > > - if(uid) { > > - struct passwd *pwd = getpwuid(uid); > > + if ((sinfo->uid > 0) && (sinfo->gid != getgid())) { > > + struct passwd *pwd = getpwuid(sinfo->uid); > > if (pwd != NULL) { > > - syslog(LOG_AUTH, "Modifications to imm > > service objects denied for %s(uid=%d)", > > - pwd->pw_name, uid); > > + syslog(LOG_AUTH, > > + "Modifications to imm service > > objects denied for %s(uid=%d)", > > + pwd->pw_name, sinfo->uid); > > } > > error = SA_AIS_ERR_ACCESS_DENIED; > > goto done; > > - } > > + } else if (sinfo->uid > 0) { > > + // non root and same group as me, disallow > > access control changes > > + const IMMSV_ATTR_MODS_LIST *attrMod = > > + > > frwrd_evt.info.immnd.info.objModify.attrMods; > > + while (attrMod != NULL) { > > + if > > ((strcmp(attrMod->attrValue.attrName.buf, > > + > > OPENSAF_IMM_ACCESS_CONTROL_MODE) == 0) || > > + > > (strcmp(attrMod->attrValue.attrName.buf, > > + > > OPENSAF_IMM_AUTHORIZED_GROUP) == 0)) { > > + struct passwd *pwd = > > getpwuid(sinfo->uid); > > + if (pwd != NULL) > > + syslog(LOG_AUTH, > > + "change of %s > > denied for %s(uid=%d)", > > + > > attrMod->attrValue.attrName.buf, pwd->pw_name, > > + sinfo->uid); > > + error = > > SA_AIS_ERR_ACCESS_DENIED; > > + goto done; > > + } > > + attrMod = attrMod->next; > > + } > > + } else > > + ; // modifications by root are OK > > } > > /* intentional fall through. */ > > case IMMND_EVT_A2ND_OBJ_CREATE: > > @@ -3349,11 +3378,11 @@ static SaAisErrorT immnd_fevs_local_chec > > Because of the very special and powerful nature of > > this operation, only > > root users should be allowed to use it, when acces > > control is enabled. > > */ > > - if(uid) { > > - struct passwd *pwd = getpwuid(uid); > > + if ((sinfo != NULL) && (sinfo->uid > 0)) { > > + struct passwd *pwd = getpwuid(sinfo->uid); > > if (pwd != NULL) { > > syslog(LOG_AUTH, > > "saImmOmAdminOwnerClear denied for %s(uid=%d)", > > - pwd->pw_name, uid); > > + pwd->pw_name, sinfo->uid); > > } > > error = SA_AIS_ERR_ACCESS_DENIED; > > goto done; > > > > > ------------------------------------------------------------------------------ > Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer > Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports > Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper > Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer > http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk > _______________________________________________ > Opensaf-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/opensaf-devel ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
