[
https://issues.apache.org/jira/browse/HIVE-3705?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13505064#comment-13505064
]
Phabricator commented on HIVE-3705:
-----------------------------------
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
> 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
