Fix checks to allow passing in of sha1 digests through StructuredResults. by jagathprakash · Pull Request #676 · tektoncd/chains

jagathprakash · 2023-01-12T23:51:18Z

Fixes #636

This PR addresses the issue of artifacts which have sha1 digest not being added into materials.

Before this fix checkDigest would only check if the digest was sha256 and would reject other digests.
This commit fixes checkDigest to ensue that it accepts all valid digests of type sha256, sha512, sha384 and sha1.

Signed-off-by: jagathprakash 31057312+jagathprakash@users.noreply.github.com

Changes

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

NONE

jagathprakash · 2023-01-12T23:52:01Z

/issue 675

tekton-robot · 2023-01-12T23:55:49Z

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	71.2%	73.1%	1.9

lcarva

A few minor comments, but overall it's pretty straight forward.

Do we actually want to support SHA1 since it's been deprecated since 2011?

lcarva · 2023-01-13T14:43:54Z

-	prefix := digest.Canonical.String() + ":"
-	if !strings.HasPrefix(dig, prefix) {
-		return fmt.Errorf("unsupported digest algorithm: %s", dig)
+	// 93c56eeba9ec70f74c9bfd297d9516642d366cb5


yeah. That was a testcase. sorry!!!
removed.

lcarva · 2023-01-13T14:49:18Z

+		if err := algo.Validate(hex); err != nil {
+			return err
+		}
+	case algo_string == "sha1":


It might be worth adding a comment here to explain why sha1 is treated differently.

lcarva · 2023-01-13T14:50:26Z

+			return err
+		}
+	case algo_string == "sha1":
+		if !regexp.MustCompile(`^[a-f0-9]{40}$`).MatchString(hex) {


Maybe regexp.MustCompile should be called at the package level? Minor performance boost.

lcarva · 2023-01-13T14:51:52Z

+		}
+	case algo_string == "sha1":
+		if !regexp.MustCompile(`^[a-f0-9]{40}$`).MatchString(hex) {
+			return fmt.Errorf("sha1 digest %s does not match regexp %s", dig, "^[a-f0-9]{40}$")


Let's use Regexp.String() so we don't have to worry about keeping these strings in sync.

jagathprakash · 2023-01-13T16:04:08Z

/assign @bobcatfish

tekton-robot · 2023-01-13T16:05:27Z

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	71.2%	73.1%	1.9

jagathprakash · 2023-01-13T18:31:26Z

/kind bug

jagathprakash · 2023-01-18T15:17:00Z

/assign lcarva

jagathprakash · 2023-01-18T15:17:48Z

@lcarva could you please take another look at the PR. I have addressed all the comments.

lcarva · 2023-01-18T16:12:36Z

/lgtm

Do we actually want to support SHA1 since it's been deprecated since 2011?

I guess adding this support is fine since it's simply used to detect information about artifacts. @wlynch, any objections?

jagathprakash · 2023-01-18T16:41:48Z

/lgtm

Do we actually want to support SHA1 since it's been deprecated since 2011?

I guess adding this support is fine since it's simply used to detect information about artifacts. @wlynch, any objections?

To provide more context on this.
Though there is experimental support for SHA256, by default Git commit hash is still SHA1. The git-clone task also emits SHA1.

wlynch

I guess adding this support is fine since it's simply used to detect information about artifacts. @wlynch, any objections?

Yeah SHA1 is unavoidable with git. AFAIK no git hosting provider supports the sha256 mode for repos yet (and even then it will take a long time to get repos to move over).

As long as we're only using it for input and not our own digest output, it's fine.

wlynch · 2023-01-18T18:00:45Z

 						}),
 					},
+					{
+						Name: "img2_input" + "_" + ArtifactsInputsResultName,


nit: I'd include the sha# in the name to distinguish these.

wlynch · 2023-01-18T18:03:56Z

 		if strings.HasSuffix(res.Name, categoryMarker) {
 			valid, err := isStructuredResult(res, categoryMarker)
 			if err != nil {
-				logger.Debugf("ExtractStructuredTargetFromResults: %v", err)


I don't think we want to error here - I think it's fine if a result happens to have the suffix but doesn't conform to the structured result format.

+1 agree that we should not error here.
Currently we are just logging a message (although at debug level which does not show up in the logs which is not useful and as such would prefer WARN level atleast) and continuing.

tekton-robot · 2023-01-18T19:24:56Z

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	71.2%	73.1%	1.9

sha1 digest not being added into materials. Before this fix checkDigest would only check if the digest was sha256 and would reject other digests. This commit fixes checkDigest to ensue that it accepts all valid digests of type sha256, sha512, sha384 and sha1. Signed-off-by: jagathprakash <31057312+jagathprakash@users.noreply.github.com>

tekton-robot · 2023-01-19T15:11:28Z

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	71.2%	73.1%	1.9

jagathprakash · 2023-01-19T15:23:16Z

/retest

wlynch

/lgtm

tekton-robot · 2023-01-19T16:28:35Z

[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

sha1 digest not being added into materials. Before this fix checkDigest would only check if the digest was sha256 and would reject other digests. This commit fixes checkDigest to ensue that it accepts all valid digests of type sha256, sha512, sha384 and sha1. Signed-off-by: jagathprakash <31057312+jagathprakash@users.noreply.github.com> Signed-off-by: jagathprakash <31057312+jagathprakash@users.noreply.github.com>

tekton-robot requested review from chuangw6 and lcarva January 12, 2023 23:51

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 12, 2023

lcarva reviewed Jan 13, 2023

View reviewed changes

jagathprakash force-pushed the fix_sha1 branch from eccbcae to 9a0c426 Compare January 13, 2023 16:01

tekton-robot assigned bobcatfish Jan 13, 2023

jagathprakash requested review from lcarva and removed request for chuangw6 January 13, 2023 16:04

tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 13, 2023

tekton-robot assigned lcarva Jan 18, 2023

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

wlynch requested changes Jan 18, 2023

View reviewed changes

jagathprakash force-pushed the fix_sha1 branch from 9a0c426 to a017743 Compare January 18, 2023 19:21

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

jagathprakash requested review from wlynch and removed request for lcarva January 18, 2023 19:30

jagathprakash force-pushed the fix_sha1 branch from a017743 to 919a6c2 Compare January 19, 2023 15:07

wlynch approved these changes Jan 19, 2023

View reviewed changes

tekton-robot assigned wlynch Jan 19, 2023

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

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

tekton-robot merged commit b4e5b53 into tektoncd:main Jan 19, 2023

Uh oh!

Conversation

jagathprakash commented Jan 12, 2023 • edited by wlynch Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

jagathprakash commented Jan 12, 2023

Uh oh!

tekton-robot commented Jan 12, 2023

Uh oh!

lcarva left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jagathprakash commented Jan 13, 2023

Uh oh!

tekton-robot commented Jan 13, 2023

Uh oh!

jagathprakash commented Jan 13, 2023

Uh oh!

jagathprakash commented Jan 18, 2023

Uh oh!

jagathprakash commented Jan 18, 2023

Uh oh!

lcarva commented Jan 18, 2023

Uh oh!

jagathprakash commented Jan 18, 2023

Uh oh!

wlynch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Jan 18, 2023

Uh oh!

tekton-robot commented Jan 19, 2023

Uh oh!

jagathprakash commented Jan 19, 2023

Uh oh!

wlynch left a comment

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented Jan 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jagathprakash commented Jan 12, 2023 •

edited by wlynch

Loading