Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate HTTP Download #9419

Merged
merged 26 commits into from Jul 3, 2019

Conversation

@JLLeitschuh
Copy link
Member

commented May 16, 2019

mitm_build

Reasoning

After a 4-month research project into the supply chain security of the JVM ecosystem I published this writeup Want to take over the Java ecosystem? All you need is a MITM!

The project details how some of the most popular projects in the JVM ecosystem were using HTTP instead of HTTPS to download their dependencies.

The goal of this PR is to help close this widespread vulnerability impacting a huge swath of the JVM ecosystem.

New API

This PR adds a few new API's to various places that allow Gradle to download code over HTTP in the cases where it's necessary.

repositories {
   maven {
       setUrl("http://my-company-requires-http.company.com")
       isAllowUntrustedProtocol = true
   }
}

Similar API's have been added to the HttpBuildCache and the TextResourceFactory.

Locations Not Covered By this PR

These should be handled in a follow-on PR.

  • The Wrapper task
  • Wrapper itself

Contributor Checklist

  • Review Contribution Guidelines
  • Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • Provide unit tests (under <subproject>/src/test) to verify logic
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:check

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation
  • Recognize contributor in release notes

JLLeitschuh added some commits May 12, 2019

Refactor HttpRedirectResolverIntegrationTest into Abstract Test
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Deprecate download of resources over HTTP; Require opt-in
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>

@JLLeitschuh JLLeitschuh force-pushed the JLLeitschuh:deprecate_http_download branch from 73f0e43 to 08d9272 May 21, 2019

Cleanup HTTP deprecation documentation and unused tests resources
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Fix compilation errors introduced by TextUrlResource.Factory change
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>

@JLLeitschuh JLLeitschuh marked this pull request as ready for review May 21, 2019

fileSystem,
clock
);
}

This comment has been minimized.

Copy link
@JLLeitschuh

JLLeitschuh May 21, 2019

Author Member

@eskatos The Kotlin DSL service logic now uses this method to create the DefaultFileOperations to basically attempt to unify on the creation of this type. Before there were multiple different ways that this object was created across three languages. Now, at least the Kotlin DSL and the DefaultScript for groovy are both using the same method.

JLLeitschuh added some commits May 22, 2019

Fix or accept public API changes for new HTTP acceptance
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Fix failing after HTTP deprecation refactor
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Merge branch 'master' into deprecate_http_download
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>

JLLeitschuh added some commits May 29, 2019

Fix integration tests failing due to new dperication
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Merge branch 'master' into deprecate_http_download
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
return format(enclosingClass) + "." + aClass.getSimpleName();
} else {
return aClass.getSimpleName();
}

This comment has been minimized.

Copy link
@JLLeitschuh

JLLeitschuh May 29, 2019

Author Member

This makes the exceptions a bit more informative when you have enclosing classes named something simple like Factory.

JLLeitschuh added some commits May 29, 2019

Fix compilation issues after merge
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Fix HttpScriptPluginIntegrationSpec
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
@JLLeitschuh

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

@adammurdoch @bigdaz How do we want to deal with the old mavenDeployer when it comes to this depreciation? I don't think we can intercept the redirect chain for the mavenDeployer because it uses all sonatype API's like org.sonatype.aether.repository.RemoteRepository.

JLLeitschuh added some commits May 31, 2019

Allow http for 127.0.0.1
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Cleanup some unnessasary changes after depricate http changes
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Resolve additional failing tests
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Merge branch 'master' into deprecate_http_download
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Merge branch 'master' into deprecate_http_download
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
@JLLeitschuh

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

@thc202 I've updated the description in PR header to describe the reasoning behind this work now that my article is finally public. Go have a look.

@JLLeitschuh JLLeitschuh changed the title Deprecate http Download Deprecate HTTP Download Jun 10, 2019

JLLeitschuh added some commits Jun 20, 2019

Merge branch 'master' into deprecate_http_download
* master: (225 commits)
  Document the purpose of PublicApi.kt
  Mention Eclipse test sources as a potential breaking change in the upgrade notes.
  Fixed managed property generation for `Property<T>` types where `T` is a parameterized type.
  Update library/language versions used by build-init templates.
  Remove the instant execution cache file when there is a failure writing to the cache file.
  Disallow references to `ConfigurationContainer` from tasks serialized to the instant execution cache.
  Recognize contributor
  Publish 5.5-20190620010535+0000
  Fix small typo in the feature variants chapter of the user guide
  Rebaseline instant-execution performance tests
  Refine MethodCodec
  Polish task actions test
  Polish BeanSchema
  Temporarily ignore instant execution performance tests
  Refine ClassLoaderCacheInternal
  Tidy up DefaultInstantExecution & DefaultClassLoaderCache
  Add some coverage for captured task actions
  Dehydrate Closure and fix BeanSchema for task actions
  Add MethodCodec for serializing StandardTaskAction
  Let BeanSchema include AbstractTask.actions
  ...

Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gradle.com>
Merge branch 'master' into deprecate_http_download
* master: (33 commits)
  Implicitly finalize the value of task properties with type `ConfigurableFileCollection` when task execution commences, as is done for `Property` types.
  Change the implementation of `ConfigurableFileCollection.finalizeValue()` so that only the locations are finalized, not the set of files.
  Separate out some shared behaviour from the `FileCollectionResolveContext` implementations.
  Add `ConfigurableFileCollection.finalizeValue()` to allow the value of the file collection to be finalized.
  Remove unnecessary check.
  Fix failure with older versions of PMD that try to enable incremental analysis
  Publish 5.5-20190621010023+0000
  Fix link syntax in upgrade guide
  Recognize contributor
  Polish on release notes and upgrade guide
  More reliably extract PMD version
  Remove spurious printlns from test build
  Use recommended Kotlin DSL syntax in test
  Rebaseline `help on the gradle build comparing the build`
  Add missing property annotation
  Fix unit test to expect a Provider<Boolean>
  Add contribution notice for PMD incremental cache
  Workaround issues with PMD inspecting Gradle's classpath
  Rework after reintroducing reverted changes
  Revert "Revert "Support PMD's analysis cache (#2223)" and "Improve test coverage for pmd incremental analysis (#2961)" (#3125)"
  ...

