Introduce Resolved dependencies for SLSA v1.0 predicate by chitrangpatel · Pull Request #798 · tektoncd/chains

chitrangpatel · 2023-05-05T20:27:12Z

Changes

This PR introduces resolved dependencies for the SLSAv1.0 predicate.
It addresses part of issue #797
Related design.

Submitter Checklist

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

Has Docs included if any changes are user facing
Has Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been updated with any user facing changes (API changes, bug fixes, changes requiring upgrade notices or deprecation warnings)
Release notes contains the string "action required" if the change requires additional action from users switching to the new release

Release Notes

Introduce resolved dependencies for SLSA v1.0 predicate.

/kind feature

tekton-robot · 2023-05-05T20:32:36Z

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

File	Old Coverage	New Coverage	Delta
pkg/artifacts/signable.go	74.1%	69.1%	-5.0
pkg/chains/formats/slsa/internal/material/material.go	84.1%	84.8%	0.7
pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go	Do not exist	77.1%

tekton-robot · 2023-05-08T18:04:30Z

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

File	Old Coverage	New Coverage	Delta
pkg/artifacts/signable.go	74.1%	69.1%	-5.0
pkg/chains/formats/slsa/internal/material/material.go	84.1%	84.8%	0.7
pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go	Do not exist	81.1%

tekton-robot · 2023-05-08T18:16:55Z

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

File	Old Coverage	New Coverage	Delta
pkg/artifacts/signable.go	74.1%	74.7%	0.6
pkg/chains/formats/slsa/internal/material/material.go	84.1%	84.8%	0.7
pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go	Do not exist	81.1%

chuangw6

Thank you @chitrangpatel for kicking off the implementation of SLSA v1.0 support!!!

pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go

tekton-robot · 2023-05-09T14:36:36Z

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

File	Old Coverage	New Coverage	Delta
pkg/artifacts/signable.go	74.1%	74.7%	0.6
pkg/chains/formats/slsa/internal/material/material.go	84.1%	84.8%	0.7
pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go	Do not exist	80.9%

tekton-robot · 2023-05-10T15:42:29Z

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

File	Old Coverage	New Coverage	Delta
pkg/artifacts/signable.go	74.1%	74.7%	0.6
pkg/chains/formats/slsa/internal/material/material.go	84.1%	84.8%	0.7
pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go	Do not exist	80.9%

pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go

pkg/chains/formats/slsa/internal/material/material.go

pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go

pkg/artifacts/signable.go

pkg/chains/formats/slsa/internal/material/material.go

pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go

chuangw6

Thank you @chitrangpatel for the refactor! Much cleaner now!

One general comment I have is that it would be great if we can have a consistent way to collect the list of materials/resolvedDependencies throughout the helper functions.

Currently, there are three categories:

some functions just receive *[]common.ProvenanceMaterials as an argument and mutate it.
some receive the []common.ProvenanceMaterials but also return []common.ProvenanceMaterials
som functions just return a new []common.ProvenanceMaterials without receiving a list of materials.

pkg/artifacts/signable.go

pkg/chains/formats/slsa/internal/material/material.go

pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go

pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies_test.go

tekton-robot · 2023-05-18T15:51:52Z

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

File	Old Coverage	New Coverage	Delta
pkg/artifacts/signable.go	74.1%	73.8%	-0.3
pkg/chains/formats/slsa/internal/material/material.go	84.3%	95.9%	11.6
pkg/chains/formats/slsa/v1/pipelinerun/pipelinerun.go	77.2%	79.6%	2.4
pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go	Do not exist	86.2%

chitrangpatel · 2023-05-18T17:26:42Z

Thank you @chitrangpatel for the refactor! Much cleaner now!

One general comment I have is that it would be great if we can have a consistent way to collect the list of materials/resolvedDependencies throughout the helper functions.

Currently, there are three categories:

some functions just receive []*common.ProvenanceMaterials as an argument and mutate it.

some receive the []common.ProvenanceMaterials but also return []common.ProvenanceMaterials

som functions just return a new []common.ProvenanceMaterials without receiving a list of materials.

Thats a good idea. Since its a lot of refactoring, I suggest we do this in a followup PR since it does not fit well with this.

tekton-robot · 2023-05-18T18:03:05Z

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

File	Old Coverage	New Coverage	Delta
pkg/artifacts/signable.go	74.1%	73.8%	-0.3
pkg/chains/formats/slsa/internal/material/material.go	84.3%	95.9%	11.7
pkg/chains/formats/slsa/v1/pipelinerun/pipelinerun.go	77.2%	79.6%	2.4
pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go	Do not exist	86.8%

tekton-robot · 2023-05-18T18:32:31Z

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

File	Old Coverage	New Coverage	Delta
pkg/artifacts/signable.go	74.1%	73.8%	-0.3
pkg/chains/formats/slsa/internal/material/material.go	84.3%	95.9%	11.7
pkg/chains/formats/slsa/v1/pipelinerun/pipelinerun.go	77.2%	79.6%	2.4
pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go	Do not exist	86.8%

tekton-robot · 2023-05-18T19:09:46Z

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

File	Old Coverage	New Coverage	Delta
pkg/artifacts/signable.go	74.1%	73.8%	-0.3
pkg/chains/formats/slsa/internal/material/material.go	84.3%	95.9%	11.7
pkg/chains/formats/slsa/v1/pipelinerun/pipelinerun.go	77.2%	79.6%	2.4
pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go	Do not exist	86.8%

tekton-robot · 2023-05-18T19:22:48Z

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

File	Old Coverage	New Coverage	Delta
pkg/artifacts/signable.go	74.1%	73.8%	-0.3
pkg/chains/formats/slsa/internal/material/material.go	84.3%	95.9%	11.7
pkg/chains/formats/slsa/v1/pipelinerun/pipelinerun.go	77.2%	79.6%	2.4
pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go	Do not exist	86.8%

lcarva

/lgtm

Left a couple of minor comments that are non-blocking.

pkg/chains/formats/slsa/internal/material/material.go

pkg/chains/formats/slsa/v1/pipelinerun/pipelinerun.go

lcarva · 2023-05-19T18:22:49Z

pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go

+	mats := []common.ProvenanceMaterial{}
+
+	// add step and sidecar images
+	if err := material.AddStepImagesToMaterials(tro.Status.Steps, &mats); err != nil {


I'm a bit concerned that some of the functions in material take a pointer address to a materials slice, like this one, for two reasons:

It seems bad practice to pass a slice pointer around.

Other functions on the materials package deal with adding things to materials but don't take a pre-existing slice, e.g. AddMaterialsFromTaskParamsAndResults.

How about something like this?

allMaterials := []common.ProvenanceMaterial{} mats, err := material.FromStepImages(tro.Status.Steps) if err != nil {...} allMaterials = append(allMaterials, mats...)

Agreed! @chuangw6 pointed this out as well. But its better left as its own cleanup PR which I will do after this.

This PR introduces resolved dependencies for the SLSAv1.0 predicate. It addresses part of issue tektoncd#797

chitrangpatel · 2023-05-19T19:14:04Z

@lcarva could you please lgtm it again? I implemented the nits you mentioned. Thanks!

tekton-robot · 2023-05-19T19:16:23Z

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

File	Old Coverage	New Coverage	Delta
pkg/artifacts/signable.go	74.1%	73.8%	-0.3
pkg/chains/formats/slsa/internal/material/material.go	84.3%	95.8%	11.5
pkg/chains/formats/slsa/v1/pipelinerun/pipelinerun.go	77.2%	79.6%	2.4
pkg/chains/formats/slsa/v2alpha2/internal/resolved_dependencies/resolved_dependencies.go	Do not exist	86.8%

lcarva · 2023-05-19T19:27:52Z

/lgtm

@wlynch, it looks like your comments have been addressed. Ok to merge this? (I'll hold off my approval for now)

wlynch

final nits, but I'll lgtm so we can get this in since we know there's other cleanup we want to do.

Thanks for making those changes. 🎉

/lgtm

wlynch · 2023-05-19T20:21:42Z

pkg/artifacts/signable.go


-func checkDigest(dig string) error {
+// ParseDigest parses the digest string and returns the algorithm and hex section of the digest.
+func ParseDigest(dig string) (algo_string string, hex string, err error) {


wlynch · 2023-05-19T20:26:06Z

pkg/chains/formats/slsa/internal/material/material.go

+		for _, input := range tro.Spec.Resources.Inputs { //nolint:all //incompatible with pipelines v0.45
+			if input.ResourceSpec == nil || input.ResourceSpec.Type != backport.PipelineResourceTypeGit { //nolint:all //incompatible with pipelines v0.45


nit

Suggested change

for _, input := range tro.Spec.Resources.Inputs { //nolint:all //incompatible with pipelines v0.45

if input.ResourceSpec == nil || input.ResourceSpec.Type != backport.PipelineResourceTypeGit { //nolint:all //incompatible with pipelines v0.45

for _, input := range tro.Spec.Resources.Inputs { //nolint:all // needed for pipelines < v0.45

if input.ResourceSpec == nil || input.ResourceSpec.Type != backport.PipelineResourceTypeGit { //nolint:all // needed for pipelines < v0.45

tekton-robot · 2023-05-19T20:34:02Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [wlynch]

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

tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label May 5, 2023

tekton-robot requested review from chuangw6 and wlynch May 5, 2023 20:27

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 5, 2023

chitrangpatel marked this pull request as draft May 5, 2023 20:27

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 5, 2023

chitrangpatel force-pushed the resolved-dependencies branch from b7b1b89 to 8a9594a Compare May 5, 2023 20:28

chitrangpatel mentioned this pull request May 5, 2023

Implementation of SLSA v1.0 predicate in Chains #797

Closed

7 tasks

chitrangpatel force-pushed the resolved-dependencies branch 6 times, most recently from 4ed12fc to 99261ab Compare May 8, 2023 17:59

chitrangpatel marked this pull request as ready for review May 8, 2023 17:59

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2023

tekton-robot requested a review from lcarva May 8, 2023 18:00

chitrangpatel force-pushed the resolved-dependencies branch from 99261ab to 5b4ba18 Compare May 8, 2023 18:12

chuangw6 reviewed May 8, 2023

View reviewed changes

chitrangpatel force-pushed the resolved-dependencies branch from 5b4ba18 to e238bd2 Compare May 9, 2023 14:31

chitrangpatel mentioned this pull request May 9, 2023

Slsa1.0 params, builderID, buildType, subjects, metadata, byProducts #800

Merged

6 tasks

chitrangpatel force-pushed the resolved-dependencies branch from e238bd2 to 7cedd97 Compare May 10, 2023 15:37

chuangw6 reviewed May 10, 2023

View reviewed changes

wlynch requested changes May 15, 2023

View reviewed changes

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2023

chitrangpatel force-pushed the resolved-dependencies branch from bdd3afe to b81fb75 Compare May 18, 2023 15:45

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 18, 2023

chuangw6 reviewed May 18, 2023

View reviewed changes

chitrangpatel force-pushed the resolved-dependencies branch 2 times, most recently from da261c3 to d84a8a8 Compare May 18, 2023 17:58

chitrangpatel force-pushed the resolved-dependencies branch from d84a8a8 to a458fb2 Compare May 18, 2023 18:28

chitrangpatel force-pushed the resolved-dependencies branch from a458fb2 to bd46e56 Compare May 18, 2023 19:05

chitrangpatel force-pushed the resolved-dependencies branch from bd46e56 to 2e25eaf Compare May 18, 2023 19:18

lcarva reviewed May 19, 2023

View reviewed changes

tekton-robot assigned lcarva May 19, 2023

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

Introduce Resolved Dependencies for SLSAv1.0 predicate

d814bdf

This PR introduces resolved dependencies for the SLSAv1.0 predicate. It addresses part of issue tektoncd#797

chitrangpatel force-pushed the resolved-dependencies branch from 2e25eaf to d814bdf Compare May 19, 2023 19:12

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

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

wlynch approved these changes May 19, 2023

View reviewed changes

tekton-robot assigned wlynch May 19, 2023

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

tekton-robot merged commit b8384e6 into tektoncd:main May 19, 2023

		for _, input := range tro.Spec.Resources.Inputs { //nolint:all //incompatible with pipelines v0.45
		if input.ResourceSpec == nil \|\| input.ResourceSpec.Type != backport.PipelineResourceTypeGit { //nolint:all //incompatible with pipelines v0.45

Conversation

chitrangpatel commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

tekton-robot commented May 5, 2023

Uh oh!

tekton-robot commented May 8, 2023

Uh oh!

tekton-robot commented May 8, 2023

Uh oh!

chuangw6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tekton-robot commented May 9, 2023

Uh oh!

tekton-robot commented May 10, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chuangw6 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tekton-robot commented May 18, 2023

Uh oh!

chitrangpatel commented May 18, 2023

Uh oh!

tekton-robot commented May 18, 2023

Uh oh!

tekton-robot commented May 18, 2023

Uh oh!

tekton-robot commented May 18, 2023

Uh oh!

tekton-robot commented May 18, 2023

Uh oh!

lcarva left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lcarva May 19, 2023

Choose a reason for hiding this comment

Uh oh!

chitrangpatel May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

chitrangpatel commented May 5, 2023 •

edited

Loading

chuangw6 left a comment •

edited

Loading

chitrangpatel May 19, 2023 •

edited

Loading