#47653 Need a way to allow users to create entries assigned to themselves.
Closed: Fixed None Opened 5 years ago by npmccallum.

I'm working on this project: http://www.freeipa.org/page/V3/OTP

Users need to be able to create, edit and delete their own tokens. Each
token has an attribute: ipatokenOwner.

I attempted creating this ACL: (target =
"ldap:///ipatokenuniqueid=*,cn=otp,dc=example,dc=com")(targetfilter =
"(objectClass=ipaToken)")(version 3.0; acl "token-add-delete"; allow
(add, delete) userattr = "ipatokenOwner#USERDN";)

After much debugging I found out this is impossible because of this:
https://git.fedorahosted.org/cgit/389/ds.git/tree/ldap/servers/plugins/acl/acllas.c#n1282

Now, in the general case, I can very much understand why this shouldn't
be allowed by default. What alternatives are there with the current
code? Would 389DS be willing to accept a patch to enable this (with a
I_KNOW_WHAT_I_AM_DOING flag)?

The general reason why this feature works in my case is that each object
created restricts the user, rather than granting new privileges. This
seems like a valid use case.


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!

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.

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.

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

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.

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?

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

Metadata Update from @simo:
- Issue assigned to tbordaz
- Issue set to the milestone: 1.3.2.11

2 years ago

Login to comment on this ticket.

Metadata