[jira] [Commented] (SSHD-1050) Race condition between early exceptions and AuthFuture

["Lyor Goldstein (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/SSHD-1050?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17170588#comment-17170588 ] 

Lyor Goldstein commented on SSHD-1050:
--------------------------------------

{quote}
Perhaps the connect future should be fulfilled only once the initial KEX is done? 
{quote}
It does not seem like a good idea - KEX occurs +after+ user has connected and provided the credentials - thus the need for auth future.
{code:java}
session = client.connect(...).verify(timeout);
session.addPassword/PublicKey(...);
session.auth().verify()
{code}
If we wait to "release" the connect future until after KEX then how would the user provide the credentials ? We would need to change the connect API in some way since we need way for the user to provide the credentials after connect but before KEX. Such a change while possible would cause a bit of a revolution and require some extensive code re-factoring.

> 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
>            Priority: Major
>
> 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: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org