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

Change the template hash function to SHA-256. #2719

Merged
merged 1 commit into from Feb 8, 2014

Conversation

lgarron
Copy link
Sponsor Contributor

@lgarron lgarron commented Jan 31, 2014

SHA-1 is slowly losing its footing, and SHA-256 is a better choice going forward. (SHA3/Keccak is still not finalized.)

SHA-256 is already supported by homebrew-cask in exactly the same way as SHA-1. In fact, my submitted casks have been using SHA-256.

This pull request doesn't break any functionality (casks with SHA-1 hashes will continue to work), but the template from brew cask create will now have a field for SHA-256 by default.

@goxberry
Copy link
Contributor

The technical idea is sound. Could the change from SHA-1 to SHA-256 be more of a staged rollout? (i.e., migrate a good chunk of the existing casks to SHA-256 so that people who look at code examples see SHA-256, add a deprecation warning, then upgrade the docs and the template?) How did Homebrew handle the obsolescence of MD5?

For the CONTRIBUTING.md, I'd suggest more comprehensive edits, like:

  • change the code examples from sha1 to sha256
  • migrate the casks corresponding to those code examples from sha1 to sha256
  • keep the sha1 field in the cask metadata explanation; add a deprecation warning
  • rephrase the part about determining hashes in "Good Things to Know" so it puts more emphasis on finding SHA-256 hashes than SHA-1 hashes; eventually put less emphasis on SHA-1
  • possibly put something into the template that lets users know we're in a transition period

I think making the sort of changes you're making in the docs and in the cask template could confuse users. The code examples in CONTRIBUTING.md still use sha1, by and large, so anyone skimming the document and pattern-matching is going to look at a code example, and then back at the default template, and wonder where sha1 is. People who pattern match from source code could also be thrown for a loop. I think a close reading would resolve any confusion. I'd prefer to make the docs so obvious that a close reading is unnecessary, making it easier for our core maintainers to merge things and focus on adding cool features.

@muescha
Copy link
Contributor

muescha commented Jan 31, 2014

i think it is less confusing if we do this change it in one step - not in a transition period. (if its scriptable to get all ~700 SHA1 changed)

@goxberry
Copy link
Contributor

@muescha For new contributors, definitely. As an existing contributor, I'm going to have to break old habits, even though I know perfectly well how to get a SHA-256 hash (and I wrote the blurb in CONTRIBUTING.md on how to do that). It would be helpful to have a more explicit deprecation of SHA-1 hashes than this commit and discussion. Implicit deprecation does not suffice; without the commit message, I would continue to submit casks with SHA-1 hashes until someone pointed out the deprecation, and then I would add a deprecation warning to the docs anyway in a PR.

Scripting is probably the most practical way to migrate, even though the premise of this changeset suggests that perhaps we shouldn't completely trust the SHA-1 hashes we currently have, despite the lack of practical attacks on SHA-1.

@lgarron
Copy link
Sponsor Contributor Author

lgarron commented Jan 31, 2014

To be clear, I wasn't suggesting deprecating SHA-1 in homebrew-cask. For example, if SHA-1 is actually broken at some point (e.g. collisions become as easy as MD5) then we would have bigger problems – git itself uses SHA-1 for integrity.

Although git can't change at this point, new applications might as well use SHA-256 where possible, and homebrew-cask has an easy (gradual) migration strategy.

If it would be more desirable for all casks to use the same hash, I don't object. I just thought suggesting it as the default for new casks was the right starting place for a pull request.

@rolandwalker
Copy link
Contributor

👍 on the merits. Checksums have a purpose. Weaker hashes are less able to meet that purpose.

But there's no need to change the Casks all at once. This patch is the right approach -- just change the docs, removing mentions of sha1. For consistency, it should also change FAQ.md and the "good things to know" section along the same lines.

We could (if it is agreed) deprecate sha1 in some future release. Then (possibly) remove them in another, later release. Smooth transitions are good. Nothing wrong with @goxberry continuing to submit SHA1s. Also, nothing wrong with @lgarron planning for the future.

@lgarron
Copy link
Sponsor Contributor Author

lgarron commented Jan 31, 2014

Amended the commit to include a change in "Good things to Know".

@goxberry
Copy link
Contributor

@rolandwalker @lgarron I agree with the change on the merits, too. I don't think migrating all casks now is necessary, and I think that planning for the future is a good thing. I am all for new casks using SHA-256. Since I like the change and want to see it succeed with a minimum of issues, I'm interested in making the transition clear, which is why I'm advocating for the sake of consistency that the code examples in CONTRIBUTING.md also be changed (along with their corresponding casks). I would also prefer that we note more explicitly that there is going to be a transition because the defaults are changing. I don't object if we don't make that transition explicit because I'll adjust.

@lgarron
Copy link
Sponsor Contributor Author

lgarron commented Feb 5, 2014

I've updated the top of CONTRIBUTING.md to use sha256 examples, and updated the corresponding casks themselves (Alfred, Vagrant) to be consistent.

Does anyone still feel this should be documented more explicitly? I think the current PR is relatively clear about things.

@goxberry
Copy link
Contributor

goxberry commented Feb 5, 2014

@lgarron Looks good to me. Thanks for editing for consistency!

@rolandwalker
Copy link
Contributor

@phinze I suppose you should weigh in with a ruling.

@phinze
Copy link
Contributor

phinze commented Feb 8, 2014

👑 great discussion and solid conclusions.

let's start the process! 🚀

phinze added a commit that referenced this pull request Feb 8, 2014
Change the template hash function to SHA-256.
@phinze phinze merged commit 1f5cc4e into Homebrew:master Feb 8, 2014
lgarron pushed a commit to lgarron/homebrew-cask that referenced this pull request Feb 9, 2014
@lgarron lgarron deleted the sha256 branch February 11, 2014 04:56
rolandwalker added a commit to rolandwalker/homebrew-cask that referenced this pull request Feb 15, 2014
Following up on Homebrew#2719.  MD5 checksums are
* nowhere mentioned in our docs
* not used in any existing Cask
* deprecated in Homebrew
Therefore it seems sensible to delete this code.
@alebcay alebcay mentioned this pull request Mar 4, 2014
30 tasks
@Homebrew Homebrew locked and limited conversation to collaborators May 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants