Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On Mon, Nov 20, 2023 at 4:21 PM Ruediger Pluem wrote:
>
> On 11/20/23 4:05 PM, Yann Ylavic wrote:
> > On Mon, Nov 20, 2023 at 3:46 PM Yann Ylavic wrote:
> >>
> >> On Mon, Nov 20, 2023 at 2:33 PM Yann Ylavic wrote:
> >>>
> >>> On Mon, Nov 20, 2023 at 1:57 PM Graham Leggett via dev
> >>> wrote:
>
> On 20 Nov 2023, at 12:26, Ruediger Pluem wrote:
>
> Or we need to ensure that authn_ldap_build_filter is NULL safe and
> returns in a sensible way if user == NULL.
>
>
> This is the option we need I think - it’s possible that ldapsearch could
> be used without a user.
> >>>
> >>> In the proposed 2.4.x backport of ldapsearch_check_authorization()
> >>> there is no call to get_dn_for_nonldap_authn() nor
> >>> authn_ldap_build_filter(). The Require expression is passed directly
> >>> to util_ldap_cache_getuserdn(), so what is building a filter with
> >>> r->user about in the ldapsearch case finally?
> >>
> >> I mean, isn't what we need for something like the attached patch?
> >> This would call get_dn_for_nonldap_authn() only "if we have been
> >> authenticated by some other module than mod_auth_ldap" (per comment in
> >> the code), and do nothing about r->user otherwise.
> >> Again, I don't really know how mod_ldap is supposed to work so
> >> possibly this is all irrelevant..
> >
> > A more complete/correct patch would be this attached v2 anyway.
>
> +1 to the stuff outside ldapsearch_check_authorization. For the stuff inside
> ldapsearch_check_authorization I guess my patch
> is closer to what ldapsearch intends to do do.
OK, I drop a new v3 here just in case and let those who know how LDAP
authn/authz work take whatever :)
This is just to show that there is some room for
factorization/disambiguation in this code..
Regards;
Yann.
Index: modules/aaa/mod_authnz_ldap.c
===
--- modules/aaa/mod_authnz_ldap.c (revision 1913977)
+++ modules/aaa/mod_authnz_ldap.c (working copy)
@@ -767,32 +767,27 @@ static authz_status ldapuser_check_authorization(r
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_authorization(
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_authorization(req
* the req structure needed for authorization needs to be created
* and
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On 11/20/23 4:19 PM, Graham Leggett via dev wrote:
> On 20 Nov 2023, at 15:10, Ruediger Pluem wrote:
>
>> Revisiting this again. I guess the below patch should fix it.
>> I assume that only the checking on req != NULL is relevant.
>> r->user and req->dn do not matter. But we should set req->dn to dn
>> if util_ldap_cache_getuserdn found one.
>>
>> BTW: get_connection_for_authz(r, LDAP_SEARCH); requires a valid req. It
>> cannot deal with req == NULL.
>>
>>
>> Index: modules/aaa/mod_authnz_ldap.c
>> ===
>> --- modules/aaa/mod_authnz_ldap.c(revision 1913980)
>> +++ modules/aaa/mod_authnz_ldap.c(working copy)
>> @@ -1442,23 +1442,10 @@
>> */
>>
>> 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))) {
>> -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;
>> -}
>> +ldc = get_connection_for_authz(r, LDAP_SEARCH);
>
> Unfortunately removing this makes it impossible to use ldapsearch for authz
> only, as the setup process normally done in the authn phase never happens.
Removing what exactly? I currently cannot follow.
Regards
Rüdiger
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On 11/20/23 4:05 PM, Yann Ylavic wrote: > On Mon, Nov 20, 2023 at 3:46 PM Yann Ylavic wrote: >> >> On Mon, Nov 20, 2023 at 2:33 PM Yann Ylavic wrote: >>> >>> On Mon, Nov 20, 2023 at 1:57 PM Graham Leggett via dev >>> wrote: On 20 Nov 2023, at 12:26, Ruediger Pluem wrote: Or we need to ensure that authn_ldap_build_filter is NULL safe and returns in a sensible way if user == NULL. This is the option we need I think - it’s possible that ldapsearch could be used without a user. >>> >>> In the proposed 2.4.x backport of ldapsearch_check_authorization() >>> there is no call to get_dn_for_nonldap_authn() nor >>> authn_ldap_build_filter(). The Require expression is passed directly >>> to util_ldap_cache_getuserdn(), so what is building a filter with >>> r->user about in the ldapsearch case finally? >> >> I mean, isn't what we need for something like the attached patch? >> This would call get_dn_for_nonldap_authn() only "if we have been >> authenticated by some other module than mod_auth_ldap" (per comment in >> the code), and do nothing about r->user otherwise. >> Again, I don't really know how mod_ldap is supposed to work so >> possibly this is all irrelevant.. > > A more complete/correct patch would be this attached v2 anyway. +1 to the stuff outside ldapsearch_check_authorization. For the stuff inside ldapsearch_check_authorization I guess my patch is closer to what ldapsearch intends to do do. Regards Rüdiger
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On 20 Nov 2023, at 15:10, Ruediger Pluem wrote:
> Revisiting this again. I guess the below patch should fix it.
> I assume that only the checking on req != NULL is relevant.
> r->user and req->dn do not matter. But we should set req->dn to dn
> if util_ldap_cache_getuserdn found one.
>
> BTW: get_connection_for_authz(r, LDAP_SEARCH); requires a valid req. It
> cannot deal with req == NULL.
>
>
> Index: modules/aaa/mod_authnz_ldap.c
> ===
> --- modules/aaa/mod_authnz_ldap.c (revision 1913980)
> +++ modules/aaa/mod_authnz_ldap.c (working copy)
> @@ -1442,23 +1442,10 @@
> */
>
> 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))) {
> -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;
> -}
> +ldc = get_connection_for_authz(r, LDAP_SEARCH);
Unfortunately removing this makes it impossible to use ldapsearch for authz
only, as the setup process normally done in the authn phase never happens.
The typical use case for ldapsearch is without authn, because you’re allowing
someone in based on something other than a user login, for example a property
of the SSL connection, or an IP address, etc.
Regards,
Graham
—
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On 11/20/23 3:46 PM, Yann Ylavic wrote:
> On Mon, Nov 20, 2023 at 2:33 PM Yann Ylavic wrote:
>>
>> On Mon, Nov 20, 2023 at 1:57 PM Graham Leggett via dev
>> wrote:
>>>
>>> On 20 Nov 2023, at 12:26, Ruediger Pluem wrote:
>>>
>>> Or we need to ensure that authn_ldap_build_filter is NULL safe and returns
>>> in a sensible way if user == NULL.
>>>
>>>
>>> This is the option we need I think - it’s possible that ldapsearch could be
>>> used without a user.
>>
>> In the proposed 2.4.x backport of ldapsearch_check_authorization()
>> there is no call to get_dn_for_nonldap_authn() nor
>> authn_ldap_build_filter(). The Require expression is passed directly
>> to util_ldap_cache_getuserdn(), so what is building a filter with
>> r->user about in the ldapsearch case finally?
>
> I mean, isn't what we need for something like the attached patch?
> This would call get_dn_for_nonldap_authn() only "if we have been
> authenticated by some other module than mod_auth_ldap" (per comment in
> the code), and do nothing about r->user otherwise.
> Again, I don't really know how mod_ldap is supposed to work so
> possibly this is all irrelevant..
>
Revisiting this again. I guess the below patch should fix it.
I assume that only the checking on req != NULL is relevant.
r->user and req->dn do not matter. But we should set req->dn to dn
if util_ldap_cache_getuserdn found one.
BTW: get_connection_for_authz(r, LDAP_SEARCH); requires a valid req. It cannot
deal with req == NULL.
Index: modules/aaa/mod_authnz_ldap.c
===
--- modules/aaa/mod_authnz_ldap.c (revision 1913980)
+++ modules/aaa/mod_authnz_ldap.c (working copy)
@@ -1442,23 +1442,10 @@
*/
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))) {
-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;
-}
+ldc = get_connection_for_authz(r, LDAP_SEARCH);
require = ap_expr_str_exec(r, expr, );
if (err) {
@@ -1482,6 +1469,7 @@
/* Make sure that the filtered search returned a single dn */
if (result == LDAP_SUCCESS && dn) {
+req->dn = dn;
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02631)
"auth_ldap authorize: require ldap-search: "
"authorization successful");
Regards
Rüdiger
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On Mon, Nov 20, 2023 at 3:46 PM Yann Ylavic wrote:
>
> On Mon, Nov 20, 2023 at 2:33 PM Yann Ylavic wrote:
> >
> > On Mon, Nov 20, 2023 at 1:57 PM Graham Leggett via dev
> > wrote:
> > >
> > > On 20 Nov 2023, at 12:26, Ruediger Pluem wrote:
> > >
> > > Or we need to ensure that authn_ldap_build_filter is NULL safe and
> > > returns in a sensible way if user == NULL.
> > >
> > >
> > > This is the option we need I think - it’s possible that ldapsearch could
> > > be used without a user.
> >
> > In the proposed 2.4.x backport of ldapsearch_check_authorization()
> > there is no call to get_dn_for_nonldap_authn() nor
> > authn_ldap_build_filter(). The Require expression is passed directly
> > to util_ldap_cache_getuserdn(), so what is building a filter with
> > r->user about in the ldapsearch case finally?
>
> I mean, isn't what we need for something like the attached patch?
> This would call get_dn_for_nonldap_authn() only "if we have been
> authenticated by some other module than mod_auth_ldap" (per comment in
> the code), and do nothing about r->user otherwise.
> Again, I don't really know how mod_ldap is supposed to work so
> possibly this is all irrelevant..
A more complete/correct patch would be this attached v2 anyway.
>
> Regards;
> Yann.
Index: modules/aaa/mod_authnz_ldap.c
===
--- modules/aaa/mod_authnz_ldap.c (revision 1913977)
+++ modules/aaa/mod_authnz_ldap.c (working copy)
@@ -767,19 +767,6 @@ static authz_status ldapuser_check_authorization(r
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
@@ -786,7 +773,6 @@ static authz_status ldapuser_check_authorization(r
* 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",
@@ -793,6 +779,17 @@ static authz_status ldapuser_check_authorization(r
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 = 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 +892,28 @@ static authz_status ldapgroup_check_authorization(
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 (!*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) {
-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 = 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
@@ -1109,16 +1117,15 @@ static authz_status ldapdn_check_authorization(req
}
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 = 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, );
if (err) {
@@ -1209,16 +1216,15 @@ static authz_status ldapattribute_check_authorizat
}
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On Mon, Nov 20, 2023 at 2:33 PM Yann Ylavic wrote:
>
> On Mon, Nov 20, 2023 at 1:57 PM Graham Leggett via dev
> wrote:
> >
> > On 20 Nov 2023, at 12:26, Ruediger Pluem wrote:
> >
> > Or we need to ensure that authn_ldap_build_filter is NULL safe and returns
> > in a sensible way if user == NULL.
> >
> >
> > This is the option we need I think - it’s possible that ldapsearch could be
> > used without a user.
>
> In the proposed 2.4.x backport of ldapsearch_check_authorization()
> there is no call to get_dn_for_nonldap_authn() nor
> authn_ldap_build_filter(). The Require expression is passed directly
> to util_ldap_cache_getuserdn(), so what is building a filter with
> r->user about in the ldapsearch case finally?
I mean, isn't what we need for something like the attached patch?
This would call get_dn_for_nonldap_authn() only "if we have been
authenticated by some other module than mod_auth_ldap" (per comment in
the code), and do nothing about r->user otherwise.
Again, I don't really know how mod_ldap is supposed to work so
possibly this is all irrelevant..
Regards;
Yann.
Index: modules/aaa/mod_authnz_ldap.c
===
--- modules/aaa/mod_authnz_ldap.c (revision 1913977)
+++ modules/aaa/mod_authnz_ldap.c (working copy)
@@ -1435,31 +1435,29 @@ static authz_status ldapsearch_check_authorization
return AUTHZ_DENIED;
}
+ldc = get_connection_for_authz(r, LDAP_SEARCH);
+
/*
* If we have been authenticated by some other module than mod_auth_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_SEARCH);
-if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
-return rv;
+if (r->user && *r->user) {
+authz_status 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(02636)
+ "auth_ldap authorize: require ldap-filter: user's DN "
+ "has not been defined; failing authorization");
+return AUTHZ_DENIED;
+}
}
}
-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;
-}
-
require = ap_expr_str_exec(r, expr, );
if (err) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02629)
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On Mon, Nov 20, 2023 at 1:57 PM Graham Leggett via dev wrote: > > On 20 Nov 2023, at 12:26, Ruediger Pluem wrote: > > Or we need to ensure that authn_ldap_build_filter is NULL safe and returns in > a sensible way if user == NULL. > > > This is the option we need I think - it’s possible that ldapsearch could be > used without a user. In the proposed 2.4.x backport of ldapsearch_check_authorization() there is no call to get_dn_for_nonldap_authn() nor authn_ldap_build_filter(). The Require expression is passed directly to util_ldap_cache_getuserdn(), so what is building a filter with r->user about in the ldapsearch case finally? > > Regards, > Graham > — >
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On 20 Nov 2023, at 12:26, Ruediger Pluem wrote: > Or we need to ensure that authn_ldap_build_filter is NULL safe and returns in > a sensible way if user == NULL. This is the option we need I think - it’s possible that ldapsearch could be used without a user. Regards, Graham —
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On 11/20/23 1:07 PM, yla...@apache.org wrote:
> Author: ylavic
> Date: Mon Nov 20 12:07:11 2023
> New Revision: 1913977
>
> URL: http://svn.apache.org/viewvc?rev=1913977=rev
> Log:
> mod_authnz_ldap: Follow up to r1913962: r->user not used in
> ldapsearch_check_authorization().
>
>
> 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=1913977=1913976=1913977=diff
> ==
> --- httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c (original)
> +++ httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c Mon Nov 20 12:07:11 2023
> @@ -1441,12 +1441,6 @@ static authz_status ldapsearch_check_aut
> * 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(10487)
> -"ldap authorize: Userid is blank, AuthType=%s",
> -r->ap_auth_type);
> -}
> -
We need r->user in get_dn_for_nonldap_authn -> authn_ldap_build_filter in case
req == NULL.
authn_ldap_build_filter will segfault if r->user is NULL. Hence I guess we
should apply the following
on top:
Index: modules/aaa/mod_authnz_ldap.c
===
--- modules/aaa/mod_authnz_ldap.c (revision 1913977)
+++ modules/aaa/mod_authnz_ldap.c (working copy)
@@ -1443,6 +1443,12 @@
if (!req) {
authz_status rv = AUTHZ_DENIED;
+if (!r->user || !*r->user) {
+ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(10487)
+"ldap authorize: Userid is blank, AuthType=%s",
+r->ap_auth_type);
+return rv;
+}
req = build_request_config(r);
ldc = get_connection_for_authz(r, LDAP_SEARCH);
if (AUTHZ_GRANTED != (rv = get_dn_for_nonldap_authn(r, ldc))) {
Or we need to ensure that authn_ldap_build_filter is NULL safe and returns in a
sensible way if user == NULL.
Regards
Rüdiger
Re: svn commit: r1913962 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On 20 Nov 2023, at 12:10, Yann Ylavic wrote: >> Fine, but if r->user is NULL here we'll segfault (NULL dereference) in >> "if (!*r->user)" here. > > Probably an unfortunate copy/paste in trunk only (not in your backport > patch3), fixed in r1913977. Thanks for this - it’s been making my head bleed. Regards, Graham —
Re: svn commit: r1913962 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On Mon, Nov 20, 2023 at 12:05 PM Yann Ylavic wrote: > > On Mon, Nov 20, 2023 at 11:54 AM Graham Leggett via dev > wrote: > > > > On 20 Nov 2023, at 10:44, Yann Ylavic wrote: > > > > >> URL: http://svn.apache.org/viewvc?rev=1913962=rev > > >> Log: > > >> Apply earlier fix to the ldapsearch case: > > >> > > >> Arrange for backend LDAP connections to be returned > > >> to the pool by a fixup hook rather than staying locked > > >> until the end of (a potentially slow) request. > > > > > > It seems that this commit aligns the checks/setup of ldapsearch with > > > the ones of ldapfilter, but nothing about LDAP connections > > > recycling/reuse? > > > > That’s correct - the recycling/reuse code is being backported separately, > > it all depends on this code. > > > > The end goal is for all trunk changes to be applied to v2.4 and each is > > aligned. > > > > > In ldapfilter_check_authorization() we bail out early if r->user is > > > NULL but not here in ldapsearch_check_authorization(), can't it > > > happen? > > > > In ldapsearch we don’t care about the user, it’s purely whether the filter > > expression being applied in the search returns exactly one result. > > Fine, but if r->user is NULL here we'll segfault (NULL dereference) in > "if (!*r->user)" here. Probably an unfortunate copy/paste in trunk only (not in your backport patch3), fixed in r1913977. Regards; Yann.
Re: svn commit: r1913962 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On Mon, Nov 20, 2023 at 11:54 AM Graham Leggett via dev wrote: > > On 20 Nov 2023, at 10:44, Yann Ylavic wrote: > > >> URL: http://svn.apache.org/viewvc?rev=1913962=rev > >> Log: > >> Apply earlier fix to the ldapsearch case: > >> > >> Arrange for backend LDAP connections to be returned > >> to the pool by a fixup hook rather than staying locked > >> until the end of (a potentially slow) request. > > > > It seems that this commit aligns the checks/setup of ldapsearch with > > the ones of ldapfilter, but nothing about LDAP connections > > recycling/reuse? > > That’s correct - the recycling/reuse code is being backported separately, it > all depends on this code. > > The end goal is for all trunk changes to be applied to v2.4 and each is > aligned. > > > In ldapfilter_check_authorization() we bail out early if r->user is > > NULL but not here in ldapsearch_check_authorization(), can't it > > happen? > > In ldapsearch we don’t care about the user, it’s purely whether the filter > expression being applied in the search returns exactly one result. Fine, but if r->user is NULL here we'll segfault (NULL dereference) in "if (!*r->user)" here. Regards; Yann.
Re: svn commit: r1913962 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On 20 Nov 2023, at 09:32, Ruediger Pluem wrote:
>> -if (sec->host) {
>> +if (!sec->host) {
>> +ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01738)
>
> This log message had the number 02636 before.
I’ve fixed these.
They don’t appear to affect the ldapsearch backport proposal, all the codes
there are unique.
Regards,
Graham
—
Re: svn commit: r1913962 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On 20 Nov 2023, at 10:44, Yann Ylavic wrote: >> URL: http://svn.apache.org/viewvc?rev=1913962=rev >> Log: >> Apply earlier fix to the ldapsearch case: >> >> Arrange for backend LDAP connections to be returned >> to the pool by a fixup hook rather than staying locked >> until the end of (a potentially slow) request. > > It seems that this commit aligns the checks/setup of ldapsearch with > the ones of ldapfilter, but nothing about LDAP connections > recycling/reuse? That’s correct - the recycling/reuse code is being backported separately, it all depends on this code. The end goal is for all trunk changes to be applied to v2.4 and each is aligned. > In ldapfilter_check_authorization() we bail out early if r->user is > NULL but not here in ldapsearch_check_authorization(), can't it > happen? In ldapsearch we don’t care about the user, it’s purely whether the filter expression being applied in the search returns exactly one result. In ldapfilter we amend the filter to add the user, which is what we don’t want in ldapsearch. Regards, Graham —
Re: svn commit: r1913962 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On Sun, Nov 19, 2023 at 11:45 AM wrote:
>
> Author: minfrin
> Date: Sun Nov 19 10:45:05 2023
> New Revision: 1913962
>
> URL: http://svn.apache.org/viewvc?rev=1913962=rev
> Log:
> Apply earlier fix to the ldapsearch case:
>
> Arrange for backend LDAP connections to be returned
> to the pool by a fixup hook rather than staying locked
> until the end of (a potentially slow) request.
It seems that this commit aligns the checks/setup of ldapsearch with
the ones of ldapfilter, but nothing about LDAP connections
recycling/reuse?
>
> --- httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c (original)
> +++ httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c Sun Nov 19 10:45:05 2023
> @@ -1429,12 +1429,40 @@ static authz_status ldapsearch_check_aut
> return AUTHZ_DENIED;
> }
>
> -if (sec->host) {
> +if (!sec->host) {
> +ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01738)
> + "auth_ldap authorize: no sec->host - weird...?");
> +return AUTHZ_DENIED;
> +}
> +
> +/*
> + * If we have been authenticated by some other module than mod_auth_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(01739)
> +"ldap authorize: Userid is blank, AuthType=%s",
> +r->ap_auth_type);
> +}
In ldapfilter_check_authorization() we bail out early if r->user is
NULL but not here in ldapsearch_check_authorization(), can't it
happen?
> +
> +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))) {
> +return rv;
> +}
> }
> else {
> -ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(02636)
> - "auth_ldap authorize: no sec->host - weird...?");
> +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 "
> + "has not been defined; failing authorization");
> return AUTHZ_DENIED;
> }
Regards;
Yann.
Re: svn commit: r1913962 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On 11/19/23 11:45 AM, minf...@apache.org wrote:
> Author: minfrin
> Date: Sun Nov 19 10:45:05 2023
> New Revision: 1913962
>
> URL: http://svn.apache.org/viewvc?rev=1913962=rev
> Log:
> Apply earlier fix to the ldapsearch case:
>
> Arrange for backend LDAP connections to be returned
> to the pool by a fixup hook rather than staying locked
> until the end of (a potentially slow) request.
>
>
> 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=1913962=1913961=1913962=diff
> ==
> --- httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c (original)
> +++ httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c Sun Nov 19 10:45:05 2023
> @@ -1429,12 +1429,40 @@ static authz_status ldapsearch_check_aut
> return AUTHZ_DENIED;
> }
>
> -if (sec->host) {
> +if (!sec->host) {
> +ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01738)
This log message had the number 02636 before.
> + "auth_ldap authorize: no sec->host - weird...?");
> +return AUTHZ_DENIED;
> +}
> +
> +/*
> + * If we have been authenticated by some other module than mod_auth_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(01739)
Where is the APLOGNO 01739 taken from? From ldapfilter_check_authorization?
> +"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))) {
> +return rv;
> +}
> }
> else {
> -ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(02636)
> - "auth_ldap authorize: no sec->host - weird...?");
> +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)
Where is the APLOGNO 01742 taken from? From ldapfilter_check_authorization?
> + "auth_ldap authorize: require ldap-filter: user's DN "
> + "has not been defined; failing authorization");
> return AUTHZ_DENIED;
> }
>
>
>
>
APLOGNO's need to be unique, even if the message has the same text. See Basic
rules at
http://svn.apache.org/viewvc/httpd/httpd/trunk/docs/log-message-tags/README?revision=1725984=markup
Regards
Rüdiger
