Repository: kubernetes-sigs/krew
Pull request #281 (2019-07-20 05:02:28)

pkg/index: validate sha256 values
This makes sure the code only deals with valid lowercase sha256 values
(i.e. all lowercase) while doing comparisons etc.

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/krew/pull/281#" title="Author self-approved">ahmetb</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fkrew).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/krew/blob/master/OWNERS)~~ [ahmetb]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
ahmetb: This is expected to have merge-conflicts with #278 as it updates some test files deleted in that PR.
codecov-io: # [Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/281?src=pr&el=h1) Report
> Merging [#281](https://codecov.io/gh/kubernetes-sigs/krew/pull/281?src=pr&el=desc) into [master](https://codecov.io/gh/kubernetes-sigs/krew/commit/5969272c23bcd5c1327da8514598b04538478ced?src=pr&el=desc) will **increase** coverage by `0.15%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/kubernetes-sigs/krew/pull/281/graphs/tree.svg?width=650&token=J3nFtARI2m&height=150&src=pr)](https://codecov.io/gh/kubernetes-sigs/krew/pull/281?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #281      +/-   ##
==========================================
+ Coverage   55.34%   55.49%   +0.15%     
==========================================
  Files          19       19              
  Lines         880      883       +3     
==========================================
+ Hits          487      490       +3     
  Misses        337      337              
  Partials       56       56
```


| [Impacted Files](https://codecov.io/gh/kubernetes-sigs/krew/pull/281?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/index/validate.go](https://codecov.io/gh/kubernetes-sigs/krew/pull/281/diff?src=pr&el=tree#diff-cGtnL2luZGV4L3ZhbGlkYXRlLmdv) | `95.34% <100%> (+0.34%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/281?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/281?src=pr&el=footer). Last update [5969272...3efd737](https://codecov.io/gh/kubernetes-sigs/krew/pull/281?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

k8s-ci-robot: @ahmetb: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
corneliusweig: /lgtm
/approve 
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/krew/pull/281#" title="Author self-approved">ahmetb</a>*, *<a href="https://github.com/kubernetes-sigs/krew/pull/281#issuecomment-513564289" title="Approved">corneliusweig</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fkrew).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/krew/blob/master/OWNERS)~~ [ahmetb,corneliusweig]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/index/validate.go b/pkg/index/validate.go
index 45cb4bc..88afdfa 100644
--- a/pkg/index/validate.go
+++ b/pkg/index/validate.go
@@ -24,8 +24,13 @@ import (
 	"sigs.k8s.io/krew/pkg/installation/semver"
 )
 
+const (
+	sha256Pattern = `^[a-f0-9]{64}$`
+)
+
 var (
 	safePluginRegexp = regexp.MustCompile(`^[\w-]+$`)
+	validSHA256      = regexp.MustCompile(sha256Pattern)
 
 	// windowsForbidden is taken from  https://docs.microsoft.com/en-us/windows/desktop/FileIO/naming-a-file
 	windowsForbidden = []string{"CON", "PRN", "AUX", "NUL", "COM1", "COM2",
@@ -50,6 +55,8 @@ func isSupportedAPIVersion(apiVersion string) bool {
 	return apiVersion == constants.CurrentAPIVersion
 }
 
+func isValidSHA256(s string) bool { return validSHA256.MatchString(s) }
+
 // Validate checks for structural validity of the Plugin object.
 func (p Plugin) Validate(name string) error {
 	if !isSupportedAPIVersion(p.APIVersion) {
@@ -93,6 +100,9 @@ func (p Platform) Validate() error {
 	if p.Sha256 == "" {
 		return errors.New("sha256 sum has to be set")
 	}
+	if !isValidSHA256(p.Sha256) {
+		return errors.Errorf("sha256 value %s is not valid, must match pattern %s", p.Sha256, sha256Pattern)
+	}
 	if p.Bin == "" {
 		return errors.New("bin has to be set")
 	}


Test patch:
diff --git a/cmd/validate-krew-manifest/main_test.go b/cmd/validate-krew-manifest/main_test.go
index 7503947..c2dd79e 100644
--- a/cmd/validate-krew-manifest/main_test.go
+++ b/cmd/validate-krew-manifest/main_test.go
@@ -84,7 +84,7 @@ func TestValidateManifestFile(t *testing.T) {
 					ShortDescription: "test",
 					Platforms: []index.Platform{{
 						URI:    "http://test.com",
-						Sha256: "deadbeef",
+						Sha256: "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 						Files:  []index.FileOperation{{From: "", To: ""}},
 						Bin:    "bin",
 						Selector: &v1.LabelSelector{
@@ -113,7 +113,7 @@ func TestValidateManifestFile(t *testing.T) {
 					Platforms: []index.Platform{
 						{
 							URI:    "http://test.com",
-							Sha256: "deadbeef",
+							Sha256: "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 							Files:  []index.FileOperation{{From: "", To: ""}},
 							Bin:    "bin",
 							Selector: &v1.LabelSelector{
@@ -121,7 +121,7 @@ func TestValidateManifestFile(t *testing.T) {
 							},
 						}, {
 							URI:    "http://test.com",
-							Sha256: "deadbeef",
+							Sha256: "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 							Files:  []index.FileOperation{{From: "", To: ""}},
 							Bin:    "bin",
 							Selector: &v1.LabelSelector{
diff --git a/pkg/index/indexscanner/testdata/testindex/dontscan.yaml b/pkg/index/indexscanner/testdata/testindex/dontscan.yaml
index 03b348a..5c50f26 100644
--- a/pkg/index/indexscanner/testdata/testindex/dontscan.yaml
+++ b/pkg/index/indexscanner/testdata/testindex/dontscan.yaml
@@ -21,14 +21,14 @@ spec:
   - files:
     - from: "*"
     uri: https://example.com
-    sha256: deadbeef
+    sha256: deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef
     selector:
       matchExpressions:
       - {key: os, operator: In, values: [macos, linux]}
   - files:
     - from: "*"
     uri: https://example.com
-    sha256: deadbeef
+    sha256: deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef
     selector:
       matchLabels:
         os: "windows"
diff --git a/pkg/index/indexscanner/testdata/testindex/plugins/bar.yaml b/pkg/index/indexscanner/testdata/testindex/plugins/bar.yaml
index 2cc2703..c9b5dbf 100644
--- a/pkg/index/indexscanner/testdata/testindex/plugins/bar.yaml
+++ b/pkg/index/indexscanner/testdata/testindex/plugins/bar.yaml
@@ -22,11 +22,11 @@ spec:
   - files:
     - from: "*"
     uri: https://example.com
-    sha256: alsoSet
+    sha256: deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef
     bin: kubectl-bar
   - files:
     - from: "*"
     uri: https://example.com
-    sha256: alsoSet
+    sha256: deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef
     bin: kubectl-bar
   shortDescription: "exists"
diff --git a/pkg/index/indexscanner/testdata/testindex/plugins/foo.yaml b/pkg/index/indexscanner/testdata/testindex/plugins/foo.yaml
index 8c66bee..f7eaff5 100644
--- a/pkg/index/indexscanner/testdata/testindex/plugins/foo.yaml
+++ b/pkg/index/indexscanner/testdata/testindex/plugins/foo.yaml
@@ -22,7 +22,7 @@ spec:
   - files:
     - from: "*"
     uri: https://example.com
-    sha256: deadbeef
+    sha256: deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef
     bin: kubectl-foo
     selector:
       matchExpressions:
@@ -30,7 +30,7 @@ spec:
   - files:
     - from: "*"
     uri: https://example.com
-    sha256: deadbeef
+    sha256: deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef
     bin: kubectl-foo
     selector:
       matchLabels:
diff --git a/pkg/index/validate_test.go b/pkg/index/validate_test.go
index e452908..8aef070 100644
--- a/pkg/index/validate_test.go
+++ b/pkg/index/validate_test.go
@@ -121,7 +121,7 @@ func TestPlugin_Validate(t *testing.T) {
 					ShortDescription: "short",
 					Platforms: []Platform{{
 						URI:      "http://example.com",
-						Sha256:   "deadbeef",
+						Sha256:   "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 						Selector: nil,
 						Files:    []FileOperation{{"", ""}},
 						Bin:      "foo",
@@ -144,7 +144,7 @@ func TestPlugin_Validate(t *testing.T) {
 					ShortDescription: "short",
 					Platforms: []Platform{{
 						URI:      "http://example.com",
-						Sha256:   "deadbeef",
+						Sha256:   "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 						Selector: nil,
 						Files:    []FileOperation{{"", ""}},
 						Bin:      "foo",
@@ -167,7 +167,7 @@ func TestPlugin_Validate(t *testing.T) {
 					ShortDescription: "short",
 					Platforms: []Platform{{
 						URI:      "http://example.com",
-						Sha256:   "deadbeef",
+						Sha256:   "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 						Selector: nil,
 						Files:    []FileOperation{{"", ""}},
 						Bin:      "foo",
@@ -187,7 +187,7 @@ func TestPlugin_Validate(t *testing.T) {
 					ShortDescription: "",
 					Platforms: []Platform{{
 						URI:      "http://example.com",
-						Sha256:   "deadbeef",
+						Sha256:   "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 						Selector: nil,
 						Files:    []FileOperation{{"", ""}},
 						Bin:      "foo",
@@ -207,7 +207,7 @@ func TestPlugin_Validate(t *testing.T) {
 					ShortDescription: "short",
 					Platforms: []Platform{{
 						URI:      "http://example.com",
-						Sha256:   "deadbeef",
+						Sha256:   "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 						Selector: nil,
 						Files:    []FileOperation{{"", ""}},
 						Bin:      "foo",
@@ -227,7 +227,7 @@ func TestPlugin_Validate(t *testing.T) {
 					ShortDescription: "short",
 					Platforms: []Platform{{
 						URI:    "http://example.com",
-						Sha256: "deadbeef",
+						Sha256: "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 						Files:  []FileOperation{{"", ""}},
 						Bin:    "foo",
 					}},
@@ -246,7 +246,7 @@ func TestPlugin_Validate(t *testing.T) {
 					ShortDescription: "short",
 					Platforms: []Platform{{
 						URI:      "http://example.com",
-						Sha256:   "deadbeef",
+						Sha256:   "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 						Selector: nil,
 						Files:    []FileOperation{},
 						Bin:      "foo",
@@ -266,7 +266,7 @@ func TestPlugin_Validate(t *testing.T) {
 					ShortDescription: "short",
 					Platforms: []Platform{{
 						URI:      "http://example.com",
-						Sha256:   "deadbeef",
+						Sha256:   "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 						Selector: nil,
 						Files:    []FileOperation{{"", ""}},
 						Bin:      "foo",
@@ -286,7 +286,7 @@ func TestPlugin_Validate(t *testing.T) {
 					ShortDescription: "short",
 					Platforms: []Platform{{
 						URI:      "http://example.com",
-						Sha256:   "deadbeef",
+						Sha256:   "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 						Selector: nil,
 						Files:    []FileOperation{{"", ""}},
 						Bin:      "foo",
@@ -328,7 +328,7 @@ func TestPlatform_Validate(t *testing.T) {
 			name: "no error validation",
 			fields: fields{
 				URI:      "http://example.com",
-				Sha256:   "deadbeef",
+				Sha256:   "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 				Selector: nil,
 				Files:    []FileOperation{{"", ""}},
 				Bin:      "foo",
@@ -336,10 +336,10 @@ func TestPlatform_Validate(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "only hash",
+			name: "url is missing",
 			fields: fields{
 				URI:      "",
-				Sha256:   "deadbeef",
+				Sha256:   "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 				Selector: nil,
 				Files:    []FileOperation{{"", ""}},
 				Bin:      "foo",
@@ -347,7 +347,7 @@ func TestPlatform_Validate(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "only uri",
+			name: "sha256 is missing",
 			fields: fields{
 				URI:      "http://example.com",
 				Sha256:   "",
@@ -357,11 +357,22 @@ func TestPlatform_Validate(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "sha256 is malformed",
+			fields: fields{
+				URI:      "http://example.com",
+				Sha256:   "DEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEFDEADBEEF",
+				Selector: nil,
+				Files:    []FileOperation{{"", ""}},
+				Bin:      "foo",
+			},
+			wantErr: true,
+		},
 		{
 			name: "no file operations",
 			fields: fields{
 				URI:      "http://example.com",
-				Sha256:   "deadbeef",
+				Sha256:   "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 				Selector: nil,
 				Files:    []FileOperation{},
 				Bin:      "foo",
@@ -372,7 +383,7 @@ func TestPlatform_Validate(t *testing.T) {
 			name: "no bin field",
 			fields: fields{
 				URI:      "http://example.com",
-				Sha256:   "deadbeef",
+				Sha256:   "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 				Selector: nil,
 				Files:    []FileOperation{{"", ""}},
 				Bin:      "",
@@ -395,3 +406,23 @@ func TestPlatform_Validate(t *testing.T) {
 		})
 	}
 }
+
+func Test_isValidSHA256(t *testing.T) {
+	tests := []struct {
+		in   string
+		want bool
+	}{
+		{"", false},
+		{"d41d8cd98f00b204e9800998ecf8427e", false},
+		{"dc2f2e1ec8a0acb6f3e23580d4a8b38c44823e948c40342e13ff6e8e12edb15a", true},
+		{"DC2F2E1EC8A0ACB6F3E23580D4A8B38C44823E948C40342E13FF6E8E12EDB15A", false},  // uppercase
+		{"xc2f2e1ec8a0acb6f3e23580d4a8b38c44823e948c40342e13ff6e8e12edb15a", false},  // has 'x'
+		{"dc2f2e1ec8a0acb6f3e23580d4a8b38c44823e948c40342e13ff6e8e12edb15", false},   // 1 missing character
+		{"dc2f2e1ec8a0acb6f3e23580d4a8b38c44823e948c40342e13ff6e8e12edb15ab", false}, // 1 extra character
+	}
+	for _, tt := range tests {
+		if got := isValidSHA256(tt.in); got != tt.want {
+			t.Errorf("isValidSHA256(%s) = %v, want %v", tt.in, got, tt.want)
+		}
+	}
+}
diff --git a/pkg/info/info_test.go b/pkg/info/info_test.go
index 829c7b6..50a7ff8 100644
--- a/pkg/info/info_test.go
+++ b/pkg/info/info_test.go
@@ -46,7 +46,7 @@ var (
 			Homepage:         "",
 			Platforms: []index.Platform{{
 				URI:      "http://example.com",
-				Sha256:   "deadbeef",
+				Sha256:   "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 				Selector: nil,
 				Files:    []index.FileOperation{{From: "", To: ""}},
 				Bin:      "foo",
diff --git a/pkg/installation/util_test.go b/pkg/installation/util_test.go
index 925c546..2d152d4 100644
--- a/pkg/installation/util_test.go
+++ b/pkg/installation/util_test.go
@@ -28,7 +28,7 @@ import (
 func Test_getDownloadTarget(t *testing.T) {
 	matchingPlatform := index.Platform{
 		URI:    "https://uri.git",
-		Sha256: "deadbeef",
+		Sha256: "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 		Selector: &v1.LabelSelector{
 			MatchLabels: map[string]string{
 				"os": runtime.GOOS,
@@ -71,7 +71,7 @@ func Test_getDownloadTarget(t *testing.T) {
 				},
 			},
 			wantVersion:   "v1.0.1",
-			wantSHA256Sum: "deadbeef",
+			wantSHA256Sum: "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 			wantURI:       "https://uri.git",
 			wantFos:       nil,
 			wantBin:       "kubectl-foo",
diff --git a/pkg/receipt/receipt_test.go b/pkg/receipt/receipt_test.go
index 41af47d..e7ae8fc 100644
--- a/pkg/receipt/receipt_test.go
+++ b/pkg/receipt/receipt_test.go
@@ -42,7 +42,7 @@ var (
 			ShortDescription: "short",
 			Platforms: []index.Platform{{
 				URI:      "http://example.com",
-				Sha256:   "deadbeef",
+				Sha256:   "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
 				Selector: nil,
 				Files:    []index.FileOperation{{From: "", To: ""}},
 				Bin:      "foo",