@tbordaz @mreynolds Do you know much about aci's? IIRC there is a slapi api that allows you to put a filter on a Slapi_Entry to see if it matches. Surely that's all we are missing here in the GER path?

@firstyear, turning on aci debug/summary logging gives the following info

[02/Jun/2017:10:54:04.374253099 +0200] - DEBUG - NSACLPlugin - _ger_get_entry_rights - SLAPI_ACL_ADD                            
[02/Jun/2017:10:54:04.380239180 +0200] - DEBUG - NSACLPlugin - acl__scan_for_acis - Using ACL Container:0 for evaluation
[02/Jun/2017:10:54:04.386644802 +0200] - DEBUG - NSACLPlugin - acl__scan_for_acis - Using ACL Container:1 for evaluation
[02/Jun/2017:10:54:04.397778778 +0200] - DEBUG - NSACLPlugin - ***BEGIN ACL INFO[ Name: "Directory Administrators Group"]***
[02/Jun/2017:10:54:04.404294242 +0200] - DEBUG - NSACLPlugin - ACL Index:4   ACL_ELEVEL:6
[02/Jun/2017:10:54:04.411278277 +0200] - DEBUG - NSACLPlugin - ACI type:(compare search read write delete add self target_attr acltxt allow_rule )
[02/Jun/2017:10:54:04.417482270 +0200] - DEBUG - NSACLPlugin - ACI RULE type:(groupdn )
[02/Jun/2017:10:54:04.423565123 +0200] - DEBUG - NSACLPlugin - Slapi_Entry DN:dc=example,dc=com
[02/Jun/2017:10:54:04.429551198 +0200] - DEBUG - NSACLPlugin - ***END ACL INFO*****************************
[02/Jun/2017:10:54:04.435428101 +0200] - DEBUG - NSACLPlugin - acl__scan_for_acis - Num of ALLOW Handles:1, DENY handles:0
[02/Jun/2017:10:54:04.441498819 +0200] - DEBUG - NSACLPlugin - acl_access_allowed - Processed attr:NULL for entry:ou=people,dc=example,dc=com
[02/Jun/2017:10:54:04.448191230 +0200] - DEBUG - NSACLPlugin - acl__TestRights - 1. Evaluating ALLOW aci(4) " "Directory Administrators Group""
[02/Jun/2017:10:54:04.454447814 +0200] - DEBUG - NSACLPlugin - acllas__user_ismember_of_group - Evaluating user uid=a_user,dc=example,dc=com in group cn=Directory Administrators,dc=example,dc=com?
[02/Jun/2017:10:54:04.461641719 +0200] - DEBUG - NSACLPlugin - -- Not in cn=Directory Manager
[02/Jun/2017:10:54:04.468053172 +0200] - DEBUG - NSACLPlugin - -- Not in cn=Directory Administrators,dc=example,dc=com
[02/Jun/2017:10:54:04.474213914 +0200] - DEBUG - NSACLPlugin - acllas__user_ismember_of_group - Evaluated ACL_FALSE
[02/Jun/2017:10:54:04.480222280 +0200] - DEBUG - NSACLPlugin - print_access_control_summary - conn=0 op=0 (main): Deny add on entry(ou=people,dc=example,dc=com).attr(NULL) to uid=a_user,dc=example,dc=com: no aci matched the subject by aci(4): aciname= "Directory Administrators Group", acidn="dc=example,dc=com"

Although the aci (granting add right) exist, it is not selected as an aci to evaluate. (only Directory admin group is evaluated).
For add right, if the entry does not exist, the targetfilter can not be evaluated and I guess the aci is skipped.
if the entry exist, the targetfilter can be evaluated. The aci is evaluated and grants/or not the 'add' right. In such case GER on that specific entry is useful to know which user was allowed to create that entry.

We can only return GER on existing entries where the targetfilter can be evaluated, why ignoring it ?

I had the same issue when writing ACIs that target objects to be created in a container based on a membership of a bind DN in the container object itself. This is a fairly common use case and there is, it seems, currently no way to define ACI that can be sensed by a GER request.

I think we should allow for the cases where the actual object does not exist but evaluation of the target filter could produce a match for the add operation.

@abbra do you think returning a new entrylevel right 'A' in addition to current 'a' would help ?
For example 'A' would mean: it exists an aci that grant 'add' right if the target matches a target condition (like targetfilter).

I'm not sure we need a new one. If we can ensure that the entry level right 'a' is returned for a request "can I create an entry in this container right below the object DN I check?", we are OK.

In fact, https://tools.ietf.org/html/draft-ietf-ldapext-acl-model in section 4.2.1 defines 'add' right exactly this way:

