feat(plugin-mongodb): TLS configuration support in MongoClientConfig by imsayari404 · Pull Request #25374 · prestodb/presto

imsayari404 · 2025-06-19T06:46:24Z

This PR introduces TLS support in the MongoDB connector

Description

Motivation and Context

Impact

Test Plan

presto> SELECT * FROM mongodb.tm_lakehouse_engine_db_2.aizen;
 emp_name | emp_location | emp_id | dep_id | u_id 
----------+--------------+--------+--------+------
 wade     | sanfransico  |      7 | AF62   |    2 
 Sam      | dallas       |     46 | Zf6    |    8 
 Root     | kochi        |     12 | AF6    |    4 
 tom      | mumbai       |      4 | Ax5    |    1 
 robert   | hyderabad    |      3 | Zf6    |    9 
 ivan     | bangalore    |     43 | BF4    |   10 
 Yan      | dubai        |      8 | BF4    |    3 
 Tom      | kochi        |     58 | AF6    |    6 
 Jorge    | Sydney       |     34 | Zf6    |    5 
 Bot      | bangalore    |     97 | BF4    |    7 
(10 rows)

Query 20250806_093152_00001_ujc79, FINISHED, 1 node
Splits: 17 total, 17 done (100.00%)
[Latency: client-side: 0:01, server-side: 0:01] [10 rows, 390B] [8 rows/s, 329B/s]

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

MongoDB Connector Changes
* Add TLS/SSL support with automatic JKS and PEM certificate format detection. Configure using ``mongodb.tls.enabled``, ``mongodb.tls.keystore-path``, 
  ``mongodb.tls.keystore-password``, ``mongodb.tls.truststore-path``, and ``mongodb.tls.truststore-password`` properties.
* Upgrade MongoDB Java Driver to 3.12.14.

prestodb-ci · 2025-06-19T07:53:45Z

@ethanyzhang imported this issue as lakehouse/presto #25374

agrawalreetika

Please add required documentation entries with the new configs in Mongo connector documentation.

presto-mongodb/src/main/java/com/facebook/presto/mongodb/MongoClientConfig.java

agrawalreetika · 2025-06-23T06:30:18Z

presto-mongodb/src/main/java/com/facebook/presto/mongodb/MongoClientModule.java

        implements Module
 {
+    private static final Logger log = Logger.get(MongoClientModule.class);
+    public static final String PROTOCOL = "SSL";


Would it be better to enhance Security Protocol to TLS?

presto-mongodb/src/main/java/com/facebook/presto/mongodb/MongoClientModule.java

agrawalreetika · 2025-06-23T06:42:04Z

presto-mongodb/src/main/java/com/facebook/presto/mongodb/MongoClientModule.java

+            }
+            validateCertificates(keystore);
+            KeyManagerFactory keyManagerFactory =
+                    KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());


Same here use static import

agrawalreetika · 2025-06-23T07:01:44Z

presto-mongodb/src/main/java/com/facebook/presto/mongodb/MongoClientConfig.java

+        return true;
+    }
+
+    public boolean getTlsEnabled()


Suggested change

public boolean getTlsEnabled()

public boolean isTlsEnabled()

presto-mongodb/src/main/java/com/facebook/presto/mongodb/MongoClientModule.java

presto-mongodb/src/test/java/com/facebook/presto/mongodb/TestMongoClientConfig.java

agrawalreetika · 2025-06-24T17:51:50Z

presto-mongodb/src/main/java/com/facebook/presto/mongodb/MongoClientConfig.java


+    /**
+     * @deprecated Use {@link #isTlsEnabled()} instead. This method is kept for backward compatibility.
+     */


Little confused, What backward compatibility is being enforced with this method now if you are already pointing @LegacyConfig("mongodb.ssl.enabled")? Do we still need this?

You're absolutely right. Since we're using @LegacyConfig("mongodb.ssl.enabled") on the setTlsEnabled() method, the old SSL configuration will automatically be routed to the new TLS configuration. I will remove the deprecated methods.

agrawalreetika · 2025-06-24T17:52:02Z

presto-mongodb/src/main/java/com/facebook/presto/mongodb/MongoClientConfig.java

    {
-        return this.sslEnabled;
+        return this.tlsEnabled;
    }


Same as above?

presto-spi/src/main/java/com/facebook/presto/spi/security/SslContextProvider.java

agrawalreetika · 2025-06-24T18:01:57Z

presto-spi/src/main/java/com/facebook/presto/spi/security/SslContextProvider.java

+        return trustStore;
+    }
+
+    public static void validateCertificates(KeyStore keyStore) throws GeneralSecurityException


Make it private?

agrawalreetika · 2025-06-24T18:02:14Z

presto-spi/src/main/java/com/facebook/presto/spi/security/SslContextProvider.java

+        return (X509TrustManager) trustManagers[0];
+    }
+
+    public static KeyStore loadTrustStore(File trustStorePath, Optional<String> trustStorePassword)


Can be private?

agrawalreetika · 2025-06-24T18:12:13Z

presto-mongodb/src/main/java/com/facebook/presto/mongodb/MongoClientModule.java

+            Optional<SSLContext> sslContext = sslContextProvider.buildSslContext();
+            if (sslContext.isPresent()) {
+                options.sslContext(sslContext.get());
+                options.sslEnabled(true);
+                log.debug("SSL enabled for MongoDB client with TLS protocol");
+            }


Suggested change

Optional<SSLContext> sslContext = sslContextProvider.buildSslContext();

if (sslContext.isPresent()) {

options.sslContext(sslContext.get());

options.sslEnabled(true);

log.debug("SSL enabled for MongoDB client with TLS protocol");

}

sslContextProvider.buildSslContext()

.ifPresent(sslContext -> {

options.sslContext(sslContext);

options.sslEnabled(true);

});

agrawalreetika · 2025-06-24T18:13:00Z

presto-mongodb/pom.xml

+        <dependency>
+            <groupId>com.facebook.airlift</groupId>
+            <artifactId>security</artifactId>
+        </dependency>


Why do we need this? and below pom changes?

agrawalreetika · 2025-06-24T18:14:48Z

presto-spi/pom.xml

+        <dependency>
+            <groupId>com.facebook.airlift</groupId>
+            <artifactId>security</artifactId>
+            <scope>provided</scope>


Why are we marking this and below as scope provided?

The presto-spi pom changes are required because I've moved SslContextProvider to the spi package to make it reusable across connectors. The presto-mongodb dependency was added to resolve compilation issues when using SslContextProvider but I have removed them now with the new changes.

I added the presto-spi pom changes because SslContextProvider directly imports and uses Logger from airlift.log and PemReader from airlift.security.pem , without these dependencies, the code won't compile as shown in the compilation errors.

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:compile (default-compile) on project presto-spi: Compilation failure: Compilation failure: [ERROR] /Users/sayarimukherjee/Desktop/os-presto/presto/presto-spi/src/main/java/com/facebook/presto/spi/security/SslContextProvider.java:[16,32] package com.facebook.airlift.log does not exist [ERROR] /Users/sayarimukherjee/Desktop/os-presto/presto/presto-spi/src/main/java/com/facebook/presto/spi/security/SslContextProvider.java:[17,41] package com.facebook.airlift.security.pem does not exist [ERROR] /Users/sayarimukherjee/Desktop/os-presto/presto/presto-spi/src/main/java/com/facebook/presto/spi/security/SslContextProvider.java:[54,26] cannot find symbol [ERROR] symbol: class Logger [ERROR] location: class com.facebook.presto.spi.security.SslContextProvider [ERROR] /Users/sayarimukherjee/Desktop/os-presto/presto/presto-spi/src/main/java/com/facebook/presto/spi/security/SslContextProvider.java:[54,39] cannot find symbol [ERROR] symbol: variable Logger [ERROR] location: class com.facebook.presto.spi.security.SslContextProvider [ERROR] /Users/sayarimukherjee/Desktop/os-presto/presto/presto-spi/src/main/java/com/facebook/presto/spi/security/SslContextProvider.java:[132,24] cannot find symbol [ERROR] symbol: variable PemReader [ERROR] location: class com.facebook.presto.spi.security.SslContextProvider [ERROR] /Users/sayarimukherjee/Desktop/os-presto/presto/presto-spi/src/main/java/com/facebook/presto/spi/security/SslContextProvider.java:[177,54] cannot find symbol [ERROR] symbol: variable PemReader [ERROR] location: class com.facebook.presto.spi.security.SslContextProvider [ERROR] -> [Help 1]

