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

Remove TaskRunResults from slsa/v2alpha1 #750

Closed
chitrangpatel opened this issue Mar 28, 2023 · 10 comments
Closed

Remove TaskRunResults from slsa/v2alpha1 #750

chitrangpatel opened this issue Mar 28, 2023 · 10 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten

Comments

@chitrangpatel
Copy link
Contributor

chitrangpatel commented Mar 28, 2023

PR #663 added taskRunResults to the buildConfig.

However, the buildConfig should only be static information from the build configuration, not the runtime info too and taskRunResults is definitely runtime information.
Having said that, I'm not sure where we would fit the taskRunResults in this case because there is no obvious place in the SLSAv0.2 predicate.

We could reach out to the SLSA folks and ask for guidance on this. But as is, I don't think taskRunResults belongs in the buildConfig should probably be removed.

Thoughts???

@chitrangpatel
Copy link
Contributor Author

@chitrangpatel
Copy link
Contributor Author

/kind bug

@tekton-robot tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 28, 2023
@lcarva
Copy link
Contributor

lcarva commented Mar 28, 2023

Runtime information, like TaskRun results, is super valuable when writing advanced policies. The new SLSA spec will have a place for this under byproducts, see #649. But I think that may not be sufficient. For instance, tracking the order the TaskRuns were executed might be challenging.

We may want to introduce a new predicate that includes pretty much the whole TaskRun/PipelineRun resource.

That way, Chains can generate a very clean SLSA Provenance attestation which makes most users happy. But also a Chains specific attestation for those of us who need additional information in their policies.

@wlynch
Copy link
Member

wlynch commented Mar 28, 2023

+1 to what @lcarva said. There's still value in storing the result values, though I'm fine if we move them out of buildConfig to somewhere else (byproducts, etc).

I like the idea of attaching the full spec as it's own separate attestation that's linked to from the main SLSA attestation. This is also a nice pattern to let us eventually attach additional attestations like Pod spec as well.

@chitrangpatel
Copy link
Contributor Author

The challenge is that in SLSA v0.2, there is no obvious place for this. In SLSA v1.0, we can certainly add it to byProducts. My feeling is that we should stick to the recommendations set out by SLSA for those implementations. Because if we are claiming that we are using the SLSA predicate and requirements then we should comply with their recommendations.

If we need more information then we can do what @lcarva suggested - i.e. implement a chains specific attestation (which provides more information on top of what SLSA is recommending)?

@wlynch
Copy link
Member

wlynch commented Mar 28, 2023

Since SLSA v1 sounds like it is imminent (last I heard ETA for final v1 spec was mid-late April), one option is we freeze support for SLSA v0.2 in our v1 spec, and focus on SLSA v1 for v2. This would sidestep the issue for v0.2.

If we have a reason to keep developing against SLSA v0.2 - we could find another home like extending invocation with our own fields that we need. I see enough value for these fields that I wouldn't want to just drop them without a replacement.

@tekton-robot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

@tekton-robot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

@tekton-robot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

@tekton-robot
Copy link

@tekton-robot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen with a justification.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/close

Send feedback to tektoncd/plumbing.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten
Projects
None yet
Development

No branches or pull requests

4 participants