[fix] Fallback to system CA certs pool (jaegertracing#6335)

## Which problem is this PR solving? - Resolves jaegertracing#6334 ## Description of the changes - Restore pre-OTEL behavior of using system cert pool if no CAFile is specified. ## How was this change tested? - unit tests --------- Signed-off-by: Yuri Shkuro <[email protected]>
Manik2708 · Dec 10, 2024 · 8f612a8 · 8f612a8
1 parent 78f5a41
commit 8f612a8
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 17 deletions.
diff --git a/cmd/ingester/app/flags_test.go b/cmd/ingester/app/flags_test.go
@@ -10,6 +10,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.opentelemetry.io/collector/config/configtls"
 
 	"github.com/jaegertracing/jaeger/pkg/config"
 	"github.com/jaegertracing/jaeger/pkg/kafka/auth"
@@ -66,12 +67,18 @@ func TestTLSFlags(t *testing.T) {
 			expected: auth.AuthenticationConfig{Authentication: "kerberos", Kerberos: kerb, PlainText: plain},
 		},
 		{
-			flags:    []string{"--kafka.consumer.authentication=tls"},
-			expected: auth.AuthenticationConfig{Authentication: "tls", Kerberos: kerb, PlainText: plain},
-		},
-		{
-			flags:    []string{"--kafka.consumer.authentication=tls", "--kafka.consumer.tls.enabled=false"},
-			expected: auth.AuthenticationConfig{Authentication: "tls", Kerberos: kerb, PlainText: plain},
+			flags: []string{"--kafka.consumer.authentication=tls"},
+			expected: auth.AuthenticationConfig{
+				Authentication: "tls",
+				Kerberos:       kerb,
+				// TODO this test is unclear - if tls.enabled != true, why is it not tls.Insecure=true?
+				TLS: configtls.ClientConfig{
+					Config: configtls.Config{
+						IncludeSystemCACertsPool: true,
+					},
+				},
+				PlainText: plain,
+			},
 		},
 	}
 

diff --git a/pkg/config/tlscfg/options.go b/pkg/config/tlscfg/options.go
@@ -139,6 +139,10 @@ func (o *Options) ToOtelClientConfig() configtls.ClientConfig {
 			MinVersion:     o.MinVersion,
 			MaxVersion:     o.MaxVersion,
 			ReloadInterval: o.ReloadInterval,
+
+			// when no truststore given, use SystemCertPool
+			// https://github.com/jaegertracing/jaeger/issues/6334
+			IncludeSystemCACertsPool: o.Enabled && (len(o.CAPath) == 0),
 		},
 	}
 }

diff --git a/pkg/kafka/auth/config_test.go b/pkg/kafka/auth/config_test.go
@@ -37,20 +37,18 @@ func Test_InitFromViper(t *testing.T) {
 		"--kafka.auth.kerberos.keytab-file=/path/to/keytab",
 		"--kafka.auth.kerberos.disable-fast-negotiation=true",
 		"--kafka.auth.tls.enabled=false",
+		"--kafka.auth.tls.ca=/not/allowed/if/tls/is/disabled",
 		"--kafka.auth.plaintext.username=user",
 		"--kafka.auth.plaintext.password=password",
 		"--kafka.auth.plaintext.mechanism=SCRAM-SHA-256",
-		"--kafka.auth.tls.ca=failing",
 	})
 
 	authConfig := &AuthenticationConfig{}
 	err := authConfig.InitFromViper(configPrefix, v)
-	require.EqualError(t, err, "failed to process Kafka TLS options: kafka.auth.tls.* options cannot be used when kafka.auth.tls.enabled is false")
+	require.ErrorContains(t, err, "kafka.auth.tls.* options cannot be used when kafka.auth.tls.enabled is false")
 
-	command.ParseFlags([]string{"--kafka.auth.tls.ca="})
-	v.BindPFlags(command.Flags())
-	err = authConfig.InitFromViper(configPrefix, v)
-	require.NoError(t, err)
+	command.ParseFlags([]string{"--kafka.auth.tls.ca="}) // incrementally update authConfig
+	require.NoError(t, authConfig.InitFromViper(configPrefix, v))
 
 	expectedConfig := &AuthenticationConfig{
 		Authentication: "tls",
@@ -64,7 +62,11 @@ func Test_InitFromViper(t *testing.T) {
 			KeyTabPath:      "/path/to/keytab",
 			DisablePAFXFast: true,
 		},
-		TLS: configtls.ClientConfig{},
+		TLS: configtls.ClientConfig{
+			Config: configtls.Config{
+				IncludeSystemCACertsPool: true,
+			},
+		},
 		PlainText: PlainTextConfig{
 			Username:  "user",
 			Password:  "password",

diff --git a/plugin/storage/kafka/options_test.go b/plugin/storage/kafka/options_test.go
@@ -184,12 +184,31 @@ func TestTLSFlags(t *testing.T) {
 			expected: auth.AuthenticationConfig{Authentication: "kerberos", Kerberos: kerb, TLS: configtls.ClientConfig{}, PlainText: plain},
 		},
 		{
-			flags:    []string{"--kafka.producer.authentication=tls"},
-			expected: auth.AuthenticationConfig{Authentication: "tls", Kerberos: kerb, TLS: configtls.ClientConfig{}, PlainText: plain},
+			flags: []string{"--kafka.producer.authentication=tls"},
+			expected: auth.AuthenticationConfig{
+				Authentication: "tls",
+				Kerberos:       kerb,
+				TLS: configtls.ClientConfig{
+					Config: configtls.Config{
+						IncludeSystemCACertsPool: true,
+					},
+				},
+				PlainText: plain,
+			},
 		},
 		{
-			flags:    []string{"--kafka.producer.authentication=tls", "--kafka.producer.tls.enabled=false"},
-			expected: auth.AuthenticationConfig{Authentication: "tls", Kerberos: kerb, TLS: configtls.ClientConfig{}, PlainText: plain},
+			flags: []string{"--kafka.producer.authentication=tls", "--kafka.producer.tls.enabled=false"},
+			expected: auth.AuthenticationConfig{
+				Authentication: "tls",
+				Kerberos:       kerb,
+				// TODO this test is unclear - if tls.enabled=false, why is it not tls.Insecure=true?
+				TLS: configtls.ClientConfig{
+					Config: configtls.Config{
+						IncludeSystemCACertsPool: true,
+					},
+				},
+				PlainText: plain,
+			},
 		},
 	}