fix(*): Validate metadata semver and printable characters

ref: https://github.com/helm/helm/security/advisories/GHSA-c38g-469g-cmgx



* Skip invalid chart versions when reading the repository index file or
  when programmatically adding a chart version.
* Adds semver validation and strips non-printable characters and
  normalizes spaces for string fields in Metadata.Validate()
* Fixes a unit test that was pulling a remote repo.  Now uses a local
  repo.
* Fixes ignored error in repo update command
Signed-off-by: Adam Reese <adam@reese.io>

cmd/helm/repo_update.go

@@ -63,9 +63,15 @@ func newRepoUpdateCmd(out io.Writer) *cobra.Command {
 
 func (o *repoUpdateOptions) run(out io.Writer) error {
 	f, err := repo.LoadFile(o.repoFile)
-	if isNotExist(err) || len(f.Repositories) == 0 {
+	switch {
+	case isNotExist(err):
+		return errNoRepositories
+	case err != nil:
+		return errors.Wrapf(err, "failed loading file: %s", o.repoFile)
+	case len(f.Repositories) == 0:
 		return errNoRepositories
 	}
+
 	var repos []*repo.ChartRepository
 	for _, cfg := range f.Repositories {
 		r, err := repo.NewChartRepository(cfg, getter.All(settings))

cmd/helm/repo_update_test.go

@@ -19,6 +19,7 @@ import (
 	"bytes"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"os"
 	"path/filepath"
 	"strings"
@@ -53,20 +54,27 @@ func TestUpdateCmd(t *testing.T) {
 }
 
 func TestUpdateCustomCacheCmd(t *testing.T) {
-	var out bytes.Buffer
 	rootDir := ensure.TempDir(t)
 	cachePath := filepath.Join(rootDir, "updcustomcache")
-	_ = os.Mkdir(cachePath, os.ModePerm)
+	os.Mkdir(cachePath, os.ModePerm)
 	defer os.RemoveAll(cachePath)
+
+	ts, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer ts.Stop()
+
 	o := &repoUpdateOptions{
 		update:    updateCharts,
-		repoFile:  "testdata/repositories.yaml",
+		repoFile:  filepath.Join(ts.Root(), "repositories.yaml"),
 		repoCache: cachePath,
 	}
-	if err := o.run(&out); err != nil {
+	b := ioutil.Discard
+	if err := o.run(b); err != nil {
 		t.Fatal(err)
 	}
-	if _, err := os.Stat(filepath.Join(cachePath, "charts-index.yaml")); err != nil {
+	if _, err := os.Stat(filepath.Join(cachePath, "test-index.yaml")); err != nil {
 		t.Fatalf("error finding created index file in custom cache: %v", err)
 	}
 }

cmd/helm/search_repo.go

@@ -143,7 +143,7 @@ func (o *searchRepoOptions) setupSearchedVersion() {
 }
 
 func (o *searchRepoOptions) applyConstraint(res []*search.Result) ([]*search.Result, error) {
-	if len(o.version) == 0 {
+	if o.version == "" {
 		return res, nil
 	}
 
@@ -154,26 +154,19 @@ func (o *searchRepoOptions) applyConstraint(res []*search.Result) ([]*search.Res
 
 	data := res[:0]
 	foundNames := map[string]bool{}
-	appendSearchResults := func(res *search.Result) {
-		data = append(data, res)
-		if !o.versions {
-			foundNames[res.Name] = true // If user hasn't requested all versions, only show the latest that matches
-		}
-	}
 	for _, r := range res {
-		if _, found := foundNames[r.Name]; found {
+		// if not returning all versions and already have found a result,
+		// you're done!
+		if !o.versions && foundNames[r.Name] {
 			continue
 		}
 		v, err := semver.NewVersion(r.Chart.Version)
-
 		if err != nil {
-			// If the current version number check appears ErrSegmentStartsZero or ErrInvalidPrerelease error and not devel mode, ignore
-			if (err == semver.ErrSegmentStartsZero || err == semver.ErrInvalidPrerelease) && !o.devel {
-				continue
-			}
-			appendSearchResults(r)
-		} else if constraint.Check(v) {
-			appendSearchResults(r)
+			continue
+		}
+		if constraint.Check(v) {
+			data = append(data, r)
+			foundNames[r.Name] = true
 		}
 	}
 
@@ -194,6 +187,7 @@ func (o *searchRepoOptions) buildIndex() (*search.Index, error) {
 		ind, err := repo.LoadIndexFile(f)
 		if err != nil {
 			warning("Repo %q is corrupt or missing. Try 'helm repo update'.", n)
+			warning("%s", err)
 			continue
 		}
 

cmd/helm/search_repo_test.go

@@ -68,14 +68,6 @@ func TestSearchRepositoriesCmd(t *testing.T) {
 		name:   "search for 'maria', expect valid json output",
 		cmd:    "search repo maria --output json",
 		golden: "output/search-output-json.txt",
-	}, {
-		name:   "search for 'maria', expect one match with semver begin with zero development version",
-		cmd:    "search repo maria --devel",
-		golden: "output/search-semver-pre-zero-devel-release.txt",
-	}, {
-		name:   "search for 'nginx-ingress', expect one match with invalid development pre version",
-		cmd:    "search repo nginx-ingress --devel",
-		golden: "output/search-semver-pre-invalid-release.txt",
 	}, {
 		name:   "search for 'alpine', expect valid yaml output",
 		cmd:    "search repo alpine --output yaml",

cmd/helm/testdata/helmhome/helm/repository/testing-index.yaml

@@ -13,6 +13,7 @@ entries:
       keywords: []
       maintainers: []
       icon: ""
+      apiVersion: v2
     - name: alpine
       url: https://charts.helm.sh/stable/alpine-0.2.0.tgz
       checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d
@@ -25,6 +26,7 @@ entries:
       keywords: []
       maintainers: []
       icon: ""
+      apiVersion: v2
     - name: alpine
       url: https://charts.helm.sh/stable/alpine-0.3.0-rc.1.tgz
       checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d
@@ -37,6 +39,7 @@ entries:
       keywords: []
       maintainers: []
       icon: ""
+      apiVersion: v2
   mariadb:
     - name: mariadb
       url: https://charts.helm.sh/stable/mariadb-0.3.0.tgz
@@ -55,33 +58,4 @@ entries:
         - name: Bitnami
           email: containers@bitnami.com
       icon: ""
-    - name: mariadb
-      url: https://charts.helm.sh/stable/mariadb-0.3.0-0565674.tgz
-      checksum: 65229f6de44a2be9f215d11dbff311673fc8ba56
-      home: https://mariadb.org
-      sources:
-        - https://github.com/bitnami/bitnami-docker-mariadb
-      version: 0.3.0-0565674
-      description: Chart for MariaDB
-      keywords:
-        - mariadb
-        - mysql
-        - database
-        - sql
-      maintainers:
-        - name: Bitnami
-          email: containers@bitnami.com
-      icon: ""
-  nginx-ingress:
-    - name: nginx-ingress
-      url: https://github.com/kubernetes/ingress-nginx/ingress-a.b.c.sdfsdf.tgz
-      checksum: 25229f6de44a2be9f215d11dbff31167ddc8ba56
-      home: https://github.com/kubernetes/ingress-nginx
-      sources:
-        - https://github.com/kubernetes/ingress-nginx
-      version: a.b.c.sdfsdf
-      description: Chart for nginx-ingress
-      keywords:
-        - ingress
-        - nginx
-      icon: ""
+      apiVersion: v2

cmd/helm/testdata/output/search-semver-pre-invalid-release.txt

-NAME                 	CHART VERSION	APP VERSION	DESCRIPTION            
-testing/nginx-ingress	a.b.c.sdfsdf 	           	Chart for nginx-ingress

cmd/helm/testdata/output/search-semver-pre-zero-devel-release.txt

-NAME           	CHART VERSION	APP VERSION	DESCRIPTION      
-testing/mariadb	0.3.0-0565674	           	Chart for MariaDB

internal/resolver/testdata/repository/kubernetes-charts-index.yaml

@@ -13,6 +13,7 @@ entries:
       keywords: []
       maintainers: []
       icon: ""
+      apiVersion: v2
     - name: alpine
       urls:
         - https://charts.helm.sh/stable/alpine-0.2.0.tgz
@@ -25,6 +26,7 @@ entries:
       keywords: []
       maintainers: []
       icon: ""
+      apiVersion: v2
   mariadb:
     - name: mariadb
       urls:
@@ -44,3 +46,4 @@ entries:
         - name: Bitnami
           email: containers@bitnami.com
       icon: ""
+      apiVersion: v2

pkg/chart/dependency.go

@@ -49,6 +49,23 @@ type Dependency struct {
 	Alias string `json:"alias,omitempty"`
 }
 
+// Validate checks for common problems with the dependency datastructure in
+// the chart. This check must be done at load time before the dependency's charts are
+// loaded.
+func (d *Dependency) Validate() error {
+	d.Name = sanitizeString(d.Name)
+	d.Version = sanitizeString(d.Version)
+	d.Repository = sanitizeString(d.Repository)
+	d.Condition = sanitizeString(d.Condition)
+	for i := range d.Tags {
+		d.Tags[i] = sanitizeString(d.Tags[i])
+	}
+	if d.Alias != "" && !aliasNameFormat.MatchString(d.Alias) {
+		return ValidationErrorf("dependency %q has disallowed characters in the alias", d.Name)
+	}
+	return nil
+}
+
 // Lock is a lock file for dependencies.
 //
 // It represents the state that the dependencies should be in.

pkg/chart/dependency_test.go

+/*
+Copyright The Helm Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+package chart
+
+import (
+	"testing"
+)
+
+func TestValidateDependency(t *testing.T) {
+	dep := &Dependency{
+		Name: "example",
+	}
+	for value, shouldFail := range map[string]bool{
+		"abcdefghijklmenopQRSTUVWXYZ-0123456780_": false,
+		"-okay":      false,
+		"_okay":      false,
+		"- bad":      true,
+		" bad":       true,
+		"bad\nvalue": true,
+		"bad ":       true,
+		"bad$":       true,
+	} {
+		dep.Alias = value
+		res := dep.Validate()
+		if res != nil && !shouldFail {
+			t.Errorf("Failed on case %q", dep.Alias)
+		} else if res == nil && shouldFail {
+			t.Errorf("Expected failure for %q", dep.Alias)
+		}
+	}
+}

pkg/chart/metadata.go

@@ -15,6 +15,13 @@ limitations under the License.
 
 package chart
 
+import (
+	"strings"
+	"unicode"
+
+	"github.com/Masterminds/semver/v3"
+)
+
 // Maintainer describes a Chart maintainer.
 type Maintainer struct {
 	// Name is a user name or organization name
@@ -25,15 +32,23 @@ type Maintainer struct {
 	URL string `json:"url,omitempty"`
 }
 
+// Validate checks valid data and sanitizes string characters.
+func (m *Maintainer) Validate() error {
+	m.Name = sanitizeString(m.Name)
+	m.Email = sanitizeString(m.Email)
+	m.URL = sanitizeString(m.URL)
+	return nil
+}
+
 // Metadata for a Chart file. This models the structure of a Chart.yaml file.
 type Metadata struct {
-	// The name of the chart
+	// The name of the chart. Required.
 	Name string `json:"name,omitempty"`
 	// The URL to a relevant project page, git repo, or contact person
 	Home string `json:"home,omitempty"`
 	// Source is the URL to the source code of this chart
 	Sources []string `json:"sources,omitempty"`
-	// A SemVer 2 conformant version string of the chart
+	// A SemVer 2 conformant version string of the chart. Required.
 	Version string `json:"version,omitempty"`
 	// A one-sentence description of the chart
 	Description string `json:"description,omitempty"`
@@ -43,7 +58,7 @@ type Metadata struct {
 	Maintainers []*Maintainer `json:"maintainers,omitempty"`
 	// The URL to an icon file.
 	Icon string `json:"icon,omitempty"`
-	// The API Version of this chart.
+	// The API Version of this chart. Required.
 	APIVersion string `json:"apiVersion,omitempty"`
 	// The condition to check to enable chart
 	Condition string `json:"condition,omitempty"`
@@ -64,11 +79,28 @@ type Metadata struct {
 	Type string `json:"type,omitempty"`
 }
 
-// Validate checks the metadata for known issues, returning an error if metadata is not correct
+// Validate checks the metadata for known issues and sanitizes string
+// characters.
 func (md *Metadata) Validate() error {
 	if md == nil {
 		return ValidationError("chart.metadata is required")
 	}
+
+	md.Name = sanitizeString(md.Name)
+	md.Description = sanitizeString(md.Description)
+	md.Home = sanitizeString(md.Home)
+	md.Icon = sanitizeString(md.Icon)
+	md.Condition = sanitizeString(md.Condition)
+	md.Tags = sanitizeString(md.Tags)
+	md.AppVersion = sanitizeString(md.AppVersion)
+	md.KubeVersion = sanitizeString(md.KubeVersion)
+	for i := range md.Sources {
+		md.Sources[i] = sanitizeString(md.Sources[i])
+	}
+	for i := range md.Keywords {
+		md.Keywords[i] = sanitizeString(md.Keywords[i])
+	}
+
 	if md.APIVersion == "" {
 		return ValidationError("chart.metadata.apiVersion is required")
 	}
@@ -78,19 +110,26 @@ func (md *Metadata) Validate() error {
 	if md.Version == "" {
 		return ValidationError("chart.metadata.version is required")
 	}
+	if !isValidSemver(md.Version) {
+		return ValidationErrorf("chart.metadata.version %q is invalid", md.Version)
+	}
 	if !isValidChartType(md.Type) {
 		return ValidationError("chart.metadata.type must be application or library")
 	}
 
+	for _, m := range md.Maintainers {
+		if err := m.Validate(); err != nil {
+			return err
+		}
+	}
+
 	// Aliases need to be validated here to make sure that the alias name does
 	// not contain any illegal characters.
 	for _, dependency := range md.Dependencies {
-		if err := validateDependency(dependency); err != nil {
+		if err := dependency.Validate(); err != nil {
 			return err
 		}
 	}
-
-	// TODO validate valid semver here?
 	return nil
 }
 
@@ -102,12 +141,20 @@ func isValidChartType(in string) bool {
 	return false
 }
 
-// validateDependency checks for common problems with the dependency datastructure in
-// the chart. This check must be done at load time before the dependency's charts are
-// loaded.
-func validateDependency(dep *Dependency) error {
-	if len(dep.Alias) > 0 && !aliasNameFormat.MatchString(dep.Alias) {
-		return ValidationErrorf("dependency %q has disallowed characters in the alias", dep.Name)
-	}
-	return nil
+func isValidSemver(v string) bool {
+	_, err := semver.NewVersion(v)
+	return err == nil
+}
+
+// sanitizeString normalize spaces and removes non-printable characters.
+func sanitizeString(str string) string {
+	return strings.Map(func(r rune) rune {
+		if unicode.IsSpace(r) {
+			return ' '
+		}
+		if unicode.IsPrint(r) {
+			return r
+		}
+		return -1
+	}, str)
 }

pkg/chart/metadata_test.go

@@ -72,6 +72,10 @@ func TestValidate(t *testing.T) {
 			},
 			ValidationError("dependency \"bad\" has disallowed characters in the alias"),
 		},
+		{
+			&Metadata{APIVersion: "v2", Name: "test", Version: "1.2.3.4"},
+			ValidationError("chart.metadata.version \"1.2.3.4\" is invalid"),
+		},
 	}
 
 	for _, tt := range tests {
@@ -82,26 +86,15 @@ func TestValidate(t *testing.T) {
 	}
 }
 
-func TestValidateDependency(t *testing.T) {
-	dep := &Dependency{
-		Name: "example",
+func TestValidate_sanitize(t *testing.T) {
+	md := &Metadata{APIVersion: "v2", Name: "test", Version: "1.0", Description: "\adescr\u0081iption\rtest", Maintainers: []*Maintainer{{Name: "\r"}}}
+	if err := md.Validate(); err != nil {
+		t.Fatalf("unexpected error: %s", err)
 	}
-	for value, shouldFail := range map[string]bool{
-		"abcdefghijklmenopQRSTUVWXYZ-0123456780_": false,
-		"-okay":      false,
-		"_okay":      false,
-		"- bad":      true,
-		" bad":       true,
-		"bad\nvalue": true,
-		"bad ":       true,
-		"bad$":       true,
-	} {
-		dep.Alias = value
-		res := validateDependency(dep)
-		if res != nil && !shouldFail {
-			t.Errorf("Failed on case %q", dep.Alias)
-		} else if res == nil && shouldFail {
-			t.Errorf("Expected failure for %q", dep.Alias)
-		}
+	if md.Description != "description test" {
+		t.Fatalf("description was not sanitized: %q", md.Description)
+	}
+	if md.Maintainers[0].Name != " " {
+		t.Fatal("maintainer name was not sanitized")
 	}
 }

pkg/chartutil/save_test.go

@@ -139,7 +139,7 @@ func TestSavePreservesTimestamps(t *testing.T) {
 		Metadata: &chart.Metadata{
 			APIVersion: chart.APIVersionV1,
 			Name:       "ahab",
-			Version:    "1.2.3.4",
+			Version:    "1.2.3",
 		},
 		Values: map[string]interface{}{
 			"imageName": "testimage",

pkg/downloader/testdata/repository/kubernetes-charts-index.yaml

@@ -13,6 +13,7 @@ entries:
       keywords: []
       maintainers: []
       icon: ""
+      apiVersion: v2
     - name: alpine
       urls:
         - https://charts.helm.sh/stable/alpine-0.2.0.tgz
@@ -25,6 +26,7 @@ entries:
       keywords: []
       maintainers: []
       icon: ""
+      apiVersion: v2
   mariadb:
     - name: mariadb
       urls:
@@ -44,3 +46,4 @@ entries:
         - name: Bitnami
           email: containers@bitnami.com
       icon: ""
+      apiVersion: v2

pkg/downloader/testdata/repository/malformed-index.yaml

@@ -13,3 +13,4 @@ entries:
       keywords: []
       maintainers: []
       icon: ""
+      apiVersion: v2

pkg/downloader/testdata/repository/testing-basicauth-index.yaml

@@ -12,3 +12,4 @@ entries:
         - http://username:password@example.com/foo-1.2.3.tgz
       version: 1.2.3
       checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d
+      apiVersion: v2

pkg/downloader/testdata/repository/testing-ca-file-index.yaml

@@ -12,3 +12,4 @@ entries:
         - https://example.com/foo-1.2.3.tgz
       version: 1.2.3
       checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d
+      apiVersion: v2

pkg/downloader/testdata/repository/testing-https-index.yaml

@@ -12,3 +12,4 @@ entries:
         - https://example.com/foo-1.2.3.tgz
       version: 1.2.3
       checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d
+      apiVersion: v2

pkg/downloader/testdata/repository/testing-index.yaml

@@ -13,6 +13,7 @@ entries:
       keywords: []
       maintainers: []
       icon: ""
+      apiVersion: v2
     - name: alpine
       urls:
         - http://example.com/alpine-0.2.0.tgz
@@ -26,6 +27,7 @@ entries:
       keywords: []
       maintainers: []
       icon: ""
+      apiVersion: v2
   foo:
     - name: foo
       description: Foo Chart
@@ -38,3 +40,4 @@ entries:
         - http://example.com/foo-1.2.3.tgz
       version: 1.2.3
       checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d
+      apiVersion: v2

pkg/downloader/testdata/repository/testing-querystring-index.yaml

@@ -13,3 +13,4 @@ entries:
       keywords: []
       maintainers: []
       icon: ""
+      apiVersion: v2