-
Notifications
You must be signed in to change notification settings - Fork 54
Conversation
@simonferquel Per #233, we wish to validate the digest matches what is in the bundle.json for a given invocation image. Is the first commit sufficient (i.e. just perform a DistributionInspect) or should we get the manifest and recompute the digest to verify (second commit, using the containerD libraries)? |
I'm not sure I understand this PR. The comment says that we want to validate against the |
pkg/action/action.go
Outdated
@@ -71,6 +71,7 @@ func opFromClaim(action string, c *claim.Claim, ii bundle.InvocationImage, creds | |||
Parameters: c.Parameters, | |||
Image: ii.Image, | |||
ImageType: ii.ImageType, | |||
ImageDigest: ii.Digest, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@technosophos if the digest is present for the invocation image in the bundle.json (as https://github.com/deislabs/cnab-spec/blob/master/101-bundle-json.md indicates is should be) , won't it be set here?
opFromClaim gets invoked on the install command AFAICT. I modified the hello helm bundle.json to include the digest (none of them seem to have that in the samples repo now). With this change, that ends up in the docker_driver.go
@technosophos I tried to comment in line where I believe that happens (opFromClaim..). Did I misread that part of the spec:
So in theory, it should be there and when the invocation image config is loaded from from the bundle.json, it should be present (none of our examples do that currently though). I'm working on adding the logic to fetch the actual blobs to verify things right now, I should have put a WIP on the title. (oops I did and accidentally removed it) |
Oh! Yes, your read is correct. In But, yeah, without the WIP, I thought this was all done and couldn't figure out what I was reviewing. Sorry. |
@jeremyrickard actually I have no idea about this one :(.
Also note that if the image has already been pushed by the user using docker CLI, doing an inspect call on that image should report the digested reference in addition to the tagged reference (and so we can skip the "push image phase"). Also if the local bundle has a digested reference, we also can skip all this and directly push the bundle |
81d1776
to
9e232d7
Compare
From discussions last week with @itowlson and @radu-matei , I believe we're planning on ripping that image tag generation code in favour of a EDIT: see #361 for follow-up discussion on that topic As far as the push proposal, that sounds like how we'd want to push a "thick" bundle to a registry, right? As far as I understand we may not support pushing a thick bundle to a registry, as the use cases where a thick bundle is required are limited in scope to "export to a USB stick and run it on an IoT device". @technosophos did we get any feedback on whether an exported bundle should be push-able to a registry? That being said, if we do intend to support pushing thick bundles, I'm a little concerned about patching and re-signing a bundle without the user being able to weigh in on whether the bundle should be patched/re-signed. I like the idea to rely on digests, but |
I've amended this so that it has a single commit now. Summary:
I opened a PR in the cnab-spec repo to make changes to the |
Closes: #233
9e232d7
to
0c9f2eb
Compare
pkg/digest/docker_digester.go
Outdated
h := remotes.FetchHandler(cs, fetcher) | ||
// This traverses the OCI descriptor to fetch the image and store it into the local store initialized above. | ||
// All content hashes are verified in this step | ||
if err := images.Dispatch(ctx, h, desc); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For anyone wondering how this validates the digest....
This ends up fetching the descriptor from the registry, then fetching the blob associated with that. then it does the same for children layers. When they are written to the temporary file storage, the hash is calculated for each blob. If this is successful (this is via the fetch handler). It's essentially doing a pull, which then gets deleted.
pkg/action/install.go
Outdated
return fmt.Errorf("unable to get image validator: %s", err) | ||
} | ||
ctx := context.Background() | ||
if invocImage.Digest != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should the expected default behavior be if no digest is provided for the invocation image? From this line, it looks like if there is no digest, we do not validate.
I think we should require a digest and emit an error by default and for when someone is developing a bundle, we should override that default behavior with a flag. I'll have to check if the spec says anything about requiring a digest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is currently skipping if there is no digest in the bundle. I agree that we probably should enforce it being present, I think the spec said it must be there and we haven’t enforced that. Any suggestion on what the flag should be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I meant to leave a comment saying we should fix the bundles up and then fix this, but I forgot :(
Needs a quick rebase |
Updated the PR to.....
|
Should we go ahead and close this one out @jeremyrickard ? |
Validate docker image digest before doing a Docker pull on
duffle install
Updates action/driver code to pass the Digest along with image so it can be validated.
Closes: #233