fix: add support for sha512-only archives by lczyk · Pull Request #306 · canonical/chisel

lczyk · 2026-07-06T22:17:38Z

Have you signed the CLA?

Attempted fix for #305.

Resolve digests through a small preference list (strongest first: SHA512, then SHA256) instead of hardcoding one field, and carry the hash kind alongside the digest so downloads are verified and cached with the matching algorithm. This covers:

the Release index table (Packages digest),
the per-package digest (the .deb), and
the Acquire-By-Hash path merged from main, which now builds by-hash/<algo>/<digest> URLs from the resolved field rather than assuming by-hash/SHA256/.

Archives only guarantee a by-hash directory for the strongest hash they advertise, so the by-hash URL must be built from that one -- thanks @FinnRG for catching this. Since the preference list is strongest-first, the resolved field is already the right one, and a single ordering serves both purposes.

As of this writing Stonking publishes SHA512 only, in the base suite and in -security/-updates alike, so the whole suite resolves via SHA512. Resolute and earlier remain SHA256-only

An archive advertising both digests, content is now verified and cached under its SHA512 digest rather than its SHA256 one. Cache entries written by an older chisel are keyed by SHA256, so they go cold and are fetched once more, then expire normally (which, at the moment means they don't expire #261, but that's a separate issue). Nothing gets corrupted. Importantly, at present no archive Chisel targets advertises both digests (Resolute is SHA256-only, Stonking is SHA512-only), so in practice this is not a behaviour change.

Testing

Added unit tests for the new behaviour and tested manually against the Stonking archive. The by-hash tests cover the four archive shapes: a by-hash directory for the strongest hash only, directories for several hashes (as Stonking actually publishes), a directory for a weaker hash only (a buggy archive, falls back to the named path), and no by-hash directories at all.

Out of scope

Something i've deliberately decided is out of scope but really should be fixed in a followup: The manifest's per-package digest still reads SHA256, so on a SHA512-only archive it still errors. Most chisel cut's still work unless the user requests the manifest to be cut too.

Signed-off-by: Paul Mars <paul.mars@canonical.com>

…pt-sha512 # Conflicts: # internal/archive/archive.go # internal/archive/testarchive/testarchive.go

upils

Thanks @lczyk! This already looks very good.

I agree that, as you pointed out in the PR description, a follow-up PR must be created to deal with the manifest generation to consider this bug completely fixed. I can do it once this PR is merged, as we need to make sure it will work nicely with the current work on supporting bins.

Change the testarchive digest field from a single kind to a list so a package/release can publish several checksum fields at once. Add tests asserting SHA256 is preferred when both are published, for the named and by-hash paths.

Use "section" for the checksum groups, matching the control.Section terminology used across the archive package.

Its only caller is makeDigest; fold the sha256 sum in and drop the one-line helper.

Digest kinds are a per-archive choice, not per-package, so model them on the Release alone. The package copy becomes an unexported render cache, filled from the release at render time, so tests can only set kinds in one place and the post-adjustRelease propagation walk goes away.

Digest kinds now render exactly as set on the Release: an empty list publishes no digest sections rather than silently falling back to SHA256. The single construction site sets the SHA256 default explicitly, so tests still get it without hidden behaviour.

golangci-lint errcheck flagged the unchecked item.Walk. The callback only sets a field and always returns nil, so Walk cannot error here; ignore the return explicitly to satisfy the linter.

lczyk · 2026-07-07T15:34:49Z

@upils ok, ready for another review round. +302 -29 is on the bigger side but vast majority is tests, so i hope this is still ok

upils

Thanks @lczyk. I have more comments, and sorry for the back-and-forth on the section/field naming (see my comment below).

Archives only guarantee a by-hash directory for the strongest hash they advertise, so a by-hash URL must never be built from a weaker one. The digest used to verify content and key the cache is a separate concern, and still prefers SHA256 to keep existing caches valid. The test archive now publishes by-hash directories only for the strongest kind, matching ftpmaster, so building a by-hash URL from a weaker hash fails the tests instead of passing silently.

makeDigest silently fell back to SHA256 for any kind it did not know, so makeDigest("MD5", ...) returned a SHA256 sum. Panic instead, as the other helpers in this package do.

Render calls it before walking the items, and Walk visits the Release first, so Content always runs after the packages have inherited.

