SHA512 digest support #358
Conversation
Allow the user to set the digest type using this option. If unset, the default will be sha256. Signed-off-by: Lokesh Mandvekar <[email protected]>
Signed-off-by: Lokesh Mandvekar <[email protected]>
Signed-off-by: Lokesh Mandvekar <[email protected]>
github-actions
bot
added
storage
There was a problem hiding this comment.
As a technical convenience, please split the vendoring part into a separate commit, (or drop it if it is not directly relevant for this PR), +1.4M lines ~breaks the web UI, at least from Safari; I didn’t get past c/common/libimage/filters in the large commit.
Just an initial very brief look. There are two parts to this, designing what to do, and then actually implementing that. I think primarily we should decide on the design questions for now.
It might make sense to split the “100% uncontroversial and clearly needed” technical changes, primarily changes that can accept / validate non-sha256 digests correctly instead of comparing with digest.FromBytes() and assuming sha256.
I wouldn’t mind splitting this into many separate commits, or even PRs, up to one per subpackage or something like that — that would also require fairly precisely tracking which parts have been updated and which haven’t. That might be too much work to be worth it.
Is there an overall, precisely-specified objective that this PR aims to achieve?
-
Is this “sha512-maniacal users can set up a sha512-only system, and we don’t care what happens with sha256 images or sha256 CLI inputs?” (That might be valuable for them, but ~no-one would want such a setup by default.)
-
Is this “current users can transparently start using sha512 images, while most of the universe remains sha256, but they might need to adjust their scripts to always look up the image X by the digest used by image X”?
-
Is this “current users can transparently start using sha512 images, while most of the universe remains sha256, and the digest choices are ~transparent to them, using sha256 references to sha512 images works, and vice versa”?
-
Something else / more nuanced? You’ve been thinking about this far longer than I have.
-
Similarly, how do semantics of the internal APIs change? (I suppose some that assume sha256 can just be deprecated, but for others we might want to look up by non-matching digest, or return an unexpected value)
-
Eventually, I think we’ll need a conversion feature in c/image/copy (and elsewhere?), so that users can convert sha256 images to sha512 and vice versa, and so that users can build sha256 / sha512. I don’t think at all that that needs to be in this PR — maybe better when it isn’t — but something to think about when designing where the options live.
Reading from the top, I think container’s BigData only exist locally, and locally digests ~don’t matter (anyone able to write to the file systems we reference using digests can probably also overwrite digests in databases and rename files, either way the local files are trusted by c/storage and not authenticated), except for collisions. I.e. if we are storing externally-supplied or attacker-influenced data, we might still want to use a stronger digest; I think collisions intentionally/knowingly triggered by the invoking user are out of scope (though … build systems accepting user-provided jobs do need protecting).
And, if container’s BigData now has configurable digests, that means the callers of .ContainerBigDataDigest will see a different digest depending on users’ configuration. How should they think about that / what should they assume?
[Now, this example is hypothetical because I can’t see a single caller in Podman and I don’t know whether there is any external one, but the question applies generally.]
Knowing what “the rules” are would make it easier to review vs. “the rules”, and we could discuss “the rules” separately.
I don’t think I have seen anything for cross-digest layer matching, and AFAICS pulls store the layers using the digests contained in the manifest; so the assumption is that sha256 and sha512 layers would coexist side by side and not be deduplicated? Or is it that the digest would be forcibly re-computed on pull to the one in storage.conf?
E.g. what happens if the same image (exactly the same config digest bytes) is converted to sha256 / sha512 in the manifest, and we have a sha512 manifest + sha256 DiffIDs in config? What happens if we have two manifests, both using sha256 for the config digest, but one using sha256 for layers and the other using sha512? IIRC the existing code would define ImageID = config digest, the same sha256 for both, and then try to deduplicate using mismatched layer digests.
(I don’t know the answers… I think one thing we’ve been discussing, for other reasons, is breaking the “image ID = config digest” link, and maybe using “image ID = manifest digest” [or even “image ID is completely random and unpredictable”?? but how would we deduplicate then]. So we might keep “image ID = config digest” for sha256-only images, to keep compatibility with existing scripts, and for all new images, use something else in storageImageDestination.computeID.)
Do you have a design in mind, answering these kinds of questions? If so, great!
If not, maybe this PR should be split into “make c/image [and other parts of the code?] mostly digest-agnostic” in parts where that clearly works (validating blobs against digests); and then we will need to deal with persistence (c/storage, “match two things by digest equality, look up by digest, look up by digest prefix”), and such harder things later, maybe more slowly.
("Request changes" primarily to mark the c/image/storage DiffID check bypass)
| } | ||
| oldDigest, digestOk := c.BigDataDigests[key] | ||
| newDigest := digest.Canonical.FromBytes(data) | ||
| digestAlgorithm := getDigestAlgorithmFromType(r.digestType) |
There was a problem hiding this comment.
I think this should be parsed at initialization time, and unknown values should cause an error at that time. (Applies also elsewhere.)
There was a problem hiding this comment.
Just to cross-link: containers/podman#27170 (comment) , the currently-proposed uses suggest the digest setting should not be a global per-store option (or perhaps we need a set of several distinct options).
| driver drivers.Driver | ||
| // store is a reference to the parent store for accessing digest | ||
| // configuration | ||
| store *store |
There was a problem hiding this comment.
(Trivial, but I’m not sure the back-link here is ideal, it creates a cyclical reference which GC will not clean up until store goes away … and actually a store creates multiple instances of layerStore over its lifetime.)
In the other stores, the digest option is provided at construction time, is there a specific reason this code diverges?
| // Decide if we need to compute digests | ||
| var compressedDigest, uncompressedDigest digest.Digest // = "" | ||
| var compressedDigester, uncompressedDigester digest.Digester // = nil | ||
| digestAlgorithm := r.store.GetDigestAlgorithm() |
There was a problem hiding this comment.
storeDigestAlgorithm, in this context, to be clear whether the value is store-chosen or caller-chosen?
| GIDMap() []idtools.IDMap | ||
|
|
||
| // GetDigestType returns the configured digest type for the store. | ||
| GetDigestType() string |
There was a problem hiding this comment.
I don’t think we need both — and this is the less-precisely typed one.
| GetDigestType() string | ||
|
|
||
| // GetDigestAlgorithm returns the digest algorithm based on the configured digest type. | ||
| GetDigestAlgorithm() digest.Algorithm |
There was a problem hiding this comment.
What exactly is the semantics of the option? When would callers use it, what are the guarantees, when is it not appropriate?
| return digest.SHA512 | ||
| case "sha256": | ||
| return digest.SHA256 | ||
| default: |
There was a problem hiding this comment.
This is dangerous WRT typos, and it prevents us from adding any values in the future. Invalid values should be rejected early.
|
|
||
| // Validate the algorithm is known | ||
| switch algorithm { | ||
| case "sha256", "sha512": // common algorithms |
There was a problem hiding this comment.
(Now that we know digests can be added), this is not maintainable.
Either add a storage/pkg/supported-digests as a centralized place hard-coding our choices, or (preferably??) use https://github.com/opencontainers/go-digest/blob/89707e38ad1aab6815bde4ad095806212ec90236/algorithm.go#L198 to parse any of them.
(Generally, are other parts of this parser applicable elsewhere? “filters” is not a natural place for “prefix of digest” parser.)
| } | ||
|
|
||
| // filterDigest creates a digest filter for matching the specified value. | ||
| func filterDigest(value string) (filterFunc, error) { |
There was a problem hiding this comment.
A generic design question: What happens / what do we want to happen if the on-registry image is digest A, c/storage configuration is digest B, and the user filters using digest C - or some subset of that?
What is the goal of this PR?
| srcManifestDigest, err := manifest.Digest(ic.src.ManifestBlob) | ||
| // Get the digest algorithm from the destination store if available | ||
| digestAlgorithm := digest.Canonical // Default fallback | ||
| if digestProvider, ok := ic.c.dest.(interface{ GetDigestAlgorithm() digest.Algorithm }); ok { |
There was a problem hiding this comment.
This sort of cast is a bit difficult for tools to see / follow, and copy&pasting the fallback logic is a chore.
Instead (if this should exist, I don’t have an opinion on that yet), make the new method mandatory in private.ImageDestination, and deal with the fallback in the existing FromPublic wrapper.
| // For digest agility: if the digests use different algorithms but both are valid, | ||
| // skip the validation rather than failing. This allows images with non-canonical | ||
| // DiffIDs to be pulled successfully. | ||
| if trusted.diffID != "" && untrustedDiffID != "" && |
There was a problem hiding this comment.
This is a critical security check and just skipping it is not an option.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Below, layerID is used to compute the actual ID we store the data under; and that hard-codes sha256, i.e. is exposed to sha256 collisions. If we want to provide “sha512 level of collision resistance”, that breaks the goal.
(I don’t know what is the right digest to use, considering traditional sha256-only images, sha512-only images, mixed images, and the c/storage option.)
Why is there vendoring at all, we don't vendor the deps here in this repo as renovate cannot handle that yet?! |
My bad, I committed the vendor dir evidently. I'll get rid of that.
@mtrmac Mostly this. Goal is to not break anything related to sha256, and additonally, be able to build, run, push, inspect sha512 images. Lookup by image ID should mostly work for sha512 images (i've noticed an exception with a sha512 image built from a simple Containerfile What I'm looking to do in podman: containers/podman#27170 I'll address more in followups, but hope this clarifies some things. |
Great, let’s go for that.
is a pathological case where the current design breaks in that situation, so perhaps useful as a design test case. |
Just struck me that a storage.conf option to set digest would not go well with ^, so I'll get rid of that part, and only have sha512 additions via --digest for now as in the podman PR. So, AIUI, that would mean proceeding with:
|
|
I think we’ll be figuring out details and corner cases over time — that’s fine. Just the one decision that “transparent interoperability, including cross-digest lookups, between sha256 and sha512 is not a goal” simplifies things, and we can refer to it later when discussing the corner cases. |
See commits. There's a HUGE vendoring update involved. I can split this into multiple PRs (one each for storage, image and common) if it makes things easier. LMK.