Prepare release notes for 0.35.0

* Support SSHClient.authPassword on FreeBSD

FreeBSD "keyboard-interactive" prompt is "Password for user@host:"

* Add test for PasswordResponseProvider

.gitattributes

@@ -1 +1,2 @@
 *.bat text eol=crlf
+src/itest/docker-image/** eol=lf

README.adoc

@@ -104,6 +104,11 @@ Issue tracker: https://github.com/hierynomus/sshj/issues
 Fork away!
 
 == Release history
+SSHJ 0.35.0 (2023-01-30)::
+* Merged https://github.com/hierynomus/sshj/pull/835[#835]: TimeoutException message improved
+* Merged https://github.com/hierynomus/sshj/pull/815[#815]: Support authPassword on FreeBSD
+* Merged https://github.com/hierynomus/sshj/pull/813[#813]: Prevent `CHANNEL_CLOSE` between isOpen and write call.
+* Merged https://github.com/hierynomus/sshj/pull/811[#811]: Add `Transport.isKeyExchangeREquired` to prevent unnecessary KEXINIT
 SSHJ 0.34.0 (2022-08-10)::
 * Merged https://github.com/hierynomus/sshj/pull/743[#743]: Use default client credentials for AuthGssApiWithMic
 * Merged https://github.com/hierynomus/sshj/pull/801[#801]: Restore thread interrupt status after catching InterruptedException

src/itest/groovy/com/hierynomus/sshj/ManyChannelsSpec.groovy

@@ -0,0 +1,74 @@
+/*
+ * Copyright (C)2009 - SSHJ Contributors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.hierynomus.sshj
+
+import net.schmizz.sshj.SSHClient
+import net.schmizz.sshj.common.IOUtils
+import net.schmizz.sshj.connection.channel.direct.Session
+import spock.lang.Specification
+
+import java.util.concurrent.*
+
+import static org.codehaus.groovy.runtime.IOGroovyMethods.withCloseable
+
+class ManyChannelsSpec extends Specification {
+
+    def "should work with many channels without nonexistent channel error (GH issue #805)"() {
+        given:
+        SshdContainer sshd = new SshdContainer.Builder()
+                .withSshdConfig("""${SshdContainer.Builder.DEFAULT_SSHD_CONFIG}
+                MaxSessions 200
+                """.stripMargin())
+                .build()
+        sshd.start()
+        SSHClient client = sshd.getConnectedClient()
+        client.authPublickey("sshj", "src/test/resources/id_rsa")
+
+        when:
+        List<Future<Exception>> futures = []
+        ExecutorService executorService = Executors.newCachedThreadPool()
+
+        for (int i in 0..20) {
+            futures.add(executorService.submit((Callable<Exception>) {
+                return execute(client)
+            }))
+        }
+        executorService.shutdown()
+        executorService.awaitTermination(1, TimeUnit.DAYS)
+
+        then:
+        futures*.get().findAll { it != null }.empty
+
+        cleanup:
+        client.close()
+    }
+
+
+    private static Exception execute(SSHClient sshClient) {
+        try {
+            for (def i in 0..100) {
+                withCloseable (sshClient.startSession()) {sshSession ->
+                    Session.Command sshCommand = sshSession.exec("ls -la")
+                    IOUtils.readFully(sshCommand.getInputStream()).toString()
+                    sshCommand.close()
+                }
+            }
+        } catch (Exception e) {
+            return e
+        }
+        return null
+    }
+}

src/main/java/com/hierynomus/sshj/common/ThreadNameProvider.java

@@ -29,7 +29,8 @@ public class ThreadNameProvider {
     public static void setThreadName(final Thread thread, final RemoteAddressProvider remoteAddressProvider) {
         final InetSocketAddress remoteSocketAddress = remoteAddressProvider.getRemoteSocketAddress();
         final String address = remoteSocketAddress == null ? DISCONNECTED : remoteSocketAddress.toString();
-        final String threadName = String.format("sshj-%s-%s", thread.getClass().getSimpleName(), address);
+        final long started = System.currentTimeMillis();
+        final String threadName = String.format("sshj-%s-%s-%d", thread.getClass().getSimpleName(), address, started);
         thread.setName(threadName);
     }
 }

src/main/java/net/schmizz/concurrent/Promise.java

@@ -136,7 +136,7 @@ public class Promise<V, T extends Throwable> {
             throws T {
         final V value = tryRetrieve(timeout, unit);
         if (value == null)
-            throw chainer.chain(new TimeoutException("Timeout expired"));
+            throw chainer.chain(new TimeoutException("Timeout expired: " + timeout + " " + unit));
         else
             return value;
     }

src/main/java/net/schmizz/sshj/SSHClient.java

@@ -810,7 +810,12 @@ public class SSHClient
             ThreadNameProvider.setThreadName(conn.getKeepAlive(), trans);
             keepAliveThread.start();
         }
-        doKex();
+        if (trans.isKeyExchangeRequired()) {
+            log.debug("Initiating Key Exchange for new connection");
+            doKex();
+        } else {
+            log.debug("Key Exchange already completed for new connection");
+        }
     }
 
     /**

src/main/java/net/schmizz/sshj/connection/channel/AbstractChannel.java

@@ -304,6 +304,25 @@ public abstract class AbstractChannel
         }
     }
 
+    // Prevent CHANNEL_CLOSE to be sent between isOpen and a Transport.write call in the runnable, otherwise
+    // a disconnect with a "packet referred to nonexistent channel" message can occur.
+    //
+    // This particularly happens when the transport.Reader thread passes an eof from the server to the
+    // ChannelInputStream, the reading library-user thread returns, and closes the channel at the same time as the
+    // transport.Reader thread receives the subsequent CHANNEL_CLOSE from the server.
+    boolean whileOpen(TransportRunnable runnable) throws TransportException, ConnectionException {
+        openCloseLock.lock();
+        try {
+            if (isOpen()) {
+                runnable.run();
+                return true;
+            }
+        } finally {
+            openCloseLock.unlock();
+        }
+        return false;
+    }
+
     private void gotChannelRequest(SSHPacket buf)
             throws ConnectionException, TransportException {
         final String reqType;
@@ -427,5 +446,8 @@ public abstract class AbstractChannel
                 + rwin + " >";
     }
 
+    public interface TransportRunnable {
+        void run() throws TransportException, ConnectionException;
+    }
 
 }

src/main/java/net/schmizz/sshj/connection/channel/ChannelOutputStream.java

@@ -30,7 +30,7 @@ import java.util.concurrent.atomic.AtomicBoolean;
  */
 public final class ChannelOutputStream extends OutputStream implements ErrorNotifiable {
 
-    private final Channel chan;
+    private final AbstractChannel chan;
     private final Transport trans;
     private final Window.Remote win;
 
@@ -47,6 +47,12 @@ public final class ChannelOutputStream extends OutputStream implements ErrorNoti
 
         private final SSHPacket packet = new SSHPacket(Message.CHANNEL_DATA);
         private final Buffer.PlainBuffer leftOvers = new Buffer.PlainBuffer();
+        private final AbstractChannel.TransportRunnable packetWriteRunnable = new AbstractChannel.TransportRunnable() {
+            @Override
+            public void run() throws TransportException {
+                trans.write(packet);
+            }
+        };
 
         DataBuffer() {
             headerOffset = packet.rpos();
@@ -99,8 +105,9 @@ public final class ChannelOutputStream extends OutputStream implements ErrorNoti
                 if (leftOverBytes > 0) {
                     leftOvers.putRawBytes(packet.array(), packet.wpos(), leftOverBytes);
                 }
-
-                trans.write(packet);
+                if (!chan.whileOpen(packetWriteRunnable)) {
+                    throwStreamClosed();
+                }
                 win.consume(writeNow);
 
                 packet.rpos(headerOffset);
@@ -119,7 +126,7 @@ public final class ChannelOutputStream extends OutputStream implements ErrorNoti
 
     }
 
-    public ChannelOutputStream(Channel chan, Transport trans, Window.Remote win) {
+    public ChannelOutputStream(AbstractChannel chan, Transport trans, Window.Remote win) {
         this.chan = chan;
         this.trans = trans;
         this.win = win;
@@ -157,7 +164,7 @@ public final class ChannelOutputStream extends OutputStream implements ErrorNoti
             if (error != null) {
                 throw error;
             } else {
-                throw new ConnectionException("Stream closed");
+                throwStreamClosed();
             }
         }
     }
@@ -165,9 +172,14 @@ public final class ChannelOutputStream extends OutputStream implements ErrorNoti
     @Override
     public synchronized void close() throws IOException {
         // Not closed yet, and underlying channel is open to flush the data to.
-        if (!closed.getAndSet(true) && chan.isOpen()) {
-            buffer.flush(false);
-            trans.write(new SSHPacket(Message.CHANNEL_EOF).putUInt32(chan.getRecipient()));
+        if (!closed.getAndSet(true)) {
+            chan.whileOpen(new AbstractChannel.TransportRunnable() {
+                @Override
+                public void run() throws TransportException, ConnectionException {
+                    buffer.flush(false);
+                    trans.write(new SSHPacket(Message.CHANNEL_EOF).putUInt32(chan.getRecipient()));
+                }
+            });
         }
     }
 
@@ -188,4 +200,7 @@ public final class ChannelOutputStream extends OutputStream implements ErrorNoti
         return "< ChannelOutputStream for Channel #" + chan.getID() + " >";
     }
 
+    private static void throwStreamClosed() throws ConnectionException {
+        throw new ConnectionException("Stream closed");
+    }
 }

