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

proposal: verify-manifest: Traversing Kubernetes Resources and Verify the Given Images #437

Closed
developer-guy opened this issue Jul 15, 2021 · 22 comments

Comments

@developer-guy
Copy link
Member

developer-guy commented Jul 15, 2021

Abstract

Slightly the same with verify-dockerfile, but for Kubernetes Resources, responsible for creating containers such as Deployments, ReplicaSets, DaemonSets.
etc. The same logic we used to check ImageRefs in verify-dockerfile command can simply traverse the whole resource to check given images actually signed.
There is an alternative way of doing the same with creating ValidatingAdmissionWebhook like a project cosigned. But with this command, we can verify the whole images as early as possible in the CI pipelines before applying them into the Kubernetes cluster.

Implementation

We may have to write a Resource decider to correctly decide where we should get the image ref from the manifest by looking at the kind of the Resource. For example like the following examples:

Kind: Deployment
|
|-> spec.template.spec.containers[*].image

Kind: Pod
|
|-> spec.containers[*].image

After founding the images, we should verify them one by one against the public key or keyless.

Usage

Maybe we can give this command the following name: verify-manifest.

$ cosign verify-manifest -key cosign.pub <path/to/manifest>

To propose as simple as possible, I don't want to add all the other options that we can suğpport, but we can support all the other commands that verify-dockerfile currently supports.

cc: @Dentrax

@Dentrax
Copy link
Member

Dentrax commented Jul 16, 2021

KIND IMAGE PATH
Deployment spec.template.spec.containers[*].image
DaemonSet spec.template.spec.containers[*].image
ReplicaSet spec.template.spec.containers[*].image
StatefulSet spec.template.spec.containers[*].image
CronJob spec.spec.template.spec.containers[*].image

@dlorenc
Copy link
Member

dlorenc commented Jul 16, 2021

This is awesome! I've also heard another use case that might be good to capture here: "inlining" the signatures for each image nto the k8s object itself as an annotation. That would make verification by a policy system easier.

@developer-guy
Copy link
Member Author

oh nice, yes we would definitely consider that once we start to implement this, what do you suggest we use as an annotation key, is there any standard key for it ?

@dlorenc
Copy link
Member

dlorenc commented Jul 16, 2021

No, not that I know of yet!

@dlorenc
Copy link
Member

dlorenc commented Jul 22, 2021

Does anyone want to take a stab at the "inlining" aspect?

@dlorenc
Copy link
Member

dlorenc commented Jul 22, 2021

Here's a possible UX:

cosign verify-manifest (similar to blob and image syntax)
cosign inline-attestation -f manifest.yaml -key key [-attestationType=something] [-annotation=something]

@joshes
Copy link
Contributor

joshes commented Jul 25, 2021

I made a few comments over on #38 and was made aware of this thread. I think this is a good idea and could possibly dovetail nicely into that issue as a downstream side-effect, perhaps as a separate flag on the verify or a more explicit cli option outright.

Verify only:

cosign verify-manifest -key cosign.pub <path/to/manifest>

Verify and write meta (on verify):

cosign verify-manifest -write-meta -key cosign.pub <path/to/manifest>

Or explicitly:

cosign write-meta-manifest -key cosign.pub <path/to/manifest>

Once that exists, it doesn't feel like #38 would be required.

I can try and get something together this coming week if this sounds like an ok start. Probably two PR's would be incoming, one for the verify and another follow-up with the metadata write, they'd be closely linked as it could be done as part of the validation pass.

@joshes
Copy link
Contributor

joshes commented Jul 26, 2021

Question around the manifest parser implementation.

Happy path parsing via regex solves for YAML resources and gets this out quickly which is a good thing. But, it is regex and wouldn't support JSON resources OOTB. We could feature-add JSON support by traversing the object looking for "image" keys to keep it simple - but wanted some feedback on this.

Should we enforce a .yaml extension for sane error for the time-being and also, how complex of an image "finder" should we consider?

Simple is better is my opinion.

[Edit]
Also available for discussion: thinking if we add explicit support for manifests which I mentioned in #38 (e.g., push a signed manifest to a registry and perform validation on the resource itself) then we may want to rename this from verify-manifest to something more explicit like verify-manifest-images.

