Revise SSH public key integration with AuthenticationManager

8 files changed, 67 insertions, 59 deletions

diff --git a/src/main/java/com/gitblit/Constants.java b/src/main/java/com/gitblit/Constants.java
index 889e5a30..56dfec06 100644
--- a/src/main/java/com/gitblit/Constants.java
+++ b/src/main/java/com/gitblit/Constants.java
@@ -501,7 +501,7 @@ public class Constants {
 	}

 

 	public static enum AuthenticationType {

-		SSH, CREDENTIALS, COOKIE, CERTIFICATE, CONTAINER;

+		PUBLIC_KEY, CREDENTIALS, COOKIE, CERTIFICATE, CONTAINER;

 

 		public boolean isStandard() {

 			return ordinal() <= COOKIE.ordinal();

diff --git a/src/main/java/com/gitblit/git/GitblitUploadPackFactory.java b/src/main/java/com/gitblit/git/GitblitUploadPackFactory.java
index a72d4ad9..7a476775 100644
--- a/src/main/java/com/gitblit/git/GitblitUploadPackFactory.java
+++ b/src/main/java/com/gitblit/git/GitblitUploadPackFactory.java
@@ -26,7 +26,6 @@ import org.eclipse.jgit.transport.resolver.UploadPackFactory;
 import com.gitblit.manager.IAuthenticationManager;
 import com.gitblit.models.UserModel;
 import com.gitblit.transport.git.GitDaemonClient;
-import com.gitblit.transport.ssh.SshSession;
 
 /**
  * The upload pack factory creates an upload pack which controls what refs are
@@ -48,28 +47,13 @@ public class GitblitUploadPackFactory<X> implements UploadPackFactory<X> {
 	public UploadPack create(X req, Repository db)
 			throws ServiceNotEnabledException, ServiceNotAuthorizedException {
 
-		UserModel user = UserModel.ANONYMOUS;
 		int timeout = 0;
 
-		if (req instanceof HttpServletRequest) {
-			// http/https request may or may not be authenticated
-			HttpServletRequest client = (HttpServletRequest) req;
-			user = authenticationManager.authenticate(client);
-			if (user == null) {
-				user = UserModel.ANONYMOUS;
-			}
-		} else if (req instanceof GitDaemonClient) {
+		if (req instanceof GitDaemonClient) {
 			// git daemon request is always anonymous
 			GitDaemonClient client = (GitDaemonClient) req;
 			// set timeout from Git daemon
 			timeout = client.getDaemon().getTimeout();
-		} else if (req instanceof SshSession) {
-			// SSH request is always authenticated
-			SshSession client = (SshSession) req;
-			user = authenticationManager.authenticate(client);
-			if (user == null) {
-				throw new ServiceNotAuthorizedException();
-			}
 		}
 
 		UploadPack up = new UploadPack(db);
diff --git a/src/main/java/com/gitblit/git/RepositoryResolver.java b/src/main/java/com/gitblit/git/RepositoryResolver.java
index 08048195..ad5dcf01 100644
--- a/src/main/java/com/gitblit/git/RepositoryResolver.java
+++ b/src/main/java/com/gitblit/git/RepositoryResolver.java
@@ -104,11 +104,9 @@ public class RepositoryResolver<X> extends FileResolver<X> {
 				user = UserModel.ANONYMOUS;
 			}
 		} else if (req instanceof SshSession) {
+			// ssh is always authenticated
 			SshSession s = (SshSession) req;
-			user = gitblit.authenticate(s);
-			if (user == null) {
-				throw new IOException(String.format("User %s not found",  s.getRemoteUser()));
-			}
+			user = gitblit.getUserModel(s.getRemoteUser());
 		}
 
 		if (user.canClone(model)) {
diff --git a/src/main/java/com/gitblit/manager/AuthenticationManager.java b/src/main/java/com/gitblit/manager/AuthenticationManager.java
index 658c2890..10f8fd11 100644
--- a/src/main/java/com/gitblit/manager/AuthenticationManager.java
+++ b/src/main/java/com/gitblit/manager/AuthenticationManager.java
@@ -17,6 +17,7 @@ package com.gitblit.manager;
 
 import java.nio.charset.Charset;
 import java.security.Principal;
+import java.security.PublicKey;
 import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -47,7 +48,6 @@ import com.gitblit.auth.SalesforceAuthProvider;
 import com.gitblit.auth.WindowsAuthProvider;
 import com.gitblit.models.TeamModel;
 import com.gitblit.models.UserModel;
-import com.gitblit.transport.ssh.SshSession;
 import com.gitblit.utils.Base64;
 import com.gitblit.utils.HttpUtils;
 import com.gitblit.utils.StringUtils;
@@ -291,28 +291,31 @@ public class AuthenticationManager implements IAuthenticationManager {
 	}
 
 	/**
-	 * Authenticate a user based on SSH session.
+	 * Authenticate a user based on a public key.
 	 *
-	 * @param SshSession
+	 * This implementation assumes that the authentication has already take place
+	 * (e.g. SSHDaemon) and that this is a validation/verification of the user.
+	 *
+	 * @param username
+	 * @param key
 	 * @return a user object or null
 	 */
 	@Override
-	public UserModel authenticate(SshSession sshSession) {
-		String username = sshSession.getRemoteUser();
+	public UserModel authenticate(String username, PublicKey key) {
 		if (username != null) {
 			if (!StringUtils.isEmpty(username)) {
 				UserModel user = userManager.getUserModel(username);
 				if (user != null) {
 					// existing user
-					logger.debug(MessageFormat.format("{0} authenticated by SSH key from {1}",
-							user.username, sshSession.getRemoteAddress()));
-					return validateAuthentication(user, AuthenticationType.SSH);
+					logger.debug(MessageFormat.format("{0} authenticated by {1} public key",
+							user.username, key.getAlgorithm()));
+					return validateAuthentication(user, AuthenticationType.PUBLIC_KEY);
 				}
-				logger.warn(MessageFormat.format("Failed to find UserModel for {0}, attempted ssh authentication from {1}",
-							username, sshSession.getRemoteAddress()));
+				logger.warn(MessageFormat.format("Failed to find UserModel for {0} during public key authentication",
+							username));
 			}
 		} else {
-			logger.warn("Empty user in SSH session");
+			logger.warn("Empty user passed to AuthenticationManager.authenticate!");
 		}
 		return null;
 	}
diff --git a/src/main/java/com/gitblit/manager/GitblitManager.java b/src/main/java/com/gitblit/manager/GitblitManager.java
index a5a26379..97e8efc9 100644
--- a/src/main/java/com/gitblit/manager/GitblitManager.java
+++ b/src/main/java/com/gitblit/manager/GitblitManager.java
@@ -22,6 +22,7 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.lang.reflect.Type;
+import java.security.PublicKey;
 import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -68,7 +69,6 @@ import com.gitblit.models.SettingModel;
 import com.gitblit.models.TeamModel;
 import com.gitblit.models.UserModel;
 import com.gitblit.tickets.ITicketService;
-import com.gitblit.transport.ssh.SshSession;
 import com.gitblit.utils.ArrayUtils;
 import com.gitblit.utils.HttpUtils;
 import com.gitblit.utils.JsonUtils;
@@ -652,12 +652,12 @@ public class GitblitManager implements IGitblit {
 		}
 		return user;
 	}
-	
+
 	@Override
-	public UserModel authenticate(SshSession sshSession) {
-		return authenticationManager.authenticate(sshSession);
+	public UserModel authenticate(String username, PublicKey key) {
+		return authenticationManager.authenticate(username, key);
 	}
-	
+
 	@Override
 	public UserModel authenticate(HttpServletRequest httpRequest, boolean requiresCertificate) {
 		UserModel user = authenticationManager.authenticate(httpRequest, requiresCertificate);
diff --git a/src/main/java/com/gitblit/manager/IAuthenticationManager.java b/src/main/java/com/gitblit/manager/IAuthenticationManager.java
index 5d98d127..4f43d928 100644
--- a/src/main/java/com/gitblit/manager/IAuthenticationManager.java
+++ b/src/main/java/com/gitblit/manager/IAuthenticationManager.java
@@ -15,12 +15,13 @@
  */
 package com.gitblit.manager;
 
+import java.security.PublicKey;
+
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
 import com.gitblit.models.TeamModel;
 import com.gitblit.models.UserModel;
-import com.gitblit.transport.ssh.SshSession;
 
 public interface IAuthenticationManager extends IManager {
 
@@ -34,7 +35,14 @@ public interface IAuthenticationManager extends IManager {
 	 */
 	UserModel authenticate(HttpServletRequest httpRequest);
 
-	public UserModel authenticate(SshSession sshSession);
+	/**
+	 * Authenticate a user based on a public key.
+	 *
+	 * @param username
+	 * @param key
+	 * @return a user object or null
+	 */
+	UserModel authenticate(String username, PublicKey key);
 
 	/**
 	 * Authenticate a user based on HTTP request parameters.
diff --git a/src/main/java/com/gitblit/transport/ssh/SshKeyAuthenticator.java b/src/main/java/com/gitblit/transport/ssh/SshKeyAuthenticator.java
index f1bff4f5..044d2643 100644
--- a/src/main/java/com/gitblit/transport/ssh/SshKeyAuthenticator.java
+++ b/src/main/java/com/gitblit/transport/ssh/SshKeyAuthenticator.java
@@ -23,6 +23,8 @@ import java.util.concurrent.TimeUnit;
 
 import org.apache.sshd.server.PublickeyAuthenticator;
 import org.apache.sshd.server.session.ServerSession;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import com.gitblit.manager.IAuthenticationManager;
 import com.gitblit.models.UserModel;
@@ -38,6 +40,8 @@ import com.google.common.cache.LoadingCache;
  */
 public class SshKeyAuthenticator implements PublickeyAuthenticator {
 
+	protected final Logger log = LoggerFactory.getLogger(getClass());
+
 	protected final IKeyManager keyManager;
 	
 	protected final IAuthenticationManager authManager;
@@ -47,6 +51,7 @@ public class SshKeyAuthenticator implements PublickeyAuthenticator {
 			expireAfterAccess(15, TimeUnit.MINUTES).
 			maximumSize(100)
 			.build(new CacheLoader<String, List<PublicKey>>() {
+				@Override
 				public List<PublicKey> load(String username) {
 					return keyManager.getKeys(username);
 				}
@@ -60,43 +65,42 @@ public class SshKeyAuthenticator implements PublickeyAuthenticator {
 	@Override
 	public boolean authenticate(String username, final PublicKey suppliedKey,
 			final ServerSession session) {
-		final SshSession sd = session.getAttribute(SshSession.KEY);
+		final SshSession client = session.getAttribute(SshSession.KEY);
+
+		if (client.getRemoteUser() != null) {
+			// TODO why do we re-authenticate?
+			log.info("{} has already authenticated!", username);
+			return true;
+		}
 
 		username = username.toLowerCase(Locale.US);
 		try {
 			List<PublicKey> keys = sshKeyCache.get(username);
 			if (keys == null || keys.isEmpty()) {
-				sd.authenticationError(username, "no-matching-key");
+				log.info("{} has not added any public keys for ssh authentication", username);
 				return false;
 			}
+
 			for (PublicKey key : keys) {
 				if (key.equals(suppliedKey)) {
-					return validate(username, sd);
+					UserModel user = authManager.authenticate(username, key);
+					if (user != null) {
+						client.authenticationSuccess(username);
+						return true;
+					}
 				}
 			}
-			return false;
 		} catch (ExecutionException e) {
-			sd.authenticationError(username, "user-not-found");
-			return false;
 		}
-	}
 
-	boolean validate(String username, SshSession sd) {
-		// now that the key has been validated, check with the authentication
-		// manager to ensure that this user exists and can authenticate
-		sd.authenticationSuccess(username);
-		UserModel user = authManager.authenticate(sd);
-		if (user != null) {
-			return true;
-		}
-		sd.authenticationError(username, "user-not-found");
+		log.warn("could not authenticate {} for SSH using the supplied public key", username);
 		return false;
 	}
 
 	public IKeyManager getKeyManager() {
 		return keyManager;
 	}
-	
+
 	public Cache<String, List<PublicKey>> getKeyCache() {
 		return sshKeyCache;
 	}
diff --git a/src/main/java/com/gitblit/transport/ssh/SshPasswordAuthenticator.java b/src/main/java/com/gitblit/transport/ssh/SshPasswordAuthenticator.java
index ce01df76..3baf985d 100644
--- a/src/main/java/com/gitblit/transport/ssh/SshPasswordAuthenticator.java
+++ b/src/main/java/com/gitblit/transport/ssh/SshPasswordAuthenticator.java
@@ -19,6 +19,8 @@ import java.util.Locale;
 
 import org.apache.sshd.server.PasswordAuthenticator;
 import org.apache.sshd.server.session.ServerSession;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import com.gitblit.manager.IAuthenticationManager;
 import com.gitblit.models.UserModel;
@@ -30,6 +32,8 @@ import com.gitblit.models.UserModel;
  */
 public class SshPasswordAuthenticator implements PasswordAuthenticator {
 
+	protected final Logger log = LoggerFactory.getLogger(getClass());
+
 	protected final IAuthenticationManager authManager;
 
 	public SshPasswordAuthenticator(IAuthenticationManager authManager) {
@@ -38,13 +42,20 @@ public class SshPasswordAuthenticator implements PasswordAuthenticator {
 
 	@Override
 	public boolean authenticate(String username, String password, ServerSession session) {
+		SshSession client = session.getAttribute(SshSession.KEY);
+		if (client.getRemoteUser() != null) {
+			log.info("{} has already authenticated!", username);
+			return true;
+		}
+
 		username = username.toLowerCase(Locale.US);
 		UserModel user = authManager.authenticate(username, password.toCharArray());
 		if (user != null) {
-			SshSession sd = session.getAttribute(SshSession.KEY);
-			sd.authenticationSuccess(username);
+			client.authenticationSuccess(username);
 			return true;
 		}
+
+		log.warn("could not authenticate {} for SSH using the supplied password", username);
 		return false;
 	}
 }