Enforce digest validation

[jeremyrickard]

Currently, bundles don't have digests for invocation images (and other images). The cnabio/duffle#355 PR is skipping validation when there is no digest present. Bundles should have digests and we should instead enforce the validation. This would be a breaking change at the moment (given state of bundles) so once cnabio/duffle#391 is completed, swap digest validation to be required, unless explicitly disabled.

[migmartri]

Having the digest in the invocationImage (as per the CNAB spec) would be really useful for me.

My use-case is that I am adding some automation to auto-push the container image after changes in the bundle repository. That way we do not forget to push the container image on bundle change while we implement a proper release process.

Having the digest will prevent me from "overriding" an existing image on rootfs (cnab/) changes.

[jeremyrickard]

In the last CNAB community meeting, we discussed validation in cnab-go with regard to this same issue. Note that cnabio/duffle#355 was closed a while ago without actually adding the validation in, so it's time to at a minimum revisit that and add the digest validation in. This issue is still highly relevant though, and we should tackle it in implementation of that. We still need to resolve the default behavior. Bundles built by cnab-go (i.e. duffle and other tools) are currently not inserting the digest. Indeed, this can't be done until a bundle is pushed to an OCI registry.

With that in mind:

1.) Should the validation by default assume there is a digest and fail if there isn't one?
2.) Should the validation by default skip any attempt to validate if no digest is present and not produce an error?

For either of these cases, we probably need to include a flag that inverts the default behavior (i.e. for #1, no error would be produced if the flag is provided, #2 would error if the flag is provided).

Any thoughts @chris-crone @silvin-lubecki @radu-matei @youreddy @ryanmoran @astrieanna @technosophos @carolynvs @glyn

[radu-matei]

As @ryanmoran pointed out in the last CNAB community meeting, the runtime spec does not specify anything about digest validation.

However, the bundle definition does specify the following:

During bundle development, it may be ideal to omit the contentDigest field and/or skip validation. Once a bundle is ready to be transmitted as a thick or thin bundle, it must have a contentDigest field.

In the proposed changes for cnab-to-oci, the behaviour is the following: if the bundle does not contain a digest, size, and media type, by default it populates them (by pushing the images). There is a flag to control this and avoid mutating the bundle.

My opinion is that we should enforce the digest validation at runtime (option 1).

[jeremyrickard]

In the proposed changes for cnab-to-oci, the behaviour is the following: if the bundle does not contain a digest, size, and media type, by default it populates them (by pushing the images). There is a flag to control this and avoid mutating the bundle.

Yeah, so that occurs after we "push" the bundle somewhere. So for local use/development, we'd need to do something like tool install --skip-validation for option 1. That's my preferred approach.

My original question about validation happening at all for 1.0 really relates to:

As @ryanmoran pointed out in the last CNAB community meeting, the runtime spec does not specify anything about digest validation.

[trishankatdatadog]

To me, it's still very strange that the final contentDigest for a bundle or image is obtained remotely from an image registry, instead of a local build. This allows a compromised image registry to deceive bundle or image builders into signing malicious images. This is something we should keep in mind while we design CNAB security.

I concur with @radu-matei that we should enforce digest validation at runtime (option 1) by default, and fail hard.

[jeremyrickard]

To me, it's still very strange that the final contentDigest for a bundle or image is obtained remotely from an image registry, instead of a local build.

This is a common view :)

[jeremyrickard]

@radu-matei @trishankatdatadog more relevant text:

The contentDigest field MUST contain a digest, in OCI format, to be used to compute the integrity of the image. The calculation of how the image matches the contentDigest is dependent upon image type. (OCI, for example, uses a Merkle tree while VM images are checksums.) If this field is omitted, a runtime is not obligated to validate the image. During bundle development, it may be ideal to omit the contentDigest field and/or skip validation. Once a bundle is ready to be transmitted as a thick or thin bundle, it must have a contentDigest field.

Of importance there: If this field is omitted, a runtime is not obligated to validate the image.

@ryanmoran also suggested we open a spec issue to clarify. It would seem the opposite is true, if it is there, we are obligated to validate it.

We're not obligated to validate if it's NOT there but we can, which I think is the meet of this issue.

[jeremyrickard]

Since it's always fun to rehash an issue over and over again, here is a comment from @simonferquel from a while back: cnabio/duffle#355 (comment)

 think a simpler solution would be to change a bit the logic behind build and push:

