Merge branch 'stable-3.3' into stable-3.4
* stable-3.3:
Adapt SendMessage of the NoShell command to AsyncCommand type
Update jgit to 73f8acdc5c97e068143c86765995c4fb6923ee91
Clarify the CI validation process for security fixes
Fix registration redirect on OpenID
Update jgit to f2e5bace4841758927d47db7d20e4a6f7353ce57
Update jgit to 00386272264f65c41e36406f7c2e9ea6e901276e
Log when a new SSH connection is rejected due to exceeded limit
Change-Id: I35e2018bad6294f8b5bcb2e7a4ef9663b541848c
diff --git a/Documentation/dev-processes.txt b/Documentation/dev-processes.txt
index 68e56ba..e43e021 100644
--- a/Documentation/dev-processes.txt
+++ b/Documentation/dev-processes.txt
@@ -296,14 +296,32 @@
test that verifies that the security vulnerability is no longer present.
+
Review and approval of the security fixes must be done by the Gerrit
-maintainers. Verifications must be done manually since the Gerrit CI doesn't
-build and test changes of the `gerrit-security-fixes` repository (and it
-shouldn't because everything on the CI server is public which would break
-the embargo).
+maintainers.
+
Once a security fix is ready and submitted, it should be cherry-picked to all
branches that should be fixed.
+. CI validation of the security fix:
++
+The validation of the security fixes does not happen on the regular Gerrit CI,
+because it would compromise the confidentiality of the fix and therefore break
+the embargo.
++
+The release manager maintains a private branch on the
+link:https://gerrit-review.googlesource.com/admin/repos/gerrit-ci-scripts[gerrit-ci-scripts,role=external,window=_blank] repository
+which contains a special build pipeline with special visibility restrictions.
++
+The validation process provides feedback, in terms of Code-Style, Verification
+and Checks, to the incoming security changes. The links associated
+with the build logs are exposed over the Internet but their access limited
+to only those who are actively participating in the development and review of
+the security fix.
++
+The maintainers that are willing to access the links to the CI logs need
+to request a time-limited (maximum 30 days) nominal X.509 certificate from a
+CI maintainer, which allows to access the build logs and analyze failures.
+The release manager may help obtaining that certificate from CI maintainers.
+
. Creation of fixed releases and announcement of the security vulnerability:
+
A release manager should create new bug fix releases for all fixed branches.
diff --git a/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java b/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java
index be975c5..b685011 100644
--- a/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java
+++ b/java/com/google/gerrit/httpd/auth/openid/OpenIdServiceImpl.java
@@ -477,8 +477,9 @@
final StringBuilder rdr = new StringBuilder();
rdr.append(urlProvider.get(req));
String nextToken = Url.decode(token);
- if (isNew && !token.startsWith(PageLinks.REGISTER + "/")) {
- rdr.append('#' + PageLinks.REGISTER);
+ String registerUri = PageLinks.REGISTER + "/";
+ if (isNew && !token.startsWith(registerUri)) {
+ rdr.append('#' + registerUri);
if (nextToken.startsWith("#")) {
// Need to strip the leading # off the token to fix registration page redirect
nextToken = nextToken.substring(1);
diff --git a/java/com/google/gerrit/sshd/LogMaxConnectionsPerUserExceeded.java b/java/com/google/gerrit/sshd/LogMaxConnectionsPerUserExceeded.java
new file mode 100644
index 0000000..6f568b1
--- /dev/null
+++ b/java/com/google/gerrit/sshd/LogMaxConnectionsPerUserExceeded.java
@@ -0,0 +1,42 @@
+// 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.sshd;
+
+import com.google.common.flogger.FluentLogger;
+import com.google.inject.Singleton;
+import java.io.IOException;
+import org.apache.sshd.common.Service;
+import org.apache.sshd.common.session.Session;
+import org.apache.sshd.common.session.SessionDisconnectHandler;
+
+@Singleton
+public class LogMaxConnectionsPerUserExceeded implements SessionDisconnectHandler {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ @Override
+ public boolean handleSessionsCountDisconnectReason(
+ Session session,
+ Service service,
+ String username,
+ int currentSessionCount,
+ int maxSessionCount)
+ throws IOException {
+ logger.atWarning().log(
+ "Max connection count for user %s exceeded, rejecting new connection."
+ + " currentSessionCount = %d, maxSessionCount = %d",
+ username, currentSessionCount, maxSessionCount);
+ return false;
+ }
+}
diff --git a/java/com/google/gerrit/sshd/NoShell.java b/java/com/google/gerrit/sshd/NoShell.java
index dd31e4c..e3f654b 100644
--- a/java/com/google/gerrit/sshd/NoShell.java
+++ b/java/com/google/gerrit/sshd/NoShell.java
@@ -27,10 +27,14 @@
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 @@
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 @@
}
@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 @@
} finally {
sshScope.set(old);
}
- err.write(Constants.encode(message));
- err.flush();
+ err.writeBuffer(new ByteArrayBuffer(Constants.encode(message)));
in.close();
out.close();
diff --git a/java/com/google/gerrit/sshd/SshDaemon.java b/java/com/google/gerrit/sshd/SshDaemon.java
index 9ae8660..fa20b9c 100644
--- a/java/com/google/gerrit/sshd/SshDaemon.java
+++ b/java/com/google/gerrit/sshd/SshDaemon.java
@@ -162,7 +162,8 @@
SshLog sshLog,
@SshListenAddresses List<SocketAddress> listen,
@SshAdvertisedAddresses List<String> advertised,
- MetricMaker metricMaker) {
+ MetricMaker metricMaker,
+ LogMaxConnectionsPerUserExceeded logMaxConnectionsPerUserExceeded) {
setPort(IANA_SSH_PORT /* never used */);
this.cfg = cfg;
@@ -235,6 +236,7 @@
setKeyPairProvider(hostKeyProvider);
setCommandFactory(commandFactory);
setShellFactory(noShell);
+ setSessionDisconnectHandler(logMaxConnectionsPerUserExceeded);
final AtomicInteger connected = new AtomicInteger();
metricMaker.newCallbackMetric(
diff --git a/javatests/com/google/gerrit/integration/ssh/BUILD b/javatests/com/google/gerrit/integration/ssh/BUILD
index dc8e68c..412aad8 100644
--- a/javatests/com/google/gerrit/integration/ssh/BUILD
+++ b/javatests/com/google/gerrit/integration/ssh/BUILD
@@ -5,3 +5,9 @@
group = "peer-keys-auth",
labels = ["ssh"],
)
+
+acceptance_tests(
+ srcs = ["NoShellIT.java"],
+ group = "no-shell",
+ labels = ["ssh"],
+)
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 0000000..ccaf085
--- /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());
+ }
+}
