[
https://issues.apache.org/jira/browse/HADOOP-10771?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14072642#comment-14072642
]
Aaron T. Myers commented on HADOOP-10771:
-----------------------------------------
Thanks a lot for reorganizing the patch, and for the comments/explanations,
Tucu. That made it much easier to review.
A few pretty minor comments below, and a few nits on top of that. +1 once these
are addressed:
Comments:
# Not clear to me how it is that
{{DelegationTokenAuthenticatedURL#cancelDelegationToken}} can throw an
{{AuthenticationException}}, given that that action does not in fact require
authentication. How can that be?
# In {{DelegationTokenAuthenticator#validateResponse}}, recommend including the
expected response code in the exception message of the error case. Might also
consider logging the {{IOException}} in the case of a failed close, at DEBUG
level though.
# Why remove the javadocs in {{DelegationTokenIdentifier}}? Maybe that makes
sense for the overridden {{getKind}} method, but I think you should leave the
javadocs for the constructor at least.
# Perhaps this is pre-existing behavior, but I'm surprised to see that the
{{KerberosDelegationTokenAuthenticator}} allows unconditional fallback to
{{PseudoDelegationTokenAuthenticator}}. Wouldn't that allow for a malicious
endpoint to unilaterally choose to use simple auth, and in so doing defeat the
bidirectional auth capability of Kerberos?
# Seems a little goofy to me to be implementing signature secret file reading
in {{HttpFSAuthenticationFilter}}. Will that not be something that other
authentication filters will need as well? For that matter, seems a bit curious
to be reading that file one character at a time, but probably doesn't matter
since I think we expect that file to be a few kb at most.
Nits:
# Extra space here: {{this( authenticator, null);}}
# Typo "usin": {{Creates an <code>DelegationTokenAuthenticatedURL</code> usin
gthe default}}
# Typo (in two places) "the a": {{Returns an authenticated {@link
HttpURLConnection}. If the a Delegation}}
# Typo - missing "f": {{with the type o the wrapped
<code>AuthenticationHandler</code>}}
# Probably should remove the "a" here (three places): {{This is useful for use
cases where a secrets}}
# I think the indentation is off in
{{DelegationTokenAuthenticator#cancelDelegationToken}}
# Remove "a" here: {{without triggering a http parameters...}}
> Refactor HTTP delegation support out of httpfs to common
> --------------------------------------------------------
>
> Key: HADOOP-10771
> URL: https://issues.apache.org/jira/browse/HADOOP-10771
> Project: Hadoop Common
> Issue Type: Improvement
> Components: security
> Affects Versions: 3.0.0
> Reporter: Alejandro Abdelnur
> Assignee: Alejandro Abdelnur
> Attachments: HADOOP-10771.patch, HADOOP-10771.sh
>
>
> HttpFS implements delegation token support in {{AuthenticationFilter}} &
> {{AuthenticationHandler}} subclasses.
> For HADOOP-10770 we need similar functionality for KMS.
> Not to duplicate code, we should refactor existing code to common.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
