You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Prasad Mujumdar <pr...@cloudera.com> on 2014/11/11 08:43:33 UTC
Re: Review Request 25983: SENTRY-459 Security mode (Kerberos) support
for SENTRY high availability
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25983/#review60764
-----------------------------------------------------------
A couple of comments below, related to the JAAS code.
Also with Sentry HA enabled, the client side sentry-site much contain _HOST in the server principal. You might want to add a Precondition check to verify the configuration.
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java
<https://reviews.apache.org/r/25983/#comment102172>
Nit: Is it possible to avoid hardcoding these properties ?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java
<https://reviews.apache.org/r/25983/#comment102173>
In secure mode, Sentry service is already running with JAAS context. https://github.com/apache/incubator-sentry/blob/master/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java#L151
Do we still need to setup all the security parameters for ZK ?
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/JaasConfiguration.java
<https://reviews.apache.org/r/25983/#comment102175>
Same as above. SentryService is already obtaining Kerberos ticket. Do we need to redo all that in ZK access code path ?
- Prasad Mujumdar
On Oct. 27, 2014, 6:35 a.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25983/
> -----------------------------------------------------------
>
> (Updated Oct. 27, 2014, 6:35 a.m.)
>
>
> Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-459
> https://issues.apache.org/jira/browse/SENTRY-459
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Support Kerberos for SENTRY high availability. In security mode, Zookeeper will use Kerberos for authentication, SENTRY should use the **principal** and **keytab** in sentry configuration for authentication
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/JaasConfiguration.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java 52eaeed
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForHAWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java cc12099
>
> Diff: https://reviews.apache.org/r/25983/diff/
>
>
> Testing
> -------
>
> All Unit tests passed in local
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 25983: SENTRY-459 Security mode (Kerberos) support
for SENTRY high availability
Posted by Prasad Mujumdar <pr...@cloudera.com>.
> On Nov. 11, 2014, 7:43 a.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java, line 161
> > <https://reviews.apache.org/r/25983/diff/2/?file=733850#file733850line161>
> >
> > In secure mode, Sentry service is already running with JAAS context. https://github.com/apache/incubator-sentry/blob/master/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java#L151
> >
> > Do we still need to setup all the security parameters for ZK ?
>
> Dapeng Sun wrote:
> Hi Prasad. Yes,we need. **org.apache.zookeeper.client.ZooKeeperSaslClient** get the LoginConfiguration like these https://github.com/apache/zookeeper/blob/94210618ffbb0f236e02b388c0aced2bde0ed000/src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java#L119 , but our SENTRY specify the Configuration when create the LoginContext https://github.com/apache/incubator-sentry/blob/70c47e803512efbec51dc5ec5c5d725a5dfa04c3/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java#L59
> So we should setup the security parameters for ZK. otherwise, in real cluster it must set a **jaas.conf** with JVM arguments, but it seems other hadoop components prefer **setJaasConfiguration**
> https://github.com/apache/hadoop/blob/8a261e68e4177b47be01ceae7310ea56aeb7ca38/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java#L163
>
> https://github.com/apache/hadoop/blob/db890eef3208cc557476fa510f7a253ba22bc68a/hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/ZKSignerSecretProvider.java#L390
ah ok, so ZK client does allow resuing the existing login context of the caller. It's unfortunate that we have to duplicate the Jaas setup code in all projects, but doesn't looks like there's an alternative.
Thanks for clarifying that.
- Prasad
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25983/#review60764
-----------------------------------------------------------
On Nov. 12, 2014, 12:16 p.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25983/
> -----------------------------------------------------------
>
> (Updated Nov. 12, 2014, 12:16 p.m.)
>
>
> Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-459
> https://issues.apache.org/jira/browse/SENTRY-459
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Support Kerberos for SENTRY high availability. In security mode, Zookeeper will use Kerberos for authentication, SENTRY should use the **principal** and **keytab** in sentry configuration for authentication
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/JaasConfiguration.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java bc86963
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForHAWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java cc12099
>
> Diff: https://reviews.apache.org/r/25983/diff/
>
>
> Testing
> -------
>
> All Unit tests passed in local
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 25983: SENTRY-459 Security mode (Kerberos) support
for SENTRY high availability
Posted by Dapeng Sun <da...@intel.com>.
> On 十一月 11, 2014, 3:43 p.m., Prasad Mujumdar wrote:
> > A couple of comments below, related to the JAAS code.
> > Also with Sentry HA enabled, the client side sentry-site much contain _HOST in the server principal. You might want to add a Precondition check to verify the configuration.
Good suggestion, thank Prasad, it should throw Exception if **server principal** doesn't contain **_HOST**, I added it to **HAClientInvocationHandler**
> On 十一月 11, 2014, 3:43 p.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java, line 161
> > <https://reviews.apache.org/r/25983/diff/2/?file=733850#file733850line161>
> >
> > In secure mode, Sentry service is already running with JAAS context. https://github.com/apache/incubator-sentry/blob/master/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java#L151
> >
> > Do we still need to setup all the security parameters for ZK ?
Hi Prasad. Yes,we need. **org.apache.zookeeper.client.ZooKeeperSaslClient** get the LoginConfiguration like these https://github.com/apache/zookeeper/blob/94210618ffbb0f236e02b388c0aced2bde0ed000/src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java#L119 , but our SENTRY specify the Configuration when create the LoginContext https://github.com/apache/incubator-sentry/blob/70c47e803512efbec51dc5ec5c5d725a5dfa04c3/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryKerberosContext.java#L59
So we should setup the security parameters for ZK. otherwise, in real cluster it must set a **jaas.conf** with JVM arguments, but it seems other hadoop components prefer **setJaasConfiguration**
https://github.com/apache/hadoop/blob/8a261e68e4177b47be01ceae7310ea56aeb7ca38/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java#L163
https://github.com/apache/hadoop/blob/db890eef3208cc557476fa510f7a253ba22bc68a/hadoop-common-project/hadoop-auth/src/main/java/org/apache/hadoop/security/authentication/util/ZKSignerSecretProvider.java#L390
> On 十一月 11, 2014, 3:43 p.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java, line 84
> > <https://reviews.apache.org/r/25983/diff/2/?file=733850#file733850line84>
> >
> > Nit: Is it possible to avoid hardcoding these properties ?
It's okay, I removed the property, it is easy to be set by other way.
> On 十一月 11, 2014, 3:43 p.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/JaasConfiguration.java, line 35
> > <https://reviews.apache.org/r/25983/diff/2/?file=733851#file733851line35>
> >
> > Same as above. SentryService is already obtaining Kerberos ticket. Do we need to redo all that in ZK access code path ?
Same as above. Thank you very much for your review.
- Dapeng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25983/#review60764
-----------------------------------------------------------
On 十一月 12, 2014, 8:02 p.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25983/
> -----------------------------------------------------------
>
> (Updated 十一月 12, 2014, 8:02 p.m.)
>
>
> Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-459
> https://issues.apache.org/jira/browse/SENTRY-459
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Support Kerberos for SENTRY high availability. In security mode, Zookeeper will use Kerberos for authentication, SENTRY should use the **principal** and **keytab** in sentry configuration for authentication
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/JaasConfiguration.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java bc86963
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForHAWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java cc12099
>
> Diff: https://reviews.apache.org/r/25983/diff/
>
>
> Testing
> -------
>
> All Unit tests passed in local
>
>
> Thanks,
>
> Dapeng Sun
>
>
Re: Review Request 25983: SENTRY-459 Security mode (Kerberos) support
for SENTRY high availability
Posted by Dapeng Sun <da...@intel.com>.
> On 十一月 11, 2014, 3:43 p.m., Prasad Mujumdar wrote:
> > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java, line 84
> > <https://reviews.apache.org/r/25983/diff/2/?file=733850#file733850line84>
> >
> > Nit: Is it possible to avoid hardcoding these properties ?
>
> Dapeng Sun wrote:
> It's okay, I removed the property, it is easy to be set by other way.
Hi Prasad, it is a property on server side, remove the property is okay in my local environment.
- Dapeng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25983/#review60764
-----------------------------------------------------------
On 十一月 12, 2014, 8:16 p.m., Dapeng Sun wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25983/
> -----------------------------------------------------------
>
> (Updated 十一月 12, 2014, 8:16 p.m.)
>
>
> Review request for sentry, Arun Suresh, Lenni Kuff, Prasad Mujumdar, and Sravya Tirukkovalur.
>
>
> Bugs: SENTRY-459
> https://issues.apache.org/jira/browse/SENTRY-459
>
>
> Repository: sentry
>
>
> Description
> -------
>
> Support Kerberos for SENTRY high availability. In security mode, Zookeeper will use Kerberos for authentication, SENTRY should use the **principal** and **keytab** in sentry configuration for authentication
>
>
> Diffs
> -----
>
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/JaasConfiguration.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java bc86963
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServerForHaWithoutKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceForHAWithKerberos.java PRE-CREATION
> sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java cc12099
>
> Diff: https://reviews.apache.org/r/25983/diff/
>
>
> Testing
> -------
>
> All Unit tests passed in local
>
>
> Thanks,
>
> Dapeng Sun
>
>