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

[TEP-0091] Add VerificationPolicy types to configure public keys #5714

Merged
merged 1 commit into from
Dec 19, 2022

Conversation

Yongxuanzhang
Copy link
Member

@Yongxuanzhang Yongxuanzhang commented Nov 3, 2022

Changes

This commit adds VerificationPolicy as a new type under pkg/apis/pipeline/v1alpha1, via VerificationPolicy users can config public keys for resources verification via CRD. The Pattern field in VerificationPolicy can be configured to filter out the resources to get corresponding keys.

This commits mainly consists of

  1. New types at pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go;
  2. Verification via VerificationPolicy at pkg/trustedresources/verify.go

Signed-off-by: Yongxuanzhang [email protected]

/kind feature

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

  • Has Docs included if any changes are user facing
  • Has Tests included if any functionality added or changed
  • Follows the commit message standard
  • Meets the Tekton contributor standards (including
    functionality, content, code)
  • Has a kind label. You can add one by adding a comment on this PR that contains /kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tep
  • Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
  • Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

VerificationPolicy is added as a v1alpha1 type to enable users to config public keys for trusted resources.
Please refer to https://github.com/tektoncd/pipeline/blob/main/docs/trusted-resources.md for more details

@tekton-robot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@tekton-robot tekton-robot added kind/feature Categorizes issue or PR as related to a new feature. release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Nov 3, 2022
@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 3, 2022
@Yongxuanzhang
Copy link
Member Author

/hold
better wait tektoncd/community#830 is merged

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 3, 2022
@Yongxuanzhang Yongxuanzhang marked this pull request as ready for review November 3, 2022 14:32
@tekton-robot 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 Nov 3, 2022
@Yongxuanzhang Yongxuanzhang changed the title Add verificaitonpolicy [TEP-0091] Add VerificationPolicy types to configure public keys Nov 3, 2022
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/verificationpolicy_defaults.go Do not exist N/A
pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go Do not exist 0.0%
pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go Do not exist 100.0%
pkg/trustedresources/verify.go 86.3% 78.4% -7.9

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/verificationpolicy_defaults.go Do not exist N/A
pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go Do not exist 0.0%
pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go Do not exist 100.0%
pkg/trustedresources/verify.go 86.3% 78.4% -7.9
test/trustedresources.go 15.4% 12.8% -2.6

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2022
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2022
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/verificationpolicy_defaults.go Do not exist N/A
pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go Do not exist 0.0%
pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go Do not exist 100.0%
pkg/trustedresources/verify.go 86.3% 78.4% -7.9
test/trustedresources.go 15.4% 12.8% -2.6

@Yongxuanzhang
Copy link
Member Author

/assign @wlynch @jagathprakash

@tekton-robot
Copy link
Collaborator

@Yongxuanzhang: GitHub didn't allow me to assign the following users: jagathprakash.

Note that only tektoncd members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @wlynch @jagathprakash

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.

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/verificationpolicy_defaults.go Do not exist N/A
pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go Do not exist 0.0%
pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go Do not exist 100.0%
pkg/trustedresources/verify.go 86.3% 78.4% -7.9
test/trustedresources.go 15.4% 12.8% -2.6

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/verificationpolicy_defaults.go Do not exist N/A
pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go Do not exist 0.0%
pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go Do not exist 100.0%
pkg/trustedresources/verify.go 86.3% 78.4% -7.9
test/trustedresources.go 15.4% 12.8% -2.6

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/pipeline/v1alpha1/verificationpolicy_defaults.go Do not exist N/A
pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go Do not exist 0.0%
pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go Do not exist 100.0%
pkg/trustedresources/verify.go 86.3% 77.3% -9.0
test/trustedresources.go 15.4% 12.8% -2.6

Copy link
Collaborator

