Prevent double authentication for the same public key

Openssh client sends two requests, one without a key signature to verify
that the public key is acceptable and the second one with the signature
after having loaded the private key and signed some data for actual
verification.

To prevent that the PublickeyAuthenticator#authenticate is called twice
cache the authentication status for session and public key.  Implement
SessionListener to clean up the cache entry when session is destroyed.

This is a workaround for SSHD bug [1].

Inspired-By: Guillaume Nodet <gnodet@apache.org>
[1] https://issues.apache.org/jira/browse/SSHD-300

diff --git a/src/main/java/com/gitblit/transport/ssh/CachingPublicKeyAuthenticator.java b/src/main/java/com/gitblit/transport/ssh/CachingPublicKeyAuthenticator.java
new file mode 100644 (file)
index 0000000..ee1de59
--- /dev/null
+++ b/src/main/java/com/gitblit/transport/ssh/CachingPublicKeyAuthenticator.java
@@ -0,0 +1,117 @@
+/*
+ * Copyright 2014 gitblit.com.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.gitblit.transport.ssh;
+
+import java.security.PublicKey;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.apache.sshd.common.Session;
+import org.apache.sshd.common.SessionListener;
+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;
+import com.google.common.base.Preconditions;
+
+/**
+ * 
+ * @author Eric Myrhe
+ * 
+ */
+public class CachingPublicKeyAuthenticator implements PublickeyAuthenticator,
+               SessionListener {
+
+       protected final Logger log = LoggerFactory.getLogger(getClass());
+
+       protected final IKeyManager keyManager;
+
+       protected final IAuthenticationManager authManager;
+
+       private final Map<ServerSession, Map<PublicKey, Boolean>> cache =
+                       new ConcurrentHashMap<ServerSession, Map<PublicKey, Boolean>>();
+
+       public CachingPublicKeyAuthenticator(IKeyManager keyManager,
+                       IAuthenticationManager authManager) {
+               this.keyManager = keyManager;
+               this.authManager = authManager;
+       }
+
+       @Override
+       public boolean authenticate(String username, PublicKey key,
+                       ServerSession session) {
+               Map<PublicKey, Boolean> map = cache.get(session);
+               if (map == null) {
+                       map = new HashMap<PublicKey, Boolean>();
+                       cache.put(session, map);
+                       session.addListener(this);
+               }
+               if (map.containsKey(key)) {
+                       return map.get(key);
+               }
+               boolean result = doAuthenticate(username, key, session);
+               map.put(key, result);
+               return result;
+       }
+
+       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<PublicKey> keys = keyManager.getKeys(username);
+               if (keys == null || keys.isEmpty()) {
+                       log.info("{} has not added any public keys for ssh authentication",
+                                       username);
+                       return false;
+               }
+
+               for (PublicKey key : keys) {
+                       if (key.equals(suppliedKey)) {
+                               UserModel user = authManager.authenticate(username, key);
+                               if (user != null) {
+                                       client.setUser(user);
+                                       return true;
+                               }
+                       }
+               }
+
+               log.warn(
+                               "could not authenticate {} for SSH using the supplied public key",
+                               username);
+               return false;
+       }
+
+       public IKeyManager getKeyManager() {
+               return keyManager;
+       }
+
+       public void sessionCreated(Session session) {
+       }
+
+       public void sessionEvent(Session sesssion, Event event) {
+       }
+
+       public void sessionClosed(Session session) {
+               cache.remove(session);
+       }
+}

