[ https://issues.apache.org/jira/browse/HIVE-3705?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13504069#comment-13504069 ]
Phabricator commented on HIVE-3705: ----------------------------------- 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 > Adding authorization capability to the metastore > ------------------------------------------------ > > Key: HIVE-3705 > URL: https://issues.apache.org/jira/browse/HIVE-3705 > Project: Hive > Issue Type: New Feature > Components: Authorization, Metastore > Reporter: Sushanth Sowmyan > Assignee: Sushanth Sowmyan > Attachments: HIVE-3705.D6681.1.patch, HIVE-3705.D6681.2.patch, > HIVE-3705.D6681.3.patch, hive-backend-auth.2.git.patch, > hive-backend-auth.git.patch, hivesec_investigation.pdf > > > 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. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators For more information on JIRA, see: http://www.atlassian.com/software/jira