Add new signing and storage features by rgreinho · Pull Request #245 · tektoncd/chains

rgreinho · 2021-09-30T16:32:44Z

Adds a set of new features to the signing and storage packages to allow
extracting chains information and verify the signature.

tekton-robot · 2021-09-30T16:32:59Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

priyawadhwa

Thanks for opening the draft PR! This is a great start. WDYT of creating a Verifier interface analogous to the Signer interface we already have?

I'm kinda imagining something like this, but haven't fully thought through if it would be easier to implement this or do like a TaskRunSignerVerifier type thing instead :)

type Verifier interface {
    VerifyTaskRun(ctx context.Context, tr *v1beta1.TaskRun) error)
}

type TaskRunVerifier struct {
         // use this to read the configmap and get details for the signer&storage backend
	KubeClient        kubernetes.Interface

         // looks like we need this for getting signers
       	SecretPath        string
}

rgreinho · 2021-10-08T00:31:08Z

@priyawadhwa I think I am on a good track here.

I turned the signer into a SignerVerifyer so that we can use the same object for both operations and it was straightforward to implement.

I added functions to the various supported backends to allow retrieving the payload and the signature. However for OCI backend I don't think we can implement these features right now because we do not store the information of the target image, mostly the digest which is computed for the upload then forgotten. Please correct me if I am mistaken, in which case a few pointers would be appreciated.

I am going to try to use what I created in the CLI (tektoncd/cli#1440) in order to move forward with this PR as well and bring more testing/validation to theses new features.

priyawadhwa

This is looking really good! Just left a couple small comments.

I think we can actually use this code in our integration tests as well, which would be nice because it should simplify the test & we can make sure this works correctly!

For each test we do manual verification -- would it be easy to use this interface to verify instead? e.g. we could replace this code with your verification code:

chains/test/e2e_test.go

Lines 92 to 107 in 231a880

    
           sigKey := fmt.Sprintf("chains.tekton.dev/signature-taskrun-%s", tr.UID) 
        
           payloadKey := fmt.Sprintf("chains.tekton.dev/payload-taskrun-%s", tr.UID) 
        
           signature, body := tr.Annotations[sigKey], tr.Annotations[payloadKey] 
        
           // base64 decode them 
        
           sigBytes, err := base64.StdEncoding.DecodeString(signature) 
        
           if err != nil { 
        
           	t.Error(err) 
        
           } 
        
           bodyBytes, err := base64.StdEncoding.DecodeString(body) 
        
           if err != nil { 
        
           	t.Error(err) 
        
           } 
        
           if err := c.secret.x509priv.VerifySignature(bytes.NewReader(sigBytes), bytes.NewReader(bodyBytes)); err != nil { 
        
           	t.Fatal(err) 
        
           }

priyawadhwa · 2021-10-08T19:08:26Z

 	SecretPath        string
 }

+type Verifier interface {


could we move all verification code to pkg/chains/verify.go? might just make it easier to find

This is looking really good! Just left a couple small comments.

I think we can actually use this code in our integration tests as well, which would be nice because it should simplify the test & we can make sure this works correctly!

For each test we do manual verification -- would it be easy to use this interface to verify instead? e.g. we could replace this code with your verification code:

I like the sound of it 😃

However I am facing the same problem as in the CLI, thus I am glad you brought this up.

In order to do what you described above, I need to create a new backend. I attempted to do create one using the storage.InitializeBackends() function, but I am not sure what the parameters are and how to get them, especially ps versioned.Interface, kc kubernetes.Interface and cfg config.Config. I already had the TaskRun, so this parameter was fine, and for the logger, I just created a new Zap SugarLogger. Could you provide some pointers regarding the rest?

could we move all verification code to pkg/chains/verify.go? might just make it easier to find

Absolutely 👍

For sure! So to verify I believe all we should need are:

cfg config.Config

kc kubernetes.Interface, which we'll use to actually read the ConfigMap into cfg

We can use kc to get the ConfigMap object from the cluster, and then we can pass the data into NewConfigFromConfigMap to get cfg. Once we have cfg, I believe that's everything we need for verification.

I don't think we need the PipelineClientset for anything, just the kubernetes Client should be sufficient.

Does this make sense?

rgreinho · 2021-10-10T19:50:03Z

Yes it does! Thank you for the pointers,

So I updated a few tests. I actually needed the PipelineClientset for the storage function I added for the tekton storage ( retrieveAnnotationValue). I created a helper function to verify the signature. Let me know what you think.

Side question: when are the e2e tests being executed? For instance I could not test the GCS implementation as it requires an account, so being able to see it run in a CI somewhere would have been fantastic.

priyawadhwa

left a few comments :)

priyawadhwa · 2021-10-11T17:43:45Z

I actually needed the PipelineClientset for the storage function I added for the tekton storage ( retrieveAnnotationValue). I created a helper function to verify the signature. Let me know what you think.

LGTM!

Side question: when are the e2e tests being executed? For instance I could not test the GCS implementation as it requires an account, so being able to see it run in a CI somewhere would have been fantastic.

Ah yes, so the e2e tests are executed on an ephemeral k8s cluster created by prow. we acutally don't run the GCS tests at the moment bc that cluster doesn't have the required auth.

you can skip testing it for now, i can manually run the test once this is merged!

rgreinho · 2021-10-11T23:19:00Z

Alright, all the comments are addressed.

I also used this change in tektoncd/cli#1440 to see how it would look, and I think it reads pretty well (e.g.: https://github.com/tektoncd/cli/blob/16aa1f2da2b9ae697d76b4ebf91a7163a5f36a52/pkg/cmd/chains/payload.go).

priyawadhwa · 2021-10-11T23:43:39Z

Awesome! This is looking pretty good to me, let's try and run the tests and hopefully they're happy 🤞🏽

priyawadhwa · 2021-10-11T23:43:46Z

/ok-to-test

tekton-robot · 2021-10-11T23:45:05Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/storage/docdb/docdb.go	35.7%	55.2%	19.5
pkg/chains/storage/gcs/gcs.go	31.7%	41.8%	10.1
pkg/chains/storage/tekton/tekton.go	55.6%	73.3%	17.8
pkg/chains/verifier.go	Do not exist	0.0%

tekton-robot · 2021-10-12T00:54:28Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/storage/docdb/docdb.go	35.7%	55.2%	19.5
pkg/chains/storage/gcs/gcs.go	31.7%	41.8%	10.1
pkg/chains/storage/tekton/tekton.go	55.6%	73.3%	17.8
pkg/chains/verifier.go	Do not exist	0.0%

tekton-robot · 2021-10-12T01:03:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/storage/docdb/docdb.go	35.7%	55.2%	19.5
pkg/chains/storage/gcs/gcs.go	31.7%	41.8%	10.1
pkg/chains/storage/tekton/tekton.go	55.6%	73.3%	17.8
pkg/chains/verifier.go	Do not exist	0.0%

rgreinho · 2021-10-12T01:49:56Z

OK, looks like I got it right after a few tries 😅

Do you need me to squash the commits?

Also, side note for now, but it will become important soon, if I import the chains module into tekton cli, the CLI does not build anymore:

╭ cli on  chains-cli via 🐹 v1.17.2 ⌛ 4s
╰❯ go build ./cmd/tkn
# github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1
vendor/github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1/openapi_generated.go:117:3: cannot use "github.com/go-openapi/spec".Schema{...} (type "github.com/go-openapi/spec".Schema) as type "k8s.io/kube-openapi/pkg/validation/spec".Schema in field value
vendor/github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1/openapi_generated.go:146:11: cannot use ref("k8s.io/api/core/v1.Toleration") (type "k8s.io/kube-openapi/pkg/validation/spec".Ref) as type "github.com/go-openapi/spec".Ref in field value
vendor/github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1/openapi_generated.go:155:8: cannot use ref("k8s.io/api/core/v1.Affinity") (type "k8s.io/kube-openapi/pkg/validation/spec".Ref) as type "github.com/go-openapi/spec".Ref in field value
vendor/github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1/openapi_generated.go:161:8: cannot use ref("k8s.io/api/core/v1.PodSecurityContext") (type "k8s.io/kube-openapi/pkg/validation/spec".Ref) as type "github.com/go-openapi/spec".Ref in field value
vendor/github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1/openapi_generated.go:178:11: cannot use ref("k8s.io/api/core/v1.Volume") (type "k8s.io/kube-openapi/pkg/validation/spec".Ref) as type "github.com/go-openapi/spec".Ref in field value
vendor/github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1/openapi_generated.go:208:8: cannot use ref("k8s.io/api/core/v1.PodDNSConfig") (type "k8s.io/kube-openapi/pkg/validation/spec".Ref) as type "github.com/go-openapi/spec".Ref in field value
vendor/github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1/openapi_generated.go:240:11: cannot use ref("k8s.io/api/core/v1.LocalObjectReference") (type "k8s.io/kube-openapi/pkg/validation/spec".Ref) as type "github.com/go-openapi/spec".Ref in field value
vendor/github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1/openapi_generated.go:254:11: cannot use ref("k8s.io/api/core/v1.HostAlias") (type "k8s.io/kube-openapi/pkg/validation/spec".Ref) as type "github.com/go-openapi/spec".Ref in field value
vendor/github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1/openapi_generated.go:277:3: cannot use "github.com/go-openapi/spec".Schema{...} (type "github.com/go-openapi/spec".Schema) as type "k8s.io/kube-openapi/pkg/validation/spec".Schema in field value
vendor/github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1/openapi_generated.go:320:3: cannot use "github.com/go-openapi/spec".Schema{...} (type "github.com/go-openapi/spec".Schema) as type "k8s.io/kube-openapi/pkg/validation/spec".Schema in field value
vendor/github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1/openapi_generated.go:320:3: too many errors

@vdemeester fixed some dependencies and replacements in tektoncd/cli@f44ccbd, so we may have to do something similar here as well.

sbose78 · 2021-10-12T16:10:05Z

@rgreinho Noob request: would you mind adding a PR description listing the changes/improvements that are going in :) ?

rgreinho · 2021-10-12T18:08:59Z

Absolutely:

Update the Signer interface to also handle verifying a signature.
Add new TaskRunVerifier struct to verify the signature of a task run.
Update the Backend interface to add functions allowing to easily retrieve a payload and a signature.
Implement the new functions of the Backend interface for the tekton, gcs, and docdb backends.

@priyawadhwa there is no changelog file for this project, should we start one?

priyawadhwa · 2021-10-13T05:14:27Z

Do you need me to squash the commits?

Yah I think the commits will need to be squashed for prow to merge :)

if I import the chains module into tekton cli, the CLI does not build anymore:

Hmm if it's just a matter of updating our deps that should be easy enough! I can look into it more if it's not that simple.

@priyawadhwa there is no changelog file for this project, should we start one?

Sure! Thanks :)

vdemeester · 2021-10-13T06:42:32Z

Do you need me to squash the commits?

Yah I think the commits will need to be squashed for prow to merge :)

Not necessarily needed. I think tektoncd/chains can support all github merge ways and doesn't require commits to be squashed. But usually it is prefered 😝 .

@priyawadhwa there is no changelog file for this project, should we start one?

Sure! Thanks :)

