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.
>
> 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)
