Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side

2016-05-27 Thread Siddharth Seth 

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


Ship it!




Looks good. Could you please add a timeout annotation on the test before 
committing.

- Siddharth Seth


On May 27, 2016, 12:39 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46956/
> ---
> 
> (Updated May 27, 2016, 12:39 a.m.)
> 
> 
> Review request for hive, Gunther Hagleitner, Jason Dere, and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6a404bd 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenClient.java
>  PRE-CREATION 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenClientFactory.java
>  ebc91b1 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java
>  f10351b 
>   llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java 
> PRE-CREATION 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java
>  7c47f0b 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 
> 465b204 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/SigningSecretManager.java
>  PRE-CREATION 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  2524dc2 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> 5ab7b3c 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
>  b94fc2e 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java
>  04df929 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java
>  c55436b 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorService.java
>  eac0e8f 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
>  74359fa 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java 
> PRE-CREATION 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
>  e0f0676 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapTokenChecker.java
>  762 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
>  a250882 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/security/TestLlapSignerImpl.java
>  PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java c9b912b 
> 
> Diff: https://reviews.apache.org/r/46956/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side

2016-05-26 Thread Sergey Shelukhin 

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




llap-server/src/test/org/apache/hadoop/hive/llap/security/TestLlapSignerImpl.java
 (line 172)


bogus comment


- Sergey Shelukhin


On May 27, 2016, 12:39 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46956/
> ---
> 
> (Updated May 27, 2016, 12:39 a.m.)
> 
> 
> Review request for hive, Gunther Hagleitner, Jason Dere, and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6a404bd 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenClient.java
>  PRE-CREATION 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenClientFactory.java
>  ebc91b1 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java
>  f10351b 
>   llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java 
> PRE-CREATION 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java
>  7c47f0b 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 
> 465b204 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/SigningSecretManager.java
>  PRE-CREATION 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  2524dc2 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> 5ab7b3c 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
>  b94fc2e 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java
>  04df929 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java
>  c55436b 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorService.java
>  eac0e8f 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
>  74359fa 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java 
> PRE-CREATION 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
>  e0f0676 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapTokenChecker.java
>  762 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
>  a250882 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/security/TestLlapSignerImpl.java
>  PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java c9b912b 
> 
> Diff: https://reviews.apache.org/r/46956/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side

2016-05-26 Thread Sergey Shelukhin 

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

(Updated May 27, 2016, 12:39 a.m.)


Review request for hive, Gunther Hagleitner, Jason Dere, and Siddharth Seth.


Repository: hive-git


Description
---

see jira


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 6a404bd 
  
llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenClient.java 
PRE-CREATION 
  
llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenClientFactory.java
 ebc91b1 
  
llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java
 f10351b 
  llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java 
PRE-CREATION 
  
llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java
 7c47f0b 
  llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 
465b204 
  
llap-common/src/java/org/apache/hadoop/hive/llap/security/SigningSecretManager.java
 PRE-CREATION 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
 2524dc2 
  llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
5ab7b3c 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
 b94fc2e 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java
 04df929 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java 
c55436b 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorService.java
 eac0e8f 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
 74359fa 
  llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java 
PRE-CREATION 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
 e0f0676 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapTokenChecker.java
 762 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
 a250882 
  
llap-server/src/test/org/apache/hadoop/hive/llap/security/TestLlapSignerImpl.java
 PRE-CREATION 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java c9b912b 

Diff: https://reviews.apache.org/r/46956/diff/


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side

2016-05-24 Thread Siddharth Seth 

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


Fix it, then Ship it!




Looks good, after addressing the 2 minor comments.


llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
 (line 145)


Should be an explicit check for "false" - otherwise all string other than 
"true", "except_llap_owner" end up getting treated as false.



ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java 


This parameter can be removed from HiveConf now that it's not used.


- Siddharth Seth


