feat(docker): add verification of image digest(s)

[vdice]

• Adds invocation image digest verification per the CNAB Spec to the Docker driver.

The specific part of the spec that this PR currently implements is the following line:

If a contentDigest field is present, a runtime MUST validate the image digest prior to executing an action.

I didn't see how this would be done generically, e.g. outside of specific driver implementations, hence implementing it on the Docker driver only. Am I right here? If so, perhaps a follow-up could implement similar in the kubernetes driver.

Note some remaining TODOs/Qs inline. Feel free to respond/answer in the form of code review comments.

[vdice]

@carolynvs @radu-matei @silvin-lubecki if any of y'all have a spare moment, it would be great to get feedback on this PR, as it addresses an area where cnab-go isn't in adherence to the spec.

[carolynvs]

Looks good, I've provided some suggestions on how to use the docker library for the parsing.

[carolynvs]

Thanks! I've had this bite me a few times. Now I just need to remember to run coverage before pushing the PR. 😊

[carolynvs]

suggestion: swap the check and do an immediate return at the top instead. It's a bit easier to follow and the majority of the code in the function isn't nested in an if statement, e.g.

if image.Digest == "" {
  return nil
}

[vdice]

👍 I feel like I always miss this one; please keep reminding me! Definitely like the clarity/readability improvement.

[carolynvs]

Here's something

// RepoDigests are of the form 'imageName@sha256:<sha256>'; we parse out the digest itself for comparison
repoDigest := repoDigests[0]
ref, err := reference.ParseNormalizedNamed(repoDigest)
if err != nil {
	return err
}
digestRef, ok := ref.(reference.Digested)
if !ok {
	return fmt.Errorf("unable to parse repo digest %s", repoDigest)
}
digest := digestRef.Digest().String()

This is the updated unit test I used to verify

func TestDriver_ValidateImageDigest(t *testing.T) {
	repoDigests := []string{
		"myreg/myimg@sha256:9d4178a3b6059e46a02e4c5b39ac26d51d91046faeb7bb9f3c3825f92adafc5a",
	}

	t.Run("no image digest", func(t *testing.T) {
		d := &Driver{}

		image := bundle.InvocationImage{}
		image.Image = "myreg/myimg"

		err := d.validateImageDigest(image, repoDigests)
		assert.NoError(t, err)
	})

	t.Run("image digest exists - no match exists", func(t *testing.T) {
		d := &Driver{}

		image := bundle.InvocationImage{}
		image.Image = "myreg/myimg"
		image.Digest = "sha256:8b92b7269f59e3ed824e811a1ff1ee64f0d44c0218efefada57a4bebc2d7ef6f"

		err := d.validateImageDigest(image, repoDigests)
		assert.NotNil(t, err, "expected an error")
		assert.Contains(t, err.Error(), "content digest mismatch")
	})

	t.Run("image digest exists - a match exists", func(t *testing.T) {
		d := &Driver{}

		image := bundle.InvocationImage{}
		image.Image = "myreg/myimg"
		image.Digest = "sha256:9d4178a3b6059e46a02e4c5b39ac26d51d91046faeb7bb9f3c3825f92adafc5a"

		err := d.validateImageDigest(image, repoDigests)
		assert.NoError(t, err)
	})
}

I had to switch to full digests because the docker library doesn't parse short digests.

[vdice]

Thank you, I've incorporated these changes. Changing to full digests in the tests is also an improvement.

Also added a test case around an image having multiple repo digests.

[carolynvs]

I was sad to see livebeef/deadbeef go as test values. 😂

[carolynvs]

I think this is something that the the tool should do. In developer loops, we fully expect the digest to be left off until it has stabilized. So they (tools like porter or duffle) are in a better position to bubble up and make that warning proper.

For example, allowing it to be empty when running from a local bundle (and only displaying a warning) but perhaps displaying a stronger error if there is no digest in a published bundle.

[vdice]

I agree. I've removed this TODO and we'll leave implementation up to tools.

[carolynvs]

LGTM just one nit to resolve if you don't mind

[carolynvs]

Sorry I gave you informal code. We should probably wrap this error and provide a better error message with context. Otherwise it won't be clear what reference is invalid.

[vdice]

Updated! And deadbeef returns in a test for this scenario! 🎉

[carolynvs]

LGTM for reals this time

[carolynvs]

THE RETURN OF DEADBEEFS 👍

[carolynvs]

github is not liking my approvals

/force LGTM dagnabbit