Validate digests of data downloaded while fetching sigstore attachments #2689
Conversation
This is not a security vulnerability because the registry can just as well send a manifest modified to match, but doing this correctly protects us in case this function were used for other purposes in the future. Fixes containers#2687. Signed-off-by: Miloslav Trmač <[email protected]>
| if !digestAlgorithm.Available() { | ||
| return nil, fmt.Errorf("invalid digest %q: unsupported digest algorithm %q", desc.Digest.String(), digestAlgorithm.String()) | ||
| } | ||
|
|
||
| reader, _, err := c.getBlob(ctx, ref, manifest.BlobInfoFromOCI1Descriptor(desc), cache) |
There was a problem hiding this comment.
This looks fine, but I think it'd be even clearer to merge getBlob and this function. Or at a minimum, rename getBlob to getRawBlobNoChecksumVerification or something...
There was a problem hiding this comment.
getBlob is primarily used by the public dockerImageSource.GetBlob, which must support streaming very large objects, and inherits both its interface and its (non-digesting) semantics.
And GetBlob is (sadly?) a public stable API. Having it automatically digest the contents would be interesting in the abstract, but all existing callers would should already be digesting it themselves, and doing that twice is rather costly, so I don’t think it makes sense to change the behavior of the existing function.
There was a problem hiding this comment.
OK sorry, I thought it was only called here.
There was a problem hiding this comment.
… another point, we don’t want every single transport to implement the digest validation itself; centralizing that in c/image/copy makes it easier to prove that the validation is always happening.
But at least the naming point is very apt; if we were ever redesigning the ImageSource API (it should be an object with methods, not an interface), GetBlob should be definitely renamed to something that emphasizes the caller needs to validate the contents — or maybe this should be centralized into the ImageSource-replacing wrapper object.
mtrmac
added
the
kind/bug
This is not a security vulnerability because the registry can just as well send a manifest modified to match, but doing this correctly protects us in case this function were used for other purposes in the future.
Fixes #2687.