Resolve cipher suites directly from tlsProfile (#1989)

Cipher suites were being filtered out of the tls configuration when tls min version is 1.3 by the controller-runtime-commmon lib. We need the ciphers to be present because we need to set them on the mesh config for other components to use. This commit copies the cipher conversion directly from that lib.

Signed-off-by: Nick Fox <nfox@redhat.com>

Author: nrfox

pkg/config/tls.go

@@ -51,6 +51,46 @@ type OpenShiftTLS struct {
 	TLSConfigFunc func(*tls.Config)
 }
 
+// These functions are ripped directly from:
+// https://github.com/openshift/controller-runtime-common/blob/64ee174f5e2ebc630fbb554dd114d7a7a878693f/pkg/tls/tls.go#L134-L168
+// until https://github.com/openshift/controller-runtime-common/issues/19 is resolved.
+
+// cipherCode returns the TLS cipher code for an OpenSSL or IANA cipher name.
+// Returns 0 if the cipher is not supported.
+func cipherCode(cipher string) uint16 {
+	// First try as IANA name directly.
+	if code, err := openshiftcrypto.CipherSuite(cipher); err == nil {
+		return code
+	}
+
+	// Try converting from OpenSSL name to IANA name.
+	ianaCiphers := openshiftcrypto.OpenSSLToIANACipherSuites([]string{cipher})
+	if len(ianaCiphers) == 1 {
+		if code, err := openshiftcrypto.CipherSuite(ianaCiphers[0]); err == nil {
+			return code
+		}
+	}
+
+	// Return 0 if the cipher is not supported.
+	return 0
+}
+
+// cipherCodes converts a list of cipher names (OpenSSL or IANA format) to their uint16 codes.
+// Returns the converted codes and a list of any unsupported cipher names.
+func cipherCodes(ciphers []string) (codes []uint16, unsupportedCiphers []string) {
+	for _, cipher := range ciphers {
+		code := cipherCode(cipher)
+		if code == 0 {
+			unsupportedCiphers = append(unsupportedCiphers, cipher)
+			continue
+		}
+
+		codes = append(codes, code)
+	}
+
+	return codes, unsupportedCiphers
+}
+
 // NewTLSConfigForOpenShift fetches TLS configuration from the OpenShift
 // APIServer and returns a TLSConfig with the OpenShift field populated.
 func NewTLSConfigForOpenShift(ctx context.Context, log logr.Logger, cl client.Client) (*TLSConfig, error) {
@@ -72,17 +112,22 @@ func NewTLSConfigForOpenShift(ctx context.Context, log logr.Logger, cl client.Cl
 	}
 
 	if openshiftcrypto.ShouldHonorClusterTLSProfile(adherencePolicy) {
-		tlsConfigFunc, unsupportedCiphers := openshifttls.NewTLSConfigFromProfile(profileSpec)
+		tlsConfigFunc, _ := openshifttls.NewTLSConfigFromProfile(profileSpec)
 		tlsConfig.OpenShift.TLSConfigFunc = tlsConfigFunc
 
-		if len(unsupportedCiphers) > 0 {
-			log.Info("Some ciphers from TLS profile are unsupported and will be ignored", "unsupportedCiphers", unsupportedCiphers)
-		}
-
 		goTLSConfig := &tls.Config{MinVersion: tls.VersionTLS12}
 		tlsConfigFunc(goTLSConfig)
-		tlsConfig.CipherSuites = goTLSConfig.CipherSuites
 		tlsConfig.MinVersion = goTLSConfig.MinVersion
+		// Resolve cipher suites directly from the profile spec
+		// instead of reading them back from the tls.Config set by tlsConfigFunc,
+		// because the controller-runtime-common lib filters out the CipherSuites when
+		// MinTLSVersion is 1.3. We still need the cipher IDs to configure non-Go components like Envoy.
+		cipherSuites, unsupportedCiphers := cipherCodes(profileSpec.Ciphers)
+		tlsConfig.CipherSuites = cipherSuites
+
+		if len(unsupportedCiphers) > 0 {
+			log.Info("Some ciphers from TLS profile are unsupported and will be ignored", "unsupportedCiphers", unsupportedCiphers)
+		}
 	}
 
 	return tlsConfig, nil

pkg/config/tls_test.go

@@ -16,9 +16,11 @@ package config
 
 import (
 	"crypto/tls"
+	"fmt"
 	"testing"
 
 	configv1 "github.com/openshift/api/config/v1"
+	openshifttls "github.com/openshift/controller-runtime-common/pkg/tls"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -27,6 +29,11 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 )
 
+var (
+	defaultTLSCiphers, _ = cipherCodes(openshifttls.DefaultTLSCiphers)
+	modernTLSCiphers, _  = cipherCodes(configv1.TLSProfiles[configv1.TLSProfileModernType].Ciphers)
+)
+
 func TestNewTLSConfigForOpenShift(t *testing.T) {
 	log := zap.New(zap.UseDevMode(true))
 	scheme := runtime.NewScheme()
@@ -44,6 +51,7 @@ func TestNewTLSConfigForOpenShift(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
 			},
 			expected: TLSConfig{
+				MinVersion: tls.VersionTLS12,
 				OpenShift: &OpenShiftTLS{
 					TLSAdherencePolicy: configv1.TLSAdherencePolicyNoOpinion,
 				},
@@ -61,6 +69,7 @@ func TestNewTLSConfigForOpenShift(t *testing.T) {
 				},
 			},
 			expected: TLSConfig{
+				MinVersion: tls.VersionTLS12,
 				OpenShift: &OpenShiftTLS{
 					TLSAdherencePolicy: configv1.TLSAdherencePolicyLegacyAdheringComponentsOnly,
 				},
@@ -73,11 +82,13 @@ func TestNewTLSConfigForOpenShift(t *testing.T) {
 				Spec: configv1.APIServerSpec{
 					TLSAdherence: configv1.TLSAdherencePolicyStrictAllComponents,
 					TLSSecurityProfile: &configv1.TLSSecurityProfile{
-						Type: configv1.TLSProfileOldType,
+						Type: configv1.TLSProfileModernType,
 					},
 				},
 			},
 			expected: TLSConfig{
+				MinVersion:   tls.VersionTLS13,
+				CipherSuites: modernTLSCiphers,
 				OpenShift: &OpenShiftTLS{
 					TLSAdherencePolicy: configv1.TLSAdherencePolicyStrictAllComponents,
 				},
@@ -92,6 +103,8 @@ func TestNewTLSConfigForOpenShift(t *testing.T) {
 				},
 			},
 			expected: TLSConfig{
+				MinVersion:   tls.VersionTLS12,
+				CipherSuites: defaultTLSCiphers,
 				OpenShift: &OpenShiftTLS{
 					TLSAdherencePolicy: configv1.TLSAdherencePolicyStrictAllComponents,
 				},
@@ -105,6 +118,8 @@ func TestNewTLSConfigForOpenShift(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			require := require.New(t)
+			assert := assert.New(t)
 			builder := fake.NewClientBuilder().WithScheme(scheme)
 			if tt.apiServer != nil {
 				builder = builder.WithObjects(tt.apiServer)
@@ -113,24 +128,23 @@ func TestNewTLSConfigForOpenShift(t *testing.T) {
 
 			tlsConfig, err := NewTLSConfigForOpenShift(t.Context(), log, cl)
 			if tt.wantErr {
-				require.Error(t, err)
+				require.Error(err)
 				return
 			}
-			require.NoError(t, err)
-			require.NotNil(t, tlsConfig)
-			require.NotNil(t, tlsConfig.OpenShift)
+			require.NoError(err)
+			require.NotNil(tlsConfig)
+			require.NotNil(tlsConfig.OpenShift)
 
-			assert.Equal(t, tt.expected.OpenShift.TLSAdherencePolicy, tlsConfig.OpenShift.TLSAdherencePolicy)
+			assert.Equal(tt.expected.OpenShift.TLSAdherencePolicy, tlsConfig.OpenShift.TLSAdherencePolicy)
 
 			if tt.expected.OpenShift.TLSAdherencePolicy == configv1.TLSAdherencePolicyStrictAllComponents {
-				assert.NotEmpty(t, tlsConfig.CipherSuites)
-				require.NotNil(t, tlsConfig.OpenShift.TLSConfigFunc)
-				goTLS := &tls.Config{MinVersion: tls.VersionTLS12}
-				tlsConfig.OpenShift.TLSConfigFunc(goTLS)
-				assert.NotEmpty(t, goTLS.CipherSuites)
+				assert.Equal(tt.expected.CipherSuites, tlsConfig.CipherSuites)
+				assert.Equal(tt.expected.MinVersion, tlsConfig.MinVersion,
+					fmt.Sprintf("TLS MinVersion mismatch: expected %s, got %s", tls.VersionName(tt.expected.MinVersion), tls.VersionName(tlsConfig.MinVersion)))
+				require.NotNil(tlsConfig.OpenShift.TLSConfigFunc)
 			} else {
-				assert.Empty(t, tlsConfig.CipherSuites)
-				assert.Nil(t, tlsConfig.OpenShift.TLSConfigFunc)
+				assert.Empty(tlsConfig.CipherSuites)
+				assert.Nil(tlsConfig.OpenShift.TLSConfigFunc)
 			}
 		})
 	}