On May 23, 2016, 10:22 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46956/
> ---
> 
> (Updated May 23, 2016, 10:22 p.m.)
> 
> 
> Review request for hive, Gunther Hagleitner, Jason Dere, and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java c0843b9 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenClient.java
>  PRE-CREATION 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenClientFactory.java
>  ebc91b1 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java
>  f10351b 
>   llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java 
> PRE-CREATION 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java
>  7c47f0b 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 
> 465b204 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  2524dc2 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> 5ab7b3c 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
>  b94fc2e 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java
>  04df929 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java
>  c55436b 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorService.java
>  eac0e8f 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
>  74359fa 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java 
> PRE-CREATION 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
>  e0f0676 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapTokenChecker.java
>  762 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
>  a250882 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java c9b912b 
> 
> Diff: https://reviews.apache.org/r/46956/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side

2016-05-23 Thread Sergey Shelukhin 

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

(Updated May 23, 2016, 10:22 p.m.)


Review request for hive, Gunther Hagleitner, Jason Dere, and Siddharth Seth.


Repository: hive-git


Description
---

see jira


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java c0843b9 
  
llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenClient.java 
PRE-CREATION 
  
llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenClientFactory.java
 ebc91b1 
  
llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java
 f10351b 
  llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java 
PRE-CREATION 
  
llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java
 7c47f0b 
  llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 
465b204 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
 2524dc2 
  llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
5ab7b3c 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
 b94fc2e 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java
 04df929 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java 
c55436b 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorService.java
 eac0e8f 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
 74359fa 
  llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java 
PRE-CREATION 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
 e0f0676 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapTokenChecker.java
 762 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
 a250882 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java c9b912b 

Diff: https://reviews.apache.org/r/46956/diff/


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side

2016-05-23 Thread Sergey Shelukhin 


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java,
> >  line 290
> > 
> >
> > What user is expected over here.
> > 1. In case of an invocation by HS2 to run a Tez query - I'm assuming 
> > this would be the HS2 service user (which is the same as the LLAP service 
> > user). (That needs to be validated)
> > 2. In case of external services - would this be the HS2 service user or 
> > the user associated with the external service ?
> > 
> > If it's the HS2 user each time, is the "user"/"realuser" field in the 
> > TokenIdentifier required ? That seems to be passed in as a null everywhere.
> > Assuming the appId is what will be used to differentiate different 
> > external clients ? and that in case of Tez - there's no differentiation.
> 
> Sergey Shelukhin wrote:
> This is the calling user in case of RPC.
> 
> Siddharth Seth wrote:
> This goes back to whether it's invoked locally or remotely.
> Local and Remote calls by HS2 to obtain a token on behalf of an external 
> client, will need to pass in the user name to generate the token correctly. 
> What's obtained on the RPC call will almost always be the Hive user - at 
> least for the remote call.
> Behaviour should not change if the flag to get the token locally / 
> remotely is changed.
> 
> Sergey Shelukhin wrote:
> This is protobuf, it's always invoked remotely. Two paths only meet at 
> SecretManager level.
> 
> Siddharth Seth wrote:
> Reference was to the way the token is obtained (locally or remotely) - 
> not the specific call.

fixed by only retaining the remote API for CLI and removing the setting. HS2 
user should have access to ZK paths, for registry if nothing else, so this 
shouldn't be a problem


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java,
> >  line 61
> > 
> >
> > I don't think we need to create a new instance of the 
> > ZKDelegationTokenSecretManager.
> > 
> > The one created earlier to generate tokens should be passed in.
> > 
> > The KeySigner could be an interface instead, and SecretManager (extends 
> > ZKDelegationTokenSecretManager) can implement this. ACL checks etc are 
> > already setup there. There's no requirement to have two independent copies 
> > of the ZKSecretManager running in the same daemon.
> 
> Sergey Shelukhin wrote:
> This one has completely different logic and even different template 
> parameter.
> 
> Siddharth Seth wrote:
> Most of the logic is in the BaseSecretManager itself, correct ?
> The same instance can be used to generate tokens, as well as sign. Is 
> there a downside to that ?
> Setting up two instances would create extra threads, and confusion while 
> debugging; Also potentially additional load to ZK, additional logins, etc

Merged the classes. They are created separately for now, I'll add central 
creation in one of the subsequent patches that adds llapcoordinator.


