> 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 > >
