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

Verify cosign signatures of distroless base images #2011

Closed
wants to merge 0 commits into from

Conversation

dekkagaijin
Copy link

This'll ensure that the distroless base images were built on trusted infrastructure

$ docker run -it gcr.io/projectsigstore/cosign/ci/cosign@sha256:c581a4f0f6dd158220fa05d2351d8015f969b68de5c61d52c41478fae84e7064 -- verify -key https://raw.githubusercontent.com/GoogleContainerTools/distroless/main/cosign.pub gcr.io/distroless/base:latest
Unable to find image 'gcr.io/projectsigstore/cosign/ci/cosign@sha256:c581a4f0f6dd158220fa05d2351d8015f969b68de5c61d52c41478fae84e7064' locally
gcr.io/projectsigstore/cosign/ci/cosign@sha256:c581a4f0f6dd158220fa05d2351d8015f969b68de5c61d52c41478fae84e7064: Pulling from projectsigstore/cosign/ci/cosign
5dea5ec2316d: Already exists
bb771d6dc9a1: Already exists
9127c3610b7e: Already exists
72164b581b02: Already exists
6fe218878cac: Pull complete
Digest: sha256:c581a4f0f6dd158220fa05d2351d8015f969b68de5c61d52c41478fae84e7064
Status: Downloaded newer image for gcr.io/projectsigstore/cosign/ci/cosign@sha256:c581a4f0f6dd158220fa05d2351d8015f969b68de5c61d52c41478fae84e7064

Verification for gcr.io/distroless/base:latest --
The following checks were performed on each of these signatures:
  - The cosign claims were validated
  - The signatures were verified against the specified public key
  - Any certificates were verified against the Fulcio roots.

[{"critical":{"identity":{"docker-reference":""},"image":{"docker-manifest-digest":"sha256:6ec6da1888b18dd971802c2a58a76a7702902b4c9c1be28f38e75e871cedc2df"},"type":"cosign container signature"},"optional":null},{"critical":{"identity":{"docker-reference":""},"image":{"docker-manifest-digest":"sha256:6ec6da1888b18dd971802c2a58a76a7702902b4c9c1be28f38e75e871cedc2df"},"type":"cosign container signature"},"optional":null},{"critical":{"identity":{"docker-reference":""},"image":{"docker-manifest-digest":"sha256:6ec6da1888b18dd971802c2a58a76a7702902b4c9c1be28f38e75e871cedc2df"},"type":"cosign container signature"},"optional":null},{"critical":{"identity":{"docker-reference":""},"image":{"docker-manifest-digest":"sha256:6ec6da1888b18dd971802c2a58a76a7702902b4c9c1be28f38e75e871cedc2df"},"type":"cosign container signature"},"optional":null},{"critical":{"identity":{"docker-reference":""},"image":{"docker-manifest-digest":"sha256:6ec6da1888b18dd971802c2a58a76a7702902b4c9c1be28f38e75e871cedc2df"},"type":"cosign container signature"},"optional":null}]

/kind feature

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. needs-priority cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 20, 2021
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 20, 2021
@dekkagaijin
Copy link
Author

/cc @justaugustus @dlorenc

@dlorenc
Copy link

dlorenc commented Apr 20, 2021

This is my favorite PR.

@dlorenc
Copy link

dlorenc commented Apr 20, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 20, 2021
@inferno-chromium
Copy link

Do we need to pin the public key url, right now tip-of-tree/main/master

@justaugustus
Copy link
Member

Weeeeeeeeeeee!! 🎉
...but also:
/hold for discussion

cc: @kubernetes/release-engineering

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 21, 2021
@justaugustus justaugustus added this to the v1.22 milestone Apr 21, 2021
@dekkagaijin
Copy link
Author

Do we need to pin the public key url, right now tip-of-tree/main/master

That'd be one option, otherwise vendoring the public key in this repo would be a good option

@@ -9,6 +9,12 @@ options:
machineType: 'N1_HIGHCPU_8'

steps:
- name: 'gcr.io/projectsigstore/cosign/ci/cosign@sha256:c581a4f0f6dd158220fa05d2351d8015f969b68de5c61d52c41478fae84e7064' # v0.3.1
Copy link
Member

Choose a reason for hiding this comment

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

I said to Dan some time ago that we might want to start to publish the tagged image releases in another bucket that does not contain the ci on it to avoid confusion for the users.
But I know we all are working to make the cosign image a bit better before doing that. 😃

Copy link

Choose a reason for hiding this comment

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

Ah, it's published in both places actually! Good catch. The releases are at gcr.io/projectsigstore/cosign:v0.3.1 as well.

Copy link
Author

Choose a reason for hiding this comment

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

done

@cpanato
Copy link
Member

cpanato commented Apr 21, 2021

