Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-6406

Add capability for NFS gateway to reject connections from unprivileged ports

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.0
    • Fix Version/s: 2.5.0
    • Component/s: nfs
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      Many NFS servers have the ability to only accept client connections originating from privileged ports. It would be nice if the HDFS NFS gateway had the same feature.

      1. HDFS-6406.patch
        19 kB
        Aaron T. Myers
      2. HDFS-6406.patch
        20 kB
        Aaron T. Myers

        Issue Links

          Activity

          Hide
          Aaron T. Myers added a comment -

          Ah, gotcha. Sure, that'd save the creation of a Configuration object, but there's no real incorrect behavior as a result.

          Thanks, Brandon.

          Show
          Aaron T. Myers added a comment - Ah, gotcha. Sure, that'd save the creation of a Configuration object, but there's no real incorrect behavior as a result. Thanks, Brandon.
          Hide
          Brandon Li added a comment -

          My bad. The code in #1 is actually correct. I was just thinking a trivial change:
          final Nfs3 nfsServer = new Nfs3(conf, registrationSocket, allowInsecurePorts);
          to replace
          final Nfs3 nfsServer = new Nfs3(new Configuration(), registrationSocket, allowInsecurePorts);

          Show
          Brandon Li added a comment - My bad. The code in #1 is actually correct. I was just thinking a trivial change: final Nfs3 nfsServer = new Nfs3(conf, registrationSocket, allowInsecurePorts); to replace final Nfs3 nfsServer = new Nfs3(new Configuration(), registrationSocket, allowInsecurePorts);
          Hide
          Aaron T. Myers added a comment -

          Thanks for the comments, Brandon. 2/3/4 make total sense to me, let's continue discussion of those on HDFS-6439.

          As for #1, maybe I'm being dense, but I don't see the problem with the code you pointed out there. Could you elaborate a bit on what the trouble is? How is it that the configuration setting isn't taken?

          Show
          Aaron T. Myers added a comment - Thanks for the comments, Brandon. 2/3/4 make total sense to me, let's continue discussion of those on HDFS-6439 . As for #1, maybe I'm being dense, but I don't see the problem with the code you pointed out there. Could you elaborate a bit on what the trouble is? How is it that the configuration setting isn't taken?
          Hide
          Brandon Li added a comment -

          Thank you, Aaron T. Myers. I am so sorry for the late comments. The patch looks good, and it adds a nice feature. Given we don't have NFS kerberos supported yet, this feature adds additional security to the NFS gateway. I have a few comments.

          1. Nfs3.java: the configuration setting is not taken. This can be fixed as part of the config cleanup in HDFS-6056 since it's a trivial change.

          +    Configuration conf = new Configuration();
          +    boolean allowInsecurePorts = conf.getBoolean(
          +        DFSConfigKeys.DFS_NFS_ALLOW_INSECURE_PORTS_KEY,
          +        DFSConfigKeys.DFS_NFS_ALLOW_INSECURE_PORTS_DEFAULT);
          +    final Nfs3 nfsServer = new Nfs3(new Configuration(), registrationSocket,
          +        allowInsecurePorts);
          

          2. Port monitoring is the feature name with traditional NFS server and we may want to make the config property (along with related variable allowInsecurePorts) something as dfs.nfs.port.monitoring. Even though traditional NFS has two port monitoring for NFS server and mountd, I think one config property is good enough for both of them in our NFS gateway.

          3 . According to RFC2623 (http://www.rfc-editor.org/rfc/rfc2623.txt):

          Whether port monitoring is enabled or not, NFS servers SHOULD NOT reject NFS requests to the NULL procedure (procedure number 0). See subsection 2.3.1, "NULL procedure" for a complete explanation.

          I do notice that NFS clients (most time) send mount NULL and nfs NULL from no privileged port. If we deny that call in mountd or nfs server, the client can't mount the export even as user root.

          4. it would be nice to have the user guide updated too.

          Let's use HDFS-6439 to track the change for 2,3,4 and I will post more comments there.

          Show
          Brandon Li added a comment - Thank you, Aaron T. Myers . I am so sorry for the late comments. The patch looks good, and it adds a nice feature. Given we don't have NFS kerberos supported yet, this feature adds additional security to the NFS gateway. I have a few comments. 1. Nfs3.java: the configuration setting is not taken. This can be fixed as part of the config cleanup in HDFS-6056 since it's a trivial change. + Configuration conf = new Configuration(); + boolean allowInsecurePorts = conf.getBoolean( + DFSConfigKeys.DFS_NFS_ALLOW_INSECURE_PORTS_KEY, + DFSConfigKeys.DFS_NFS_ALLOW_INSECURE_PORTS_DEFAULT); + final Nfs3 nfsServer = new Nfs3(new Configuration(), registrationSocket, + allowInsecurePorts); 2. Port monitoring is the feature name with traditional NFS server and we may want to make the config property (along with related variable allowInsecurePorts) something as dfs.nfs.port.monitoring. Even though traditional NFS has two port monitoring for NFS server and mountd, I think one config property is good enough for both of them in our NFS gateway. 3 . According to RFC2623 ( http://www.rfc-editor.org/rfc/rfc2623.txt): Whether port monitoring is enabled or not, NFS servers SHOULD NOT reject NFS requests to the NULL procedure (procedure number 0). See subsection 2.3.1, "NULL procedure" for a complete explanation. I do notice that NFS clients (most time) send mount NULL and nfs NULL from no privileged port. If we deny that call in mountd or nfs server, the client can't mount the export even as user root. 4. it would be nice to have the user guide updated too. Let's use HDFS-6439 to track the change for 2,3,4 and I will post more comments there.
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Mapreduce-trunk #1780 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1780/)
          HDFS-6406. Add capability for NFS gateway to reject connections from unprivileged ports. Contributed by Aaron T. Myers. (atm: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1595351)

          • /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/RpcProgram.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/java/org/apache/hadoop/oncrpc/TestFrameDecoder.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/resources
          • /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/resources/log4j.properties
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/Mountd.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/RpcProgramMountd.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/Nfs3.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #1780 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1780/ ) HDFS-6406 . Add capability for NFS gateway to reject connections from unprivileged ports. Contributed by Aaron T. Myers. (atm: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1595351 ) /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/RpcProgram.java /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/java/org/apache/hadoop/oncrpc/TestFrameDecoder.java /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/resources /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/resources/log4j.properties /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/Mountd.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/RpcProgramMountd.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/Nfs3.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Hdfs-trunk #1754 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1754/)
          HDFS-6406. Add capability for NFS gateway to reject connections from unprivileged ports. Contributed by Aaron T. Myers. (atm: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1595351)

          • /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/RpcProgram.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/java/org/apache/hadoop/oncrpc/TestFrameDecoder.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/resources
          • /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/resources/log4j.properties
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/Mountd.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/RpcProgramMountd.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/Nfs3.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #1754 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1754/ ) HDFS-6406 . Add capability for NFS gateway to reject connections from unprivileged ports. Contributed by Aaron T. Myers. (atm: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1595351 ) /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/RpcProgram.java /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/java/org/apache/hadoop/oncrpc/TestFrameDecoder.java /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/resources /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/resources/log4j.properties /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/Mountd.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/RpcProgramMountd.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/Nfs3.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-Yarn-trunk #562 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/562/)
          HDFS-6406. Add capability for NFS gateway to reject connections from unprivileged ports. Contributed by Aaron T. Myers. (atm: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1595351)

          • /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/RpcProgram.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/java/org/apache/hadoop/oncrpc/TestFrameDecoder.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/resources
          • /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/resources/log4j.properties
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/Mountd.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/RpcProgramMountd.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/Nfs3.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #562 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/562/ ) HDFS-6406 . Add capability for NFS gateway to reject connections from unprivileged ports. Contributed by Aaron T. Myers. (atm: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1595351 ) /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/RpcProgram.java /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/java/org/apache/hadoop/oncrpc/TestFrameDecoder.java /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/resources /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/resources/log4j.properties /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/Mountd.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/RpcProgramMountd.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/Nfs3.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-trunk-Commit #5606 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5606/)
          HDFS-6406. Add capability for NFS gateway to reject connections from unprivileged ports. Contributed by Aaron T. Myers. (atm: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1595351)

          • /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/RpcProgram.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/java/org/apache/hadoop/oncrpc/TestFrameDecoder.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/resources
          • /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/resources/log4j.properties
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/Mountd.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/RpcProgramMountd.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/Nfs3.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-trunk-Commit #5606 (See https://builds.apache.org/job/Hadoop-trunk-Commit/5606/ ) HDFS-6406 . Add capability for NFS gateway to reject connections from unprivileged ports. Contributed by Aaron T. Myers. (atm: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1595351 ) /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/main/java/org/apache/hadoop/oncrpc/RpcProgram.java /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/java/org/apache/hadoop/oncrpc/TestFrameDecoder.java /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/resources /hadoop/common/trunk/hadoop-common-project/hadoop-nfs/src/test/resources/log4j.properties /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/Mountd.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/mount/RpcProgramMountd.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/Nfs3.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs-nfs/src/main/java/org/apache/hadoop/hdfs/nfs/nfs3/RpcProgramNfs3.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
          Hide
          Aaron T. Myers added a comment -

          Thanks, Brandon. Sorry about that. If you do find anything, please CC me on the new JIRA you file.

          Show
          Aaron T. Myers added a comment - Thanks, Brandon. Sorry about that. If you do find anything, please CC me on the new JIRA you file.
          Hide
          Brandon Li added a comment -

          ops, I missed the train. I will open a different JIRA if I notice anything to be improved later. Thanks.

          Show
          Brandon Li added a comment - ops, I missed the train. I will open a different JIRA if I notice anything to be improved later. Thanks.
          Hide
          Brandon Li added a comment -

          Please give me a bit time. I will review the patch late today or tomorrow.

          Show
          Brandon Li added a comment - Please give me a bit time. I will review the patch late today or tomorrow.
          Hide
          Aaron T. Myers added a comment -

          I've just committed this to trunk and branch-2.

          Thanks a lot for the reviews and comments, folks.

          Show
          Aaron T. Myers added a comment - I've just committed this to trunk and branch-2. Thanks a lot for the reviews and comments, folks.
          Hide
          Aaron T. Myers added a comment -

          SLF4J doesn't let you at the log4j appender behind a log, so switching to log4j would break your test. The workaround there is to use commons logging in the test and request the same log -which is what most of today's tests do. If you stick with commons logging, it's a non-issue.

          Note that the test does not actually rely on the increased log level - that was just for convenience so that I could look at the logs after running it.

          this would be good time to replace the inline "nfs3.mountd.port", with a constant.

          Agreed, but I've been deliberately avoiding messing with the NFS-related configs because HDFS-6056 is doing a larger cleanup of that whole thing.

          Sounds like there are no objections to this patch, despite the inherent limits of the approach. I'm going to go ahead and commit this shortly based on Andrew's +1.

          Show
          Aaron T. Myers added a comment - SLF4J doesn't let you at the log4j appender behind a log, so switching to log4j would break your test. The workaround there is to use commons logging in the test and request the same log -which is what most of today's tests do. If you stick with commons logging, it's a non-issue. Note that the test does not actually rely on the increased log level - that was just for convenience so that I could look at the logs after running it. this would be good time to replace the inline "nfs3.mountd.port", with a constant. Agreed, but I've been deliberately avoiding messing with the NFS-related configs because HDFS-6056 is doing a larger cleanup of that whole thing. Sounds like there are no objections to this patch, despite the inherent limits of the approach. I'm going to go ahead and commit this shortly based on Andrew's +1.
          Hide
          Brandon Li added a comment -

          Port monitoring is relic, but still, it's an additional source of NFS security.
          This is a nice feature to have.

          in a world of Linux and Linux VMs, there can be a lot of unix root admins in a cluster

          Yes, this can be a problem. In this case, users can configure the export table ("dfs.nfs.exports.allowed.hosts") to limit the access in an environment where IP can be trusted.

          Show
          Brandon Li added a comment - Port monitoring is relic, but still, it's an additional source of NFS security. This is a nice feature to have. in a world of Linux and Linux VMs, there can be a lot of unix root admins in a cluster Yes, this can be a problem. In this case, users can configure the export table ("dfs.nfs.exports.allowed.hosts") to limit the access in an environment where IP can be trusted.
          Hide
          Steve Loughran added a comment -
          1. SLF4J doesn't let you at the log4j appender behind a log, so switching to log4j would break your test. The workaround there is to use commons logging in the test and request the same log -which is what most of today's tests do. If you stick with commons logging, it's a non-issue.
          2. this would be good time to replace the inline "nfs3.mountd.port", with a constant.

          w.r.t the patch, IMO the port # filter is a relic of the days when NFS lacked authentication only identification and you could control all the workstations on the lan. The restricted port policy let the root user make requests (if the exported drive allowed it), and if they claimed to be another user, they were. Windows doesn't have that same port value and in a world of Linux and Linux VMs, there can be a lot of unix root admins in a cluster.

          summary: it doesn't really boost security, unless the network is really locked down. But if it is, it may

          Show
          Steve Loughran added a comment - SLF4J doesn't let you at the log4j appender behind a log, so switching to log4j would break your test. The workaround there is to use commons logging in the test and request the same log -which is what most of today's tests do. If you stick with commons logging, it's a non-issue. this would be good time to replace the inline "nfs3.mountd.port", with a constant. w.r.t the patch, IMO the port # filter is a relic of the days when NFS lacked authentication only identification and you could control all the workstations on the lan. The restricted port policy let the root user make requests (if the exported drive allowed it), and if they claimed to be another user, they were. Windows doesn't have that same port value and in a world of Linux and Linux VMs, there can be a lot of unix root admins in a cluster. summary: it doesn't really boost security, unless the network is really locked down. But if it is, it may
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12645108/HDFS-6406.patch
          against trunk revision .

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

          +1 tests included. The patch appears to include 2 new or modified test files.

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

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +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 unit tests in hadoop-common-project/hadoop-nfs hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-nfs:

          org.apache.hadoop.fs.TestHdfsNativeCodeLoader

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6914//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6914//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/12645108/HDFS-6406.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +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 unit tests in hadoop-common-project/hadoop-nfs hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-nfs: org.apache.hadoop.fs.TestHdfsNativeCodeLoader +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6914//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6914//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/12644942/HDFS-6406.patch
          against trunk revision .

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

          +1 tests included. The patch appears to include 2 new or modified test files.

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

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 1 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 unit tests in hadoop-common-project/hadoop-nfs hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-nfs:

          org.apache.hadoop.fs.TestHdfsNativeCodeLoader

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6911//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/6911//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6911//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/12644942/HDFS-6406.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 1 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 unit tests in hadoop-common-project/hadoop-nfs hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-nfs: org.apache.hadoop.fs.TestHdfsNativeCodeLoader +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6911//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/6911//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6911//console This message is automatically generated.
          Hide
          Andrew Wang added a comment -

          +1 pending, I agree those failures look unrelated. There's been some flux recently with FsPermission because of HDFS-6326, so it might be because of that.

          Show
          Andrew Wang added a comment - +1 pending, I agree those failures look unrelated. There's been some flux recently with FsPermission because of HDFS-6326 , so it might be because of that.
          Hide
          Aaron T. Myers added a comment -

          Thanks a lot for the review, Andrew. I think it's a good suggestion to switch it to a boolean. The latest patch does that. I also fixed the typo you pointed out.

          Didn't switch to slf4j, but maybe I will in my next patch.

          Pretty sure that the findbugs warning and test failure from the pre-commit build are unrelated. This patch doesn't touch any relevant code in those areas.

          Show
          Aaron T. Myers added a comment - Thanks a lot for the review, Andrew. I think it's a good suggestion to switch it to a boolean. The latest patch does that. I also fixed the typo you pointed out. Didn't switch to slf4j, but maybe I will in my next patch. Pretty sure that the findbugs warning and test failure from the pre-commit build are unrelated. This patch doesn't touch any relevant code in those areas.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12644942/HDFS-6406.patch
          against trunk revision .

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

          +1 tests included. The patch appears to include 2 new or modified test files.

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

          +1 javadoc. There were no new javadoc warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          -1 findbugs. The patch appears to introduce 1 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 unit tests in hadoop-common-project/hadoop-nfs hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-nfs:

          org.apache.hadoop.fs.TestHdfsNativeCodeLoader

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

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6903//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/6903//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6903//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/12644942/HDFS-6406.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 2 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. -1 findbugs . The patch appears to introduce 1 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 unit tests in hadoop-common-project/hadoop-nfs hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-nfs: org.apache.hadoop.fs.TestHdfsNativeCodeLoader +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6903//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/6903//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6903//console This message is automatically generated.
          Hide
          Andrew Wang added a comment -

          Hey ATM, definitely a nice feature here. A few review comments:

          • I believe privileged ports are [0-1023] inclusive, so 1024 actually isn't a privileged port. Based on the comment in hdfs-default.xml, the correct value would actually be 1023.
          • There's some ambiguity about setting a value of 0 for this config parameter. Zero isn't positive or negative and also isn't a valid port (and thus not a valid config setting either), but we don't WARN or abort on this. It might make sense to have 0 be the default special value rather than introducing the special "-1", and then throwing some exception if a negative value is set.
          • Because of the above two comments, I'd prefer a boolean rather than specifying an int, which seems more error prone. AFAIK you can't configure the privileged port range (which I think is kind of the point), so I don't see much utility in being able to specify a range.
          • This is a good opportunity to try out SLF4J if you're interested, since we can skip the isDebugEnabled if wrappers
          • Typo in test: "s/rung/run"
          Show
          Andrew Wang added a comment - Hey ATM, definitely a nice feature here. A few review comments: I believe privileged ports are [0-1023] inclusive, so 1024 actually isn't a privileged port. Based on the comment in hdfs-default.xml, the correct value would actually be 1023. There's some ambiguity about setting a value of 0 for this config parameter. Zero isn't positive or negative and also isn't a valid port (and thus not a valid config setting either), but we don't WARN or abort on this. It might make sense to have 0 be the default special value rather than introducing the special "-1", and then throwing some exception if a negative value is set. Because of the above two comments, I'd prefer a boolean rather than specifying an int, which seems more error prone. AFAIK you can't configure the privileged port range (which I think is kind of the point), so I don't see much utility in being able to specify a range. This is a good opportunity to try out SLF4J if you're interested, since we can skip the isDebugEnabled if wrappers Typo in test: "s/rung/run"
          Hide
          Aaron T. Myers added a comment -

          Straightforward patch attached which implements this feature.

          This patch also takes the opportunity to refactor a method or two to reduce some repeated code, and add some javadoc that I noticed was missing.

          Please review.

          Show
          Aaron T. Myers added a comment - Straightforward patch attached which implements this feature. This patch also takes the opportunity to refactor a method or two to reduce some repeated code, and add some javadoc that I noticed was missing. Please review.

            People

            • Assignee:
              Aaron T. Myers
              Reporter:
              Aaron T. Myers
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development