-
Notifications
You must be signed in to change notification settings - Fork 147
Implement missing features to OCI backend #284
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
Conversation
Hi @rgreinho. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
1e11061
to
ee5e138
Compare
I did attempt to add some unit tests here, but I am not exactly sure how to do it, and it looks like there would be a ton of things to mock. Would you have any pointers by chance? @priyawadhwa @dlorenc Otherwise, e2e tests should be fine as the use case would be covered there. |
/ok-to-test |
The following is the coverage report on the affected files.
|
Oh I need to remove the skeleton I wrote for the unit tests. Regarding the e2e tests, it looks like some cluster issues. |
The following is the coverage report on the affected files.
|
Yep, it definitely looks like there are Kubernetes cluster issues. 🙁 |
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.
Hey @rgreinho thanks for fixing this! I have a few thoughts:
- When given a TaskRun, it can have multiple images , each of which can have multiple signatures. We should change these function definitions to reflect that:
// RetrievePayload maps [image name]:[payload] for a TaskRun
RetrievePayload(opts config.StorageOpts) (map[string]string, error)
// RetrieveSignature maps [image name]:[list of signatures] for a TaskRun
RetrieveSignature(opts config.StorageOpts) (map[string][]string, error)
this should make it easy for clients to get all the info they need for verification!
I'm also wondering if something like this could be useful in the CLI for later:
$ tkn chains list artifacts
# image1
# image2
$ tkn chains payload --image [image name]
but I think we can consider that later! Just throwing it in here so I don't forget 😄
pkg/chains/storage/oci/oci.go
Outdated
func (b *Backend) RetrieveSignature(opts config.StorageOpts) (string, error) { | ||
return "", fmt.Errorf("not implemented") | ||
envelopes, err := b.extractEnvelopes(opts) |
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'm wondering if we can take more advantage of the cosign/pkg/oci
package in the cosign repository here. Do you think something like this would work?
func (b *Backend) RetrieveSignature(opts config.StorageOpts) (string, error) {
// given the TaskRun, retrieve the signature
images := artifacts.ExtractOCIImagesFromResults(b.tr, b.logger)
// we can remove this check once the function def is changed
if len(images) != 1 {
return "", errors.New("multiple images found, unsure which to provide signature for")
}
// get signature for a specific image
ref, ok := images[0].(name.Digest)
if !ok {
return "", errors.New("error parsing image")
}
img, err := ociremote.SignedImage(ref)
if err != nil {
return "", err
}
sigImage, err := img.Signatures()
if err != nil {
return "", err
}
sigs, err := sigImage.Get()
if err != nil {
return "", err
}
for _, s := range sigs {
// also update this once the function def is updated
if sig, err := s.Base64Signature(); err == nil {
return sig, nil
}
}
return "", fmt.Errorf("failed to find a signature")
}
and this should require a small change for getting the payload instead:
for _, s := range sigs {
if payload, err := s.Payload(); err == nil {
return string(payload), nil
}
}
(as mentioned in the other comment, I do think we should change the function definition here, but this is just meant to be an example!)
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.
Oh sure! I'll give it a try 👍
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 Where did you get the ociremote
package from? From version 1.2.1, it looks like it cannot be imported (use of internal package github.com/sigstore/cosign/internal/oci/remote not allowed
), and if I upgrade the dependency to 1.3.1, it breaks a bunch of stuff.
Hey @rgreinho think this just needs a rebase, but the cosign library upgrade should be in so you should be unblocked now 😄 |
Awesome! Thanks a lot @priyawadhwa 👍 |
@priyawadhwa Quick update: that almost works! I just lost one part of the behavior I wanted, but I'm sure we can fix it with the new packages you created. I'm going to keep digging around, but I wanted to keep you updated. If I attempt to retrieve the signature I get what I want: ╭ cli on chains-cli via 🐹 v1.17.3
╰❯ ./tkn chains signature $(tkn tr describe --last -o json | jq -r '.metadata.name')
E1202 15:33:11.781098 6531 aws_credentials.go:77] while getting AWS credentials NoCredentialProviders: no valid providers in chain. Deprecated.
For verbose messaging see aws.Config.CredentialsChainVerboseErrors
MEUCIQDwYrr0XAp/jp5vDv6TEBZvj8lwPE6f1fI/pY8609GVMwIgMQ6PQZYio/VKypjKEqfumo7it4I6it5BzDRG86uEtgw= But if I attempt to retrieve the payload, I actually don't get the payload: cli on chains-cli via 🐹 v1.17.3
╰❯ ./tkn chains payload $(tkn tr describe --last -o json | jq -r '.metadata.name')
Clients created successfully.
Taskrun created successfully.
E1202 15:33:26.186332 6549 aws_credentials.go:77] while getting AWS credentials NoCredentialProviders: no valid providers in chain. Deprecated.
For verbose messaging see aws.Config.CredentialsChainVerboseErrors
Backend initialized successfully.
{"Critical":{"Identity":{"docker-reference":"ttl.sh/b5e53772774f48bf80dd6b528e5715a8/slsapoc"},"Image":{"Docker-manifest-digest":"sha256:f82fe2b635e304c7d8445c0117a4dbe35dd3c840078a39e21c88073a885c5e0f"},"Type":"Tekton container signature"},"Optional":{}} Which is similar to the output of ╭ cli on chains-cli via 🐹 v1.17.3
╰❯ cosign verify --key "${COSIGN_KEY}" ttl.sh/b5e53772774f48bf80dd6b528e5715a8/slsapoc
Verification for ttl.sh/b5e53772774f48bf80dd6b528e5715a8/slsapoc --
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":"ttl.sh/b5e53772774f48bf80dd6b528e5715a8/slsapoc"},"image":{"docker-manifest-digest":"sha256:f82fe2b635e304c7d8445c0117a4dbe35dd3c840078a39e21c88073a885c5e0f"},"type":"Tekton container signature"},"optional":{}}] However I wanted the output of ╭ cli on chains-cli via 🐹 v1.17.3 ⌛ 2s
╰❯ cosign verify-attestation --key "${COSIGN_KEY}" ttl.sh/b5e53772774f48bf80dd6b528e5715a8/slsapoc
Verification for ttl.sh/b5e53772774f48bf80dd6b528e5715a8/slsapoc --
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.
{"payloadType":"application/vnd.in-toto+json","payload":"eyJfdHlwZSI6Imh0dHBzOi8vaW4tdG90by5pby9TdGF0ZW1lbnQvdjAuMSIsInByZWRpY2F0ZVR5cGUiOiJodHRwczovL3Rla3Rvbi5kZXYvY2hhaW5zL3Byb3ZlbmFuY2UiLCJzdWJqZWN0IjpbeyJuYW1lIjoidHRsLnNoL2I1ZTUzNzcyNzc0ZjQ4YmY4MGRkNmI1MjhlNTcxNWE4L3Nsc2Fwb2MiLCJkaWdlc3QiOnsic2hhMjU2IjoiZjgyZmUyYjYzNWUzMDRjN2Q4NDQ1YzAxMTdhNGRiZTM1ZGQzYzg0MDA3OGEzOWUyMWM4ODA3M2E4ODVjNWUwZiJ9fV0sInByZWRpY2F0ZSI6eyJpbnZvY2F0aW9uIjp7InBhcmFtZXRlcnMiOlsiQlVJTERFUl9JTUFHRT17c3RyaW5nIGRvY2tlci5pby9jbmJzL3NhbXBsZS1idWlsZGVyOmJpb25pY0BzaGEyNTY6NmMwM2RkNjA0NTAzYjU5ODIwZmQxNWFkYmM2NWMwYTA3N2E0N2UzMWQ0MDRhM2RjYWQxOTBmMzE3OWU5MjBiNSBbXX0iLCJBUFBfSU1BR0U9e3N0cmluZyB0dGwuc2gvYjVlNTM3NzI3NzRmNDhiZjgwZGQ2YjUyOGU1NzE1YTgvc2xzYXBvYyBbXX0iLCJTT1VSQ0VfU1VCUEFUSD17c3RyaW5nIGFwcHMvcnVieS1idW5kbGVyIFtdfSIsIlBST0NFU1NfVFlQRT17c3RyaW5nIHdlYiBbXX0iLCJFTlZfVkFSUz17YXJyYXkgIFtdfSIsIlJVTl9JTUFHRT17c3RyaW5nICBbXX0iLCJDQUNIRV9JTUFHRT17c3RyaW5nIHR0bC5zaC9iNWU1Mzc3Mjc3NGY0OGJmODBkZDZiNTI4ZTU3MTVhOC9zbHNhcG9jLWNhY2hlIFtdfSIsIlVTRVJfSUQ9e3N0cmluZyAxMDAwIFtdfSIsIkdST1VQX0lEPXtzdHJpbmcgMTAwMCBbXX0iLCJTT1VSQ0VfU1VCUEFUSD1bXSIsIkVOVl9WQVJTPVtdIiwiUFJPQ0VTU19UWVBFPXdlYiIsIlJVTl9JTUFHRT1bXSIsIkNBQ0hFX0lNQUdFPVtdIiwiU0tJUF9SRVNUT1JFPWZhbHNlIiwiVVNFUl9JRD0xMDAwIiwiR1JPVVBfSUQ9MTAwMCIsIlBMQVRGT1JNX0RJUj1lbXB0eS1kaXIiXSwicmVjaXBlX3VyaSI6InRhc2s6Ly9idWlsZHBhY2tzIiwiZXZlbnRfaWQiOiI0OTYyMmEyYy04ZWY0LTQ2NTMtOTIyYi02ZTU4NDFiMjliN2YiLCJidWlsZGVyLmlkIjoidGVrdG9uLWNoYWlucyJ9LCJyZWNpcGUiOnsic3RlcHMiOlt7ImVudHJ5UG9pbnQiOiIjIS91c3IvYmluL2VudiBiYXNoXG5zZXQgLWVcblxuaWYgW1sgXCIkKHdvcmtzcGFjZXMuY2FjaGUuYm91bmQpXCIgPT0gXCJ0cnVlXCIgXV07IHRoZW5cbiAgZWNobyBcIlx1MDAzZSBTZXR0aW5nIHBlcm1pc3Npb25zIG9uICckKHdvcmtzcGFjZXMuY2FjaGUucGF0aCknLi4uXCJcbiAgY2hvd24gLVIgXCIkKHBhcmFtcy5VU0VSX0lEKTokKHBhcmFtcy5HUk9VUF9JRClcIiBcIiQod29ya3NwYWNlcy5jYWNoZS5wYXRoKVwiXG5maVxuXG5mb3IgcGF0aCBpbiBcIi90ZWt0b24vaG9tZVwiIFwiL2xheWVyc1wiIFwiJCh3b3Jrc3BhY2VzLnNvdXJjZS5wYXRoKVwiOyBkb1xuICBlY2hvIFwiXHUwMDNlIFNldHRpbmcgcGVybWlzc2lvbnMgb24gJyRwYXRoJy4uLlwiXG4gIGNob3duIC1SIFwiJChwYXJhbXMuVVNFUl9JRCk6JChwYXJhbXMuR1JPVVBfSUQpXCIgXCIkcGF0aFwiXG5kb25lXG5cbmVjaG8gXCJcdTAwM2UgUGFyc2luZyBhZGRpdGlvbmFsIGNvbmZpZ3VyYXRpb24uLi5cIlxucGFyc2luZ19mbGFnPVwiXCJcbmVudnM9KClcbmZvciBhcmcgaW4gXCIkQFwiOyBkb1xuICAgIGlmIFtbIFwiJGFyZ1wiID09IFwiLS1lbnYtdmFyc1wiIF1dOyB0aGVuXG4gICAgICAgIGVjaG8gXCItXHUwMDNlIFBhcnNpbmcgZW52IHZhcmlhYmxlcy4uLlwiXG4gICAgICAgIHBhcnNpbmdfZmxhZz1cImVudi12YXJzXCJcbiAgICBlbGlmIFtbIFwiJHBhcnNpbmdfZmxhZ1wiID09IFwiZW52LXZhcnNcIiBdXTsgdGhlblxuICAgICAgICBlbnZzKz0oXCIkYXJnXCIpXG4gICAgZmlcbmRvbmVcblxuZWNobyBcIlx1MDAzZSBQcm9jZXNzaW5nIGFueSBlbnZpcm9ubWVudCB2YXJpYWJsZXMuLi5cIlxuRU5WX0RJUj1cIi9wbGF0Zm9ybS9lbnZcIlxuXG5lY2hvIFwiLS1cdTAwM2UgQ3JlYXRpbmcgJ2VudicgZGlyZWN0b3J5OiAkRU5WX0RJUlwiXG5ta2RpciAtcCBcIiRFTlZfRElSXCJcblxuZm9yIGVudiBpbiBcIiR7ZW52c1tAXX1cIjsgZG9cbiAgICBJRlM9Jz0nIHJlYWQgLXIga2V5IHZhbHVlIFx1MDAzY1x1MDAzY1x1MDAzYyBcIiRlbnZcIlxuICAgIGlmIFtbIFwiJGtleVwiICE9IFwiXCIgXHUwMDI2XHUwMDI2IFwiJHZhbHVlXCIgIT0gXCJcIiBdXTsgdGhlblxuICAgICAgICBwYXRoPVwiJHtFTlZfRElSfS8ke2tleX1cIlxuICAgICAgICBlY2hvIFwiLS1cdTAwM2UgV3JpdGluZyAke3BhdGh9Li4uXCJcbiAgICAgICAgZWNobyAtbiBcIiR2YWx1ZVwiIFx1MDAzZSBcIiRwYXRoXCJcbiAgICBmaVxuZG9uZVxuIiwiYXJndW1lbnRzIjpbIi0tZW52LXZhcnMiLCIkKHBhcmFtcy5FTlZfVkFSU1sqXSkiXSwiZW52aXJvbm1lbnQiOnsiY29udGFpbmVyIjoicHJlcGFyZSIsImltYWdlIjoiZG9ja2VyLXB1bGxhYmxlOi8vYmFzaEBzaGEyNTY6YjIwODIxNWE0NjU1NTM4YmU2NTJiMjc2OWQ4MmU1NzZiYzRkMGEyYmIxMzIxNDRjMDYwZWZjNWJlOGMzZjVkNiJ9LCJhbm5vdGF0aW9ucyI6bnVsbH0seyJlbnRyeVBvaW50IjoiL2NuYi9saWZlY3ljbGUvY3JlYXRvciIsImFyZ3VtZW50cyI6WyItYXBwPSQod29ya3NwYWNlcy5zb3VyY2UucGF0aCkvJChwYXJhbXMuU09VUkNFX1NVQlBBVEgpIiwiLWNhY2hlLWRpcj0kKHdvcmtzcGFjZXMuY2FjaGUucGF0aCkiLCItY2FjaGUtaW1hZ2U9JChwYXJhbXMuQ0FDSEVfSU1BR0UpIiwiLXVpZD0kKHBhcmFtcy5VU0VSX0lEKSIsIi1naWQ9JChwYXJhbXMuR1JPVVBfSUQpIiwiLWxheWVycz0vbGF5ZXJzIiwiLXBsYXRmb3JtPS9wbGF0Zm9ybSIsIi1yZXBvcnQ9L2xheWVycy9yZXBvcnQudG9tbCIsIi1wcm9jZXNzLXR5cGU9JChwYXJhbXMuUFJPQ0VTU19UWVBFKSIsIi1za2lwLXJlc3RvcmU9JChwYXJhbXMuU0tJUF9SRVNUT1JFKSIsIi1wcmV2aW91cy1pbWFnZT0kKHBhcmFtcy5BUFBfSU1BR0UpIiwiLXJ1bi1pbWFnZT0kKHBhcmFtcy5SVU5fSU1BR0UpIiwiJChwYXJhbXMuQVBQX0lNQUdFKSJdLCJlbnZpcm9ubWVudCI6eyJjb250YWluZXIiOiJjcmVhdGUiLCJpbWFnZSI6ImRvY2tlci1wdWxsYWJsZTovL2NuYnMvc2FtcGxlLWJ1aWxkZXJAc2hhMjU2OjZjMDNkZDYwNDUwM2I1OTgyMGZkMTVhZGJjNjVjMGEwNzdhNDdlMzFkNDA0YTNkY2FkMTkwZjMxNzllOTIwYjUifSwiYW5ub3RhdGlvbnMiOm51bGx9LHsiZW50cnlQb2ludCI6IiMhL3Vzci9iaW4vZW52IGJhc2hcbnNldCAtZVxuZ3JlcCBcImRpZ2VzdFwiIC9sYXllcnMvcmVwb3J0LnRvbWwgfCBjdXQgLWQnXCInIC1mMiB8IGN1dCAtZCdcIicgLWYyIHwgdHIgLWQgJ1xcbicgfCB0ZWUgXCIkKHJlc3VsdHMuQVBQX0lNQUdFX0RJR0VTVC5wYXRoKVwiXG5cbiMgRGlzYWJsZSBzaGVsbGNoZWNrIGhlcmUgc2luY2UgJCgpIGlzIHVzZSBmb3IgdmFyaWFibGUgc3Vic3RpdHV0aW9uIGFuZCBub3QgY29tbWFuZCBleGVjdXRpb24uXG4jIHNoZWxsY2hlY2sgZGlzYWJsZT1TQzIwMDVcbmVjaG8gXCIkKHBhcmFtcy5BUFBfSU1BR0UpXCIgfCB0ZWUgXCIkKHJlc3VsdHMuQVBQX0lNQUdFX1VSTC5wYXRoKVwiXG4iLCJhcmd1bWVudHMiOm51bGwsImVudmlyb25tZW50Ijp7ImNvbnRhaW5lciI6InJlc3VsdHMiLCJpbWFnZSI6ImRvY2tlci1wdWxsYWJsZTovL2Jhc2hAc2hhMjU2OmIyMDgyMTVhNDY1NTUzOGJlNjUyYjI3NjlkODJlNTc2YmM0ZDBhMmJiMTMyMTQ0YzA2MGVmYzViZThjM2Y1ZDYifSwiYW5ub3RhdGlvbnMiOm51bGx9XX0sIm1ldGFkYXRhIjp7ImJ1aWxkU3RhcnRlZE9uIjoiMjAyMS0xMi0wMlQyMToyNzo1MloiLCJidWlsZEZpbmlzaGVkT24iOiIyMDIxLTEyLTAyVDIxOjI4OjU5WiJ9fX0=","signatures":[{"keyid":"SHA256:DojOZnqYRvBOs0OiTE4Zkivfx8nAqHuX0s2n/roaHQI","sig":"MEYCIQDR2nraonxpg0gD5P5IgQBBR7mSahNKzV5IgECIzZb5vAIhALNDJGTZCoCEIP1DmWmGpYI3c0ySuK/wM8Q/mGPIIqKc"}]} |
b36f795
to
a324aed
Compare
The following is the coverage report on the affected files.
|
pkg/chains/storage/oci/oci.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
sigImage, err := img.Signatures() |
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.
Hey @rgreinho, I believe if you want to get the payload for the attestation, you'll have to call img.Attestations()
instead of img.Signatures()
here, and then call Payload()
on the output of 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.
It worked 🤗
The following is the coverage report on the affected files.
|
@priyawadhwa I think I implemented everything now 😀 However I have a few of questions:
|
The e2e tests failed due to a problem with the test Kubernetes cluster 🧪 |
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 looking really good! Left a few comments.
The following is the coverage report on the affected files.
|
bf02129
to
88268cf
Compare
The following is the coverage report on the affected files.
|
88268cf
to
53c7ec9
Compare
The following is the coverage report on the affected files.
|
53c7ec9
to
b50fb02
Compare
The following is the coverage report on the affected files.
|
b50fb02
to
9294d8c
Compare
9294d8c
to
6ba8dcf
Compare
The following is the coverage report on the affected files.
|
Implements the features allowing to retrrieve the payloads and the signatures stored in an OCI backend. Fixes tektoncd#220
6ba8dcf
to
29e9cdd
Compare
The following is the coverage report on the affected files.
|
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! Thanks for your patience and work in getting this merged @rgreinho , this is awesome :D
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: priyawadhwa 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 |
/lgtm |
Implements the features allowing to retrrieve the payloads and the
signatures stored in an OCI backend.
Fixes #220