Changes to enable ssl/tls in hms #24745

bibith4 · 2025-03-18T09:48:40Z

Description

Added SSL/TLS support for hive metastore

Impact

The connection to HMS will be encrypted with TLS for secure communication.

Test Plan

Executed basic queries after enabling TLS in HMS.
Queries got executed successfully.

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 ==

Hive Connector Changes
* Add changes to enable SSL/TLS encryption by accepting the following configuration properties:

    * ``hive.metastore.thrift.client.tls.enabled``
    * ``hive.metastore.thrift.client.tls.keystore-path``
    * ``hive.metastore.thrift.client.tls.keystore-password``
    * ``hive.metastore.thrift.client.tls.truststore-path``
    * ``hive.metastore.thrift.client.tls.truststore-password``

linux-foundation-easycla · 2025-03-18T09:48:45Z

The committers listed above are authorized under a signed CLA.

✅ login: bibith4 (693c872)

steveburnett

Please fix the formatting of the table in lines 216-245. A local doc build doesn't show the table. See screenshot.

Tag me to request a review after the table is restored and I'll be happy to review this again.

steveburnett

Nit of formatting in the Metastore Configuration Properties table.

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

steveburnett

Just one nit of formatting in the text, looks good other than that. Thanks for the quick turnaround!

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

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build, looks good.

Thanks for the doc!

agrawalreetika · 2025-03-23T13:02:03Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/MetastoreClientConfig.java

+    private File metastoreTlsKeystorePath;
+    private String metastoreTlsKeystorePassword;
+    private File metastoreTlsTruststorePath;
+    private String metastoreTlsTruststorePassword;


Since these are already metastore configs, I think we can shorten these variable and call it like in other SSL enabled class -

private boolean tlsEnabled; private File keystorePath; private String keystorePassword; private File truststorePath; private String trustStorePassword;

@agrawalreetika Changed as per the suggestion. Please check

agrawalreetika · 2025-03-23T13:16:11Z

...tore/src/main/java/com/facebook/presto/hive/metastore/thrift/HiveMetastoreClientFactory.java

+     * @param metastoreTrustStorePassword
+     * @return SSLContext
+     */
+    private static Optional<SSLContext> metastoreSslContext(boolean metastoreTlsEnabled, Optional<File> metastoreKeyStorePath, Optional<String> metastoreKeyStorePassword, Optional<File> metastoreTrustStorePath, Optional<String> metastoreTrustStorePassword)


We can call this method something like - buildSslContext

@agrawalreetika Changed as per the suggestion. Please check

agrawalreetika · 2025-03-23T15:23:09Z

...tore/src/main/java/com/facebook/presto/hive/metastore/thrift/HiveMetastoreClientFactory.java