On build, generate an image tag consisting of the hash of the build context + build settings used as the image receipe (I think we only hash the build context right now)
On push,
first, push the image with this tag on the registry an retrieve the digested reference from the registry (myimage: reference would become myimage@sha256:)
patch the local bundle with the digested reference, re-sign it if it was signed
push the bundle
This way, the system would rely on un-modifiable digested-reference, and would not need to do anything locally to validate the image content.

Specifically the comment: "and would not need to do anything locally to validate the image content"

[jeremyrickard]

In the proposed changes for cnab-to-oci, the behaviour is the following: if the bundle does not contain a digest, size, and media type, by default it populates them (by pushing the images). There is a flag to control this and avoid mutating the bundle.

Yeah, so that occurs after we "push" the bundle somewhere. So for local use/development, we'd need to do something like tool install --skip-validation for option 1. That's my preferred approach.

My original question about validation happening at all for 1.0 really relates to:

As @ryanmoran pointed out in the last CNAB community meeting, the runtime spec does not specify anything about digest validation.

Ha, I meant "That's not my preferred approach". I feel like that gets cumbersome for any scenario until we are publishing bundles..and the spec says: "If this field is omitted, a runtime is not obligated to validate the image."

Oopps :)

[carolynvs]

If all tools had to validate a digest, and the only way to get a digest is to push to registry, this would make the development experience for authoring bundles frustrating. Authors either have to push constantly, a time tax, or they would have to always add this --skip-validation flag, a typing tax. I would not be surprised if the end result was that it trained people to ignore our efforts at security all the time, not just during development.

This is going to be a poor experience that I really want to avoid.

I don't know what's on the table, but if we can solve having a digest without pushing to a registry that would be ideal.

If not, I would vastly prefer a solution a solution that didn't have such an impact on the authoring loop. For instance, when the digest is blank, tools can skip validation. It only needs to be enforced when present.

[trishankatdatadog]

@carolynvs While I understand your sentiment, and they must certainly be taken into serious consideration, I am also equally worried about a world where security is opt-in, and no one ends up using it by default. Ideally, security should be opt-out and usable.

[squillace]

This is an interesting issue. I definitely want to ensure that validation is the default, which is implied but not so stated in the key line:

Once a bundle is ready to be transmitted as a thick or thin bundle, it must have a contentDigest field.

This is way too imprecise, methinks. "ready to be transmitted" is like, what precise state? I realize that validation before the bundle is moved off the disk will be quite useful for dev iteration scenarios, but I think the direction that we took with --force in the cnab spec is useful here:

1. the contentDigest MUST be populated to be "ready to be trasmitted"
2. a runtime MUST warn the user and fail if the contentDigest is not populated unless specifically overridden with a --skip-verification type call.
3. IFF the contentDigest is populated, it must be verified regardless of the --skip-verification state.

This enables users to happily use tools that can "force"-warn devs to set --skip-validation in some .settings file and respect it prior to certain commands for "readying for transmission", but enables the smooth developer workflow that does't torture the poor working developer class.

What do we think? Is that a good way through the thickets here? @trishankatdatadog?

[silvin-lubecki]

I join @trishankatdatadog thinking that we should have a secured model by default.

@carolynvs I also understand your concern about removing friction for developer, but I think that tools can handle that gracefully, like @squillace proposed in a .settings file and/or in an environment variable.

1. the contentDigest MUST be populated to be "ready to be transmitted"
2. a runtime MUST warn the user and fail if the contentDigest is not populated unless specifically overridden with a --skip-verification type call.
3. IFF the contentDigest is populated, it must be verified regardless of the --skip-verification state.

I like this behavior 👍

[trishankatdatadog]

I agree with @squillace and @silvin-lubecki. As long as we don't make security-by-default difficult for developers, we will be in a better place. I'm even willing to accept that developers can override digest validation with --skip-verification if they really, really want to, even if contentDigest is populated.

[jeremyrickard]

I've opened a pull request that introduces a digest validation interface that clients can call. It implements point 2 and point 3 in what @squillace calls out. It only supports Docker/OCI images for now, for obvious reasons.

Duffle and other clients will need to call the validation with the appropriate images. This enables duffle to do things like support image relocation. I didn't automatically insert this into the Operation and driver code path, since that stuff currently doesn't know anything aside from the Invocation Image that will be run, but I'm happy to modify that stuff (we'll need to give the operation the digest as well as a collection of images for the image map, as well as the validation flag....) and make the validation calls happen from in there.