[sha512] storage: Get/Set digest #374

lsm5 · 2025-10-06T14:56:49Z

Adds Get and Set methods for configuring digest. Currently, only sha256 and sha512 are supported, but should make future additions easier.

Defaults to sha256.

(cursor assisted)

Will create separate PRs for image and common.

podmanbot · 2025-10-06T14:59:22Z

✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6411

lsm5 · 2025-10-06T15:16:41Z

No other changes to storage for now. This will get used in other repos that depend on storage.

mtrmac

What is the precise semantics of the “current” value? “digests to use when creating new things”? “the expected level of collision-resistance when pulling”? “digests to convert to when pushing”?

lsm5 · 2025-10-06T17:45:41Z

“digests to use when creating new things”?

@mtrmac This. Let me know if I need to account for anything additionally here itself, unless it can wait for future work.

mtrmac

Per the Podman PR, “digest to use when creating new objects” is conceptually more of a per-operation parameter. It’s not at all clear to me that we should have a c/storage-scoped per-process global for this. (E.g. perhaps we do need a global default for this, set in containers.conf or storage.conf, but the decision which algorithm to use might plausibly be best placed into the Podman CLI layer, and be passed as an explicit parameter. Also consider podman-remote, client-side vs. server-side configs)
Although not as pedantic about it as c/image, c/storage is generally promising to have a stable API. In that sense, supporteddigests.Get is not quite an explanatory name for ”digests to use when creating new things”, and I’d like the long-term-committed name to be more specific (and less confusing, “supported digests” vs. “digests for new things”)

Ultimately, I appreciate that we need to start somewhere, and that building a digest parameter passing infrastructure all over the codebase before we understand the semantics really well would be a lot of tedious work with uncertain payoff.

WDYT about:

very explicitly documenting that these functions are a WIP API and should not be called outside of Podman+friends, even if they ship in a stable release
(absolutely optional) perhaps, even, naming them TemporaryGet/TemporarySet, or, I don’t know, TemporaryDigestForNewObjects/TemporarySetDigestForNewObjects?

?

The implementation LGTM other than concurrency.

storage/pkg/supported-digests/algorithm.go

lsm5 · 2025-10-07T17:29:13Z

function documentation updated
function names updated
cautinonary notes added
concurrency added
additional logic for digest validation, digest detection that can be reused in libraries (RE: your comment on my image/ PR).

PTAL.

mtrmac

Thanks!

With the explicit non-promise of API stability, I’m much less worried about getting this perfect.

Some of the new parts seem unnecessary and not a clear improvement compared to using go-digest directly. Let me know if you want this merge as is anyway.

[Man the AI is verbose…]

mtrmac · 2025-10-08T09:07:25Z

storage/pkg/supported-digests/algorithm.go

+// calls from multiple goroutines. It is typically used for validation before
+// calling TmpSetDigestForNewObjects().


I don’t know why anything would (“typically”) call this prior to …TmpSetDigest…, it can just call the setter and handle an error.

mtrmac · 2025-10-08T09:08:19Z

storage/pkg/supported-digests/algorithm.go

