6744 - Fix Kafka TLS configuration with plaintext authentication #6764

amilbcahat · 2025-02-21T00:15:45Z

Which problem is this PR solving?

Resolves [Bug]: kafka: cannot connect to TLS kafka with TLS + plaintext #6744

Description of the changes

Fixed TLS configuration to work with plaintext authentication when TLS is enabled
Modified TLS configuration logic in Kafka authentication to support SASL-SSL with PLAIN
Restored behavior to enable TLS regardless of authentication method when TLS is configured
Fixed regression introduced in PR [kafka] OTEL helper instead of tlscfg package #6270

How was this change tested?

Verified connectivity with TLS-enabled Kafka cluster using plaintext authentication
Tested both collector and ingester components
Validated SASL-SSL with PLAIN authentication works as expected

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
[] I have added unit tests for the new functionality
[] I have run lint and test steps successfully
- for jaeger: make lint test

This change fixes the Kafka TLS configuration to work correctly when tls.enabled flag is not provided but authentication=tls is set. Previously, TLS would not be enabled in this case. Changes: - TLS is now properly configured when authentication=tls, regardless of tls.enabled - Maintains backward compatibility with existing tls.enabled flag - Sets explicit insecure mode only when TLS is intentionally disabled Testing: - Added unit tests for TLS configuration scenarios - Verified with local Kafka cluster using TLS authentication - Tested with HotROD example application Resolves jaegertracing#6744 Signed-off-by: Amol Verma <[email protected]>

This change fixes the Kafka TLS configuration to work correctly when using plaintext authentication with TLS enabled. Previously, TLS would only be configured when authentication=tls, breaking SASL-SSL with PLAIN authentication. Changes: - Modified TLS configuration logic to support TLS with other authentication methods - Fixed SASL-SSL with PLAIN authentication scenario - Maintained backward compatibility with existing authentication methods - Restored pre-PR-6270 behavior for TLS configuration Resolves jaegertracing#6744 Signed-off-by: Amol Verma <[email protected]>

codecov · 2025-02-21T01:00:06Z

Codecov Report

Attention: Patch coverage is 41.66667% with 7 lines in your changes missing coverage. Please review.

Project coverage is 49.05%. Comparing base (84212d2) to head (0bdd1a8).

Files with missing lines	Patch %	Lines
pkg/kafka/auth/tls.go	0.00%	5 Missing ⚠️
pkg/kafka/auth/config.go	71.42%	0 Missing and 2 partials ⚠️

❗ There is a different number of reports uploaded between BASE (84212d2) and HEAD (0bdd1a8). Click for more details.

HEAD has 23 uploads less than BASE

Flag BASE (84212d2) HEAD (0bdd1a8)

badger_v1 2 1

kafka-3.x-v1 2 1

memory_v2 2 1

badger_v2 2 1

tailsampling-processor 2 1

grpc_v2 2 1

grpc_v1 2 1

kafka-3.x-v2 2 1

elasticsearch-6.x-v1 2 1

elasticsearch-7.x-v1 2 1

cassandra-4.x-v1-manual 2 1

opensearch-2.x-v1 2 1

cassandra-5.x-v1-manual 2 1

opensearch-1.x-v1 2 1

elasticsearch-8.x-v1 2 1

opensearch-2.x-v2 2 1

elasticsearch-8.x-v2 2 1

unittests 2 0

cassandra-4.x-v2-auto 2 1

cassandra-5.x-v2-auto 2 1

cassandra-4.x-v2-manual 2 1

cassandra-5.x-v2-manual 2 1

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #6764       +/-   ##
===========================================
- Coverage   96.04%   49.05%   -47.00%     
===========================================
  Files         364      177      -187     
  Lines       20692    10697     -9995     
===========================================
- Hits        19874     5247    -14627     
- Misses        624     5056     +4432     
- Partials      194      394      +200

