Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-14352

Make some HttpServer2 SSL properties optional

    Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.0-alpha2
    • Fix Version/s: 3.0.0-alpha4
    • Component/s: kms
    • Labels:
      None

      Description

      HttpServer2#loadSSLConfiguration loads 5 SSL properties but only keystore location and password are required, the rest of them, keystore keypassword, truststore location, and truststore password, can be optional.

      According to http://www.eclipse.org/jetty/documentation/current/configuring-ssl.html:

      • If there is no keymanagerpassword, then the keystorepassword is used instead.
      • Trust store is typically set to the same path as the keystore.
      1. HADOOP-14352.001.patch
        2 kB
        John Zhuge
      2. HADOOP-14352.002.patch
        4 kB
        John Zhuge
      3. HADOOP-14352.003.patch
        4 kB
        John Zhuge

        Issue Links

          Activity

          Hide
          jzhuge John Zhuge added a comment -

          Patch 001

          • Add a new method HttpServer2$Build#getOptionalPassword that returns null when the password not found
          • Change HttpServer2$Build#loadSSLConfiguration not to require keystore keypassword, trust store location, and trust store password

          Testing done

          • KMS and HttpFS sanity tests in SSL mode
          Show
          jzhuge John Zhuge added a comment - Patch 001 Add a new method HttpServer2$Build#getOptionalPassword that returns null when the password not found Change HttpServer2$Build#loadSSLConfiguration not to require keystore keypassword, trust store location, and trust store password Testing done KMS and HttpFS sanity tests in SSL mode
          Hide
          rkanter Robert Kanter added a comment -

          Thanks John Zhuge; here's some comments:

          • For your second bullet point (not requiring those configs), I don't see any code changes for that. I only see code changes for your first bullet point (getOptionalPassword). Unless the code already does that?
          • In your first bullet point, that would be that the keystoremanager password is not required, right? The keystore password is required.
          • I don't see why the method should be named getOptionalPassword. It's really just a wrapper around getPassword that returns a String instead of a char[], so why not call it getPassword or getPasswordString?
          Show
          rkanter Robert Kanter added a comment - Thanks John Zhuge ; here's some comments: For your second bullet point (not requiring those configs), I don't see any code changes for that. I only see code changes for your first bullet point ( getOptionalPassword ). Unless the code already does that? In your first bullet point, that would be that the keystoremanager password is not required, right? The keystore password is required. I don't see why the method should be named getOptionalPassword . It's really just a wrapper around getPassword that returns a String instead of a char[], so why not call it getPassword or getPasswordString ?
          Hide
          jzhuge John Zhuge added a comment -

          Thanks Robert Kanter for the review!

          For your second bullet point (not requiring those configs), I don't see any code changes for that. I only see code changes for your first bullet point (getOptionalPassword). Unless the code already does that?

          Sorry, forgot to mention that there is an existing HttpServer2$Build#getPassword that throws an exception when password not found. Replacing HttpServer2$Build#getPassword with HttpServer2$Build#getOptionalPassword makes keystore keypassword and trust store password.

          BTW, trust store location is already optional due to its Configuration#get call.

          In your first bullet point, that would be that the keystoremanager password is not required, right? The keystore password is required.

          Yes, HttpServer2 uses the term keypassword to stand for keystoremanager password.

          I don't see why the method should be named getOptionalPassword. It's really just a wrapper around getPassword that returns a String instead of a char[], so why not call it getPassword or getPasswordString?

          See comment for the first item.

          Show
          jzhuge John Zhuge added a comment - Thanks Robert Kanter for the review! For your second bullet point (not requiring those configs), I don't see any code changes for that. I only see code changes for your first bullet point (getOptionalPassword). Unless the code already does that? Sorry, forgot to mention that there is an existing HttpServer2$Build#getPassword that throws an exception when password not found. Replacing HttpServer2$Build#getPassword with HttpServer2$Build#getOptionalPassword makes keystore keypassword and trust store password. BTW, trust store location is already optional due to its Configuration#get call. In your first bullet point, that would be that the keystoremanager password is not required, right? The keystore password is required. Yes, HttpServer2 uses the term keypassword to stand for keystoremanager password. I don't see why the method should be named getOptionalPassword. It's really just a wrapper around getPassword that returns a String instead of a char[], so why not call it getPassword or getPasswordString? See comment for the first item.
          Hide
          rkanter Robert Kanter added a comment -

          I see. LGTM +1. Will commit shortly.

          Show
          rkanter Robert Kanter added a comment - I see. LGTM +1. Will commit shortly.
          Hide
          rkanter Robert Kanter added a comment -

          Actually, I'll wait for Jenkins

          Show
          rkanter Robert Kanter added a comment - Actually, I'll wait for Jenkins
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 17s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 mvninstall 14m 38s trunk passed
          +1 compile 17m 3s trunk passed
          +1 checkstyle 0m 34s trunk passed
          +1 mvnsite 1m 11s trunk passed
          +1 mvneclipse 0m 21s trunk passed
          -1 findbugs 1m 23s hadoop-common-project/hadoop-common in trunk has 17 extant Findbugs warnings.
          +1 javadoc 0m 49s trunk passed
          +1 mvninstall 0m 38s the patch passed
          +1 compile 14m 6s the patch passed
          +1 javac 14m 6s the patch passed
          -0 checkstyle 0m 35s hadoop-common-project/hadoop-common: The patch generated 1 new + 54 unchanged - 0 fixed = 55 total (was 54)
          +1 mvnsite 1m 3s the patch passed
          +1 mvneclipse 0m 20s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 30s the patch passed
          +1 javadoc 0m 50s the patch passed
          +1 unit 8m 21s hadoop-common in the patch passed.
          +1 asflicense 0m 36s The patch does not generate ASF License warnings.
          66m 11s



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:0ac17dc
          JIRA Issue HADOOP-14352
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12864984/HADOOP-14352.001.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 4ea20a38005f 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 475f933
          Default Java 1.8.0_121
          findbugs v3.1.0-RC1
          findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/12186/artifact/patchprocess/branch-findbugs-hadoop-common-project_hadoop-common-warnings.html
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12186/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12186/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12186/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 17s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 14m 38s trunk passed +1 compile 17m 3s trunk passed +1 checkstyle 0m 34s trunk passed +1 mvnsite 1m 11s trunk passed +1 mvneclipse 0m 21s trunk passed -1 findbugs 1m 23s hadoop-common-project/hadoop-common in trunk has 17 extant Findbugs warnings. +1 javadoc 0m 49s trunk passed +1 mvninstall 0m 38s the patch passed +1 compile 14m 6s the patch passed +1 javac 14m 6s the patch passed -0 checkstyle 0m 35s hadoop-common-project/hadoop-common: The patch generated 1 new + 54 unchanged - 0 fixed = 55 total (was 54) +1 mvnsite 1m 3s the patch passed +1 mvneclipse 0m 20s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 30s the patch passed +1 javadoc 0m 50s the patch passed +1 unit 8m 21s hadoop-common in the patch passed. +1 asflicense 0m 36s The patch does not generate ASF License warnings. 66m 11s Subsystem Report/Notes Docker Image:yetus/hadoop:0ac17dc JIRA Issue HADOOP-14352 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12864984/HADOOP-14352.001.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 4ea20a38005f 3.13.0-107-generic #154-Ubuntu SMP Tue Dec 20 09:57:27 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 475f933 Default Java 1.8.0_121 findbugs v3.1.0-RC1 findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/12186/artifact/patchprocess/branch-findbugs-hadoop-common-project_hadoop-common-warnings.html checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12186/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12186/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12186/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jzhuge John Zhuge added a comment -

          Patch 002

          • Rename getPasswordString and make it return null when password not found
          • Add null check for keystore location and password in loadSSLConfiguration. This fixes a bug in patch 001 where null keystore location is not detected.

          Robert Kanter I made some changes in patch 002. Could you please take a look?

          Show
          jzhuge John Zhuge added a comment - Patch 002 Rename getPasswordString and make it return null when password not found Add null check for keystore location and password in loadSSLConfiguration . This fixes a bug in patch 001 where null keystore location is not detected. Robert Kanter I made some changes in patch 002. Could you please take a look?
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 0s Docker mode activated.
          -1 patch 0m 12s HADOOP-14352 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help.



          Subsystem Report/Notes
          JIRA Issue HADOOP-14352
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12865088/HADOOP-14352.002.patch
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12193/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 0s Docker mode activated. -1 patch 0m 12s HADOOP-14352 does not apply to trunk. Rebase required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for help. Subsystem Report/Notes JIRA Issue HADOOP-14352 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12865088/HADOOP-14352.002.patch Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12193/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jzhuge John Zhuge added a comment -

          Patch 003

          • Fix merge issue in 003

          Sample exception when keystore location or password not specified:

          2017-04-26 12:48:55,145 INFO conf.ConfigurationWithLogging: Got ssl.server.keystore.location = 'null'
          Exception in thread "main" java.io.IOException: Property ssl.server.keystore.location not specified
          	at org.apache.hadoop.http.HttpServer2$Builder.loadSSLConfiguration(HttpServer2.java:378)
          	at org.apache.hadoop.http.HttpServer2$Builder.build(HttpServer2.java:419)
          	at org.apache.hadoop.crypto.key.kms.server.KMSWebServer.<init>(KMSWebServer.java:91)
          	at org.apache.hadoop.crypto.key.kms.server.KMSWebServer.main(KMSWebServer.java:150)
          
          2017-04-26 12:51:02,975 INFO conf.ConfigurationWithLogging: Got ssl.server.keystore.password = '<redacted>'
          Exception in thread "main" java.io.IOException: Property ssl.server.keystore.password not specified
          	at org.apache.hadoop.http.HttpServer2$Builder.loadSSLConfiguration(HttpServer2.java:384)
          	at org.apache.hadoop.http.HttpServer2$Builder.build(HttpServer2.java:419)
          	at org.apache.hadoop.crypto.key.kms.server.KMSWebServer.<init>(KMSWebServer.java:91)
          	at org.apache.hadoop.crypto.key.kms.server.KMSWebServer.main(KMSWebServer.java:150)
          
          Show
          jzhuge John Zhuge added a comment - Patch 003 Fix merge issue in 003 Sample exception when keystore location or password not specified: 2017-04-26 12:48:55,145 INFO conf.ConfigurationWithLogging: Got ssl.server.keystore.location = 'null' Exception in thread "main" java.io.IOException: Property ssl.server.keystore.location not specified at org.apache.hadoop.http.HttpServer2$Builder.loadSSLConfiguration(HttpServer2.java:378) at org.apache.hadoop.http.HttpServer2$Builder.build(HttpServer2.java:419) at org.apache.hadoop.crypto.key.kms.server.KMSWebServer.<init>(KMSWebServer.java:91) at org.apache.hadoop.crypto.key.kms.server.KMSWebServer.main(KMSWebServer.java:150) 2017-04-26 12:51:02,975 INFO conf.ConfigurationWithLogging: Got ssl.server.keystore.password = '<redacted>' Exception in thread "main" java.io.IOException: Property ssl.server.keystore.password not specified at org.apache.hadoop.http.HttpServer2$Builder.loadSSLConfiguration(HttpServer2.java:384) at org.apache.hadoop.http.HttpServer2$Builder.build(HttpServer2.java:419) at org.apache.hadoop.crypto.key.kms.server.KMSWebServer.<init>(KMSWebServer.java:91) at org.apache.hadoop.crypto.key.kms.server.KMSWebServer.main(KMSWebServer.java:150)
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 0m 13s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
          +1 mvninstall 14m 50s trunk passed
          +1 compile 17m 28s trunk passed
          +1 checkstyle 0m 38s trunk passed
          +1 mvnsite 1m 9s trunk passed
          +1 mvneclipse 0m 19s trunk passed
          -1 findbugs 1m 26s hadoop-common-project/hadoop-common in trunk has 17 extant Findbugs warnings.
          +1 javadoc 0m 53s trunk passed
          +1 mvninstall 0m 47s the patch passed
          +1 compile 14m 50s the patch passed
          +1 javac 14m 50s the patch passed
          +1 checkstyle 0m 40s the patch passed
          +1 mvnsite 1m 13s the patch passed
          +1 mvneclipse 0m 21s the patch passed
          +1 whitespace 0m 0s The patch has no whitespace issues.
          +1 findbugs 1m 38s the patch passed
          +1 javadoc 0m 50s the patch passed
          -1 unit 8m 27s hadoop-common in the patch failed.
          +1 asflicense 0m 34s The patch does not generate ASF License warnings.
          68m 15s



          Reason Tests
          Failed junit tests hadoop.security.TestKDiag



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:0ac17dc
          JIRA Issue HADOOP-14352
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12865204/HADOOP-14352.003.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 15c92a0c6110 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision trunk / 8b5f2c3
          Default Java 1.8.0_121
          findbugs v3.1.0-RC1
          findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/12197/artifact/patchprocess/branch-findbugs-hadoop-common-project_hadoop-common-warnings.html
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12197/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt
          Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12197/testReport/
          modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12197/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 0m 13s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. -1 test4tests 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 mvninstall 14m 50s trunk passed +1 compile 17m 28s trunk passed +1 checkstyle 0m 38s trunk passed +1 mvnsite 1m 9s trunk passed +1 mvneclipse 0m 19s trunk passed -1 findbugs 1m 26s hadoop-common-project/hadoop-common in trunk has 17 extant Findbugs warnings. +1 javadoc 0m 53s trunk passed +1 mvninstall 0m 47s the patch passed +1 compile 14m 50s the patch passed +1 javac 14m 50s the patch passed +1 checkstyle 0m 40s the patch passed +1 mvnsite 1m 13s the patch passed +1 mvneclipse 0m 21s the patch passed +1 whitespace 0m 0s The patch has no whitespace issues. +1 findbugs 1m 38s the patch passed +1 javadoc 0m 50s the patch passed -1 unit 8m 27s hadoop-common in the patch failed. +1 asflicense 0m 34s The patch does not generate ASF License warnings. 68m 15s Reason Tests Failed junit tests hadoop.security.TestKDiag Subsystem Report/Notes Docker Image:yetus/hadoop:0ac17dc JIRA Issue HADOOP-14352 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12865204/HADOOP-14352.003.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 15c92a0c6110 3.13.0-106-generic #153-Ubuntu SMP Tue Dec 6 15:44:32 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision trunk / 8b5f2c3 Default Java 1.8.0_121 findbugs v3.1.0-RC1 findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/12197/artifact/patchprocess/branch-findbugs-hadoop-common-project_hadoop-common-warnings.html unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12197/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12197/testReport/ modules C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12197/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          jzhuge John Zhuge added a comment -

          TestKDiag failure is unrelated.

          Show
          jzhuge John Zhuge added a comment - TestKDiag failure is unrelated.
          Hide
          rkanter Robert Kanter added a comment -

          +1

          Show
          rkanter Robert Kanter added a comment - +1
          Hide
          rkanter Robert Kanter added a comment -

          Thanks John Zhuge. Committed to trunk!

          Show
          rkanter Robert Kanter added a comment - Thanks John Zhuge . Committed to trunk!
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11676 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11676/)
          HADOOP-14352. Make some HttpServer2 SSL properties optional (jzhuge via (rkanter: rev 8b82317fab0cb3023da333d4d557e226712a9c92)

          • (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11676 (See https://builds.apache.org/job/Hadoop-trunk-Commit/11676/ ) HADOOP-14352 . Make some HttpServer2 SSL properties optional (jzhuge via (rkanter: rev 8b82317fab0cb3023da333d4d557e226712a9c92) (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
          Hide
          jzhuge John Zhuge added a comment -

          Thanks Robert Kanter for the review and commit!

          Show
          jzhuge John Zhuge added a comment - Thanks Robert Kanter for the review and commit!

            People

            • Assignee:
              jzhuge John Zhuge
              Reporter:
              jzhuge John Zhuge
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development