[
https://issues.apache.org/jira/browse/SSHD-1050?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17172744#comment-17172744
]
Guillaume Nodet commented on SSHD-1050:
---------------------------------------
I haven't checked the results as I'm in holidays, but wouldn't it be easier to
have a patch like the following instead of using multiple fields ?
{code}
diff --git
a/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
b/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
index 21e0631d..4037f23a 100644
---
a/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
+++
b/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
@@ -86,7 +86,6 @@ public class ClientSessionImpl extends AbstractClientSession {
}
authFuture = new DefaultAuthFuture(ioSession.getRemoteAddress(),
futureLock);
- authFuture.setAuthed(false);
signalSessionCreated(ioSession);
@@ -124,9 +123,17 @@ public class ClientSessionImpl extends
AbstractClientSession {
ClientUserAuthService authService = getUserAuthService();
synchronized (sessionLock) {
String serviceName = nextServiceName();
- authFuture = ValidateUtils.checkNotNull(
+ AuthFuture future = ValidateUtils.checkNotNull(
authService.auth(serviceName), "No auth future generated
by service=%s", serviceName);
- return authFuture;
+ future.addListener(f -> {
+ Throwable e = f.getException();
+ if (e != null) {
+ this.authFuture.setException(e);
+ } else {
+ this.authFuture.setAuthed(f.isSuccess());
+ }
+ });
+ return this.authFuture;
}
}
{code}
> Race condition between early exceptions and AuthFuture
> ------------------------------------------------------
>
> Key: SSHD-1050
> URL: https://issues.apache.org/jira/browse/SSHD-1050
> Project: MINA SSHD
> Issue Type: Bug
> Affects Versions: 2.4.0, 2.6.0
> Reporter: Thomas Wolf
> Assignee: Lyor Goldstein
> Priority: Major
> Time Spent: 2h
> Remaining Estimate: 0h
>
> It appears that sometimes exceptions that occur early in connection setup are
> not reported on the AuthFuture. When that happens, AuthFuture.verify(timeout)
> will spend the whole timeout waiting and then report a timeout. The earlier
> exception is lost and nowhere to be seen.
> I stumbled over this when analyzing [Eclipse bug
> 565394|https://bugs.eclipse.org/bugs/show_bug.cgi?id=565394].
> It's not easy to reproduce, but with the below client test I can manage to
> make the test fail from time to time if run repeatedly. (That test uses a
> large preamble before the server identification to provoke an early
> exception. Using publickey auth instead of password auth increases the
> chances to get a failure. Running in a debugger increases the chances even
> more. It's clearly timing-related.)
> {code:java}
> private String longPreamble() {
> StringBuilder b = new StringBuilder();
> for (int i = 0; i < 250; i++) {
> b.append('a');
> }
> String line = b.toString();
> b = new StringBuilder(line);
> int limit = CoreModuleProperties.MAX_IDENTIFICATION_SIZE.get(sshd)
> .orElse(Integer.valueOf(16 * 1024)).intValue();
> limit = limit / 250 + 1;
> for (int i = 0; i < limit; i++) {
> b.append(CoreModuleProperties.SERVER_EXTRA_IDENT_LINES_SEPARATOR)
> .append(line);
> }
> return b.toString();
> }
> @Test
> public void testAuthGetsNotifiedOnLongPreamble() throws Exception {
> CoreModuleProperties.SERVER_EXTRA_IDENTIFICATION_LINES.set(sshd,
> longPreamble());
> sshd.setPasswordAuthenticator(RejectAllPasswordAuthenticator.INSTANCE);
>
> sshd.setKeyboardInteractiveAuthenticator(KeyboardInteractiveAuthenticator.NONE);
> client.setUserAuthFactories(
> Collections.singletonList(UserAuthPublicKeyFactory.INSTANCE));
> client.start();
> try (ClientSession session =
> client.connect(getCurrentTestName(), TEST_LOCALHOST, port)
> .verify(CONNECT_TIMEOUT).getSession()) {
> KeyPairProvider keys = createTestHostKeyProvider();
> session.addPublicKeyIdentity(
> keys.loadKey(session,
> CommonTestSupportUtils.DEFAULT_TEST_HOST_KEY_TYPE));
> // This auth should fail because the server sends too many lines before
> // the server identification. However, the auth must not time out!
> There's
> // an exception raised early on when the identification is read, but
> // frequently this exception gets not reported on the auth future that
> // we are waiting on here, and then we wait for the whole timeout.
> //
> // There's a race condition somewhere. The test succeeds frequently, and
> is
> // hard to make fail, but if run enough times it will usually fail.
> Running
> // this in a debugger increases the chances of it failing.
> Throwable e = assertThrows(Throwable.class,
> () -> session.auth().verify(AUTH_TIMEOUT));
> assertFalse(e.getMessage().contains("timeout"));
> assertFalse(session.isAuthenticated());
> } finally {
> client.stop();
> }
> }
> {code}
> Perhaps the connect future should be fulfilled only once the initial KEX is
> done? OpenSSH's ConnectTimeout does include the time until after the initial
> KEX.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
