Nathaniel, I confirmed removing the ADD check in the acllas.c allows the user to create its own entries. Are there any other ACI issues you have run across while testing the otp plugin? I just want to make sure I have everything covered before working on the official fix. Thanks!

attachment
0001-Ticket-47653-Need-a-way-to-allow-users-to-create-ent.patch

Works for me. I think this is all I need.

git merge ticket47653
Updating bacdf01..a9cd4e7
Fast-forward
ldap/ldif/template-dse.ldif.in | 1 +
ldap/servers/plugins/acl/acllas.c | 11 ++++++---
ldap/servers/slapd/libglobs.c | 39 +++++++++++++++++++++++++++++++++++++
ldap/servers/slapd/proto-slap.h | 2 +
ldap/servers/slapd/slap.h | 2 +
5 files changed, 51 insertions(+), 4 deletions(-)

git push origin master
bacdf01..a9cd4e7 master -> master

commit a9cd4e7
Author: Mark Reynolds mreynolds@redhat.com
Date: Fri Jan 3 14:25:15 2014 -0500

git push origin 389-ds-base-1.3.2
1929869..258bcb5 389-ds-base-1.3.2 -> 389-ds-base-1.3.2

Recap:

A new config attribute was added to cn=config:

nsslapd-access-userattr-strict

The default is "on", which will reject these updates. So for the IPA OTP plugin, it needs to be set to "off". Note, it does not require a server restart to take effect.

It turned out this patch does not satisfy the IPA needs and has a possibility to introduce a security issue. Thus, this patch is being pulled out.

<npmccallum> The ACI approves the addition of a new entry if any of the multi-valued attributes match the userattr ACI

We need to come up with a better design to allow only the adding/modifying operator's userDN to be added to the type in userattr="type#USERDN".

Revert "Ticket 47653 - Need a way to allow users to create entries assigned to themselves"

Pushed to master:
85a7874..c25c08f master -> master
commit c25c08f

Pushed to 389-ds-base-1.3.2 branch:
eab3222..9a1b6da 389-ds-base-1.3.2 -> 389-ds-base-1.3.2
commit 9a1b6da

my proposal is to add a new keyword to the ACI system: selfattr

selfattr is similar to userattr + #userdn, the syntax is simply:
(selfattr=attrname)

attrname is the name of the attribute that will be tested by the ACI
evaluator, and the semantics combined with the various allow flags are
the following:

allow add:
an entry can be added if and only if attrname = userDN where the user DN
is the authenticated user DN.
Furthermore the only value (is attrname is multivalued) allowed is again
userDN

allow delete:
an entry can be deleted only if attrname has exclusively one value, the
DN of the authenticated user

For the write,read,search operation I am not sure we really need a
special syntax, maybe we should actually disallow the combination. If we
allow it we may (or not) want to have the following behavior:

allow write:
the only value that can be added to or removed from attrname is the
authenticated userDN

allow read:
the entry can be searched but the only value returned for attrname is
that of the authenticated userDN if present

allow search/compare:
the entry can be searched only if attrname contains the authenticated
userDN

I suspect these 3 behavrios can be obtained with different syntaxes too,
for example using targeattr/targetattrfilter, in that case, maybe it is
better to just restrict selfattr to be valid with add/delete operations
for now.

There is one gotcha to my proposal, and that is what happens if an updated server has an older replica ?

Will it fail to replicate this new ACI type ?

If so would it help to instead use a new syntax for userattr like: userattr=attrname#selfDN ?

Simo, can you explain why a new syntax is necessary rather than changing the behavior of userattr = "attr#USERDN" to do the right thing?

Replying to [comment:14 npmccallum]:

Simo, can you explain why a new syntax is necessary rather than changing the behavior of userattr = "attr#USERDN" to do the right thing?

To avoid havint to use something like nsslapd-access-userattr-strict
If the syntax is new there is no existing deployments to be wary about.

attachment
0001-Ticket-47653-Need-a-way-to-allow-users-to-create-ent.2.patch

attachment
0002-Ticket-47653-Need-a-way-to-allow-users-to-create-ent.patch

The attachement 0002, provides a fix following Simo proposal (new keyword 'SELFDN'). Now I will see if it is possible to implement it using targattrfilters and USERDN.

attachment
0003-Ticket-47653-Need-a-way-to-allow-users-to-create-ent.patch