a  Add

      If granted, permits creation of an entry in the DIT subject to
      access control on all attributes and values to be placed in the
      new entry at time of creation.  In order to add an entry,
      permission must also be granted to add all attributes that exist
      in the add operation.

@abbra @tbordaz

the design is already there and the implementation as well, only it is broken, if you look at:
https://access.redhat.com/documentation/en-us/red_hat_directory_server/10/html/administration_guide/viewing_the_acis_for_an_entry-get_effective_rights_control#ex-ger-non-entry

the idea is to pass an objetclass along with the geteffective right control, the server would create a template entry and verify access against this entry.

But I don't know if it ever worked or if it got broken by some code changes

I'm not sure we need a new one. If we can ensure that the entry level right 'a' is returned for a request "can I create an entry in this container right below the object DN I check?", we are OK.
In fact, https://tools.ietf.org/html/draft-ietf-ldapext-acl-model in section 4.2.1 defines 'add' right exactly this way:
a Add

If granted, permits creation of an entry in the DIT subject to
access control on all attributes and values to be placed in the
new entry at time of creation. In order to add an entry,
permission must also be granted to add all attributes that exist
in the add operation.

you cannot decide based on the container alone, or you cannot specfiy an aci using a targetfilter.

Issue linked to Bugzilla: Bug 1459946

we have not yet fixed it, and it could be quite complex.

so what is the status in IPA, is this still needed or has there been a workaround and we can push this out to future ?

There is no workaround on IPA side, unfortunately. We cannot implement any as far as I can see.

Here is the status:

GER does not work for non existing entries, it cannot find an entry to determine which acis would apply and what access would result.
But GER has a mechnism to create template entries based on an object class and evaluate GER for this template entry

This mechanism is broken in current versions (and maybe for some time).

I have a patch to make it work again, but want to make sure that the method how it works is understood and accepted.

1] you cannot use the non-existing entry as a search base, the search will not come to the phase where template entries can be generated. An existing entry must be used as search base.

2] The search filter provided will be applied in selecting which entries to return with GER info. If the filter matches existing entries they will be returned and if a template entry is reuested this will be returned as last entry. If the search filter does not match any entry only the requested template will be returned

3] the generation of a template entry has to be explicitely triggered, it is done by requesting a specific attribute in the attribute list, it is of the form:

 <attribute>@<objectlass>[:<naming_attribute>]

where
attribute: is the attribute to be generated in the template entry
objectclass: is the name of the objectclass for the template entry
naming_attribute: attribute used to generate the rdn of the template entry, the value of the template entry is always: template_<objectclass>_objectclass

4] there can be multiple template requests in one search

Examples:

Assume the aci:

 aci: (targetattr="*")(targetfilter ="(objectClass=person)")(version 3.0; acl "allow all to target"; allow (all) userdn = "ldap:///cn=other_entry3,dc=example,dc=com";)

and do the following search:

    ldapsearch -LLL -o ldif-wrap=no -h localhost  -p 38901 -x -D "cn=directory manager" -w password -b "dc=example,dc=com" -E '!1.3.6.1.4.1.42.2.27.9.5.2=:dn:cn=other_entry3,dc=example,dc=com' cn=sub_entry11 member@groupofnames:cn sn@person:cn

It requests two template entries, one for objectclass person and one for groupofnames, the result is:

 dn: cn=template_groupofnames_objectclass,dc=example,dc=com
 member: (template_attribute)
 entryLevelRights: none
 attributeLevelRights: member:none, sn:none

 dn: cn=template_person_objectclass,dc=example,dc=com
 sn: (template_attribute)
 entryLevelRights: vadn
 attributeLevelRights: member:none, sn:rscwo

It shows allow access to a objectclass person entry and deny to groupofnames.

as expected from the aci

@lkrispen How do these template entries work in this case? Are they in a txn and then aborted? Or commited and deleted? Wouldn't this cause replication event?

Thanks! I'll read the code a bit more closely tomorrow, It's too late for my jetlagged brain :)

The template entries are created on the fly inside the search operation just to check effective rights and return to the client. They don't even come close to the db or cl

Ahh great! So they are an inmemory slapi_entry then I guess.

Okay, I'll read the code more carefully tomorrow with this in mind. Thank you!!

Ahh great! So they are an inmemory slapi_entry then I guess.

yes, see: _ger_generate_template_entry()

@lkrispen I have not really reviewed the patch but did some tests with it.
I create a testcase that allow a user (subject) to 'add' a 'person' entry under the suffix. Your fix works like a charm. But doing a GER search there is something unexpected:

entryLevelRights: vadn

I do not understand why the user (user11) is granted the right to delete the entry. While the 'allow all to target' (that actually only grants 'add' right) does not grant 'delete'.

