Reject Empty Passwords in DirContextAuthenticationStrategy Implementations

A simple bind with a non-empty principal and an empty password is the
LDAP unauthenticated bind shape (RFC 4513 5.1.2), which many directories
accept as an anonymous bind and report as success. This can cause callers
to believe authentication succeeded when no password was actually verified.

Add the check to the default strategy and to those that perform a simple
bind variant or take a password as an authentication credential, so the
rejection is consistent across all entry points (LdapTemplate, LdapClient,
direct ContextSource use). Throw AuthenticationException so the caller sees
the same exception type as for any other authentication failure.

Applications that need a different bind shape can configure a different
DirContextAuthenticationStrategy.

Closes gh-407

Signed-off-by: Josh Cummings <3627351+jzheaux@users.noreply.github.com>

Author: jzheaux

core/src/main/java/org/springframework/ldap/core/support/DefaultTlsDirContextAuthenticationStrategy.java

@@ -20,6 +20,9 @@
 import javax.naming.NamingException;
 import javax.naming.ldap.LdapContext;
 
+import org.springframework.ldap.AuthenticationException;
+import org.springframework.util.StringUtils;
+
 /**
  * Default implementation of TLS authentication. Applies <code>SIMPLE</code>
  * authentication on top of the negotiated TLS session. Refer to
@@ -34,6 +37,10 @@ public class DefaultTlsDirContextAuthenticationStrategy extends AbstractTlsDirCo
 	private static final String SIMPLE_AUTHENTICATION = "simple";
 
 	protected void applyAuthentication(LdapContext ctx, String userDn, String password) throws NamingException {
+		if (StringUtils.hasLength(userDn) && !StringUtils.hasLength(password)) {
+			throw new AuthenticationException(
+					new javax.naming.AuthenticationException("password must be provided when userDn is set"));
+		}
 		ctx.addToEnvironment(Context.SECURITY_AUTHENTICATION, SIMPLE_AUTHENTICATION);
 		ctx.addToEnvironment(Context.SECURITY_PRINCIPAL, userDn);
 		ctx.addToEnvironment(Context.SECURITY_CREDENTIALS, password);

core/src/main/java/org/springframework/ldap/core/support/DigestMd5DirContextAuthenticationStrategy.java

@@ -21,6 +21,9 @@
 import javax.naming.Context;
 import javax.naming.directory.DirContext;
 
+import org.springframework.ldap.AuthenticationException;
+import org.springframework.util.StringUtils;
+
 /**
  * Authentication strategy for LDAP DIGEST-MD5 SASL mechanism.
  *
@@ -50,6 +53,10 @@ public DirContext processContextAfterCreation(DirContext ctx, String userDn, Str
 	 * setupEnvironment(java.util.Hashtable, java.lang.String, java.lang.String)
 	 */
 	public void setupEnvironment(Hashtable<String, Object> env, String userDn, String password) {
+		if (StringUtils.hasLength(userDn) && !StringUtils.hasLength(password)) {
+			throw new AuthenticationException(
+					new javax.naming.AuthenticationException("password must be provided when userDn is set"));
+		}
 		env.put(Context.SECURITY_AUTHENTICATION, DIGEST_MD5_AUTHENTICATION);
 		// userDn should be a bare username for DIGEST-MD5
 		env.put(Context.SECURITY_PRINCIPAL, userDn);

core/src/main/java/org/springframework/ldap/core/support/SimpleDirContextAuthenticationStrategy.java

@@ -21,6 +21,9 @@
 import javax.naming.Context;
 import javax.naming.directory.DirContext;
 
+import org.springframework.ldap.AuthenticationException;
+import org.springframework.util.StringUtils;
+
 /**
  * The default {@link DirContextAuthenticationStrategy} implementation, setting the
  * <code>DirContext</code> environment up for 'SIMPLE' authentication, and specifying the
@@ -40,6 +43,10 @@ public class SimpleDirContextAuthenticationStrategy implements DirContextAuthent
 	 * setupEnvironment(java.util.Hashtable, java.lang.String, java.lang.String)
 	 */
 	public void setupEnvironment(Hashtable<String, Object> env, String userDn, String password) {
+		if (StringUtils.hasLength(userDn) && !StringUtils.hasLength(password)) {
+			throw new AuthenticationException(
+					new javax.naming.AuthenticationException("password must be provided when userDn is set"));
+		}
 		env.put(Context.SECURITY_AUTHENTICATION, SIMPLE_AUTHENTICATION);
 		env.put(Context.SECURITY_PRINCIPAL, userDn);
 		env.put(Context.SECURITY_CREDENTIALS, password);

core/src/test/java/org/springframework/ldap/core/DefaultLdapClientTests.java

@@ -36,6 +36,7 @@
 
 import org.springframework.dao.EmptyResultDataAccessException;
 import org.springframework.dao.IncorrectResultSizeDataAccessException;
+import org.springframework.ldap.AuthenticationException;
 import org.springframework.ldap.LimitExceededException;
 import org.springframework.ldap.NameNotFoundException;
 import org.springframework.ldap.PartialResultException;
@@ -660,6 +661,26 @@ public void createWhenLdapTemplateThenUses() {
 		verify(ldap).getDefaultTimeLimit();
 	}
 
+	// gh-407
+	@Test
+	public void authenticateWhenPasswordIsEmptyAndDefaultStrategyThenAuthenticationException() throws Exception {
+		given(this.contextSourceMock.getReadOnlyContext()).willReturn(this.dirContextMock);
+
+		Object expectedObject = new DirContextAdapter(new BasicAttributes(), LdapUtils.newLdapName("cn=john doe"),
+				LdapUtils.newLdapName("dc=jayway, dc=se"));
+		SearchResult searchResult = new SearchResult("", expectedObject, new BasicAttributes());
+		singleSearchResult(searchControlsRecursive(), searchResult);
+
+		given(this.contextSourceMock.getContext("cn=john doe,dc=jayway,dc=se", ""))
+			.willThrow(new AuthenticationException(
+					new javax.naming.AuthenticationException("password must be provided when userDn is set")));
+
+		LdapQuery query = LdapQueryBuilder.query().base(this.nameMock).filter("(ou=somevalue)");
+		assertThatExceptionOfType(AuthenticationException.class)
+			.isThrownBy(() -> this.tested.authenticate().query(query).password("").execute());
+		verify(this.dirContextMock).close();
+	}
+
 	private void noSearchResults(SearchControls controls) throws Exception {
 		given(this.dirContextMock.search(eq(this.nameMock), eq("(ou=somevalue)"),
 				argThat(new SearchControlsMatcher(controls))))

core/src/test/java/org/springframework/ldap/core/LdapTemplateTests.java

@@ -40,6 +40,7 @@
 import org.springframework.LdapDataEntry;
 import org.springframework.dao.EmptyResultDataAccessException;
 import org.springframework.dao.IncorrectResultSizeDataAccessException;
+import org.springframework.ldap.AuthenticationException;
 import org.springframework.ldap.LimitExceededException;
 import org.springframework.ldap.NameNotFoundException;
 import org.springframework.ldap.PartialResultException;
@@ -1880,6 +1881,26 @@ public void testAuthenticateWithErrorInCallbackShouldFail() throws Exception {
 		assertThat(result).isFalse();
 	}
 
+	// gh-407
+	@Test
+	public void authenticateWhenPasswordIsEmptyAndDefaultStrategyThenReturnsFalse() throws Exception {
+		given(this.contextSourceMock.getReadOnlyContext()).willReturn(this.dirContextMock);
+
+		Object expectedObject = new DirContextAdapter(new BasicAttributes(), LdapUtils.newLdapName("cn=john doe"),
+				LdapUtils.newLdapName("dc=jayway, dc=se"));
+		SearchResult searchResult = new SearchResult("", expectedObject, new BasicAttributes());
+		singleSearchResult(searchControlsRecursive(), searchResult);
+
+		given(this.contextSourceMock.getContext("cn=john doe,dc=jayway,dc=se", ""))
+			.willThrow(new AuthenticationException(
+					new javax.naming.AuthenticationException("password must be provided when userDn is set")));
+
+		boolean result = this.tested.authenticate(this.nameMock, "(ou=somevalue)", "", this.entryContextCallbackMock);
+
+		assertThat(result).isFalse();
+		verify(this.dirContextMock).close();
+	}
+
 	private void noSearchResults(SearchControls controls) throws Exception {
 		given(this.dirContextMock.search(eq(this.nameMock), eq("(ou=somevalue)"),
 				argThat(new SearchControlsMatcher(controls))))

core/src/test/java/org/springframework/ldap/core/support/DefaultTlsDirContextAuthenticationStrategyTests.java

@@ -23,6 +23,9 @@
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnitRunner;
 
+import org.springframework.ldap.AuthenticationException;
+
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.mockito.Mockito.verify;
 
 /**
@@ -45,4 +48,16 @@ public void applyAuthenticationThenLookupInvoked() throws Exception {
 		verify(this.context).lookup("");
 	}
 
+	@Test
+	public void applyAuthenticationWhenUserDnIsSetAndPasswordIsEmptyThenAuthenticationException() {
+		assertThatExceptionOfType(AuthenticationException.class)
+			.isThrownBy(() -> this.strategy.applyAuthentication(this.context, "username", ""));
+	}
+
+	@Test
+	public void applyAuthenticationWhenUserDnIsSetAndPasswordIsNullThenAuthenticationException() {
+		assertThatExceptionOfType(AuthenticationException.class)
+			.isThrownBy(() -> this.strategy.applyAuthentication(this.context, "username", null));
+	}
+
 }

core/src/test/java/org/springframework/ldap/core/support/DigestMd5DirContextAuthenticationStrategyTests.java

@@ -0,0 +1,64 @@
+/*
+ * Copyright 2006-present the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.ldap.core.support;
+
+import java.util.Hashtable;
+
+import javax.naming.Context;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import org.springframework.ldap.AuthenticationException;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
+
+public class DigestMd5DirContextAuthenticationStrategyTests {
+
+	private DigestMd5DirContextAuthenticationStrategy tested;
+
+	@Before
+	public void setUp() {
+		this.tested = new DigestMd5DirContextAuthenticationStrategy();
+	}
+
+	@Test
+	public void setupEnvironmentSetsAuthenticationPrincipalAndCredentials() {
+		Hashtable<String, Object> env = new Hashtable<>();
+		this.tested.setupEnvironment(env, "username", "pw");
+
+		assertThat(env.get(Context.SECURITY_AUTHENTICATION)).isEqualTo("DIGEST-MD5");
+		assertThat(env.get(Context.SECURITY_PRINCIPAL)).isEqualTo("username");
+		assertThat(env.get(Context.SECURITY_CREDENTIALS)).isEqualTo("pw");
+	}
+
+	@Test
+	public void setupEnvironmentWhenUserDnIsSetAndPasswordIsEmptyThenAuthenticationException() {
+		Hashtable<String, Object> env = new Hashtable<>();
+		assertThatExceptionOfType(AuthenticationException.class)
+			.isThrownBy(() -> this.tested.setupEnvironment(env, "username", ""));
+	}
+
+	@Test
+	public void setupEnvironmentWhenUserDnIsSetAndPasswordIsNullThenAuthenticationException() {
+		Hashtable<String, Object> env = new Hashtable<>();
+		assertThatExceptionOfType(AuthenticationException.class)
+			.isThrownBy(() -> this.tested.setupEnvironment(env, "username", null));
+	}
+
+}

core/src/test/java/org/springframework/ldap/core/support/LdapContextSourceTests.java

@@ -24,9 +24,11 @@
 import org.junit.Before;
 import org.junit.Test;
 
+import org.springframework.ldap.AuthenticationException;
 import org.springframework.ldap.support.LdapUtils;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 
 /**
  * Unit tests for the LdapContextSource class.
@@ -158,6 +160,26 @@ public void testGetAuthenticatedEnv() throws Exception {
 			.isEqualTo(LdapUtils.newLdapName("dc=example,dc=se"));
 	}
 
+	@Test
+	public void getContextWhenUserDnIsSetAndPasswordIsEmptyThenAuthenticationException() {
+		this.tested.setUrl("ldap://ldap.example.com:389");
+		this.tested.setUserDn("cn=service");
+		this.tested.setPassword("secret");
+		this.tested.afterPropertiesSet();
+		assertThatExceptionOfType(AuthenticationException.class)
+			.isThrownBy(() -> this.tested.getContext("cn=admin", ""));
+	}
+
+	@Test
+	public void getContextWhenUserDnIsSetAndPasswordIsNullThenAuthenticationException() {
+		this.tested.setUrl("ldap://ldap.example.com:389");
+		this.tested.setUserDn("cn=service");
+		this.tested.setPassword("secret");
+		this.tested.afterPropertiesSet();
+		assertThatExceptionOfType(AuthenticationException.class)
+			.isThrownBy(() -> this.tested.getContext("cn=admin", null));
+	}
+
 	@Test
 	public void testGetAnonymousEnvWhenCacheIsOff() throws Exception {
 		this.tested.setBase("dc=example,dc=se");

core/src/test/java/org/springframework/ldap/core/support/SimpleDirContextAuthenticationStrategyTests.java

@@ -23,7 +23,10 @@
 import org.junit.Before;
 import org.junit.Test;
 
+import org.springframework.ldap.AuthenticationException;
+
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 
 public class SimpleDirContextAuthenticationStrategyTests {
 
@@ -52,4 +55,27 @@ public void testProcessContextAfterCreation() {
 		assertThat(env.isEmpty()).isTrue();
 	}
 
+	@Test
+	public void setupEnvironmentWhenUserDnIsSetAndPasswordIsEmptyThenAuthenticationException() {
+		Hashtable<String, Object> env = new Hashtable<>();
+		assertThatExceptionOfType(AuthenticationException.class)
+			.isThrownBy(() -> this.tested.setupEnvironment(env, "cn=John Doe", ""));
+	}
+
+	@Test
+	public void setupEnvironmentWhenUserDnIsSetAndPasswordIsNullThenAuthenticationException() {
+		Hashtable<String, Object> env = new Hashtable<>();
+		assertThatExceptionOfType(AuthenticationException.class)
+			.isThrownBy(() -> this.tested.setupEnvironment(env, "cn=John Doe", null));
+	}
+
+	@Test
+	public void setupEnvironmentWhenUserDnAndPasswordAreEmptyThenSucceeds() {
+		Hashtable<String, Object> env = new Hashtable<>();
+		this.tested.setupEnvironment(env, "", "");
+		assertThat(env.get(Context.SECURITY_AUTHENTICATION)).isEqualTo("simple");
+		assertThat(env.get(Context.SECURITY_PRINCIPAL)).isEqualTo("");
+		assertThat(env.get(Context.SECURITY_CREDENTIALS)).isEqualTo("");
+	}
+
 }