Tests:
acceptance ACL -> OK
acceptance replication -> ok
ticket47653_test.py -> ok
ticket47653MMR_test.py
'master' -> 1.3.1 OK ( with workaround 'printer-uri' of https://fedorahosted.org/389/ticket/47676
'master' -> 1.2.11 replication ok but failure to replicate schema because'unhashed#user#password' of https://fedorahosted.org/389/ticket/47676

attachment
0004-Ticket-47653-alternate-fix.patch

attachment
0005-Ticket-47653-Need-a-way-to-allow-users-to-create-ent.patch

The new function DS_LASSelfDnAttrEval is almost a full copy of DS_LASUserDnAttrEval. Couldbn't you just add a param to DS_LASUserDnAttrEval to allow/disallow it for ADD and the call it differently for UDSERDN and SELFDN ?

Could we split ther review for the bugfix and the test case ?

Yes, they can share most of the code. I will change that.

I think splitting the review is possible. If I create a patch for the test case and a patch for the fix.

attachment
0006-Ticket-47653-Need-a-way-to-allow-users-to-create-ent.patch

attachment
0001-Ticket-47653-test-case.patch

ack for the fix

I think the logging should specify which keyword you are using, rather than just saying "either userdnattr or selfdnattr has a problem". You could do something like this:

{{{
DS_LASUserDnAttrEval(NSErr_t errp, char attr_name, CmpOp_t comparator,...)
{
...
PRBool selfdn = (attr_name && (strcasecmp(DS_LAS_SELFDNATTR, attr_name) == 0)) ? PR_TRUE : PR_FALSE;
const char kwname = selfdn ? DS_LAS_SELFDNATTR : DS_LAS_USERDNATTR;
...
if ( 0 != (rc = __acllas_setup (errp, attr_name, comparator, 0, / Don't allow range comparators */
...
kwname, "DS_LASUserDnAttrEval",
&lasinfo)) ) {
...

if (selfdn) { 
    slapi_log_error(SLAPI_LOG_ACL, plugin_name, 
                    "ACL info: %s DOES allow ADD permission at level 0.\n", kwname); 
} else { 
    slapi_log_error(SLAPI_LOG_ACL, plugin_name, 
                    "ACL info: %s does not allow ADD permission at level 0.\n", kwname); 
    got_undefined = 1; 
    continue; 
}

}}}
etc. for other places where the acl keyword is mentioned in logging.

or perhaps just use attr_name instead of kwname?

attachment
0007-Ticket-47653-Need-a-way-to-allow-users-to-create-ent.patch

server ack and test ack

The patch looks nice. I have one minor question...

You are comparing the attrValue with SELFDN with strncasecmp with 6 characters. What happens if you set like this?
<userattr> = <attribute>#SELFDNXXX
{{{
3418 } else if (0 == strncasecmp ( attrValue, "SELFDN", 6)) {
3419 matched = DS_LASUserDnAttrEval (errp,DS_LAS_SELFDNATTR, comparator,
3420 attrName, cachable, LAS_cookie,
3421 subject, resource, auth_info, global_auth);
3422 goto done_las;
3423 }
}}}

the extra XXX are ignored. in DS_LASUserAttrEval, it just compares if the attribute value starts with a known keyword strings (ROLEDN, GROUPDN, USERDN,...). Then it calls the associated function DS_LASUserDnAttrEval, DS_LASRoleDnAttrEval, DS_LASGroupDnAttrEval... with a define DS_LAS_USERDNATTR or DS_LAS_GROUPDNATTR...

so the attribute value (with the extra XX) is ignored.

git merge ticket47653
Updating a6f66e7..4db4a0e
Fast-forward
dirsrvtests/tickets/ticket47653MMR_test.py | 552 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
dirsrvtests/tickets/ticket47653_test.py | 432 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
ldap/servers/plugins/acl/acl.h | 1 +
ldap/servers/plugins/acl/acllas.c | 55 ++++++--
4 files changed, 1030 insertions(+), 10 deletions(-)
create mode 100644 dirsrvtests/tickets/ticket47653MMR_test.py
create mode 100644 dirsrvtests/tickets/ticket47653_test.py

git push origin master

Counting objects: 21, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (12/12), done.
Writing objects: 100% (12/12), 8.10 KiB, done.
Total 12 (delta 8), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
a6f66e7..4db4a0e master -> master

commit 4db4a0e
Author: Thierry bordaz (tbordaz) tbordaz@redhat.com
Date: Thu Jan 30 19:09:32 2014 +0100

Replying to [comment:26 tbordaz]:

the extra XXX are ignored. in DS_LASUserAttrEval, it just compares if the attribute value starts with a known keyword strings (ROLEDN, GROUPDN, USERDN,...). Then it calls the associated function DS_LASUserDnAttrEval, DS_LASRoleDnAttrEval, DS_LASGroupDnAttrEval... with a define DS_LAS_USERDNATTR or DS_LAS_GROUPDNATTR...

so the attribute value (with the extra XX) is ignored.
Yes, that's what I thought. That is, we don't want to return any error against such keywords with trailing garbage characters... :)

'''1.3.2.11'''

git push origin 389-ds-base-1.3.2

Counting objects: 19, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (11/11), done.
Writing objects: 100% (12/12), 8.05 KiB, done.
Total 12 (delta 7), reused 0 (delta 0)
To ssh://git.fedorahosted.org/git/389/ds.git
cfbda42..147cfbb 389-ds-base-1.3.2 -> 389-ds-base-1.3.2

commit 147cfbb
Author: Thierry bordaz (tbordaz) tbordaz@redhat.com
Date: Thu Jan 30 19:09:32 2014 +0100

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

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.