+
+	// Check against the list of supported algorithms
+	supportedAlgorithms := GetSupportedDigestAlgorithms()
+	for _, supported := range supportedAlgorithms {


(slices.Contains)

mtrmac · 2025-10-08T09:09:37Z

storage/pkg/supported-digests/algorithm.go

+// This is a pure function with no side effects and is thread-safe for concurrent
+// calls from multiple goroutines. It is typically used for logging and user-facing
+// display purposes.
+func GetDigestAlgorithmName(algorithm digest.Algorithm) string {


Why does this need to exist, distinctly from algorithm.String()? Also see below about the “canonical” code path.

Having a go-digest format that uses lowercase, and this function that uses uppercase, seems potentially confusing. I mean, if we do need to have two distinct string types, then making the two string sets distinct / incompatible is better than making strings ambiguous, but, still…

(I can, sort of, see a case for a logging/debugging utility that formats values specifically for error texts, maybe highlighting unsupported algorithms with an extra text. But I think "%q", algo.String() is good enough and adding an extra utility is not clearly necessary.)

mtrmac · 2025-10-08T09:12:50Z

storage/pkg/supported-digests/algorithm.go

+	case "sha512":
+		return "SHA512"
+	default:
+		if algorithm == digest.Canonical {


Given that digest.Canonical == digest.SHA256, AFAICS this is never reachable; so if the primary purpose of this function is this code path, that doesn’t work.

mtrmac · 2025-10-08T09:14:36Z

storage/pkg/supported-digests/algorithm.go

+// This is a pure function with no side effects and is thread-safe for concurrent
+// calls from multiple goroutines. It is typically used for validation and algorithm
+// detection from hex string lengths.
+func GetDigestAlgorithmExpectedLength(algorithm digest.Algorithm) (int, bool) {


Algorithm.Size() exists — do we need to hard-code a table, or even have this at all?

mtrmac · 2025-10-08T09:21:16Z

storage/pkg/supported-digests/algorithm_test.go

+		{"Unknown", digest.Digest("unknown:invalid").Algorithm(), "unknown"},
+		// Case-insensitive tests
+		{"sha256 lowercase", digest.Algorithm("sha256"), "SHA256"},
+		{"SHA256 uppercase", digest.Algorithm("SHA256"), "SHA256"},


I don’t know why this should silently work - any operations on digest.Algorithm("SHA256") is going to fail (or panic if not checked via Available); in that sense at least, such a value of a digest.Algorithm is simply invalid.

mtrmac · 2025-10-08T09:23:48Z

storage/pkg/supported-digests/algorithm.go

+
+	// Validate the digest type
+	switch algorithm {
+	case digest.SHA256, digest.SHA512:


(It might be valuable for this to rely on GetSupportedDigestAlgorithm, or for the two functions to refer to the same data.)

lsm5 · 2025-10-08T12:31:51Z

Thanks!

With the explicit non-promise of API stability, I’m much less worried about getting this perfect.

Some of the new parts seem unnecessary and not a clear improvement compared to using go-digest directly. Let me know if you want this merge as is anyway.

I would prefer a merge to get the other parts moving forward. If you'd prefer a TODO: refactor using go-digest before we can even consider calling this stable note in the pkg warning, I could add that. Let me know

mtrmac · 2025-10-08T12:41:06Z

Sure, I think some kind of a TODO marker pointing back here would be nice.

lsm5 · 2025-10-08T12:41:58Z

Added a FIXME at the top of storage/pkg/supported-digests/algorithm.go. Thanks.

- TmpDigestForNewObjects() and TmpSetDigestForNewObjects() for global state - IsSupportedDigestAlgorithm() and GetSupportedDigestAlgorithms() for validation - GetDigestAlgorithmName() with case-insensitive support - GetDigestAlgorithmExpectedLength() for algorithm-to-length mapping - DetectDigestAlgorithmFromLength() for length-based algorithm detection Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>

lsm5 · 2025-10-08T12:50:02Z

Added a FIXME at the top of storage/pkg/supported-digests/algorithm.go. Thanks.

updated the FIXME with a note saying it's a blocker for WIP removal.

podmanbot pushed a commit to podmanbot/buildah that referenced this pull request Oct 6, 2025

dnm: Vendor changes from containers/container-libs#374

3f28552

podmanbot mentioned this pull request Oct 6, 2025

Sync: [sha512] storage: Get/Set digest containers/buildah#6411

Closed

github-actions bot added the storage Related to "storage" package label Oct 6, 2025

lsm5 marked this pull request as ready for review October 6, 2025 15:16

mtrmac reviewed Oct 6, 2025

View reviewed changes

This was referenced Oct 6, 2025

[sha512] image: add configurable digest support #375

Open

[sha512] common: enable sha512 support #376

Open

mtrmac reviewed Oct 7, 2025

View reviewed changes

storage/pkg/supported-digests/algorithm.go Outdated Show resolved Hide resolved

storage/pkg/supported-digests/algorithm.go Outdated Show resolved Hide resolved

storage/pkg/supported-digests/algorithm.go Outdated Show resolved Hide resolved

lsm5 marked this pull request as draft October 7, 2025 16:37

lsm5 force-pushed the storage-agile-digest branch from 94df9c1 to 58c51d2 Compare October 7, 2025 16:37

podmanbot pushed a commit to podmanbot/buildah that referenced this pull request Oct 7, 2025

dnm: Vendor changes from containers/container-libs#374

615f933

lsm5 force-pushed the storage-agile-digest branch from 58c51d2 to 79f1c23 Compare October 7, 2025 17:08

podmanbot pushed a commit to podmanbot/buildah that referenced this pull request Oct 7, 2025

dnm: Vendor changes from containers/container-libs#374

c09220e

lsm5 marked this pull request as ready for review October 7, 2025 17:29

mtrmac reviewed Oct 8, 2025

View reviewed changes

lsm5 force-pushed the storage-agile-digest branch from 79f1c23 to 50b724f Compare October 8, 2025 12:41

podmanbot pushed a commit to podmanbot/buildah that referenced this pull request Oct 8, 2025

dnm: Vendor changes from containers/container-libs#374

0c595b4

lsm5 force-pushed the storage-agile-digest branch from 50b724f to 0d11523 Compare October 8, 2025 12:49

mtrmac approved these changes Oct 8, 2025

View reviewed changes

mtrmac enabled auto-merge October 8, 2025 12:49

podmanbot pushed a commit to podmanbot/buildah that referenced this pull request Oct 8, 2025

dnm: Vendor changes from containers/container-libs#374

e84b6d7

mtrmac merged commit 647a384 into containers:main Oct 8, 2025
37 checks passed

lsm5 deleted the storage-agile-digest branch October 8, 2025 13:08

lsm5 mentioned this pull request Oct 8, 2025

SHA512 digest support #358

Closed

		// calls from multiple goroutines. It is typically used for validation before
		// calling TmpSetDigestForNewObjects().

[sha512] storage: Get/Set digest #374

[sha512] storage: Get/Set digest #374

Uh oh!

Conversation

lsm5 commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

podmanbot commented Oct 6, 2025

Uh oh!

lsm5 commented Oct 6, 2025

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

lsm5 commented Oct 6, 2025

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lsm5 commented Oct 7, 2025

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

mtrmac Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

lsm5 commented Oct 8, 2025

Uh oh!

mtrmac commented Oct 8, 2025

Uh oh!

lsm5 commented Oct 8, 2025

Uh oh!

lsm5 commented Oct 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lsm5 commented Oct 6, 2025 •

edited

Loading