-
Notifications
You must be signed in to change notification settings - Fork 885
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
Sign images #3434
Sign images #3434
Conversation
c368350
to
930055e
Compare
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #3434 +/- ##
==========================================
- Coverage 56.64% 56.62% -0.02%
==========================================
Files 221 221
Lines 20832 20832
==========================================
- Hits 11800 11796 -4
- Misses 8410 8414 +4
Partials 622 622
Flags with carried forward coverage won't be shown. Click here to find out more. |
26a41f4
to
5f17c26
Compare
5f17c26
to
014c828
Compare
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.
/assign
I'm looking at it.
@@ -36,6 +38,8 @@ jobs: | |||
uses: actions/setup-go@v3 | |||
with: | |||
go-version: 1.19.5 | |||
- name: Install Cosign | |||
uses: sigstore/cosign-installer@main |
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.
@main
means the master branch or the latest released version?
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'd prefer to use a pinned version of cosign
.
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.
You absolutely right,Change main
to v3.0.3
(The latest version today)
Thanks for your review.
@@ -45,4 +49,5 @@ jobs: | |||
env: | |||
REGISTRY: karmada | |||
VERSION: ${{ github.ref_name }} | |||
COSIGN_EXPERIMENTAL: 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.
What does this env mean? Is there any reference about it?
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.
It's mean use keyless signatures work in Cosign .
Doc from cosign:
https://docs.sigstore.dev/cosign/keyless/#keyless-verifying
@@ -6,6 +6,8 @@ on: | |||
jobs: | |||
publish-image-to-dockerhub: | |||
name: publish to DockerHub | |||
permissions: | |||
id-token: write # To be able to get OIDC ID token to sign images. |
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.
Can you help to explain in more detail about the id-token?
I can get some info from cosign-installer usage:
This action does not need any GitHub permission to run, however, if your workflow needs to update, create or perform any action against your repository, then you should change the scope of the permission appropriately.
For example, if you are using the gcr.io as your registry to push the images you will need to give the write permission to the packages scope.
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.
Need the permissions of id-token to auto get the OIDC Identity Tokens from github action if we use keyless signatures.It would be failed if github action have not this permissions.
It's not necessary if we sign with cert or use other OIDC provider.
Here is the doc: https://docs.sigstore.dev/cosign/keyless/#identity-tokens
...
+ cosign sign --yes -a run_id=4913600087 ***/karmada-operator:cosign
Generating ephemeral keys...
Retrieving signed certificate...
Non-interactive mode detected, using device flow.
Enter the verification code MSBL-QTPQ in your browser at: https://oauth2.sigstore.dev/auth/device?user_code=MSBL-QTPQ
Code will be valid for 300 seconds
Error: signing [***/karmada-operator:cosign]: getting signer: getting key from Fulcio: retrieving cert: error obtaining token: expired_token
main.go:74: error during command execution: signing [***/karmada-operator:cosign]: getting signer: getting key from Fulcio: retrieving cert: error obtaining token: expired_token
cb1b25a
to
8763ddb
Compare
@@ -32,6 +34,8 @@ jobs: | |||
uses: actions/setup-go@v3 | |||
with: | |||
go-version: 1.19.5 | |||
- name: Install Cosign | |||
uses: sigstore/cosign-installer@v3.0.3 |
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.
sorry, I found the newest cosign is 2.0.2. Can you give me the url?
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.
This is the cosign-installer version and not cosign version.
https://github.com/sigstore/cosign-installer
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.
Thanks, good.
And I think this feature timely make up for karmada shortcomings in image verification.
Can you explain how to use in customer env in doc?
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.
Can you explain how to use in customer env in doc?
Check links from issue #3435 .
It's the same thing.
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.
@zishen I'm change cosign version to v1.13.1
for consistent behavior with https://kubernetes.io/docs/tasks/administer-cluster/verify-signed-artifacts/ . Thanks for your check.
8763ddb
to
1365af6
Compare
@@ -66,6 +66,7 @@ function build_local_image() { | |||
|
|||
if [[ "$output_type" == "registry" ]]; then | |||
docker push "${image_name}" | |||
signImage ${image_name} |
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.
This script maybe ran in some guy's private CI enviromment without cosign
command.
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.
You're right, we can skip sign image when have not command of cosign
.
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.
Thanks @ikaven1024 for the reminder.
If guys want to build the image and push it to their private registry, we shouldn't force them to sign the images with cosign.
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.
kindly ping @liangyuanpeng
You're right, we can skip sign image when have not command of cosign.
Can we go with the approach that make images
does not sign the image by default? We can have an environment variable to indicate it to sign the image.
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.
@RainbowMango Updated, PTAL,Thanks.
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.
Thanks. Sorry for letting this sit so long. I'll look at it as soon as possible.
088cce7
to
00b5cad
Compare
Signed-off-by: Lan Liang <gcslyp@gmail.com> u
00b5cad
to
421d83c
Compare
Thanks. I'll test it on my side. |
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
/approve
Thanks @liangyuanpeng !
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @liangyuanpeng The CI is failing on master after this PR, could you please take a look? |
Just echo the error message here:
|
What type of PR is this?
/kind feature
What this PR does / why we need it:
Let images of kamada signed with cosign.
It would be help with the implementation of supply chain security practices on the user side.
Which issue(s) this PR fixes:
Part1 of #3435
Special notes for your reviewer:
After sign you can see the signatures from command of
cosign verify xxx
Just like that:
Also can be check the CI message from github action:
https://github.com/liangyuanpeng/karmada/actions/runs/4749383701/jobs/8436594750#step:8:140
Does this PR introduce a user-facing change?: