On Mon, Nov 20, 2023 at 4:21 PM Ruediger Pluem <rpl...@apache.org> wrote: > > On 11/20/23 4:05 PM, Yann Ylavic wrote: > > On Mon, Nov 20, 2023 at 3:46 PM Yann Ylavic <ylavic....@gmail.com> wrote: > >> > >> On Mon, Nov 20, 2023 at 2:33 PM Yann Ylavic <ylavic....@gmail.com> wrote: > >>> > >>> On Mon, Nov 20, 2023 at 1:57 PM Graham Leggett via dev > >>> <dev@httpd.apache.org> wrote: > >>>> > >>>> On 20 Nov 2023, at 12:26, Ruediger Pluem <rpl...@apache.org> 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 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_authorizat * 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_authorization * 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,26 +1437,29 @@ static authz_status ldapsearch_check_authorization * 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; } + 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, &err); if (err) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02629)