src/main/java/net/schmizz/sshj/transport/Transport.java

@@ -71,6 +71,13 @@ public interface Transport
     void doKex()
             throws TransportException;
 
+    /**
+     * Is Key Exchange required based on current transport status
+     *
+     * @return Key Exchange required status
+     */
+    boolean isKeyExchangeRequired();
+
     /** @return the version string used by this client to identify itself to an SSH server, e.g. "SSHJ_3_0" */
     String getClientVersion();
 

src/main/java/net/schmizz/sshj/transport/TransportImpl.java

@@ -254,6 +254,16 @@ public final class TransportImpl
         kexer.startKex(true);
     }
 
+    /**
+     * Is Key Exchange required returns true when Key Exchange is not done and when Key Exchange is not ongoing
+     *
+     * @return Key Exchange required status
+     */
+    @Override
+    public boolean isKeyExchangeRequired() {
+        return !kexer.isKexDone() && !kexer.isKexOngoing();
+    }
+
     public boolean isKexDone() {
         return kexer.isKexDone();
     }

src/main/java/net/schmizz/sshj/userauth/method/PasswordResponseProvider.java

@@ -27,7 +27,8 @@ import java.util.regex.Pattern;
 public class PasswordResponseProvider
         implements ChallengeResponseProvider {
 
-    public static final Pattern DEFAULT_PROMPT_PATTERN = Pattern.compile(".*[pP]assword:\\s?\\z", Pattern.DOTALL);
+    // FreeBSD prompt is "Password for user@host:"
+    public static final Pattern DEFAULT_PROMPT_PATTERN = Pattern.compile(".*[pP]assword(?: for .*)?:\\s?\\z", Pattern.DOTALL);
 
     private static final char[] EMPTY_RESPONSE = new char[0];
 

src/test/java/com/hierynomus/sshj/keepalive/KeepAliveThreadTerminationTest.java

@@ -59,10 +59,11 @@ public class KeepAliveThreadTerminationTest {
         assertEquals(Thread.State.NEW, keepAlive.getState());
 
         fixture.connectClient(sshClient);
-        assertEquals(Thread.State.TIMED_WAITING, keepAlive.getState());
 
         assertThrows(UserAuthException.class, () -> sshClient.authPassword("bad", "credentials"));
 
+        assertEquals(Thread.State.TIMED_WAITING, keepAlive.getState());
+
         fixture.stopClient();
         Thread.sleep(STOP_SLEEP);
 

src/test/java/com/hierynomus/sshj/test/SshFixture.java

@@ -20,6 +20,7 @@ import net.schmizz.sshj.DefaultConfig;
 import net.schmizz.sshj.SSHClient;
 import net.schmizz.sshj.util.gss.BogusGSSAuthenticator;
 import org.apache.sshd.common.keyprovider.ClassLoadableResourceKeyPairProvider;
+import org.apache.sshd.common.util.OsUtils;
 import org.apache.sshd.scp.server.ScpCommandFactory;
 import org.apache.sshd.server.SshServer;
 import org.apache.sshd.server.shell.ProcessShellFactory;
@@ -38,6 +39,7 @@ import java.util.concurrent.atomic.AtomicBoolean;
 public class SshFixture extends ExternalResource {
     public static final String hostkey = "hostkey.pem";
     public static final String fingerprint = "ce:a7:c1:cf:17:3f:96:49:6a:53:1a:05:0b:ba:90:db";
+    public static final String listCommand = OsUtils.isWin32() ? "cmd.exe /C dir" : "ls";
 
     private final SshServer server = defaultSshServer();
     private SSHClient client = null;
@@ -110,7 +112,7 @@ public class SshFixture extends ExternalResource {
         ScpCommandFactory commandFactory = new ScpCommandFactory();
         commandFactory.setDelegateCommandFactory((session, command) -> new ProcessShellFactory(command, command.split(" ")).createShell(session));
         sshServer.setCommandFactory(commandFactory);
-        sshServer.setShellFactory(new ProcessShellFactory("ls", "ls"));
+        sshServer.setShellFactory(new ProcessShellFactory(listCommand, listCommand.split(" ")));
         return sshServer;
     }
 

src/test/java/com/hierynomus/sshj/userauth/method/PasswordResponseProviderTest.java

@@ -0,0 +1,124 @@
+/*
+ * Copyright (C)2009 - SSHJ Contributors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.hierynomus.sshj.userauth.method;
+
+import net.schmizz.sshj.userauth.method.PasswordResponseProvider;
+import net.schmizz.sshj.userauth.password.AccountResource;
+import net.schmizz.sshj.userauth.password.PasswordFinder;
+import net.schmizz.sshj.userauth.password.Resource;
+import org.jetbrains.annotations.NotNull;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Collections;
+import java.util.regex.Pattern;
+
+public class PasswordResponseProviderTest {
+    private static final char[] PASSWORD = "the_password".toCharArray();
+    private static final AccountResource ACCOUNT_RESOURCE = new AccountResource("user", "host");
+
+    @Test
+    public void shouldMatchCommonPrompts() {
+        PasswordResponseProvider responseProvider = createDefaultResponseProvider(false);
+        shouldMatch(responseProvider, "Password: ");
+        shouldMatch(responseProvider, "password: ");
+        shouldMatch(responseProvider, "Password:");
+        shouldMatch(responseProvider, "password:");
+        shouldMatch(responseProvider, "user@host's Password: ");
+        shouldMatch(responseProvider, "user@host's password: ");
+        shouldMatch(responseProvider, "user@host's Password:");
+        shouldMatch(responseProvider, "user@host's password:");
+        shouldMatch(responseProvider, "user@host: Password: ");
+        shouldMatch(responseProvider, "(user@host) Password: ");
+        shouldMatch(responseProvider, "any prefix Password for user@host: ");
+        shouldMatch(responseProvider, "any prefix password for user@host: ");
+        shouldMatch(responseProvider, "any prefix Password for user@host:");
+        shouldMatch(responseProvider, "any prefix password for user@host:");
+    }
+
+    @Test
+    public void shouldNotMatchOtherPrompts() {
+        PasswordResponseProvider responseProvider = createDefaultResponseProvider(false);
+        shouldNotMatch(responseProvider, "Password");
+        shouldNotMatch(responseProvider, "password");
+        shouldNotMatch(responseProvider, "Password:  ");
+        shouldNotMatch(responseProvider, "password: suffix");
+        shouldNotMatch(responseProvider, "Password of user@host:");
+        shouldNotMatch(responseProvider, "");
+        shouldNotMatch(responseProvider, "password :");
+        shouldNotMatch(responseProvider, "something else");
+    }
+
+    @Test
+    public void shouldPassRetry() {
+        Assert.assertFalse(createDefaultResponseProvider(false).shouldRetry());
+        Assert.assertTrue(createDefaultResponseProvider(true).shouldRetry());
+    }
+
+    @Test
+    public void shouldHaveNoSubmethods() {
+        Assert.assertEquals(createDefaultResponseProvider(true).getSubmethods(), Collections.emptyList());
+    }
+
+    @Test
+    public void shouldWorkWithCustomPattern() {
+        PasswordFinder passwordFinder = new TestPasswordFinder(true);
+        PasswordResponseProvider responseProvider = new PasswordResponseProvider(passwordFinder, Pattern.compile(".*custom.*"));
+        responseProvider.init(ACCOUNT_RESOURCE, "name", "instruction");
+        shouldMatch(responseProvider, "prefix custom suffix: ");
+        shouldNotMatch(responseProvider, "something else");
+    }
+
+    private static void shouldMatch(PasswordResponseProvider responseProvider, String prompt) {
+        checkPrompt(responseProvider, prompt, PASSWORD);
+    }
+
+    private static void shouldNotMatch(PasswordResponseProvider responseProvider, String prompt) {
+        checkPrompt(responseProvider, prompt, new char[0]);
+    }
+
+    private static void checkPrompt(PasswordResponseProvider responseProvider, String prompt, char[] expected) {
+        Assert.assertArrayEquals("Prompt '" + prompt + "'", expected, responseProvider.getResponse(prompt, false));
+    }
+
+    @NotNull
+    private static PasswordResponseProvider createDefaultResponseProvider(final boolean shouldRetry) {
+        PasswordFinder passwordFinder = new TestPasswordFinder(shouldRetry);
+        PasswordResponseProvider responseProvider = new PasswordResponseProvider(passwordFinder);
+        responseProvider.init(ACCOUNT_RESOURCE, "name", "instruction");
+        return responseProvider;
+    }
+
+    private static class TestPasswordFinder implements PasswordFinder {
+        private final boolean shouldRetry;
+
+        public TestPasswordFinder(boolean shouldRetry) {
+            this.shouldRetry = shouldRetry;
+        }
+
+        @Override
+        public char[] reqPassword(Resource<?> resource) {
+            Assert.assertEquals(resource, ACCOUNT_RESOURCE);
+            return PASSWORD;
+        }
+
+        @Override
+        public boolean shouldRetry(Resource<?> resource) {
+            Assert.assertEquals(resource, ACCOUNT_RESOURCE);
+            return shouldRetry;
+        }
+    }
+}

src/test/java/net/schmizz/sshj/LoadsOfConnects.java

@@ -40,7 +40,7 @@ public class LoadsOfConnects {
                 SSHClient client = fixture.setupConnectedDefaultClient();
                 client.authPassword("test", "test");
                 Session s = client.startSession();
-                Session.Command c = s.exec("ls");
+                Session.Command c = s.exec(SshFixture.listCommand);
                 IOUtils.readFully(c.getErrorStream());
                 IOUtils.readFully(c.getInputStream());
                 c.close();