Should patches be cached & have SHA256 verification?

[dlee]

Right now, it seems like only the main download is cached & verified with SHA256. Should the same be done for patches?

[jacknagel]

Not sure about caching, but I think checksumming patches is a good idea.

[adamv]

They probably should, though it will involve rewriting how "DATA/END" patches are handled. I started down this route and things got a little messy so I gave up. But it is certainly doable.

[dlee]

@adamv What would need to be changed? I don't think the DATA patches need to be cached/checksummed.

[dhess]

Is anyone working on the checksum part of this? I think this should be labeled as a security issue, not just a feature request.

[jacknagel]

A while ago I started to work on moving patches into the DSL, but I got busy with other things.

It would maybe look something like this

patch :p0, URL, checksum

and maybe some special cases like

patch :macports, 'revision/path/to/patch', checksum

to get around having really long lines, and might work nicely since all macports patches are -p0 anyway.

I'd like to use a different hash than sha256 though, to keep the lines of reasonable length.

[adamv]

Not keen on the macports special case; I want to keep the lines a reasonable length but also like to be able to copy-paste URLs out of formula easier.

Maybe a brew info --patches thing to show patch URLs if we're going to do shortening.

Would also like to get the options DSL changes in before more DSL changes for patches; will try to push a new options thing for review this week.

[jacknagel]

To further address what you said in the issue you opened, you're right about the Gist-ed patches. They shouldn't have been allowed in in that state. In the short term we could start moving some of them inline if they're not ridiculously huge.

[dhess]

Which hash algorithm to use isn't particularly important at this stage. Even MD5 would be preferable to patching blindly!

A quick fix that would close the obvious holes is to find the brews that apply patches from raw gists and make sure they use the SHA1 URL, not the short, unversioned URL. That's not sufficient (the patch system should require inline checksums and check them on the client side for each download), but it's a start.

[jacknagel]

Which hash algorithm to use isn't particularly important at this stage. Even MD5 would be preferable to patching blindly!

That part was directed at the original feature request.

[jacknagel]

Not keen on the macports special case; I want to keep the lines a reasonable length but also like to be able to copy-paste URLs out of formula easier.

If people are OK with really long lines then I'm fine with not doing any shortening.

[dhess]

Before anyone charges ahead with a patch download DSL, I think the maintainers should revisit the idea of downloading patches, period.

The best-of-breed packaging systems I'm familiar with (e.g., Debian) don't permit any patch downloads at compile time. They store patches in the packaging system. Personally, I think this is a much better system, as it makes the patches easy to review (for both the users and the maintainers), which in turn means it's more likely that someone will actually review the patch before applying or committing it. It also avoids all of the sticky security concerns of relying on servers out of the control of the packagers -- no checksums needed, let git manage that for you.

Speaking as a user who likes to review patches, I'd prefer to see effort spent on a better inline/inclusive patch system. Why not focus on just one robust mechanism?

[jacknagel]

A few months ago I might have agreed but nowadays we rarely accept new patches without ensuring they are submitted upstream first, so going forward patches in Homebrew should be relatively short-lived.

Given that policy, checksumming downloaded patches should be sufficient. We don't need to get into the business of maintaining more patches in the repository.

[dhess]

OK, I understand the rationale, but as a counter-example, there are patches in the Emacs brew that have been there for nearly a year, and still haven't been accepted upstream.

[jacknagel]

If it were today we'd probably be less accepting of the patches in Emacs. But as you said, they've been there for nearly a year and we weren't as anal about sending patches upstream then.

Another thing to keep in mind is that package managers that keep patches in source control usually have some concept of package maintainers, which isn't the case with Homebrew.

[dhess]

Ironically, Homebrew would probably work much better with package maintainers than most of the systems that do: with Debian, for example, it's a lot of work to override a package maintainer's decisions, but with Homebrew, it's trivial! And I think Homebrew would be much improved with package maintainers for certain packages. Anyway, that's a different discussion....

One last point about the lifetime of patches in Homebrew. I completely agree with the sentiment that patches should be short-lived in Homebrew, and I understand the Emacs patches may have been accepted prior to any policy change, but the point stands that while Homebrew can ensure that patches have been submitted upstream, it can't control when or whether they're accepted upstream, let alone when they're released. Not all the packages in Homebrew release on "Internet time." Emacs is lucky if it gets 2 releases in per year.

So while it's a noble idea to say that patches in Homebrew will be short-lived, the maintainers can't predict the future. Some patches will inevitably lie around indefinitely.

[jacknagel]

There are exceptions to every rule. And with checksums we can handle those exceptions.

[dhess]

For the record, when did new policy for accepting patches go into effect?

[jacknagel]

There wasn't really a flag day but I'd say between Thanksgiving and January 1 is when we really started to crack down on it.

[adamv]

Implemented checksums (but not caching) in #11956

[adamv]

Closing in favor of #11956 - note that #11956 is waiting on Jack's specs branch to land first.