From 40e2d7f620f023374fd91a34f24f7159d03d9312 Mon Sep 17 00:00:00 2001 From: Klaus Aehlig Date: Fri, 12 Jul 2019 09:24:40 -0700 Subject: [PATCH] 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 --- .../skylark/SkylarkRepositoryContext.java | 30 +++++++++++--- .../packages/StarlarkSemanticsOptions.java | 14 +++++++ .../build/lib/syntax/StarlarkSemantics.java | 5 +++ .../SkylarkSemanticsConsistencyTest.java | 2 + .../shell/bazel/skylark_repository_test.sh | 40 +++++++++++++++++++ 5 files changed, 86 insertions(+), 5 deletions(-) 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 08b74b6afd9502..09f33002d424de 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 @@ public StructImpl download( throws RepositoryFunctionException, EvalException, InterruptedException { Map> authHeaders = getAuthHeaders(auth); - List urls = getUrls(url, /* ensureNonEmpty= */ !allowFail); + List 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> authHeaders = getAuthHeaders(auth); - List urls = getUrls(url, /* ensureNonEmpty= */ !allowFail); + List 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 checkAllUrls(Iterable urlList) throws Ev return result.build(); } - private static List getUrls(Object urlOrList, boolean ensureNonEmpty) - throws RepositoryFunctionException, EvalException { + private static List getUrls( + Object urlOrList, boolean ensureNonEmpty, Environment env, boolean checksumGiven) + throws RepositoryFunctionException, EvalException, InterruptedException { List urlStrings; if (urlOrList instanceof String) { urlStrings = ImmutableList.of((String) urlOrList); @@ -802,6 +808,7 @@ private static List 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 urls = new ArrayList<>(); for (String urlString : urlStrings) { URL url; @@ -815,7 +822,20 @@ private static List 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; } 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 13d233a903c93e..868697d347871b 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 @@ -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) 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 048262ee212ce4..87b8c5f4b7bf4a 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 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); 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 3d50b23a67cefe..fbd3c0a5b06ae5 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 @@ 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()) diff --git a/src/test/shell/bazel/skylark_repository_test.sh b/src/test/shell/bazel/skylark_repository_test.sh index afc84c3fbdf690..84629e1444ba80 100755 --- a/src/test/shell/bazel/skylark_repository_test.sh +++ b/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 < 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 <