diff --git a/src/main/java/com/gitblit/transport/ssh/PublicKeyAuthenticator.java b/src/main/java/com/gitblit/transport/ssh/PublicKeyAuthenticator.java
deleted file mode 100644 (file)
index 84e7afa..0000000
--- a/src/main/java/com/gitblit/transport/ssh/PublicKeyAuthenticator.java
+++ /dev/null
@@ -1,83 +0,0 @@
-/*
- * Copyright 2014 gitblit.com.
- *
- * Licensed under the Apache License, Version 2.0 (the "License"); you may not
- * use this file except in compliance with the License. You may obtain a copy of
- * the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
- * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
- * License for the specific language governing permissions and limitations under
- * the License.
- */
-package com.gitblit.transport.ssh;
-
-import java.security.PublicKey;
-import java.util.List;
-import java.util.Locale;
-
-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;
-
-/**
- *
- * @author Eric Myrhe
- *
- */
-public class PublicKeyAuthenticator implements PublickeyAuthenticator {
-
-       protected final Logger log = LoggerFactory.getLogger(getClass());
-
-       protected final IKeyManager keyManager;
-
-       protected final IAuthenticationManager authManager;
-
-       public PublicKeyAuthenticator(IKeyManager keyManager, IAuthenticationManager authManager) {
-               this.keyManager = keyManager;
-               this.authManager = authManager;
-       }
-
-       @Override
-       public boolean authenticate(String username, final PublicKey suppliedKey,
-                       final ServerSession session) {
-               final SshDaemonClient client = session.getAttribute(SshDaemonClient.KEY);
-
-               if (client.getUser() != null) {
-                       // TODO why do we re-authenticate?
-                       log.info("{} has already authenticated!", username);
-                       return true;
-               }
-
-               username = username.toLowerCase(Locale.US);
-               List<PublicKey> keys = keyManager.getKeys(username);
-               if (keys == null || keys.isEmpty()) {
-                       log.info("{} has not added any public keys for ssh authentication", username);
-                       return false;
-               }
-
-               for (PublicKey key : keys) {
-                       if (key.equals(suppliedKey)) {
-                               UserModel user = authManager.authenticate(username, key);
-                               if (user != null) {
-                                       client.setUser(user);
-                                       return true;
-                               }
-                       }
-               }
-
-               log.warn("could not authenticate {} for SSH using the supplied public key", username);
-               return false;
-       }
-
-       public IKeyManager getKeyManager() {
-               return keyManager;
-       }
-}

