Skip to content

feat: handle different digests in manifest - #322

Open
upils wants to merge 19 commits into
canonical:mainfrom
upils:sha512-manifest
Open

upils wants to merge 19 commits into
canonical:mainfrom
upils:sha512-manifest

Conversation

@upils

@upils upils commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator
  • Have you signed the CLA?

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 (new sha512/sha384 fields alongside sha256), so manifests remain readable and byte-compatible across old and new readers.

Notes:

  • Fetching a package whose section advertises no supported digest is now an error (previously a silent unverified download), and digest kinds weaker than SHA256 (e.g. MD5) remain ignored.
  • manifestutil.Validate is 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.Package replaces the Digest/DigestKind fields with Digests map[string]string. Downstream Go code reading these fields must be updated.

Fixes #305

@upils
upils marked this pull request as ready for review September 2, 2026 11:23
Comment thread internal/testutil/archive.go Outdated
Hash string
// HashKind is the digest kind of Hash. When unset, Hash is treated as
// a sha256 digest.
HashKind string

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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"

@upils upils Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 lczyk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lczyk

lczyk commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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 lczyk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lczyk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after a separate discussion, dropping to sha256 validation for now is also a good way forward. 👍

@lczyk
lczyk self-requested a review September 7, 2026 15:17
@lczyk
lczyk self-requested a review September 17, 2026 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(26.10): chisel cut failing for 26.10

2 participants