Adding support for SSL/TLS in hive metastore

[ArinMathew]

Introduce SSL/TLS support for hive metastore

Cherry-pick of trinodb/trino/pull/3087

Co-authored-by: Dongwoo Son [email protected]

Test plan
Added unit test cases.

== RELEASE NOTES ==

Hive metastore changes

Added SSL/TLS support for hive metastore

[rohanpednekar]

@imjalpreet or @agrawalreetika Do you think you can do a first review of this PR?

[rschlussel]

I haven't looked at the implementation, but two comments

1. The commit message should follow our commit message guidelines, including using the imperative in the commit title ("Add XXX" rather than "Adding XXX") and giving proper attribution in the commit message body itself (not just the PR description) https://github.com/prestodb/presto/wiki/Review-and-Commit-guidelines#commit-formatting-and-pull-requests.
2. Is there a way to add a test to the code base that uses the metastore with TLS? Otherwise we can't validate that this works, and also won't know if it breaks.

[linux-foundation-easycla]

• ✅ login: ArinMathew (b514052, 36f071f)
• ❌ - login: @dongwoo1005 / name: Dongwoo Son . The commit (b514052) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

[skairali]

@rschlussel

1). We have corrected the comment format , please verify

2). On this point - this is a cherry picked code drop from trino with essential changes to fit to presto. We have tested this and its working fine. Similar SSL support is there in presto-cassandra and presto-elasticsearch. In all such modules we are not able to find end to end test cases. What we can do is to ensure that there are test cases to verify newly introduced methods such as metastoreSslContext, getTrustStore,validateKeyStoreCertificates are functionally intact.

And also want to mention that this change will be inactive or kind of no impact if we do not specify or configure hive.metastore.thrift.client.tls.enabled. By default this will false and none of these changes will be in active execution path.

[rschlussel]

I'm not concerned that this will break the hive connector with the feature disabled (that should be covered by current tests). Nor am I even very concerned that this feature doesn't work currently, as you said you have tried it out. I am most concerned that if we say we have support for this feature, we have tests that ensure that it continues to work. I hear you that cassandra and elastic search also don't have tests for their equivalent feature, but honestly, i think they should too.

If it's not easy to test with our regular query runner set up, perhaps the docker based product tests will be a good way to test this. I haven't used them in a while, but as I recall the README was pretty good https://github.com/prestodb/presto/blob/master/presto-product-tests/README.md. We have some tests there that test specific security configurations that could be helpful for inspiration (e.g. tls between presto nodes, LDAP). For example, see this PR, which adds LDAP support to the JDBC driver https://github.com/prestodb/presto/pull/7725/commits or these commits which tests TLS between Presto nodes 190f260 b8f8031. Or cacccb8 tests HDFS Impersonation.

[github-actions]

Codenotify: Notifying subscribers in CODENOTIFY files for diff 762a82f...36f071f.

Notify	File(s)
@steveburnett	presto-docs/src/main/sphinx/connector/hive.rst

[steveburnett]

Nit, checking if these are correct explanations for the properties.

[steveburnett]

Could you check these last four and confirm they are correct?

• hive.metastore.thrift.client.tls.truststore.path and hive.metastore.thrift.client.tls.truststore.password are described as for the key store
• hive.metastore.thrift.client.tls.keystore.path and hive.metastore.thrift.client.tls.keystore.password are described as for the trust store