On Mon, Nov 20, 2023 at 4:21 PM Ruediger Pluem <[email protected]> wrote:
>
> On 11/20/23 4:05 PM, Yann Ylavic wrote:
> > On Mon, Nov 20, 2023 at 3:46 PM Yann Ylavic <[email protected]> wrote:
> >>
> >> On Mon, Nov 20, 2023 at 2:33 PM Yann Ylavic <[email protected]> wrote:
> >>>
> >>> On Mon, Nov 20, 2023 at 1:57 PM Graham Leggett via dev
> >>> <[email protected]> wrote:
> >>>>
> >>>> On 20 Nov 2023, at 12:26, Ruediger Pluem <[email protected]> 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)
