From 387a01d699ed2c98c4e8f22cba464f4d03083918 Mon Sep 17 00:00:00 2001 From: Mark Bekhet Date: Tue, 6 Apr 2021 15:01:42 -0400 Subject: 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 --- java/com/google/gerrit/sshd/NoShell.java | 42 ++++++++-- javatests/com/google/gerrit/integration/ssh/BUILD | 7 ++ .../google/gerrit/integration/ssh/NoShellIT.java | 96 ++++++++++++++++++++++ 3 files changed, 136 insertions(+), 9 deletions(-) create mode 100644 javatests/com/google/gerrit/integration/ssh/BUILD create mode 100644 javatests/com/google/gerrit/integration/ssh/NoShellIT.java 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; 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,20 +83,35 @@ 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.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 cmd() { + return ImmutableList.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 cmd) throws Exception { + return execute(cmd, sitePaths.data_dir.toFile(), ImmutableMap.of()); + } +} -- cgit v1.2.3