-
Notifications
You must be signed in to change notification settings - Fork 507
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
Conversation
This is my favorite PR. |
/lgtm |
Do we need to pin the public key url, right now tip-of-tree/main/master |
Weeeeeeeeeeee!! 🎉 cc: @kubernetes/release-engineering |
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 |
There was a problem hiding this comment.
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. 😃
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
/priority important-soon |
args: | ||
- 'verify' | ||
- '-key' | ||
- 'https://raw.githubusercontent.com/GoogleContainerTools/distroless/main/cosign.pub' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done :)
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:
|
Can we throw something like:
In there to resolve the digest? cc @imjasonh another use case for
This seems very similar to what we'd want to do, if not exactly what we'd want to do. |
/test pull-release-image-go-runner |
/test pull-release-image-go-runner |
There was a problem hiding this 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----- |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done :)
It's wonderful to see this is place!! Thanks @dekkagaijin! /lgtm |
One more time for paranoia's sake: |
This is awesome! |
New changes are detected. LGTM label has been removed. |
[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 |
I did a git oops when pushing updates to @dekkagaijin's branch, which accidentally closed this. |
Et tu, Stephen? |
Hehe, sometimes you get git and sometimes git gets you! |
#2016 has merged. |
This'll ensure that the
distroless
base images were built on trusted infrastructure/kind feature