From 44e2ee1d05a9d455ae60dd64058b31f006d551b7 Mon Sep 17 00:00:00 2001
From: James Moger <james.moger@gitblit.com>
Date: Thu, 10 Apr 2014 18:58:08 -0400
Subject: [PATCH] Revise SSH public key integration with AuthenticationManager
---
src/main/java/com/gitblit/transport/ssh/SshPasswordAuthenticator.java | 15 ++++
src/main/java/com/gitblit/manager/IAuthenticationManager.java | 12 +++
src/main/java/com/gitblit/Constants.java | 2
src/main/java/com/gitblit/manager/GitblitManager.java | 10 +-
src/main/java/com/gitblit/manager/AuthenticationManager.java | 25 ++++---
src/main/java/com/gitblit/git/GitblitUploadPackFactory.java | 18 -----
src/main/java/com/gitblit/transport/ssh/SshKeyAuthenticator.java | 38 +++++++-----
src/main/java/com/gitblit/git/RepositoryResolver.java | 6 -
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 889e5a3..56dfec0 100644
--- a/src/main/java/com/gitblit/Constants.java
+++ b/src/main/java/com/gitblit/Constants.java
@@ -501,7 +501,7 @@
}
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 a72d4ad..7a47677 100644
--- a/src/main/java/com/gitblit/git/GitblitUploadPackFactory.java
+++ b/src/main/java/com/gitblit/git/GitblitUploadPackFactory.java
@@ -26,7 +26,6 @@
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 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 0804819..ad5dcf0 100644
--- a/src/main/java/com/gitblit/git/RepositoryResolver.java
+++ b/src/main/java/com/gitblit/git/RepositoryResolver.java
@@ -104,11 +104,9 @@
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 658c289..10f8fd1 100644
--- a/src/main/java/com/gitblit/manager/AuthenticationManager.java
+++ b/src/main/java/com/gitblit/manager/AuthenticationManager.java
@@ -17,6 +17,7 @@
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.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 @@
}
/**
- * 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 a5a2637..97e8efc 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.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.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 @@
}
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 5d98d12..4f43d92 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 @@
*/
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 f1bff4f..044d264 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 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 @@
*/
public class SshKeyAuthenticator implements PublickeyAuthenticator {
+ protected final Logger log = LoggerFactory.getLogger(getClass());
+
protected final IKeyManager keyManager;
protected final IAuthenticationManager authManager;
@@ -47,6 +51,7 @@
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 @@
@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 ce01df7..3baf985 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 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 @@
*/
public class SshPasswordAuthenticator implements PasswordAuthenticator {
+ protected final Logger log = LoggerFactory.getLogger(getClass());
+
protected final IAuthenticationManager authManager;
public SshPasswordAuthenticator(IAuthenticationManager authManager) {
@@ -38,13 +42,20 @@
@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;
}
}
--
Gitblit v1.9.1