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

incompatible_remove_native_maven_jar #6799

Closed
dkelmer opened this issue Nov 29, 2018 · 33 comments
Closed

incompatible_remove_native_maven_jar #6799

dkelmer opened this issue Nov 29, 2018 · 33 comments

Comments

@dkelmer
Copy link
Contributor

@dkelmer dkelmer commented Nov 29, 2018

The native maven_jar rule will no longer be available with this flag set to true.


(Update: managed by @jin as of July 12)

A migration tool will be provided to automatically convert maven_jar targets to rules_jvm_external's maven_install rule, and modify BUILD files to use the new labels.

See https://docs.google.com/document/d/1CqxPv-TwvFWBHgtg7_QhVfck4vH4rnmCU_IuMdC7O2M/edit# for a design document of this migration tool.


Proposal by @dkelmer as of Nov 28, which is another valid migration solution that is done manually:

Use

load("@bazel_tools//tools/build_defs/repo:java.bzl", "java_import_external")

or the convenience wrapper

load("@bazel_tools//tools/build_defs/repo:jvm.bzl", "jvm_maven_import_external")

instead.

Given a WORKSPACE file that looks like the following:

maven_jar(
    name = "truth",
    artifact = "com.google.truth:truth:0.30",
    sha1 = "9d591b5a66eda81f0b88cf1c748ab8853d99b18b",
)

It will need to look like this after updating:

load("@bazel_tools//tools/build_defs/repo:jvm.bzl", "jvm_maven_import_external")
jvm_maven_import_external(
    name = "truth",
    artifact = "com.google.truth:truth:0.30",
    artifact_sha256 = "59721f0805e223d84b90677887d9ff567dc534d7c502ca903c0c2b17f05c116a",
    server_urls = ["http://central.maven.org/maven2"],
    licenses = ["notice"],  # Apache 2.0
)

Notably

  • the licenses attribute is mandatory
  • sha1 is no longer supported, only sha256 is
  • the server_urls attribute is mandatory. If your maven_jar rule did not specify a url
    then you should use the default server ("http://central.maven.org/maven2"). If your rule
    did specify a url then keep using that one.
bazel-io pushed a commit that referenced this issue Nov 29, 2018
Closes #6768.
Relevant to #6799.

PiperOrigin-RevId: 223375698
@meteorcloudy meteorcloudy changed the title --incompatible_remove_native_maven_jar incompatible_remove_native_maven_jar Jan 2, 2019
@meteorcloudy

This comment has been minimized.

Copy link
Member

@meteorcloudy meteorcloudy commented Jan 3, 2019

This flag will break many projects in downstream, you can see the details at
https://buildkite.com/bazel/bazel-at-release-plus-incompatible-flags/builds/48

Please help them migrate (by filing issues and sending PRs and pinging the owners), thanks!

@dkelmer

This comment has been minimized.

Copy link
Contributor Author

@dkelmer dkelmer commented Jan 16, 2019

Broken projects:

@meisterT

This comment has been minimized.

Copy link
Member

@meisterT meisterT commented Feb 3, 2019

cc @vladmos this is a classical example where buildifier could help with fixing the projects, because it's a mostly mechanical change

Danna: it would be great if we could prioritize this migration, because the maven rules are one of the two "users" of the java.desktop module which contributes to 1/3 of the embedded JDK size.

@meisterT meisterT added this to the Shrinking the bazel binary milestone Feb 3, 2019
dkelmer added a commit to dkelmer/rules_gwt that referenced this issue Feb 6, 2019
See bazelbuild/bazel#6799 for more details
@dkelmer

This comment has been minimized.

Copy link
Contributor Author

@dkelmer dkelmer commented Feb 6, 2019

merged: bazelbuild/rules_gwt#20
merged: bazelbuild/rules_appengine#96
merged: https://critique.corp.google.com/#review/229611659&tab=a (intellij, SOT is internal)
merged: grpc/grpc-java#5327
merged: android/testing-samples#245
filed issue: bazelbuild/BUILD_file_generator#48 (they use bazel-deps and updating requires updating their custom scripts so I'll leave that to them)

rules_k8s is breaking because of grpc, so marking it as fixed

