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

proxy: Add OpenImageWithRequiredSignatures #1829

Open
cgwalters opened this issue Dec 15, 2022 · 9 comments
Open

proxy: Add OpenImageWithRequiredSignatures #1829

cgwalters opened this issue Dec 15, 2022 · 9 comments
Labels
kind/feature A request for, or a PR adding, new functionality stale-issue

Comments

@cgwalters
Copy link
Contributor

In the bootc/ostree-container effort, I am trying to enforce signatures being enabled by default. The thing is, we kind of say that e.g. podman run <some image from docker hub or whatever> is "secure" - in the sense I'm using the word, we can and do fix security problems we find (mostly in the kernel) in a relatively timely fashion.

But booting a container (or running with --privileged as well as some more subtle options) completely change that.

As part of the proxy, I'd like to add an OpenImageWithRequiredSignatures API that requires that the remote image is signed in some way configured in containers-policy.json - IOW that the policy for fetching the image does not fall through to insecureAcceptAnything.

(I think it would make sense to also add podman pull --sigpolicy=required or so)

When I looked at this, it seemed feasible but would require some changes in c/image. Let me know if you have any thoughts.

@mtrmac
Copy link
Collaborator

mtrmac commented Dec 15, 2022

Just to reply quickly, some unorganized thoughts:

My first thought: And would the caller of this API then have an override toggle to disable this, for debugging and troubleshooting? If so, is that so much different from allowing insecureAcceptAnything in the policy in the first place?

The RHEL default policy.json handles the Red Hat registries. Is the goal basically an extra safeguard against something/someone removing that?

… but then the OpenShift default is https://github.com/openshift/machine-config-operator/blob/9c6c2bfd7ed498bfbc296d530d1839bd6a177b0b/templates/worker/01-worker-container-runtime/_base/files/policy.yaml . So… that just wouldn’t work as is.

The specially-designed system that uses bootc (or OpenShift’s MCO) might be able to manage the policy.json to ensure there is an entry for the bootc repo. (Well… except that c/image doesn’t really have an “edit policy” API to modify an user-created file; there is an independent implementation in podman image trust).

For some use cases, running skopeo --policy /some/bootc-specific/policy.json might be appropriate, completely separating the “can sign the OS” and “can sign containers that run on the OS”. OTOH if this is supposed to just rely on the default vendor signatures (and just use a separate repo for those boot images), it does make a lot of sense to use the global policy instead of duplicating “is the vendor’s signature valid” policy in two places.


If the goal is to specifically use the system-wide policy, however it was configured, but force the user to set the policy up to be enforcing, yes, that would be a new feature. (It might be possible to do that externally to c/image, but that would run into TOCTOU issues if we assume a concurrently-managed policy.json file.)

Implementation-wise, adding a new option to PolicyContext.IsRunningImageAllowed would mean adding a new method beside; and then copy.Options could be used to forward that configuration on. Maybe then PolicyRequirement.isRunningImageAllowed could get a new parameter that causes prInsecureAcceptAnything to reject. So, that seems quite feasible to implement — whether it needs to exist is really the question I’m not sure about.

Another question: Should this reject insecureAcceptAnything, or specifically accept both (or only one of??) signedBy, sigstoreSigned)?

@cgwalters
Copy link
Contributor Author

Is the goal basically an extra safeguard against something/someone removing that?

Kind of, but really a more important goal here for me is to create a "speed bump" for anyone doing custom derived images from their own registries and to strongly encourage signing in some form.

In case you didn't see, just recently: https://blog.lightspin.io/aws-ecr-public-vulnerability

For some use cases, running skopeo --policy /some/bootc-specific/policy.json might be appropriate, completely separating the “can sign the OS” and “can sign containers that run on the OS”.

Hmm. Yes, I think arguably bootc should support /etc/bootc/policy.json (and normal systemd-style /usr and /run equivalents) indeed.

But it's very much also a feature that bootc reads the standard container configuration in general - particularly things like containers-registries.conf.

(It might be possible to do that externally to c/image, but that would run into TOCTOU issues if we assume a concurrently-managed policy.json file.)

This is how I did it today actually, see the code in https://github.com/ostreedev/ostree-rs-ext/blob/main/lib/src/container/skopeo.rs which is definitely too ugly to live much longer -
hence this issue.

So, that seems quite feasible to implement — whether it needs to exist is really the question I’m not sure about.

Fair - does the above seem convincing?

Should this reject insecureAcceptAnything, or specifically accept both (or only one of??) signedBy, sigstoreSigned)?

I was thinking it's just about rejecting insecureAcceptAnything; we'd support any signing mechanism configurable today or in the future.

@mtrmac
Copy link
Collaborator

mtrmac commented Dec 16, 2022

Is the goal basically an extra safeguard against something/someone removing that?

Kind of, but really a more important goal here for me is to create a "speed bump" for anyone doing custom derived images from their own registries and to strongly encourage signing in some form.

That’s a good point.

OK, let’s do this.

@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Dec 16, 2022
@mtrmac
Copy link
Collaborator

mtrmac commented Dec 16, 2022

OpenImageWithRequiredSignatures

Looking at OpenImageOptional, wouldn’t an OpenImageWithOptions and an extensible options object be warranted by now?

cgwalters added a commit to cgwalters/skopeo that referenced this issue Jan 10, 2023
See containers#1829 (comment)

This is prep for adding another option like `require-signatures`.
cgwalters added a commit to cgwalters/skopeo that referenced this issue Jan 10, 2023
See containers#1829 (comment)

This is prep for adding another option like `require-signatures`.

Signed-off-by: Colin Walters <walters@verbum.org>
cgwalters added a commit to cgwalters/skopeo that referenced this issue Jan 10, 2023
See containers#1829 (comment)

This is prep for adding another option like `require-signatures`.

Signed-off-by: Colin Walters <walters@verbum.org>
cgwalters added a commit to cgwalters/skopeo that referenced this issue Jan 10, 2023
See containers#1829 (comment)

This is prep for adding another option like `require-signatures`.

Signed-off-by: Colin Walters <walters@verbum.org>
@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@cgwalters
Copy link
Contributor Author

/lifecycle frozen

@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Apr 12, 2023

Not sure what
/lifecycle frozen
is supposed to do?

@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature A request for, or a PR adding, new functionality stale-issue
Projects
None yet
Development

No branches or pull requests

3 participants