On other tekton project, so far we rely on a release-note "special preformatted paragraph" to build the release-note/changelog of a release (https://github.com/tektoncd/pipeline/releases/tag/v0.28.0 is mostly generated automatically). Not sure if chains wants to adopt this or not, just thought it was worth mentionning 😉

tekton-robot · 2021-10-13T13:45:07Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/storage/docdb/docdb.go	35.7%	55.2%	19.5
pkg/chains/storage/gcs/gcs.go	31.7%	41.8%	10.1
pkg/chains/storage/tekton/tekton.go	55.6%	73.3%	17.8
pkg/chains/verifier.go	Do not exist	0.0%

priyawadhwa · 2021-10-14T16:49:12Z

We could try updating the github.com/tektoncd/pipeline dep here to v0.28.0 to match what we have in cli, might fix the dependency issue 🤞🏽

priyawadhwa · 2021-10-14T16:57:46Z

Not sure if chains wants to adopt this or not, just thought it was worth mentionning 😉

That sounds really good! I'll look into it :)

rgreinho · 2021-10-14T18:26:59Z

We could try updating the github.com/tektoncd/pipeline dep here to v0.28.0 to match what we have in cli, might fix the dependency issue 🤞🏽

I moved this problem to another issue: #269

Adds a set of new features to the signing and storage packages to allow extracting chains information and verify the signature. Adds implementations andi for the following backends: * Tekton * DocDB * GCS The unit and end to end tests have been updated accordingly.

rgreinho · 2021-10-14T18:35:57Z

I cleaned up the PR and it is now ready for the final review 🤗

tekton-robot · 2021-10-14T18:38:20Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/storage/docdb/docdb.go	35.7%	55.2%	19.5
pkg/chains/storage/gcs/gcs.go	31.7%	41.8%	10.1
pkg/chains/storage/tekton/tekton.go	55.6%	73.3%	17.8
pkg/chains/verifier.go	Do not exist	0.0%

priyawadhwa

lgtm! thanks @rgreinho 🎉 🔥

tekton-robot · 2021-10-19T16:55:03Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [priyawadhwa]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

priyawadhwa · 2021-10-19T16:55:04Z

/ok-to-test

priyawadhwa · 2021-10-19T16:55:10Z

/lgtm

tekton-robot · 2021-10-19T16:57:10Z

The following is the coverage report on the affected files.
Say /test pull-tekton-chains-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/chains/storage/docdb/docdb.go	35.7%	55.2%	19.5
pkg/chains/storage/gcs/gcs.go	31.7%	41.8%	10.1
pkg/chains/storage/tekton/tekton.go	55.6%	73.3%	17.8
pkg/chains/verifier.go	Do not exist	0.0%

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 30, 2021

tekton-robot requested review from font and lukehinds September 30, 2021 16:32

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 30, 2021

priyawadhwa reviewed Sep 30, 2021

View reviewed changes

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 3, 2021

rgreinho force-pushed the storage-features branch from 067e209 to f63379a Compare October 8, 2021 00:19

rgreinho marked this pull request as ready for review October 8, 2021 00:44

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 8, 2021

priyawadhwa reviewed Oct 8, 2021

View reviewed changes

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 10, 2021

priyawadhwa reviewed Oct 11, 2021

View reviewed changes

Comment thread pkg/chains/storage/gcs/gcs.go Outdated

Comment thread pkg/chains/storage/oci/oci.go

Comment thread test/test_utils.go Outdated

rgreinho mentioned this pull request Oct 11, 2021

Implement missing function of the Backend interface in OCI storage #260

Closed

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 11, 2021

rgreinho force-pushed the storage-features branch from e87c85d to e39fc3e Compare October 13, 2021 13:42

priyawadhwa mentioned this pull request Oct 14, 2021

Look into generating automatic release docs #268

Closed

rgreinho mentioned this pull request Oct 14, 2021

CLI does not build when importing Chains module #269

Closed

rgreinho force-pushed the storage-features branch from e39fc3e to 2c6b266 Compare October 14, 2021 18:35

priyawadhwa approved these changes Oct 19, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2021

tekton-robot assigned priyawadhwa Oct 19, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2021

tekton-robot merged commit eee4475 into tektoncd:main Oct 19, 2021

rgreinho deleted the storage-features branch February 2, 2022 21:42

	sigKey := fmt.Sprintf("chains.tekton.dev/signature-taskrun-%s", tr.UID)
	payloadKey := fmt.Sprintf("chains.tekton.dev/payload-taskrun-%s", tr.UID)
	signature, body := tr.Annotations[sigKey], tr.Annotations[payloadKey]
	// base64 decode them
	sigBytes, err := base64.StdEncoding.DecodeString(signature)
	if err != nil {
	t.Error(err)
	}
	bodyBytes, err := base64.StdEncoding.DecodeString(body)
	if err != nil {
	t.Error(err)
	}

	if err := c.secret.x509priv.VerifySignature(bytes.NewReader(sigBytes), bytes.NewReader(bodyBytes)); err != nil {
	t.Fatal(err)
	}

Uh oh!

Conversation

rgreinho commented Sep 30, 2021

Uh oh!

tekton-robot commented Sep 30, 2021

Uh oh!

priyawadhwa left a comment

Choose a reason for hiding this comment

Uh oh!

rgreinho commented Oct 8, 2021

Uh oh!

priyawadhwa left a comment

Choose a reason for hiding this comment

Uh oh!

priyawadhwa Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

rgreinho Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

rgreinho Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

priyawadhwa Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rgreinho commented Oct 10, 2021

Uh oh!

priyawadhwa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

priyawadhwa commented Oct 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rgreinho commented Oct 11, 2021

Uh oh!

priyawadhwa commented Oct 11, 2021

Uh oh!

priyawadhwa commented Oct 11, 2021

Uh oh!

tekton-robot commented Oct 11, 2021

Uh oh!

tekton-robot commented Oct 12, 2021

Uh oh!

tekton-robot commented Oct 12, 2021

Uh oh!

rgreinho commented Oct 12, 2021

Uh oh!

sbose78 commented Oct 12, 2021

Uh oh!

rgreinho commented Oct 12, 2021

Uh oh!

priyawadhwa commented Oct 13, 2021

Uh oh!

vdemeester commented Oct 13, 2021

Uh oh!

tekton-robot commented Oct 13, 2021

Uh oh!

priyawadhwa commented Oct 14, 2021

Uh oh!

priyawadhwa commented Oct 14, 2021

Uh oh!

rgreinho commented Oct 14, 2021

Uh oh!

rgreinho commented Oct 14, 2021

Uh oh!

tekton-robot commented Oct 14, 2021

Uh oh!

priyawadhwa left a comment

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Oct 19, 2021

Uh oh!

priyawadhwa commented Oct 19, 2021

priyawadhwa commented Oct 11, 2021 •

edited

Loading