Add sha256 and sha256_src attributes to maven_jar

..and print warnings if sha256 or sha256_src aren't used, like this: > `WARNING: /usr/local/google/home/jingwen/code/copybara/WORKSPACE:192:1: maven_jar rule @junit//jar: Not using a checksum to verify the integrity of the artifact or the usage of SHA-1 is not secure (see https://shattered.io) and can result in an non-reproducible build. Please specify the SHA-256 checksum with: sha256 = "90a8e1603eeca48e7e879f3afbc9560715322985f39a274f6f6070b43f9d06fe",` > `WARNING: /usr/local/google/home/jingwen/code/copybara/WORKSPACE:192:1: maven_jar rule @junit//jar: Not using a checksum to verify the integrity of the artifact or the usage of SHA-1 is not secure (see https://shattered.io) and can result in an non-reproducible build. Please specify the SHA-256 checksum with: sha256_src = "694f4694a51f67dadea4d2045742d38fb4efb92d82d42744b15e26ce653bcd3e",` The warning message is designed to be copy paste-able directly into the WORKSPACE file. #6799 #8880 RELNOTES: Added `sha256` and `sha256_src` attributes to `maven_jar`. Please consider migrating to SHA-256 as SHA-1 has been deemed cryptographically insecure ([https://shattered.io]()). Or, use [`rules_jvm_external`](https://github.com/bazelbuild/rules_jvm_external) to manage your transitive Maven dependencies with artifact pinning and SHA-256 verification support. Closes #9237. Change-Id: I17ef2f88911efbb4527303b1dc1bcb827cc5e308 PiperOrigin-RevId: 265530046
bazelbuild · Aug 26, 2019 · bc9ff0c · bc9ff0c
1 parent 75b98f0
commit bc9ff0c
Show file tree

Hide file tree

Showing 6 changed files with 271 additions and 28 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/MavenDownloader.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/MavenDownloader.java
@@ -23,6 +23,7 @@
 import com.google.devtools.build.lib.bazel.repository.downloader.HttpDownloader;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