diff --git a/src/main/java/com/gitblit/transport/ssh/SshCommandFactory.java b/src/main/java/com/gitblit/transport/ssh/SshCommandFactory.java
index da57f76e48d1f912459060b7c1cb2848ee335a54..48e8869af63f335a9d532048ea30d6b9cfe42d27 100644 (file)
--- a/src/main/java/com/gitblit/transport/ssh/SshCommandFactory.java
+++ b/src/main/java/com/gitblit/transport/ssh/SshCommandFactory.java
@@ -61,10 +61,12 @@ public class SshCommandFactory implements CommandFactory {
        private static final Logger logger = LoggerFactory.getLogger(SshCommandFactory.class);
 
        private final IGitblit gitblit;
-       private final PublicKeyAuthenticator keyAuthenticator;
+       private final CachingPublicKeyAuthenticator keyAuthenticator;
        private final ScheduledExecutorService startExecutor;
 
-       public SshCommandFactory(IGitblit gitblit, PublicKeyAuthenticator keyAuthenticator, IdGenerator idGenerator) {
+       public SshCommandFactory(IGitblit gitblit,
+                       CachingPublicKeyAuthenticator keyAuthenticator,
+                       IdGenerator idGenerator) {
                this.gitblit = gitblit;
                this.keyAuthenticator = keyAuthenticator;
 
@@ -252,6 +254,7 @@ public class SshCommandFactory implements CommandFactory {
                        }
                }
 
+               @SuppressWarnings("unused")
                private void onDestroy() {
                        synchronized (this) {
                                if (cmd != null) {

diff --git a/src/main/java/com/gitblit/transport/ssh/SshDaemon.java b/src/main/java/com/gitblit/transport/ssh/SshDaemon.java
index c3d486007ca4896eab52e702b3f93d11a2f21a1d..c954b347c9430231d7b7c3322b048dea9675347b 100644 (file)
--- a/src/main/java/com/gitblit/transport/ssh/SshDaemon.java
+++ b/src/main/java/com/gitblit/transport/ssh/SshDaemon.java
@@ -104,7 +104,8 @@ public class SshDaemon {
                        addr = new InetSocketAddress(bindInterface, port);
                }
 
-               PublicKeyAuthenticator keyAuthenticator = new PublicKeyAuthenticator(keyManager, gitblit);
+               CachingPublicKeyAuthenticator keyAuthenticator = 
+                               new CachingPublicKeyAuthenticator(keyManager, gitblit);
 
                sshd = SshServer.setUpDefaultServer();
                sshd.setPort(addr.getPort());
@@ -176,6 +177,7 @@ public class SshDaemon {
                }
        }
 
+       @SuppressWarnings("unchecked")
        protected IKeyManager getKeyManager() {
                IKeyManager keyManager = null;
                IStoredSettings settings = gitblit.getSettings();

diff --git a/src/main/java/com/gitblit/transport/ssh/commands/BaseKeyCommand.java b/src/main/java/com/gitblit/transport/ssh/commands/BaseKeyCommand.java
index 36475244541f2d18ab2d23ff0e5c28ad805a477d..f92ea6f92353193a2c62d4f56ae1bef38c35435e 100644 (file)
--- a/src/main/java/com/gitblit/transport/ssh/commands/BaseKeyCommand.java
+++ b/src/main/java/com/gitblit/transport/ssh/commands/BaseKeyCommand.java
@@ -21,7 +21,7 @@ import java.io.InputStreamReader;
 import java.io.UnsupportedEncodingException;
 import java.util.List;
 
-import com.gitblit.transport.ssh.PublicKeyAuthenticator;
+import com.gitblit.transport.ssh.CachingPublicKeyAuthenticator;
 import com.google.common.base.Charsets;
 
 /**
@@ -51,8 +51,8 @@ public abstract class BaseKeyCommand extends SshCommand {
                return sshKeys;
        }
 
-       protected PublicKeyAuthenticator authenticator;
-       public void setAuthenticator(PublicKeyAuthenticator authenticator) {
+       protected CachingPublicKeyAuthenticator authenticator;
+       public void setAuthenticator(CachingPublicKeyAuthenticator authenticator) {
                this.authenticator = authenticator;
        }
 }

diff --git a/src/main/java/com/gitblit/transport/ssh/commands/DispatchCommand.java b/src/main/java/com/gitblit/transport/ssh/commands/DispatchCommand.java
index 1e43e2f2b0b069f343434009369762b5178e8dfd..3c041af6bfa0740c804eb19e8ced34eaa10fe2d5 100644 (file)
--- a/src/main/java/com/gitblit/transport/ssh/commands/DispatchCommand.java
+++ b/src/main/java/com/gitblit/transport/ssh/commands/DispatchCommand.java
@@ -34,7 +34,7 @@ import com.gitblit.git.GitblitUploadPackFactory;
 import com.gitblit.git.RepositoryResolver;
 import com.gitblit.models.UserModel;
 import com.gitblit.transport.ssh.CommandMetaData;
-import com.gitblit.transport.ssh.PublicKeyAuthenticator;
+import com.gitblit.transport.ssh.CachingPublicKeyAuthenticator;
 import com.gitblit.transport.ssh.SshDaemonClient;
 import com.gitblit.utils.cli.SubcommandHandler;
 import com.google.common.base.Charsets;
@@ -237,9 +237,9 @@ public class DispatchCommand extends BaseCommand {
                this.gitblitReceivePackFactory = gitblitReceivePackFactory;
        }
 
-       private PublicKeyAuthenticator authenticator;
+       private CachingPublicKeyAuthenticator authenticator;
 
-       public void setAuthenticator(PublicKeyAuthenticator authenticator) {
+       public void setAuthenticator(CachingPublicKeyAuthenticator authenticator) {
                this.authenticator = authenticator;
        }
 }