hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Siddharth Seth <ss...@apache.org>
Subject Re: Review Request 46956: HIVE-13444 LLAP: add HMAC signatures to LLAP; verify them on LLAP side
Date Fri, 20 May 2016 02:21:04 GMT

-----------------------------------------------------------
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)
<https://reviews.apache.org/r/46956/#comment198707>

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



common/src/java/org/apache/hadoop/hive/conf/HiveConf.java (line 2699)
<https://reviews.apache.org/r/46956/#comment198708>

    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)
<https://reviews.apache.org/r/46956/#comment198715>

    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)
<https://reviews.apache.org/r/46956/#comment198722>

    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)
<https://reviews.apache.org/r/46956/#comment198710>

    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)
<https://reviews.apache.org/r/46956/#comment198713>

    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)
<https://reviews.apache.org/r/46956/#comment198718>

    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)
<https://reviews.apache.org/r/46956/#comment198721>

    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)
<https://reviews.apache.org/r/46956/#comment198719>

    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)
<https://reviews.apache.org/r/46956/#comment198724>

    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)
<https://reviews.apache.org/r/46956/#comment198711>

    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)
<https://reviews.apache.org/r/46956/#comment198712>

    "user"



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java (line
284)
<https://reviews.apache.org/r/46956/#comment198716>

    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.



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapProtocolServerImpl.java (line
287)
<https://reviews.apache.org/r/46956/#comment198714>

    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.



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java 
<https://reviews.apache.org/r/46956/#comment198717>

    Think the patch which added Pair/ImmutablePair may have added a maven dependency. Should
be removed if it was added explicitly for this.



llap-server/src/java/org/apache/hadoop/hive/llap/daemon/impl/LlapTokenChecker.java (line 35)
<https://reviews.apache.org/r/46956/#comment198725>

    Nit: final



llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSecurityHelper.java (line 1)
<https://reviews.apache.org/r/46956/#comment198709>

    This entire class is not used anywhere. Can it be dropped, and re-introduced in another
jira if it is actually required.



llap-server/src/java/org/apache/hadoop/hive/llap/security/LlapSignerImpl.java (line 61)
<https://reviews.apache.org/r/46956/#comment198720>

    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.


- Siddharth Seth


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message