Repository: helm/helm
Pull request #9318 (2021-02-04 01:46:33)

Fix issue with helm pull failing if pulling from a repository that redirects to another domain
<!--  Thanks for sending a pull request!  Here are some tips for you:
1. Make sure to read the Contributing Guide before submitting your PR: https://github.com/helm/helm/blob/master/CONTRIBUTING.md
2. If this PR closes another issue, add 'closes #<issue number>' somewhere in the PR summary. GitHub will automatically close that issue when this PR gets merged. Alternatively, adding 'refs #<issue number>' will not close the issue, but help provide the reviewer more context.-->

**What this PR does / why we need it**:
This PR fixes issue #9317

**Special notes for your reviewer**:

**If applicable**:
- [ ] this PR contains documentation
- [x] this PR contains unit tests
- [ ] this PR has been tested for backwards compatibility


bridgetkromhout: I don't have permission to re-trigger the build in https://app.circleci.com/pipelines/github/helm/helm/2128/workflows/9899ccf8-58b5-4862-8a8a-23e3b3c4b953/jobs/13676 but I suspect it may be an intermittent failure; I can't reproduce it.
wahabmk: > I don't have permission to re-trigger the build in https://app.circleci.com/pipelines/github/helm/helm/2128/workflows/9899ccf8-58b5-4862-8a8a-23e3b3c4b953/jobs/13676 but I suspect it may be an intermittent failure; I can't reproduce it.

The builds pass now. Added to the existing tests for new changes, and refactored the tests a bit to make it easier to read.
technosophos: NOTE: This PR _MUST_ got through a security audit before merge, as it alters the SSL code.
wahabmk: > NOTE: This PR _MUST_ got through a security audit before merge, as it alters the SSL code.

@technosophos what is the process for that? Anything I can do to contribute?
technosophos: At least one LGTM must come from a security team member.
wahabmk: > At least one LGTM must come from a security team member.

@technosophos could you (or somebody else) add a security team for review whenever schedule allows. Thanks for clarifying the process!
wahabmk: > The changes suggested here open us up to several root cert chain attacks. Ones which we explicitly avoided by only allowing Helm to load a local certificate pool and add ONLY the certificates which were explicitly added by the end user.
> 
> I'm also highly concerned about the complete removal of the hostname verification that was done in httpgetter.go. While it is true that `BuildNameToCertificate` was deprecated, that doesn't mean we should remove all of the server name validation guards we put in.
> 
> If you can provide a set of unit tests which test for root cert chain attacks and hostname verification attacks, then I'd be more comfortable with the changes involved here. But at this time this PR could open us up to several significant security flaws. All of which I'm VERY uncomfortable with.

@bacongobbler thanks for the review! You pointed out 2 concerns:
1. Loading system cert pool by default causes vulnerability to root cert chain attacks.
2. Not specifying the SNI through `tls.Config.ServerName` causes vulnerability to hostname verification attacks.

I reverted the changes made for 1 so it is no longer applicable to this PR. As for 2, Go by default, already [validates](https://github.com/golang/go/blob/3979fb9af9ccfc0b7ccb613dcf256b18c2c295f0/src/net/http/transport.go#L1505-L1513) the remote hostname against certificate for every request. So if there is a redirect, Go will do the validation for the hostname in the original request and then for the hostname in the subsequent redirect as well. Specifying the `ServerName` manually just overwrites the hostname of the request. So, removing the `tls.Config.ServerName` overwrite would not introduce any new vulnerability.
wahabmk: @bridgetkromhout the build failed due to timeout, could you re-run. Tanks!
bacongobbler: > As for 2, Go by default already validates the remote hostname against certificate for every request. So if there is a redirect, Go will do the validation for the hostname in the original request and then for the hostname in the subsequent redirect as well. Specifying the ServerName manually just overwrites the hostname of the request. So, removing the tls.Config.ServerName overwrite would not introduce any new vulnerability.

Thank you for clarifying. However I'd still feel more comfortable if there were unit tests prepared along with this PR to validate that assumption. As at the end of the day we're liable for millions of users on a daily basis. I'd hate for there to be an unintentional security flaw introduced in a PR because we did not add a few test fixtures to ensure this is the case. At the end of the day it's up to the maintainers to respond to security issues, so taking a few extra steps to assert those assumptions would be wonderful.
wahabmk: > > As for 2, Go by default already validates the remote hostname against certificate for every request. So if there is a redirect, Go will do the validation for the hostname in the original request and then for the hostname in the subsequent redirect as well. Specifying the ServerName manually just overwrites the hostname of the request. So, removing the tls.Config.ServerName overwrite would not introduce any new vulnerability.
> 
> Thank you for clarifying. However I'd still feel more comfortable if there were unit tests prepared along with this PR to validate that assumption. As at the end of the day we're liable for millions of users on a daily basis. I'd hate for there to be an unintentional security flaw introduced in a PR because we did not add a few test fixtures to ensure this is the case. At the end of the day it's up to the maintainers to respond to security issues, so taking a few extra steps to assert those assumptions before removing that code would really help us out (and make us more comfortable with merging these changes).
> 
> While we're at it... Tests to validate the first assumption (these changes allow `helm pull` to fetch with a redirect) would be great to see as well. It's important to write tests along with any fixes to ensure future iterations do not break backwards compatibility. There have been many times where a fix _looks_ correct but doesn't actually fix the problem, so it's worth writing that down.
> 
> I restarted the CI build a few times. Try rebasing on HEAD to see if there was a fix applied to resolve this issue. It's also possible that CircleCI is seeing a performance degradation in its service so that could be causing a slowdown/timeout.

@bacongobbler updated the PR with a unit test to verify TLS works with redirects and the client receives a response from redirected server.
wahabmk: @bacongobbler could you do a re-review, thanks.
shahzadlone: Is there any progress on this? seems like it's been marinating with a single approval for over a year now.
joejulian: @wahabmk can you fix the conflict?
wahabmk: Working on it...
joejulian: @wahabmk I merged in main to kick off CI which, unfortunately, failed. Could you take a look please?
pkprzekwas: What's the status of this issue. We are affected and wonder when it could possibly be merged.
SimonTheLeg: I have pushed a PR that builds on this one and fixes all the CI issues (at least when running the tests locally). Additionally it includes some documentation on how to generate the certificates:

https://github.com/helm/helm/pull/12545
wahabmk: @SimonTheLeg Thanks for the mention. I fixed the unit tests but I still have the DCO failing. @bacongobbler does this PR still need 1 more approval?
wahabmk: @bacongobbler @joejulian @technosophos @mattfarina I have fixed the CI and rebased from `main` branch in the latest commit I pushed. There was a previous commit by me that was not signed which caused the DCO to fail. So I rebased my commits into a single one and signed it. This PR needs 1 more approval. Could somebody kindly review it as it has been sitting since 2021. Thanks!
gjenkins8: @joejulian -- want to rerun CI here? (not sure why the github checks are referring to circle CI here, this branch is up to date / on top of recent helm main branch)
shahzadlone: > @joejulian -- want to rerun CI here? (not sure why the github checks are referring to circle CI here, this branch is up to date / on top of recent helm main branch)

Branch seems to be 13 commits behind at the moment: https://github.com/helm/helm/compare/2745909d...main

![image](https://github.com/helm/helm/assets/30120428/774e9e7c-bb5e-40a6-b81a-28a6dfef6b9e)





wahabmk: Rebased from upstream `main` branch
wahabmk: @gjenkins8 just wanted to inquire if there is any progress for getting this PR reviewed/merged. Thanks!
gjenkins8: @wahabmk -- would you be able to rebase please
wahabmk: @gjenkins8 Rebased from `main`. Looks like the CI needs approval to run. 
gjenkins8: > @gjenkins8 Rebased from `main`. Looks like the CI needs approval to run.

Thank! @wahabmk new version of golangci-lint (I think) has errored on your change now (minor fix needed) 
wahabmk: @gjenkins8 Fixed the reported lint error in 2nd commit.
wahabmk: @gjenkins8 could you run the CI please, thanks.
shahzadlone: The lack of priority on this small PR, which resolves noteable issues for many, is mind-boggling. 
gjenkins8: > @gjenkins8 could you run the CI please, thanks.

Apologies, I missed this. Can you rebase (again sorry). Have this PR on the TODO list now.

Patch:
diff --git a/pkg/getter/httpgetter.go b/pkg/getter/httpgetter.go
index 37d80cda7..a945dec2b 100644
--- a/pkg/getter/httpgetter.go
+++ b/pkg/getter/httpgetter.go
@@ -26,7 +26,6 @@ import (
 	"github.com/pkg/errors"
 
 	"helm.sh/helm/v4/internal/tlsutil"
-	"helm.sh/helm/v4/internal/urlutil"
 	"helm.sh/helm/v4/internal/version"
 )
 
@@ -137,12 +136,6 @@ func (g *HTTPGetter) httpClient() (*http.Client, error) {
 			return nil, errors.Wrap(err, "can't create TLS config for client")
 		}
 
-		sni, err := urlutil.ExtractHostname(g.opts.url)
-		if err != nil {
-			return nil, err
-		}
-		tlsConf.ServerName = sni
-
 		g.transport.TLSClientConfig = tlsConf
 	}
 


Test patch:
diff --git a/pkg/getter/httpgetter_test.go b/pkg/getter/httpgetter_test.go
index 24e670f6e..27752a257 100644
--- a/pkg/getter/httpgetter_test.go
+++ b/pkg/getter/httpgetter_test.go
@@ -358,6 +358,131 @@ func TestDownloadTLS(t *testing.T) {
 	}
 }
 
+func TestDownloadTLSWithRedirect(t *testing.T) {
+	cd := "../../testdata"
+	srv2Resp := "hello"
+	insecureSkipTLSverify := false
+
+	// Server 2 that will actually fulfil the request.
+	ca, pub, priv := filepath.Join(cd, "rootca.crt"), filepath.Join(cd, "localhost-crt.pem"), filepath.Join(cd, "key.pem")
+	tlsConf, err := tlsutil.NewTLSConfig(
+		tlsutil.WithCAFile(ca),
+		tlsutil.WithCertKeyPairFiles(pub, priv),
+		tlsutil.WithInsecureSkipVerify(insecureSkipTLSverify),
+	)
+
+	if err != nil {
+		t.Fatal(errors.Wrap(err, "can't create TLS config for client"))
+	}
+
+	tlsSrv2 := httptest.NewUnstartedServer(http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) {
+		rw.Header().Set("Content-Type", "text/plain")
+		rw.Write([]byte(srv2Resp))
+	}))
+
+	tlsSrv2.TLS = tlsConf
+	tlsSrv2.StartTLS()
+	defer tlsSrv2.Close()
+
+	// Server 1 responds with a redirect to Server 2.
+	ca, pub, priv = filepath.Join(cd, "rootca.crt"), filepath.Join(cd, "crt.pem"), filepath.Join(cd, "key.pem")
+	tlsConf, err = tlsutil.NewTLSConfig(
+		tlsutil.WithCAFile(ca),
+		tlsutil.WithCertKeyPairFiles(pub, priv),
+		tlsutil.WithInsecureSkipVerify(insecureSkipTLSverify),
+	)
+
+	if err != nil {
+		t.Fatal(errors.Wrap(err, "can't create TLS config for client"))
+	}
+
+	tlsSrv1 := httptest.NewUnstartedServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+		u, _ := url.ParseRequestURI(tlsSrv2.URL)
+
+		// Make the request using the hostname 'localhost' (to which 'localhost-crt.pem' is issued)
+		// to verify that a successful TLS connection is made even if the client doesn't specify
+		// the hostname (SNI) in `tls.Config.ServerName`. By default the hostname is derived from the
+		// request URL for every request (including redirects). Setting `tls.Config.ServerName` on the
+		// client just overrides the remote endpoint's hostname.
+		// See https://github.com/golang/go/blob/3979fb9/src/net/http/transport.go#L1505-L1513.
+		u.Host = fmt.Sprintf("localhost:%s", u.Port())
+
+		http.Redirect(rw, r, u.String(), http.StatusTemporaryRedirect)
+	}))
+
+	tlsSrv1.TLS = tlsConf
+	tlsSrv1.StartTLS()
+	defer tlsSrv1.Close()
+
+	u, _ := url.ParseRequestURI(tlsSrv1.URL)
+
+	t.Run("Test with TLS", func(t *testing.T) {
+		g, err := NewHTTPGetter(
+			WithURL(u.String()),
+			WithTLSClientConfig(pub, priv, ca),
+		)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		buf, err := g.Get(u.String())
+		if err != nil {
+			t.Error(err)
+		}
+
+		b, err := io.ReadAll(buf)
+		if err != nil {
+			t.Error(err)
+		}
+
+		if string(b) != srv2Resp {
+			t.Errorf("expected response from Server2 to be '%s', instead got: %s", srv2Resp, string(b))
+		}
+	})
+
+	t.Run("Test with TLS config being passed along in .Get (see #6635)", func(t *testing.T) {
+		g, err := NewHTTPGetter()
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		buf, err := g.Get(u.String(), WithURL(u.String()), WithTLSClientConfig(pub, priv, ca))
+		if err != nil {
+			t.Error(err)
+		}
+
+		b, err := io.ReadAll(buf)
+		if err != nil {
+			t.Error(err)
+		}
+
+		if string(b) != srv2Resp {
+			t.Errorf("expected response from Server2 to be '%s', instead got: %s", srv2Resp, string(b))
+		}
+	})
+
+	t.Run("Test with only the CA file (see also #6635)", func(t *testing.T) {
+		g, err := NewHTTPGetter()
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		buf, err := g.Get(u.String(), WithURL(u.String()), WithTLSClientConfig("", "", ca))
+		if err != nil {
+			t.Error(err)
+		}
+
+		b, err := io.ReadAll(buf)
+		if err != nil {
+			t.Error(err)
+		}
+
+		if string(b) != srv2Resp {
+			t.Errorf("expected response from Server2 to be '%s', instead got: %s", srv2Resp, string(b))
+		}
+	})
+}
+
 func TestDownloadInsecureSkipTLSVerify(t *testing.T) {
 	ts := httptest.NewTLSServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {}))
 	defer ts.Close()
