Deprecate plain HTTP downloads without checksum

Add a new incompatible flag. Once flipped, we disallow downloads of files
via plain HTTP unless a checksum is provided. Downloads via HTTPS as well
as downloads with a specified hash will still be allowed.

Background #8607

Change-Id: I95f6fa9e230401117de050173f3105ccc7fa7bb4
PiperOrigin-RevId: 257815568

src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java

@@ -17,6 +17,7 @@
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Ascii;
 import com.google.common.base.Optional;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
@@ -48,13 +49,15 @@
 import com.google.devtools.build.lib.shell.Command;
 import com.google.devtools.build.lib.shell.CommandException;
 import com.google.devtools.build.lib.shell.CommandResult;
+import com.google.devtools.build.lib.skyframe.PrecomputedValue;
 import com.google.devtools.build.lib.skylarkbuildapi.repository.SkylarkRepositoryContextApi;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.EvalUtils;
 import com.google.devtools.build.lib.syntax.Runtime;
 import com.google.devtools.build.lib.syntax.SkylarkDict;
 import com.google.devtools.build.lib.syntax.SkylarkList;
 import com.google.devtools.build.lib.syntax.SkylarkType;
+import com.google.devtools.build.lib.syntax.StarlarkSemantics;
 import com.google.devtools.build.lib.util.OsUtils;
 import com.google.devtools.build.lib.util.StringUtilities;
 import com.google.devtools.build.lib.vfs.FileSystem;
@@ -548,7 +551,8 @@ public StructImpl download(
       throws RepositoryFunctionException, EvalException, InterruptedException {
     Map<URI, Map<String, String>> authHeaders = getAuthHeaders(auth);
 
-    List<URL> urls = getUrls(url, /* ensureNonEmpty= */ !allowFail);
+    List<URL> urls =
+        getUrls(url, /* ensureNonEmpty= */ !allowFail, env, !Strings.isNullOrEmpty(sha256));
     RepositoryFunctionException sha256Validation = validateSha256(sha256, location);
     if (sha256Validation != null) {
       warnAboutSha256Error(urls, sha256);
@@ -659,7 +663,8 @@ public StructImpl downloadAndExtract(
       throws RepositoryFunctionException, InterruptedException, EvalException {
     Map<URI, Map<String, String>> authHeaders = getAuthHeaders(auth);
 
-    List<URL> urls = getUrls(url, /* ensureNonEmpty= */ !allowFail);
+    List<URL> urls =
+        getUrls(url, /* ensureNonEmpty= */ !allowFail, env, !Strings.isNullOrEmpty(sha256));
     RepositoryFunctionException sha256Validation = validateSha256(sha256, location);
     if (sha256Validation != null) {
       warnAboutSha256Error(urls, sha256);
@@ -791,8 +796,9 @@ private static ImmutableList<String> checkAllUrls(Iterable<?> urlList) throws Ev
     return result.build();
   }
 
-  private static List<URL> getUrls(Object urlOrList, boolean ensureNonEmpty)
-      throws RepositoryFunctionException, EvalException {
+  private static List<URL> getUrls(
+      Object urlOrList, boolean ensureNonEmpty, Environment env, boolean checksumGiven)
+      throws RepositoryFunctionException, EvalException, InterruptedException {
     List<String> urlStrings;
     if (urlOrList instanceof String) {
       urlStrings = ImmutableList.of((String) urlOrList);
@@ -802,6 +808,7 @@ private static List<URL> getUrls(Object urlOrList, boolean ensureNonEmpty)
     if (ensureNonEmpty && urlStrings.isEmpty()) {
       throw new RepositoryFunctionException(new IOException("urls not set"), Transience.PERSISTENT);
     }
+    StarlarkSemantics semantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
     List<URL> urls = new ArrayList<>();
     for (String urlString : urlStrings) {
       URL url;
@@ -815,7 +822,20 @@ private static List<URL> getUrls(Object urlOrList, boolean ensureNonEmpty)
         throw new RepositoryFunctionException(
             new IOException("Unsupported protocol: " + url.getProtocol()), Transience.PERSISTENT);
       }
-      urls.add(url);
+      if (semantics.incompatibleDisallowUnverifiedHttpDownloads() && !checksumGiven) {
+        if (!Ascii.equalsIgnoreCase("http", url.getProtocol())) {
+          urls.add(url);
+        }
+      } else {
+        urls.add(url);
+      }
+    }
+    if (ensureNonEmpty && urls.isEmpty()) {
+      throw new RepositoryFunctionException(
+          new IOException(
+              "No URLs left after removing plain http URLs due to missing checksum."
+                  + " Please provde either a checksum or an https download location."),
+          Transience.PERSISTENT);
     }
     return urls;
   }

src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java

@@ -383,6 +383,18 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl
       help = "If set to true, vectorized calls to Args#add are disallowed.")
   public boolean incompatibleDisallowOldStyleArgsAdd;
 
+  @Option(
+      name = "incompatible_disallow_unverified_http_downloads",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
+      effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
+      metadataTags = {
+        OptionMetadataTag.INCOMPATIBLE_CHANGE,
+        OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
+      },
+      help = "If set, disallow downloads via plain http if no checksum is given")
+  public boolean incompatibleDisallowUnverifiedHttpDownloads;
+
   @Option(
       name = "incompatible_expand_directories",
       defaultValue = "true",
@@ -661,6 +673,8 @@ public StarlarkSemantics toSkylarkSemantics() {
             .incompatibleDisallowStructProviderSyntax(incompatibleDisallowStructProviderSyntax)
             .incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(
                 incompatibleDisallowRuleExecutionPlatformConstraintsAllowed)
+            .incompatibleDisallowUnverifiedHttpDownloads(
+                incompatibleDisallowUnverifiedHttpDownloads)
             .incompatibleExpandDirectories(incompatibleExpandDirectories)
             .incompatibleNewActionsApi(incompatibleNewActionsApi)
             .incompatibleNoAttrLicense(incompatibleNoAttrLicense)

src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java

@@ -166,6 +166,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) {
 
   public abstract boolean incompatibleDisallowStructProviderSyntax();
 
+  public abstract boolean incompatibleDisallowUnverifiedHttpDownloads();
+
   public abstract boolean incompatibleExpandDirectories();
 
   public abstract boolean incompatibleNewActionsApi();
@@ -263,6 +265,7 @@ public static Builder builderWithDefaults() {
           .incompatibleDisallowOldStyleArgsAdd(true)
           .incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(false)
           .incompatibleDisallowStructProviderSyntax(false)
+          .incompatibleDisallowUnverifiedHttpDownloads(false)
           .incompatibleExpandDirectories(true)
           .incompatibleNewActionsApi(true)
           .incompatibleNoAttrLicense(true)
@@ -336,6 +339,8 @@ public abstract Builder incompatibleDisallowRuleExecutionPlatformConstraintsAllo
 
     public abstract Builder incompatibleDisallowStructProviderSyntax(boolean value);
 
+    public abstract Builder incompatibleDisallowUnverifiedHttpDownloads(boolean value);
+
     public abstract Builder incompatibleExpandDirectories(boolean value);
 
     public abstract Builder incompatibleNewActionsApi(boolean value);

src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java

@@ -148,6 +148,7 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E
         "--incompatible_disallow_rule_execution_platform_constraints_allowed=" + rand.nextBoolean(),
         "--incompatible_disallow_split_empty_separator=" + rand.nextBoolean(),
         "--incompatible_disallow_struct_provider_syntax=" + rand.nextBoolean(),
+        "--incompatible_disallow_unverified_http_downloads=" + rand.nextBoolean(),
         "--incompatible_do_not_split_linking_cmdline=" + rand.nextBoolean(),
         "--incompatible_expand_directories=" + rand.nextBoolean(),
         "--incompatible_new_actions_api=" + rand.nextBoolean(),
@@ -202,6 +203,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
         .incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(rand.nextBoolean())
         .incompatibleDisallowSplitEmptySeparator(rand.nextBoolean())
         .incompatibleDisallowStructProviderSyntax(rand.nextBoolean())
+        .incompatibleDisallowUnverifiedHttpDownloads(rand.nextBoolean())
         .incompatibleDoNotSplitLinkingCmdline(rand.nextBoolean())
         .incompatibleExpandDirectories(rand.nextBoolean())
         .incompatibleNewActionsApi(rand.nextBoolean())

src/test/shell/bazel/skylark_repository_test.sh

@@ -1653,6 +1653,46 @@ EOF
        || fail "Authentication merged incorrectly"
 }
 
+function test_disallow_unverified_http() {
+  mkdir x
+  echo 'exports_files(["file.txt"])' > x/BUILD
+  echo 'Hello World' > x/file.txt
+  tar cvf x.tar x
+  sha256="$(sha256sum x.tar | head -c 64)"
+  serve_file x.tar
+  cat > WORKSPACE <<EOF
+load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")
+http_archive(
+  name="ext",
+  url = "http://127.0.0.1:$nc_port/x.tar",
+)
+EOF
+  cat > BUILD <<'EOF'
+genrule(
+  name = "it",
+  srcs = ["@ext//x:file.txt"],
+  outs = ["it.txt"],
+  cmd = "cp $< $@",
+)
+EOF
+  bazel build --incompatible_disallow_unverified_http_downloads //:it \
+        > "${TEST_log}" 2>&1 && fail "Expeceted failure" || :
+  expect_log 'plain http.*missing checksum'
+
+  # After adding a good checksum, we expect success
+  ed WORKSPACE <<EOF
+/url
+a
+sha256 = "$sha256",
+.
+w
+q
+EOF
+  bazel build --incompatible_disallow_unverified_http_downloads //:it \
+        || fail "Expected success one the checksum is given"
+
+}
+
 function tear_down() {
   shutdown_server
   if [ -d "${TEST_TMPDIR}/server_dir" ]; then