inheritDigestKinds assumed its callback could never error, so neither could Walk. Return the error instead, and handle it at the call sites, so a future Walk with a real error path cannot fail silently.

Replace the render-time copy of the release's digest kinds with a backpointer set when the release adopts the package. Sections always see the release's current choice, and rendering a section before any release adopted the package panics instead of silently rendering without digests.

adoptPackages hand-rolled the recursion and error propagation that Walk already provides.

upils

Thanks again @lczyk. This is indeed getting a bit bigger than we wanted but I do not see a sensible way to split this work further.

upils · 2026-07-08T13:02:25Z

+	// release is set when a release adopts this package at render time.
+	// Section reads the archive-wide digest kinds through it, so the package
+	// always sees the release's current choice.
+	release *Release


I wonder why we now need to wire the whole release object here when we are only interested in the digest kinds. I saw your commit message but it is unclear why a copy at render time is not enough. Is the digest kinds slice changing over time in some cases?

i thought a backpointer to Release was a cleaner way to implement the if p.release == nil guard.

with DigestKinds []string, the empty/nil slice is ambiguous: it means both "archive publishes no digests" and "not rendered yet", so we'd need another field (like rendered bool). with release *Release this is only one field sincep.release == nil` always means "not rendered yet".

happy to change back to copy but then we grow one more field. if that's fine then no problem. 👍

I agree with you, let's keep it.

This indeed does not feel so great. We are improving Chisel to generalize the digest logic where we assumed a constraint in the past, and on the way we are again establishing a new constraint that is not a ground truth for how archives are organized.

Just to make the point more clear, which release of Ubuntu is this:

https://www.archive.ubuntu.com/ubuntu/pool/main/l/linux/

Also, there is a question about the fact this is being done per release instead of per package. How did we transition between the hashes? Was the entire release rebuilt at once, and has apt survived the transition phase without failures? How?

The ftpmaster publisher always rebuilds the entire Release (and associated index files) all at once, so there was no transition phase.

Just to be clear, the hashes inside the Packages and Sources index files may be different from the hash(es) used in the Release file and therefore the by-hash lookup.

lczyk · 2026-07-08T21:34:43Z

This is indeed getting a bit bigger than we wanted but I do not see a sensible way to split this work further.

yeah, alas neither. very open for ideas though, and trying to keep scope small and focused.

"adopt" implied release-owns-package, but the wiring runs the other way: each package gets a back-reference to the release.

upils

Thanks @lczyk. Let's do the simplification you proposed and this should be good for the core review.

upils · 2026-07-09T08:24:50Z

+	// release is set when a release adopts this package at render time.
+	// Section reads the archive-wide digest kinds through it, so the package
+	// always sees the release's current choice.
+	release *Release


I agree with you, let's keep it.

Co-authored-by: Upils <5464641+upils@users.noreply.github.com>

Drop the SHA256-first cache ordering: content is verified and cached under the strongest advertised digest. Worst case, entries cached by an older chisel under SHA256 go cold and are fetched once more. No current archive advertises both digests, so today this changes nothing.

lczyk · 2026-07-09T10:32:56Z

@upils changes made and desc updated

upils

Thanks!

niemeyer

Thanks a lot for this. The code looks clean and simple, but some of the wiring choices might do with some improve.

Given this was already reviewed repeatedly, I suggest you pick up from here @upils.

niemeyer · 2026-07-10T08:13:27Z

+	// release is set when a release adopts this package at render time.
+	// Section reads the archive-wide digest kinds through it, so the package
+	// always sees the release's current choice.
+	release *Release


This indeed does not feel so great. We are improving Chisel to generalize the digest logic where we assumed a constraint in the past, and on the way we are again establishing a new constraint that is not a ground truth for how archives are organized.

Just to make the point more clear, which release of Ubuntu is this:

https://www.archive.ubuntu.com/ubuntu/pool/main/l/linux/

Also, there is a question about the fact this is being done per release instead of per package. How did we transition between the hashes? Was the entire release rebuilt at once, and has apt survived the transition phase without failures? How?