- Sergey


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


On May 21, 2016, 12:07 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46956/
> ---
> 
> (Updated May 21, 2016, 12:07 a.m.)
> 
> 
> Review request for hive, Gunther Hagleitner, Jason Dere, and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 4cfa5f1 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java
>  f10351b 
>   llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java 
> PRE-CREATION 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java
>  e28eddd 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 
> 465b204 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  2524dc2 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> de817e3 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
>  b94fc2e 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java
>  03ee055 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java
>  8abd198 
>   
>

Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side

2016-05-23 Thread Siddharth Seth 


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java,
> >  line 290
> > 
> >
> > What user is expected over here.
> > 1. In case of an invocation by HS2 to run a Tez query - I'm assuming 
> > this would be the HS2 service user (which is the same as the LLAP service 
> > user). (That needs to be validated)
> > 2. In case of external services - would this be the HS2 service user or 
> > the user associated with the external service ?
> > 
> > If it's the HS2 user each time, is the "user"/"realuser" field in the 
> > TokenIdentifier required ? That seems to be passed in as a null everywhere.
> > Assuming the appId is what will be used to differentiate different 
> > external clients ? and that in case of Tez - there's no differentiation.
> 
> Sergey Shelukhin wrote:
> This is the calling user in case of RPC.
> 
> Siddharth Seth wrote:
> This goes back to whether it's invoked locally or remotely.
> Local and Remote calls by HS2 to obtain a token on behalf of an external 
> client, will need to pass in the user name to generate the token correctly. 
> What's obtained on the RPC call will almost always be the Hive user - at 
> least for the remote call.
> Behaviour should not change if the flag to get the token locally / 
> remotely is changed.
> 
> Sergey Shelukhin wrote:
> This is protobuf, it's always invoked remotely. Two paths only meet at 
> SecretManager level.

Reference was to the way the token is obtained (locally or remotely) - not the 
specific call.


- Siddharth


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


On May 21, 2016, 12:07 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46956/
> ---
> 
> (Updated May 21, 2016, 12:07 a.m.)
> 
> 
> Review request for hive, Gunther Hagleitner, Jason Dere, and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 4cfa5f1 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java
>  f10351b 
>   llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java 
> PRE-CREATION 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java
>  e28eddd 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 
> 465b204 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  2524dc2 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> de817e3 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
>  b94fc2e 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java
>  03ee055 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java
>  8abd198 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorService.java
>  eac0e8f 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
>  74359fa 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java 
> PRE-CREATION 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
>  279baf1 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapTokenChecker.java
>  762 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
>  a250882 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java c9b912b 
> 
> Diff: https://reviews.apache.org/r/46956/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side

2016-05-23 Thread Sergey Shelukhin 


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java,
> >  line 290
> > 
> >
> > What user is expected over here.
> > 1. In case of an invocation by HS2 to run a Tez query - I'm assuming 
> > this would be the HS2 service user (which is the same as the LLAP service 
> > user). (That needs to be validated)
> > 2. In case of external services - would this be the HS2 service user or 
> > the user associated with the external service ?
> > 
> > If it's the HS2 user each time, is the "user"/"realuser" field in the 
> > TokenIdentifier required ? That seems to be passed in as a null everywhere.
> > Assuming the appId is what will be used to differentiate different 
> > external clients ? and that in case of Tez - there's no differentiation.
> 
> Sergey Shelukhin wrote:
> This is the calling user in case of RPC.
> 
> Siddharth Seth wrote:
> This goes back to whether it's invoked locally or remotely.
> Local and Remote calls by HS2 to obtain a token on behalf of an external 
> client, will need to pass in the user name to generate the token correctly. 
> What's obtained on the RPC call will almost always be the Hive user - at 
> least for the remote call.
> Behaviour should not change if the flag to get the token locally / 
> remotely is changed.

This is protobuf, it's always invoked remotely. Two paths only meet at 
SecretManager level.


- Sergey


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


