From 6bd0fc58de49c4ff2433adeee49222570a28d706 Mon Sep 17 00:00:00 2001 From: jianglai Date: Wed, 6 Dec 2017 15:32:53 -0800 Subject: [PATCH] Fix some GPG related test issues We were running FOSS tests on an old version of Ubuntu (14.04) which comes with a rather old gpg version 1.4.16 compared to the current version 2.1.22 (https://lists.gnupg.org/pipermail/gnupg-announce/2017q3/000411.html). As a result some default settings have changed between these versions, leading to test failures when tests are run on newer platforms. In this CL several of the settings are made explicit, no longer depending on default values, which makes them work on either platform. 1. "--no-mdc-error" is set. We do not have mdc integrity protection for the test keys, which results in a non-zero return value for newer versions of GPG. Setting this flag makes return value zero again. 2. "--keyid-format" long is set. GPG key IDs are the last 16 (long key id) or 8 (short key id) octets of the key fingerprint (https://security.stackexchange.com/questions/84280/short-openpgp-key-ids-are-insecure-how-to-configure-gnupg-to-use-long-key-ids-i). Older version uses the short id as default, whereas newer versions defaults to long id. Also change the expected key ID to 16 bytes accordingly. 3. Output stderr in GpgSystemCommandRule when failure occurs during key import. This help debug key import failure due to too long gpg root path. Note: this failure itself is not fixed and still would occur on newer Debian or macOS systems. 4. Set gpg root folder permission correctly to 700, otherwise the newer version gpg will return non-zero value. Previously used method set it to 755. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=178163297 --- .../registry/rde/BrdaCopyActionTest.java | 17 +++++--- .../rde/GhostrydeGpgIntegrationTest.java | 7 ++-- .../registry/rde/RydeGpgIntegrationTest.java | 18 ++++++-- .../testing/GpgSystemCommandRule.java | 42 +++++++++++++------ 4 files changed, 59 insertions(+), 25 deletions(-) diff --git a/javatests/google/registry/rde/BrdaCopyActionTest.java b/javatests/google/registry/rde/BrdaCopyActionTest.java index 177cfc1f3..b6565b66d 100644 --- a/javatests/google/registry/rde/BrdaCopyActionTest.java +++ b/javatests/google/registry/rde/BrdaCopyActionTest.java @@ -37,7 +37,6 @@ import google.registry.testing.GcsTestingUtils; import google.registry.testing.GpgSystemCommandRule; import google.registry.testing.ShardableTestCase; import java.io.File; -import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; @@ -138,8 +137,14 @@ public class BrdaCopyActionTest extends ShardableTestCase { File rydeTmp = new File(gpg.getCwd(), "ryde"); Files.write(readGcsFile(gcsService, RYDE_FILE), rydeTmp); - - Process pid = gpg.exec("gpg", "--list-packets", rydeTmp.toString()); + Process pid = + gpg.exec( + "gpg", + "--list-packets", + "--ignore-mdc-error", + "--keyid-format", + "long", + rydeTmp.toString()); String stdout = slurp(pid.getInputStream()); String stderr = slurp(pid.getErrorStream()); assertWithMessage(stderr).that(pid.waitFor()).isEqualTo(0); @@ -167,7 +172,9 @@ public class BrdaCopyActionTest extends ShardableTestCase { assertWithMessage("Unexpected asymmetric encryption algorithm") .that(stderr) .contains("encrypted with 2048-bit RSA key"); - assertWithMessage("Unexpected receiver public key").that(stderr).contains("ID 54E1EB0F"); + assertWithMessage("Unexpected receiver public key") + .that(stderr) + .contains("ID 7F9084EE54E1EB0F"); } @Test @@ -187,7 +194,7 @@ public class BrdaCopyActionTest extends ShardableTestCase { assertThat(stderr).contains("rde-unittest@registry.test"); } - private String slurp(InputStream is) throws FileNotFoundException, IOException { + private String slurp(InputStream is) throws IOException { return CharStreams.toString(new InputStreamReader(is, UTF_8)); } } diff --git a/javatests/google/registry/rde/GhostrydeGpgIntegrationTest.java b/javatests/google/registry/rde/GhostrydeGpgIntegrationTest.java index 2b29b3d1e..da31d92e4 100644 --- a/javatests/google/registry/rde/GhostrydeGpgIntegrationTest.java +++ b/javatests/google/registry/rde/GhostrydeGpgIntegrationTest.java @@ -29,7 +29,6 @@ import google.registry.testing.GpgSystemCommandRule; import google.registry.testing.ShardableTestCase; import java.io.File; import java.io.FileInputStream; -import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStreamReader; @@ -100,7 +99,7 @@ public class GhostrydeGpgIntegrationTest extends ShardableTestCase { os.write(data); } - Process pid = gpg.exec(cmd.get(), "--list-packets", file.getPath()); + Process pid = gpg.exec(cmd.get(), "--list-packets", "--keyid-format", "long", file.getPath()); String stdout = CharStreams.toString(new InputStreamReader(pid.getInputStream(), UTF_8)); String stderr = CharStreams.toString(new InputStreamReader(pid.getErrorStream(), UTF_8)); assertWithMessage(stderr).that(pid.waitFor()).isEqualTo(0); @@ -108,7 +107,7 @@ public class GhostrydeGpgIntegrationTest extends ShardableTestCase { assertThat(stdout).contains(":encrypted data packet:"); assertThat(stdout).contains("version 3, algo 1, keyid A59C132F3589A1D5"); assertThat(stdout).contains("name=\"" + filename.get() + "\""); - assertThat(stderr).contains("encrypted with 2048-bit RSA key, ID 3589A1D5"); + assertThat(stderr).contains("encrypted with 2048-bit RSA key, ID A59C132F3589A1D5"); pid = gpg.exec(cmd.get(), "--use-embedded-filename", file.getPath()); stderr = CharStreams.toString(new InputStreamReader(pid.getErrorStream(), UTF_8)); @@ -118,7 +117,7 @@ public class GhostrydeGpgIntegrationTest extends ShardableTestCase { assertThat(slurp(dataFile)).isEqualTo(content.get()); } - private String slurp(File file) throws FileNotFoundException, IOException { + private String slurp(File file) throws IOException { return CharStreams.toString(new InputStreamReader(new FileInputStream(file), UTF_8)); } diff --git a/javatests/google/registry/rde/RydeGpgIntegrationTest.java b/javatests/google/registry/rde/RydeGpgIntegrationTest.java index 4ef58a7b0..9db22c43e 100644 --- a/javatests/google/registry/rde/RydeGpgIntegrationTest.java +++ b/javatests/google/registry/rde/RydeGpgIntegrationTest.java @@ -147,7 +147,14 @@ public class RydeGpgIntegrationTest extends ShardableTestCase { // gpg: WARNING: message was not integrity protected logger.info("Running GPG to list info about OpenPGP message..."); { - Process pid = gpg.exec(cmd.get(), "--list-packets", rydeFile.toString()); + Process pid = + gpg.exec( + cmd.get(), + "--list-packets", + "--ignore-mdc-error", + "--keyid-format", + "long", + rydeFile.toString()); String stdout = slurp(pid.getInputStream()); String stderr = slurp(pid.getErrorStream()); assertWithMessage(stderr).that(pid.waitFor()).isEqualTo(0); @@ -175,7 +182,9 @@ public class RydeGpgIntegrationTest extends ShardableTestCase { assertWithMessage("Unexpected asymmetric encryption algorithm") .that(stderr) .contains("encrypted with 2048-bit RSA key"); - assertWithMessage("Unexpected receiver public key").that(stderr).contains("ID 54E1EB0F"); + assertWithMessage("Unexpected receiver public key") + .that(stderr) + .contains("ID 7F9084EE54E1EB0F"); } // Iron Mountain now verifies that rydeFile is authentic and was signed appropriately. @@ -203,7 +212,8 @@ public class RydeGpgIntegrationTest extends ShardableTestCase { // gpg: WARNING: message was not integrity protected logger.info("Running GPG to extract tar..."); { - Process pid = gpg.exec(cmd.get(), "--use-embedded-filename", rydeFile.toString()); + Process pid = + gpg.exec(cmd.get(), "--use-embedded-filename", "--ignore-mdc-error", rydeFile.toString()); String stderr = slurp(pid.getErrorStream()); assertWithMessage(stderr).that(pid.waitFor()).isEqualTo(0); } @@ -226,7 +236,7 @@ public class RydeGpgIntegrationTest extends ShardableTestCase { return CharStreams.toString(new InputStreamReader(new FileInputStream(file), UTF_8)); } - private String slurp(InputStream is) throws FileNotFoundException, IOException { + private String slurp(InputStream is) throws IOException { return CharStreams.toString(new InputStreamReader(is, UTF_8)); } diff --git a/javatests/google/registry/testing/GpgSystemCommandRule.java b/javatests/google/registry/testing/GpgSystemCommandRule.java index ffcf9c7ff..b68c03fea 100644 --- a/javatests/google/registry/testing/GpgSystemCommandRule.java +++ b/javatests/google/registry/testing/GpgSystemCommandRule.java @@ -18,10 +18,17 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; import static com.google.common.truth.Truth.assertWithMessage; +import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.io.ByteSource; +import com.google.common.io.CharStreams; import java.io.File; import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.nio.file.Files; +import java.nio.file.attribute.PosixFilePermissions; +import java.util.Objects; import org.junit.rules.ExternalResource; /** @@ -53,51 +60,58 @@ public final class GpgSystemCommandRule extends ExternalResource { /** Returns the temporary directory from which commands are run. */ public File getCwd() { - checkState(cwd != DEV_NULL); + checkState(!Objects.equals(cwd, DEV_NULL)); return cwd; } /** Returns the temporary directory in which GnuPG configs are stored. */ public File getConf() { - checkState(conf != DEV_NULL); + checkState(!Objects.equals(conf, DEV_NULL)); return conf; } /** * Runs specified system command and arguments within the GPG testing environment. * - * @throws IOException * @see Runtime#exec(String[]) */ public final Process exec(String... args) throws IOException { - checkState(cwd != DEV_NULL); + checkState(!Objects.equals(cwd, DEV_NULL)); checkArgument(args.length > 0, "args"); return runtime.exec(args, env, cwd); } @Override protected void before() throws IOException, InterruptedException { - checkState(cwd == DEV_NULL); + checkState(Objects.equals(cwd, DEV_NULL)); cwd = File.createTempFile(TEMP_FILE_PREFIX, "", null); cwd.delete(); cwd.mkdir(); conf = new File(cwd, ".gnupg"); conf.mkdir(); - conf.setReadable(true, true); - env = new String[] { - "PATH=" + System.getenv("PATH"), - "GNUPGHOME=" + conf.getAbsolutePath(), - }; + Files.setPosixFilePermissions(conf.toPath(), PosixFilePermissions.fromString("rwx------")); + env = + new String[] { + "PATH=" + System.getenv("PATH"), "GNUPGHOME=" + conf.getAbsolutePath(), + }; Process pid = exec("gpg", "--import"); publicKeyring.copyTo(pid.getOutputStream()); pid.getOutputStream().close(); - assertWithMessage("Failed to import public keyring").that(pid.waitFor()).isEqualTo(0); + int returnValue = pid.waitFor(); + assertWithMessage( + String.format("Failed to import public keyring: \n%s", slurp(pid.getErrorStream()))) + .that(returnValue) + .isEqualTo(0); pid = exec("gpg", "--allow-secret-key-import", "--import"); privateKeyring.copyTo(pid.getOutputStream()); pid.getOutputStream().close(); - assertWithMessage("Failed to import private keyring").that(pid.waitFor()).isEqualTo(0); + returnValue = pid.waitFor(); + assertWithMessage( + String.format("Failed to import private keyring: \n%s", slurp(pid.getErrorStream()))) + .that(returnValue) + .isEqualTo(0); } @Override @@ -105,4 +119,8 @@ public final class GpgSystemCommandRule extends ExternalResource { cwd = DEV_NULL; conf = DEV_NULL; } + + private String slurp(InputStream is) throws IOException { + return CharStreams.toString(new InputStreamReader(is, UTF_8)); + } }