Skip to content

[feature] support Kafka client SCRAM authentication - #4320

Open
goutamadwant wants to merge 2 commits into
apache:masterfrom
goutamadwant:feature-kafka-client-scram-auth
Open

[feature] support Kafka client SCRAM authentication#4320
goutamadwant wants to merge 2 commits into
apache:masterfrom
goutamadwant:feature-kafka-client-scram-auth

Conversation

@goutamadwant

Copy link
Copy Markdown

What's changed?

Adds optional SCRAM authentication to Kafka Client monitoring.

  • add PLAINTEXT, SASL_PLAINTEXT, and SASL_SSL security protocol options
  • support SCRAM-SHA-256 and SCRAM-SHA-512 with username and password credentials
  • configure the Kafka AdminClient with the selected security settings while preserving unauthenticated monitoring by default
  • isolate cached AdminClient connections by authentication settings and keep passwords out of protocol diagnostics
  • document the new settings in the English and Chinese Kafka Client monitoring guides

Closes #4209

Verification

  • KafkaProtocolTest (23 tests)
  • KafkaCollectTest (7 tests)
  • YamlCheckScript through the 22-module manager reactor
  • repository Markdown lint (567 files)

Checklist

  • I have read the Contributing Guide
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

Add or update API

  • I have added the necessary e2e tests and all cases have passed.

Not applicable: this change does not add or update an API. SCRAM validation, AdminClient property mapping, credential escaping, cache isolation, and plaintext compatibility are covered by unit tests.

if (!hasSaslAuthentication()) {
return StringUtils.isNotBlank(saslMechanism)
|| StringUtils.isNotBlank(username)
|| StringUtils.isNotBlank(password);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could this cause the existing monitoring to become unavailable after the upgrade?

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review: [feature] support Kafka client SCRAM authentication

Verdict: ✅ APPROVED — well-structured feature with solid test coverage.

What this PR does

Adds SCRAM (SCRAM-SHA-256/512) authentication support for the Kafka client collector:

  • KafkaProtocol: new securityProtocol, saslMechanism, username, password fields; password is @ToString.Exclude (no credential leak in logs); isInvalid() now validates the security protocol against a whitelist and requires credentials when SASL is selected.
  • KafkaCollectImpl: builds SASL JAAS config, correctly escapes \ and " via escapeJaasValue, and folds credentials into the CacheIdentifier so different creds get distinct cached AdminClients.
  • app-kafka_client.yml: conditional fields (depend + hide) for SASL, wired into all 4 metric sets.
  • Docs (EN/ZH) updated; tests added for both collector and protocol.

Assessment

  • Security: Credential escaping + @ToString.Exclude are the right calls. The cache key now distinguishes by username/password/security protocol, preventing credential cross-contamination between monitors.
  • Correctness: hasSaslAuthentication() safely handles a null securityProtocol (no NPE via String.equalsIgnoreCase(null) → false). isInvalid() correctly treats a blank protocol as PLAINTEXT (backward compatible) and enforces all three SASL fields when SASL is on.
  • Tests: Good matrix — no-auth, SCRAM auth, JAAS escaping, cache isolation, whitelist validation, and the toString-no-password guarantee.

Minor (non-blocking) suggestions

  • escapeJaasValue does not escape ;, which would prematurely terminate the JAAS statement if present in a credential. Extreme edge case, but worth a comment or an additional escape.
  • password YAML field has no limit (username has limit: 50); long passwords are plausible — consider adding one.
  • SASL_SSL relies on the JVM trust store with no ssl.* properties supplied; the doc correctly warns about this, but a follow-up could surface truststore config. Acceptable for this scope.

No blocking issues. Nice contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend collector doc Improvements or additions to documentation monitoring-template

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Support Kafka SCRAM authentication in Kafka Client monitoring

3 participants