Fixes based on comments:

Make gpg signing config optional Force detached sigs on Remote artifacts to be Remote Artifacts, no longer cast Log more during gpg signing. Fixed logic around checking gpg prerequisites.
HubSpot · Sep 30, 2015 · bd380ac · bd380ac
1 parent eb3afe3
commit bd380ac
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 33 deletions.
diff --git a/SingularityBase/src/main/java/com/hubspot/deploy/RemoteArtifact.java b/SingularityBase/src/main/java/com/hubspot/deploy/RemoteArtifact.java
@@ -5,9 +5,9 @@
 public abstract class RemoteArtifact extends Artifact {
 
   private final Optional<Long> filesize;
-  private final Optional<Artifact> gpgSignatureArtifact;
+  private final Optional<RemoteArtifact> gpgSignatureArtifact;
 
-  public RemoteArtifact(String name, String filename, Optional<String> md5sum, Optional<Long> filesize, Optional<Artifact> gpgSignatureArtifact) {
+  public RemoteArtifact(String name, String filename, Optional<String> md5sum, Optional<Long> filesize, Optional<RemoteArtifact> gpgSignatureArtifact) {
     super(name, filename, md5sum);
     this.filesize = filesize;
     this.gpgSignatureArtifact = gpgSignatureArtifact;
@@ -17,7 +17,7 @@ public Optional<Long> getFilesize() {
     return filesize;
   }
 
-  public Optional<Artifact> getGpgSignatureArtifact() {
+  public Optional<RemoteArtifact> getGpgSignatureArtifact() {
     return gpgSignatureArtifact;
   }
 

diff --git a/SingularityS3Base/src/main/java/com/hubspot/singularity/s3/base/ArtifactManager.java b/SingularityS3Base/src/main/java/com/hubspot/singularity/s3/base/ArtifactManager.java
@@ -1,9 +1,7 @@
 package com.hubspot.singularity.s3.base;
 
-import java.io.BufferedReader;
 import java.io.File;
 import java.io.IOException;
-import java.io.InputStreamReader;
 import java.io.OutputStream;
 import java.nio.ByteBuffer;
 import java.nio.channels.SeekableByteChannel;
@@ -12,18 +10,17 @@
 import java.nio.file.Paths;
 import java.nio.file.StandardCopyOption;
 import java.nio.file.StandardOpenOption;
-import java.security.GeneralSecurityException;
 import java.util.EnumSet;
 import java.util.List;
 import java.util.concurrent.TimeUnit;
 
-import org.bouncycastle.openpgp.PGPException;
 import org.slf4j.Logger;
 
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
 import com.google.common.hash.HashCode;
 import com.google.common.hash.Hashing;
+import com.google.common.io.ByteStreams;
 import com.hubspot.deploy.Artifact;
 import com.hubspot.deploy.EmbeddedArtifact;
 import com.hubspot.deploy.ExternalArtifact;
@@ -32,7 +29,6 @@
 import com.hubspot.singularity.runner.base.shared.ProcessFailedException;
 import com.hubspot.singularity.runner.base.shared.SimpleProcessManager;
 import com.hubspot.singularity.s3.base.config.SingularityS3Configuration;
-import com.hubspot.singularity.s3.base.gpg.DetachedSignatureVerifier;
 
 public class ArtifactManager extends SimpleProcessManager {
 
@@ -101,32 +97,41 @@ private void downloadAndCheck(RemoteArtifact artifact, Path downloadTo) {
   }
 
   private void checkGpgSignature(RemoteArtifact baseArtifact, Path downloadTo) {
-    if (baseArtifact.getGpgSignatureArtifact().isPresent() && baseArtifact.getGpgSignatureArtifact().get() instanceof RemoteArtifact) {
+    Boolean gpgEnabled = configuration.isGpgCheckingEnabled();
+    Boolean gpgArtifactIsPresent = baseArtifact.getGpgSignatureArtifact().isPresent();
+    Boolean gpgArtifactIsRemote = baseArtifact.getGpgSignatureArtifact().get() instanceof RemoteArtifact;
+    if (!gpgEnabled || !gpgArtifactIsPresent || !gpgArtifactIsRemote) {
+      log.info(String.format("Not verifying gpg signature because: gpgEnabled=%s || gpgArtifactIsPresent=%s || gpgArtifactIsRemote=%s",  gpgEnabled.toString(), gpgArtifactIsPresent.toString(), gpgArtifactIsRemote.toString()));
+      return;
+    }
 
-      Path signatureArtifactPath =  fetch((RemoteArtifact) baseArtifact.getGpgSignatureArtifact().get());
+    File gpgBin = new File(configuration.getGpgBinaryPath());
+    if (!gpgBin.exists() || !gpgBin.canExecute()) {
+      throw new RuntimeException(String.format("gpg binary at %s not found or not executable", configuration.getGpgBinaryPath()));
+    }
 
-      File gpgBin = new File(configuration.getGpgBinaryPath());
-      if (!gpgBin.exists() || !gpgBin.canExecute()) {
-        throw new RuntimeException(String.format("gpg binary at %s not found or not executable", configuration.getGpgBinaryPath()));
-      }
+    Path signatureArtifactPath = fetch(baseArtifact.getGpgSignatureArtifact().get());
+
+    String verifyCmd = String.format("%s --batch --yes --passphrase-fd 0 --homedir %s -u %s --verify %s", configuration.getGpgBinaryPath(), configuration.getGpgHome(), configuration.getGpgKeyUsername(), signatureArtifactPath);
+    log.debug(String.format("Running gpg verify cmd: %s", verifyCmd));
+    try {
+      Process p = Runtime.getRuntime().exec(verifyCmd);
 
-      String verifyCmd = String.format("%s --batch --yes --passphrase-fd 0 --homedir %s -u %s --verify %s", configuration.getGpgBinaryPath(), configuration.getGpgHome(), configuration.getGpgKeyUsername(), signatureArtifactPath);
-      try {
-        Process p = Runtime.getRuntime().exec(verifyCmd);
-        OutputStream stdin = p.getOutputStream();
-        stdin.write(configuration.getGpgKeyPassword().getBytes());
-        p.wait(TimeUnit.SECONDS.toMillis(5));
-        int returnCode = p.exitValue();
-        if (returnCode != 0) {
-          throw new RuntimeException(String.format("Gpg verify failed (rc: %s) could not verify signature %s for file %s", Integer.toString(returnCode), signatureArtifactPath.toString(), downloadTo.toString()));
-        } else {
-          return;
-        }
-      } catch (IOException e) {
-        throw Throwables.propagate(e);
-      } catch (InterruptedException e) {
-        throw Throwables.propagate(e);
+      OutputStream stdin = p.getOutputStream();
+      stdin.write(configuration.getGpgKeyPassword().getBytes());
+
+      p.wait(TimeUnit.SECONDS.toMillis(5));
+      int returnCode = p.exitValue();
+
+      if (returnCode != 0) {
+        String stdErr = ByteStreams.toByteArray(p.getErrorStream()).toString();
+        log.error(String.format("Error running gpg. GPG stderr: %s", stdErr));
+        throw new RuntimeException(String.format("Gpg verify failed (rc: %s) could not verify signature %s for file %s", Integer.toString(returnCode), signatureArtifactPath.toString(), downloadTo.toString()));
       }
+    } catch (IOException e) {
+      throw Throwables.propagate(e);
+    } catch (InterruptedException e) {
+      throw Throwables.propagate(e);
     }
   }
 

diff --git a/...Base/src/main/java/com/hubspot/singularity/s3/base/config/SingularityS3Configuration.java b/...Base/src/main/java/com/hubspot/singularity/s3/base/config/SingularityS3Configuration.java
@@ -67,20 +67,21 @@ public class SingularityS3Configuration extends BaseRunnerConfiguration {
   @JsonProperty
   private String localDownloadPath = "/download";
 
+
   @NotEmpty
+  @JsonProperty
+  private boolean gpgCheckingEnabled = false;
+
   @JsonProperty
   private String gpgBinaryPath = "/usr/bin/gpg";
 
-  @NotEmpty
   @Obfuscate
   @JsonProperty
   private String gpgKeyPassword;
 
-  @NotEmpty
   @JsonProperty
   private String gpgHome;
 
-  @NotEmpty
   @JsonProperty
   private String gpgKeyUsername;
 
@@ -160,6 +161,14 @@ public void setS3ChunkDownloadTimeoutMillis(long s3ChunkDownloadTimeoutMillis) {
     this.s3ChunkDownloadTimeoutMillis = s3ChunkDownloadTimeoutMillis;
   }
 
+  public boolean isGpgCheckingEnabled() {
+    return gpgCheckingEnabled;
+  }
+
+  public void setGpgCheckingEnabled(boolean gpgCheckingEnabled) {
+    this.gpgCheckingEnabled = gpgCheckingEnabled;
+  }
+
   public String getGpgBinaryPath() {
     return gpgBinaryPath;
   }
@@ -188,6 +197,9 @@ public String getGpgKeyUsername() {
     return gpgKeyUsername;
   }
 
+  public void setGpgKeyUsername(String gpgKeyUsername) {
+    this.gpgKeyUsername = gpgKeyUsername;
+  }
 
   @Override
   public String toString() {