steveburnett · 2025-06-30T13:43:17Z

Does https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/connector/mongodb.rst need any documentation update for this?

imsayari404 · 2025-06-30T14:37:16Z

Does https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/connector/mongodb.rst need any documentation update for this?

@steveburnett , yes the documentation will need to be updated. I'm currently working on addressing the review feedback from @agrawalreetika and will include the necessary documentation updates for all the new TLS configuration properties in the next commit. This will include proper documentation for the new mongodb.tls.* configs and deprecation notices for the old SSL setting. I'll make sure to get these changes pushed soon.

agrawalreetika

Thanks for making the changes. Overall LGTM.

Please add the documentation & squash all your commits into one.

agrawalreetika · 2025-06-30T17:09:22Z

presto-mongodb/src/main/java/com/facebook/presto/mongodb/MongoClientConfig.java

+                keystorePath != null ||
+                keystorePassword != null ||
+                truststorePath != null ||
+                truststorePassword != null;


Why are we checking these here?

This was added to handle cases where users provide TLS-related configurations like mongodb.tls.keystore-path but forget to explicitly enable mongodb.tls.enabled=true. Automatically enabling TLS in such cases helps prevent common configuration mistakes. However, I'm happy to remove or adjust this logic if you think it’s unnecessary.

But whenever mongodb.tls.enabled=false then anyways SSL wouldn't be configured right? https://github.com/prestodb/presto/pull/25374/files#diff-5461194df57bf18a7c80e310cc21d80dc5e078b9707226b0d5f49728c02ae12eR87

You're right. I see the issue now. The current logic in isTlsEnabled() is problematic because it automatically enables TLS whenever any TLS-related properties are set, regardless of the mongodb.tls.enabled flag. This could lead to unexpected behavior.
I'll simplify this to just return this.tlsEnabled and remove the automatic detection logic.

agrawalreetika

@imsayari404 There are compilation failures, please fix those, add the documentation & squash all your commits into one.

imsayari404 · 2025-07-04T15:15:34Z

@imsayari404 There are compilation failures, please fix those, add the documentation & squash all your commits into one.

@agrawalreetika The updated changes are now pushed to the branch.

presto-docs/src/main/sphinx/connector/mongodb.rst

presto-spi/src/main/java/com/facebook/presto/spi/security/SslContextProvider.java

presto-docs/src/main/sphinx/connector/mongodb.rst

tdcmeehan · 2025-08-20T19:15:35Z

presto-plugin-toolkit/src/test/resources/future-keystore.jks

I just want to make sure that this will expire very far into the future?

or we could generate it programmatically? I just remembered we were trying it for HMS SSL tests: #25313. Looks like I forgot to review it further but that could be one alternative.

