Force the use of secure TLS config

This change updates the TLS config used by all pinniped components.
There are no configuration knobs associated with this change.  Thus
this change tightens our static defaults.

There are four TLS config levels:

1. Secure (TLS 1.3 only)
2. Default (TLS 1.2+ best ciphers that are well supported)
3. Default LDAP (TLS 1.2+ with less good ciphers)
4. Legacy (currently unused, TLS 1.2+ with all non-broken ciphers)

Highlights per component:

1. pinniped CLI
   - uses "secure" config against KAS
   - uses "default" for all other connections
2. concierge
   - uses "secure" config as an aggregated API server
   - uses "default" config as a impersonation proxy API server
   - uses "secure" config against KAS
   - uses "default" config for JWT authenticater (mostly, see code)
   - no changes to webhook authenticater (see code)
3. supervisor
   - uses "default" config as a server
   - uses "secure" config against KAS
   - uses "default" config against OIDC IDPs
   - uses "default LDAP" config against LDAP IDPs

Signed-off-by: Monis Khan <mok@vmware.com>

internal/concierge/impersonator/impersonator.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"io"
 	"net"
 	"net/http"
 	"net/http/httputil"
@@ -14,6 +15,7 @@ import (
 	"os"
 	"regexp"
 	"strings"
+	"sync"
 	"time"
 
 	authenticationv1 "k8s.io/api/authentication/v1"
@@ -47,6 +49,7 @@ import (
 	"k8s.io/client-go/transport"
 
 	"go.pinniped.dev/internal/constable"
+	"go.pinniped.dev/internal/crypto/ptls"
 	"go.pinniped.dev/internal/dynamiccert"
 	"go.pinniped.dev/internal/httputil/securityheader"
 	"go.pinniped.dev/internal/kubeclient"
@@ -70,13 +73,14 @@ func New(
 	dynamicCertProvider dynamiccert.Private,
 	impersonationProxySignerCA dynamiccert.Public,
 ) (func(stopCh <-chan struct{}) error, error) {
-	return newInternal(port, dynamicCertProvider, impersonationProxySignerCA, nil, nil, nil)
+	return newInternal(port, dynamicCertProvider, impersonationProxySignerCA, kubeclient.Secure, nil, nil, nil)
 }
 
 func newInternal( //nolint:funlen // yeah, it's kind of long.
 	port int,
 	dynamicCertProvider dynamiccert.Private,
 	impersonationProxySignerCA dynamiccert.Public,
+	restConfigFunc ptls.RestConfigFunc, // for unit testing, should always be kubeclient.Secure in production
 	clientOpts []kubeclient.Option, // for unit testing, should always be nil in production
 	recOpts func(*genericoptions.RecommendedOptions), // for unit testing, should always be nil in production
 	recConfig func(*genericapiserver.RecommendedConfig), // for unit testing, should always be nil in production
@@ -100,6 +104,13 @@ func newInternal( //nolint:funlen // yeah, it's kind of long.
 		recommendedOptions.SecureServing.ServerCert.GeneratedCert = dynamicCertProvider // serving certs (end user facing)
 		recommendedOptions.SecureServing.BindPort = port
 
+		// secure TLS for connections coming from external clients and going to the Kube API server
+		// this is best effort because not all options provide the right hooks to override TLS config
+		// since any client could connect to the impersonation proxy, this uses the default TLS config
+		if err := ptls.DefaultRecommendedOptions(recommendedOptions, restConfigFunc); err != nil {
+			return nil, fmt.Errorf("failed to secure recommended options: %w", err)
+		}
+
 		// Wire up the impersonation proxy signer CA as another valid authenticator for client cert auth,
 		// along with the Kube API server's CA.
 		// Note: any changes to the Authentication stack need to be kept in sync with any assumptions made
@@ -258,7 +269,7 @@ func newInternal( //nolint:funlen // yeah, it's kind of long.
 
 		delegatingAuthorizer := serverConfig.Authorization.Authorizer
 		customReasonAuthorizer := &comparableAuthorizer{
-			authorizerFunc: func(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) {
+			AuthorizerFunc: func(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) {
 				const baseReason = "decision made by impersonation-proxy.concierge.pinniped.dev"
 				switch a.GetVerb() {
 				case "":
@@ -347,14 +358,14 @@ func getReverseProxyClient(clientOpts []kubeclient.Option) (*kubeclient.Client,
 	if err != nil {
 		return nil, err
 	}
-	impersonationProxyRestConfig = rest.AnonymousClientConfig(impersonationProxyRestConfig)
+	impersonationProxyRestConfig = kubeclient.SecureAnonymousClientConfig(impersonationProxyRestConfig)
 	impersonationProxyRestConfig.BearerTokenFile = tokenFile
 
 	return kubeclient.New(kubeclient.WithConfig(impersonationProxyRestConfig))
 }
 
 func isAnonymousAuthEnabled(config *rest.Config) (bool, error) {
-	anonymousConfig := rest.AnonymousClientConfig(config)
+	anonymousConfig := kubeclient.SecureAnonymousClientConfig(config)
 
 	// we do not need either of these but RESTClientFor complains if they are not set
 	anonymousConfig.GroupVersion = &schema.GroupVersion{}
@@ -413,14 +424,7 @@ type comparableAuthenticator struct {
 
 // No-op wrapping around AuthorizerFunc to allow for comparisons.
 type comparableAuthorizer struct {
-	authorizerFunc
-}
-
-// TODO: delete when we pick up https://github.com/kubernetes/kubernetes/pull/100963
-type authorizerFunc func(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error)
-
-func (f authorizerFunc) Authorize(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) {
-	return f(ctx, a)
+	authorizer.AuthorizerFunc
 }
 
 func withBearerTokenPreservation(delegate http.Handler) http.Handler {
@@ -462,16 +466,16 @@ func newImpersonationReverseProxyFunc(restConfig *rest.Config) (func(*genericapi
 	if err != nil {
 		return nil, fmt.Errorf("could not get http/1.1 round tripper: %w", err)
 	}
-	http1RoundTripperAnonymous, err := getTransportForProtocol(rest.AnonymousClientConfig(restConfig), "http/1.1")
+	http1RoundTripperAnonymous, err := getTransportForProtocol(kubeclient.SecureAnonymousClientConfig(restConfig), "http/1.1")
 	if err != nil {
 		return nil, fmt.Errorf("could not get http/1.1 anonymous round tripper: %w", err)
 	}
 
-	http2RoundTripper, err := getTransportForProtocol(restConfig, "h2")
+	http2RoundTripper, err := getTransportForProtocol(restConfig, "h2") // TODO figure out why this leads to still supporting http1
 	if err != nil {
 		return nil, fmt.Errorf("could not get http/2.0 round tripper: %w", err)
 	}
-	http2RoundTripperAnonymous, err := getTransportForProtocol(rest.AnonymousClientConfig(restConfig), "h2")
+	http2RoundTripperAnonymous, err := getTransportForProtocol(kubeclient.SecureAnonymousClientConfig(restConfig), "h2")
 	if err != nil {
 		return nil, fmt.Errorf("could not get http/2.0 anonymous round tripper: %w", err)
 	}
@@ -565,6 +569,12 @@ func newImpersonationReverseProxyFunc(restConfig *rest.Config) (func(*genericapi
 				r.Header.Del("X-Forwarded-For")
 			}
 
+			// the http2 code seems to call Close concurrently which can lead to data races
+			if r.Body != nil {
+				r = utilnet.CloneRequest(r)
+				r.Body = &safeReadWriteCloser{rc: r.Body}
+			}
+
 			reverseProxy := httputil.NewSingleHostReverseProxy(serverURL)
 			reverseProxy.Transport = rt
 			reverseProxy.FlushInterval = 200 * time.Millisecond // the "watch" verb will not work without this line
@@ -573,6 +583,43 @@ func newImpersonationReverseProxyFunc(restConfig *rest.Config) (func(*genericapi
 	}, nil
 }
 
+var _ io.ReadWriteCloser = &safeReadWriteCloser{}
+
+type safeReadWriteCloser struct {
+	m sync.Mutex // all methods allowed concurrently, this only guards concurrent calls to Close
+
+	rc io.ReadCloser
+
+	once     sync.Once // set up rwc and writeErr
+	rwc      io.ReadWriteCloser
+	writeErr error
+}
+
+func (r *safeReadWriteCloser) Read(p []byte) (int, error) {
+	return r.rc.Read(p)
+}
+
+func (r *safeReadWriteCloser) Write(p []byte) (int, error) {
+	r.once.Do(func() {
+		var ok bool
+		r.rwc, ok = r.rc.(io.ReadWriteCloser)
+		if !ok { // this method should only be caused during flows that switch protocols
+			r.writeErr = fmt.Errorf("switching protocols failed: io.ReadCloser %T is not io.ReadWriteCloser", r.rc)
+		}
+	})
+	if r.writeErr != nil {
+		return 0, r.writeErr
+	}
+	return r.rwc.Write(p)
+}
+
+func (r *safeReadWriteCloser) Close() error {
+	r.m.Lock()
+	defer r.m.Unlock()
+
+	return r.rc.Close()
+}
+
 func ensureNoImpersonationHeaders(r *http.Request) error {
 	for key := range r.Header {
 		// even though we have unit tests that try to cover this case, it is hard to tell if Go does
@@ -759,5 +806,14 @@ func getTransportForProtocol(restConfig *rest.Config, protocol string) (http.Rou
 	}
 	transportConfig.TLS.NextProtos = []string{protocol}
 
-	return transport.New(transportConfig)
+	rt, err := transport.New(transportConfig)
+	if err != nil {
+		return nil, fmt.Errorf("could not build transport: %w", err)
+	}
+
+	if err := kubeclient.AssertSecureTransport(rt); err != nil {
+		return nil, err // make sure we only use a secure TLS config
+	}
+
+	return rt, nil
 }

internal/concierge/impersonator/impersonator_test.go

@@ -36,6 +36,7 @@ import (
 	genericapiserver "k8s.io/apiserver/pkg/server"
 	genericoptions "k8s.io/apiserver/pkg/server/options"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
+	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd/api"
 	featuregatetesting "k8s.io/component-base/featuregate/testing"
@@ -44,12 +45,13 @@ import (
 	loginv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/login/v1alpha1"
 	"go.pinniped.dev/internal/certauthority"
 	"go.pinniped.dev/internal/constable"
+	"go.pinniped.dev/internal/crypto/ptls"
 	"go.pinniped.dev/internal/dynamiccert"
 	"go.pinniped.dev/internal/groupsuffix"
 	"go.pinniped.dev/internal/here"
 	"go.pinniped.dev/internal/httputil/roundtripper"
 	"go.pinniped.dev/internal/kubeclient"
-	"go.pinniped.dev/internal/testutil"
+	"go.pinniped.dev/internal/testutil/tlsserver"
 )
 
 func TestImpersonator(t *testing.T) {
@@ -697,7 +699,9 @@ func TestImpersonator(t *testing.T) {
 			// will proxy incoming calls to this fake server.
 			testKubeAPIServerWasCalled := false
 			var testKubeAPIServerSawHeaders http.Header
-			testKubeAPIServerCA, testKubeAPIServerURL := testutil.TLSTestServer(t, func(w http.ResponseWriter, r *http.Request) {
+			testKubeAPIServer := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				tlsserver.AssertTLS(t, r, ptls.Secure)
+
 				switch r.URL.Path {
 				case "/api/v1/namespaces/kube-system/configmaps":
 					require.Equal(t, http.MethodGet, r.Method)
@@ -708,6 +712,13 @@ func TestImpersonator(t *testing.T) {
 					http.NotFound(w, r)
 					return
 
+				case "/apis/flowcontrol.apiserver.k8s.io/v1beta1/prioritylevelconfigurations",
+					"/apis/flowcontrol.apiserver.k8s.io/v1beta1/flowschemas":
+					// ignore requests related to priority and fairness logic
+					require.Equal(t, http.MethodGet, r.Method)
+					http.NotFound(w, r)
+					return
+
 				case "/api/v1/namespaces":
 					require.Equal(t, http.MethodGet, r.Method)
 
@@ -785,13 +796,13 @@ func TestImpersonator(t *testing.T) {
 					require.Fail(t, "fake Kube API server got an unexpected request", "path: %s", r.URL.Path)
 					return
 				}
-			})
+			}), tlsserver.RecordTLSHello)
 
 			// Create the client config that the impersonation server should use to talk to the Kube API server.
 			testKubeAPIServerKubeconfig := rest.Config{
-				Host:            testKubeAPIServerURL,
+				Host:            testKubeAPIServer.URL,
 				BearerToken:     "some-service-account-token",
-				TLSClientConfig: rest.TLSClientConfig{CAData: []byte(testKubeAPIServerCA)},
+				TLSClientConfig: rest.TLSClientConfig{CAData: tlsserver.TLSTestServerCA(testKubeAPIServer)},
 				BearerTokenFile: tt.kubeAPIServerClientBearerTokenFile,
 			}
 			clientOpts := []kubeclient.Option{kubeclient.WithConfig(&testKubeAPIServerKubeconfig)}
@@ -800,7 +811,6 @@ func TestImpersonator(t *testing.T) {
 			recOpts := func(options *genericoptions.RecommendedOptions) {
 				options.Authentication.RemoteKubeConfigFileOptional = true
 				options.Authorization.RemoteKubeConfigFileOptional = true
-				options.CoreAPI = nil
 				options.Admission = nil
 				options.SecureServing.Listener = listener // use our listener with the dynamic port
 			}
@@ -814,8 +824,8 @@ func TestImpersonator(t *testing.T) {
 			// Allow standard REST verbs to be authorized so that tests pass without invasive changes
 			recConfig := func(config *genericapiserver.RecommendedConfig) {
 				authz := config.Authorization.Authorizer.(*comparableAuthorizer)
-				delegate := authz.authorizerFunc
-				authz.authorizerFunc = func(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) {
+				delegate := authz.AuthorizerFunc
+				authz.AuthorizerFunc = func(ctx context.Context, a authorizer.Attributes) (authorizer.Decision, string, error) {
 					recorder.record(a)
 					switch a.GetVerb() {
 					case "create", "get", "list":
@@ -826,8 +836,15 @@ func TestImpersonator(t *testing.T) {
 				}
 			}
 
+			restConfigFunc := func(config *rest.Config) (kubernetes.Interface, *rest.Config, error) {
+				if config == nil {
+					config = &testKubeAPIServerKubeconfig
+				}
+				return kubeclient.Secure(config)
+			}
+
 			// Create an impersonator.  Use an invalid port number to make sure our listener override works.
-			runner, constructionErr := newInternal(-1000, certKeyContent, caContent, clientOpts, recOpts, recConfig)
+			runner, constructionErr := newInternal(-1000, certKeyContent, caContent, restConfigFunc, clientOpts, recOpts, recConfig)
 			if len(tt.wantConstructionError) > 0 {
 				require.EqualError(t, constructionErr, tt.wantConstructionError)
 				require.Nil(t, runner)
@@ -864,7 +881,7 @@ func TestImpersonator(t *testing.T) {
 						return rt
 					}
 
-					return roundtripper.Func(func(req *http.Request) (*http.Response, error) {
+					return roundtripper.WrapFunc(rt, func(req *http.Request) (*http.Response, error) {
 						req = req.Clone(req.Context())
 						tt.clientMutateHeaders(req.Header)
 						return rt.RoundTrip(req)
@@ -928,10 +945,10 @@ func TestImpersonator(t *testing.T) {
 
 			// anonymous TCR should always work
 
-			tcrRegGroup, err := kubeclient.New(kubeclient.WithConfig(rest.AnonymousClientConfig(clientKubeconfig)))
+			tcrRegGroup, err := kubeclient.New(kubeclient.WithConfig(kubeclient.SecureAnonymousClientConfig(clientKubeconfig)))
 			require.NoError(t, err)
 
-			tcrOtherGroup, err := kubeclient.New(kubeclient.WithConfig(rest.AnonymousClientConfig(clientKubeconfig)),
+			tcrOtherGroup, err := kubeclient.New(kubeclient.WithConfig(kubeclient.SecureAnonymousClientConfig(clientKubeconfig)),
 				kubeclient.WithMiddleware(groupsuffix.New("walrus.tld")))
 			require.NoError(t, err)
 
@@ -950,7 +967,7 @@ func TestImpersonator(t *testing.T) {
 
 			// these calls should only work when anonymous auth is enabled
 
-			anonymousConfig := rest.AnonymousClientConfig(clientKubeconfig)
+			anonymousConfig := kubeclient.SecureAnonymousClientConfig(clientKubeconfig)
 			anonymousConfig.GroupVersion = &schema.GroupVersion{
 				Group:   "not-concierge.walrus.tld",
 				Version: "v1",
@@ -989,7 +1006,7 @@ func TestImpersonator(t *testing.T) {
 
 			// this should always fail as unauthorized (even for TCR) because the cert is not valid
 
-			badCertConfig := rest.AnonymousClientConfig(clientKubeconfig)
+			badCertConfig := kubeclient.SecureAnonymousClientConfig(clientKubeconfig)
 			badCert := newClientCert(t, unrelatedCA, "bad-user", []string{"bad-group"})
 			badCertConfig.TLSClientConfig.CertData = badCert.certPEM
 			badCertConfig.TLSClientConfig.KeyData = badCert.keyPEM
@@ -1041,7 +1058,7 @@ func TestImpersonatorHTTPHandler(t *testing.T) {
 				ExecProvider: &api.ExecConfig{},
 				AuthProvider: &api.AuthProviderConfig{},
 			},
-			wantCreationErr: "could not get http/1.1 round tripper: could not get in-cluster transport config: execProvider and authProvider cannot be used in combination",
+			wantCreationErr: "could not create secure client config: execProvider and authProvider cannot be used in combination",
 		},
 		{
 			name: "fail to get transport from config",
@@ -1051,7 +1068,7 @@ func TestImpersonatorHTTPHandler(t *testing.T) {
 				Transport:       http.DefaultTransport,
 				TLSClientConfig: rest.TLSClientConfig{Insecure: true},
 			},
-			wantCreationErr: "could not get http/1.1 round tripper: using a custom transport with TLS certificate options or the insecure flag is not allowed",
+			wantCreationErr: "could not create secure client config: failed to build transport: using a custom transport with TLS certificate options or the insecure flag is not allowed",
 		},
 		{
 			name:           "Impersonate-User header already in request",
@@ -1761,7 +1778,9 @@ func TestImpersonatorHTTPHandler(t *testing.T) {
 
 			testKubeAPIServerWasCalled := false
 			testKubeAPIServerSawHeaders := http.Header{}
-			testKubeAPIServerCA, testKubeAPIServerURL := testutil.TLSTestServer(t, func(w http.ResponseWriter, r *http.Request) {
+			testKubeAPIServer := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				tlsserver.AssertTLS(t, r, ptls.Secure)
+
 				testKubeAPIServerWasCalled = true
 				testKubeAPIServerSawHeaders = r.Header
 				if tt.kubeAPIServerStatusCode != http.StatusOK {
@@ -1769,17 +1788,26 @@ func TestImpersonatorHTTPHandler(t *testing.T) {
 				} else {
 					_, _ = w.Write([]byte("successful proxied response"))
 				}
-			})
+			}), tlsserver.RecordTLSHello)
+
 			testKubeAPIServerKubeconfig := rest.Config{
-				Host:            testKubeAPIServerURL,
+				Host:            testKubeAPIServer.URL,
 				BearerToken:     "some-service-account-token",
-				TLSClientConfig: rest.TLSClientConfig{CAData: []byte(testKubeAPIServerCA)},
+				TLSClientConfig: rest.TLSClientConfig{CAData: tlsserver.TLSTestServerCA(testKubeAPIServer)},
 			}
 			if tt.restConfig == nil {
 				tt.restConfig = &testKubeAPIServerKubeconfig
 			}
 
-			impersonatorHTTPHandlerFunc, err := newImpersonationReverseProxyFunc(tt.restConfig)
+			// mimic how newInternal would call newImpersonationReverseProxyFunc
+			impersonatorHTTPHandlerFunc, err := func() (func(*genericapiserver.Config) http.Handler, error) {
+				kubeClientForProxy, err := kubeclient.New(kubeclient.WithConfig(tt.restConfig))
+				if err != nil {
+					return nil, err
+				}
+				return newImpersonationReverseProxyFunc(rest.CopyConfig(kubeClientForProxy.ProtoConfig))
+			}()
+
 			if tt.wantCreationErr != "" {
 				require.EqualError(t, err, tt.wantCreationErr)
 				require.Nil(t, impersonatorHTTPHandlerFunc)

internal/concierge/server/server.go

@@ -29,10 +29,12 @@ import (
 	"go.pinniped.dev/internal/controller/authenticator/authncache"
 	"go.pinniped.dev/internal/controllerinit"
 	"go.pinniped.dev/internal/controllermanager"
+	"go.pinniped.dev/internal/crypto/ptls"
 	"go.pinniped.dev/internal/downward"
 	"go.pinniped.dev/internal/dynamiccert"
 	"go.pinniped.dev/internal/here"
 	"go.pinniped.dev/internal/issuer"
+	"go.pinniped.dev/internal/kubeclient"
 	"go.pinniped.dev/internal/plog"
 	"go.pinniped.dev/internal/registry/credentialrequest"
 )
@@ -209,6 +211,13 @@ func getAggregatedAPIServerConfig(
 	recommendedOptions.SecureServing.ServerCert.GeneratedCert = dynamicCertProvider
 	recommendedOptions.SecureServing.BindPort = 8443 // Don't run on default 443 because that requires root
 
+	// secure TLS for connections coming from and going to the Kube API server
+	// this is best effort because not all options provide the right hooks to override TLS config
+	// since our only client is the Kube API server, this uses the most secure TLS config
+	if err := ptls.SecureRecommendedOptions(recommendedOptions, kubeclient.Secure); err != nil {
+		return nil, fmt.Errorf("failed to secure recommended options: %w", err)
+	}
+
 	serverConfig := genericapiserver.NewRecommendedConfig(codecs)
 	// Note that among other things, this ApplyTo() function copies
 	// `recommendedOptions.SecureServing.ServerCert.GeneratedCert` into
@@ -220,7 +229,7 @@ func getAggregatedAPIServerConfig(
 	// If the provider later starts returning certs, then the API server
 	// will use them to handle the incoming requests successfully.
 	if err := recommendedOptions.ApplyTo(serverConfig); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to apply recommended options: %w", err)
 	}
 
 	apiServerConfig := &apiserver.Config{