[
https://issues.apache.org/jira/browse/YARN-617?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13654919#comment-13654919
]
Daryn Sharp commented on YARN-617:
----------------------------------
In addition to Vinod:
* {{NodeManager}}: Minor, does it really need to log that security is on?
{{ContainerManagerImpl}}
* {{getContainerTokenIdentifier}}: Why is it conditionalized to call
{{selectContainerTokenIdentifier}} on either the remote ugi, or the container?
It should be one or the other. Even if security is off, a client will pass a
token, and a server will accept it if it has a secret manager. Until YARN-613
is complete, it should always use the ugi token else with security on I can
auth with one token, but then put another in the launch context.
* {{selectContainerTokenIdentifier(Container)}}: you can simply use
{{Token#decodeIdentifier()}}
* Minor, "ContainerTokenIdentifier cannot be null! Null found for $containerID"
seems a bit redundant and confusing to the casual user. Perhaps a more
succinct"No ContainerToken found for $containerID".
* {{authorizeRequest}}
** {{launchContext.getUser().equals(tokenId.getApplicationSubmitter()}} - this
is now a worthless check. The application submitter in the token is the
trusted/authoritative value. The launch context user used to be used in lieu
of a token, so now with tokens always used, all it does is catch a buggy AM. A
malicious AM will just set the context user to match the token. I'd remove the
check entirely, if not even remove the user from the launch context.
** {{resource == null}} is added, when resources should removed from the launch
context. The check was there to ensure an AM in a secure cluster couldn't lie
in the launch context. The premise of this jira is to not let AMs in an
insecure cluster lie, so the value in the context is moot.
** Overall, seems this method should be reduced to verifying the validity of
the token, and not that it matches the context values.
* In general seems unnecessary for every caller of {{authorizerequest}} to have
duplicate logic to extract and pass the token to the method.
{{authorizerequest}} should locate the token itself.
* I'd revert the order of the logic back to authorizing requests before
checking if the container exists. By flipping them, I can now probe nodes for
locations of containers.
I didn't review the test cases due to time constraints. I'll look at them when
the main issues Vinod and I have cited are addressed.
> In unsercure mode, AM can fake resource requirements
> -----------------------------------------------------
>
> Key: YARN-617
> URL: https://issues.apache.org/jira/browse/YARN-617
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Vinod Kumar Vavilapalli
> Assignee: Omkar Vinit Joshi
> Priority: Minor
> Attachments: YARN-617.20130501.1.patch, YARN-617.20130501.patch,
> YARN-617.20130502.patch, YARN-617-20130507.patch, YARN-617.20130508.patch
>
>
> Without security, it is impossible to completely avoid AMs faking resources.
> We can at the least make it as difficult as possible by using the same
> container tokens and the RM-NM shared key mechanism over unauthenticated
> RM-NM channel.
> In the minimum, this will avoid accidental bugs in AMs in unsecure mode.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
