Clean up `LdapAuthProvider` to properly cover different LDAP search scenarios.

Gitblit allows in its configuration to set a "manager" user (and password) which can be used
to search for the entry of a user wanting to log in. If they are both not set, an anonymous search
is attempted. In the description below, when I say "...as manager", it is either as manager or
anonymous.
So far the behaviour of Gitblit, with respect to binding to and searching in LDAP,
has been the following when a user logs in:

**bind as manager**
**search for the user**
_bind as the user_
_search for the teams_

I'll call this code flow A.

Later an additional configuration option had been added: `realm.ldap.bindpattern`.
(PR gitblit/gitblit#162) It was meant to allow for not using a manager nor anonymous binds,
by searching the directory as the user logging in.
This is done in code flow B:

**bind as manager**
_bind as user_
_search for user_
_search for teams_

Both A and B are flawed, I think. In A, it looks like a mistake to me that the binding stays with the
user after authentication. The problem that this causes is, that in LDAP server configurations
where normal users are not allowed to read groups, the team information cannot be retrieved.
I tried but failed to understand how B is supposed to work. There will always be a bind request
as either anonymous or the manager DN when the LDAP connection is created. If neither is
possible, the authentication process will fail and the user cannot log in.

When synchronizing users and teams from LDAP, the following code flow is exercised:

F:
**bind as manager**
**search for users**
**search for teams**

This patch fixes both code flows by introducing a new flow.

C:
**bind as manager**
**search for user**
_bind as user to authenticate_
**bind as manager**
**search for teams**

And it changes code flow B to the following code flow D:

_bind as user_
_search for user_
_search for teams_

With code flows A, C, D and F the following usage (and authentication) scenarios are covered.
They are described from the view of a Gitblit administrator's intent and his LDAP setup.

* Users and team should be snychronized with LDAP
	This means anonymous or a fixed account must be able to read users and groups.
	=> covered by C and F

As the above allows for authentication and is required for synchronisation, all the others below
do not cover synchronization.

* No anonymous binding allowed and no special manager binding required
	This means that users must be able to read user an group entries.
	=> covered by D

* The user DN needs to be searched, e.g. because they are not all under the same parent DN.
	This means that anonymous or a fixed account must be able to read users.
		-- anonymous or the "manager" account can also read groups
			=> covered by C
		-- anonymous or the "manager" account cannot read groups but a user can
			=> covered by A

I therefore believe that the new code will cover all common use cases. The implementation
either directly binds as the user, when `bindpattern` is not empty, or it binds anonymous or
against the manger DN to search for the user DN entry.

If it directly bound against the user DN, the user is already authenticated. It will then only check
that the user DN it found in the search is identical to the one it is currently bound against. If it
was bound against a manager DN (or anonymously) it will bind against the found user DN to
authenticate the user logging in, and will then rebind against the manager DN.

When searching for groups in LDAP, if the search fails with a result code other than SUCCESS,
the implementation will bind against the user DN, if it isn't already bound against it. It will then
repeat the search for groups under the user authorization. This is to keep backwards
compatible with the original behaviour A, in order to not break cases where the LDAP setup
would deny a manager account to search for groups but allow it for normal users.

To achieve this the implementation introduces an internal `LdapConnection` class that wraps
the connection and keeps bind state, so that a rebind as a user is possible.
This also fixes a resource leak where the connection was not closed in case that the initial bind
as the manager account did not succeed.

This commit would fix gitblit/gitblit#920

1 files changed, 284 insertions, 114 deletions

diff --git a/src/main/java/com/gitblit/auth/LdapAuthProvider.java b/src/main/java/com/gitblit/auth/LdapAuthProvider.java
index cc772e7b..e1dec48f 100644
--- a/src/main/java/com/gitblit/auth/LdapAuthProvider.java
+++ b/src/main/java/com/gitblit/auth/LdapAuthProvider.java
@@ -39,6 +39,8 @@ import com.gitblit.service.LdapSyncService;
 import com.gitblit.utils.ArrayUtils;
 import com.gitblit.utils.StringUtils;
 import com.unboundid.ldap.sdk.Attribute;
+import com.unboundid.ldap.sdk.BindRequest;
+import com.unboundid.ldap.sdk.BindResult;
 import com.unboundid.ldap.sdk.DereferencePolicy;
 import com.unboundid.ldap.sdk.ExtendedResult;
 import com.unboundid.ldap.sdk.LDAPConnection;
@@ -107,8 +109,14 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 		if (enabled) {
 			logger.info("Synchronizing with LDAP @ " + settings.getRequiredString(Keys.realm.ldap.server));
 			final boolean deleteRemovedLdapUsers = settings.getBoolean(Keys.realm.ldap.removeDeletedUsers, true);
-			LDAPConnection ldapConnection = getLdapConnection();
-			if (ldapConnection != null) {
+			LdapConnection ldapConnection = new LdapConnection();
+			if (ldapConnection.connect()) {
+				if (ldapConnection.bind() == null) {
+					ldapConnection.close();
+					logger.error("Cannot synchronize with LDAP.");
+					return;
+				}
+
 				try {
 					String accountBase = settings.getString(Keys.realm.ldap.accountBase, "");
 					String uidAttribute = settings.getString(Keys.realm.ldap.uid, "uid");
@@ -179,66 +187,6 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 		}
 	}
 
-	private LDAPConnection getLdapConnection() {
-		try {
-
-			URI ldapUrl = new URI(settings.getRequiredString(Keys.realm.ldap.server));
-			String ldapHost = ldapUrl.getHost();
-			int ldapPort = ldapUrl.getPort();
-			String bindUserName = settings.getString(Keys.realm.ldap.username, "");
-			String bindPassword = settings.getString(Keys.realm.ldap.password, "");
-
-			LDAPConnection conn;
-			if (ldapUrl.getScheme().equalsIgnoreCase("ldaps")) {
-				// SSL
-				SSLUtil sslUtil = new SSLUtil(new TrustAllTrustManager());
-				conn = new LDAPConnection(sslUtil.createSSLSocketFactory());
-				if (ldapPort == -1) {
-					ldapPort = 636;
-				}
-			} else if (ldapUrl.getScheme().equalsIgnoreCase("ldap") || ldapUrl.getScheme().equalsIgnoreCase("ldap+tls")) {
-				// no encryption or StartTLS
-				conn = new LDAPConnection();
-				 if (ldapPort == -1) {
-					 ldapPort = 389;
-				 }
-			} else {
-				logger.error("Unsupported LDAP URL scheme: " + ldapUrl.getScheme());
-				return null;
-			}
-
-			conn.connect(ldapHost, ldapPort);
-
-			if (ldapUrl.getScheme().equalsIgnoreCase("ldap+tls")) {
-				SSLUtil sslUtil = new SSLUtil(new TrustAllTrustManager());
-				ExtendedResult extendedResult = conn.processExtendedOperation(
-						new StartTLSExtendedRequest(sslUtil.createSSLContext()));
-				if (extendedResult.getResultCode() != ResultCode.SUCCESS) {
-					throw new LDAPException(extendedResult.getResultCode());
-				}
-			}
-
-			if (StringUtils.isEmpty(bindUserName) && StringUtils.isEmpty(bindPassword)) {
-				// anonymous bind
-				conn.bind(new SimpleBindRequest());
-			} else {
-				// authenticated bind
-				conn.bind(new SimpleBindRequest(bindUserName, bindPassword));
-			}
-
-			return conn;
-
-		} catch (URISyntaxException e) {
-			logger.error("Bad LDAP URL, should be in the form: ldap(s|+tls)://<server>:<port>", e);
-		} catch (GeneralSecurityException e) {
-			logger.error("Unable to create SSL Connection", e);
-		} catch (LDAPException e) {
-			logger.error("Error Connecting to LDAP", e);
-		}
-
-		return null;
-	}
-
 	/**
 	 * Credentials are defined in the LDAP server and can not be manipulated
 	 * from Gitblit.
@@ -321,23 +269,26 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 	public UserModel authenticate(String username, char[] password) {
 		String simpleUsername = getSimpleUsername(username);
 
-		LDAPConnection ldapConnection = getLdapConnection();
-		if (ldapConnection != null) {
-			try {
-				boolean alreadyAuthenticated = false;
+		LdapConnection ldapConnection = new LdapConnection();
+		if (ldapConnection.connect()) {
 
-				String bindPattern = settings.getString(Keys.realm.ldap.bindpattern, "");
-				if (!StringUtils.isEmpty(bindPattern)) {
-					try {
-						String bindUser = StringUtils.replace(bindPattern, "${username}", escapeLDAPSearchFilter(simpleUsername));
-						ldapConnection.bind(bindUser, new String(password));
+			// Try to bind either to the "manager" account,
+			// or directly to the DN of the user logging in, if realm.ldap.bindpattern is configured.
+			String passwd = new String(password);
+			BindResult bindResult = null;
+			String bindPattern = settings.getString(Keys.realm.ldap.bindpattern, "");
+			if (! StringUtils.isEmpty(bindPattern)) {
+				bindResult = ldapConnection.bind(bindPattern, simpleUsername, passwd);
+			} else {
+				bindResult = ldapConnection.bind();
+			}
+			if (bindResult == null) {
+				ldapConnection.close();
+				return null;
+			}
 
-						alreadyAuthenticated = true;
-					} catch (LDAPException e) {
-						return null;
-					}
-				}
 
+			try {
 				// Find the logging in user's DN
 				String accountBase = settings.getString(Keys.realm.ldap.accountBase, "");
 				String accountPattern = settings.getString(Keys.realm.ldap.accountPattern, "(&(objectClass=person)(sAMAccountName=${username}))");
@@ -348,7 +299,7 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 					SearchResultEntry loggingInUser = result.getSearchEntries().get(0);
 					String loggingInUserDN = loggingInUser.getDN();
 
-					if (alreadyAuthenticated || isAuthenticated(ldapConnection, loggingInUserDN, new String(password))) {
+					if (ldapConnection.isAuthenticated(loggingInUserDN, passwd)) {
 						logger.debug("LDAP authenticated: " + username);
 
 						UserModel user = null;
@@ -462,7 +413,7 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 		}
 	}
 
-	private void getTeamsFromLdap(LDAPConnection ldapConnection, String simpleUsername, SearchResultEntry loggingInUser, UserModel user) {
+	private void getTeamsFromLdap(LdapConnection ldapConnection, String simpleUsername, SearchResultEntry loggingInUser, UserModel user) {
 		String loggingInUserDN = loggingInUser.getDN();
 
 		// Clear the users team memberships - we're going to get them from LDAP
@@ -479,7 +430,7 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 			groupMemberPattern = StringUtils.replace(groupMemberPattern, "${" + userAttribute.getName() + "}", escapeLDAPSearchFilter(userAttribute.getValue()));
 		}
 
-		SearchResult teamMembershipResult = doSearch(ldapConnection, groupBase, true, groupMemberPattern, Arrays.asList("cn"));
+		SearchResult teamMembershipResult = searchTeamsInLdap(ldapConnection, groupBase, true, groupMemberPattern, Arrays.asList("cn"));
 		if (teamMembershipResult != null && teamMembershipResult.getEntryCount() > 0) {
 			for (int i = 0; i < teamMembershipResult.getEntryCount(); i++) {
 				SearchResultEntry teamEntry = teamMembershipResult.getSearchEntries().get(i);
@@ -496,12 +447,12 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 		}
 	}
 
-	private void getEmptyTeamsFromLdap(LDAPConnection ldapConnection) {
+	private void getEmptyTeamsFromLdap(LdapConnection ldapConnection) {
 		logger.info("Start fetching empty teams from ldap.");
 		String groupBase = settings.getString(Keys.realm.ldap.groupBase, "");
 		String groupMemberPattern = settings.getString(Keys.realm.ldap.groupEmptyMemberPattern, "(&(objectClass=group)(!(member=*)))");
 
-		SearchResult teamMembershipResult = doSearch(ldapConnection, groupBase, true, groupMemberPattern, null);
+		SearchResult teamMembershipResult = searchTeamsInLdap(ldapConnection, groupBase, true, groupMemberPattern, null);
 		if (teamMembershipResult != null && teamMembershipResult.getEntryCount() > 0) {
 			for (int i = 0; i < teamMembershipResult.getEntryCount(); i++) {
 				SearchResultEntry teamEntry = teamMembershipResult.getSearchEntries().get(i);
@@ -519,6 +470,30 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 		logger.info("Finished fetching empty teams from ldap.");
 	}
 
+
+	private SearchResult searchTeamsInLdap(LdapConnection ldapConnection, String base, boolean dereferenceAliases, String filter, List<String> attributes) {
+		SearchResult result = ldapConnection.search(base, dereferenceAliases, filter, attributes);
+		if (result == null) {
+			return null;
+		}
+
+		if (result.getResultCode() != ResultCode.SUCCESS) {
+			// Retry the search with user authorization in case we searched as a manager account that could not search for teams.
+			logger.debug("Rebinding as user to search for teams in LDAP");
+			result = null;
+			if (ldapConnection.rebindAsUser()) {
+				result = ldapConnection.search(base, dereferenceAliases, filter, attributes);
+				if (result.getResultCode() != ResultCode.SUCCESS) {
+					return null;
+				}
+				logger.info("Successful search after rebinding as user.");
+			}
+		}
+
+		return result;
+	}
+
+
 	private TeamModel createTeamFromLdap(SearchResultEntry teamEntry) {
 		TeamModel answer = new TeamModel(teamEntry.getAttributeValue("cn"));
 		answer.accountType = getAccountType();
@@ -527,47 +502,21 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 		return answer;
 	}
 
-	private SearchResult doSearch(LDAPConnection ldapConnection, String base, String filter) {
-		try {
-			return ldapConnection.search(base, SearchScope.SUB, filter);
-		} catch (LDAPSearchException e) {
-			logger.error("Problem Searching LDAP", e);
-
-			return null;
-		}
-	}
-
-	private SearchResult doSearch(LDAPConnection ldapConnection, String base, boolean dereferenceAliases, String filter, List<String> attributes) {
+	private SearchResult doSearch(LdapConnection ldapConnection, String base, String filter) {
 		try {
 			SearchRequest searchRequest = new SearchRequest(base, SearchScope.SUB, filter);
-			if (dereferenceAliases) {
-				searchRequest.setDerefPolicy(DereferencePolicy.SEARCHING);
-			}
-			if (attributes != null) {
-				searchRequest.setAttributes(attributes);
+			SearchResult result = ldapConnection.search(searchRequest);
+			if (result.getResultCode() != ResultCode.SUCCESS) {
+				return null;
 			}
-			return ldapConnection.search(searchRequest);
-
-		} catch (LDAPSearchException e) {
-			logger.error("Problem Searching LDAP", e);
-
-			return null;
+			return result;
 		} catch (LDAPException e) {
 			logger.error("Problem creating LDAP search", e);
 			return null;
 		}
 	}
 
-	private boolean isAuthenticated(LDAPConnection ldapConnection, String userDn, String password) {
-		try {
-			// Binding will stop any LDAP-Injection Attacks since the searched-for user needs to bind to that DN
-			ldapConnection.bind(userDn, password);
-			return true;
-		} catch (LDAPException e) {
-			logger.error("Error authenticating user", e);
-			return false;
-		}
-	}
+
 
 	/**
 	 * Returns a simple username without any domain prefixes.
@@ -585,7 +534,7 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 	}
 
 	// From: https://www.owasp.org/index.php/Preventing_LDAP_Injection_in_Java
-	public static final String escapeLDAPSearchFilter(String filter) {
+	private static final String escapeLDAPSearchFilter(String filter) {
 		StringBuilder sb = new StringBuilder();
 		for (int i = 0; i < filter.length(); i++) {
 			char curChar = filter.charAt(i);
@@ -625,4 +574,225 @@ public class LdapAuthProvider extends UsernamePasswordAuthenticationProvider {
 		}
 	}
 
+
+
+	private class LdapConnection {
+		private LDAPConnection conn;
+		private SimpleBindRequest currentBindRequest;
+		private SimpleBindRequest managerBindRequest;
+		private SimpleBindRequest userBindRequest;
+
+
+		public LdapConnection() {
+			String bindUserName = settings.getString(Keys.realm.ldap.username, "");
+			String bindPassword = settings.getString(Keys.realm.ldap.password, "");
+			if (StringUtils.isEmpty(bindUserName) && StringUtils.isEmpty(bindPassword)) {
+				this.managerBindRequest = new SimpleBindRequest();
+			}
+			this.managerBindRequest = new SimpleBindRequest(bindUserName, bindPassword);
+		}
+
+
+		boolean connect() {
+			try {
+				URI ldapUrl = new URI(settings.getRequiredString(Keys.realm.ldap.server));
+				String ldapHost = ldapUrl.getHost();
+				int ldapPort = ldapUrl.getPort();
+
+				if (ldapUrl.getScheme().equalsIgnoreCase("ldaps")) {
+					// SSL
+					SSLUtil sslUtil = new SSLUtil(new TrustAllTrustManager());
+					conn = new LDAPConnection(sslUtil.createSSLSocketFactory());
+					if (ldapPort == -1) {
+						ldapPort = 636;
+					}
+				} else if (ldapUrl.getScheme().equalsIgnoreCase("ldap") || ldapUrl.getScheme().equalsIgnoreCase("ldap+tls")) {
+					// no encryption or StartTLS
+					conn = new LDAPConnection();
+					 if (ldapPort == -1) {
+						 ldapPort = 389;
+					 }
+				} else {
+					logger.error("Unsupported LDAP URL scheme: " + ldapUrl.getScheme());
+					return false;
+				}
+
+				conn.connect(ldapHost, ldapPort);
+
+				if (ldapUrl.getScheme().equalsIgnoreCase("ldap+tls")) {
+					SSLUtil sslUtil = new SSLUtil(new TrustAllTrustManager());
+					ExtendedResult extendedResult = conn.processExtendedOperation(
+							new StartTLSExtendedRequest(sslUtil.createSSLContext()));
+					if (extendedResult.getResultCode() != ResultCode.SUCCESS) {
+						throw new LDAPException(extendedResult.getResultCode());
+					}
+				}
+
+				return true;
+
+			} catch (URISyntaxException e) {
+				logger.error("Bad LDAP URL, should be in the form: ldap(s|+tls)://<server>:<port>", e);
+			} catch (GeneralSecurityException e) {
+				logger.error("Unable to create SSL Connection", e);
+			} catch (LDAPException e) {
+				logger.error("Error Connecting to LDAP", e);
+			}
+
+			return false;
+		}
+
+
+		void close() {
+			if (conn != null) {
+				conn.close();
+			}
+		}
+
+
+		SearchResult search(SearchRequest request) {
+			try {
+				return conn.search(request);
+			} catch (LDAPSearchException e) {
+				logger.error("Problem Searching LDAP [{}]",  e.getResultCode());
+				return e.getSearchResult();
+			}
+		}
+
+
+		SearchResult search(String base, boolean dereferenceAliases, String filter, List<String> attributes) {
+			try {
+				SearchRequest searchRequest = new SearchRequest(base, SearchScope.SUB, filter);
+				if (dereferenceAliases) {
+					searchRequest.setDerefPolicy(DereferencePolicy.SEARCHING);
+				}
+				if (attributes != null) {
+					searchRequest.setAttributes(attributes);
+				}
+				SearchResult result = search(searchRequest);
+				return result;
+
+			} catch (LDAPException e) {
+				logger.error("Problem creating LDAP search", e);
+				return null;
+			}
+		}
+
+
+
+		/**
+		 * Bind using the manager credentials set in realm.ldap.username and ..password
+		 * @return A bind result, or null if binding failed.
+		 */
+		BindResult bind() {
+			BindResult result = null;
+			try {
+				result = conn.bind(managerBindRequest);
+				currentBindRequest = managerBindRequest;
+			} catch (LDAPException e) {
+				logger.error("Error authenticating to LDAP with manager account to search the directory.");
+				logger.error("  Please check your settings for realm.ldap.username and realm.ldap.password.");
+				logger.debug("  Received exception when binding to LDAP", e);
+				return null;
+			}
+			return result;
+		}
+
+
+		/**
+		 * Bind using the given credentials, by filling in the username in the given {@code bindPattern} to
+		 * create the DN.
+		 * @return A bind result, or null if binding failed.
+		 */
+		BindResult bind(String bindPattern, String simpleUsername, String password) {
+			BindResult result = null;
+			try {
+				String bindUser = StringUtils.replace(bindPattern, "${username}", escapeLDAPSearchFilter(simpleUsername));
+				SimpleBindRequest request = new SimpleBindRequest(bindUser, password);
+				result = conn.bind(request);
+				userBindRequest = request;
+				currentBindRequest = userBindRequest;
+			} catch (LDAPException e) {
+				logger.error("Error authenticating to LDAP with user account to search the directory.");
+				logger.error("  Please check your settings for realm.ldap.bindpattern.");
+				logger.debug("  Received exception when binding to LDAP", e);
+				return null;
+			}
+			return result;
+		}
+
+
+		boolean rebindAsUser() {
+			if (userBindRequest == null || currentBindRequest == userBindRequest) {
+				return false;
+			}
+			try {
+				conn.bind(userBindRequest);
+				currentBindRequest = userBindRequest;
+			} catch (LDAPException e) {
+				conn.close();
+				logger.error("Error rebinding to LDAP with user account.", e);
+				return false;
+			}
+			return true;
+		}
+
+
+		boolean isAuthenticated(String userDn, String password) {
+			verifyCurrentBinding();
+
+			// If the currently bound DN is already the DN of the logging in user, authentication has already happened
+			// during the previous bind operation. We accept this and return with the current bind left in place.
+			// This could also be changed to always retry binding as the logging in user, to make sure that the
+			// connection binding has not been tampered with in between. So far I see no way how this could happen
+			// and thus skip the repeated binding.
+			// This check also makes sure that the DN in realm.ldap.bindpattern actually matches the DN that was found
+			// when searching the user entry.
+			String boundDN = currentBindRequest.getBindDN();
+			if (boundDN != null && boundDN.equals(userDn)) {
+				return true;
+			}
+
+			// Bind a the logging in user to check for authentication.
+			// Afterwards, bind as the original bound DN again, to restore the previous authorization.
+			boolean isAuthenticated = false;
+			try {
+				// Binding will stop any LDAP-Injection Attacks since the searched-for user needs to bind to that DN
+				SimpleBindRequest ubr = new SimpleBindRequest(userDn, password);
+				conn.bind(ubr);
+				isAuthenticated = true;
+				userBindRequest = ubr;
+			} catch (LDAPException e) {
+				logger.error("Error authenticating user ({})", userDn, e);
+			}
+
+			try {
+				conn.bind(currentBindRequest);
+			} catch (LDAPException e) {
+				logger.error("Error reinstating original LDAP authorization (code {}). Team information may be inaccurate for this log in.",
+							e.getResultCode(), e);
+			}
+			return isAuthenticated;
+		}
+
+
+
+		private boolean verifyCurrentBinding() {
+			BindRequest lastBind = conn.getLastBindRequest();
+			if (lastBind == currentBindRequest) {
+				return true;
+			}
+			logger.debug("Unexpected binding in LdapConnection. {} != {}", lastBind, currentBindRequest);
+
+			String lastBoundDN = ((SimpleBindRequest)lastBind).getBindDN();
+			String boundDN = currentBindRequest.getBindDN();
+			logger.debug("Currently bound as '{}', check authentication for '{}'", lastBoundDN, boundDN);
+			if (boundDN != null && ! boundDN.equals(lastBoundDN)) {
+				logger.warn("Unexpected binding DN in LdapConnection. '{}' != '{}'.", lastBoundDN, boundDN);
+				logger.warn("Updated binding information in LDAP connection.");
+				currentBindRequest = (SimpleBindRequest)lastBind;
+				return false;
+			}
+			return true;
+		}
+	}
 }