Support custom trust stores for https by madhavvishnubhatta · Pull Request #295 · Aiven-Open/http-connector-for-apache-kafka

madhavvishnubhatta · 2025-10-01T10:16:02Z

Added custom trust store support. Can you please review this?

This will fix the issue #293

I tested this with both http and https (with custom trust store) endpoints.

…https endpoints

…rust store

src/main/java/io/aiven/kafka/connect/http/sender/SslContextBuilder.java

madhavvishnubhatta · 2025-10-03T09:12:10Z

@muralibasani Just a follow-up to see if you have any other feedback on the pull request.

muralibasani · 2025-10-03T09:13:09Z

@muralibasani Just a follow-up to see if you have any other feedback on the pull request.

@madhavvishnubhatta ok from me, but pinged other team members for a 2nd review.

Claudenw

Please try with NO_DEFAULT values. It should work as expected. Otherwise I will approve this as it stands.

src/main/java/io/aiven/kafka/connect/http/config/HttpSinkConfig.java

jclarysse · 2025-10-03T12:57:31Z

src/main/java/io/aiven/kafka/connect/http/sender/SslContextBuilder.java

+            return null;
+        }
+        try {
+            final KeyStore trustStore = KeyStore.getInstance("JKS");


Would it be a good idea to also add a config for the keystore type?

Yes, but jks meets my need. I can think of adding other types later if I have the time, but it would be good to not hold this off until other support is available. Let me know what you think. if you agree, I can update teh documentation to make it clear that only JKS is supported for now.

I think it's probably fine to not add the keystore type immediately -- IIUC, adding the truststore here is just a mechanism to create an allow list of acceptable HTTPS servers (not to verify that the client is acceptable). This is by far the most frequent usage (e.g., when my browser contacts https://github.com, the server never checks who I am).

Correct. This is only for the client to validate the server. This is not a mechanism for the server to check who is connecting. But makes me think if mTLS might be a future feature request. Anyway, that is out of scope for this PR.

docs/sink-connector-config-options.rst

RyanSkraba · 2025-10-03T17:05:09Z

Hello! I'm going through this and it looks pretty good so far! Thanks.

I'm assuming the use case is that you have a self-signed HTTPS server, or with a private certificate authority, and you want to avoid http.ssl.trust.all.certs in production? This seems like the right way to go.

I guess technically trusting all certs is dubious 🤔 but since the end user is the only one telling the connector where to connect, it's already a pretty explicit "allow list". Is there another use for adding the trust store? It might be worthwhile documenting in the PR!

RyanSkraba

Hey, this looks good -- do you think it would be possible to generate the truststore.jks programmatically instead of checking in the binary?

If it's possible, it's probably more future proof.

RyanSkraba · 2025-10-03T17:11:15Z

src/main/java/io/aiven/kafka/connect/http/sender/TruststoreLoader.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+final class TruststoreLoader {


Suggested change

final class TruststoreLoader {

final class TrustStoreLoader {

Haha, the canonical spelling of TrustStore has an internal capital S. It's definitely NOT that important but it's definitely worth getting it right at the start (as someone who has fussed with APIs and documentation around Elasticsearch and OpenSearch).

RyanSkraba · 2025-10-03T17:15:34Z

src/main/java/io/aiven/kafka/connect/http/config/HttpSinkConfig.java

        return getBoolean(HTTP_SSL_TRUST_ALL_CERTIFICATES);
    }

+    public final String sslTruststoreLocation() {


Suggested change

public final String sslTruststoreLocation() {

public final String sslTrustStoreLocation() {

Haha, the Java spelling of TrustStore has an internal capital S.

It's a nit-pick, but I've spent too much time chasing down Elasticsearch vs. OpenSearch inconsistencies 😆 I only care about the public methods and classnames though!

Absolutely. As a pedant myself 😄 , I shouldn't have missed it. And will definitely fix it.

Fixed this in the latest commit.

RyanSkraba · 2025-10-03T17:16:40Z

src/test/java/io/aiven/kafka/connect/http/config/HttpSinkConfigTest.java

+
+        final var config = new HttpSinkConfig(properties);
+        assertThat(config.sslTruststoreLocation()).isEqualTo("/path/to/truststore.jks");
+        assertThat(config.sslTruststorePassword()).isEqualTo("password123");


If I understand correctly (especially for TrustStores), the password is only used to check integrity.

Correct. Password for the trust stores is only for integrity.

madhavvishnubhatta · 2025-10-03T19:11:02Z

Hello! I'm going through this and it looks pretty good so far! Thanks.

I'm assuming the use case is that you have a self-signed HTTPS server, or with a private certificate authority, and you want to avoid http.ssl.trust.all.certs in production? This seems like the right way to go.

I guess technically trusting all certs is dubious 🤔 but since the end user is the only one telling the connector where to connect, it's already a pretty explicit "allow list". Is there another use for adding the trust store? It might be worthwhile documenting in the PR!

Your assumption about the usecase is correct.

To your question about whether telling the connector where to connect is already an explicit allow list, and why there is a need for more security, I dont think that is valid. The trust store ensures that the https endpoint being reached is not compromised by some bad actors (or due to some DNS trickery). This is especially true when the https endpoint is managed/controlled by someone external to the org that is running the connector. Let me know if that makes sense.

… done. Also changed 'Truststore' to 'TrustStore' (note the 'S' in caps)

madhavvishnubhatta · 2025-10-06T09:55:42Z

I have answered all questions asked so far and also made the suggested changes. Please take a look and let me know if you have more feedback.

RyanSkraba · 2025-10-06T11:50:14Z

Oh my, this is amazing -- I was just going to log in and say not to bother doing the generated truststore exercise, because it was too difficult 😆 Really nice work, I appreciate you taking the time to educate us on your use cases. I'll merge this as soon as the CI passes!

RyanSkraba · 2025-10-06T11:53:57Z

Ooops, looks like it's missing a dependency for the trust store generation! I still think it's a good idea (to not check in a truststore binary), but if you just want to revert that chunk of code, we can make an issue to future-proof that part later! Up to you -- I'm happy with the tests, and the non-test code looks just right!

madhavvishnubhatta · 2025-10-06T12:04:20Z

Ooops, looks like it's missing a dependency for the trust store generation! I still think it's a good idea (to not check in a truststore binary), but if you just want to revert that chunk of code, we can make an issue to future-proof that part later! Up to you -- I'm happy with the tests, and the non-test code looks just right!

I have pushed in another commit with the dependencies. Let me check if the CI passes now.

madhavvishnubhatta · 2025-10-06T12:15:10Z

Oh my, this is amazing -- I was just going to log in and say not to bother doing the generated truststore exercise, because it was too difficult 😆 Really nice work, I appreciate you taking the time to educate us on your use cases. I'll merge this as soon as the CI passes!

Just to be transparent, I used Amazon Q Developer to help 🙂

madhavvishnubhatta · 2025-10-07T09:56:18Z

Can someone approve this workflow so that I can debug failures (if any)?

RyanSkraba · 2025-10-07T13:50:19Z

LGTM, thanks again!

madhavvishnubhatta · 2025-10-30T17:19:24Z

@RyanSkraba What is the release policy? Any idea when the next release will be, of which this will be a part?

madhavvishnubhatta added 8 commits October 1, 2025 11:10

With a Keystore, the connect workers get deployed on MSK

4e4f014

Added code to try out different ways to access the trust store file

eb93d75

Modified to use the jks as trust store. Tested and it works to reach …

37095bc

…https endpoints

Refactored code to meet all Style Checks

f3ab9e7

Added Test cases

b5da6a7

Modified the parameters to add the 'http' prefix

206832b

Updated documentation for the two new config options for SSL custom t…

44c4212

…rust store

Used Logger, instead of print statements

b60f6b0

madhavvishnubhatta requested a review from a team as a code owner October 1, 2025 10:16

madhavvishnubhatta marked this pull request as draft October 1, 2025 10:19

madhavvishnubhatta marked this pull request as ready for review October 1, 2025 10:28

muralibasani reviewed Oct 1, 2025

View reviewed changes

src/main/java/io/aiven/kafka/connect/http/sender/SslContextBuilder.java Outdated Show resolved Hide resolved

Used 'try-with-resources' for TrustStore input stream

0f6a39e

Claudenw requested changes Oct 3, 2025

View reviewed changes

src/main/java/io/aiven/kafka/connect/http/config/HttpSinkConfig.java Show resolved Hide resolved

src/main/java/io/aiven/kafka/connect/http/config/HttpSinkConfig.java Show resolved Hide resolved

madhavvishnubhatta requested a review from Claudenw October 3, 2025 11:17

jclarysse reviewed Oct 3, 2025

View reviewed changes

docs/sink-connector-config-options.rst Show resolved Hide resolved

RyanSkraba changed the title ~~F ssl custom keystore support~~ Support custom trust stores for https Oct 3, 2025

RyanSkraba requested changes Oct 3, 2025

View reviewed changes

Modified test case to generate jks file on the fly and remove it when…

77bbe75

… done. Also changed 'Truststore' to 'TrustStore' (note the 'S' in caps)

Fixed style checks

a20ce63

Claudenw previously approved these changes Oct 6, 2025

View reviewed changes

RyanSkraba previously approved these changes Oct 6, 2025

View reviewed changes

Added missing dependencies for jks generation for testing

0d21144

madhavvishnubhatta dismissed stale reviews from RyanSkraba and Claudenw via 0d21144 October 6, 2025 12:03

File rename from Truststore to TrustStore

6639b09

RyanSkraba approved these changes Oct 7, 2025

View reviewed changes

RyanSkraba merged commit 7247fdf into Aiven-Open:main Oct 7, 2025
6 checks passed

madhavvishnubhatta mentioned this pull request Oct 7, 2025

Ability to pass Custom SSL trust store path as a config parameter #293

Closed

This was referenced Nov 19, 2025

Add Support for Absolute File System Paths in SSL Truststore Location #298

Closed

Allow absolute path for truststore #299

Merged

	final class TruststoreLoader {
	final class TrustStoreLoader {

	public final String sslTruststoreLocation() {
	public final String sslTrustStoreLocation() {

Conversation

madhavvishnubhatta commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

madhavvishnubhatta commented Oct 3, 2025

Uh oh!

muralibasani commented Oct 3, 2025

Uh oh!

Claudenw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madhavvishnubhatta Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RyanSkraba commented Oct 3, 2025

Uh oh!

RyanSkraba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madhavvishnubhatta commented Oct 3, 2025

Uh oh!

madhavvishnubhatta commented Oct 6, 2025

Uh oh!

RyanSkraba commented Oct 6, 2025

Uh oh!

RyanSkraba commented Oct 6, 2025

Uh oh!

madhavvishnubhatta commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madhavvishnubhatta commented Oct 6, 2025

Uh oh!

madhavvishnubhatta commented Oct 7, 2025

Uh oh!

Uh oh!

RyanSkraba commented Oct 7, 2025

Uh oh!

madhavvishnubhatta commented Oct 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

madhavvishnubhatta commented Oct 1, 2025 •

edited

Loading

madhavvishnubhatta Oct 3, 2025 •

edited

Loading

madhavvishnubhatta commented Oct 6, 2025 •

edited

Loading