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

RFC for sigstore/cosign integration with kpack. #694

Merged

Conversation

stormqueen1990
Copy link
Contributor

@stormqueen1990 stormqueen1990 commented May 12, 2021

Initial proposal for the kpack integration with cosign.

Readable

cc @rszumlakowski @matthewmcnew

@matthewmcnew
Copy link
Collaborator

This is great! Thank you!

`cosign` uses the `DefaultKeychain` from
[`go-containerregistry`](https://github.com/google/go-containerregistry/blob/main/pkg/authn/README.md#tldr-for-consumers-of-this-package)
to authenticate the command-line interface using the machine's Docker CLI
credentials. For `kpack`, it may be possible to use the same authentication
Copy link
Collaborator

Choose a reason for hiding this comment

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

kpack makes heavy use of go-containerregistry as well 🙂

@tylerphelan
Copy link
Contributor

this is amazing!

@tylerphelan
Copy link
Contributor

i recommend changing the readable link to use the branch link instead of the blob/commit link

@stormqueen1990
Copy link
Contributor Author

i recommend changing the readable link to use the branch link instead of the blob/commit link

Sorry about that! Just updated the link to point to the branch.


### Enable image signing using `cosign`

- Create a new configuration in the `Image` resource that allow users to
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add an option to set the cosign registry or state that it will be supported?

It is currently controlled via COSIGN_REPOSITORY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me. Does a single repository URL configuration address this concern, or should we add it as a part of the secret structure?

Copy link

Choose a reason for hiding this comment

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

@samj1912 how do you envision using COSIGN_REPOSITORY?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ncdc currently my usecase is more around the flexibility of having application tags being in one repository and the cosign tag in another so that it doesn't pollute the original repository with cosign tags and also so that I can control/manage the lifecycle of the repository with cosign signatures differently than the application one. This also allows me to store signatures of images in an "operator" repository whereas the application images could be saved in a "user" repository.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me. Does a single repository URL configuration address this concern, or should we add it as a part of the secret structure?

Technically cosign allows each signature to be stored in a separate repository. It might be most flexible if we keep it along with the secret structure. But upto you and the maintainers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matthewmcnew @tylerphelan would it make sense to combine this URL property with the annotations in a ConfigMap?

Copy link
Contributor Author

@stormqueen1990 stormqueen1990 May 21, 2021

Choose a reason for hiding this comment

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

@ncdc do you think it would make more sense to move forward using ConfigMaps for the static annotations, taking into account @rszumlakowski's scenario, so we can combine static annotations and COSIGN_REPOSITORY URL in the same place?

Copy link

Choose a reason for hiding this comment

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

I think there is a valid use case for supporting spec.cosign.signatureRepository (COSIGN_REPOSITORY).

I also think that because a cosign signature supports annotations, it could make sense to have spec.cosign.annotations. (Unless there are good reasons not to support these per-Image?)

For static annotations that apply to multiple Images, we can either try to design for that now, or we could defer while we continue to gather more use cases. My preference would be to defer.

- name: <secret-name 1>
...
- name: <secret-name n>
annotations: # optional map of key-value pairs the user wants to add to images
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make more sense for annotations to be configured outside of the image spec such as in a ConfigMap?

  1. Allows shared annotations per namespace
  2. It might be easier to update a configmap than this field for non-static annotations (ex. git commit)
  • kpack cli doesn't currently support configuring the cosign field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it would be more flexible. In this case, we could have a list of ConfigMaps under the annotations key.

About kpack CLI not currently supporting the cosign field, can we create a separate RFC to address this concern?

Copy link
Collaborator

Choose a reason for hiding this comment

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

How commonly do we think annotations will be used, especially non-static ones?

Copy link

Choose a reason for hiding this comment

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

What if we support multiple levels of annotations?

  1. Per image - highest priority & overrides any key from a lower priority
  2. Per namespace - lower priority

We could implement 1 first, and add 2 later, if it's needed?

How would you imagine the workflow for annotating the git commit?

Copy link

Choose a reason for hiding this comment

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

How commonly do we think annotations will be used, especially non-static ones?

I'd probably want to start with some automatically-created dynamic ones, e.g. the git commit/url for the source for the build, information about the build (number, trigger(s), etc.).

I'm not sure about what you might put for static ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about what you might put for static ones?

Our thought was that users could want to put annotations with some static information such as a container image name, but it's possible that this is already addressed by other metadata pieces (such as build number).

Copy link
Contributor

Choose a reason for hiding this comment

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

More arguments why static annotations are useful:

I pictured the signature annotations to be used help an attestation system or as a part of a end users supply chain processes.

  1. Someone could put a signature on a container image at build time with an annotation saying, "Built on my build system".
  2. Later on they could run integration tests on the container image saying, "Passed integration tests". Let's say both those processes occur at VMware and they would have the same signing key. If I remember correctly, you can only sign an image with Cosign more than once if the two signatures have different annotations.
  3. Now, let's say the end user has a policy that they have to bless every image that comes into their air-gapped environment. After image relocation (which would preserve the original keys and their annotations) and some pass through a test environment they would put another signature, with another annotation that says "this image is allowed to be promoted to production".

Now, maybe only the first stage in this example would actually be signed with kpack, but I would like enable the rest of this chain of signatures by using kpack to allow its users to apply signatures with static annotations.

Copy link

Choose a reason for hiding this comment

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

Someone could put a signature on a container image at build time with an annotation saying, "Built on my build system".

Would it be safer to rely on secure identity built into the systems themselves (@anvega can hopefully provide more detail here) rather than a user-supplied value for a build server's identity?

Also cc @joshuagl in case you've got thoughts 😄

As I wrote in another comment thread, given that signature annotations are a thing, it seems reasonable to support them at the Image level, unless folks have significant reasons not to.

Copy link

Choose a reason for hiding this comment

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

cosign author here! I put the "annotations" into the tool to basically act as "lightweight" attestations. There are some other richer formats we're looking to support down the road (mostly the in-toto Attestation format: https://github.com/in-toto/attestation) but nothing is concrete yet.

I hope that helps!

Copy link

Choose a reason for hiding this comment

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

Aside from the constraint pointed out of annotations meant to support lightweight attestations, secure identities are inherently short-lived. As such, there'd be limitations around how to validate those without additional moving parts or a broader framework.

That said, consideration for secure identities is directionally correct. With support for in-toto down the line, it does open up the possibilities to integrate in-toto attestations with secure identities.

@stormqueen1990
Copy link
Contributor Author

Updated the RFC file to incorporate feedback received.

@sambhav
Copy link
Contributor

sambhav commented Jun 14, 2021

Also relevant to the secret storage format sigstore/cosign#345

I wonder if we should add a PR upstream to add the repository and docker media types fields if we want to consolidate the secret storage format and make it super easy for kpack users to use cosign?

@dlorenc
Copy link

dlorenc commented Jun 14, 2021

I wonder if we should add a PR upstream to add the repository and docker media types fields if we want to consolidate the secret storage format and make it super easy for kpack users to use cosign?

We're not really opinionated on the secret storage format itself, we just wanted a secure way to get a usable secret into k8s somehow :) If there's anything we could change about the naming to make it easier for kpack please let us know! We were running into this type of thing over and over again: https://smallstep.com/blog/command-line-secrets/

I'm not super sold on plumbing the rest of the config env vars up into k8s through this path yet, what would it look like for a kpack/cosign user?

Mauren Berti and others added 10 commits July 26, 2021 10:59
Co-authored-by: Robert Szumlakowski <rszumlakowsk@vmware.com>
Co-authored-by: Mauren Berti <mribeirobert@vmware.com>
Co-authored-by: Robert Szumlakowski <rszumlakowsk@vmware.com>
Co-authored-by: Mauren Berti <mribeirobert@vmware.com>
Remove the parts that specify the build metadata,
since this information is available in the image.
Signed-off-by: Mauren Berti <mribeirobert@vmware.com>
Signed-off-by: Mauren Berti <mribeirobert@vmware.com>
Signed-off-by: Mauren Berti <mribeirobert@vmware.com>
Signed-off-by: Mauren Berti <mribeirobert@vmware.com>
Co-authored-by: Matthew McNew <mmcnew@pivotal.io>
@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2021

Codecov Report

Merging #694 (2f836b4) into main (4e56473) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #694   +/-   ##
=======================================
  Coverage   69.41%   69.41%           
=======================================
  Files         117      117           
  Lines        5074     5074           
=======================================
  Hits         3522     3522           
  Misses       1185     1185           
  Partials      367      367           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e56473...2f836b4. Read the comment docs.

Signed-off-by: Mauren Berti <mribeirobert@vmware.com>
@matthewmcnew matthewmcnew merged commit 1b02a19 into buildpacks-community:main Aug 9, 2021
@matthewmcnew
Copy link
Collaborator

Thanks! @stormqueen1990

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

Successfully merging this pull request may close these issues.