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

Add SHA-1 to subresource integrity format for download() checksums #12777

Closed
wants to merge 1 commit into from

Conversation

alexeagle
Copy link
Contributor

npm packages commonly still use SHA-1. While it may be discouraged for its poor security, Bazel cannot enforce what external ecosystems currently do.

I tested this locally against a feature we are working on in rules_nodejs.

npm packages commonly still use SHA-1. While it may be discouraged for its poor security, Bazel cannot enforce what external ecosystems currently do.

Signed-off-by: Alex Eagle <alex.eagle@robinhood.com>
@google-cla google-cla bot added the cla: yes label Jan 6, 2021
@alexeagle
Copy link
Contributor Author

Here's someone who works at NPM confirming that packages published with an older version of the tool always have SHA-1 integrity: https://npm.community/t/sha1-vs-sha512-integrity/3416/2
so there isn't a different fix for this.

@alexeagle
Copy link
Contributor Author

As for testing in this PR, I followed what was here for SHA-384 which is to say, no test case specifically for each format.

@aiuto
Copy link
Contributor

aiuto commented Jan 7, 2021

Aggh..... we explicitly got rid of SHA1 for the obvious insecurity reasons. Perhaps we can find a way to restrict the use to particular upstream respositories, or flag it so that you have to explicitly allow it.

@alexeagle
Copy link
Contributor Author

Hi Tony!

I think it's reasonable for Bazel to be opinionated about not producing any SHA-1 hashes (maybe requiring some flag to opt-in as you say) but I don't understand why it should be opinionated about verifying those produced in other systems.

WDYT?

@aiuto aiuto requested a review from philwo January 14, 2021 05:38
@aiuto
Copy link
Contributor

aiuto commented Jan 14, 2021

I don't remember the specific objections to SHA1. ISTR someone getting a peer bonus for removing it.
@philwo Do you remember the details and have an opinion?

@philwo
Copy link
Member

philwo commented Jan 19, 2021

I also remember that someone complained about Bazel supporting SHA-1. This was the context: #8880

In that issue, the team agreed that supporting SHA-1 is better than not verifying the hash at all and that we should warn about it in the docs. In this case, there's nothing to add to our docs, because we have to take what's coming from upstream sources anyway (e.g. npm would be the place where that notice has to go).

IMHO we should merge this.

@alexeagle
Copy link
Contributor Author

Ping! Need this for a rules_nodejs repository rule improvement

@philwo
Copy link
Member

philwo commented Jan 25, 2021

I'll import this.

@bazel-io bazel-io closed this in c9e2be5 Jan 25, 2021
@alexeagle alexeagle deleted the sha-1 branch January 25, 2021 15:18
philwo pushed a commit that referenced this pull request Apr 19, 2021
npm packages commonly still use SHA-1. While it may be discouraged for its poor security, Bazel cannot enforce what external ecosystems currently do.

I tested this locally against a feature we are working on in rules_nodejs.

Closes #12777.

PiperOrigin-RevId: 353633120
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants