From 82398667dfe208407be9fe499ac96240aa8ce54b Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Tue, 8 Sep 2020 17:15:01 -0600 Subject: [PATCH 1/4] fix: check mode bits on kubeconfig file Signed-off-by: Matt Butcher --- cmd/helm/root.go | 3 ++ cmd/helm/root_unix.go | 58 ++++++++++++++++++++++++++++++++++++ cmd/helm/root_unix_test.go | 61 ++++++++++++++++++++++++++++++++++++++ cmd/helm/root_windows.go | 24 +++++++++++++++ 4 files changed, 146 insertions(+) create mode 100644 cmd/helm/root_unix.go create mode 100644 cmd/helm/root_unix_test.go create mode 100644 cmd/helm/root_windows.go diff --git a/cmd/helm/root.go b/cmd/helm/root.go index 82c87bd7d..91542bb7e 100644 --- a/cmd/helm/root.go +++ b/cmd/helm/root.go @@ -204,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..210842b35 --- /dev/null +++ b/cmd/helm/root_unix.go @@ -0,0 +1,58 @@ +/* +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..73f18ec28 --- /dev/null +++ b/cmd/helm/root_unix_test.go @@ -0,0 +1,61 @@ +/* +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! +} From c4ef82be13a0a3b6b42ce92bdd0357f4f6ac9e62 Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Wed, 9 Sep 2020 16:33:18 -0600 Subject: [PATCH 2/4] validate the name passed in during helm create Signed-off-by: Matt Butcher --- pkg/chartutil/create.go | 27 +++++++++++++++++++++++++++ pkg/chartutil/create_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/pkg/chartutil/create.go b/pkg/chartutil/create.go index 6e382b961..d4b65e9b8 100644 --- a/pkg/chartutil/create.go +++ b/pkg/chartutil/create.go @@ -21,6 +21,7 @@ import ( "io/ioutil" "os" "path/filepath" + "regexp" "strings" "github.com/pkg/errors" @@ -30,6 +31,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 +70,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 @@ -522,6 +533,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 @@ -627,3 +644,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..f68ebbd63 100644 --- a/pkg/chartutil/create_test.go +++ b/pkg/chartutil/create_test.go @@ -117,3 +117,30 @@ func TestCreateFrom(t *testing.T) { } } } + +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) + } + } +} From ed5fba5142fa5a2366df143616e9161ff866a53d Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Wed, 9 Sep 2020 13:30:30 -0600 Subject: [PATCH 3/4] refactor the release name validation to be consistent across Helm Signed-off-by: Matt Butcher --- pkg/action/action.go | 7 +- pkg/action/action_test.go | 37 ----------- pkg/action/history.go | 3 +- pkg/action/release_testing.go | 3 +- pkg/action/rollback.go | 3 +- pkg/action/uninstall.go | 3 +- pkg/action/upgrade.go | 15 +---- pkg/chartutil/validate_name.go | 99 +++++++++++++++++++++++++++++ pkg/chartutil/validate_name_test.go | 91 ++++++++++++++++++++++++++ pkg/lint/rules/template.go | 14 +--- 10 files changed, 206 insertions(+), 69 deletions(-) create mode 100644 pkg/chartutil/validate_name.go create mode 100644 pkg/chartutil/validate_name_test.go 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 c05b4403d..fedf260fb 100644 --- a/pkg/action/action_test.go +++ b/pkg/action/action_test.go @@ -319,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/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/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/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/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 { From 40b78002873d525a31c5dec75c8607be67327360 Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Fri, 11 Sep 2020 16:23:34 -0600 Subject: [PATCH 4/4] handle case where dependency name collisions break dependency resolution Signed-off-by: Matt Butcher --- pkg/action/dependency.go | 87 ++++++++++++++++++++-------- pkg/action/dependency_test.go | 103 ++++++++++++++++++++++++++++++++++ 2 files changed, 167 insertions(+), 23 deletions(-) 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)) +}