proxyauth support does not work when bound as directory manager

[389-ds-bot]

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/48366

• Created at 2015-12-01 16:45:00 by abbra
• Closed as Fixed
• Assigned to lkrispen (@elkris)
• Associated bugzillas
  • https://bugzilla.redhat.com/show_bug.cgi?id=1290101

---

using RFC 4370 proxy auth LDAP control when bound as cn=Directory Manager does not allow ACIs to be evaluated as the proxied identity. We need this to make sure we can consider LDAP ACIs in IPA KDC driver.

[389-ds-bot]

Comment from pspacek at 2015-12-01 16:48:17

Surprisingly, the access log shows the authzid in this case:

[01/Dec/2015:10:51:22 +0100] conn=12466 op=0 BIND dn="cn=Directory Manager" method=128 version=3
[01/Dec/2015:10:51:22 +0100] conn=12466 op=0 RESULT err=0 tag=97 nentries=0 etime=0 dn="cn=directory manager"
[01/Dec/2015:10:51:22 +0100] conn=12466 op=1 SRCH base="uid=admin,cn=users,cn=accounts,dc=vda,dc=li" scope=2 filter="(objectClass=*)" attrs="userPassword" authzid="uid=abokovoy,cn=users,cn=accounts,dc=vda,dc=li"
[01/Dec/2015:10:51:22 +0100] conn=12466 op=1 RESULT err=0 tag=101 nentries=1 etime=0 notes=U
[01/Dec/2015:10:51:22 +0100] conn=12466 op=2 UNBIND

but the authzid is not effective.

Furthermore, DS does not return an error even though the control is marked as critical in the request.

[389-ds-bot]

Comment from nkinder (@nkinder) at 2015-12-09 22:34:16

Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1290101

[389-ds-bot]

Comment from lkrispen (@elkris) at 2016-02-16 20:30:15

attachment
0001-Ticket-48366-proxyauth-does-not-work-bound-as-direct.patch

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2016-02-16 23:58:57

Description: when binding as directory manager always full access is granted, even if a proxyauthzid is presnt

4049	 	        if ( isRoot ) return ACL_TRUE; 
 	4050	        /* need to check if root is proying another user */ 
 	4051	        aclpb = acl_get_aclpb ( pb, ACLPB_PROXYDN_PBLOCK ); 
 	4052	        if ( isRoot && ((access &SLAPI_ACL_PROXY) || !aclpb)) return ACL_TRUE;

Hi Ludwig, please help me understanding this condition at the line 4052.

So, instead of granting full access if isRoot is true, this condition ((access &SLAPI_ACL_PROXY) || !aclpb) is added. The aclpb from the line 4051 is non NULL, if proxyauthzid is given. Good. How about the (access &SLAPI_ACL_PROXY) part? Is it true if it is a proxy auth? If that's true, ((access &SLAPI_ACL_PROXY) || !aclpb) could be always true? I guess I should be missing something...

[389-ds-bot]

Comment from lkrispen (@elkris) at 2016-02-17 17:13:54

(access &SLAPI_ACL_PROXY) is only true if we want to test if the bound user has the proxy rights, so in case of directory manager this should always be granted,
in the next steps when testing for SRCH or READ or other access rights this will be false and the evaluation only depend on the presence of the aclpb for the proxy auth.

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2016-05-20 07:57:58

if ( isRoot && ((access &SLAPI_ACL_PROXY) || !aclpb)) return ACL_TRUE;

This code is very confusing to me. I like things simple. Couldn't we do:

if ( isRoot && (!aclpb && !isProxy) ) {
   return ACL_ALL_GOOD_MATE;
}

Where isProxy is set similar to your change in aclplugin.c.

That would be cleaner, avoids the need to change lots of function signatures and visually is much easier to see what is occuring.

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2016-06-03 03:09:04

Replying to [comment:10 Firstyear]:

if ( isRoot && ((access &SLAPI_ACL_PROXY) || !aclpb)) return ACL_TRUE;

This code is very confusing to me. I like things simple. Couldn't we do:

if ( isRoot && (!aclpb && !isProxy) ) {
   return ACL_ALL_GOOD_MATE;
}

Where isProxy is set similar to your change in aclplugin.c.

That would be cleaner, avoids the need to change lots of function signatures and visually is much easier to see what is occuring.

Thanks, William! Sounds promising. In the meantime, could your proposal be translated like this? Or do you want to add isProxy? I'm more than happy to review your patch... ;)

4052	 if ( isRoot && (!aclpb && !(access & SLAPI_ACL_PROXY)) return ACL_ALL_GOOD_MATE;

[389-ds-bot]

Comment from firstyear (@Firstyear) at 2016-06-03 04:56:04

Well, isProxy already exists in Ludwig's patch. ldap/servers/plugins/acl/aclplugin.c line 120 (Bottom of the patch).

So either we can dup the method to get is proxy, or make a smaller helper function.

This way we don't need to pass around the access flag and bitmask on it.

Depends on if Ludwig is happy for me to take over or not to be honest.

[389-ds-bot]

Comment from lkrispen (@elkris) at 2016-06-17 14:39:44

sorry for the late response. your suggestions change the logic from

( (access & SLAPI_ACL_PROXY)|| !aclpb) to
(!(access & SLAPI_ACL_PROXY) && !aclpb )

The test (access & SLAPI_ACL_PROXY) does not check if this is a proxyuser, but if the proxy right should be tested.

Let my try to explain the logic in line 4052 again.

We are testing if access control can be skipped, before the fix this was simple:
if isRoot skip, else evaluate.

Now, if bound as root (isRoot) we have two different scenarios:
a) we need to check if the bound user has the rights to proxy another user (access == SLAPI_ACL_PROXY) - this is always true for root. so we can skip the evaluation if:

(isRoot && ((access & SLAPI_ACL_PROXY)

b) if root is proxying another user and testing some access right like SEARCH, READ .. we need to continue with evaluation, the evaluation will be done with the proxydn. To determine if we are in this case we check the existence of a ACLPB_PROXYDN_PBLOCK.
If isRoot and there is no proxy pblock we have a normal root query and can skip aci evaluation:

(isRoot && !aclpb)

so ( a || b) becomes:

( isRoot && ((access &SLAPI_ACL_PROXY) || !aclpb)

The patch has been tested and confirmed to work, hope I addressed your concerns.

[389-ds-bot]

Comment from lkrispen (@elkris) at 2016-06-17 17:50:43

attachment
0001-add-testcase-for-ticket-48366-proxyauth-for-root.patch

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2016-06-17 23:03:24

Thank you so much for the explanation. Now, it's perfectly clear. You have my ack.

[389-ds-bot]

Comment from lkrispen (@elkris) at 2016-06-20 18:24:54

committed to master:

New commits:
commit 4d15435
commit 59e45a7

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2016-06-20 21:59:10

Closing with fixed on behalf of Ludwig... ;)

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2016-06-22 04:40:30

git patch file (master) -- one line coverity fix
0001-Ticket-48366-proxyauth-does-not-work-bound-as-direct.2.patch

[389-ds-bot]

Comment from nhosoi (@nhosoi) at 2016-06-22 04:41:20

One line coverity fix -- pushed to master:
0e52877..8b34963 master -> master
commit 4c689e7

[389-ds-bot]

Comment from lkrispen (@elkris) at 2017-02-11 23:07:54

Metadata Update from @elkris:

• Issue assigned to elkris
• Issue set to the milestone: 1.3.5.7