feat: handle different digests in manifest by upils · Pull Request #322 · canonical/chisel

upils · 2026-09-02T10:19:34Z

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 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 can effectively contain an new sha512 field alongside sha256, so manifests remain readable and byte-compatible across old and new readers.

API changes: manifest.Package gains Digests map[string]string and a NewPackage constructor that validates digest kinds and derives the legacy Digest field (sha256 when present, sha512 otherwise), which is kept for source compatibility with existing consumers. manifestutil.PackageInfo replaces PkgDigest()/PkgDigestKind() with PkgDigests(). This requires at least a minor release.

Also of note: fetching a package whose section advertises no supported digest is now an error rather than a silent unverified download.

Fixes #305

upils · 2026-09-02T11:42:52Z

 	"io"

 	"github.com/canonical/chisel/internal/archive"
+	"github.com/canonical/chisel/internal/cache"


[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

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 · 2026-09-03T09:27:11Z

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

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

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

upils · 2026-09-23T09:28:04Z

+	SHA256  string `json:"sha256,omitempty"`
+	SHA512  string `json:"sha512,omitempty"`
+	SHA384  string `json:"sha384,omitempty"`


[Note to reviewer]: The set of supported digest kinds is repeated in several places in this package. I tried different approaches to avoid this and improve maintainability but none was notably better than the current approach here.

upils · 2026-09-23T09:30:40Z

-	for _, f := range order {
-		if d, _, ok := control.ParsePathInfo(release.Get(f.name), path); ok {
-			return d, f
+func findDigest(release control.Section, path string) (digest string, field digestField, ok bool) {


[Note to reviewer]: This change in signature (returning a bool) addresses a comment of the previous PR that was missed. Same thing for packageDigests below.

lczyk · 2026-09-24T20:55:01Z

+			Name:    "package1",
+			Version: "v1",
+			Arch:    "a1",
+			Digests: map[cache.DigestKind]string{cache.SHA384: "s384"},


note that cache.SHA384 caches with sha-3 (sha3.New384(), added in #298). there is also sha-2-384 and that could be a source of confusion. maybe the manifest key should be called SHA3_384 instead?

great loc to comment on this, ik 😅

I agree this can be a bit imprecise, but we discussed it with Gustavo and the name sha384 was chosen on purpose, to be consistent with sha256.

lczyk · 2026-09-24T21:12:28Z

note that copa has chisel manifest support (project-copacetic/copacetic#1667) and atm it will hard-fail for rows w/out sha256. we should let them know and maybe even do a followup pr. they're using the canonical/chisel-manifest mirror so we need to update that too ofc (canonical/chisel-manifest#3).

lczyk · 2026-09-24T21:14:59Z


-var digestKinds = []DigestKind{SHA256, SHA384, SHA512}
+// digestKinds sorted in decreasing order of strength.
+var digestKinds = []DigestKind{SHA384, SHA512, SHA256}


Suggested change

var digestKinds = []DigestKind{SHA384, SHA512, SHA256}

var digestKinds = []DigestKind{SHA512, SHA384, SHA256}

The existing order is correct. I know this can be debatable but as we are comparing SHA-512 and SHA3-384, I considered SHA3-384 to be "stronger".

lczyk

im not sold on the custom martial and unmartiallers.

also, since we're leaving Digest for compat, how about we just make it always sha256 ( yes, sometimes compute it ). then its 100% compatibility, and consumer like copa dont need patching

lczyk · 2026-09-24T21:17:57Z

@@ -12,13 +13,124 @@ import (
 const Schema = "1.0"

 type Package struct {


id love to avoid the custom marshall and unmarshall. what about something like:

type Package struct { Kind string `json:"kind"` Name string `json:"name,omitempty"` Version string `json:"version,omitempty"` } func (p Package) Digests() map[string]string

??

Can you elaborate? Do you suggest not exporting a Digest field? How would that not break compatibility?

i forgot to add Digest to the enum. the point of the comment is to not add the Digests field with the custom marshallers, but add a method Digests which is the public api to these, and then store the rest in private fields for which we can use bock-standard Json marshaller

I considered this but this has some drawbacks/blockers:

The API is now slightly inconsistent: a Digest is accessible via a field, the rest via a method. Maybe not a big deal but still

it does not really solve how to init a Package object. How to declare the map of digests?

To use the JSON marshaller/unmarshaller the fields must be exported, and I want to avoid going back to one field per digest kind as this will need to change whenever we add/remove kinds. So I think what you describe is not possible unless I am missing something.

lczyk · 2026-09-24T21:23:10Z

+		case "sha384":
+			pj.SHA384 = digest
+		default:
+			return nil, fmt.Errorf("cannot marshal package %q: unsupported digest kind %q", p.Name, kind)


marshalling a Package couldn't fail before; with a free-form map it now can. :/

This is consequence of having Digests map[string]string designed to avoid breaking compatibility of old readers when additional digest kinds are added. The marshaller can indeed fail but in a case where we really do not want to continue because we do not want to produce a manifest with arbitrary digest kinds.

lczyk · 2026-09-24T21:24:07Z

+			digests[kind] = digest
+		}
+	}
+	pkg, err := NewPackage(&PackageOptions{


going through NewPackage here drops the decoded kind (NewPackage hardcodes "package"), and the error below can't happen. fill the struct directly instead?

Unless I am missing something I think you mix 2 different kinds:

the manifest entry kind, in that case "package". Other values for this are for example "slice" or "content".

digest kinds, used above as key to the digests map.

Oh now I think I see what you meant.

You raised an interesting point. In a sense UnmarshalJSON was lying and dealing with something that is not its concern. I fixed it so the unmarshaling is properly representing the received data.

While digging on that I spotted a possible bug though: The current implementation of IteratePackages would match on entries like
{“kind”: “package-foo”, …} due to db.IteratePrefix(prefix) removing the ”, suffix of the prefix before iterating on the db. This works well for other kinds as the goal is to use the value as a prefix, but is probably not what was intended for package entries. At this point I also think that relying on this behavior is a misuse of the API.

lczyk · 2026-09-24T21:25:22Z

+	Digests map[string]string
+}
+
+func NewPackage(opts *PackageOptions) (*Package, error) {


NewPackage(nil) panics. do we need NewPackage / PackageOptions as public api anyway?

I wanted an entry point to create "proper" Package object, with correct Digest/Digests. This is used in manifestutil and I expect consumers of the API to use it too, so it made sense to export the function and the PackageOptions struct.

upils · 2026-09-28T07:30:15Z

@lczyk Thanks for the review.

also, since we're leaving Digest for compat, how about we just make it always sha256 ( yes, sometimes compute it ).

We decided to record what the archive published, as we do not want to make assumptions on how these digests are used (integrity check? caching? something else?). I think we should be careful not being too influenced by a single use case, at the risk of putting us in a corner.

lczyk · 2026-09-28T08:53:55Z

I think we should be careful not being too influenced by a single use case, at the risk of putting us in a corner.

yeah.. fair, fair. just a ponder to see what hard backwards compatibility would look there. the original comment is not a blocker from me by any means.

feat: handle different digests in manifest

f1d045c

upils marked this pull request as ready for review September 2, 2026 11:23

docs: refine comments

a3e7036

upils commented Sep 2, 2026

View reviewed changes

upils added 4 commits September 2, 2026 14:11

style: cleaning

ef98829

fix: fail loudly on invalid marshalling input

2013aab

test: improve TestManifestRead

1f8982e

docs: cleaning

e013b76

lczyk suggested changes Sep 2, 2026

View reviewed changes

lczyk mentioned this pull request Sep 4, 2026

bug(26.10): chisel cut failing for 26.10 #305

Open

upils added 2 commits September 4, 2026 12:09

fix: use sha256 for packages by default

ace8a36

docs: refine comments

1e5c4d8

lczyk suggested changes Sep 5, 2026

View reviewed changes

lczyk approved these changes Sep 7, 2026

View reviewed changes

lczyk self-requested a review September 7, 2026 15:17

lczyk approved these changes Sep 8, 2026

View reviewed changes

upils added 11 commits September 16, 2026 16:19

fet: record multiple hashes in the manifest

cb3f498

fix: order by strength

5466124

refactor: simplify getting strongest pkg digest kind

7ab32ed

tests: cleaning

ca4f18d

docs: refine comments

104f089

tests: rename Hashes field to fit usage

8009bb0

docs: remove useless comment

d0d2b9c

style: improve naming and docs

02919e5

fix: make sure reported digest values are not empty

ea1734a

style: linting

f7011a8

fix: revert unrelated change in behavior

6c03948

lczyk self-requested a review September 17, 2026 21:54

fix: keep Digest field to avoid breaking change

e53e543

upils added 3 commits September 22, 2026 17:23

feat: add cleaner package creation

a73fbdc

refactor: rework NewPackage

811c992

style: clean docs and tests

42eed4b

upils commented Sep 23, 2026

View reviewed changes

docs: add deprecation notice for Digest field

2e2c407

upils added the Priority Look at me first label Sep 24, 2026

lczyk reviewed Sep 24, 2026

View reviewed changes

lczyk suggested changes Sep 24, 2026

View reviewed changes

upils added 2 commits September 28, 2026 08:42

style: fix lint

886dcd5

fix: appy review suggestions

94ddf8c

upils added 2 commits September 28, 2026 11:52

fix: apply review comments

4c5f6e5

fix: simplify

9f8ba1c

	var digestKinds = []DigestKind{SHA384, SHA512, SHA256}
	var digestKinds = []DigestKind{SHA512, SHA384, SHA256}

		@@ -12,13 +13,124 @@ import (
		const Schema = "1.0"

		type Package struct {

Conversation

upils commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

upils Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lczyk left a comment

Choose a reason for hiding this comment

Uh oh!

lczyk commented Sep 3, 2026

Uh oh!

lczyk left a comment

Choose a reason for hiding this comment

Uh oh!

lczyk left a comment

Choose a reason for hiding this comment

Uh oh!

upils Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lczyk commented Sep 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

upils Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lczyk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

upils commented Sep 28, 2026

Uh oh!

lczyk commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

upils commented Sep 2, 2026 •

edited

Loading

upils Sep 2, 2026 •

edited

Loading

upils Sep 23, 2026 •

edited

Loading

upils Sep 28, 2026 •

edited

Loading

lczyk commented Sep 28, 2026 •

edited

Loading