From 44e2ee1d05a9d455ae60dd64058b31f006d551b7 Mon Sep 17 00:00:00 2001 From: James Moger Date: Fri, 14 Mar 2014 12:10:25 -0400 Subject: [PATCH] Revise SSH public key integration with AuthenticationManager --- src/main/java/com/gitblit/Constants.java | 2 +- .../gitblit/git/GitblitUploadPackFactory.java | 18 +-------- .../com/gitblit/git/RepositoryResolver.java | 6 +-- .../manager/AuthenticationManager.java | 25 ++++++------ .../com/gitblit/manager/GitblitManager.java | 10 ++--- .../manager/IAuthenticationManager.java | 12 +++++- .../transport/ssh/SshKeyAuthenticator.java | 38 ++++++++++--------- .../ssh/SshPasswordAuthenticator.java | 15 +++++++- 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 implements UploadPackFactory { 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 extends FileResolver { 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>() { + @Override public List 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 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> 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; } } -- 2.39.5