From cc9975ff68158a602fde8fb1c396e164081262ab Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Mon, 3 Aug 2020 16:22:37 +0200 Subject: 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 --- .../internal/transport/sshd/SshdText.properties | 2 + .../internal/transport/sshd/JGitClientSession.java | 133 +++++++++++++++++++++ .../jgit/internal/transport/sshd/SshdText.java | 2 + 3 files changed, 137 insertions(+) (limited to 'org.eclipse.jgit.ssh.apache') 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; @@ -73,6 +76,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 earlyErrors = new ArrayList<>(); + + /** Guards setting an earlyError and the authFuture together. */ + private final Object errorLock = new Object(); + /** * @param manager * @param session @@ -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 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 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 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; -- cgit v1.2.3