[WIP] feat: Implement CRI image verification plugin system #6994
Conversation
|
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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
| # to handling runtime verification of container images. | ||
| [plugins."io.containerd.grpc.v1.cri".image_verification] | ||
| # address specifies address of the TTRPC plugin handing image verification | ||
| # requests. |
There was a problem hiding this comment.
We use gRPC for most plugins
There was a problem hiding this comment.
We can also consider exec()-ing a verifier binary, as in stream processor binaries and binary loggers.
(We don’t support binary loggers for CRI, but there is on-going discussion about that)
There was a problem hiding this comment.
I think TTRPC or gRPC would yield lower verification latencies than the exec approach for some plugin implementations, since long-lived resources (e.g. network connections or caches) could be reused across multiple verification requests. TTRPC was recommended in a previous meeting with @mikebrow, but from my perspective as a user of this feature, either protocol meets my requirements.
| message VerifyImageResponse { | ||
| bool ok = 1; | ||
| string reason = 2; | ||
| } |
There was a problem hiding this comment.
These fields can be just included in the error message, so probably VerifyImageResponse can be empty
There was a problem hiding this comment.
My reasoning here is that it may be desirable to have a "fail open" setting in the CRI, where errors in communicating with the image verifier plugin result in allowed images. If images are rejected by returning an error, then there is no way for the CRI to distinguish a rejected image from an error.
|
|
||
| message VerifyImageRequest { | ||
| string image_name = 1; | ||
| string image_digest = 2; |
There was a problem hiding this comment.
Can be combined into a single string image_ref
There was a problem hiding this comment.
If image_name is docker.io/library/nginx:latest and the resolved image_digest is sha256:2d17cc4981bf1e22a87ef3b3dd20fbb72c3868738e3f307662eb40e2630d4320, then I think you mean combining these into:
image_ref = "docker.io/library/nginx:latest@sha256:2d17cc4981bf1e22a87ef3b3dd20fbb72c3868738e3f307662eb40e2630d4320"
This works for my particular use case (verifying digests), but there is some loss of information here -- if the digest and image name/tag are combined into one field, there is no way for the verifier plugin to see what image was originally requested. For example, an image verifier plugin implementation that prohibited using the unsafe latest tag without a digest would not be able to distinguish an image pull of nginx (which resolves to docker.io/library/nginx:latest) and nginx:1.21.6@sha256:2d17cc4981bf1e22a87ef3b3dd20fbb72c3868738e3f307662eb40e2630d4320, where the former is unsafe but the latter is safe.
| return nil, fmt.Errorf("failed to connect to image verification plugin: %w", err) | ||
| } | ||
| client := image_verifier.NewImageVerifierClient(ttrpc.NewClient(conn)) | ||
| resolver = NewVerifyingResolver(resolver, client) |
There was a problem hiding this comment.
Perhaps this should be implemented as a client.RemoteOpt rather than a resolver?
Because potentially there can be a code path to skip calling the resolver (in a future version of containerd).
AkihiroSuda
added
status/needs-discussion
ethan-lowman-dd
force-pushed
the
cri-image-verifier-plugin
branch
2 times, most recently
from
4c84e79 to
7a9f9f9
Compare
|
Rebased on |
@ethan-lowman-dd Looking at the Todo list in this issue, #7592, it looks like CRI has not been updated to utilize the transfer service yet. It looks like it might have a chance of getting done for the 1.7 release though. |
|
I talked with @dmcgowan about this a few weeks ago, and it sounded like implementing the image verification features as a proxy plugin would align better with the long-term architecture plans for containerd. Implementing it in the CRI means the feature is unavailable for other clients, like nerdctl. I haven't gotten the time to implement the proxy plugin approach yet. I will probably do it in a new draft PR since I think the relative simplicity of the CRI approach is a useful reference to not have lost in git history, even though it probably won't be merged. |
|
@ethan-lowman-dd If there is anything I can do to assist, I'd be happy to help. |
ethan-lowman-dd
force-pushed
the
cri-image-verifier-plugin
branch
from
f0a1533 to
528823a
Compare
|
Closing in favor of #8493 |
4 participants
This PR implements #6691. If a TTRPC image verifier plugin is configured, the CRI will make a call to that plugin to decide whether an image should be pulled.
Signed-off-by: Ethan Lowman [email protected]