sshd: work around a race condition in Apache MINA sshd 2.4.0/2.5.x

When exceptions occur very early in the SSH connection setup, it's
possible that an exception gets lost. A subsequent authentication
attempt may then never be notified of the failure, and then wait
indefinitely or until its timeout expires.

This is caused by race conditions in sshd. The issue has been reported
upstream as SSHD-1050,[1] but will be fixed at the earliest in sshd
2.6.0.

[1] https://issues.apache.org/jira/projects/SSHD/issues/SSHD-1050

Bug: 565394
Change-Id: If9b62839db38f9e59a5e1137c2257039ba82de98
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

4 files changed, 149 insertions, 6 deletions

diff --git a/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java b/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java
index f27af6e196..651ae7dec1 100644
--- a/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java
+++ b/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java
@@ -11,6 +11,7 @@ package org.eclipse.jgit.transport.sshd;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 
 import java.io.File;
@@ -159,7 +160,7 @@ public class ApacheSshTest extends SshTestBase {
 				"IdentityFile " + privateKey1.getAbsolutePath());
 	}
 
-	@Test (expected = TransportException.class)
+	@Test
 	public void testHugePreamble() throws Exception {
 		// Test that the connection fails when the preamble is longer than 64k.
 		StringBuilder b = new StringBuilder();
@@ -172,11 +173,16 @@ public class ApacheSshTest extends SshTestBase {
 			lines[i] = line;
 		}
 		server.setPreamble(lines);
-		cloneWith(
-				"ssh://" + TEST_USER + "@localhost:" + testPort
-						+ "/doesntmatter",
-				defaultCloneDir, null,
-				"IdentityFile " + privateKey1.getAbsolutePath());
+		TransportException e = assertThrows(TransportException.class,
+				() -> cloneWith(
+						"ssh://" + TEST_USER + "@localhost:" + testPort
+								+ "/doesntmatter",
+						defaultCloneDir, null,
+						"IdentityFile " + privateKey1.getAbsolutePath()));
+		// The assertions test that we don't run into bug 565394 / SSHD-1050
+		assertFalse(e.getMessage().contains("timeout"));
+		assertTrue(e.getMessage().contains("65536")
+				|| e.getMessage().contains("closed"));
 	}
 
 	/**
diff --git a/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties b/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties
index 4f85ebe100..b89bc606a7 100644
--- a/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties
+++ b/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties
@@ -1,4 +1,5 @@
 authenticationCanceled=Authentication canceled: no password
+authenticationOnClosedSession=Authentication canceled: session is already closing or closed
 closeListenerFailed=Ssh session close listener failed
 configInvalidPath=Invalid path in ssh config key {0}: {1}
 configInvalidPattern=Invalid pattern in ssh config key {0}: {1}
@@ -75,6 +76,7 @@ serverIdNotReceived=No server identification received within {0} bytes
 serverIdTooLong=Server identification is longer than 255 characters (including line ending): {0}
 serverIdWithNul=Server identification contains a NUL character: {0}
 sessionCloseFailed=Closing the session failed
+sessionWithoutUsername=SSH session created without user name; cannot authenticate
 sshClosingDown=Apache MINA sshd session factory is closing down; cannot create new ssh sessions on this factory
 sshCommandTimeout={0} timed out after {1} seconds while opening the channel
 sshProcessStillRunning={0} is not yet completed, cannot get exit code
diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java
index bf891742b7..0d6f3027f2 100644
--- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java
+++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java
@@ -29,8 +29,10 @@ import java.util.Set;
 
 import org.apache.sshd.client.ClientFactoryManager;
 import org.apache.sshd.client.config.hosts.HostConfigEntry;
+import org.apache.sshd.client.future.AuthFuture;
 import org.apache.sshd.client.keyverifier.ServerKeyVerifier;
 import org.apache.sshd.client.session.ClientSessionImpl;
+import org.apache.sshd.client.session.ClientUserAuthService;
 import org.apache.sshd.common.AttributeRepository;
 import org.apache.sshd.common.FactoryManager;
 import org.apache.sshd.common.PropertyResolver;
@@ -39,6 +41,7 @@ import org.apache.sshd.common.SshException;
 import org.apache.sshd.common.config.keys.KeyUtils;
 import org.apache.sshd.common.io.IoSession;
 import org.apache.sshd.common.io.IoWriteFuture;
+import org.apache.sshd.common.kex.KexState;
 import org.apache.sshd.common.util.Readable;
 import org.apache.sshd.common.util.buffer.Buffer;
 import org.eclipse.jgit.errors.InvalidPatternException;
@@ -74,6 +77,17 @@ public class JGitClientSession extends ClientSessionImpl {
 	private volatile StatefulProxyConnector proxyHandler;
 
 	/**
+	 * Work-around for bug 565394 / SSHD-1050; remove when using sshd 2.6.0.
+	 */
+	private volatile AuthFuture authFuture;
+
+	/** Records exceptions before there is an authFuture. */
+	private List<Throwable> earlyErrors = new ArrayList<>();
+
+	/** Guards setting an earlyError and the authFuture together. */
+	private final Object errorLock = new Object();
+
+	/**
 	 * @param manager
 	 * @param session
 	 * @throws Exception
@@ -83,6 +97,125 @@ public class JGitClientSession extends ClientSessionImpl {
 		super(manager, session);
 	}
 
+	// BEGIN Work-around for bug 565394 / SSHD-1050
+	// Remove when using sshd 2.6.0.
+
+	@Override
+	public AuthFuture auth() throws IOException {
+		if (getUsername() == null) {
+			throw new IllegalStateException(
+					SshdText.get().sessionWithoutUsername);
+		}
+		ClientUserAuthService authService = getUserAuthService();
+		String serviceName = nextServiceName();
+		List<Throwable> errors = null;
+		AuthFuture future;
+		// Guard both getting early errors and setting authFuture
+		synchronized (errorLock) {
+			future = authService.auth(serviceName);
+			if (future == null) {
+				// Internal error; no translation.
+				throw new IllegalStateException(
+						"No auth future generated by service '" //$NON-NLS-1$
+								+ serviceName + '\'');
+			}
+			errors = earlyErrors;
+			earlyErrors = null;
+			authFuture = future;
+		}
+		if (errors != null && !errors.isEmpty()) {
+			Iterator<Throwable> iter = errors.iterator();
+			Throwable first = iter.next();
+			iter.forEachRemaining(t -> {
+				if (t != first && t != null) {
+					first.addSuppressed(t);
+				}
+			});
+			// Mark the future as having had an exception; just to be on the
+			// safe side. Actually, there shouldn't be anyone waiting on this
+			// future yet.
+			future.setException(first);
+			if (log.isDebugEnabled()) {
+				log.debug("auth({}) early exception type={}: {}", //$NON-NLS-1$
+						this, first.getClass().getSimpleName(),
+						first.getMessage());
+			}
+			if (first instanceof SshException) {
+				throw new SshException(
+						((SshException) first).getDisconnectCode(),
+						first.getMessage(), first);
+			}
+			throw new IOException(first.getMessage(), first);
+		}
+		return future;
+	}
+
+	@Override
+	protected void signalAuthFailure(AuthFuture future, Throwable t) {
+		signalAuthFailure(t);
+	}
+
+	private void signalAuthFailure(Throwable t) {
+		AuthFuture future = authFuture;
+		if (future == null) {
+			synchronized (errorLock) {
+				if (earlyErrors != null) {
+					earlyErrors.add(t);
+				}
+				future = authFuture;
+			}
+		}
+		if (future != null) {
+			future.setException(t);
+		}
+		if (log.isDebugEnabled()) {
+			boolean signalled = future != null && t == future.getException();
+			log.debug("signalAuthFailure({}) type={}, signalled={}: {}", this, //$NON-NLS-1$
+					t.getClass().getSimpleName(), Boolean.valueOf(signalled),
+					t.getMessage());
+		}
+	}
+
+	@Override
+	public void exceptionCaught(Throwable t) {
+		signalAuthFailure(t);
+		super.exceptionCaught(t);
+	}
+
+	@Override
+	protected void preClose() {
+		signalAuthFailure(
+				new SshException(SshdText.get().authenticationOnClosedSession));
+		super.preClose();
+	}
+
+	@Override
+	protected void handleDisconnect(int code, String msg, String lang,
+			Buffer buffer) throws Exception {
+		signalAuthFailure(new SshException(code, msg));
+		super.handleDisconnect(code, msg, lang, buffer);
+	}
+
+	@Override
+	protected <C extends Collection<ClientSessionEvent>> C updateCurrentSessionState(
+			C newState) {
+		if (closeFuture.isClosed()) {
+			newState.add(ClientSessionEvent.CLOSED);
+		}
+		if (isAuthenticated()) { // authFuture.isSuccess()
+			newState.add(ClientSessionEvent.AUTHED);
+		}
+		if (KexState.DONE.equals(getKexState())) {
+			AuthFuture future = authFuture;
+			if (future == null || future.isFailure()) {
+				newState.add(ClientSessionEvent.WAIT_AUTH);
+			}
+		}
+		return newState;
+	}
+
+	// END Work-around for bug 565394 / SSHD-1050
+
 	/**
 	 * Retrieves the {@link HostConfigEntry} this session was created for.
 	 *
diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java
index f67170e407..22966f956e 100644
--- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java
+++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java
@@ -19,6 +19,7 @@ public final class SshdText extends TranslationBundle {
 
 	// @formatter:off
 	/***/ public String authenticationCanceled;
+	/***/ public String authenticationOnClosedSession;
 	/***/ public String closeListenerFailed;
 	/***/ public String configInvalidPath;
 	/***/ public String configInvalidPattern;
@@ -87,6 +88,7 @@ public final class SshdText extends TranslationBundle {
 	/***/ public String serverIdTooLong;
 	/***/ public String serverIdWithNul;
 	/***/ public String sessionCloseFailed;
+	/***/ public String sessionWithoutUsername;
 	/***/ public String sshClosingDown;
 	/***/ public String sshCommandTimeout;
 	/***/ public String sshProcessStillRunning;