+     */
+    private static Optional<SSLContext> metastoreSslContext(boolean metastoreTlsEnabled, Optional<File> metastoreKeyStorePath, Optional<String> metastoreKeyStorePassword, Optional<File> metastoreTrustStorePath, Optional<String> metastoreTrustStorePassword)
+    {
+        if (!metastoreTlsEnabled || (!metastoreKeyStorePath.isPresent() && !metastoreTrustStorePath.isPresent())) {


I think we can simply this here -

Suggested change

if (!metastoreTlsEnabled || (!metastoreKeyStorePath.isPresent() && !metastoreTrustStorePath.isPresent())) {

if (!metastoreTlsEnabled || (metastoreKeyStorePath.isEmpty() && metastoreTrustStorePath.isEmpty()) {

@agrawalreetika Optional#isEmpty() is a Java 11 method. Not available in java 8

agrawalreetika · 2025-03-23T15:26:06Z

...tore/src/main/java/com/facebook/presto/hive/metastore/thrift/HiveMetastoreClientFactory.java

+            // get X509TrustManager
+            final TrustManager[] trustManagers = trustManagerFactory.getTrustManagers();
+            if (trustManagers.length != 1 || !(trustManagers[0] instanceof X509TrustManager)) {
+                throw new RuntimeException("Unexpected default trust managers:" + Arrays.toString(trustManagers));


This could be more clear here based on the condition here -

Suggested change

throw new RuntimeException("Unexpected default trust managers:" + Arrays.toString(trustManagers));

throw new RuntimeException("Expected exactly one X509TrustManager, but found: " + Arrays.toString(trustManagers));

;

@agrawalreetika Changed as per the suggestion. Please check

agrawalreetika · 2025-03-23T15:41:29Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSslEnableTrustStore.java

+                testTable));
+        computeActual(format("INSERT INTO %s values(1, 'TestName1')", testTable));
+        computeActual(format("INSERT INTO %s values(1, 'TestName2')", testTable));
+        assertQuery(format("SELECT count(*) FROM %s", testTable), "SELECT 2");


Add one more assert for checking the expected SELECT * values from a table

@agrawalreetika added assert for select * also

agrawalreetika · 2025-03-23T15:48:54Z

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveSslEnableTrustStore.java

+        computeActual(format("INSERT INTO %s values(1, 'TestName1')", testTable));
+        computeActual(format("INSERT INTO %s values(1, 'TestName2')", testTable));
+        assertQuery(format("SELECT count(*) FROM %s", testTable), "SELECT 2");
+    }


We could add some more combinations of test and provide different combination of sslConfigurations in queryRunner -

When just truststore is set

When just keystore is set

When both truststore + keystore is set

@agrawalreetika added test cases with all the above mentioned connfigurations

agrawalreetika

LGTM, Thanks for adding the support

Please plan to squash your commits into one with relevant commit message.

bibith4 · 2025-03-26T18:57:59Z

LGTM, Thanks for adding the support

Please plan to squash your commits into one with relevant commit message.

Done

yingsu00 · 2025-03-27T06:04:15Z

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

@@ -232,7 +232,17 @@ Property Name                                                         Descriptio
 ``hive.invalidate-metastore-cache-procedure-enabled``    When enabled, users will be able to invalidate metastore        false
                                                         cache on demand.

-======================================================= ============================================================= ============
+``hive.metastore.thrift.client.tls.enabled``             Whether TLS security is enabled.                                false


The release notes shall show these property names. E.g. "Add a configuration property plan-checker.config-dir to set the configuration directory for PlanCheckerProvider configurations. #23955"

@yingsu00 Updated release notes with newly added properties

yingsu00 · 2025-03-27T06:04:31Z

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

@@ -232,7 +232,17 @@ Property Name                                                         Descriptio
 ``hive.invalidate-metastore-cache-procedure-enabled``    When enabled, users will be able to invalidate metastore        false
                                                         cache on demand.

-======================================================= ============================================================= ============
+``hive.metastore.thrift.client.tls.enabled``             Whether TLS security is enabled.                                false


Why are they all dot separated?

I think we can follow the similar config naming like other connectors with TLS configs -
https://prestodb.io/docs/current/connector/elasticsearch.html#tls-security

.keystore.path -> .keystore-path
.keystore.password -> .keystore-password
.truststore.path -> .truststore-path
.truststore.password -> .truststore-password

WDYT @yingsu00?

@agrawalreetika @yingsu00 existing properties specified in below format
hive.metastore-thrift-client-tls-enabled
Can we follow the same ?

@agrawalreetika @yingsu00 existing properties specified in below format hive.metastore-thrift-client-tls-enabled Can we follow the same ?

Please see my comments inline.

@yingsu00 Changed as per the suggestion

yingsu00 · 2025-03-28T22:08:32Z

...tore/src/main/java/com/facebook/presto/hive/metastore/thrift/HiveMetastoreClientFactory.java

@@ -49,12 +72,138 @@ public HiveMetastoreClientFactory(
    @Inject
    public HiveMetastoreClientFactory(MetastoreClientConfig metastoreClientConfig, HiveMetastoreAuthentication metastoreAuthentication)
    {
-        this(Optional.empty(), Optional.ofNullable(metastoreClientConfig.getMetastoreSocksProxy()), metastoreClientConfig.getMetastoreTimeout(), metastoreAuthentication);
+        this(buildSslContext(metastoreClientConfig.isTlsEnabled(), Optional.ofNullable(metastoreClientConfig.getKeystorePath()), Optional.ofNullable(metastoreClientConfig.getKeystorePassword()), Optional.ofNullable(metastoreClientConfig.getTruststorePath()), Optional.ofNullable(metastoreClientConfig.getTrustStorePassword())), Optional.ofNullable(metastoreClientConfig.getMetastoreSocksProxy()), metastoreClientConfig.getMetastoreTimeout(), metastoreAuthentication);


Line too long. Put each parameter on a separate line

@yingsu00 Updated it by adding parameters in separate lines

yingsu00 · 2025-03-28T22:11:09Z

...tore/src/main/java/com/facebook/presto/hive/metastore/thrift/HiveMetastoreClientFactory.java

+     * @param trustStorePassword
+     * @return SSLContext
+     */
+    private static Optional<SSLContext> buildSslContext(boolean tlsEnabled, Optional<File> keystorePath, Optional<String> keystorePassword, Optional<File> truststorePath, Optional<String> trustStorePassword)


Line too long. Put each parameter on a separate line

@yingsu00 Updated it by adding parameters in separate lines

yingsu00 · 2025-03-28T22:40:58Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/MetastoreClientConfig.java

@@ -317,4 +324,60 @@ public MetastoreClientConfig setInvalidateMetastoreCacheProcedureEnabled(boolean
        this.invalidateMetastoreCacheProcedureEnabled = invalidateMetastoreCacheProcedureEnabled;
        return this;
    }
+
+    @Config("hive.metastore.thrift.client.tls.enabled")


these config properties are specific to the thrift metastore, and they are all used by com/facebook/presto/hive/metastore/thrift/HiveMetastoreClientFactory.java. I think we should create a separate config class under com/facebook/presto/hive/metastore/thrift/. We can name it as ThriftHiveMetastoreConfig.java. All config properties in it shall start with hive.metastore.thrift.

I'd suggest we name these new config properties prefix in ThriftHiveMetastoreConfig.java hive.metastore.thrift.client.tls.xxx. e.g.
hive.metastore.thrift.client.tls.enabled
hive.metastore.thrift.client.tls.keystore-path
"hive.metastore.thrift.client.tls.keystore-password...

@yingsu00 created a separate class 'ThriftHiveMetastoreConfig' and moved all the ssl related properties to that

yingsu00 · 2025-03-28T22:56:30Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/MetastoreClientConfig.java

+    private String keystorePassword;
+    private File truststorePath;
+    private String trustStorePassword;
+


This comment is for line 64-74
Move it to the new com/facebook/presto/hive/metastore/thrift/ThriftHiveMetastoreConfig.java

@yingsu00 when checked i could see that moving this field require changes in many files. Can we do that as a seperate follow-up PR?
cc : @agrawalreetika @imjalpreet

yingsu00 · 2025-03-28T23:00:12Z

presto-hive-metastore/src/main/java/com/facebook/presto/hive/MetastoreClientConfig.java

+    public String getTrustStorePassword()
+    {
+        return trustStorePassword;
+    }


This comment is for line 302 to 314
Move "hive.metastore.thrift.delete-files-on-table-drop" to the new com/facebook/presto/hive/metastore/thrift/ThriftHiveMetastoreConfig.java

I'm not sure if this is only for the thrift client? If it is then it's better to rename it to "hive.metastore.thrift.client.delete-files-on-table-drop" and deprecate "hive.metastore.thrift.delete-files-on-table-drop". I think it is not. @imjalpreet Could you please confirm?

Yes, we can move this to the ThriftHiveMetastoreConfig. The config is only valid when Thrift Hive Metastore is used as the metastore in Hive connector due to a limitation observed a few years back: #17369

@yingsu00 when checked i could see that moving this field require changes in many files. Can we do that as a seperate follow-up PR?
cc : @agrawalreetika @imjalpreet

ethanyzhang · 2025-03-29T02:00:05Z

@bibith4 can you fix the CLA check? also the failing test

Co-authored-by: Arin Mathew <[email protected]> Changes to move ssl related properties to seperate class

bibith4 · 2025-04-01T15:39:45Z

@bibith4 can you fix the CLA check? also the failing test

@ethanyzhang Corrected. Please check

steveburnett · 2025-04-01T18:08:21Z

Thanks for the release note entry! Some suggestions for formatting and to follow the Order of changes in the Release Notes Guidelines:

== RELEASE NOTES ==

General Changes
* Add changes to accept SSL/TLS properties to enable encryption.
* Add configuration properties for SSL/TLS:

    * ``hive.metastore.thrift.client.tls.enabled``
    * ``hive.metastore.thrift.client.tls.keystore-path``
    * ``hive.metastore.thrift.client.tls.keystore-password``
    * ``hive.metastore.thrift.client.tls.truststore-path``
    * ``hive.metastore.thrift.client.tls.truststore-password``
      
Hive Connector Changes
* Improve hive-site.xml to allow configuration of SSL/TLS properties.

steveburnett

LGTM! (docs)

Pull branch, local doc build. Thanks!

yingsu00

The release note should only have Hive Connector changes. Could you please move them? Also cc @steveburnett for a second round of the release note section review

bibith4 · 2025-04-02T03:18:38Z

The release note should only have Hive Connector changes. Could you please move them? Also cc @steveburnett for a second round of the release note section review

@yingsu00 changed release note as per suggestion. @steveburnett Can you please verify

agrawalreetika

Code changes, LGTM

steveburnett · 2025-04-02T13:52:03Z

The release note should only have Hive Connector changes. Could you please move them? Also cc @steveburnett for a second round of the release note section review

@yingsu00 changed release note as per suggestion. @steveburnett Can you please verify

Yes, verified. Thank you!

yingsu00 · 2025-04-03T00:25:08Z

@bibith4 Could you please squash the 3 bullet points of the release notes into one? The second bullet point can be removed.

bibith4 · 2025-04-03T04:27:36Z

@bibith4 Could you please squash the 3 bullet points of the release notes into one? The second bullet point can be removed.

@yingsu00 Changed release note as per the suggestion. Can you please check

prestodb-ci added the from:IBM PR from IBM label Mar 18, 2025

bibith4 force-pushed the ssl_tls_support_for_hms branch 3 times, most recently from 93d3e50 to 237591e Compare March 18, 2025 15:20

bibith4 marked this pull request as ready for review March 19, 2025 06:25

bibith4 requested review from czentgr, a team, steveburnett and elharo as code owners March 19, 2025 06:25

bibith4 requested a review from jaystarshot March 19, 2025 06:25

prestodb-ci requested review from a team and jp-sivaprasad and removed request for a team March 19, 2025 06:25

steveburnett requested changes Mar 19, 2025

View reviewed changes

bibith4 force-pushed the ssl_tls_support_for_hms branch 4 times, most recently from 2e8df56 to cf2e4dc Compare March 20, 2025 09:58

steveburnett requested changes Mar 20, 2025

View reviewed changes

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

bibith4 requested a review from steveburnett March 20, 2025 14:08

bibith4 force-pushed the ssl_tls_support_for_hms branch from dd36298 to a62601e Compare March 20, 2025 14:10

steveburnett requested changes Mar 20, 2025

View reviewed changes

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

bibith4 force-pushed the ssl_tls_support_for_hms branch from 9ebeb22 to 577afe2 Compare March 20, 2025 15:36

steveburnett previously approved these changes Mar 20, 2025

View reviewed changes

bibith4 requested a review from agrawalreetika March 20, 2025 16:46

agrawalreetika requested changes Mar 23, 2025

View reviewed changes

bibith4 dismissed steveburnett’s stale review via 2bb324a March 24, 2025 16:28

bibith4 force-pushed the ssl_tls_support_for_hms branch from 2bb324a to 49127b4 Compare March 24, 2025 16:33

bibith4 requested a review from agrawalreetika March 24, 2025 16:34

agrawalreetika previously approved these changes Mar 26, 2025

View reviewed changes

bibith4 force-pushed the ssl_tls_support_for_hms branch 2 times, most recently from d92d49d to e794b25 Compare March 26, 2025 18:56

bibith4 requested a review from yingsu00 March 26, 2025 19:01

yingsu00 reviewed Mar 27, 2025

View reviewed changes

bibith4 dismissed agrawalreetika’s stale review via 3f8a560 March 27, 2025 06:19

yingsu00 reviewed Mar 28, 2025

View reviewed changes

bibith4 force-pushed the ssl_tls_support_for_hms branch 3 times, most recently from 64e2a10 to bcc1294 Compare April 1, 2025 07:41

bibith4 requested review from hantangwangd, ZacBlanco, vinothchandar and 7c00 as code owners April 1, 2025 13:28

Changes to enable ssl/tls in hms

693c872

Co-authored-by: Arin Mathew <[email protected]> Changes to move ssl related properties to seperate class

bibith4 force-pushed the ssl_tls_support_for_hms branch from 60d92c5 to 693c872 Compare April 1, 2025 13:31

steveburnett approved these changes Apr 1, 2025

View reviewed changes

yingsu00 reviewed Apr 2, 2025

View reviewed changes

agrawalreetika approved these changes Apr 2, 2025

View reviewed changes

bibith4 requested a review from yingsu00 April 3, 2025 16:36

yingsu00 approved these changes Apr 7, 2025

View reviewed changes

yingsu00 merged commit 832aefb into prestodb:master Apr 7, 2025
98 checks passed

	if (!metastoreTlsEnabled \|\| (!metastoreKeyStorePath.isPresent() && !metastoreTrustStorePath.isPresent())) {
	if (!metastoreTlsEnabled \|\| (metastoreKeyStorePath.isEmpty() && metastoreTrustStorePath.isEmpty()) {

	throw new RuntimeException("Unexpected default trust managers:" + Arrays.toString(trustManagers));
	throw new RuntimeException("Expected exactly one X509TrustManager, but found: " + Arrays.toString(trustManagers));
	;

Changes to enable ssl/tls in hms #24745

Changes to enable ssl/tls in hms #24745

Conversation

bibith4 commented Mar 18, 2025 • edited Loading

Description

Impact

Test Plan

Contributor checklist

Release Notes

linux-foundation-easycla bot commented Mar 18, 2025 • edited Loading

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawalreetika Mar 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawalreetika left a comment • edited Loading

Choose a reason for hiding this comment

bibith4 commented Mar 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bibith4 Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bibith4 Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

ethanyzhang commented Mar 29, 2025

bibith4 commented Apr 1, 2025

steveburnett commented Apr 1, 2025

steveburnett left a comment

Choose a reason for hiding this comment

yingsu00 left a comment

Choose a reason for hiding this comment

bibith4 commented Apr 2, 2025

agrawalreetika left a comment

Choose a reason for hiding this comment

steveburnett commented Apr 2, 2025

yingsu00 commented Apr 3, 2025

bibith4 commented Apr 3, 2025

bibith4 commented Mar 18, 2025 •

edited

Loading

linux-foundation-easycla bot commented Mar 18, 2025 •

edited

Loading

agrawalreetika Mar 23, 2025 •

edited

Loading

agrawalreetika left a comment •

edited

Loading

bibith4 Apr 1, 2025 •

edited

Loading

bibith4 Apr 1, 2025 •

edited

Loading