Cleaned up some todos, added some comments

Signed-off-by: Margo Crawford <margaretc@vmware.com>

internal/concierge/impersonator/impersonator_test.go

@@ -701,7 +701,7 @@ 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(nil))
+				tlsserver.AssertTLS(t, r, ptls.Secure)
 
 				switch r.URL.Path {
 				case "/api/v1/namespaces/kube-system/configmaps":
@@ -1780,7 +1780,7 @@ 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(nil))
+				tlsserver.AssertTLS(t, r, ptls.Secure)
 
 				testKubeAPIServerWasCalled = true
 				testKubeAPIServerSawHeaders = r.Header

internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go

@@ -59,7 +59,7 @@ func TestController(t *testing.T) {
 
 	mux := http.NewServeMux()
 	server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		tlsserver.AssertTLS(t, r, ptls.Default(nil))
+		tlsserver.AssertTLS(t, r, ptls.Default)
 		mux.ServeHTTP(w, r)
 	}), tlsserver.RecordTLSHello)
 

internal/controller/kubecertagent/pod_command_executor_test.go

@@ -19,7 +19,7 @@ import (
 func TestSecureTLS(t *testing.T) {
 	var sawRequest bool
 	server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
-		tlsserver.AssertTLS(t, r, ptls.Secure(nil))
+		tlsserver.AssertTLS(t, r, ptls.Secure)
 		sawRequest = true
 	}), tlsserver.RecordTLSHello)
 

internal/crypto/ptls/fips_strict.go

@@ -1,6 +1,9 @@
 // Copyright 2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
+// The configurations here override the usual ptls.Secure, ptls.Default, and ptls.DefaultLDAP
+// configs when Pinniped is built in fips-only mode.
+// All of these are the same because FIPs is already so limited.
 //go:build fips_strict
 // +build fips_strict
 
@@ -25,23 +28,9 @@ func init() {
 	}()
 }
 
-// FIPS does not support TLS 1.3.
-// Therefore, we cannot use Pinniped's usual secure configuration,
-// which requires TLS 1.3.
-// Secure is just a wrapper for Default in this case.
-func Secure(rootCAs *x509.CertPool) *tls.Config {
-	return Default(rootCAs)
-}
-
 func Default(rootCAs *x509.CertPool) *tls.Config {
 	return &tls.Config{
-		// Can't use SSLv3 because of POODLE and BEAST
+		// goboring requires TLS 1.2 and only TLS 1.2
-		// Can't use TLSv1.0 because of POODLE and BEAST using CBC cipher
-		// Can't use TLSv1.1 because of RC4 cipher usage
-		//
-		// The Kubernetes API Server must use TLS 1.2, at a minimum,
-		// to protect the confidentiality of sensitive data during electronic dissemination.
-		// https://stigviewer.com/stig/kubernetes/2021-06-17/finding/V-242378
 		MinVersion: SecureTLSConfigMinTLSVersion,
 
 		// enable HTTP2 for go's 1.7 HTTP Server
@@ -51,10 +40,15 @@ func Default(rootCAs *x509.CertPool) *tls.Config {
 
 		// optional root CAs, nil means use the host's root CA set
 		RootCAs: rootCAs,
+
+		// Don't set CipherSuites, which means it will default to the FIPS-compatible ones.
 	}
 }
 
-func DefaultLDAP(rootCAs *x509.CertPool) *tls.Config {
+func Secure(rootCAs *x509.CertPool) *tls.Config {
-	c := Default(rootCAs)
+	return Default(rootCAs)
-	return c
+}
+
+func DefaultLDAP(rootCAs *x509.CertPool) *tls.Config {
+	return Default(rootCAs)
 }

internal/net/phttp/phttp_test.go

@@ -84,7 +84,7 @@ func TestClient(t *testing.T) {
 
 			var sawRequest bool
 			server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
-				tlsserver.AssertTLS(t, r, tt.configFunc(nil))
+				tlsserver.AssertTLS(t, r, tt.configFunc)
 				assertUserAgent(t, r)
 				sawRequest = true
 			}), tlsserver.RecordTLSHello)

