-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Fix Kafka TLS configuration with plaintext authentication #6764
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
9c3ec31
47d4984
55a2f32
8d43f54
27a5d86
0dcb445
7a95b5a
f12b009
db7b869
9a157b8
5678260
1646f84
84ed2a4
9ed89a3
9b98342
375c83b
7a5d379
baa70e1
54c27ff
5689792
745805f
226173c
1ff71a9
050e222
414287f
0334913
6413e69
3c1e2a6
61505fc
29edb5d
1d059f2
543c934
95b2541
43f4493
f81a07d
f9e385a
9b9c2d0
1dd8f5d
0595c9e
53db82f
e48da8c
a161fd2
571b713
2773835
44c3181
a9011f8
4ecc7e5
0985bcc
81398ab
c54c7dd
9707cbf
7f398ad
bb7a117
ab7ee8b
7dc07e0
21ac946
2e90afc
7094051
14cd2b9
fd5e57b
bde07e5
3f92204
fb3764c
57edf75
c54ef8f
4005176
8175ac1
e91c794
85e569b
48f599a
61b7cbd
367edfc
6ce35cf
84cc45a
5940d0b
dcd70df
a7580ca
47f7c0a
2f5578d
b6c33df
9433894
281d4aa
d6c6844
1716433
ab0ab1b
498871f
a45cf93
eaf4f77
7be2d7a
93931be
5b0e6f5
09afb8d
a67cfc1
e8c5706
5b6ecb7
f5d28e6
f5b146a
573d7ad
2672fec
272f440
5f7ad47
134c283
bc82281
70bab89
d3a2c68
f83aae9
cfc7108
628323b
fcb7787
2046eb9
4a2f549
0220ac3
6cc83d6
f61cd0c
441aac7
4853cc6
3e754f2
055466d
31010c1
9353d3b
245cbc4
bc45128
903d039
4f157de
ef5d4ae
d595ecd
173fdb0
70419f2
88e0784
f1981ab
48b3b5a
a155f09
ef3584e
90029d1
cd2bb59
656d030
308cf1d
e2ba040
1fd8eae
9e122f3
2764ee1
a037094
a070d61
aee34ff
6179775
34cadc8
e16f8fa
246eec3
933160c
b21764b
52fc5d3
2542dc1
8c6fa49
ed7d50e
fe34670
fb97c27
3d7d094
80c7596
2c3b1a5
b874c50
0f03fde
6a90827
e6e273f
9cde1f3
ef2084f
f0920af
d68ed19
496352e
995e098
8bdc001
284bf3d
e0e8b27
021c04d
2d32c46
3ff6ccc
df230c1
e783b44
4a06f9c
da26568
f418712
99e54a9
7665a92
211072a
f6d13e8
8ef884d
e039098
886299d
89b8e33
f667ef5
bb8ccc2
057c896
2697f80
3603ce2
19ff251
c1017e4
bc538f4
851be94
719d9e3
a1623a6
fba042c
cf59a28
65087fa
b5c32a1
541fa5f
3e71d01
752df6b
fc04ce3
b650859
d2888f5
a774fcd
10268cb
c8d585b
12b1163
840ca7d
90776df
1ba836b
942e738
7de5ce6
41dc68c
601ec4a
c798f71
708f405
c76c09a
8496d29
dd232eb
df25180
9b98fc2
f048864
d801254
e0cd314
2154dc2
75309e8
6f0ab10
be3cdb4
f69b83b
d7fb871
38d08bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,58 @@ | ||
version: "3" | ||
services: | ||
kafka: | ||
image: bitnami/kafka:3.9.0@sha256:55df55bfc7ed5980447387620afa3498eab3985a4d8c731013d82b3fa8b43bff | ||
user: "0:0" # Run as root to avoid permission issues | ||
ports: | ||
- "9092:9092" | ||
- "9093:9093" | ||
- "9094:9094" | ||
- "9095:9095" | ||
volumes: | ||
- ../../../internal/config/tlscfg/testdata:/bitnami/kafka/config/certs | ||
- kafka-data:/bitnami | ||
environment: | ||
- KAFKA_CFG_NODE_ID=0 | ||
# KRaft settings | ||
- KAFKA_CFG_NODE_ID=1 | ||
- KAFKA_CFG_PROCESS_ROLES=controller,broker | ||
- KAFKA_CFG_CONTROLLER_QUORUM_VOTERS=0@localhost:9093 | ||
- KAFKA_CFG_LISTENERS=PLAINTEXT://:9092,CONTROLLER://:9093 | ||
- KAFKA_CFG_ADVERTISED_LISTENERS=PLAINTEXT://localhost:9092 | ||
- KAFKA_CFG_LISTENER_SECURITY_PROTOCOL_MAP=CONTROLLER:PLAINTEXT,PLAINTEXT:PLAINTEXT | ||
- KAFKA_CFG_CONTROLLER_QUORUM_VOTERS=1@localhost:9096 | ||
# Listeners | ||
- KAFKA_CFG_LISTENERS=PLAINTEXT://:9092,CONTROLLER://:9096,SASL_SSL://:9093,SASL_PLAINTEXT://:9095 | ||
- KAFKA_CFG_ADVERTISED_LISTENERS=PLAINTEXT://127.0.0.1:9092,SASL_SSL://127.0.0.1:9093,SASL_PLAINTEXT://127.0.0.1:9095 | ||
- KAFKA_CFG_LISTENER_SECURITY_PROTOCOL_MAP=CONTROLLER:PLAINTEXT,PLAINTEXT:PLAINTEXT,SASL_SSL:SASL_SSL,SASL_PLAINTEXT:SASL_PLAINTEXT | ||
- KAFKA_CFG_CONTROLLER_LISTENER_NAMES=CONTROLLER | ||
- KAFKA_CFG_INTER_BROKER_LISTENER_NAME=PLAINTEXT | ||
# SASL Configuration | ||
- KAFKA_CFG_SASL_ENABLED_MECHANISMS=PLAIN | ||
- KAFKA_CLIENT_USERS=admin | ||
- KAFKA_CLIENT_PASSWORDS=admin-secret | ||
# SSL Configuration | ||
- KAFKA_TLS_TYPE=JKS | ||
- KAFKA_CFG_SSL_KEYSTORE_LOCATION=/bitnami/kafka/config/certs/kafka.keystore.jks | ||
- KAFKA_CFG_SSL_KEYSTORE_PASSWORD=kafkapass123 | ||
- KAFKA_CFG_SSL_KEY_PASSWORD=kafkapass123 | ||
- KAFKA_CFG_SSL_TRUSTSTORE_LOCATION=/bitnami/kafka/config/certs/kafka.truststore.jks | ||
- KAFKA_CFG_SSL_TRUSTSTORE_PASSWORD=kafkapass123 | ||
# Allow plaintext listener for development | ||
- ALLOW_PLAINTEXT_LISTENER=yes | ||
# Debug mode for more verbose logs | ||
- BITNAMI_DEBUG=true | ||
# Additional settings | ||
- KAFKA_CFG_AUTO_CREATE_TOPICS_ENABLE=true | ||
# Force skip KRaft initialization if already done | ||
- KAFKA_SKIP_KRAFT_STORAGE_INITIALIZATION=yes | ||
# KRaft cluster ID | ||
- KAFKA_KRAFT_CLUSTER_ID=MkU3OEVBNTcwNTJENDM2Qg | ||
healthcheck: | ||
test: | ||
[ | ||
"CMD-SHELL", | ||
"kafka-topics.sh --list --bootstrap-server localhost:9092", | ||
] | ||
interval: 30s | ||
timeout: 10s | ||
retries: 5 | ||
start_period: 30s | ||
|
||
volumes: | ||
kafka-data: |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,11 +43,20 @@ func (config *AuthenticationConfig) SetConfiguration(saramaConfig *sarama.Config | |
if strings.Trim(authentication, " ") == "" { | ||
authentication = none | ||
} | ||
if config.Authentication == tls { | ||
|
||
tlsConfigured := config.Authentication == tls || | ||
config.TLS.CAFile != "" || | ||
config.TLS.CertFile != "" || | ||
config.TLS.KeyFile != "" || | ||
config.TLS.ServerName != "" || | ||
config.TLS.IncludeSystemCACertsPool | ||
|
||
if tlsConfigured && !config.TLS.Insecure { | ||
if err := setTLSConfiguration(&config.TLS, saramaConfig, logger); err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @amilbcahat @yurishkuro It's checking for individual TLS file fields. I think it's complex when the simple solution can be |
||
return err | ||
} | ||
} | ||
|
||
switch authentication { | ||
case none: | ||
return nil | ||
|
@@ -75,17 +84,20 @@ func (config *AuthenticationConfig) InitFromViper(configPrefix string, v *viper. | |
config.Kerberos.KeyTabPath = v.GetString(configPrefix + kerberosPrefix + suffixKerberosKeyTab) | ||
config.Kerberos.DisablePAFXFast = v.GetBool(configPrefix + kerberosPrefix + suffixKerberosDisablePAFXFAST) | ||
|
||
if config.Authentication == tls { | ||
if !v.IsSet(configPrefix + ".tls.enabled") { | ||
v.Set(configPrefix+".tls.enabled", "true") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't you need this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, we don't need this. The tests are already passing without this, indicating the current implementation handles this case correctly. |
||
} | ||
// For TLS authentication or when TLS is enabled, process TLS options | ||
if config.Authentication == tls || v.GetBool(configPrefix+".tls.enabled") { | ||
tlsClientConfig := tlscfg.ClientFlagsConfig{ | ||
Prefix: configPrefix, | ||
} | ||
var err error | ||
tlsCfg, err := tlsClientConfig.InitFromViper(v) | ||
if err != nil { | ||
return fmt.Errorf("failed to process Kafka TLS options: %w", err) | ||
} | ||
// Set IncludeSystemCACertsPool to true for TLS authentication | ||
tlsCfg.IncludeSystemCACertsPool = (config.Authentication == tls) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this may introduces a security vulnerability by disabling system CA validation for SASL_SSL connections |
||
tlsCfg.Insecure = false | ||
|
||
config.TLS = tlsCfg | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the need for these changes. Some of them just saying the same thing in reverse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right about the PROCESS_ROLES reordering from controller,broker to broker,controller is purely cosmetic. I have reverted that.
However, the other changes are functionally required because we're adding SSL and SASL authentication support to our Kafka integration tests. Here's why:
Our integration test suite now includes TestKafkaStorageWithSASLSSLPlaintext and TestKafkaStorageWithSASLPlaintext
These tests require different connection types: basic plaintext, SASL with SSL, and SASL without SSL
Each needs its own listener and port
Port 9093 was needed for SASL_SSL connections (test requirement)
Had to move controller to port 9096 to avoid conflict
Port 9095 added for SASL_PLAINTEXT connections
SASL_SSL:SASL_SSL,SASL_PLAINTEXT:SASL_PLAINTEXT
This isn't redundant - Kafka requires explicit mapping of each listener to its security protocol
Without these mappings, Kafka doesn't know how to handle the new connection types
4. SSL/SASL Configuration:
All the SSL keystore and SASL authentication settings support the kafka.sh script that generates certificates and runs the secure connection tests
The NODE_ID change from 0→1 was needed because NODE_ID=0 failed to start with our specific multi-listener configuration, while NODE_ID=1 works.