diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 000000000..68334cf33 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,7 @@ +version: 2 + +updates: + - package-ecosystem: "gomod" + directory: "/" + schedule: + interval: "daily" \ No newline at end of file diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a8028dd01..ac88d13f2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -232,8 +232,9 @@ Like any good open source project, we use Pull Requests (PRs) to track code chan 3. Assigning reviews - Once a review has the `awaiting review` label, maintainers will review them as schedule permits. The maintainer who takes the issue should self-request a review. - - Any PR with the `size/large` label requires 2 review approvals from maintainers before it can - be merged. Those with `size/medium` or `size/small` are per the judgement of the maintainers. + - PRs from a community member with the label `size/S` or larger requires 2 review approvals from + maintainers before it can be merged. Those with `size/XS` are per the judgement of the + maintainers. 4. Reviewing/Discussion - All reviews will be completed using Github review tool. - A "Comment" review should be used when there are questions about the code that should be @@ -313,15 +314,16 @@ makes 30 lines of changes in 1 file, but it changes key functionality, it will l feature, but requires another 150 lines of tests to cover all cases, could be labeled as `size/S` even though the number of lines is greater than defined below. -PRs submitted by a core maintainer, regardless of size, only requires approval from one additional -maintainer. This ensures there are at least two maintainers who are aware of any significant PRs -introduced to the codebase. +Any changes from the community labeled as `size/S` or larger should be thoroughly tested before +merging and always requires approval from 2 core maintainers. PRs submitted by a core maintainer, +regardless of size, only requires approval from one additional maintainer. This ensures there are at +least two maintainers who are aware of any significant PRs introduced to the codebase. | Label | Description | | ----- | ----------- | | `size/XS` | Denotes a PR that changes 0-9 lines, ignoring generated files. Very little testing may be required depending on the change. | | `size/S` | Denotes a PR that changes 10-29 lines, ignoring generated files. Only small amounts of manual testing may be required. | | `size/M` | Denotes a PR that changes 30-99 lines, ignoring generated files. Manual validation should be required. | -| `size/L` | Denotes a PR that changes 100-499 lines, ignoring generated files. This should be thoroughly tested before merging and always requires 2 approvals. | -| `size/XL` | Denotes a PR that changes 500-999 lines, ignoring generated files. This should be thoroughly tested before merging and always requires 2 approvals. | -| `size/XXL` | Denotes a PR that changes 1000+ lines, ignoring generated files. This should be thoroughly tested before merging and always requires 2 approvals. | +| `size/L` | Denotes a PR that changes 100-499 lines, ignoring generated files. | +| `size/XL` | Denotes a PR that changes 500-999 lines, ignoring generated files. | +| `size/XXL` | Denotes a PR that changes 1000+ lines, ignoring generated files. | diff --git a/Makefile b/Makefile index 97f99fd86..a9eac8033 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,5 @@ BINDIR := $(CURDIR)/bin +INSTALL_PATH ?= /usr/local/bin DIST_DIRS := find * -type d -exec TARGETS := darwin/amd64 linux/amd64 linux/386 linux/arm linux/arm64 linux/ppc64le linux/s390x windows/amd64 TARGET_OBJS ?= darwin-amd64.tar.gz darwin-amd64.tar.gz.sha256 darwin-amd64.tar.gz.sha256sum linux-amd64.tar.gz linux-amd64.tar.gz.sha256 linux-amd64.tar.gz.sha256sum linux-386.tar.gz linux-386.tar.gz.sha256 linux-386.tar.gz.sha256sum linux-arm.tar.gz linux-arm.tar.gz.sha256 linux-arm.tar.gz.sha256sum linux-arm64.tar.gz linux-arm64.tar.gz.sha256 linux-arm64.tar.gz.sha256sum linux-ppc64le.tar.gz linux-ppc64le.tar.gz.sha256 linux-ppc64le.tar.gz.sha256sum linux-s390x.tar.gz linux-s390x.tar.gz.sha256 linux-s390x.tar.gz.sha256sum windows-amd64.zip windows-amd64.zip.sha256 windows-amd64.zip.sha256sum @@ -49,6 +50,7 @@ endif LDFLAGS += -X helm.sh/helm/v3/internal/version.metadata=${VERSION_METADATA} LDFLAGS += -X helm.sh/helm/v3/internal/version.gitCommit=${GIT_COMMIT} LDFLAGS += -X helm.sh/helm/v3/internal/version.gitTreeState=${GIT_DIRTY} +LDFLAGS += $(EXT_LDFLAGS) .PHONY: all all: build @@ -62,6 +64,13 @@ build: $(BINDIR)/$(BINNAME) $(BINDIR)/$(BINNAME): $(SRC) GO111MODULE=on go build $(GOFLAGS) -tags '$(TAGS)' -ldflags '$(LDFLAGS)' -o '$(BINDIR)'/$(BINNAME) ./cmd/helm +# ------------------------------------------------------------------------------ +# install + +.PHONY: install +install: build + @install "$(BINDIR)/$(BINNAME)" "$(INSTALL_PATH)/$(BINNAME)" + # ------------------------------------------------------------------------------ # test diff --git a/cmd/helm/create.go b/cmd/helm/create.go index 21a7e026c..fe5cc540a 100644 --- a/cmd/helm/create.go +++ b/cmd/helm/create.go @@ -107,6 +107,7 @@ func (o *createOptions) run(out io.Writer) error { return chartutil.CreateFrom(cfile, filepath.Dir(o.name), lstarter) } + chartutil.Stderr = out _, err := chartutil.Create(chartname, filepath.Dir(o.name)) return err } diff --git a/cmd/helm/dependency_build_test.go b/cmd/helm/dependency_build_test.go index eeca12fa6..d6dfdabcb 100644 --- a/cmd/helm/dependency_build_test.go +++ b/cmd/helm/dependency_build_test.go @@ -28,7 +28,7 @@ import ( ) func TestDependencyBuildCmd(t *testing.T) { - srv, err := repotest.NewTempServer("testdata/testcharts/*.tgz") + srv, err := repotest.NewTempServerWithCleanup(t, "testdata/testcharts/*.tgz") defer srv.Stop() if err != nil { t.Fatal(err) diff --git a/cmd/helm/dependency_update_test.go b/cmd/helm/dependency_update_test.go index 1f9d55867..bf27c7b6c 100644 --- a/cmd/helm/dependency_update_test.go +++ b/cmd/helm/dependency_update_test.go @@ -33,7 +33,7 @@ import ( ) func TestDependencyUpdateCmd(t *testing.T) { - srv, err := repotest.NewTempServer("testdata/testcharts/*.tgz") + srv, err := repotest.NewTempServerWithCleanup(t, "testdata/testcharts/*.tgz") if err != nil { t.Fatal(err) } @@ -121,7 +121,7 @@ func TestDependencyUpdateCmd_DontDeleteOldChartsOnError(t *testing.T) { defer resetEnv()() defer ensure.HelmHome(t)() - srv, err := repotest.NewTempServer("testdata/testcharts/*.tgz") + srv, err := repotest.NewTempServerWithCleanup(t, "testdata/testcharts/*.tgz") if err != nil { t.Fatal(err) } diff --git a/cmd/helm/flags.go b/cmd/helm/flags.go index 544fb7608..d1329c279 100644 --- a/cmd/helm/flags.go +++ b/cmd/helm/flags.go @@ -69,7 +69,7 @@ func bindOutputFlag(cmd *cobra.Command, varRef *output.Format) { formatNames = append(formatNames, format) } } - return formatNames, cobra.ShellCompDirectiveDefault + return formatNames, cobra.ShellCompDirectiveNoFileComp }) if err != nil { diff --git a/cmd/helm/load_plugins.go b/cmd/helm/load_plugins.go index a6e0c4eae..83590210a 100644 --- a/cmd/helm/load_plugins.go +++ b/cmd/helm/load_plugins.go @@ -59,7 +59,7 @@ func loadPlugins(baseCmd *cobra.Command, out io.Writer) { found, err := plugin.FindPlugins(settings.PluginsDirectory) if err != nil { - fmt.Fprintf(os.Stderr, "failed to load plugins: %s", err) + fmt.Fprintf(os.Stderr, "failed to load plugins: %s\n", err) return } @@ -154,7 +154,7 @@ func callPluginExecutable(pluginName string, main string, argv []string, out io. func manuallyProcessArgs(args []string) ([]string, []string) { known := []string{} unknown := []string{} - kvargs := []string{"--kube-context", "--namespace", "-n", "--kubeconfig", "--kube-apiserver", "--kube-token", "--registry-config", "--repository-cache", "--repository-config"} + kvargs := []string{"--kube-context", "--namespace", "-n", "--kubeconfig", "--kube-apiserver", "--kube-token", "--kube-as-user", "--kube-as-group", "--registry-config", "--repository-cache", "--repository-config"} knownArg := func(a string) bool { for _, pre := range kvargs { if strings.HasPrefix(a, pre+"=") { diff --git a/cmd/helm/package.go b/cmd/helm/package.go index 00fe0ef11..7134a8784 100644 --- a/cmd/helm/package.go +++ b/cmd/helm/package.go @@ -114,6 +114,7 @@ func newPackageCmd(out io.Writer) *cobra.Command { f.BoolVar(&client.Sign, "sign", false, "use a PGP private key to sign this package") f.StringVar(&client.Key, "key", "", "name of the key to use when signing. Used if --sign is true") f.StringVar(&client.Keyring, "keyring", defaultKeyring(), "location of a public keyring") + f.StringVar(&client.PassphraseFile, "passphrase-file", "", `location of a file which contains the passphrase for the signing key. Use "-" in order to read from stdin.`) f.StringVar(&client.Version, "version", "", "set the version on the chart to this semver version") f.StringVar(&client.AppVersion, "app-version", "", "set the appVersion on the chart to this version") f.StringVarP(&client.Destination, "destination", "d", ".", "location to write the chart.") diff --git a/cmd/helm/plugin_install.go b/cmd/helm/plugin_install.go index 183d3dc57..4e8ee327b 100644 --- a/cmd/helm/plugin_install.go +++ b/cmd/helm/plugin_install.go @@ -19,6 +19,7 @@ import ( "fmt" "io" + "github.com/pkg/errors" "github.com/spf13/cobra" "helm.sh/helm/v3/cmd/helm/require" @@ -81,7 +82,7 @@ func (o *pluginInstallOptions) run(out io.Writer) error { debug("loading plugin from %s", i.Path()) p, err := plugin.LoadDir(i.Path()) if err != nil { - return err + return errors.Wrap(err, "plugin is installed but unusable") } if err := runHook(p, plugin.Install); err != nil { diff --git a/cmd/helm/plugin_test.go b/cmd/helm/plugin_test.go index cf21d8460..0bf867f2a 100644 --- a/cmd/helm/plugin_test.go +++ b/cmd/helm/plugin_test.go @@ -37,6 +37,9 @@ func TestManuallyProcessArgs(t *testing.T) { "--kubeconfig", "/home/foo", "--kube-context=test1", "--kube-context", "test1", + "--kube-as-user", "pikachu", + "--kube-as-group", "teatime", + "--kube-as-group", "admins", "-n=test2", "-n", "test2", "--namespace=test2", @@ -51,6 +54,9 @@ func TestManuallyProcessArgs(t *testing.T) { "--kubeconfig", "/home/foo", "--kube-context=test1", "--kube-context", "test1", + "--kube-as-user", "pikachu", + "--kube-as-group", "teatime", + "--kube-as-group", "admins", "-n=test2", "-n", "test2", "--namespace=test2", diff --git a/cmd/helm/pull_test.go b/cmd/helm/pull_test.go index 3f769a1bc..1d439e873 100644 --- a/cmd/helm/pull_test.go +++ b/cmd/helm/pull_test.go @@ -26,7 +26,7 @@ import ( ) func TestPullCmd(t *testing.T) { - srv, err := repotest.NewTempServer("testdata/testcharts/*.tgz*") + srv, err := repotest.NewTempServerWithCleanup(t, "testdata/testcharts/*.tgz*") if err != nil { t.Fatal(err) } diff --git a/cmd/helm/repo_add.go b/cmd/helm/repo_add.go index 3eeb342f5..f79c213c0 100644 --- a/cmd/helm/repo_add.go +++ b/cmd/helm/repo_add.go @@ -38,11 +38,11 @@ import ( ) type repoAddOptions struct { - name string - url string - username string - password string - noUpdate bool + name string + url string + username string + password string + forceUpdate bool certFile string keyFile string @@ -51,6 +51,9 @@ type repoAddOptions struct { repoFile string repoCache string + + // Deprecated, but cannot be removed until Helm 4 + deprecatedNoUpdate bool } func newRepoAddCmd(out io.Writer) *cobra.Command { @@ -74,7 +77,8 @@ func newRepoAddCmd(out io.Writer) *cobra.Command { f := cmd.Flags() f.StringVar(&o.username, "username", "", "chart repository username") f.StringVar(&o.password, "password", "", "chart repository password") - f.BoolVar(&o.noUpdate, "no-update", false, "raise error if repo is already registered") + f.BoolVar(&o.forceUpdate, "force-update", false, "replace (overwrite) the repo if it already exists") + f.BoolVar(&o.deprecatedNoUpdate, "no-update", false, "Ignored. Formerly, it would disabled forced updates. It is deprecated by force-update.") f.StringVar(&o.certFile, "cert-file", "", "identify HTTPS client using this SSL certificate file") f.StringVar(&o.keyFile, "key-file", "", "identify HTTPS client using this SSL key file") f.StringVar(&o.caFile, "ca-file", "", "verify certificates of HTTPS-enabled servers using this CA bundle") @@ -112,10 +116,6 @@ func (o *repoAddOptions) run(out io.Writer) error { return err } - if o.noUpdate && f.Has(o.name) { - return errors.Errorf("repository name (%s) already exists, please specify a different name", o.name) - } - if o.username != "" && o.password == "" { fd := int(os.Stdin.Fd()) fmt.Fprint(out, "Password: ") @@ -138,6 +138,23 @@ func (o *repoAddOptions) run(out io.Writer) error { InsecureSkipTLSverify: o.insecureSkipTLSverify, } + // If the repo exists do one of two things: + // 1. If the configuration for the name is the same continue without error + // 2. When the config is different require --force-update + if !o.forceUpdate && f.Has(o.name) { + existing := f.Get(o.name) + if c != *existing { + + // The input coming in for the name is different from what is already + // configured. Return an error. + return errors.Errorf("repository name (%s) already exists, please specify a different name", o.name) + } + + // The add is idempotent so do nothing + fmt.Fprintf(out, "%q already exists with the same configuration, skipping\n", o.name) + return nil + } + r, err := repo.NewChartRepository(&c, getter.All(settings)) if err != nil { return err diff --git a/cmd/helm/repo_add_test.go b/cmd/helm/repo_add_test.go index 9ef64390b..f3bc54985 100644 --- a/cmd/helm/repo_add_test.go +++ b/cmd/helm/repo_add_test.go @@ -34,26 +34,50 @@ import ( ) func TestRepoAddCmd(t *testing.T) { - srv, err := repotest.NewTempServer("testdata/testserver/*.*") + srv, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*") if err != nil { t.Fatal(err) } defer srv.Stop() + // A second test server is setup to verify URL changing + srv2, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*") + if err != nil { + t.Fatal(err) + } + defer srv2.Stop() + tmpdir := ensure.TempDir(t) repoFile := filepath.Join(tmpdir, "repositories.yaml") - tests := []cmdTestCase{{ - name: "add a repository", - cmd: fmt.Sprintf("repo add test-name %s --repository-config %s --repository-cache %s", srv.URL(), repoFile, tmpdir), - golden: "output/repo-add.txt", - }} + tests := []cmdTestCase{ + { + name: "add a repository", + cmd: fmt.Sprintf("repo add test-name %s --repository-config %s --repository-cache %s", srv.URL(), repoFile, tmpdir), + golden: "output/repo-add.txt", + }, + { + name: "add repository second time", + cmd: fmt.Sprintf("repo add test-name %s --repository-config %s --repository-cache %s", srv.URL(), repoFile, tmpdir), + golden: "output/repo-add2.txt", + }, + { + name: "add repository different url", + cmd: fmt.Sprintf("repo add test-name %s --repository-config %s --repository-cache %s", srv2.URL(), repoFile, tmpdir), + wantError: true, + }, + { + name: "add repository second time", + cmd: fmt.Sprintf("repo add test-name %s --repository-config %s --repository-cache %s --force-update", srv2.URL(), repoFile, tmpdir), + golden: "output/repo-add.txt", + }, + } runTestCmd(t, tests) } func TestRepoAdd(t *testing.T) { - ts, err := repotest.NewTempServer("testdata/testserver/*.*") + ts, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*") if err != nil { t.Fatal(err) } @@ -65,10 +89,11 @@ func TestRepoAdd(t *testing.T) { const testRepoName = "test-name" o := &repoAddOptions{ - name: testRepoName, - url: ts.URL(), - noUpdate: true, - repoFile: repoFile, + name: testRepoName, + url: ts.URL(), + forceUpdate: false, + deprecatedNoUpdate: true, + repoFile: repoFile, } os.Setenv(xdg.CacheHomeEnvVar, rootDir) @@ -94,7 +119,7 @@ func TestRepoAdd(t *testing.T) { t.Errorf("Error cache charts file was not created for repository %s", testRepoName) } - o.noUpdate = false + o.forceUpdate = true if err := o.run(ioutil.Discard); err != nil { t.Errorf("Repository was not updated: %s", err) @@ -118,7 +143,7 @@ func TestRepoAddConcurrentDirNotExist(t *testing.T) { } func repoAddConcurrent(t *testing.T, testName, repoFile string) { - ts, err := repotest.NewTempServer("testdata/testserver/*.*") + ts, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*") if err != nil { t.Fatal(err) } @@ -130,10 +155,11 @@ func repoAddConcurrent(t *testing.T, testName, repoFile string) { go func(name string) { defer wg.Done() o := &repoAddOptions{ - name: name, - url: ts.URL(), - noUpdate: true, - repoFile: repoFile, + name: name, + url: ts.URL(), + deprecatedNoUpdate: true, + forceUpdate: false, + repoFile: repoFile, } if err := o.run(ioutil.Discard); err != nil { t.Error(err) diff --git a/cmd/helm/repo_remove_test.go b/cmd/helm/repo_remove_test.go index 0ea1d63d2..cb5c6e9ab 100644 --- a/cmd/helm/repo_remove_test.go +++ b/cmd/helm/repo_remove_test.go @@ -30,7 +30,7 @@ import ( ) func TestRepoRemove(t *testing.T) { - ts, err := repotest.NewTempServer("testdata/testserver/*.*") + ts, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*") if err != nil { t.Fatal(err) } diff --git a/cmd/helm/repo_update_test.go b/cmd/helm/repo_update_test.go index e5e4eb337..4b16a1ea7 100644 --- a/cmd/helm/repo_update_test.go +++ b/cmd/helm/repo_update_test.go @@ -75,7 +75,7 @@ func TestUpdateCharts(t *testing.T) { defer resetEnv()() defer ensure.HelmHome(t)() - ts, err := repotest.NewTempServer("testdata/testserver/*.*") + ts, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*") if err != nil { t.Fatal(err) } diff --git a/cmd/helm/root.go b/cmd/helm/root.go index 904f11a21..91542bb7e 100644 --- a/cmd/helm/root.go +++ b/cmd/helm/root.go @@ -48,11 +48,22 @@ Environment variables: | $HELM_CACHE_HOME | set an alternative location for storing cached files. | | $HELM_CONFIG_HOME | set an alternative location for storing Helm configuration. | | $HELM_DATA_HOME | set an alternative location for storing Helm data. | +| $HELM_DEBUG | indicate whether or not Helm is running in Debug mode | | $HELM_DRIVER | set the backend storage driver. Values are: configmap, secret, memory, postgres | | $HELM_DRIVER_SQL_CONNECTION_STRING | set the connection string the SQL storage driver should use. | | $HELM_MAX_HISTORY | set the maximum number of helm release history. | +| $HELM_NAMESPACE | set the namespace used for the helm operations. | | $HELM_NO_PLUGINS | disable plugins. Set HELM_NO_PLUGINS=1 to disable plugins. | +| $HELM_PLUGINS | set the path to the plugins directory | +| $HELM_REGISTRY_CONFIG | set the path to the registry config file. | +| $HELM_REPOSITORY_CACHE | set the path to the repository cache directory | +| $HELM_REPOSITORY_CONFIG | set the path to the repositories file. | | $KUBECONFIG | set an alternative Kubernetes configuration file (default "~/.kube/config") | +| $HELM_KUBEAPISERVER | set the Kubernetes API Server Endpoint for authentication | +| $HELM_KUBEASGROUPS | set the Username to impersonate for the operation. | +| $HELM_KUBEASUSER | set the Groups to use for impoersonation using a comma-separated list. | +| $HELM_KUBECONTEXT | set the name of the kubeconfig context. | +| $HELM_KUBETOKEN | set the Bearer KubeToken used for authentication. | Helm stores cache, configuration, and data based on the following configuration order: @@ -193,5 +204,8 @@ func newRootCmd(actionConfig *action.Configuration, out io.Writer, args []string // Find and add plugins loadPlugins(cmd, out) + // Check permissions on critical files + checkPerms(out) + return cmd, nil } diff --git a/cmd/helm/root_unix.go b/cmd/helm/root_unix.go new file mode 100644 index 000000000..4eb0b442b --- /dev/null +++ b/cmd/helm/root_unix.go @@ -0,0 +1,60 @@ +// +build !windows + +/* +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 main + +import ( + "fmt" + "io" + "os" + "os/user" + "path/filepath" +) + +func checkPerms(out io.Writer) { + // This function MUST NOT FAIL, as it is just a check for a common permissions problem. + // If for some reason the function hits a stopping condition, it may panic. But only if + // we can be sure that it is panicing because Helm cannot proceed. + + kc := settings.KubeConfig + if kc == "" { + kc = os.Getenv("KUBECONFIG") + } + if kc == "" { + u, err := user.Current() + if err != nil { + // No idea where to find KubeConfig, so return silently. Many helm commands + // can proceed happily without a KUBECONFIG, so this is not a fatal error. + return + } + kc = filepath.Join(u.HomeDir, ".kube", "config") + } + fi, err := os.Stat(kc) + if err != nil { + // DO NOT error if no KubeConfig is found. Not all commands require one. + return + } + + perm := fi.Mode().Perm() + if perm&0040 > 0 { + fmt.Fprintf(out, "WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: %s\n", kc) + } + if perm&0004 > 0 { + fmt.Fprintf(out, "WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: %s\n", kc) + } +} diff --git a/cmd/helm/root_unix_test.go b/cmd/helm/root_unix_test.go new file mode 100644 index 000000000..b1fcfbc66 --- /dev/null +++ b/cmd/helm/root_unix_test.go @@ -0,0 +1,63 @@ +// +build !windows + +/* +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 main + +import ( + "bytes" + "io/ioutil" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestCheckPerms(t *testing.T) { + tdir, err := ioutil.TempDir("", "helmtest") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tdir) + tfile := filepath.Join(tdir, "testconfig") + fh, err := os.OpenFile(tfile, os.O_CREATE|os.O_APPEND|os.O_RDWR, 0440) + if err != nil { + t.Errorf("Failed to create temp file: %s", err) + } + + tconfig := settings.KubeConfig + settings.KubeConfig = tfile + defer func() { settings.KubeConfig = tconfig }() + + var b bytes.Buffer + checkPerms(&b) + expectPrefix := "WARNING: Kubernetes configuration file is group-readable. This is insecure. Location:" + if !strings.HasPrefix(b.String(), expectPrefix) { + t.Errorf("Expected to get a warning for group perms. Got %q", b.String()) + } + + if err := fh.Chmod(0404); err != nil { + t.Errorf("Could not change mode on file: %s", err) + } + b.Reset() + checkPerms(&b) + expectPrefix = "WARNING: Kubernetes configuration file is world-readable. This is insecure. Location:" + if !strings.HasPrefix(b.String(), expectPrefix) { + t.Errorf("Expected to get a warning for world perms. Got %q", b.String()) + } + +} diff --git a/cmd/helm/root_windows.go b/cmd/helm/root_windows.go new file mode 100644 index 000000000..243780d40 --- /dev/null +++ b/cmd/helm/root_windows.go @@ -0,0 +1,24 @@ +/* +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 main + +import "io" + +func checkPerms(out io.Writer) { + // Not yet implemented on Windows. If you know how to do a comprehensive perms + // check on Windows, contributions welcomed! +} diff --git a/cmd/helm/show_test.go b/cmd/helm/show_test.go index 2734faf5e..ac5294d3c 100644 --- a/cmd/helm/show_test.go +++ b/cmd/helm/show_test.go @@ -26,7 +26,7 @@ import ( ) func TestShowPreReleaseChart(t *testing.T) { - srv, err := repotest.NewTempServer("testdata/testcharts/*.tgz*") + srv, err := repotest.NewTempServerWithCleanup(t, "testdata/testcharts/*.tgz*") if err != nil { t.Fatal(err) } diff --git a/cmd/helm/testdata/output/env-comp.txt b/cmd/helm/testdata/output/env-comp.txt index c4b46ae6b..3739d8bc1 100644 --- a/cmd/helm/testdata/output/env-comp.txt +++ b/cmd/helm/testdata/output/env-comp.txt @@ -4,6 +4,8 @@ HELM_CONFIG_HOME HELM_DATA_HOME HELM_DEBUG HELM_KUBEAPISERVER +HELM_KUBEASGROUPS +HELM_KUBEASUSER HELM_KUBECONTEXT HELM_KUBETOKEN HELM_MAX_HISTORY diff --git a/cmd/helm/testdata/output/output-comp.txt b/cmd/helm/testdata/output/output-comp.txt index be574756b..e7799a56b 100644 --- a/cmd/helm/testdata/output/output-comp.txt +++ b/cmd/helm/testdata/output/output-comp.txt @@ -1,5 +1,5 @@ table json yaml -:0 -Completion ended with directive: ShellCompDirectiveDefault +:4 +Completion ended with directive: ShellCompDirectiveNoFileComp diff --git a/cmd/helm/testdata/output/repo-add2.txt b/cmd/helm/testdata/output/repo-add2.txt new file mode 100644 index 000000000..263ffa9e4 --- /dev/null +++ b/cmd/helm/testdata/output/repo-add2.txt @@ -0,0 +1 @@ +"test-name" already exists with the same configuration, skipping diff --git a/pkg/action/action.go b/pkg/action/action.go index 071db709b..79bb4f638 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -58,14 +58,15 @@ var ( errMissingRelease = errors.New("no release provided") // errInvalidRevision indicates that an invalid release revision number was provided. errInvalidRevision = errors.New("invalid release revision") - // errInvalidName indicates that an invalid release name was provided - errInvalidName = errors.New("invalid release name, must match regex ^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])+$ and the length must not longer than 53") // errPending indicates that another instance of Helm is already applying an operation on a release. errPending = errors.New("another operation (install/upgrade/rollback) is in progress") ) // ValidName is a regular expression for resource names. // +// DEPRECATED: This will be removed in Helm 4, and is no longer used here. See +// pkg/chartutil.ValidateName for the replacement. +// // According to the Kubernetes help text, the regular expression it uses is: // // [a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)* @@ -294,7 +295,7 @@ func (c *Configuration) Now() time.Time { } func (c *Configuration) releaseContent(name string, version int) (*release.Release, error) { - if err := validateReleaseName(name); err != nil { + if err := chartutil.ValidateReleaseName(name); err != nil { return nil, errors.Errorf("releaseContent: Release name is invalid: %s", name) } diff --git a/pkg/action/action_test.go b/pkg/action/action_test.go index 0cbdb162b..fedf260fb 100644 --- a/pkg/action/action_test.go +++ b/pkg/action/action_test.go @@ -20,6 +20,7 @@ import ( "flag" "io/ioutil" "net/http" + "os" "path/filepath" "testing" @@ -56,6 +57,8 @@ func actionConfigFixture(t *testing.T) *Configuration { t.Fatal(err) } + t.Cleanup(func() { os.RemoveAll(tdir) }) + cache, err := registry.NewCache( registry.CacheOptDebug(true), registry.CacheOptRoot(filepath.Join(tdir, registry.CacheRootDir)), @@ -316,40 +319,3 @@ func TestGetVersionSet(t *testing.T) { t.Error("Non-existent version is reported found.") } } - -// TestValidName is a regression test for ValidName -// -// Kubernetes has strict naming conventions for resource names. This test represents -// those conventions. -// -// See https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names -// -// NOTE: At the time of this writing, the docs above say that names cannot begin with -// digits. However, `kubectl`'s regular expression explicit allows this, and -// Kubernetes (at least as of 1.18) also accepts resources whose names begin with digits. -func TestValidName(t *testing.T) { - names := map[string]bool{ - "": false, - "foo": true, - "foo.bar1234baz.seventyone": true, - "FOO": false, - "123baz": true, - "foo.BAR.baz": false, - "one-two": true, - "-two": false, - "one_two": false, - "a..b": false, - "%^&#$%*@^*@&#^": false, - "example:com": false, - "example%%com": false, - } - for input, expectPass := range names { - if ValidName.MatchString(input) != expectPass { - st := "fail" - if expectPass { - st = "succeed" - } - t.Errorf("Expected %q to %s", input, st) - } - } -} diff --git a/pkg/action/dependency.go b/pkg/action/dependency.go index 4a4b8ebad..4c80d0159 100644 --- a/pkg/action/dependency.go +++ b/pkg/action/dependency.go @@ -21,6 +21,7 @@ import ( "io" "os" "path/filepath" + "strings" "github.com/Masterminds/semver/v3" "github.com/gosuri/uitable" @@ -61,6 +62,7 @@ func (d *Dependency) List(chartpath string, out io.Writer) error { return nil } +// dependecyStatus returns a string describing the status of a dependency viz a viz the parent chart. func (d *Dependency) dependencyStatus(chartpath string, dep *chart.Dependency, parent *chart.Chart) string { filename := fmt.Sprintf("%s-%s.tgz", dep.Name, "*") @@ -75,35 +77,40 @@ func (d *Dependency) dependencyStatus(chartpath string, dep *chart.Dependency, p case err != nil: return "bad pattern" case len(archives) > 1: - return "too many matches" - case len(archives) == 1: - archive := archives[0] - if _, err := os.Stat(archive); err == nil { - c, err := loader.Load(archive) - if err != nil { - return "corrupt" + // See if the second part is a SemVer + found := []string{} + for _, arc := range archives { + // we need to trip the prefix dirs and the extension off. + filename = strings.TrimSuffix(filepath.Base(arc), ".tgz") + maybeVersion := strings.TrimPrefix(filename, fmt.Sprintf("%s-", dep.Name)) + + if _, err := semver.StrictNewVersion(maybeVersion); err == nil { + // If the version parsed without an error, it is possibly a valid + // version. + found = append(found, arc) } - if c.Name() != dep.Name { - return "misnamed" + } + + if l := len(found); l == 1 { + // If we get here, we do the same thing as in len(archives) == 1. + if r := statArchiveForStatus(found[0], dep); r != "" { + return r } - if c.Metadata.Version != dep.Version { - constraint, err := semver.NewConstraint(dep.Version) - if err != nil { - return "invalid version" - } + // Fall through and look for directories + } else if l > 1 { + return "too many matches" + } - v, err := semver.NewVersion(c.Metadata.Version) - if err != nil { - return "invalid version" - } + // The sanest thing to do here is to fall through and see if we have any directory + // matches. - if !constraint.Check(v) { - return "wrong version" - } - } - return "ok" + case len(archives) == 1: + archive := archives[0] + if r := statArchiveForStatus(archive, dep); r != "" { + return r } + } // End unnecessary code. @@ -137,6 +144,40 @@ func (d *Dependency) dependencyStatus(chartpath string, dep *chart.Dependency, p return "unpacked" } +// stat an archive and return a message if the stat is successful +// +// This is a refactor of the code originally in dependencyStatus. It is here to +// support legacy behavior, and should be removed in Helm 4. +func statArchiveForStatus(archive string, dep *chart.Dependency) string { + if _, err := os.Stat(archive); err == nil { + c, err := loader.Load(archive) + if err != nil { + return "corrupt" + } + if c.Name() != dep.Name { + return "misnamed" + } + + if c.Metadata.Version != dep.Version { + constraint, err := semver.NewConstraint(dep.Version) + if err != nil { + return "invalid version" + } + + v, err := semver.NewVersion(c.Metadata.Version) + if err != nil { + return "invalid version" + } + + if !constraint.Check(v) { + return "wrong version" + } + } + return "ok" + } + return "" +} + // printDependencies prints all of the dependencies in the yaml file. func (d *Dependency) printDependencies(chartpath string, out io.Writer, c *chart.Chart) { table := uitable.New() diff --git a/pkg/action/dependency_test.go b/pkg/action/dependency_test.go index 4f3cb69a5..b5032a377 100644 --- a/pkg/action/dependency_test.go +++ b/pkg/action/dependency_test.go @@ -18,9 +18,16 @@ package action import ( "bytes" + "io/ioutil" + "os" + "path/filepath" "testing" + "github.com/stretchr/testify/assert" + "helm.sh/helm/v3/internal/test" + "helm.sh/helm/v3/pkg/chart" + "helm.sh/helm/v3/pkg/chartutil" ) func TestList(t *testing.T) { @@ -56,3 +63,99 @@ func TestList(t *testing.T) { test.AssertGoldenBytes(t, buf.Bytes(), tcase.golden) } } + +// TestDependencyStatus_Dashes is a regression test to make sure that dashes in +// chart names do not cause resolution problems. +func TestDependencyStatus_Dashes(t *testing.T) { + // Make a temp dir + dir, err := ioutil.TempDir("", "helmtest-") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + chartpath := filepath.Join(dir, "charts") + if err := os.MkdirAll(chartpath, 0700); err != nil { + t.Fatal(err) + } + + // Add some fake charts + first := buildChart(withName("first-chart")) + _, err = chartutil.Save(first, chartpath) + if err != nil { + t.Fatal(err) + } + + second := buildChart(withName("first-chart-second-chart")) + _, err = chartutil.Save(second, chartpath) + if err != nil { + t.Fatal(err) + } + + dep := &chart.Dependency{ + Name: "first-chart", + Version: "0.1.0", + } + + // Now try to get the deps + stat := NewDependency().dependencyStatus(dir, dep, first) + if stat != "ok" { + t.Errorf("Unexpected status: %q", stat) + } +} + +func TestStatArchiveForStatus(t *testing.T) { + // Make a temp dir + dir, err := ioutil.TempDir("", "helmtest-") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + chartpath := filepath.Join(dir, "charts") + if err := os.MkdirAll(chartpath, 0700); err != nil { + t.Fatal(err) + } + + // unsaved chart + lilith := buildChart(withName("lilith")) + + // dep referring to chart + dep := &chart.Dependency{ + Name: "lilith", + Version: "1.2.3", + } + + is := assert.New(t) + + lilithpath := filepath.Join(chartpath, "lilith-1.2.3.tgz") + is.Empty(statArchiveForStatus(lilithpath, dep)) + + // save the chart (version 0.1.0, because that is the default) + where, err := chartutil.Save(lilith, chartpath) + is.NoError(err) + + // Should get "wrong version" because we asked for 1.2.3 and got 0.1.0 + is.Equal("wrong version", statArchiveForStatus(where, dep)) + + // Break version on dep + dep = &chart.Dependency{ + Name: "lilith", + Version: "1.2.3.4.5", + } + is.Equal("invalid version", statArchiveForStatus(where, dep)) + + // Break the name + dep = &chart.Dependency{ + Name: "lilith2", + Version: "1.2.3", + } + is.Equal("misnamed", statArchiveForStatus(where, dep)) + + // Now create the right version + dep = &chart.Dependency{ + Name: "lilith", + Version: "0.1.0", + } + is.Equal("ok", statArchiveForStatus(where, dep)) +} diff --git a/pkg/action/history.go b/pkg/action/history.go index a592745e9..f4043609c 100644 --- a/pkg/action/history.go +++ b/pkg/action/history.go @@ -19,6 +19,7 @@ package action import ( "github.com/pkg/errors" + "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/release" ) @@ -45,7 +46,7 @@ func (h *History) Run(name string) ([]*release.Release, error) { return nil, err } - if err := validateReleaseName(name); err != nil { + if err := chartutil.ValidateReleaseName(name); err != nil { return nil, errors.Errorf("release name is invalid: %s", name) } diff --git a/pkg/action/install.go b/pkg/action/install.go index 00fb208b0..9bfecd915 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -654,8 +654,8 @@ func (c *ChartPathOptions) LocateChart(name string, settings *cli.EnvSettings) ( dl.Verify = downloader.VerifyAlways } if c.RepoURL != "" { - chartURL, err := repo.FindChartInAuthRepoURL(c.RepoURL, c.Username, c.Password, name, version, - c.CertFile, c.KeyFile, c.CaFile, getter.All(settings)) + chartURL, err := repo.FindChartInAuthAndTLSRepoURL(c.RepoURL, c.Username, c.Password, name, version, + c.CertFile, c.KeyFile, c.CaFile, c.InsecureSkipTLSverify, getter.All(settings)) if err != nil { return "", err } diff --git a/pkg/action/package.go b/pkg/action/package.go index 0a927cd41..8f53bcac4 100644 --- a/pkg/action/package.go +++ b/pkg/action/package.go @@ -17,6 +17,7 @@ limitations under the License. package action import ( + "bufio" "fmt" "io/ioutil" "os" @@ -39,6 +40,7 @@ type Package struct { Sign bool Key string Keyring string + PassphraseFile string Version string AppVersion string Destination string @@ -120,7 +122,15 @@ func (p *Package) Clearsign(filename string) error { return err } - if err := signer.DecryptKey(promptUser); err != nil { + passphraseFetcher := promptUser + if p.PassphraseFile != "" { + passphraseFetcher, err = passphraseFileFetcher(p.PassphraseFile, os.Stdin) + if err != nil { + return err + } + } + + if err := signer.DecryptKey(passphraseFetcher); err != nil { return err } @@ -141,3 +151,34 @@ func promptUser(name string) ([]byte, error) { fmt.Println() return pw, err } + +func passphraseFileFetcher(passphraseFile string, stdin *os.File) (provenance.PassphraseFetcher, error) { + file, err := openPassphraseFile(passphraseFile, stdin) + if err != nil { + return nil, err + } + defer file.Close() + + reader := bufio.NewReader(file) + passphrase, _, err := reader.ReadLine() + if err != nil { + return nil, err + } + return func(name string) ([]byte, error) { + return passphrase, nil + }, nil +} + +func openPassphraseFile(passphraseFile string, stdin *os.File) (*os.File, error) { + if passphraseFile == "-" { + stat, err := stdin.Stat() + if err != nil { + return nil, err + } + if (stat.Mode() & os.ModeNamedPipe) == 0 { + return nil, errors.New("specified reading passphrase from stdin, without input on stdin") + } + return stdin, nil + } + return os.Open(passphraseFile) +} diff --git a/pkg/action/package_test.go b/pkg/action/package_test.go index 0f716118d..9a202cde4 100644 --- a/pkg/action/package_test.go +++ b/pkg/action/package_test.go @@ -17,8 +17,12 @@ limitations under the License. package action import ( + "io/ioutil" + "os" + "path" "testing" + "helm.sh/helm/v3/internal/test/ensure" "helm.sh/helm/v3/pkg/chart" ) @@ -42,3 +46,57 @@ func TestSetVersion(t *testing.T) { t.Error("Expected bogus version to return an error.") } } + +func TestPassphraseFileFetcher(t *testing.T) { + secret := "secret" + directory := ensure.TempFile(t, "passphrase-file", []byte(secret)) + defer os.RemoveAll(directory) + + fetcher, err := passphraseFileFetcher(path.Join(directory, "passphrase-file"), nil) + if err != nil { + t.Fatal("Unable to create passphraseFileFetcher", err) + } + + passphrase, err := fetcher("key") + if err != nil { + t.Fatal("Unable to fetch passphrase") + } + + if string(passphrase) != secret { + t.Errorf("Expected %s got %s", secret, string(passphrase)) + } +} + +func TestPassphraseFileFetcher_WithLineBreak(t *testing.T) { + secret := "secret" + directory := ensure.TempFile(t, "passphrase-file", []byte(secret+"\n\n.")) + defer os.RemoveAll(directory) + + fetcher, err := passphraseFileFetcher(path.Join(directory, "passphrase-file"), nil) + if err != nil { + t.Fatal("Unable to create passphraseFileFetcher", err) + } + + passphrase, err := fetcher("key") + if err != nil { + t.Fatal("Unable to fetch passphrase") + } + + if string(passphrase) != secret { + t.Errorf("Expected %s got %s", secret, string(passphrase)) + } +} + +func TestPassphraseFileFetcher_WithInvalidStdin(t *testing.T) { + directory := ensure.TempDir(t) + defer os.RemoveAll(directory) + + stdin, err := ioutil.TempFile(directory, "non-existing") + if err != nil { + t.Fatal("Unable to create test file", err) + } + + if _, err := passphraseFileFetcher("-", stdin); err == nil { + t.Error("Expected passphraseFileFetcher returning an error") + } +} diff --git a/pkg/action/pull.go b/pkg/action/pull.go index a46e98bae..220ca11b2 100644 --- a/pkg/action/pull.go +++ b/pkg/action/pull.go @@ -89,7 +89,7 @@ func (p *Pull) Run(chartRef string) (string, error) { } if p.RepoURL != "" { - chartURL, err := repo.FindChartInAuthRepoURL(p.RepoURL, p.Username, p.Password, chartRef, p.Version, p.CertFile, p.KeyFile, p.CaFile, getter.All(p.Settings)) + chartURL, err := repo.FindChartInAuthAndTLSRepoURL(p.RepoURL, p.Username, p.Password, chartRef, p.Version, p.CertFile, p.KeyFile, p.CaFile, p.InsecureSkipTLSverify, getter.All(p.Settings)) if err != nil { return out.String(), err } diff --git a/pkg/action/release_testing.go b/pkg/action/release_testing.go index 795c3c747..2f6f5cfce 100644 --- a/pkg/action/release_testing.go +++ b/pkg/action/release_testing.go @@ -25,6 +25,7 @@ import ( "github.com/pkg/errors" v1 "k8s.io/api/core/v1" + "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/release" ) @@ -51,7 +52,7 @@ func (r *ReleaseTesting) Run(name string) (*release.Release, error) { return nil, err } - if err := validateReleaseName(name); err != nil { + if err := chartutil.ValidateReleaseName(name); err != nil { return nil, errors.Errorf("releaseTest: Release name is invalid: %s", name) } diff --git a/pkg/action/rollback.go b/pkg/action/rollback.go index 8773b6271..542acefae 100644 --- a/pkg/action/rollback.go +++ b/pkg/action/rollback.go @@ -24,6 +24,7 @@ import ( "github.com/pkg/errors" + "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/release" helmtime "helm.sh/helm/v3/pkg/time" ) @@ -90,7 +91,7 @@ func (r *Rollback) Run(name string) error { // prepareRollback finds the previous release and prepares a new release object with // the previous release's configuration func (r *Rollback) prepareRollback(name string) (*release.Release, *release.Release, error) { - if err := validateReleaseName(name); err != nil { + if err := chartutil.ValidateReleaseName(name); err != nil { return nil, nil, errors.Errorf("prepareRollback: Release name is invalid: %s", name) } diff --git a/pkg/action/uninstall.go b/pkg/action/uninstall.go index a51a283d6..c466c6ee2 100644 --- a/pkg/action/uninstall.go +++ b/pkg/action/uninstall.go @@ -22,6 +22,7 @@ import ( "github.com/pkg/errors" + "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/release" "helm.sh/helm/v3/pkg/releaseutil" helmtime "helm.sh/helm/v3/pkg/time" @@ -62,7 +63,7 @@ func (u *Uninstall) Run(name string) (*release.UninstallReleaseResponse, error) return &release.UninstallReleaseResponse{Release: r}, nil } - if err := validateReleaseName(name); err != nil { + if err := chartutil.ValidateReleaseName(name); err != nil { return nil, errors.Errorf("uninstall: Release name is invalid: %s", name) } diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index b707e7e69..c439af79d 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -115,7 +115,7 @@ func (u *Upgrade) Run(name string, chart *chart.Chart, vals map[string]interface // the user doesn't have to specify both u.Wait = u.Wait || u.Atomic - if err := validateReleaseName(name); err != nil { + if err := chartutil.ValidateReleaseName(name); err != nil { return nil, errors.Errorf("release name is invalid: %s", name) } u.cfg.Log("preparing upgrade for %s", name) @@ -142,19 +142,6 @@ func (u *Upgrade) Run(name string, chart *chart.Chart, vals map[string]interface return res, nil } -func validateReleaseName(releaseName string) error { - if releaseName == "" { - return errMissingRelease - } - - // Check length first, since that is a less expensive operation. - if len(releaseName) > releaseNameMaxLen || !ValidName.MatchString(releaseName) { - return errInvalidName - } - - return nil -} - // prepareUpgrade builds an upgraded release for an upgrade operation. func (u *Upgrade) prepareUpgrade(name string, chart *chart.Chart, vals map[string]interface{}) (*release.Release, *release.Release, error) { if chart == nil { diff --git a/pkg/chart/chart.go b/pkg/chart/chart.go index bd75375a4..a3bed63a3 100644 --- a/pkg/chart/chart.go +++ b/pkg/chart/chart.go @@ -17,6 +17,7 @@ package chart import ( "path/filepath" + "regexp" "strings" ) @@ -26,6 +27,9 @@ const APIVersionV1 = "v1" // APIVersionV2 is the API version number for version 2. const APIVersionV2 = "v2" +// aliasNameFormat defines the characters that are legal in an alias name. +var aliasNameFormat = regexp.MustCompile("^[a-zA-Z0-9_-]+$") + // Chart is a helm package that contains metadata, a default config, zero or more // optionally parameterizable templates, and zero or more charts (dependencies). type Chart struct { diff --git a/pkg/chart/errors.go b/pkg/chart/errors.go index 4cb4189e6..2fad5f370 100644 --- a/pkg/chart/errors.go +++ b/pkg/chart/errors.go @@ -15,9 +15,16 @@ limitations under the License. package chart +import "fmt" + // ValidationError represents a data validation error. type ValidationError string func (v ValidationError) Error() string { return "validation: " + string(v) } + +// ValidationErrorf takes a message and formatting options and creates a ValidationError +func ValidationErrorf(msg string, args ...interface{}) ValidationError { + return ValidationError(fmt.Sprintf(msg, args...)) +} diff --git a/pkg/chart/metadata.go b/pkg/chart/metadata.go index 96a3965b9..1848eb280 100644 --- a/pkg/chart/metadata.go +++ b/pkg/chart/metadata.go @@ -81,6 +81,15 @@ func (md *Metadata) Validate() error { if !isValidChartType(md.Type) { return ValidationError("chart.metadata.type must be application or library") } + + // 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 { + return err + } + } + // TODO validate valid semver here? return nil } @@ -92,3 +101,13 @@ 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 +} diff --git a/pkg/chart/metadata_test.go b/pkg/chart/metadata_test.go index 8b436000b..0c7b173dd 100644 --- a/pkg/chart/metadata_test.go +++ b/pkg/chart/metadata_test.go @@ -48,12 +48,60 @@ func TestValidate(t *testing.T) { &Metadata{Name: "test", APIVersion: "v2", Version: "1.0", Type: "application"}, nil, }, + { + &Metadata{ + Name: "test", + APIVersion: "v2", + Version: "1.0", + Type: "application", + Dependencies: []*Dependency{ + {Name: "dependency", Alias: "legal-alias"}, + }, + }, + nil, + }, + { + &Metadata{ + Name: "test", + APIVersion: "v2", + Version: "1.0", + Type: "application", + Dependencies: []*Dependency{ + {Name: "bad", Alias: "illegal alias"}, + }, + }, + ValidationError("dependency \"bad\" has disallowed characters in the alias"), + }, } for _, tt := range tests { result := tt.md.Validate() if result != tt.err { - t.Errorf("expected %s, got %s", tt.err, result) + t.Errorf("expected '%s', got '%s'", tt.err, result) + } + } +} + +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 := 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) } } } diff --git a/pkg/chartutil/create.go b/pkg/chartutil/create.go index 8d8f48176..86681247e 100644 --- a/pkg/chartutil/create.go +++ b/pkg/chartutil/create.go @@ -18,9 +18,11 @@ package chartutil import ( "fmt" + "io" "io/ioutil" "os" "path/filepath" + "regexp" "strings" "github.com/pkg/errors" @@ -30,6 +32,12 @@ import ( "helm.sh/helm/v3/pkg/chart/loader" ) +// chartName is a regular expression for testing the supplied name of a chart. +// This regular expression is probably stricter than it needs to be. We can relax it +// somewhat. Newline characters, as well as $, quotes, +, parens, and % are known to be +// problematic. +var chartName = regexp.MustCompile("^[a-zA-Z0-9._-]+$") + const ( // ChartfileName is the default Chart file name. ChartfileName = "Chart.yaml" @@ -63,6 +71,10 @@ const ( TestConnectionName = TemplatesTestsDir + sep + "test-connection.yaml" ) +// maxChartNameLength is lower than the limits we know of with certain file systems, +// and with certain Kubernetes fields. +const maxChartNameLength = 250 + const sep = string(filepath.Separator) const defaultChartfile = `apiVersion: v2 @@ -425,7 +437,9 @@ Common labels {{- define ".labels" -}} helm.sh/chart: {{ include ".chart" . }} {{ include ".selectorLabels" . }} -app.kubernetes.io/version: {{ .Values.image.tag | default .Chart.AppVersion | quote }} +{{- if .Chart.AppVersion }} +app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} +{{- end }} app.kubernetes.io/managed-by: {{ .Release.Service }} {{- end }} @@ -466,6 +480,12 @@ spec: restartPolicy: Never ` +// Stderr is an io.Writer to which error messages can be written +// +// In Helm 4, this will be replaced. It is needed in Helm 3 to preserve API backward +// compatibility. +var Stderr io.Writer = os.Stderr + // CreateFrom creates a new chart, but scaffolds it from the src chart. func CreateFrom(chartfile *chart.Metadata, dest, src string) error { schart, err := loader.Load(src) @@ -520,6 +540,12 @@ func CreateFrom(chartfile *chart.Metadata, dest, src string) error { // error. In such a case, this will attempt to clean up by removing the // new chart directory. func Create(name, dir string) (string, error) { + + // Sanity-check the name of a chart so user doesn't create one that causes problems. + if err := validateChartName(name); err != nil { + return "", err + } + path, err := filepath.Abs(dir) if err != nil { return path, err @@ -599,8 +625,8 @@ func Create(name, dir string) (string, error) { for _, file := range files { if _, err := os.Stat(file.path); err == nil { - // File exists and is okay. Skip it. - continue + // There is no handle to a preferred output stream here. + fmt.Fprintf(Stderr, "WARNING: File %q already exists. Overwriting.\n", file.path) } if err := writeFile(file.path, file.content); err != nil { return cdir, err @@ -625,3 +651,13 @@ func writeFile(name string, content []byte) error { } return ioutil.WriteFile(name, content, 0644) } + +func validateChartName(name string) error { + if name == "" || len(name) > maxChartNameLength { + return fmt.Errorf("chart name must be between 1 and %d characters", maxChartNameLength) + } + if !chartName.MatchString(name) { + return fmt.Errorf("chart name must match the regular expression %q", chartName.String()) + } + return nil +} diff --git a/pkg/chartutil/create_test.go b/pkg/chartutil/create_test.go index a11c45140..9a473fc59 100644 --- a/pkg/chartutil/create_test.go +++ b/pkg/chartutil/create_test.go @@ -117,3 +117,69 @@ func TestCreateFrom(t *testing.T) { } } } + +// TestCreate_Overwrite is a regression test for making sure that files are overwritten. +func TestCreate_Overwrite(t *testing.T) { + tdir, err := ioutil.TempDir("", "helm-") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tdir) + + var errlog bytes.Buffer + + if _, err := Create("foo", tdir); err != nil { + t.Fatal(err) + } + + dir := filepath.Join(tdir, "foo") + + tplname := filepath.Join(dir, "templates/hpa.yaml") + writeFile(tplname, []byte("FOO")) + + // Now re-run the create + Stderr = &errlog + if _, err := Create("foo", tdir); err != nil { + t.Fatal(err) + } + + data, err := ioutil.ReadFile(tplname) + if err != nil { + t.Fatal(err) + } + + if string(data) == "FOO" { + t.Fatal("File that should have been modified was not.") + } + + if errlog.Len() == 0 { + t.Errorf("Expected warnings about overwriting files.") + } +} + +func TestValidateChartName(t *testing.T) { + for name, shouldPass := range map[string]bool{ + "": false, + "abcdefghijklmnopqrstuvwxyz-_.": true, + "ABCDEFGHIJKLMNOPQRSTUVWXYZ-_.": true, + "$hello": false, + "Hellô": false, + "he%%o": false, + "he\nllo": false, + + "abcdefghijklmnopqrstuvwxyz-_." + + "abcdefghijklmnopqrstuvwxyz-_." + + "abcdefghijklmnopqrstuvwxyz-_." + + "abcdefghijklmnopqrstuvwxyz-_." + + "abcdefghijklmnopqrstuvwxyz-_." + + "abcdefghijklmnopqrstuvwxyz-_." + + "abcdefghijklmnopqrstuvwxyz-_." + + "abcdefghijklmnopqrstuvwxyz-_." + + "abcdefghijklmnopqrstuvwxyz-_." + + "ABCDEFGHIJKLMNOPQRSTUVWXYZ-_.": false, + } { + if err := validateChartName(name); (err != nil) == shouldPass { + t.Errorf("test for %q failed", name) + } + } +} diff --git a/pkg/chartutil/validate_name.go b/pkg/chartutil/validate_name.go new file mode 100644 index 000000000..22132c80e --- /dev/null +++ b/pkg/chartutil/validate_name.go @@ -0,0 +1,99 @@ +/* +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 chartutil + +import ( + "regexp" + + "github.com/pkg/errors" +) + +// validName is a regular expression for resource names. +// +// According to the Kubernetes help text, the regular expression it uses is: +// +// [a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)* +// +// This follows the above regular expression (but requires a full string match, not partial). +// +// The Kubernetes documentation is here, though it is not entirely correct: +// https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names +var validName = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`) + +var ( + // errMissingName indicates that a release (name) was not provided. + errMissingName = errors.New("no name provided") + + // errInvalidName indicates that an invalid release name was provided + errInvalidName = errors.New("invalid release name, must match regex ^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])+$ and the length must not longer than 53") + + // errInvalidKubernetesName indicates that the name does not meet the Kubernetes + // restrictions on metadata names. + errInvalidKubernetesName = errors.New("invalid metadata name, must match regex ^(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])+$ and the length must not longer than 253") +) + +const ( + // maxNameLen is the maximum length Helm allows for a release name + maxReleaseNameLen = 53 + // maxMetadataNameLen is the maximum length Kubernetes allows for any name. + maxMetadataNameLen = 253 +) + +// ValidateReleaseName performs checks for an entry for a Helm release name +// +// For Helm to allow a name, it must be below a certain character count (53) and also match +// a reguar expression. +// +// According to the Kubernetes help text, the regular expression it uses is: +// +// [a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)* +// +// This follows the above regular expression (but requires a full string match, not partial). +// +// The Kubernetes documentation is here, though it is not entirely correct: +// https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names +func ValidateReleaseName(name string) error { + // This case is preserved for backwards compatibility + if name == "" { + return errMissingName + + } + if len(name) > maxReleaseNameLen || !validName.MatchString(name) { + return errInvalidName + } + return nil +} + +// ValidateMetadataName validates the name field of a Kubernetes metadata object. +// +// Empty strings, strings longer than 253 chars, or strings that don't match the regexp +// will fail. +// +// According to the Kubernetes help text, the regular expression it uses is: +// +// [a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)* +// +// This follows the above regular expression (but requires a full string match, not partial). +// +// The Kubernetes documentation is here, though it is not entirely correct: +// https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names +func ValidateMetadataName(name string) error { + if name == "" || len(name) > maxMetadataNameLen || !validName.MatchString(name) { + return errInvalidKubernetesName + } + return nil +} diff --git a/pkg/chartutil/validate_name_test.go b/pkg/chartutil/validate_name_test.go new file mode 100644 index 000000000..5f0792f94 --- /dev/null +++ b/pkg/chartutil/validate_name_test.go @@ -0,0 +1,91 @@ +/* +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 chartutil + +import "testing" + +// TestValidateName is a regression test for ValidateName +// +// Kubernetes has strict naming conventions for resource names. This test represents +// those conventions. +// +// See https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names +// +// NOTE: At the time of this writing, the docs above say that names cannot begin with +// digits. However, `kubectl`'s regular expression explicit allows this, and +// Kubernetes (at least as of 1.18) also accepts resources whose names begin with digits. +func TestValidateReleaseName(t *testing.T) { + names := map[string]bool{ + "": false, + "foo": true, + "foo.bar1234baz.seventyone": true, + "FOO": false, + "123baz": true, + "foo.BAR.baz": false, + "one-two": true, + "-two": false, + "one_two": false, + "a..b": false, + "%^&#$%*@^*@&#^": false, + "example:com": false, + "example%%com": false, + "a1111111111111111111111111111111111111111111111111111111111z": false, + } + for input, expectPass := range names { + if err := ValidateReleaseName(input); (err == nil) != expectPass { + st := "fail" + if expectPass { + st = "succeed" + } + t.Errorf("Expected %q to %s", input, st) + } + } +} + +func TestValidateMetadataName(t *testing.T) { + names := map[string]bool{ + "": false, + "foo": true, + "foo.bar1234baz.seventyone": true, + "FOO": false, + "123baz": true, + "foo.BAR.baz": false, + "one-two": true, + "-two": false, + "one_two": false, + "a..b": false, + "%^&#$%*@^*@&#^": false, + "example:com": false, + "example%%com": false, + "a1111111111111111111111111111111111111111111111111111111111z": true, + "a1111111111111111111111111111111111111111111111111111111111z" + + "a1111111111111111111111111111111111111111111111111111111111z" + + "a1111111111111111111111111111111111111111111111111111111111z" + + "a1111111111111111111111111111111111111111111111111111111111z" + + "a1111111111111111111111111111111111111111111111111111111111z" + + "a1111111111111111111111111111111111111111111111111111111111z": false, + } + for input, expectPass := range names { + if err := ValidateMetadataName(input); (err == nil) != expectPass { + st := "fail" + if expectPass { + st = "succeed" + } + t.Errorf("Expected %q to %s", input, st) + } + } +} diff --git a/pkg/cli/environment.go b/pkg/cli/environment.go index a9994f03d..4f3abc08b 100644 --- a/pkg/cli/environment.go +++ b/pkg/cli/environment.go @@ -26,6 +26,7 @@ import ( "fmt" "os" "strconv" + "strings" "github.com/spf13/pflag" "k8s.io/cli-runtime/pkg/genericclioptions" @@ -47,6 +48,10 @@ type EnvSettings struct { KubeContext string // Bearer KubeToken used for authentication KubeToken string + // Username to impersonate for the operation + KubeAsUser string + // Groups to impersonate for the operation, multiple groups parsed from a comma delimited list + KubeAsGroups []string // Kubernetes API Server Endpoint for authentication KubeAPIServer string // Debug indicates whether or not Helm is running in Debug mode. @@ -69,6 +74,8 @@ func New() *EnvSettings { MaxHistory: envIntOr("HELM_MAX_HISTORY", defaultMaxHistory), KubeContext: os.Getenv("HELM_KUBECONTEXT"), KubeToken: os.Getenv("HELM_KUBETOKEN"), + KubeAsUser: os.Getenv("HELM_KUBEASUSER"), + KubeAsGroups: envCSV("HELM_KUBEASGROUPS"), KubeAPIServer: os.Getenv("HELM_KUBEAPISERVER"), PluginsDirectory: envOr("HELM_PLUGINS", helmpath.DataPath("plugins")), RegistryConfig: envOr("HELM_REGISTRY_CONFIG", helmpath.ConfigPath("registry.json")), @@ -79,11 +86,13 @@ func New() *EnvSettings { // bind to kubernetes config flags env.config = &genericclioptions.ConfigFlags{ - Namespace: &env.namespace, - Context: &env.KubeContext, - BearerToken: &env.KubeToken, - APIServer: &env.KubeAPIServer, - KubeConfig: &env.KubeConfig, + Namespace: &env.namespace, + Context: &env.KubeContext, + BearerToken: &env.KubeToken, + APIServer: &env.KubeAPIServer, + KubeConfig: &env.KubeConfig, + Impersonate: &env.KubeAsUser, + ImpersonateGroup: &env.KubeAsGroups, } return env } @@ -94,6 +103,8 @@ func (s *EnvSettings) AddFlags(fs *pflag.FlagSet) { fs.StringVar(&s.KubeConfig, "kubeconfig", "", "path to the kubeconfig file") fs.StringVar(&s.KubeContext, "kube-context", s.KubeContext, "name of the kubeconfig context to use") fs.StringVar(&s.KubeToken, "kube-token", s.KubeToken, "bearer token used for authentication") + fs.StringVar(&s.KubeAsUser, "kube-as-user", s.KubeAsUser, "Username to impersonate for the operation") + fs.StringArrayVar(&s.KubeAsGroups, "kube-as-group", s.KubeAsGroups, "Group to impersonate for the operation, this flag can be repeated to specify multiple groups.") fs.StringVar(&s.KubeAPIServer, "kube-apiserver", s.KubeAPIServer, "the address and the port for the Kubernetes API server") fs.BoolVar(&s.Debug, "debug", s.Debug, "enable verbose output") fs.StringVar(&s.RegistryConfig, "registry-config", s.RegistryConfig, "path to the registry config file") @@ -120,6 +131,14 @@ func envIntOr(name string, def int) int { return ret } +func envCSV(name string) (ls []string) { + trimmed := strings.Trim(os.Getenv(name), ", ") + if trimmed != "" { + ls = strings.Split(trimmed, ",") + } + return +} + func (s *EnvSettings) EnvVars() map[string]string { envvars := map[string]string{ "HELM_BIN": os.Args[0], @@ -137,6 +156,8 @@ func (s *EnvSettings) EnvVars() map[string]string { // broken, these are populated from helm flags and not kubeconfig. "HELM_KUBECONTEXT": s.KubeContext, "HELM_KUBETOKEN": s.KubeToken, + "HELM_KUBEASUSER": s.KubeAsUser, + "HELM_KUBEASGROUPS": strings.Join(s.KubeAsGroups, ","), "HELM_KUBEAPISERVER": s.KubeAPIServer, } if s.KubeConfig != "" { diff --git a/pkg/cli/environment_test.go b/pkg/cli/environment_test.go index 3234a133b..ffdbce68b 100644 --- a/pkg/cli/environment_test.go +++ b/pkg/cli/environment_test.go @@ -18,6 +18,7 @@ package cli import ( "os" + "reflect" "strings" "testing" @@ -36,6 +37,8 @@ func TestEnvSettings(t *testing.T) { ns, kcontext string debug bool maxhistory int + kAsUser string + kAsGroups []string }{ { name: "defaults", @@ -44,25 +47,31 @@ func TestEnvSettings(t *testing.T) { }, { name: "with flags set", - args: "--debug --namespace=myns", + args: "--debug --namespace=myns --kube-as-user=poro --kube-as-group=admins --kube-as-group=teatime --kube-as-group=snackeaters", ns: "myns", debug: true, maxhistory: defaultMaxHistory, + kAsUser: "poro", + kAsGroups: []string{"admins", "teatime", "snackeaters"}, }, { name: "with envvars set", - envvars: map[string]string{"HELM_DEBUG": "1", "HELM_NAMESPACE": "yourns", "HELM_MAX_HISTORY": "5"}, + envvars: map[string]string{"HELM_DEBUG": "1", "HELM_NAMESPACE": "yourns", "HELM_KUBEASUSER": "pikachu", "HELM_KUBEASGROUPS": ",,,operators,snackeaters,partyanimals", "HELM_MAX_HISTORY": "5"}, ns: "yourns", maxhistory: 5, debug: true, + kAsUser: "pikachu", + kAsGroups: []string{"operators", "snackeaters", "partyanimals"}, }, { name: "with flags and envvars set", - args: "--debug --namespace=myns", - envvars: map[string]string{"HELM_DEBUG": "1", "HELM_NAMESPACE": "yourns"}, + args: "--debug --namespace=myns --kube-as-user=poro --kube-as-group=admins --kube-as-group=teatime --kube-as-group=snackeaters", + envvars: map[string]string{"HELM_DEBUG": "1", "HELM_NAMESPACE": "yourns", "HELM_KUBEASUSER": "pikachu", "HELM_KUBEASGROUPS": ",,,operators,snackeaters,partyanimals", "HELM_MAX_HISTORY": "5"}, ns: "myns", debug: true, - maxhistory: defaultMaxHistory, + maxhistory: 5, + kAsUser: "poro", + kAsGroups: []string{"admins", "teatime", "snackeaters"}, }, } @@ -92,6 +101,12 @@ func TestEnvSettings(t *testing.T) { if settings.MaxHistory != tt.maxhistory { t.Errorf("expected maxHistory %d, got %d", tt.maxhistory, settings.MaxHistory) } + if tt.kAsUser != settings.KubeAsUser { + t.Errorf("expected kAsUser %q, got %q", tt.kAsUser, settings.KubeAsUser) + } + if !reflect.DeepEqual(tt.kAsGroups, settings.KubeAsGroups) { + t.Errorf("expected kAsGroups %+v, got %+v", len(tt.kAsGroups), len(settings.KubeAsGroups)) + } }) } } diff --git a/pkg/downloader/chart_downloader_test.go b/pkg/downloader/chart_downloader_test.go index abfb007ff..b9fd3bf87 100644 --- a/pkg/downloader/chart_downloader_test.go +++ b/pkg/downloader/chart_downloader_test.go @@ -71,7 +71,7 @@ func TestResolveChartRef(t *testing.T) { if tt.fail { continue } - t.Errorf("%s: failed with error %s", tt.name, err) + t.Errorf("%s: failed with error %q", tt.name, err) continue } if got := u.String(); got != tt.expect { @@ -172,7 +172,7 @@ func TestIsTar(t *testing.T) { func TestDownloadTo(t *testing.T) { // Set up a fake repo with basic auth enabled - srv, err := repotest.NewTempServer("testdata/*.tgz*") + srv, err := repotest.NewTempServerWithCleanup(t, "testdata/*.tgz*") srv.Stop() if err != nil { t.Fatal(err) @@ -229,7 +229,7 @@ func TestDownloadTo(t *testing.T) { func TestDownloadTo_TLS(t *testing.T) { // Set up mock server w/ tls enabled - srv, err := repotest.NewTempServer("testdata/*.tgz*") + srv, err := repotest.NewTempServerWithCleanup(t, "testdata/*.tgz*") srv.Stop() if err != nil { t.Fatal(err) @@ -285,7 +285,7 @@ func TestDownloadTo_VerifyLater(t *testing.T) { dest := ensure.TempDir(t) // Set up a fake repo - srv, err := repotest.NewTempServer("testdata/*.tgz*") + srv, err := repotest.NewTempServerWithCleanup(t, "testdata/*.tgz*") if err != nil { t.Fatal(err) } diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index e60cf7624..9532cca7c 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -183,7 +183,7 @@ func TestGetRepoNames(t *testing.T) { func TestUpdateBeforeBuild(t *testing.T) { // Set up a fake repo - srv, err := repotest.NewTempServer("testdata/*.tgz*") + srv, err := repotest.NewTempServerWithCleanup(t, "testdata/*.tgz*") if err != nil { t.Fatal(err) } @@ -257,7 +257,7 @@ func TestUpdateBeforeBuild(t *testing.T) { // If each of these main fields (name, version, repository) is not supplied by dep param, default value will be used. func checkBuildWithOptionalFields(t *testing.T, chartName string, dep chart.Dependency) { // Set up a fake repo - srv, err := repotest.NewTempServer("testdata/*.tgz*") + srv, err := repotest.NewTempServerWithCleanup(t, "testdata/*.tgz*") if err != nil { t.Fatal(err) } diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 5aa0ed8ec..155d50a38 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -40,7 +40,7 @@ type Engine struct { Strict bool // In LintMode, some 'required' template values may be missing, so don't fail LintMode bool - // the rest config to connect to te kubernetes api + // the rest config to connect to the kubernetes api config *rest.Config } diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go index 568afa094..de5358aee 100644 --- a/pkg/kube/client_test.go +++ b/pkg/kube/client_test.go @@ -91,9 +91,12 @@ func newResponse(code int, obj runtime.Object) (*http.Response, error) { return &http.Response{StatusCode: code, Header: header, Body: body}, nil } -func newTestClient() *Client { +func newTestClient(t *testing.T) *Client { + testFactory := cmdtesting.NewTestFactory() + t.Cleanup(testFactory.Cleanup) + return &Client{ - Factory: cmdtesting.NewTestFactory().WithNamespace("default"), + Factory: testFactory.WithNamespace("default"), Log: nopLogger, } } @@ -107,7 +110,7 @@ func TestUpdate(t *testing.T) { var actions []string - c := newTestClient() + c := newTestClient(t) c.Factory.(*cmdtesting.TestFactory).UnstructuredClient = &fake.RESTClient{ NegotiatedSerializer: unstructuredSerializer, Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) { @@ -232,7 +235,7 @@ func TestBuild(t *testing.T) { }, } - c := newTestClient() + c := newTestClient(t) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Test for an invalid manifest @@ -279,7 +282,7 @@ func TestPerform(t *testing.T) { return nil } - c := newTestClient() + c := newTestClient(t) infos, err := c.Build(tt.reader, false) if err != nil && err.Error() != tt.errMessage { t.Errorf("Error while building manifests: %v", err) diff --git a/pkg/lint/rules/template.go b/pkg/lint/rules/template.go index 73d645264..5de0819c4 100644 --- a/pkg/lint/rules/template.go +++ b/pkg/lint/rules/template.go @@ -40,14 +40,6 @@ var ( releaseTimeSearch = regexp.MustCompile(`\.Release\.Time`) ) -// validName is a regular expression for names. -// -// This is different than action.ValidName. It conforms to the regular expression -// `kubectl` says it uses, plus it disallows empty names. -// -// For details, see https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names -var validName = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`) - // Templates lints the templates in the Linter. func Templates(linter *support.Linter, values map[string]interface{}, namespace string, strict bool) { fpath := "templates/" @@ -199,10 +191,10 @@ func validateMetadataName(obj *K8sYamlStruct) error { } // This will return an error if the characters do not abide by the standard OR if the // name is left empty. - if validName.MatchString(obj.Metadata.Name) { - return nil + if err := chartutil.ValidateMetadataName(obj.Metadata.Name); err != nil { + return errors.Wrapf(err, "object name does not conform to Kubernetes naming requirements: %q", obj.Metadata.Name) } - return fmt.Errorf("object name does not conform to Kubernetes naming requirements: %q", obj.Metadata.Name) + return nil } func validateNoCRDHooks(manifest []byte) error { diff --git a/pkg/plugin/installer/http_installer.go b/pkg/plugin/installer/http_installer.go index 28e50b72b..bcbcbde93 100644 --- a/pkg/plugin/installer/http_installer.go +++ b/pkg/plugin/installer/http_installer.go @@ -59,6 +59,18 @@ var Extractors = map[string]Extractor{ ".tgz": &TarGzExtractor{}, } +// Convert a media type to an extractor extension. +// +// This should be refactored in Helm 4, combined with the extension-based mechanism. +func mediaTypeToExtension(mt string) (string, bool) { + switch strings.ToLower(mt) { + case "application/gzip", "application/x-gzip", "application/x-tgz", "application/x-gtar": + return ".tgz", true + default: + return "", false + } +} + // NewExtractor creates a new extractor matching the source file name func NewExtractor(source string) (Extractor, error) { for suffix, extractor := range Extractors { diff --git a/pkg/plugin/installer/http_installer_test.go b/pkg/plugin/installer/http_installer_test.go index 3eb92ee77..e89fea29d 100644 --- a/pkg/plugin/installer/http_installer_test.go +++ b/pkg/plugin/installer/http_installer_test.go @@ -20,9 +20,13 @@ import ( "bytes" "compress/gzip" "encoding/base64" + "fmt" "io/ioutil" + "net/http" + "net/http/httptest" "os" "path/filepath" + "strings" "syscall" "testing" @@ -63,9 +67,24 @@ func TestStripName(t *testing.T) { } } +func mockArchiveServer() *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !strings.HasSuffix(r.URL.Path, ".tar.gz") { + w.Header().Add("Content-Type", "text/html") + fmt.Fprintln(w, "broken") + return + } + w.Header().Add("Content-Type", "application/gzip") + fmt.Fprintln(w, "test") + })) +} + func TestHTTPInstaller(t *testing.T) { defer ensure.HelmHome(t)() - source := "https://repo.localdomain/plugins/fake-plugin-0.0.1.tar.gz" + + srv := mockArchiveServer() + defer srv.Close() + source := srv.URL + "/plugins/fake-plugin-0.0.1.tar.gz" if err := os.MkdirAll(helmpath.DataPath("plugins"), 0755); err != nil { t.Fatalf("Could not create %s: %s", helmpath.DataPath("plugins"), err) @@ -111,7 +130,9 @@ func TestHTTPInstaller(t *testing.T) { func TestHTTPInstallerNonExistentVersion(t *testing.T) { defer ensure.HelmHome(t)() - source := "https://repo.localdomain/plugins/fake-plugin-0.0.2.tar.gz" + srv := mockArchiveServer() + defer srv.Close() + source := srv.URL + "/plugins/fake-plugin-0.0.1.tar.gz" if err := os.MkdirAll(helmpath.DataPath("plugins"), 0755); err != nil { t.Fatalf("Could not create %s: %s", helmpath.DataPath("plugins"), err) @@ -141,7 +162,9 @@ func TestHTTPInstallerNonExistentVersion(t *testing.T) { } func TestHTTPInstallerUpdate(t *testing.T) { - source := "https://repo.localdomain/plugins/fake-plugin-0.0.1.tar.gz" + srv := mockArchiveServer() + defer srv.Close() + source := srv.URL + "/plugins/fake-plugin-0.0.1.tar.gz" defer ensure.HelmHome(t)() if err := os.MkdirAll(helmpath.DataPath("plugins"), 0755); err != nil { @@ -307,3 +330,26 @@ func TestCleanJoin(t *testing.T) { } } + +func TestMediaTypeToExtension(t *testing.T) { + + for mt, shouldPass := range map[string]bool{ + "": false, + "application/gzip": true, + "application/x-gzip": true, + "application/x-tgz": true, + "application/x-gtar": true, + "application/json": false, + } { + ext, ok := mediaTypeToExtension(mt) + if ok != shouldPass { + t.Errorf("Media type %q failed test", mt) + } + if shouldPass && ext == "" { + t.Errorf("Expected an extension but got empty string") + } + if !shouldPass && len(ext) != 0 { + t.Error("Expected extension to be empty for unrecognized type") + } + } +} diff --git a/pkg/plugin/installer/installer.go b/pkg/plugin/installer/installer.go index 61b49ab3b..6f01494e5 100644 --- a/pkg/plugin/installer/installer.go +++ b/pkg/plugin/installer/installer.go @@ -18,6 +18,7 @@ package installer import ( "fmt" "log" + "net/http" "os" "path/filepath" "strings" @@ -89,10 +90,29 @@ func isLocalReference(source string) bool { } // isRemoteHTTPArchive checks if the source is a http/https url and is an archive +// +// It works by checking whether the source looks like a URL and, if it does, running a +// HEAD operation to see if the remote resource is a file that we understand. func isRemoteHTTPArchive(source string) bool { if strings.HasPrefix(source, "http://") || strings.HasPrefix(source, "https://") { + res, err := http.Head(source) + if err != nil { + // If we get an error at the network layer, we can't install it. So + // we return false. + return false + } + + // Next, we look for the content type or content disposition headers to see + // if they have matching extractors. + contentType := res.Header.Get("content-type") + foundSuffix, ok := mediaTypeToExtension(contentType) + if !ok { + // Media type not recognized + return false + } + for suffix := range Extractors { - if strings.HasSuffix(source, suffix) { + if strings.HasSuffix(foundSuffix, suffix) { return true } } diff --git a/pkg/plugin/installer/installer_test.go b/pkg/plugin/installer/installer_test.go new file mode 100644 index 000000000..a11464924 --- /dev/null +++ b/pkg/plugin/installer/installer_test.go @@ -0,0 +1,40 @@ +/* +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 installer + +import "testing" + +func TestIsRemoteHTTPArchive(t *testing.T) { + srv := mockArchiveServer() + defer srv.Close() + source := srv.URL + "/plugins/fake-plugin-0.0.1.tar.gz" + + if isRemoteHTTPArchive("/not/a/URL") { + t.Errorf("Expected non-URL to return false") + } + + if isRemoteHTTPArchive("https://127.0.0.1:123/fake/plugin-1.2.3.tgz") { + t.Errorf("Bad URL should not have succeeded.") + } + + if !isRemoteHTTPArchive(source) { + t.Errorf("Expected %q to be a valid archive URL", source) + } + + if isRemoteHTTPArchive(source + "-not-an-extension") { + t.Error("Expected media type match to fail") + } +} diff --git a/pkg/plugin/installer/local_installer_test.go b/pkg/plugin/installer/local_installer_test.go index 3d9607331..96958ab09 100644 --- a/pkg/plugin/installer/local_installer_test.go +++ b/pkg/plugin/installer/local_installer_test.go @@ -37,7 +37,7 @@ func TestLocalInstaller(t *testing.T) { t.Fatal(err) } - source := "../testdata/plugdir/echo" + source := "../testdata/plugdir/good/echo" i, err := NewForSource(source, "") if err != nil { t.Fatalf("unexpected error: %s", err) diff --git a/pkg/plugin/installer/vcs_installer_test.go b/pkg/plugin/installer/vcs_installer_test.go index b8dc6b1e2..6785264b3 100644 --- a/pkg/plugin/installer/vcs_installer_test.go +++ b/pkg/plugin/installer/vcs_installer_test.go @@ -56,7 +56,7 @@ func TestVCSInstaller(t *testing.T) { } source := "https://github.com/adamreese/helm-env" - testRepoPath, _ := filepath.Abs("../testdata/plugdir/echo") + testRepoPath, _ := filepath.Abs("../testdata/plugdir/good/echo") repo := &testRepo{ local: testRepoPath, tags: []string{"0.1.0", "0.1.1"}, diff --git a/pkg/plugin/plugin.go b/pkg/plugin/plugin.go index caa34fbd3..93b5527a1 100644 --- a/pkg/plugin/plugin.go +++ b/pkg/plugin/plugin.go @@ -20,9 +20,11 @@ import ( "io/ioutil" "os" "path/filepath" + "regexp" "runtime" "strings" + "github.com/pkg/errors" "sigs.k8s.io/yaml" "helm.sh/helm/v3/pkg/cli" @@ -94,6 +96,12 @@ type Metadata struct { // Downloaders field is used if the plugin supply downloader mechanism // for special protocols. Downloaders []Downloaders `json:"downloaders"` + + // UseTunnelDeprecated indicates that this command needs a tunnel. + // Setting this will cause a number of side effects, such as the + // automatic setting of HELM_HOST. + // DEPRECATED and unused, but retained for backwards compatibility with Helm 2 plugins. Remove in Helm 4 + UseTunnelDeprecated bool `json:"useTunnel,omitempty"` } // Plugin represents a plugin. @@ -157,18 +165,51 @@ func (p *Plugin) PrepareCommand(extraArgs []string) (string, []string, error) { return main, baseArgs, nil } +// validPluginName is a regular expression that validates plugin names. +// +// Plugin names can only contain the ASCII characters a-z, A-Z, 0-9, ​_​ and ​-. +var validPluginName = regexp.MustCompile("^[A-Za-z0-9_-]+$") + +// validatePluginData validates a plugin's YAML data. +func validatePluginData(plug *Plugin, filepath string) error { + if !validPluginName.MatchString(plug.Metadata.Name) { + return fmt.Errorf("invalid plugin name at %q", filepath) + } + // We could also validate SemVer, executable, and other fields should we so choose. + return nil +} + +func detectDuplicates(plugs []*Plugin) error { + names := map[string]string{} + + for _, plug := range plugs { + if oldpath, ok := names[plug.Metadata.Name]; ok { + return fmt.Errorf( + "two plugins claim the name %q at %q and %q", + plug.Metadata.Name, + oldpath, + plug.Dir, + ) + } + names[plug.Metadata.Name] = plug.Dir + } + + return nil +} + // LoadDir loads a plugin from the given directory. func LoadDir(dirname string) (*Plugin, error) { - data, err := ioutil.ReadFile(filepath.Join(dirname, PluginFileName)) + pluginfile := filepath.Join(dirname, PluginFileName) + data, err := ioutil.ReadFile(pluginfile) if err != nil { - return nil, err + return nil, errors.Wrapf(err, "failed to read plugin at %q", pluginfile) } plug := &Plugin{Dir: dirname} - if err := yaml.Unmarshal(data, &plug.Metadata); err != nil { - return nil, err + if err := yaml.UnmarshalStrict(data, &plug.Metadata); err != nil { + return nil, errors.Wrapf(err, "failed to load plugin at %q", pluginfile) } - return plug, nil + return plug, validatePluginData(plug, pluginfile) } // LoadAll loads all plugins found beneath the base directory. @@ -180,7 +221,7 @@ func LoadAll(basedir string) ([]*Plugin, error) { scanpath := filepath.Join(basedir, "*", PluginFileName) matches, err := filepath.Glob(scanpath) if err != nil { - return plugins, err + return plugins, errors.Wrapf(err, "failed to find plugins in %q", scanpath) } if matches == nil { @@ -195,7 +236,7 @@ func LoadAll(basedir string) ([]*Plugin, error) { } plugins = append(plugins, p) } - return plugins, nil + return plugins, detectDuplicates(plugins) } // FindPlugins returns a list of YAML files that describe plugins. diff --git a/pkg/plugin/plugin_test.go b/pkg/plugin/plugin_test.go index af0b61846..2c4478953 100644 --- a/pkg/plugin/plugin_test.go +++ b/pkg/plugin/plugin_test.go @@ -16,6 +16,7 @@ limitations under the License. package plugin // import "helm.sh/helm/v3/pkg/plugin" import ( + "fmt" "os" "path/filepath" "reflect" @@ -177,7 +178,7 @@ func TestNoMatchPrepareCommand(t *testing.T) { } func TestLoadDir(t *testing.T) { - dirname := "testdata/plugdir/hello" + dirname := "testdata/plugdir/good/hello" plug, err := LoadDir(dirname) if err != nil { t.Fatalf("error loading Hello plugin: %s", err) @@ -204,8 +205,15 @@ func TestLoadDir(t *testing.T) { } } +func TestLoadDirDuplicateEntries(t *testing.T) { + dirname := "testdata/plugdir/bad/duplicate-entries" + if _, err := LoadDir(dirname); err == nil { + t.Errorf("successfully loaded plugin with duplicate entries when it should've failed") + } +} + func TestDownloader(t *testing.T) { - dirname := "testdata/plugdir/downloader" + dirname := "testdata/plugdir/good/downloader" plug, err := LoadDir(dirname) if err != nil { t.Fatalf("error loading Hello plugin: %s", err) @@ -243,7 +251,7 @@ func TestLoadAll(t *testing.T) { t.Fatalf("expected empty dir to have 0 plugins") } - basedir := "testdata/plugdir" + basedir := "testdata/plugdir/good" plugs, err := LoadAll(basedir) if err != nil { t.Fatalf("Could not load %q: %s", basedir, err) @@ -287,7 +295,7 @@ func TestFindPlugins(t *testing.T) { }, { name: "normal", - plugdirs: "./testdata/plugdir", + plugdirs: "./testdata/plugdir/good", expected: 3, }, } @@ -320,3 +328,51 @@ func TestSetupEnv(t *testing.T) { } } } + +func TestValidatePluginData(t *testing.T) { + for i, item := range []struct { + pass bool + plug *Plugin + }{ + {true, mockPlugin("abcdefghijklmnopqrstuvwxyz0123456789_-ABC")}, + {true, mockPlugin("foo-bar-FOO-BAR_1234")}, + {false, mockPlugin("foo -bar")}, + {false, mockPlugin("$foo -bar")}, // Test leading chars + {false, mockPlugin("foo -bar ")}, // Test trailing chars + {false, mockPlugin("foo\nbar")}, // Test newline + } { + err := validatePluginData(item.plug, fmt.Sprintf("test-%d", i)) + if item.pass && err != nil { + t.Errorf("failed to validate case %d: %s", i, err) + } else if !item.pass && err == nil { + t.Errorf("expected case %d to fail", i) + } + } +} + +func TestDetectDuplicates(t *testing.T) { + plugs := []*Plugin{ + mockPlugin("foo"), + mockPlugin("bar"), + } + if err := detectDuplicates(plugs); err != nil { + t.Error("no duplicates in the first set") + } + plugs = append(plugs, mockPlugin("foo")) + if err := detectDuplicates(plugs); err == nil { + t.Error("duplicates in the second set") + } +} + +func mockPlugin(name string) *Plugin { + return &Plugin{ + Metadata: &Metadata{ + Name: name, + Version: "v0.1.2", + Usage: "Mock plugin", + Description: "Mock plugin for testing", + Command: "echo mock plugin", + }, + Dir: "no-such-dir", + } +} diff --git a/pkg/plugin/testdata/plugdir/bad/duplicate-entries/plugin.yaml b/pkg/plugin/testdata/plugdir/bad/duplicate-entries/plugin.yaml new file mode 100644 index 000000000..66498be96 --- /dev/null +++ b/pkg/plugin/testdata/plugdir/bad/duplicate-entries/plugin.yaml @@ -0,0 +1,11 @@ +name: "duplicate-entries" +version: "0.1.0" +usage: "usage" +description: |- + description +command: "echo hello" +ignoreFlags: true +hooks: + install: "echo installing..." +hooks: + install: "echo installing something different" diff --git a/pkg/plugin/testdata/plugdir/downloader/plugin.yaml b/pkg/plugin/testdata/plugdir/good/downloader/plugin.yaml similarity index 100% rename from pkg/plugin/testdata/plugdir/downloader/plugin.yaml rename to pkg/plugin/testdata/plugdir/good/downloader/plugin.yaml diff --git a/pkg/plugin/testdata/plugdir/echo/plugin.yaml b/pkg/plugin/testdata/plugdir/good/echo/plugin.yaml similarity index 100% rename from pkg/plugin/testdata/plugdir/echo/plugin.yaml rename to pkg/plugin/testdata/plugdir/good/echo/plugin.yaml diff --git a/pkg/plugin/testdata/plugdir/hello/hello.sh b/pkg/plugin/testdata/plugdir/good/hello/hello.sh similarity index 100% rename from pkg/plugin/testdata/plugdir/hello/hello.sh rename to pkg/plugin/testdata/plugdir/good/hello/hello.sh diff --git a/pkg/plugin/testdata/plugdir/hello/plugin.yaml b/pkg/plugin/testdata/plugdir/good/hello/plugin.yaml similarity index 85% rename from pkg/plugin/testdata/plugdir/hello/plugin.yaml rename to pkg/plugin/testdata/plugdir/good/hello/plugin.yaml index 6a78756d3..2b972da59 100644 --- a/pkg/plugin/testdata/plugdir/hello/plugin.yaml +++ b/pkg/plugin/testdata/plugdir/good/hello/plugin.yaml @@ -5,6 +5,5 @@ description: |- description command: "$HELM_PLUGIN_SELF/hello.sh" ignoreFlags: true -install: "echo installing..." hooks: install: "echo installing..." diff --git a/pkg/repo/chartrepo.go b/pkg/repo/chartrepo.go index c2c366a1e..edb86eaeb 100644 --- a/pkg/repo/chartrepo.go +++ b/pkg/repo/chartrepo.go @@ -205,6 +205,14 @@ func FindChartInRepoURL(repoURL, chartName, chartVersion, certFile, keyFile, caF // without adding repo to repositories, like FindChartInRepoURL, // but it also receives credentials for the chart repository. func FindChartInAuthRepoURL(repoURL, username, password, chartName, chartVersion, certFile, keyFile, caFile string, getters getter.Providers) (string, error) { + return FindChartInAuthAndTLSRepoURL(repoURL, username, password, chartName, chartVersion, certFile, keyFile, caFile, false, getters) +} + +// FindChartInAuthRepoURL finds chart in chart repository pointed by repoURL +// without adding repo to repositories, like FindChartInRepoURL, +// but it also receives credentials and TLS verify flag for the chart repository. +// TODO Helm 4, FindChartInAuthAndTLSRepoURL should be integrated into FindChartInAuthRepoURL. +func FindChartInAuthAndTLSRepoURL(repoURL, username, password, chartName, chartVersion, certFile, keyFile, caFile string, insecureSkipTLSverify bool, getters getter.Providers) (string, error) { // Download and write the index file to a temporary location buf := make([]byte, 20) @@ -212,13 +220,14 @@ func FindChartInAuthRepoURL(repoURL, username, password, chartName, chartVersion name := strings.ReplaceAll(base64.StdEncoding.EncodeToString(buf), "/", "-") c := Entry{ - URL: repoURL, - Username: username, - Password: password, - CertFile: certFile, - KeyFile: keyFile, - CAFile: caFile, - Name: name, + URL: repoURL, + Username: username, + Password: password, + CertFile: certFile, + KeyFile: keyFile, + CAFile: caFile, + Name: name, + InsecureSkipTLSverify: insecureSkipTLSverify, } r, err := NewChartRepository(&c, getters) if err != nil { diff --git a/pkg/repo/chartrepo_test.go b/pkg/repo/chartrepo_test.go index eceb3009e..800d9ae55 100644 --- a/pkg/repo/chartrepo_test.go +++ b/pkg/repo/chartrepo_test.go @@ -276,6 +276,44 @@ func startLocalServerForTests(handler http.Handler) (*httptest.Server, error) { return httptest.NewServer(handler), nil } +// startLocalTLSServerForTests Start the local helm server with TLS +func startLocalTLSServerForTests(handler http.Handler) (*httptest.Server, error) { + if handler == nil { + fileBytes, err := ioutil.ReadFile("testdata/local-index.yaml") + if err != nil { + return nil, err + } + handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write(fileBytes) + }) + } + + return httptest.NewTLSServer(handler), nil +} + +func TestFindChartInAuthAndTLSRepoURL(t *testing.T) { + srv, err := startLocalTLSServerForTests(nil) + if err != nil { + t.Fatal(err) + } + defer srv.Close() + + chartURL, err := FindChartInAuthAndTLSRepoURL(srv.URL, "", "", "nginx", "", "", "", "", true, getter.All(&cli.EnvSettings{})) + if err != nil { + t.Fatalf("%v", err) + } + if chartURL != "https://kubernetes-charts.storage.googleapis.com/nginx-0.2.0.tgz" { + t.Errorf("%s is not the valid URL", chartURL) + } + + // If the insecureSkipTLsverify is false, it will return an error that contains "x509: certificate signed by unknown authority". + _, err = FindChartInAuthAndTLSRepoURL(srv.URL, "", "", "nginx", "0.1.0", "", "", "", false, getter.All(&cli.EnvSettings{})) + + if !strings.Contains(err.Error(), "x509: certificate signed by unknown authority") { + t.Errorf("Expected TLS error for function FindChartInAuthAndTLSRepoURL not found, but got a different error (%v)", err) + } +} + func TestFindChartInRepoURL(t *testing.T) { srv, err := startLocalServerForTests(nil) if err != nil { diff --git a/pkg/repo/index.go b/pkg/repo/index.go index 6ef2cf8b5..55b984eea 100644 --- a/pkg/repo/index.go +++ b/pkg/repo/index.go @@ -77,6 +77,8 @@ func (c ChartVersions) Less(a, b int) bool { // IndexFile represents the index file in a chart repository type IndexFile struct { + // This is used ONLY for validation against chartmuseum's index files and is discarded after validation. + ServerInfo map[string]interface{} `json:"serverInfo,omitempty"` APIVersion string `json:"apiVersion"` Generated time.Time `json:"generated"` Entries map[string]ChartVersions `json:"entries"` @@ -228,6 +230,23 @@ type ChartVersion struct { Created time.Time `json:"created,omitempty"` Removed bool `json:"removed,omitempty"` Digest string `json:"digest,omitempty"` + + // ChecksumDeprecated is deprecated in Helm 3, and therefore ignored. Helm 3 replaced + // this with Digest. However, with a strict YAML parser enabled, a field must be + // present on the struct for backwards compatibility. + ChecksumDeprecated string `json:"checksum,omitempty"` + + // EngineDeprecated is deprecated in Helm 3, and therefore ignored. However, with a strict + // YAML parser enabled, this field must be present. + EngineDeprecated string `json:"engine,omitempty"` + + // TillerVersionDeprecated is deprecated in Helm 3, and therefore ignored. However, with a strict + // YAML parser enabled, this field must be present. + TillerVersionDeprecated string `json:"tillerVersion,omitempty"` + + // URLDeprecated is deprectaed in Helm 3, superseded by URLs. It is ignored. However, + // with a strict YAML parser enabled, this must be present on the struct. + URLDeprecated string `json:"url,omitempty"` } // IndexDirectory reads a (flat) directory and generates an index. @@ -281,7 +300,7 @@ func IndexDirectory(dir, baseURL string) (*IndexFile, error) { // This will fail if API Version is not set (ErrNoAPIVersion) or if the unmarshal fails. func loadIndex(data []byte) (*IndexFile, error) { i := &IndexFile{} - if err := yaml.Unmarshal(data, i); err != nil { + if err := yaml.UnmarshalStrict(data, i); err != nil { return i, err } i.SortEntries() diff --git a/pkg/repo/index_test.go b/pkg/repo/index_test.go index 466a2c306..c22588971 100644 --- a/pkg/repo/index_test.go +++ b/pkg/repo/index_test.go @@ -35,9 +35,31 @@ import ( ) const ( - testfile = "testdata/local-index.yaml" - unorderedTestfile = "testdata/local-index-unordered.yaml" - testRepo = "test-repo" + testfile = "testdata/local-index.yaml" + chartmuseumtestfile = "testdata/chartmuseum-index.yaml" + unorderedTestfile = "testdata/local-index-unordered.yaml" + testRepo = "test-repo" + indexWithDuplicates = ` +apiVersion: v1 +entries: + nginx: + - urls: + - https://kubernetes-charts.storage.googleapis.com/nginx-0.2.0.tgz + name: nginx + description: string + version: 0.2.0 + home: https://github.com/something/else + digest: "sha256:1234567890abcdef" + nginx: + - urls: + - https://kubernetes-charts.storage.googleapis.com/alpine-1.0.0.tgz + - http://storage2.googleapis.com/kubernetes-charts/alpine-1.0.0.tgz + name: alpine + description: string + version: 1.0.0 + home: https://github.com/something + digest: "sha256:1234567890abcdef" +` ) func TestIndexFile(t *testing.T) { @@ -84,15 +106,43 @@ func TestIndexFile(t *testing.T) { } func TestLoadIndex(t *testing.T) { - b, err := ioutil.ReadFile(testfile) - if err != nil { - t.Fatal(err) + + tests := []struct { + Name string + Filename string + }{ + { + Name: "regular index file", + Filename: testfile, + }, + { + Name: "chartmuseum index file", + Filename: chartmuseumtestfile, + }, } - i, err := loadIndex(b) - if err != nil { - t.Fatal(err) + + for _, tc := range tests { + tc := tc + t.Run(tc.Name, func(t *testing.T) { + t.Parallel() + b, err := ioutil.ReadFile(tc.Filename) + if err != nil { + t.Fatal(err) + } + i, err := loadIndex(b) + if err != nil { + t.Fatal(err) + } + verifyLocalIndex(t, i) + }) + } +} + +// 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 { + t.Errorf("Expected an error when duplicate entries are present") } - verifyLocalIndex(t, i) } func TestLoadIndexFile(t *testing.T) { diff --git a/pkg/repo/repotest/server.go b/pkg/repo/repotest/server.go index b18bce49c..270c8958a 100644 --- a/pkg/repo/repotest/server.go +++ b/pkg/repo/repotest/server.go @@ -21,6 +21,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "testing" "helm.sh/helm/v3/internal/tlsutil" @@ -29,6 +30,19 @@ import ( "helm.sh/helm/v3/pkg/repo" ) +// NewTempServerWithCleanup creates a server inside of a temp dir. +// +// If the passed in string is not "", it will be treated as a shell glob, and files +// will be copied from that path to the server's docroot. +// +// The caller is responsible for stopping the server. +// The temp dir will be removed by testing package automatically when test finished. +func NewTempServerWithCleanup(t *testing.T, glob string) (*Server, error) { + srv, err := NewTempServer(glob) + t.Cleanup(func() { os.RemoveAll(srv.docroot) }) + return srv, err +} + // NewTempServer creates a server inside of a temp dir. // // If the passed in string is not "", it will be treated as a shell glob, and files @@ -36,6 +50,8 @@ import ( // // The caller is responsible for destroying the temp directory as well as stopping // the server. +// +// Deprecated: use NewTempServerWithCleanup func NewTempServer(glob string) (*Server, error) { tdir, err := ioutil.TempDir("", "helm-repotest-") if err != nil { diff --git a/pkg/repo/repotest/server_test.go b/pkg/repo/repotest/server_test.go index ee62791af..6d71071da 100644 --- a/pkg/repo/repotest/server_test.go +++ b/pkg/repo/repotest/server_test.go @@ -99,7 +99,7 @@ func TestServer(t *testing.T) { func TestNewTempServer(t *testing.T) { defer ensure.HelmHome(t)() - srv, err := NewTempServer("testdata/examplechart-0.1.0.tgz") + srv, err := NewTempServerWithCleanup(t, "testdata/examplechart-0.1.0.tgz") if err != nil { t.Fatal(err) } diff --git a/pkg/repo/testdata/chartmuseum-index.yaml b/pkg/repo/testdata/chartmuseum-index.yaml new file mode 100644 index 000000000..3077596f4 --- /dev/null +++ b/pkg/repo/testdata/chartmuseum-index.yaml @@ -0,0 +1,50 @@ +serverInfo: + contextPath: /v1/helm +apiVersion: v1 +entries: + nginx: + - urls: + - https://kubernetes-charts.storage.googleapis.com/nginx-0.2.0.tgz + name: nginx + description: string + version: 0.2.0 + home: https://github.com/something/else + digest: "sha256:1234567890abcdef" + keywords: + - popular + - web server + - proxy + - urls: + - https://kubernetes-charts.storage.googleapis.com/nginx-0.1.0.tgz + name: nginx + description: string + version: 0.1.0 + home: https://github.com/something + digest: "sha256:1234567890abcdef" + keywords: + - popular + - web server + - proxy + alpine: + - urls: + - https://kubernetes-charts.storage.googleapis.com/alpine-1.0.0.tgz + - http://storage2.googleapis.com/kubernetes-charts/alpine-1.0.0.tgz + name: alpine + description: string + version: 1.0.0 + home: https://github.com/something + keywords: + - linux + - alpine + - small + - sumtin + digest: "sha256:1234567890abcdef" + chartWithNoURL: + - name: chartWithNoURL + description: string + version: 1.0.0 + home: https://github.com/something + keywords: + - small + - sumtin + digest: "sha256:1234567890abcdef" diff --git a/scripts/get-helm-3 b/scripts/get-helm-3 index f2495e444..08d0e14ca 100755 --- a/scripts/get-helm-3 +++ b/scripts/get-helm-3 @@ -19,8 +19,16 @@ : ${BINARY_NAME:="helm"} : ${USE_SUDO:="true"} +: ${DEBUG:="false"} +: ${VERIFY_CHECKSUM:="true"} +: ${VERIFY_SIGNATURES:="false"} : ${HELM_INSTALL_DIR:="/usr/local/bin"} +HAS_CURL="$(type "curl" &> /dev/null && echo true || echo false)" +HAS_WGET="$(type "wget" &> /dev/null && echo true || echo false)" +HAS_OPENSSL="$(type "openssl" &> /dev/null && echo true || echo false)" +HAS_GPG="$(type "gpg" &> /dev/null && echo true || echo false)" + # initArch discovers the architecture for this system. initArch() { ARCH=$(uname -m) @@ -58,7 +66,7 @@ runAsRoot() { } # verifySupported checks that the os/arch combination is supported for -# binary builds. +# binary builds, as well whether or not necessary tools are present. verifySupported() { local supported="darwin-amd64\nlinux-386\nlinux-amd64\nlinux-arm\nlinux-arm64\nlinux-ppc64le\nlinux-s390x\nwindows-amd64" if ! echo "${supported}" | grep -q "${OS}-${ARCH}"; then @@ -67,10 +75,29 @@ verifySupported() { exit 1 fi - if ! type "curl" > /dev/null && ! type "wget" > /dev/null; then + if [ "${HAS_CURL}" != "true" ] && [ "${HAS_WGET}" != "true" ]; then echo "Either curl or wget is required" exit 1 fi + + if [ "${VERIFY_CHECKSUM}" == "true" ] && [ "${HAS_OPENSSL}" != "true" ]; then + echo "In order to verify checksum, openssl must first be installed." + echo "Please install openssl or set VERIFY_CHECKSUM=false in your environment." + exit 1 + fi + + if [ "${VERIFY_SIGNATURES}" == "true" ]; then + if [ "${HAS_GPG}" != "true" ]; then + echo "In order to verify signatures, gpg must first be installed." + echo "Please install gpg or set VERIFY_SIGNATURES=false in your environment." + exit 1 + fi + if [ "${OS}" != "linux" ]; then + echo "Signature verification is currently only supported on Linux." + echo "Please set VERIFY_SIGNATURES=false or verify the signatures manually." + exit 1 + fi + fi } # checkDesiredVersion checks if the desired version is available. @@ -78,9 +105,9 @@ checkDesiredVersion() { if [ "x$DESIRED_VERSION" == "x" ]; then # Get tag from release URL local latest_release_url="https://github.com/helm/helm/releases" - if type "curl" > /dev/null; then + if [ "${HAS_CURL}" == "true" ]; then TAG=$(curl -Ls $latest_release_url | grep 'href="/helm/helm/releases/tag/v3.[0-9]*.[0-9]*\"' | grep -v no-underline | head -n 1 | cut -d '"' -f 2 | awk '{n=split($NF,a,"/");print a[n]}' | awk 'a !~ $0{print}; {a=$0}') - elif type "wget" > /dev/null; then + elif [ "${HAS_WGET}" == "true" ]; then TAG=$(wget $latest_release_url -O - 2>&1 | grep 'href="/helm/helm/releases/tag/v3.[0-9]*.[0-9]*\"' | grep -v no-underline | head -n 1 | cut -d '"' -f 2 | awk '{n=split($NF,a,"/");print a[n]}' | awk 'a !~ $0{print}; {a=$0}') fi else @@ -115,35 +142,94 @@ downloadFile() { HELM_TMP_FILE="$HELM_TMP_ROOT/$HELM_DIST" HELM_SUM_FILE="$HELM_TMP_ROOT/$HELM_DIST.sha256" echo "Downloading $DOWNLOAD_URL" - if type "curl" > /dev/null; then + if [ "${HAS_CURL}" == "true" ]; then curl -SsL "$CHECKSUM_URL" -o "$HELM_SUM_FILE" - elif type "wget" > /dev/null; then - wget -q -O "$HELM_SUM_FILE" "$CHECKSUM_URL" - fi - if type "curl" > /dev/null; then curl -SsL "$DOWNLOAD_URL" -o "$HELM_TMP_FILE" - elif type "wget" > /dev/null; then + elif [ "${HAS_WGET}" == "true" ]; then + wget -q -O "$HELM_SUM_FILE" "$CHECKSUM_URL" wget -q -O "$HELM_TMP_FILE" "$DOWNLOAD_URL" fi } -# installFile verifies the SHA256 for the file, then unpacks and -# installs it. +# verifyFile verifies the SHA256 checksum of the binary package +# and the GPG signatures for both the package and checksum file +# (depending on settings in environment). +verifyFile() { + if [ "${VERIFY_CHECKSUM}" == "true" ]; then + verifyChecksum + fi + if [ "${VERIFY_SIGNATURES}" == "true" ]; then + verifySignatures + fi +} + +# installFile installs the Helm binary. installFile() { HELM_TMP="$HELM_TMP_ROOT/$BINARY_NAME" + mkdir -p "$HELM_TMP" + tar xf "$HELM_TMP_FILE" -C "$HELM_TMP" + HELM_TMP_BIN="$HELM_TMP/$OS-$ARCH/helm" + echo "Preparing to install $BINARY_NAME into ${HELM_INSTALL_DIR}" + runAsRoot cp "$HELM_TMP_BIN" "$HELM_INSTALL_DIR/$BINARY_NAME" + echo "$BINARY_NAME installed into $HELM_INSTALL_DIR/$BINARY_NAME" +} + +# verifyChecksum verifies the SHA256 checksum of the binary package. +verifyChecksum() { + printf "Verifying checksum... " local sum=$(openssl sha1 -sha256 ${HELM_TMP_FILE} | awk '{print $2}') local expected_sum=$(cat ${HELM_SUM_FILE}) if [ "$sum" != "$expected_sum" ]; then echo "SHA sum of ${HELM_TMP_FILE} does not match. Aborting." exit 1 fi + echo "Done." +} - mkdir -p "$HELM_TMP" - tar xf "$HELM_TMP_FILE" -C "$HELM_TMP" - HELM_TMP_BIN="$HELM_TMP/$OS-$ARCH/helm" - echo "Preparing to install $BINARY_NAME into ${HELM_INSTALL_DIR}" - runAsRoot cp "$HELM_TMP_BIN" "$HELM_INSTALL_DIR/$BINARY_NAME" - echo "$BINARY_NAME installed into $HELM_INSTALL_DIR/$BINARY_NAME" +# verifySignatures obtains the latest KEYS file from GitHub master branch +# as well as the signature .asc files from the specific GitHub release, +# then verifies that the release artifacts were signed by a maintainer's key. +verifySignatures() { + printf "Verifying signatures... " + local keys_filename="KEYS" + local github_keys_url="https://raw.githubusercontent.com/helm/helm/master/${keys_filename}" + if [ "${HAS_CURL}" == "true" ]; then + curl -SsL "${github_keys_url}" -o "${HELM_TMP_ROOT}/${keys_filename}" + elif [ "${HAS_WGET}" == "true" ]; then + wget -q -O "${HELM_TMP_ROOT}/${keys_filename}" "${github_keys_url}" + fi + local gpg_keyring="${HELM_TMP_ROOT}/keyring.gpg" + local gpg_homedir="${HELM_TMP_ROOT}/gnupg" + mkdir -p -m 0700 "${gpg_homedir}" + local gpg_stderr_device="/dev/null" + if [ "${DEBUG}" == "true" ]; then + gpg_stderr_device="/dev/stderr" + fi + gpg --batch --quiet --homedir="${gpg_homedir}" --import "${HELM_TMP_ROOT}/${keys_filename}" 2> "${gpg_stderr_device}" + gpg --batch --no-default-keyring --keyring "${gpg_homedir}/pubring.kbx" --export > "${gpg_keyring}" + local github_release_url="https://github.com/helm/helm/releases/download/${TAG}" + if [ "${HAS_CURL}" == "true" ]; then + curl -SsL "${github_release_url}/helm-${TAG}-${OS}-${ARCH}.tar.gz.sha256.asc" -o "${HELM_TMP_ROOT}/helm-${TAG}-${OS}-${ARCH}.tar.gz.sha256.asc" + curl -SsL "${github_release_url}/helm-${TAG}-${OS}-${ARCH}.tar.gz.asc" -o "${HELM_TMP_ROOT}/helm-${TAG}-${OS}-${ARCH}.tar.gz.asc" + elif [ "${HAS_WGET}" == "true" ]; then + wget -q -O "${HELM_TMP_ROOT}/helm-${TAG}-${OS}-${ARCH}.tar.gz.sha256.asc" "${github_release_url}/helm-${TAG}-${OS}-${ARCH}.tar.gz.sha256.asc" + wget -q -O "${HELM_TMP_ROOT}/helm-${TAG}-${OS}-${ARCH}.tar.gz.asc" "${github_release_url}/helm-${TAG}-${OS}-${ARCH}.tar.gz.asc" + fi + local error_text="If you think this might be a potential security issue," + error_text="${error_text}\nplease see here: https://github.com/helm/community/blob/master/SECURITY.md" + local num_goodlines_sha=$(gpg --verify --keyring="${gpg_keyring}" --status-fd=1 "${HELM_TMP_ROOT}/helm-${TAG}-${OS}-${ARCH}.tar.gz.sha256.asc" 2> "${gpg_stderr_device}" | grep -c -E '^\[GNUPG:\] (GOODSIG|VALIDSIG)') + if [[ ${num_goodlines_sha} -lt 2 ]]; then + echo "Unable to verify the signature of helm-${TAG}-${OS}-${ARCH}.tar.gz.sha256!" + echo -e "${error_text}" + exit 1 + fi + local num_goodlines_tar=$(gpg --verify --keyring="${gpg_keyring}" --status-fd=1 "${HELM_TMP_ROOT}/helm-${TAG}-${OS}-${ARCH}.tar.gz.asc" 2> "${gpg_stderr_device}" | grep -c -E '^\[GNUPG:\] (GOODSIG|VALIDSIG)') + if [[ ${num_goodlines_tar} -lt 2 ]]; then + echo "Unable to verify the signature of helm-${TAG}-${OS}-${ARCH}.tar.gz!" + echo -e "${error_text}" + exit 1 + fi + echo "Done." } # fail_trap is executed if an error occurs. @@ -195,6 +281,11 @@ cleanup() { trap "fail_trap" EXIT set -e +# Set debug if desired +if [ "${DEBUG}" == "true" ]; then + set -x +fi + # Parsing input arguments (if any) export INPUT_ARGUMENTS="${@}" set -u @@ -229,6 +320,7 @@ verifySupported checkDesiredVersion if ! checkHelmInstalledVersion; then downloadFile + verifyFile installFile fi testVersion