You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by mi...@apache.org on 2023/11/23 10:22:59 UTC
svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c
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
Added: httpd/httpd/trunk/changes-entries/ldap-optimise.txt
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/changes-entries/ldap-optimise.txt?rev=1914067&view=auto
==============================================================================
--- httpd/httpd/trunk/changes-entries/ldap-optimise.txt (added)
+++ httpd/httpd/trunk/changes-entries/ldap-optimise.txt Thu Nov 23 10:22:58 2023
@@ -0,0 +1,4 @@
+
+ *) Optimise handling LDAP authorization where LDAP was not used
+ previously for LDAP authentication. [Yann Ylavic]
+
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)
+++ httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c Thu Nov 23 10:22:58 2023
@@ -767,32 +767,27 @@ static authz_status ldapuser_check_autho
return AUTHZ_DENIED;
}
- if (!req) {
- authz_status rv = AUTHZ_DENIED;
- req = build_request_config(r);
- ldc = get_connection_for_authz(r, LDAP_COMPARE);
- if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
- return rv;
- }
- }
- else {
- ldc = get_connection_for_authz(r, LDAP_COMPARE);
- }
-
-
/*
* If we have been authenticated by some other module than mod_authnz_ldap,
* the req structure needed for authorization needs to be created
* and populated with the userid and DN of the account in LDAP
*/
-
-
- if (!*r->user) {
- ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01699)
- "ldap authorize: Userid is blank, AuthType=%s",
- r->ap_auth_type);
+ if (!req) {
+ req = build_request_config(r);
+ }
+ ldc = get_connection_for_authz(r, LDAP_COMPARE);
+ if (!req->dn) {
+ authz_status rv;
+ if (!*r->user) {
+ ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01699)
+ "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;
+ }
}
-
if (req->dn == NULL || !*req->dn) {
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01702)
"auth_ldap authorize: require user: user's DN has not "
@@ -895,17 +890,27 @@ static authz_status ldapgroup_check_auth
return AUTHZ_DENIED;
}
+ /*
+ * If we have been authenticated by some other module than mod_authnz_ldap,
+ * 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_COMPARE);
- if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
+ }
+ ldc = get_connection_for_authz(r, LDAP_COMPARE);
+ if (!req->dn) {
+ authz_status rv;
+ if (!*r->user) {
+ ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01699)
+ "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_COMPARE);
- }
/*
* If there are no elements in the group attribute array, the default should be
@@ -1101,24 +1106,22 @@ static authz_status ldapdn_check_authori
* the req structure needed for authorization needs to be created
* and populated with the userid and DN of the account in LDAP
*/
-
- if (!*r->user) {
- ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01722)
- "ldap authorize: Userid is blank, AuthType=%s",
- r->ap_auth_type);
- }
-
if (!req) {
- authz_status rv = AUTHZ_DENIED;
req = build_request_config(r);
- ldc = get_connection_for_authz(r, LDAP_SEARCH); /* comparedn is a search */
- if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
+ }
+ ldc = get_connection_for_authz(r, LDAP_SEARCH); /* comparedn is a search */
+ if (!req->dn) {
+ authz_status rv;
+ if (!*r->user) {
+ ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01722)
+ "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); /* comparedn is a search */
- }
require = ap_expr_str_exec(r, expr, &err);
if (err) {
@@ -1201,25 +1204,22 @@ static authz_status ldapattribute_check_
* the req structure needed for authorization needs to be created
* and populated with the userid and DN of the account in LDAP
*/
-
- if (!*r->user) {
- ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01730)
- "ldap authorize: Userid is blank, AuthType=%s",
- r->ap_auth_type);
- }
-
if (!req) {
- authz_status rv = AUTHZ_DENIED;
req = build_request_config(r);
- ldc = get_connection_for_authz(r, LDAP_COMPARE);
- if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
+ }
+ ldc = get_connection_for_authz(r, LDAP_COMPARE);
+ if (!req->dn) {
+ authz_status rv;
+ if (!*r->user) {
+ ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01730)
+ "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_COMPARE);
- }
-
if (req->dn == NULL || !*req->dn) {
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01733)
"auth_ldap authorize: require ldap-attribute: user's DN "
@@ -1310,25 +1310,22 @@ static authz_status ldapfilter_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 (!*r->user) {
- ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01739)
- "ldap authorize: Userid is blank, AuthType=%s",
- r->ap_auth_type);
- }
-
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) {
+ authz_status rv;
+ if (!*r->user) {
+ ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01739)
+ "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(01742)
"auth_ldap authorize: require ldap-filter: user's DN "
@@ -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;
+ }
}
require = ap_expr_str_exec(r, expr, &err);
Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Nov 23, 2023 at 11:06 PM Graham Leggett via dev
<de...@httpd.apache.org> wrote:
>
> On 23 Nov 2023, at 13:28, Yann Ylavic <yl...@gmail.com> 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 foo@example.com, and then it combines them to create a new filter (&(uid=foo@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
Posted by Graham Leggett via dev <de...@httpd.apache.org>.
On 23 Nov 2023, at 13:28, Yann Ylavic <yl...@gmail.com> 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 foo@example.com, and then it combines them to create a new filter (&(uid=foo@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
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Nov 23, 2023 at 3:00 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 11/23/23 2:28 PM, Yann Ylavic wrote:
> > On Thu, Nov 23, 2023 at 1:33 PM Ruediger Pluem <rp...@apache.org> wrote:
> >>
> >>
> >>
> >> On 11/23/23 12:52 PM, Graham Leggett via dev wrote:
> >>> On 23 Nov 2023, at 11:25, Ruediger Pluem <rp...@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.
Thanks, got it.
Regards;
Yann.
Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 11/23/23 2:28 PM, Yann Ylavic wrote:
> On Thu, Nov 23, 2023 at 1:33 PM Ruediger Pluem <rp...@apache.org> wrote:
>>
>>
>>
>> On 11/23/23 12:52 PM, Graham Leggett via dev wrote:
>>> On 23 Nov 2023, at 11:25, Ruediger Pluem <rp...@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
Re: svn commit: r1914067 - in /httpd/httpd/trunk: changes-entries/ldap-optimise.txt modules/aaa/mod_authnz_ldap.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Nov 23, 2023 at 1:33 PM Ruediger Pluem <rp...@apache.org> wrote:
>
>
>
> On 11/23/23 12:52 PM, Graham Leggett via dev wrote:
> > On 23 Nov 2023, at 11:25, Ruediger Pluem <rp...@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..
> 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
Posted by Ruediger Pluem <rp...@apache.org>.
On 11/23/23 12:52 PM, Graham Leggett via dev wrote:
> On 23 Nov 2023, at 11:25, Ruediger Pluem <rp...@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.
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
Posted by Graham Leggett via dev <de...@httpd.apache.org>.
On 23 Nov 2023, at 11:25, Ruediger Pluem <rp...@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.
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
Posted by Ruediger Pluem <rp...@apache.org>.
On 11/23/23 11:22 AM, minfrin@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