Merge pull request from GHSA-c38g-469g-cmgx

fix(*): Validate metadata semver and printable characters

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
+			continue
-			if (err == semver.ErrSegmentStartsZero || err == semver.ErrInvalidPrerelease) && !o.devel {
+		}
-				continue
+		if constraint.Check(v) {
-			}
+			data = append(data, r)
-			appendSearchResults(r)
+			foundNames[r.Name] = true
-		} else if constraint.Check(v) {
-			appendSearchResults(r)
 		}
 	}
 
@@ -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
+      apiVersion: v2
-      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: ""

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

@@ -1,2 +0,0 @@
-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

@@ -1,2 +0,0 @@
-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

@@ -0,0 +1,44 @@
+/*
+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
+func isValidSemver(v string) bool {
-// the chart. This check must be done at load time before the dependency's charts are
+	_, err := semver.NewVersion(v)
-// loaded.
+	return err == nil
-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)
+// sanitizeString normalize spaces and removes non-printable characters.
-	}
+func sanitizeString(str string) string {
-	return nil
+	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) {
+func TestValidate_sanitize(t *testing.T) {
-	dep := &Dependency{
+	md := &Metadata{APIVersion: "v2", Name: "test", Version: "1.0", Description: "\adescr\u0081iption\rtest", Maintainers: []*Maintainer{{Name: "\r"}}}
-		Name: "example",
+	if err := md.Validate(); err != nil {
+		t.Fatalf("unexpected error: %s", err)
 	}
-	for value, shouldFail := range map[string]bool{
+	if md.Description != "description test" {
-		"abcdefghijklmenopQRSTUVWXYZ-0123456780_": false,
+		t.Fatalf("description was not sanitized: %q", md.Description)
-		"-okay":      false,
+	}
-		"_okay":      false,
+	if md.Maintainers[0].Name != " " {
-		"- bad":      true,
+		t.Fatal("maintainer name was not sanitized")
-		" 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)
-		}
 	}
 }

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

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

@@ -1,26 +1,28 @@
-apiVersion: v1
+apiVersion: v1
-entries:
+entries:
-  foo:
+  foo:
-    - name: foo
+    - name: foo
-      description: Foo Chart With Relative Path
+      description: Foo Chart With Relative Path
-      home: https://helm.sh/helm
+      home: https://helm.sh/helm
-      keywords: []
+      keywords: []
-      maintainers: []
+      maintainers: []
-      sources:
+      sources:
-        - https://github.com/helm/charts
+        - https://github.com/helm/charts
-      urls:
+      urls:
-        - charts/foo-1.2.3.tgz
+        - charts/foo-1.2.3.tgz
-      version: 1.2.3
+      version: 1.2.3
-      checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d
+      checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d
-  bar:
+      apiVersion: v2
-    - name: bar
+  bar:
-      description: Bar Chart With Relative Path
+    - name: bar
-      home: https://helm.sh/helm
+      description: Bar Chart With Relative Path
-      keywords: []
+      home: https://helm.sh/helm
-      maintainers: []
+      keywords: []
-      sources:
+      maintainers: []
-        - https://github.com/helm/charts
+      sources:
-      urls:
+        - https://github.com/helm/charts
-        - bar-1.2.3.tgz
+      urls:
-      version: 1.2.3
+        - bar-1.2.3.tgz
-      checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d
+      version: 1.2.3
+      checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d
+      apiVersion: v2

pkg/downloader/testdata/repository/testing-relative-trailing-slash-index.yaml

@@ -1,26 +1,28 @@
-apiVersion: v1
+apiVersion: v1
-entries:
+entries:
-  foo:
+  foo:
-    - name: foo
+    - name: foo
-      description: Foo Chart With Relative Path
+      description: Foo Chart With Relative Path
-      home: https://helm.sh/helm
+      home: https://helm.sh/helm
-      keywords: []
+      keywords: []
-      maintainers: []
+      maintainers: []
-      sources:
+      sources:
-        - https://github.com/helm/charts
+        - https://github.com/helm/charts
-      urls:
+      urls:
-        - charts/foo-1.2.3.tgz
+        - charts/foo-1.2.3.tgz
-      version: 1.2.3
+      version: 1.2.3
-      checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d
+      checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d
-  bar:
+      apiVersion: v2
-    - name: bar
+  bar:
-      description: Bar Chart With Relative Path
+    - name: bar
-      home: https://helm.sh/helm
+      description: Bar Chart With Relative Path
-      keywords: []
+      home: https://helm.sh/helm
-      maintainers: []
+      keywords: []
-      sources:
+      maintainers: []
-        - https://github.com/helm/charts
+      sources:
-      urls:
+        - https://github.com/helm/charts
-        - bar-1.2.3.tgz
+      urls:
-      version: 1.2.3
+        - bar-1.2.3.tgz
-      checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d
+      version: 1.2.3
+      checksum: 0e6661f193211d7a5206918d42f5c2a9470b737d
+      apiVersion: v2

pkg/plugin/plugin.go

@@ -23,6 +23,7 @@ import (
 	"regexp"
 	"runtime"
 	"strings"
+	"unicode"
 
 	"github.com/pkg/errors"
 	"sigs.k8s.io/yaml"
@@ -175,10 +176,25 @@ func validatePluginData(plug *Plugin, filepath string) error {
 	if !validPluginName.MatchString(plug.Metadata.Name) {
 		return fmt.Errorf("invalid plugin name at %q", filepath)
 	}
+	plug.Metadata.Usage = sanitizeString(plug.Metadata.Usage)
+
 	// We could also validate SemVer, executable, and other fields should we so choose.
 	return 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)
+}
+
 func detectDuplicates(plugs []*Plugin) error {
 	names := map[string]string{}
 

pkg/repo/chartrepo.go

@@ -82,6 +82,8 @@ func NewChartRepository(cfg *Entry, getters getter.Providers) (*ChartRepository,
 // Load loads a directory of charts as if it were a repository.
 //
 // It requires the presence of an index.yaml file in the directory.
+//
+// Deprecated: remove in Helm 4.
 func (r *ChartRepository) Load() error {
 	dirInfo, err := os.Stat(r.Config.Name)
 	if err != nil {
@@ -99,7 +101,7 @@ func (r *ChartRepository) Load() error {
 			if strings.Contains(f.Name(), "-index.yaml") {
 				i, err := LoadIndexFile(path)
 				if err != nil {
-					return nil
+					return err
 				}
 				r.IndexFile = i
 			} else if strings.HasSuffix(f.Name(), ".tgz") {
@@ -137,7 +139,7 @@ func (r *ChartRepository) DownloadIndexFile() (string, error) {
 		return "", err
 	}
 
-	indexFile, err := loadIndex(index)
+	indexFile, err := loadIndex(index, r.Config.URL)
 	if err != nil {
 		return "", err
 	}
@@ -187,7 +189,9 @@ func (r *ChartRepository) generateIndex() error {
 		}
 
 		if !r.IndexFile.Has(ch.Name(), ch.Metadata.Version) {
-			r.IndexFile.Add(ch.Metadata, path, r.Config.URL, digest)
+			if err := r.IndexFile.MustAdd(ch.Metadata, path, r.Config.URL, digest); err != nil {
+				return errors.Wrapf(err, "failed adding to %s to index", path)
+			}
 		}
 		// TODO: If a chart exists, but has a different Digest, should we error?
 	}

pkg/repo/index.go

@@ -19,6 +19,7 @@ package repo
 import (
 	"bytes"
 	"io/ioutil"
+	"log"
 	"os"
 	"path"
 	"path/filepath"
@@ -105,16 +106,27 @@ func LoadIndexFile(path string) (*IndexFile, error) {
 	if err != nil {
 		return nil, err
 	}
-	return loadIndex(b)
+	i, err := loadIndex(b, path)
+	if err != nil {
+		return nil, errors.Wrapf(err, "error loading %s", path)
+	}
+	return i, nil
 }
 
-// Add adds a file to the index
+// MustAdd adds a file to the index
 // This can leave the index in an unsorted state
-func (i IndexFile) Add(md *chart.Metadata, filename, baseURL, digest string) {
+func (i IndexFile) MustAdd(md *chart.Metadata, filename, baseURL, digest string) error {
+	if md.APIVersion == "" {
+		md.APIVersion = chart.APIVersionV1
+	}
+	if err := md.Validate(); err != nil {
+		return errors.Wrapf(err, "validate failed for %s", filename)
+	}
+
 	u := filename
 	if baseURL != "" {
-		var err error
 		_, file := filepath.Split(filename)
+		var err error
 		u, err = urlutil.URLJoin(baseURL, file)
 		if err != nil {
 			u = path.Join(baseURL, file)
@@ -126,10 +138,17 @@ func (i IndexFile) Add(md *chart.Metadata, filename, baseURL, digest string) {
 		Digest:   digest,
 		Created:  time.Now(),
 	}
-	if ee, ok := i.Entries[md.Name]; !ok {
+	ee := i.Entries[md.Name]
-		i.Entries[md.Name] = ChartVersions{cr}
+	i.Entries[md.Name] = append(ee, cr)
-	} else {
+	return nil
-		i.Entries[md.Name] = append(ee, cr)
+}
+
+// Add adds a file to the index and logs an error.
+//
+// Deprecated: Use index.MustAdd instead.
+func (i IndexFile) Add(md *chart.Metadata, filename, baseURL, digest string) {
+	if err := i.MustAdd(md, filename, baseURL, digest); err != nil {
+		log.Printf("skipping loading invalid entry for chart %q %q from %s: %s", md.Name, md.Version, filename, err)
 	}
 }
 
@@ -294,19 +313,34 @@ func IndexDirectory(dir, baseURL string) (*IndexFile, error) {
 		if err != nil {
 			return index, err
 		}
-		index.Add(c.Metadata, fname, parentURL, hash)
+		if err := index.MustAdd(c.Metadata, fname, parentURL, hash); err != nil {
+			return index, errors.Wrapf(err, "failed adding to %s to index", fname)
+		}
 	}
 	return index, nil
 }
 
 // loadIndex loads an index file and does minimal validity checking.
 //
+// The source parameter is only used for logging.
 // This will fail if API Version is not set (ErrNoAPIVersion) or if the unmarshal fails.
-func loadIndex(data []byte) (*IndexFile, error) {
+func loadIndex(data []byte, source string) (*IndexFile, error) {
 	i := &IndexFile{}
 	if err := yaml.UnmarshalStrict(data, i); err != nil {
 		return i, err
 	}
+
+	for name, cvs := range i.Entries {
+		for idx := len(cvs) - 1; idx >= 0; idx-- {
+			if cvs[idx].APIVersion == "" {
+				cvs[idx].APIVersion = chart.APIVersionV1
+			}
+			if err := cvs[idx].Validate(); err != nil {
+				log.Printf("skipping loading invalid entry for chart %q %q from %s: %s", name, cvs[idx].Version, source, err)
+				cvs = append(cvs[:idx], cvs[idx+1:]...)
+			}
+		}
+	}
 	i.SortEntries()
 	if i.APIVersion == "" {
 		return i, ErrNoAPIVersion

pkg/repo/index_test.go

@@ -27,11 +27,10 @@ import (
 	"strings"
 	"testing"
 
+	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/cli"
 	"helm.sh/helm/v3/pkg/getter"
 	"helm.sh/helm/v3/pkg/helmpath"
-
-	"helm.sh/helm/v3/pkg/chart"
 )
 
 const (
@@ -65,12 +64,23 @@ entries:
 
 func TestIndexFile(t *testing.T) {
 	i := NewIndexFile()
-	i.Add(&chart.Metadata{Name: "clipper", Version: "0.1.0"}, "clipper-0.1.0.tgz", "http://example.com/charts", "sha256:1234567890")
+	for _, x := range []struct {
-	i.Add(&chart.Metadata{Name: "cutter", Version: "0.1.1"}, "cutter-0.1.1.tgz", "http://example.com/charts", "sha256:1234567890abc")
+		md       *chart.Metadata
-	i.Add(&chart.Metadata{Name: "cutter", Version: "0.1.0"}, "cutter-0.1.0.tgz", "http://example.com/charts", "sha256:1234567890abc")
+		filename string
-	i.Add(&chart.Metadata{Name: "cutter", Version: "0.2.0"}, "cutter-0.2.0.tgz", "http://example.com/charts", "sha256:1234567890abc")
+		baseURL  string
-	i.Add(&chart.Metadata{Name: "setter", Version: "0.1.9+alpha"}, "setter-0.1.9+alpha.tgz", "http://example.com/charts", "sha256:1234567890abc")
+		digest   string
-	i.Add(&chart.Metadata{Name: "setter", Version: "0.1.9+beta"}, "setter-0.1.9+beta.tgz", "http://example.com/charts", "sha256:1234567890abc")
+	}{
+		{&chart.Metadata{APIVersion: "v2", Name: "clipper", Version: "0.1.0"}, "clipper-0.1.0.tgz", "http://example.com/charts", "sha256:1234567890"},
+		{&chart.Metadata{APIVersion: "v2", Name: "cutter", Version: "0.1.1"}, "cutter-0.1.1.tgz", "http://example.com/charts", "sha256:1234567890abc"},
+		{&chart.Metadata{APIVersion: "v2", Name: "cutter", Version: "0.1.0"}, "cutter-0.1.0.tgz", "http://example.com/charts", "sha256:1234567890abc"},
+		{&chart.Metadata{APIVersion: "v2", Name: "cutter", Version: "0.2.0"}, "cutter-0.2.0.tgz", "http://example.com/charts", "sha256:1234567890abc"},
+		{&chart.Metadata{APIVersion: "v2", Name: "setter", Version: "0.1.9+alpha"}, "setter-0.1.9+alpha.tgz", "http://example.com/charts", "sha256:1234567890abc"},
+		{&chart.Metadata{APIVersion: "v2", Name: "setter", Version: "0.1.9+beta"}, "setter-0.1.9+beta.tgz", "http://example.com/charts", "sha256:1234567890abc"},
+	} {
+		if err := i.MustAdd(x.md, x.filename, x.baseURL, x.digest); err != nil {
+			t.Errorf("unexpected error adding to index: %s", err)
+		}
+	}
 
 	i.SortEntries()
 
@@ -126,11 +136,7 @@ func TestLoadIndex(t *testing.T) {
 		tc := tc
 		t.Run(tc.Name, func(t *testing.T) {
 			t.Parallel()
-			b, err := ioutil.ReadFile(tc.Filename)
+			i, err := LoadIndexFile(tc.Filename)
-			if err != nil {
-				t.Fatal(err)
-			}
-			i, err := loadIndex(b)
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -141,19 +147,11 @@ func TestLoadIndex(t *testing.T) {
 
 // TestLoadIndex_Duplicates is a regression to make sure that we don't non-deterministically allow duplicate packages.
 func TestLoadIndex_Duplicates(t *testing.T) {
-	if _, err := loadIndex([]byte(indexWithDuplicates)); err == nil {
+	if _, err := loadIndex([]byte(indexWithDuplicates), "indexWithDuplicates"); err == nil {
 		t.Errorf("Expected an error when duplicate entries are present")
 	}
 }
 
-func TestLoadIndexFile(t *testing.T) {
-	i, err := LoadIndexFile(testfile)
-	if err != nil {
-		t.Fatal(err)
-	}
-	verifyLocalIndex(t, i)
-}
-
 func TestLoadIndexFileAnnotations(t *testing.T) {
 	i, err := LoadIndexFile(annotationstestfile)
 	if err != nil {
@@ -170,11 +168,7 @@ func TestLoadIndexFileAnnotations(t *testing.T) {
 }
 
 func TestLoadUnorderedIndex(t *testing.T) {
-	b, err := ioutil.ReadFile(unorderedTestfile)
+	i, err := LoadIndexFile(unorderedTestfile)
-	if err != nil {
-		t.Fatal(err)
-	}
-	i, err := loadIndex(b)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -183,20 +177,26 @@ func TestLoadUnorderedIndex(t *testing.T) {
 
 func TestMerge(t *testing.T) {
 	ind1 := NewIndexFile()
-	ind1.Add(&chart.Metadata{
+
-		Name:    "dreadnought",
+	if err := ind1.MustAdd(&chart.Metadata{APIVersion: "v2", Name: "dreadnought", Version: "0.1.0"}, "dreadnought-0.1.0.tgz", "http://example.com", "aaaa"); err != nil {
-		Version: "0.1.0",
+		t.Fatalf("unexpected error: %s", err)
-	}, "dreadnought-0.1.0.tgz", "http://example.com", "aaaa")
+	}
 
 	ind2 := NewIndexFile()
-	ind2.Add(&chart.Metadata{
+
-		Name:    "dreadnought",
+	for _, x := range []struct {
-		Version: "0.2.0",
+		md       *chart.Metadata
-	}, "dreadnought-0.2.0.tgz", "http://example.com", "aaaabbbb")
+		filename string
-	ind2.Add(&chart.Metadata{
+		baseURL  string
-		Name:    "doughnut",
+		digest   string
-		Version: "0.2.0",
+	}{
-	}, "doughnut-0.2.0.tgz", "http://example.com", "ccccbbbb")
+		{&chart.Metadata{APIVersion: "v2", Name: "dreadnought", Version: "0.2.0"}, "dreadnought-0.2.0.tgz", "http://example.com", "aaaabbbb"},
+		{&chart.Metadata{APIVersion: "v2", Name: "doughnut", Version: "0.2.0"}, "doughnut-0.2.0.tgz", "http://example.com", "ccccbbbb"},
+	} {
+		if err := ind2.MustAdd(x.md, x.filename, x.baseURL, x.digest); err != nil {
+			t.Errorf("unexpected error: %s", err)
+		}
+	}
 
 	ind1.Merge(ind2)
 
@@ -239,12 +239,7 @@ func TestDownloadIndexFile(t *testing.T) {
 			t.Fatalf("error finding created index file: %#v", err)
 		}
 
-		b, err := ioutil.ReadFile(idx)
+		i, err := LoadIndexFile(idx)
-		if err != nil {
-			t.Fatalf("error reading index file: %#v", err)
-		}
-
-		i, err := loadIndex(b)
 		if err != nil {
 			t.Fatalf("Index %q failed to parse: %s", testfile, err)
 		}
@@ -256,7 +251,7 @@ func TestDownloadIndexFile(t *testing.T) {
 			t.Fatalf("error finding created charts file: %#v", err)
 		}
 
-		b, err = ioutil.ReadFile(idx)
+		b, err := ioutil.ReadFile(idx)
 		if err != nil {
 			t.Fatalf("error reading charts file: %#v", err)
 		}
@@ -297,12 +292,7 @@ func TestDownloadIndexFile(t *testing.T) {
 			t.Fatalf("error finding created index file: %#v", err)
 		}
 
-		b, err := ioutil.ReadFile(idx)
+		i, err := LoadIndexFile(idx)
-		if err != nil {
-			t.Fatalf("error reading index file: %#v", err)
-		}
-
-		i, err := loadIndex(b)
 		if err != nil {
 			t.Fatalf("Index %q failed to parse: %s", testfile, err)
 		}
@@ -314,7 +304,7 @@ func TestDownloadIndexFile(t *testing.T) {
 			t.Fatalf("error finding created charts file: %#v", err)
 		}
 
-		b, err = ioutil.ReadFile(idx)
+		b, err := ioutil.ReadFile(idx)
 		if err != nil {
 			t.Fatalf("error reading charts file: %#v", err)
 		}
@@ -345,6 +335,7 @@ func verifyLocalIndex(t *testing.T, i *IndexFile) {
 	expects := []*ChartVersion{
 		{
 			Metadata: &chart.Metadata{
+				APIVersion:  "v2",
 				Name:        "alpine",
 				Description: "string",
 				Version:     "1.0.0",
@@ -359,6 +350,7 @@ func verifyLocalIndex(t *testing.T, i *IndexFile) {
 		},
 		{
 			Metadata: &chart.Metadata{
+				APIVersion:  "v2",
 				Name:        "nginx",
 				Description: "string",
 				Version:     "0.2.0",
@@ -372,6 +364,7 @@ func verifyLocalIndex(t *testing.T, i *IndexFile) {
 		},
 		{
 			Metadata: &chart.Metadata{
+				APIVersion:  "v2",
 				Name:        "nginx",
 				Description: "string",
 				Version:     "0.1.0",
@@ -476,28 +469,44 @@ func TestIndexDirectory(t *testing.T) {
 
 func TestIndexAdd(t *testing.T) {
 	i := NewIndexFile()
-	i.Add(&chart.Metadata{Name: "clipper", Version: "0.1.0"}, "clipper-0.1.0.tgz", "http://example.com/charts", "sha256:1234567890")
+
+	for _, x := range []struct {
+		md       *chart.Metadata
+		filename string
+		baseURL  string
+		digest   string
+	}{
+
+		{&chart.Metadata{APIVersion: "v2", Name: "clipper", Version: "0.1.0"}, "clipper-0.1.0.tgz", "http://example.com/charts", "sha256:1234567890"},
+		{&chart.Metadata{APIVersion: "v2", Name: "alpine", Version: "0.1.0"}, "/home/charts/alpine-0.1.0.tgz", "http://example.com/charts", "sha256:1234567890"},
+		{&chart.Metadata{APIVersion: "v2", Name: "deis", Version: "0.1.0"}, "/home/charts/deis-0.1.0.tgz", "http://example.com/charts/", "sha256:1234567890"},
+	} {
+		if err := i.MustAdd(x.md, x.filename, x.baseURL, x.digest); err != nil {
+			t.Errorf("unexpected error adding to index: %s", err)
+		}
+	}
 
 	if i.Entries["clipper"][0].URLs[0] != "http://example.com/charts/clipper-0.1.0.tgz" {
 		t.Errorf("Expected http://example.com/charts/clipper-0.1.0.tgz, got %s", i.Entries["clipper"][0].URLs[0])
 	}
-
-	i.Add(&chart.Metadata{Name: "alpine", Version: "0.1.0"}, "/home/charts/alpine-0.1.0.tgz", "http://example.com/charts", "sha256:1234567890")
-
 	if i.Entries["alpine"][0].URLs[0] != "http://example.com/charts/alpine-0.1.0.tgz" {
 		t.Errorf("Expected http://example.com/charts/alpine-0.1.0.tgz, got %s", i.Entries["alpine"][0].URLs[0])
 	}
-
-	i.Add(&chart.Metadata{Name: "deis", Version: "0.1.0"}, "/home/charts/deis-0.1.0.tgz", "http://example.com/charts/", "sha256:1234567890")
-
 	if i.Entries["deis"][0].URLs[0] != "http://example.com/charts/deis-0.1.0.tgz" {
 		t.Errorf("Expected http://example.com/charts/deis-0.1.0.tgz, got %s", i.Entries["deis"][0].URLs[0])
 	}
+
+	// test error condition
+	if err := i.MustAdd(&chart.Metadata{}, "error-0.1.0.tgz", "", ""); err == nil {
+		t.Fatal("expected error adding to index")
+	}
 }
 
 func TestIndexWrite(t *testing.T) {
 	i := NewIndexFile()
-	i.Add(&chart.Metadata{Name: "clipper", Version: "0.1.0"}, "clipper-0.1.0.tgz", "http://example.com/charts", "sha256:1234567890")
+	if err := i.MustAdd(&chart.Metadata{APIVersion: "v2", Name: "clipper", Version: "0.1.0"}, "clipper-0.1.0.tgz", "http://example.com/charts", "sha256:1234567890"); err != nil {
+		t.Fatalf("unexpected error: %s", err)
+	}
 	dir, err := ioutil.TempDir("", "helm-tmp")
 	if err != nil {
 		t.Fatal(err)

pkg/repo/testdata/chartmuseum-index.yaml

@@ -14,6 +14,7 @@ entries:
         - popular
         - web server
         - proxy
+      apiVersion: v2
     - urls:
         - https://charts.helm.sh/stable/nginx-0.1.0.tgz
       name: nginx
@@ -25,6 +26,7 @@ entries:
         - popular
         - web server
         - proxy
+      apiVersion: v2
   alpine:
     - urls:
         - https://charts.helm.sh/stable/alpine-1.0.0.tgz
@@ -39,6 +41,7 @@ entries:
         - small
         - sumtin
       digest: "sha256:1234567890abcdef"
+      apiVersion: v2
   chartWithNoURL:
     - name: chartWithNoURL
       description: string
@@ -48,3 +51,4 @@ entries:
         - small
         - sumtin
       digest: "sha256:1234567890abcdef"
+      apiVersion: v2

pkg/repo/testdata/local-index-annotations.yaml

@@ -12,6 +12,7 @@ entries:
         - popular
         - web server
         - proxy
+      apiVersion: v2
     - urls:
         - https://charts.helm.sh/stable/nginx-0.1.0.tgz
       name: nginx
@@ -23,6 +24,7 @@ entries:
         - popular
         - web server
         - proxy
+      apiVersion: v2
   alpine:
     - urls:
         - https://charts.helm.sh/stable/alpine-1.0.0.tgz
@@ -37,6 +39,7 @@ entries:
         - small
         - sumtin
       digest: "sha256:1234567890abcdef"
+      apiVersion: v2
   chartWithNoURL:
     - name: chartWithNoURL
       description: string
@@ -46,5 +49,6 @@ entries:
         - small
         - sumtin
       digest: "sha256:1234567890abcdef"
+      apiVersion: v2
 annotations:
   helm.sh/test: foo bar

pkg/repo/testdata/local-index-unordered.yaml

@@ -12,6 +12,7 @@ entries:
         - popular
         - web server
         - proxy
+      apiVersion: v2
     - urls:
         - https://charts.helm.sh/stable/nginx-0.2.0.tgz
       name: nginx
@@ -23,6 +24,7 @@ entries:
         - popular
         - web server
         - proxy
+      apiVersion: v2
   alpine:
     - urls:
         - https://charts.helm.sh/stable/alpine-1.0.0.tgz
@@ -37,6 +39,7 @@ entries:
         - small
         - sumtin
       digest: "sha256:1234567890abcdef"
+      apiVersion: v2
   chartWithNoURL:
     - name: chartWithNoURL
       description: string
@@ -46,3 +49,4 @@ entries:
         - small
         - sumtin
       digest: "sha256:1234567890abcdef"
+      apiVersion: v2

pkg/repo/testdata/local-index.yaml

@@ -12,6 +12,7 @@ entries:
         - popular
         - web server
         - proxy
+      apiVersion: v2
     - urls:
         - https://charts.helm.sh/stable/nginx-0.1.0.tgz
       name: nginx
@@ -23,6 +24,7 @@ entries:
         - popular
         - web server
         - proxy
+      apiVersion: v2
   alpine:
     - urls:
         - https://charts.helm.sh/stable/alpine-1.0.0.tgz
@@ -37,6 +39,7 @@ entries:
         - small
         - sumtin
       digest: "sha256:1234567890abcdef"
+      apiVersion: v2
   chartWithNoURL:
     - name: chartWithNoURL
       description: string
@@ -46,3 +49,4 @@ entries:
         - small
         - sumtin
       digest: "sha256:1234567890abcdef"
+      apiVersion: v2