From 8d27912b0f7f0a67a929671a9c6ff3c8052e3497 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Wed, 23 Nov 2016 02:48:38 +0100 Subject: [PATCH 1/7] Create base unit test class for LDAP tests. Extract the creation of the in-memory servers and the interceptor code to a base class that LDAP related unit tests can extend to have the servers available. --- .../gitblit/tests/LdapAuthenticationTest.java | 339 +-------------- .../com/gitblit/tests/LdapBasedUnitTest.java | 409 ++++++++++++++++++ 2 files changed, 416 insertions(+), 332 deletions(-) create mode 100644 src/test/java/com/gitblit/tests/LdapBasedUnitTest.java diff --git a/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java b/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java index b7a77fc24..4f79edfb8 100644 --- a/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java +++ b/src/test/java/com/gitblit/tests/LdapAuthenticationTest.java @@ -18,25 +18,10 @@ import static org.junit.Assume.*; -import java.io.File; -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; import com.gitblit.Keys; @@ -50,26 +35,8 @@ import com.gitblit.tests.mock.MemorySettings; import com.gitblit.utils.XssFilter; 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; /** @@ -80,68 +47,7 @@ * */ @RunWith(Parameterized.class) -public class LdapAuthenticationTest extends GitblitUnitTest { - - 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"; - - - /** - * 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); - - - 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; - - +public class LdapAuthenticationTest extends LdapBasedUnitTest { private LdapAuthProvider ldap; @@ -149,87 +55,9 @@ void restoreSnapshot() { private AuthenticationManager auth; - private MemorySettings settings; - - - /** - * Run the tests with each authentication scenario once. - */ - @Parameters(name = "{0}") - public static Collection 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 setup() throws Exception { - authMode.restoreSnapshot(); - - usersConf = folder.newFile("users.conf"); - FileUtils.copyFile(new File(RESOURCE_DIR + "users.conf"), usersConf); - settings = getSettings(); ldap = newLdapAuthentication(settings); auth = newAuthenticationManager(settings); } @@ -251,45 +79,6 @@ private AuthenticationManager newAuthenticationManager(IStoredSettings settings) return auth; } - private MemorySettings getSettings() { - Map backingMap = new HashMap(); - backingMap.put(Keys.realm.userService, usersConf.getAbsolutePath()); - 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, ACCOUNT_BASE); - backingMap.put(Keys.realm.ldap.accountPattern, "(&(objectClass=person)(sAMAccountName=${username}))"); - 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"); - backingMap.put(Keys.realm.ldap.email, "email"); - backingMap.put(Keys.realm.ldap.uid, "sAMAccountName"); - - MemorySettings ms = new MemorySettings(backingMap); - return ms; - } - - - @Test public void testAuthenticate() { UserModel userOneModel = ldap.authenticate("UserOne", "userOnePassword".toCharArray()); @@ -708,10 +497,12 @@ public void testBindWithUser() { } - private InMemoryDirectoryServer getDS() - { - return authMode.getDS(); - } + + + + + + private int countLdapUsersInUserManager() { int ldapAccountCount = 0; @@ -733,120 +524,4 @@ private int countLdapTeamsInUserManager() { 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 lastSuccessfulBindDN = new HashMap<>(); - Map 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; - } - } - } diff --git a/src/test/java/com/gitblit/tests/LdapBasedUnitTest.java b/src/test/java/com/gitblit/tests/LdapBasedUnitTest.java new file mode 100644 index 000000000..cf3ab1fa0 --- /dev/null +++ b/src/test/java/com/gitblit/tests/LdapBasedUnitTest.java @@ -0,0 +1,409 @@ +package com.gitblit.tests; + +import java.io.File; +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.rules.TemporaryFolder; +import org.junit.runners.Parameterized.Parameter; +import org.junit.runners.Parameterized.Parameters; + +import com.gitblit.Keys; +import com.gitblit.tests.mock.MemorySettings; +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.SimpleBindRequest; + + + +/** + * Base class for Unit (/Integration) tests that test going against an + * in-memory UnboundID LDAP server. + * + * This base class creates separate in-memory LDAP servers for different scenarios: + * - ANONYMOUS: anonymous bind to LDAP. + * - DS_MANAGER: The DIRECTORY_MANAGER is set as DN to bind as an admin. + * Normal users are prohibited to search the DS, they can only bind. + * - USR_MANAGER: The USER_MANAGER is set as DN to bind as an admin. + * This account can only search users but not groups. Normal users can search groups. + * + * @author Florian Zschocke + * + */ +public abstract class LdapBasedUnitTest extends GitblitUnitTest { + + protected 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"; + protected 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"; + protected static final String DN_USER_ONE = "CN=UserOne,OU=US," + ACCOUNT_BASE; + protected static final String DN_USER_TWO = "CN=UserTwo,OU=US," + ACCOUNT_BASE; + protected static final String DN_USER_THREE = "CN=UserThree,OU=Canada," + ACCOUNT_BASE; + + + /** + * 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. + * + */ + protected enum AuthMode { + ANONYMOUS(1389), + DS_MANAGER(2389), + USR_MANAGER(3389); + + + private int ldapPort; + private InMemoryDirectoryServer ds; + private InMemoryDirectoryServerSnapshot dsSnapshot; + private BindTracker bindTracker; + + 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 setBindTracker(BindTracker bindTracker) { + this.bindTracker = bindTracker; + } + + BindTracker getBindTracker() { + return bindTracker; + } + + void restoreSnapshot() { + ds.restoreSnapshot(dsSnapshot); + } + } + + @Parameter + public AuthMode authMode = AuthMode.ANONYMOUS; + + @Rule + public TemporaryFolder folder = new TemporaryFolder(); + + + protected File usersConf; + + protected MemorySettings settings; + + + /** + * Run the tests with each authentication scenario once. + */ + @Parameters(name = "{0}") + public static Collection 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 ldapInit() 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); + + authMode.setBindTracker(new BindTracker()); + config.addInMemoryOperationInterceptor(authMode.getBindTracker()); + config.addInMemoryOperationInterceptor(new AccessInterceptor(authMode)); + + return config; + } + + + + @Before + public void setupBase() throws Exception { + authMode.restoreSnapshot(); + authMode.getBindTracker().reset(); + + usersConf = folder.newFile("users.conf"); + FileUtils.copyFile(new File(RESOURCE_DIR + "users.conf"), usersConf); + settings = getSettings(); + } + + + protected InMemoryDirectoryServer getDS() { + return authMode.getDS(); + } + + + + protected MemorySettings getSettings() { + Map backingMap = new HashMap(); + backingMap.put(Keys.realm.userService, usersConf.getAbsolutePath()); + 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, ACCOUNT_BASE); + backingMap.put(Keys.realm.ldap.accountPattern, "(&(objectClass=person)(sAMAccountName=${username}))"); + 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"); + backingMap.put(Keys.realm.ldap.email, "email"); + backingMap.put(Keys.realm.ldap.uid, "sAMAccountName"); + + MemorySettings ms = new MemorySettings(backingMap); + return ms; + } + + + + + /** + * Operation interceptor for the in memory DS. This interceptor + * tracks bind requests. + * + */ + protected static class BindTracker extends InMemoryOperationInterceptor { + private Map lastSuccessfulBindDNs = new HashMap<>(); + private String lastSuccessfulBindDN; + + + @Override + public void processSimpleBindResult(InMemoryInterceptedSimpleBindResult bind) { + BindResult result = bind.getResult(); + if (result.getResultCode() == ResultCode.SUCCESS) { + BindRequest bindRequest = bind.getRequest(); + lastSuccessfulBindDNs.put(bind.getMessageID(), ((SimpleBindRequest)bindRequest).getBindDN()); + lastSuccessfulBindDN = ((SimpleBindRequest)bindRequest).getBindDN(); + } + } + + String getLastSuccessfulBindDN() { + return lastSuccessfulBindDN; + } + + String getLastSuccessfulBindDN(int messageID) { + return lastSuccessfulBindDNs.get(messageID); + } + + void reset() { + lastSuccessfulBindDNs = new HashMap<>(); + lastSuccessfulBindDN = null; + } + } + + + + /** + * 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. + * + */ + protected static class AccessInterceptor extends InMemoryOperationInterceptor { + AuthMode authMode; + Map lastSuccessfulBindDN = new HashMap<>(); + Map 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; + } + } + +} From 967c2422591b70a82bd8fc991e87088e880f5024 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Wed, 23 Nov 2016 02:59:39 +0100 Subject: [PATCH 2/7] Extract LdapConnection into new class from LdapAuthProvider Extract the inner class `LdapConnection` from the `LdapAuthProvider` into a separate class, so that it can be used from multiple classes that have to connect to an LDAP directory. The new class is placed into the new package `com.gitblit.ldap`, since it isn't specific to authentication. --- .../com/gitblit/auth/LdapAuthProvider.java | 275 +---------------- .../java/com/gitblit/ldap/LdapConnection.java | 288 ++++++++++++++++++ .../com/gitblit/tests/LdapConnectionTest.java | 248 +++++++++++++++ 3 files changed, 543 insertions(+), 268 deletions(-) create mode 100644 src/main/java/com/gitblit/ldap/LdapConnection.java create mode 100644 src/test/java/com/gitblit/tests/LdapConnectionTest.java diff --git a/src/main/java/com/gitblit/auth/LdapAuthProvider.java b/src/main/java/com/gitblit/auth/LdapAuthProvider.java index 19fd46325..8a326cdc4 100644 --- a/src/main/java/com/gitblit/auth/LdapAuthProvider.java +++ b/src/main/java/com/gitblit/auth/LdapAuthProvider.java @@ -16,9 +16,6 @@ */ package com.gitblit.auth; -import java.net.URI; -import java.net.URISyntaxException; -import java.security.GeneralSecurityException; import java.text.MessageFormat; import java.util.Arrays; import java.util.HashMap; @@ -33,28 +30,20 @@ import com.gitblit.Constants.Role; import com.gitblit.Keys; import com.gitblit.auth.AuthenticationProvider.UsernamePasswordAuthenticationProvider; +import com.gitblit.ldap.LdapConnection; import com.gitblit.models.TeamModel; import com.gitblit.models.UserModel; 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; import com.unboundid.ldap.sdk.LDAPException; -import com.unboundid.ldap.sdk.LDAPSearchException; import com.unboundid.ldap.sdk.ResultCode; import com.unboundid.ldap.sdk.SearchRequest; import com.unboundid.ldap.sdk.SearchResult; import com.unboundid.ldap.sdk.SearchResultEntry; import com.unboundid.ldap.sdk.SearchScope; -import com.unboundid.ldap.sdk.SimpleBindRequest; -import com.unboundid.ldap.sdk.extensions.StartTLSExtendedRequest; -import com.unboundid.util.ssl.SSLUtil; -import com.unboundid.util.ssl.TrustAllTrustManager; /** * Implementation of an LDAP user service. @@ -109,7 +98,7 @@ public synchronized void sync() { 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 = new LdapConnection(); + LdapConnection ldapConnection = new LdapConnection(settings); if (ldapConnection.connect()) { if (ldapConnection.bind() == null) { ldapConnection.close(); @@ -265,7 +254,7 @@ public AccountType getAccountType() { public UserModel authenticate(String username, char[] password) { String simpleUsername = getSimpleUsername(username); - LdapConnection ldapConnection = new LdapConnection(); + LdapConnection ldapConnection = new LdapConnection(settings); if (ldapConnection.connect()) { // Try to bind either to the "manager" account, @@ -288,7 +277,7 @@ public UserModel authenticate(String username, char[] password) { // 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}))"); - accountPattern = StringUtils.replace(accountPattern, "${username}", escapeLDAPSearchFilter(simpleUsername)); + accountPattern = StringUtils.replace(accountPattern, "${username}", LdapConnection.escapeLDAPSearchFilter(simpleUsername)); SearchResult result = doSearch(ldapConnection, accountBase, accountPattern); if (result != null && result.getEntryCount() == 1) { @@ -441,12 +430,12 @@ private void getTeamsFromLdap(LdapConnection ldapConnection, String simpleUserna String groupBase = settings.getString(Keys.realm.ldap.groupBase, ""); String groupMemberPattern = settings.getString(Keys.realm.ldap.groupMemberPattern, "(&(objectClass=group)(member=${dn}))"); - groupMemberPattern = StringUtils.replace(groupMemberPattern, "${dn}", escapeLDAPSearchFilter(loggingInUserDN)); - groupMemberPattern = StringUtils.replace(groupMemberPattern, "${username}", escapeLDAPSearchFilter(simpleUsername)); + groupMemberPattern = StringUtils.replace(groupMemberPattern, "${dn}", LdapConnection.escapeLDAPSearchFilter(loggingInUserDN)); + groupMemberPattern = StringUtils.replace(groupMemberPattern, "${username}", LdapConnection.escapeLDAPSearchFilter(simpleUsername)); // Fill in attributes into groupMemberPattern for (Attribute userAttribute : loggingInUser.getAttributes()) { - groupMemberPattern = StringUtils.replace(groupMemberPattern, "${" + userAttribute.getName() + "}", escapeLDAPSearchFilter(userAttribute.getValue())); + groupMemberPattern = StringUtils.replace(groupMemberPattern, "${" + userAttribute.getName() + "}", LdapConnection.escapeLDAPSearchFilter(userAttribute.getValue())); } SearchResult teamMembershipResult = searchTeamsInLdap(ldapConnection, groupBase, true, groupMemberPattern, Arrays.asList("cn")); @@ -553,34 +542,6 @@ protected String getSimpleUsername(String username) { return username; } - // From: https://www.owasp.org/index.php/Preventing_LDAP_Injection_in_Java - private static final String escapeLDAPSearchFilter(String filter) { - StringBuilder sb = new StringBuilder(); - for (int i = 0; i < filter.length(); i++) { - char curChar = filter.charAt(i); - switch (curChar) { - case '\\': - sb.append("\\5c"); - break; - case '*': - sb.append("\\2a"); - break; - case '(': - sb.append("\\28"); - break; - case ')': - sb.append("\\29"); - break; - case '\u0000': - sb.append("\\00"); - break; - default: - sb.append(curChar); - } - } - return sb.toString(); - } - private void configureSyncService() { LdapSyncService ldapSyncService = new LdapSyncService(settings, this); if (ldapSyncService.isReady()) { @@ -593,226 +554,4 @@ private void configureSyncService() { logger.info("Ldap sync service is disabled."); } } - - - - 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)://:", 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 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/main/java/com/gitblit/ldap/LdapConnection.java b/src/main/java/com/gitblit/ldap/LdapConnection.java new file mode 100644 index 000000000..b7f07a1e6 --- /dev/null +++ b/src/main/java/com/gitblit/ldap/LdapConnection.java @@ -0,0 +1,288 @@ +package com.gitblit.ldap; + +import java.net.URI; +import java.net.URISyntaxException; +import java.security.GeneralSecurityException; +import java.util.List; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import com.gitblit.IStoredSettings; +import com.gitblit.Keys; +import com.gitblit.utils.StringUtils; +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; +import com.unboundid.ldap.sdk.LDAPException; +import com.unboundid.ldap.sdk.LDAPSearchException; +import com.unboundid.ldap.sdk.ResultCode; +import com.unboundid.ldap.sdk.SearchRequest; +import com.unboundid.ldap.sdk.SearchResult; +import com.unboundid.ldap.sdk.SearchScope; +import com.unboundid.ldap.sdk.SimpleBindRequest; +import com.unboundid.ldap.sdk.extensions.StartTLSExtendedRequest; +import com.unboundid.util.ssl.SSLUtil; +import com.unboundid.util.ssl.TrustAllTrustManager; + +public class LdapConnection implements AutoCloseable { + + private final Logger logger = LoggerFactory.getLogger(getClass()); + + private IStoredSettings settings; + + private LDAPConnection conn; + private SimpleBindRequest currentBindRequest; + private SimpleBindRequest managerBindRequest; + private SimpleBindRequest userBindRequest; + + + // From: https://www.owasp.org/index.php/Preventing_LDAP_Injection_in_Java + public static final String escapeLDAPSearchFilter(String filter) { + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < filter.length(); i++) { + char curChar = filter.charAt(i); + switch (curChar) { + case '\\': + sb.append("\\5c"); + break; + case '*': + sb.append("\\2a"); + break; + case '(': + sb.append("\\28"); + break; + case ')': + sb.append("\\29"); + break; + case '\u0000': + sb.append("\\00"); + break; + default: + sb.append(curChar); + } + } + return sb.toString(); + } + + + + public LdapConnection(IStoredSettings settings) { + this.settings = settings; + + 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); + } + + + + public 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)://:", e); + } catch (GeneralSecurityException e) { + logger.error("Unable to create SSL Connection", e); + } catch (LDAPException e) { + logger.error("Error Connecting to LDAP", e); + } + + return false; + } + + + public void close() { + if (conn != null) { + conn.close(); + } + } + + + + /** + * Bind using the manager credentials set in realm.ldap.username and ..password + * @return A bind result, or null if binding failed. + */ + public 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. + */ + public 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; + } + + + public 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; + } + + + + public SearchResult search(SearchRequest request) { + try { + return conn.search(request); + } catch (LDAPSearchException e) { + logger.error("Problem Searching LDAP [{}]", e.getResultCode()); + return e.getSearchResult(); + } + } + + + public SearchResult search(String base, boolean dereferenceAliases, String filter, List 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; + } + } + + + + public 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/LdapConnectionTest.java b/src/test/java/com/gitblit/tests/LdapConnectionTest.java new file mode 100644 index 000000000..f8d2fed0d --- /dev/null +++ b/src/test/java/com/gitblit/tests/LdapConnectionTest.java @@ -0,0 +1,248 @@ +package com.gitblit.tests; + +import static org.junit.Assume.assumeTrue; + +import java.util.ArrayList; +import java.util.Arrays; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +import com.gitblit.Keys; +import com.gitblit.ldap.LdapConnection; +import com.unboundid.ldap.sdk.BindResult; +import com.unboundid.ldap.sdk.LDAPException; +import com.unboundid.ldap.sdk.ResultCode; +import com.unboundid.ldap.sdk.SearchRequest; +import com.unboundid.ldap.sdk.SearchResult; +import com.unboundid.ldap.sdk.SearchResultEntry; +import com.unboundid.ldap.sdk.SearchScope; + +/* + * Test for the LdapConnection + * + * @author Florian Zschocke + * + */ +@RunWith(Parameterized.class) +public class LdapConnectionTest extends LdapBasedUnitTest { + + @Test + public void testEscapeLDAPFilterString() { + // This test is independent from authentication mode, so run only once. + assumeTrue(authMode == AuthMode.ANONYMOUS); + + // From: https://www.owasp.org/index.php/Preventing_LDAP_Injection_in_Java + assertEquals("No special characters to escape", "Hi This is a test #çà", LdapConnection.escapeLDAPSearchFilter("Hi This is a test #çà")); + assertEquals("LDAP Christams Tree", "Hi \\28This\\29 = is \\2a a \\5c test # ç à ô", LdapConnection.escapeLDAPSearchFilter("Hi (This) = is * a \\ test # ç à ô")); + + assertEquals("Injection", "\\2a\\29\\28userPassword=secret", LdapConnection.escapeLDAPSearchFilter("*)(userPassword=secret")); + } + + + @Test + public void testConnect() { + // This test is independent from authentication mode, so run only once. + assumeTrue(authMode == AuthMode.ANONYMOUS); + + LdapConnection conn = new LdapConnection(settings); + try { + assertTrue(conn.connect()); + } finally { + conn.close(); + } + } + + + @Test + public void testBindAnonymous() { + // This test tests for anonymous bind, so run only in authentication mode ANONYMOUS. + assumeTrue(authMode == AuthMode.ANONYMOUS); + + LdapConnection conn = new LdapConnection(settings); + try { + assertTrue(conn.connect()); + + BindResult br = conn.bind(); + assertNotNull(br); + assertEquals(ResultCode.SUCCESS, br.getResultCode()); + assertEquals("", authMode.getBindTracker().getLastSuccessfulBindDN(br.getMessageID())); + + } finally { + conn.close(); + } + } + + + @Test + public void testBindAsAdmin() { + // This test tests for anonymous bind, so run only in authentication mode DS_MANAGER. + assumeTrue(authMode == AuthMode.DS_MANAGER); + + LdapConnection conn = new LdapConnection(settings); + try { + assertTrue(conn.connect()); + + BindResult br = conn.bind(); + assertNotNull(br); + assertEquals(ResultCode.SUCCESS, br.getResultCode()); + assertEquals(settings.getString(Keys.realm.ldap.username, "UNSET"), authMode.getBindTracker().getLastSuccessfulBindDN(br.getMessageID())); + + } finally { + conn.close(); + } + } + + + @Test + public void testBindToBindpattern() { + LdapConnection conn = new LdapConnection(settings); + try { + assertTrue(conn.connect()); + + String bindPattern = "CN=${username},OU=Canada," + ACCOUNT_BASE; + + BindResult br = conn.bind(bindPattern, "UserThree", "userThreePassword"); + assertNotNull(br); + assertEquals(ResultCode.SUCCESS, br.getResultCode()); + assertEquals("CN=UserThree,OU=Canada," + ACCOUNT_BASE, authMode.getBindTracker().getLastSuccessfulBindDN(br.getMessageID())); + + br = conn.bind(bindPattern, "UserFour", "userThreePassword"); + assertNull(br); + + br = conn.bind(bindPattern, "UserTwo", "userTwoPassword"); + assertNull(br); + + } finally { + conn.close(); + } + } + + + @Test + public void testRebindAsUser() { + LdapConnection conn = new LdapConnection(settings); + try { + assertTrue(conn.connect()); + + assertFalse(conn.rebindAsUser()); + + BindResult br = conn.bind(); + assertNotNull(br); + assertFalse(conn.rebindAsUser()); + + + String bindPattern = "CN=${username},OU=Canada," + ACCOUNT_BASE; + br = conn.bind(bindPattern, "UserThree", "userThreePassword"); + assertNotNull(br); + assertFalse(conn.rebindAsUser()); + + br = conn.bind(); + assertNotNull(br); + assertTrue(conn.rebindAsUser()); + assertEquals(ResultCode.SUCCESS, br.getResultCode()); + assertEquals("CN=UserThree,OU=Canada," + ACCOUNT_BASE, authMode.getBindTracker().getLastSuccessfulBindDN()); + + } finally { + conn.close(); + } + } + + + + @Test + public void testSearchRequest() throws LDAPException { + LdapConnection conn = new LdapConnection(settings); + try { + assertTrue(conn.connect()); + BindResult br = conn.bind(); + assertNotNull(br); + + SearchRequest req; + SearchResult result; + SearchResultEntry entry; + + req = new SearchRequest(ACCOUNT_BASE, SearchScope.BASE, "(CN=UserOne)"); + result = conn.search(req); + assertNotNull(result); + assertEquals(0, result.getEntryCount()); + + req = new SearchRequest(ACCOUNT_BASE, SearchScope.ONE, "(CN=UserTwo)"); + result = conn.search(req); + assertNotNull(result); + assertEquals(0, result.getEntryCount()); + + req = new SearchRequest(ACCOUNT_BASE, SearchScope.SUB, "(CN=UserThree)"); + result = conn.search(req); + assertNotNull(result); + assertEquals(1, result.getEntryCount()); + entry = result.getSearchEntries().get(0); + assertEquals("CN=UserThree,OU=Canada," + ACCOUNT_BASE, entry.getDN()); + + req = new SearchRequest(ACCOUNT_BASE, SearchScope.SUBORDINATE_SUBTREE, "(CN=UserFour)"); + result = conn.search(req); + assertNotNull(result); + assertEquals(1, result.getEntryCount()); + entry = result.getSearchEntries().get(0); + assertEquals("CN=UserFour,OU=Canada," + ACCOUNT_BASE, entry.getDN()); + + } finally { + conn.close(); + } + } + + + @Test + public void testSearch() throws LDAPException { + LdapConnection conn = new LdapConnection(settings); + try { + assertTrue(conn.connect()); + BindResult br = conn.bind(); + assertNotNull(br); + + SearchResult result; + SearchResultEntry entry; + + result = conn.search(ACCOUNT_BASE, false, "(CN=UserOne)", null); + assertNotNull(result); + assertEquals(1, result.getEntryCount()); + entry = result.getSearchEntries().get(0); + assertEquals("CN=UserOne,OU=US," + ACCOUNT_BASE, entry.getDN()); + + result = conn.search(ACCOUNT_BASE, true, "(&(CN=UserOne)(surname=One))", null); + assertNotNull(result); + assertEquals(1, result.getEntryCount()); + entry = result.getSearchEntries().get(0); + assertEquals("CN=UserOne,OU=US," + ACCOUNT_BASE, entry.getDN()); + + result = conn.search(ACCOUNT_BASE, true, "(&(CN=UserOne)(surname=Two))", null); + assertNotNull(result); + assertEquals(0, result.getEntryCount()); + + result = conn.search(ACCOUNT_BASE, true, "(surname=Two)", Arrays.asList("givenName", "surname")); + assertNotNull(result); + assertEquals(1, result.getEntryCount()); + entry = result.getSearchEntries().get(0); + assertEquals("CN=UserTwo,OU=US," + ACCOUNT_BASE, entry.getDN()); + assertEquals(2, entry.getAttributes().size()); + assertEquals("User", entry.getAttributeValue("givenName")); + assertEquals("Two", entry.getAttributeValue("surname")); + + result = conn.search(ACCOUNT_BASE, true, "(personalTitle=Mr*)", null); + assertNotNull(result); + assertEquals(3, result.getEntryCount()); + ArrayList names = new ArrayList<>(3); + names.add(result.getSearchEntries().get(0).getAttributeValue("surname")); + names.add(result.getSearchEntries().get(1).getAttributeValue("surname")); + names.add(result.getSearchEntries().get(2).getAttributeValue("surname")); + assertTrue(names.contains("One")); + assertTrue(names.contains("Two")); + assertTrue(names.contains("Three")); + + } finally { + conn.close(); + } + } + +} From f639d966cb5e7026cb30e6b25be55fb681feb896 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Fri, 25 Nov 2016 18:21:27 +0100 Subject: [PATCH 3/7] Retrieve public SSH keys from LDAP. Add new class `LdapPublicKeyManager` which retrieves public SSH keys from LDAP. The attribute can be configured with the new configuration option `realm.ldap.sshPublicKey`. The setting can be a simple attribute name, like `sshPublicKey`, or an attribute name and a prefix for the value, like `altSecurityIdentities:SshKey`, in which case attributes are selected that have the name `altSecurityIdentities` and whose values start with `SshKey:`. --- src/main/distrib/data/defaults.properties | 12 + .../com/gitblit/auth/LdapAuthProvider.java | 11 +- .../java/com/gitblit/ldap/LdapConnection.java | 110 ++- .../gitblit/transport/ssh/LdapKeyManager.java | 397 ++++++++++ .../com/gitblit/tests/LdapConnectionTest.java | 32 + .../tests/LdapPublicKeyManagerTest.java | 723 ++++++++++++++++++ 6 files changed, 1248 insertions(+), 37 deletions(-) create mode 100644 src/main/java/com/gitblit/transport/ssh/LdapKeyManager.java create mode 100644 src/test/java/com/gitblit/tests/LdapPublicKeyManagerTest.java diff --git a/src/main/distrib/data/defaults.properties b/src/main/distrib/data/defaults.properties index 16be84763..1fe5b345e 100644 --- a/src/main/distrib/data/defaults.properties +++ b/src/main/distrib/data/defaults.properties @@ -1935,6 +1935,18 @@ realm.ldap.email = email # SINCE 1.0.0 realm.ldap.uid = uid +# Attribute on the USER record that indicates their public SSH key. +# Leave blank when public SSH keys shall not be retrieved from LDAP. +# +# This may be a simple attribute or an attribute and a value prefix. Examples: +# sshPublicKey - Use the attribute 'sshPublicKey' on the user record. +# altSecurityIdentities:SshKey - Use the attribute 'altSecurityIdentities' +# on the user record, for which the record value +# starts with 'SshKey:', followed by the SSH key entry. +# +# SINCE 1.9.0 +realm.ldap.sshPublicKey = + # 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 diff --git a/src/main/java/com/gitblit/auth/LdapAuthProvider.java b/src/main/java/com/gitblit/auth/LdapAuthProvider.java index 8a326cdc4..7ea8f1137 100644 --- a/src/main/java/com/gitblit/auth/LdapAuthProvider.java +++ b/src/main/java/com/gitblit/auth/LdapAuthProvider.java @@ -107,9 +107,9 @@ public synchronized void sync() { } try { - String accountBase = settings.getString(Keys.realm.ldap.accountBase, ""); String uidAttribute = settings.getString(Keys.realm.ldap.uid, "uid"); - String accountPattern = settings.getString(Keys.realm.ldap.accountPattern, "(&(objectClass=person)(sAMAccountName=${username}))"); + String accountBase = ldapConnection.getAccountBase(); + String accountPattern = ldapConnection.getAccountPattern(); accountPattern = StringUtils.replace(accountPattern, "${username}", "*"); SearchResult result = doSearch(ldapConnection, accountBase, accountPattern); @@ -275,11 +275,7 @@ public UserModel authenticate(String username, char[] password) { 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}))"); - accountPattern = StringUtils.replace(accountPattern, "${username}", LdapConnection.escapeLDAPSearchFilter(simpleUsername)); - - SearchResult result = doSearch(ldapConnection, accountBase, accountPattern); + SearchResult result = ldapConnection.searchUser(simpleUsername); if (result != null && result.getEntryCount() == 1) { SearchResultEntry loggingInUser = result.getSearchEntries().get(0); String loggingInUserDN = loggingInUser.getDN(); @@ -527,6 +523,7 @@ private SearchResult doSearch(LdapConnection ldapConnection, String base, String + /** * Returns a simple username without any domain prefixes. * diff --git a/src/main/java/com/gitblit/ldap/LdapConnection.java b/src/main/java/com/gitblit/ldap/LdapConnection.java index b7f07a1e6..14fedf100 100644 --- a/src/main/java/com/gitblit/ldap/LdapConnection.java +++ b/src/main/java/com/gitblit/ldap/LdapConnection.java @@ -1,3 +1,18 @@ +/* + * Copyright 2016 gitblit.com. + * + * 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 + * + * http://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 com.gitblit.ldap; import java.net.URI; @@ -69,6 +84,16 @@ public static final String escapeLDAPSearchFilter(String filter) { + public static String getAccountBase(IStoredSettings settings) { + return settings.getString(Keys.realm.ldap.accountBase, ""); + } + + public static String getAccountPattern(IStoredSettings settings) { + return settings.getString(Keys.realm.ldap.accountPattern, "(&(objectClass=person)(sAMAccountName=${username}))"); + } + + + public LdapConnection(IStoredSettings settings) { this.settings = settings; @@ -82,6 +107,16 @@ public LdapConnection(IStoredSettings settings) { + public String getAccountBase() { + return getAccountBase(settings); + } + + public String getAccountPattern() { + return getAccountPattern(settings); + } + + + public boolean connect() { try { URI ldapUrl = new URI(settings.getRequiredString(Keys.realm.ldap.server)); @@ -198,36 +233,6 @@ public boolean rebindAsUser() { - public SearchResult search(SearchRequest request) { - try { - return conn.search(request); - } catch (LDAPSearchException e) { - logger.error("Problem Searching LDAP [{}]", e.getResultCode()); - return e.getSearchResult(); - } - } - - - public SearchResult search(String base, boolean dereferenceAliases, String filter, List 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; - } - } - - - public boolean isAuthenticated(String userDn, String password) { verifyCurrentBinding(); @@ -267,6 +272,51 @@ public boolean isAuthenticated(String userDn, String password) { + + public SearchResult search(SearchRequest request) { + try { + return conn.search(request); + } catch (LDAPSearchException e) { + logger.error("Problem Searching LDAP [{}]", e.getResultCode()); + return e.getSearchResult(); + } + } + + + public SearchResult search(String base, boolean dereferenceAliases, String filter, List 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; + } + } + + + public SearchResult searchUser(String username, List attributes) { + + String accountPattern = getAccountPattern(); + accountPattern = StringUtils.replace(accountPattern, "${username}", escapeLDAPSearchFilter(username)); + + return search(getAccountBase(), false, accountPattern, attributes); + } + + + public SearchResult searchUser(String username) { + return searchUser(username, null); + } + + + private boolean verifyCurrentBinding() { BindRequest lastBind = conn.getLastBindRequest(); if (lastBind == currentBindRequest) { diff --git a/src/main/java/com/gitblit/transport/ssh/LdapKeyManager.java b/src/main/java/com/gitblit/transport/ssh/LdapKeyManager.java new file mode 100644 index 000000000..9612a96b9 --- /dev/null +++ b/src/main/java/com/gitblit/transport/ssh/LdapKeyManager.java @@ -0,0 +1,397 @@ +/* + * Copyright 2016 gitblit.com. + * + * 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 + * + * http://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 com.gitblit.transport.ssh; + +import java.io.IOException; +import java.security.GeneralSecurityException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.TreeMap; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import org.apache.sshd.common.config.keys.KeyUtils; +import org.apache.sshd.common.util.GenericUtils; +import org.apache.sshd.server.config.keys.AuthorizedKeyEntry; + +import com.gitblit.IStoredSettings; +import com.gitblit.Keys; +import com.gitblit.Constants.AccessPermission; +import com.gitblit.ldap.LdapConnection; +import com.gitblit.utils.StringUtils; +import com.google.common.base.Joiner; +import com.google.inject.Inject; +import com.unboundid.ldap.sdk.BindResult; +import com.unboundid.ldap.sdk.ResultCode; +import com.unboundid.ldap.sdk.SearchResult; +import com.unboundid.ldap.sdk.SearchResultEntry; + +/** + * LDAP public key manager + * + * Retrieves public keys from user's LDAP entries. Using this key manager, + * no SSH keys can be edited, i.e. added, removed, permissions changed, etc. + * + * @author Florian Zschocke + * + */ +public class LdapKeyManager extends IPublicKeyManager { + + /** + * Pattern to find prefixes like 'SSHKey:' in key entries. + * These prefixes describe the type of an altSecurityIdentity. + * The pattern accepts anything but quote and colon up to the + * first colon at the start of a string. + */ + private static final Pattern PREFIX_PATTERN = Pattern.compile("^([^\":]+):"); + /** + * Pattern to find the string describing Gitblit permissions for a SSH key. + * The pattern matches on a string starting with 'gbPerm', matched case-insensitive, + * followed by '=' with optional whitespace around it, followed by a string of + * upper and lower case letters and '+' and '-' for the permission, which can optionally + * be enclosed in '"' or '\"' (only the leading quote is matched in the pattern). + * Only the group describing the permission is a capturing group. + */ + private static final Pattern GB_PERM_PATTERN = Pattern.compile("(?i:gbPerm)\\s*=\\s*(?:\\\\\"|\")?\\s*([A-Za-z+-]+)"); + + + private final IStoredSettings settings; + + + + @Inject + public LdapKeyManager(IStoredSettings settings) { + this.settings = settings; + } + + + @Override + public String toString() { + return getClass().getSimpleName(); + } + + @Override + public LdapKeyManager start() { + log.info(toString()); + return this; + } + + @Override + public boolean isReady() { + return true; + } + + @Override + public LdapKeyManager stop() { + return this; + } + + @Override + protected boolean isStale(String username) { + // always return true so we gets keys from LDAP every time + return true; + } + + @Override + protected List getKeysImpl(String username) { + try (LdapConnection conn = new LdapConnection(settings)) { + if (conn.connect()) { + log.info("loading ssh key for {} from LDAP directory", username); + + BindResult bindResult = conn.bind(); + if (bindResult == null) { + conn.close(); + return null; + } + + // Search the user entity + + // Support prefixing the key data, e.g. when using altSecurityIdentities in AD. + String pubKeyAttribute = settings.getString(Keys.realm.ldap.sshPublicKey, "sshPublicKey"); + String pkaPrefix = null; + int idx = pubKeyAttribute.indexOf(':'); + if (idx > 0) { + pkaPrefix = pubKeyAttribute.substring(idx +1); + pubKeyAttribute = pubKeyAttribute.substring(0, idx); + } + + SearchResult result = conn.searchUser(getSimpleUsername(username), Arrays.asList(pubKeyAttribute)); + conn.close(); + + if (result != null && result.getResultCode() == ResultCode.SUCCESS) { + if ( result.getEntryCount() > 1) { + log.info("Found more than one entry for user {} in LDAP. Cannot retrieve SSH key.", username); + return null; + } else if ( result.getEntryCount() < 1) { + log.info("Found no entry for user {} in LDAP. Cannot retrieve SSH key.", username); + return null; + } + + // Retrieve the SSH key attributes + SearchResultEntry foundUser = result.getSearchEntries().get(0); + String[] attrs = foundUser.getAttributeValues(pubKeyAttribute); + if (attrs == null ||attrs.length == 0) { + log.info("found no keys for user {} under attribute {} in directory", username, pubKeyAttribute); + return null; + } + + + // Filter resulting list to match with required special prefix in entry + List authorizedKeys = new ArrayList<>(attrs.length); + Matcher m = PREFIX_PATTERN.matcher(""); + for (int i = 0; i < attrs.length; ++i) { + // strip out line breaks + String keyEntry = Joiner.on("").join(attrs[i].replace("\r\n", "\n").split("\n")); + m.reset(keyEntry); + try { + if (m.lookingAt()) { // Key is prefixed in LDAP + if (pkaPrefix == null) { + continue; + } + String prefix = m.group(1).trim(); + if (! pkaPrefix.equalsIgnoreCase(prefix)) { + continue; + } + String s = keyEntry.substring(m.end()); // Strip prefix off + authorizedKeys.add(GbAuthorizedKeyEntry.parseAuthorizedKeyEntry(s)); + + } else { // Key is not prefixed in LDAP + if (pkaPrefix != null) { + continue; + } + String s = keyEntry; // Strip prefix off + authorizedKeys.add(GbAuthorizedKeyEntry.parseAuthorizedKeyEntry(s)); + } + } catch (IllegalArgumentException e) { + log.info("Failed to parse key entry={}:", keyEntry, e.getMessage()); + } + } + + List keyList = new ArrayList<>(authorizedKeys.size()); + for (GbAuthorizedKeyEntry keyEntry : authorizedKeys) { + try { + SshKey key = new SshKey(keyEntry.resolvePublicKey()); + key.setComment(keyEntry.getComment()); + setKeyPermissions(key, keyEntry); + keyList.add(key); + } catch (GeneralSecurityException | IOException e) { + log.warn("Error resolving key entry for user {}. Entry={}", username, keyEntry, e); + } + } + return keyList; + } + } + } + + return null; + } + + + @Override + public boolean addKey(String username, SshKey key) { + return false; + } + + @Override + public boolean removeKey(String username, SshKey key) { + return false; + } + + @Override + public boolean removeAllKeys(String username) { + return false; + } + + + + private void setKeyPermissions(SshKey key, GbAuthorizedKeyEntry keyEntry) { + List env = keyEntry.getLoginOptionValues("environment"); + if (env != null && !env.isEmpty()) { + // Walk over all entries and find one that sets 'gbPerm'. The last one wins. + for (String envi : env) { + Matcher m = GB_PERM_PATTERN.matcher(envi); + if (m.find()) { + String perm = m.group(1).trim(); + AccessPermission ap = AccessPermission.fromCode(perm); + if (ap == AccessPermission.NONE) { + ap = AccessPermission.valueOf(perm.toUpperCase()); + } + + if (ap != null && ap != AccessPermission.NONE) { + try { + key.setPermission(ap); + } catch (IllegalArgumentException e) { + log.warn("Incorrect permissions ({}) set for SSH key entry {}.", ap, envi, e); + } + } + } + } + } + } + + + /** + * Returns a simple username without any domain prefixes. + * + * @param username + * @return a simple username + */ + private String getSimpleUsername(String username) { + int lastSlash = username.lastIndexOf('\\'); + if (lastSlash > -1) { + username = username.substring(lastSlash + 1); + } + + return username; + } + + + /** + * Extension of the AuthorizedKeyEntry from Mina SSHD with better option parsing. + * + * The class makes use of code from the two methods copied from the original + * Mina SSHD AuthorizedKeyEntry class. The code is rewritten to improve user login + * option support. Options are correctly parsed even if they have whitespace within + * double quotes. Options can occur multiple times, which is needed for example for + * the "environment" option. Thus for an option a list of strings is kept, holding + * multiple option values. + */ + private static class GbAuthorizedKeyEntry extends AuthorizedKeyEntry { + + private static final long serialVersionUID = 1L; + /** + * Pattern to extract the first part of the key entry without whitespace or only with quoted whitespace. + * The pattern essentially splits the line in two parts with two capturing groups. All other groups + * in the pattern are non-capturing. The first part is a continuous string that only includes double quoted + * whitespace and ends in whitespace. The second part is the rest of the line. + * The first part is at the beginning of the line, the lead-in. For a SSH key entry this can either be + * login options (see authorized keys file description) or the key type. Since options, other than the + * key type, can include whitespace and escaped double quotes within double quotes, the pattern takes + * care of that by searching for either "characters that are not whitespace and not double quotes" + * or "a double quote, followed by 'characters that are not a double quote or backslash, or a backslash + * and then a double quote, or a backslash', followed by a double quote". + */ + private static final Pattern LEADIN_PATTERN = Pattern.compile("^((?:[^\\s\"]*|(?:\"(?:[^\"\\\\]|\\\\\"|\\\\)*\"))*\\s+)(.+)"); + /** + * Pattern to split a comma separated list of options. + * Since an option could contain commas (as well as escaped double quotes) within double quotes + * in the option value, a simple split on comma is not enough. So the pattern searches for multiple + * occurrences of: + * characters that are not double quotes or a comma, or + * a double quote followed by: characters that are not a double quote or backslash, or + * a backslash and then a double quote, or + * a backslash, + * followed by a double quote. + */ + private static final Pattern OPTION_PATTERN = Pattern.compile("([^\",]+|(?:\"(?:[^\"\\\\]|\\\\\"|\\\\)*\"))+"); + + // for options that have no value, "true" is used + private Map> loginOptionsMulti = Collections.emptyMap(); + + + List getLoginOptionValues(String option) { + return loginOptionsMulti.get(option); + } + + + + /** + * @param line Original line from an authorized_keys file + * @return {@link GbAuthorizedKeyEntry} or {@code null} if the line is + * {@code null}/empty or a comment line + * @throws IllegalArgumentException If failed to parse/decode the line + * @see #COMMENT_CHAR + */ + public static GbAuthorizedKeyEntry parseAuthorizedKeyEntry(String line) throws IllegalArgumentException { + line = GenericUtils.trimToEmpty(line); + if (StringUtils.isEmpty(line) || (line.charAt(0) == COMMENT_CHAR) /* comment ? */) { + return null; + } + + Matcher m = LEADIN_PATTERN.matcher(line); + if (! m.lookingAt()) { + throw new IllegalArgumentException("Bad format (no key data delimiter): " + line); + } + + String keyType = m.group(1).trim(); + final GbAuthorizedKeyEntry entry; + if (KeyUtils.getPublicKeyEntryDecoder(keyType) == null) { // assume this is due to the fact that it starts with login options + entry = parseAuthorizedKeyEntry(m.group(2)); + if (entry == null) { + throw new IllegalArgumentException("Bad format (no key data after login options): " + line); + } + + entry.parseAndSetLoginOptions(keyType); + } else { + int startPos = line.indexOf(' '); + if (startPos <= 0) { + throw new IllegalArgumentException("Bad format (no key data delimiter): " + line); + } + + int endPos = line.indexOf(' ', startPos + 1); + if (endPos <= startPos) { + endPos = line.length(); + } + + String encData = (endPos < (line.length() - 1)) ? line.substring(0, endPos).trim() : line; + String comment = (endPos < (line.length() - 1)) ? line.substring(endPos + 1).trim() : null; + entry = parsePublicKeyEntry(new GbAuthorizedKeyEntry(), encData); + entry.setComment(comment); + } + + return entry; + } + + private void parseAndSetLoginOptions(String options) { + Matcher m = OPTION_PATTERN.matcher(options); + if (! m.find()) { + loginOptionsMulti = Collections.emptyMap(); + } + Map> optsMap = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); + + do { + String p = m.group(); + p = GenericUtils.trimToEmpty(p); + if (StringUtils.isEmpty(p)) { + continue; + } + + int pos = p.indexOf('='); + String name = (pos < 0) ? p : GenericUtils.trimToEmpty(p.substring(0, pos)); + CharSequence value = (pos < 0) ? null : GenericUtils.trimToEmpty(p.substring(pos + 1)); + value = GenericUtils.stripQuotes(value); + + // For options without value the value is set to TRUE. + if (value == null) { + value = Boolean.TRUE.toString(); + } + + List opts = optsMap.get(name); + if (opts == null) { + opts = new ArrayList(); + optsMap.put(name, opts); + } + opts.add(value.toString()); + } while(m.find()); + + loginOptionsMulti = optsMap; + } + } + +} diff --git a/src/test/java/com/gitblit/tests/LdapConnectionTest.java b/src/test/java/com/gitblit/tests/LdapConnectionTest.java index f8d2fed0d..3da547772 100644 --- a/src/test/java/com/gitblit/tests/LdapConnectionTest.java +++ b/src/test/java/com/gitblit/tests/LdapConnectionTest.java @@ -245,4 +245,36 @@ public void testSearch() throws LDAPException { } } + + @Test + public void testSearchUser() throws LDAPException { + LdapConnection conn = new LdapConnection(settings); + try { + assertTrue(conn.connect()); + BindResult br = conn.bind(); + assertNotNull(br); + + SearchResult result; + SearchResultEntry entry; + + result = conn.searchUser("UserOne"); + assertNotNull(result); + assertEquals(1, result.getEntryCount()); + entry = result.getSearchEntries().get(0); + assertEquals("CN=UserOne,OU=US," + ACCOUNT_BASE, entry.getDN()); + + result = conn.searchUser("UserFour", Arrays.asList("givenName", "surname")); + assertNotNull(result); + assertEquals(1, result.getEntryCount()); + entry = result.getSearchEntries().get(0); + assertEquals("CN=UserFour,OU=Canada," + ACCOUNT_BASE, entry.getDN()); + assertEquals(2, entry.getAttributes().size()); + assertEquals("User", entry.getAttributeValue("givenName")); + assertEquals("Four", entry.getAttributeValue("surname")); + + } finally { + conn.close(); + } + } + } diff --git a/src/test/java/com/gitblit/tests/LdapPublicKeyManagerTest.java b/src/test/java/com/gitblit/tests/LdapPublicKeyManagerTest.java new file mode 100644 index 000000000..c426254f1 --- /dev/null +++ b/src/test/java/com/gitblit/tests/LdapPublicKeyManagerTest.java @@ -0,0 +1,723 @@ +/* + * Copyright 2016 Florian Zschocke + * Copyright 2016 gitblit.com + * + * 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 + * + * http://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 com.gitblit.tests; + +import static org.junit.Assume.assumeTrue; + +import java.security.GeneralSecurityException; +import java.security.InvalidAlgorithmParameterException; +import java.security.KeyPair; +import java.security.KeyPairGenerator; +import java.security.Signature; +import java.security.spec.ECGenParameterSpec; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import org.apache.sshd.common.util.SecurityUtils; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; + +import com.gitblit.Keys; +import com.gitblit.Constants.AccessPermission; +import com.gitblit.transport.ssh.LdapKeyManager; +import com.gitblit.transport.ssh.SshKey; +import com.unboundid.ldap.sdk.LDAPException; +import com.unboundid.ldap.sdk.Modification; +import com.unboundid.ldap.sdk.ModificationType; + +/** + * Test LdapPublicKeyManager going against an in-memory UnboundID + * LDAP server. + * + * @author Florian Zschocke + * + */ +@RunWith(Parameterized.class) +public class LdapPublicKeyManagerTest extends LdapBasedUnitTest { + + private static Map keyPairs = new HashMap<>(10); + private static KeyPairGenerator rsaGenerator; + private static KeyPairGenerator dsaGenerator; + private static KeyPairGenerator ecGenerator; + + + + @BeforeClass + public static void init() throws GeneralSecurityException { + rsaGenerator = SecurityUtils.getKeyPairGenerator("RSA"); + dsaGenerator = SecurityUtils.getKeyPairGenerator("DSA"); + ecGenerator = SecurityUtils.getKeyPairGenerator("ECDSA"); + } + + + + @Test + public void testGetKeys() throws LDAPException { + String keyRsaOne = getRsaPubKey("UserOne@example.com"); + getDS().modify(DN_USER_ONE, new Modification(ModificationType.ADD, "sshPublicKey", keyRsaOne)); + + String keyRsaTwo = getRsaPubKey("UserTwo@example.com"); + String keyDsaTwo = getDsaPubKey("UserTwo@example.com"); + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "sshPublicKey", keyRsaTwo, keyDsaTwo)); + + String keyRsaThree = getRsaPubKey("UserThree@example.com"); + String keyDsaThree = getDsaPubKey("UserThree@example.com"); + String keyEcThree = getEcPubKey("UserThree@example.com"); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "sshPublicKey", keyEcThree, keyRsaThree, keyDsaThree)); + + LdapKeyManager kmgr = new LdapKeyManager(settings); + + List keys = kmgr.getKeys("UserOne"); + assertNotNull(keys); + assertTrue(keys.size() == 1); + assertEquals(keyRsaOne, keys.get(0).getRawData()); + + + keys = kmgr.getKeys("UserTwo"); + assertNotNull(keys); + assertTrue(keys.size() == 2); + if (keyRsaTwo.equals(keys.get(0).getRawData())) { + assertEquals(keyDsaTwo, keys.get(1).getRawData()); + } else if (keyDsaTwo.equals(keys.get(0).getRawData())) { + assertEquals(keyRsaTwo, keys.get(1).getRawData()); + } else { + fail("Mismatch in UserTwo keys."); + } + + + keys = kmgr.getKeys("UserThree"); + assertNotNull(keys); + assertTrue(keys.size() == 3); + assertEquals(keyEcThree, keys.get(0).getRawData()); + assertEquals(keyRsaThree, keys.get(1).getRawData()); + assertEquals(keyDsaThree, keys.get(2).getRawData()); + + keys = kmgr.getKeys("UserFour"); + assertNotNull(keys); + assertTrue(keys.size() == 0); + } + + + @Test + public void testGetKeysAttributeName() throws LDAPException { + settings.put(Keys.realm.ldap.sshPublicKey, "sshPublicKey"); + + String keyRsaOne = getRsaPubKey("UserOne@example.com"); + getDS().modify(DN_USER_ONE, new Modification(ModificationType.ADD, "sshPublicKey", keyRsaOne)); + + String keyDsaTwo = getDsaPubKey("UserTwo@example.com"); + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "publicsshkey", keyDsaTwo)); + + String keyRsaThree = getRsaPubKey("UserThree@example.com"); + String keyDsaThree = getDsaPubKey("UserThree@example.com"); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "sshPublicKey", keyRsaThree)); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "publicsshkey", keyDsaThree)); + + + LdapKeyManager kmgr = new LdapKeyManager(settings); + + List keys = kmgr.getKeys("UserOne"); + assertNotNull(keys); + assertEquals(1, keys.size()); + assertEquals(keyRsaOne, keys.get(0).getRawData()); + + keys = kmgr.getKeys("UserTwo"); + assertNotNull(keys); + assertEquals(0, keys.size()); + + keys = kmgr.getKeys("UserThree"); + assertNotNull(keys); + assertEquals(1, keys.size()); + assertEquals(keyRsaThree, keys.get(0).getRawData()); + + keys = kmgr.getKeys("UserFour"); + assertNotNull(keys); + assertEquals(0, keys.size()); + + + settings.put(Keys.realm.ldap.sshPublicKey, "publicsshkey"); + + keys = kmgr.getKeys("UserOne"); + assertNotNull(keys); + assertEquals(0, keys.size()); + + keys = kmgr.getKeys("UserTwo"); + assertNotNull(keys); + assertEquals(1, keys.size()); + assertEquals(keyDsaTwo, keys.get(0).getRawData()); + + keys = kmgr.getKeys("UserThree"); + assertNotNull(keys); + assertEquals(1, keys.size()); + assertEquals(keyDsaThree, keys.get(0).getRawData()); + + keys = kmgr.getKeys("UserFour"); + assertNotNull(keys); + assertEquals(0, keys.size()); + } + + + @Test + public void testGetKeysPrefixed() throws LDAPException { + // This test is independent from authentication mode, so run only once. + assumeTrue(authMode == AuthMode.ANONYMOUS); + + String keyRsaOne = getRsaPubKey("UserOne@example.com"); + getDS().modify(DN_USER_ONE, new Modification(ModificationType.ADD, "sshPublicKey", keyRsaOne)); + + String keyRsaTwo = getRsaPubKey("UserTwo@example.com"); + String keyDsaTwo = getDsaPubKey("UserTwo@example.com"); + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "altSecurityIdentities", keyRsaTwo)); + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "altSecurityIdentities", "SSHKey: " + keyDsaTwo)); + + String keyRsaThree = getRsaPubKey("UserThree@example.com"); + String keyDsaThree = getDsaPubKey("UserThree@example.com"); + String keyEcThree = getEcPubKey("UserThree@example.com"); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "altSecurityIdentities", " SshKey :\r\n" + keyRsaThree)); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "altSecurityIdentities", " sshkey: " + keyDsaThree)); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "altSecurityIdentities", "ECDSAKey :\n " + keyEcThree)); + + + LdapKeyManager kmgr = new LdapKeyManager(settings); + + settings.put(Keys.realm.ldap.sshPublicKey, "altSecurityIdentities"); + + List keys = kmgr.getKeys("UserOne"); + assertNotNull(keys); + assertEquals(0, keys.size()); + + keys = kmgr.getKeys("UserTwo"); + assertNotNull(keys); + assertEquals(1, keys.size()); + assertEquals(keyRsaTwo, keys.get(0).getRawData()); + + keys = kmgr.getKeys("UserThree"); + assertNotNull(keys); + assertEquals(0, keys.size()); + + keys = kmgr.getKeys("UserFour"); + assertNotNull(keys); + assertEquals(0, keys.size()); + + + + settings.put(Keys.realm.ldap.sshPublicKey, "altSecurityIdentities:SSHKey"); + + keys = kmgr.getKeys("UserOne"); + assertNotNull(keys); + assertEquals(0, keys.size()); + + keys = kmgr.getKeys("UserTwo"); + assertNotNull(keys); + assertEquals(1, keys.size()); + assertEquals(keyDsaTwo, keys.get(0).getRawData()); + + keys = kmgr.getKeys("UserThree"); + assertNotNull(keys); + assertEquals(2, keys.size()); + assertEquals(keyRsaThree, keys.get(0).getRawData()); + assertEquals(keyDsaThree, keys.get(1).getRawData()); + + keys = kmgr.getKeys("UserFour"); + assertNotNull(keys); + assertEquals(0, keys.size()); + + + + settings.put(Keys.realm.ldap.sshPublicKey, "altSecurityIdentities:ECDSAKey"); + + keys = kmgr.getKeys("UserOne"); + assertNotNull(keys); + assertEquals(0, keys.size()); + + keys = kmgr.getKeys("UserTwo"); + assertNotNull(keys); + assertEquals(0, keys.size()); + + keys = kmgr.getKeys("UserThree"); + assertNotNull(keys); + assertEquals(1, keys.size()); + assertEquals(keyEcThree, keys.get(0).getRawData()); + + keys = kmgr.getKeys("UserFour"); + assertNotNull(keys); + assertEquals(0, keys.size()); + } + + + @Test + public void testGetKeysPermissions() throws LDAPException { + // This test is independent from authentication mode, so run only once. + assumeTrue(authMode == AuthMode.ANONYMOUS); + + String keyRsaOne = getRsaPubKey("UserOne@example.com"); + String keyRsaTwo = getRsaPubKey(""); + String keyDsaTwo = getDsaPubKey("UserTwo at example.com"); + String keyRsaThree = getRsaPubKey("UserThree@example.com"); + String keyDsaThree = getDsaPubKey("READ key for user 'Three' @example.com"); + String keyEcThree = getEcPubKey("UserThree@example.com"); + + getDS().modify(DN_USER_ONE, new Modification(ModificationType.ADD, "sshPublicKey", keyRsaOne)); + getDS().modify(DN_USER_ONE, new Modification(ModificationType.ADD, "sshPublicKey", " " + keyRsaTwo)); + getDS().modify(DN_USER_ONE, new Modification(ModificationType.ADD, "sshPublicKey", "no-agent-forwarding " + keyDsaTwo)); + getDS().modify(DN_USER_ONE, new Modification(ModificationType.ADD, "sshPublicKey", " command=\"sh /etc/netstart tun0 \" " + keyRsaThree)); + getDS().modify(DN_USER_ONE, new Modification(ModificationType.ADD, "sshPublicKey", " command=\"netstat -nult\",environment=\"gb=\\\"What now\\\"\" " + keyDsaThree)); + getDS().modify(DN_USER_ONE, new Modification(ModificationType.ADD, "sshPublicKey", "environment=\"SSH=git\",command=\"netstat -nult\",environment=\"gbPerms=VIEW\" " + keyEcThree)); + + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "sshPublicKey", "environment=\"gbPerm=R\" " + keyRsaOne)); + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "sshPublicKey", " restrict,environment=\"gbperm=V\" " + keyRsaTwo)); + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "sshPublicKey", "restrict,environment=\"GBPerm=RW\",pty " + keyDsaTwo)); + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "sshPublicKey", " environment=\"gbPerm=CLONE\",environment=\"X=\\\" Y \\\"\" " + keyRsaThree)); + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "sshPublicKey", " environment=\"A = B \",from=\"*.example.com,!pc.example.com\",environment=\"gbPerm=VIEW\" " + keyDsaThree)); + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "sshPublicKey", "environment=\"SSH=git\",environment=\"gbPerm=PUSH\",environment=\"XYZ='Ali Baba'\" " + keyEcThree)); + + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "sshPublicKey", "environment=\"gbPerm=R\",environment=\"josh=\\\"mean\\\"\",tunnel=\"0\" " + keyRsaOne)); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "sshPublicKey", " environment=\" gbPerm = V \" " + keyRsaTwo)); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "sshPublicKey", "command=\"sh echo \\\"Nope, not you!\\\" \",user-rc,environment=\"gbPerm=RW\" " + keyDsaTwo)); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "sshPublicKey", "environment=\"gbPerm=VIEW\",command=\"sh /etc/netstart tun0 \",environment=\"gbPerm=CLONE\",no-pty " + keyRsaThree)); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "sshPublicKey", " command=\"netstat -nult\",environment=\"gbPerm=VIEW\" " + keyDsaThree)); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "sshPublicKey", "environment=\"SSH=git\",command=\"netstat -nult\",environment=\"gbPerm=PUSH\" " + keyEcThree)); + + + LdapKeyManager kmgr = new LdapKeyManager(settings); + + List keys = kmgr.getKeys("UserOne"); + assertNotNull(keys); + assertEquals(6, keys.size()); + for (SshKey key : keys) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + } + + keys = kmgr.getKeys("UserTwo"); + assertNotNull(keys); + assertEquals(6, keys.size()); + int seen = 0; + for (SshKey key : keys) { + if (keyRsaOne.equals(key.getRawData())) { + assertEquals(AccessPermission.CLONE, key.getPermission()); + seen += 1 << 0; + } + else if (keyRsaTwo.equals(key.getRawData())) { + assertEquals(AccessPermission.VIEW, key.getPermission()); + seen += 1 << 1; + } + else if (keyDsaTwo.equals(key.getRawData())) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + seen += 1 << 2; + } + else if (keyRsaThree.equals(key.getRawData())) { + assertEquals(AccessPermission.CLONE, key.getPermission()); + seen += 1 << 3; + } + else if (keyDsaThree.equals(key.getRawData())) { + assertEquals(AccessPermission.VIEW, key.getPermission()); + seen += 1 << 4; + } + else if (keyEcThree.equals(key.getRawData())) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + seen += 1 << 5; + } + } + assertEquals(63, seen); + + keys = kmgr.getKeys("UserThree"); + assertNotNull(keys); + assertEquals(6, keys.size()); + seen = 0; + for (SshKey key : keys) { + if (keyRsaOne.equals(key.getRawData())) { + assertEquals(AccessPermission.CLONE, key.getPermission()); + seen += 1 << 0; + } + else if (keyRsaTwo.equals(key.getRawData())) { + assertEquals(AccessPermission.VIEW, key.getPermission()); + seen += 1 << 1; + } + else if (keyDsaTwo.equals(key.getRawData())) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + seen += 1 << 2; + } + else if (keyRsaThree.equals(key.getRawData())) { + assertEquals(AccessPermission.CLONE, key.getPermission()); + seen += 1 << 3; + } + else if (keyDsaThree.equals(key.getRawData())) { + assertEquals(AccessPermission.VIEW, key.getPermission()); + seen += 1 << 4; + } + else if (keyEcThree.equals(key.getRawData())) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + seen += 1 << 5; + } + } + assertEquals(63, seen); + } + + + @Test + public void testGetKeysPrefixedPermissions() throws LDAPException { + // This test is independent from authentication mode, so run only once. + assumeTrue(authMode == AuthMode.ANONYMOUS); + + String keyRsaOne = getRsaPubKey("UserOne@example.com"); + String keyRsaTwo = getRsaPubKey("UserTwo at example.com"); + String keyDsaTwo = getDsaPubKey("UserTwo@example.com"); + String keyRsaThree = getRsaPubKey("example.com: user Three"); + String keyDsaThree = getDsaPubKey(""); + String keyEcThree = getEcPubKey(" "); + + getDS().modify(DN_USER_ONE, new Modification(ModificationType.ADD, "altSecurityIdentities", "permitopen=\"host:220\"" + keyRsaOne)); + getDS().modify(DN_USER_ONE, new Modification(ModificationType.ADD, "altSecurityIdentities", "sshkey:" + " " + keyRsaTwo)); + getDS().modify(DN_USER_ONE, new Modification(ModificationType.ADD, "altSecurityIdentities", "SSHKEY :" + "no-agent-forwarding " + keyDsaTwo)); + getDS().modify(DN_USER_ONE, new Modification(ModificationType.ADD, "altSecurityIdentities", "pubkey: " + " command=\"sh /etc/netstart tun0 \" " + keyRsaThree)); + getDS().modify(DN_USER_ONE, new Modification(ModificationType.ADD, "altSecurityIdentities", "pubkey: " + " command=\"netstat -nult\",environment=\"gb=\\\"What now\\\"\" " + keyDsaThree)); + getDS().modify(DN_USER_ONE, new Modification(ModificationType.ADD, "altSecurityIdentities", "pubkey: " + "environment=\"SSH=git\",command=\"netstat -nult\",environment=\"gbPerms=VIEW\" " + keyEcThree)); + + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "altSecurityIdentities", "SSHkey: " + "environment=\"gbPerm=R\" " + keyRsaOne)); + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "altSecurityIdentities", "SSHKey : " + " restrict,environment=\"gbPerm=V\",permitopen=\"sshkey: 220\" " + keyRsaTwo)); + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "altSecurityIdentities", "SSHkey: " + "permitopen=\"sshkey: 443\",restrict,environment=\"gbPerm=RW\",pty " + keyDsaTwo)); + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "altSecurityIdentities", "pubkey: " + "environment=\"gbPerm=CLONE\",permitopen=\"pubkey: 29184\",environment=\"X=\\\" Y \\\"\" " + keyRsaThree)); + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "altSecurityIdentities", "pubkey: " + " environment=\"A = B \",from=\"*.example.com,!pc.example.com\",environment=\"gbPerm=VIEW\" " + keyDsaThree)); + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "altSecurityIdentities", "pubkey: " + "environment=\"SSH=git\",environment=\"gbPerm=PUSH\",environemnt=\"XYZ='Ali Baba'\" " + keyEcThree)); + + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "altSecurityIdentities", "SSHkey: " + "environment=\"gbPerm=R\",environment=\"josh=\\\"mean\\\"\",tunnel=\"0\" " + keyRsaOne)); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "altSecurityIdentities", "SSHkey : " + " environment=\" gbPerm = V \" " + keyRsaTwo)); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "altSecurityIdentities", "SSHkey: " + "command=\"sh echo \\\"Nope, not you! \\b (bell)\\\" \",user-rc,environment=\"gbPerm=RW\" " + keyDsaTwo)); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "altSecurityIdentities", "pubkey: " + "environment=\"gbPerm=VIEW\",command=\"sh /etc/netstart tun0 \",environment=\"gbPerm=CLONE\",no-pty " + keyRsaThree)); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "altSecurityIdentities", "pubkey: " + " command=\"netstat -nult\",environment=\"gbPerm=VIEW\" " + keyDsaThree)); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "altSecurityIdentities", "pubkey: " + "environment=\"SSH=git\",command=\"netstat -nult\",environment=\"gbPerm=PUSH\" " + keyEcThree)); + + // Weird stuff, not to specification but shouldn't make it stumble. + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "altSecurityIdentities", "opttest: " + "permitopen=host:443,command=,environment=\"gbPerm=CLONE\",no-pty= " + keyRsaThree)); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "altSecurityIdentities", " opttest: " + " cmd=git,environment=\"gbPerm=\\\"VIEW\\\"\" " + keyDsaThree)); + getDS().modify(DN_USER_THREE, new Modification(ModificationType.ADD, "altSecurityIdentities", " opttest:" + "environment=,command=netstat,environment=gbperm=push " + keyEcThree)); + + + LdapKeyManager kmgr = new LdapKeyManager(settings); + + settings.put(Keys.realm.ldap.sshPublicKey, "altSecurityIdentities:SSHkey"); + + List keys = kmgr.getKeys("UserOne"); + assertNotNull(keys); + assertEquals(2, keys.size()); + int seen = 0; + for (SshKey key : keys) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + if (keyRsaOne.equals(key.getRawData())) { + seen += 1 << 0; + } + else if (keyRsaTwo.equals(key.getRawData())) { + seen += 1 << 1; + } + else if (keyDsaTwo.equals(key.getRawData())) { + seen += 1 << 2; + } + else if (keyRsaThree.equals(key.getRawData())) { + seen += 1 << 3; + } + else if (keyDsaThree.equals(key.getRawData())) { + seen += 1 << 4; + } + else if (keyEcThree.equals(key.getRawData())) { + seen += 1 << 5; + } + } + assertEquals(6, seen); + + keys = kmgr.getKeys("UserTwo"); + assertNotNull(keys); + assertEquals(3, keys.size()); + seen = 0; + for (SshKey key : keys) { + if (keyRsaOne.equals(key.getRawData())) { + assertEquals(AccessPermission.CLONE, key.getPermission()); + seen += 1 << 0; + } + else if (keyRsaTwo.equals(key.getRawData())) { + assertEquals(AccessPermission.VIEW, key.getPermission()); + seen += 1 << 1; + } + else if (keyDsaTwo.equals(key.getRawData())) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + seen += 1 << 2; + } + else if (keyRsaThree.equals(key.getRawData())) { + assertEquals(AccessPermission.CLONE, key.getPermission()); + seen += 1 << 3; + } + else if (keyDsaThree.equals(key.getRawData())) { + assertEquals(AccessPermission.VIEW, key.getPermission()); + seen += 1 << 4; + } + else if (keyEcThree.equals(key.getRawData())) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + seen += 1 << 5; + } + } + assertEquals(7, seen); + + keys = kmgr.getKeys("UserThree"); + assertNotNull(keys); + assertEquals(3, keys.size()); + seen = 0; + for (SshKey key : keys) { + if (keyRsaOne.equals(key.getRawData())) { + assertEquals(AccessPermission.CLONE, key.getPermission()); + seen += 1 << 0; + } + else if (keyRsaTwo.equals(key.getRawData())) { + assertEquals(AccessPermission.VIEW, key.getPermission()); + seen += 1 << 1; + } + else if (keyDsaTwo.equals(key.getRawData())) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + seen += 1 << 2; + } + else if (keyRsaThree.equals(key.getRawData())) { + assertEquals(AccessPermission.CLONE, key.getPermission()); + seen += 1 << 3; + } + else if (keyDsaThree.equals(key.getRawData())) { + assertEquals(AccessPermission.VIEW, key.getPermission()); + seen += 1 << 4; + } + else if (keyEcThree.equals(key.getRawData())) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + seen += 1 << 5; + } + } + assertEquals(7, seen); + + + + settings.put(Keys.realm.ldap.sshPublicKey, "altSecurityIdentities:pubKey"); + + keys = kmgr.getKeys("UserOne"); + assertNotNull(keys); + assertEquals(3, keys.size()); + seen = 0; + for (SshKey key : keys) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + if (keyRsaOne.equals(key.getRawData())) { + seen += 1 << 0; + } + else if (keyRsaTwo.equals(key.getRawData())) { + seen += 1 << 1; + } + else if (keyDsaTwo.equals(key.getRawData())) { + seen += 1 << 2; + } + else if (keyRsaThree.equals(key.getRawData())) { + seen += 1 << 3; + } + else if (keyDsaThree.equals(key.getRawData())) { + seen += 1 << 4; + } + else if (keyEcThree.equals(key.getRawData())) { + seen += 1 << 5; + } + } + assertEquals(56, seen); + + keys = kmgr.getKeys("UserTwo"); + assertNotNull(keys); + assertEquals(3, keys.size()); + seen = 0; + for (SshKey key : keys) { + if (keyRsaOne.equals(key.getRawData())) { + assertEquals(AccessPermission.CLONE, key.getPermission()); + seen += 1 << 0; + } + else if (keyRsaTwo.equals(key.getRawData())) { + assertEquals(AccessPermission.VIEW, key.getPermission()); + seen += 1 << 1; + } + else if (keyDsaTwo.equals(key.getRawData())) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + seen += 1 << 2; + } + else if (keyRsaThree.equals(key.getRawData())) { + assertEquals(AccessPermission.CLONE, key.getPermission()); + seen += 1 << 3; + } + else if (keyDsaThree.equals(key.getRawData())) { + assertEquals(AccessPermission.VIEW, key.getPermission()); + seen += 1 << 4; + } + else if (keyEcThree.equals(key.getRawData())) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + seen += 1 << 5; + } + } + assertEquals(56, seen); + + keys = kmgr.getKeys("UserThree"); + assertNotNull(keys); + assertEquals(3, keys.size()); + seen = 0; + for (SshKey key : keys) { + if (keyRsaOne.equals(key.getRawData())) { + assertEquals(AccessPermission.CLONE, key.getPermission()); + seen += 1 << 0; + } + else if (keyRsaTwo.equals(key.getRawData())) { + assertEquals(AccessPermission.VIEW, key.getPermission()); + seen += 1 << 1; + } + else if (keyDsaTwo.equals(key.getRawData())) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + seen += 1 << 2; + } + else if (keyRsaThree.equals(key.getRawData())) { + assertEquals(AccessPermission.CLONE, key.getPermission()); + seen += 1 << 3; + } + else if (keyDsaThree.equals(key.getRawData())) { + assertEquals(AccessPermission.VIEW, key.getPermission()); + seen += 1 << 4; + } + else if (keyEcThree.equals(key.getRawData())) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + seen += 1 << 5; + } + } + assertEquals(56, seen); + + + settings.put(Keys.realm.ldap.sshPublicKey, "altSecurityIdentities:opttest"); + keys = kmgr.getKeys("UserThree"); + assertNotNull(keys); + assertEquals(3, keys.size()); + seen = 0; + for (SshKey key : keys) { + if (keyRsaOne.equals(key.getRawData())) { + assertEquals(AccessPermission.CLONE, key.getPermission()); + seen += 1 << 0; + } + else if (keyRsaTwo.equals(key.getRawData())) { + assertEquals(AccessPermission.VIEW, key.getPermission()); + seen += 1 << 1; + } + else if (keyDsaTwo.equals(key.getRawData())) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + seen += 1 << 2; + } + else if (keyRsaThree.equals(key.getRawData())) { + assertEquals(AccessPermission.CLONE, key.getPermission()); + seen += 1 << 3; + } + else if (keyDsaThree.equals(key.getRawData())) { + assertEquals(AccessPermission.VIEW, key.getPermission()); + seen += 1 << 4; + } + else if (keyEcThree.equals(key.getRawData())) { + assertEquals(AccessPermission.PUSH, key.getPermission()); + seen += 1 << 5; + } + } + assertEquals(56, seen); + + } + + + @Test + public void testKeyValidity() throws LDAPException, GeneralSecurityException { + LdapKeyManager kmgr = new LdapKeyManager(settings); + + String comment = "UserTwo@example.com"; + String keyDsaTwo = getDsaPubKey(comment); + getDS().modify(DN_USER_TWO, new Modification(ModificationType.ADD, "sshPublicKey", keyDsaTwo)); + + + List keys = kmgr.getKeys("UserTwo"); + assertNotNull(keys); + assertEquals(1, keys.size()); + SshKey sshKey = keys.get(0); + assertEquals(keyDsaTwo, sshKey.getRawData()); + + Signature signature = SecurityUtils.getSignature("DSA"); + signature.initSign(getDsaKeyPair(comment).getPrivate()); + byte[] message = comment.getBytes(); + signature.update(message); + byte[] sigBytes = signature.sign(); + + signature.initVerify(sshKey.getPublicKey()); + signature.update(message); + assertTrue("Verify failed with retrieved SSH key.", signature.verify(sigBytes)); + } + + + + + + + + + private KeyPair getDsaKeyPair(String comment) { + return getKeyPair("DSA", comment, dsaGenerator); + } + + private KeyPair getKeyPair(String type, String comment, KeyPairGenerator generator) { + String kpkey = type + ":" + comment; + KeyPair kp = keyPairs.get(kpkey); + if (kp == null) { + if ("EC".equals(type)) { + ECGenParameterSpec ecSpec = new ECGenParameterSpec("P-384"); + try { + ecGenerator.initialize(ecSpec); + } catch (InvalidAlgorithmParameterException e) { + kp = generator.generateKeyPair(); + e.printStackTrace(); + } + kp = ecGenerator.generateKeyPair(); + } else { + kp = generator.generateKeyPair(); + } + keyPairs.put(kpkey, kp); + } + + return kp; + } + + + private String getRsaPubKey(String comment) { + return getPubKey("RSA", comment, rsaGenerator); + } + + private String getDsaPubKey(String comment) { + return getPubKey("DSA", comment, dsaGenerator); + } + + private String getEcPubKey(String comment) { + return getPubKey("EC", comment, ecGenerator); + } + + private String getPubKey(String type, String comment, KeyPairGenerator generator) { + KeyPair kp = getKeyPair(type, comment, generator); + if (kp == null) { + return null; + } + + SshKey sk = new SshKey(kp.getPublic()); + sk.setComment(comment); + return sk.getRawData(); + } + +} From cb89090d936ff8383d26f69eaeae6717d3a701e1 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Sat, 26 Nov 2016 17:35:21 +0100 Subject: [PATCH 4/7] Use dynamic port selection for LDAP listeners in LDAP tests. Instead of using fixed ports for the listeners of the in-memory LDAP server, let the listeners select ports and then save them in the authentication mode instance. This way we prevent port collisions, which especially showed up under Windows. --- .../com/gitblit/tests/LdapBasedUnitTest.java | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/test/java/com/gitblit/tests/LdapBasedUnitTest.java b/src/test/java/com/gitblit/tests/LdapBasedUnitTest.java index cf3ab1fa0..7aec50e6e 100644 --- a/src/test/java/com/gitblit/tests/LdapBasedUnitTest.java +++ b/src/test/java/com/gitblit/tests/LdapBasedUnitTest.java @@ -74,9 +74,9 @@ public abstract class LdapBasedUnitTest extends GitblitUnitTest { * */ protected enum AuthMode { - ANONYMOUS(1389), - DS_MANAGER(2389), - USR_MANAGER(3389); + ANONYMOUS, + DS_MANAGER, + USR_MANAGER; private int ldapPort; @@ -84,7 +84,7 @@ protected enum AuthMode { private InMemoryDirectoryServerSnapshot dsSnapshot; private BindTracker bindTracker; - AuthMode(int port) { + void setLdapPort(int port) { this.ldapPort = port; } @@ -151,23 +151,26 @@ public static Collection data() { public static void ldapInit() throws Exception { InMemoryDirectoryServer ds; InMemoryDirectoryServerConfig config = createInMemoryLdapServerConfig(AuthMode.ANONYMOUS); - config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("default", AuthMode.ANONYMOUS.ldapPort())); + config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("anonymous")); ds = createInMemoryLdapServer(config); AuthMode.ANONYMOUS.setDS(ds); + AuthMode.ANONYMOUS.setLdapPort(ds.getListenPort("anonymous")); config = createInMemoryLdapServerConfig(AuthMode.DS_MANAGER); - config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("default", AuthMode.DS_MANAGER.ldapPort())); + config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("ds_manager")); config.setAuthenticationRequiredOperationTypes(EnumSet.allOf(OperationType.class)); ds = createInMemoryLdapServer(config); AuthMode.DS_MANAGER.setDS(ds); + AuthMode.DS_MANAGER.setLdapPort(ds.getListenPort("ds_manager")); config = createInMemoryLdapServerConfig(AuthMode.USR_MANAGER); - config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("default", AuthMode.USR_MANAGER.ldapPort())); + config.setListenerConfigs(InMemoryListenerConfig.createLDAPConfig("usr_manager")); config.setAuthenticationRequiredOperationTypes(EnumSet.allOf(OperationType.class)); ds = createInMemoryLdapServer(config); AuthMode.USR_MANAGER.setDS(ds); + AuthMode.USR_MANAGER.setLdapPort(ds.getListenPort("usr_manager")); } @@ -220,19 +223,17 @@ protected InMemoryDirectoryServer getDS() { protected MemorySettings getSettings() { Map backingMap = new HashMap(); backingMap.put(Keys.realm.userService, usersConf.getAbsolutePath()); + backingMap.put(Keys.realm.ldap.server, "ldap://localhost:" + authMode.ldapPort()); 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; From a3f9b4f64e52ba1833c3bcb18cf7f05b4d35714e Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Tue, 29 Nov 2016 21:46:54 +0100 Subject: [PATCH 5/7] Fix SshKeysDispatcher test failing on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `SshKeysDispatcher` tests that use the keys list command are failing on Windows because they assume a Unix line ending after each key. But the command will use a system line ending. So this fix uses system line endings in the reference string for the assert, too. In addition, two `assertTrue(false)´ are replaced with a proper `fail`. --- .../com/gitblit/tests/SshKeysDispatcherTest.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/test/java/com/gitblit/tests/SshKeysDispatcherTest.java b/src/test/java/com/gitblit/tests/SshKeysDispatcherTest.java index 23e617952..4784e4685 100644 --- a/src/test/java/com/gitblit/tests/SshKeysDispatcherTest.java +++ b/src/test/java/com/gitblit/tests/SshKeysDispatcherTest.java @@ -37,7 +37,7 @@ public void testKeysListCommand() throws Exception { String result = testSshCommand("keys ls -L"); List keys = getKeyManager().getKeys(username); assertEquals(String.format("There are %d keys!", keys.size()), 2, keys.size()); - assertEquals(keys.get(0).getRawData() + "\n" + keys.get(1).getRawData(), result); + assertEquals(String.format("%s%n%s", keys.get(0).getRawData(), keys.get(1).getRawData()), result); } @Test @@ -64,9 +64,9 @@ public void testKeysRmAllByIndexCommand() throws Exception { assertEquals(String.format("There are %d keys!", keys.size()), 0, keys.size()); try { testSshCommand("keys ls -L"); - assertTrue("Authentication worked without a public key?!", false); + fail("Authentication worked without a public key?!"); } catch (AssertionError e) { - assertTrue(true); + // expected } } @@ -77,9 +77,9 @@ public void testKeysRmAllCommand() throws Exception { assertEquals(String.format("There are %d keys!", keys.size()), 0, keys.size()); try { testSshCommand("keys ls -L"); - assertTrue("Authentication worked without a public key?!", false); + fail("Authentication worked without a public key?!"); } catch (AssertionError e) { - assertTrue(true); + // expected } } @@ -96,9 +96,9 @@ public void testKeysAddCommand() throws Exception { StringBuilder sb = new StringBuilder(); for (SshKey sk : keys) { sb.append(sk.getRawData()); - sb.append('\n'); + sb.append(System.getProperty("line.separator", "\n")); } - sb.setLength(sb.length() - 1); + sb.setLength(sb.length() - System.getProperty("line.separator", "\n").length()); assertEquals(sb.toString(), result); } From 40040b656299bfafcaa92b12b916f93e8c5aed1d Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Tue, 29 Nov 2016 22:08:50 +0100 Subject: [PATCH 6/7] The public key manager can disable writing keys, which hides commands Some public key mangers may be read-only, i.e. not allow to add or delete keys, or to change the key comment or assigned permissions. In such a case the respective commands should not be available on the SSH shell and the SSH Keys panel should also not offer the possibility. The `IPublicKeyManager` gets three new methods, modelled after the `AuthenticationManager`: `supportsWritingKeys`, `supportsCommentChanges` and `supportsPermissionChanges`. They return true if a key manager allows for keys to be written or updated. For example the existing `FileKeyManager` will return true for all three since it allows to store and update keys in a file. The new `LdapKeyManager` returns false since it only accesses LDAP and can not add or update any keys in the directory. A future key manager might get keys from an LDAP directory but still keep comments and permissions for it in a local copy. If writing of keys is not supported: * the welcome shell does not suggest adding a key, * the `SshKeysDispatcher` does not offer the "add", "remove", "comment" and "permission" commands, and * the SSH keys panel hides the "delete" button in the key list, and the "Add Key" form. The hiding of the "Add key" form is not perfect since the surrounding div is still shown, but I don't know how to hide it and it didn't look too bad, either. --- .../transport/ssh/IPublicKeyManager.java | 13 ++++++++++++ .../gitblit/transport/ssh/LdapKeyManager.java | 13 ++++++++++++ .../com/gitblit/transport/ssh/SshDaemon.java | 2 +- .../gitblit/transport/ssh/WelcomeShell.java | 21 ++++++++++++------- .../transport/ssh/keys/KeysDispatcher.java | 17 +++++++++++---- .../gitblit/wicket/panels/SshKeysPanel.java | 9 ++++++++ 6 files changed, 63 insertions(+), 12 deletions(-) diff --git a/src/main/java/com/gitblit/transport/ssh/IPublicKeyManager.java b/src/main/java/com/gitblit/transport/ssh/IPublicKeyManager.java index 1e74b2f03..ffe64f593 100644 --- a/src/main/java/com/gitblit/transport/ssh/IPublicKeyManager.java +++ b/src/main/java/com/gitblit/transport/ssh/IPublicKeyManager.java @@ -25,6 +25,7 @@ import org.slf4j.LoggerFactory; import com.gitblit.manager.IManager; +import com.gitblit.models.UserModel; import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; import com.google.common.cache.CacheLoader.InvalidCacheLoadException; @@ -99,4 +100,16 @@ public final void renameUser(String oldName, String newName) { public abstract boolean removeKey(String username, SshKey key); public abstract boolean removeAllKeys(String username); + + public boolean supportsWritingKeys(UserModel user) { + return (user != null); + } + + public boolean supportsCommentChanges(UserModel user) { + return (user != null); + } + + public boolean supportsPermissionChanges(UserModel user) { + return (user != null); + } } diff --git a/src/main/java/com/gitblit/transport/ssh/LdapKeyManager.java b/src/main/java/com/gitblit/transport/ssh/LdapKeyManager.java index 9612a96b9..6b8f1e45f 100644 --- a/src/main/java/com/gitblit/transport/ssh/LdapKeyManager.java +++ b/src/main/java/com/gitblit/transport/ssh/LdapKeyManager.java @@ -34,6 +34,7 @@ import com.gitblit.Keys; import com.gitblit.Constants.AccessPermission; import com.gitblit.ldap.LdapConnection; +import com.gitblit.models.UserModel; import com.gitblit.utils.StringUtils; import com.google.common.base.Joiner; import com.google.inject.Inject; @@ -219,6 +220,18 @@ public boolean removeAllKeys(String username) { } + public boolean supportsWritingKeys(UserModel user) { + return false; + } + + public boolean supportsCommentChanges(UserModel user) { + return false; + } + + public boolean supportsPermissionChanges(UserModel user) { + return false; + } + private void setKeyPermissions(SshKey key, GbAuthorizedKeyEntry keyEntry) { List env = keyEntry.getLoginOptionValues("environment"); diff --git a/src/main/java/com/gitblit/transport/ssh/SshDaemon.java b/src/main/java/com/gitblit/transport/ssh/SshDaemon.java index 5a94c9a3f..4fb05f79c 100644 --- a/src/main/java/com/gitblit/transport/ssh/SshDaemon.java +++ b/src/main/java/com/gitblit/transport/ssh/SshDaemon.java @@ -134,7 +134,7 @@ public SshDaemon(IGitblit gitblit, WorkQueue workQueue) { sshd.setFileSystemFactory(new DisabledFilesystemFactory()); sshd.setTcpipForwardingFilter(new NonForwardingFilter()); sshd.setCommandFactory(new SshCommandFactory(gitblit, workQueue)); - sshd.setShellFactory(new WelcomeShell(settings)); + sshd.setShellFactory(new WelcomeShell(gitblit)); // Set the server id. This can be queried with: // ssh-keyscan -t rsa,dsa -p 29418 localhost diff --git a/src/main/java/com/gitblit/transport/ssh/WelcomeShell.java b/src/main/java/com/gitblit/transport/ssh/WelcomeShell.java index ec6f72914..7c407d365 100644 --- a/src/main/java/com/gitblit/transport/ssh/WelcomeShell.java +++ b/src/main/java/com/gitblit/transport/ssh/WelcomeShell.java @@ -34,6 +34,7 @@ import com.gitblit.IStoredSettings; import com.gitblit.Keys; +import com.gitblit.manager.IGitblit; import com.gitblit.models.UserModel; import com.gitblit.transport.ssh.commands.DispatchCommand; import com.gitblit.transport.ssh.commands.SshCommandFactory; @@ -45,19 +46,20 @@ */ public class WelcomeShell implements Factory { - private final IStoredSettings settings; + private final IGitblit gitblit; - public WelcomeShell(IStoredSettings settings) { - this.settings = settings; + public WelcomeShell(IGitblit gitblit) { + this.gitblit = gitblit; } @Override public Command create() { - return new SendMessage(settings); + return new SendMessage(gitblit); } private static class SendMessage implements Command, SessionAware { + private final IPublicKeyManager km; private final IStoredSettings settings; private ServerSession session; @@ -66,8 +68,9 @@ private static class SendMessage implements Command, SessionAware { private OutputStream err; private ExitCallback exit; - SendMessage(IStoredSettings settings) { - this.settings = settings; + SendMessage(IGitblit gitblit) { + this.km = gitblit.getPublicKeyManager(); + this.settings = gitblit.getSettings(); } @Override @@ -116,6 +119,10 @@ String getMessage() { UserModel user = client.getUser(); String hostname = getHostname(); int port = settings.getInteger(Keys.git.sshPort, 0); + boolean writeKeysIsSupported = true; + if (km != null) { + writeKeysIsSupported = km.supportsWritingKeys(user); + } final String b1 = StringUtils.rightPad("", 72, '═'); final String b2 = StringUtils.rightPad("", 72, '─'); @@ -159,7 +166,7 @@ String getMessage() { msg.append(nl); msg.append(nl); - if (client.getKey() == null) { + if (writeKeysIsSupported && client.getKey() == null) { // user has authenticated with a password // display add public key instructions msg.append(" You may upload an SSH public key with the following syntax:"); diff --git a/src/main/java/com/gitblit/transport/ssh/keys/KeysDispatcher.java b/src/main/java/com/gitblit/transport/ssh/keys/KeysDispatcher.java index da58584c9..817a98ffc 100644 --- a/src/main/java/com/gitblit/transport/ssh/keys/KeysDispatcher.java +++ b/src/main/java/com/gitblit/transport/ssh/keys/KeysDispatcher.java @@ -25,6 +25,7 @@ import org.slf4j.LoggerFactory; import com.gitblit.Constants.AccessPermission; +import com.gitblit.models.UserModel; import com.gitblit.transport.ssh.IPublicKeyManager; import com.gitblit.transport.ssh.SshKey; import com.gitblit.transport.ssh.commands.CommandMetaData; @@ -47,12 +48,20 @@ public class KeysDispatcher extends DispatchCommand { @Override protected void setup() { - register(AddKey.class); - register(RemoveKey.class); + IPublicKeyManager km = getContext().getGitblit().getPublicKeyManager(); + UserModel user = getContext().getClient().getUser(); + if (km != null && km.supportsWritingKeys(user)) { + register(AddKey.class); + register(RemoveKey.class); + } register(ListKeys.class); register(WhichKey.class); - register(CommentKey.class); - register(PermissionKey.class); + if (km != null && km.supportsCommentChanges(user)) { + register(CommentKey.class); + } + if (km != null && km.supportsPermissionChanges(user)) { + register(PermissionKey.class); + } } @CommandMetaData(name = "add", description = "Add an SSH public key to your account") diff --git a/src/main/java/com/gitblit/wicket/panels/SshKeysPanel.java b/src/main/java/com/gitblit/wicket/panels/SshKeysPanel.java index 15ebd67b7..4b8787630 100644 --- a/src/main/java/com/gitblit/wicket/panels/SshKeysPanel.java +++ b/src/main/java/com/gitblit/wicket/panels/SshKeysPanel.java @@ -48,11 +48,13 @@ public class SshKeysPanel extends BasePanel { private static final long serialVersionUID = 1L; private final UserModel user; + private final boolean canWriteKeys; public SshKeysPanel(String wicketId, UserModel user) { super(wicketId); this.user = user; + this.canWriteKeys = app().keys().supportsWritingKeys(user); } @Override @@ -90,6 +92,9 @@ public void onClick(AjaxRequestTarget target) { } } }; + if (!canWriteKeys) { + delete.setVisibilityAllowed(false); + } item.add(delete); } }; @@ -164,6 +169,10 @@ protected void onSubmit(AjaxRequestTarget target, Form form) { } }); + if (! canWriteKeys) { + addKeyForm.setVisibilityAllowed(false); + } + add(addKeyForm); } } From 1afeccc09bfaa885b5c01d3db29d42695b8290a1 Mon Sep 17 00:00:00 2001 From: Florian Zschocke Date: Mon, 5 Dec 2016 15:58:06 +0100 Subject: [PATCH 7/7] Extend documentation in default.properties and LdapKeyManager.java. --- src/main/distrib/data/defaults.properties | 6 ++++- .../gitblit/transport/ssh/LdapKeyManager.java | 27 ++++++++++++++++++- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/main/distrib/data/defaults.properties b/src/main/distrib/data/defaults.properties index 1fe5b345e..b9d77fe7f 100644 --- a/src/main/distrib/data/defaults.properties +++ b/src/main/distrib/data/defaults.properties @@ -1938,7 +1938,11 @@ realm.ldap.uid = uid # Attribute on the USER record that indicates their public SSH key. # Leave blank when public SSH keys shall not be retrieved from LDAP. # -# This may be a simple attribute or an attribute and a value prefix. Examples: +# This setting is only relevant when a public key manager is used that +# retrieves SSH keys from LDAP (e.g. com.gitblit.transport.ssh.LdapKeyManager). +# +# The accepted format of the value is dependent on the public key manager used. +# Examples: # sshPublicKey - Use the attribute 'sshPublicKey' on the user record. # altSecurityIdentities:SshKey - Use the attribute 'altSecurityIdentities' # on the user record, for which the record value diff --git a/src/main/java/com/gitblit/transport/ssh/LdapKeyManager.java b/src/main/java/com/gitblit/transport/ssh/LdapKeyManager.java index 6b8f1e45f..c62c4dee2 100644 --- a/src/main/java/com/gitblit/transport/ssh/LdapKeyManager.java +++ b/src/main/java/com/gitblit/transport/ssh/LdapKeyManager.java @@ -44,11 +44,36 @@ import com.unboundid.ldap.sdk.SearchResultEntry; /** - * LDAP public key manager + * LDAP-only public key manager * * Retrieves public keys from user's LDAP entries. Using this key manager, * no SSH keys can be edited, i.e. added, removed, permissions changed, etc. * + * This key manager supports SSH key entries in LDAP of the following form: + * [:] [] [] + * This follows the required form of entries in the authenticated_keys file, + * with an additional optional prefix. Key entries must have a key type + * (like "ssh-rsa") and a key, and may have a comment at the end. + * + * An entry may specify login options as specified for the authorized_keys file. + * The 'environment' option may be used to set the permissions for the key + * by setting a 'gbPerm' environment variable. The key manager will interpret + * such a environment variable option and use the set permission string to set + * the permission on the key in Gitblit. Example: + * environment="gbPerm=V",pty ssh-rsa AAAxjka.....dv= Clone only key + * Above entry would create a RSA key with the comment "Clone only key" and + * set the key permission to CLONE. All other options are ignored. + * + * In Active Directory SSH public keys are sometimes stored in the attribute + * 'altSecurityIdentity'. The attribute value is usually prefixed by a type + * identifier. LDAP entries could have the following attribute values: + * altSecurityIdentity: X.509: ADKEJBAKDBZUPABBD... + * altSecurityIdentity: SshKey: ssh-dsa AAAAknenazuzucbhda... + * This key manager supports this by allowing an optional prefix to identify + * SSH keys. The prefix to be used should be set in the 'realm.ldap.sshPublicKey' + * setting by separating it from the attribute name with a colon, e.g.: + * realm.ldap.sshPublicKey = altSecurityIdentity:SshKey + * * @author Florian Zschocke * */