Hadoop Common
  1. Hadoop Common
  2. HADOOP-6907

Rpc client doesn't use the per-connection conf to figure out server's Kerberos principal

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.20.203.0, 0.22.0
    • Component/s: ipc, security
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Currently, RPC client caches the conf that was passed in to its constructor and uses that same conf (or values obtained from it) for every connection it sets up. This is not sufficient for security since each connection needs to figure out server's Kerberos principal on a per-connection basis. It's not reasonable to expect the first conf used by a user to contain all the Kerberos principals that her future connections will ever need. Or worse, if her first conf contains an incorrect principal name, it will prevent the user from connecting to the server even if she later on passes in a correct conf on retry (by calling RPC.getProxy()).

      1. c6907-Y20S.1xx.05.patch
        27 kB
        Kan Zhang
      2. c6907-18.patch
        28 kB
        Kan Zhang
      3. c6907-16.patch
        23 kB
        Kan Zhang
      4. c6907-15.patch
        23 kB
        Kan Zhang
      5. c6907-12.patch
        22 kB
        Kan Zhang

        Issue Links

          Activity

          Hide
          Owen O'Malley added a comment -

          Closing for 0.20.203.0

          Show
          Owen O'Malley added a comment - Closing for 0.20.203.0
          Hide
          Matt Foley added a comment -

          This was committed to 0.20-security branch on Mar 4, just before 20.203 was branched off it. Thus it is in 20.203 and all future releases from 0.20-security sustaining.

          Show
          Matt Foley added a comment - This was committed to 0.20-security branch on Mar 4, just before 20.203 was branched off it. Thus it is in 20.203 and all future releases from 0.20-security sustaining.
          Hide
          Kan Zhang added a comment -

          Attaching a patch for Yahoo internal Y20S branch (includes HADOOP-6907, HADOOP-6938 and HADOOP-6905). Not for commit.

          Show
          Kan Zhang added a comment - Attaching a patch for Yahoo internal Y20S branch (includes HADOOP-6907 , HADOOP-6938 and HADOOP-6905 ). Not for commit.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #441 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/441/)
          HADOOP-6907. Rpc client doesn't use the per-connection conf to figure out server's Kerberos principal. Contributed by Kan Zhang.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #441 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk/441/ ) HADOOP-6907 . Rpc client doesn't use the per-connection conf to figure out server's Kerberos principal. Contributed by Kan Zhang.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #371 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/371/)
          HADOOP-6907. Rpc client doesn't use the per-connection conf to figure out server's Kerberos principal. Contributed by Kan Zhang.

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #371 (See https://hudson.apache.org/hudson/job/Hadoop-Common-trunk-Commit/371/ ) HADOOP-6907 . Rpc client doesn't use the per-connection conf to figure out server's Kerberos principal. Contributed by Kan Zhang.
          Hide
          Hairong Kuang added a comment -

          I've just committed this. Thanks, Kan!

          Show
          Hairong Kuang added a comment - I've just committed this. Thanks, Kan!
          Hide
          Hairong Kuang added a comment -

          +1. The patch looks good to me.

          Show
          Hairong Kuang added a comment - +1. The patch looks good to me.
          Hide
          Kan Zhang added a comment -

          The javadoc warnings are unrelated to this patch.

          Show
          Kan Zhang added a comment - The javadoc warnings are unrelated to this patch.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12453460/c6907-18.patch
          against trunk revision 991038.

          +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 appears to have generated 1 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.

          +1 system tests framework. The patch passed system tests framework compile.

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/665/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/665/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/665/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/665/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/12453460/c6907-18.patch against trunk revision 991038. +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 appears to have generated 1 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. +1 system tests framework. The patch passed system tests framework compile. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/665/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/665/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/665/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/665/console This message is automatically generated.
          Hide
          Kan Zhang added a comment -

          I guess it was due to newly deprecated methods. Uploading a new patch.

          Show
          Kan Zhang added a comment - I guess it was due to newly deprecated methods. Uploading a new patch.
          Hide
          Kan Zhang added a comment -

          The 6 javadoc warnings are from SecurityUtil.java and KerberosName.java and not related to this patch. The number of javac warnings from compile-core-classes stayed at 15, I don't know why test-patch reported there is an increase of javac warnings.

          Show
          Kan Zhang added a comment - The 6 javadoc warnings are from SecurityUtil.java and KerberosName.java and not related to this patch. The number of javac warnings from compile-core-classes stayed at 15, I don't know why test-patch reported there is an increase of javac warnings.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12453285/c6907-16.patch
          against trunk revision 989999.

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

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

          -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

          -1 javac. The applied patch generated 1025 javac compiler warnings (more than the trunk's current 1017 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/664/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/664/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/664/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/664/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/12453285/c6907-16.patch against trunk revision 989999. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 warning messages. -1 javac. The applied patch generated 1025 javac compiler warnings (more than the trunk's current 1017 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/664/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/664/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/664/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h4.grid.sp2.yahoo.net/664/console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12453285/c6907-16.patch
          against trunk revision 989999.

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

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

          -1 javadoc. The javadoc tool appears to have generated 1 warning messages.

          -1 javac. The applied patch generated 1025 javac compiler warnings (more than the trunk's current 1017 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-h9.grid.sp2.yahoo.net/41/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h9.grid.sp2.yahoo.net/41/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h9.grid.sp2.yahoo.net/41/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h9.grid.sp2.yahoo.net/41/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/12453285/c6907-16.patch against trunk revision 989999. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 javadoc. The javadoc tool appears to have generated 1 warning messages. -1 javac. The applied patch generated 1025 javac compiler warnings (more than the trunk's current 1017 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-h9.grid.sp2.yahoo.net/41/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h9.grid.sp2.yahoo.net/41/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h9.grid.sp2.yahoo.net/41/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h9.grid.sp2.yahoo.net/41/console This message is automatically generated.
          Hide
          Kan Zhang added a comment -

          an updated patch for current trunk.

          Show
          Kan Zhang added a comment - an updated patch for current trunk.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12452346/c6907-15.patch
          against trunk revision 989999.

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

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

          -1 patch. The patch command could not apply the patch.

          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h9.grid.sp2.yahoo.net/39/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/12452346/c6907-15.patch against trunk revision 989999. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-h9.grid.sp2.yahoo.net/39/console This message is automatically generated.
          Hide
          Kan Zhang added a comment -

          new patch addressing Jakob's comments. My equals() and hashcode() implementations are the same as Eclipse-provided defaults. I see Eclipse's hashcode() method is easier to read. So I coupled it with my own equals().

          Show
          Kan Zhang added a comment - new patch addressing Jakob's comments. My equals() and hashcode() implementations are the same as Eclipse-provided defaults. I see Eclipse's hashcode() method is easier to read. So I coupled it with my own equals().
          Hide
          Jakob Homan added a comment -

          Canceling patch post-review.

          Show
          Jakob Homan added a comment - Canceling patch post-review.
          Hide
          Jakob Homan added a comment -

          Patch review:

          • Methods added for only unit testing should be marked as Private and Unstable
          • Move Client::getConnectionId to getConnectionID itself. With these changes ConnectionID may be large enough to warrant its own class rather than being nested in Client.
          • The custom hash method in ConnectionID seems a bit odd. Would the default provided by Eclipse be more workable? (as well as Eclipse-provided equals to guarantee hash/equals equivalency?)
          • Provide messages for asserts in unit tests
          Show
          Jakob Homan added a comment - Patch review: Methods added for only unit testing should be marked as Private and Unstable Move Client::getConnectionId to getConnectionID itself. With these changes ConnectionID may be large enough to warrant its own class rather than being nested in Client. The custom hash method in ConnectionID seems a bit odd. Would the default provided by Eclipse be more workable? (as well as Eclipse-provided equals to guarantee hash/equals equivalency?) Provide messages for asserts in unit tests
          Hide
          Kan Zhang added a comment -

          This patch does the following.

          1. It refactored a bunch of connection-related params from being global (stay the same for new connections) to become per-connection params and their values are obtained from the conf used when calling RPC.getProxy(). This allows new connections to be set up based on the conf used for that connection, which is more intuitive.

          2. In particular, the server's Kerberos principal is now obtained from the per connection conf.

          3. The conf cached globally in Client is not removed since it provides a cache for Classes that ObjectWritable.NullInstance relies on to figure out the declaredClass. It seems to provide a useful function and the cache can be shared among connections. No per-connection params are obtained from this global conf.

          Show
          Kan Zhang added a comment - This patch does the following. 1. It refactored a bunch of connection-related params from being global (stay the same for new connections) to become per-connection params and their values are obtained from the conf used when calling RPC.getProxy(). This allows new connections to be set up based on the conf used for that connection, which is more intuitive. 2. In particular, the server's Kerberos principal is now obtained from the per connection conf. 3. The conf cached globally in Client is not removed since it provides a cache for Classes that ObjectWritable.NullInstance relies on to figure out the declaredClass. It seems to provide a useful function and the cache can be shared among connections. No per-connection params are obtained from this global conf.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development