/priority important-soon

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority labels Apr 21, 2021
args:
- 'verify'
- '-key'
- 'https://raw.githubusercontent.com/GoogleContainerTools/distroless/main/cosign.pub'
Copy link
Member

Choose a reason for hiding this comment

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

Do we wanna pin this to an actual version/sha? If the main branch changes then the build will silently fail.

Copy link

Choose a reason for hiding this comment

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

@priyawadhwa set this up intentionally - but she's out this week so I'll try to document the rationale in that repo.

The key rotation strategy for distroless is that we'll commit a new public key into the repo, then the images are built and signed with the corresponding private key. Since K8s takes distroless only from HEAD, the only appropriate key to check against is latest.

The branch name one is a good point though - is there a good way to access whatever the default branch is?

Copy link
Author

Choose a reason for hiding this comment

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

done :)

@cppforlife
Copy link

if i'm reading this right, verification runs against image with :latest tag and the later another process resolves :latest again. even though this is unlikely, there is a time gap where :latest could change to something different/malicious. are we not able to resolve image ref to a digest, verify it and use it in the following steps? since we are pretty early in the signing journey, it would be good to set a precedent for using exact assets that were verified as part of a build.

@dlorenc
Copy link

dlorenc commented Apr 21, 2021

if i'm reading this right, verification runs against image with :latest tag and the later another process resolves :latest again. even though this is unlikely, there is a time gap where :latest could change to something different/malicious. are we not able to resolve image ref to a digest, verify it and use it in the following steps? since we are pretty early in the signing journey, it would be good to set a precedent for using exact assets that were verified as part of a build.

Yeah, that would be better IMO. See here for how Istio handles this (for distroless): https://github.com/istio/istio/blob/master/bin/update_deps.sh#L50

I can think of a few approaches:

  • Do a docker pull before the build, and then get the pulled digest out of the daemon. Use that to check a signature, and leave ":latest" in the Dockerfile, relying on the daemon to do the right thing during build (and not pull again)
  • Resolve, check the signature, and then template the tag into the Dockerfile itself. I made a POC here that does all of this except for the rewriting of the Dockerfile: WIP: Hypotenuse - a tool for extracting the base images out of a Dockerfile. sigstore/cosign#189
  • Probably more!

@jonjohnsonjr
Copy link

Can we throw something like:

crane digest gcr.io/distroless/$_DISTROLESS_IMAGE:latest

In there to resolve the digest?

cc @imjasonh another use case for --full-ref flag or something.

I made a POC here that does all of this except for the rewriting of the Dockerfile: sigstore/cosign#189

This seems very similar to what we'd want to do, if not exactly what we'd want to do.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 21, 2021
@dekkagaijin
Copy link
Author

/test pull-release-image-go-runner

@dekkagaijin
Copy link
Author

/test pull-release-image-go-runner

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 22, 2021
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

/lgtm

-----BEGIN PUBLIC KEY-----
MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEWZzVzkb8A+DbgDpaJId/bOmV8n7Q
OqxYbK0Iro6GzSmOzxkn+N2AKawLyXi84WSwJQBK//psATakCgAQKkNTAA==
-----END PUBLIC KEY-----
Copy link
Member

Choose a reason for hiding this comment

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

nit non blocking: maybe a new line

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -9,6 +9,13 @@ options:
machineType: 'N1_HIGHCPU_8'

steps:
- name: 'gcr.io/projectsigstore/cosign@sha256:c581a4f0f6dd158220fa05d2351d8015f969b68de5c61d52c41478fae84e7064' # v0.3.1
Copy link
Member

Choose a reason for hiding this comment

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

maybe: gcr.io/projectsigstore/cosign:v0.3.1@sha256:c581a4f0f6dd158220fa05d2351d8015f969b68de5c61d52c41478fae84e7064

Copy link
Author

Choose a reason for hiding this comment

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

done :)

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 22, 2021
@puerco
Copy link
Member

puerco commented Apr 22, 2021

It's wonderful to see this is place!! Thanks @dekkagaijin!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 22, 2021
@justaugustus
Copy link
Member

One more time for paranoia's sake:
/test all

@dlorenc
Copy link

dlorenc commented Apr 25, 2021

This is awesome!

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2021
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cpanato, dekkagaijin, saschagrunert

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 25, 2021
@justaugustus
Copy link
Member

I did a git oops when pushing updates to @dekkagaijin's branch, which accidentally closed this.
Re-pushed with attribution here: #2016

@dekkagaijin
Copy link
Author

Et tu, Stephen?

@justaugustus
Copy link
Member

Et tu, Stephen?

Hehe, sometimes you get git and sometimes git gets you!

@justaugustus
Copy link
Member

#2016 has merged.
Thank you again, @dekkagaijin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants