Skip to content
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

KAFKA-18371 TopicBasedRemoteLogMetadataManagerConfig exposes sensitive configuration data in logs #18349

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

Wadimz
Copy link

@Wadimz Wadimz commented Dec 30, 2024

This change addresses issue described in KAFKA-18371

It was noticed that sensitive information is printed as clear text while using default TopicBasedRemoteLogMetadataManagerConfig implementation. Example:
2024-12-20 14:52:56,805] INFO Successfully configured topic-based RLMM with config: TopicBasedRemoteLogMetadataManagerConfig{clientIdPrefix='__remote_log_metadata_client_6', metadataTopicPartitionsCount=50, consumeWaitMs=120000, metadataTopicRetentionMs=-1, metadataTopicReplicationFactor=3, initializationRetryMaxTimeoutMs=120000, initializationRetryIntervalMs=100, commonProps={request.timeout.ms=10000, ssl.client.auth=none, ssl.keystore.location=/etc/eystore.p12, bootstrap.servers=server1:9094, security.protocol=SASL_SSL, password=CLEARTEXT, ssl.truststore.location=/etc/cacerts, ssl.keystore.password=CLEARTEXT, sasl.mechanism=SCRAM-SHA-512, ssl.key.password=CLEARTEXT, sasl.jaas.config=org.apache.kafka.common.security.scram.ScramLoginModule required username="username" password="CLEARTEXT";, ssl.truststore.password=CLEARTEXT, …

  • updated toString method of TopicBasedRemoteLogMetadataManagerConfig to check configuration keys for being sensitive
  • Verified that produced logs has masking applied according to existing rules.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@github-actions github-actions bot added triage PRs from the community storage Pull requests that target the storage module tiered-storage Related to the Tiered Storage feature small Small PRs labels Dec 30, 2024
", commonProps=" + commonProps +
", consumerProps=" + consumerProps +
", producerProps=" + producerProps +
", commonProps=" + configMapToRedactedString(commonProps, AdminClientConfig.configDef()) +
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commonProps used to create AdminClient only

Copy link
Member

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Wadimz nice find!

", producerProps=" + producerProps +
", commonProps=" + configMapToRedactedString(commonProps, AdminClientConfig.configDef()) +
", consumerProps=" + configMapToRedactedString(consumerProps, ConsumerConfig.configDef()) +
", producerProps=" + configMapToRedactedString(producerProps, ProducerConfig.configDef()) +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a unit test to verify that no sensitive strings are output

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chia7712 thanks for the review! Updated test to include validations

@github-actions github-actions bot removed the triage PRs from the community label Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
small Small PRs storage Pull requests that target the storage module tiered-storage Related to the Tiered Storage feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants