feat: adds cosign support #341
Conversation
Signed-off-by: ChrisJBurns <[email protected]>
ChrisJBurns
force-pushed
the
adds-cosign-support
branch
from
f48bf60 to
28317a1
Compare
|
This would be really useful. |
|
looks good, can confirm would be a very useful feature for us to have. |
Signed-off-by: ChrisJBurns <[email protected]>
There was a problem hiding this comment.
Thanks for the PR Chris! Sorry for the long delay in taking a look at it.
I'm noticing there are no tests for this feature. We'd need some tests before merging this in: https://github.com/concourse/registry-image-resource/blob/master/out_test.go
|
Thanks @taylorsilva , I'll get those added 👍! |
Signed-off-by: ChrisJBurns <[email protected]>
Signed-off-by: ChrisJBurns <[email protected]>
|
I've added a test but I'm not entirely sure if there's a way to test if the time has been signed as there is no response, only an error. So I've just asserted that there is no error that comes back for that test. Wondering what other thoughts you had? |
Signed-off-by: ChrisJBurns <[email protected]>
There was a problem hiding this comment.
Overall this is looking good. I made a suggestion for the test, let me know what you think.
I left a bunch of code-style related comments. My intention behind those comments is to keep the coding style within the code-base consistent.
I noticed the README hasn't been updated with the new cosign field. Could you please update the table with a new row describing which fields in cosign are required/optional. Not sure if the cosign field should live in the main Source or in PutParams instead. I left a comment below about that as well. Would someone want to use cosign in the get step? If they would, then it makes sense to leave Cosign in the Source struct.
Thanks again for the PR and making the changes you've made so far! Next review will go faster now that I've actually read all the code :)
Signed-off-by: ChrisJBurns <[email protected]>
Signed-off-by: ChrisJBurns <[email protected]>
|
The Slightly more detailed error: |
actualErr is just the `error` type returned by the out command in Go. We want what was output to stderr, which is what actualErrOutput is Signed-off-by: Taylor Silva <[email protected]>
Signed-off-by: Taylor Silva <[email protected]>
|
Made a small fix to the test. Looks like the image isn't being signed though as we're not seeing any of the info log lines output: |
Currently, Content Trust is a feature offered by the
registry-image-resource, but the gold standard for image signing now is using (if it isn't already) Cosign. This PR implements the functionality to sign images with Cosign. Currently, although it is possible usingSignCmd, Cosign is a CLI first - meaning using it as a library in code can be a little less than ideal. This results in large number of config objects having to be passed into theSignCmddue to the fact that there is no CLI framework setting the default values.When Cosign becomes more and more usable as a library, the code in this PR can be reduced. This includes, the way we have to set a temporary environment variable for
COSIGN_KEYandCOSIGN_PASSWORDuntil these are values that can be more easily passed into the Cosign code. Another one is the Keychain. Currently, Cosign works that if you have a docker config JSON file with registries and auth configured for them in a local cred store, Cosign will just use them via thego-containerregistrylibary. Due to security reasons, we don't want to have to put the credentials in a file in theregistry-image-resourcetask as any developer that intercepts the container can easily view those credentials. Instead we use an InMemoryKeychain that the underlying Cosign/go-containerregsitry libraries will pick up and use for the pushing of signatures to the registry.implements: #329