Skip to content
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

artifacts.oci.format: tekton causes signature not being stored to the OCI or annotations. #332

Closed
pxp928 opened this issue Jan 13, 2022 · 11 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten

Comments

@pxp928
Copy link
Member

pxp928 commented Jan 13, 2022

Expected Behavior

Configmap setting for chains:

  artifacts.oci.format: tekton
  artifacts.oci.storage: tekton,oci
  artifacts.taskrun.format: in-toto
  artifacts.taskrun.storage: tekton,oci

Does the OCI format tekton need to be removed? It causes signature not to be store in OCI. The documentation and code options need to be removed if this is not intended to work.

Running cosign verify --key k8s://tekton-chains/signing-secrets "${DOCKER_IMG}":
latest
sha256-46fbe3e8658a6eb85b99693aee3137ccc522af95213fb10d19785404c98e4b92.att
Error: no matching signatures:

main.go:48: error during command execution: no matching signatures:

Steps to Reproduce the Problem

  1. Build and deployed chains via the main branch using ko
  2. Change the configmap to the above
  3. Running through the tutorial on chains to store in oci

Additional Info

  • Kubernetes version:
kubectl version
Client Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.4", GitCommit:"b695d79d4f967c403a96986f1750a35eb75e75f1", GitTreeState:"clean", BuildDate:"2021-11-17T15:48:33Z", GoVersion:"go1.16.10", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.4", GitCommit:"b695d79d4f967c403a96986f1750a35eb75e75f1", GitTreeState:"clean", BuildDate:"2021-11-17T15:42:41Z", GoVersion:"go1.16.10", Compiler:"gc", Platform:"linux/amd64"}
  • Tekton Pipeline version:
kubectl get pods -n tekton-pipelines -l app=tekton-pipelines-controller -o=jsonpath='{.items[0].metadata.labels.version}'
v0.29.0
@pxp928 pxp928 added the kind/bug Categorizes issue or PR as related to a bug. label Jan 13, 2022
@priyawadhwa
Copy link
Contributor

+1 to removing the tekton option for artifacts.oci.format

@pxp928
Copy link
Member Author

pxp928 commented Jan 13, 2022

@priyawadhwa Should the storage backend for artifacts.oci.storage also not have tekton as an option?

asString(ociFormatKey, &cfg.Artifacts.OCI.Format, "tekton", "simplesigning"),
asStringSet(ociStorageKey, &cfg.Artifacts.OCI.StorageBackend, sets.NewString("tekton", "oci", "gcs", "docdb")),
asString(ociSignerKey, &cfg.Artifacts.OCI.Signer, "x509", "kms"),

@priyawadhwa
Copy link
Contributor

Good catch! It probably makes sense to only have oci as an option for artifacts.oci.storage actually

@pxp928
Copy link
Member Author

pxp928 commented Jan 18, 2022

Good catch! It probably makes sense to only have oci as an option for artifacts.oci.storage actually

@priyawadhwa
Hmm...Looks like a lot of the integration test fail when the artifacts.oci.storage to only oci. Does the tekton option need to remain for artifacts.oci.storage

=== RUN   TestOCISigning
=== RUN   TestOCISigning/x509
    e2e_test.go:147: Create namespace earth-8rxwm to deploy to
    e2e_test.go:159: Created TaskRun: image-taskv2xmt
    e2e_test.go:168: map[chains.tekton.dev/retries:3 chains.tekton.dev/signed:failed chains.tekton.dev/transparency-upload:true pipeline.tekton.dev/release:918ca4f]
    e2e_test.go:189: failed to verify signature
    clients.go:114: Deleting namespace earth-8rxwm
=== RUN   TestOCISigning/cosign
    e2e_test.go:147: Create namespace earth-sd76s to deploy to
    e2e_test.go:159: Created TaskRun: image-task4swzg
    e2e_test.go:168: map[chains.tekton.dev/retries:3 chains.tekton.dev/signed:failed chains.tekton.dev/transparency-upload:true pipeline.tekton.dev/release:918ca4f]
    e2e_test.go:189: failed to verify signature
    clients.go:114: Deleting namespace earth-sd76s
--- FAIL: TestOCISigning (196.93s)
    --- FAIL: TestOCISigning/x509 (97.97s)
    --- FAIL: TestOCISigning/cosign (98.96s)

When changing to artifacts.oci.storage to nothing (empty quotes) or to oci both cause the integration test to fail and signing to fail.

@priyawadhwa
Copy link
Contributor

Thanks for pointing this out, we probably should keep the tests just as an easy way to test signing the simplesigning format. for artifacts.oci.format we can remove the tekton format and only allow simplesigning. For artifacts.oci.storage let's leave the storage options as they are for now!

WDYT @pxp928 ?

@pxp928
Copy link
Member Author

pxp928 commented Jan 18, 2022

@priyawadhwa I can update the integration test and such to do it the proper way. I am just not sure what the artifacts.oci.storage set to tekton is used for? In the past disabling the the artifacts.oci.storage to nothing ("") works to solve the issue but still fails to verify the signature. Could this be where cosign is now looking for base64 encoded signature and in the test we are decoding?

sig, body := tr.Annotations["chains.tekton.dev/signature-05f95b26ed10"], tr.Annotations["chains.tekton.dev/payload-05f95b26ed10"]
// base64 decode them
sigBytes, err := base64.StdEncoding.DecodeString(sig)
if err != nil {
	t.Error(err)
}

@priyawadhwa
Copy link
Contributor

I am just not sure what the artifacts.oci.storage set to tekton is used for?

I think it's used for tests like you said, as an easy way to make sure signing is happening without requiring a push to an OCI registry. We do need to set artifacts.oci.storage=tekton for the annotation to show up. Does that answer your question??

@tekton-robot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

@tekton-robot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

@tekton-robot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

@tekton-robot
Copy link

@tekton-robot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten
Projects
None yet
Development

No branches or pull requests

3 participants