Adding changes to generate jks file programatically to enable ssl/tls in hms by bibith4 · Pull Request #25313 · prestodb/presto

bibith4 · 2025-06-13T12:39:27Z

Description

Adding changes to generate jks file programatically to enable ssl/tls in hms.

Keystore validity

Motivation and Context

Impact

Fixes 25297

Test Plan

CI

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

== NO RELEASE NOTE ==

imjalpreet

@bibith4, please include the keystore validity information in the PR description.

Additionally, I was considering whether we could explore generating the keystore and truststore in memory. This could help eliminate the need for any refreshing in the future.

Let's wait for feedback from others in the community.

tdcmeehan · 2025-06-13T17:41:11Z

I agree with @imjalpreet, we need to avoid checking in the keystore entirely.

imjalpreet

Before starting the review, just a quick note: please exclude these tests from the presto-hive module’s test suite execution, as we have a dedicated suite to run them separately.

You can add the exclusion in the following section:

presto/presto-hive/pom.xml

Lines 536 to 553 in f19fee9

    
           <groupId>org.apache.maven.plugins</groupId> 
        
           <artifactId>maven-surefire-plugin</artifactId> 
        
           <configuration> 
        
               <excludes> 
        
                   <!-- Exclude tests against Glue from build --> 
        
                   <exclude>**/TestHiveClientGlueMetastore.java</exclude> 
        
                   <exclude>**/TestHiveDistributedAggregationsWithExchangeMaterialization.java</exclude> 
        
                   <exclude>**/TestHiveDistributedQueriesWithExchangeMaterialization.java</exclude> 
        
                   <exclude>**/TestHiveDistributedQueriesWithOptimizedRepartitioning.java</exclude> 
        
                   <exclude>**/TestHiveRecoverableExecution.java</exclude> 
        
                   <exclude>**/TestHivePushdownFilterQueries.java</exclude> 
        
                   <exclude>**/TestHivePushdownIntegrationSmokeTest.java</exclude> 
        
                   <exclude>**/TestHivePushdownDistributedQueries.java</exclude> 
        
                   <exclude>**/TestParquetDistributedQueries.java</exclude> 
        
                   <exclude>**/TestHive2InsertOverwrite.java</exclude> 
        
                   <exclude>**/TestHive3InsertOverwrite.java</exclude> 
        
               </excludes> 
        
           </configuration>

bibith4 · 2025-06-19T07:29:37Z

@imjalpreet @tdcmeehan added code to generate keystore and truststore programatically. Please check

imjalpreet

Thank you, @bibith4. Let's try to make this utility generic.

imjalpreet · 2025-08-25T10:25:03Z

Let's try to make this class generic and move it to presto-plugin-toolkit so that we can reuse it across plugins.

@imjalpreet moved the keystore file generation to presto-plugin-toolkit. Can you please check

agrawalreetika

Thanks for making the changes.

I have a question: since we’re making this generic, is the intention also that we’ll be using a single keystore/truststore instance per JVM as I see files & paths are static currenlty?

agrawalreetika · 2025-09-18T16:28:37Z

                // This is required when connecting to ssl enabled hms
                .put("hive.metastore.thrift.client.tls.enabled", "true")
-                .put("hive.metastore.thrift.client.tls.keystore-path", Paths.get((TestHiveSslWithTrustStoreKeyStore.class.getResource("/hive_ssl_enable/hive-metastore.jks")).toURI()).toFile().toString())
+                .put("hive.metastore.thrift.client.tls.keystore-path", SslKeystoreManager.getKeystorePath())


use static import

@agrawalreetika Corrected . Please check.

agrawalreetika · 2025-09-18T16:28:41Z

+                .put("hive.metastore.thrift.client.tls.keystore-path", SslKeystoreManager.getKeystorePath())
                .put("hive.metastore.thrift.client.tls.keystore-password", "123456")
-                .put("hive.metastore.thrift.client.tls.truststore-path", Paths.get((TestHiveSslWithTrustStoreKeyStore.class.getResource("/hive_ssl_enable/hive-metastore-truststore.jks")).toURI()).toFile().toString())
+                .put("hive.metastore.thrift.client.tls.truststore-path", SslKeystoreManager.getTruststorePath())


use static import

@agrawalreetika Corrected . Please check.

agrawalreetika · 2025-09-18T16:28:47Z

                // This is required when connecting to ssl enabled hms
                .put("hive.metastore.thrift.client.tls.enabled", "true")
-                .put("hive.metastore.thrift.client.tls.truststore-path", Paths.get((TestHiveSslWithTrustStore.class.getResource("/hive_ssl_enable/hive-metastore-truststore.jks")).toURI()).toFile().toString())
+                .put("hive.metastore.thrift.client.tls.truststore-path", SslKeystoreManager.getTruststorePath())


