attachment
users.ldif

I am able to reproduce this with 389-ds-base 1.2.11.x by following these steps:

• Create a new DS instance using "dc=example,dc=com" as the suffix.
• Add the user entries in the attached LDIF file (users.ldif).
• Delete the default anonymous access ACI from "dc=example,dc=com".

• Add the following ACI to "dc=example,dc=com":

  aci: (targetattr="*")(version 3.0; acl "Allow self entry access"; allow (read,search,compare) userdn = "ldap:///self";)

At this point, a search for "uid=test" as the first user in the LDIF will return both user entries. A search as the second user will return no entries.

'''Here is the current status'''

• The problem is reproducible (on master)

• The RC is likely identified. On a given connection/bind the previous results of aci evaluations are cached. The problem is that some evaluation are entry related and so not applicable when we evaluate the aci on an other entry. [[BR]]
  In the testcase (bound as test2), the index (uid) returns cn=test1 then cn=test2. In that case we evaluate the self aci against cn=test1 first. This evaluation returns FAIL (entry is cn=test1 but bound as cn=test2). Then this evaluation is kept into the aclpb and reused when evaluating the entry cn=test2. So it also fails for cn=test2.[[BR]]
  The same issue when bound as test1, is that access is granted for cn=test1 but access is cached/reused for test2.

• A fix is under evaluation. I admit I am not familiar with this code and it is a complex code so it may take some time to identify a fix

'''Here are the next steps'''

• working on a fix

'''Here is the current status'''

• A fix is identified, attached a review template and sent a review.

• Running the acceptance there is no regression (regarding acl testsuite)
  testcase cacheoverflow_02 is failing because it is looking for a message in the errors log and this message changed.[[BR]]
  Expected msg: acl__TestRights - cache overflown[[BR]]

Actual msg: [23/Apr/2013:14:25:21 +0200] acl__TestRights - Your ACL cache of 200 slots has overflowed. This can happen when you have many ACIs. This ACI evaluation requires 200 slots to cache. You can increase your max value by setting the attribute nsslapd-aclpb-max-selected-acls in cn=ACL Plugin,cn=plugins,cn=config to a value higher. A server restart is required.[[BR]]

So I think the test case in my environment is not up to date or it needs to be updated in tet framework

'''Here are the next steps'''

• wait for review

attachment
0001-Ticket-47331-Self-entry-access-ACI-not-working-prope.patch

good catch in the parser. but is the second part necessary, shouldn't now that the SELF flag is set acl__reset_cached_result handle this ?

Hi Ludwig,

Thanks for the review. I admit that this part of code made me crazy. There are actually two caches, one for the aci evaluation results (aclpb_cache_result). One for the previously evaluated entry/operation (aclpb_prev_entryEval_context).[[BR]]

"acl__reset_cached_result" resets the "aci results" cache if they are per entry.[[BR]]
"acl__match_handlesFromCache" retrieves attribute access results from the previous entry.

What happens if "acl__match_handlesFromCache does not return on per entry flag is:

Using the test case:
Bound as test1, the candidate list is cn=test1 and cn=test2. 'uid' access is granted for cn=test1, self aci is evaluated and cn=test1 is returns. Then 'uid' access is granted for cn=test2 (because it reuses the result from the previous entry cn=test1. "acl__match_handlesFromCache") but the self aci is evaluated and cn=test2 is not returned. So bound as test1, only test1 is returned.

Bound as test2. 'uid' access is not granted for cn=test1. then 'uid' access is also not granted for cn=test2 because the result 'deny' is found in the previous entry (cn=test1) cache ("acl__match_handlesFromCache"). So bound as test2, no entry are returned.

The design is complex and I may have miss something but in my understanding we need these two parts of the fix.

best regards
theirry

This logic looks a bit odd to me... If userdn is "ldaps:///self" or "BOGUS:///self", the returned p at the line 828 is NULL? And it crashes the server at the line 829?
{{{
826 826
827 / skip the ldap prefix /
828 p = PL_strnstr(p, LDAP_URL_prefix, end - p);
829 if (strncasecmp(p, LDAP_URL_prefix, strlen(LDAP_URL_prefix)) == 0) {
830 p += strlen(LDAP_URL_prefix);
831 } else if (strncasecmp(p, LDAPS_URL_prefix, strlen(LDAPS_URL_prefix)) == 0) {
832 p += strlen(LDAPS_URL_prefix);
833 } else {
834 goto error;
835 }
836
}}}

Hi Noriko,

good catch. This part of code was definitely invalid. I changed it and re attached a new review.

Note that regarding userdn value like "BOGUS:///self", such aci are rejected during a ldapmodify or skipped at startup (if imported). So they are not evaluated during an access.

regards
thierry

attachment
0002-Ticket-47331-Self-entry-access-ACI-not-working-prope.patch

Thank you for the update, Thierry! Your new patch looks good to me.

Note that regarding userdn value like "BOGUS:///self", such aci are rejected during a ldapmodify or skipped at startup (if imported). So they are not evaluated during an access.
All right. So, this never happens... ;)
{{{
839 if (prefix == NULL) {
840 / userdn value does not starts with LDAP(S)_URL_prefix /
841 goto error;
842 }
}}}

git merge ticket47331
Updating 68cb916..79346de
Fast-forward
ldap/servers/plugins/acl/acl.c | 28 ++++++++++++++++++++++++++--
ldap/servers/plugins/acl/acl.h | 1 +
ldap/servers/plugins/acl/aclparse.c | 19 +++++++++++++++++++
3 files changed, 46 insertions(+), 2 deletions(-)

commit 79346de
Author: Thierry bordaz (tbordaz) tbordaz@redhat.com
Date: Mon Apr 22 14:15:33 2013 +0200

git push origin master
Counting objects: 17, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (9/9), done.
Writing objects: 100% (9/9), 1.87 KiB, done.
Total 9 (delta 7), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
68cb916..79346de master -> master

git patch file (master) -- additional change to commit 79346de
0001-Ticket-47331-Self-entry-access-ACI-not-working-prope.2.patch

Bug Description: sample of odd error messages:
[] NSACLPlugin - The aclpb.state value (9765037) is incorrect. Exceeded the limit (4194303)
[] NSACLPlugin - ACLPB value is:25045a0
[] NSACLPlugin - curr_entry:7f8814003ee0 num_entries:1 curr_dn:7f8818005ac0
[] NSACLPlugin - Last attr:24da8b0, Plist:24e29d0 acleval: 24dda60
[] NSACLPlugin - The aclpb.state value (11862189) is incorrect. Exceeded the limit (4194303)

Fix Description: Additional change to
commit 79346de
which added a macro ACLPB_CACHE_RESULT_PER_ENTRY_SKIP, but
ACLPB_STATE_ALL was not updated to cover the bit.
This patch updates ACLPB_STATE_ALL to support the new bit.

Pushed to master: commit 86fea4b

Pushed to 389-ds-base-1.3.1:
89b78da..6d43552 389-ds-base-1.3.1 -> 389-ds-base-1.3.1
commit 6d43552d862234334f94a5768132d71847bfcd20
commit 1580bcd71cbb60f0e97a36bf83faca6e079cd861

Pushed to 389-ds-base-1.2.11:
a184bbb..d8f6af4 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit d8f6af4
commit 46b06bb

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

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.