Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On 24 Nov 2023, at 19:26, Ruediger Pluem wrote: >> We do - and we also need to apr_pstrdup() the dn to be consistent with the >> rest. > > Why? dn is already apr_pstrdup from r->pool: > > https://github.com/apache/httpd/blob/dc76ce4c43efb8c0c36a5990aeb0468a87458087/modules/ldap/util_ldap.c#L2133 Looks like we were consistently wasteful due to lack of const, fixed in r1914281. Regards, Graham —
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On 11/24/23 5:41 PM, Graham Leggett via dev wrote: > On 24 Nov 2023, at 14:25, Ruediger Pluem wrote: > >>> + req->dn = dn; >> >> Don't we need to save the vals as well? > > We do - and we also need to apr_pstrdup() the dn to be consistent with the > rest. Why? dn is already apr_pstrdup from r->pool: https://github.com/apache/httpd/blob/dc76ce4c43efb8c0c36a5990aeb0468a87458087/modules/ldap/util_ldap.c#L2133 Regards Rüdiger
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On 24 Nov 2023, at 14:25, Ruediger Pluem wrote:
>> +req->dn = dn;
>
> Don't we need to save the vals as well?
We do - and we also need to apr_pstrdup() the dn to be consistent with the rest.
Index: modules/aaa/mod_authnz_ldap.c
===
--- modules/aaa/mod_authnz_ldap.c (revision 1914090)
+++ modules/aaa/mod_authnz_ldap.c (working copy)
@@ -1453,7 +1453,6 @@
t = require;
if (t[0]) {
-const char **vals;
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02630)
"auth_ldap authorize: checking filter %s", t);
@@ -1460,11 +1459,11 @@
/* Search for the user DN */
result = util_ldap_cache_getuserdn(r, ldc, sec->url, sec->basedn,
- sec->scope, sec->attributes, t, , );
+ sec->scope, sec->attributes, t, , &(req->vals));
/* Make sure that the filtered search returned a single dn */
if (result == LDAP_SUCCESS && dn) {
-req->dn = dn;
+req->dn = apr_pstrdup(r->pool, dn);
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02631)
"auth_ldap authorize: require ldap-search: "
"authorization successful");
Regards,
Graham
—
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On 11/24/23 10:45 AM, Graham Leggett via dev wrote:
>
>
>
> /* Make sure that the filtered search returned a single dn */
>
> if (result == LDAP_SUCCESS && dn) {
>
> + req->dn = dn;
Don't we need to save the vals as well?
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");
Regards
Rüdiger
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On Fri, Nov 24, 2023 at 12:41 PM Graham Leggett wrote: > > On 24 Nov 2023, at 10:03, Yann Ylavic wrote: > > > +1 this is pretty much what Rüdiger proposed earlier and it aligns > > with the proposed 2.4.x backport so I understand better :) > > Rüdiger’s patch had conflicts in the time he posted it and now, I reapplied > his changes in the above patch and confirmed they worked. > > Who wants to commit? (Am happy to) Sure, go ahead. Regards; Yann.
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On 24 Nov 2023, at 10:03, Yann Ylavic wrote: > +1 this is pretty much what Rüdiger proposed earlier and it aligns > with the proposed 2.4.x backport so I understand better :) Rüdiger’s patch had conflicts in the time he posted it and now, I reapplied his changes in the above patch and confirmed they worked. Who wants to commit? (Am happy to) Regards, Graham —
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On Fri, Nov 24, 2023 at 10:45 AM Graham Leggett via dev
wrote:
>
> I completely misunderstood this - I had the idea that build_request_config()
> was being removed, when it was being left behind, sorry about that.
> The patch that applies to trunk looks like this, and I just tested it and it
> works:
>
> Index: modules/aaa/mod_authnz_ldap.c
> ===
> --- modules/aaa/mod_authnz_ldap.c (revision 1914067)
> +++ modules/aaa/mod_authnz_ldap.c (working copy)
> @@ -1441,24 +1441,6 @@
> req = build_request_config(r);
> }
> 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;
> -}
> -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, );
> if (err) {
> @@ -1482,6 +1464,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");
+1 this is pretty much what Rüdiger proposed earlier and it aligns
with the proposed 2.4.x backport so I understand better :)
Regards;
Yann.
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);
I completely misunderstood this - I had the idea that build_request_config()
was being removed, when it was being left behind, sorry about that.
The patch that applies to trunk looks like this, and I just tested it and it
works:
Index: modules/aaa/mod_authnz_ldap.c
===
--- modules/aaa/mod_authnz_ldap.c (revision 1914067)
+++ modules/aaa/mod_authnz_ldap.c (working copy)
@@ -1441,24 +1441,6 @@
req = build_request_config(r);
}
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;
-}
-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, );
if (err) {
@@ -1482,6 +1464,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,
Graham
—
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On Wed, Nov 22, 2023 at 3:40 PM Graham Leggett via dev wrote: > > On 20 Nov 2023, at 15:32, Yann Ylavic wrote: >> >> 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.. > > Just tested this, and it appears to work fine. > > One tiny detail, these is a logging line that refers to ldap-filter inside > ldap-search, but other than that, +1. > > + "auth_ldap authorize: require ldap-filter: user's > DN " Please take it, I do not have the environment for now to compile and/or test this. Regards; Yann.
Re: svn commit: r1913977 - /httpd/httpd/trunk/modules/aaa/mod_authnz_ldap.c
On 20 Nov 2023, at 15:32, Yann Ylavic wrote: > 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. > Just tested this, and it appears to work fine. One tiny detail, these is a logging line that refers to ldap-filter inside ldap-search, but other than that, +1. + "auth_ldap authorize: require ldap-filter: user's DN " Regards, Graham —
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
