Ack on this one with comments in-line.
We need to write a defect ticket to do a bit of validation for the
attribute value.
Hans Feldt wrote:
> osaf/services/saf/immsv/immnd/immnd_evt.c | 26 +++++++++++++++++++++++---
> 1 files changed, 23 insertions(+), 3 deletions(-)
>
>
> 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);
> +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));
> + 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)
> +static SaAisErrorT immnd_fevs_local_checks(IMMND_CB *cb, IMMSV_FEVS *fevsReq,
> + const IMMSV_SEND_INFO *sinfo)
> {
> SaAisErrorT error = SA_AIS_OK;
> osafassert(fevsReq);
> @@ -3104,6 +3105,24 @@ static SaAisErrorT immnd_fevs_local_chec
> switch (frwrd_evt.info.immnd.type) {
> case IMMND_EVT_A2ND_OBJ_CREATE:
> case IMMND_EVT_A2ND_OBJ_MODIFY:
> + if ((frwrd_evt.info.immnd.type == IMMND_EVT_A2ND_OBJ_MODIFY) &&
> +
> (strcmp(frwrd_evt.info.immnd.info.objModify.objectName.buf,
> + OPENSAF_IMM_OBJECT_DN) == 0)) {
> + 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) {
> + if (sinfo->uid != 0) {
> + struct passwd *pwd =
> getpwuid(sinfo->uid);
> + if (pwd != NULL)
> + syslog(LOG_AUTH,
> "change of access control mode denied for %s(uid=%d)",
> + pwd->pw_name,
> sinfo->uid);
> + error =
> SA_AIS_ERR_ACCESS_DENIED;
> + goto done;
> + }
> + }
> + attrMod = attrMod->next;
> + }
> + }
>
The above is ok and fevs_local_checks is a reasonable place to actually
do additional (more detailed) ACCESS CONTROL.
There is however in general missing a little bit of VALIDATION..
Since an enum type has been defined for the access-control attribute,
there should be
validation that the attribute value is one of the legal enum values.
This is *local* validation that can be done as part of ccbObjectModify.
In theory it applies to create also but we know that only one instance
of this object can be
created (enforced already by imm) and that create is done under strictly
controlled conditions.
Some corresponding validation is done in ImmModel::ccbObjectModify() for
the longDnsAllowed attribute.
The trap in ccbObjectModify() fro changes to the opensaf service object
is this place:
if (ignoreImpl) {
LOG_IN("Skipping OI upcall for modify on %s since OI is same "
"as the originator of the modify (OI augmented ccb)",
objectName.c_str());
} else if(objectName == immObjectDn) {
............
I think we can write a separate defect ticket for validating the enum
value range.
In fact while re-reading my own code for validating longDnsAllowed I
realize that it
only checks the DNs for existing objects. But it really needs to check
ALL SaNameT
attributes to cover any dangling DNs.
Dangling DNs as such are allowed, but we cant allow LONG dangling DNs if we
are to disable longDNs. Because any long DN can pop up to the user.
And this validation (for longDNs allowed) is here in the wrong place. It
has to be done
in the the apply phase (corresponding to completed). Otherwise the
validation could succeed in
the modify, then the user adds a long DN via create in the same CCB.
That would be one hell of a crazy provocative CCB: disable long DNs
*and* create an object
with a long DN or containing a long DN in the same CCB, but......I will
write a ticket for that.
But to rpeat, the validation of the access control attribute(s) is local
validation. At least I can not see
any relation to any other data in immmodel. So that valiadtion could be
put in ccbObjectModify in
this place I pointed to.
/AndersBj
> case IMMND_EVT_A2ND_OBJ_DELETE:
> case IMMND_EVT_A2ND_CCB_FINALIZE:
> case IMMND_EVT_A2ND_OI_CCB_AUG_INIT:
> @@ -3423,6 +3442,7 @@ static SaAisErrorT immnd_fevs_local_chec
> frwrd_evt.info.immnd.type, error);
> }
>
> +done:
> TRACE_LEAVE();
> return error;
> }
>
------------------------------------------------------------------------------
Slashdot TV.
Video for Nerds. Stuff that matters.
http://tv.slashdot.org/
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
