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&view=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&view=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&view=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&view=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, [email protected] wrote: > Author: minfrin > Date: Sun Nov 19 10:45:05 2023 > New Revision: 1913962 > > URL: http://svn.apache.org/viewvc?rev=1913962&view=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&r1=1913961&r2=1913962&view=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&view=markup Regards Rüdiger
