Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c

2023-11-24 Thread Yann Ylavic
On Thu, Nov 23, 2023 at 3:00 PM Ruediger Pluem  wrote:
>
> On 11/23/23 2:28 PM, Yann Ylavic wrote:
> > On Thu, Nov 23, 2023 at 1:33 PM Ruediger Pluem  wrote:
> >>
> >>
> >>
> >> On 11/23/23 12:52 PM, Graham Leggett via dev wrote:
> >>> On 23 Nov 2023, at 11:25, Ruediger Pluem  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.

Thanks, got it.


Regards;
Yann.


Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c

2023-11-24 Thread Yann Ylavic
On Thu, Nov 23, 2023 at 11:06 PM Graham Leggett via dev
 wrote:
>
> On 23 Nov 2023, at 13:28, Yann Ylavic  wrote:
>
> 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..
>
>
> We already have that, it’s ldap-filter.
>
> In more detail, ldap-filter takes an LDAP filter, for example 
> (objectclass=mailRecipient), and then it takes an r->user, for example 
> f...@example.com, and then it combines them to create a new filter 
> (&(uid=f...@example.com)(objectclass=mailRecipient)), and if exactly one (not 
> zero, not two) objects exist, it is a match.
>
> ldap-search ignores the r->user special case and broadens this to any 
> expression you can image.
>
> 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?
>
>
> It’s enforced here:
>
> https://github.com/apache/httpd/blob/trunk/modules/ldap/util_ldap.c#L2118
>
> For historic reasons, the function is called uldap_cache_getuserdn(), but 
> what the function actually does is run a single LDAP query, and expect one 
> and only one object in return.
>
> ldap-search runs a single LDAP query, and expects one and only one object in 
> return, it just isn’t a user.

Thanks for the explanation.


Regards;
Yann.


Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c

2023-11-23 Thread Graham Leggett via dev
On 23 Nov 2023, at 13:28, Yann Ylavic  wrote:

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

We already have that, it’s ldap-filter.

In more detail, ldap-filter takes an LDAP filter, for example 
(objectclass=mailRecipient), and then it takes an r->user, for example 
f...@example.com, and then it combines them to create a new filter 
(&(uid=f...@example.com)(objectclass=mailRecipient)), and if exactly one (not 
zero, not two) objects exist, it is a match.

ldap-search ignores the r->user special case and broadens this to any 
expression you can image.

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

It’s enforced here:

https://github.com/apache/httpd/blob/trunk/modules/ldap/util_ldap.c#L2118

For historic reasons, the function is called uldap_cache_getuserdn(), but what 
the function actually does is run a single LDAP query, and expect one and only 
one object in return.

ldap-search runs a single LDAP query, and expects one and only one object in 
return, it just isn’t a user.

Regards,
Graham
—



Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c

2023-11-23 Thread Ruediger Pluem



On 11/23/23 2:28 PM, Yann Ylavic wrote:
> On Thu, Nov 23, 2023 at 1:33 PM Ruediger Pluem  wrote:
>>
>>
>>
>> On 11/23/23 12:52 PM, Graham Leggett via dev wrote:
>>> On 23 Nov 2023, at 11:25, Ruediger Pluem  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


Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c

2023-11-23 Thread Yann Ylavic
On Thu, Nov 23, 2023 at 1:33 PM Ruediger Pluem  wrote:
>
>
>
> On 11/23/23 12:52 PM, Graham Leggett via dev wrote:
> > On 23 Nov 2023, at 11:25, Ruediger Pluem  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..

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


Regards;
Yann.


Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c

2023-11-23 Thread Ruediger Pluem



On 11/23/23 12:52 PM, Graham Leggett via dev wrote:
> On 23 Nov 2023, at 11:25, Ruediger Pluem  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.
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.
> 

Regards

Rüdiger



Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c

2023-11-23 Thread Graham Leggett via dev
On 23 Nov 2023, at 11:25, Ruediger Pluem  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.

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.

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.

Regards,
Graham
—



Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c

2023-11-23 Thread Ruediger Pluem



On 11/23/23 11:22 AM, minf...@apache.org wrote:
> Author: minfrin
> Date: Thu Nov 23 10:22:58 2023
> New Revision: 1914067
> 
> URL: http://svn.apache.org/viewvc?rev=1914067=rev
> Log:
> Optimise handling LDAP authorization where LDAP was not used
> previously for LDAP authentication.
> 
> Added:
> httpd/httpd/trunk/changes-entries/ldap-optimise.txt
> Modified:
> httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
> 

> Modified: httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c?rev=1914067=1914066=1914067=diff
> ==
> --- httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c (original)

> @@ -1440,24 +1437,27 @@ static authz_status ldapsearch_check_aut
>   * the req structure needed for authorization needs to be created
>   * and populated with the userid and DN of the account in LDAP
>   */
> -
>  if (!req) {
> -authz_status rv = AUTHZ_DENIED;
>  req = build_request_config(r);
> -ldc = get_connection_for_authz(r, LDAP_SEARCH);
> -if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
> +}
> +ldc = get_connection_for_authz(r, LDAP_SEARCH);
> +if (!req->dn && r->user) {
> +authz_status rv;
> +if (!*r->user) {
> +ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(10487)
> +  "ldap authorize: Userid is blank, AuthType=%s",
> +  r->ap_auth_type);
> +}
> +rv = get_dn_for_nonldap_authn(r, ldc);
> +if (rv != AUTHZ_GRANTED) {
>  return rv;
>  }
> -}
> -else {
> -ldc = get_connection_for_authz(r, LDAP_SEARCH);
> -}
> -
> -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?

>  }
>  
>  require = ap_expr_str_exec(r, expr, );
> 
> 
> 


Regards

Rüdiger