Updated to use programmatic certificate generation similar to the Hive SSL tests (#25313).

imsayari404 · 2025-08-22T19:20:40Z

@tdcmeehan, could you please review this PR at your convenience?

Needs squash

tdcmeehan · 2025-08-22T19:37:02Z

@imjalpreet PTAL.

@imsayari404 please squash commits.

imsayari404 · 2025-08-22T20:01:42Z

Thanks @tdcmeehan , I've squashed the commits
@imjalpreet, could you please review this PR at your convenience?

imjalpreet · 2025-08-25T10:20:42Z

...t/src/test/java/com/facebook/presto/plugin/base/security/MongoSslTestCertificateManager.java

Since we are using the same implementation, I will finish the review of #25313 and get it added to a common module so that we can re-use across plugins.

okay @imjalpreet, thank you!

imjalpreet

@imsayari404, I don't see the new test for TLS I was reviewing earlier. Can you please take a look?

imjalpreet

Thanks, @imsayari404. I did another pass. One question: Is it possible to add a test for TLS specifically for the MongoDB connector?

presto-cassandra/pom.xml

presto-mongodb/src/main/java/com/facebook/presto/mongodb/MongoClientConfig.java

presto-tests/src/test/java/com/facebook/presto/tests/TestSslContextProvider.java

presto-tests/pom.xml

presto-mongodb/src/test/java/com/facebook/presto/mongodb/TestMongoClientConfig.java

agrawalreetika · 2025-10-14T07:32:16Z

presto-mongodb/src/test/java/com/facebook/presto/mongodb/TestMongoTlsConfiguration.java

+            SslContextProvider sslContextProvider = new SslContextProvider(
+                    config.getKeystorePath(),
+                    config.getKeystorePassword(),
+                    config.getTruststorePath(),
+                    config.getTruststorePassword());


Let's create a method for creating this object and reuse

I've updated

presto-mongodb/src/test/java/com/facebook/presto/mongodb/TestMongoTlsConfiguration.java

agrawalreetika · 2025-10-14T07:36:41Z

presto-tests/src/test/java/com/facebook/presto/tests/TestSslContextProvider.java

+        SslKeystoreManager.initializeKeystoreAndTruststore();
+
+        keystoreFile = new File(SslKeystoreManager.getKeystorePath());
+        truststoreFile = new File(SslKeystoreManager.getTruststorePath());


use static imports

I've updated

agrawalreetika

Mostly lgtm, just few nits

presto-tests/src/test/java/com/facebook/presto/tests/TestSslContextProvider.java

presto-mongodb/src/main/java/com/facebook/presto/mongodb/MongoClientModule.java

Cherry-pick of commits in https://github.ibm.com/lakehouse/presto/pull/447/commits Commits picked - d2c50df 7c6a8b1 Co-authored-by: Anant Aneja <1797669+aaneja@users.noreply.github.com>

agrawalreetika

lgtm

imjalpreet

Thanks, @imsayari404. LGTM now.

prestodb-ci added the from:IBM PR from IBM label Jun 19, 2025

imsayari404 marked this pull request as ready for review June 19, 2025 06:47

imsayari404 requested a review from a team as a code owner June 19, 2025 06:47

imsayari404 requested a review from hantangwangd June 19, 2025 06:47

prestodb-ci requested review from a team, agrawalreetika and wanglinsong and removed request for a team June 19, 2025 06:47

imsayari404 requested a review from aaneja June 19, 2025 06:49

agrawalreetika requested changes Jun 23, 2025

View reviewed changes

imsayari404 force-pushed the mongodb_tls branch from 5168f18 to 1a59fd2 Compare June 23, 2025 15:19

imsayari404 requested a review from agrawalreetika June 23, 2025 15:23

agrawalreetika requested changes Jun 24, 2025

View reviewed changes

imsayari404 requested a review from agrawalreetika June 30, 2025 14:29

agrawalreetika reviewed Jun 30, 2025

View reviewed changes

imsayari404 requested a review from agrawalreetika June 30, 2025 17:53

agrawalreetika reviewed Jul 1, 2025

View reviewed changes

imsayari404 force-pushed the mongodb_tls branch from 0d1b1c4 to a212417 Compare July 4, 2025 14:34

imsayari404 requested review from elharo and steveburnett as code owners July 4, 2025 14:34

imsayari404 force-pushed the mongodb_tls branch 3 times, most recently from f6d48f7 to e89a22c Compare July 4, 2025 15:11

agrawalreetika reviewed Jul 7, 2025

View reviewed changes

presto-docs/src/main/sphinx/connector/mongodb.rst Show resolved Hide resolved

agrawalreetika reviewed Jul 7, 2025

View reviewed changes

presto-spi/src/main/java/com/facebook/presto/spi/security/SslContextProvider.java Show resolved Hide resolved

presto-docs/src/main/sphinx/connector/mongodb.rst Show resolved Hide resolved

imsayari404 force-pushed the mongodb_tls branch 2 times, most recently from e025cf6 to d1b46b5 Compare August 20, 2025 16:36

tdcmeehan reviewed Aug 20, 2025

View reviewed changes

imsayari404 force-pushed the mongodb_tls branch from d1b46b5 to 3224516 Compare August 21, 2025 06:29

imsayari404 requested a review from tdcmeehan August 21, 2025 06:29

tdcmeehan previously approved these changes Aug 22, 2025

View reviewed changes

imsayari404 force-pushed the mongodb_tls branch from 3224516 to e46ea96 Compare August 22, 2025 20:00

imjalpreet reviewed Aug 25, 2025

View reviewed changes

tdcmeehan previously approved these changes Aug 25, 2025

View reviewed changes

imsayari404 dismissed tdcmeehan’s stale review via 8d72d78 September 22, 2025 09:56

imsayari404 force-pushed the mongodb_tls branch 2 times, most recently from 8d72d78 to a5cb2b3 Compare September 22, 2025 15:29

imsayari404 requested a review from ZacBlanco as a code owner September 22, 2025 15:29

imjalpreet requested changes Oct 13, 2025

View reviewed changes

imjalpreet reviewed Oct 13, 2025

View reviewed changes

agrawalreetika requested changes Oct 14, 2025

View reviewed changes

agrawalreetika reviewed Oct 22, 2025

View reviewed changes

presto-tests/src/test/java/com/facebook/presto/tests/TestSslContextProvider.java Show resolved Hide resolved

presto-mongodb/src/main/java/com/facebook/presto/mongodb/MongoClientModule.java Outdated Show resolved Hide resolved

Add TLS support in mongodb connector

3b52f3a

Cherry-pick of commits in https://github.ibm.com/lakehouse/presto/pull/447/commits Commits picked - d2c50df 7c6a8b1 Co-authored-by: Anant Aneja <1797669+aaneja@users.noreply.github.com>

agrawalreetika approved these changes Oct 22, 2025

View reviewed changes

tdcmeehan approved these changes Oct 27, 2025

View reviewed changes

imjalpreet approved these changes Oct 27, 2025

View reviewed changes

unidevel mentioned this pull request Nov 14, 2025

Add release notes for 0.296 unix280/presto#47

Merged

15 tasks

This was referenced Nov 24, 2025

Add release notes for 0.296 unix280/presto#48

Merged

Add release notes for 0.296 unix280/presto#49

Merged

unidevel mentioned this pull request Dec 1, 2025

Add release notes for 0.296 unix280/presto#50

Merged

prestodb-ci mentioned this pull request Dec 1, 2025

fix(docs): Add release notes for 0.296 #26726

Merged

19 tasks

Conversation

imsayari404 commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

prestodb-ci commented Jun 19, 2025

Uh oh!

agrawalreetika left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

imsayari404 Jun 30, 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!

steveburnett commented Jun 30, 2025

Uh oh!

imsayari404 commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agrawalreetika 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!

agrawalreetika left a comment • 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!

imsayari404 commented Jul 4, 2025

Uh oh!

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!

imsayari404 commented Aug 22, 2025

imsayari404 commented Jun 19, 2025 •

edited

Loading

imsayari404 Jun 30, 2025 •

edited

Loading

imsayari404 commented Jun 30, 2025 •

edited

Loading

agrawalreetika left a comment •

edited

Loading

agrawalreetika left a comment •

edited

Loading