Merge pull request #1149 from fzs/fixLDAPbinding
Fix LDAP binding strategies
diff --git a/src/main/distrib/data/defaults.properties b/src/main/distrib/data/defaults.properties
index 208fd99..0416634 100644
--- a/src/main/distrib/data/defaults.properties
+++ b/src/main/distrib/data/defaults.properties
@@ -1812,6 +1812,10 @@
realm.ldap.server = ldap://localhost
# Login username for LDAP searches.
+# This is usually a user with permissions to search LDAP users and groups.
+# It must have at least have the permission to search users. If it does not
+# have permission to search groups, the normal user logging in must have
+# the permission in LDAP to search groups.
# If this value is unspecified, anonymous LDAP login will be used.
#
# e.g. mydomain\\username
@@ -1824,8 +1828,14 @@
# SINCE 1.0.0
realm.ldap.password = password
-# Bind pattern for Authentication.
-# Allow to directly authenticate an user without LDAP Searches.
+# Bind pattern for user authentication.
+# Allow to directly authenticate an user without searching for it in LDAP.
+# Use this if the LDAP server does not allow anonymous access and you don't
+# want to use a specific account to run searches. When set, it will override
+# the settings realm.ldap.username and realm.ldap.password.
+# This requires that all relevant user entries are children to the same DN,
+# and that logging users have permission to search for their groups in LDAP.
+# This will disable synchronization as a specific LDAP account is needed for that.
#
# e.g. CN=${username},OU=Users,OU=UserControl,OU=MyOrganization,DC=MyDomain
#
@@ -1941,6 +1951,9 @@
realm.ldap.uid = uid
# Defines whether to synchronize all LDAP users and teams into the user service
+# This requires either anonymous LDAP access or that a specific account is set
+# in realm.ldap.username and realm.ldap.password, that has permission to read
+# users and groups in LDAP.
#
# Valid values: true, false
# If left blank, false is assumed
diff --git a/src/main/java/com/gitblit/auth/LdapAuthProvider.java b/src/main/java/com/gitblit/auth/LdapAuthProvider.java
index cc772e7..e1dec48 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.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 @@
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 @@
}
}
- 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 UserModel authenticate(String username, char[] password) {
String simpleUsername = getSimpleUsername(username);
- LDAPConnection ldapConnection = getLdapConnection();
- if (ldapConnection != null) {
+ LdapConnection ldapConnection = new LdapConnection();
+ if (ldapConnection.connect()) {
+
+ // 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;
+ }
+
+
try {
- boolean alreadyAuthenticated = false;
-
- 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));
-
- alreadyAuthenticated = true;
- } catch (LDAPException e) {
- return null;
- }
- }
-
// 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 @@
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 @@
}
}
- 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 @@
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 @@
}
}
- 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 @@
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 @@
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);
+ SearchResult result = ldapConnection.search(searchRequest);
+ if (result.getResultCode() != ResultCode.SUCCESS) {
+ return null;
}
- if (attributes != null) {
- searchRequest.setAttributes(attributes);
- }
- 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 @@
}
// 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 @@
}
}
+
+
+ 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;
+ }
+ }
}
diff --git a/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java b/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java
index 84dd138..2ade681 100644
--- a/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java
+++ b/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java
@@ -16,17 +16,26 @@
*/
package com.gitblit.tests;
+import static org.junit.Assume.*;
+
import java.io.File;
-import java.io.FileInputStream;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.EnumSet;
import java.util.HashMap;
import java.util.Map;
import org.apache.commons.io.FileUtils;
+import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TemporaryFolder;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
import com.gitblit.Constants.AccountType;
import com.gitblit.IStoredSettings;
@@ -43,9 +52,24 @@
import com.gitblit.utils.XssFilter.AllowXssFilter;
import com.unboundid.ldap.listener.InMemoryDirectoryServer;
import com.unboundid.ldap.listener.InMemoryDirectoryServerConfig;
+import com.unboundid.ldap.listener.InMemoryDirectoryServerSnapshot;
import com.unboundid.ldap.listener.InMemoryListenerConfig;
+import com.unboundid.ldap.listener.interceptor.InMemoryInterceptedRequest;
+import com.unboundid.ldap.listener.interceptor.InMemoryInterceptedResult;
+import com.unboundid.ldap.listener.interceptor.InMemoryInterceptedSearchEntry;
+import com.unboundid.ldap.listener.interceptor.InMemoryInterceptedSearchRequest;
+import com.unboundid.ldap.listener.interceptor.InMemoryInterceptedSearchResult;
+import com.unboundid.ldap.listener.interceptor.InMemoryInterceptedSimpleBindResult;
+import com.unboundid.ldap.listener.interceptor.InMemoryOperationInterceptor;
+import com.unboundid.ldap.sdk.BindRequest;
+import com.unboundid.ldap.sdk.BindResult;
+import com.unboundid.ldap.sdk.LDAPException;
+import com.unboundid.ldap.sdk.LDAPResult;
+import com.unboundid.ldap.sdk.OperationType;
+import com.unboundid.ldap.sdk.ResultCode;
import com.unboundid.ldap.sdk.SearchResult;
import com.unboundid.ldap.sdk.SearchScope;
+import com.unboundid.ldap.sdk.SimpleBindRequest;
import com.unboundid.ldif.LDIFReader;
/**
@@ -55,19 +79,71 @@
* @author jcrygier
*
*/
+@RunWith(Parameterized.class)
public class LdapAuthenticationTest extends GitblitUnitTest {
- @Rule
- public TemporaryFolder folder = new TemporaryFolder();
private static final String RESOURCE_DIR = "src/test/resources/ldap/";
+ private static final String DIRECTORY_MANAGER = "cn=Directory Manager";
+ private static final String USER_MANAGER = "cn=UserManager";
+ private static final String ACCOUNT_BASE = "OU=Users,OU=UserControl,OU=MyOrganization,DC=MyDomain";
+ private static final String GROUP_BASE = "OU=Groups,OU=UserControl,OU=MyOrganization,DC=MyDomain";
- private File usersConf;
- private LdapAuthProvider ldap;
+ /**
+ * Enumeration of different test modes, representing different use scenarios.
+ * With ANONYMOUS anonymous binds are used to search LDAP.
+ * DS_MANAGER will use a DIRECTORY_MANAGER to search LDAP. Normal users are prohibited to search the DS.
+ * With USR_MANAGER, a USER_MANAGER account is used to search in LDAP. This account can only search users
+ * but not groups. Normal users can search groups, though.
+ *
+ */
+ enum AuthMode {
+ ANONYMOUS(1389),
+ DS_MANAGER(2389),
+ USR_MANAGER(3389);
- static int ldapPort = 1389;
- private static InMemoryDirectoryServer ds;
+ private int ldapPort;
+ private InMemoryDirectoryServer ds;
+ private InMemoryDirectoryServerSnapshot dsSnapshot;
+
+ AuthMode(int port) {
+ this.ldapPort = port;
+ }
+
+ int ldapPort() {
+ return this.ldapPort;
+ }
+
+ void setDS(InMemoryDirectoryServer ds) {
+ if (this.ds == null) {
+ this.ds = ds;
+ this.dsSnapshot = ds.createSnapshot();
+ };
+ }
+
+ InMemoryDirectoryServer getDS() {
+ return ds;
+ }
+
+ void restoreSnapshot() {
+ ds.restoreSnapshot(dsSnapshot);
+ }
+ };
+
+
+
+ @Parameter
+ public AuthMode authMode;
+
+ @Rule
+ public TemporaryFolder folder = new TemporaryFolder();
+
+ private File usersConf;
+
+
+
+ private LdapAuthProvider ldap;
private IUserManager userManager;
@@ -75,21 +151,82 @@
private MemorySettings settings;
- @BeforeClass
- public static void createInMemoryLdapServer() throws Exception {
- InMemoryDirectoryServerConfig config = new InMemoryDirectoryServerConfig("dc=MyDomain");
- config.addAdditionalBindCredentials("cn=Directory Manager", "password");
- config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("default", ldapPort));
- config.setSchema(null);
- ds = new InMemoryDirectoryServer(config);
- ds.startListening();
+ /**
+ * Run the tests with each authentication scenario once.
+ */
+ @Parameters(name = "{0}")
+ public static Collection<Object[]> data() {
+ return Arrays.asList(new Object[][] { {AuthMode.ANONYMOUS}, {AuthMode.DS_MANAGER}, {AuthMode.USR_MANAGER} });
}
+
+
+ /**
+ * Create three different in memory DS.
+ *
+ * Each DS has a different configuration:
+ * The first allows anonymous binds.
+ * The second requires authentication for all operations. It will only allow the DIRECTORY_MANAGER account
+ * to search for users and groups.
+ * The third one is like the second, but it allows users to search for users and groups, and restricts the
+ * USER_MANAGER from searching for groups.
+ */
+ @BeforeClass
+ public static void init() throws Exception {
+ InMemoryDirectoryServer ds;
+ InMemoryDirectoryServerConfig config = createInMemoryLdapServerConfig(AuthMode.ANONYMOUS);
+ config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("default", AuthMode.ANONYMOUS.ldapPort()));
+ ds = createInMemoryLdapServer(config);
+ AuthMode.ANONYMOUS.setDS(ds);
+
+
+ config = createInMemoryLdapServerConfig(AuthMode.DS_MANAGER);
+ config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("default", AuthMode.DS_MANAGER.ldapPort()));
+ config.setAuthenticationRequiredOperationTypes(EnumSet.allOf(OperationType.class));
+ ds = createInMemoryLdapServer(config);
+ AuthMode.DS_MANAGER.setDS(ds);
+
+
+ config = createInMemoryLdapServerConfig(AuthMode.USR_MANAGER);
+ config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("default", AuthMode.USR_MANAGER.ldapPort()));
+ config.setAuthenticationRequiredOperationTypes(EnumSet.allOf(OperationType.class));
+ ds = createInMemoryLdapServer(config);
+ AuthMode.USR_MANAGER.setDS(ds);
+
+ }
+
+ @AfterClass
+ public static void destroy() throws Exception {
+ for (AuthMode am : AuthMode.values()) {
+ am.getDS().shutDown(true);
+ }
+ }
+
+ public static InMemoryDirectoryServer createInMemoryLdapServer(InMemoryDirectoryServerConfig config) throws Exception {
+ InMemoryDirectoryServer imds = new InMemoryDirectoryServer(config);
+ imds.importFromLDIF(true, RESOURCE_DIR + "sampledata.ldif");
+ imds.startListening();
+ return imds;
+ }
+
+ public static InMemoryDirectoryServerConfig createInMemoryLdapServerConfig(AuthMode authMode) throws Exception {
+ InMemoryDirectoryServerConfig config = new InMemoryDirectoryServerConfig("dc=MyDomain");
+ config.addAdditionalBindCredentials(DIRECTORY_MANAGER, "password");
+ config.addAdditionalBindCredentials(USER_MANAGER, "passwd");
+ config.setSchema(null);
+
+ config.addInMemoryOperationInterceptor(new AccessInterceptor(authMode));
+
+ return config;
+ }
+
+
+
@Before
- public void init() throws Exception {
- ds.clear();
- ds.importFromLDIF(true, new LDIFReader(new FileInputStream(RESOURCE_DIR + "sampledata.ldif")));
+ public void setup() throws Exception {
+ authMode.restoreSnapshot();
+
usersConf = folder.newFile("users.conf");
FileUtils.copyFile(new File(RESOURCE_DIR + "users.conf"), usersConf);
settings = getSettings();
@@ -117,15 +254,30 @@
private MemorySettings getSettings() {
Map<String, Object> backingMap = new HashMap<String, Object>();
backingMap.put(Keys.realm.userService, usersConf.getAbsolutePath());
- backingMap.put(Keys.realm.ldap.server, "ldap://localhost:" + ldapPort);
-// backingMap.put(Keys.realm.ldap.domain, "");
- backingMap.put(Keys.realm.ldap.username, "cn=Directory Manager");
- backingMap.put(Keys.realm.ldap.password, "password");
-// backingMap.put(Keys.realm.ldap.backingUserService, "users.conf");
+ switch(authMode) {
+ case ANONYMOUS:
+ backingMap.put(Keys.realm.ldap.server, "ldap://localhost:" + authMode.ldapPort());
+ backingMap.put(Keys.realm.ldap.username, "");
+ backingMap.put(Keys.realm.ldap.password, "");
+ break;
+ case DS_MANAGER:
+ backingMap.put(Keys.realm.ldap.server, "ldap://localhost:" + authMode.ldapPort());
+ backingMap.put(Keys.realm.ldap.username, DIRECTORY_MANAGER);
+ backingMap.put(Keys.realm.ldap.password, "password");
+ break;
+ case USR_MANAGER:
+ backingMap.put(Keys.realm.ldap.server, "ldap://localhost:" + authMode.ldapPort());
+ backingMap.put(Keys.realm.ldap.username, USER_MANAGER);
+ backingMap.put(Keys.realm.ldap.password, "passwd");
+ break;
+ default:
+ throw new RuntimeException("Unimplemented AuthMode case!");
+
+ }
backingMap.put(Keys.realm.ldap.maintainTeams, "true");
- backingMap.put(Keys.realm.ldap.accountBase, "OU=Users,OU=UserControl,OU=MyOrganization,DC=MyDomain");
+ backingMap.put(Keys.realm.ldap.accountBase, ACCOUNT_BASE);
backingMap.put(Keys.realm.ldap.accountPattern, "(&(objectClass=person)(sAMAccountName=${username}))");
- backingMap.put(Keys.realm.ldap.groupBase, "OU=Groups,OU=UserControl,OU=MyOrganization,DC=MyDomain");
+ backingMap.put(Keys.realm.ldap.groupBase, GROUP_BASE);
backingMap.put(Keys.realm.ldap.groupMemberPattern, "(&(objectClass=group)(member=${dn}))");
backingMap.put(Keys.realm.ldap.admins, "UserThree @Git_Admins \"@Git Admins\"");
backingMap.put(Keys.realm.ldap.displayName, "displayName");
@@ -136,6 +288,8 @@
return ms;
}
+
+
@Test
public void testAuthenticate() {
UserModel userOneModel = ldap.authenticate("UserOne", "userOnePassword".toCharArray());
@@ -159,6 +313,13 @@
assertNotNull(userThreeModel.getTeam("git_users"));
assertNull(userThreeModel.getTeam("git_admins"));
assertTrue(userThreeModel.canAdmin);
+
+ UserModel userFourModel = ldap.authenticate("UserFour", "userFourPassword".toCharArray());
+ assertNotNull(userFourModel);
+ assertNotNull(userFourModel.getTeam("git_users"));
+ assertNull(userFourModel.getTeam("git_admins"));
+ assertNull(userFourModel.getTeam("git admins"));
+ assertFalse(userFourModel.canAdmin);
}
@Test
@@ -204,13 +365,13 @@
@Test
public void checkIfUsersConfContainsAllUsersFromSampleDataLdif() throws Exception {
- SearchResult searchResult = ds.search("OU=Users,OU=UserControl,OU=MyOrganization,DC=MyDomain", SearchScope.SUB, "objectClass=person");
+ SearchResult searchResult = getDS().search(ACCOUNT_BASE, SearchScope.SUB, "objectClass=person");
assertEquals("Number of ldap users in gitblit user model", searchResult.getEntryCount(), countLdapUsersInUserManager());
}
@Test
public void addingUserInLdapShouldNotUpdateGitBlitUsersAndGroups() throws Exception {
- ds.addEntries(LDIFReader.readEntries(RESOURCE_DIR + "adduser.ldif"));
+ getDS().addEntries(LDIFReader.readEntries(RESOURCE_DIR + "adduser.ldif"));
ldap.sync();
assertEquals("Number of ldap users in gitblit user model", 5, countLdapUsersInUserManager());
}
@@ -218,22 +379,25 @@
@Test
public void addingUserInLdapShouldUpdateGitBlitUsersAndGroups() throws Exception {
settings.put(Keys.realm.ldap.synchronize, "true");
- ds.addEntries(LDIFReader.readEntries(RESOURCE_DIR + "adduser.ldif"));
+ getDS().addEntries(LDIFReader.readEntries(RESOURCE_DIR + "adduser.ldif"));
ldap.sync();
assertEquals("Number of ldap users in gitblit user model", 6, countLdapUsersInUserManager());
}
@Test
public void addingGroupsInLdapShouldNotUpdateGitBlitUsersAndGroups() throws Exception {
- ds.addEntries(LDIFReader.readEntries(RESOURCE_DIR + "addgroup.ldif"));
+ getDS().addEntries(LDIFReader.readEntries(RESOURCE_DIR + "addgroup.ldif"));
ldap.sync();
assertEquals("Number of ldap groups in gitblit team model", 0, countLdapTeamsInUserManager());
}
@Test
public void addingGroupsInLdapShouldUpdateGitBlitUsersAndGroups() throws Exception {
+ // This test only makes sense if the authentication mode allows for synchronization.
+ assumeTrue(authMode == AuthMode.ANONYMOUS || authMode == AuthMode.DS_MANAGER);
+
settings.put(Keys.realm.ldap.synchronize, "true");
- ds.addEntries(LDIFReader.readEntries(RESOURCE_DIR + "addgroup.ldif"));
+ getDS().addEntries(LDIFReader.readEntries(RESOURCE_DIR + "addgroup.ldif"));
ldap.sync();
assertEquals("Number of ldap groups in gitblit team model", 1, countLdapTeamsInUserManager());
}
@@ -261,11 +425,21 @@
assertNotNull(userThreeModel.getTeam("git_users"));
assertNull(userThreeModel.getTeam("git_admins"));
assertTrue(userThreeModel.canAdmin);
+
+ UserModel userFourModel = auth.authenticate("UserFour", "userFourPassword".toCharArray(), null);
+ assertNotNull(userFourModel);
+ assertNotNull(userFourModel.getTeam("git_users"));
+ assertNull(userFourModel.getTeam("git_admins"));
+ assertNull(userFourModel.getTeam("git admins"));
+ assertFalse(userFourModel.canAdmin);
}
@Test
public void testBindWithUser() {
- settings.put(Keys.realm.ldap.bindpattern, "CN=${username},OU=US,OU=Users,OU=UserControl,OU=MyOrganization,DC=MyDomain");
+ // This test only makes sense if the user is not prevented from reading users and teams.
+ assumeTrue(authMode != AuthMode.DS_MANAGER);
+
+ settings.put(Keys.realm.ldap.bindpattern, "CN=${username},OU=US," + ACCOUNT_BASE);
settings.put(Keys.realm.ldap.username, "");
settings.put(Keys.realm.ldap.password, "");
@@ -276,6 +450,12 @@
assertNull(userOneModelFailedAuth);
}
+
+ private InMemoryDirectoryServer getDS()
+ {
+ return authMode.getDS();
+ }
+
private int countLdapUsersInUserManager() {
int ldapAccountCount = 0;
for (UserModel userModel : userManager.getAllUsers()) {
@@ -296,4 +476,120 @@
return ldapAccountCount;
}
+
+
+
+ /**
+ * Operation interceptor for the in memory DS. This interceptor
+ * implements access restrictions for certain user/DN combinations.
+ *
+ * The USER_MANAGER is only allowed to search for users, but not for groups.
+ * This is to test the original behaviour where the teams were searched under
+ * the user binding.
+ * When running in a DIRECTORY_MANAGER scenario, only the manager account
+ * is allowed to search for users and groups, while a normal user may not do so.
+ * This tests the scenario where a normal user cannot read teams and thus the
+ * manager account needs to be used for all searches.
+ *
+ */
+ private static class AccessInterceptor extends InMemoryOperationInterceptor {
+ AuthMode authMode;
+ Map<Long,String> lastSuccessfulBindDN = new HashMap<>();
+ Map<Long,Boolean> resultProhibited = new HashMap<>();
+
+ public AccessInterceptor(AuthMode authMode) {
+ this.authMode = authMode;
+ }
+
+
+ @Override
+ public void processSimpleBindResult(InMemoryInterceptedSimpleBindResult bind) {
+ BindResult result = bind.getResult();
+ if (result.getResultCode() == ResultCode.SUCCESS) {
+ BindRequest bindRequest = bind.getRequest();
+ lastSuccessfulBindDN.put(bind.getConnectionID(), ((SimpleBindRequest)bindRequest).getBindDN());
+ resultProhibited.remove(bind.getConnectionID());
+ }
+ }
+
+
+
+ @Override
+ public void processSearchRequest(InMemoryInterceptedSearchRequest request) throws LDAPException {
+ String bindDN = getLastBindDN(request);
+
+ if (USER_MANAGER.equals(bindDN)) {
+ if (request.getRequest().getBaseDN().endsWith(GROUP_BASE)) {
+ throw new LDAPException(ResultCode.NO_SUCH_OBJECT);
+ }
+ }
+ else if(authMode == AuthMode.DS_MANAGER && !DIRECTORY_MANAGER.equals(bindDN)) {
+ throw new LDAPException(ResultCode.NO_SUCH_OBJECT);
+ }
+ }
+
+
+ @Override
+ public void processSearchEntry(InMemoryInterceptedSearchEntry entry) {
+ String bindDN = getLastBindDN(entry);
+
+ boolean prohibited = false;
+
+ if (USER_MANAGER.equals(bindDN)) {
+ if (entry.getSearchEntry().getDN().endsWith(GROUP_BASE)) {
+ prohibited = true;
+ }
+ }
+ else if(authMode == AuthMode.DS_MANAGER && !DIRECTORY_MANAGER.equals(bindDN)) {
+ prohibited = true;
+ }
+
+ if (prohibited) {
+ // Found entry prohibited for bound user. Setting entry to null.
+ entry.setSearchEntry(null);
+ resultProhibited.put(entry.getConnectionID(), Boolean.TRUE);
+ }
+ }
+
+ @Override
+ public void processSearchResult(InMemoryInterceptedSearchResult result) {
+ String bindDN = getLastBindDN(result);
+
+ boolean prohibited = false;
+
+ Boolean rspb = resultProhibited.get(result.getConnectionID());
+ if (USER_MANAGER.equals(bindDN)) {
+ if (rspb != null && rspb) {
+ prohibited = true;
+ }
+ }
+ else if(authMode == AuthMode.DS_MANAGER && !DIRECTORY_MANAGER.equals(bindDN)) {
+ if (rspb != null && rspb) {
+ prohibited = true;
+ }
+ }
+
+ if (prohibited) {
+ // Result prohibited for bound user. Returning error
+ result.setResult(new LDAPResult(result.getMessageID(), ResultCode.INSUFFICIENT_ACCESS_RIGHTS));
+ resultProhibited.remove(result.getConnectionID());
+ }
+ }
+
+ private String getLastBindDN(InMemoryInterceptedResult result) {
+ String bindDN = lastSuccessfulBindDN.get(result.getConnectionID());
+ if (bindDN == null) {
+ return "UNKNOWN";
+ }
+ return bindDN;
+ }
+ private String getLastBindDN(InMemoryInterceptedRequest request) {
+ String bindDN = lastSuccessfulBindDN.get(request.getConnectionID());
+ if (bindDN == null) {
+ return "UNKNOWN";
+ }
+ return bindDN;
+ }
+ }
+
}