On May 21, 2016, 12:07 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46956/
> ---
> 
> (Updated May 21, 2016, 12:07 a.m.)
> 
> 
> Review request for hive, Gunther Hagleitner, Jason Dere, and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 4cfa5f1 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java
>  f10351b 
>   llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java 
> PRE-CREATION 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java
>  e28eddd 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 
> 465b204 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  2524dc2 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> de817e3 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
>  b94fc2e 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java
>  03ee055 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java
>  8abd198 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorService.java
>  eac0e8f 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
>  74359fa 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java 
> PRE-CREATION 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
>  279baf1 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapTokenChecker.java
>  762 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
>  a250882 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java c9b912b 
> 
> Diff: https://reviews.apache.org/r/46956/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side

2016-05-23 Thread Siddharth Seth 

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



Still needs some tests.


llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
 (line 79)


Default to TRUE. Alternately - see next comment.



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
 (line 145)


This should explicitly check for "false". Any other value should either 
default to true or thrown an error. Err on the side of being secure.


- Siddharth Seth


On May 21, 2016, 12:07 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46956/
> ---
> 
> (Updated May 21, 2016, 12:07 a.m.)
> 
> 
> Review request for hive, Gunther Hagleitner, Jason Dere, and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 4cfa5f1 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java
>  f10351b 
>   llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java 
> PRE-CREATION 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java
>  e28eddd 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 
> 465b204 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  2524dc2 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> de817e3 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
>  b94fc2e 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java
>  03ee055 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java
>  8abd198 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorService.java
>  eac0e8f 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
>  74359fa 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java 
> PRE-CREATION 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
>  279baf1 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapTokenChecker.java
>  762 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
>  a250882 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java c9b912b 
> 
> Diff: https://reviews.apache.org/r/46956/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side

2016-05-23 Thread Siddharth Seth 


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java,
> >  line 134
> > 
> >
> > Can a second login be avoided. I'm guessing this is because the ZK 
> > principla may be different from the llap principla.
> > What was the reason for them to be different again ? (Especially w.r.t 
> > the SecretManager). Not sure if the fallback to using the llap principal 
> > and keytab will work if they have to be different.
> 
> Sergey Shelukhin wrote:
> The same principal didn't work on the test cluster I had for some reason 
> that I no longer remember :(

The logic does a login with ZK keytab, otherwise fallback to the one used by 
LLAP.
Either the LLAP keytab works - in which case the ZK login is unnecessary, OR 
the LLAP keytab does not work - in which case the fallback should not exist.

If this is an unknown, should we create a follow up jira to investigate and 
fix, and meanwhile add a comment in the code ?


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java,
> >  line 170
> > 
> >
> > Maybe move all of these checks into the RPC layers itself ... i.e. 
> > LlapServiceServerImpl. As early as possible.
> 
> Sergey Shelukhin wrote:
> The permissions are checked in calls by ContainerRunner. RPC right now 
> just propagates the request...

The suggestion is to move it up :)


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java,
> >  line 287
> > 
> >
> > All of this logic should be invoked even when obtaining tokens from 
> > ZKSM directly.
> > 
> > Whether Tez is being used, or an external client - as long as HS2 is 
> > obtaining a token, it can do it directly from ZK. This code path is not 
> > likely to be exercised a lot.
> > Assuming that invocation (when it happens, and likely needs another 
> > jira) - will call in to LlapTokenLocalClient.createToken directly - and 
> > will send in isSigningRequired based on all of the same configs.
> > 
> > Would be better to move the logic out of this function in that case.
> > 
> > Maybe the config flag itself could be dropped. If Tez, no singing, if 
> > external - force signing.
> 
> Sergey Shelukhin wrote:
> this actually kind of orthogonal. This logic doesn't apply to when HS2 
> creates the token preciusely because HS2 knows whether it's creating the 
> token for Tez or external, so it can set the flag accordingly.
> When the method is called remotely, by default we always require signing, 
> but that can be disabled for CLI, or HS2 calling remotely (presumably under 
> the same user as LLAP).

