Conversation
| Hash string | ||
| // HashKind is the digest kind of Hash. When unset, Hash is treated as | ||
| // a sha256 digest. | ||
| HashKind string |
There was a problem hiding this comment.
[Note to reviewer]: This is named HashKind out of consistency with the other Hash field on this same struct, even though it holds the DigestKind value.
| "io" | ||
|
|
||
| "github.com/canonical/chisel/internal/archive" | ||
| "github.com/canonical/chisel/internal/cache" |
There was a problem hiding this comment.
[Note to reviewer]: Importing cache in more and more packages only to get access to the list of supported digestkinds looks increasingly wrong. Conceptually it does not make a lot of sense that archive or manifesutil depends on cache. I am tempted to extract the digestkind-related bits out of cache to a dedicated package. If we decide to proceed, I will do that in a follow-up as I don't want to pollute this PR with a refactor.
lczyk
left a comment
There was a problem hiding this comment.
ok, discussed this to exhaustion in a separate thread so i will just drop a digest (🥁🥁🐍) of that conversation here:
- fixed 26.10
- changes the manifest for older releases to sha512(!), even 20.04 because chisel resolves from -updates.
- chisel is release-agnostic so any special rules for e.g. 26.04 or earlier are off-the-table
- we could maintain backwards compatibility by choosing to always record sha256 preferentially over sha512 but then:
- we're weakening the manifest
- we're decoupling validation from the manifest, since validation would stay 512 ( or we'd flip validation too to prefer weaker sha. no. )
- we could take this PR and bump the schema version of the manifest 1.0 -> 1.1
atm it feels to me like the last option is the best, since all others introduce in-perpetuum compromises which go in hacky/insecure directions.
|
also, leaving a note here so we don't forget, this was not covered in #306 and, possibly, should have been. we might need to look into test coverage for chisel. |
lczyk
left a comment
There was a problem hiding this comment.
this version is almost ok imo. it does "kick the can down the road" regarding the breaking change -- at some point we'll have to flip the preference to recording the strongest supported digest, but we can do that in 1.6.0 rather than now and in a rush.
my one issue is that it's a regression from 1.5.0 in what we verify the .debs against -- the per-package digest inside Packages.gz, not the index itself, which this PR doesn't touch. 1.5.0 checks those with sha512, and ace8a36 (this PR's current HEAD) flips them back to sha256. all releases until 26.04 have only sha256 in InRelease, but their Packages.gz publish sha256 and sha512 (curl -fsSL http://archive.ubuntu.com/ubuntu/dists/noble-updates/main/binary-amd64/Packages.gz | gzcat | grep SHA512), so in 1.5.0 they are verified with sha512.
seemingly, if we wanted to address this, we'd need to decouple the verification (sha512) form what's recorded in the manifest (sha256) which completely defeats the purpose of the manifest. no. here is a proposed solution though: verify both shas. strongest-first as the primary, the way we want it, then, if sha256 is also published, verify that too since that's the one we will be recording. abit of a perf hit, but i think it will give us the best result we could get with the constraints we have:
- 26.10 works -- sha512 only, nothing to double-check
- manifests unchanged on 20.04..26.04, no schema bump
- what we record is what we checked
- we keep the strongest published digest for verification, so no regression from 1.5.0
lczyk
left a comment
There was a problem hiding this comment.
after a separate discussion, dropping to sha256 validation for now is also a good way forward. 👍
Ubuntu archives publish multiple checksums per package (SHA256 and SHA512 today, SHA512-only on 26.10+), but the manifest recorded only a single SHA256 digest, which is wrong. Chisel now records all digests published by the archive (and supported by Chisel) for each package in the manifest, while the strongest one continues to be used for fetch verification and content-addressable caching. The manifest schema stays
"1.0": the wire format is purely additive (newsha512/sha384fields alongsidesha256), so manifests remain readable and byte-compatible across old and new readers.Notes:
manifestutil.Validateis now stricter than before: it validates package entries (name, arch, version, digest kind and value) which it previously ignored entirely, so malformed manifests that used to pass validation are now rejected.BREAKING CHANGE:
manifest.Packagereplaces theDigest/DigestKindfields withDigests map[string]string. Downstream Go code reading these fields must be updated.Fixes #305