HMS Impersonation Access and breakdown metrics by hosts by BlueStalker · Pull Request #13699 · prestodb/presto

BlueStalker · 2019-11-14T01:48:12Z

Please make sure your submission complies with our Development, Formatting, and Commit Message guidelines.

Fill in the release notes towards the bottom of the PR description.
See Release Notes Guidelines for details.

== RELEASE NOTES ==
Hive Changes
* Impersonation Access by using HMS delegation token
* Enable multi HMS instances load balancing and breakdown metrics by HMS hosts.

linux-foundation-easycla · 2019-11-14T01:48:16Z

The committers are authorized under a signed CLA.

✅ Curt (590b8ea69f5bbd810db2c349b717899c724de91e, 9f2ce5b344b141e4bc9989eb7bbe1d2a7c1a0b47, 67e74b207e8f90bab6dcd0b1b46c1afb9bbfea76, 96a66f03b039757fa009dfef485bc9096f0a45ac, b189adacfc81cb4487b6d281b5948b7ce05eabe4, 5ed93ffb11ac069d710cbaa04a9c491e53c4afd5)

BlueStalker · 2019-11-14T21:30:35Z

ping @zhenxiao and @nezihyigitbasi
This is changes for the HMS impersonation, together with enabling multiple HMS instances with breakdown metrics by hms hosts.

BlueStalker · 2019-11-16T01:40:27Z

Interesting that that hive tests always report SocketTimeoutException (Read timeout) here, while it is successful in my local env, I am not very sure this related to the memory pressure of the integration test cause it brings up all the Hadoop related service.

Anyone has thoughts on this?

BlueStalker · 2019-11-18T21:37:19Z

I create another diff in docker images, trying to fix this timeout issues, details goes to
prestodb/docker-images#49

@nezihyigitbasi Do you know someone can also take a look at this code (changes are related with Hive/HMS).

rongrong · 2019-11-20T01:08:13Z

Before anyone starting to do code review, could you please reformat the changes into logical commits? For example, please squash the "fix test" changes with the commit that broke the test. Thanks!

BlueStalker · 2019-11-20T06:35:03Z

@rongrong updated, Thanks.

...c/main/java/com/facebook/presto/hive/authentication/KerberosHiveMetastoreAuthentication.java

zhenxiao

nice work, @BlueStalker
I went through it a first pass. I reviewed commit by commit. Some comments on the first commit are resolved by your following commits.
Mostly formatting:

do not use abbreviation for variable naming
try not use hms, use Metastore, or Hive Metastore

This feature is very useful. Looking forward to merge it!

...store/src/main/java/com/facebook/presto/hive/authentication/HiveMetastoreAuthentication.java

...tastore/src/main/java/com/facebook/presto/hive/metastore/SemiTransactionalHiveMetastore.java

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

presto-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/thrift/Transport.java

...c/main/java/com/facebook/presto/hive/authentication/KerberosHiveMetastoreAuthentication.java

...e-metastore/src/main/java/com/facebook/presto/hive/metastore/thrift/ThriftHiveMetastore.java

...ive-metastore/src/main/java/com/facebook/presto/hive/metastore/thrift/StaticHiveCluster.java

...metastore/src/main/java/com/facebook/presto/hive/metastore/thrift/StaticMetastoreConfig.java

BlueStalker · 2019-12-12T18:44:50Z

@zhenxiao @beinan Please take a look at this again, Thanks.

zhenxiao

Hi @BlueStalker mostly good. Just a few variable naming and formatting

pom.xml

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

...e-metastore/src/main/java/com/facebook/presto/hive/metastore/thrift/ThriftHiveMetastore.java

...store/src/main/java/com/facebook/presto/hive/metastore/thrift/ThriftHiveMetastoreClient.java

...ive-metastore/src/test/java/com/facebook/presto/hive/metastore/TestCachingHiveMetastore.java

presto-hive/src/main/java/com/facebook/presto/hive/HiveWriteUtils.java

BlueStalker · 2020-04-03T20:53:24Z

@zhenxiao Just got some time to look at your comments. Updated, and plz check it again

zhenxiao

nice work @BlueStalker
looks good to me
just one indentation issue

...e-metastore/src/main/java/com/facebook/presto/hive/metastore/thrift/ThriftHiveMetastore.java

BlueStalker · 2020-04-03T23:39:51Z

[test-facebook]

arhimondr

Could you please elaborate why hive metastore impersonation is needed?

The hive metastore is just a metadata storage. Enforcing security on the metadata level is generally a security anti pattern. The session agnostic design of the metastore communication is designed this way deliberately.

Presto has an extensive system of security check interfaces, see (ConnectorAccessControl, AccessControl) where all the required security checks can be implemented without directly relying on the Metastore.

Additionally the points of metastore communication are ill defined. The metadata can be cached in Presto. The metadata can be fetched lazily. And all this is a subject to change. This is another reason for using existing access controll mechanisms instead of trying to piggy back the security on the metastore itself.

P.S.:

Please format commit messages according to our standard that can be found in this PR description.

arhimondr · 2020-04-07T01:33:26Z

pom.xml

                <version>1.3.5-4</version>
            </dependency>

+            <dependency>


We don't use apache-commons in Presto. What is this library needed for?

arhimondr · 2020-04-07T01:34:15Z

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


 ``hive.metastore.client.keytab``                   Hive metastore client keytab location.
+``hive.metastore.impersonation.enabled``           Enable metastore end-user impersonation.
+``hive.metastore.impersonation.user``              Default impersonation user when communicating with Hive Metastore


Why this parameter needed?

This is because the getPartitionsStatistics calling from the initializer and need a "default" impersonation user to call, we can make this user as authenticated user in presto process, should be "presto"

arhimondr · 2020-04-07T01:35:16Z

presto-hive-hadoop2/bin/run_hive_tests.sh

 start_docker_containers

+# restart HMS to pickup memory settings
+exec_in_hadoop_master_container cp /etc/hadoop/conf/hive-env.sh /etc/hive/conf/hive-env.sh


This has to be embeded into the container instead of being copied every time.

arhimondr · 2020-04-07T01:35:35Z

presto-hive-hadoop2/conf/docker-compose.yml

      - ../../presto-hive/src/test/sql:/files/sql:ro
      - ./files/words:/usr/share/dict/words:ro
      - ./files/core-site.xml.s3-template:/etc/hadoop/conf/core-site.xml.s3-template:ro
+      - ./files/hive-env.sh:/etc/hadoop/conf/hive-env.sh:ro


arhimondr · 2020-04-07T01:35:48Z

presto-hive-hadoop2/conf/files/hive-env.sh

@@ -0,0 +1 @@
+export HADOOP_OPTS="$HADOOP_OPTS -Xmx1024m"


Please add this file to the docker image itself

arhimondr · 2020-04-07T01:35:56Z

presto-hive-hadoop2/conf/files/hive-env.sh

@@ -0,0 +1 @@
+export HADOOP_OPTS="$HADOOP_OPTS -Xmx1024m"


Also new line (required by Git)

arhimondr · 2020-04-07T01:36:18Z

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

    private String recordingPath;
    private boolean replay;
    private Duration recordingDuration = new Duration(0, MINUTES);
+    private String metastoreDefaultImpersonationUser = "";


We don't use empty strings as default values as the semantics are not clear

BlueStalker · 2020-04-07T03:35:58Z

Could you please elaborate why hive metastore impersonation is needed?

The hive metastore is just a metadata storage. Enforcing security on the metadata level is generally a security anti pattern. The session agnostic design of the metastore communication is designed this way deliberately.

Presto has an extensive system of security check interfaces, see (ConnectorAccessControl, AccessControl) where all the required security checks can be implemented without directly relying on the Metastore.

Additionally the points of metastore communication are ill defined. The metadata can be cached in Presto. The metadata can be fetched lazily. And all this is a subject to change. This is another reason for using existing access controll mechanisms instead of trying to piggy back the security on the metastore itself.

P.S.:

Please format commit messages according to our standard that can be found in this PR description.

Thanks for commenting on this. Hive metastore is a metadata system, not a storage system, we totally agree on that, but in the secure production system, impersonation required because we should not make a common user (usually it is "presto") to be able to do anything on the metastore. Actually, without this patch, I think people who turn on the metastore security must either turn off AuthZ, or do something to make "presto" to be able to do any metastore operations, then AuthN become meaningless to Hive metastore anymore.

I appreciate that Presto has its own mechanism to do AuthZ, but I think people don't want to fully replicate existing rules in metastore to presto, and it is probably a lot of work to define all the interface/rules for metastore.

For the caching stuff, I am not sure exactly what you are asking, if presto is doing the read operation it should be fine (because HDFS will ensure the authZ in the storage layer), when writing, which is probably the major reason of the patch, we can not cache anything here.

Let me know if you still have some concerns. Thanks.

arhimondr · 2020-04-07T04:16:16Z

I think people who turn on the metastore security must either turn off AuthZ

Is this a feature in the Hive Metastore? Could you please share some information about this feature?

but I think people don't want to fully replicate existing rules in metastore to presto, and it is probably a lot of work to define all the interface/rules for metastore.

That's something that we are doing at Facebook. We support column based and even row based security models. Additionally we support fine grained security model for most of the DDL and DML operations.

There's actually nothing wrong with calling the metastore from out of the ConnectorAccessControl interface. For example for checking if reading a table is allowed - you can simply call the getTable method of the Metastore from this method: https://github.com/prestodb/presto/blob/master/presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorAccessControl.java#L148. The ConnectorAccessControl provides the identity information in the ConnectorIdentity.

For the caching stuff, I am not sure exactly what you are asking

Currently Presto supports caching of the metadata. Either globally or on per transaction level. (https://github.com/prestodb/presto/blob/master/presto-hive-metastore/src/main/java/com/facebook/presto/hive/MetastoreClientConfig.java#L35, https://github.com/prestodb/presto/blob/master/presto-hive-metastore/src/main/java/com/facebook/presto/hive/MetastoreClientConfig.java#L38). Making the metastore communication to be session sensitive makes the caching either impossible or greatly inefficient.

And one more thing. Presto currently has the strongly defined exception system. Authorization checks failures triggered by one of the metastore calls should be classified as authorization checks failures and shouldn't cause Presto queries to fail with more generic, external https://github.com/prestodb/presto/blob/master/presto-hive-common/src/main/java/com/facebook/presto/hive/HiveErrorCode.java#L27. Thus the logic of handling authorization failure has to be implemented somewhere anyway. And it is better to have it in some explicit integration points then spread across the code base.

BlueStalker · 2020-04-07T21:28:55Z

I think people who turn on the metastore security must either turn off AuthZ

Is this a feature in the Hive Metastore? Could you please share some information about this feature?
https://docs.cloudera.com/documentation/enterprise/5-8-x/topics/cdh_sg_hive_metastore_security.html
Concept is very similar with HDFS impersonation.

but I think people don't want to fully replicate existing rules in metastore to presto, and it is probably a lot of work to define all the interface/rules for metastore.

That's something that we are doing at Facebook. We support column based and even row based security models. Additionally we support fine grained security model for most of the DDL and DML operations.

Things guarantee in presto itself is not same as in external system. Even you have full set of rules defined exactly same in Presto according to Metastore (apparently it is not), to make it work, you need to basically disable the authZ in metastore, because when these API goes to metastore, it only knows there is an authenticated user (pretty much is presto) to make the call to do any APIs.

Actually I think there is similar thing in prestosql. refer trinodb/trino#1441

There's actually nothing wrong with calling the metastore from out of the ConnectorAccessControl interface. For example for checking if reading a table is allowed - you can simply call the getTable method of the Metastore from this method: https://github.com/prestodb/presto/blob/master/presto-spi/src/main/java/com/facebook/presto/spi/connector/ConnectorAccessControl.java#L148. The ConnectorAccessControl provides the identity information in the ConnectorIdentity.

For the caching stuff, I am not sure exactly what you are asking

Currently Presto supports caching of the metadata. Either globally or on per transaction level. (https://github.com/prestodb/presto/blob/master/presto-hive-metastore/src/main/java/com/facebook/presto/hive/MetastoreClientConfig.java#L35, https://github.com/prestodb/presto/blob/master/presto-hive-metastore/src/main/java/com/facebook/presto/hive/MetastoreClientConfig.java#L38). Making the metastore communication to be session sensitive makes the caching either impossible or greatly inefficient.

From my quick grab of the code, this cache just save some time for (listtable, getable, getpartitions, etc) read, right? Then how this change affected the performance, it is just some overhead during the cache invalidations? Also, this implementation is following what hive does, and people can disable it by config.

And one more thing. Presto currently has the strongly defined exception system. Authorization checks failures triggered by one of the metastore calls should be classified as authorization checks failures and shouldn't cause Presto queries to fail with more generic, external https://github.com/prestodb/presto/blob/master/presto-hive-common/src/main/java/com/facebook/presto/hive/HiveErrorCode.java#L27. Thus the logic of handling authorization failure has to be implemented somewhere anyway. And it is better to have it in some explicit integration points then spread across the code base.

arhimondr · 2020-04-07T21:53:37Z

From my quick grab of the code, this cache just save some time for (listtable, getable, getpartitions, etc) read, right? Then how this change affected the performance, it is just some overhead during the cache invalidations? Also, this implementation is following what hive does, and people can disable it by config.

Let's take getTable as an example. Currently the getTable method takes two parameters, schema name and table name. The metadata for a table can be cached by a key of two parameters, schema and table: https://github.com/prestodb/presto/blob/master/presto-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/CachingHiveMetastore.java#L78. It is possible as the getTable result is not session specific. If it becomes identity sensitive the identity has to be included as part of the caching key. That's what is done in the prestosql PR: https://github.com/prestosql/presto/pull/1441/files#diff-af79b96af45afbf7e6273904305d3988R94. I cannot see any changes to the CachingHiveMetastore in this PR, what is a little strange. But the point is that if the metastore communication becomes session sensitive - it should either not be cached all together, or the caching mechanism should also be session aware.

https://docs.cloudera.com/documentation/enterprise/5-8-x/topics/cdh_sg_hive_metastore_security.html Concept is very similar with HDFS impersonation.

I see. Impersonating metastore access still seems a little weird to me. But since it is something that is widely being done then i think it is fine. I think we should support it.

Actually I think there is similar thing in prestosql. refer trinodb/trino#1441

What are the key differences between this PR and the PR in the prestosql? Did you consider cherry picking it?

BlueStalker · 2020-04-07T23:06:37Z

From my quick grab of the code, this cache just save some time for (listtable, getable, getpartitions, etc) read, right? Then how this change affected the performance, it is just some overhead during the cache invalidations? Also, this implementation is following what hive does, and people can disable it by config.

Let's take getTable as an example. Currently the getTable method takes two parameters, schema name and table name. The metadata for a table can be cached by a key of two parameters, schema and table: https://github.com/prestodb/presto/blob/master/presto-hive-metastore/src/main/java/com/facebook/presto/hive/metastore/CachingHiveMetastore.java#L78. It is possible as the getTable result is not session specific. If it becomes identity sensitive the identity has to be included as part of the caching key. That's what is done in the prestosql PR: https://github.com/prestosql/presto/pull/1441/files#diff-af79b96af45afbf7e6273904305d3988R94. I cannot see any changes to the CachingHiveMetastore in this PR, what is a little strange. But the point is that if the metastore communication becomes session sensitive - it should either not be cached all together, or the caching mechanism should also be session aware.
I see. To fully solve this, it could be annoying since the ACL rules could be complicated and to make identity in cache key will grow the memory and so on. Then this could be potentially a good solution for presto's ACL as you mentioned above. And meanwhile, even we are not using the identity as the cache key, from the security perspective it is a bit wired but not that bad, since people always have the storage layer of ACL (HDFS), that's actually how things are working today.
Back to the patch, it is really annoying that for the writing operations, without impersonation there is basically security leak.

https://docs.cloudera.com/documentation/enterprise/5-8-x/topics/cdh_sg_hive_metastore_security.html Concept is very similar with HDFS impersonation.

I see. Impersonating metastore access still seems a little weird to me. But since it is something that is widely being done then i think it is fine. I think we should support it.

Actually I think there is similar thing in prestosql. refer prestosql/presto#1441

What are the key differences between this PR and the PR in the prestosql? Did you consider cherry picking it?
I went thru some of the code, the underline transport implementation is similar (I think we are all from hive code), but others part are not, mainly because in this patch, it also contains the support for multiple HMS instance and stats(com.facebook.presto.hive.metastore.thrift.ThriftHiveMetastore), and a lot of refactoring there.

arhimondr · 2020-04-08T00:41:00Z

@BlueStalker Do you think it is possible to cherry pick the patch from presto-sql and apply additional changes on top of that? It will save us time on review round trips as the contribution standards for both projects are similar and also it will help us keep two Presto streams closer together.

BlueStalker · 2020-04-29T23:10:24Z

Actually, I put my comments in trinodb/trino#1441, I doubt whether the solution they choose actually works, right now, the solution we choose lives in our production for quite a long time, I think it doesn't make too much sense to cherry-pick their uncertain changes and made additional development on top of that to make this even risky.
Meanwhile, I'd like to hear more opinions about code sync between two repos, cause there is already a lot of difference, especially on hive/hms area.

arhimondr · 2020-04-30T19:53:38Z

I doubt whether the solution they choose actually works, right now, the solution we choose lives in our production for quite a long time, I think it doesn't make too much sense to cherry-pick their uncertain changes and made additional development on top of that to make this even risky.

That makes sense. Please reopen the pull request and have it prepared for another round of review. Please make sure your submission complies with our Development, Formatting, and Commit Message guidelines. The links can be found in this very PR description.

Meanwhile, I'd like to hear more opinions about code sync between two repos, cause there is already a lot of difference, especially on hive/hms area.

I don't think there's any specific plan for maintaining the code in sync. But generally it feels like a good idea to keep the codebases close to each other when possible, as it simplifies cherry picking between the projects.

This commits squash the original commits from PR prestodb#13699 which includes the follow commits: HMS impersonation access refactoring to use HMS Authentication Module add Config for multiple hms instances Update HMS memory settings address review comments

Summary: This commits squash the original commits from PR prestodb#13699 which includes the follow commits: HMS impersonation access refactoring to use HMS Authentication Module add Config for multiple hms instances Update HMS memory settings address review comments Make Hive metastore caching and impersonation mutually exclusive Update to not use TLS to save the identity info Refactor the implementation to explicitly carry over the authentication information, create a MetastoreContext object which includes the auth info coming from the ConnectorSession. Refactoring with code review comments Reviewers: #ldap_presto-core, chliang Reviewed By: #ldap_presto-core, chliang Subscribers: O4263 subscribe to presto changes Differential Revision: https://code.uberinternal.com/D5935797

BlueStalker force-pushed the curt branch from 8c3d5f4 to aedebe9 Compare November 14, 2019 02:37

zhenxiao self-requested a review November 14, 2019 22:34

zhenxiao self-assigned this Nov 14, 2019

BlueStalker force-pushed the curt branch 2 times, most recently from 9a5e5c8 to 35cba33 Compare November 15, 2019 23:47

BlueStalker force-pushed the curt branch 2 times, most recently from 8d6c8fa to 172864e Compare November 18, 2019 21:14

BlueStalker mentioned this pull request Nov 18, 2019

Memory settings for hive/hms prestodb/docker-images#49

Closed

BlueStalker force-pushed the curt branch from 0a297bd to 08a3d64 Compare November 20, 2019 06:33

beinan reviewed Dec 2, 2019

View reviewed changes

...c/main/java/com/facebook/presto/hive/authentication/KerberosHiveMetastoreAuthentication.java Outdated Show resolved Hide resolved

zhenxiao requested changes Dec 6, 2019

View reviewed changes

BlueStalker force-pushed the curt branch from f5f4a2f to 2593f82 Compare December 6, 2019 23:54

BlueStalker requested a review from zhenxiao December 9, 2019 19:19

zhenxiao reviewed Dec 20, 2019

View reviewed changes

BlueStalker force-pushed the curt branch from 2593f82 to 5602186 Compare January 8, 2020 00:19

BlueStalker closed this Jan 8, 2020

BlueStalker reopened this Jan 8, 2020

BlueStalker closed this Jan 8, 2020

BlueStalker reopened this Jan 8, 2020

BlueStalker closed this Jan 8, 2020

BlueStalker reopened this Jan 8, 2020

BlueStalker closed this Jan 9, 2020

Update HMS memory settings

1b246e3

BlueStalker force-pushed the curt branch from cd8d549 to 92dcc05 Compare April 3, 2020 20:52

zhenxiao approved these changes Apr 3, 2020

View reviewed changes

...e-metastore/src/main/java/com/facebook/presto/hive/metastore/thrift/ThriftHiveMetastore.java Outdated Show resolved Hide resolved

BlueStalker closed this Apr 6, 2020

BlueStalker reopened this Apr 6, 2020

address review comments

358572d

BlueStalker force-pushed the curt branch from 815ae43 to 358572d Compare April 6, 2020 20:41

zhenxiao merged commit 824fbc2 into prestodb:master Apr 6, 2020

caithagoras requested a review from arhimondr April 7, 2020 01:26

arhimondr reviewed Apr 7, 2020

View reviewed changes

arhimondr mentioned this pull request Apr 7, 2020

Revert #13699 #14356

Merged

BlueStalker mentioned this pull request May 1, 2020

Hive Meta Store impersonation access #14464

Closed

caithagoras mentioned this pull request May 4, 2020

Add release notes for 0.235 #14476

Merged

13 tasks

BlueStalker mentioned this pull request Feb 23, 2021

Hive Meta Store impersonation access #15736

Merged

		@@ -0,0 +1 @@
		export HADOOP_OPTS="$HADOOP_OPTS -Xmx1024m" No newline at end of file

Comments

Conversation

BlueStalker commented Nov 14, 2019 • edited by caithagoras Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BlueStalker commented Nov 14, 2019

Uh oh!

BlueStalker commented Nov 16, 2019

Uh oh!

BlueStalker commented Nov 18, 2019

Uh oh!

rongrong commented Nov 20, 2019

Uh oh!

BlueStalker commented Nov 20, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhenxiao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BlueStalker commented Dec 12, 2019

Uh oh!

zhenxiao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BlueStalker commented Apr 3, 2020

Uh oh!

zhenxiao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BlueStalker commented Apr 3, 2020

Uh oh!

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

BlueStalker commented Apr 7, 2020

BlueStalker commented Nov 14, 2019 •

edited by caithagoras

Loading

linux-foundation-easycla bot commented Nov 14, 2019 •

edited

Loading

arhimondr commented Apr 7, 2020 •

edited

Loading

arhimondr commented Apr 8, 2020 •

edited

Loading