fix: Clone *tls.Config returned by TLSConfigManager.TLSConfig

pkg/tlsconfig/configmanager.go

@@ -86,8 +86,10 @@ func NewDisabledTLSConfigManager() *TLSConfigManager {
 }
 
 // TLSConfig returns a tls.Config for use with dial and listen functions. When TLS is disabled the return is nil.
+// The returned tls.Config is a clone and does not need to be cloned again.
 func (cm *TLSConfigManager) TLSConfig() *tls.Config {
-	return cm.tlsConfig
+	// Clone returns nil for a nil tlsConfig
+	return cm.tlsConfig.Clone()
 }
 
 // TLSCertLoader returns the certificate loader for this TLSConfigManager. When no certificate is provided
@@ -99,7 +101,8 @@ func (cm *TLSConfigManager) TLSCertLoader() *TLSCertLoader {
 // UseTLS returns true if this TLSConfigManager is configured to use TLS. It is a convenience wrapper
 // around TLSConfig.
 func (cm *TLSConfigManager) UseTLS() bool {
-	return cm.TLSConfig() != nil
+	// Don't use TLSConfig() to avoid cloning a tlsConfig only to throw it away.
+	return cm.tlsConfig != nil
 }
 
 // Return a net.Listener for network and address based on current configuration.

pkg/tlsconfig/configmanager_test.go

@@ -10,10 +10,14 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestTLSConfigManager_ConsistentInstances(t *testing.T) {
+func TestTLSConfigManager_ConsistentClonedConfigs(t *testing.T) {
 	ss := selfsigned.NewSelfSignedCert(t)
 
-	manager, err := NewTLSConfigManager(true, nil, ss.CertPath, ss.KeyPath, false)
+	// Create an initial baseConfig for manager.
+	baseTLSConfig := ss.ClientTLSConfig(t, false, false)
+	require.NotNil(t, baseTLSConfig.RootCAs, "ClientTLSConfig should have set RootCAs")
+
+	manager, err := NewTLSConfigManager(true, baseTLSConfig, ss.CertPath, ss.KeyPath, false)
 	require.NoError(t, err)
 	require.NotNil(t, manager)
 	defer func() {
@@ -24,9 +28,27 @@ func TestTLSConfigManager_ConsistentInstances(t *testing.T) {
 	tlsConfig := manager.TLSConfig()
 	require.NotNil(t, tlsConfig)
 
-	// Subsequent calls should return the same instance
+	// The manager's TLS config and baseTLSConfig should not be the same and and should not be shared,
+	// but we should be able to set that the config was cloned from baseTLSConfig by looking at RootCAs.
+	require.NotSame(t, tlsConfig, baseTLSConfig)
+	require.NotEqual(t, tlsConfig, baseTLSConfig)
+	require.NotNil(t, tlsConfig.RootCAs)
+	require.Equal(t, baseTLSConfig.RootCAs, tlsConfig.RootCAs)
+
+	// Subsequent calls should return different instances that are equal.
 	tlsConfig2 := manager.TLSConfig()
-	require.Same(t, tlsConfig, tlsConfig2)
+	require.NotSame(t, tlsConfig, tlsConfig2)
+	// We can't compare the function pointers directly, just that they are non-nil.
+	// Clear out the function pointers before calling require.Equal.
+	require.NotNil(t, tlsConfig.GetCertificate)
+	require.NotNil(t, tlsConfig2.GetCertificate)
+	require.NotNil(t, tlsConfig.GetClientCertificate)
+	require.NotNil(t, tlsConfig2.GetClientCertificate)
+	tlsConfig.GetCertificate = nil
+	tlsConfig.GetClientCertificate = nil
+	tlsConfig2.GetCertificate = nil
+	tlsConfig2.GetClientCertificate = nil
+	require.Equal(t, tlsConfig, tlsConfig2)
 
 	// TLSCertLoader should also be available
 	certLoader := manager.TLSCertLoader()