On 11/23/23 2:28 PM, Yann Ylavic wrote:
> On Thu, Nov 23, 2023 at 1:33 PM Ruediger Pluem <rpl...@apache.org> wrote:
>>
>>
>>
>> On 11/23/23 12:52 PM, Graham Leggett via dev wrote:
>>> On 23 Nov 2023, at 11:25, Ruediger Pluem <rpl...@apache.org> wrote:
>>>
>>>>> - if (req->dn == NULL || !*req->dn) {
>>>>> - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
>>>>> - "auth_ldap authorize: require ldap-filter: user's
>>>>> DN "
>>>>> - "has not been defined; failing authorization");
>>>>> - return AUTHZ_DENIED;
>>>>> + if (req->dn == NULL || !*req->dn) {
>>>>> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02636)
>>>>> + "auth_ldap authorize: require ldap-search:
>>>>> user's DN "
>>>>> + "has not been defined; failing authorization");
>>>>> + return AUTHZ_DENIED;
>>>>> + }
>>>>
>>>> Why do we need to get the dn in case that r->user is not NULL and why is
>>>> it a reason to fail if we don't get a dn for this user?
>>>
>>> This message is misleading, the DN we care about is not the DN of the user,
>>> but rather the DN of the object returned in the
>>> ldap-search, which may or may not bear a relation to the user.
>>
>> Unfortunately the message is correct and we do just that via
>> get_dn_for_nonldap_authn (line 1451). The DN you mention we look for
>> later in line 1480 via util_ldap_cache_getuserdn.
>
> My guess was that if r->user is set we always want to use it for ldap
> requests/authn? Or maybe there should be another Require ldap-!search
> configured to achieve that (if wanted) since ldap-search is special..
I think for ldap-search things depend on the configured filter expression. It
may involve a previously authenticated user, in
which case it has similar approaches like ldap-filter or even require
valid-user or it does not include that and may remind more
of a mod_authz_host with a dynamic IP list stored in LDAP. Or the certificate
example Graham gave.
>
>> BTW: I guess the following patch is missing as we don't store the dn / vals
>> we get from util_ldap_cache_getuserdn but just
>> "forget" it once we leave the function.
>>
>>
>> Index: modules/aaa/mod_authnz_ldap.c
>> ===================================================================
>> --- modules/aaa/mod_authnz_ldap.c (revision 1914069)
>> +++ modules/aaa/mod_authnz_ldap.c (working copy)
>> @@ -1482,10 +1482,12 @@
>>
>> /* Make sure that the filtered search returned a single dn */
>> if (result == LDAP_SUCCESS && dn) {
>> + req->dn = dn;
>> + req->vals = vals;
>> ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02631)
>> "auth_ldap authorize: require ldap-search: "
>> "authorization successful");
>> - set_request_vars(r, LDAP_AUTHZ, req->vals);
>> + set_request_vars(r, LDAP_AUTHZ, vals);
>> return AUTHZ_GRANTED;
>> }
>> else {
>>
>>
>>>
>>> For example the ldap-search might be doing a lookup on (sucks thumb) the
>>> issuer of a certificate, which if it matches some LDAP
>>> object means it is allowed. The DN will not refer to the user, but
>>> something else.
>>
>> I get that. Hence I wonder why we care of the dn fitting to r->user set
>> before.
>>
>>>
>>> On a side note, ldap-search requires that exactly one LDAP object matches,
>>> we might want to support many matches, or a specific
>>> number of matches.
>
> I still don't get where "Make sure that the filtered search returned a
> single dn" is enforced, is it because result != LDAP_SUCCESS
> otherwise, or dn == NULL if there are multiple objects/DNs?
Have a look at line 2117 and following in modules/ldap/util_ldap.c. We call it
in line 1480 of modules/aaa/mod_authnz_ldap.c via
util_ldap_cache_getuserdn. Result will be LDAP_NO_SUCH_OBJECT and dn will stay
the same as before the call.
Regards
Rüdiger
