Repository: vmware-tanzu/pinniped (go)

Issue:
TLS follow-up: determine why http2Round only transport still supports http1
The http2RoundTripper will speak protocols other than http 2 (e.g. http1). 

It shouldn't. 

Please fix this. If it turns out that this outcome cannot be realized because we are misunderstanding what support client-go provides on this subject, then let's discuss how we should change the code to better work with that library.

Follow-up from #820

https://github.com/vmware-tanzu/pinniped/blob/cd686ffdf31ae2a5f7241cd8d3ddf304c0684a19/internal/concierge/impersonator/impersonator.go#L474

Pull request description:
the http2RoundTripper should only use http2
```release-note
Fixed bug where http2-only client was also allowing http1.1.
```

Fixes #896


Patch:
diff --git a/internal/concierge/impersonator/impersonator.go b/internal/concierge/impersonator/impersonator.go
index 828d94e9..70155dba 100644
--- a/internal/concierge/impersonator/impersonator.go
+++ b/internal/concierge/impersonator/impersonator.go
@@ -472,7 +472,7 @@ func newImpersonationReverseProxyFunc(restConfig *rest.Config) (func(*genericapi
 		return nil, fmt.Errorf("could not get http/1.1 anonymous round tripper: %w", err)
 	}
 
-	http2RoundTripper, err := getTransportForProtocol(restConfig, "h2") // TODO figure out why this leads to still supporting http1
+	http2RoundTripper, err := getTransportForProtocol(restConfig, "h2")
 	if err != nil {
 		return nil, fmt.Errorf("could not get http/2.0 round tripper: %w", err)
 	}
@@ -812,6 +812,15 @@ func getTransportForProtocol(restConfig *rest.Config, protocol string) (http.Rou
 		return nil, fmt.Errorf("could not build transport: %w", err)
 	}
 
+	// For clients that support http2, transport.New calls http2.ConfigureTransports,
+	// which configures with both h2 and http/1.1,
+	// even when you explicitly only ask for h2.
+	// Override that change.
+	cfg, err := utilnet.TLSClientConfig(rt)
+	if err != nil {
+		return nil, fmt.Errorf("could not extract TLS config: %w", err)
+	}
+	cfg.NextProtos = []string{protocol}
 	if err := kubeclient.AssertSecureTransport(rt); err != nil {
 		return nil, err // make sure we only use a secure TLS config
 	}


Test patch:
diff --git a/internal/concierge/impersonator/impersonator_test.go b/internal/concierge/impersonator/impersonator_test.go
index 6eedd950..cf68a510 100644
--- a/internal/concierge/impersonator/impersonator_test.go
+++ b/internal/concierge/impersonator/impersonator_test.go
@@ -5,6 +5,8 @@ package impersonator
 
 import (
 	"context"
+	"crypto/tls"
+	"crypto/x509"
 	"fmt"
 	"math/rand"
 	"net"
@@ -701,7 +703,28 @@ func TestImpersonator(t *testing.T) {
 			testKubeAPIServerWasCalled := false
 			var testKubeAPIServerSawHeaders http.Header
 			testKubeAPIServer := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-				tlsserver.AssertTLS(t, r, ptls.Secure)
+				tlsConfigFunc := func(rootCAs *x509.CertPool) *tls.Config {
+					// Requests to get configmaps, flowcontrol requests, and healthz requests
+					// are not done by our http round trippers that specify only one protocol
+					// (either http1.1 or http2, not both).
+					// For all other requests from the impersonator, if it is not an upgrade
+					// request it should only be using http2.
+					// If it is an upgrade request it should only be using http1.1, but that
+					// is covered by the AssertTLS function.
+					secure := ptls.Secure(rootCAs)
+					switch r.URL.Path {
+					case "/api/v1/namespaces/kube-system/configmaps",
+						"/apis/flowcontrol.apiserver.k8s.io/v1beta2/prioritylevelconfigurations",
+						"/apis/flowcontrol.apiserver.k8s.io/v1beta2/flowschemas",
+						"/healthz":
+					default:
+						if !httpstream.IsUpgradeRequest(r) {
+							secure.NextProtos = []string{secure.NextProtos[0]}
+						}
+					}
+					return secure
+				}
+				tlsserver.AssertTLS(t, r, tlsConfigFunc)
 
 				switch r.URL.Path {
 				case "/api/v1/namespaces/kube-system/configmaps":
@@ -1780,7 +1803,28 @@ func TestImpersonatorHTTPHandler(t *testing.T) {
 			testKubeAPIServerWasCalled := false
 			testKubeAPIServerSawHeaders := http.Header{}
 			testKubeAPIServer := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-				tlsserver.AssertTLS(t, r, ptls.Secure)
+				tlsConfigFunc := func(rootCAs *x509.CertPool) *tls.Config {
+					// Requests to get configmaps, flowcontrol requests, and healthz requests
+					// are not done by our http round trippers that specify only one protocol
+					// (either http1.1 or http2, not both).
+					// For all other requests from the impersonator, if it is not an upgrade
+					// request it should only be using http2.
+					// If it is an upgrade request it should only be using http1.1, but that
+					// is covered by the AssertTLS function.
+					secure := ptls.Secure(rootCAs)
+					switch r.URL.Path {
+					case "/api/v1/namespaces/kube-system/configmaps",
+						"/apis/flowcontrol.apiserver.k8s.io/v1beta2/prioritylevelconfigurations",
+						"/apis/flowcontrol.apiserver.k8s.io/v1beta2/flowschemas",
+						"/healthz":
+					default:
+						if !httpstream.IsUpgradeRequest(r) {
+							secure.NextProtos = []string{secure.NextProtos[0]}
+						}
+					}
+					return secure
+				}
+				tlsserver.AssertTLS(t, r, tlsConfigFunc)
 
 				testKubeAPIServerWasCalled = true
 				testKubeAPIServerSawHeaders = r.Header