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: Policy verification of OCI Image before pulling #2029

Merged
merged 1 commit into from
Jul 4, 2023

Conversation

RishabhSaini
Copy link
Contributor

Helps address the lack of policy verification in rpm-ostree:
coreos/rpm-ostree#4272

An Image pull triggered by rpm-ostree rebase will look to verify the policy by default as a part of OpenImage

The other route taken here adds a new API call
#1829

@RishabhSaini
Copy link
Contributor Author

The above change causes the following output if a container image signed by sigstore is given the wrong key:

[root@cosa-devsh ~]# rpm-ostree rebase ostree-image-signed:docker://ghcr.io/ahgencer/silverblue:pr-3
Pulling manifest: ostree-image-signed:docker://ghcr.io/ahgencer/silverblue:pr-3
error: Creating importer: Failed to invoke skopeo proxy method OpenImage: remote error: cryptographic signature verification failed: invalid signature when validating ASN.1 encoded signature

@RishabhSaini RishabhSaini force-pushed the sig branch 2 times, most recently from 92beea2 to 1cf4feb Compare June 29, 2023 23:23
Copy link
Contributor

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

Yeah, OK I think this makes sense to just do by default.

Are there cases where we want the equivalent of skopeo inspect --insecure-policy? Hmm...perhaps not.

@cgwalters cgwalters added the experimental-image-proxy Issues related to experimental-image-proxy label Jun 30, 2023
@cgwalters
Copy link
Contributor

As a procedural note I've created experimental-image-proxy label to track PRs to this subsystem.

@RishabhSaini
Copy link
Contributor Author

Rebasing to sync the branch

@RishabhSaini
Copy link
Contributor Author

RishabhSaini commented Jun 30, 2023

Unsure why the CI is failing "timed out". The tests for ostree-rs-ext passed on the previous run and work locally on my end. Would the owners kindly re-rerun the ostree-rs-ext test as I suspect it was a flake

@cgwalters
Copy link
Contributor

There's a "Re-run" button next to the check...I clicked it, but so far not seeing a change?
Anyways you should be able to force it with git commit --amend --allow-empty or so and re-pushing

@mtrmac
Copy link
Contributor

mtrmac commented Jun 30, 2023

FWIW we do see the ostree-rs-ext timing out fairly frequently. Either it takes just a few minutes, or it times out after 30.

@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Jun 30, 2023
@cgwalters
Copy link
Contributor

FWIW we do see the ostree-rs-ext timing out fairly frequently. Either it takes just a few minutes, or it times out after 30.

Ah, dang. OK, I'm reopening ostreedev/ostree-rs-ext#322 (comment)

Signed-off-by: RishabhSaini <rsaini@redhat.com>
@RishabhSaini
Copy link
Contributor Author

Updated

@@ -266,6 +268,23 @@ func (h *proxyHandler) openImageImpl(args []any, allowNotFound bool) (replyBuf,
return ret, err
}

unparsedTopLevel := image.UnparsedInstance(imgsrc, nil)
policy, err := signature.DefaultPolicy(h.sysctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

While I think this is OK for now, maybe it makes sense to cache this in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, cache the PolicyContext (as long as it is used from a single thread).

(Right now that does not make a difference, but a branch caching the imported keys in a native format inside a PolicyContext is floating around.)

@cgwalters
Copy link
Contributor

I was thinking more about this and one problem, and this is basically what motivated #1829 is that basically well I kind of originally painted us into a corner in having e.g. ostree-unverified-image...which kind of promises to ignore the container signature policy. This PR changes it to enforce the policy.

But then...in practice the policy does default to being unset except for a few special base images in e.g. RHEL. So...we may mostly be OK here.

I also think issue 1829 is still highly relevant in that we do want to drop the ultra-hacky code in https://github.com/ostreedev/ostree-rs-ext/blob/main/lib/src/container/skopeo.rs#L10

Anyways...I think we're probably OK. I want to do some more testing of this PR myself. To be clear did you do the whole thing with setting up e.g. cosign and requiring it in the container policy?

@RishabhSaini
Copy link
Contributor Author

Did you do the whole thing with setting up e.g. cosign and requiring it in the container policy?

Yes, I have tested this system by singing and uploading a signed container image to my registry using cosign and skopeo. Then rebasing from the remote registry behaves appropriately.

if err != nil {
return ret, err
}
allowed, err := policyContext.IsRunningImageAllowed(context.Background(), unparsedTopLevel)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here'd what we want is a new API like IsRunningImageSigned and it would fail if the signature policy is insecureAcceptAnything for ostreedev/ostree-rs-ext#79

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.

A late review I’m afraid.

@@ -266,6 +268,23 @@ func (h *proxyHandler) openImageImpl(args []any, allowNotFound bool) (replyBuf,
return ret, err
}

unparsedTopLevel := image.UnparsedInstance(imgsrc, nil)
policy, err := signature.DefaultPolicy(h.sysctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there a specific reason not to use opts.global.getPolicyContext here?

That would also immediately handle --insecure-policy per #2029 (review) .

policyContext, err := signature.NewPolicyContext(policy)
if err != nil {
return ret, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

policyContext.Destroy is required by the API when done.

if !allowed || err != nil {
return ret, err
}
if !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 is dead code AFAICS. The code has already returned ret, nil above.

@mtrmac
Copy link
Contributor

mtrmac commented Jul 18, 2023

A late review I’m afraid.

Should be resolved by #2048.

cgwalters added a commit that referenced this pull request Jul 19, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
experimental-image-proxy Issues related to experimental-image-proxy kind/feature A request for, or a PR adding, new functionality locked - please file new issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants