[TEP-0091] Update TEP with design evaluations and more examples #830

Yongxuanzhang · 2022-09-22T22:20:36Z

This commit adds more details of design evaluation. Since we need to pull in sigstore to make this proposal work, this needs to be included in this TEP.
And also add more examples of keys, remove the configmap to store the keys since the crd covers all the needed features.

lbernick · 2022-09-26T13:41:34Z

/kind tep

dibyom · 2022-09-26T16:06:35Z

/assign @jerop

afrittoli · 2022-09-26T16:07:03Z

/assign @afrittoli

afrittoli · 2022-10-03T16:06:22Z

/test pull-community-teps-lint

tekton-robot · 2022-10-03T16:06:24Z

@afrittoli: No presubmit jobs available for tektoncd/community@main

In response to this:

/test pull-community-teps-lint

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.

teps/0091-trusted-resources.md

jerop

Thanks @Yongxuanzhang!

teps/0091-trusted-resources.md

bobcatfish

Thanks for adding the design evaluation details! I've got a question about the coupling we're introducing (apologies if you've already addressed this question elsewhere 🙏 - if so might be worth including a few more details about that for posterity in the TEP at least)

bobcatfish · 2022-10-24T21:19:36Z

teps/0091-trusted-resources.md

+
+Are there opinionated choices being made in this proposal? If so, are they necessary and can users extend it with
+
+* We choose to use Sigstore to create signer and verifier, users cannot extend/replace it with other options. But users can use whatever keys supported by Sigstore.


can you explain a bit more about why this required? Is there any Sigstore specific logic or is it just convenient to use the libraries? are there any alternatives?

Sure! The reasons in my mind are:

It is convenient to be used as it supports KMS from major cloud providers, and also has the logic to load

https://github.com/sigstore/sigstore/blob/32b357aca9bcd5f75a67667beb775fa13e2b66b4/pkg/signature/verifier.go#L42

https://github.com/sigstore/sigstore/tree/main/pkg/signature/kms

It (sigstore/cosign, sigstore/rekor) is also used in Kubernetes, Tekton to sign releases and maintained by Google, Chainguard etc.

One alternative would be we implement these functions on our own?
Other alternatives would be use libraries such as https://github.com/hashicorp/vault.

So it sounds like what we really want form sigstore is the cloud provider KMS interface 🤔 A couple thoughts/questions:

does sigstore implement these from scratch, or does it pull in libraries that do this? if it pulls in libraries, we could also pull in the same libraries instead of using sigstore

it sounds like we're not really so much trying to couple ourselves to sigstore as we are trying to use a library that is part of sigstore? if so, maybe we can be explicit that we are only pulling in that library, and NOT the rest of sigstore (e.g. fulcio, rekor, etc.) and that coupling Tekton to those would require further review

how much work would it be to implement our own?

They pulled in cloud providers' libraries. But I think it's not a simple wrapper of cloud kms. For reference: https://github.com/sigstore/sigstore/tree/main/pkg/signature/kms

Yes we're using github.com/sigstore/sigstore now and not using other libraries. In the TEP I have mentioned that we only pull sigstore/sigstore for now

It depends on how we want to implement them. Like if just copy paste the code then it shouldn't be of much work. (Not sure if this is a good practice) And if we want to design something similar like it then it would be much work.

Besides, in chains we have already pulled sigstore, cosign, and rekor.
https://github.com/tektoncd/chains/blob/b0c9c54930bce7d8288a3c68447fd4cac1b14176/go.mod#L41-L43

I think the long term goal is to support OCI bundle signing and Sigstore keyless + identity verification, but focusing on simple key signing as a first step?

re users cannot extend/replace it with other options: it's pretty easy to add additional ones - you just have to register them via AddProvider. You also need to explicitly import the providers you want, so you can choose which you want to pull in. The SignerVerifier interface is flexible, so it should also be easy to add more if the existing impls don't quite do what you want.

I'd say it's probably fine if you want to pull in sigstore/sigstore and/or cosign libraries since you'll probably need a good chunk of the OCI manipulation for bundles and transparency log validation eventually anyway.

Of interest - we just started some work on the Sigstore side to rewrite a Go client(background, repo). Happy to work together if there's something pipelines needs!

Of interest - we just started some work on the Sigstore side to rewrite a Go client(background, repo). Happy to work together if there's something pipelines needs!

Thanks @wlynch - I cannot access the google doc, do I need to join a specific group to have access?

Probably need to join https://groups.google.com/g/sigstore-dev 🙂

Yongxuanzhang · 2022-11-03T14:56:59Z

teps/0091-trusted-resources.md

-type ResourcesPattern struct {
-	// Pattern defines a resource pattern. Regex is created to filter resources based on `Pattern`
+type ResourceAuthority struct {
+	// Pattern defines a resource pattern. The pattern is used to filter out remote resources by their sources, by default is "*"


The pattern we will match against comes from the url of configsource of tektoncd/pipeline#5397

Yongxuanzhang · 2022-11-03T18:25:40Z

/assign @wlynch @jagathprakash

tekton-robot · 2022-11-03T18:25:42Z

@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.

jagathprakash · 2022-11-12T13:25:52Z

teps/0091-trusted-resources.md

-        url: "https://fulcio.sigstore.dev"
+  resourceAuthorities:
+      # pattern is used to filter out remote resources by their sources URL. e.g. if the resources from https://github.com/tektoncd/catalog.git, then the pattern should be set to https://github.com/tektoncd/catalog.git. ".*" will match any resources.
+    - pattern: ".*"


Is there an order in which the patterns are applied?
In the above case it would be better if https://github.com/tektoncd/catalog.git is first checked and then ".*".

Oh I think there is no such an order, if the pattern is empty we will set it as .*

jagathprakash · 2022-11-24T20:52:28Z

teps/0091-trusted-resources.md

+To learn more about regex syntax please refer to [syntax](https://pkg.go.dev/regexp/syntax).
+To learn more about `ConfigSource` please refer to resolvers doc for more context. e.g. [gitresolver](./git-resolver.md)
+
+`key` is used to store the public key, note that secretRef and data cannot be configured at the same time.


Maybe change to Only one among secretRet, data and kms.

jagathprakash · 2022-11-24T21:36:22Z

teps/0091-trusted-resources.md

+
+`pattern` is used to filter out remote resources by their sources URL. e.g. git resources pattern can be set to https://github.com/tektoncd/catalog.git. The `pattern` should follow regex schema, we use go regex library's [`Match`](https://pkg.go.dev/regexp#Match) to match the pattern from VerificationPolicy to the `ConfigSource` URL resolved by remote resolution. Note that `.*` will match all resources.
+To learn more about regex syntax please refer to [syntax](https://pkg.go.dev/regexp/syntax).
+To learn more about `ConfigSource` please refer to resolvers doc for more context. e.g. [gitresolver](./git-resolver.md)


git-resolver.md is in pipelines/docs/ .Please fix the link.

tekton-robot · 2022-11-24T21:37:30Z

@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.

bobcatfish

Looking great, thanks for the extra details! I think the update to multiple VerificationPolicy instances makes sense, and also making it so that all policies that match must pass. Once people actually start using this I could imagine us needing to make some tweaks but I this seems like a great starting place!

I recommend we get this update to the TEP in ASAP and then we can focus on the implementation PR and if any other changes need to be made we can open more PRs vs. having to keep this PR and the implementation PR going at the same time - but want to make sure the other approver (@wlynch) agrees with these changes (and @afrittoli and another other reviewers as well)

pritidesai · 2022-11-28T17:05:49Z

@afrittoli please take another look, thanks!

wlynch

LGTM! Minor tweaks, but nothing major.

teps/0091-trusted-resources.md

wlynch · 2022-11-28T17:08:34Z

teps/0091-trusted-resources.md

+surprising? Are there security implications for these implicit behaviors?
+-->
+
+This proposal doesn't have any effect if users don't enable it. If enabled, users need to sign the resources and config the public key in cluster to bypass verification, the mutating webhook will be skipped for API resources to avoid the failing verification.


My guess is this will probably be wanted sooner rather than later, since it's much easier for maintenance to have both paths have the same logic rather than have to have different assumptions depending on whether the mutating webhook ran or not.

To support verification you're already going to make sure similar defaulting behavior happens in the reconciler anyway, and there shouldn't be a harm in doing this post-mutating webhook for non-verified objects since they should produce the same result. If defaulting is happening at reconcile for all requests and we're allowing certain requests (i.e. verified resources) to skip the mutating webhook, then the mutating webhook isn't adding value and can just be removed since even non-verified resources should be able to safely skip the webhook as well.

This doesn't change this design, just more-so an FYI to other projects that this will be happening since this will change the stored API object for created objects which may be considered a backwards incompatible change - though I suspect most other tools are agnostic to the defaulting that the webhook is doing and won't even notice it's gone. If we need to follow the deprecation policy strictly here I'd rather do it sooner than later, since the main reason I can see keeping the webhook around is for backwards-compatibility (and may eventually be a blocker for removing the feature flag).

afrittoli · 2022-11-29T15:42:44Z

teps/0091-trusted-resources.md

 ```

+`namespace` should be the same of corresponding resources' namespace.
+
+`pattern` is used to filter out remote resources by their sources URL. e.g. git resources pattern can be set to https://github.com/tektoncd/catalog.git. The `pattern` should follow regex schema, we use go regex library's [`Match`](https://pkg.go.dev/regexp#Match) to match the pattern from VerificationPolicy to the `ConfigSource` URL resolved by remote resolution. Note that `.*` will match all resources.


I don't see pattern in the example above.
Is filtering possible only for remote resources? What about resources in the cluster?

Oh thanks! I just updated the example as well! It is indeed missing by mistake. Thanks!

resources in the cluster are also supported by using clusterresolver.
https://github.com/tektoncd/pipeline/blob/main/docs/cluster-resolver.md#resolutionrequest-status
We can use the configsource to filter

OK, but in Tekton we support a direct reference to a resource in the cluster, which is probably what most Tekton users use today since remote resources have only been introduced recently.

Does this mean that verified resources requires will only work for remote resources (of any kind) but not for the plain taskRef and pipelineRef that points to resources in cluster?

cluster resources verification are also supported if using the local resolver, the only missing part is that is doesn't has source information, so we cannot use a pattern to filter it. In that case users can create one policy to match all cluster resources.

afrittoli

Thanks for this @Yongxuanzhang.

The only question I have is support for non-remote resources.

I think we shall keep a close eye on the work done in sigstore land that @wlynch pointed to - thank you for that link!

Something that I missed in here is a way for a user to verify a resource and test the new Policy CRD without trying to execute the resource with a *Run, so something like a tkn verify that can use a VerificationPolicy. The implementation would not be in scope for this TEP, but I think we should mention that we'll need some way to help users verify their own VerificationPolicies.

Nothing really blocking this PR in any case.

/approve

afrittoli · 2022-12-01T09:46:02Z

teps/0091-trusted-resources.md

 ```

+`namespace` should be the same of corresponding resources' namespace.
+
+`pattern` is used to filter out remote resources by their sources URL. e.g. git resources pattern can be set to https://github.com/tektoncd/catalog.git. The `pattern` should follow regex schema, we use go regex library's [`Match`](https://pkg.go.dev/regexp#Match) to match the pattern from VerificationPolicy to the `ConfigSource` URL resolved by remote resolution. Note that `.*` will match all resources.


OK, but in Tekton we support a direct reference to a resource in the cluster, which is probably what most Tekton users use today since remote resources have only been introduced recently.

Does this mean that verified resources requires will only work for remote resources (of any kind) but not for the plain taskRef and pipelineRef that points to resources in cluster?

afrittoli · 2022-12-01T09:48:24Z

teps/0091-trusted-resources.md

+
+`pattern` is used to filter out remote resources by their sources URL. e.g. git resources pattern can be set to https://github.com/tektoncd/catalog.git. The `pattern` should follow regex schema, we use go regex library's [`Match`](https://pkg.go.dev/regexp#Match) to match the pattern from VerificationPolicy to the `ConfigSource` URL resolved by remote resolution. Note that `.*` will match all resources.
+To learn more about regex syntax please refer to [syntax](https://pkg.go.dev/regexp/syntax).
+To learn more about `ConfigSource` please refer to resolvers doc for more context. e.g. [gitresolver](https://github.com/tektoncd/pipeline/blob/main/docs/git-resolver.md#resolutionrequest-status)


The link provided does not mention ConfigSource at all - btw, the SPDX links in there are broken 🙏

I have talked to @chuangw6 and we don't have a ConfigSource in our repo, so I linked to a resolver using ConfigSource. I will clarify this in the tep

I think it might be worth briefly mentioning what is configsource first, and then where this field came from (i.e. ResolutionRequest status) and what subfield we use.

tektoncd/chains#521 has full details about how Chains captures this in provenance, but step 1-3 are pipeline-related work.

I think https://github.com/tektoncd/pipeline/blob/main/docs/pipeline-api.md#configsource-1 might be a good link

tekton-robot · 2022-12-01T10:23:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, bobcatfish, wlynch

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~~ [afrittoli,bobcatfish,wlynch]

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

This commit adds more details of design evaluation. Since we need to pull in sigstore to make this proposal work, this needs to be included in this TEP.

bobcatfish · 2022-12-01T16:41:32Z

Looks like all assigned reviewers have approved, thanks everyone!

/lgtm

tekton-robot requested review from AlanGreene and mnuttall September 22, 2022 22:20

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 22, 2022

tekton-robot added the kind/tep Categorizes issue or PR as related to a TEP (or needs a TEP). label Sep 26, 2022

tekton-robot assigned jerop Sep 26, 2022

afrittoli changed the title ~~[TEP-0091]Update TEP with design evaluaitons~~ [TEP-0091]Update TEP with design evaluations Sep 26, 2022

tekton-robot assigned afrittoli Sep 26, 2022

jerop reviewed Oct 17, 2022

View reviewed changes

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

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

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

Yongxuanzhang force-pushed the tep-91-update branch from b63db69 to 75059dd Compare October 18, 2022 18:26

Yongxuanzhang requested review from jerop and removed request for AlanGreene and mnuttall October 18, 2022 18:27

jerop approved these changes Oct 18, 2022

View reviewed changes

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

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

Yongxuanzhang force-pushed the tep-91-update branch from db53363 to 5cf21ee Compare October 18, 2022 19:49

bobcatfish reviewed Oct 24, 2022

View reviewed changes

bobcatfish mentioned this pull request Oct 24, 2022

[TEP-0091] Add Verification at reconciler tektoncd/pipeline#5581

Merged

7 tasks

Yongxuanzhang mentioned this pull request Nov 3, 2022

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

Merged

7 tasks

Yongxuanzhang changed the title ~~[TEP-0091]Update TEP with design evaluations~~ [TEP-0091] Update TEP with design evaluations Nov 3, 2022

Yongxuanzhang changed the title ~~[TEP-0091] Update TEP with design evaluations~~ [TEP-0091] Update TEP with design evaluations and api changes Nov 3, 2022

Yongxuanzhang force-pushed the tep-91-update branch from 6cc142d to b35d135 Compare November 3, 2022 14:55

Yongxuanzhang commented Nov 3, 2022

View reviewed changes

tekton-robot assigned wlynch Nov 3, 2022

jagathprakash reviewed Nov 12, 2022

View reviewed changes

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

Yongxuanzhang changed the title ~~[TEP-0091] Update TEP with design evaluations and api changes~~ [TEP-0091] Update TEP with design evaluations and more examples Nov 15, 2022

Yongxuanzhang force-pushed the tep-91-update branch from 6d0c1d1 to 19de2f9 Compare November 15, 2022 22:26

Yongxuanzhang force-pushed the tep-91-update branch from 19de2f9 to e2d2757 Compare November 22, 2022 22:47

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 Nov 22, 2022

Yongxuanzhang force-pushed the tep-91-update branch from e2d2757 to e54b912 Compare November 24, 2022 20:11

jagathprakash reviewed Nov 24, 2022

View reviewed changes

Yongxuanzhang force-pushed the tep-91-update branch from e54b912 to faa452e Compare November 24, 2022 23:19

bobcatfish reviewed Nov 25, 2022

View reviewed changes

Yongxuanzhang force-pushed the tep-91-update branch from faa452e to c4cfd22 Compare November 28, 2022 17:15

wlynch reviewed Nov 28, 2022

View reviewed changes

Yongxuanzhang force-pushed the tep-91-update branch 2 times, most recently from 13df7a7 to fbb3f96 Compare November 28, 2022 19:12

wlynch approved these changes Nov 28, 2022

View reviewed changes

afrittoli reviewed Nov 29, 2022

View reviewed changes

Yongxuanzhang force-pushed the tep-91-update branch from fbb3f96 to 0b9844a Compare November 29, 2022 15:56

Yongxuanzhang requested a review from afrittoli November 29, 2022 15:58

afrittoli reviewed Dec 1, 2022

View reviewed changes

[TEP-0091]Update TEP with design evaluaitons and more examples

b061245

This commit adds more details of design evaluation. Since we need to pull in sigstore to make this proposal work, this needs to be included in this TEP.

Yongxuanzhang force-pushed the tep-91-update branch from 0b9844a to b061245 Compare December 1, 2022 15:37

tekton-robot assigned bobcatfish Dec 1, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2022

tekton-robot merged commit 7f988ae into tektoncd:main Dec 1, 2022


		Are there opinionated choices being made in this proposal? If so, are they necessary and can users extend it with

		* We choose to use Sigstore to create signer and verifier, users cannot extend/replace it with other options. But users can use whatever keys supported by Sigstore.

[TEP-0091] Update TEP with design evaluations and more examples #830

[TEP-0091] Update TEP with design evaluations and more examples #830

Conversation

Yongxuanzhang commented Sep 22, 2022 • edited Loading

lbernick commented Sep 26, 2022

dibyom commented Sep 26, 2022

afrittoli commented Sep 26, 2022

afrittoli commented Oct 3, 2022

tekton-robot commented Oct 3, 2022

jerop left a comment

Choose a reason for hiding this comment

bobcatfish left a comment

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 Nov 3, 2022 • edited Loading

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

Choose a reason for hiding this comment

Yongxuanzhang commented Nov 3, 2022

tekton-robot commented Nov 3, 2022

Choose a reason for hiding this comment

Yongxuanzhang Nov 14, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Nov 24, 2022

bobcatfish left a comment

Choose a reason for hiding this comment

pritidesai commented Nov 28, 2022

wlynch left a comment

Choose a reason for hiding this comment

wlynch Nov 28, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afrittoli left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Dec 1, 2022

bobcatfish commented Dec 1, 2022

Yongxuanzhang commented Sep 22, 2022 •

edited

Loading

Yongxuanzhang Nov 3, 2022 •

edited

Loading

Yongxuanzhang Nov 14, 2022 •

edited

Loading

wlynch Nov 28, 2022 •

edited

Loading