Hadoop Common
  1. Hadoop Common
  2. HADOOP-6419

Change RPC layer to support SASL based mutual authentication

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.21.0
    • Component/s: security
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      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.

      1. 6419-bp20-jobsubmitprotocol.patch
        0.9 kB
        Devaraj Das
      2. HADOOP-6419-0.20-15.patch
        139 kB
        Kan Zhang
      3. c6419-75.patch
        85 kB
        Kan Zhang
      4. c6419-73.patch
        82 kB
        Kan Zhang
      5. c6419-72.patch
        82 kB
        Kan Zhang
      6. c6419-70.patch
        82 kB
        Kan Zhang
      7. c6419-69.patch
        80 kB
        Kan Zhang
      8. c6419-67.patch
        81 kB
        Kan Zhang
      9. c6419-66.patch
        81 kB
        Kan Zhang
      10. c6419-45.patch
        74 kB
        Kan Zhang
      11. c6419-39.patch
        69 kB
        Kan Zhang
      12. c6419-26.patch
        66 kB
        Kan Zhang

        Issue Links

          Activity

          Hide
          Kan Zhang added a comment -

          Attaching a preliminary patch for review. The added UGI methods are temporary (for testing) and will be removed once new UGI interface is in place.

          Show
          Kan Zhang added a comment - Attaching a preliminary patch for review. The added UGI methods are temporary (for testing) and will be removed once new UGI interface is in place.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12428970/c6419-26.patch
          against trunk revision 893666.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          -1 javac. The applied patch generated 163 javac compiler warnings (more than the trunk's current 161 warnings).

          -1 findbugs. The patch appears to introduce 2 new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/240/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/240/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/240/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/240/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12428970/c6419-26.patch against trunk revision 893666. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. -1 javac. The applied patch generated 163 javac compiler warnings (more than the trunk's current 161 warnings). -1 findbugs. The patch appears to introduce 2 new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/240/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/240/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/240/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/240/console This message is automatically generated.
          Hide
          Doug Cutting added a comment -

          We hope to soon replace WritableRpcEngine with something that's not so Java-specific, so that we can easily write RPC clients and servers in languages besides Java. The existing AvroRpcEngine just tunnels Avro-format requests over WritableRpcEngine, and is not the language-indepdendent client-server implementation we'll probably eventually need. It would make this future evolution easier if the Sasl code were as cleanly separated from the RPC engine implementation as possible.

          In the current patch large chunks of Sasl-specific code are added to Client.java and Server.java, which implement WritableRpcEngine. Would it instead be possible to move most of this to separate Sasl-specific files, to minimize the changes to Client.java and Server.java, and maximize the (hopefully) reusable Sasl code?

          Show
          Doug Cutting added a comment - We hope to soon replace WritableRpcEngine with something that's not so Java-specific, so that we can easily write RPC clients and servers in languages besides Java. The existing AvroRpcEngine just tunnels Avro-format requests over WritableRpcEngine, and is not the language-indepdendent client-server implementation we'll probably eventually need. It would make this future evolution easier if the Sasl code were as cleanly separated from the RPC engine implementation as possible. In the current patch large chunks of Sasl-specific code are added to Client.java and Server.java, which implement WritableRpcEngine. Would it instead be possible to move most of this to separate Sasl-specific files, to minimize the changes to Client.java and Server.java, and maximize the (hopefully) reusable Sasl code?
          Hide
          Owen O'Malley added a comment -

          The patch actually does a pretty good job of staying out of the Server and Client files. It does need to replace the transport and therefore it must involve changes. I suspect that all of the SASL code could be pulled into a SocketFactory and ServerSocketFactory and that would minimize the coupling.

          Show
          Owen O'Malley added a comment - The patch actually does a pretty good job of staying out of the Server and Client files. It does need to replace the transport and therefore it must involve changes. I suspect that all of the SASL code could be pulled into a SocketFactory and ServerSocketFactory and that would minimize the coupling.
          Hide
          Doug Cutting added a comment -

          > I suspect that all of the SASL code could be pulled into a SocketFactory and ServerSocketFactory [ ... ]

          That would be great, if possible.

          Show
          Doug Cutting added a comment - > I suspect that all of the SASL code could be pulled into a SocketFactory and ServerSocketFactory [ ... ] That would be great, if possible.
          Hide
          Kan Zhang added a comment -

          > I suspect that all of the SASL code could be pulled into a SocketFactory and ServerSocketFactory [ ... ]

          It would be great if we could do so. I think it's possible on the client side. However, on the server side, since we use non-blocking IO and the RPC listener thread does all the connection setup and reading, it won't save much even if we try to move SASL related code to a sperate file since the listener thread still has to be aware of whether the incoming connection is a SASL connection and act accordingly.

          Show
          Kan Zhang added a comment - > I suspect that all of the SASL code could be pulled into a SocketFactory and ServerSocketFactory [ ... ] It would be great if we could do so. I think it's possible on the client side. However, on the server side, since we use non-blocking IO and the RPC listener thread does all the connection setup and reading, it won't save much even if we try to move SASL related code to a sperate file since the listener thread still has to be aware of whether the incoming connection is a SASL connection and act accordingly.
          Hide
          Kan Zhang added a comment -

          My current plan is to leave the server side as it is, and upload a new patch that hides the client side SASL code into a custom socket.

          Show
          Kan Zhang added a comment - My current plan is to leave the server side as it is, and upload a new patch that hides the client side SASL code into a custom socket.
          Hide
          Doug Cutting added a comment -

          > My current plan is to leave the server side as it is, and upload a new patch that hides the client side SASL code into a custom socket.

          +1

          Show
          Doug Cutting added a comment - > My current plan is to leave the server side as it is, and upload a new patch that hides the client side SASL code into a custom socket. +1
          Hide
          Raghu Angadi added a comment -

          +1 for client side to start with.

          It is fairly straight fwd to extend a java.net.Socket through a SocketFactory.

          But to achieve the same for nio channels transparently requires "SocketChannelFactory" (and "ServerSocketChannelFactory", etc). I don't know of any working examples of such factories that create a custom socket channel that works transparently.

          I suspect, the reason is that the whole channel interface and implementation in Java is pretty complicated involves multiple classes interacting together. We might have to implement not just our own SocketChannel, but SelectorProvider, Select etc.

          Many frameworks handle these issues by providing their own i/o api and by adding support for pluggable protocols in a 'chain of control' pattern above the socket io layer.

          In our context, short term we could start with a simple i/o interface (connect, read, write, getChannelForSelect()) that would support pluggable protocol for client and server sides of RPC.. Ideally we would move to NIO framework like netty, but that would much larger effort.

          In summary, I don't think we can easily implement SocketChannel factories or is the recommended direction to proceed.

          Show
          Raghu Angadi added a comment - +1 for client side to start with. It is fairly straight fwd to extend a java.net.Socket through a SocketFactory. But to achieve the same for nio channels transparently requires "SocketChannelFactory" (and "ServerSocketChannelFactory", etc). I don't know of any working examples of such factories that create a custom socket channel that works transparently. I suspect, the reason is that the whole channel interface and implementation in Java is pretty complicated involves multiple classes interacting together. We might have to implement not just our own SocketChannel, but SelectorProvider, Select etc. Many frameworks handle these issues by providing their own i/o api and by adding support for pluggable protocols in a 'chain of control' pattern above the socket io layer. In our context, short term we could start with a simple i/o interface (connect, read, write, getChannelForSelect()) that would support pluggable protocol for client and server sides of RPC.. Ideally we would move to NIO framework like netty, but that would much larger effort. In summary, I don't think we can easily implement SocketChannel factories or is the recommended direction to proceed.
          Hide
          Doug Cutting added a comment -

          > Ideally we would move to NIO framework like netty, but that would much larger effort.

          I'd like to, in AVRO-341, define a standard, language-independent, secure RPC transport for Avro. In Java we can then either implement this from scratch, perhaps stealing code from ipc.Server, or we can try to implement it using a framework. If possible, a framework would be preferred, I think. Then we can port Hadoop's RPC to use this, building on HDFS-892. Does that sounds like a reasonable plan?

          Show
          Doug Cutting added a comment - > Ideally we would move to NIO framework like netty, but that would much larger effort. I'd like to, in AVRO-341 , define a standard, language-independent, secure RPC transport for Avro. In Java we can then either implement this from scratch, perhaps stealing code from ipc.Server, or we can try to implement it using a framework. If possible, a framework would be preferred, I think. Then we can port Hadoop's RPC to use this, building on HDFS-892 . Does that sounds like a reasonable plan?
          Hide
          Raghu Angadi added a comment -

          I think so. RPC implementation in Hadoop is well isolated from user code, making it some what easier to replace with it with another one. I am interested in implementing a Netty based RPC server (as prototype).. I am planning to spend more time on this this week. will check how it would fit in with AVRO-341 and HDFS-892.

          Show
          Raghu Angadi added a comment - I think so. RPC implementation in Hadoop is well isolated from user code, making it some what easier to replace with it with another one. I am interested in implementing a Netty based RPC server (as prototype).. I am planning to spend more time on this this week. will check how it would fit in with AVRO-341 and HDFS-892 .
          Hide
          Kan Zhang added a comment -

          > +1 for client side to start with.
          I was trying to re-factor the client side code. However, I feel it might not worth it under our current code structure. Firstly, since we obtain our sockets from socket channels, a custom socket has to be instantiated by wrapping an existing socket, which leads to a lot of boilerplate code. More importantly, we don't have a framework to plug in a security layer. One possibility is to make NetUtils class security aware. However, NetUtils isn't a good place since it's just a utility class consisting of all static methods. On the client side, SASL logic is already well captured in a single method initSASLContext(). I don't think polluting NetUtils would bring much benefit. The server side arguably needs more re-factoring. But NetUtils won't help there since it's only used on the client side. Hence, I suggest we leave factoring out security layer from Client and Server to a future date when there is a framework to work with.

          Attaching a new patch that 1) added a header element to RPC that specifies the authentication method to be used (or none). Part of existing header (ugi and protocol) will be sent after authentication and in protected form. 2) re-factored Server code to be more readable.

          Show
          Kan Zhang added a comment - > +1 for client side to start with. I was trying to re-factor the client side code. However, I feel it might not worth it under our current code structure. Firstly, since we obtain our sockets from socket channels, a custom socket has to be instantiated by wrapping an existing socket, which leads to a lot of boilerplate code. More importantly, we don't have a framework to plug in a security layer. One possibility is to make NetUtils class security aware. However, NetUtils isn't a good place since it's just a utility class consisting of all static methods. On the client side, SASL logic is already well captured in a single method initSASLContext(). I don't think polluting NetUtils would bring much benefit. The server side arguably needs more re-factoring. But NetUtils won't help there since it's only used on the client side. Hence, I suggest we leave factoring out security layer from Client and Server to a future date when there is a framework to work with. Attaching a new patch that 1) added a header element to RPC that specifies the authentication method to be used (or none). Part of existing header (ugi and protocol) will be sent after authentication and in protected form. 2) re-factored Server code to be more readable.
          Hide
          Doug Cutting added a comment -

          Looking at the most recent patch, it appear to me that:

          • AuthMethod and SaslDigestCallbackHandler could be independent of Server.java. The constructor for the latter would need to accept a secretManager. Can we please move these to a SaslRpcServer utility class?
          • similarly, SaslClientCallbackHandler, initSASLContext could be independent of Client.java, where the latter could return the saslClient instance. These might move to a SaslRpcClient utility class.

          We could refactor these out of Client.java and Server.java later, but in the meantime folks might inadvertantly take advantage of the fact that they're in the same file and make refactoring later more difficult. So I'd rather they were in separate classes now. The abstractions may not be perfect for other client and server implementations, but we can address that later, when we add such implementations. I'd just like to keep the authentication logic as independent from the RPC logic as possible.

          Show
          Doug Cutting added a comment - Looking at the most recent patch, it appear to me that: AuthMethod and SaslDigestCallbackHandler could be independent of Server.java. The constructor for the latter would need to accept a secretManager. Can we please move these to a SaslRpcServer utility class? similarly, SaslClientCallbackHandler, initSASLContext could be independent of Client.java, where the latter could return the saslClient instance. These might move to a SaslRpcClient utility class. We could refactor these out of Client.java and Server.java later, but in the meantime folks might inadvertantly take advantage of the fact that they're in the same file and make refactoring later more difficult. So I'd rather they were in separate classes now. The abstractions may not be perfect for other client and server implementations, but we can address that later, when we add such implementations. I'd just like to keep the authentication logic as independent from the RPC logic as possible.
          Hide
          Kan Zhang added a comment -

          Attached a new patch that takes into account Doug' suggestion. On the client side, all SASL logic is encapsulated in SaslRpcClient which implements SaslClient. However, similar things are harder to do for the server side since we use non-blocking IO.

          Show
          Kan Zhang added a comment - Attached a new patch that takes into account Doug' suggestion. On the client side, all SASL logic is encapsulated in SaslRpcClient which implements SaslClient. However, similar things are harder to do for the server side since we use non-blocking IO.
          Hide
          Doug Cutting added a comment -

          Kan, this encapsulation looks great. Thank you!

          Show
          Doug Cutting added a comment - Kan, this encapsulation looks great. Thank you!
          Hide
          Kan Zhang added a comment -

          uploading a new patch for review. This patch uses the new UGI interface done in HADOOP-6299.

          Show
          Kan Zhang added a comment - uploading a new patch for review. This patch uses the new UGI interface done in HADOOP-6299 .
          Hide
          Kan Zhang added a comment -

          The last patch also supports Kerberos authentication through the same SASL API.

          Show
          Kan Zhang added a comment - The last patch also supports Kerberos authentication through the same SASL API.
          Hide
          Kan Zhang added a comment -

          a new patch for review, which changes the Client to use UGI.getCurrentUser rather than ticket (a hack to get around h-6517).

          Show
          Kan Zhang added a comment - a new patch for review, which changes the Client to use UGI.getCurrentUser rather than ticket (a hack to get around h-6517).
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12431814/c6419-67.patch
          against trunk revision 904339.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 9 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/302/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/302/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/302/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/302/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12431814/c6419-67.patch against trunk revision 904339. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/302/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/302/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/302/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/302/console This message is automatically generated.
          Hide
          Owen O'Malley added a comment -

          Comments:
          1. Please keep the lines to 80 characters.
          2. Change the new testcase to Junit 4 annotations.
          3. TokenInfo needs more documentation on the intended purpose of the class.
          4. SaslRpcClient shouldn't implement SaslClient and forward the calls to the contained SaslClient.
          5. SaslOutputStream (and SaslInutStream).disposeSasl shouldn't throw away exceptions because in the non-error case exceptions could be important. The cases where disposeSasl is called while handling an exception should ignore IOErrors from dispose.

          I agree with Doug that you've done a great job of minimizing the impact on the RPC code.

          Show
          Owen O'Malley added a comment - Comments: 1. Please keep the lines to 80 characters. 2. Change the new testcase to Junit 4 annotations. 3. TokenInfo needs more documentation on the intended purpose of the class. 4. SaslRpcClient shouldn't implement SaslClient and forward the calls to the contained SaslClient. 5. SaslOutputStream (and SaslInutStream).disposeSasl shouldn't throw away exceptions because in the non-error case exceptions could be important. The cases where disposeSasl is called while handling an exception should ignore IOErrors from dispose. I agree with Doug that you've done a great job of minimizing the impact on the RPC code.
          Hide
          Kan Zhang added a comment -

          attaching a new patch that incorporates Owen's comments.

          Show
          Kan Zhang added a comment - attaching a new patch that incorporates Owen's comments.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12431875/c6419-69.patch
          against trunk revision 904862.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 9 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/306/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/306/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/306/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/306/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12431875/c6419-69.patch against trunk revision 904862. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/306/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/306/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/306/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/306/console This message is automatically generated.
          Hide
          Kan Zhang added a comment -

          uploading a new patch that separates out KeberosInfo from TokenInfo, since many protocols will use Kerberos authentication only (no tokens available to be used on them).

          Show
          Kan Zhang added a comment - uploading a new patch that separates out KeberosInfo from TokenInfo, since many protocols will use Kerberos authentication only (no tokens available to be used on them).
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12431897/c6419-70.patch
          against trunk revision 904862.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 9 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/308/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/308/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/308/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/308/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12431897/c6419-70.patch against trunk revision 904862. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/308/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/308/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/308/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/308/console This message is automatically generated.
          Hide
          Philip Zeyliger added a comment -

          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.

          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.

                    saslClient = Sasl.createSaslClient(
                    new String[] { SaslRpcServer.SASL_DIGEST_MECH }, null, null,
                    SaslRpcServer.SASL_DEFAULT_REALM, SaslRpcServer.SASL_PROPS,
                    new SaslClientCallbackHandler(token));
          

          Instead of SaslRpcServer.SASL_DIGEST_MECH, you could
          put the constant inside the AuthMethod enum, which you
          have available here.

                saslClient = Sasl.createSaslClient(
                    new String[] { SaslRpcServer.SASL_KERBEROS_MECH }, null, names[0],
                    names[1], SaslRpcServer.SASL_PROPS, null);
          

          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.

          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?

          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.

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

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

          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

          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.

            @KerberosInfo(
                serverPrincipalKey = SERVER_PRINCIPAL_KEY
                )
            @TokenInfo(
                identifierClass = TestTokenIdentifier.class,
                secretManagerClass = TestTokenSecretManager.class,
                selectorClass = TestTokenSelector.class
                )
          

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

          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.

          SaslInputStream/SaslOutputStream

          Should these have tests?

          Thanks for your patience!

          – Philip

          Show
          Philip Zeyliger added a comment - 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. 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. saslClient = Sasl.createSaslClient( new String[] { SaslRpcServer.SASL_DIGEST_MECH }, null, null, SaslRpcServer.SASL_DEFAULT_REALM, SaslRpcServer.SASL_PROPS, new SaslClientCallbackHandler(token)); Instead of SaslRpcServer.SASL_DIGEST_MECH, you could put the constant inside the AuthMethod enum, which you have available here. saslClient = Sasl.createSaslClient( new String[] { SaslRpcServer.SASL_KERBEROS_MECH }, null, names[0], names[1], SaslRpcServer.SASL_PROPS, null); 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. 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? 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. byte[] token = saslServer.unwrap(saslToken, 0, saslToken.length); 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.) 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 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. @KerberosInfo( serverPrincipalKey = SERVER_PRINCIPAL_KEY ) @TokenInfo( identifierClass = TestTokenIdentifier.class, secretManagerClass = TestTokenSecretManager.class, selectorClass = TestTokenSelector.class ) 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...) 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. SaslInputStream/SaslOutputStream Should these have tests? Thanks for your patience! – Philip
          Hide
          Owen O'Malley added a comment -

          I can answer some of the Token questions.

          TokenIdentifier is the information that is specific to that kind of token. In the case of the HDFS delegation token, it is things like the user's name, when it was granted, the maximum lifetime, etc. For job token's the identifier is the job id. The other part of the token, which is the password is the hmac hash of the serialized identifier combined with the secret key.

          TokenSelector is the class that selects the token (from the set that the user has) to use for this particular rpc connection. For example, map 0 of my job will have a JobToken and a HDFS DelegationToken. The JobToken will be used to connect to the TaskTracker to ask for work and the DelegationToken will be used to connect to the NameNode. Also note that a single job may talk to multiple NameNodes and will need a different delegation token for each.

          Show
          Owen O'Malley added a comment - I can answer some of the Token questions. TokenIdentifier is the information that is specific to that kind of token. In the case of the HDFS delegation token, it is things like the user's name, when it was granted, the maximum lifetime, etc. For job token's the identifier is the job id. The other part of the token, which is the password is the hmac hash of the serialized identifier combined with the secret key. TokenSelector is the class that selects the token (from the set that the user has) to use for this particular rpc connection. For example, map 0 of my job will have a JobToken and a HDFS DelegationToken. The JobToken will be used to connect to the TaskTracker to ask for work and the DelegationToken will be used to connect to the NameNode. Also note that a single job may talk to multiple NameNodes and will need a different delegation token for each.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12434434/c6419-72.patch
          against trunk revision 904975.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 9 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/310/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/310/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/310/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/310/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12434434/c6419-72.patch against trunk revision 904975. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/310/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/310/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/310/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/310/console This message is automatically generated.
          Hide
          Kan Zhang added a comment -

          Hi Philip, I uploaded a new patch that incorporates your comments. Specifically,

          you could put the constant inside the AuthMethod enum

          Done.

          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?

          Same as you did, from the source code. If we use Java GSSAPI directly, it allows
          you to specify the full Kerberos principal name as a single parameter, which makes
          sense. However, Java Sasl splits it into 2 parts and the second hostname part can't
          be null (if I remember corerctly).

          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?

          Yes.

          Are there likely to be different types of TokenSelectors?

          Yes, we currently have Delegation Token (for authenticating to NN) and Job Token
          (for Task to authenticate to TaskTracker). We may have more types of tokens in the
          future. A token is just a shared secret key known to the two authenticating parties. How
          to generate and distribute the keys may be different for different protocols, but the way
          the keys are used by the authentication layer is the same, which is what has been
          abstracted out to the RPC layer.

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

          It doesn't buy you much (mainly for the benefit of developers who read the source code)
          as the type information are erased by the compiler.

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

          Done.

          The code here would be clearer if you extracted this into a "void wrapWithSasl(response)" method.

          Done.

          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.

          Yes, and we don't want to be dependent on com.sun packages.

          Could this just be @TokenRpcAuth(enum) and @KerberosRpcAuth(SERVER_PRINCIPLE_KEY)?

          TokenInfo has been changed to single element annotation. However, we can't
          use enum. We need to specify the type of TokenSelector used, as there may be
          more than one type. KeberosInfo's only element has been changed to "value",
          so that one can use the short form you suggested above.

          Turns out it will fall back to Kerberos if the token's not present.

          The reason for this is that on some protocols, both Kerberos and token-based
          authentication will be supported. And when a client has both credentials available,
          we favor token over Kerberos since it's cheaper to use tokens.

          I take it that this is a main() test and not a @Test test because Kerberos doesn't exist on Hudson?

          Yes, we don't have Kerberos infrastructure (KDC and keys, etc) on Hudson.
          This test is currently ran manually with a local Kerberos setup.

          SaslInputStream/SaslOutputStream

          They are tested by TestSaslRPC.

          Thank you for your comments. Let me know if you have any further comments.

          Show
          Kan Zhang added a comment - Hi Philip, I uploaded a new patch that incorporates your comments. Specifically, you could put the constant inside the AuthMethod enum Done. 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? Same as you did, from the source code. If we use Java GSSAPI directly, it allows you to specify the full Kerberos principal name as a single parameter, which makes sense. However, Java Sasl splits it into 2 parts and the second hostname part can't be null (if I remember corerctly). 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? Yes. Are there likely to be different types of TokenSelectors? Yes, we currently have Delegation Token (for authenticating to NN) and Job Token (for Task to authenticate to TaskTracker). We may have more types of tokens in the future. A token is just a shared secret key known to the two authenticating parties. How to generate and distribute the keys may be different for different protocols, but the way the keys are used by the authentication layer is the same, which is what has been abstracted out to the RPC layer. I think you're going for some type safety by using generics there, but I'm missing what it's buying you. It doesn't buy you much (mainly for the benefit of developers who read the source code) as the type information are erased by the compiler. At this point, token's not a token, but merely data, no? You might rename that variable, to avoid confusion. Done. The code here would be clearer if you extracted this into a "void wrapWithSasl(response)" method. Done. 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. Yes, and we don't want to be dependent on com.sun packages. Could this just be @TokenRpcAuth(enum) and @KerberosRpcAuth(SERVER_PRINCIPLE_KEY)? TokenInfo has been changed to single element annotation. However, we can't use enum. We need to specify the type of TokenSelector used, as there may be more than one type. KeberosInfo's only element has been changed to "value", so that one can use the short form you suggested above. Turns out it will fall back to Kerberos if the token's not present. The reason for this is that on some protocols, both Kerberos and token-based authentication will be supported. And when a client has both credentials available, we favor token over Kerberos since it's cheaper to use tokens. I take it that this is a main() test and not a @Test test because Kerberos doesn't exist on Hudson? Yes, we don't have Kerberos infrastructure (KDC and keys, etc) on Hudson. This test is currently ran manually with a local Kerberos setup. SaslInputStream/SaslOutputStream They are tested by TestSaslRPC. Thank you for your comments. Let me know if you have any further comments.
          Hide
          Allen Wittenauer added a comment -

          I think it's fine that we restrict the implementation to support only one mechanism per protocol.

          Hmm. Are we worried this is short-sighted?

          Show
          Allen Wittenauer added a comment - I think it's fine that we restrict the implementation to support only one mechanism per protocol. Hmm. Are we worried this is short-sighted?
          Hide
          Kan Zhang added a comment -

          Hmm. Are we worried this is short-sighted?

          Allen, I think there might be some terminology mismatch here. I take Philip's "protocol" to mean "authentication method". For a particular method, there is usually only one mechanism that implements that method. However, we do support multiple authentication methods on the same Hadoop protocol. Did I answer your question?

          Show
          Kan Zhang added a comment - Hmm. Are we worried this is short-sighted? Allen, I think there might be some terminology mismatch here. I take Philip's "protocol" to mean "authentication method". For a particular method, there is usually only one mechanism that implements that method. However, we do support multiple authentication methods on the same Hadoop protocol. Did I answer your question?
          Hide
          Allen Wittenauer added a comment -

          Yes, thanks.

          Show
          Allen Wittenauer added a comment - Yes, thanks.
          Hide
          Philip Zeyliger added a comment -

          Thanks, Kan and Owen, for answering my questions.

          Kan, I looked through the new patch very briefly. Thanks for addressing my concerns!

          – Philip

          Show
          Philip Zeyliger added a comment - Thanks, Kan and Owen, for answering my questions. Kan, I looked through the new patch very briefly. Thanks for addressing my concerns! – Philip
          Hide
          Kan Zhang added a comment -

          a trivial refactoring to avoid creating unnecessary "username" string.

          Show
          Kan Zhang added a comment - a trivial refactoring to avoid creating unnecessary "username" string.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12434477/c6419-73.patch
          against trunk revision 904975.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 9 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/311/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/311/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/311/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/311/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12434477/c6419-73.patch against trunk revision 904975. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/311/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/311/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/311/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/311/console This message is automatically generated.
          Hide
          Owen O'Malley added a comment -

          Did you intend to leave all of the logging levels at all in your TestSaslRpc or was that for your own debugging?

          I'd suggest that disposeSasl set the saslClient (or saslServer) to null after it has been disposed, unless you are sure that disposing of it a second time is ignored.

          A quibble is that your regex for splitting principal names would be easier to read as "[/@]" instead of "(/|@)". It should however, be pulled out into a utility function, since you do it a couple of places in the code.

          Does it matter that we don't allow server principals like "a@B.ORG" and insist on "a/c@B.ORG"? Does SASL insist on it? It is certainly the standard practice, but we are forcing it as a requirement.

          Instead of throwing IOException with an authorization failure, please use hadoop.security.AccessControlException.

          Show
          Owen O'Malley added a comment - Did you intend to leave all of the logging levels at all in your TestSaslRpc or was that for your own debugging? I'd suggest that disposeSasl set the saslClient (or saslServer) to null after it has been disposed, unless you are sure that disposing of it a second time is ignored. A quibble is that your regex for splitting principal names would be easier to read as " [/@] " instead of "(/|@)". It should however, be pulled out into a utility function, since you do it a couple of places in the code. Does it matter that we don't allow server principals like "a@B.ORG" and insist on "a/c@B.ORG"? Does SASL insist on it? It is certainly the standard practice, but we are forcing it as a requirement. Instead of throwing IOException with an authorization failure, please use hadoop.security.AccessControlException.
          Hide
          Allen Wittenauer added a comment -

          Does it matter that we don't allow server principals like "a@B.ORG" and insist on "a/c@B.ORG"? Does SASL insist on it? It is certainly the standard practice, but we are forcing it as a requirement.

          IMO, I do not think Hadoop should force it as a requirement.

          Show
          Allen Wittenauer added a comment - Does it matter that we don't allow server principals like "a@B.ORG" and insist on "a/c@B.ORG"? Does SASL insist on it? It is certainly the standard practice, but we are forcing it as a requirement. IMO, I do not think Hadoop should force it as a requirement.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12434590/c6419-75.patch
          against trunk revision 904975.

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 9 new or modified tests.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed core unit tests.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/316/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/316/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/316/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/316/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12434590/c6419-75.patch against trunk revision 904975. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 9 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/316/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/316/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/316/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/316/console This message is automatically generated.
          Hide
          Kan Zhang added a comment -

          attached a new patch that incorporated Owen's comments.

          Did you intend to leave all of the logging levels at all in your TestSaslRpc or was that for your own debugging?

          I intend to leave the logging levels on for that test. It's helpful for debugging.

          unless you are sure that disposing of it a second time is ignored.

          Java SASL API says dispose() is idempotent.

          A quibble is that your regex for splitting principal names would be easier to read as "[/@]" instead of "(/|@)". It should however, be pulled out into a utility function, since you do it a couple of places in the code.

          Done.

          Does it matter that we don't allow server principals like "a@B.ORG" and insist on "a/c@B.ORG"? Does SASL insist on it? It is certainly the standard practice, but we are forcing it as a requirement.

          When I tried to call Java SASL API with serverName parameter set to null or "", I got library exceptions. I think it's better we throw an exception with a meaningful message, rather than letting the library throw ArrayIndexOutOfBoundsException, etc. If we prefer to let library deal with it, let me know and I can remove the checking.

          Instead of throwing IOException with an authorization failure, please use hadoop.security.AccessControlException.

          Done.

          Show
          Kan Zhang added a comment - attached a new patch that incorporated Owen's comments. Did you intend to leave all of the logging levels at all in your TestSaslRpc or was that for your own debugging? I intend to leave the logging levels on for that test. It's helpful for debugging. unless you are sure that disposing of it a second time is ignored. Java SASL API says dispose() is idempotent. A quibble is that your regex for splitting principal names would be easier to read as " [/@] " instead of "(/|@)". It should however, be pulled out into a utility function, since you do it a couple of places in the code. Done. Does it matter that we don't allow server principals like "a@B.ORG" and insist on "a/c@B.ORG"? Does SASL insist on it? It is certainly the standard practice, but we are forcing it as a requirement. When I tried to call Java SASL API with serverName parameter set to null or "", I got library exceptions. I think it's better we throw an exception with a meaningful message, rather than letting the library throw ArrayIndexOutOfBoundsException, etc. If we prefer to let library deal with it, let me know and I can remove the checking. Instead of throwing IOException with an authorization failure, please use hadoop.security.AccessControlException. Done.
          Hide
          Devaraj Das added a comment -

          Owen offline said he is okay with committing the patch as it stands now. I am going to commit this. Any objections?

          Show
          Devaraj Das added a comment - Owen offline said he is okay with committing the patch as it stands now. I am going to commit this. Any objections?
          Hide
          Devaraj Das added a comment -

          I just committed this. Thanks, Kan!

          Show
          Devaraj Das added a comment - I just committed this. Thanks, Kan!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #155 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/155/)
          . Adds SASL based authentication to RPC. Contributed by Kan Zhang.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #155 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk-Commit/155/ ) . Adds SASL based authentication to RPC. Contributed by Kan Zhang.
          Hide
          Todd Lipcon added a comment -

          This seems to have broken the mapreduce trunk build.

          [javac] Compiling 424 source files to /home/todd/git/hadoop-mapreduce/build/classes
          [javac] /home/todd/git/hadoop-mapreduce/src/java/org/apache/hadoop/mapreduce/security/token/JobTokenSecretManager.java:34: org.apache.hadoop.mapreduce.security.token.JobTokenSecretManager is not abstract and does not override abstract method createIdentifier() in org.apache.hadoop.security.token.SecretManager
          [javac] public class JobTokenSecretManager extends SecretManager<JobTokenIdentifier> {
          [javac] ^
          [javac] /home/todd/git/hadoop-mapreduce/src/java/org/apache/hadoop/mapreduce/security/token/JobTokenIdentifier.java:33: org.apache.hadoop.mapreduce.security.token.JobTokenIdentifier is not abstract and does not override abstract method getUsername() in org.apache.hadoop.security.token.TokenIdentifier
          [javac] public class JobTokenIdentifier extends TokenIdentifier {
          [javac] ^
          [javac] Note: Some input files use or override a deprecated API.
          [javac] Note: Recompile with -Xlint:deprecation for details.
          [javac] 2 errors

          Show
          Todd Lipcon added a comment - This seems to have broken the mapreduce trunk build. [javac] Compiling 424 source files to /home/todd/git/hadoop-mapreduce/build/classes [javac] /home/todd/git/hadoop-mapreduce/src/java/org/apache/hadoop/mapreduce/security/token/JobTokenSecretManager.java:34: org.apache.hadoop.mapreduce.security.token.JobTokenSecretManager is not abstract and does not override abstract method createIdentifier() in org.apache.hadoop.security.token.SecretManager [javac] public class JobTokenSecretManager extends SecretManager<JobTokenIdentifier> { [javac] ^ [javac] /home/todd/git/hadoop-mapreduce/src/java/org/apache/hadoop/mapreduce/security/token/JobTokenIdentifier.java:33: org.apache.hadoop.mapreduce.security.token.JobTokenIdentifier is not abstract and does not override abstract method getUsername() in org.apache.hadoop.security.token.TokenIdentifier [javac] public class JobTokenIdentifier extends TokenIdentifier { [javac] ^ [javac] Note: Some input files use or override a deprecated API. [javac] Note: Recompile with -Xlint:deprecation for details. [javac] 2 errors
          Hide
          Todd Lipcon added a comment -

          And HDFS as well. In the future if we're going to add patches that break the dependent builds, is it possible to either design the patches in such a way that they can be staged in? eg the abstract methods here could have been temporarily non-abstract and throw RuntimeExceptions.

          Show
          Todd Lipcon added a comment - And HDFS as well. In the future if we're going to add patches that break the dependent builds, is it possible to either design the patches in such a way that they can be staged in? eg the abstract methods here could have been temporarily non-abstract and throw RuntimeExceptions.
          Hide
          Jakob Homan added a comment -

          The patches were submitted in relatively rapid exception, but with the expectation that new jars would be submitted in time for dependencies to be fixed. It's a temporary problem until the new jars are generated by Hudson. Once the new jar is available for the build, the method is available and all is well.

          Show
          Jakob Homan added a comment - The patches were submitted in relatively rapid exception, but with the expectation that new jars would be submitted in time for dependencies to be fixed. It's a temporary problem until the new jars are generated by Hudson. Once the new jar is available for the build, the method is available and all is well.
          Hide
          Todd Lipcon added a comment -

          Jakob: it's the other way around - those other patches haven't been committed, or even reviewed best I can tell (unless the review was off-JIRA). Until they're committed, the dependent builds are broken, regardless of jars.

          Show
          Todd Lipcon added a comment - Jakob: it's the other way around - those other patches haven't been committed, or even reviewed best I can tell (unless the review was off-JIRA). Until they're committed, the dependent builds are broken, regardless of jars.
          Hide
          Devaraj Das added a comment -

          Todd, the patches have been reviewed off jira. I will commit the HDFS/MR patches in the next 10 minutes.

          Show
          Devaraj Das added a comment - Todd, the patches have been reviewed off jira. I will commit the HDFS/MR patches in the next 10 minutes.
          Hide
          Tom White added a comment -

          > the patches have been reviewed off jira

          Devaraj, shouldn't reviews be happening on JIRA, so that others can see them?

          Show
          Tom White added a comment - > the patches have been reviewed off jira Devaraj, shouldn't reviews be happening on JIRA, so that others can see them?
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #241 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/241/)
          . Adds SASL based authentication to RPC. Contributed by Kan Zhang.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #241 (See http://hudson.zones.apache.org/hudson/job/Hadoop-Common-trunk/241/ ) . Adds SASL based authentication to RPC. Contributed by Kan Zhang.
          Hide
          Devaraj Das added a comment -

          Devaraj, shouldn't reviews be happening on JIRA, so that others can see them?

          Tom, that's a fair point. But in these specific cases, there were no specific review comments, and hence nothing was posted on the associated JIRAs.

          Show
          Devaraj Das added a comment - Devaraj, shouldn't reviews be happening on JIRA, so that others can see them? Tom, that's a fair point. But in these specific cases, there were no specific review comments, and hence nothing was posted on the associated JIRAs.
          Hide
          Aaron Kimball added a comment -

          It would still be good to get reassurance that someone else was looking at the issue, even if the review is just a "+1 patch looks good"

          Show
          Aaron Kimball added a comment - It would still be good to get reassurance that someone else was looking at the issue, even if the review is just a "+1 patch looks good"
          Hide
          Doug Cutting added a comment -

          > It would still be good to get reassurance that someone else was looking at the issue, even if the review is just a "+1 patch looks good"

          Yes, that is project policy. A committer who is not the patch author should provide a +1 comment before a patch is committed.

          Show
          Doug Cutting added a comment - > It would still be good to get reassurance that someone else was looking at the issue, even if the review is just a "+1 patch looks good" Yes, that is project policy. A committer who is not the patch author should provide a +1 comment before a patch is committed.
          Hide
          Kan Zhang added a comment -

          attaching a patch for 0.20

          Show
          Kan Zhang added a comment - attaching a patch for 0.20
          Hide
          Kan Zhang added a comment -

          adding a new patch for 0.20 that removes a commented-out line in FSNamesystem.java

          Show
          Kan Zhang added a comment - adding a new patch for 0.20 that removes a commented-out line in FSNamesystem.java
          Hide
          Devaraj Das added a comment -

          A bugfix for the backported patch. Not for commit here.

          Show
          Devaraj Das added a comment - A bugfix for the backported patch. Not for commit here.

            People

            • Assignee:
              Kan Zhang
              Reporter:
              Kan Zhang
            • Votes:
              0 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development