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
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java
index 08b74b6..09f3300 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryContext.java
+++ b/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,6 +49,7 @@
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;
@@ -55,6 +57,7 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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;
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
index 13d233a..868697d 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
@@ -384,6 +384,18 @@
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",
category = "incompatible changes",
@@ -661,6 +673,8 @@
.incompatibleDisallowStructProviderSyntax(incompatibleDisallowStructProviderSyntax)
.incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(
incompatibleDisallowRuleExecutionPlatformConstraintsAllowed)
+ .incompatibleDisallowUnverifiedHttpDownloads(
+ incompatibleDisallowUnverifiedHttpDownloads)
.incompatibleExpandDirectories(incompatibleExpandDirectories)
.incompatibleNewActionsApi(incompatibleNewActionsApi)
.incompatibleNoAttrLicense(incompatibleNoAttrLicense)
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
index 048262e..87b8c5f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
@@ -166,6 +166,8 @@
public abstract boolean incompatibleDisallowStructProviderSyntax();
+ public abstract boolean incompatibleDisallowUnverifiedHttpDownloads();
+
public abstract boolean incompatibleExpandDirectories();
public abstract boolean incompatibleNewActionsApi();
@@ -263,6 +265,7 @@
.incompatibleDisallowOldStyleArgsAdd(true)
.incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(false)
.incompatibleDisallowStructProviderSyntax(false)
+ .incompatibleDisallowUnverifiedHttpDownloads(false)
.incompatibleExpandDirectories(true)
.incompatibleNewActionsApi(true)
.incompatibleNoAttrLicense(true)
@@ -336,6 +339,8 @@
public abstract Builder incompatibleDisallowStructProviderSyntax(boolean value);
+ public abstract Builder incompatibleDisallowUnverifiedHttpDownloads(boolean value);
+
public abstract Builder incompatibleExpandDirectories(boolean value);
public abstract Builder incompatibleNewActionsApi(boolean value);
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
index 3d50b23..fbd3c0a 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
@@ -148,6 +148,7 @@
"--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 @@
.incompatibleDisallowRuleExecutionPlatformConstraintsAllowed(rand.nextBoolean())
.incompatibleDisallowSplitEmptySeparator(rand.nextBoolean())
.incompatibleDisallowStructProviderSyntax(rand.nextBoolean())
+ .incompatibleDisallowUnverifiedHttpDownloads(rand.nextBoolean())
.incompatibleDoNotSplitLinkingCmdline(rand.nextBoolean())
.incompatibleExpandDirectories(rand.nextBoolean())
.incompatibleNewActionsApi(rand.nextBoolean())
diff --git a/src/test/shell/bazel/skylark_repository_test.sh b/src/test/shell/bazel/skylark_repository_test.sh
index afc84c3..84629e1 100755
--- a/src/test/shell/bazel/skylark_repository_test.sh
+++ b/src/test/shell/bazel/skylark_repository_test.sh
@@ -1653,6 +1653,46 @@
|| 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
