Re: Review Request 68788: SENTRY-2413: Provide a configuration option to permit specific DB privileges to be granted explicitly

2018-09-25 Thread kalyan kumar kalvagadda via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68788/#review208992
---


Ship it!




Ship It!

- kalyan kumar kalvagadda


On Sept. 21, 2018, 2:24 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68788/
> ---
> 
> (Updated Sept. 21, 2018, 2:24 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: sentry-2413
> https://issues.apache.org/jira/browse/sentry-2413
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add a new configuration 'sentry.db.explicit.grants.permitted' that accepts a 
> comma-separated list of privileges that can be granted by Sentry DB clients. 
> If the value is empty, then any privilege can be granted as it works normally 
> (this is the default behavior).
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  adc1947a1a1dd72ef9e6dd743e166979759709b2 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  8cdbde4f9e81b278c8737ea031820e20e0cf5704 
>   
> sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46f7c4335db18113574170f761da9a4ca 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  c23b3850f097b563912c06b29ffd26ea0709b9fd 
> 
> 
> Diff: https://reviews.apache.org/r/68788/diff/2/
> 
> 
> Testing
> ---
> 
> Unit tests added.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68788: SENTRY-2413: Provide a configuration option to permit specific DB privileges to be granted explicitly

2018-09-24 Thread Sergio Pena via Review Board 


> On Sept. 21, 2018, 5:18 p.m., kalyan kumar kalvagadda wrote:
> > Sergio,
> > 
> > Sentry server is policy store and does not need to understand the 
> > action.Let's keep it that way. Logic of validiting the actions is present 
> > in the sentry bindings. I think this logic should go there instead of 
> > SentryPolicyStoreProcessor.
> 
> Sergio Pena wrote:
> I agree, but then we'll need to have the configuration in all the 
> bindings. Hive, Impala (and next SparkSQL) will need this configuration 
> otherwise they will be able to grant those privileges that we're trying to 
> avoid. If Impala does not configure those privileges correctly ,then Hive 
> will use them.
> 
> kalyan kumar kalvagadda wrote:
> I understand that the approach I suggested would need configuration for 
> both Hive and Impala. That is fine.
> 
> All these compute engines have are same. Each have them support different 
> commands. Not all operations that Impala supports are supported by Hive and 
> Impala would like to allow set of operatins which hive may not even support. 
> It could be true other way around. I really feel each of them should have 
> seperate configuration and should be validated at the client side. Sentry 
> server is just a policy store and it may not be the right place to enforce 
> this configuration.

The configuration accepts any list of privileges. The idea behind this config 
is that admins can prevent that Sentry clients grant DB policies that are not 
authorized. Doing it on the clients will not enforce this as any user with the 
access to the Sentry API will be able to grant them. This config must be in the 
server for those reasons. It is up to the Admin to decide which privileges will 
be permitted.


- Sergio


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68788/#review208868
---


On Sept. 21, 2018, 2:24 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68788/
> ---
> 
> (Updated Sept. 21, 2018, 2:24 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: sentry-2413
> https://issues.apache.org/jira/browse/sentry-2413
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add a new configuration 'sentry.db.explicit.grants.permitted' that accepts a 
> comma-separated list of privileges that can be granted by Sentry DB clients. 
> If the value is empty, then any privilege can be granted as it works normally 
> (this is the default behavior).
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  adc1947a1a1dd72ef9e6dd743e166979759709b2 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  8cdbde4f9e81b278c8737ea031820e20e0cf5704 
>   
> sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46f7c4335db18113574170f761da9a4ca 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  c23b3850f097b563912c06b29ffd26ea0709b9fd 
> 
> 
> Diff: https://reviews.apache.org/r/68788/diff/2/
> 
> 
> Testing
> ---
> 
> Unit tests added.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68788: SENTRY-2413: Provide a configuration option to permit specific DB privileges to be granted explicitly

2018-09-21 Thread kalyan kumar kalvagadda via Review Board 


> On Sept. 21, 2018, 5:18 p.m., kalyan kumar kalvagadda wrote:
> > Sergio,
> > 
> > Sentry server is policy store and does not need to understand the 
> > action.Let's keep it that way. Logic of validiting the actions is present 
> > in the sentry bindings. I think this logic should go there instead of 
> > SentryPolicyStoreProcessor.
> 
> Sergio Pena wrote:
> I agree, but then we'll need to have the configuration in all the 
> bindings. Hive, Impala (and next SparkSQL) will need this configuration 
> otherwise they will be able to grant those privileges that we're trying to 
> avoid. If Impala does not configure those privileges correctly ,then Hive 
> will use them.

I understand that the approach I suggested would need configuration for both 
Hive and Impala. That is fine.

All these compute engines have are same. Each have them support different 
commands. Not all operations that Impala supports are supported by Hive and 
Impala would like to allow set of operatins which hive may not even support. It 
could be true other way around. I really feel each of them should have seperate 
configuration and should be validated at the client side. Sentry server is just 
a policy store and it may not be the right place to enforce this configuration.


- kalyan kumar


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68788/#review208868
---


On Sept. 21, 2018, 2:24 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68788/
> ---
> 
> (Updated Sept. 21, 2018, 2:24 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: sentry-2413
> https://issues.apache.org/jira/browse/sentry-2413
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add a new configuration 'sentry.db.explicit.grants.permitted' that accepts a 
> comma-separated list of privileges that can be granted by Sentry DB clients. 
> If the value is empty, then any privilege can be granted as it works normally 
> (this is the default behavior).
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  adc1947a1a1dd72ef9e6dd743e166979759709b2 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  8cdbde4f9e81b278c8737ea031820e20e0cf5704 
>   
> sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46f7c4335db18113574170f761da9a4ca 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  c23b3850f097b563912c06b29ffd26ea0709b9fd 
> 
> 
> Diff: https://reviews.apache.org/r/68788/diff/2/
> 
> 
> Testing
> ---
> 
> Unit tests added.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68788: SENTRY-2413: Provide a configuration option to permit specific DB privileges to be granted explicitly

2018-09-21 Thread kalyan kumar kalvagadda via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68788/#review208868
---



Sergio,

Sentry server is policy store and does not need to understand the action.Let's 
keep it that way. Logic of validiting the actions is present in the sentry 
bindings. I think this logic should go there instead of 
SentryPolicyStoreProcessor.

- kalyan kumar kalvagadda


On Sept. 21, 2018, 2:24 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68788/
> ---
> 
> (Updated Sept. 21, 2018, 2:24 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: sentry-2413
> https://issues.apache.org/jira/browse/sentry-2413
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add a new configuration 'sentry.db.explicit.grants.permitted' that accepts a 
> comma-separated list of privileges that can be granted by Sentry DB clients. 
> If the value is empty, then any privilege can be granted as it works normally 
> (this is the default behavior).
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  adc1947a1a1dd72ef9e6dd743e166979759709b2 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  8cdbde4f9e81b278c8737ea031820e20e0cf5704 
>   
> sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46f7c4335db18113574170f761da9a4ca 
>   
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
>  c23b3850f097b563912c06b29ffd26ea0709b9fd 
> 
> 
> Diff: https://reviews.apache.org/r/68788/diff/2/
> 
> 
> Testing
> ---
> 
> Unit tests added.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Re: Review Request 68788: SENTRY-2413: Provide a configuration option to permit specific DB privileges to be granted explicitly

2018-09-21 Thread Sergio Pena via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68788/
---

(Updated Sept. 21, 2018, 2:24 p.m.)


Review request for sentry.


Bugs: sentry-2413
https://issues.apache.org/jira/browse/sentry-2413


Repository: sentry


Description
---

Add a new configuration 'sentry.db.explicit.grants.permitted' that accepts a 
comma-separated list of privileges that can be granted by Sentry DB clients. 
If the value is empty, then any privilege can be granted as it works normally 
(this is the default behavior).


Diffs (updated)
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
 adc1947a1a1dd72ef9e6dd743e166979759709b2 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
 8cdbde4f9e81b278c8737ea031820e20e0cf5704 
  
sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 3a9623b46f7c4335db18113574170f761da9a4ca 
  
sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/service/thrift/TestSentryPolicyStoreProcessor.java
 c23b3850f097b563912c06b29ffd26ea0709b9fd 


Diff: https://reviews.apache.org/r/68788/diff/2/

Changes: https://reviews.apache.org/r/68788/diff/1-2/


Testing
---

Unit tests added.


Thanks,

Sergio Pena

Re: Review Request 68788: SENTRY-2413: Provide a configuration option to permit specific DB privileges to be granted explicitly

2018-09-20 Thread Na Li via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68788/#review208814
---




sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
Lines 53 (patched)


You can use

Assert.fail("ALL, SELECT and INSERT privileges should be permitted");

It is more intuitive.



sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
Lines 60 (patched)


same thing Assert.fail()



sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
Lines 264 (patched)


Can you add e2e test case for this? To make sure this is called when client 
requests some privileges not supported?


- Na Li


On Sept. 20, 2018, 8:28 p.m., Sergio Pena wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68788/
> ---
> 
> (Updated Sept. 20, 2018, 8:28 p.m.)
> 
> 
> Review request for sentry.
> 
> 
> Bugs: sentry-2413
> https://issues.apache.org/jira/browse/sentry-2413
> 
> 
> Repository: sentry
> 
> 
> Description
> ---
> 
> Add a new configuration 'sentry.db.explicit.grants.permitted' that accepts a 
> comma-separated list of privileges that can be granted by Sentry DB clients. 
> If the value is empty, then any privilege can be granted as it works normally 
> (this is the default behavior).
> 
> 
> Diffs
> -
> 
>   
> sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
>  adc1947a1 
>   
> sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
>  8cdbde4f9 
>   
> sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
>  PRE-CREATION 
>   
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
>  3a9623b46 
> 
> 
> Diff: https://reviews.apache.org/r/68788/diff/1/
> 
> 
> Testing
> ---
> 
> Unit tests added.
> 
> 
> Thanks,
> 
> Sergio Pena
> 
>

Review Request 68788: SENTRY-2413: Provide a configuration option to permit specific DB privileges to be granted explicitly

2018-09-20 Thread Sergio Pena via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68788/
---

Review request for sentry.


Bugs: sentry-2413
https://issues.apache.org/jira/browse/sentry-2413


Repository: sentry


Description
---

Add a new configuration 'sentry.db.explicit.grants.permitted' that accepts a 
comma-separated list of privileges that can be granted by Sentry DB clients. 
If the value is empty, then any privilege can be granted as it works normally 
(this is the default behavior).


Diffs
-

  
sentry-core/sentry-core-common/src/main/java/org/apache/sentry/service/common/ServiceConstants.java
 adc1947a1 
  
sentry-service/sentry-service-api/src/main/java/org/apache/sentry/api/common/SentryServiceUtil.java
 8cdbde4f9 
  
sentry-service/sentry-service-api/src/test/java/org/apache/sentry/api/common/TestSentryServiceUtil.java
 PRE-CREATION 
  
sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/service/thrift/SentryPolicyStoreProcessor.java
 3a9623b46 


Diff: https://reviews.apache.org/r/68788/diff/1/


Testing
---

Unit tests added.


Thanks,

Sergio Pena