[23/Nov/2017:14:20:39.749066826 +0100] - DEBUG - NSACLPlugin - _ger_get_entry_rights - SLAPI_ACL_DELETE                             
[23/Nov/2017:14:20:41.103182196 +0100] - DEBUG - NSACLPlugin - acl__scan_for_acis - Using ACL Container:0 for evaluation
....
[23/Nov/2017:14:20:41.152112029 +0100] - DEBUG - NSACLPlugin - ***BEGIN ACL INFO[ Name: "allow all to target"]***
[23/Nov/2017:14:20:41.158040849 +0100] - DEBUG - NSACLPlugin - ACL Index:5   ACL_ELEVEL:2
[23/Nov/2017:14:20:41.163834708 +0100] - DEBUG - NSACLPlugin - ACI type:(compare search read write delete add self target_attr target_filter acltxt allow_rule )
[23/Nov/2017:14:20:41.169658712 +0100] - DEBUG - NSACLPlugin - ACI RULE type:(userdn )
[23/Nov/2017:14:20:41.175838953 +0100] - DEBUG - NSACLPlugin - Slapi_Entry DN:dc=example,dc=com
[23/Nov/2017:14:20:41.181683244 +0100] - DEBUG - NSACLPlugin - ***END ACL INFO*****************************
[23/Nov/2017:14:20:41.187496147 +0100] - DEBUG - NSACLPlugin - acl__scan_for_acis - Num of ALLOW Handles:2, DENY handles:0
[23/Nov/2017:14:20:41.193541844 +0100] - DEBUG - NSACLPlugin - acl_access_allowed - Processed attr:NULL for entry:cn=template_person_objectclass,dc=example,dc=com
[23/Nov/2017:14:20:41.200333955 +0100] - DEBUG - NSACLPlugin - acl__TestRights - 1. Evaluating ALLOW aci(5) " "allow all to target""
[23/Nov/2017:14:23:01.253148966 +0100] - DEBUG - NSACLPlugin - print_access_control_summary - conn=0 op=0 (main): Allow delete on entry(cn=template_person_objectclass,dc=example,dc=com).attr(NULL) to cn=user11,ou=people,dc=example,dc=com: allowed by aci(5): aciname= "allow all to target", acidn="dc=example,dc=com"

@lkrispen Sorry, I did a typo in my test case. I thought I limited aci to 'add' right but I did 'all.
sorry for the noise

The patch looks good to me. I also tested with GER and PR.
Just a minor comment, the following part of code is duplicated server times

if (gerentry) {
      slapi_pblock_set(pb, SLAPI_SEARCH_RESULT_ENTRY, NULL);
       slapi_entry_free(gerentry);
       gerentry = e = NULL;
  }

would it be valuable to be subfonction or a macro

Subfunction please :) You can't break or inspect a macro with gdb.

If you're worried about performance you can use __attribute((always_inline))

this part of code is not affected by the patch and putting it into a function only makes it look more important although nicer. the calls are all inside the do..while(gap) loop and either at the end or followed by a continue - we could eventually do it in one place only
I think we should probably make the code generally better, not sure if in this context.

I looked into this again and noticed a behaviour which I think is incorrect.

If the attribute list in a geteffective right search contains one or more "templates": "attribute@objectclass" for each of these a template entry should be created and geteffective rights applied. With the patch this works, but with and without the patch it is also triggering a multiple return or normal entries.
In my understanding it is efficient to apply geteffective rights to normal entries without the ger-templates and if ger templates exist return them after the normal entries have been processed.

suggested fix

Hey mate, looks really good. I like the cleanup with send_entry, and some of the other changes.

I think the indentation in send_entry might be off though?

Do we have a test case we can use to prove the patch works for lib389 also?

thanks for feedback, will add a test case

Is there (or would someone please make) a COPR build containing this patch?
I would like to test it with IPA.

consolidated patch, ticket test in progress

Great cleanup !!

Just remark, I would change the variable 'gerentry' to 'gertemplate_entry' or something like this to explain it is a built entry. Also a comment could help to know it is a temporary entry returned by plugin_call_acl_plugin using the pblock.

added comment and change var name,

committed to master:
To ssh://pagure.io/389-ds-base.git
3aa02b6..fa71a0a master -> master

and 1.3.7:
To ssh://pagure.io/389-ds-base.git
288e731..11a2517 389-ds-base-1.3.7 -> 389-ds-base-1.3.7

I tried the new build of 389 that includes this patch. It does not have the expected behaviour. See transcript and commentary below.

Build (from fedora updates-testing):

ftweedal% rpm -qa |grep 389-ds-base
389-ds-base-libs-1.3.7.9-1.fc27.x86_64
389-ds-base-devel-1.3.7.9-1.fc27.x86_64
389-ds-base-1.3.7.9-1.fc27.x86_64

