Do not explicitly set SNI in HTTPGetter

Signed-off-by: Wahab Ali <wahabalimk@gmail.com>
2 years ago · 3ca2558f04
parent 2e6437beb5
commit 3ca2558f04
4 changed files with 192 additions and 10 deletions
--- 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
 	}

--- a/pkg/getter/httpgetter_test.go
+++ b/pkg/getter/httpgetter_test.go
@ -358,6 +358,121 @@ 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.NewClientTLS(pub, priv, ca, 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, r *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.NewClientTLS(pub, priv, ca, 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 +565,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 {
--- a/testdata/localhost-crt.pem
+++ 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-----
--- 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