You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Na Li <li...@cloudera.com> on 2017/09/28 03:13:33 UTC
Review Request 62654: SENTRY-1957: Do not parse hive sql string in
Sentry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62654/
-----------------------------------------------------------
Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and Sergio Pena.
Repository: sentry
Description
-------
Sentry extracts info for authorization based on input from hive instead of parsing the sql command again
Diffs
-----
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/SentryAuthorizerFactory.java 8b56c49
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java 6c2410b
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/SentryHiveAuthorizationValidator.java 7bf7b87
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SentryAuthorizerUtil.java 35bd68c
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SimpleSemanticAnalyzer.java 85afe52
sentry-binding/sentry-binding-hive-v2/src/test/java/org/apache/sentry/binding/hive/util/TestSentryAuthorizerUtil.java PRE-CREATION
Diff: https://reviews.apache.org/r/62654/diff/1/
Testing
-------
e2e tests and system test
Thanks,
Na Li
Re: Review Request 62654: SENTRY-1957: Do not parse hive sql string
in Sentry
Posted by kalyan kumar kalvagadda <kk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62654/#review186843
-----------------------------------------------------------
I need to have closer look at your code. I found it in my first pass.
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java
Line 243 (original), 255-336 (patched)
<https://reviews.apache.org/r/62654/#comment263677>
All these three methods have duplicate functionality. Only varient is, AuthorizableType.
You can move this iteratinv logic to a utility method and re-use it.
I need to have closer look at your code. I found it in my first pass.
- kalyan kumar kalvagadda
On Sept. 28, 2017, 4:59 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62654/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2017, 4:59 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Sentry extracts info for authorization based on input from hive instead of parsing the sql command again
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/SentryAuthorizerFactory.java 8b56c49
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java 6c2410b
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/SentryHiveAuthorizationValidator.java 7bf7b87
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SentryAuthorizerUtil.java 35bd68c
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SimpleSemanticAnalyzer.java 85afe52
> sentry-binding/sentry-binding-hive-v2/src/test/java/org/apache/sentry/binding/hive/util/TestSentryAuthorizerUtil.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/62654/diff/2/
>
>
> Testing
> -------
>
> e2e tests and system test
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 62654: SENTRY-1957: Do not parse hive sql string
in Sentry
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62654/#review186684
-----------------------------------------------------------
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java
Lines 88 (patched)
<https://reviews.apache.org/r/62654/#comment263494>
Why is this public? I don't see it is used anywhere.
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java
Lines 188-195 (patched)
<https://reviews.apache.org/r/62654/#comment263476>
Hive should handle this issue if a user attempts to drop a non-existent database or table. If it is gotten this far, then it means Hive found the DB. How can this happen?
We should avoid skipping any authorization requests for unknown data. If Hive has a bug in the future where it forgets to send the input/output data, then Sentry will allow the DROP operation?
I think we should throw an exception here. But, we should investigate how this scenario can be possible.
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java
Line 243 (original), 255 (patched)
<https://reviews.apache.org/r/62654/#comment263483>
Avoid short names like 'Curr'. We could rename this method to getDatabaseName() (same to getTableName() and getUdfClassName())
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java
Lines 268 (patched)
<https://reviews.apache.org/r/62654/#comment263489>
What's the difference between input and output hierarchy on Hive databases? Can't they be different in some scenarios?
What if input has a 'default' and output has a 'db1' ? Which one is the one we want to authorize?
Same questions for getCurrTableName() and getCurrUdfClassName().
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java
Lines 279 (patched)
<https://reviews.apache.org/r/62654/#comment263486>
Error/warning/info messages should be clear to the user about something that is happening on the system and what they can do for it.
This message is not clear to the user that Hive did not send enough information to Sentry. Also, I don't think we should log a WARN here. Better to throw an exception from the caller that some info could not be obtained?
Same for getCurrTableName and getCurrUdfClassName.
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java
Lines 315 (patched)
<https://reviews.apache.org/r/62654/#comment263492>
Is this correct? Do we get the URI from the hierarchy list? I didn't see this info when I was testing hive.
If we not get this info, then we should not add this fix yet because we are leaving a security flaw when authorizing UDF (or perhaps an error).
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java
Lines 339-340 (patched)
<https://reviews.apache.org/r/62654/#comment263478>
I don't think we should put in the comment that the caller should skip the processing. It's up to the caller what to do with it later. Let's keep what the function does only.
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java
Lines 341-346 (patched)
<https://reviews.apache.org/r/62654/#comment263482>
What about the comments for all the parameters and exceptions?
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java
Lines 348 (patched)
<https://reviews.apache.org/r/62654/#comment263480>
Same comment here. We just need to comment about why it returns false but not say what the caller should do.
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SentryAuthorizerUtil.java
Line 164 (original), 164 (patched)
<https://reviews.apache.org/r/62654/#comment263493>
Can we remove this and the other comment on this file? This is not related to the patch.
- Sergio Pena
On Sept. 28, 2017, 4:59 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62654/
> -----------------------------------------------------------
>
> (Updated Sept. 28, 2017, 4:59 p.m.)
>
>
> Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Sentry extracts info for authorization based on input from hive instead of parsing the sql command again
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/SentryAuthorizerFactory.java 8b56c49
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java 6c2410b
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/SentryHiveAuthorizationValidator.java 7bf7b87
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SentryAuthorizerUtil.java 35bd68c
> sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SimpleSemanticAnalyzer.java 85afe52
> sentry-binding/sentry-binding-hive-v2/src/test/java/org/apache/sentry/binding/hive/util/TestSentryAuthorizerUtil.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/62654/diff/2/
>
>
> Testing
> -------
>
> e2e tests and system test
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 62654: SENTRY-1957: Do not parse hive sql string
in Sentry
Posted by Na Li <li...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62654/
-----------------------------------------------------------
(Updated Sept. 28, 2017, 4:59 p.m.)
Review request for sentry, Alexander Kolbasov, kalyan kumar kalvagadda, and Sergio Pena.
Repository: sentry
Description
-------
Sentry extracts info for authorization based on input from hive instead of parsing the sql command again
Diffs (updated)
-----
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/SentryAuthorizerFactory.java 8b56c49
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/DefaultSentryValidator.java 6c2410b
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/authorizer/SentryHiveAuthorizationValidator.java 7bf7b87
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SentryAuthorizerUtil.java 35bd68c
sentry-binding/sentry-binding-hive-v2/src/main/java/org/apache/sentry/binding/hive/v2/util/SimpleSemanticAnalyzer.java 85afe52
sentry-binding/sentry-binding-hive-v2/src/test/java/org/apache/sentry/binding/hive/util/TestSentryAuthorizerUtil.java PRE-CREATION
Diff: https://reviews.apache.org/r/62654/diff/2/
Changes: https://reviews.apache.org/r/62654/diff/1-2/
Testing
-------
e2e tests and system test
Thanks,
Na Li