The ACIs for cn=cas,cn=ca,dc=ipa,dc=local. Note in particular System: Add CA.

ftweedal% ldapsearch -LLL -o ldif-wrap=no -D "cn=directory manager" -b cn=cas,cn=ca,dc=ipa,dc=local -w 4me2Test -s base '(objectclass=*)' aci
dn: cn=cas,cn=ca,dc=ipa,dc=local
aci: (targetfilter = "(objectclass=ipaca)")(version 3.0;acl "permission:System: Add CA";allow (add) groupdn = "ldap:///cn=System: Add CA,cn=permissions,cn=pbac,dc=ipa,dc=local";)
aci: (targetfilter = "(objectclass=ipaca)")(version 3.0;acl "permission:System: Delete CA";allow (delete) groupdn = "ldap:///cn=System: Delete CA,cn=permissions,cn=pbac,dc=ipa,dc=local";)
aci: (targetattr = "cn || description")(targetfilter = "(objectclass=ipaca)")(version 3.0;acl "permission:System: Modify CA";allow (write) groupdn = "ldap:///cn=System: Modify CA,cn=permissions,cn=pbac,dc=ipa,dc=local";)
aci: (targetattr = "cn || createtimestamp || description || entryusn || ipacaid || ipacaissuerdn || ipacasubjectdn || modifytimestamp || objectclass")(targetfilter = "(objectclass=ipaca)")(version 3.0;acl "permission:System: Read CAs";allow (compare,read,search) userdn = "ldap:///all";)

uid=alice,... is a member of the cn=System: Add CA,... permission:

ftweedal% ldapsearch -LLL -o ldif-wrap=no -D "cn=directory manager" -b dc=ipa,dc=local -w 4me2Test '(uid=alice)' memberof
dn: uid=alice,cn=users,cn=compat,dc=ipa,dc=local

dn: uid=alice,cn=users,cn=accounts,dc=ipa,dc=local
memberof: cn=ipausers,cn=groups,cn=accounts,dc=ipa,dc=local
memberof: cn=CA Administrator,cn=roles,cn=accounts,dc=ipa,dc=local
memberof: cn=CA Administrator,cn=privileges,cn=pbac,dc=ipa,dc=local
memberof: cn=System: Add CA,cn=permissions,cn=pbac,dc=ipa,dc=local
memberof: cn=System: Delete CA,cn=permissions,cn=pbac,dc=ipa,dc=local
memberof: cn=System: Modify CA,cn=permissions,cn=pbac,dc=ipa,dc=local
memberof: cn=System: Add CA ACL,cn=permissions,cn=pbac,dc=ipa,dc=local
memberof: cn=System: Delete CA ACL,cn=permissions,cn=pbac,dc=ipa,dc=local
memberof: cn=System: Manage CA ACL Membership,cn=permissions,cn=pbac,dc=ipa,dc=local
memberof: cn=System: Modify CA ACL,cn=permissions,cn=pbac,dc=ipa,dc=local
memberof: cn=System: Delete Certificate Profile,cn=permissions,cn=pbac,dc=ipa,dc=local
memberof: cn=System: Import Certificate Profile,cn=permissions,cn=pbac,dc=ipa,dc=local
memberof: cn=System: Modify Certificate Profile,cn=permissions,cn=pbac,dc=ipa,dc=local

But she still gets entryLevelRights: none:

% /usr/lib64/mozldap/ldapsearch -D "cn=directory manager" -w 4me2Test -b "cn=cas,cn=ca,dc=ipa,dc=local" -J "1.3.6.1.4.1.42.2.27.9.5.2:true:dn:uid=alice,cn=users,cn=accounts,dc=ipa,dc=local" -s base "(objectclass=*)"
version: 1
dn: cn=cas,cn=ca,dc=ipa,dc=local
objectClass: nsContainer
objectClass: top
cn: cas
entryLevelRights: none
attributeLevelRights: objectClass:rsc, cn:rsc

Expected behaviour: the entryLevelRights should contain a, indicating permission
to add an entry below cn=cas,cn=ca,dc=ipa,dc=local.

@ftweedal
It works, but not as you expect it to work, if you do a scope base search you can only get the search base entry, and in this case it doesn't have the objectclass ipaca and does not get the add rights.

geteffectiverights also can only provide for the entries it returns, so if you want to see rights for non exiting entries the getffectiverights feature allows you to trigger the creation of a template entry and check the rights for this.

I did explain this: https://pagure.io/389-ds-base/issue/49278#comment-480856

so you should change your scope from "base" to "one" use a search filter of "objectclass=ipaca" which will not exist and provide the trigger in the required attributes "cn@ipaca"

@lkrispen thanks for the explanation, those suggestions did the trick. Much appreciated!

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

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.