I am testing an ACI like the following:
(targetattr=ipaProtectedOperation;read_keys) (version 3.0; acl "allow to read keys"; allow (read) userattr = "ipaAllowedToPerform;getKeytab#GROUPDN";)
The ACI code completely ignores the subtype, and allows access to all the attributes named ipaProtectedOperation regardless of subtype.
I think the right behavior would be to allow access to any subtype if targetattr=ipaProtectedOperation but to restrict access to the specific subtype if one is defined as in the ACI above.
I tested this behavior both by doing an ldapsearch and also in a plugin using slapi_access_allowed, in both cases the usbtype was ignored completely for the target attribute.
git patch file (master) 0001-Ticket-47571-targetattr-ACIs-ignore-subtype.patch
{{{ Description: Subtypes in targetattr, userattr in aci as well as filter and attribute list in the search are supported. If targetattr contains subtypes, the base type only as well as other subtypes are not allowed to access (or denied to access). If userattr contains subtypes, the base type as well as other subtypes in entries do not match the userattr value. * If attribute list in search has a base type attribute, and a targetattr has a type with subtypes, then only the subtyped value is returned. E.g., attribute list: sn targetattr: sn;en ==> sn;en: <sn-en-value> is returned but sn or sn;fr is not. If attribute list has a type with subtype, then if the targetattr allows the subtype, the value is returned. E.g., attribute list: sn;en targetattr: sn;en ==> sn;en: <sn-en-value> is returned but sn or sn;fr is not.
1) slapd/attr.c Added another compare type SLAPI_TYPE_CMP_SUBTYPES to comp_cmp which is called by slapi_attr_type_cmp to support full compare subtypes. 2) plugin/acl.c: Added a helper function acl__attr_subtype_cmp, which calls slapi_attr_type_ cmp with SLAPI_TYPE_CMP_SUBTYPES if a type in aci contains subtypes. Some slapi_attr_type_cmp takes SLAPI_TYPE_CMP_SUBTYPES instead of BASE, which was one of the causes of ignoring subtypes. 3) slapd/search.c,result.c send_all_attrs/send_specific_attrs use a dontsendattr array to control the duplicate attribute types. Replaced the logic with a simpler one by creating an charray with no duplicates.
}}}
Hi Noriko,
It looks good to me. I have just a question regarding slapi_attr_type_cmp and the new case SLAPI_TYPE_CMP_SUBTYPES. It checks that base + options of a1 are all present in b1, then it checks that base+option of b1 are all present in a1. Could it be replaced by strcasecmp(a1,b1) ?
Replying to [comment:5 tbordaz]:
I have just a question regarding slapi_attr_type_cmp and the new case SLAPI_TYPE_CMP_SUBTYPES. It checks that base + options of a1 are all present in b1, then it checks that base+option of b1 are all present in a1. Could it be replaced by strcasecmp(a1,b1) ?
Thanks for your comment, Therry! Yeah, it's redundant, isn't it? Only an issue is we don't "normalize" the subtypes, i.e., it could be "sn;fr;phonetic" or "sn;phonetic;fr". The comparison would fail if it is replaced by strcasecmp...
Thanks Noriko for your answer. I was mislead by the comment saying 'the order must be the same'. Now I have still a doubt that the code would produce comparison success with a1="sn;fr;phonetic" or b2="sn;phonetic;fr". I did a try and the comparison fails at line 197 because to retrieve 'fr' from a1, the loop consumed 'b2'.
Thanks a lot to Thierry for his comments. I revisited the code and revised as he suggested (I believe ;). I'm attaching the patch take 2.
git patch file (master) -- take 2 0001-Ticket-47571-targetattr-ACIs-ignore-subtype.2.patch
{{{ Description: Subtypes in targetattr, userattr in aci as well as filter and attribute list in the search are supported. If targetattr contains subtypes, the base type only as well as other subtypes are not allowed to access (or denied to access). If userattr contains subtypes, the base type as well as other subtypes in entries do not match the userattr value. * If attribute list in search has a base type attribute, and a targetattr has a type with subtypes, then only the subtyped value is returned. E.g., attribute list: sn targetattr: sn;en ==> sn;en: <sn-en-value> and sn;en;phonetic: <sn-en-phonetic-value> are returned but sn or sn;fr is not. If attribute list has a type with subtype, then if the targetattr allows the subtype, the value is returned. E.g., attribute list: sn;en targetattr: sn;en ==> sn;en: <sn-en-value> and sn;en;phonetic: <sn-en-phonetic-value> are returned but sn or sn;fr is not. 1) slapd/attr.c * slapi_attr_type_cmp assumed the subtype order in 2 args are identical, but it is not always guaranteed. Removed the assumption. * Added another compare type SLAPI_TYPE_CMP_SUBTYPES to comp_cmp which is called by slapi_attr_type_cmp to support full subtypes comparison. 2) plugin/acl.c: * Changed to call slapi_attr_type_cmp with human readable macros, e.g., SLAPI_TYPE_CMP_BASE, SLAPI_TYPE_CMP_SUBTYPE, etc. * Replaced strcasecmp with slapi_attr_type_cmp for attribute type comparison. * Changed to call slapi_attr_type_cmp with SLAPI_TYPE_CMP_SUBTYPES (full subtype comparison) in acl__get_attrEval, where the next attribute to compare is determined. 3) slapd/search.c,result.c send_all_attrs/send_specific_attrs use a dontsendattr array to control the duplicate attribute types. Replaced the logic with a simpler one by creating an charray with no duplicates. }}}
Thanks Noriko, yes it works well. Ack. Just you may skip the 'the order must be the same...' comments in line 209
Replying to [comment:11 tbordaz]:
A good catch! Thanks, Thierry! I'm adding this update to the patch... {{{ diff --git a/ldap/servers/slapd/attr.c b/ldap/servers/slapd/attr.c index bdbea15..ef3fa10 100644 --- a/ldap/servers/slapd/attr.c +++ b/ldap/servers/slapd/attr.c @@ -205,9 +205,8 @@ slapi_attr_type_cmp( const char a1, const char a2, int opt ) b1 = a1; b2 = a2; / - * next, for each component in a1, make sure there is a - * matching component in a2. the order must be the same, - * so we can keep looking where we left off each time in a2 + * next, for each component in a2, make sure there is a + * matching component in a1. / for (b2 = next_comp(b2); b2; b2 = next_comp(b2)) { for (b1 = next_comp(b1); b1; b1 = next_comp(b1)) { }}}
git patch file (master) -- take 3 0001-Ticket-47571-targetattr-ACIs-ignore-subtype.3.patch
Thank you for the reviews and advices, Thierry!
Pushed to master: 31cd7a8..85a7874 master -> master commit 85a7874
Note: I changed the Review status to "ack" based upon Thierry's comment.
Coverity errors: 12423: Explicit null dereferenced do_search: {{{ 285 } else if (strcmp(attrs[i], LDAP_ALL_USER_ATTRS / '' /) == 0) { 27. Condition "!charray_inlist(attrs, normaci)", taking true branch 32. Condition "!charray_inlist(attrs, normaci)", taking true branch CID 12423 (#1 of 1): Explicit null dereferenced (FORWARD_NULL)38. var_deref_model: Passing null pointer "normaci" to function "charray_inlist(char , char )", which dereferences it.[show details] 286 if (!charray_inlist(attrs, normaci)) { 287 charray_add(&attrs, normaci); / consumed / 33. assign_zero: Assigning: "normaci" = "NULL". 288 normaci = NULL; 289 } }}}
12422: Logically dead code comp_cmp: {{{ cond_notnull: Condition "NULL == s1", taking false branch. Now the value of "s1" is not NULL. 104 if (NULL == s1) { 105 if (s2) { 106 return 1; 107 } 108 return 0; 109 } 110 if (NULL == s2) { notnull: At condition "s1", the value of "s1" cannot be NULL. dead_error_condition: The condition "s1" must be true. 111 if (s1) { 112 return 1; 113 } CID 12422 (#1 of 1): Logically dead code (DEADCODE)dead_error_line: Execution cannot reach this statement "return 0;". 114 return 0; 115 } }}}
Description: commit 85a7874 introduced 2 coverity issues: 12423: Explicit null dereferenced do_search (slapd/search.c) If attribute list contains multiple ""s and "aci"s in ldapsearch, the previous code attempted to add "aci" once for "" and replacing "aci" with normalized aci (if any) once and eliminating the duplicated "aci"s. The code contained a null dereferenced bug. Even if duplicated attributes are included in the attribute list, they are removed later in send_specific_attrs. Thus, this patch simplifies the logic to avoid the null dereference.
12422: Logically dead code comp_cmp (slapd/attr.c) Eliminated the dead code (case s1 == NULL AND s2 == NULL).
git patch file (master) -- Fixing "Coverity 12422 and 12423" 0001-Ticket-47571-targetattr-ACIs-ignore-subtype.4.patch
Reviewed by Rich (Thank you!!)
Pushed to master: 1a788bf..36c48b8 master -> master commit 36c48b8
Any chance we can get this in 1.3.2.11 together with a fix for #47653 ?
Replying to [comment:19 simo]:
Sure. We discussed in the scram and agreed to set 1.3.2.11 to the milestone.
Pushed to 389-ds-base-1.3.2 branch: 22c24f0..270ad9a 389-ds-base-1.3.2 -> 389-ds-base-1.3.2 commit 9d2d939 commit 270ad9a
Metadata Update from @nhosoi: - Issue assigned to nhosoi - Issue set to the milestone: 1.3.2.11
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/908
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Fixed)
Login to comment on this ticket.