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

dev-cmd/audit: add audit for checksum #9471

Merged
merged 1 commit into from
Dec 10, 2020

Conversation

bayandin
Copy link
Member

@bayandin bayandin commented Dec 8, 2020

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?
  • Have you successfully run brew man locally and committed any changes?

This PR make checksum mandatory for all curl downloadable resources to prevent cases like Homebrew/homebrew-core#66471 (comment) / Homebrew/homebrew-core#66481

@BrewTestBot
Copy link
Member

Review period will end on 2020-12-09 at 22:53:17 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 8, 2020
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 10, 2020
@BrewTestBot
Copy link
Member

Review period ended.

@Rylan12
Copy link
Member

Rylan12 commented Dec 10, 2020

Just a thought: do we want this to be enabled only for homebrew/core or for third-party taps as well?

I ask only because there have been a few instances in the past where audit/style changes have broke third-party taps in a way that isn't necessary.

Pros of restricting to homebrew/core:

  • Gives third-party taps the ability to choose whether this requirement applies to their tap or not
  • No change for third-party taps

Cons:

  • I think that including the sha256 is a good idea anyway so, of all the things we could "force" third-party taps into doing, this seems totally reasonable
  • Potential for causing issues where audit now fails for taps

I'm leaning toward not-restricting. I think this is such a fundamental part of the formula that it totally makes sense to require it for everyone (and those that don't want it for their taps can choose to ignore the audit). Just figured I'd raise the issue as we've (or, rather: I've) had a few issues with this in the past and I just want to make sure that we're being mindful of how our internal decisions can affect third-party taps.

@bayandin
Copy link
Member Author

bayandin commented Dec 10, 2020

Good point @Rylan12, my initial thoughts about it were that since it's an audit, it shouldn't break the main use case brew install <formula>, it's possible to skip (with a bunch of other checks, but still) using brew audit --except=specs <formula> so it's ok to have it enabled by default for all taps.

Nevertheless, I can imagine some terribly insecure case when a third-party tap has bottle :unneeded-formula where it always points to the latest version (like https://github.com/User/repo/archive/master.tar.gz). But we shouldn't encourage such examples I believe.

@MikeMcQuaid
Copy link
Member

I'm leaning toward not-restricting. I think this is such a fundamental part of the formula that it totally makes sense to require it for everyone (and those that don't want it for their taps can choose to ignore the audit).

Agreed, it's also a security risk not having it there so I'd say having a brew audit complain is pretty reasonable.

But we shouldn't encourage such examples I believe.

👍

@MikeMcQuaid MikeMcQuaid merged commit 96218ec into Homebrew:master Dec 10, 2020
@MikeMcQuaid
Copy link
Member

Thanks @bayandin!

@bayandin bayandin deleted the check-checksum branch December 10, 2020 13:37
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 10, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants