composer-patches
composer-patches copied to clipboard
cweagans
Add checksum to patch definition
In addition to the patch title and URL, there should be an optional checksum to verify the file. This is critical for security particularly when applying patches from d.o or other external sources.
Drupal make supported this with md5.
If you're concerned about it, is there any reason you can't just download the patch and keep it locally? That way, you're not depending on external sources and verification isn't necessary.
If this is really 100% necessary, we'll have to find some way to do it that satisfies these requirements:
- No BC break for existing users
- Completely optional
- Doesn't add a massive amount of complexity to the plugin
For the record: I'm not opposed to this, but there are definitely some other things ahead of this on my priority list.
Thank you for your quick response and I fully understand there are other priorities ahead of this. I think it will be required, especially for enterprise usage. I noticed some potential BC issues with the patch definition format so I wanted to make sure it was on the radar.
especially for enterprise usage.
That's actually the reason I ask. We're using it internally @ NBCUniversal, but when we did the work on this plugin, we didn't discuss that particular requirement.
@ericduran: If implemented, is this something we would use?
@cweagans I wrote the patch that added checksum support to drush make. Being able to reference online patches and be confident that they haven't changed is a key requirement for several of the clients I've worked with over the years.
The checksums in drush make have always been optional and were implemented in a non BC breaking way. I am yet to dig into the code, but I think it should be possible to do the same here.
IMO checksums should be used for any patch, even from "trusted" sources. It ensures that nothing has changed since it was added to the composer.json. Maybe it is a small tweak to a gist or it could be something more malicious. Either way users should have the option of this change breaking a build and a human investigating it.
Given the current state of cryptographic standards I propose that we use SHA-256 checksums.
@skwashd Like I said, I'm not opposed to adding it, but it's not a high priority for me right now. If a PR is opened that satisfied the criteria in https://github.com/cweagans/composer-patches/issues/19#issuecomment-151601734, I'll happily review and merge.
(Also, yes - not md5 please. SHA-1 or SHA-256 are both fine. SHA-1 is okay in this case since it's more of a consistency check than a security feature - see https://en.wikipedia.org/wiki/SHA-1#Data_integrity for related discussion from Linus)
About MD5 versus SHA-*: What if we allow composer.json authors to specify the used algorithm? This plugin can support any algorithm that PHP supports, and then hashes can easily be upgraded/changed to use different algorithms without breaking BC in the future.
First off, let me just rule out MD5. That's not going to happen in this plugin under any circumstances.
I would not be interested in adding a flag to change the hashing scheme. If we were going to support alternate hashing schemes (which I don't think we should - more on this in a moment), I'd rather just auto-detect the scheme in use from the hash, rather than having composer.json authors manually set it.
That said, I think it's okay to be somewhat opinionated in this plugin as a way to avoid code cruft. SHA-1 is very commonly used for download verification, so I don't have a problem just using that for now, and then in the future, if everyone starts using SHA-256, we can revisit this. Changing the hashing scheme and tagging a new major release is not so bad, especially if we tag a new minor release first that adds a way to rehash everything (i.e. composer patches-hash-upgrade && composer update cweagans/composer-patches would be the upgrade path). SHA-1 vs SHA-256, I don't care so much. SHA-256 is probably a bit more forward-thinking, but as long as we just pick one and stick with it until the industry moves in a different direction, I don't think it will be an overly contentious issue.
I was thinking about picking this one up for different reasons than enterprise level security (thought its a nice benefit).
Occasionally, we have to patch core or a module in a way that doesnt make sense as a new plugin override, etc. If it is specific to that project we would like to store the patch in the repo along side the project (/patches folder) but composer-patches doesn't detect changes to patch file contents (only changes to the patch location are detected currently). As a result the patch can be changed locally and not get applied then our CI system gets it, applies it and things go south from there.
I first considered caching the SHA1 of the patches on last application and testing against the current values to determine if we need to reapply but then I realized that it would require fetching the patches on every "composer update". Which is already slow enough in my life to be adding to the latency problem there.
My solution was to implement this functionality which requires a little manual work fromt he DX side but ensure the same result.
@cweagans do you have a preference here for upstream and I'll get to work?
This is on the roadmap for 2.0.0. If you'd still like to work on it, master is the place! I think #212 needs to go in first though.
This is now in main - you can manually specify a sha256 for an individual patch or you can not specify the hash and it will be calculated and written to a lock file.