-
Notifications
You must be signed in to change notification settings - Fork 985
Enable dependency checksum verification #4251
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
Conversation
d8fd144 to
3fb3fb4
Compare
6f71a22 to
cb9084d
Compare
|
Generally speaking, I really like this. Some ideas (not for this PR)
I'll click approve later in the week, I want to give the consensys maintainers a few business days to digest this. |
|
Hi @shemnon, I tried commenting out an entry from the |
|
Generally speaking, "I'm for it" and the following questions should not be considered blocking adoption.
|
c93c592 to
20c1280
Compare
|
@jflo I'm sorry for the late reply. Allow me to go through some of your comments
Regarding the supply chain protection of checksums, it's true that an attacker can compromise the original server, but this mechanism can protect other layers used for building, like proxies or tainted local caches.
Regarding the overhead, for checksums verification this is negligible. For signatures, Gradle by default downloads all the required keys which certainly adds time to the build. One option could be to distribute a local keyring, with the potential security implications that this would carry. What is good is that verification can be disabled but it is not so granular i.e. you cannot disable just signatures and let the checksums run. By the time we add signatures, we can analyze if it makes sense to disable all verification by default and only make this strict on CI. |
|
Thanks for the responses Diego, if you can rebase this onto main maybe we can get it into the next RC. |
d7e4e08 to
ac4dc4f
Compare
ac4dc4f to
b018845
Compare
b018845 to
ec7b5b0
Compare
|
@macfarla not for me. Let me rebase it just to see if there has been some new dependency |
e554815 to
d915e32
Compare
This file was automatically generated using `./gradlew --write-verification-metadata sha256 help` Signed-off-by: Diego López León <[email protected]> Signed-off-by: Diego López León <[email protected]> Signed-off-by: Diego López León <[email protected]>
Signed-off-by: Diego López León <[email protected]> Signed-off-by: Diego López León <[email protected]> Signed-off-by: Diego López León <[email protected]>
e60984b to
c71577f
Compare
* Initial checksums This file was automatically generated using `./gradlew --write-verification-metadata sha256 help` Signed-off-by: Diego López León <[email protected]> * Add missing checksums for artifacts Co-authored-by: Sally MacFarlane <[email protected]> Signed-off-by: Sally MacFarlane <[email protected]>
* Initial checksums This file was automatically generated using `./gradlew --write-verification-metadata sha256 help` Signed-off-by: Diego López León <[email protected]> * Add missing checksums for artifacts Co-authored-by: Sally MacFarlane <[email protected]>
PR description
This PR enables dependency verification using a mechanism introduced in Gradle 6.2.
The
gradle/verification-metadata.xmlfile was autogenerated relying on the TOFU scheme.HEAD~1: adds auto generated sha-256 checksumsHEAD: adds missing sha-256 checksums manuallyThe proposal lets pgp signatures out of scope just to check if we feel confortable with this verification step.
Accepting this PR means that every time a dependency is upgraded, this file needs to be updated too. I think this is a positive thing, since it will make the dev more aware of what the upgrade really implies dependecy wise.
Everytime you update a dependency,
./gradlew --write-verification-metadata sha256 + tasks_to_executemust be called in order to update theverification-metadata.xmlfile, but this command will only add entries, so the developer should be careful to remove the replaced entry to prevent perpetual grow of the file (more info in the official documentation).Documentation
doc-change-requiredlabel to this PR ifupdates are required.
Changelog