[TEP-0091] Update trusted resources feature flag and add condition #949

Yongxuanzhang · 2023-01-31T19:24:59Z

This commit proposes to

add mode field intoVerificationPolicy, update the feature flag to verification-no-match-policy with allow / warn / deny
add condition into taskrun and pipelinerun to indicate the verification passes or not.

Yongxuanzhang · 2023-01-31T19:27:04Z

Hi @afrittoli @wlynch @bobcatfish @jagathprakash
This is a proposal to update the trusted resources tep, plz take a look and share your comments.
Thanks!

jerop · 2023-02-07T15:32:49Z

/kind tep

teps/0091-trusted-resources.md

pritidesai · 2023-02-13T17:24:05Z

API WG - waiting for the review

/assign @jagathprakash
/assign @afrittoli

tekton-robot · 2023-02-13T17:24:08Z

@pritidesai: 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:

API WG - waiting for the review

/assign @jagathprakash
/assign @afrittoli

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.

jagathprakash · 2023-02-13T18:51:37Z

/lgtm

tekton-robot · 2023-02-13T18:51:39Z

@jagathprakash: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

wlynch · 2023-02-13T19:03:15Z

teps/0091-trusted-resources.md

- * `skip`: Directly skip the verification.
+`resource-verification-mode`. (Optional, `verify_all`, `verify_match` or `no_verification`, default `no_verification`):
+ * `verify_all`: Failing verification will mark the taskruns/pipelineruns as failed.
+ * `verify_match`: Fetch VerificationPolicy from the namespace, and if policies matched then verify, otherwise skip the verification. Details explained in [ Verify the Resources](#verify-the-resources)


I don't think this is a good idea to support globally, though I get why we'd want something like this for incremental adoption. It would be generally be bad if Tasks could circumvent verification just by renaming to something that's not covered by policy, and new Tasks that are probably riskier would be exempted by default since they're unlikely to match existing policies.

Instead of making verification opt-in, it would probably be safer to make the mode opt-out on a per-policy basis (see ClusterImagePolicySpec.mode for an example). This way things are directed to the more secure way by default, forcing users to either configure the matching policy or explicitly opt out the Tasks.

Thanks! That makes sense. By looking at the ClusterImagePolicySpec.mode. How about changing the verify_match to:

If found matched polices then verify and fail

If no matched policies then warn

Preferring warn over skip generally sounds good, but that doesn't address the larger problem of opt-in enforcement and the ability to evade enforcement by crafting a non-matching artifact. 😢

Having policies that opt-out matching patterns seems like a better global policy - this way new artifacts that we haven't encountered before (whether they are malicious or not) can't bypass enforcement.

wlynch · 2023-02-17T20:36:33Z

Followed up with @Yongxuanzhang off thread -

We're leaning towards modifying this slightly by:

adding a mode field to VerificationPolicy to control allow/warn behavior rather than having a global config map value.
Modifying resources-verification-mode to specify the action to take if no policy matches (we may rename this to make the behavior clearer).

If you want to have the same affect as what is proposed for verify-match, you can achieve this in one of 2 ways:

Set ConfigMap resources-verification-mode: enforce, then define a policy like:
```
apiVersion: tekton.dev/v1alpha1
kind: VerificationPolicy
metadata:
  name: foo
spec:
  resources:
    - pattern: ".*"
  mode: warn
```
Ideally this should be scoped to only a few sets of resources, and acts as an allowlist. Ideally for cases like "we want to enforce everything, but we know a set of resources isn't signed yet so we will allow those". Enforce mode policies would take precedence over warn.

Set ConfigMap resources-verification-mode: warn, then define a policy like:

apiVersion: tekton.dev/v1alpha1
kind: VerificationPolicy
metadata:
  name: foo
spec:
  resources:
    - pattern: "https://github.com/tektoncd/catalog.git"
  mode: enforce  # this would be the default so could also be omitted.

This would be closer to what was originally proposed for resources-verification-mode: verify_match - i.e. only enforce matching policies without an explicit opt-out policy.

Much of this was inspired by https://github.com/sigstore/policy-controller so, feel free to take a look there to read up on additional background.

We'll plan to discuss this more in next week's s3c working group, so please join if you have thoughts / opinions! 🙏

Yongxuanzhang · 2023-02-23T16:36:41Z

/assign @bobcatfish

Yongxuanzhang · 2023-02-23T16:36:50Z

/assign @jagathprakash

teps/0091-trusted-resources.md

jerop

Thanks for the addressing my feedback, the conditions are much clearer now 🎉

jerop · 2023-03-10T18:07:20Z

teps/0091-trusted-resources.md

+        status: "False"
+        type: TrustedResourcesVerified
+      - lastTransitionTime: "2023-03-01T18:17:10Z"
+        message: resource verification failed


Suggested change

message: resource verification failed

message: Trusted resource verification failed

teps/0091-trusted-resources.md

jerop · 2023-03-10T18:11:05Z

@afrittoli @bobcatfish @jagathprakash @wlynch please take another look 🙏🏾

jerop · 2023-03-10T18:12:50Z

once all assignees have approved, feel free to unhold (just don't want it to merge accidentally)

wlynch

/lgtm

bobcatfish

Thanks for all the updates @Yongxuanzhang ! There's one last nit from me about referring to the "knative API" but nothing blocking

/approve

bobcatfish · 2023-03-13T22:27:38Z

teps/0091-trusted-resources.md

+ * `warn`: Don't fail the taskrun/pipelinerun and log a warning if no matching policies are found.
+ * `fail`: Fail the taskrun/pipelinerun if no matching policies are found.
+
+**Note:** The current proposed `trusted-resources-verification-no-match-policy` will be added to replace the old `resource-verification-mode` in one release and this is not a backwards compatible change.


I think this is already on your radar, but please send a notification to tekton-dev@ and tekton-users@ to warn folks this is going to happen within one release, with some warning (at least a week I'm thinking?) before the release goes out 🙏

teps/0091-trusted-resources.md

tekton-robot · 2023-03-13T22:31:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, jerop

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:

~~teps/OWNERS~~ [bobcatfish,jerop]

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

This commit proposes to add mode field into VerificationPolicy, update the feature flag trusted-resources-verification-no-match-policy to control the behaviour of no matching policies for a resource. Besides this commit proposes to add condition into taskrun and pipelinerun to indicate the verification passes or not.

Yongxuanzhang · 2023-03-14T15:53:38Z

/hold cancel

wlynch

/lgtm

tekton-robot requested review from piyush-garg and pritidesai January 31, 2023 19:25

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 31, 2023

Yongxuanzhang force-pushed the more-updates-91 branch from 36d2736 to c741f0a Compare February 2, 2023 17:44

tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Feb 7, 2023

jagathprakash reviewed Feb 7, 2023

View reviewed changes

teps/0091-trusted-resources.md Outdated Show resolved Hide resolved

teps/0091-trusted-resources.md Outdated Show resolved Hide resolved

Yongxuanzhang force-pushed the more-updates-91 branch from c741f0a to 2ea7d57 Compare February 8, 2023 22:27

tekton-robot assigned afrittoli Feb 13, 2023

wlynch requested changes Feb 13, 2023

View reviewed changes

Yongxuanzhang force-pushed the more-updates-91 branch from 2ea7d57 to 0fd4c55 Compare February 23, 2023 04:39

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 23, 2023

Yongxuanzhang requested review from wlynch and jagathprakash and removed request for pritidesai, piyush-garg and wlynch February 23, 2023 04:42

Yongxuanzhang force-pushed the more-updates-91 branch 3 times, most recently from 782f142 to 828e730 Compare February 23, 2023 05:24

tekton-robot assigned bobcatfish Feb 23, 2023

This comment was marked as duplicate.

Sign in to view

Yongxuanzhang force-pushed the more-updates-91 branch 6 times, most recently from eab08cc to f883bbb Compare March 10, 2023 17:17

jerop reviewed Mar 10, 2023

View reviewed changes

teps/0091-trusted-resources.md Outdated Show resolved Hide resolved

Yongxuanzhang force-pushed the more-updates-91 branch from f883bbb to 759435b Compare March 10, 2023 17:38

jerop reviewed Mar 10, 2023

View reviewed changes

Yongxuanzhang force-pushed the more-updates-91 branch from 759435b to 882467c Compare March 10, 2023 18:05

jerop approved these changes Mar 10, 2023

View reviewed changes

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

Yongxuanzhang force-pushed the more-updates-91 branch from 882467c to 7c62627 Compare March 10, 2023 18:16

wlynch reviewed Mar 13, 2023

View reviewed changes

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

bobcatfish reviewed Mar 13, 2023

View reviewed changes

Yongxuanzhang force-pushed the more-updates-91 branch from 7c62627 to 96d45fe Compare March 14, 2023 15:53

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

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 14, 2023

Yongxuanzhang requested a review from wlynch March 14, 2023 18:12

wlynch reviewed Mar 14, 2023

View reviewed changes

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

tekton-robot merged commit dbbe9e8 into tektoncd:main Mar 14, 2023

Yongxuanzhang mentioned this pull request Mar 14, 2023

TEP-0091: Trusted Resources milestone 2 work before Beta tektoncd/pipeline#6356

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TEP-0091] Update trusted resources feature flag and add condition #949

[TEP-0091] Update trusted resources feature flag and add condition #949

Yongxuanzhang commented Jan 31, 2023 •

edited

Loading

Yongxuanzhang commented Jan 31, 2023

jerop commented Feb 7, 2023

pritidesai commented Feb 13, 2023

tekton-robot commented Feb 13, 2023

jagathprakash commented Feb 13, 2023

tekton-robot commented Feb 13, 2023

wlynch Feb 13, 2023

Yongxuanzhang Feb 15, 2023

wlynch Feb 15, 2023

wlynch commented Feb 17, 2023 •

edited

Loading

Yongxuanzhang commented Feb 23, 2023

Yongxuanzhang commented Feb 23, 2023

This comment was marked as duplicate.

jerop left a comment

jerop Mar 10, 2023

jerop commented Mar 10, 2023

jerop commented Mar 10, 2023

wlynch left a comment

bobcatfish left a comment

bobcatfish Mar 13, 2023

tekton-robot commented Mar 13, 2023

Yongxuanzhang commented Mar 14, 2023

wlynch left a comment

	message: resource verification failed
	message: Trusted resource verification failed

[TEP-0091] Update trusted resources feature flag and add condition #949

[TEP-0091] Update trusted resources feature flag and add condition #949

Conversation

Yongxuanzhang commented Jan 31, 2023 • edited Loading

Yongxuanzhang commented Jan 31, 2023

jerop commented Feb 7, 2023

pritidesai commented Feb 13, 2023

tekton-robot commented Feb 13, 2023

jagathprakash commented Feb 13, 2023

tekton-robot commented Feb 13, 2023

wlynch Feb 13, 2023

Choose a reason for hiding this comment

Yongxuanzhang Feb 15, 2023

Choose a reason for hiding this comment

wlynch Feb 15, 2023

Choose a reason for hiding this comment

wlynch commented Feb 17, 2023 • edited Loading

Yongxuanzhang commented Feb 23, 2023

Yongxuanzhang commented Feb 23, 2023

This comment was marked as duplicate.

jerop left a comment

Choose a reason for hiding this comment

jerop Mar 10, 2023

Choose a reason for hiding this comment

jerop commented Mar 10, 2023

jerop commented Mar 10, 2023

wlynch left a comment

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Mar 13, 2023

Choose a reason for hiding this comment

tekton-robot commented Mar 13, 2023

Yongxuanzhang commented Mar 14, 2023

wlynch left a comment

Choose a reason for hiding this comment

Yongxuanzhang commented Jan 31, 2023 •

edited

Loading

wlynch commented Feb 17, 2023 •

edited

Loading