TEP-84: PipelineRun Attestations #436
Conversation
|
Hi @lcarva. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
Just wanted to bring another TEP 109 that's related to this. There are 2 things in scope related to TEP 84: 1. type hinting extended to structured results and params (this should also be considered for PipelineRun) 2. I'm trying to categorize singable targets into inputs / outputs in that TEP in the scope of TaskRun. (if this is making sense in TaskRun, then it probably also makes sense in PipelineRun) |
pkg/chains/objects/objects.go
Outdated
| // Represents a generic K8s object | ||
| // This isn't meant to be a final implementation, just one approach | ||
| // Many of these methods can be abstracted away further | ||
| type K8sObject interface { |
There was a problem hiding this comment.
You might be interested in some prior art in https://github.com/tektoncd/results/tree/main/pkg/watcher/reconciler - which watched both TaskRuns and PipelineRuns to update annotations for Results :)
The premise is roughly the same, but the other way makes heavy use of unstructured.Unstructured + defining the client to use when creating the reconciler.
There was a problem hiding this comment.
Okay i see you already have an implementation of a generic object there, i think we'll just re-use that implementation - what do you think?
pkg/chains/objects/objects.go
Outdated
| } | ||
|
|
||
| func (tro *TaskRunObject) GetLatestAnnotation(annotation string) *Annotation { | ||
| tr, err := tro.clientSet.TektonV1beta1().TaskRuns(tro.tr.Namespace).Get(tro.ctx, tro.tr.Name, v1.GetOptions{}) |
There was a problem hiding this comment.
I'd lean towards relying on Listers where possible (unless we absolutely need the freshest value here)
There was a problem hiding this comment.
Yup we were keeping that in mind. This call happens because we wanted to keep the original behavior, as of now the some storage options will make calls to the master API. Here's an example.
| }, | ||
| Predicate: slsa.ProvenancePredicate{ | ||
| Builder: slsa.ProvenanceBuilder{ | ||
| ID: i.builderID, |
There was a problem hiding this comment.
Hmmm so this is interesting, because IIUC the builder id is supposed to be an identifier for "the thing that ran the build" - but this is actually ambiguous because a Pipeline by itself doesn't actually run anything - it farms things out to Tasks to execute. 🤔
Maybe this should be some reference to the controller that orchestrated the run, but that's also ambiguous if multiple controllers happen to pick up the same pipeline (i.e. retries, etc.). 🤔 🤔 🤔
Don't need to solve this in this PR - but if you have any thoughts I'd love to discuss more.
| }, | ||
| BuildType: tektonID, | ||
| Invocation: invocation, | ||
| // BuildConfig: buildConfig(tr), |
There was a problem hiding this comment.
This should probably be the run payload?
There was a problem hiding this comment.
We currently are creating an attestation that serves our needs while keeping in line with the provenance predicate. We want to keep in line with the schema, so i would like to discuss what we have so far in another thread. Perhaps the chains slack?
| } | ||
|
|
||
| // generateAttestationFromTaskRun translates a Tekton TaskRun into an in-toto attestation | ||
| // with the slsa-provenance predicate type | ||
| func (i *InTotoIte6) generateAttestationFromTaskRun(tr *v1beta1.TaskRun) (interface{}, error) { | ||
| subjects := GetSubjectDigests(tr, i.logger) | ||
| // We only want access to the original object, no need to pass in a client/context | ||
| tro := objects.NewTaskRunObject(tr, nil, nil) |
There was a problem hiding this comment.
The fact that we're passing nil, nil here and a bunch of other places makes me think we may want to re think the object abstraction. 😅
There was a problem hiding this comment.
Yes that's right, I made a refactor that removes those calls but after finding the abstraction that already exists we'll most likely move to that anyway
| @@ -79,7 +80,8 @@ func TestBackend_StorePayload(t *testing.T) { | |||
|
|
|||
| // Store the document. | |||
| opts := config.StorageOpts{Key: tt.args.key} | |||
| if err := b.StorePayload(ctx, tt.args.tr, sb, tt.args.signature, opts); (err != nil) != tt.wantErr { | |||
| trObj := objects.NewTaskRunObject(tt.args.tr, nil, ctx) | |||
There was a problem hiding this comment.
We'd want these to play nice with pipeline runs as well in the future.
There was a problem hiding this comment.
Can you expand on this? Do you mean the other storage options?
There was a problem hiding this comment.
Yup! Just a note that we'll want to support the same storage options we do for TaskRuns for PipelineRuns as well.
Totally okay with starting with a 1-2 backends and adding more as we go though! 👍
|
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
pkg/chains/signing.go
Outdated
|
|
||
| // Signs TaskRun and PipelineRun objects, as well as generates attesations for each | ||
| // Follows process of extract payload, sign payload, store payload and signature | ||
| func (ts *ObjectSigner) Sign(ctx context.Context, tektonObj objects.TektonObject) error { |
There was a problem hiding this comment.
nit: can we change the variable name ts to something more meaningful? I think it was short for taskrunSigner
There was a problem hiding this comment.
This is always debated 😆 https://github.com/golang/go/wiki/CodeReviewComments#receiver-names has some general advice, which suggests this is fine (though maybe use os / o / s to better match the type)
| for _, p := range pro.Spec.Params { | ||
| if p.Name == util.CommitParam { | ||
| commit = p.Value.StringVal | ||
| continue | ||
| } | ||
| if p.Name == util.UrlParam { | ||
| url = p.Value.StringVal | ||
| } | ||
| } | ||
|
|
||
| // search status.PipelineSpec.params | ||
| if pro.Status.PipelineSpec != nil { | ||
| for _, p := range pro.Status.PipelineSpec.Params { | ||
| if p.Default == nil { | ||
| continue | ||
| } | ||
| if p.Name == util.CommitParam { | ||
| commit = p.Default.StringVal | ||
| continue | ||
| } | ||
| if p.Name == util.UrlParam { | ||
| url = p.Default.StringVal | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I am a bit skeptical about the ordering here. Shouldn't we retrieve the values from PipelineSpec.Params[x].Default first and then retrieve from PipelineRun.Spec.Params[x].Value so that we are assured that we always get the latest value from pipelinerun level?
This problem exists for the materials in the taskrun level - task spec level default param value will overwrite the param value provided from run level.
wdyt?
There was a problem hiding this comment.
Yes, if a run value was provided it should choose that over the default value. Have you been able to reproduce that behavior? If so it should be fixed.
Let me know if you want to submit a PR to resolve these, otherwise I'll create a work item to to track them.
There was a problem hiding this comment.
It should be fixed for TaskRun attestations in the main branch: #449
We just need to apply the same fix for PipelineRun attestations as well (and ensure this PR doesn't revert the fix on TaskRuns)
pkg/chains/signing.go
Outdated
|
|
||
| // Signs TaskRun and PipelineRun objects, as well as generates attesations for each | ||
| // Follows process of extract payload, sign payload, store payload and signature | ||
| func (ts *ObjectSigner) Sign(ctx context.Context, tektonObj objects.TektonObject) error { |
There was a problem hiding this comment.
This is always debated 😆 https://github.com/golang/go/wiki/CodeReviewComments#receiver-names has some general advice, which suggests this is fine (though maybe use os / o / s to better match the type)
pkg/chains/storage/tekton/tekton.go
Outdated
| // Retrieve the TaskRun. | ||
| b.logger.Infof("Retrieving annotation %q on TaskRun %s/%s", annotationKey, tr.Namespace, tr.Name) | ||
| tr, err := b.pipelienclientset.TektonV1beta1().TaskRuns(tr.Namespace).Get(ctx, tr.Name, v1.GetOptions{}) |
There was a problem hiding this comment.
@chuangw6 Do you remember why we're getting the latest annotations here?
I'm wondering why the annotations of the state we're reconciling isn't sufficient 🤔
There was a problem hiding this comment.
It looks like retrieveAnnotationValue is only ever called by RetrieveSignatures and RetrievePayloads below. The first is not called anywhere within Chains. The second is called from VerifyTaskRun in pkg/chains/verifier.go. VerifyTaskRun is not called from anywhere.
This is either meant to be used by something outside of Chains, or it is dead code. I wonder if it was added to assist in the cli? https://github.com/tektoncd/cli/blob/main/pkg/chain/chain.go (In which case, it makes perfect sense that it fetches the TaskRun).
There was a problem hiding this comment.
It looks like the RetrieveSignatures and RetrievePayloads are also used as helper functions for testing the Tekton backend. Maybe they were placed here in case they had value in the future? As of now they would probably fit better in a testing utilities directory.
There was a problem hiding this comment.
There was a problem hiding this comment.
@chuangw6 Do you remember why we're getting the latest annotations here?
I'm wondering why the annotations of the state we're reconciling isn't sufficient 🤔
I think it's only for retrieving the payload from taskrun's annotation.
@wlynch @chuangw6 Is keeping VerifyTaskRun required if it isn't being used anywhere? Is it being imported by other projects? It would make the most sense to clean this up and separate RetrieveSignatures and RetrievePayloads into the internal testing directory.
It's not required nor used by other projects AFAIK.
There was a problem hiding this comment.
Actually what's interesting is RetrieveSignatures and RetrievePayloads has been implemented by every backend but none are used anywhere except tests
There was a problem hiding this comment.
Talked to @wlynch a while ago about the usage of these retrieval functions. There might be potential usage in future when people want to add CLI capability to retrieve & verify attestations. But other than that, I agree they are just used in tests for now.
There was a problem hiding this comment.
okay i left them in for now. Removing such a large amount of code that may have use in future seems out of scope for this PR
| limitations under the License. | ||
| */ | ||
|
|
||
| package util |
There was a problem hiding this comment.
Let's avoid the util name if possible, unless these are all just test helper funcs - https://go.dev/blog/package-names
If they are test helper funcs, put them into pkg/internal/util so it's not exposed as part of the library.
There was a problem hiding this comment.
Also for things like *RunFromFile that I'm pretty sure are only used in tests, you can enforce they are test only by having clients pass in a t.Testing, which also lets you t.Fatal if it fails and consolidates an error check that needs to be done in each test.
There was a problem hiding this comment.
Added them to the internal package. Although I left out the t.Testing parameter since this are used in the init functions to load the files once for tests.
| TektonID = "https://tekton.dev/attestations/chains@v2" | ||
| TektonPipelineRunID = "https://tekton.dev/attestations/chains/pipelinerun@v2" | ||
| CommitParam = "CHAINS-GIT_COMMIT" | ||
| UrlParam = "CHAINS-GIT_URL" |
There was a problem hiding this comment.
| UrlParam = "CHAINS-GIT_URL" | |
| URLParam = "CHAINS-GIT_URL" |
https://github.com/golang/go/wiki/CodeReviewComments#initialisms
| // supports the SPDX format which is recommended by in-toto | ||
| // ref: https://spdx.dev/spdx-specification-21-web-version/#h.49x2ik5 | ||
| // ref: https://github.com/in-toto/attestation/blob/849867bee97e33678f61cc6bd5da293097f84c25/spec/field_types.md | ||
| func SpdxGit(url, revision string) string { |
There was a problem hiding this comment.
| func SpdxGit(url, revision string) string { | |
| func SPDXGit(url, revision string) string { |
| @@ -60,7 +62,9 @@ func NewStorageBackend(ctx context.Context, logger *zap.SugaredLogger, cfg confi | |||
| } | |||
|
|
|||
| // StorePayload implements the storage.Backend interface. | |||
| func (b *Backend) StorePayload(ctx context.Context, tr *v1beta1.TaskRun, rawPayload []byte, signature string, opts config.StorageOpts) error { | |||
| func (b *Backend) StorePayload(ctx context.Context, _ versioned.Interface, obj objects.TektonObject, rawPayload []byte, signature string, opts config.StorageOpts) error { | |||
| // TODO: Handle unsupported type gracefully | |||
There was a problem hiding this comment.
Is this a "not yet supported" unsupported, or a "won't ever support" unsupported? (if the first option, can we include a tracking issue so people can follow?)
There was a problem hiding this comment.
It's not yet supported, although there's no reason this shouldn't work. We just haven't tested this ourselves. We can add an issue to track testing the backend to make sure it works as intended. Where's the best place to create that ticket?
| psSigner.Formatters = chains.AllFormatters(cfg, logger) | ||
|
|
||
| // get all backends for storing provenance | ||
| backends, err := storage.InitializeBackends(ctx, pipelineClient, kubeClient, logger, cfg) |
There was a problem hiding this comment.
If you want to restrict which backends are available for which type, this is a good place to do it.
You're loading all backends for pipelineruns here, even though you call out several of them as TaskRun only above.
There was a problem hiding this comment.
Would here be a better place? This way the deployment fails right away due to misconfiguration.
There was a problem hiding this comment.
Ended up doing this during the initial configuration check.
|
|
||
| // Check to see if it has already been signed. | ||
| if signing.Reconciled(pro) { | ||
| logging.FromContext(ctx).Infof("pipelinerun %s/%s has been reconciled", pr.Namespace, pr.Name) |
There was a problem hiding this comment.
optional: Instead of making sure "pipelinerun %s/%s" is in every log statement, you could do something like this in ReconcileKind:
log := logging.FromContext(ctx).With(
"pipelinerun", fmt.Sprintf("%s/%s", pr.Namespace, pr.Name),
...,
)
return r.FinalizeKind(logging.WithLogger(log), pr)Which will label all logging statements with the k/v pair within the context.
| } | ||
| } | ||
|
|
||
| type mockSigner struct { |
There was a problem hiding this comment.
Should we share this in an internal package with the taskrun reconciler?
pkg/test/clients.go
Outdated
| limitations under the License. | ||
| */ | ||
|
|
||
| package test |
There was a problem hiding this comment.
put in pkg/internal.
Perhaps pkg/internal/tekton? Then we could have tekton.CreateObject, tekton.GetTaskRun, ...
| type TaskAttestation struct { | ||
| Name string `json:"name,omitempty"` | ||
| After []string `json:"after,omitempty"` | ||
| Ref v1beta1.TaskRef `json:"ref,omitempty"` | ||
| StartedOn time.Time `json:"startedOn,omitempty"` | ||
| FinishedOn time.Time `json:"finishedOn,omitempty"` | ||
| Status string `json:"status,omitempty"` | ||
| Steps []attest.StepAttestation `json:"steps,omitempty"` | ||
| Invocation slsa.ProvenanceInvocation `json:"invocation,omitempty"` | ||
| Results []v1beta1.TaskRunResult `json:"results,omitempty"` | ||
| } |
There was a problem hiding this comment.
Maybe out of scope of this pr.
I think we could consolidate Ref and Invocation once #554 and related work in pipeline repo are done. Specifically, we can remove the Ref field and just record remote refs in TaskAttestation.Invocation.configSource. Happy to take on this part after this PR is merged if we agree on this.
There was a problem hiding this comment.
Yeah I agree, that would be great
|
/ok-to-test |
|
The following is the coverage report on the affected files.
|
There was a problem hiding this comment.
Sorry about the delay! (was at a conference last week).
Generally LGTM! Some minor comments that I think would be good follow ups, but nothing blocking.
It does look like there was a bunch of commits from the HABCS fork that made it into the commit history via a merge - I'd recommend squashing on your PR branch first just to clean things up.
Otherwise looks great and thank you for seeing this through! 🙏
| type TektonObject interface { | ||
| Object | ||
| GetKind() string | ||
| GetObject() interface{} | ||
| GetLatestAnnotations(ctx context.Context, clientSet versioned.Interface) (map[string]string, error) | ||
| Patch(ctx context.Context, clientSet versioned.Interface, patchBytes []byte) error | ||
| GetResults() []Result | ||
| GetServiceAccountName() string | ||
| } |
There was a problem hiding this comment.
Yeah fine to start here. Just wanted to call it out as it being generally useful 👀
| func (oa *OCIArtifact) ExtractObjects(obj objects.TektonObject) []interface{} { | ||
| objs := []interface{}{} | ||
|
|
||
| // TODO: Not applicable to PipelineRuns, should look into a better way to separate this out |
There was a problem hiding this comment.
Not blocking for this PR (because I do want to get this in), but something we could do is add additional methods to the TektonObject interface -
e.g.
func GetOCI() []name.Digest{}then PipelineRuns and TaskRuns could implement these separately and this just becomes a thin wrapper around the impl methods.
There was a problem hiding this comment.
Yeah that would be much cleaner.
|
Is squash commits set up for this repository? Github should be able to squash the commits itself. That may be easier since the upstream and POC commits have been interweaved on the poc branch. |
Squash commits are set up via prow, which unfortunately means you don't have the same control over the merge message as you would with the native GitHub merge message UI. Prow is just going to concat all 48 commit messages together into one uber message. 😭 Normally I don't think it's a big deal for a handful of commits but this is quite a lot so I think we should take the extra step to make sure are cleaned up, especially since some of the merge commits appear to reference unrelated changes like adding additional GitHub Actions workflows. 🙏 |
|
Ahhh okay understood, let me get something together |
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
Implementation of TEP-84, adding support to sign and attest PipelineRun artifacts. Signed-off-by: Brandon Caton <bcaton@redhat.com> Co-authored-by: Luiz Carvalho <lucarval@redhat.com>
|
The following is the coverage report on the affected files.
|
|
Updated with squash commit. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
Related to tektoncd#436 (comment). Prior, pipeline level provenance had a `ref` field in the `predicate.buildConfig` section. In this commit, we remove this `ref` field from buildconfig in favor of `buildConfig.tasks[i].invocation.configSource` tektoncd#554 because configsource should be enough to record the source information that we need to track the origin of the remote definitions. Since predicate.buildConfig is pretty much a free text, removing this field shouldn't be backward incompatible. Signed-off-by: Chuang Wang <chuangw@google.com>
Related to tektoncd#436 (comment). Prior, pipeline level provenance had a `ref` field in the `predicate.buildConfig` section. In this commit, we remove this `ref` field from buildconfig in favor of `buildConfig.tasks[i].invocation.configSource` tektoncd#554 because configsource should be enough to record the source information that we need to track the origin of the remote definitions. Since `predicate.buildConfig` is pretty much free text, this deletion shouldn't introduce backward incompatible changes. Signed-off-by: Chuang Wang <chuangw@google.com>
Related to tektoncd#436 (comment). Prior, pipeline level provenance had a `ref` field in the `predicate.buildConfig.tasks[i]` section. In this commit, we remove this `ref` field from buildConfig in favor of `buildConfig.tasks[i].invocation.configSource` tektoncd#554 because configsource should be enough to record the source information that we need to track the origin of the remote definitions. Since `predicate.buildConfig` is pretty much free text, this deletion shouldn't introduce backward incompatible changes. Signed-off-by: Chuang Wang <chuangw@google.com>
TEP-84 calls for end-to-end provenance collection.
In this POC,Chains is modified to attest PipelineRun resources in addition to TaskRun resources. Three new options are added to the configuration:artifacts.pipelinerun.format: The format of the attestation.tektonis the default value and, similarly toartifacts.taskrun.format, it's simply the json representation of the resource itself.in-totois also partially supported. Further work is needed to convert a PipelineRun resource into https://slsa.dev/provenance/v0.2. (Suggestions please!)in-totois also fully supported.artifacts.pipelinerun.storage: The place where the attestation will be stored.tekton, the default value, causes attestation to be attached to an annotation on the PipelineRun resource, also similarly toartifacts.taskrun.storage.ociis also supported causing the attestation to be pushed to the corresponding OCI registry.artifacts.pipelinerun.signer. Just likeartifacts.taskrun.signerType hinting is used for determining the attestation subjects. For PipelineRun attestations, chains relies on the PipelineRun results.
This is a POC meant to gather feedback from the community before proceeding further.This PR is now ready for a full review.