From 8232a76aef5dd9671e2b4cdfa0fbca358725fabc Mon Sep 17 00:00:00 2001 From: Christophe VILA Date: Fri, 18 Jun 2021 13:41:57 +0200 Subject: [PATCH 1/4] use TLS client information from repo config when downloading a chart Signed-off-by: Christophe VILA --- pkg/downloader/manager.go | 16 ++++++++++------ pkg/downloader/manager_test.go | 4 ++-- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index 22db8bfdd..ff5f9c4e7 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -310,7 +310,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { // Any failure to resolve/download a chart should fail: // https://github.com/helm/helm/issues/1439 - churl, username, password, insecureskiptlsverify, passcredentialsall, err := m.findChartURL(dep.Name, dep.Version, dep.Repository, repos) + churl, username, password, insecureskiptlsverify, passcredentialsall, caFile, certFile, keyFile, err := m.findChartURL(dep.Name, dep.Version, dep.Repository, repos) if err != nil { saveError = errors.Wrapf(err, "could not find %s", churl) break @@ -334,6 +334,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { getter.WithBasicAuth(username, password), getter.WithPassCredentialsAll(passcredentialsall), getter.WithInsecureSkipVerifyTLS(insecureskiptlsverify), + getter.WithTLSClientConfig(certFile, keyFile, caFile), }, } @@ -687,9 +688,9 @@ func (m *Manager) parallelRepoUpdate(repos []*repo.Entry) error { // repoURL is the repository to search // // If it finds a URL that is "relative", it will prepend the repoURL. -func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]*repo.ChartRepository) (url, username, password string, insecureskiptlsverify, passcredentialsall bool, err error) { +func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]*repo.ChartRepository) (url, username, password string, insecureskiptlsverify, passcredentialsall bool, caFile, certFile, keyFile string, err error) { if strings.HasPrefix(repoURL, "oci://") { - return fmt.Sprintf("%s/%s:%s", repoURL, name, version), "", "", false, false, nil + return fmt.Sprintf("%s/%s:%s", repoURL, name, version), "", "", false, false, "", "", "", nil } for _, cr := range repos { @@ -713,15 +714,18 @@ func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]* password = cr.Config.Password passcredentialsall = cr.Config.PassCredentialsAll insecureskiptlsverify = cr.Config.InsecureSkipTLSverify + caFile = cr.Config.CAFile + certFile = cr.Config.CertFile + keyFile = cr.Config.KeyFile return } } - url, err = repo.FindChartInRepoURL(repoURL, name, version, "", "", "", m.Getters) + url, err = repo.FindChartInRepoURL(repoURL, name, version, certFile, keyFile, caFile, m.Getters) if err == nil { - return url, username, password, false, false, err + return url, username, password, false, false, "", "", "", err } err = errors.Errorf("chart %s not found in %s: %s", name, repoURL, err) - return url, username, password, false, false, err + return url, username, password, false, false, "", "", "", err } // findEntryByName finds an entry in the chart repository whose name matches the given name. diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index 0cc6d6f12..b8da42cd2 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -81,7 +81,7 @@ func TestFindChartURL(t *testing.T) { version := "0.1.0" repoURL := "http://example.com/charts" - churl, username, password, insecureSkipTLSVerify, passcredentialsall, err := m.findChartURL(name, version, repoURL, repos) + churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err := m.findChartURL(name, version, repoURL, repos) if err != nil { t.Fatal(err) } @@ -106,7 +106,7 @@ func TestFindChartURL(t *testing.T) { version = "1.2.3" repoURL = "https://example-https-insecureskiptlsverify.com" - churl, username, password, insecureSkipTLSVerify, passcredentialsall, err = m.findChartURL(name, version, repoURL, repos) + churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(name, version, repoURL, repos) if err != nil { t.Fatal(err) } From 0d9ebc7885e427373fbdaa95f461b349fb0b1ac1 Mon Sep 17 00:00:00 2001 From: longkai Date: Fri, 25 Jun 2021 12:36:12 +0800 Subject: [PATCH 2/4] refactor(getter): refine http GET request It just makes the code better, I suppose the following is rational: - use standard libaray common constants instead of hardcode though it's really common - close the response body even if the http status code is not 200 OK. The doc says *It is the caller's responsibility to close Body*. - move the `bytes.Buffer` return value declaration where it gets used. Signed-off-by: longkai --- pkg/getter/httpgetter.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/pkg/getter/httpgetter.go b/pkg/getter/httpgetter.go index 94b64381c..454eb6eb6 100644 --- a/pkg/getter/httpgetter.go +++ b/pkg/getter/httpgetter.go @@ -43,13 +43,11 @@ func (g *HTTPGetter) Get(href string, options ...Option) (*bytes.Buffer, error) } func (g *HTTPGetter) get(href string) (*bytes.Buffer, error) { - buf := bytes.NewBuffer(nil) - // Set a helm specific user agent so that a repo server and metrics can // separate helm calls from other tools interacting with repos. - req, err := http.NewRequest("GET", href, nil) + req, err := http.NewRequest(http.MethodGet, href, nil) if err != nil { - return buf, err + return nil, err } req.Header.Set("User-Agent", version.GetUserAgent()) @@ -61,11 +59,11 @@ func (g *HTTPGetter) get(href string) (*bytes.Buffer, error) { // with the basic auth is the one being fetched. u1, err := url.Parse(g.opts.url) if err != nil { - return buf, errors.Wrap(err, "Unable to parse getter URL") + return nil, errors.Wrap(err, "Unable to parse getter URL") } u2, err := url.Parse(href) if err != nil { - return buf, errors.Wrap(err, "Unable to parse URL getting from") + return nil, errors.Wrap(err, "Unable to parse URL getting from") } // Host on URL (returned from url.Parse) contains the port if present. @@ -84,14 +82,15 @@ func (g *HTTPGetter) get(href string) (*bytes.Buffer, error) { resp, err := client.Do(req) if err != nil { - return buf, err + return nil, err } - if resp.StatusCode != 200 { - return buf, errors.Errorf("failed to fetch %s : %s", href, resp.Status) + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return nil, errors.Errorf("failed to fetch %s : %s", href, resp.Status) } + buf := bytes.NewBuffer(nil) _, err = io.Copy(buf, resp.Body) - resp.Body.Close() return buf, err } From 4e2e4084edc0fa1e21d6a9a83b3ffdc8c1ec5e01 Mon Sep 17 00:00:00 2001 From: Matt Farina Date: Fri, 25 Jun 2021 20:08:06 -0400 Subject: [PATCH 3/4] Fix the url being set by WithURL on the getters The URL passed to the getter for WithURL needs to be a full URL rather than a chart reference used at the CLI. For example, bitnami/wordpress can point to the wordpress chart in the bitnami repo where the bitnami repo is at https://charts.bitnami.com. WithURL needs the full URL to the repo and not bitnami/wordpress. This is important because getters use the full URL information. In this case the http getter uses the host name for SNI handling. Before this change WithURL was being set to the chart reference instead of the URL. This was a silent bug. This change sets WithURL using a URL after for the repo is available when a reference is used instead of a full url. Signed-off-by: Matt Farina --- pkg/downloader/chart_downloader.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index 2c0d55a55..575c94151 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -158,7 +158,6 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, er if err != nil { return nil, errors.Errorf("invalid chart URL format: %s", ref) } - c.Options = append(c.Options, getter.WithURL(ref)) rf, err := loadRepoConfig(c.RepositoryConfig) if err != nil { @@ -177,6 +176,8 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, er // If there is no special config, return the default HTTP client and // swallow the error. if err == ErrNoOwnerRepo { + // Make sure to add the ref URL as the URL for the getter + c.Options = append(c.Options, getter.WithURL(ref)) return u, nil } return u, err @@ -215,6 +216,10 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (*url.URL, er return u, err } + // Now that we have the chart repository information we can use that URL + // to set the URL for the getter. + c.Options = append(c.Options, getter.WithURL(rc.URL)) + r, err := repo.NewChartRepository(rc, c.Getters) if err != nil { return u, err From 385fcae1ba0c8450bd89795cade5fafa99f4d771 Mon Sep 17 00:00:00 2001 From: Matt Farina Date: Mon, 28 Jun 2021 11:36:37 -0400 Subject: [PATCH 4/4] Adding test for user/pass without repo on Helm install Signed-off-by: Matt Farina --- cmd/helm/install_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cmd/helm/install_test.go b/cmd/helm/install_test.go index 13c994b92..b7349e3d5 100644 --- a/cmd/helm/install_test.go +++ b/cmd/helm/install_test.go @@ -20,6 +20,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "path/filepath" "testing" "helm.sh/helm/v3/pkg/repo/repotest" @@ -48,6 +49,8 @@ func TestInstall(t *testing.T) { t.Fatal(err) } + repoFile := filepath.Join(srv.Root(), "repositories.yaml") + tests := []cmdTestCase{ // Install, base case { @@ -244,6 +247,11 @@ func TestInstall(t *testing.T) { cmd: "install aeneas reqtest --namespace default --repo " + srv2.URL + " --username username --password password --pass-credentials", golden: "output/install.txt", }, + { + name: "basic install with credentials and no repo", + cmd: fmt.Sprintf("install aeneas test/reqtest --username username --password password --repository-config %s --repository-cache %s", repoFile, srv.Root()), + golden: "output/install.txt", + }, } runTestActionCmd(t, tests)