Skip to content

ZOOKEEPER-5049: Redact passwords from PrometheusMetricsProvider configuration logging#2387

Merged
anmolnar merged 3 commits into
apache:masterfrom
PDavid:ZOOKEEPER-5049-PrometheusMetricsProvider-log-redact
May 19, 2026
Merged

ZOOKEEPER-5049: Redact passwords from PrometheusMetricsProvider configuration logging#2387
anmolnar merged 3 commits into
apache:masterfrom
PDavid:ZOOKEEPER-5049-PrometheusMetricsProvider-log-redact

Conversation

@PDavid
Copy link
Copy Markdown
Contributor

@PDavid PDavid commented May 14, 2026

Before

When PrometheusMetricsProvider was enabled and configured for HTTPS, on startup, PrometheusMetricsProvider logged all it's configs in clear text on INFO level. This included KeyStore and TrustStore passwords.

Fix

Use the same redaction logic as ZKConfig to avoid logging passwords.

…guration logging

Before

When PrometheusMetricsProvider was enabled and configured for HTTPS, on startup, PrometheusMetricsProvider logged all it's configs in clear text on INFO level. This included KeyStore and TrustStore passwords.

Fix

Use the same redaction logic as ZKConfig to avoid logging passwords.
@PDavid PDavid marked this pull request as ready for review May 14, 2026 15:38
@PDavid
Copy link
Copy Markdown
Contributor Author

PDavid commented May 15, 2026

Log before:

2026-05-13 16:49:22,852 [myid:] - INFO  [main:o.a.z.m.p.PrometheusMetricsProvider@135] - Initializing Prometheus metrics with Jetty, configuration: {ssl.keyStore.location=keystore.jks, ssl.keyStore.password=password, ssl.trustStore.password=password, ssl.enabledProtocols=TLSv1.2,TLSv1.3, httpPort=7000, ssl.ciphersuites=TLS_AES_128_GCM_SHA256,TLS_AES_256_GCM_SHA384,TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, ssl.need.client.auth=false, ssl.trustStore.location=truststore.jks, httpsPort=7000}

Log after:

2026-05-15 08:05:36,017 [myid:] - INFO  [main:o.a.z.m.p.PrometheusMetricsProvider@140] - Initializing Prometheus metrics with Jetty, configuration: {httpsPort=7000, ssl.keyStore.location=keystore.jks, ssl.trustStore.location=truststore.jks, ssl.keyStore.password=***, ssl.trustStore.password=***, ssl.need.client.auth=false, httpPort=7000, ssl.enabledProtocols=TLSv1.2,TLSv1.3, ssl.ciphersuites=TLS_AES_128_GCM_SHA256,TLS_AES_256_GCM_SHA384,TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384}

Copy link
Copy Markdown
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

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

Thank @PDavid , I think this is good. Please check logredactor in ZKConfig and try to consolidate the two approaches.

@PDavid PDavid requested a review from anmolnar May 18, 2026 12:44
@PDavid
Copy link
Copy Markdown
Contributor Author

PDavid commented May 18, 2026

Thank @PDavid , I think this is good. Please check logredactor in ZKConfig and try to consolidate the two approaches.

Thanks, good point. I extracted the log redaction logic to a shared utility.

Copy link
Copy Markdown
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

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

lgtm.

@anmolnar
Copy link
Copy Markdown
Contributor

@meszibalu PTAL.

@anmolnar anmolnar merged commit 34a9492 into apache:master May 19, 2026
16 checks passed
@anmolnar
Copy link
Copy Markdown
Contributor

Merged to master branch. Thanks @PDavid !
branch-3.9 has some conflicts, please create separate pull request.

@PDavid PDavid deleted the ZOOKEEPER-5049-PrometheusMetricsProvider-log-redact branch May 20, 2026 07:08
@PDavid
Copy link
Copy Markdown
Contributor Author

PDavid commented May 20, 2026

Merged to master branch. Thanks @PDavid ! branch-3.9 has some conflicts, please create separate pull request.

Many thanks. I'll create a backport.

PAC-MAJ pushed a commit to PAC-MAJ/zookeeper that referenced this pull request May 20, 2026
…guration logging

Reviewers: anmolnar, meszibalu
Author: PDavid
Closes apache#2387 from PDavid/ZOOKEEPER-5049-PrometheusMetricsProvider-log-redact
@PDavid
Copy link
Copy Markdown
Contributor Author

PDavid commented May 20, 2026

Merged to master branch. Thanks @PDavid ! branch-3.9 has some conflicts, please create separate pull request.

@anmolnar I just realized that secure PrometheusMetricsProvider (HTTPS support) only exists on master branch. So on branch-3.9 there will be no passwords logged because in PrometheusMetricsProvider we only load the following properties:

  • metricsProvider.httpHost
  • metricsProvider.httpPort
  • exportJvmInfo
  • numWorkerThreads
  • maxQueueSize
  • workerShutdownTimeoutMs

In PrometheusMetricsProvider we log the whole Properties object on INFO level though. So if I add metricsProvider.ssl.keyStore.password=password to my zoo.cfg, then it will be logged, does not matter but PrometheusMetricsProvider will not use it.

Do you think we still need to backport this patch to branch-3.9?

@PDavid
Copy link
Copy Markdown
Contributor Author

PDavid commented May 20, 2026

I created the 3.9 backport here #2392 (though I'm not sure how useful it is).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants