diff --git a/internal/crypto/ptls/fips_strict.go b/internal/crypto/ptls/fips_strict.go index d5039854b..464a91f2b 100644 --- a/internal/crypto/ptls/fips_strict.go +++ b/internal/crypto/ptls/fips_strict.go @@ -1,9 +1,8 @@ -// Copyright 2022-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2022-2024 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 +// The configurations here override the usual 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 package ptls @@ -15,16 +14,16 @@ import ( "path/filepath" "runtime" - "k8s.io/apiserver/pkg/server/options" - // Cause fipsonly tls mode with this side effect import. _ "go.pinniped.dev/internal/crypto/fips" "go.pinniped.dev/internal/plog" ) -// Always use TLS 1.2 for FIPs -const secureServingOptionsMinTLSVersion = "VersionTLS12" -const SecureTLSConfigMinTLSVersion = tls.VersionTLS12 +// goboring now also supports TLS 1.3 starting in Golang 1.21.6 +// (see https://github.com/golang/go/issues/64717), +// so we can use TLS 1.3 as the minimum TLS version for our "secure" configuration +// profile in both FIPS and non-FIPS compiled binaries. +// Hence, we no longer redefine the Secure() function in this file. func init() { switch filepath.Base(os.Args[0]) { @@ -40,40 +39,34 @@ func init() { func Default(rootCAs *x509.CertPool) *tls.Config { return &tls.Config{ - // goboring requires TLS 1.2 and only TLS 1.2 - MinVersion: SecureTLSConfigMinTLSVersion, + MinVersion: tls.VersionTLS12, + // goboring now also supports TLS 1.3 (see https://github.com/golang/go/issues/64717) + // so this default configuration can allow either 1.2 or 1.3 MaxVersion: SecureTLSConfigMinTLSVersion, - // enable HTTP2 for go's 1.7 HTTP Server - // setting this explicitly is only required in very specific circumstances - // it is simpler to just set it here than to try and determine if we need to - NextProtos: []string{"h2", "http/1.1"}, - - // optional root CAs, nil means use the host's root CA set - RootCAs: rootCAs, - - // This is all of the fips-approved ciphers. + // This is all the fips-approved TLS 1.2 ciphers. // The list is hard-coded for convenience of testing. - // This is kept in sync with the boring crypto compiler via TestFIPSCipherSuites. + // If this list does not match the boring crypto compiler's list then the TestFIPSCipherSuites integration + // test should fail, which indicates that this list needs to be updated. CipherSuites: []uint16{ tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, - tls.TLS_RSA_WITH_AES_128_GCM_SHA256, - tls.TLS_RSA_WITH_AES_256_GCM_SHA384, + tls.TLS_AES_128_GCM_SHA256, + tls.TLS_AES_256_GCM_SHA384, }, - } -} -func Secure(rootCAs *x509.CertPool) *tls.Config { - return Default(rootCAs) + // enable HTTP2 for go's 1.7 HTTP Server + // setting this explicitly is only required in very specific circumstances + // it is simpler to just set it here than to try and determine if we need to + NextProtos: []string{"h2", "http/1.1"}, + + // optional root CAs, nil means use the host's root CA set + RootCAs: rootCAs, + } } func DefaultLDAP(rootCAs *x509.CertPool) *tls.Config { return Default(rootCAs) } - -func secureServing(opts *options.SecureServingOptionsWithLoopback) { - defaultServing(opts) -} diff --git a/internal/crypto/ptls/ptls_test.go b/internal/crypto/ptls/ptls_test.go index e8475b958..27aa13231 100644 --- a/internal/crypto/ptls/ptls_test.go +++ b/internal/crypto/ptls/ptls_test.go @@ -1,4 +1,4 @@ -// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package ptls @@ -53,7 +53,7 @@ func TestMerge(t *testing.T) { want *tls.Config }{ { - name: "default no protos", + name: "default without NextProtos", tlsConfigFunc: Default, tlsConfig: &tls.Config{ //nolint:gosec // not concerned with TLS MinVersion here ServerName: "something-to-check-passthrough", @@ -73,7 +73,7 @@ func TestMerge(t *testing.T) { }, }, { - name: "default with protos", + name: "default with NextProtos", tlsConfigFunc: Default, tlsConfig: &tls.Config{ //nolint:gosec // not concerned with TLS MinVersion here ServerName: "a different thing for passthrough", @@ -94,42 +94,34 @@ func TestMerge(t *testing.T) { }, }, { - name: "secure no protos", + name: "secure without NextProtos", tlsConfigFunc: Secure, tlsConfig: &tls.Config{ //nolint:gosec // not concerned with TLS MinVersion here ServerName: "something-to-check-passthrough", }, want: &tls.Config{ - ServerName: "something-to-check-passthrough", - MinVersion: tls.VersionTLS13, - CipherSuites: []uint16{ - tls.TLS_AES_128_GCM_SHA256, - tls.TLS_AES_256_GCM_SHA384, - tls.TLS_CHACHA20_POLY1305_SHA256, - }, - NextProtos: []string{"h2", "http/1.1"}, + ServerName: "something-to-check-passthrough", + MinVersion: tls.VersionTLS13, + CipherSuites: nil, + NextProtos: []string{"h2", "http/1.1"}, }, }, { - name: "secure with protos", + name: "secure with NextProtos", tlsConfigFunc: Secure, tlsConfig: &tls.Config{ //nolint:gosec // not concerned with TLS MinVersion here ServerName: "a different thing for passthrough", NextProtos: []string{"panda"}, }, want: &tls.Config{ - ServerName: "a different thing for passthrough", - MinVersion: tls.VersionTLS13, - CipherSuites: []uint16{ - tls.TLS_AES_128_GCM_SHA256, - tls.TLS_AES_256_GCM_SHA384, - tls.TLS_CHACHA20_POLY1305_SHA256, - }, - NextProtos: []string{"panda"}, + ServerName: "a different thing for passthrough", + MinVersion: tls.VersionTLS13, + CipherSuites: nil, + NextProtos: []string{"panda"}, }, }, { - name: "default ldap no protos", + name: "default ldap without NextProtos", tlsConfigFunc: DefaultLDAP, tlsConfig: &tls.Config{ //nolint:gosec // not concerned with TLS MinVersion here ServerName: "something-to-check-passthrough", @@ -153,7 +145,7 @@ func TestMerge(t *testing.T) { }, }, { - name: "default ldap with protos", + name: "default ldap with NextProtos", tlsConfigFunc: DefaultLDAP, tlsConfig: &tls.Config{ ServerName: "a different thing for passthrough", @@ -178,7 +170,7 @@ func TestMerge(t *testing.T) { }, }, { - name: "legacy no protos", + name: "legacy without NextProtos", tlsConfigFunc: Legacy, tlsConfig: &tls.Config{ ServerName: "something-to-check-passthrough", @@ -209,7 +201,7 @@ func TestMerge(t *testing.T) { }, }, { - name: "legacy with protos", + name: "legacy with NextProtos", tlsConfigFunc: Legacy, tlsConfig: &tls.Config{ ServerName: "a different thing for passthrough", diff --git a/internal/crypto/ptls/secure.go b/internal/crypto/ptls/secure.go index 68e65b0ec..7f628f18f 100644 --- a/internal/crypto/ptls/secure.go +++ b/internal/crypto/ptls/secure.go @@ -1,8 +1,6 @@ -// Copyright 2021-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -//go:build !fips_strict - package ptls import ( @@ -34,13 +32,7 @@ func Secure(rootCAs *x509.CertPool) *tls.Config { // https://ssl-config.mozilla.org/#server=go&version=1.17.2&config=modern&guideline=5.6 c := Default(rootCAs) c.MinVersion = SecureTLSConfigMinTLSVersion // max out the security - c.CipherSuites = []uint16{ - // TLS 1.3 ciphers are not configurable, but we need to explicitly set them here to make our client hello behave correctly - // See https://github.com/golang/go/pull/49293 - tls.TLS_AES_128_GCM_SHA256, - tls.TLS_AES_256_GCM_SHA384, - tls.TLS_CHACHA20_POLY1305_SHA256, - } + c.CipherSuites = nil // TLS 1.3 ciphers are not configurable return c } diff --git a/internal/testutil/tlsserver/tls13_ciphers.go b/internal/testutil/tlsserver/tls13_ciphers.go new file mode 100644 index 000000000..8f0b0647e --- /dev/null +++ b/internal/testutil/tlsserver/tls13_ciphers.go @@ -0,0 +1,31 @@ +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build !fips_strict + +package tlsserver + +import "crypto/tls" + +// GetExpectedTLS13Ciphers returns the expected TLS 1.3 cipher for a non-FIPS build. +func GetExpectedTLS13Ciphers() []uint16 { + // TLS 1.3 ciphers are not configurable, so we can hard-code them here. + return []uint16{ + tls.TLS_AES_128_GCM_SHA256, + tls.TLS_AES_256_GCM_SHA384, + tls.TLS_CHACHA20_POLY1305_SHA256, + } +} + +// GetExpectedTLS13CipherNMapKeyExchangeInfoValue returns the expected key exchange info value +// which is shown by nmap in parenthesis next to the cipher name for a non-FIPS build. +func GetExpectedTLS13CipherNMapKeyExchangeInfoValue(cipher uint16) string { + switch cipher { + case tls.TLS_AES_128_GCM_SHA256, + tls.TLS_AES_256_GCM_SHA384, + tls.TLS_CHACHA20_POLY1305_SHA256: + return "ecdh_x25519" + default: + return "unknown key exchange value" + } +} diff --git a/internal/testutil/tlsserver/tls13_ciphers_fips.go b/internal/testutil/tlsserver/tls13_ciphers_fips.go new file mode 100644 index 000000000..ead0e3ffd --- /dev/null +++ b/internal/testutil/tlsserver/tls13_ciphers_fips.go @@ -0,0 +1,30 @@ +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +//go:build fips_strict + +package tlsserver + +import "crypto/tls" + +// GetExpectedTLS13Ciphers returns the expected TLS 1.3 cipher for a FIPS build. +func GetExpectedTLS13Ciphers() []uint16 { + // TLS 1.3 ciphers are not configurable, so we can hard-code them here. + return []uint16{ + tls.TLS_AES_128_GCM_SHA256, + tls.TLS_AES_256_GCM_SHA384, + // tls.TLS_CHACHA20_POLY1305_SHA256 is not supported by boring crypto + } +} + +// GetExpectedTLS13CipherNMapKeyExchangeInfoValue returns the expected key exchange info value +// which is shown by nmap in parenthesis next to the cipher name for a FIPS build. +func GetExpectedTLS13CipherNMapKeyExchangeInfoValue(cipher uint16) string { + switch cipher { + case tls.TLS_AES_128_GCM_SHA256, + tls.TLS_AES_256_GCM_SHA384: + return "secp256r1" + default: + return "unknown key exchange value" + } +} diff --git a/internal/testutil/tlsserver/tlsserver.go b/internal/testutil/tlsserver/tlsserver.go index 1585284da..56eddba39 100644 --- a/internal/testutil/tlsserver/tlsserver.go +++ b/internal/testutil/tlsserver/tlsserver.go @@ -1,4 +1,4 @@ -// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package tlsserver @@ -18,6 +18,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/util/httpstream" + "k8s.io/apimachinery/pkg/util/sets" "go.pinniped.dev/internal/crypto/ptls" ) @@ -66,7 +67,7 @@ func RecordTLSHello(server *httptest.Server) { } } -func AssertTLS(t *testing.T, r *http.Request, tlsConfigFunc ptls.ConfigFunc) { +func AssertTLS(t *testing.T, r *http.Request, clientTLSConfigFunc ptls.ConfigFunc) { t.Helper() m, ok := getCtxMap(r.Context()) @@ -75,35 +76,55 @@ func AssertTLS(t *testing.T, r *http.Request, tlsConfigFunc ptls.ConfigFunc) { h, ok := m.Load(helloKey) require.True(t, ok) - info, ok := h.(*tls.ClientHelloInfo) + actualClientHello, ok := h.(*tls.ClientHelloInfo) require.True(t, ok) - tlsConfig := tlsConfigFunc(nil) - - supportedVersions := []uint16{tlsConfig.MinVersion} - ciphers := tlsConfig.CipherSuites - - if secureTLSConfig := ptls.Secure(nil); tlsConfig.MinVersion != secureTLSConfig.MinVersion { - supportedVersions = append([]uint16{secureTLSConfig.MinVersion}, supportedVersions...) - ciphers = append(ciphers, secureTLSConfig.CipherSuites...) + clientTLSConfig := clientTLSConfigFunc(nil) + + var wantClientSupportedVersions []uint16 + var wantClientSupportedCiphers []uint16 + + switch { + // When the provided config only supports TLS 1.3, then set up the expected values for TLS 1.3. + case clientTLSConfig.MinVersion == tls.VersionTLS13: + wantClientSupportedVersions = []uint16{tls.VersionTLS13} + wantClientSupportedCiphers = GetExpectedTLS13Ciphers() + // When the provided config supports both TLS 1.2 and 1.3, then set up the expected values for both. + case clientTLSConfig.MinVersion == tls.VersionTLS12 && (clientTLSConfig.MaxVersion == 0 || clientTLSConfig.MaxVersion == tls.VersionTLS13): + wantClientSupportedVersions = []uint16{tls.VersionTLS13, tls.VersionTLS12} + wantClientSupportedCiphers = appendIfNotAlreadyIncluded(clientTLSConfig.CipherSuites, GetExpectedTLS13Ciphers()) + default: + require.Fail(t, "incorrect test setup: clientTLSConfig supports an unexpected combination of TLS versions") } - protos := tlsConfig.NextProtos + wantClientProtos := clientTLSConfig.NextProtos if httpstream.IsUpgradeRequest(r) { - protos = tlsConfig.NextProtos[1:] + wantClientProtos = clientTLSConfig.NextProtos[1:] } // use assert instead of require to not break the http.Handler with a panic - ok1 := assert.Equal(t, supportedVersions, info.SupportedVersions) - ok2 := assert.Equal(t, cipherSuiteIDsToStrings(ciphers), cipherSuiteIDsToStrings(info.CipherSuites)) - ok3 := assert.Equal(t, protos, info.SupportedProtos) + ok1 := assert.Equal(t, wantClientSupportedVersions, actualClientHello.SupportedVersions) + ok2 := assert.Equal(t, cipherSuiteIDsToStrings(wantClientSupportedCiphers), cipherSuiteIDsToStrings(actualClientHello.CipherSuites)) + ok3 := assert.Equal(t, wantClientProtos, actualClientHello.SupportedProtos) if all := ok1 && ok2 && ok3; !all { - t.Errorf("insecure TLS detected for %q %q %q upgrade=%v supportedVersions=%v ciphers=%v protos=%v", + t.Errorf("insecure TLS detected for %q %q %q upgrade=%v wantClientSupportedVersions=%v wantClientSupportedCiphers=%v wantClientProtos=%v", r.Proto, r.Method, r.URL.String(), httpstream.IsUpgradeRequest(r), ok1, ok2, ok3) } } +// appendIfNotAlreadyIncluded only adds the newItems to the list if they are not already included +// in this list. It returns the potentially updated list. +func appendIfNotAlreadyIncluded(list []uint16, newItems []uint16) []uint16 { + originals := sets.New(list...) + for _, newItem := range newItems { + if !originals.Has(newItem) { + list = append(list, newItem) + } + } + return list +} + func cipherSuiteIDsToStrings(ids []uint16) []string { cipherSuites := make([]string, 0, len(ids)) for _, id := range ids { diff --git a/test/integration/securetls_test.go b/test/integration/securetls_test.go index e4324692b..1cc9997b3 100644 --- a/test/integration/securetls_test.go +++ b/test/integration/securetls_test.go @@ -1,4 +1,4 @@ -// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package integration @@ -25,8 +25,8 @@ func TestSecureTLSPinnipedCLIToKAS_Parallel(t *testing.T) { server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // pinniped CLI uses ptls.Secure when talking to KAS - // in FIPs mode the distinction doesn't matter much because - // each of the configs is a wrapper for the same base FIPs config + // in FIPS mode the distinction doesn't matter much because + // each of the configs is a wrapper for the same base FIPS config tlsserver.AssertTLS(t, r, ptls.Secure) w.Header().Set("content-type", "application/json") fmt.Fprint(w, `{"kind":"TokenCredentialRequest","apiVersion":"login.concierge.pinniped.dev/v1alpha1",`+ @@ -59,8 +59,8 @@ func TestSecureTLSPinnipedCLIToSupervisor_Parallel(t *testing.T) { server := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // pinniped CLI uses ptls.Default when talking to supervisor - // in FIPs mode the distinction doesn't matter much because - // each of the configs is a wrapper for the same base FIPs config + // in FIPS mode the distinction doesn't matter much because + // each of the configs is a wrapper for the same base FIPS config tlsserver.AssertTLS(t, r, ptls.Default) w.Header().Set("content-type", "application/json") fmt.Fprint(w, `{"issuer":"https://not-a-good-issuer"}`) diff --git a/test/testlib/securetls.go b/test/testlib/securetls.go index b7f45b924..3111c39fc 100644 --- a/test/testlib/securetls.go +++ b/test/testlib/securetls.go @@ -1,4 +1,4 @@ -// Copyright 2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2022-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package testlib @@ -18,7 +18,7 @@ import ( "github.com/stretchr/testify/require" - "go.pinniped.dev/internal/crypto/ptls" + "go.pinniped.dev/internal/testutil/tlsserver" ) func RunNmapSSLEnum(t *testing.T, host string, port uint16) (string, string) { @@ -51,9 +51,8 @@ func RunNmapSSLEnum(t *testing.T, host string, port uint16) (string, string) { } func GetExpectedCiphers(config *tls.Config) string { - secureConfig := ptls.Secure(nil) - skip12 := config.MinVersion == tls.VersionTLS13 + skip13 := config.MaxVersion == tls.VersionTLS12 var tls12Bit, tls13Bit string @@ -90,12 +89,15 @@ func GetExpectedCiphers(config *tls.Config) string { tls12Bit = fmt.Sprintf(tls12Base, s.String(), cipherSuitePreference) } - skip13 := config.MaxVersion == tls.VersionTLS12 if !skip13 { var s strings.Builder - for i, id := range secureConfig.CipherSuites { - s.WriteString(fmt.Sprintf(tls13Item, strings.Replace(tls.CipherSuiteName(id), "TLS_", "TLS_AKE_WITH_", 1))) - if i == len(secureConfig.CipherSuites)-1 { + tls13CipherSuites := tlsserver.GetExpectedTLS13Ciphers() + for i, id := range tls13CipherSuites { + s.WriteString(fmt.Sprintf(tls13Item, + strings.Replace(tls.CipherSuiteName(id), "TLS_", "TLS_AKE_WITH_", 1), + tlsserver.GetExpectedTLS13CipherNMapKeyExchangeInfoValue(id)), + ) + if i == len(tls13CipherSuites)-1 { break } s.WriteString("\n") @@ -114,7 +116,7 @@ const ( Nmap done: 1 IP address (1 host up) scanned in` - // cipher preference is a variable because in FIPs mode it is server + // cipher preference is a variable because in FIPS mode it is server // but in normal mode it is client. tls12Base = ` | TLSv1.2: @@ -138,5 +140,5 @@ Nmap done: 1 IP address (1 host up) scanned in` // For the RSA ciphers, we expect this output to be RSA 2048. rsa2048 = "rsa 2048" - tls13Item = `| %s (ecdh_x25519) - A` + tls13Item = `| %s (%s) - A` )