Revise SSH public key integration with AuthenticationManager

diff --git a/src/main/java/com/gitblit/Constants.java b/src/main/java/com/gitblit/Constants.java
index 889e5a30ad838adb4307ceeb52c4eced18e35203..56dfec064561ff23f765571451b7c04b2d07da34 100644 (file)
--- a/src/main/java/com/gitblit/Constants.java
+++ b/src/main/java/com/gitblit/Constants.java
@@ -501,7 +501,7 @@ public class Constants {
        }\r
 \r
        public static enum AuthenticationType {\r
-               SSH, CREDENTIALS, COOKIE, CERTIFICATE, CONTAINER;\r
+               PUBLIC_KEY, CREDENTIALS, COOKIE, CERTIFICATE, CONTAINER;\r
 \r
                public boolean isStandard() {\r
                        return ordinal() <= COOKIE.ordinal();\r

diff --git a/src/main/java/com/gitblit/git/GitblitUploadPackFactory.java b/src/main/java/com/gitblit/git/GitblitUploadPackFactory.java
index a72d4ad95b49681181cd4aad74e140a5c920e639..7a476775fbd2a445730d7244354a32b8d16ac2e6 100644 (file)
--- 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 080481952305940d85e99496ef0e143e7f05ee7d..ad5dcf010c65c103164cc21009f170a87ee27d48 100644 (file)
--- 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 658c2890a325b6959f291b6d5108666e775690c1..10f8fd11377a9bf42b97494ea28660d6ad3d8102 100644 (file)
--- 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 a5a263797242c1d1a6d7462d50b0ce81fb07d7f2..97e8efc9eef05efc4ea95ab5ec180e8fa3719abe 100644 (file)
--- 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 5d98d1275f4774e6a06498b241b719a0fd5da61a..4f43d928fcf212278a22773d361023f1e760f622 100644 (file)
--- 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 f1bff4f584092670d5fc4b1a92bceed1411fb0f3..044d264385b8aafc57c0872c2c039c876d52323d 100644 (file)
--- 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 ce01df76543a1d1cd9ff854d1b352265c4542f66..3baf985d6ec3d4e90e2311934e11bfb2e01a9973 100644 (file)
--- 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;
        }
 }