Hadoop Common
  1. Hadoop Common
  2. HADOOP-7172

SecureIO should not check owner on non-secure clusters that have no native support

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.22.0
    • Fix Version/s: 0.22.0, 0.23.0
    • Component/s: io, security
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      The SecureIOUtils.openForRead function currently uses a racy stat/open combo if security is disabled and the native libraries are not available. This ends up shelling out to "ls -ld" which is very very slow. We've seen this cause significant performance regressions on clusters that match this profile.

      Since the racy permissions check doesn't buy us any security anyway, we should just fall back to a normal "open" without any stat() at all, if we can't use the native support to do it efficiently.

      1. hadoop-7172.txt
        2 kB
        Todd Lipcon
      2. hadoop-7172.txt
        3 kB
        Todd Lipcon
      3. hadoop-7172.txt
        3 kB
        Todd Lipcon

        Issue Links

          Activity

          Hide
          Todd Lipcon added a comment -

          Marking this as depending on HADOOP-7115, since it will conflict with that patch. We may as well do that first and then do this.

          Show
          Todd Lipcon added a comment - Marking this as depending on HADOOP-7115 , since it will conflict with that patch. We may as well do that first and then do this.
          Hide
          Todd Lipcon added a comment -

          Here's a patch, but it will end up conflicting once 7115 is committed.

          Show
          Todd Lipcon added a comment - Here's a patch, but it will end up conflicting once 7115 is committed.
          Hide
          Eli Collins added a comment -

          +1

          Change looks good. Please update the javadoc ("@throws IOException if an IO Error occurred, or the user does not match") to qualify this for the new behavior.

          Show
          Eli Collins added a comment - +1 Change looks good. Please update the javadoc ("@throws IOException if an IO Error occurred, or the user does not match") to qualify this for the new behavior.
          Hide
          Todd Lipcon added a comment -

          After sleeping on this, I had second thoughts about the original way I did the patch. It doesn't seem good to me that, on an insecure cluster, we have different semantics based on whether the native libraries are available. So, I've changed the condition such that the check is always skipped when security is disabled, regardless of presence of the native libs.

          Updated the JavaDoc to indicate that the security check is only done when security is enabled.

          Show
          Todd Lipcon added a comment - After sleeping on this, I had second thoughts about the original way I did the patch. It doesn't seem good to me that, on an insecure cluster, we have different semantics based on whether the native libraries are available. So, I've changed the condition such that the check is always skipped when security is disabled, regardless of presence of the native libs. Updated the JavaDoc to indicate that the security check is only done when security is enabled.
          Hide
          Todd Lipcon added a comment -

          Sorry, forgot to tweak unit tests for the change. This patch should pass both with security on and off

          Show
          Todd Lipcon added a comment - Sorry, forgot to tweak unit tests for the change. This patch should pass both with security on and off
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12473024/hadoop-7172.txt
          against trunk revision 1082788.

          +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 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 contrib tests. The patch passed contrib unit tests.

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

          Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/312//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/312//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/312//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/12473024/hadoop-7172.txt against trunk revision 1082788. +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 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 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/312//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/312//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/312//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/12473024/hadoop-7172.txt
          against trunk revision 1094750.

          +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 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 contrib tests. The patch passed contrib unit tests.

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

          Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/361//testReport/
          Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/361//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/361//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/12473024/hadoop-7172.txt against trunk revision 1094750. +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 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 contrib tests. The patch passed contrib unit tests. +1 system test framework. The patch passed system test framework compile. Test results: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/361//testReport/ Findbugs warnings: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/361//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: https://hudson.apache.org/hudson/job/PreCommit-HADOOP-Build/361//console This message is automatically generated.
          Hide
          Todd Lipcon added a comment -

          Eli, would you mind taking a look at this updated patch?

          Show
          Todd Lipcon added a comment - Eli, would you mind taking a look at this updated patch?
          Hide
          Eli Collins added a comment -

          It doesn't seem good to me that, on an insecure cluster, we have different semantics based on whether the native libraries are available. So, I've changed the condition such that the check is always skipped when security is disabled, regardless of presence of the native libs.

          Nice, I agree. Doesn't it make sense for createForWrite to get the same treatment?

          I found my brain wanting to rewrite the code as follows, feel free to ignore if you think this is less clear.

          if (shouldBeSecure) {
            return forceSecureOpenForRead(f, expectedOwner, expectedGroup);
          }
          return new FileInputStream(f);
          
          Show
          Eli Collins added a comment - It doesn't seem good to me that, on an insecure cluster, we have different semantics based on whether the native libraries are available. So, I've changed the condition such that the check is always skipped when security is disabled, regardless of presence of the native libs. Nice, I agree. Doesn't it make sense for createForWrite to get the same treatment? I found my brain wanting to rewrite the code as follows, feel free to ignore if you think this is less clear. if (shouldBeSecure) { return forceSecureOpenForRead(f, expectedOwner, expectedGroup); } return new FileInputStream(f);
          Hide
          Todd Lipcon added a comment -

          Doesn't it make sense for createForWrite to get the same treatment

          Well, the reason for this JIRA is that checking permissions is super-expensive, since it shells out to ls -ld. But, for createForWrite, we only have to check f.exists() which is only a syscall, and not too expensive.

          So, I think we may as well keep the behavior we have for createForWrite.

          I found my brain wanting to rewrite the code as follows, feel free to ignore if you think this is less clear.

          Hm, I could go either way. Since we already have test-patch results, I'd like to just keep it as is if that's alright.

          Show
          Todd Lipcon added a comment - Doesn't it make sense for createForWrite to get the same treatment Well, the reason for this JIRA is that checking permissions is super-expensive, since it shells out to ls -ld . But, for createForWrite , we only have to check f.exists() which is only a syscall, and not too expensive. So, I think we may as well keep the behavior we have for createForWrite. I found my brain wanting to rewrite the code as follows, feel free to ignore if you think this is less clear. Hm, I could go either way. Since we already have test-patch results, I'd like to just keep it as is if that's alright.
          Hide
          Eli Collins added a comment -

          Makes sense.

          +1

          Show
          Eli Collins added a comment - Makes sense. +1
          Hide
          Eli Collins added a comment -

          I've committed this to trunk and 22. Thanks Todd!

          Show
          Eli Collins added a comment - I've committed this to trunk and 22. Thanks Todd!
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #562 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/562/)
          HADOOP-7172. SecureIO should not check owner on non-secure clusters that have no native support. Contributed by Todd Lipcon

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #562 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk-Commit/562/ ) HADOOP-7172 . SecureIO should not check owner on non-secure clusters that have no native support. Contributed by Todd Lipcon
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-22-branch #41 (See https://builds.apache.org/hudson/job/Hadoop-Common-22-branch/41/)
          HADOOP-7172. svn merge -c 1095958 from trunk

          Show
          Hudson added a comment - Integrated in Hadoop-Common-22-branch #41 (See https://builds.apache.org/hudson/job/Hadoop-Common-22-branch/41/ ) HADOOP-7172 . svn merge -c 1095958 from trunk
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk #667 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk/667/)
          HADOOP-7172. SecureIO should not check owner on non-secure clusters that have no native support. Contributed by Todd Lipcon

          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk #667 (See https://builds.apache.org/hudson/job/Hadoop-Common-trunk/667/ ) HADOOP-7172 . SecureIO should not check owner on non-secure clusters that have no native support. Contributed by Todd Lipcon

            People

            • Assignee:
              Todd Lipcon
              Reporter:
              Todd Lipcon
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development