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
Merged

Conversation

JLLeitschuh
Copy link
Contributor

@JLLeitschuh JLLeitschuh 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

Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
@JLLeitschuh JLLeitschuh force-pushed the deprecate_http_download branch from 73f0e43 to 08d9272 Compare May 21, 2019 18:44
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
@JLLeitschuh JLLeitschuh marked this pull request as ready for review May 21, 2019 21:02
fileSystem,
clock
);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
return format(enclosingClass) + "." + aClass.getSimpleName();
} else {
return aClass.getSimpleName();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
@JLLeitschuh
Copy link
Contributor Author

@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.

Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Signed-off-by: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
@JLLeitschuh
Copy link
Contributor Author

@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
* 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>
* 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 (gradle#2223)" and "Improve test coverage for pmd incremental analysis (gradle#2961)" (gradle#3125)"
  ...
@JLLeitschuh JLLeitschuh requested a review from big-guy June 21, 2019 07:47
* 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())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about IPV6?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@big-guy big-guy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* </a>
* </b>
*/
void allowInsecureProtocol(boolean allowInsecureProtocol);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* master: (146 commits)
  Publish 5.5-20190701010031+0000
  Enable Groovy compilation avoidance for Groovy template project (gradle#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 (gradle#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
Copy link
Contributor Author

@big-guy Looks like just performance failures. 🤔

@ljacomet
Copy link
Member

This was reverted and introduced back as #10065

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@core Issue owned by GBT Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants