Hive
  1. Hive
  2. HIVE-3705

Adding authorization capability to the metastore

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.10.0
    • Component/s: Authorization, Metastore
    • Labels:
      None

      Description

      In an environment where multiple clients access a single metastore, and we want to evolve hive security to a point where it's no longer simply preventing users from shooting their own foot, we need to be able to authorize metastore calls as well, instead of simply performing every metastore api call that's made.

      1. HIVE-3705.D6681.1.patch
        75 kB
        Phabricator
      2. HIVE-3705.D6681.2.patch
        77 kB
        Phabricator
      3. HIVE-3705.D6681.3.patch
        94 kB
        Phabricator
      4. HIVE-3705.D6681.4.patch
        100 kB
        Phabricator
      5. HIVE-3705.D6681.5.patch
        101 kB
        Phabricator
      6. HIVE-3705.giant.svn.patch
        112 kB
        Sushanth Sowmyan
      7. HIVE-3705.giant.svn-0.10.patch
        113 kB
        Sushanth Sowmyan
      8. hive-backend-auth.2.git.patch
        105 kB
        Sushanth Sowmyan
      9. hive-backend-auth.git.patch
        88 kB
        Sushanth Sowmyan
      10. hive-backend-auth.post-review.git.patch
        31 kB
        Sushanth Sowmyan
      11. hive-backend-auth.post-review-part2.git.patch
        0.9 kB
        Sushanth Sowmyan
      12. hive-backend-auth.post-review-part3.git.patch
        12 kB
        Sushanth Sowmyan
      13. hivesec_investigation.pdf
        391 kB
        Sushanth Sowmyan

        Issue Links

          Activity

          Hide
          Sushanth Sowmyan added a comment -

          Attaching the results of an investigation I carried out, to determine what areas exist for improvement/evolution of the hive security model.

          Show
          Sushanth Sowmyan added a comment - Attaching the results of an investigation I carried out, to determine what areas exist for improvement/evolution of the hive security model.
          Hide
          Sushanth Sowmyan added a comment -

          Phabricator review submitted : https://reviews.facebook.net/D6681

          HIVE-3705 Enabling authorization from the metastore:

          New HiveConf parameters:

          hive.security.metastore.authorization.enabled : true/false determining whether or not to do authorization in the metastore
          hive.security.metastore.authorization.manager : The class to load to do metastore-side authorization
          hive.security.metastore.authenticator.manager : The class to load to do metastore-side authentication

          If the first parameter isn't set, default behaviour of hive in both client-mode and metastore-mode is not affected, and this is disabled by default.

          New Interface :

          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java :
          an extension of HiveAuthorizationProvider, except with one more function that allows the metastore to pass a HMSHandler to it

          Modifications of existing classes :

          Minor modifications :
          ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java :
          added ability to instantiate HiveAuth

          {orization,entication}

          Providers given HiveConf key to use
          ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java :
          changed to account for above

          Major modifications :
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java :
          refactored to introduce a new HiveProxy that can proxy for either a hive object or a HMSHandler to perform necessary metadata operations
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java :
          refactored most of the functionality into a new class : BitSetCheckedAuthorizationProvider, which in turn is extended trivially by
          DefaultHiveAuthorizationProvider and DefaultHiveMetastoreAuthorizationProvider which implement small glue functionality to make them
          work from the hive client side and from the hive metastore respectively.

          New Classes :

          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java :
          As discussed above.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java :
          As discussed above.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java :
          An implementation of the Hive Metastore PreEventListener interface that kicks off the metastore-side authorization

          Show
          Sushanth Sowmyan added a comment - Phabricator review submitted : https://reviews.facebook.net/D6681 – HIVE-3705 Enabling authorization from the metastore: New HiveConf parameters: hive.security.metastore.authorization.enabled : true/false determining whether or not to do authorization in the metastore hive.security.metastore.authorization.manager : The class to load to do metastore-side authorization hive.security.metastore.authenticator.manager : The class to load to do metastore-side authentication If the first parameter isn't set, default behaviour of hive in both client-mode and metastore-mode is not affected, and this is disabled by default. New Interface : ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java : an extension of HiveAuthorizationProvider, except with one more function that allows the metastore to pass a HMSHandler to it Modifications of existing classes : Minor modifications : ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java : added ability to instantiate HiveAuth {orization,entication} Providers given HiveConf key to use ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java : changed to account for above Major modifications : ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java : refactored to introduce a new HiveProxy that can proxy for either a hive object or a HMSHandler to perform necessary metadata operations ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java : refactored most of the functionality into a new class : BitSetCheckedAuthorizationProvider, which in turn is extended trivially by DefaultHiveAuthorizationProvider and DefaultHiveMetastoreAuthorizationProvider which implement small glue functionality to make them work from the hive client side and from the hive metastore respectively. New Classes : ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java : As discussed above. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java : As discussed above. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java : An implementation of the Hive Metastore PreEventListener interface that kicks off the metastore-side authorization
          Hide
          Phabricator added a comment -

          khorgath requested code review of "HIVE-3705 [jira] Adding authorization capability to the metastore".
          Reviewers: JIRA

          HIVE-3705 Enabling authorization from the metastore:

          New HiveConf parameters:

          hive.security.metastore.authorization.enabled : true/false determining whether or not to do authorization in the metastore
          hive.security.metastore.authorization.manager : The class to load to do metastore-side authorization
          hive.security.metastore.authenticator.manager : The class to load to do metastore-side authentication

          If the first parameter isn't set, default behaviour of hive in both client-mode and metastore-mode is not affected, and this is disabled by default.

          New Interface :

          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java :
          an extension of HiveAuthorizationProvider, except with one more function that allows the metastore to pass a HMSHandler to it

          Modifications of existing classes :

          Minor modifications :
          ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java :
          added ability to instantiate HiveAuth

          {orization,entication}

          Providers given HiveConf key to use
          ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java :
          changed to account for above

          Major modifications :
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java :
          refactored to introduce a new HiveProxy that can proxy for either a hive object or a HMSHandler to perform necessary metadata operations
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java :
          refactored most of the functionality into a new class : BitSetCheckedAuthorizationProvider, which in turn is extended trivially by
          DefaultHiveAuthorizationProvider and DefaultHiveMetastoreAuthorizationProvider which implement small glue functionality to make them
          work from the hive client side and from the hive metastore respectively.

          New Classes :

          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java :
          As discussed above.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java :
          As discussed above.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java :
          An implementation of the Hive Metastore PreEventListener interface that kicks off the metastore-side authorization

          TEST PLAN
          Following testcases added :
          ql/src/test/org/apache/hadoop/hive/ql/security/DummyHiveMetastoreAuthorizationProvider.java
          ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java
          ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java

          In an environment where multiple clients access a single metastore, and we want to evolve hive security to a point where it's no longer simply preventing users from shooting their own foot, we need to be able to authorize metastore calls as well, instead of simply performing every metastore api call that's made.

          REVISION DETAIL
          https://reviews.facebook.net/D6681

          AFFECTED FILES
          common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
          conf/hive-default.xml.template
          ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
          ql/src/test/org/apache/hadoop/hive/ql/security/DummyHiveMetastoreAuthorizationProvider.java
          ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java
          ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java

          MANAGE HERALD DIFFERENTIAL RULES
          https://reviews.facebook.net/herald/view/differential/

          WHY DID I GET THIS EMAIL?
          https://reviews.facebook.net/herald/transcript/15915/

          To: JIRA, khorgath

          Show
          Phabricator added a comment - khorgath requested code review of " HIVE-3705 [jira] Adding authorization capability to the metastore". Reviewers: JIRA HIVE-3705 Enabling authorization from the metastore: New HiveConf parameters: hive.security.metastore.authorization.enabled : true/false determining whether or not to do authorization in the metastore hive.security.metastore.authorization.manager : The class to load to do metastore-side authorization hive.security.metastore.authenticator.manager : The class to load to do metastore-side authentication If the first parameter isn't set, default behaviour of hive in both client-mode and metastore-mode is not affected, and this is disabled by default. New Interface : ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java : an extension of HiveAuthorizationProvider, except with one more function that allows the metastore to pass a HMSHandler to it Modifications of existing classes : Minor modifications : ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java : added ability to instantiate HiveAuth {orization,entication} Providers given HiveConf key to use ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java : changed to account for above Major modifications : ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java : refactored to introduce a new HiveProxy that can proxy for either a hive object or a HMSHandler to perform necessary metadata operations ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java : refactored most of the functionality into a new class : BitSetCheckedAuthorizationProvider, which in turn is extended trivially by DefaultHiveAuthorizationProvider and DefaultHiveMetastoreAuthorizationProvider which implement small glue functionality to make them work from the hive client side and from the hive metastore respectively. New Classes : ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java : As discussed above. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java : As discussed above. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java : An implementation of the Hive Metastore PreEventListener interface that kicks off the metastore-side authorization TEST PLAN Following testcases added : ql/src/test/org/apache/hadoop/hive/ql/security/DummyHiveMetastoreAuthorizationProvider.java ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java In an environment where multiple clients access a single metastore, and we want to evolve hive security to a point where it's no longer simply preventing users from shooting their own foot, we need to be able to authorize metastore calls as well, instead of simply performing every metastore api call that's made. REVISION DETAIL https://reviews.facebook.net/D6681 AFFECTED FILES common/src/java/org/apache/hadoop/hive/conf/HiveConf.java conf/hive-default.xml.template ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java ql/src/test/org/apache/hadoop/hive/ql/security/DummyHiveMetastoreAuthorizationProvider.java ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java MANAGE HERALD DIFFERENTIAL RULES https://reviews.facebook.net/herald/view/differential/ WHY DID I GET THIS EMAIL? https://reviews.facebook.net/herald/transcript/15915/ To: JIRA, khorgath
          Hide
          Sushanth Sowmyan added a comment -

          (Patch available for review)

          Show
          Sushanth Sowmyan added a comment - (Patch available for review)
          Hide
          Phabricator added a comment -

          khorgath updated the revision "HIVE-3705 [jira] Adding authorization capability to the metastore".
          Reviewers: JIRA

          Updated to stop cacheing HMSHandler, which was a mistake, and adding setHandler on the metastore-side Authenticators.
          This introduces a new interface HiveMetastoreAuthenticationProvider which extends HiveAuthenticationProvider, and
          provides a default implementation of it as well.

          REVISION DETAIL
          https://reviews.facebook.net/D6681

          AFFECTED FILES
          common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
          conf/hive-default.xml.template
          ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java
          ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java
          ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
          ql/src/test/org/apache/hadoop/hive/ql/security/DummyHiveMetastoreAuthorizationProvider.java
          ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java
          ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java

          To: JIRA, khorgath

          Show
          Phabricator added a comment - khorgath updated the revision " HIVE-3705 [jira] Adding authorization capability to the metastore". Reviewers: JIRA Updated to stop cacheing HMSHandler, which was a mistake, and adding setHandler on the metastore-side Authenticators. This introduces a new interface HiveMetastoreAuthenticationProvider which extends HiveAuthenticationProvider, and provides a default implementation of it as well. REVISION DETAIL https://reviews.facebook.net/D6681 AFFECTED FILES common/src/java/org/apache/hadoop/hive/conf/HiveConf.java conf/hive-default.xml.template ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java ql/src/test/org/apache/hadoop/hive/ql/security/DummyHiveMetastoreAuthorizationProvider.java ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java To: JIRA, khorgath
          Hide
          Ashutosh Chauhan added a comment -

          Sushanth,
          Patch doesn't apply cleanly on trunk. Can you refresh it for trunk.

          Show
          Ashutosh Chauhan added a comment - Sushanth, Patch doesn't apply cleanly on trunk. Can you refresh it for trunk.
          Hide
          Shreepadma Venugopalan added a comment -

          @Sushanth: Thanks for posting the document and the patch. Securing the metastore is necessary to provide reliable authorization in Hive. I looked at the document and the code and have the following high level questions,

          a)The document contains an example of how the current pluggable authorization provider can be exploited to circumvent security. This patch seems to introduce a new config param - hive.security.metastore.authorization.manager - that allows a pluggable authorization provider. Perhaps I'm missing something here, but wondering how we would prevent a user from plugging in their own authorization provider.

          b)The current Hive authorization model exposes semantics that is confusing and at times inconsistent. While this patch has moved the auth checks to the metastore (IMO, this is the right thing to do) it seems to implement the existing semantics. Wondering if there is a plan to fix the semantics at some point.

          c)How do we obtain the userid for performing authorization? Are we using the authentication id from the Thrift context? If so, how do we handle the case where the authentication id is different from the authorization id, for e.g., HS2 authenticates to the metastore as HS2 but is executing a statement on behalf of user 'u1'? Thanks.

          Show
          Shreepadma Venugopalan added a comment - @Sushanth: Thanks for posting the document and the patch. Securing the metastore is necessary to provide reliable authorization in Hive. I looked at the document and the code and have the following high level questions, a)The document contains an example of how the current pluggable authorization provider can be exploited to circumvent security. This patch seems to introduce a new config param - hive.security.metastore.authorization.manager - that allows a pluggable authorization provider. Perhaps I'm missing something here, but wondering how we would prevent a user from plugging in their own authorization provider. b)The current Hive authorization model exposes semantics that is confusing and at times inconsistent. While this patch has moved the auth checks to the metastore (IMO, this is the right thing to do) it seems to implement the existing semantics. Wondering if there is a plan to fix the semantics at some point. c)How do we obtain the userid for performing authorization? Are we using the authentication id from the Thrift context? If so, how do we handle the case where the authentication id is different from the authorization id, for e.g., HS2 authenticates to the metastore as HS2 but is executing a statement on behalf of user 'u1'? Thanks.
          Hide
          Sushanth Sowmyan added a comment -

          @Ashutosh : Funny - I've uploaded a lone patch as well now, just in case something was wrong with the autogeneration. It's called hive-backend-auth.git.patch and can be applied with a patch -p1

          @Shreepadma : Thanks for your comments!

          a) The idea is that the box (or vm/etc) on which the metastore server runs can be secured and given limited login privileges, and kept away from most of the hive users. That way, the "secrets" such as the hive-site.xml that contains the mysql password, and the hive-site.xml config that sets which AuthorizationProvider to use from the metastore is under strict control of the admin.

          b) I agree that the current model's semantics can sometimes be confusing, especially when you consider separate grant/revoke based permissions and HDFS permissions, and indeed, the authorization provider I intend to use with hive that builds on top of this will be a HDFS-permissions based Authorization Provider, but the hooks for how to launch the Authorization(&Authentication) Providers and what objects they authorize on are good interfaces, I think, and I've merely extended them in where/how they can be invoked in this patch.

          I think there's a lot of work yet to be done, in cleaning up the privilege definitions in HiveOperation, and making sure that the various authorization calls are actually called. Take, for example the notion that "drop database" does not make any auth calls! So yes, there is more work to be done on this end. I'll be glad to put up a list of other items I felt needed to be changed/fixed.

          c) The userid for performing authorization is obtained from the provided authenticator. I have an implementation of a default MetastoreAuthenticator with this patch which tries to obtain the userid from the ugi that the HMSHandler has access to. I use this because it makes the most sense as it is as that user that HMSHandler uses to create wh objects, and to perform filesystem calls, for example. I'm afraid I don't know the thrift-side plumbing too well to know where it gets that setting from. As for how it interacts with HS2, again, I don't know how HS2 itself works, but if HS2 is performing a task on behalf of a user, then it would call create_table_core, for eg., as that user.

          Additional Authentication providers can also be written and plugged in to this scheme - I merely describe how the one I wrote and use works.

          Show
          Sushanth Sowmyan added a comment - @Ashutosh : Funny - I've uploaded a lone patch as well now, just in case something was wrong with the autogeneration. It's called hive-backend-auth.git.patch and can be applied with a patch -p1 @Shreepadma : Thanks for your comments! a) The idea is that the box (or vm/etc) on which the metastore server runs can be secured and given limited login privileges, and kept away from most of the hive users. That way, the "secrets" such as the hive-site.xml that contains the mysql password, and the hive-site.xml config that sets which AuthorizationProvider to use from the metastore is under strict control of the admin. b) I agree that the current model's semantics can sometimes be confusing, especially when you consider separate grant/revoke based permissions and HDFS permissions, and indeed, the authorization provider I intend to use with hive that builds on top of this will be a HDFS-permissions based Authorization Provider, but the hooks for how to launch the Authorization(&Authentication) Providers and what objects they authorize on are good interfaces, I think, and I've merely extended them in where/how they can be invoked in this patch. I think there's a lot of work yet to be done, in cleaning up the privilege definitions in HiveOperation, and making sure that the various authorization calls are actually called. Take, for example the notion that "drop database" does not make any auth calls! So yes, there is more work to be done on this end. I'll be glad to put up a list of other items I felt needed to be changed/fixed. c) The userid for performing authorization is obtained from the provided authenticator. I have an implementation of a default MetastoreAuthenticator with this patch which tries to obtain the userid from the ugi that the HMSHandler has access to. I use this because it makes the most sense as it is as that user that HMSHandler uses to create wh objects, and to perform filesystem calls, for example. I'm afraid I don't know the thrift-side plumbing too well to know where it gets that setting from. As for how it interacts with HS2, again, I don't know how HS2 itself works, but if HS2 is performing a task on behalf of a user, then it would call create_table_core, for eg., as that user. Additional Authentication providers can also be written and plugged in to this scheme - I merely describe how the one I wrote and use works.
          Hide
          Rob Weltman added a comment -

          A new JIRA has been opened for the larger issues around the desired semantics of Hive authorization and ensuring they are enforced:

          https://issues.apache.org/jira/browse/HIVE-3720

          Show
          Rob Weltman added a comment - A new JIRA has been opened for the larger issues around the desired semantics of Hive authorization and ensuring they are enforced: https://issues.apache.org/jira/browse/HIVE-3720
          Hide
          Sushanth Sowmyan added a comment -

          @Shreepadma :

          I've now updated the diff with a reference implementation of a StorageBasedAuthorizationProvider, which is a port of HCatalog's HdfsAuthorizationProvider.

          Show
          Sushanth Sowmyan added a comment - @Shreepadma : I've now updated the diff with a reference implementation of a StorageBasedAuthorizationProvider, which is a port of HCatalog's HdfsAuthorizationProvider.
          Hide
          Phabricator added a comment -

          khorgath updated the revision "HIVE-3705 [jira] Adding authorization capability to the metastore".
          Reviewers: JIRA

          Passing on HMSHandler to MetastoreAuthenticator as well, added in reference implementation of a StorageBasedAuthorizationProvider
          as an example of implementable metastore-side AuthorizationProviders

          REVISION DETAIL
          https://reviews.facebook.net/D6681

          AFFECTED FILES
          common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
          conf/hive-default.xml.template
          metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
          ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java
          ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultAuthenticator.java
          ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java
          ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
          ql/src/test/org/apache/hadoop/hive/ql/security/DummyHiveMetastoreAuthorizationProvider.java
          ql/src/test/org/apache/hadoop/hive/ql/security/InjectableDummyAuthenticator.java
          ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java
          ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java

          To: JIRA, khorgath

          Show
          Phabricator added a comment - khorgath updated the revision " HIVE-3705 [jira] Adding authorization capability to the metastore". Reviewers: JIRA Passing on HMSHandler to MetastoreAuthenticator as well, added in reference implementation of a StorageBasedAuthorizationProvider as an example of implementable metastore-side AuthorizationProviders REVISION DETAIL https://reviews.facebook.net/D6681 AFFECTED FILES common/src/java/org/apache/hadoop/hive/conf/HiveConf.java conf/hive-default.xml.template metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultAuthenticator.java ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java ql/src/test/org/apache/hadoop/hive/ql/security/DummyHiveMetastoreAuthorizationProvider.java ql/src/test/org/apache/hadoop/hive/ql/security/InjectableDummyAuthenticator.java ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java To: JIRA, khorgath
          Hide
          Sushanth Sowmyan added a comment -

          @Ashutosh : And in case there are any issues with merging against trunk, I'm attaching a monolithic git patch as well.

          Show
          Sushanth Sowmyan added a comment - @Ashutosh : And in case there are any issues with merging against trunk, I'm attaching a monolithic git patch as well.
          Hide
          Phabricator added a comment -

          ashutoshc has requested changes to the revision "HIVE-3705 [jira] Adding authorization capability to the metastore".

          Please see inline comments.

          INLINE COMMENTS
          conf/hive-default.xml.template:1254 I think it can be better worded as ...
          The hive metastore authorization..
          conf/hive-default.xml.template:1269 same as previous comment.
          conf/hive-default.xml.template:1269 This will read better as "The authorization manager class name..."
          conf/hive-default.xml.template:1284 Same as above.
          ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java:27 It seems that you have created new interface HiveMetaStoreAuthenticationProvider and added method setHandler() just to setConf(). If this is the only reason, then there is no need of this new interface, since HiveAuthenticationProvider already extends Configurable and at instantiation time of this interface in HiveUtils, you have access to conf which you can set. So, this interface and this new implementation of it seems overkill.
          ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java:25 Need to update this javadoc.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:62 Is there any reason for creating new instance of HiveConf? You can just save passed-in config.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:64 Ideally, this bool should not be checked in here. If pre-event listener is already set for this listener class, we can safely assume that he intends this listener to fire and checks to happen.

          Infact, I don't see point of this boolean in HiveConf at all. Since, these checks are meant to be invoked via listener interface, if user has to consciously put these class names in the config. Why then he has to also set that boolean to be true?

          More configs usually results in more confusion.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:125 This will break chaining of exception stack. I think it will be better done as:
          InvalidOperationException ex = new InvalidOperationException(e.getMessage());
          ex.initCause(e.getCause());
          throw ex;

          Same comment applies for all other try-catch blocks.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:112 For all other operations (including grant, revoke etc.) no checks are performed and are allowed straight through. I think you should add a note in javadoc of this listener class that it only performs checks for create/add/alter/drop of db/tbl/partitions.

          I am not sure if following deployment is supported: This listener configured with DefaultHiveAuthProvider which does auth based on privs stored in metastore? If it is, then it has same problem which that provider has. User can grant himself all privs, no checks are done for that and then drop tables/dbs. I understand you are not improving semantics in this patch, but merely shifting checks on metastore from client, but just wanted to make sure my understanding is correct.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:232 This method should be private.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:250 This method should be private.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:256 Why is this needed ? In particular, this will set location of partition to table's location in null-case. Is that desirable?
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java:30 You can just cast (HiveConf)conf, instead of doing new HiveConf()?
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:49 This is also defined in HiveMetaStore. This should be statically defined in HiveConf and should be referenced from there, instead of private copy in each class.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:165 I am not sure about this, but is this about creating index, in which case Write makes sense or is this about reading indexing, in which case read should suffice ?
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:168 Same as above. Locks could be a shared lock or exclusive lock, resulting in equivalent of read and write privs?
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:301 This method already exists in HiveMetaStore ? Either we make that public or put that in some utils class, so its useful at both places.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:308 This method looks same as Warehouse::getDatabasePath(), we should reuse that.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:95 These two checks should be performed on table.getPath()
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:77 If db == null here that sounds like a bug upstream. Masking that bug in authorize() doesnt only mask that bug, it weakens this check as well.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:89 Same comment as above for db == null.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:117 This method should be private.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:111 Same comment as above for null check.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:122 In what case part or its location will be null, such that making that check instead on table makes sense?
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:218 For better exception chaining, please do initCause() for all exceptions before throwing them.
          ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java:44 Instead of hard-coding port number, use the standard trick to find free port on the system.
          ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java:137 It will make sense to check response string which user gets here, it should be Access denied or some such. Can you add check for that as well.

          Checking for responseCode is good, but it will be better to check for exception type that is expected.

          If not possible via Driver, we should construct test in some other fashion, which makes that plausible, perhaps via HiveMetaStoreClient?

          REVISION DETAIL
          https://reviews.facebook.net/D6681

          BRANCH
          HIVE-3705

          To: JIRA, ashutoshc, khorgath

          Show
          Phabricator added a comment - ashutoshc has requested changes to the revision " HIVE-3705 [jira] Adding authorization capability to the metastore". Please see inline comments. INLINE COMMENTS conf/hive-default.xml.template:1254 I think it can be better worded as ... The hive metastore authorization.. conf/hive-default.xml.template:1269 same as previous comment. conf/hive-default.xml.template:1269 This will read better as "The authorization manager class name..." conf/hive-default.xml.template:1284 Same as above. ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java:27 It seems that you have created new interface HiveMetaStoreAuthenticationProvider and added method setHandler() just to setConf(). If this is the only reason, then there is no need of this new interface, since HiveAuthenticationProvider already extends Configurable and at instantiation time of this interface in HiveUtils, you have access to conf which you can set. So, this interface and this new implementation of it seems overkill. ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java:25 Need to update this javadoc. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:62 Is there any reason for creating new instance of HiveConf? You can just save passed-in config. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:64 Ideally, this bool should not be checked in here. If pre-event listener is already set for this listener class, we can safely assume that he intends this listener to fire and checks to happen. Infact, I don't see point of this boolean in HiveConf at all. Since, these checks are meant to be invoked via listener interface, if user has to consciously put these class names in the config. Why then he has to also set that boolean to be true? More configs usually results in more confusion. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:125 This will break chaining of exception stack. I think it will be better done as: InvalidOperationException ex = new InvalidOperationException(e.getMessage()); ex.initCause(e.getCause()); throw ex; Same comment applies for all other try-catch blocks. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:112 For all other operations (including grant, revoke etc.) no checks are performed and are allowed straight through. I think you should add a note in javadoc of this listener class that it only performs checks for create/add/alter/drop of db/tbl/partitions. I am not sure if following deployment is supported: This listener configured with DefaultHiveAuthProvider which does auth based on privs stored in metastore? If it is, then it has same problem which that provider has. User can grant himself all privs, no checks are done for that and then drop tables/dbs. I understand you are not improving semantics in this patch, but merely shifting checks on metastore from client, but just wanted to make sure my understanding is correct. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:232 This method should be private. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:250 This method should be private. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:256 Why is this needed ? In particular, this will set location of partition to table's location in null-case. Is that desirable? ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java:30 You can just cast (HiveConf)conf, instead of doing new HiveConf()? ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:49 This is also defined in HiveMetaStore. This should be statically defined in HiveConf and should be referenced from there, instead of private copy in each class. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:165 I am not sure about this, but is this about creating index, in which case Write makes sense or is this about reading indexing, in which case read should suffice ? ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:168 Same as above. Locks could be a shared lock or exclusive lock, resulting in equivalent of read and write privs? ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:301 This method already exists in HiveMetaStore ? Either we make that public or put that in some utils class, so its useful at both places. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:308 This method looks same as Warehouse::getDatabasePath(), we should reuse that. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:95 These two checks should be performed on table.getPath() ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:77 If db == null here that sounds like a bug upstream. Masking that bug in authorize() doesnt only mask that bug, it weakens this check as well. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:89 Same comment as above for db == null. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:117 This method should be private. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:111 Same comment as above for null check. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:122 In what case part or its location will be null, such that making that check instead on table makes sense? ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:218 For better exception chaining, please do initCause() for all exceptions before throwing them. ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java:44 Instead of hard-coding port number, use the standard trick to find free port on the system. ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java:137 It will make sense to check response string which user gets here, it should be Access denied or some such. Can you add check for that as well. Checking for responseCode is good, but it will be better to check for exception type that is expected. If not possible via Driver, we should construct test in some other fashion, which makes that plausible, perhaps via HiveMetaStoreClient? REVISION DETAIL https://reviews.facebook.net/D6681 BRANCH HIVE-3705 To: JIRA, ashutoshc, khorgath
          Hide
          Phabricator added a comment -

          khorgath has commented on the revision "HIVE-3705 [jira] Adding authorization capability to the metastore".

          Replied to a few of the review comments.

          INLINE COMMENTS
          conf/hive-default.xml.template:1269 Agreed, changing.
          conf/hive-default.xml.template:1284 Agreed, changing
          ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java:27 Not true. The handler that is set is used to do things like fetch dbs, fetch privilege bitmaps, etc for the Default authorizer, and in general, as an interface point, it is an important bit of functionality to extend to the authorization managers that run on the metastore-side to check metadata. Without this, a metastore-side authorization provider will wind up making much more costly calls (instantiating a hive object/etc) to do any metadata fetches.
          ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java:25 Agreed, done.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:62 It was because we want some HiveConf parameters from a HiveConf object, but the PreEventListener interface is set up to be instantiated by a Configuration object as opposed to a HiveConf object. In implementation, it is instantiated with a HiveConf which is-a Configuration, and thus, we can get away with it, but this is a case of relying on implementation as opposed to interface. That said, this is easily fixed by HiveConf.getVar used by MetastoreUtils, so I was avoiding a nonexistant problem. Fixed.

          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:64 I did not want to leave metastore-auth to a side-effect of configuring the AuthorizationPreListener, and wanted to have the metastore-side auth config to mimic client-side auth config, but I see the benefit in what you're suggesting. I'll change it.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:112 The listener can only act on the types of events that a listener is triggered for. We'd have to follow it up by changing so that grant/revokes also cause events. But yes, will change javadoc to reflect. And yes, your understanding is correct, a user can still use a DefaultHiveMetastoreAuthProvider and grant privileges to themselves with this patch. We'd have to change that further as we improve it.

          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:125 Good point, changing.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:232 Agreed, was a result of an eclipse refactor default, changed.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:250 Agreed, was a result of an eclipse refactor default, changed.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:256 This is needed because leaving it as null(as the case will be with Partition objects yet to be created) will cause a NullPointerException when trying to construct a ql.metadata.Partition, which assumes a .getSd() will not return null. Given that we then need to pick a default, the table's .getSd() is a useful default for when the Partition object has not yet been populated. This way, authorizaton providers can check the table directory for permissions if they need to, rather than being faced with a null, which is not useful. Also, the only place this is used is for Authorization, so it does not affect the rest of the codeflow.

          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java:30 This would be a bad idea because you're being passed a Configuration object. I understand that in practice you'll likely get a HiveConf object, but this type of runtime casting is asking for trouble later on when someone instantiates it with a Configuration object.

          We can do runtime checks to see if we got passed a HiveConf object, in which case we retain it, or if we got a Configuration object, in which case we create a new HiveConf, but that leads to kludgy code.

          If we want to prevent instantiating a HiveConf object, then either Hive.get needs to change to accept a Configuration, or HiveAuthorizationProvider.init interface needs to change to require a HiveConf. Also, this is not new code, this is taken verbatim from HiveAuthorizationProviderBase that already did this.

          REVISION DETAIL
          https://reviews.facebook.net/D6681

          BRANCH
          HIVE-3705

          To: JIRA, ashutoshc, khorgath

          Show
          Phabricator added a comment - khorgath has commented on the revision " HIVE-3705 [jira] Adding authorization capability to the metastore". Replied to a few of the review comments. INLINE COMMENTS conf/hive-default.xml.template:1269 Agreed, changing. conf/hive-default.xml.template:1284 Agreed, changing ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java:27 Not true. The handler that is set is used to do things like fetch dbs, fetch privilege bitmaps, etc for the Default authorizer, and in general, as an interface point, it is an important bit of functionality to extend to the authorization managers that run on the metastore-side to check metadata. Without this, a metastore-side authorization provider will wind up making much more costly calls (instantiating a hive object/etc) to do any metadata fetches. ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java:25 Agreed, done. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:62 It was because we want some HiveConf parameters from a HiveConf object, but the PreEventListener interface is set up to be instantiated by a Configuration object as opposed to a HiveConf object. In implementation, it is instantiated with a HiveConf which is-a Configuration, and thus, we can get away with it, but this is a case of relying on implementation as opposed to interface. That said, this is easily fixed by HiveConf.getVar used by MetastoreUtils, so I was avoiding a nonexistant problem. Fixed. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:64 I did not want to leave metastore-auth to a side-effect of configuring the AuthorizationPreListener, and wanted to have the metastore-side auth config to mimic client-side auth config, but I see the benefit in what you're suggesting. I'll change it. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:112 The listener can only act on the types of events that a listener is triggered for. We'd have to follow it up by changing so that grant/revokes also cause events. But yes, will change javadoc to reflect. And yes, your understanding is correct, a user can still use a DefaultHiveMetastoreAuthProvider and grant privileges to themselves with this patch. We'd have to change that further as we improve it. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:125 Good point, changing. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:232 Agreed, was a result of an eclipse refactor default, changed. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:250 Agreed, was a result of an eclipse refactor default, changed. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:256 This is needed because leaving it as null(as the case will be with Partition objects yet to be created) will cause a NullPointerException when trying to construct a ql.metadata.Partition, which assumes a .getSd() will not return null. Given that we then need to pick a default, the table's .getSd() is a useful default for when the Partition object has not yet been populated. This way, authorizaton providers can check the table directory for permissions if they need to, rather than being faced with a null, which is not useful. Also, the only place this is used is for Authorization, so it does not affect the rest of the codeflow. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java:30 This would be a bad idea because you're being passed a Configuration object. I understand that in practice you'll likely get a HiveConf object, but this type of runtime casting is asking for trouble later on when someone instantiates it with a Configuration object. We can do runtime checks to see if we got passed a HiveConf object, in which case we retain it, or if we got a Configuration object, in which case we create a new HiveConf, but that leads to kludgy code. If we want to prevent instantiating a HiveConf object, then either Hive.get needs to change to accept a Configuration, or HiveAuthorizationProvider.init interface needs to change to require a HiveConf. Also, this is not new code, this is taken verbatim from HiveAuthorizationProviderBase that already did this. REVISION DETAIL https://reviews.facebook.net/D6681 BRANCH HIVE-3705 To: JIRA, ashutoshc, khorgath
          Hide
          Phabricator added a comment -

          khorgath has commented on the revision "HIVE-3705 [jira] Adding authorization capability to the metastore".

          INLINE COMMENTS
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:49 Good point. Thought about it a bit, and decided that the best place for this constant was in MetaStoreUtils along with some other similar constants there. Have refactored to push it there.

          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:77 Agreed. Removing. I was mimicing existing code in HCatalog's HDFS Auth Provider, but you're right, we need to be stricter.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:89 Agreed, same response.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:111 Agreed, same response.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:95 getPath is equivalent to a path constructed on table.getTTable().getSd().getLocation() only if it is nonEmpty, which it is in the else segment. Have made it clearer by avoiding referencing .getPath altogether.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:117 Done.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:122 There isn't a case with partition being null, removing that bit as with 77,89 and 111 above. But it is possible for the location to be null as with the case of creating a partition - when the PreEventListener is triggered, it's possible for the part location to be null, in which case the correct behaviour is to check the table's permissions.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:165 Interesting question - I assumed it was for creating an Index. That said, this is currently unused in Hive - there's no reference to this that I find in the codebase.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:168 Ditto as with 165, but with the exception that HiveOperation does define them as read privileges for both LOCKTABLE and UNLOCKTABLE. That doesn't sound terribly right to me, as I don't think read privileges are enough to be able to perform either of these operations. I'm going to leave this as-is, and ditto with the INDEX case above unless you think we should change it. At any rate, these are not privileges currently in use, even if LOCK is partially defined.

          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:218 Agreed, changing across all files in this patch.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:301 I was trying to keep changes minimal and not change HiveMetaStore too much, but yes, okay, refactoring, moving over to Warehouse
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:308 This does not reimplement Warehouse::getDatabasePath, it extends it(it calls it) by providing a default path if the location was null.

          ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java:44 Agreed, done across tests.
          ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java:137 Added as requested.

          REVISION DETAIL
          https://reviews.facebook.net/D6681

          BRANCH
          HIVE-3705

          To: JIRA, ashutoshc, khorgath

          Show
          Phabricator added a comment - khorgath has commented on the revision " HIVE-3705 [jira] Adding authorization capability to the metastore". INLINE COMMENTS ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:49 Good point. Thought about it a bit, and decided that the best place for this constant was in MetaStoreUtils along with some other similar constants there. Have refactored to push it there. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:77 Agreed. Removing. I was mimicing existing code in HCatalog's HDFS Auth Provider, but you're right, we need to be stricter. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:89 Agreed, same response. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:111 Agreed, same response. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:95 getPath is equivalent to a path constructed on table.getTTable().getSd().getLocation() only if it is nonEmpty, which it is in the else segment. Have made it clearer by avoiding referencing .getPath altogether. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:117 Done. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:122 There isn't a case with partition being null, removing that bit as with 77,89 and 111 above. But it is possible for the location to be null as with the case of creating a partition - when the PreEventListener is triggered, it's possible for the part location to be null, in which case the correct behaviour is to check the table's permissions. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:165 Interesting question - I assumed it was for creating an Index. That said, this is currently unused in Hive - there's no reference to this that I find in the codebase. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:168 Ditto as with 165, but with the exception that HiveOperation does define them as read privileges for both LOCKTABLE and UNLOCKTABLE. That doesn't sound terribly right to me, as I don't think read privileges are enough to be able to perform either of these operations. I'm going to leave this as-is, and ditto with the INDEX case above unless you think we should change it. At any rate, these are not privileges currently in use, even if LOCK is partially defined. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:218 Agreed, changing across all files in this patch. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:301 I was trying to keep changes minimal and not change HiveMetaStore too much, but yes, okay, refactoring, moving over to Warehouse ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:308 This does not reimplement Warehouse::getDatabasePath, it extends it(it calls it) by providing a default path if the location was null. ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java:44 Agreed, done across tests. ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java:137 Added as requested. REVISION DETAIL https://reviews.facebook.net/D6681 BRANCH HIVE-3705 To: JIRA, ashutoshc, khorgath
          Hide
          Phabricator added a comment -

          khorgath updated the revision "HIVE-3705 [jira] Adding authorization capability to the metastore".
          Reviewers: JIRA, ashutoshc

          Updates per review comments

          REVISION DETAIL
          https://reviews.facebook.net/D6681

          AFFECTED FILES
          common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
          conf/hive-default.xml.template
          metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
          metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java
          metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java
          ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java
          ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultAuthenticator.java
          ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java
          ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
          ql/src/test/org/apache/hadoop/hive/ql/security/DummyHiveMetastoreAuthorizationProvider.java
          ql/src/test/org/apache/hadoop/hive/ql/security/InjectableDummyAuthenticator.java
          ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java
          ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java

          To: JIRA, ashutoshc, khorgath

          Show
          Phabricator added a comment - khorgath updated the revision " HIVE-3705 [jira] Adding authorization capability to the metastore". Reviewers: JIRA, ashutoshc Updates per review comments REVISION DETAIL https://reviews.facebook.net/D6681 AFFECTED FILES common/src/java/org/apache/hadoop/hive/conf/HiveConf.java conf/hive-default.xml.template metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultAuthenticator.java ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java ql/src/test/org/apache/hadoop/hive/ql/security/DummyHiveMetastoreAuthorizationProvider.java ql/src/test/org/apache/hadoop/hive/ql/security/InjectableDummyAuthenticator.java ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java To: JIRA, ashutoshc, khorgath
          Hide
          Phabricator added a comment -

          ashutoshc has requested changes to the revision "HIVE-3705 [jira] Adding authorization capability to the metastore".

          Patch is getting close. I added few more comments, mostly around documenting semantics used in the patch.

          INLINE COMMENTS
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:256 This warrants a java-doc comment that in case of partition for which location is not available yet, table's location is used. This will be the case when some one does alter table add partition.
          Actually, this will be true even in case of create table, where dir will get created only after the authorization has happened, so will it check permissions of parent ?

          I think it will be good to add java-doc of all the scenarios, which dir will be used to base authentication. e.g., in case if user does create table and provides location and dir already exists, then that dir is used, otherwise parent dir.

          Semantics of which dir is used in which use-case need to be called out for explicitly. I assume your semantics model is based on http://wiki.apache.org/pig/Howl/HowlAuthorizationProposal and http://wiki.apache.org/pig/Howl/AuthorizationImplNotes

          If so, please document various pieces of code of this patch with all this information.

          ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java:27 You don't store the handler, just use its conf. So, I don't see how this interface and its impl is useful as it stands today.
          On the other hand, I agree that, this is an important interface and is good to have this interface here because there could be alternative impl possible here. So, lets have the interface.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:122 As I noted above, these kind of semantic decision need to be documented in the code.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:168 If they are not used currently than instead of defining semantics now, we should let it fail in default branch and have the exception thrown, such that when someone later does add support for these, we can formalize the semantics.
          ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java:44 In the trunk, this is done via MetaStoreUtils::startMetaStore() . See, https://issues.apache.org/jira/browse/HIVE-3724 for recommended usage pattern for it.

          REVISION DETAIL
          https://reviews.facebook.net/D6681

          BRANCH
          HIVE-3705

          To: JIRA, ashutoshc, khorgath

          Show
          Phabricator added a comment - ashutoshc has requested changes to the revision " HIVE-3705 [jira] Adding authorization capability to the metastore". Patch is getting close. I added few more comments, mostly around documenting semantics used in the patch. INLINE COMMENTS ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:256 This warrants a java-doc comment that in case of partition for which location is not available yet, table's location is used. This will be the case when some one does alter table add partition. Actually, this will be true even in case of create table, where dir will get created only after the authorization has happened, so will it check permissions of parent ? I think it will be good to add java-doc of all the scenarios, which dir will be used to base authentication. e.g., in case if user does create table and provides location and dir already exists, then that dir is used, otherwise parent dir. Semantics of which dir is used in which use-case need to be called out for explicitly. I assume your semantics model is based on http://wiki.apache.org/pig/Howl/HowlAuthorizationProposal and http://wiki.apache.org/pig/Howl/AuthorizationImplNotes If so, please document various pieces of code of this patch with all this information. ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java:27 You don't store the handler, just use its conf. So, I don't see how this interface and its impl is useful as it stands today. On the other hand, I agree that, this is an important interface and is good to have this interface here because there could be alternative impl possible here. So, lets have the interface. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:122 As I noted above, these kind of semantic decision need to be documented in the code. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:168 If they are not used currently than instead of defining semantics now, we should let it fail in default branch and have the exception thrown, such that when someone later does add support for these, we can formalize the semantics. ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java:44 In the trunk, this is done via MetaStoreUtils::startMetaStore() . See, https://issues.apache.org/jira/browse/HIVE-3724 for recommended usage pattern for it. REVISION DETAIL https://reviews.facebook.net/D6681 BRANCH HIVE-3705 To: JIRA, ashutoshc, khorgath
          Hide
          Phabricator added a comment -

          khorgath has commented on the revision "HIVE-3705 [jira] Adding authorization capability to the metastore".

          INLINE COMMENTS
          ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java:27 Ah, wait, I see what you mean - sorry, some of my response is because I thought I was responding to the setHandler() method in the Metastore Authorization Provider as opposed to the Metastore Authentication Provider. And yes, as of right now, from the authentication side, all we really need is the conf, but my point, as you note as well, is from a perspective of thinking about what interface makes sense.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:256 Makes sense, adding.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:122 Agreed. Adding docs.
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:168 Makes sense, changing.
          ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java:44 That's useful. Changing.

          REVISION DETAIL
          https://reviews.facebook.net/D6681

          BRANCH
          HIVE-3705

          To: JIRA, ashutoshc, khorgath

          Show
          Phabricator added a comment - khorgath has commented on the revision " HIVE-3705 [jira] Adding authorization capability to the metastore". INLINE COMMENTS ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java:27 Ah, wait, I see what you mean - sorry, some of my response is because I thought I was responding to the setHandler() method in the Metastore Authorization Provider as opposed to the Metastore Authentication Provider. And yes, as of right now, from the authentication side, all we really need is the conf, but my point, as you note as well, is from a perspective of thinking about what interface makes sense. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java:256 Makes sense, adding. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:122 Agreed. Adding docs. ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java:168 Makes sense, changing. ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java:44 That's useful. Changing. REVISION DETAIL https://reviews.facebook.net/D6681 BRANCH HIVE-3705 To: JIRA, ashutoshc, khorgath
          Hide
          Phabricator added a comment -

          khorgath updated the revision "HIVE-3705 [jira] Adding authorization capability to the metastore".
          Reviewers: JIRA, ashutoshc

          Updating patch for documentation and testing requirements per code review.

          REVISION DETAIL
          https://reviews.facebook.net/D6681

          AFFECTED FILES
          common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
          conf/hive-default.xml.template
          metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
          metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java
          metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java
          ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java
          ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultAuthenticator.java
          ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java
          ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java
          ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
          ql/src/test/org/apache/hadoop/hive/ql/security/DummyHiveMetastoreAuthorizationProvider.java
          ql/src/test/org/apache/hadoop/hive/ql/security/InjectableDummyAuthenticator.java
          ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java
          ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java

          To: JIRA, ashutoshc, khorgath

          Show
          Phabricator added a comment - khorgath updated the revision " HIVE-3705 [jira] Adding authorization capability to the metastore". Reviewers: JIRA, ashutoshc Updating patch for documentation and testing requirements per code review. REVISION DETAIL https://reviews.facebook.net/D6681 AFFECTED FILES common/src/java/org/apache/hadoop/hive/conf/HiveConf.java conf/hive-default.xml.template metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultAuthenticator.java ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java ql/src/test/org/apache/hadoop/hive/ql/security/DummyHiveMetastoreAuthorizationProvider.java ql/src/test/org/apache/hadoop/hive/ql/security/InjectableDummyAuthenticator.java ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java To: JIRA, ashutoshc, khorgath
          Hide
          Phabricator added a comment -

          khorgath has commented on the revision "HIVE-3705 [jira] Adding authorization capability to the metastore".

          INLINE COMMENTS
          ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java:44 Ashutosh, I have tried rebasing to head, and I do not see the changes in MetaStoreUtils that provides that function. This may probably be because I modify MetaStoreUtils in my patch as well, so I have updated the latest patch as if that were available.

          REVISION DETAIL
          https://reviews.facebook.net/D6681

          To: JIRA, ashutoshc, khorgath

          Show
          Phabricator added a comment - khorgath has commented on the revision " HIVE-3705 [jira] Adding authorization capability to the metastore". INLINE COMMENTS ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java:44 Ashutosh, I have tried rebasing to head, and I do not see the changes in MetaStoreUtils that provides that function. This may probably be because I modify MetaStoreUtils in my patch as well, so I have updated the latest patch as if that were available. REVISION DETAIL https://reviews.facebook.net/D6681 To: JIRA, ashutoshc, khorgath
          Hide
          Sushanth Sowmyan added a comment -

          Adding incremental patches in case the phabricator-generated patch does not cleanly apply (which it does not on my setup)

          Show
          Sushanth Sowmyan added a comment - Adding incremental patches in case the phabricator-generated patch does not cleanly apply (which it does not on my setup)
          Hide
          Sushanth Sowmyan added a comment -

          In my setup, the phabricator-generated patch does not cleanly apply on trunk, so I have generated incremental patches for each of the review changes. So, to apply this patch in full, the following patches can be applied in order :

          hive-backend-auth.2.git.patch -> hive-backend-auth.post-review.git.patch -> hive-backend-auth.post-review-part2.git.patch -> hive-backend-auth.post-review-part3.git.patch

          Show
          Sushanth Sowmyan added a comment - In my setup, the phabricator-generated patch does not cleanly apply on trunk, so I have generated incremental patches for each of the review changes. So, to apply this patch in full, the following patches can be applied in order : hive-backend-auth.2.git.patch -> hive-backend-auth.post-review.git.patch -> hive-backend-auth.post-review-part2.git.patch -> hive-backend-auth.post-review-part3.git.patch
          Hide
          Sushanth Sowmyan added a comment -

          Attaching monolithic patch generated from svn trunk to make applying easier.

          Show
          Sushanth Sowmyan added a comment - Attaching monolithic patch generated from svn trunk to make applying easier.
          Hide
          Ashutosh Chauhan added a comment -

          +1 Running tests.

          Show
          Ashutosh Chauhan added a comment - +1 Running tests.
          Hide
          Ashutosh Chauhan added a comment -

          Committed to trunk. Thanks, Sushanth!
          I tried applying this on 0.10 branch, patch doesn't apply. Can you please generate a consolidated backport patch for 0.10

          Show
          Ashutosh Chauhan added a comment - Committed to trunk. Thanks, Sushanth! I tried applying this on 0.10 branch, patch doesn't apply. Can you please generate a consolidated backport patch for 0.10
          Hide
          Hudson added a comment -

          Integrated in Hive-trunk-h0.21 #1844 (See https://builds.apache.org/job/Hive-trunk-h0.21/1844/)
          HIVE-3705 : Adding authorization capability to the metastore (Sushanth Sowmyan via Ashutosh Chauhan) (Revision 1418802)

          Result = FAILURE
          hashutosh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1418802
          Files :

          • /hive/trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
          • /hive/trunk/conf/hive-default.xml.template
          • /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
          • /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java
          • /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultAuthenticator.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
          • /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/DummyHiveMetastoreAuthorizationProvider.java
          • /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/InjectableDummyAuthenticator.java
          • /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java
          • /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java
          Show
          Hudson added a comment - Integrated in Hive-trunk-h0.21 #1844 (See https://builds.apache.org/job/Hive-trunk-h0.21/1844/ ) HIVE-3705 : Adding authorization capability to the metastore (Sushanth Sowmyan via Ashutosh Chauhan) (Revision 1418802) Result = FAILURE hashutosh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1418802 Files : /hive/trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java /hive/trunk/conf/hive-default.xml.template /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultAuthenticator.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/DummyHiveMetastoreAuthorizationProvider.java /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/InjectableDummyAuthenticator.java /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java
          Hide
          Sushanth Sowmyan added a comment -

          I've regenerated this patch for svn branch-0.10, but with the changes in the testcases, we also require the patch for HIVE-3724 to be applied to 0.10 first. I'll attach a patch there too for the patch ported to branch-0.10.

          Show
          Sushanth Sowmyan added a comment - I've regenerated this patch for svn branch-0.10, but with the changes in the testcases, we also require the patch for HIVE-3724 to be applied to 0.10 first. I'll attach a patch there too for the patch ported to branch-0.10.
          Hide
          Ashutosh Chauhan added a comment -

          Committed to 0.10 branch. Thanks, Sushanth!

          Show
          Ashutosh Chauhan added a comment - Committed to 0.10 branch. Thanks, Sushanth!
          Hide
          Hudson added a comment -

          Integrated in Hive-trunk-hadoop2 #54 (See https://builds.apache.org/job/Hive-trunk-hadoop2/54/)
          HIVE-3705 : Adding authorization capability to the metastore (Sushanth Sowmyan via Ashutosh Chauhan) (Revision 1418802)

          Result = ABORTED
          hashutosh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1418802
          Files :

          • /hive/trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java
          • /hive/trunk/conf/hive-default.xml.template
          • /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
          • /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java
          • /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultAuthenticator.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java
          • /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
          • /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/DummyHiveMetastoreAuthorizationProvider.java
          • /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/InjectableDummyAuthenticator.java
          • /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java
          • /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java
          Show
          Hudson added a comment - Integrated in Hive-trunk-hadoop2 #54 (See https://builds.apache.org/job/Hive-trunk-hadoop2/54/ ) HIVE-3705 : Adding authorization capability to the metastore (Sushanth Sowmyan via Ashutosh Chauhan) (Revision 1418802) Result = ABORTED hashutosh : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1418802 Files : /hive/trunk/common/src/java/org/apache/hadoop/hive/conf/HiveConf.java /hive/trunk/conf/hive-default.xml.template /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/MetaStoreUtils.java /hive/trunk/metastore/src/java/org/apache/hadoop/hive/metastore/Warehouse.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/metadata/HiveUtils.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultAuthenticator.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/HadoopDefaultMetastoreAuthenticator.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/HiveMetastoreAuthenticationProvider.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/AuthorizationPreEventListener.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/BitSetCheckedAuthorizationProvider.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveAuthorizationProvider.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/DefaultHiveMetastoreAuthorizationProvider.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveAuthorizationProviderBase.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/HiveMetastoreAuthorizationProvider.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java /hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/DummyHiveMetastoreAuthorizationProvider.java /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/InjectableDummyAuthenticator.java /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/TestAuthorizationPreEventListener.java /hive/trunk/ql/src/test/org/apache/hadoop/hive/ql/security/TestDefaultHiveMetastoreAuthorizationProvider.java
          Hide
          Ashutosh Chauhan added a comment -

          This issue is fixed and released as part of 0.10.0 release. If you find an issue which seems to be related to this one, please create a new jira and link this one with new jira.

          Show
          Ashutosh Chauhan added a comment - This issue is fixed and released as part of 0.10.0 release. If you find an issue which seems to be related to this one, please create a new jira and link this one with new jira.

            People

            • Assignee:
              Sushanth Sowmyan
              Reporter:
              Sushanth Sowmyan
            • Votes:
              0 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development