move trusted resources verification after we resolve the remote resources #6621

Yongxuanzhang · 2023-05-04T21:06:05Z

Changes

This PR moves the trusted resources verification to readRuntimeObjectAsTask and readRuntimeObjectAsPipline, the reasons we need this change include:

unblock the work for v1, since v1 will mutate, validate and convert the resources, the mutation will break trusted resources verification thus we need to verify right after we resolve the remote resources.
Prepare the support for verifying different api versions. This commit also makes it clear that currently we only support verification for remote resources.

/kind misc
Closes #6618

Signed-off-by: Yongxuan Zhang [email protected]

Submitter Checklist

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

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps
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). See some examples of good release notes.
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

NONE

tekton-robot · 2023-05-04T21:06:07Z

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 · 2023-05-04T21:06:07Z

@Yongxuanzhang: The label(s) kind/misce cannot be applied, because the repository doesn't have them.

In response to this:

Changes

This PR moves the trusted resources verification to readRuntimeObjectAsTask and readRuntimeObjectAsPipline, the reasons we need this change include:

unblock the work for v1, since v1 will mutate, validate and convert the resources, the mutation will break trusted resources verification thus we need to verify right after we resolve the remote resources.

Prepare the support for verifying different api versions. This commit also makes it clear that currently we only support verification for remote resources.

/kind misce

Signed-off-by: Yongxuan Zhang [email protected]

Submitter Checklist

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

Has Docs if any changes are user facing, including updates to minimum requirements e.g. Kubernetes version bumps

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). See some examples of good release notes.

Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes
NONE

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.

Yongxuanzhang · 2023-05-04T21:06:16Z

/test all

Yongxuanzhang · 2023-05-04T21:07:40Z

/kind misc

tekton-robot · 2023-05-04T21:11:49Z

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/reconciler/pipelinerun/resources/pipelineref.go	93.1%	93.8%	0.6
pkg/reconciler/taskrun/resources/taskref.go	89.7%	90.0%	0.3
pkg/trustedresources/verify.go	95.2%	95.6%	0.3

tekton-robot · 2023-05-04T21:13:35Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.1%	93.8%	0.6
pkg/reconciler/taskrun/resources/taskref.go	89.7%	90.0%	0.3
pkg/trustedresources/verify.go	95.2%	95.6%	0.3

tekton-robot · 2023-05-05T03:08:56Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.1%	93.8%	0.6
pkg/reconciler/taskrun/resources/taskref.go	89.7%	90.0%	0.3
pkg/trustedresources/verify.go	95.2%	95.6%	0.3

Yongxuanzhang · 2023-05-05T03:58:23Z

/test all

tekton-robot · 2023-05-05T04:03:59Z

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/reconciler/pipelinerun/resources/pipelineref.go	93.1%	93.8%	0.6
pkg/reconciler/taskrun/resources/taskref.go	89.7%	90.0%	0.3
pkg/trustedresources/verify.go	95.2%	95.6%	0.3

Yongxuanzhang · 2023-05-05T04:07:57Z

test/trusted_resources_test.go

@@ -112,9 +112,16 @@ spec:
  tasks:
  - name: task
    taskRef:
-      name: %s
+      resolver: cluster


Use cluster resolve for e2e test, since we don't have these examples stored in remote storage, it's hard to test other resolvers.

Yongxuanzhang · 2023-05-05T04:11:09Z

