[
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
