#626 Possible to add nonexistent target to ACI
Closed: wontfix None Opened 8 years ago by nkinder.

Description of problem:
ACI acts as access control mechanism. The "target" part of ACI can be used to
specify, to which entry this ACI applies. If the target is not specified, the
ACI applies to the entry containing the aci attribute and to the entries below
it.

Currently, it is possible to add non-existent entry as target and ACI is
accepted by DS.

Steps to Reproduce:
[jrusnack@dhcp-31-42 /]$ ldapmodify -h 192.168.122.188 -p 389 -D "cn=directory
manager" -w Secret123 <<EOF
dn: dc=example,dc=com
changetype: modify
add: aci
aci: (targetattr != "userPassword") (target =
"ldap:///ou=invalid,dc=example,dc=com") (version 3.0;acl "Enable anonymous
access";allow (read,compare,search)(userdn = "ldap:///anyone");)
EOF

[jrusnack@dhcp-31-42 /]$ echo $?
0

Actual results:
ACI is accepted.

Expected results:
DS should indicate that ACI applies to non-existent entry.


We need to allow ACI creation for non-existent targets, as it is a valid use case when you want to prevent addition of a specific target. It is also possible that someone would want to create their ACIs before adding the target entries.

If we do anything here, it should be to simply log and return a warning to the client but still accept the ACI.

Replying to [comment:3 nkinder]:

If we do anything here, it should be to simply log and return a warning to the client but still accept the ACI.

Anupam, please follow above Nathan's comment.
When adding/modifying an ACI, search the target and if it does not exist, file a warning.

This patch does not validate wildcards and macros in the ACI target

Comparing DN values is tricky with the normalization, processing escape values, multi-valued RDNs, etc. In order to handle comparisons, convert DN values to Slapi_DN.

acl_parse()
{{{
Slapi_DN *targdn = slapi_sdn_new();
slapi_filter_get_ava ( f, &avaType, &avaValue );
slapi_sdn_init_dn_byref(targdn, avaValue->bv_val);

if (!slapi_sdn_get_dn(targdn)) {
    /* not a valid DN */
    slapi_sdn_free(&targdn);
    return ACL_INVALID_TARGET;
}

if (!slapi_sdn_issuffix(targdn, aci_item->aci_sdn)) {
    slapi_sdn_free(&targdn);
return ACL_INVALID_TARGET;
}

if (0 == slapi_sdn_compare(targdn, aci_item->aci_sdn)) {
    int target_check = 0;
    if (pb &&
        !slapi_pblock_get(pb, SLAPI_ACI_TARGET_CHECK, &target_check) &&
        (target_check != 1)) {
        /* Make sure that the target exists */
        int rc = 0;
        Slapi_PBlock *temppb = slapi_pblock_new();
        slapi_search_internal_set_pb_ext(temppb, targdn,
            LDAP_SCOPE_BASE, "(objectclass=*)", NULL, 1, NULL, NULL,
            (void *)plugin_get_default_component_id(), 0);
        slapi_search_internal_pb(temppb);
        slapi_pblock_get(temppb, SLAPI_PLUGIN_INTOP_RESULT, &rc);
        if (rc != LDAP_SUCCESS) {
            slapi_log_error(SLAPI_LOG_FATAL, plugin_name,
                            "The ACL target %s does not exist\n", slapi_sdn_get_dn(targdn));
            rc = ACL_INVALID_TARGET;
        }
        slapi_free_search_results_internal(temppb);
    slapi_pblock_destroy(temppb);
        target_check = 1;
        slapi_pblock_set(pb, SLAPI_ACI_TARGET_CHECK, &target_check);
    }
}
slapi_sdn_free(&targdn);
if (rc) {
    return rc;
}

Replying to [comment:8 rmeggins]:

Comparing DN values is tricky with the normalization, processing escape values, multi-valued RDNs, etc. In order to handle comparisons, convert DN values to Slapi_DN.

acl_parse()
{{{
Slapi_DN *targdn = slapi_sdn_new();
slapi_filter_get_ava ( f, &avaType, &avaValue );
slapi_sdn_init_dn_byref(targdn, avaValue->bv_val);

if (!slapi_sdn_get_dn(targdn)) {
    /* not a valid DN */
    slapi_sdn_free(&targdn);
    return ACL_INVALID_TARGET;
}

if (!slapi_sdn_issuffix(targdn, aci_item->aci_sdn)) {
    slapi_sdn_free(&targdn);

return ACL_INVALID_TARGET;
}

if (0 == slapi_sdn_compare(targdn, aci_item->aci_sdn)) {
    int target_check = 0;
    if (pb &&
        !slapi_pblock_get(pb, SLAPI_ACI_TARGET_CHECK, &target_check) &&
        (target_check != 1)) {
        /* Make sure that the target exists */
        int rc = 0;
        Slapi_PBlock *temppb = slapi_pblock_new();
        slapi_search_internal_set_pb_ext(temppb, targdn,
            LDAP_SCOPE_BASE, "(objectclass=*)", NULL, 1, NULL, NULL,
            (void *)plugin_get_default_component_id(), 0);
        slapi_search_internal_pb(temppb);
        slapi_pblock_get(temppb, SLAPI_PLUGIN_INTOP_RESULT, &rc);
        if (rc != LDAP_SUCCESS) {
            slapi_log_error(SLAPI_LOG_FATAL, plugin_name,
                            "The ACL target %s does not exist\n", slapi_sdn_get_dn(targdn));
            rc = ACL_INVALID_TARGET;
        }
        slapi_free_search_results_internal(temppb);
  slapi_pblock_destroy(temppb);
        target_check = 1;
        slapi_pblock_set(pb, SLAPI_ACI_TARGET_CHECK, &target_check);
    }
}
slapi_sdn_free(&targdn);
if (rc) {
    return rc;
}

Thanks for the review Rich. I have a couple of questions though

  1. As per Noriko's comment on this ticket, we should allow the creation of ACI with non existent target and only log a warning in the server logs. However, you have suggested returning ACI_INVALID_TARGET in case of non-existent targets which won't allow the creation of ACI. Is this the expected behaviour?

  2. My code structure was something like -
    if (pb) {
    slapi_pblock_get(pb, SLAPI_ACI_TARGET_CHECK, &target_check);
    }
    if (target_check != 1) {
    .
    .

}
if (pb) {
target_check = 1;
slapi_pblock_set(pb, SLAPI_ACI_TARGET_CHECK, &target_check);
}

I used this structure intentionally so that the target check is performed even when pb is NULL which happens at the time the directory server is started
If I use something like
if (pb &&
!slapi_pblock_get(pb, SLAPI_ACI_TARGET_CHECK, &target_check) &&
(target_check != 1)) {

}
it won't be able to perform the target check at the time the server is started.

Please correct me if I am wrong.

Replying to [comment:10 anjain]:

Replying to [comment:8 rmeggins]:

Comparing DN values is tricky with the normalization, processing escape values, multi-valued RDNs, etc. In order to handle comparisons, convert DN values to Slapi_DN.

<snip>

Thanks for the review Rich. I have a couple of questions though

  1. As per Noriko's comment on this ticket, we should allow the creation of ACI with non existent target and only log a warning in the server logs. However, you have suggested returning ACI_INVALID_TARGET in case of non-existent targets which won't allow the creation of ACI. Is this the expected behaviour?

No. In the case where the target does not exist, we should log a warning, but allow the creation of the ACI. So you had it correct in your original code.

  1. My code structure was something like -
    if (pb) {
    slapi_pblock_get(pb, SLAPI_ACI_TARGET_CHECK, &target_check);
    }
    if (target_check != 1) {
    .
    .

}
if (pb) {
target_check = 1;
slapi_pblock_set(pb, SLAPI_ACI_TARGET_CHECK, &target_check);
}

I used this structure intentionally so that the target check is performed even when pb is NULL which happens at the time the directory server is started
If I use something like
if (pb &&
!slapi_pblock_get(pb, SLAPI_ACI_TARGET_CHECK, &target_check) &&
(target_check != 1)) {

}
it won't be able to perform the target check at the time the server is started.

Please correct me if I am wrong.

No, you are correct, I misunderstood.

Ticket #47459 has been created to validate the target when they contain wildcards and macros

If the target DN can contain wildcards now, then they will fail DN syntax checking. So you must change this code:
{{{

159                         if (!slapi_sdn_get_dn(targdn)) { 
160                                 /* not a valid DN */ 
161                                 slapi_sdn_free(&targdn); 
162                                 return ACL_INVALID_TARGET; 
163                         }

}}}

In this case, you should not return ACL_INVALID_TARGET, since we must allow strings that are not valid DNs.

Replying to [comment:14 rmeggins]:

If the target DN can contain wildcards now, then they will fail DN syntax checking. So you must change this code:
{{{

159 if (!slapi_sdn_get_dn(targdn)) {
160 / not a valid DN /
161 slapi_sdn_free(&targdn);
162 return ACL_INVALID_TARGET;
163 }
}}}

In this case, you should not return ACL_INVALID_TARGET, since we must allow strings that are not valid DNs.

Rich, The code is something like

if (aci_item->aci_type & ACI_TARGET_DN) {
.
.
if (!slapi_sdn_get_dn(targdn)) {
/ not a valid DN /
slapi_sdn_free(&targdn);
return ACL_INVALID_TARGET;
}
.
.
}
If the target contains wildcards then the condition "if (aci_item->aci_type & ACI_TARGET_DN)" won't be satisfied. So, I believe this is fine.

Replying to [comment:15 anjain]:

Replying to [comment:14 rmeggins]:

If the target DN can contain wildcards now, then they will fail DN syntax checking. So you must change this code:
{{{

159                         if (!slapi_sdn_get_dn(targdn)) { 
160                                 /* not a valid DN */ 
161                                 slapi_sdn_free(&targdn); 
162                                 return ACL_INVALID_TARGET; 
163                         }

}}}

In this case, you should not return ACL_INVALID_TARGET, since we must allow strings that are not valid DNs.

Rich, The code is something like

if (aci_item->aci_type & ACI_TARGET_DN) {
.
.
if (!slapi_sdn_get_dn(targdn)) {
/ not a valid DN /
slapi_sdn_free(&targdn);
return ACL_INVALID_TARGET;
}
.
.
}
If the target contains wildcards then the condition "if (aci_item->aci_type & ACI_TARGET_DN)" won't be satisfied. So, I believe this is fine.

Ah, ok. Then if at this point in the code, it really is supposed to be a valid DN string, then checking for it is a good thing.

Pushed to master on behalf of anjain:
a4daf1a..44ebe22 master -> master
commit 44ebe22
Author: Anupam Jain anjain@localhost.localdomain
Date: Thu Aug 1 15:13:32 2013 -0700

Metadata Update from @nhosoi:
- Issue assigned to anjain
- Issue set to the milestone: 1.3.2 - 08/13 (August)

4 years ago

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/626

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: Fixed)

7 months ago

Login to comment on this ticket.

Metadata