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