@JLLeitschuh JLLeitschuh requested a review from big-guy Jun 21, 2019

Show resolved Hide resolved ...es/src/main/java/org/gradle/internal/service/DefaultServiceRegistry.java
Show resolved Hide resolved ...ild-cache-http/src/main/java/org/gradle/caching/http/HttpBuildCache.java Outdated
Show resolved Hide resolved ...egTest/groovy/org/gradle/api/resource/TextResourceIntegrationTest.groovy Outdated
Show resolved Hide resolved ...ntegtests/resolve/http/HttpsToHttpsRedirectResolveIntegrationTest.groovy
* Additionally, in the rare case that a user or a plugin author truly needs to test with a localhost
* server, they can use http://127.0.0.1
*/
if ("127.0.0.1".equals(url.getHost())) {

This comment has been minimized.

Copy link
@paplorinc

paplorinc Jun 21, 2019

Member

what about IPV6?

This comment has been minimized.

Copy link
@JLLeitschuh

JLLeitschuh Jun 21, 2019

Author Member

This bypass is pretty much for our testing exclusively. It's not even going to be an externally documented feature.

If it's important, I can update this if you think I should.

This comment has been minimized.

Copy link
@paplorinc

paplorinc Jun 21, 2019

Member

Not important for me, just wanted to point out that it's a possibility, too.

Show resolved Hide resolved ...n/java/org/gradle/internal/resource/transport/http/HttpClientHelper.java Outdated
Show resolved Hide resolved .../tooling/r40/ProjectConfigurationChildrenProgressCrossVersionSpec.groovy
@big-guy
Copy link
Member

left a comment

We should rollback the changes to ObjectConfigurationAction and the TextResources since this seems to ripple into a lot of different areas. In those cases, we should just always produce a warning/deprecation and not allow for an opt-out.

For script plugins applied over http, these should migrate to https (at least) or proper binary plugins or live with a warning. I don't think it's worth the confusion over having to do enable a flag.

For text resources being loaded over http, I think the argument is similar. Maybe it would be useful to have a fromInsecureUri(...), but let's see if that's actually onerous.

When we clamp down on HTTP, we can revisit this to see if we need to still allow some HTTP (with warnings) or provide more opt-in configuration options.

Show resolved Hide resolved ...es/src/main/java/org/gradle/internal/service/DefaultServiceRegistry.java
Show resolved Hide resolved ...ild-cache-http/src/main/java/org/gradle/caching/http/HttpBuildCache.java Outdated
* </a>
* </b>
*/
void allowInsecureProtocol(boolean allowInsecureProtocol);

This comment has been minimized.

Copy link
@big-guy

big-guy Jun 26, 2019

Member

I think this should either be setAllowInsecureProtocol(boolean) (allow or disallow insecure protocols) or allowInsecureProtocol() (allow insecure protocols to be used). I think we want the first option.

Show resolved Hide resolved ...n/java/org/gradle/internal/resource/transport/http/HttpClientHelper.java Outdated

JLLeitschuh added some commits Jul 1, 2019

Merge branch 'master' into deprecate_http_download
* master: (146 commits)
  Publish 5.5-20190701010031+0000
  Enable Groovy compilation avoidance for Groovy template project (#9816)
  Publish 5.5-20190630025152+0000
  Publish 5.5-20190629014809+0000
  Allow property replacement in version strings in the `plugins` block
  Update notes.md
  Publish 5.5
  Polish comment
  Alter PULL_REQUEST_TEMPLATE to simplify issue resolution
  Address reviews
  Add member Github issue template for the build-cache team
  Cleanup contributor issue templates
  Add 'Fixes' to the PR template
  Rename compilePluginClasspath to astTransformationClasspath (#9794)
  Enable compilation avoidance for Groovy performance tests
  Publish 5.5-20190628010030+0000
  Fix example of generated resources
  Provide the line-number of the plugins block when invoking
  Remove unused `BuildScriptMetadata`
  Polish `InitialPassStatementTransformer`
  ...
@JLLeitschuh

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

@big-guy Looks like just performance failures. 🤔

@big-guy big-guy merged commit 13934c2 into gradle:master Jul 3, 2019

6 of 9 checks passed

CI Status master branch failure since 2019-07-03T14:02:13.773Z
Details
Performance Test Coordinator - Linux (Ready for Merge) TeamCity build failed
Details
Ready for Merge (Trigger) (Check) TeamCity build failed
Details
Compile All (Quick Feedback - Linux Only) TeamCity build finished
Details
DCO DCO
Details
Quick Feedback (Trigger) (Check) TeamCity build finished
Details
Quick Feedback - Linux Only (Trigger) (Check) TeamCity build finished
Details
Sanity Check (Quick Feedback - Linux Only) TeamCity build finished
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.