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

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

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

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

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

(Patch available for review)

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

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

@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.

@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.

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

@Shreepadma :

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

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

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

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

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

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

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

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

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

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

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

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

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

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

+1 Running tests.

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

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

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.

Committed to 0.10 branch. Thanks, Sushanth!

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

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.