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>
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 f27af6e..651ae7d 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 @@
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 @@
"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 @@
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 4f85ebe..b89bc60 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 @@
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 bf89174..0d6f302 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 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.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 @@
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 @@
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 f67170e..22966f9 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 @@
// @formatter:off
/***/ public String authenticationCanceled;
+ /***/ public String authenticationOnClosedSession;
/***/ public String closeListenerFailed;
/***/ public String configInvalidPath;
/***/ public String configInvalidPattern;
@@ -87,6 +88,7 @@
/***/ public String serverIdTooLong;
/***/ public String serverIdWithNul;
/***/ public String sessionCloseFailed;
+ /***/ public String sessionWithoutUsername;
/***/ public String sshClosingDown;
/***/ public String sshCommandTimeout;
/***/ public String sshProcessStillRunning;