@@ -450,9 +575,6 @@ func TestHttpClientInsecureSkipVerify(t *testing.T) {
 	if len(transport.TLSClientConfig.Certificates) <= 0 {
 		t.Fatal("transport.TLSClientConfig.Certificates is not present")
 	}
-	if transport.TLSClientConfig.ServerName == "" {
-		t.Fatal("TLSClientConfig.ServerName is blank")
-	}
 }
 
 func verifyInsecureSkipVerify(t *testing.T, g *HTTPGetter, caseName string, expectedValue bool) *http.Transport {
diff --git a/testdata/localhost-crt.pem b/testdata/localhost-crt.pem
new file mode 100644
index 000000000..70fa0a429
--- /dev/null
+++ b/testdata/localhost-crt.pem
@@ -0,0 +1,73 @@
+Certificate:
+    Data:
+        Version: 3 (0x2)
+        Serial Number:
+            7f:5e:fa:21:fa:ee:e4:6a:be:9b:c2:80:bf:ed:42:f3:2d:47:f5:d2
+        Signature Algorithm: sha256WithRSAEncryption
+        Issuer: C=US, ST=CO, L=Boulder, O=Helm, CN=helm.sh
+        Validity
+            Not Before: Nov  6 21:59:18 2023 GMT
+            Not After : Nov  3 21:59:18 2033 GMT
+        Subject: C=CA, ST=ON, L=Kitchener, O=Helm, CN=localhost
+        Subject Public Key Info:
+            Public Key Algorithm: rsaEncryption
+                RSA Public-Key: (2048 bit)
+                Modulus:
+                    00:c8:89:55:0d:0b:f1:da:e6:c0:70:7d:d3:27:cd:
+                    b8:a8:81:8b:7c:a4:89:e5:d1:b1:78:01:1d:df:44:
+                    88:0b:fc:d6:81:35:3d:d1:3b:5e:8f:bb:93:b3:7e:
+                    28:db:ed:ff:a0:13:3a:70:a3:fe:94:6b:0b:fe:fb:
+                    63:00:b0:cb:dc:81:cd:80:dc:d0:2f:bf:b2:4f:9a:
+                    81:d4:22:dc:97:c8:8f:27:86:59:91:fa:92:05:75:
+                    c4:cc:6b:f5:a9:6b:74:1e:f5:db:a9:f8:bf:8c:a2:
+                    25:fd:a0:cc:79:f4:25:57:74:a9:23:9b:e2:b7:22:
+                    7a:14:7a:3d:ea:f1:7e:32:6b:57:6c:2e:c6:4f:75:
+                    54:f9:6b:54:d2:ca:eb:54:1c:af:39:15:9b:d0:7c:
+                    0f:f8:55:51:04:ea:da:fa:7b:8b:63:0f:ac:39:b1:
+                    f6:4b:8e:4e:f6:ea:e9:7b:e6:ba:5e:5a:8e:91:ef:
+                    dc:b1:7d:52:3f:73:83:52:46:83:48:49:ff:f2:2d:
+                    ca:54:f2:36:bb:49:cc:59:99:c0:9e:cf:8e:78:55:
+                    6c:ed:7d:7e:83:b8:59:2c:7d:f8:1a:81:f0:7d:f5:
+                    27:f2:db:ae:d4:31:54:38:fe:47:b2:ee:16:20:0f:
+                    f1:db:2d:28:bf:6f:38:eb:11:bb:9a:d4:b2:5a:3a:
+                    4a:7f
+                Exponent: 65537 (0x10001)
+        X509v3 extensions:
+            X509v3 Subject Alternative Name: 
+                DNS:localhost
+    Signature Algorithm: sha256WithRSAEncryption
+         47:47:fe:29:ca:94:28:75:59:ba:ab:67:ab:c6:a6:0b:0a:f2:
+         0f:26:d9:1d:35:db:68:a5:d8:f5:1f:d1:87:e7:a7:74:fd:c0:
+         22:aa:c8:ec:6c:d3:ac:8a:0b:ed:59:3a:a0:12:77:7c:53:74:
+         fd:30:59:34:8f:a4:ef:5b:98:3f:ff:cf:89:87:ed:d3:7f:41:
+         2f:b1:9a:12:71:bb:fe:3a:cf:77:16:32:bc:83:90:cc:52:2f:
+         3b:f4:ae:db:b1:bb:f0:dd:30:d4:03:17:5e:47:b7:06:86:7a:
+         16:b1:72:2f:80:5d:d4:c0:f9:6c:91:df:5a:c5:15:86:66:68:
+         c8:90:8e:f1:a2:bb:40:0f:ef:26:1b:02:c4:42:de:8c:69:ec:
+         ad:27:d0:bc:da:7c:76:33:86:de:b7:c4:04:64:e6:f6:dc:44:
+         89:7b:b8:2f:c7:28:7a:4c:a6:01:ad:a5:17:64:3a:23:da:aa:
+         db:ce:3f:86:e9:92:dc:0d:c4:5a:b4:52:a8:8a:ee:3d:62:7d:
+         b1:c8:fa:ef:96:2b:ab:f1:e1:6d:6f:7d:1e:ce:bc:7a:d0:92:
+         02:1b:c8:55:36:77:bf:d4:42:d3:fc:57:ca:b7:cc:95:be:ce:
+         f8:6e:b2:28:ca:4d:9a:00:7d:78:c8:56:04:2e:b3:ac:03:fa:
+         05:d8:42:bd
+-----BEGIN CERTIFICATE-----
+MIIDRDCCAiygAwIBAgIUf176Ifru5Gq+m8KAv+1C8y1H9dIwDQYJKoZIhvcNAQEL
+BQAwTTELMAkGA1UEBhMCVVMxCzAJBgNVBAgMAkNPMRAwDgYDVQQHDAdCb3VsZGVy
+MQ0wCwYDVQQKDARIZWxtMRAwDgYDVQQDDAdoZWxtLnNoMB4XDTIzMTEwNjIxNTkx
+OFoXDTMzMTEwMzIxNTkxOFowUTELMAkGA1UEBhMCQ0ExCzAJBgNVBAgMAk9OMRIw
+EAYDVQQHDAlLaXRjaGVuZXIxDTALBgNVBAoMBEhlbG0xEjAQBgNVBAMMCWxvY2Fs
+aG9zdDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAMiJVQ0L8drmwHB9
+0yfNuKiBi3ykieXRsXgBHd9EiAv81oE1PdE7Xo+7k7N+KNvt/6ATOnCj/pRrC/77
+YwCwy9yBzYDc0C+/sk+agdQi3JfIjyeGWZH6kgV1xMxr9alrdB7126n4v4yiJf2g
+zHn0JVd0qSOb4rciehR6PerxfjJrV2wuxk91VPlrVNLK61QcrzkVm9B8D/hVUQTq
+2vp7i2MPrDmx9kuOTvbq6Xvmul5ajpHv3LF9Uj9zg1JGg0hJ//ItylTyNrtJzFmZ
+wJ7PjnhVbO19foO4WSx9+BqB8H31J/LbrtQxVDj+R7LuFiAP8dstKL9vOOsRu5rU
+slo6Sn8CAwEAAaMYMBYwFAYDVR0RBA0wC4IJbG9jYWxob3N0MA0GCSqGSIb3DQEB
+CwUAA4IBAQBHR/4pypQodVm6q2erxqYLCvIPJtkdNdtopdj1H9GH56d0/cAiqsjs
+bNOsigvtWTqgEnd8U3T9MFk0j6TvW5g//8+Jh+3Tf0EvsZoScbv+Os93FjK8g5DM
+Ui879K7bsbvw3TDUAxdeR7cGhnoWsXIvgF3UwPlskd9axRWGZmjIkI7xortAD+8m
+GwLEQt6MaeytJ9C82nx2M4bet8QEZOb23ESJe7gvxyh6TKYBraUXZDoj2qrbzj+G
+6ZLcDcRatFKoiu49Yn2xyPrvliur8eFtb30ezrx60JICG8hVNne/1ELT/FfKt8yV
+vs74brIoyk2aAH14yFYELrOsA/oF2EK9
+-----END CERTIFICATE-----
diff --git a/testdata/openssl.conf b/testdata/openssl.conf
index 9b27e445b..be5ff04b7 100644
--- a/testdata/openssl.conf
+++ b/testdata/openssl.conf
@@ -40,3 +40,7 @@ subjectAltName = @alternate_names
 [alternate_names]
 DNS.1   = helm.sh
 IP.1    = 127.0.0.1
+
+# # Used to generate localhost-crt.pem
+# [alternate_names]
+# DNS.1   = localhost