Cosign Signing Integration #817
Conversation
Codecov Report
@@ Coverage Diff @@
## main #817 +/- ##
==========================================
+ Coverage 68.20% 68.46% +0.26%
==========================================
Files 112 114 +2
Lines 4878 4938 +60
==========================================
+ Hits 3327 3381 +54
- Misses 1186 1189 +3
- Partials 365 368 +3
Continue to review full report at Codecov.
|
| @@ -141,6 +142,56 @@ To create the notary secret used by kpack for image signing, run the following c | |||
| - `<password>`: The password provided to encrypt the private key. | |||
| - `<hash>.key`: The private key file. | |||
|
|
|||
| ### <a id='cosign-config'></a>Cosign Configuration | |||
There was a problem hiding this comment.
Would it be alright to have Signing/Verifying builder/clusterBuilder images as a separate PR such that the next version of kpack could have the initial image signing first? Then, we will work on implementing the Signing/Verifying builder/clusterBuilder images portion of the RFC as the following kpack version since upcoming feature cutoff is near for the next release candidate for kpack.
| logger.Fatal(err) | ||
| } | ||
|
|
||
| for _, c := range append(dockerCfgCredentials, dockerConfigCredentials...) { |
There was a problem hiding this comment.
I realize this was the existing behavior in completion but, it seems unnecessary to rebuild the keychain to just write to docker/config.json again. Can we just mount the same credentials from export into completion?
There was a problem hiding this comment.
Did you mean that I remount the homeVolume and set the Env HOME to /builder/home?
I noticed that build-init would save the config to /builder/home/docker/config.json and could therefore be reused by completion during the regular buildPod flow. However, during the rebasePod flow, as the only initContainer is the "rebase" container. It does load some docker credentials but it does not save it. I could introduce the "Save" portion to the rebase image and therefore the completion container could leverage a shared volume for both buildPod and rebasePod scenarios. Is it ideal to introduce this logic into "rebase" when it itself doesn't need the file to be saved just so we can leverage the byproduct of that outside of that container in completion?
The keychain still needed to be built or loaded into a variable to be passed into Notary while cosign only requires the config to be writing to a file named config.json. The keychain could be built from the config.json though instead of from each secret individually if mounting a shared volume is the way to go
There was a problem hiding this comment.
@matthewmcnew If I'm understanding this wrong, please lemme know what the best approach is :D
There was a problem hiding this comment.
I forgot about the complexities introduced by the rebase container. I think it would eventually be a good idea for the rebase container to "save" the credentials to a volume that can be loaded in completion. I don't think you need to introduce this in your PR.
There was a problem hiding this comment.
Since that is the case, I will leave the current credential generation and saving inside the completion image. When we introduce and find a more uniform way of saving credentials, we can adjust the completion container then. For cosign portion in the completion container, with the shared volume during non rebase case, it seemed to work from my quick testing when mounting the shared home volume. For the notary portion, it is still likely that the keychain would need to be loaded to be passed into the notary sign function. Is there a place I can put stories or track this concern? Do I just make a Github issue saying to standardize docker credential saving using shared volumes to be more consistent between rebasePod/buildPod/initContainers/containers?
DennyHoang
changed the title
|
@tomkennedy513 @tylerphelan @matthewmcnew I believe I have currently addressed all the outstanding review comments :) Please take a look when you have time, thanks. |
|
|
||
| cliSignCmd := func(ctx context.Context, ko cli.KeyOpts, annotations map[string]interface{}, imageRef, certPath string, upload bool, payloadPath string, force, recursive bool) error { | ||
| t.Helper() | ||
| assert.Equal(t, testCtx, ctx) |
There was a problem hiding this comment.
I really like that these tests actually validate the signature. I think the additional validation of the values passed to the cliSignCmd here might be unnecessary (and may make it hard to use new api versions).
I think it is fine for now but, perhaps something to think about in the future.
|
Please dont merge yet
Figured out thanks to Tom! |
|
Hmm, small change is required. Going to make a small adjustment to log there instead of returning an error as it should continue if there are no secrets found. |
|
Thanks @tomkennedy513 for your help with the squash! If people are okay with that adjustment I made in the above comment/commit, then this PR is good to go! |
Co-authored-by: Mauren Berti <mribeirobert@vmware.com> Co-authored-by: Denny Hoang <dhoang@vmware.com> Co-authored-by: Andrés Torres <andrest@vmware.com> Co-authored-by: Kavitha Krishnan <krishnanka@vmware.com>
This PR integrates cosign for image signing as specified in RFC #694
When an operator attaches a cosign formatted secret (contains at least cosign.key data key) to the service account used for building the image, the image will then attempt to be signed after it has been built and pushed to a repository.
Status: Ready for Review
Currently, we are opening this draft PR to get an early review of our current implementation. There are still features that have not been implemented as listed in the
Featureschecklist below. Also, open for discussion, would it be alright to haveSigning/Verifying builder/clusterBuilder imagesas a separate PR such that the next version of kpack could have the initial image signing first? Then, we will work on implementing theSigning/Verifying builder/clusterBuilder imagesportion afterwards as the upcoming feature cutoff is nearing for the next release candidate for kpack.Features:
Signing/Verifying builder/clusterBuilder imagesThis will be addressed in a separate PR. Currently requires ability to provide a different keychain. Issue raised to cosign here and here for signing builder images as mentioned hereWe had some questions which are added as "Review" items to help organize discussion into threads.