Hadoop Common
  1. Hadoop Common
  2. HADOOP-6929

RPC should have a way to pass Security information other than protocol annotations

    Details

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

      Description

      Currently Hadoop RPC allows protocol annotations as the only way to pass security information. This becomes a problem if protocols are generated and not hand written. For example protocols generated via Avro and passed over Avro tunnel (AvroRpcEngine.java) can't pass the security information.

      1. h-6929.patch
        22 kB
        Owen O'Malley
      2. h-6929.patch
        21 kB
        Owen O'Malley
      3. h-6929.patch
        19 kB
        Owen O'Malley
      4. Hadoop-6929_v1.patch
        20 kB
        Sharad Agarwal

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-maven #43 (See https://builds.apache.org/job/Hadoop-Common-trunk-maven/43/)

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-maven #43 (See https://builds.apache.org/job/Hadoop-Common-trunk-maven/43/ )
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #661 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/661/)

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #661 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/661/ )
          Hide
          Mahadev konar added a comment -

          I just committed this to trunk. Thanks Sharad and Owen!

          Show
          Mahadev konar added a comment - I just committed this to trunk. Thanks Sharad and Owen!
          Hide
          Mahadev konar added a comment -

          +1 the patch looks good. Ill run through some hdfs tests just to make sure everything is all fine and then commit.

          Show
          Mahadev konar added a comment - +1 the patch looks good. Ill run through some hdfs tests just to make sure everything is all fine and then commit.
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12482605/h-6929.patch
          against trunk revision 1135820.

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

          +1 tests included. The patch appears to include 10 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 (version 1.3.9) 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 system test framework. The patch passed system test framework compile.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/632//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/632//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/632//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/12482605/h-6929.patch against trunk revision 1135820. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 10 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 (version 1.3.9) 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 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/632//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/632//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/632//console This message is automatically generated.
          Hide
          Owen O'Malley added a comment -

          Fix the metrics testcase that was using a hardcoded build/classes path name.

          Show
          Owen O'Malley added a comment - Fix the metrics testcase that was using a hardcoded build/classes path name.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12482567/h-6929.patch
          against trunk revision 1135636.

          +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 does not increase the total number of javac compiler warnings.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

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

          -1 core tests. The patch failed these core unit tests:
          org.apache.hadoop.metrics2.impl.TestMetricsConfig
          org.apache.hadoop.metrics2.impl.TestMetricsSystemImpl

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/630//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/630//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/630//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/12482567/h-6929.patch against trunk revision 1135636. +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 does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: org.apache.hadoop.metrics2.impl.TestMetricsConfig org.apache.hadoop.metrics2.impl.TestMetricsSystemImpl +1 system test framework. The patch passed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/630//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/630//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/630//console This message is automatically generated.
          Hide
          Owen O'Malley added a comment -

          I forgot the two new java classes.

          Show
          Owen O'Malley added a comment - I forgot the two new java classes.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12482554/h-6929.patch
          against trunk revision 1135636.

          +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 patch appears to cause tar ant target to fail.

          -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail.

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

          -1 core tests. The patch failed these core unit tests:

          -1 system test framework. The patch failed system test framework compile.

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/629//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/629//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/12482554/h-6929.patch against trunk revision 1135636. +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 patch appears to cause tar ant target to fail. -1 findbugs. The patch appears to cause Findbugs (version 1.3.9) to fail. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these core unit tests: -1 system test framework. The patch failed system test framework compile. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/629//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/629//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/12482554/h-6929.patch
          against trunk revision 1135633.

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/627//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/12482554/h-6929.patch against trunk revision 1135633. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/627//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          I just committed HADOOP-7384 which should enable this patch to work properly. I'll re-trigger the build.

          Show
          Todd Lipcon added a comment - I just committed HADOOP-7384 which should enable this patch to work properly. I'll re-trigger the build.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12482554/h-6929.patch
          against trunk revision 1135333.

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

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

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

          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/625//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/12482554/h-6929.patch against trunk revision 1135333. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified tests. -1 patch. The patch command could not apply the patch. Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/625//console This message is automatically generated.
          Hide
          Owen O'Malley added a comment -

          This patch replaces the configured security info classes with security info providers that are listed in the jar.

          Show
          Owen O'Malley added a comment - This patch replaces the configured security info classes with security info providers that are listed in the jar.
          Hide
          Luke Lu added a comment -

          In particular, you don't want to put class names in configuration

          This is a pervasive anti-pattern used in hadoop all over the place, HADOOP-7150 is supposed to address that.

          and certainly don't want the new SecurityContext to replace the current one.

          Agreed. This is a major flaw of the current patch, though the flawed mechanism is still workable if the new security info implements the fallback mechanism.

          private static ServiceLoader<SecurityInfo> securityInfoProviders = new ServiceLoader<SecurityInfo>(SecurityInfo.class);

          The usage should be:

          ServiceLoader<SecurityInfo> securityInfoProviders = ServiceLoader.load(SecurityInfo.class);
          
          Show
          Luke Lu added a comment - In particular, you don't want to put class names in configuration This is a pervasive anti-pattern used in hadoop all over the place, HADOOP-7150 is supposed to address that. and certainly don't want the new SecurityContext to replace the current one. Agreed. This is a major flaw of the current patch, though the flawed mechanism is still workable if the new security info implements the fallback mechanism. private static ServiceLoader<SecurityInfo> securityInfoProviders = new ServiceLoader<SecurityInfo>(SecurityInfo.class); The usage should be: ServiceLoader<SecurityInfo> securityInfoProviders = ServiceLoader.load(SecurityInfo.class);
          Hide
          Owen O'Malley added a comment -

          This isn't the right approach. In particular, you don't want to put class names in configuration and certainly don't want the new SecurityContext to replace the current one. We want to use the annotations if they exist and fall back on other mechanisms when they don't.

          public abstract class SecurityInfo {
            public abstract KerberofInfo getKerberosInfo(Class<?> protocol);
            public abstract TokenInfo getTokenInfo(Class<?> protocol);
          }
          
          public class SecurityUtil {
            private static ServiceLoader<SecurityInfo> securityInfoProviders =
              new ServiceLoader<SecurityInfo>(SecurityInfo.class);
            public static KerberosInfo getKerberosInfo(Class<?> protocol) {
              for(SecurityInfo provider: securityInfoProviders) {
                Class<?> result = provider.getKerberosInfo(protocol);
                if (result != null) return result;
              }
              return null;
            }
            public static TokenInfo getTokenInfo(Class<?> protocol) {...
            }
          }
          

          The Hadoop jar can register the AnnotatedSecurityInfo as the default. If we wish to implement more than one in the default jar, we can define a StandardSecurityInfo that first checks AnnotatedSecurityInfo and then falls back to the second one.

          Show
          Owen O'Malley added a comment - This isn't the right approach. In particular, you don't want to put class names in configuration and certainly don't want the new SecurityContext to replace the current one. We want to use the annotations if they exist and fall back on other mechanisms when they don't. public abstract class SecurityInfo { public abstract KerberofInfo getKerberosInfo( Class <?> protocol); public abstract TokenInfo getTokenInfo( Class <?> protocol); } public class SecurityUtil { private static ServiceLoader<SecurityInfo> securityInfoProviders = new ServiceLoader<SecurityInfo>(SecurityInfo.class); public static KerberosInfo getKerberosInfo( Class <?> protocol) { for (SecurityInfo provider: securityInfoProviders) { Class <?> result = provider.getKerberosInfo(protocol); if (result != null ) return result; } return null ; } public static TokenInfo getTokenInfo( Class <?> protocol) {... } } The Hadoop jar can register the AnnotatedSecurityInfo as the default. If we wish to implement more than one in the default jar, we can define a StandardSecurityInfo that first checks AnnotatedSecurityInfo and then falls back to the second one.
          Hide
          Luke Lu added a comment -

          +1, lgtm. This is required for mrv2.

          Show
          Luke Lu added a comment - +1, lgtm. This is required for mrv2.
          Hide
          Sharad Agarwal added a comment -

          Patch for review. This allows to pass Security information via Avro tunnel to Hadoop RPC. Adds tests to Avro RPC (for reflect and specific).

          Show
          Sharad Agarwal added a comment - Patch for review. This allows to pass Security information via Avro tunnel to Hadoop RPC. Adds tests to Avro RPC (for reflect and specific).
          Hide
          Vinod Kumar Vavilapalli added a comment -

          Can we do something like this? (An updated version of Sharad's offline proposal)

          interface SecurityContext {
            KerberosContext getKerberosContext(Class protocol);
            TokenContext getTokenContext(Class protocol);
          }
          
          //get the info from annotations
          AnnotatedSecurityContext implements SecurityContext {
            KerberosContext getKerborosContext(Class protocol) {
               // construct KerberosContext from annotation KerberosInfo
            }
          
            TokenContext getTokenContext(Class protocol) {
               // construct TokenContext from annotation TokenInfo
            }
          }
          
          // get the information from hand-crafted context object.
          HandCraftedSecurityContext implements SecurityContext {
          }
          
          

          We will need to pass this context information all through into the RPC layer. (Is this why annotations were originally used?)

          Show
          Vinod Kumar Vavilapalli added a comment - Can we do something like this? (An updated version of Sharad's offline proposal) interface SecurityContext { KerberosContext getKerberosContext( Class protocol); TokenContext getTokenContext( Class protocol); } //get the info from annotations AnnotatedSecurityContext implements SecurityContext { KerberosContext getKerborosContext( Class protocol) { // construct KerberosContext from annotation KerberosInfo } TokenContext getTokenContext( Class protocol) { // construct TokenContext from annotation TokenInfo } } // get the information from hand-crafted context object. HandCraftedSecurityContext implements SecurityContext { } We will need to pass this context information all through into the RPC layer. (Is this why annotations were originally used?)

            People

            • Assignee:
              Sharad Agarwal
              Reporter:
              Sharad Agarwal
            • Votes:
              0 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development