ekuefler added a commit to bazelbuild/rules_gwt that referenced this issue Feb 7, 2019
See bazelbuild/bazel#6799 for more details
@dkelmer dkelmer self-assigned this Feb 7, 2019
dkelmer added a commit to dkelmer/android-testing that referenced this issue Feb 7, 2019
See bazelbuild/bazel#6799 for more details
@meteorcloudy

This comment has been minimized.

Copy link
Member

@meteorcloudy meteorcloudy commented Feb 12, 2019

Since this flag is still not flipped in 0.22, 0.22 should still be its migration window.

@guw

This comment has been minimized.

Copy link

@guw guw commented Feb 15, 2019

Going through the examples above, how would one specify a Maven server which requires authentication?

The native.maven_rule has a server attribute which takes a name of a server. Authentication credentials for such a server would be specified in ~/.m2/settings.xml. The location of the settings file was configurable with native.maven_server rule.

@dkelmer

This comment has been minimized.

Copy link
Contributor Author

@dkelmer dkelmer commented Feb 21, 2019

Thanks for bringing this up, the native maven_jar rule will not be deprecated before there is support for authentication with a local maven server

@meisterT meisterT mentioned this issue Feb 22, 2019
4 of 11 tasks complete
cloud-robotics-github-robot pushed a commit to googlecloudrobotics/core that referenced this issue Feb 22, 2019
This removes our dependency on the deprecated native maven_jar rule:
bazelbuild/bazel#6799

All changes except maven_dependencies.yaml are generated by the tool.

At the same time, remove missing-jar-workaround.sh, which I haven't
needed for some time.

Change-Id: I22e15e340d98183156ef5942d0a11f4a759c6baa
GitOrigin-RevId: 525db4983b47ac2b139a61843e68fde02e3612fb
@dapengzhang0

This comment has been minimized.

Copy link
Contributor

@dapengzhang0 dapengzhang0 commented Mar 5, 2019

@dkelmer How did you get the sha256 values? As the default server ("http://central.maven.org/maven2") only provides sh1. Is there an automated tool that can get sha256 values for a lot of dependent maven jars?

@dkelmer

This comment has been minimized.

Copy link
Contributor Author

@dkelmer dkelmer commented Mar 5, 2019

@dapengzhang0 There is not an automated tool. You need to independently fetch the jars and calculate the sha256. One thing you can do is omit the artifact_sha256 attribute and run bazel fetch. Bazel will then tell you what sha it expected for each jar.

@jin

This comment has been minimized.

Copy link
Member

@jin jin commented Sep 3, 2019

I have verified with the security team that the referenced remediation approaches for the SHA-1 issue are sufficient.

  1. The docs for the maven_jar rule have been updated to discourage use of sha1, and use sha256 instead. https://docs.bazel.build/versions/master/be/workspace.html#maven_jar

"A SHA-1 hash of the desired jar.

If the downloaded jar does not match this hash, Bazel will error out. It is a security risk to download a file without verifying cryptographic secure hash. Note that SHA-1 is no longer considered a secure cryptographic hash function, but specifying the hash is still marginally better than no check at all. This attribute is kept here for legacy support purposes. Please either use the 'sha256' attribute or migrate to rules_jvm_external and pin your Maven artifacts with their SHA-256 checksums."

  1. As in 1), we added sha256 attributes to the maven_jar rule in the next Bazel release (1.0). We are also actively recommending usage of sha256 if we detect if no checksum or sha1 is used. #9237

Sample warning message: "WARNING: /usr/local/google/home/jingwen/code/copybara/WORKSPACE:192:1: maven_jar rule @junit//jar: Not using a checksum to verify the integrity of the artifact or the usage of SHA-1 is not secure (see https://shattered.io) and can result in an non-reproducible build. Please specify the SHA-256 checksum with: sha256 = "90a8e1603eeca48e7e879f3afbc9560715322985f39a274f6f6070b43f9d06fe","

@JLLeitschuh Google is a CNA only for Chrome/Chromium issues.

@JLLeitschuh

This comment has been minimized.

Copy link

@JLLeitschuh JLLeitschuh commented Sep 4, 2019

+1 on the mention of shattered in your warning message.

