Add cache validation - #625
Conversation
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
cert-manager-prow
Bot
added
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: inteon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cert-manager-prow
Bot
added
the
approved
| ## restore in CI environments where the cache write-side is not fully trusted | ||
| ## (e.g. node-local hostPath shared with low-trust presubmit jobs). | ||
| ## @category [shared] Tools | ||
| verify-cache: |
There was a problem hiding this comment.
Does it mean we need to run make verify-cache in every single Prow job? I imagine that's not a big downside, but I wish it was a bit more automatic
There was a problem hiding this comment.
Yes, I'll update the prowjob definitions to first call make verify-cache.
|
at first, I didn't believe it as I thought that the inode modification timestamp mismatch would prevent the attack. And I thought that the report was wrong as it mentioned poisonning I realized that I was wrong. $ echo $'#!/bin/bash\necho I GOT YOU>&2'> ~/.cache/makefile-modules/downloaded/tools/kind@v0.31.0_darwin_arm64 && chmod +x ~/.cache/makefile-modules/downloaded/tools/kind@v0.31.0_darwin_arm64
$ make _bin/tools/kind
$ _bin/tools/kind
I GOT YOUI'm surprised that we aren't verifying the hash when creating the symlink. Right now, we verify after downloading and then never verify again. And since some artifacts are large tarballs, we would need check their tarballs' hash and untar them just before creating the symlink. My recommendation would be exactly this: instead of uncompressing + checking hash on download, I'd uncompress + check hash upon creating the symlink. This would 100% mitigate the issue IMO. Quick proof of concept for verifying hashes at runtime instead of at the time of download: cert-manager/cert-manager#8833 |
|
Thanks @inteon — this has grown into the right shape: hash-check every restored file against the reviewed SHA-256 in Two thoughts:
Net: I'd like the tool-cache half of this to land here in makefile-modules, folding in cert-manager/cert-manager#8833's "verify at point of use, not just at download" insight rather than running it as a separate command. Happy to help push it over the line. with claude fable-5 |
The download directory ($(DOWNLOAD_DIR)) is persisted between CI runs, and for some repositories it is a node-local directory shared with less-trusted jobs that can overwrite a cached binary in place. Until now a tool's SHA-256 was only checked when it was first downloaded, so a binary swapped in the cache afterwards was linked onto PATH and executed unverified. Make the per-tool symlink a .PHONY target and re-hash the cached binary against the reviewed SHA-256 in this file on every build, before linking. A mismatch deletes the binary and re-downloads it, so a poisoned cache cannot be used. The reviewed hash is the trust anchor, so no signing or fork-write-scoping is needed. For this the reviewed SHA must be the hash of the extracted binary, not the downloaded archive: the archive recipes now hash the binary after extraction (the download still fails closed via the lock script if the hash is wrong), etcd and kube-apiserver gain their own binary hashes, and the SHAs for the affected tools have been relearned with "make learn-tools-shas". Tools built from source with "go install" have no reviewed hash here; they are anchored by go.sum/GOSUMDB when built and their staleness is already handled by keying the download path on the Go toolchain version, so the check skips them (empty hash variables are defined to keep --warn-undefined-variables quiet). Stacked on cert-manager#708. Supersedes cert-manager#625 (whole-cache purge run as a separate step you must remember) and cert-manager/cert-manager#8833 (verify at symlink-creation), folding both into cert-manager#708's tool_link_defs seam so the check is automatic and cannot be bypassed by a job that forgets to run it. Known limitation: the vendored-Go tarball is still verified at download only; the shared PATH is process-wide, so this closes cache poisoning, not the separate problem of a target using an undeclared tool. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Richard Wall <richard@the-moon.net>
The download directory ($(DOWNLOAD_DIR)) is persisted between CI runs, and for some repositories it is a node-local directory shared with less-trusted jobs that can overwrite a cached binary in place. Until now a tool's SHA-256 was only checked when it was first downloaded, so a binary swapped in the cache afterwards was linked onto PATH and executed unverified. Make the per-tool symlink a .PHONY target and re-hash the cached binary against the reviewed SHA-256 in this file on every build, before linking. A mismatch deletes the binary and re-downloads it, so a poisoned cache cannot be used. The reviewed hash is the trust anchor, so no signing or fork-write-scoping is needed. For this the reviewed SHA must be the hash of the extracted binary, not the downloaded archive: the archive recipes now hash the binary after extraction (the download still fails closed via the lock script if the hash is wrong), etcd and kube-apiserver gain their own binary hashes, and the SHAs for the affected tools have been relearned with "make learn-tools-shas". Tools built from source with "go install" have no reviewed hash here; they are anchored by go.sum/GOSUMDB when built and their staleness is already handled by keying the download path on the Go toolchain version, so the check skips them (empty hash variables are defined to keep --warn-undefined-variables quiet). Stacked on cert-manager#708. Supersedes cert-manager#625 (whole-cache purge run as a separate step you must remember) and cert-manager/cert-manager#8833 (verify at symlink-creation), folding both into cert-manager#708's tool_link_defs seam so the check is automatic and cannot be bypassed by a job that forgets to run it. Known limitation: the vendored-Go tarball is still verified at download only; the shared PATH is process-wide, so this closes cache poisoning, not the separate problem of a target using an undeclared tool. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Richard Wall <richard@the-moon.net>
3 participants
Add verify-cache command that validates the cache, preventing cache poisoning.