I think we need a ManagementInterface - which has the local and remote 
implementation. The if/else block in the core (TezSessionState) is fairly ugly. 
All of the implementation details and logic for the individual cases can hide 
in there. Likely a separate jira ?


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java,
> >  line 290
> > 
> >
> > What user is expected over here.
> > 1. In case of an invocation by HS2 to run a Tez query - I'm assuming 
> > this would be the HS2 service user (which is the same as the LLAP service 
> > user). (That needs to be validated)
> > 2. In case of external services - would this be the HS2 service user or 
> > the user associated with the external service ?
> > 
> > If it's the HS2 user each time, is the "user"/"realuser" field in the 
> > TokenIdentifier required ? That seems to be passed in as a null everywhere.
> > Assuming the appId is what will be used to differentiate different 
> > external clients ? and that in case of Tez - there's no differentiation.
> 
> Sergey Shelukhin wrote:
> This is the calling user in case of RPC.

This goes back to whether it's invoked locally or remotely.
Local and Remote calls by HS2 to obtain a token on behalf of an external 
client, will need to pass in the user name to generate the token correctly. 
What's obtained on the RPC call will almost always be the Hive user - at least 
for the remote call.
Behaviour should not change if the flag to get the token locally / remotely is 
changed.


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java,
> >  line 61
> > 
> >
> > I don't think we need to create a new

Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side

2016-05-23 Thread Sergey Shelukhin 


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java,
> >  line 267
> > 
> >
> > A follow up jira may be to limit the age of keys.
> > i.e. if a keyId is older than a certain amount of time - fail the 
> > request. I'm not sure how ZKSecretManager rotates these keys, and when they 
> > are invalidated.
> > 
> > A user can potentially use an old (presumably compromsied key) to 
> > generate requests - which will be valid if keys are not rotated/aged.

https://issues.apache.org/jira/browse/HIVE-13820


- Sergey


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


On May 21, 2016, 12:07 a.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46956/
> ---
> 
> (Updated May 21, 2016, 12:07 a.m.)
> 
> 
> Review request for hive, Gunther Hagleitner, Jason Dere, and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 4cfa5f1 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java
>  f10351b 
>   llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java 
> PRE-CREATION 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java
>  e28eddd 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 
> 465b204 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  2524dc2 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> de817e3 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
>  b94fc2e 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java
>  03ee055 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java
>  8abd198 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorService.java
>  eac0e8f 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
>  74359fa 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java 
> PRE-CREATION 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
>  279baf1 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapTokenChecker.java
>  762 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
>  a250882 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java c9b912b 
> 
> Diff: https://reviews.apache.org/r/46956/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side

2016-05-20 Thread Sergey Shelukhin 

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

(Updated May 21, 2016, 12:07 a.m.)


Review request for hive, Gunther Hagleitner, Jason Dere, and Siddharth Seth.


Repository: hive-git


Description
---

see jira


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 4cfa5f1 
  
llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java
 f10351b 
  llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java 
PRE-CREATION 
  
llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java
 e28eddd 
  llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 
465b204 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
 2524dc2 
  llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
de817e3 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
 b94fc2e 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java
 03ee055 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java 
8abd198 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorService.java
 eac0e8f 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
 74359fa 
  llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java 
PRE-CREATION 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
 279baf1 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapTokenChecker.java
 762 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
 a250882 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java c9b912b 

Diff: https://reviews.apache.org/r/46956/diff/


Testing
---


Thanks,

Sergey Shelukhin

Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side

2016-05-20 Thread Sergey Shelukhin 


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java,
> >  line 25
> > 
> >
> > Think the patch which added Pair/ImmutablePair may have added a maven 
> > dependency. Should be removed if it was added explicitly for this.

it has since become necessary for StringUtils


- Sergey


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


On May 18, 2016, 8:36 p.m., Sergey Shelukhin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46956/
> ---
> 
> (Updated May 18, 2016, 8:36 p.m.)
> 
> 
> Review request for hive, Gunther Hagleitner, Jason Dere, and Siddharth Seth.
> 
> 
> Repository: hive-git
> 
> 
> Description
> ---
> 
> see jira
> 
> 
> Diffs
> -
> 
>   common/src/java/org/apache/hadoop/hive/conf/HiveConf.java cbb3a72 
>   
> llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java
>  f10351b 
>   llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java 
> PRE-CREATION 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java
>  e28eddd 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenProvider.java
>  PRE-CREATION 
>   
> llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 
> 465b204 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
>  2524dc2 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
> de817e3 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
>  b94fc2e 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java
>  03ee055 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java
>  8abd198 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorService.java
>  eac0e8f 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
>  74359fa 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java
>  PRE-CREATION 
>   
> llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java 
> PRE-CREATION 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
>  279baf1 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapTokenChecker.java
>  762 
>   
> llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
>  a250882 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java c9b912b 
> 
> Diff: https://reviews.apache.org/r/46956/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Sergey Shelukhin
> 
>

Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side

2016-05-20 Thread Sergey Shelukhin 


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConf.java, lines 2698-2699
> > 
> >
> > Is this primarily for config ? Rename to have a positive connotation 
> > maybe ?

it says in the description :)


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java, 
> > line 29
> > 
> >
> > I'm not sure this will actually be usable, given that what is being 
> > signed is a protobuf generated class.

It's used to implement a wrapper over protobuf


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java,
> >  line 134
> > 
> >
> > Can a second login be avoided. I'm guessing this is because the ZK 
> > principla may be different from the llap principla.
> > What was the reason for them to be different again ? (Especially w.r.t 
> > the SecretManager). Not sure if the fallback to using the llap principal 
> > and keytab will work if they have to be different.

The same principal didn't work on the test cluster I had for some reason that I 
no longer remember :(


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java,
> >  line 168
> > 
> >
> > Move this to after checking if vertexBinary is set ? Potentially error 
> > out if both are set.
> > 
> > IIRC, vertexBinary will be set by external clients, and vertex will be 
> > set by Tez ?

yes


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java,
> >  line 262
> > 
> >
> > Why is this required ? The signature will only exist if vertexBinary is 
> > present ?

No reason why someone cannot set signature and vertex.


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java,
> >  line 170
> > 
> >
> > Maybe move all of these checks into the RPC layers itself ... i.e. 
> > LlapServiceServerImpl. As early as possible.

The permissions are checked in calls by ContainerRunner. RPC right now just 
propagates the request...


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java,
> >  line 287
> > 
> >
> > All of this logic should be invoked even when obtaining tokens from 
> > ZKSM directly.
> > 
> > Whether Tez is being used, or an external client - as long as HS2 is 
> > obtaining a token, it can do it directly from ZK. This code path is not 
> > likely to be exercised a lot.
> > Assuming that invocation (when it happens, and likely needs another 
> > jira) - will call in to LlapTokenLocalClient.createToken directly - and 
> > will send in isSigningRequired based on all of the same configs.
> > 
> > Would be better to move the logic out of this function in that case.
> > 
> > Maybe the config flag itself could be dropped. If Tez, no singing, if 
> > external - force signing.

this actually kind of orthogonal. This logic doesn't apply to when HS2 creates 
the token preciusely because HS2 knows whether it's creating the token for Tez 
or external, so it can set the flag accordingly.
When the method is called remotely, by default we always require signing, but 
that can be disabled for CLI, or HS2 calling remotely (presumably under the 
same user as LLAP).


> On May 20, 2016, 2:21 a.m., Siddharth Seth wrote:
> > llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java,
> >  line 290
> > 
> >
> > What user is expected over here.
> > 1. In case of an invocation by HS2 to run a Tez query - I'm assuming 
> > this would be the HS2 service user (which is the same as the LLAP service 
> > user). (That needs to be validated)
> > 2. In case of external services - would this be the HS2 service user or 
> > the user associated with the external service ?
> > 
> > If it's the HS2 user each time, is the "user"/"realuser" field in the 
> > TokenIdentifier required ? That seems to be passed in as a null everywhere.
> > Assuming the appId is what will be used to differentiate different 
> > external clients ? and that

Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side

2016-05-19 Thread Siddharth Seth 

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



Think it will be useful to add some tests around
1) signing / validation
2) The config parameter (assuming it stays), and it behaving the way it's 
intended to make sure tokens are created with the correct parameters. There's a 
lot of ! of ! of ! checks happening.


common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (lines 2698 - 2699)