Do you want me to move forward with the CVE issuance process via MITRE or will you?

If you want me to do it, I need to know the first 'fixed' version as well as the commit hash/hashes or PR that fixes this issue.

Some context here: I'm actually not a Bazel user, I do work for Gradle though & in my free time I focus my software security research on build infrastructure and the Java supply chain.

@JLLeitschuh

This comment has been minimized.

Copy link

@JLLeitschuh JLLeitschuh commented Sep 12, 2019

Do you want me to move forward with the CVE issuance process via MITRE or will you?

Ping: @jin

@jin

This comment has been minimized.

Copy link
Member

@jin jin commented Sep 12, 2019

Here are the commits that Google Security have verified to be sufficient as remediation:

bc9ff0c
06d79dd
b065b13

Please go ahead with the issuance.

@JLLeitschuh

This comment has been minimized.

Copy link

@JLLeitschuh JLLeitschuh commented Sep 12, 2019

@jin Can you confirm the first fixed version of Bazel for the CVE disclosure?

@jin

This comment has been minimized.

Copy link
Member

@jin jin commented Sep 12, 2019

@JLLeitschuh

This comment has been minimized.

Copy link

@JLLeitschuh JLLeitschuh commented Sep 24, 2019

Hey @jin unless Google plans to make an official disclosure for this, the CVE to use is the generic one for SHA-1 https://nvd.nist.gov/vuln/detail/CVE-2005-4900

@jin

This comment has been minimized.

Copy link
Member

@jin jin commented Oct 30, 2019

Please see this document for a design proposal for a migration tool from maven_jar to rules_jvm_external.

@JLLeitschuh

This comment has been minimized.

Copy link

@JLLeitschuh JLLeitschuh commented Oct 31, 2019

Thanks! Cool to see progress on this!

@jin

This comment has been minimized.

Copy link
Member

@jin jin commented Nov 14, 2019

The automated migration tool is available here: https://github.com/bazelbuild/rules_jvm_external/tree/master/migration

If you don't wish to migrate to rules_jvm_external, please follow the alternative instructions to manually migrate to jvm_maven_import_external.

@bazel-io bazel-io closed this in 489a9e5 Nov 14, 2019
laszlocsomor added a commit to laszlocsomor/rules_scala that referenced this issue Nov 28, 2019
laszlocsomor added a commit to laszlocsomor/rules_scala that referenced this issue Nov 28, 2019
bazel-io pushed a commit that referenced this issue Dec 19, 2019
Baseline: 807ed23

Cherry picks:

   + db0e32c:
     build.sh: Fix bug in build script for RC release
   + 85e84f7:
     Set --incompatible_prohibit_aapt1 default to true.
   + 84eae2f:
     Let shellzelisk fallback to bazel-real if it's the requested
     version.
   + d5ae460:
     Fix a typo in bazel.sh

Incompatible changes:

  - --incompatible_remap_main_repo is enabled by default. Therefore,
    both ways of addressing the main repository, by its name and by
    '@' are now considered referring to the same repository.
    see #7130
  - --incompatible_disallow_dict_lookup_unhashable_keys is enabled by
    default #9184
  - --incompatible_remove_native_maven_jar is now enabled by default
    and the flag removed. See #6799
  - --incompatible_prohibit_aapt1 is enabled by default.
    See #10000

Important changes:

  - --incompatible_proto_output_v2: proto v2 for aquery proto output
    formats, which reduces the output size compared to v1. Note that
    the messages' ids in v2 are in uint64 instead of string like in
    v1.
  - Adds --incompatible_remove_enabled_toolchain_types.
  - Package loading now consistently fails if package loading had a
    glob evaluation that encountered a symlink cycle or symlink
    infinite expansion. Previously, such package loading with such
    glob evaluations would fail only in some cases.
  - The --disk_cache flag can now also be used together
    with the gRPC remote cache.
  - An action's discover inputs runtime metrics is now categorized as
    parse time on the CriticalPathComponent.
  - Make the formatting example more like to the written text by
    adding an initial description.
  - An action's discover inputs runtime metrics is now categorized as
    parse time on the CriticalPathComponent.
  - Bazel's Debian package and the binary installer now include an
    improved wrapper that understands `<WORKSPACE>/.bazelversion`
    files and the `$USE_BAZEL_VERSION` environment variable. This is
    similar to what Bazelisk offers
    (https://github.com/bazelbuild/bazelisk#how-does-bazelisk-know-whi
    ch-bazel-version-to-run-and-where-to-get-it-from), except that it
    works offline and integrates with apt-get.
  - We are planning to deprecate the runfiles manifest files, which
    aren't safe in the presence of whitespace, and also unnecessarily
    require local CPU when remote execution is used. This release
    adds --experimental_skip_runfiles_manifests to disable the
    generation of the input manifests (rule.manifest files) in most
    cases. Note that this flag has no effect on Windows by default or
    if --experimental_enable_runfiles is explicitly set to false.

