[jira] Commented: (HADOOP-6419) Change RPC layer to support SASL based mutual authentication

["Philip Zeyliger (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/HADOOP-6419?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12828011#action_12828011 ] 

Philip Zeyliger commented on HADOOP-6419:
-----------------------------------------

Hi,

Coming late to the game here.  I've been reading SASL RFCs (oy), so wanted to
take a look (for my own education and to advise Avro-SASL) at how one
implements it in Java.  I've got some comments and quite a few questions;
thanks in advance for your patience.

bq. public static enum AuthMethod 

RFC4422 calls these "mechanisms".  Admittedly,
in their land, mechanisms are NUL-terminated 
C-strings, and not enums.  I think it's fine
that we restrict the implementation to support
only one mechanism per protocol.

{noformat}
          saslClient = Sasl.createSaslClient(
          new String[] { SaslRpcServer.SASL_DIGEST_MECH }, null, null,
          SaslRpcServer.SASL_DEFAULT_REALM, SaslRpcServer.SASL_PROPS,
          new SaslClientCallbackHandler(token));
{noformat}
Instead of SaslRpcServer.SASL_DIGEST_MECH, you could
put the constant inside the AuthMethod enum, which you
have available here.

{noformat}
      saslClient = Sasl.createSaslClient(
          new String[] { SaslRpcServer.SASL_KERBEROS_MECH }, null, names[0],
          names[1], SaslRpcServer.SASL_PROPS, null);
{noformat}
It's pretty unintuitive that you had to pass the two
parts of the server's kerberos identity as the protocol
and server parameters here.  How did you figure that out?
I couldn't find any documentation for it, outside of the
source for GssKrb5Client.java.

bq. useSasl = false

Am I right in guessing that the reason we don't use
the "plain" mechanism for SASL is that we wish
to avoid SASL's extra framing?

bq. TokenSelector, TokenIdentifier

Could you explain (perhaps in the javadoc) why
you need both of these classes.  The implementation
of TestTokenSelector suggests to me that all TokenSelectors
are just going to compare (kind, service) Text objects,
and that's all.  Are there likely to be different
types of TokenSelectors?  Likewise, when would TokenIdenitifier
not just be (kind, username)?  

I think you're going for some type safety by using generics
there, but I'm missing what it's buying you.

bq.         byte[] token = saslServer.unwrap(saslToken, 0, saslToken.length);
bq.        processUnwrappedData(token);

At this point, token's not a token, but merely data, no?
You might rename that variable, to avoid confusion.
(I was confused.)


bq. setupResponse(): if (call.connection.useSasl) {

The code here would be clearer if you extracted
this into a "void wrapWithSasl(response)" method.
I missed that response was being re-used,
and was scratching my head for a while :)

bq. SaslInputStream

Isn't it totally bizarre that the SaslServer javaDoc talks
about "SecureInputStream", and there doesn't seem to be 
such a thin?  I think they must have meant
com.sun.jndi.ldap.sasl.SaslInputStream, which seems
to be part of OpenJDK and GPL'd, so never us mind.

{noformat}
  @KerberosInfo(
      serverPrincipalKey = SERVER_PRINCIPAL_KEY
      )
  @TokenInfo(
      identifierClass = TestTokenIdentifier.class,
      secretManagerClass = TestTokenSecretManager.class,
      selectorClass = TestTokenSelector.class
      )
{noformat}
With my "how-much-work-is-this-going-to-be-to-port-to-AVRO" hat on,
I've been thinking about whether these annotations
should be on the protocol (like they are), or just part of 
RPC.getProxy()/RPC.getServer().  I think they're fine
as annotations: Hadoop's protocols are closely
tied with the type of authentication they expect.

That said: there's a lot of implicit information
being passed in this annotation (and Client.java is
correspondingly complicated).  Could this just be
@TokenRpcAuth(enum) and @KerberosRpcAuth(SERVER_PRINCIPLE_KEY)?
I can't imagine a case where one of the three parameters for
the @TokenInfo annotation wouldn't imply the other two, but 
I might be missing something.

I'll also point out that your test works by a little bit of trickery:
I initially thought that if @TokenInfo is specified, Client.java
would use that.  Turns out it will fall back to Kerberos if 
the token's not present.  This is all fine; it was just a bit
complicated to figure out how your test tries to cover both cases.
(It wouldn't be crazy to assert that only one non-plain authentication
type is supported, but maybe there are protocols where you could
do either...)

bq. static void testKerberosRpc

I take it that this is a main() test and not a @Test test
because Kerberos doesn't exist on Hudson?  Might be appropriate
to call that out.

bq. SaslInputStream/SaslOutputStream

Should these have tests?


Thanks for your patience!

-- Philip


> Change RPC layer to support SASL based mutual authentication
> ------------------------------------------------------------
>
>                 Key: HADOOP-6419
>                 URL: https://issues.apache.org/jira/browse/HADOOP-6419
>             Project: Hadoop Common
>          Issue Type: New Feature
>          Components: security
>            Reporter: Kan Zhang
>            Assignee: Kan Zhang
>         Attachments: c6419-26.patch, c6419-39.patch, c6419-45.patch, c6419-66.patch, c6419-67.patch, c6419-69.patch, c6419-70.patch
>
>
> The authentication mechanism to use will be SASL DIGEST-MD5 (see RFC-2222 and RFC-2831) or SASL GSSAPI/Kerberos. Since J2SE 5, Sun provides a SASL implementation by default. Both our delegation token and job token can be used as credentials for SASL DIGEST-MD5 authentication.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.