@pafco007
Copy link

pafco007 commented Jul 27, 2021 via email

@pafco007
Copy link

pafco007 commented Jul 27, 2021 via email

@dlorenc
Copy link
Member

dlorenc commented Jul 30, 2021

Actually, instead of "verify-manifest", what if we move this into the "verify" command itself? It could take an image URL or a path to a local manifest file.

Im worried now, because I'd also like to add "verify-attestations" for "manifests", so flattening "manifest" into the verify/verify-attestation commands might be nicer.

@joshes
Copy link
Contributor

joshes commented Jul 30, 2021

I posted this on Slack yesterday and wondering if its applicable here or not (apologies for the cross-post)

... We are verifying Images, Dockerfiles & now Manifests. Do we see this continuing with specialized verifiers? If so I may propose a pluggable verifier system so you could add them generally and run them akin to a kubectl plugin (e.g., cosign verify -key ... ). OR…do we think the verifiers we have will be the generally supported ones?

I like the idea of flattening it to basically a URI or Manifest, so long as that covers the expected future wants of cosign and we are explicit about what a Manifest is and expectations. I could imagine arbitrary verifiers being wanted by future users though for unhandled use-cases.

@dlorenc
Copy link
Member

dlorenc commented Jul 30, 2021

Ah, now I get it! Yeah, "verifiers" would be "the thing that lists the objects to verify"?

@joshes
Copy link
Contributor

joshes commented Jul 30, 2021

Kind of. Here's an example session of what I think would be a good ux (IMO).

# List 
cosign list-verifiers
Available verifiers:
  image (default)
  dockerfile
  docker-compose
  manifest (json or yaml k8s resources)
  my-custom-verifier
 
# Still use image as default verifier - no change, no breaky!
cosign verify -key key.pub <IMAGE>

# Verify dockerfile
cosign verify -key key.pub --verifier dockerfile <URI or Path to Dockerfile>

# Verify manifest
cosign verify -key key.pub --verifier manifest <URI or Path to Manifest>

If the system was pluggable, new verifiers could be added without being so explicitly in the main cosign repository which may add to a more narrower surface area for cosign directly (e.g., "core" functionality vs extended functionality).

So requirements would be:

  • Basic plugin system (effectively a driver to add to list of verifiers, load URI or file path, execute verifier itself)
  • Verifier interface itself which would need to get/set cli opts and do the custom validation logic on the resource type
  • The verifier implementation(s)

Of course this is all overkill if cosign will ever only want to validate Dockerfiles and Manifests.

Food for thought.

@Dentrax
Copy link
Member

Dentrax commented Jul 30, 2021

list-verifiers sounds cool idea! We may do not have to pass --verifier flag since if we check and recognize the input format. Same idea fits to list-signers command, IMO.

@dlorenc
Copy link
Member

dlorenc commented Jul 30, 2021

  • Basic plugin system (effectively a driver to add to list of verifiers, load URI or file path, execute verifier itself)

I'd really like to avoid going with an extra install/plugin system. Keeping everything in-tree here would be fine with me!

Do you think we would actually need the --verifier flag in all cases, or could we sometimes detect the type of file automatically?

@joshes
Copy link
Contributor

joshes commented Jul 30, 2021

Seems we wouldn't require the --verifier flag so no, which would be much simpler overall. After thinking on it more, what you propose makes sense, just keep the verifiers here (in-repo).

Sounds good!

@dekkagaijin
Copy link
Member

dekkagaijin commented Sep 15, 2021

I only just saw the latter part of this discussion 😔

I have concerns about the usability and maintainability of overloading verify with different image discovery logic for each type of artifact we want to scan. With the intention of eventually adding additional artifact-specific commands, I went ahead and re-organized verify-dockerfile into dockerfile verify. The next step would be to add a command which resolves images to their digests, allowing users to avoid race conditions between verify and an image pull.

@dekkagaijin
Copy link
Member

/cc @n3wscott

@hectorj2f
Copy link
Contributor

@dekkagaijin Do we still need this issue open ?

@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions
Copy link

This issue was closed because it has been stalled for 5 days with no activity.

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

No branches or pull requests

7 participants