Flag	Coverage Δ
badger_v1	`9.76% <0.00%> (-0.01%)`	⬇️
badger_v2	`1.82% <0.00%> (-0.01%)`	⬇️
cassandra-4.x-v1-manual	`14.81% <0.00%> (-0.01%)`	⬇️
cassandra-4.x-v2-auto	`1.81% <0.00%> (-0.01%)`	⬇️
cassandra-4.x-v2-manual	`1.81% <0.00%> (-0.01%)`	⬇️
cassandra-5.x-v1-manual	`14.81% <0.00%> (-0.01%)`	⬇️
cassandra-5.x-v2-auto	`1.81% <0.00%> (-0.01%)`	⬇️
cassandra-5.x-v2-manual	`1.81% <0.00%> (-0.01%)`	⬇️
elasticsearch-6.x-v1	`19.15% <0.00%> (-0.01%)`	⬇️
elasticsearch-7.x-v1	`19.23% <0.00%> (-0.01%)`	⬇️
elasticsearch-8.x-v1	`19.40% <0.00%> (-0.01%)`	⬇️
elasticsearch-8.x-v2	`1.82% <0.00%> (-0.01%)`	⬇️
grpc_v1	`10.81% <0.00%> (-0.01%)`	⬇️
grpc_v2	`7.80% <0.00%> (-0.01%)`	⬇️
kafka-3.x-v1	`10.19% <41.66%> (+0.05%)`	⬆️
kafka-3.x-v2	`1.82% <0.00%> (-0.01%)`	⬇️
memory_v2	`1.82% <0.00%> (-0.01%)`	⬇️
opensearch-1.x-v1	`19.28% <0.00%> (-0.01%)`	⬇️
opensearch-2.x-v1	`19.28% <0.00%> (-0.01%)`	⬇️
opensearch-2.x-v2	`1.82% <0.00%> (-0.01%)`	⬇️
tailsampling-processor	`0.48% <0.00%> (-0.01%)`	⬇️
unittests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yurishkuro · 2025-02-21T22:20:05Z

what are you trying to achieve by merging main? It erases the CI checks which clearly show that your PR does not pass the linter.

Signed-off-by: Amol Verma <[email protected]>

amilbcahat · 2025-02-21T23:43:47Z

what are you trying to achieve by merging main? It erases the CI checks which clearly show that your PR does not pass the linter.

I was updating the branch to latest, just that. I have committed for Lint checks , now. Can you check again ?

This change fixes the Kafka TLS configuration to work correctly when using plaintext authentication with TLS enabled. Previously, TLS would only be configured when authentication=tls, breaking SASL-SSL with PLAIN authentication. Resolves jaegertracing#6744 Signed-off-by: Amol Verma <[email protected]>

- Fix TLS configuration initialization for Kafka auth - Add proper handling of system CA certs pool - Set secure defaults for TLS configuration - Remove redundant code comments Signed-off-by: Amol Verma <[email protected]> Signed-off-by: Amol Verma <[email protected]>

amilbcahat · 2025-02-23T14:02:27Z

I have made the corrections for Unit Tests, can you update the PR label please ? @yurishkuro and run it again, I dont have necessary permissions to add the label I think.

yurishkuro · 2025-02-23T21:31:25Z

pkg/kafka/auth/config.go

 		tlsClientConfig := tlscfg.ClientFlagsConfig{
 			Prefix: configPrefix,
 		}
 		tlsCfg, err := tlsClientConfig.InitFromViper(v)
 		if err != nil {
 			return fmt.Errorf("failed to process Kafka TLS options: %w", err)
 		}
+		tlsCfg.IncludeSystemCACertsPool = (config.Authentication == tls)


This is needed to maintain the security model difference between TLS authentication and TLS encryption:

When using TLS authentication (auth="tls"), we need system CA certs to validate client certificates

When using TLS encryption with SASL PLAIN auth, we don't need system CA certs
The unit tests specifically verify this distinction.

When using TLS encryption with SASL PLAIN auth, we don't need system CA certs

why? The only time you don't need system certs is if you are providing your own. Am I wrong about that?

Yes, you are right. Both modes need CA certs for TLS validation, but they use different sources:

TLS auth (authentication="tls"):

IncludeSystemCACertsPool=true to validate client/server certificates using system CA pool

Uses mutual TLS where both authenticate each other

SASL PLAIN with TLS:

IncludeSystemCACertsPool=false because it validates the server certificate using explicitly provided CA cert

Client authentication happens via username/password

The tests expect this specific behavior to enforce these different trust models, without this distinction tests fails. Both approaches provide certificate validation, just from different trust sources.

yurishkuro · 2025-02-23T21:32:19Z

pkg/kafka/auth/config.go

 		tlsClientConfig := tlscfg.ClientFlagsConfig{
 			Prefix: configPrefix,
 		}
 		tlsCfg, err := tlsClientConfig.InitFromViper(v)
 		if err != nil {
 			return fmt.Errorf("failed to process Kafka TLS options: %w", err)
 		}
+		tlsCfg.IncludeSystemCACertsPool = (config.Authentication == tls)
+		tlsCfg.Insecure = false


isn't this already being set via .tls.enabled?

While .tls.enabled sets up TLS configuration, we still need to explicitly ensure Insecure=false for all TLS usage. Without this line, the tests fail because they expect Insecure to be false when TLS auth is used.

yurishkuro · 2025-02-23T21:32:48Z

what is the testing procedure for this change? How do we know it does what's needed?

amilbcahat · 2025-02-24T13:42:06Z