internal/testutil/fakekubeapi/fakekubeapi.go

@@ -56,7 +56,7 @@ func Start(t *testing.T, resources map[string]runtime.Object) (*httptest.Server,
 	}
 
 	server := tlsserver.TLSTestServer(t, httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error {
-		tlsserver.AssertTLS(t, r, ptls.Secure(nil))
+		tlsserver.AssertTLS(t, r, ptls.Secure)
 
 		obj, err := decodeObj(r)
 		if err != nil {

internal/testutil/tlsserver/tlsserver.go

@@ -67,9 +67,15 @@ func RecordTLSHello(server *httptest.Server) {
 	}
 }
 
-// TODO maybe change this back to just taking a configfunc and making a wrapper for the
+func AssertTLS(t *testing.T, r *http.Request, tlsConfigFunc ptls.ConfigFunc) {
-//  fips stuff
+	t.Helper()
-func AssertTLS(t *testing.T, r *http.Request, tlsConfig *tls.Config) {
+
+	tlsConfig := tlsConfigFunc(nil)
+
+	AssertTLSConfig(t, r, tlsConfig)
+}
+
+func AssertTLSConfig(t *testing.T, r *http.Request, tlsConfig *tls.Config) {
 	t.Helper()
 
 	m, ok := getCtxMap(r.Context())

internal/upstreamldap/upstreamldap_test.go

@@ -1820,7 +1820,7 @@ func TestRealTLSDialing(t *testing.T) {
 				_, _ = recordFunc(info)
 				r, err := http.NewRequestWithContext(info.Context(), http.MethodGet, "/this-is-ldap", nil)
 				require.NoError(t, err)
-				tlsserver.AssertTLS(t, r, ptls.DefaultLDAP(nil))
+				tlsserver.AssertTLS(t, r, ptls.DefaultLDAP)
 				return nil, nil
 			}
 		})

test/integration/securetls_fips_test.go

@@ -10,7 +10,6 @@ import (
 	"bytes"
 	"context"
 	"crypto/tls"
-	_ "crypto/tls/fipsonly" // restricts all TLS configuration to FIPS-approved settings.
 	"crypto/x509"
 	"encoding/base64"
 	"fmt"
@@ -35,8 +34,6 @@ import (
 // The expected cipher suites should belong to this
 // hard-coded list, copied from here:
 // https://github.com/golang/go/blob/dev.boringcrypto/src/crypto/tls/boring.go.
-// TODO this is a private variable in the tls package... is there a better
-//  way to get access to it than just copying?
 var defaultCipherSuitesFIPS []uint16 = []uint16{
 	tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
 	tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
@@ -47,18 +44,20 @@ var defaultCipherSuitesFIPS []uint16 = []uint16{
 }
 
 // This test mirrors securetls_test.go, but adapted for fips mode.
-// e.g. checks for only TLS 1.2 ciphers
+// e.g. checks for only TLS 1.2 ciphers and checks for the
+// list of fips-approved ciphers above.
 // TLS checks safe to run in parallel with serial tests, see main_test.go.
 func TestSecureTLSPinnipedCLIToKAS_Parallel(t *testing.T) {
 	_ = testlib.IntegrationEnv(t)
 	t.Log("testing FIPs tls config")
 
 	server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		// in fips mode the ciphers are nil, so we need to replace them with what we actually expect.
+		// pinniped CLI uses ptls.Secure when talking to KAS,
+		// although the distinction doesn't matter much in FIPs mode because
+		// each of the configs is a wrapper for the same base FIPs config.
 		secure := ptls.Secure(nil)
-		// TODO this is kind of ugly... but I want different sort orders...
 		secure.CipherSuites = deepcopy.Copy(defaultCipherSuitesFIPS).([]uint16)
-		tlsserver.AssertTLS(t, r, secure) // pinniped CLI uses ptls.Secure when talking to KAS
+		tlsserver.AssertTLSConfig(t, r, secure)
 		w.Header().Set("content-type", "application/json")
 		fmt.Fprint(w, `{"kind":"TokenCredentialRequest","apiVersion":"login.concierge.pinniped.dev/v1alpha1",`+
 			`"status":{"credential":{"token":"some-fancy-token"}}}`)
@@ -89,10 +88,12 @@ func TestSecureTLSPinnipedCLIToSupervisor_Parallel(t *testing.T) {
 	_ = testlib.IntegrationEnv(t)
 
 	server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// pinniped CLI uses ptls.Default when talking to supervisor,
+		// although the distinction doesn't matter much in FIPs mode because
+		// each of the configs is a wrapper for the same base FIPs config.
 		defaultTLS := ptls.Default(nil)
-		// TODO this is kind of ugly... but I want different sort orders...
 		defaultTLS.CipherSuites = deepcopy.Copy(defaultCipherSuitesFIPS).([]uint16)
-		tlsserver.AssertTLS(t, r, defaultTLS) // pinniped CLI uses ptls.Default when talking to supervisor
+		tlsserver.AssertTLSConfig(t, r, defaultTLS)
 		w.Header().Set("content-type", "application/json")
 		fmt.Fprint(w, `{"issuer":"https://not-a-good-issuer"}`)
 	}), tlsserver.RecordTLSHello)

test/integration/securetls_test.go

@@ -34,7 +34,7 @@ func TestSecureTLSPinnipedCLIToKAS_Parallel(t *testing.T) {
 	_ = testlib.IntegrationEnv(t)
 
 	server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		tlsserver.AssertTLS(t, r, ptls.Secure(nil)) // pinniped CLI uses ptls.Secure when talking to KAS
+		tlsserver.AssertTLS(t, r, ptls.Secure) // pinniped CLI uses ptls.Secure when talking to KAS
 		w.Header().Set("content-type", "application/json")
 		fmt.Fprint(w, `{"kind":"TokenCredentialRequest","apiVersion":"login.concierge.pinniped.dev/v1alpha1",`+
 			`"status":{"credential":{"token":"some-fancy-token"}}}`)
@@ -65,7 +65,7 @@ func TestSecureTLSPinnipedCLIToSupervisor_Parallel(t *testing.T) {
 	_ = testlib.IntegrationEnv(t)
 
 	server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		tlsserver.AssertTLS(t, r, ptls.Default(nil)) // pinniped CLI uses ptls.Default when talking to supervisor
+		tlsserver.AssertTLS(t, r, ptls.Default) // pinniped CLI uses ptls.Default when talking to supervisor
 		w.Header().Set("content-type", "application/json")
 		fmt.Fprint(w, `{"issuer":"https://not-a-good-issuer"}`)
 	}), tlsserver.RecordTLSHello)

test/integration/supervisor_discovery_test.go

@@ -662,7 +662,7 @@ func newHTTPClient(t *testing.T, caBundle string, dnsOverrides map[string]string
 		caCertPool.AppendCertsFromPEM([]byte(caBundle))
 		c.Transport = &http.Transport{
 			DialContext:     overrideDialContext,
-			TLSClientConfig: &tls.Config{MinVersion: ptls.SecureTLSConfigMinTLSVersion, RootCAs: caCertPool}, //nolint: gosec // this seems to be a false flag, min tls version is 1.3 or 1.2 in fips mode
+			TLSClientConfig: &tls.Config{MinVersion: ptls.SecureTLSConfigMinTLSVersion, RootCAs: caCertPool}, //nolint: gosec // this seems to be a false flag, min tls version is 1.3 in normal mode or 1.2 in fips mode
 		}
 	} else {
 		c.Transport = &http.Transport{