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

Add support for truststore and keystore password parameters in Kafdrop #708

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wkd-woo
Copy link

@wkd-woo wkd-woo commented Dec 10, 2024

Description

This PR introduces support for specifying passwords for the truststore and keystore used in SSL communication with Kafka brokers in Kafdrop.

Previously, while the truststore file location could be specified using the KAFKA_TRUSTSTORE_FILE parameter, there was no way to provide the password required to access it.

This enhancement addresses that limitation.

refer: https://docs.oracle.com/javadb/10.8.3.0/adminguide/cadminsslclient.html

Key changes include

  1. New Configuration Parameters
  • Added KAFKA_TRUSTSTORE_PASSWORD to specify the truststore password.
  • Added KAFKA_KEYSTORE_PASSWORD to specify the keystore password.
  1. Code Changes
  • Modified KafkaConfiguration.java to handle these new parameters and set ssl.truststore.password and ssl.keystore.password in the Kafka properties if provided.
  • Ensured that these parameters are optional, maintaining backward compatibility for users who do not require truststore or keystore passwords.
  1. Configuration File Updates
  • Updated application.yml to include placeholders for the new parameters, making them configurable via environment variables:
    • KAFKA_TRUSTSTORE_PASSWORD
    • KAFKA_KEYSTORE_PASSWORD

@Bert-R
Copy link
Collaborator

Bert-R commented Dec 12, 2024

I do not understand the objective of this PR. As mentioned in the README, you can use kafka.properties for configuration information "including key/truststore passwords".

What do you want to add to that?

@wkd-woo
Copy link
Author

wkd-woo commented Dec 14, 2024

@Bert-R
As mentioned in the README, I have already tried specifying truststore.password in kafka.properties, but Kafdrop still couldn't connect to our TLS-enabled Kafka brokers. This issue persisted even though the kafka.properties file was correctly mounted as a volume.

Additionally, in the company I work for, security policies prohibit storing passwords in configuration files like properties. (Honestly, I find this requirement quite perplexing myself.) However, storing passwords in objects such as Kubernetes Secrets is allowed.

This is why I created this PR. I believe that passwords required for certificate access should also be managed as separate parameters.

@Bert-R
Copy link
Collaborator

Bert-R commented Dec 14, 2024

In that case, I suggest a much simpler implementation. We already support SSL_KEY_STORE_PASSWORD. If you search for that string, you'll find two occurrences:

If you copy these lines and adapt them for the trust store, you're all set.

@wkd-woo
Copy link
Author

wkd-woo commented Jan 6, 2025

Thank you for your suggestion, @Bert-R

I understand that we already have SSL_KEY_STORE_PASSWORD for the keystore password.
However, keystore and truststore serve different purposes in TLS communication.
While the keystore is used for server-side authentication, the truststore is necessary for verifying the broker’s certificate in client-side authentication.

Currently, there is no dedicated parameter for truststore.password, which is required for TLS connections with Kafka brokers. Simply duplicating SSL_KEY_STORE_PASSWORD would not solve the issue, as it only addresses the keystore, not the truststore.

This PR proposes adding new parameters (SSL_TRUST_STORE and SSL_TRUST_STORE_PASSWORD) to handle truststore configuration separately, ensuring proper TLS support.

I hope this clarifies the need for the PR. Please review my PR again.

@Bert-R
Copy link
Collaborator

Bert-R commented Jan 6, 2025

Your PR adds KAFKA_TRUSTSTORE_PASSWORD and KAFKA_KEYSTORE_PASSWORD, right? The latter one, we already have. It's named SSL_KEY_STORE_PASSWORD. So there is no need to add that. The only thing that is missing is the trust store password.

We do not have the SSL_TRUST_STORE_PASSWORD yet, but that could be added with this simple change:

diff --git a/README.md b/README.md
index e8e3b41..e4fc3a5 100644
--- a/README.md
+++ b/README.md
@@ -353,6 +353,7 @@ docker run -d --rm -p 9000:9000 \
 | `SSL_KEY_STORE_TYPE`     | Type of SSL keystore. Default is `PKCS12`
 | `SSL_KEY_STORE`          | Path to keystore file
 | `SSL_KEY_STORE_PASSWORD` | Keystore password
+| `SSL_TRUST_STORE_PASSWORD` | Truststore password
 | `SSL_KEY_ALIAS`          | Key alias

 ### Using Helm
diff --git a/src/main/resources/application.yml b/src/main/resources/application.yml
index d80ad48..d52ec18 100644
--- a/src/main/resources/application.yml
+++ b/src/main/resources/application.yml
@@ -10,6 +10,7 @@ server:
     key-store-type: ${SSL_KEY_STORE_TYPE:PKCS12}
     key-store: ${SSL_KEY_STORE:}
     key-store-password: ${SSL_KEY_STORE_PASSWORD:}
+    trust-store-password: ${SSL_TRUST_STORE_PASSWORD:}
     key-alias: ${SSL_KEY_ALIAS:}
     enabled: ${SSL_ENABLED:false}

That's what I meant with "copy these lines and adapt them for the trust store".
Makes sense?

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.

2 participants