Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add --check-policy option to skopeo inspect #2153

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AlbanBedel
Copy link

When handling images with skopeo it can be useful to be able to check that the image is valid before processing any of its metadata. With the new --check-policy flag skopeo inspect will error out if the image doesn't pass the policy check.

When handling images with skopeo it can be useful to be able to check
that the image is valid before processing any of its metadata. With
the new `--check-policy` flag `skopeo inspect` will error out if the
image doesn't pass the policy check.

Signed-off-by: Alban Bedel <[email protected]>
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

Um… There are definitely some valid use cases, but frequently users ask for things like this to build incorrect verification workflows like #560 (comment) (there’s much more in there). The balance of utility vs. risk is not just in the existence of the feature, but also in the prevalence of one vs. the other user base.

I don’t really know what to here…

Maybe name this --dangerous-{check,use,enforce}-policy and strongly warn in both in the man page and --help that this command succeeding gives no guarantees at all about the inspected image being correctly signed at any other time or for any other purpose? That would be certainly not elegant, but it would be at least some way to give naive users pause.

}()

// Be paranoid and fail if either return value indicates so.
if allowed, err := policyContext.IsRunningImageAllowed(ctx, unparsedImage); !allowed || err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be inside retry.IfNecessary because it contacts remote servers to fetch the signatures.

}()

// Be paranoid and fail if either return value indicates so.
if allowed, err := policyContext.IsRunningImageAllowed(ctx, unparsedImage); !allowed || err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, being security-critical, this should have at least minimal tests that the option works and policy is enforced (not that all possible kinds of policy are enforced), maybe somewhere around copySuite.TestCopySignatures which already creates correctly/incorrectly signed images.

@AlbanBedel
Copy link
Author

Thanks for the PR.

Um… There are definitely some valid use cases, but frequently users ask for things like this to build incorrect verification workflows like #560 (comment) (there’s much more in there). The balance of utility vs. risk is not just in the existence of the feature, but also in the prevalence of one vs. the other user base.

First of all thanks for the review. Before continuing I would like to be sure that I understand correctly which risk you are talking about. In the issue you pointed the risk was about having an image referenced by tag being later replaced by another image. But with skopeo inspect you get the digest of the image, so if from then on the image is only referenced using the returned digest this attack is not possible, or am I missing something?

My use case is to create some k8s Jobs using some parameters derived from the image metadata, so even without verification I have to reference the images by digest to ensure that the image and metadata are matching.

@mtrmac
Copy link
Contributor

mtrmac commented Nov 13, 2023

But with skopeo inspect you get the digest of the image, so if from then on the image is only referenced using the returned digest this attack is not possible

Yes. If. I’ve encountered enough users who don’t immediately, unprompted, see the need for this (and even users who are hard to convince this is necessary) to make me worried.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants