Unit tests for ssh daemon and keys dispatcher

8 files changed, 66 insertions, 56 deletions

diff --git a/src/main/java/com/gitblit/transport/ssh/CachingPublicKeyAuthenticator.java b/src/main/java/com/gitblit/transport/ssh/CachingPublicKeyAuthenticator.java
index 48e5aa28..4ce26d0f 100644
--- a/src/main/java/com/gitblit/transport/ssh/CachingPublicKeyAuthenticator.java
+++ b/src/main/java/com/gitblit/transport/ssh/CachingPublicKeyAuthenticator.java
@@ -38,8 +38,7 @@ import com.google.common.base.Preconditions;
  * @author Eric Myrhe
  *
  */
-public class CachingPublicKeyAuthenticator implements PublickeyAuthenticator,
-		SessionListener {
+public class CachingPublicKeyAuthenticator implements PublickeyAuthenticator, SessionListener {
 
 	protected final Logger log = LoggerFactory.getLogger(getClass());
 
@@ -47,18 +46,15 @@ public class CachingPublicKeyAuthenticator implements PublickeyAuthenticator,
 
 	protected final IAuthenticationManager authManager;
 
-	private final Map<ServerSession, Map<PublicKey, Boolean>> cache =
-			new ConcurrentHashMap<ServerSession, Map<PublicKey, Boolean>>();
+	private final Map<ServerSession, Map<PublicKey, Boolean>> cache = new ConcurrentHashMap<ServerSession, Map<PublicKey, Boolean>>();
 
-	public CachingPublicKeyAuthenticator(IPublicKeyManager keyManager,
-			IAuthenticationManager authManager) {
+	public CachingPublicKeyAuthenticator(IPublicKeyManager keyManager, IAuthenticationManager authManager) {
 		this.keyManager = keyManager;
 		this.authManager = authManager;
 	}
 
 	@Override
-	public boolean authenticate(String username, PublicKey key,
-			ServerSession session) {
+	public boolean authenticate(String username, PublicKey key, ServerSession session) {
 		Map<PublicKey, Boolean> map = cache.get(session);
 		if (map == null) {
 			map = new HashMap<PublicKey, Boolean>();
@@ -73,19 +69,21 @@ public class CachingPublicKeyAuthenticator implements PublickeyAuthenticator,
 		return result;
 	}
 
-	private boolean doAuthenticate(String username, PublicKey suppliedKey,
-			ServerSession session) {
+	private boolean doAuthenticate(String username, PublicKey suppliedKey, ServerSession session) {
 		SshDaemonClient client = session.getAttribute(SshDaemonClient.KEY);
 		Preconditions.checkState(client.getUser() == null);
 		username = username.toLowerCase(Locale.US);
 		List<SshKey> keys = keyManager.getKeys(username);
-		if (keys == null || keys.isEmpty()) {
-			log.info("{} has not added any public keys for ssh authentication",
-					username);
+		if (keys.isEmpty()) {
+			log.info("{} has not added any public keys for ssh authentication", username);
 			return false;
 		}
 
+		SshKey pk = new SshKey(suppliedKey);
+		log.debug("auth supplied {}", pk.getFingerprint());
+
 		for (SshKey key : keys) {
+			log.debug("auth compare to {}", key.getFingerprint());
 			if (key.equals(suppliedKey)) {
 				UserModel user = authManager.authenticate(username, key);
 				if (user != null) {
@@ -96,8 +94,7 @@ public class CachingPublicKeyAuthenticator implements PublickeyAuthenticator,
 			}
 		}
 
-		log.warn("could not authenticate {} for SSH using the supplied public key",
-				username);
+		log.warn("could not authenticate {} for SSH using the supplied public key", username);
 		return false;
 	}
 
diff --git a/src/main/java/com/gitblit/transport/ssh/FileKeyManager.java b/src/main/java/com/gitblit/transport/ssh/FileKeyManager.java
index ae4588ae..a063dc7d 100644
--- a/src/main/java/com/gitblit/transport/ssh/FileKeyManager.java
+++ b/src/main/java/com/gitblit/transport/ssh/FileKeyManager.java
@@ -90,7 +90,7 @@ public class FileKeyManager extends IPublicKeyManager {
 	@Override
 	protected List<SshKey> getKeysImpl(String username) {
 		try {
-			log.info("loading keystore for {}", username);
+			log.info("loading ssh keystore for {}", username);
 			File keystore = getKeystore(username);
 			if (!keystore.exists()) {
 				return null;
@@ -128,7 +128,7 @@ public class FileKeyManager extends IPublicKeyManager {
 				return list;
 			}
 		} catch (IOException e) {
-			throw new RuntimeException("Canot read ssh keys", e);
+			throw new RuntimeException("Cannot read ssh keys", e);
 		}
 		return null;
 	}
diff --git a/src/main/java/com/gitblit/transport/ssh/IPublicKeyManager.java b/src/main/java/com/gitblit/transport/ssh/IPublicKeyManager.java
index 956a76ef..0dbee637 100644
--- a/src/main/java/com/gitblit/transport/ssh/IPublicKeyManager.java
+++ b/src/main/java/com/gitblit/transport/ssh/IPublicKeyManager.java
@@ -16,6 +16,7 @@
 package com.gitblit.transport.ssh;
 
 import java.text.MessageFormat;
+import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
@@ -46,7 +47,11 @@ public abstract class IPublicKeyManager implements IManager {
 			.build(new CacheLoader<String, List<SshKey>>() {
 				@Override
 				public List<SshKey> load(String username) {
-					return getKeysImpl(username);
+					List<SshKey> keys = getKeysImpl(username);
+					if (keys == null) {
+						return Collections.emptyList();
+					}
+					return Collections.unmodifiableList(keys);
 				}
 			});
 
diff --git a/src/main/java/com/gitblit/transport/ssh/MemoryKeyManager.java b/src/main/java/com/gitblit/transport/ssh/MemoryKeyManager.java
index 18f9a4e1..357b34a2 100644
--- a/src/main/java/com/gitblit/transport/ssh/MemoryKeyManager.java
+++ b/src/main/java/com/gitblit/transport/ssh/MemoryKeyManager.java
@@ -28,7 +28,7 @@ import java.util.Map;
  */
 public class MemoryKeyManager extends IPublicKeyManager {
 
-	Map<String, List<SshKey>> keys;
+	final Map<String, List<SshKey>> keys;
 
 	public MemoryKeyManager() {
 		keys = new HashMap<String, List<SshKey>>();
@@ -57,7 +57,8 @@ public class MemoryKeyManager extends IPublicKeyManager {
 
 	@Override
 	protected boolean isStale(String username) {
-		return false;
+		// always return true so we gets keys from our hashmap
+		return true;
 	}
 
 	@Override
@@ -75,6 +76,7 @@ public class MemoryKeyManager extends IPublicKeyManager {
 		if (!keys.containsKey(id)) {
 			keys.put(id, new ArrayList<SshKey>());
 		}
+		log.info("added {} key {}", username, key.getFingerprint());
 		return keys.get(id).add(key);
 	}
 
@@ -82,15 +84,27 @@ public class MemoryKeyManager extends IPublicKeyManager {
 	public boolean removeKey(String username, SshKey key) {
 		String id = username.toLowerCase();
 		if (!keys.containsKey(id)) {
+			log.info("can't remove keys for {}", username);
 			return false;
 		}
-		return keys.get(id).remove(key);
+		List<SshKey> list = keys.get(id);
+		boolean success = list.remove(key);
+		if (success) {
+			log.info("removed {} key {}", username, key.getFingerprint());
+		}
+
+		if (list.isEmpty()) {
+			keys.remove(id);
+			log.info("no {} keys left, removed {}", username, username);
+		}
+		return success;
 	}
 
 	@Override
 	public boolean removeAllKeys(String username) {
 		String id = username.toLowerCase();
 		keys.remove(id.toLowerCase());
+		log.info("removed all keys for {}", username);
 		return true;
 	}
 }
diff --git a/src/main/java/com/gitblit/transport/ssh/SshKey.java b/src/main/java/com/gitblit/transport/ssh/SshKey.java
index 6ac0cdcb..6a20d7dd 100644
--- a/src/main/java/com/gitblit/transport/ssh/SshKey.java
+++ b/src/main/java/com/gitblit/transport/ssh/SshKey.java
@@ -155,8 +155,8 @@ public class SshKey implements Serializable {
 				final byte [] bin = Base64.decodeBase64(Constants.encodeASCII(parts[1]));
 				hash = StringUtils.getMD5(bin);
 			} else {
-				// TODO get hash from publickey
-				hash = "todo";
+				// TODO calculate the correct hash from a PublicKey instance
+				hash = StringUtils.getMD5(getPublicKey().getEncoded());
 			}
 			for (int i = 0; i < hash.length(); i += 2) {
 				sb.append(hash.charAt(i)).append(hash.charAt(i + 1)).append(':');
diff --git a/src/main/java/com/gitblit/transport/ssh/SshServerSessionFactory.java b/src/main/java/com/gitblit/transport/ssh/SshServerSessionFactory.java
index dd3c139d..0c018f02 100644
--- a/src/main/java/com/gitblit/transport/ssh/SshServerSessionFactory.java
+++ b/src/main/java/com/gitblit/transport/ssh/SshServerSessionFactory.java
@@ -41,7 +41,7 @@ public class SshServerSessionFactory extends SessionFactory {
 
 	@Override
 	protected AbstractSession createSession(final IoSession io) throws Exception {
-		log.info("connection accepted on " + io);
+		log.info("creating ssh session from {}", io.getRemoteAddress());
 
 		if (io instanceof MinaSession) {
 			if (((MinaSession) io).getSession().getConfig() instanceof SocketSessionConfig) {
@@ -59,7 +59,7 @@ public class SshServerSessionFactory extends SessionFactory {
 		session.addCloseSessionListener(new SshFutureListener<CloseFuture>() {
 			@Override
 			public void operationComplete(CloseFuture future) {
-				log.info("connection closed on " + io);
+				log.info("closed ssh session from {}", io.getRemoteAddress());
 			}
 		});
 		return session;
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 62daec6a..3f581462 100644
--- a/src/main/java/com/gitblit/transport/ssh/keys/KeysDispatcher.java
+++ b/src/main/java/com/gitblit/transport/ssh/keys/KeysDispatcher.java
@@ -61,7 +61,7 @@ public class KeysDispatcher extends DispatchCommand {
 
 		protected final Logger log = LoggerFactory.getLogger(getClass());
 
-		@Argument(metaVar = "<KEY>", usage = "the key(s) to add")
+		@Argument(metaVar = "<STDIN>", usage = "the key to add")
 		private List<String> addKeys = new ArrayList<String>();
 
 		@Option(name = "--permission", aliases = { "-p" }, metaVar = "PERMISSION", usage = "set the key access permission")
@@ -76,7 +76,7 @@ public class KeysDispatcher extends DispatchCommand {
 		}
 
 		@Override
-		public void run() throws IOException, UnloggedFailure {
+		public void run() throws IOException, Failure {
 			String username = getContext().getClient().getUsername();
 			List<String> keys = readKeys(addKeys);
 			for (String key : keys) {
@@ -87,7 +87,7 @@ public class KeysDispatcher extends DispatchCommand {
 						try {
 							sshKey.setPermission(ap);
 						} catch (IllegalArgumentException e) {
-							throw new UnloggedFailure(1, e.getMessage());
+							throw new Failure(1, e.getMessage());
 						}
 					}
 				}
@@ -105,22 +105,21 @@ public class KeysDispatcher extends DispatchCommand {
 
 		private final String ALL = "ALL";
 
-		@Argument(metaVar = "<INDEX>|<KEY>|ALL", usage = "the key to remove", required = true)
-		private List<String> removeKeys = new ArrayList<String>();
+		@Argument(metaVar = "<INDEX>|ALL", usage = "the key to remove", required = true)
+		private List<String> keyParameters = new ArrayList<String>();
 
 		@Override
-		public void run() throws IOException, UnloggedFailure {
+		public void run() throws IOException, Failure {
 			String username = getContext().getClient().getUsername();
 			// remove a key that has been piped to the command
 			// or remove all keys
 
-			List<SshKey> currentKeys = getKeyManager().getKeys(username);
-			if (currentKeys == null || currentKeys.isEmpty()) {
+			List<SshKey> registeredKeys = new ArrayList<SshKey>(getKeyManager().getKeys(username));
+			if (registeredKeys.isEmpty()) {
 				throw new UnloggedFailure(1, "There are no registered keys!");
 			}
 
-			List<String> keys = readKeys(removeKeys);
-			if (keys.contains(ALL)) {
+			if (keyParameters.contains(ALL)) {
 				if (getKeyManager().removeAllKeys(username)) {
 					stdout.println("Removed all keys.");
 					log.info("removed all SSH public keys from {}", username);
@@ -128,32 +127,25 @@ public class KeysDispatcher extends DispatchCommand {
 					log.warn("failed to remove all SSH public keys from {}", username);
 				}
 			} else {
-				for (String key : keys) {
+				for (String keyParameter : keyParameters) {
 					try {
 						// remove a key by it's index (1-based indexing)
-						int index = Integer.parseInt(key);
-						if (index > keys.size()) {
-							if (keys.size() == 1) {
-								throw new UnloggedFailure(1, "Invalid index specified. There is only 1 registered key.");
+						int index = Integer.parseInt(keyParameter);
+						if (index > registeredKeys.size()) {
+							if (keyParameters.size() == 1) {
+								throw new Failure(1, "Invalid index specified. There is only 1 registered key.");
 							}
-							throw new UnloggedFailure(1, String.format("Invalid index specified. There are %d registered keys.", keys.size()));
+							throw new Failure(1, String.format("Invalid index specified. There are %d registered keys.", registeredKeys.size()));
 						}
-						SshKey sshKey = currentKeys.get(index - 1);
+						SshKey sshKey = registeredKeys.get(index - 1);
 						if (getKeyManager().removeKey(username, sshKey)) {
 							stdout.println(String.format("Removed %s", sshKey.getFingerprint()));
 						} else {
-							throw new UnloggedFailure(1,  String.format("failed to remove #%s: %s", key, sshKey.getFingerprint()));
-						}
-					} catch (Exception e) {
-						// remove key by raw key data
-						SshKey sshKey = parseKey(key);
-						if (getKeyManager().removeKey(username, sshKey)) {
-							stdout.println(String.format("Removed %s", sshKey.getFingerprint()));
-							log.info("removed SSH public key {} from {}", sshKey.getFingerprint(), username);
-						} else {
-							log.warn("failed to remove SSH public key {} from {}", sshKey.getFingerprint(), username);
-							throw new UnloggedFailure(1,  String.format("failed to remove %s", sshKey.getFingerprint()));
+							throw new Failure(1,  String.format("failed to remove #%s: %s", keyParameter, sshKey.getFingerprint()));
 						}
+					} catch (NumberFormatException e) {
+						log.warn("failed to remove SSH public key {} from {}", keyParameter, username);
+						throw new Failure(1,  String.format("failed to remove key %s", keyParameter));
 					}
 				}
 			}
@@ -254,7 +246,7 @@ public class KeysDispatcher extends DispatchCommand {
 		private List<String> values = new ArrayList<String>();
 
 		@Override
-		public void run() throws UnloggedFailure {
+		public void run() throws Failure {
 			final String username = getContext().getClient().getUsername();
 			IPublicKeyManager keyManager = getContext().getGitblit().getPublicKeyManager();
 			List<SshKey> keys = keyManager.getKeys(username);
@@ -268,7 +260,7 @@ public class KeysDispatcher extends DispatchCommand {
 			if (keyManager.addKey(username, key)) {
 				stdout.println(String.format("Updated the comment for key #%d.", index));
 			} else {
-				throw new UnloggedFailure(1, String.format("Failed to update the comment for key #%d!", index));
+				throw new Failure(1, String.format("Failed to update the comment for key #%d!", index));
 			}
 		}
 
diff --git a/src/main/java/log4j.properties b/src/main/java/log4j.properties
index 115dcd01..43d31d80 100644
--- a/src/main/java/log4j.properties
+++ b/src/main/java/log4j.properties
@@ -25,7 +25,9 @@ log4j.rootCategory=INFO, S
 #log4j.logger.net=INFO
 
 #log4j.logger.com.gitblit=DEBUG
-log4j.logger.org.apache.sshd=ERROR
+log4j.logger.com.gitblit.transport.ssh.SshServerSession=WARN
+log4j.logger.org.apache.sshd=WARN
+log4j.logger.org.apache.mina=WARN
 
 log4j.logger.org.apache.wicket=INFO
 log4j.logger.org.apache.wicket.RequestListenerInterface=WARN