niemeyer · 2026-07-10T08:15:14Z


 func (p *Package) Section() []byte {
+	if p.release == nil {
+		panic("package section rendered before a release adopted the package; render via Release.Render")


As another consequence, and I know this is clear by now, but just to state it: this used to pure a pure data type and with this change we cannot render a Package without a private field being set, and it panics instead of failing. If nothing else, it's more evidence that it seems to be making things less nice.

I changed it back to setting a slice of DigestKinds in Package to avoid all these drawbacks. This is consistent with how we deal with other fields (especially arch) . Only prepareArchiveAdjustRelease considers that both Release.DigestKinds and Package.DigestKinds should be in sync by default, but callers are free to tweak that If not using prepareArchiveAdjustRelease, callers are also responsible to properly fill Release and Package. I think this is a better separation of concerns and is more consistent with the existing code.

niemeyer · 2026-07-10T08:23:02Z

-			byHashPath := path.Join(prefix, "dists", r.Suite, path.Dir(itemPath), "by-hash", "SHA256", makeSha256(itemContent))
-			content[byHashPath] = itemContent
+			// Archives only guarantee a by-hash directory for the strongest
+			// hash they advertise, though they may publish more. Render the


Is this algorithm how apt determines those paths as well? Given the history of hash changes in several applications, I don't feel very safe to be making another guess that can easily break when we start to have multiple hash kinds for the same archives in the near future.

The aim here is to be consistent with the promise made by a real valid archive: when supporting by-hash URL, publish them at least for the strongest supported hash in the release. This piece of code is unrelated to how the client should behave when consuming the archive.

Regarding the actual behavior of Chisel: when trying to fetch the package index, findDigest is called to get the strongest hash associated to the file, and this hash is then used to try to get the file "by-hash". In the scenario where the archive published multiple hashes for files, Chisel only tries to use the "by-hash" URL with the strongest hash it supports and falls back to the regular URLs if encounters a 404 error. This is consistent with apt's behavior.

niemeyer · 2026-07-10T08:25:51Z

+			// guaranteed one only, so tests catch clients building by-hash
+			// URLs from a weaker hash; tests wanting extra directories add
+			// them on top of the rendered content.
+			if kind := strongestKind(r.DigestKinds); kind != "" {


It's at least worth of note that we are doing this for every individual element instead of the overall release. Is that how it works for the archives as well? What happened during the transition between the the hash kinds, did we have half the release with one hash and half with the other? Was apt making the right choices during that time?

I moved half of this question to earlier on in the review, so feel free to ignore the points already replied to.

@FinnRG answer to these questions: #306 (comment). I do not really trust GH linking so I am pasting the answer here:

The ftpmaster publisher always rebuilds the entire Release (and associated index files) all at once, so there was no transition phase.

Just to be clear, the hashes inside the Packages and Sources index files may be different from the hash(es) used in the Release file and therefore the by-hash lookup.

As the used hash is a release-level decision, I am moving the call strongestKind before the call to Walk to make it obvious.

Signed-off-by: Paul Mars <paul.mars@canonical.com>

upils · 2026-07-15T08:59:06Z

+	if adjustRelease != nil {
+		adjustRelease(release)


[Note to reviewer]: This was moved before preparing the packages so they pick up the adjusted DigestKinds.

Signed-off-by: Paul Mars <paul.mars@canonical.com>

lczyk · 2026-07-15T10:32:39Z

i agree with @upils changes 👍 have reached roughly the same conclusion / direction when looking at this

lczyk added 2 commits July 6, 2026 22:58

feat: add sha256

a948b10

docs: comment

e1b7e40

lczyk marked this pull request as ready for review July 6, 2026 22:24

upils added the Bug An undesired feature ;-) label Jul 7, 2026

upils added 4 commits July 7, 2026 09:21

tests: test SHA512 cache entries

daf5864

Signed-off-by: Paul Mars <paul.mars@canonical.com>

fix: refine comments

46d5658

Signed-off-by: Paul Mars <paul.mars@canonical.com>

tests: use stonking for SHA512 tests

1511187

Signed-off-by: Paul Mars <paul.mars@canonical.com>

fix: rely on ok to check digest

bbdf2fe

Signed-off-by: Paul Mars <paul.mars@canonical.com>

lczyk changed the title ~~feat: add sha256~~ feat: add support for sha256-only archives Jul 7, 2026

lczyk changed the title ~~feat: add support for sha256-only archives~~ fix: add support for sha256-only archives Jul 7, 2026

Merge remote-tracking branch 'canonical/main' into fix/issue-305-acce…

7501ca0

…pt-sha512 # Conflicts: # internal/archive/archive.go # internal/archive/testarchive/testarchive.go

lczyk requested a review from upils July 7, 2026 07:51

test: cover by-hash over sha512

c973b90

lczyk changed the title ~~fix: add support for sha256-only archives~~ fix: add support for sha512-only archives Jul 7, 2026

test: propagate digest kind to packages in test helper

9fa6d53

lczyk commented Jul 7, 2026

View reviewed changes

Comment thread internal/archive/testarchive/testarchive.go Outdated

upils suggested changes Jul 7, 2026

View reviewed changes

lczyk added 6 commits July 7, 2026 13:33

test: cover release publishing both digests

46fefb3

Change the testarchive digest field from a single kind to a list so a package/release can publish several checksum fields at once. Add tests asserting SHA256 is preferred when both are published, for the named and by-hash paths.

refactor: rename digestField to digestSection

1e1a0d1

Use "section" for the checksum groups, matching the control.Section terminology used across the archive package.

test: inline makeSha256

58c0e97

Its only caller is makeDigest; fold the sha256 sum in and drop the one-line helper.

fix: check Walk return in inheritDigestKinds

bc4b7f6

golangci-lint errcheck flagged the unchecked item.Walk. The callback only sets a field and always returns nil, so Walk cannot error here; ignore the return explicitly to satisfy the linter.

upils suggested changes Jul 8, 2026

View reviewed changes

lczyk added 5 commits July 8, 2026 10:04

refactor: name digest fields, not sections

744fae9

fix: panic on unknown digest kind in test archive

8c5cbc1

makeDigest silently fell back to SHA256 for any kind it did not know, so makeDigest("MD5", ...) returned a SHA256 sum. Panic instead, as the other helpers in this package do.

refactor: drop redundant inheritDigestKinds call

7a308a3

Render calls it before walking the items, and Walk visits the Release first, so Content always runs after the packages have inherited.

refactor: propagate Walk errors in test archive

2ec8ab9

inheritDigestKinds assumed its callback could never error, so neither could Walk. Return the error instead, and handle it at the call sites, so a future Walk with a real error path cannot fail silently.

lczyk added 3 commits July 8, 2026 10:26

docs: describe digestKinds field, not its writer

c5ce9e3

refactor: adopt packages through Walk

7cf965b

adoptPackages hand-rolled the recursion and error propagation that Walk already provides.

upils suggested changes Jul 8, 2026

View reviewed changes

lczyk added 4 commits July 8, 2026 22:50

refactor: rename adoptPackages to wirePackages

abfc91f

"adopt" implied release-owns-package, but the wiring runs the other way: each package gets a back-reference to the release.

docs: keep findDigest comment above the table format

0ebadf2

test: cover by-hash dirs beyond the strongest digest

606e7a1

docs: by-hash dirs beyond the strongest are legal

86024cd

lczyk requested a review from upils July 8, 2026 22:25

upils suggested changes Jul 9, 2026

View reviewed changes

lczyk and others added 2 commits July 9, 2026 11:01

Update internal/archive/testarchive/testarchive.go

b70e1b4

Co-authored-by: Upils <5464641+upils@users.noreply.github.com>

lczyk requested a review from upils July 9, 2026 10:31

upils approved these changes Jul 9, 2026

View reviewed changes

niemeyer requested changes Jul 10, 2026

View reviewed changes

test: pass DigestKinds instead of release pointer

0c9e9e3

Signed-off-by: Paul Mars <paul.mars@canonical.com>

upils reviewed Jul 15, 2026

View reviewed changes

refactor: strongest kind at the release level

b9da3c9

Signed-off-by: Paul Mars <paul.mars@canonical.com>

Uh oh!

Conversation

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

Uh oh!

Testing

Out of scope

Uh oh!

Uh oh!

upils left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lczyk commented Jul 7, 2026

Uh oh!

upils left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

upils left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

lczyk commented Jul 8, 2026

Uh oh!

upils left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lczyk commented Jul 9, 2026

Uh oh!

upils left a comment

Choose a reason for hiding this comment

Uh oh!

niemeyer 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!

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 Jul 6, 2026 •

edited

Loading