Is this primarily for config ? Rename to have a positive connotation maybe ?



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2699)


Rename "user" to "llapuser"/"serviceowner" - something that implies this is 
only for the user owning the service.
Maybe call the other two settings "always", "never" - instead of "true", 
"false"



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2700)


What should the default value here be ?
"false" seems to imply sign alyway. In case the client config is setup to 
obtain tokens remotely - instead of directly from ZK on the client side in HS2 
- Tez would end up obtaining tokens which require signing as well ?



llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java (line 
29)


I'm not sure this will actually be usable, given that what is being signed 
is a protobuf generated class.



llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenProvider.java
 (line 1)


Not used anywhere. Re-introduce in the patch where it's required ?



llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 
(line 126)


Can a second login be avoided. I'm guessing this is because the ZK 
principla may be different from the llap principla.
What was the reason for them to be different again ? (Especially w.r.t the 
SecretManager). Not sure if the fallback to using the llap principal and keytab 
will work if they have to be different.



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
 (line 168)


Move this to after checking if vertexBinary is set ? Potentially error out 
if both are set.

IIRC, vertexBinary will be set by external clients, and vertex will be set 
by Tez ?



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
 (line 170)


Maybe move all of these checks into the RPC layers itself ... i.e. 
LlapServiceServerImpl. As early as possible.



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
 (line 262)


Why is this required ? The signature will only exist if vertexBinary is 
present ?



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
 (line 267)


A follow up jira may be to limit the age of keys.
i.e. if a keyId is older than a certain amount of time - fail the request. 
I'm not sure how ZKSecretManager rotates these keys, and when they are 
invalidated.

A user can potentially use an old (presumably compromsied key) to generate 
requests - which will be valid if keys are not rotated/aged.



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
 (line 66)


The meaning is a little unclear, when considered along with the negative 
connotation of the config parameter. I don't actually know what a TRUE value 
here means. Even more so when considered alongside the parameter called 
"isNoSigning"



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
 (line 144)


"user"



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
 (line 284)


All of this logic should be invoked even when obtaining tokens from ZKSM 
directly.

Whether Tez is being used, or an external client - as long as HS2 is 
obtaining a token, it can do it directly from ZK. This code path is not likely 
to be exercised a lot.
Assuming that invocation (when it happens, and likely needs another jira) - 
will call in to LlapTokenLocalClient.createToken directly - and will send in 
isSigningRequired based on all of the same configs.

Would be better to move the

Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side

2016-05-18 Thread Sergey Shelukhin 

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

(Updated May 18, 2016, 8:36 p.m.)


Review request for hive, Gunther Hagleitner, Jason Dere, and Siddharth Seth.


Repository: hive-git


Description
---

see jira


Diffs (updated)
-

  common/src/java/org/apache/hadoop/hive/conf/HiveConf.java cbb3a72 
  
llap-client/src/java/org/apache/hadoop/hive/llap/security/LlapTokenLocalClient.java
 f10351b 
  llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapSigner.java 
PRE-CREATION 
  
llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenIdentifier.java
 e28eddd 
  
llap-common/src/java/org/apache/hadoop/hive/llap/security/LlapTokenProvider.java
 PRE-CREATION 
  llap-common/src/java/org/apache/hadoop/hive/llap/security/SecretManager.java 
465b204 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/ContainerRunnerImpl.java
 2524dc2 
  llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapDaemon.java 
de817e3 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java
 b94fc2e 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java
 03ee055 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/QueryTracker.java 
8abd198 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorService.java
 eac0e8f 
  
llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/TaskRunnerCallable.java
 74359fa 
  
llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java
 PRE-CREATION 
  llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java 
PRE-CREATION 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TaskExecutorTestHelpers.java
 279baf1 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/TestLlapTokenChecker.java
 762 
  
llap-server/src/test/org/apache/hadoop/hive/llap/daemon/impl/comparator/TestFirstInFirstOutComparator.java
 a250882 
  ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java c9b912b 

Diff: https://reviews.apache.org/r/46956/diff/


Testing
---


Thanks,

Sergey Shelukhin