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.

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

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

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...

(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.

{{{
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.

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;
}}}

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.

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.

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

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

committed to master:

New commits:
commit 4d15435
commit 59e45a7

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

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

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

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

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.