what is the testing procedure for this change? How do we know it does what's needed?

To verify this fix works, I've set up a test environment with:

Kafka container configured for SASL_SSL:
- Uses TLS encryption with SASL PLAIN authentication

Docker Image configuration used -

version: "3"
services:
  zookeeper:
    image: bitnami/zookeeper:latest
    ports:
      - "2181:2181"
    environment:
      - ALLOW_ANONYMOUS_LOGIN=yes
  kafka:
    image: bitnami/kafka:3.7.0
    ports:
      - "9092:9092"
      - "29093:29093"
    environment:
      - KAFKA_BROKER_ID=1
      - KAFKA_CFG_ZOOKEEPER_CONNECT=zookeeper:2181
      - KAFKA_OFFSETS_TOPIC_REPLICATION_FACTOR=1
      - ALLOW_PLAINTEXT_LISTENER=yes
      - KAFKA_CFG_LISTENER_SECURITY_PROTOCOL_MAP=INTERNAL:PLAINTEXT,EXTERNAL:SASL_SSL
      - KAFKA_CFG_LISTENERS=internal://0.0.0.0:9092,external://0.0.0.0:29093
      - KAFKA_CFG_ADVERTISED_LISTENERS=internal://kafka:9092,external://localhost:29093
      - KAFKA_INTER_BROKER_LISTENER_NAME=INTERNAL
      - KAFKA_TLS_TYPE=JKS
      - KAFKA_CFG_SSL_KEYSTORE_LOCATION=/opt/bitnami/kafka/config/certs/kafka.keystore.jks
      - KAFKA_CFG_SSL_KEYSTORE_PASSWORD=kafkapass123
      - KAFKA_CFG_SSL_KEY_PASSWORD=kafkapass123
      - KAFKA_CFG_SSL_TRUSTSTORE_LOCATION=/opt/bitnami/kafka/config/certs/kafka.truststore.jks
      - KAFKA_CFG_SSL_TRUSTSTORE_PASSWORD=kafkapass123
      - KAFKA_CFG_SSL_ENDPOINT_IDENTIFICATION_ALGORITHM=
      - KAFKA_CFG_SASL_ENABLED_MECHANISMS=PLAIN
      - KAFKA_CFG_SASL_MECHANISM_INTER_BROKER_PROTOCOL=PLAIN
      - KAFKA_CLIENT_USERS=admin
      - KAFKA_CLIENT_PASSWORDS=admin-secret
    volumes:
      - ./certs:/opt/bitnami/kafka/config/certs
    depends_on:
      - zookeeper

Exposes port 29093 for SASL_SSL connections
Configured with admin/admin-secret credentials locally

Test with ingester accessing Kafka:

   ./ingester \
     --kafka.consumer.brokers=localhost:29093 \
     --kafka.consumer.topic=jaeger-spans \
     --kafka.consumer.authentication=plaintext \
     --kafka.consumer.plaintext.username=admin \
     --kafka.consumer.plaintext.password=admin-secret \
     --kafka.consumer.plaintext.mechanism=PLAIN \
     --kafka.consumer.group-id=jaeger-ingester \
     --kafka.consumer.tls.enabled=true \
     --kafka.consumer.tls.ca=./certs/ca.crt \
     --kafka.consumer.tls.skip-host-verify=true

Verified with standard Kafka clients (producer/consumer) that the connection works with the same settings

Without the fix in PR #6764, the ingester command would fail because TLS settings weren't properly applied when using SASL PLAIN authentication. With the fix, the connection succeeds. Do you need any more configuration related information used for testing here ?

yurishkuro · 2025-02-24T13:45:48Z

To verify this fix works, I've set up a test environment with:

Is this something we can add to internal/storage/integration/kafka_test.go?

amilbcahat · 2025-02-24T21:25:35Z

To verify this fix works, I've set up a test environment with:

Is this something we can add to internal/storage/integration/kafka_test.go?

Yes, we can add an integration test in internal/storage/integration/kafka_test.go to verify this scenario works properly. The test would:

Set up a Kafka client with:
- SASL PLAIN authentication
- TLS enabled for encryption
- Connected to a Kafka broker with SASL_SSL listener
Verify the client can successfully connect and produce/consume messages

This would formalize the manual test case I've been using to verify the fix. Would you like me to implement this as part of the PR?

yurishkuro · 2025-02-24T22:04:32Z

Yes, I prefer the tests to be part of the PR. However, is it possible to configure a single instance of Kafka to work with different auth-n methods, or do we need to spin different Kafka container for each auth flavor? The latter is much more expensive to run in the CI.

amilbcahat · 2025-02-24T22:38:39Z