+import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.rules.repository.WorkspaceAttributeMapper;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Type;
@@ -60,6 +61,7 @@ public MavenDownloader(RepositoryCache repositoryCache) {
    */
   public JarPaths download(
       String name,
+      Location location,
       WorkspaceAttributeMapper mapper,
       Path outputDirectory,
       MavenServerValue serverValue,
@@ -71,8 +73,22 @@ public JarPaths download(
 
     // Initialize maven artifacts
     String artifactCoords = mapper.get("artifact", Type.STRING);
-    String sha1 = retrieveSha1(name, "sha1", mapper);
-    String sha1Src = retrieveSha1(name, "sha1_src", mapper);
+
+    KeyType keyType = KeyType.SHA256;
+    String checksum = retrieveChecksum(name, "sha256", keyType, mapper);
+
+    KeyType srcJarKeyType = KeyType.SHA256;
+    String srcJarChecksum = retrieveChecksum(name, "sha256_src", srcJarKeyType, mapper);
+
+    if (checksum == null) {
+      keyType = KeyType.SHA1;
+      checksum = retrieveChecksum(name, "sha1", keyType, mapper);
+    }
+
+    if (srcJarChecksum == null) {
+      srcJarKeyType = KeyType.SHA1;
+      srcJarChecksum = retrieveChecksum(name, "sha1_src", keyType, mapper);
+    }
 
     Artifact artifact;
     try {
@@ -83,23 +99,24 @@ public JarPaths download(
 
     Artifact artifactWithSrcs = srcjarCoords(artifact);
 
-    boolean isCaching = repositoryCache.isEnabled() && KeyType.SHA1.isValid(sha1);
+    boolean isCaching = repositoryCache.isEnabled() && keyType.isValid(checksum);
 
     if (isCaching) {
       Path downloadPath = getDownloadDestination(outputDirectory, artifact);
       try {
-        Path cachedDestination = repositoryCache.get(sha1, downloadPath, KeyType.SHA1);
+        Path cachedDestination = repositoryCache.get(checksum, downloadPath, keyType);
         if (cachedDestination != null) {
           Path cachedDestinationSrc = null;
-          if (sha1Src != null) {
+          if (srcJarChecksum != null) {
             Path downloadPathSrc = getDownloadDestination(outputDirectory, artifactWithSrcs);
-            cachedDestinationSrc = repositoryCache.get(sha1Src, downloadPathSrc, KeyType.SHA1);
+            cachedDestinationSrc =
+                repositoryCache.get(srcJarChecksum, downloadPathSrc, srcJarKeyType);
           }
           return new JarPaths(cachedDestination, Optional.fromNullable(cachedDestinationSrc));
         }
       } catch (IOException e) {
         eventHandler.handle(
-            Event.debug("RepositoryCache entry " + sha1 + " is invalid, replacing it..."));
+            Event.debug("RepositoryCache entry " + checksum + " is invalid, replacing it..."));
         // Ignore error trying to get. We'll just download again.
       }
     }
@@ -130,41 +147,72 @@ public JarPaths download(
 
     jarDownload = outputDirectory.getRelative(artifact.getFile().getAbsolutePath());
     // Verify checksum.
-    if (!Strings.isNullOrEmpty(sha1)) {
-      RepositoryCache.assertFileChecksum(sha1, jarDownload, KeyType.SHA1);
+    if (!Strings.isNullOrEmpty(checksum)) {
+      RepositoryCache.assertFileChecksum(checksum, jarDownload, keyType);
     }
 
     Path srcjarDownload = null;
     if (artifactWithSrcs.getFile() != null) {
       srcjarDownload = outputDirectory.getRelative(artifactWithSrcs.getFile().getAbsolutePath());
-      if (!Strings.isNullOrEmpty(sha1Src)) {
-        RepositoryCache.assertFileChecksum(sha1Src, srcjarDownload, KeyType.SHA1);
+      if (!Strings.isNullOrEmpty(srcJarChecksum)) {
+        RepositoryCache.assertFileChecksum(srcJarChecksum, srcjarDownload, srcJarKeyType);
       }
     }
 
     if (isCaching) {
-      repositoryCache.put(sha1, jarDownload, KeyType.SHA1);
-      if (srcjarDownload != null && !Strings.isNullOrEmpty(sha1Src)) {
-        repositoryCache.put(sha1Src, srcjarDownload, KeyType.SHA1);
+      repositoryCache.put(checksum, jarDownload, keyType);
+      if (srcjarDownload != null && !Strings.isNullOrEmpty(srcJarChecksum)) {
+        repositoryCache.put(srcJarChecksum, srcjarDownload, srcJarKeyType);
+      }
+    }
+
+    // The detected keytype is not SHA-256. This is a security risk because missing checksums mean
+    // there's no integrity checking and SHA-1 is cryptographically insecure. Let's be helpful and
+    // print out the computed SHA-256 from the downloaded jar(s).
+    if (keyType != KeyType.SHA256) {
+      String commonMessage =
+          String.format(
+              "maven_jar rule @%s//jar: Not using a checksum to verify the integrity of the"
+                  + " artifact or the usage of SHA-1 (see https://shattered.io) is not secure, and"
+                  + " can result in a non-reproducible build.",
+              name);
+
+      String warningMessage =
+          String.format(
+              commonMessage + " Please specify the SHA-256 checksum with: sha256 = \"%s\",",
+              RepositoryCache.getChecksum(KeyType.SHA256, jarDownload));
+
+      eventHandler.handle(Event.warn(location, warningMessage));
+
+      if (srcjarDownload != null && srcJarKeyType != KeyType.SHA256) {
+        warningMessage =
+            String.format(
+                commonMessage + " Please specify the SHA-256 checksum with: sha256_src = \"%s\",",
+                RepositoryCache.getChecksum(KeyType.SHA256, srcjarDownload));
+
+        eventHandler.handle(Event.warn(location, warningMessage));
       }
     }
 
     return new JarPaths(jarDownload, Optional.fromNullable(srcjarDownload));
   }
 
-  private String retrieveSha1(String name, String attribute, WorkspaceAttributeMapper mapper)
+  @Nullable
+  private static String retrieveChecksum(
+      String name, String attribute, KeyType keyType, WorkspaceAttributeMapper mapper)
       throws EvalException, IOException {
-    String sha1 =
+    String checksum =
         mapper.isAttributeValueExplicitlySpecified(attribute)
             ? mapper.get(attribute, Type.STRING)
             : null;
-    if (sha1 != null && !KeyType.SHA1.isValid(sha1)) {
-      throw new IOException("Invalid SHA-1 for maven_jar " + name + ": '" + sha1 + "'");
+    if (checksum != null && !keyType.isValid(checksum)) {
+      throw new IOException(
+          "Invalid " + keyType + " for maven_jar " + name + ": '" + checksum + "'");
     }
-    return sha1;
+    return checksum;
   }
 
-  private Path getDownloadDestination(Path outputDirectory, Artifact artifact) {
+  private static Path getDownloadDestination(Path outputDirectory, Artifact artifact) {
     String groupIdPath = artifact.getGroupId().replace('.', '/');
     String artifactId = artifact.getArtifactId();
     String classifier = artifact.getClassifier();
@@ -182,15 +230,15 @@ private Path getDownloadDestination(Path outputDirectory, Artifact artifact) {
     return outputDirectory.getRelative(joiner.toString());
   }
 
-  private Artifact srcjarCoords(Artifact jar) {
+  private static Artifact srcjarCoords(Artifact jar) {
     return new DefaultArtifact(
         jar.getGroupId(), jar.getArtifactId(), "sources", jar.getExtension(), jar.getVersion());
   }
 
   /*
    * Set up request for and resolve (retrieve to local repo) artifact
    */
-  private Artifact downloadArtifact(
+  private static Artifact downloadArtifact(
       Artifact artifact,
       RemoteRepository repository,
       RepositorySystemSession session,

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/MavenJarFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/MavenJarFunction.java
@@ -108,6 +108,9 @@ public RepositoryDirectoryValue.Builder fetch(
       SkyKey key)
       throws RepositoryFunctionException, InterruptedException {
 
+    validateShaAttributes(rule, "sha1", "sha256");
+    validateShaAttributes(rule, "sha1_src", "sha256_src");
+
     // Deprecation in favor of the Starlark rule
     StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
     if (starlarkSemantics == null) {
@@ -144,7 +147,8 @@ void validateServerUrl(Rule rule, String serverUrl, boolean disallowUnverifiedHt
       throws RepositoryFunctionException {
 
     boolean hasChecksum =
-        WorkspaceAttributeMapper.of(rule).isAttributeValueExplicitlySpecified("sha1");
+        WorkspaceAttributeMapper.of(rule).isAttributeValueExplicitlySpecified("sha1")
+            || WorkspaceAttributeMapper.of(rule).isAttributeValueExplicitlySpecified("sha256");
 
     if (disallowUnverifiedHttpDownloads && !hasChecksum && serverUrl.startsWith("http://")) {
       throw new RepositoryFunctionException(
@@ -159,6 +163,22 @@ void validateServerUrl(Rule rule, String serverUrl, boolean disallowUnverifiedHt
     }
   }
 
+  private static void validateShaAttributes(Rule rule, String sha1, String sha256)
+      throws RepositoryFunctionException {
+    if (WorkspaceAttributeMapper.of(rule).isAttributeValueExplicitlySpecified(sha1)
+        && WorkspaceAttributeMapper.of(rule).isAttributeValueExplicitlySpecified(sha256)) {
+      throw new RepositoryFunctionException(
+          new EvalException(
+              rule.getLocation(),
+              String.format(
+                  "Attributes '%s' and '%s' cannot be specified at the same time. Please remove "
+                      + "the '%s' attribute in favor of '%s' as SHA-1 is cryptographically "
+                      + "insecure. See https://shattered.io for more information.",
+                  sha1, sha256, sha1, sha256)),
+          Transience.PERSISTENT);
+    }
+  }
+
   private void createDirectory(Path path) throws RepositoryFunctionException {
     try {
       FileSystemUtils.createDirectoryAndParents(path);
@@ -181,7 +201,12 @@ private RepositoryDirectoryValue.Builder createOutputTree(
     try {
       repositoryJars =
           mavenDownloader.download(
-              name, WorkspaceAttributeMapper.of(rule), outputDirectory, serverValue, eventHandler);
+              name,
+              rule.getLocation(),
+              WorkspaceAttributeMapper.of(rule),
+              outputDirectory,
+              serverValue,
+              eventHandler);
     } catch (IOException e) {
       throw new RepositoryFunctionException(e, Transience.TRANSIENT);
     } catch (EvalException e) {

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/workspace/MavenJarRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/workspace/MavenJarRule.java
@@ -55,21 +55,37 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment envi
         <p>Either this or <code>repository</code> can be specified.</p>
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(attr("server", Type.STRING))
+        /* <!-- #BLAZE_RULE(maven_jar).ATTRIBUTE(sha256) -->
+        A SHA-256 hash of the desired jar.
+
+        <p>If the downloaded jar does not match this hash, Bazel will error out. It is a security
+        risk to download a file without verifying cryptographic secure hash.</p>
+        <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
+        .add(attr("sha256", Type.STRING))
+        /* <!-- #BLAZE_RULE(maven_jar).ATTRIBUTE(sha256_src) -->
+        A SHA-256 hash of the desired jar source file.
+
+        <p>If the downloaded source jar does not match this hash, Bazel will error out. It is a
+        security risk to download a file without verifying cryptographic secure hash.</p>
+        <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
+        .add(attr("sha256_src", Type.STRING))
         /* <!-- #BLAZE_RULE(maven_jar).ATTRIBUTE(sha1) -->
         A SHA-1 hash of the desired jar.
 
         <p>If the downloaded jar does not match this hash, Bazel will error out. It is a security
         risk to download a file without verifying cryptographic secure hash. <b>Note that SHA-1 is
         no longer considered a secure cryptographic hash function</b>, but specifying the hash is
         still marginally better than no check at all. This attribute is kept here for legacy support
-        purposes. Please migrate to <a href="https://github.com/bazelbuild/rules_jvm_external/">
-        <code>rules_jvm_external</code></a> and pin your Maven artifacts with their SHA-256
-        checksums.</p>
+        purposes. Please either use the 'sha256' attribute or migrate to
+        <a href="https://github.com/bazelbuild/rules_jvm_external/"><code>rules_jvm_external</code>
+        </a> and pin your Maven artifacts with their SHA-256 checksums.</p>
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(attr("sha1", Type.STRING))
         /* <!-- #BLAZE_RULE(maven_jar).ATTRIBUTE(sha1_src) -->
-        A SHA-1 hash of the desired jar source file.
+        A SHA-1 hash of the desired jar source file. Please consider using 'sha256_src' instead.
 
+        <p>If the downloaded source jar does not match this hash, Bazel will error out. It is a
+        security risk to download a file without verifying cryptographic secure hash.</p>
         <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
         .add(attr("sha1_src", Type.STRING))
         .setWorkspaceOnly()

diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/MavenJarFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/MavenJarFunctionTest.java
@@ -58,6 +58,7 @@ public void testInvalidSha1() throws Exception {
     try {
       downloader.download(
           "foo",
+          rule.getLocation(),
           WorkspaceAttributeMapper.of(rule),
           scratch.dir("/whatever"),
           TEST_SERVER,
@@ -81,6 +82,7 @@ public void testValidSha1() throws Exception {
     try {
       downloader.download(
           "foo",
+          rule.getLocation(),
           WorkspaceAttributeMapper.of(rule),
           scratch.dir("/whatever"),
           TEST_SERVER,
@@ -103,6 +105,7 @@ public void testNoSha1() throws Exception {
     try {
       downloader.download(
           "foo",
+          rule.getLocation(),
           WorkspaceAttributeMapper.of(rule),
           scratch.dir("/whatever"),
           TEST_SERVER,
@@ -127,6 +130,7 @@ public void testNoSha1WithCache() throws Exception {
     try {
       downloader.download(
           "foo",
+          rule.getLocation(),
           WorkspaceAttributeMapper.of(rule),
           scratch.dir("/whatever"),
           TEST_SERVER,
@@ -158,4 +162,58 @@ public void testValidateHttpServerUrlWithNoChecksum() throws Exception {
           .contains("Plain HTTP URLs are not allowed without checksums");
     }
   }
+
+  @Test
+  public void testInvalidSha256() throws Exception {
+    Rule rule =
+        scratchRule(
+            "external",
+            "foo",
+            "maven_jar(",
+            "    name = 'foo',",
+            "    artifact = 'x:y:z:1.1',",
+            "    sha256 = '12345',",
+            ")");
+    MavenDownloader downloader = new MavenDownloader(Mockito.mock(RepositoryCache.class));
+    try {
+      downloader.download(
+          "foo",
+          rule.getLocation(),
+          WorkspaceAttributeMapper.of(rule),
+          scratch.dir("/whatever"),
+          TEST_SERVER,
+          extendedEventHandler);
+      fail("Invalid sha256 should have thrown.");
+    } catch (IOException expected) {
+      assertThat(expected.getMessage()).contains("Invalid SHA-256 for maven_jar foo");
+    }
+  }
+
+  @Test
+  public void testValidSha256() throws Exception {
+    Rule rule =
+        scratchRule(
+            "external",
+            "foo",
+            "maven_jar(",
+            "    name = 'foo',",
+            "    artifact = 'x:y:z:1.1',",
+            "    sha256 = '766ad2a0783f2687962c8ad74ceecc38a28b9f72a2d085ee438b7813e928d0c7',",
+            ")");
+    MavenDownloader downloader = new MavenDownloader(Mockito.mock(RepositoryCache.class));
+    try {
+      downloader.download(
+          "foo",
+          rule.getLocation(),
+          WorkspaceAttributeMapper.of(rule),
+          scratch.dir("/whatever"),
+          TEST_SERVER,
+          extendedEventHandler);
+      fail(
+          "Expected failure to fetch artifact because of nonexistent server and not due to "
+              + "the existence of a valid SHA256");
+    } catch (IOException expected) {
+      assertThat(expected.getMessage()).contains("Failed to fetch Maven dependency:");
+    }
+  }
 }