This release contains contributions from many people at Google, as well as aldersondrive, Benjamin Peterson, Bor Kae Hwang, David Ostrovsky, Jakob Buchgraber, Jin, John Millikin, Keith Smiley, Lauri Peltonen, nikola-sh, Peter Mounce, Tony Hsu.
scode added a commit to scode/bazelplay that referenced this issue Dec 28, 2019
This is no longer supported in bazel 2:

bazelbuild/bazel#6799
meteorcloudy pushed a commit to meteorcloudy/bazel that referenced this issue Jan 10, 2020
Baseline: 807ed23

Cherry picks:

   + db0e32c:
     build.sh: Fix bug in build script for RC release
   + 85e84f7:
     Set --incompatible_prohibit_aapt1 default to true.
   + 84eae2f:
     Let shellzelisk fallback to bazel-real if it's the requested
     version.
   + d5ae460:
     Fix a typo in bazel.sh

Incompatible changes:

  - --incompatible_remap_main_repo is enabled by default. Therefore,
    both ways of addressing the main repository, by its name and by
    '@' are now considered referring to the same repository.
    see bazelbuild#7130
  - --incompatible_disallow_dict_lookup_unhashable_keys is enabled by
    default bazelbuild#9184
  - --incompatible_remove_native_maven_jar is now enabled by default
    and the flag removed. See bazelbuild#6799
  - --incompatible_prohibit_aapt1 is enabled by default.
    See bazelbuild#10000

Important changes:

  - --incompatible_proto_output_v2: proto v2 for aquery proto output
    formats, which reduces the output size compared to v1. Note that
    the messages' ids in v2 are in uint64 instead of string like in
    v1.
  - Adds --incompatible_remove_enabled_toolchain_types.
  - Package loading now consistently fails if package loading had a
    glob evaluation that encountered a symlink cycle or symlink
    infinite expansion. Previously, such package loading with such
    glob evaluations would fail only in some cases.
  - The --disk_cache flag can now also be used together
    with the gRPC remote cache.
  - An action's discover inputs runtime metrics is now categorized as
    parse time on the CriticalPathComponent.
  - Make the formatting example more like to the written text by
    adding an initial description.
  - An action's discover inputs runtime metrics is now categorized as
    parse time on the CriticalPathComponent.
  - Bazel's Debian package and the binary installer now include an
    improved wrapper that understands `<WORKSPACE>/.bazelversion`
    files and the `$USE_BAZEL_VERSION` environment variable. This is
    similar to what Bazelisk offers
    (https://github.com/bazelbuild/bazelisk#how-does-bazelisk-know-whi
    ch-bazel-version-to-run-and-where-to-get-it-from), except that it
    works offline and integrates with apt-get.
  - We are planning to deprecate the runfiles manifest files, which
    aren't safe in the presence of whitespace, and also unnecessarily
    require local CPU when remote execution is used. This release
    adds --experimental_skip_runfiles_manifests to disable the
    generation of the input manifests (rule.manifest files) in most
    cases. Note that this flag has no effect on Windows by default or
    if --experimental_enable_runfiles is explicitly set to false.

This release contains contributions from many people at Google, as well as aldersondrive, Benjamin Peterson, Bor Kae Hwang, David Ostrovsky, Jakob Buchgraber, Jin, John Millikin, Keith Smiley, Lauri Peltonen, nikola-sh, Peter Mounce, Tony Hsu.
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.