Add image verifier transfer service plugin system based on a binary directory

[ethan-lowman-dd]

Implements #6691

See the added docs for configuration details. To test, run a command like ctr image pull --local=false index.docker.io/library/alpine@sha256:c75ac27b49326926b803b9ed43bf088bc220d22556de1bc5f72d742c91398f69.

The simplest verifier binaries for testing can be something like this:

#!/usr/bin/env bash

echo Approved

or

#!/usr/bin/env bash

echo Rejected
exit 1

Real implementations would need to parse the -name and -digest flags.

##### Image Verification Plugins

The transfer service now support image verification plugins.

[k8s-ci-robot]

Hi @ethan-lowman-dd. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ktock]

Left some comments on documentation.
Is this CRI-specific feature (i.e. should be implemented under pkg/cri) or does this expand containerd's scope?

[ktock]

Are there any use cases where the plugin is registered but enabled = false?

[ethan-lowman-dd]

I primarily added this opt-in flag because I think it's useful to output logs about image verification before/after calling the plugin, but if the binary directory is empty (i.e. no verifications are happening, the logs are basically noise.

[ktock]

What is the expected verification the plugin performs based on name and digest?

[ethan-lowman-dd]

They can do essentially anything, such as:

1. compare registry against a trusted list of registries
2. block the latest tag
3. block pulling images without digest references.
4. check image signatures (this would probably be the most popular use case -- see [transfer] plugin to transfer service for image verification #6691 for details)

[ethan-lowman-dd]

Is this CRI-specific feature

@ktock I had previously prototyped an implementation of this feature in the CRI (here: #6994) but the feedback from maintainers was that it should live somewhere closer to the core so all containerd clients can benefit, not just the CRI. In particular, the Transfer service was named as the ideal integration spot.

[samuelkarp]

   * 002cf10b "Prototype image verifier transfer service plugin system based on a binary directory" ... FAIL
    - PASS - commit does not have any whitespace errors
    - FAIL - does not have a valid DCO
    - PASS - commit subject is under 90 characters, but is still more than 72 chars

CI is failing since the commit is missing a sign-off. See the contribution guidelines for instructions.

[ruiwen-zhao]

/cc

[k8s-ci-robot]

@ruiwen-zhao: GitHub didn't allow me to request PR reviews from the following users: ruiwen-zhao.

Note that only containerd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[samuelkarp]

I really like a lot of this. As we get closer on a few of the open questions (and this moves from "WIP" to more ready to merge) we'll want to make sure we add some unit and integration tests.

[akashsinghal]

Hi @ethan-lowman-dd :) Appreciate the work you're doing here on adding the verification capability. I actually built a very rudimentary prototype binary for Ratify for this and got it working with notation verifier configured. You can check it out here. Thought I'd point you to it if you're curious. Ratify is a verification engine for supply chain artifacts and can be configured with many different verifiers. It's been primarily used as a service working with an admission controller but the same principal could maybe be applied here.

[ethan-lowman-dd]

@samuelkarp @dmcgowan It seems like there's consensus on the design, so I went ahead and added tests, which takes this PR from WIP to something looking for a proper review.

[vishal-chdhry]

Hi @ethan-lowman-dd, Great job on the plugin btw!
I had a question,
How should we pass the signature policy to provide the rules for image verification?
Is there already a way to do that?
I think we consider adding something like --signature-policy which will take the file location as input

[ethan-lowman-dd]

@vishal-chdhry This plugin system does not assume that the verification involves signatures at all. There could be an implementation of a plugin that validates using a policy.json somewhere on disk, but I believe that would be outside the scope of containerd.

[ethan-lowman-dd]

I think 1905eff should do the trick. I was killing the exec'ed process, but by default on Unix systems, that doesn't kill grand-child processes, leaving them orphaned if they don't exit quickly on their own. I added a unit test to check for orphaned processes similar to the one that was failing by chance in a different package.

Also, since I'm not super familiar with the Windows process model, I wanted to make sure the bindir tests ran on Windows too. I updated the testing strategy in bindir_test.go to use binaries built by the Go toolchain at test time instead of Bash scripts to support Windows. (Using the Go toolchain in Go tests may raise some eyebrows, but it's something that the Go standard library does in a few places.)

[ethan-lowman-dd]

It might take me a bit of time to get this working on Windows (this is my first time building anything against Windows). I need to wrap my head around the differences in the Windows process model to avoid the two issues we have remaining affecting only Windows:

• when stdout reading is truncated, the child process is exiting with code 1
• child processes of verifiers are still leaking when verifiers are killed (having trouble finding a good way to kill a process and all its children on Windows)

[ethan-lowman-dd]

I made an attempt to avoid orphan processes on Windows in this commit (not currently part of this PR):
ethan-lowman-dd@0eaa502

It's based on ideas I found here: https://devblogs.microsoft.com/oldnewthing/20131209-00/?p=2433

That commit doesn't current work -- AssignProcessToJobObject returns "Access Denied" and I'm not sure why. If there's someone who actually knows Windows who could give a hand, I'd really appreciate that :)

Or alternatively, we could let it be the case that orphan processes are a possibility on Windows. There doesn't seem to be good native Go support for avoiding that -- everything's through the Windows syscall package.

[ethan-lowman-dd]

With the advice of some colleagues, I got things working on Windows in: 6e83767

[ethan-lowman-dd]

/retest

[ethan-lowman-dd]

The failing e2e tests looks to be this flake: kubernetes/kubernetes#119600

But otherwise CI is green now. @dmcgowan This probably needs another look since I had to change a few things related to process management.

I will squash the commits, but I'll keep them for now so it's easy to see what changed since the first approvals.

[dmcgowan]

/retest

[dmcgowan]

@ethan-lowman-dd looks good, if the Windows stuff works, I'll believe the tests. Otherwise some Windows folks can comment on it. Can you squash some of those middle commits down and then I think it looks ready to go.

[ethan-lowman-dd]

Squashed!