composer-patches
composer-patches copied to clipboard
cweagans
Add support for patch checksums
As the status of version 2 is not clear to me, this is a PR against 1.x instead of the master branch. It would be nice if we could have this feature in version 1.
This PR is based upon #163 . That PR no longer matches. Biggest difference is that this PR keeps using the key as description. Also instead of hash, the term checksum is used.
As I said in https://github.com/cweagans/composer-patches/issues/347#issuecomment-1004603765 I am really happy that there is some movement around this topic, so thanks for this PR.
What I like about this approach that it is simple and straightforward. What I do not like that it is opt-in feature and security must not be optional. So I started to think about how we can have this security feature enabled by default.
We have two significantly different scenarios, when a patch is installed from a local source or from a remote URL. We only have problems with remote patch sources because in general, we cannot trust that the content of a patch does not change on a remote. If a patch is served from the local filesystem that was potentially downloaded and verified by a human, hopefully also passed on a code review and it is part of a VCS repo. Why do I think it is essential to differentiate these two scenarios? The reason is that if hash based protection is enabled by default and it would also be applied on local patches, then I think it could become a bottleneck because the content of those patch files can rightfully change anytime and no additional steps (like running composer update-composer-patch-hash ./path-to.patch) should be required for updating a local patch file.
So if we would like to auto-generate hashes (with a preferred algorithm) of newly added remote patches, how can we do that?
When I saw these manually generated hashes, it made me remember again that with Composer 1, all applied patches were listed in composer.lock also. I am not sure how and why that changed with Composer 2 but my opinion is that auto-generated hashes should be in composer.lock, just like hashes of downloaded packages.
When a new patch is added and composer install or composer update is executed, then Composer Patches plugin could generate hashes for remote patches that did have hashes before. This also implies that the URL of a remote patch is considered unique, which was avoided here as a design decision, but is it a good idea? When a diff of a Gitlab MR is referenced and the MR changes what we want is make the build failed because it is a mutable (non-static) reference.
In which circumstances we need hashes in composer.json? I can imagine one common scenario when patches are distributed via libraries/modules (example 1) by leveraging the enable-patching: true feature of Composer Patches (which is another open door for supply chain attackes). In that scenario, the package provider probably also would like define a hash for every remote patch "suggested" by the library so whenever patches are installed on a project, Composer Patches could verify that the content of a proposed patch is the same as it was when it had been added.
What are others thinking about these ideas?
(Possibly the easiest and safest option would be only supporting local patches :grin: )
Thank you for opening this -- I'm swamped but will review ASAP. Thank you @gapple for the original PR as well.
One (hopefully) quick thing: let's just pick a hashing algo and only support that one. sha256 seems like a good candidate, but I'm open to other options. I'd prefer to reduce the number of possible configurations + limit the scope of this change as much as possible.
I agree on limiting the scope as much as possible; refactored the PR so it only supports sha256 checksums.
This really would be awesome to get moving. This would be an awesome step forward, even though there is probable next steps to make this even more automated.
@cweagans, haven't found the time yet, or is something regarding the PR holding you back?
This is an attempt to push this pull request :)
