Adapt SendMessage of the NoShell command to AsyncCommand type

Before this change, the SendMessage class inside the NoShell command
implemented the Command interface. Therefore, the command was using
instances of OutputStream and closing them at the end of the command.
Given that the channel isn't closed at this stage, this produced a
problem as apache-sshd calls the flush method of one of the output
streams. Therefore, the command was hanging and didn't release the
terminal until the user pressed a key.

This change adapts the SendMessage class to an AyncCommand type.
Therefore, the Apache library treats the command as AsyncCommand and
uses the newly introduced I/O streams instead of normal streams. As a
result, the problematic flush call of the output stream doesn't happen,
because the normal output stream is not initialized, instead the I/O
streams are initialized and used.

An integration test is added to verify that the command doesn't hang
indefinitely. It does so by setting a timeout to the command.

Bug: Issue 11142
Change-Id: Ia6ed0d4ee264d2e901eaa17ea444bf715e3b44db

3 files changed, 136 insertions, 9 deletions

diff --git a/java/com/google/gerrit/sshd/NoShell.java b/java/com/google/gerrit/sshd/NoShell.java
index dd31e4c081..2a29a624a3 100644
--- a/java/com/google/gerrit/sshd/NoShell.java
+++ b/java/com/google/gerrit/sshd/NoShell.java
@@ -27,10 +27,14 @@ import java.io.InputStream;
 import java.io.OutputStream;
 import java.net.MalformedURLException;
 import java.net.URL;
+import org.apache.sshd.common.io.IoInputStream;
+import org.apache.sshd.common.io.IoOutputStream;
+import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
 import org.apache.sshd.server.Environment;
 import org.apache.sshd.server.ExitCallback;
 import org.apache.sshd.server.SessionAware;
 import org.apache.sshd.server.channel.ChannelSession;
+import org.apache.sshd.server.command.AsyncCommand;
 import org.apache.sshd.server.command.Command;
 import org.apache.sshd.server.session.ServerSession;
 import org.apache.sshd.server.shell.ShellFactory;
@@ -56,13 +60,19 @@ class NoShell implements ShellFactory {
     return shell.get();
   }
 
-  static class SendMessage implements Command, SessionAware {
+  /**
+   * When AsyncCommand is implemented by a command as below, the usual blocking streams aren't set.
+   *
+   * @see org.apache.sshd.server.command.AsyncCommand
+   */
+  static class SendMessage implements AsyncCommand, SessionAware {
     private final Provider<MessageFactory> messageFactory;
     private final SshScope sshScope;
 
-    private InputStream in;
-    private OutputStream out;
-    private OutputStream err;
+    private IoInputStream in;
+    private IoOutputStream out;
+    private IoOutputStream err;
+
     private ExitCallback exit;
     private Context context;
 
@@ -73,21 +83,36 @@ class NoShell implements ShellFactory {
     }
 
     @Override
-    public void setInputStream(InputStream in) {
+    public void setIoInputStream(IoInputStream in) {
       this.in = in;
     }
 
     @Override
-    public void setOutputStream(OutputStream out) {
+    public void setIoOutputStream(IoOutputStream out) {
       this.out = out;
     }
 
     @Override
-    public void setErrorStream(OutputStream err) {
+    public void setIoErrorStream(IoOutputStream err) {
       this.err = err;
     }
 
     @Override
+    public void setInputStream(InputStream in) {
+      // ignored
+    }
+
+    @Override
+    public void setOutputStream(OutputStream out) {
+      // ignore
+    }
+
+    @Override
+    public void setErrorStream(OutputStream err) {
+      // ignore
+    }
+
+    @Override
     public void setExitCallback(ExitCallback callback) {
       this.exit = callback;
     }
@@ -107,8 +132,7 @@ class NoShell implements ShellFactory {
       } finally {
         sshScope.set(old);
       }
-      err.write(Constants.encode(message));
-      err.flush();
+      err.writePacket(new ByteArrayBuffer(Constants.encode(message)));
 
       in.close();
       out.close();
diff --git a/javatests/com/google/gerrit/integration/ssh/BUILD b/javatests/com/google/gerrit/integration/ssh/BUILD
new file mode 100644
index 0000000000..72f07854ba
--- /dev/null
+++ b/javatests/com/google/gerrit/integration/ssh/BUILD
@@ -0,0 +1,7 @@
+load("//javatests/com/google/gerrit/acceptance:tests.bzl", "acceptance_tests")
+
+acceptance_tests(
+    srcs = ["NoShellIT.java"],
+    group = "no-shell",
+    labels = ["ssh-no-shell"],
+)
diff --git a/javatests/com/google/gerrit/integration/ssh/NoShellIT.java b/javatests/com/google/gerrit/integration/ssh/NoShellIT.java
new file mode 100644
index 0000000000..ccaf085299
--- /dev/null
+++ b/javatests/com/google/gerrit/integration/ssh/NoShellIT.java
@@ -0,0 +1,96 @@
+// Copyright (C) 2021 The Android Open Source Project
+//
+// 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.google.gerrit.integration.ssh;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.acceptance.GerritServer.TestSshServerAddress;
+import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.acceptance.StandaloneSiteTest;
+import com.google.gerrit.acceptance.UseSsh;
+import com.google.gerrit.extensions.api.GerritApi;
+import com.google.inject.Inject;
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import org.junit.Test;
+
+@NoHttpd
+@UseSsh
+public class NoShellIT extends StandaloneSiteTest {
+  private static final String[] SSH_KEYGEN_CMD =
+      new String[] {"ssh-keygen", "-t", "rsa", "-q", "-P", "", "-f"};
+
+  @Inject private GerritApi gApi;
+  @Inject private @TestSshServerAddress InetSocketAddress sshAddress;
+
+  private String identityPath;
+
+  @Test(timeout = 30000)
+  public void verifyCommandsIsClosed() throws Exception {
+    try (ServerContext ctx = startServer()) {
+      setUpTestHarness(ctx);
+
+      IOException thrown = assertThrows(IOException.class, () -> execute(cmd()));
+      assertThat(thrown)
+          .hasMessageThat()
+          .contains("Hi Administrator, you have successfully connected over SSH.");
+    }
+  }
+
+  private void setUpTestHarness(ServerContext ctx) throws Exception {
+    ctx.getInjector().injectMembers(this);
+    setUpAuthentication();
+    identityPath = sitePaths.data_dir.resolve(String.format("id_rsa_%s", "admin")).toString();
+  }
+
+  private void setUpAuthentication() throws Exception {
+    execute(
+        ImmutableList.<String>builder()
+            .add(SSH_KEYGEN_CMD)
+            .add(String.format("id_rsa_%s", "admin"))
+            .build());
+    gApi.accounts()
+        .id("admin")
+        .addSshKey(
+            new String(
+                java.nio.file.Files.readAllBytes(
+                    sitePaths.data_dir.resolve(String.format("id_rsa_%s.pub", "admin"))),
+                UTF_8));
+  }
+
+  private ImmutableList<String> cmd() {
+    return ImmutableList.<String>builder()
+        .add("ssh")
+        .add("-tt")
+        .add("-o")
+        .add("StrictHostKeyChecking=no")
+        .add("-o")
+        .add("UserKnownHostsFile=/dev/null")
+        .add("-p")
+        .add(String.valueOf(sshAddress.getPort()))
+        .add("admin@" + sshAddress.getHostName())
+        .add("-i")
+        .add(identityPath)
+        .build();
+  }
+
+  private String execute(ImmutableList<String> cmd) throws Exception {
+    return execute(cmd, sitePaths.data_dir.toFile(), ImmutableMap.of());
+  }
+}