provenance: fix possible empty digest access#3896
Conversation
If the digest for an ImageSource is the empty string, then calling `Digest.Algorithm` will panic at runtime. This scenario *can* happen if `ResolveImageConfig` returns an empty digest, but correctly returns a config object. This doesn't occur in buildkit directly, however, buildkit-in-moby implements a custom worker which also performs image lookups on local images, in which case the digest for the index may not be available (though this may be possible with the containerd image store?). Therefore, we shouldn't assume that the digest is always available in buildkit, and should instead check that is valid before inserting it into the digest set (which is already an optional map). Signed-off-by: Justin Chadwell <me@jedevc.com>
Ah, I guess this would happen when using a local image (that hasn't been pulled/pushed to/from a registry?) |
Exactly that, yup. The relevant line in moby: https://github.com/moby/moby/blob/1d68544fbfadc9807d1ddef74bb74726ae5ae63d/builder/builder-next/adapters/containerimage/pull.go#L155. I do wonder if we could potentially sometimes still have that digest if we were using the containerd image store? But that's only tangentially related to this issue. |
|
Should we have a test for this? (😅 looks like the package currently doesn't have any though 😬) |
|
We do have tests for provenance but they're in Writing a test for this is turning into a much more exciting exercise than I wanted it to be, for some reason I can't seem to reproduce any calls to Anyways, I don't think we need to block on a test, unless someone else has a much easier way to reproduce 😢 |
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
yeah, not blocking on that
🛠️ Fixes docker/buildx#1836.
If the digest for an ImageSource is the empty string, then calling
Digest.Algorithmwill panic at runtime.This scenario can happen if
ResolveImageConfigreturns an empty digest, but correctly returns a config object. This doesn't occur in buildkit directly, however, buildkit-in-moby implements a custom worker which also performs image lookups on local images, in which case the digest for the index may not be available (though this may be possible with the containerd image store?).Therefore, we shouldn't assume that the digest is always available in buildkit, and should instead check that is valid before inserting it into the digest set (which is already an optional map).