[jira] [Comment Edited] (HIVE-13445) LLAP: token should encode application and cluster ids

["Sergey Shelukhin (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HIVE-13445?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15254975#comment-15254975 ] 

Sergey Shelukhin edited comment on HIVE-13445 at 4/23/16 12:44 AM:
-------------------------------------------------------------------

{noformat}
Any possibility of performing some basic sanity checks inside LlapProtocolServerImpl - or is that already in place via the RPC layer validating the presence of a LLAP token. Don't like the fact that the security chceks are 3 calls deep - but that seems the best place for them rightnow.
{noformat}
The RPC layer validates the presence of the token.

{noformat}
String hostName = MetricsUtils.getHostName(); - Not necessarily related to this patch, but getting it from YARN is more consistent (when yarn is available). Have seen lots of issues around figuring out hostnames otherwise.
{noformat}
Is the yarn option already used somewhere? We could just change the utility method to use it too.

{noformat}
LlapDeamon: appName = UUID.randomUUID().toString();

Ths won't work on distributed clusters, right ? Tokens use this as the appSecret. Each node will generate a different appSecret. daemonId.getAppSecret is being used as the clusterId in LlapTokenIdentifier.
{noformat}
We assume this is only used in tests. It won't work indeed. Added a comment

{noformat}
In LlapTokenChecker - why are we iterating over tokens even after an LLAPToken has been found ? Are multiple tokens expected. This is in checkPermissions as well as getTokenInfo
{noformat}
Not really expected at this point; I wonder if external clients could be using something like that.


{noformat}
It looks like we end up taking the first request and linking it with the query. Also subsequent requests are validated against this. Assuming that this becomes more useful once signing comes in - to make sure someone is not submitting with incorrect parameters.
{noformat}
Yes, if we also validate it against the signature. In general, though, we assume that whoever can submit fragments (ie has the specific token) can also kill fragments. The key is not being able to submit/kill/etc. fragments for an app with a different token.

{noformat}
TaskExecutorService.findQueryByFragment - think we're better off implementing this in QueryInfo itself rather than going to the scheduler to find out this information. need to check if QueryInfo has state information about which fragments are linked to a query.
{noformat}
It doesn't, as far as I can tell.

{noformat}
getDelegationToken(String appSecret) - even in case of Tez, should this be associated with the sessionId. That prevents a lot of the if (token.appSecret == null) checks and will simplify the code.
{noformat}
Don't understand. Can you elaborate?

{noformat}
Forgot to mention, we should add some tests to validate token functionality, and how the system interacts with QueryInfo etc.
{noformat}
Separate JIRA?

{noformat}
More on this. If eventually, we're going to validate this via signatures for external access - do we actually need to store the appSecret/appId for the Query. Instead, we could validate future requests against the already stored applicationId for a fragment / query.
{noformat}
The app ID has to come from somewhere with each request; terminate/etc. requests themselves are not signed, so we cannot rely on the user to give us the correct app Id to verify against the fragment. The appId when submitting can indeed come from the signed message, not from the token (or it could be verified to be the same from both).

But, I think we'd still need it in the token for other requests. I am actually not sure how the token will work with signing right now, more specifically - will we be able to get away with not having appsecret be a secret? I think we will if HS2 would generate and sign it. However, if the client is allowed to pass it in, some other client might also pass in the same appId and secret, and get the same token. So I assume we'd still store it, although it won't really be called secret, it's just something that the signer (HS2) has to generate.

Fixing the rest.


was (Author: sershe):

{noformat}
Any possibility of performing some basic sanity checks inside LlapProtocolServerImpl - or is that already in place via the RPC layer validating the presence of a LLAP token. Don't like the fact that the security chceks are 3 calls deep - but that seems the best place for them rightnow.
{noformat}
The RPC layer validates the presence of the token.

{noformat}
String hostName = MetricsUtils.getHostName(); - Not necessarily related to this patch, but getting it from YARN is more consistent (when yarn is available). Have seen lots of issues around figuring out hostnames otherwise.
{noformat}
Is the yarn option already used somewhere? We could just change the utility method to use it too.

{noformat}
LlapDeamon: appName = UUID.randomUUID().toString();

Ths won't work on distributed clusters, right ? Tokens use this as the appSecret. Each node will generate a different appSecret. daemonId.getAppSecret is being used as the clusterId in LlapTokenIdentifier.
{noformat}
We assume this is only used in tests. It won't work indeed. Added a comment

{noformat}
In LlapTokenChecker - why are we iterating over tokens even after an LLAPToken has been found ? Are multiple tokens expected. This is in checkPermissions as well as getTokenInfo
{noformat}
Not really expected at this point; I wonder if external clients could be using something like that.


{noformat}
It looks like we end up taking the first request and linking it with the query. Also subsequent requests are validated against this. Assuming that this becomes more useful once signing comes in - to make sure someone is not submitting with incorrect parameters.
{noformat}
Yes, if we also validate it against the signature. In general, though, we assume that whoever can submit fragments (ie has the specific token) can also kill fragments. The key is not being able to submit/kill/etc. fragments for an app with a different token.

{noformat}
TaskExecutorService.findQueryByFragment - think we're better off implementing this in QueryInfo itself rather than going to the scheduler to find out this information. need to check if QueryInfo has state information about which fragments are linked to a query.
{noformat}
It doesn't, as far as I can tell.

{noformat}
getDelegationToken(String appSecret) - even in case of Tez, should this be associated with the sessionId. That prevents a lot of the if (token.appSecret == null) checks and will simplify the code.
{noformat}
Don't understand. Can you elaborate?

{noformat}
Forgot to mention, we should add some tests to validate token functionality, and how the system interacts with QueryInfo etc.
{noformat}
Separate JIRA?

{noformat}
More on this. If eventually, we're going to validate this via signatures for external access - do we actually need to store the appSecret/appId for the Query. Instead, we could validate future requests against the already stored applicationId for a fragment / query.
{noformat}
The app ID has to come from somewhere with each request; terminate/etc. requests themselves are not signed. I am actually not sure how the token will work with signing right now, more specifically - will we be able to get away with not having appsecret be a secret? I think we will if HS2 would generate and sign it. However, if the client is allowed to pass it in, some other client might also pass in the same appId and secret, and get the same token. So I assume we'd still store it, although it won't really be called secret, it's just something that the signer (HS2) has to generate.

Fixing the rest.

> LLAP: token should encode application and cluster ids
> -----------------------------------------------------
>
>                 Key: HIVE-13445
>                 URL: https://issues.apache.org/jira/browse/HIVE-13445
>             Project: Hive
>          Issue Type: Bug
>            Reporter: Sergey Shelukhin
>            Assignee: Sergey Shelukhin
>         Attachments: HIVE-13445.01.patch, HIVE-13445.patch
>
>




--
This message was sent by Atlassian JIRA
(v6.3.4#6332)