Yes, I prefer the tests to be part of the PR. However, is it possible to configure a single instance of Kafka to work with different auth-n methods, or do we need to spin different Kafka container for each auth flavor? The latter is much more expensive to run in the CI.

Yes, this is possible. We can open different listeners in the same Kafka instance to work with different types of Authentication/security protocols. Should I go ahead with implementation ?

ref: https://developer.confluent.io/courses/security/authentication-basics/#:~:text=Configuring%20Authentication%3A%20Listeners%20and%20Security%20Protocols&text=Essentially%2C%20when%20configuring%20the%20broker,authenticate%2C%20whether%20SSL%20or%20SASL_SSL.

yurishkuro · 2025-02-24T22:51:11Z

yes, sounds good. One broker/container, multiple listeners, multiple tests using different ports.

I would recommend not running a full test suite against each listener, only some basic write/read tests.

amilbcahat · 2025-02-25T05:23:09Z

yes, sounds good. One broker/container, multiple listeners, multiple tests using different ports.

I would recommend not running a full test suite against each listener, only some basic write/read tests.

Sure, I will keep in mind

Amol Verma added 2 commits February 21, 2025 05:30

amilbcahat requested a review from a team as a code owner February 21, 2025 00:15

amilbcahat requested a review from joe-elliott February 21, 2025 00:15

dosubot bot added the storage/kafka label Feb 21, 2025

amilbcahat added 2 commits February 21, 2025 10:00

Merge branch 'jaegertracing:main' into fix-kafka-tls-plaintext-6744

7d80528

Merge branch 'main' into fix-kafka-tls-plaintext-6744

b0b40ab

Fixed Lint Checks

2b355d0

Signed-off-by: Amol Verma <[email protected]>

amilbcahat force-pushed the fix-kafka-tls-plaintext-6744 branch from c2267d2 to 2b355d0 Compare February 21, 2025 23:42

Amol Verma added 3 commits February 22, 2025 05:18

yurishkuro reviewed Feb 23, 2025

View reviewed changes

amilbcahat requested a review from yurishkuro February 24, 2025 13:43

Merge branch 'main' into fix-kafka-tls-plaintext-6744

2ce9dcf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6744 - Fix Kafka TLS configuration with plaintext authentication #6764

6744 - Fix Kafka TLS configuration with plaintext authentication #6764

amilbcahat commented Feb 21, 2025 •

edited

Loading

codecov bot commented Feb 21, 2025 •

edited

Loading

yurishkuro commented Feb 21, 2025

amilbcahat commented Feb 21, 2025

amilbcahat commented Feb 23, 2025 •

edited

Loading

yurishkuro Feb 23, 2025

amilbcahat Feb 24, 2025

yurishkuro Feb 24, 2025

amilbcahat Feb 24, 2025 •

edited

Loading

yurishkuro Feb 23, 2025

amilbcahat Feb 24, 2025

yurishkuro commented Feb 23, 2025

amilbcahat commented Feb 24, 2025

yurishkuro commented Feb 24, 2025

amilbcahat commented Feb 24, 2025

yurishkuro commented Feb 24, 2025

amilbcahat commented Feb 24, 2025

yurishkuro commented Feb 24, 2025

amilbcahat commented Feb 25, 2025

6744 - Fix Kafka TLS configuration with plaintext authentication #6764

Are you sure you want to change the base?

6744 - Fix Kafka TLS configuration with plaintext authentication #6764

Conversation

amilbcahat commented Feb 21, 2025 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Feb 21, 2025 • edited Loading

Codecov Report

yurishkuro commented Feb 21, 2025

amilbcahat commented Feb 21, 2025

amilbcahat commented Feb 23, 2025 • edited Loading

yurishkuro Feb 23, 2025

Choose a reason for hiding this comment

amilbcahat Feb 24, 2025

Choose a reason for hiding this comment

yurishkuro Feb 24, 2025

Choose a reason for hiding this comment

amilbcahat Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

yurishkuro Feb 23, 2025

Choose a reason for hiding this comment

amilbcahat Feb 24, 2025

Choose a reason for hiding this comment

yurishkuro commented Feb 23, 2025

amilbcahat commented Feb 24, 2025

yurishkuro commented Feb 24, 2025

amilbcahat commented Feb 24, 2025

yurishkuro commented Feb 24, 2025

amilbcahat commented Feb 24, 2025

yurishkuro commented Feb 24, 2025

amilbcahat commented Feb 25, 2025

amilbcahat commented Feb 21, 2025 •

edited

Loading

codecov bot commented Feb 21, 2025 •

edited

Loading

amilbcahat commented Feb 23, 2025 •

edited

Loading

amilbcahat Feb 24, 2025 •

edited

Loading