Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c
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
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
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
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
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
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
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
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&view=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&r1=1914066&r2=1914067&view=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, &err);
>
>
>
Regards
Rüdiger