pkg/reconciler/taskrun/resources/taskref.go

 	if err != nil {
-		return nil, nil, fmt.Errorf("failed to convert obj %s into Task", obj.GetObjectKind().GroupVersionKind().String())


We may return other errors, so we need to return err here instead of a new convert error

Yongxuanzhang · 2023-05-05T04:17:43Z

/test all

tekton-robot · 2023-05-05T04:23:26Z

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/reconciler/pipelinerun/resources/pipelineref.go	93.1%	93.8%	0.6
pkg/reconciler/taskrun/resources/taskref.go	89.7%	90.0%	0.3
pkg/trustedresources/verify.go	95.2%	95.6%	0.3

tekton-robot · 2023-05-05T04:24:25Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.1%	93.8%	0.6
pkg/reconciler/taskrun/resources/taskref.go	89.7%	90.0%	0.3
pkg/trustedresources/verify.go	95.2%	95.6%	0.3

tekton-robot · 2023-05-05T04:25:45Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.1%	93.8%	0.6
pkg/reconciler/taskrun/resources/taskref.go	89.7%	90.0%	0.3
pkg/trustedresources/verify.go	95.2%	95.6%	0.3

tekton-robot · 2023-05-05T14:06:52Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.1%	93.8%	0.6
pkg/reconciler/taskrun/resources/taskref.go	89.7%	90.0%	0.3
pkg/trustedresources/verify.go	95.2%	95.6%	0.3

tekton-robot · 2023-05-05T14:07:08Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.1%	93.8%	0.6
pkg/reconciler/taskrun/resources/taskref.go	89.7%	90.0%	0.3
pkg/trustedresources/verify.go	95.2%	95.6%	0.3

tekton-robot · 2023-05-05T14:16:34Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.1%	93.8%	0.6
pkg/reconciler/taskrun/resources/taskref.go	89.7%	90.0%	0.3
pkg/trustedresources/verify.go	95.2%	95.6%	0.3

Yongxuanzhang · 2023-05-05T14:22:27Z

pkg/reconciler/pipelinerun/pipelinerun_test.go

+	// Set up a fake registry to push an image to.
+	s := httptest.NewServer(registry.New())
+	defer s.Close()
+	u, err := url.Parse(s.URL)
+	if err != nil {
+		t.Fatal(err)
+	}


Use oci bundle to test trusted resources, we could probably test resolver here, but I think e2e test has covered the resolver case (using cluster resolver)

tekton-robot · 2023-05-08T19:14:24Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.1%	93.8%	0.6
pkg/reconciler/taskrun/resources/taskref.go	89.7%	90.0%	0.3
pkg/trustedresources/verify.go	95.2%	95.6%	0.3

tekton-robot · 2023-05-08T19:16:41Z

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/reconciler/pipelinerun/resources/pipelineref.go	93.1%	93.8%	0.6
pkg/reconciler/taskrun/resources/taskref.go	89.7%	90.0%	0.3
pkg/trustedresources/verify.go	95.2%	95.6%	0.3

tekton-robot · 2023-05-08T19:17:27Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.1%	93.8%	0.6
pkg/reconciler/taskrun/resources/taskref.go	89.7%	90.0%	0.3
pkg/trustedresources/verify.go	95.2%	95.6%	0.3

lbernick

looks a lot better! Thanks for fixing this!

tekton-robot · 2023-05-08T19:30:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernick

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:

~~OWNERS~~ [lbernick]

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

chuangw6 · 2023-05-08T20:05:58Z

pkg/reconciler/pipelinerun/resources/pipelineref.go

+// OCI bundle and remote resolution pipelines will be verified by trusted resources if the feature is enabled
+func GetPipelineFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset.Interface, requester remoteresource.Requester, pipelineRun *v1beta1.PipelineRun, verificationPolicies []*v1alpha1.VerificationPolicy) rprp.GetPipeline {


Why don't we verify the resource in the default case where in-cluster resources are resolved by the local cluster resolver?

Prior to the move, that case seems to be included for verification but after the move the case seems ignored.

Same for task reconciler.

Thanks! Local resolver is actually not fully supported due to the mutating webhook(the verification may fail) and we didn't make is clear before.

This PR also updates the doc to highlight what we support (resources from resolver&oci bundle). I'm hesitant to update the release notes since we don't actually support it before. Let me know if you have any suggestions. 😄

Do you mean that the in-cluster resources could be mutated by the mutation webhook when they are applied to the cluster, which might cause the verification fail?

If so, the remote resolver is also able to resolve the in-cluster resources via cluster resolver. Will there be same issue and how are we planning to handle that?

Yes, I also mentioned this in the doc. If users want to use cluster resolver, the resources need to set all default values.

I think currently we could just document it, then when we finish high priority features(v1 support, condition update), we can start planning solutions for local resources. Wdyt?

STGM as long as we will support that case.

I thought it's in the scope initially b/c the TEP doc mentions that "Provide an optional mechanism for Resources (local and remote tekton tasks/ pipelines) to be signed and verified."

yes, that's still in the scope, but just not the priority at this moment.

Yongxuanzhang · 2023-05-08T20:27:06Z

/retest

lbernick · 2023-05-08T20:46:45Z

This looks related to a known flake, so I've commented on the issue tracking it: #6624

chuangw6 · 2023-05-09T20:22:57Z

/lgtm

chuangw6 · 2023-05-09T20:26:06Z

/remove-lgtm

chuangw6 · 2023-05-09T20:41:16Z

docs/trusted-resources.md

-Trusted Resources is a feature which can be used to sign Tekton Resources and verify them. Details of design can be found at [TEP--0091](https://github.com/tektoncd/community/blob/main/teps/0091-trusted-resources.md). This feature is under `alpha` version and support `v1beta1` version of `Task` and `Pipeline`.
+Trusted Resources is a feature which can be used to sign Tekton Resources and verify them. Details of design can be found at [TEP--0091](https://github.com/tektoncd/community/blob/main/teps/0091-trusted-resources.md). This is an alpha feature and supports `v1beta1` version of  `Task` and `Pipeline`.
+
+**Note**: trusted resources support verification of resources from OCI bundle or remote resolution, to use [cluster resolver](./cluster-resolver.md) make sure to set all default values for the resources before applied to cluster, otherwise the verification will fail due to the mutating webhook.


I feel this paragraph is a bit confusing to users for 3 reasons: 1/. oci bundle is part of remote resolution 2. as discussed, there are 2 ways resolving cluster resources i.e. remote resolver, and local resolver. 3. what should users set for default values?

How about this?
Note: Currently, trusted resources only support verifying Tekton resources that come from remote places i.e. git, OCI registry and Artifact Hub. For in-cluster resources, verification might fail because the mutation webhook will set default values for the type of the parameters and results, which implies the resources will be modified before verification. In-cluster resources will be supported later. If one wants to use trusted resources for this case, they should set those default values for the resources before applying to the cluster so that mutation webhook doesn't affect the originality of the resource.

(Please add other areas that mutation webhook will take affect on if missed above)

Emmm, OCI bundle is another feature not part of remote resolution, this feature is supported for v1beta1 but will not be supported in v1. I can update the doc to add link. https://github.com/tektoncd/pipeline/blob/main/docs/pipelineruns.md#tekton-bundles

Thanks for the suggestion! I will update the doc.

The link you shared is the remote resolver that resolves OCI resource. The old bundle field in PipelineRef will be deprecated for v1beta1. "Deprecated: Please use ResolverRef with the bundles resolver instead."

Generally, I think we should mention where the resource came from (i.e. remote place are well supported, in-cluster resources are not supported unless default are well set before applying to cluster), instead of mentioning remote resolution.

Thanks for the suggestion! I will update the doc.

Thanks you for your hard work.

…rces This PR moves the trusted resources verification to readRuntimeObjectAsTask and readRuntimeObjectAsPipline, the reasons we need this change include 1) unblock the work for v1, since v1 will mutate, validate and convert the resources, the mutation will break trusted resources verification thus we need to verify right after we resolve the remote resources. 2) Prepare the support for verifying different api versions. This commit also makes it clear that currently we only support verification for remote resources. Signed-off-by: Yongxuan Zhang [email protected]

tekton-robot · 2023-05-09T20:58:41Z

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/reconciler/pipelinerun/resources/pipelineref.go	93.1%	93.8%	0.6
pkg/reconciler/taskrun/resources/taskref.go	89.7%	90.0%	0.3
pkg/trustedresources/verify.go	95.2%	95.6%	0.3

tekton-robot · 2023-05-09T20:59:58Z

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

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/pipelineref.go	93.1%	93.8%	0.6
pkg/reconciler/taskrun/resources/taskref.go	89.7%	90.0%	0.3
pkg/trustedresources/verify.go	95.2%	95.6%	0.3

chuangw6 · 2023-05-10T14:16:56Z

/ltgm

chuangw6 · 2023-05-10T14:27:14Z

/lgtm

tekton-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesnt merit a release note. labels May 4, 2023

tekton-robot requested review from imjasonh and lbernick May 4, 2023 21:06

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 4, 2023

tekton-robot added the kind/misc Categorizes issue or PR as a miscellaneuous one. label May 4, 2023

Yongxuanzhang commented May 5, 2023

View reviewed changes

Yongxuanzhang force-pushed the verify-when-fetch branch from 64866e4 to 283c534 Compare May 5, 2023 04:17

Yongxuanzhang force-pushed the verify-when-fetch branch 2 times, most recently from 7ddb9a4 to b640cef Compare May 5, 2023 14:00

Yongxuanzhang force-pushed the verify-when-fetch branch from b640cef to 743f9ff Compare May 5, 2023 14:08

Yongxuanzhang force-pushed the verify-when-fetch branch from 743f9ff to 2364433 Compare May 5, 2023 14:21

Yongxuanzhang commented May 5, 2023

View reviewed changes

lbernick added this to the Pipelines v0.48 milestone May 8, 2023

Yongxuanzhang force-pushed the verify-when-fetch branch from 522fee9 to f45f6a5 Compare May 8, 2023 19:05

Yongxuanzhang requested a review from lbernick May 8, 2023 19:06

Yongxuanzhang force-pushed the verify-when-fetch branch from f45f6a5 to 3bc54fb Compare May 8, 2023 19:09

lbernick approved these changes May 8, 2023

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2023

chuangw6 reviewed May 8, 2023

View reviewed changes

lbernick mentioned this pull request May 8, 2023

Flake: TestPipelineRunTasksTimeout #6624

Closed

Yongxuanzhang requested a review from chuangw6 May 9, 2023 14:34

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 9, 2023

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label May 9, 2023

chuangw6 reviewed May 9, 2023

View reviewed changes

Yongxuanzhang force-pushed the verify-when-fetch branch from 3bc54fb to 288dc00 Compare May 9, 2023 20:52

Yongxuanzhang requested a review from chuangw6 May 9, 2023 20:52

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2023

tekton-robot merged commit 77f7d0f into tektoncd:main May 10, 2023

lbernick mentioned this pull request Jul 19, 2023

Flake: TestPipelineTaskTimeout #6943

Closed

		if err != nil {
		return nil, nil, fmt.Errorf("failed to convert obj %s into Task", obj.GetObjectKind().GroupVersionKind().String())

		// OCI bundle and remote resolution pipelines will be verified by trusted resources if the feature is enabled
		func GetPipelineFunc(ctx context.Context, k8s kubernetes.Interface, tekton clientset.Interface, requester remoteresource.Requester, pipelineRun v1beta1.PipelineRun, verificationPolicies []v1alpha1.VerificationPolicy) rprp.GetPipeline {

move trusted resources verification after we resolve the remote resources #6621

move trusted resources verification after we resolve the remote resources #6621

Conversation

Yongxuanzhang commented May 4, 2023 • edited by lbernick Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented May 4, 2023

tekton-robot commented May 4, 2023

Changes

Submitter Checklist

Release Notes

Yongxuanzhang commented May 4, 2023

Yongxuanzhang commented May 4, 2023

tekton-robot commented May 4, 2023

tekton-robot commented May 4, 2023

tekton-robot commented May 5, 2023

Yongxuanzhang commented May 5, 2023

tekton-robot commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yongxuanzhang commented May 5, 2023

tekton-robot commented May 5, 2023

tekton-robot commented May 5, 2023

tekton-robot commented May 5, 2023

tekton-robot commented May 5, 2023

tekton-robot commented May 5, 2023

tekton-robot commented May 5, 2023

Choose a reason for hiding this comment

tekton-robot commented May 8, 2023

tekton-robot commented May 8, 2023

tekton-robot commented May 8, 2023

lbernick left a comment

Choose a reason for hiding this comment

tekton-robot commented May 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yongxuanzhang May 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yongxuanzhang May 9, 2023 • edited Loading

Choose a reason for hiding this comment

Yongxuanzhang commented May 8, 2023

lbernick commented May 8, 2023

chuangw6 commented May 9, 2023

chuangw6 commented May 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuangw6 May 9, 2023 • edited Loading

Choose a reason for hiding this comment

tekton-robot commented May 9, 2023

tekton-robot commented May 9, 2023

chuangw6 commented May 10, 2023

chuangw6 commented May 10, 2023

Yongxuanzhang commented May 4, 2023 •

edited by lbernick

Loading

Yongxuanzhang May 9, 2023 •

edited

Loading

Yongxuanzhang May 9, 2023 •

edited

Loading

chuangw6 May 9, 2023 •

edited

Loading