From 90bed3f2432b243a3f19c3570c05feac38e954b9 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Tue, 9 Jun 2020 22:07:19 +0200 Subject: [PATCH] DatabasePubKeyAuth: Exclude comment from peer key line if present In I9a934bda3 base64 decoding was changed from using commons-codec library to Guava library. However, this broke users who specify not only base 64 encoded key, but also provide comment, similar to authorized keys format without leading algorithm. Clarify the documentation what format is supported in peer keys file and skip the comment part if provided. Also add integration test for peer keys authentication with three use cases: 1. Test format: 2. Test format: 3. No peer keys file present Test Plan: $ bazel test javatests/com/google/gerrit/integration/ssh:peer-keys-auth Bug: Issue 12884 Change-Id: Id748cb55497e3347477012911f716ac5b7b69863 --- Documentation/config-gerrit.txt | 8 +- .../gerrit/sshd/DatabasePubKeyAuth.java | 10 +- .../com/google/gerrit/integration/ssh/BUILD | 7 ++ .../integration/ssh/PeerKeysAuthIT.java | 104 ++++++++++++++++++ 4 files changed, 127 insertions(+), 2 deletions(-) create mode 100644 javatests/com/google/gerrit/integration/ssh/BUILD create mode 100644 javatests/com/google/gerrit/integration/ssh/PeerKeysAuthIT.java diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 257bf6f3af..5f9b369c3e 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -5389,7 +5389,13 @@ The optional file `'$site_path'/etc/peer_keys` controls who can login as the 'Gerrit Code Review' user, required for the link:cmd-suexec.html[suexec] command. -The format is one Base-64 encoded public key per line. +The format is one Base-64 encoded public key per line with optional comment, e.g.: +---- +# Comments allowed at start of line +AAAAC3...51R== john@example.net +# Another comment +AAAAB5...21S== jane@example.net +---- === Configurable Parameters diff --git a/java/com/google/gerrit/sshd/DatabasePubKeyAuth.java b/java/com/google/gerrit/sshd/DatabasePubKeyAuth.java index 916775dc61..6997d9625b 100644 --- a/java/com/google/gerrit/sshd/DatabasePubKeyAuth.java +++ b/java/com/google/gerrit/sshd/DatabasePubKeyAuth.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkState; import static java.nio.charset.StandardCharsets.ISO_8859_1; import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.common.base.Splitter; import com.google.common.flogger.FluentLogger; import com.google.common.io.BaseEncoding; import com.google.gerrit.common.FileUtil; @@ -38,6 +39,7 @@ import java.security.PublicKey; import java.util.Collection; import java.util.Collections; import java.util.HashSet; +import java.util.List; import java.util.Locale; import java.util.Set; import org.apache.sshd.common.SshException; @@ -197,9 +199,15 @@ class DatabasePubKeyAuth implements PublickeyAuthenticator { continue; } + List parts = Splitter.on(' ').splitToList(line); + if (parts.size() > 2) { + throw new IllegalArgumentException( + "Invalid peer key file format, only lines supported"); + } try { byte[] bin = - BaseEncoding.base64().decode(new String(line.getBytes(ISO_8859_1), ISO_8859_1)); + BaseEncoding.base64() + .decode(new String(parts.get(0).getBytes(ISO_8859_1), ISO_8859_1)); keys.add(new ByteArrayBuffer(bin).getRawPublicKey()); } catch (RuntimeException | SshException e) { logBadKey(path, line, e); diff --git a/javatests/com/google/gerrit/integration/ssh/BUILD b/javatests/com/google/gerrit/integration/ssh/BUILD new file mode 100644 index 0000000000..dc8e68c90d --- /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 = ["PeerKeysAuthIT.java"], + group = "peer-keys-auth", + labels = ["ssh"], +) diff --git a/javatests/com/google/gerrit/integration/ssh/PeerKeysAuthIT.java b/javatests/com/google/gerrit/integration/ssh/PeerKeysAuthIT.java new file mode 100644 index 0000000000..a219cc211d --- /dev/null +++ b/javatests/com/google/gerrit/integration/ssh/PeerKeysAuthIT.java @@ -0,0 +1,104 @@ +// Copyright (C) 2020 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.ISO_8859_1; +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.common.Version; +import com.google.gerrit.server.PeerDaemonUser; +import com.google.inject.Inject; +import java.io.IOException; +import java.net.InetSocketAddress; +import java.nio.file.Files; +import org.junit.Test; + +@NoHttpd +@UseSsh +public class PeerKeysAuthIT extends StandaloneSiteTest { + private static final String[] SSH_KEYGEN_CMD = + new String[] {"ssh-keygen", "-t", "rsa", "-q", "-P", "", "-f", "id_rsa"}; + private static final String[] SSH_COMMAND = + new String[] { + "ssh", + "-o", + "UserKnownHostsFile=/dev/null", + "-o", + "StrictHostKeyChecking=no", + "-o", + "IdentitiesOnly=yes", + "-i" + }; + + @Inject private @TestSshServerAddress InetSocketAddress sshAddress; + + @Test + public void test() throws Exception { + try (ServerContext ctx = startServer()) { + ctx.getInjector().injectMembers(this); + // Generate private/public key for user + execute(ImmutableList.builder().add(SSH_KEYGEN_CMD).build()); + + String[] parts = + new String(Files.readAllBytes(sitePaths.data_dir.resolve("id_rsa.pub")), UTF_8) + .split(" "); + + // Loose algorithm at index 0, verify the format: "key comment" + Files.write( + sitePaths.peer_keys, String.format("%s %s", parts[1], parts[2]).getBytes(ISO_8859_1)); + assertContent(execGerritVersionCommand()); + + // Only preserve the key material: no algorithm and no comment + Files.write(sitePaths.peer_keys, parts[1].getBytes(ISO_8859_1)); + assertContent(execGerritVersionCommand()); + + // Wipe out the content of the peer keys file + Files.delete(sitePaths.peer_keys); + assertThrows(IOException.class, () -> execGerritVersionCommand()); + } + } + + private String execGerritVersionCommand() throws Exception { + return execute( + ImmutableList.builder() + .add(SSH_COMMAND) + .add(sitePaths.data_dir.resolve("id_rsa").toString()) + .add("-p " + sshAddress.getPort()) + .add(PeerDaemonUser.USER_NAME + "@" + sshAddress.getHostName()) + .add("suexec") + .add("--as") + .add("admin") + .add("--") + .add("gerrit") + .add("version") + .build()); + } + + private String execute(ImmutableList cmd) throws Exception { + return execute(cmd, sitePaths.data_dir.toFile(), ImmutableMap.of()); + } + + private static void assertContent(String result) { + assertThat(result).contains("gerrit version " + Version.getVersion()); + } +}