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. > > 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, &err); if (err) { @@ -1209,16 +1216,15 @@ static authz_status ldapattribute_check_authorizat } 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 (req->dn == NULL || !*req->dn) { ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01733) @@ -1318,16 +1324,15 @@ static authz_status ldapfilter_check_authorization } if (!req) { + req = build_request_config(r); + } + ldc = get_connection_for_authz(r, LDAP_SEARCH); + if (!req->dn) { 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(01742) @@ -1442,24 +1447,22 @@ static authz_status ldapsearch_check_authorization */ 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 && *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, &err); if (err) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02629)