use static import

@agrawalreetika Corrected . Please check.

agrawalreetika · 2025-09-18T16:28:51Z

                // This is required when connecting to ssl enabled hms
                .put("hive.metastore.thrift.client.tls.enabled", "true")
-                .put("hive.metastore.thrift.client.tls.keystore-path", Paths.get((TestHiveSslWithKeyStore.class.getResource("/hive_ssl_enable/hive-metastore.jks")).toURI()).toFile().toString())
+                .put("hive.metastore.thrift.client.tls.keystore-path", SslKeystoreManager.getKeystorePath())


use static import

@agrawalreetika Corrected . Please check.

agrawalreetika · 2025-09-18T16:29:22Z


    AbstractHiveSslTest(Map<String, String> sslConfig)
    {
+        SslKeystoreManager.initializeKeystore();


use static import

@agrawalreetika Corrected . Please check.

imjalpreet

Thanks, @bibith4.

imjalpreet · 2025-09-18T23:35:46Z

+                Path targetDir = Paths.get("target", "test-classes", "ssl_enable");
+                Files.createDirectories(targetDir);
+
+                Path keyStoreTarget = targetDir.resolve("metastore.jks");
+                Path trustStoreTarget = targetDir.resolve("metastore-truststore.jks");
+
+                // Delete files if they exist
+                Files.deleteIfExists(keyStoreTarget);
+                Files.deleteIfExists(trustStoreTarget);
+
+                // Copy freshly generated keystores
+                Files.copy(keyStoreFile.toPath(), keyStoreTarget);
+                Files.copy(trustStoreFile.toPath(), trustStoreTarget);


What is the issue when we try to use SslKeystoreManager.getKeystorePath()?

In any case, we should not use the class variables(keyStoreFile, trustStoreFile) directly. We should use getter methods.

@imjalpreet If we use SslKeystoreManager.getKeystorePath(), the files will only exist on the filesystem and not in the test runtime classpath, which will cause classpath lookups to fail.

Changed to use getter methods for keystore and truststore paths

imjalpreet · 2025-09-18T23:40:20Z

+    public static File keyStoreFile;
+    public static File trustStoreFile;


nit: let's make them private

@imjalpreet Corrected . Please check.

imjalpreet · 2025-09-18T23:41:06Z

+    {
+        initializeKeystore();
+        if (trustStoreFile == null || !trustStoreFile.exists()) {
+            throw new IllegalStateException("Keystore file is not initialized or missing");


Suggested change

throw new IllegalStateException("Keystore file is not initialized or missing");

throw new IllegalStateException("Truststore file is not initialized or missing");

@agrawalreetika Corrected . Please check.

bibith4 · 2025-09-22T09:10:09Z

Thanks for making the changes.

I have a question: since we’re making this generic, is the intention also that we’ll be using a single keystore/truststore instance per JVM as I see files & paths are static currenlty?

@agrawalreetika Yes, SslKeystoreManager class is designed to use a single, static keystore and truststore instance per JVM

agrawalreetika · 2025-09-24T02:57:13Z

+    {
+        Security.addProvider(new BouncyCastleProvider());
+
+        String alias = "metastore";


Since this is gonna be used across multiple connector's ssl test so better to keep some gerric Alias here?

Also since this is gonna be used for tests of different module, I think we should move it to presto-tests module instead?

@agrawalreetika moved the jks file generation class to presto-test module and corrected alias. Please check

agrawalreetika

LGTM

imjalpreet

Thanks, @bibith4.

LGTM % final few nits

imjalpreet · 2025-09-25T05:12:05Z

                .put("hive.metastore.thrift.client.tls.enabled", "true")
-                .put("hive.metastore.thrift.client.tls.keystore-path", Paths.get((TestHiveSslWithKeyStore.class.getResource("/hive_ssl_enable/hive-metastore.jks")).toURI()).toFile().toString())
+                .put("hive.metastore.thrift.client.tls.keystore-path", getKeystorePath())
                .put("hive.metastore.thrift.client.tls.keystore-password", "123456")


nit: let's create a static constant for the password in SslKeystoreManager and use it everywhere we are currently hardcoding the password.

@imjalpreet Added static constant for the password. Please check

imjalpreet · 2025-09-25T05:19:09Z

+    {
+    }
+
+    public static synchronized void initializeKeystore()


nit: should we rename this to initializeKeystoreAndTruststore?

Corrected. Please check

imjalpreet · 2025-09-25T05:19:46Z

+        <dependency>
+            <groupId>org.bouncycastle</groupId>
+            <artifactId>bcprov-jdk18on</artifactId>
+            <version>1.81</version>


nit: introduce a pom property for the version

Corrected. Please check

imjalpreet · 2025-09-25T05:20:22Z

+            <exclusions>
+                <exclusion>
+                    <groupId>org.bouncycastle</groupId>
+                    <artifactId>bcpkix-jdk18on</artifactId>
+                </exclusion>
+                <exclusion>
+                    <groupId>org.bouncycastle</groupId>
+                    <artifactId>bcprov-jdk18on</artifactId>
+                </exclusion>
+            </exclusions>


I don't think this is required now

Corrected. Please check

imjalpreet · 2025-09-25T05:21:02Z

+            <exclusions>
+                <exclusion>
+                    <groupId>org.bouncycastle</groupId>
+                    <artifactId>bcpkix-jdk18on</artifactId>
+                </exclusion>
+                <exclusion>
+                    <groupId>org.bouncycastle</groupId>
+                    <artifactId>bcprov-jdk18on</artifactId>
+                </exclusion>
+            </exclusions>


same here, not required anymore

Corrected. Please check

imjalpreet · 2025-09-25T05:21:17Z

+            <exclusions>
+                <exclusion>
+                    <groupId>org.bouncycastle</groupId>
+                    <artifactId>bcpkix-jdk18on</artifactId>
+                </exclusion>
+                <exclusion>
+                    <groupId>org.bouncycastle</groupId>
+                    <artifactId>bcprov-jdk18on</artifactId>
+                </exclusion>
+            </exclusions>


Corrected. Please check

imjalpreet

Thanks, @bibith4. LGTM now.

imjalpreet · 2025-09-25T07:17:03Z

@tdcmeehan, this is ready for a final review, thanks!

imjalpreet

@bibith4 one minor suggestion

imjalpreet · 2025-09-25T10:16:46Z

+                filesToMount.put("ssl_enable/keystore.jks", "/opt/hive/conf/hive-metastore.jks");
+                filesToMount.put("ssl_enable/truststore.jks", "/opt/hive/conf/hive-metastore-truststore.jks");


Sorry, one more suggestion, let's move these two mounts also inside the synchronized section to ensure there is no flakiness during concurrent test runs

@imjalpreet Corrected. Please check

imjalpreet

Thank you, @bibith4.

tdcmeehan · 2025-09-26T20:59:50Z

Please rebase to pick up the test failure fix

bibith4 · 2025-09-30T14:32:42Z

Please rebase to pick up the test failure fix

@tdcmeehan Corrected. Please check

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

imjalpreet reviewed Jun 13, 2025

View reviewed changes

bibith4 force-pushed the fix_hive_ssl_tls_keystore_expiry branch 3 times, most recently from d217c93 to ef6021d Compare June 18, 2025 05:44

imjalpreet reviewed Jun 18, 2025

View reviewed changes

bibith4 force-pushed the fix_hive_ssl_tls_keystore_expiry branch 2 times, most recently from 5964162 to 64d6ad9 Compare June 19, 2025 06:47

bibith4 marked this pull request as ready for review June 19, 2025 11:13

bibith4 requested a review from a team as a code owner June 19, 2025 11:13

bibith4 requested a review from ZacBlanco June 19, 2025 11:13

prestodb-ci requested review from a team, pdabre12 and sh-shamsan and removed request for a team June 19, 2025 11:13

bibith4 changed the title ~~Adding new keystore files with extended validity for Hive tests using SSL/TLS~~ Adding changes to generate jks file programatically to enable ssl/tls in hms Jun 19, 2025

imsayari404 mentioned this pull request Aug 21, 2025

feat(plugin-mongodb): TLS configuration support in MongoClientConfig #25374

Merged

6 tasks

imjalpreet reviewed Aug 25, 2025

View reviewed changes

bibith4 force-pushed the fix_hive_ssl_tls_keystore_expiry branch 2 times, most recently from bb28bb2 to 68637f1 Compare September 2, 2025 07:31

bibith4 requested a review from hantangwangd as a code owner September 2, 2025 07:31

bibith4 force-pushed the fix_hive_ssl_tls_keystore_expiry branch from 68637f1 to e614a79 Compare September 2, 2025 08:30

bibith4 requested a review from imjalpreet September 2, 2025 09:46

agrawalreetika requested changes Sep 18, 2025

View reviewed changes

imjalpreet requested changes Sep 18, 2025

View reviewed changes

bibith4 force-pushed the fix_hive_ssl_tls_keystore_expiry branch 2 times, most recently from fa836e2 to 5e6d01f Compare September 22, 2025 09:08

bibith4 requested a review from agrawalreetika September 22, 2025 09:27

agrawalreetika requested changes Sep 24, 2025

View reviewed changes

bibith4 force-pushed the fix_hive_ssl_tls_keystore_expiry branch from 5e6d01f to 4144f6c Compare September 24, 2025 10:41

bibith4 requested review from elharo, feilong-liu, jaystarshot and vivek-bharathan as code owners September 24, 2025 10:41

bibith4 force-pushed the fix_hive_ssl_tls_keystore_expiry branch from 4144f6c to 4322a3c Compare September 24, 2025 12:28

bibith4 requested a review from agrawalreetika September 24, 2025 12:30

bibith4 force-pushed the fix_hive_ssl_tls_keystore_expiry branch 2 times, most recently from cb5d86c to 4ea07c3 Compare September 24, 2025 13:07

agrawalreetika previously approved these changes Sep 25, 2025

View reviewed changes

imjalpreet requested changes Sep 25, 2025

View reviewed changes

bibith4 force-pushed the fix_hive_ssl_tls_keystore_expiry branch from 4ea07c3 to c5bc8c3 Compare September 25, 2025 06:12

bibith4 dismissed agrawalreetika’s stale review via c3be88c September 25, 2025 06:54

bibith4 force-pushed the fix_hive_ssl_tls_keystore_expiry branch from c5bc8c3 to c3be88c Compare September 25, 2025 06:54

imjalpreet previously approved these changes Sep 25, 2025

View reviewed changes

imjalpreet reviewed Sep 25, 2025

View reviewed changes

bibith4 dismissed imjalpreet’s stale review via 237d733 September 25, 2025 10:25

bibith4 force-pushed the fix_hive_ssl_tls_keystore_expiry branch from c3be88c to 237d733 Compare September 25, 2025 10:25

imjalpreet approved these changes Sep 25, 2025

View reviewed changes

imjalpreet requested a review from tdcmeehan September 25, 2025 10:41

bibith4 force-pushed the fix_hive_ssl_tls_keystore_expiry branch from 237d733 to 984e5d5 Compare September 29, 2025 11:59

Changes to generate jks file programatically to enable ssl/tls in hms

5d62878

bibith4 force-pushed the fix_hive_ssl_tls_keystore_expiry branch from 984e5d5 to 5d62878 Compare September 29, 2025 13:56

tdcmeehan approved these changes Sep 30, 2025

View reviewed changes

tdcmeehan merged commit 7c85a98 into prestodb:master Sep 30, 2025
95 of 100 checks passed

	<groupId>org.apache.maven.plugins</groupId>
	<artifactId>maven-surefire-plugin</artifactId>
	<configuration>
	<excludes>
	<!-- Exclude tests against Glue from build -->
	<exclude>**/TestHiveClientGlueMetastore.java</exclude>
	<exclude>**/TestHiveDistributedAggregationsWithExchangeMaterialization.java</exclude>
	<exclude>**/TestHiveDistributedQueriesWithExchangeMaterialization.java</exclude>
	<exclude>**/TestHiveDistributedQueriesWithOptimizedRepartitioning.java</exclude>
	<exclude>**/TestHiveRecoverableExecution.java</exclude>
	<exclude>**/TestHivePushdownFilterQueries.java</exclude>
	<exclude>**/TestHivePushdownIntegrationSmokeTest.java</exclude>
	<exclude>**/TestHivePushdownDistributedQueries.java</exclude>
	<exclude>**/TestParquetDistributedQueries.java</exclude>
	<exclude>**/TestHive2InsertOverwrite.java</exclude>
	<exclude>**/TestHive3InsertOverwrite.java</exclude>
	</excludes>
	</configuration>

		public static File keyStoreFile;
		public static File trustStoreFile;

	throw new IllegalStateException("Keystore file is not initialized or missing");
	throw new IllegalStateException("Truststore file is not initialized or missing");

		filesToMount.put("ssl_enable/keystore.jks", "/opt/hive/conf/hive-metastore.jks");
		filesToMount.put("ssl_enable/truststore.jks", "/opt/hive/conf/hive-metastore-truststore.jks");

Conversation

bibith4 commented Jun 13, 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!

imjalpreet left a comment

Choose a reason for hiding this comment

Uh oh!

tdcmeehan commented Jun 13, 2025

Uh oh!

imjalpreet left a comment

Choose a reason for hiding this comment

Uh oh!

bibith4 commented Jun 19, 2025

Uh oh!

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

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!

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!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bibith4 commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

agrawalreetika left a comment

Choose a reason for hiding this comment

bibith4 commented Jun 13, 2025 •

edited

Loading

bibith4 commented Sep 22, 2025 •

edited

Loading