FrankChen021 commented on code in PR #19432:
URL: https://github.com/apache/druid/pull/19432#discussion_r3889505689
##########
extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authentication/validator/LDAPCredentialsValidator.java:
##########
@@ -237,22 +247,90 @@ SearchResult getLdapUserObject(BasicAuthLDAPConfig
ldapConfig, DirContext contex
ldapConfig.getBaseDn(),
StringUtils.format(ldapConfig.getUserSearch(), encodedUsername),
sc);
+ final SearchResult userResult;
try {
if (!results.hasMore()) {
return null;
}
- return results.next();
+ userResult = results.next();
}
finally {
results.close();
}
+ return userResult;
}
catch (NamingException e) {
LOG.debug(e, "Unable to find user '%s'", username);
return null;
}
}
+ private static boolean hasMemberOfAttribute(SearchResult userResult)
+ {
+ return userResult.getAttributes() != null
+ && userResult.getAttributes().get("memberOf") != null;
+ }
+
+ @SuppressWarnings("BanJNDI")
+ private void enrichWithGroupSearch(SearchResult userResult)
+ {
+ final ClassLoader currentClassLoader =
Thread.currentThread().getContextClassLoader();
+ InitialDirContext dirContext = null;
+ try {
+
Thread.currentThread().setContextClassLoader(this.getClass().getClassLoader());
+ dirContext = new InitialDirContext(bindProperties(this.ldapConfig));
+
+ final String userDn = userResult.getNameInNamespace();
+ final SearchControls sc = new SearchControls();
+ sc.setSearchScope(SearchControls.SUBTREE_SCOPE);
+ sc.setReturningAttributes(new String[]{"1.1"});
+
+ final String filter =
StringUtils.format(this.ldapConfig.getGroupSearch(), encodeForLDAP(userDn,
true));
Review Comment:
[P1] Require a user-dependent LDAP filter
StringUtils.format leaves the extra argument unused when groupSearch has no
%s placeholder. A valid filter such as (uniqueMember=*) therefore searches
every group below groupBaseDn and adds all group DNs to memberOf, granting
every user all mapped group roles. Validate that groupSearch depends on the
user DN or fail closed before performing the lookup.
##########
extensions-core/druid-basic-security/src/main/java/org/apache/druid/security/basic/authentication/validator/LDAPCredentialsValidator.java:
##########
@@ -77,7 +79,9 @@ public LDAPCredentialsValidator(
@JsonProperty("credentialIterations") Integer credentialIterations,
@JsonProperty("credentialVerifyDuration") Integer
credentialVerifyDuration,
@JsonProperty("credentialMaxDuration") Integer credentialMaxDuration,
- @JsonProperty("credentialCacheSize") Integer credentialCacheSize
+ @JsonProperty("credentialCacheSize") Integer credentialCacheSize,
+ @JsonProperty("groupBaseDn") String groupBaseDn,
Review Comment:
[P2] Preserve the existing public constructor
Adding groupBaseDn and groupSearch to the only public URL-based constructor
removes the previous 10-argument signature. Direct Java callers will fail to
compile, and already-compiled integrations can fail with NoSuchMethodError even
when reverse group lookup is unused. Keep a delegating overload for the
existing signature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
