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 <[email protected]>

CONTRIBUTING.md

@@ -114,11 +114,12 @@ docker build .
  - [`kind`](https://kind.sigs.k8s.io/docs/user/quick-start)
  - [`kubectl`](https://kubernetes.io/docs/tasks/tools/install-kubectl/)
  - [`ytt`](https://carvel.dev/#getting-started)
+ - [`nmap`](https://nmap.org/download.html)
 
  On macOS, these tools can be installed with [Homebrew](https://brew.sh/) (assuming you have Chrome installed already):
 
  ```bash
- brew install kind k14s/tap/ytt k14s/tap/kapp kubectl chromedriver && brew cask install docker
+ brew install kind k14s/tap/ytt k14s/tap/kapp kubectl chromedriver nmap && brew cask install docker
  ```
 
 1. Create a kind cluster, compile, create container images, and install Pinniped and supporting dependencies using:

cmd/pinniped/cmd/kubeconfig.go

@@ -5,7 +5,6 @@ package cmd
 
 import (
  "context"
- "crypto/tls"
  "crypto/x509"
  "encoding/base64"
  "encoding/json"
@@ -28,13 +27,13 @@ import (
  _ "k8s.io/client-go/plugin/pkg/client/auth" // Adds handlers for various dynamic auth plugins in client-go
  "k8s.io/client-go/tools/clientcmd"
  clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
- "k8s.io/client-go/transport"
 
  conciergev1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/authentication/v1alpha1"
  configv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/config/v1alpha1"
  idpdiscoveryv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1"
  conciergeclientset "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned"
  "go.pinniped.dev/internal/groupsuffix"
+ "go.pinniped.dev/internal/net/phttp"
 )
 
 type kubeconfigDeps struct {
@@ -664,17 +663,8 @@ func validateKubeconfig(ctx context.Context, flags getKubeconfigParams, kubeconf
  return fmt.Errorf("invalid kubeconfig (no certificateAuthorityData)")
  }
 
- httpClient := &http.Client{
- Transport: &http.Transport{
- TLSClientConfig: &tls.Config{
- MinVersion: tls.VersionTLS12,
- RootCAs: kubeconfigCA,
- },
- Proxy: http.ProxyFromEnvironment,
- TLSHandshakeTimeout: 10 * time.Second,
- },
- Timeout: 10 * time.Second,
- }
+ httpClient := phttp.Default(kubeconfigCA)
+ httpClient.Timeout = 10 * time.Second
 
  ticker := time.NewTicker(2 * time.Second)
  defer ticker.Stop()
@@ -781,21 +771,14 @@ func discoverSupervisorUpstreamIDP(ctx context.Context, flags *getKubeconfigPara
 }
 
 func newDiscoveryHTTPClient(caBundleFlag caBundleFlag) (*http.Client, error) {
- t := &http.Transport{
- TLSClientConfig: &tls.Config{MinVersion: tls.VersionTLS12},
- Proxy: http.ProxyFromEnvironment,
- }
- httpClient := &http.Client{Transport: t}
+ var rootCAs *x509.CertPool
  if caBundleFlag != nil {
- rootCAs := x509.NewCertPool()
- ok := rootCAs.AppendCertsFromPEM(caBundleFlag)
- if !ok {
+ rootCAs = x509.NewCertPool()
+ if ok := rootCAs.AppendCertsFromPEM(caBundleFlag); !ok {
  return nil, fmt.Errorf("unable to fetch OIDC discovery data from issuer: could not parse CA bundle")
  }
- t.TLSClientConfig.RootCAs = rootCAs
  }
- httpClient.Transport = transport.DebugWrappers(httpClient.Transport)
- return httpClient, nil
+ return phttp.Default(rootCAs), nil
 }
 
 func discoverIDPsDiscoveryEndpointURL(ctx context.Context, issuer string, httpClient *http.Client) (string, error) {

cmd/pinniped/cmd/login_oidc.go

@@ -5,7 +5,6 @@ package cmd
 
 import (
  "context"
- "crypto/tls"
  "crypto/x509"
  "encoding/base64"
  "encoding/json"
@@ -21,12 +20,12 @@ import (
  "github.com/spf13/cobra"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
  clientauthv1beta1 "k8s.io/client-go/pkg/apis/clientauthentication/v1beta1"
- "k8s.io/client-go/transport"
  "k8s.io/klog/v2/klogr"
 
  idpdiscoveryv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1"
  "go.pinniped.dev/internal/execcredcache"
  "go.pinniped.dev/internal/groupsuffix"
+ "go.pinniped.dev/internal/net/phttp"
  "go.pinniped.dev/internal/plog"
  "go.pinniped.dev/pkg/conciergeclient"
  "go.pinniped.dev/pkg/oidcclient"
@@ -308,18 +307,7 @@ func makeClient(caBundlePaths []string, caBundleData []string) (*http.Client, er
  }
  pool.AppendCertsFromPEM(pem)
  }
- client := &http.Client{
- Transport: &http.Transport{
- Proxy: http.ProxyFromEnvironment,
- TLSClientConfig: &tls.Config{
- RootCAs: pool,
- MinVersion: tls.VersionTLS12,
- },
- },
- }
-
- client.Transport = transport.DebugWrappers(client.Transport)
- return client, nil
+ return phttp.Default(pool), nil
 }
 
 func tokenCredential(token *oidctypes.Token) *clientauthv1beta1.ExecCredential {
@@ -338,7 +326,7 @@ func tokenCredential(token *oidctypes.Token) *clientauthv1beta1.ExecCredential {
  return &cred
 }
 
-func SetLogLevel(lookupEnv func(string) (string, bool)) (*plog.PLogger, error) {
+func SetLogLevel(lookupEnv func(string) (string, bool)) (plog.Logger, error) {
  debug, _ := lookupEnv("PINNIPED_DEBUG")
  if debug == "true" {
  err := plog.ValidateAndSetLogLevelGlobally(plog.LevelDebug)
@@ -347,7 +335,7 @@ func SetLogLevel(lookupEnv func(string) (string, bool)) (*plog.PLogger, error) {
  }
  }
  logger := plog.New("Pinniped login: ")
- return &logger, nil
+ return logger, nil
 }
 
 // mustGetConfigDir returns a directory that follows the XDG base directory convention:

hack/prepare-for-integration-tests.sh

@@ -132,6 +132,7 @@ check_dependency kubectl "Please install kubectl. e.g. 'brew install kubectl' fo
 check_dependency htpasswd "Please install htpasswd. Should be pre-installed on MacOS. Usually found in 'apache2-utils' package for linux."
 check_dependency openssl "Please install openssl. Should be pre-installed on MacOS."
 check_dependency chromedriver "Please install chromedriver. e.g. 'brew install chromedriver' for MacOS"
+check_dependency nmap "Please install nmap. e.g. 'brew install nmap' for MacOS"
 
 # Check that Chrome and chromedriver versions match. If chromedriver falls a couple versions behind
 # then usually tests start to fail with strange error messages.
@@ -158,6 +159,12 @@ if [ "$(kubectl version --client=true --short | cut -d '.' -f 2)" -lt 18 ]; then
  exit 1
 fi
 
+# Require nmap >= 7.92.x
+if [ "$(nmap -V | grep 'Nmap version' | cut -d ' ' -f 3 | cut -d '.' -f 2)" -lt 92 ]; then
+ log_error "nmap >= 7.92.x is required, you have $(nmap -V | grep 'Nmap version' | cut -d ' ' -f 3)"
+ exit 1
+fi
+
 if [[ "$clean_kind" == "yes" ]]; then
  log_note "Deleting running kind cluster to prepare from a clean slate..."
  ./hack/kind-down.sh

internal/concierge/impersonator/impersonator.go

@@ -7,13 +7,15 @@ import (
  "context"
  "encoding/json"
  "fmt"
+ "io"
  "net"
  "net/http"
  "net/http/httputil"
  "net/url"
  "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
 }