@bobcatfish bobcatfish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey hey I've taken an initial look at docs and tests in particular (since getting those in a good place really provides a lot of clarity around the rest of the changes XD)

  • Before merging, plz squash the 6 commits into one or two commits with detailed commit messages (right now looks like there are a few that are just 'fix lint' 🙏
  • nit: probably dont need to include the example in the release notes, can just point users toward the docs

Feel free to remove after commit squashing:
/hold

docs/trusted-resources.md Show resolved Hide resolved
docs/trusted-resources.md Outdated Show resolved Hide resolved
docs/trusted-resources.md Show resolved Hide resolved
test/trusted_resources_test.go Show resolved Hide resolved
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/config/feature_flags.go 86.1% 86.7% 0.5
pkg/apis/pipeline/v1alpha1/verificationpolicy_defaults.go Do not exist N/A
pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go Do not exist 0.0%
pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go Do not exist 100.0%
pkg/reconciler/pipelinerun/pipelinerun.go 86.2% 86.1% -0.2
pkg/reconciler/pipelinerun/resources/pipelineref.go 92.6% 95.0% 2.4
pkg/reconciler/taskrun/controller.go 95.0% 95.2% 0.2
pkg/reconciler/taskrun/resources/taskref.go 88.7% 90.7% 1.9
pkg/reconciler/taskrun/taskrun.go 82.5% 82.6% 0.1
pkg/trustedresources/verifier/verifier.go Do not exist 100.0%
pkg/trustedresources/verify.go 86.3% 95.8% 9.5
test/controller.go 29.2% 28.1% -1.1
test/trustedresources.go 15.4% 10.0% -5.4

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/config/feature_flags.go 86.1% 86.7% 0.5
pkg/apis/pipeline/v1alpha1/verificationpolicy_defaults.go Do not exist N/A
pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go Do not exist 0.0%
pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go Do not exist 100.0%
pkg/reconciler/pipelinerun/pipelinerun.go 86.2% 84.4% -1.8
pkg/reconciler/pipelinerun/resources/pipelineref.go 92.6% 95.0% 2.4
pkg/reconciler/taskrun/controller.go 95.0% 95.5% 0.5
pkg/reconciler/taskrun/resources/taskref.go 88.7% 90.7% 1.9
pkg/reconciler/taskrun/taskrun.go 82.5% 81.3% -1.2
pkg/trustedresources/verifier/verifier.go Do not exist 100.0%
pkg/trustedresources/verify.go 86.3% 95.8% 9.5
test/controller.go 29.2% 28.1% -1.1
test/trustedresources.go 15.4% 10.2% -5.2

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/config/feature_flags.go 86.1% 86.7% 0.5
pkg/apis/pipeline/v1alpha1/verificationpolicy_defaults.go Do not exist N/A
pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go Do not exist 0.0%
pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go Do not exist 100.0%
pkg/reconciler/pipelinerun/pipelinerun.go 86.2% 85.1% -1.1
pkg/reconciler/pipelinerun/resources/pipelineref.go 92.6% 95.0% 2.4
pkg/reconciler/taskrun/controller.go 95.0% 95.7% 0.7
pkg/reconciler/taskrun/resources/taskref.go 88.7% 90.7% 1.9
pkg/reconciler/taskrun/taskrun.go 82.5% 81.8% -0.7
pkg/trustedresources/verifier/verifier.go Do not exist 100.0%
pkg/trustedresources/verify.go 86.3% 95.8% 9.5
test/controller.go 29.2% 28.1% -1.1
test/trustedresources.go 15.4% 10.2% -5.2

@@ -31,6 +31,7 @@ import (
fakekubeclient "knative.dev/pkg/client/injection/kube/client/fake"
fakelimitrangeinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/limitrange/fake"
fakeserviceaccountinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/serviceaccount/fake"
_ "knative.dev/pkg/system/testing"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import side effects are pretty dangerous, and 'system testing' import side effects seem especially mysterious - if we can't remove this now and we have to include it to make the unit tests pass, can you create an issue to dig into what is going on here and see if we can avoid this side effect import?

Copy link
Member Author

@Yongxuanzhang Yongxuanzhang Dec 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some other places in our code base using this. I think it is due to the use of system.Namespace()
https://github.com/tektoncd/pipeline/search?q=_+%22knative.dev%2Fpkg%2Fsystem%2Ftesting%22

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/config/feature_flags.go 86.1% 86.7% 0.5
pkg/apis/pipeline/v1alpha1/verificationpolicy_defaults.go Do not exist N/A
pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go Do not exist 0.0%
pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go Do not exist 100.0%
pkg/reconciler/pipelinerun/pipelinerun.go 86.2% 86.1% -0.2
pkg/reconciler/pipelinerun/resources/pipelineref.go 92.6% 95.0% 2.4
pkg/reconciler/taskrun/controller.go 95.0% 95.2% 0.2
pkg/reconciler/taskrun/resources/taskref.go 88.7% 90.7% 1.9
pkg/reconciler/taskrun/taskrun.go 82.5% 82.6% 0.1
pkg/trustedresources/verifier/verifier.go Do not exist 100.0%
pkg/trustedresources/verify.go 86.3% 95.8% 9.5
test/controller.go 29.2% 28.1% -1.1
test/trustedresources.go 15.4% 10.0% -5.4

@bobcatfish
Copy link
Collaborator

Discussed making the update to use the secret lister in a subsequent PR and sgtm 👍

@wlynch you have "changes requested" set on this PR but i'm under the impression that @Yongxuanzhang followed up with you today and you don't have any blocking concerns

Thanks for all your hard work and back and forth on this @Yongxuanzhang !

dancing pusheen

/approve
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2022
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2022
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/config/feature_flags.go 86.1% 86.7% 0.5
pkg/apis/pipeline/v1alpha1/verificationpolicy_defaults.go Do not exist N/A
pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go Do not exist 0.0%
pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go Do not exist 100.0%

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 19, 2022
@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/config/feature_flags.go 86.1% 86.7% 0.5
pkg/apis/pipeline/v1alpha1/verificationpolicy_defaults.go Do not exist N/A
pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go Do not exist 0.0%
pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go Do not exist 100.0%
pkg/reconciler/pipelinerun/pipelinerun.go 86.2% 86.1% -0.2
pkg/reconciler/pipelinerun/resources/pipelineref.go 92.6% 95.0% 2.4
pkg/reconciler/taskrun/controller.go 95.0% 95.2% 0.2
pkg/reconciler/taskrun/resources/taskref.go 88.7% 90.7% 1.9
pkg/reconciler/taskrun/taskrun.go 82.5% 82.6% 0.1
pkg/trustedresources/verifier/verifier.go Do not exist 100.0%
pkg/trustedresources/verify.go 86.3% 95.8% 9.5
test/controller.go 29.2% 28.1% -1.1
test/trustedresources.go 15.4% 10.0% -5.4

This commit adds VerificationPolicy as a new type under
`pkg/apis/pipeline/v1alpha1`, via VerificationPolicy users can
config public keys in the CRD for resources verification. The mapping
from resources to keys can be done via `pattern`
Comment on lines +262 to +264
name: "signed task with sha384 key",
task: signedTask384,
source: "gcr.io/tekton-releases/catalog/upstream/sha384",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the case to test sha384 key @wlynch

@tekton-robot
Copy link
Collaborator

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

File Old Coverage New Coverage Delta
pkg/apis/config/feature_flags.go 86.1% 86.7% 0.5
pkg/apis/pipeline/v1alpha1/verificationpolicy_defaults.go Do not exist N/A
pkg/apis/pipeline/v1alpha1/verificationpolicy_types.go Do not exist 0.0%
pkg/apis/pipeline/v1alpha1/verificationpolicy_validation.go Do not exist 100.0%
pkg/reconciler/pipelinerun/pipelinerun.go 86.2% 86.1% -0.2
pkg/reconciler/pipelinerun/resources/pipelineref.go 92.6% 95.0% 2.4
pkg/reconciler/taskrun/controller.go 95.0% 95.2% 0.2
pkg/reconciler/taskrun/resources/taskref.go 88.7% 90.7% 1.9
pkg/reconciler/taskrun/taskrun.go 82.5% 82.6% 0.1
pkg/trustedresources/verifier/verifier.go Do not exist 100.0%
pkg/trustedresources/verify.go 86.3% 95.8% 9.5
test/controller.go 29.2% 28.1% -1.1
test/trustedresources.go 15.4% 10.2% -5.2

@wlynch wlynch self-requested a review December 19, 2022 18:48
Copy link
Member

@wlynch wlynch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2022
@tekton-robot tekton-robot merged commit c39fd32 into tektoncd:main Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants