Ensure content digest exists when pulling bundle via tag#1306
Ensure content digest exists when pulling bundle via tag#1306vdice merged 3 commits intogetporter:mainfrom
Conversation
…via tag Signed-off-by: Vaughn Dice <vadice@microsoft.com>
| dep.CNABFile = cachedDep.BundlePath | ||
| dep.RelocationMapping = cachedDep.RelocationFilePath | ||
|
|
||
| invocationImage := cachedDep.Bundle.InvocationImages[0] |
There was a problem hiding this comment.
Do you think we could check the digest when we pull the bundle, instead of later when we try to run it? That way the logic would be in a single spot instead of split across dependencies and the other code paths?
There was a problem hiding this comment.
Good call, thank you. Updated.
|
|
||
| invocationImage := cachedDep.Bundle.InvocationImages[0] | ||
| if invocationImage.Digest == "" { | ||
| return fmt.Errorf("no content digest is present for dependency image %s", invocationImage.Image) |
There was a problem hiding this comment.
So if I saw this error, I would have no idea what to do about it or what the problem is. Let's try to explain the security issue and how the invocation image for the bundle doesn't have a digest so we can't verify that it is is the image referenced by the bundle and hasn't been tampered with. Or something like that.
Basically we need to throw the bundle under the bus, say it's invalid and could be compromised.
There was a problem hiding this comment.
Error text has been updated to better convey the issue around this scenario. Thank you!
| "images": null, | ||
| "invocationImages": [{ | ||
| "digest": "sha256:f858bc025ad34099fe67ebe6152e03b4c91b34cc7a77d1aa10aaf1dc1389c2c2", | ||
| "contentDigest": "sha256:f858bc025ad34099fe67ebe6152e03b4c91b34cc7a77d1aa10aaf1dc1389c2c2", |
There was a problem hiding this comment.
Did you see this same mistake anywhere else in our repo? Would you mind taking a quick look?
There was a problem hiding this comment.
Ooh, yea, there are def other spots. Good call. In fact, for the images/ImageMap part of a manifest, the yaml tag is still digest, so there will be quite a few changes. Do we want to add these as an additional commit to this PR or as a follow-up?
| * deislabs/kubekahn:1.0 -> 887e7e65e39277f8744bd00278760b06 | ||
| * getporter/mysql:v0.1.2 -> 972978d3b715212b783a70fdc3449584 | ||
| * getporter/wordpress:v0.1.2 -> bfa9f2b528338f9ac1bc9b8b984c3997 | ||
| * getporter/mysql:v0.1.3 -> 4f17468b1ad86cb50cd76d9b8148d249 |
There was a problem hiding this comment.
Thanks for updating my beloved cheat sheet!
There was a problem hiding this comment.
I backed out the changes in this directory now that we've placed the logic in PullBundle (and, hence, cached bundles wouldn't hit this case). For another day!
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
vdice
changed the title
Signed-off-by: Vaughn Dice <vadice@microsoft.com>
vdice
force-pushed
the
feat/ensure-content-digest
branch
from
f1206bb to
ca87a05
Compare
2 participants
What does this change
What issue does it fix
Closes #1305
Note that the other items mentioned in #1305 are already present in the code base, namely:
Notes for the reviewer
n/a
Checklist
If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇♀️