Hadoop Common
  1. Hadoop Common
  2. HADOOP-5364

Adding SSL certificate expiration warning to hdfsproxy

    Details

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

      Description

      SSL certificate warning should be provided on both client and proxy server side.

      1. HADOOP-5364.patch
        9 kB
        zhiyong zhang
      2. HADOOP-5364.patch
        8 kB
        zhiyong zhang
      3. HADOOP-5364.patch
        8 kB
        zhiyong zhang
      4. HADOOP-5364.patch
        9 kB
        zhiyong zhang
      5. HADOOP-5364.patch
        10 kB
        zhiyong zhang
      6. HADOOP-5364.patch
        10 kB
        zhiyong zhang
      7. HADOOP-5364.patch
        10 kB
        zhiyong zhang
      8. HADOOP-5364.patch
        10 kB
        zhiyong zhang
      9. HADOOP-5364.patch
        10 kB
        zhiyong zhang
      10. HADOOP-5364.patch
        10 kB
        zhiyong zhang

        Activity

        Hide
        zhiyong zhang added a comment -
        • For client-side cert expiration warning, set a 30 day threshold. If the client cert is within 30 days of expiration, send a warning message to every client request before sending the actual response.
        • For server-side cert expiration, added such a functionality to proxyUtil. Its a performance annoyance to check the server-side cert expiration date on every client request. In case we want to know its expiration date, we can use proxyUtil to query that.

        zhiyong.

        Show
        zhiyong zhang added a comment - For client-side cert expiration warning, set a 30 day threshold. If the client cert is within 30 days of expiration, send a warning message to every client request before sending the actual response. For server-side cert expiration, added such a functionality to proxyUtil. Its a performance annoyance to check the server-side cert expiration date on every client request. In case we want to know its expiration date, we can use proxyUtil to query that. zhiyong.
        Hide
        Kan Zhang added a comment -

        The server-side warning isn't what we said earlier. Did you check with Ops
        on that?

        Show
        Kan Zhang added a comment - The server-side warning isn't what we said earlier. Did you check with Ops on that?
        Hide
        zhiyong zhang added a comment -

        yes. ops ok with this design

        Show
        zhiyong zhang added a comment - yes. ops ok with this design
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12403753/HADOOP-5364.patch
        against trunk revision 761082.

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no tests are needed for this patch.

        +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 warnings.

        +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

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

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

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

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/98/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/98/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/98/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/98/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/12403753/HADOOP-5364.patch against trunk revision 761082. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +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 warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/98/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/98/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/98/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-minerva.apache.org/98/console This message is automatically generated.
        Hide
        Kan Zhang added a comment -

        The server cert checking using ProxyUtil looks good. However, client cert checking should be done at the client-side and proxy server should not spend cycles on it for each request.

        Show
        Kan Zhang added a comment - The server cert checking using ProxyUtil looks good. However, client cert checking should be done at the client-side and proxy server should not spend cycles on it for each request.
        Hide
        zhiyong zhang added a comment -

        changed the client-side certs expiration check to HsftpFileSystem.java.

        Show
        zhiyong zhang added a comment - changed the client-side certs expiration check to HsftpFileSystem.java.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12405340/HADOOP-5364.patch
        against trunk revision 764605.

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no tests are needed for this patch.

        +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 appears to introduce 1 new Findbugs warnings.

        +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

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

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

        -1 contrib tests. The patch failed contrib unit tests.

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/194/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/194/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/194/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/194/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/12405340/HADOOP-5364.patch against trunk revision 764605. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +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 appears to introduce 1 new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. -1 contrib tests. The patch failed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/194/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/194/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/194/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/194/console This message is automatically generated.
        Hide
        zhiyong zhang added a comment -

        Changed SSL certs expiration checking from per keystore to only when connection builds.

        Show
        zhiyong zhang added a comment - Changed SSL certs expiration checking from per keystore to only when connection builds.
        Hide
        zhiyong zhang added a comment -

        How about moving
        " connection.setRequestMethod("GET");
        connection.connect(); " into the openConnection function.
        This way HsftpFileSystem does not need to override open function of HftpFileSystem.

        also to me it make more sense to really connect in the openConnection. It is how it was used everywhere.

        I uploaded the new patch to reflect that.

        Show
        zhiyong zhang added a comment - How about moving " connection.setRequestMethod("GET"); connection.connect(); " into the openConnection function. This way HsftpFileSystem does not need to override open function of HftpFileSystem. also to me it make more sense to really connect in the openConnection. It is how it was used everywhere. I uploaded the new patch to reflect that.
        Hide
        Chris Douglas added a comment -
        • The StringBuffer in HsftpFileSystem need not be built unless the certificate is expired.
        • Use System.currentTimeMillis instead of creating a new Date object and calling getTime
        • In ProxyUtil, "No Server certs was found" should be "No server certificates were found"
        • This will warn for every open/list operation in HsftpFileSystem if a certificate is expired? This seems excessive, particularly since checksum verification will generate at least two messages for each file. So this remains usable if the certificate is within the limit, consider making the 30 day check/warning configurable and disable it if it is 0. Is a warning for every request a requirement?
        Show
        Chris Douglas added a comment - The StringBuffer in HsftpFileSystem need not be built unless the certificate is expired. Use System.currentTimeMillis instead of creating a new Date object and calling getTime In ProxyUtil, "No Server certs was found" should be "No server certificates were found" This will warn for every open/list operation in HsftpFileSystem if a certificate is expired? This seems excessive, particularly since checksum verification will generate at least two messages for each file. So this remains usable if the certificate is within the limit, consider making the 30 day check/warning configurable and disable it if it is 0. Is a warning for every request a requirement?
        Hide
        Kan Zhang added a comment -

        > Is a warning for every request a requirement?
        Maybe only warn on the first use of an HsftpFileSystem object and only one warning message should be generated?

        Show
        Kan Zhang added a comment - > Is a warning for every request a requirement? Maybe only warn on the first use of an HsftpFileSystem object and only one warning message should be generated?
        Hide
        zhiyong zhang added a comment -

        Thanks for the comments and suggestions.

        In the new patch, I make the cert expiration check happen only once. Plus, I make the warning threshold configurable.

        Show
        zhiyong zhang added a comment - Thanks for the comments and suggestions. In the new patch, I make the cert expiration check happen only once. Plus, I make the warning threshold configurable.
        Hide
        zhiyong zhang added a comment -

        forgot to remove commented code in the previous patch.

        Show
        zhiyong zhang added a comment - forgot to remove commented code in the previous patch.
        Hide
        Chris Douglas added a comment -

        As Kan suggested, this should warn for each instance of HsftpFileSystem; this warns once for the life of the JVM and only one set of certs. If one were to connect to multiple servers w/ hsftp, only the first would be checked, +/- race conditions.

        Simply reading from the config and setting a member variable will work. When performing the cert expiration check for that handle (should be <= 0), set the member variable to 0 and dispense with the separate boolean flag. Since open/list are not synchronized, the member var should be volatile. The synchronization with this approach is not strictly correct; it's still possible to get multiple warnings from the same handle for multiple threads, but that's OK.

        Other:

        • The Date import in HsftpFileSystem is unnecessary
        • The expiration threshold property should include the units in which it is expressed. ssl.expiration.warn.days seems OK to me
        • Instead of setting curTime and performing the conversion for each cert, set the threshold to curTime + days * ms/day and warn if expTime < that.
        • The check should be disabled at the top, not the bottom of the block
        Show
        Chris Douglas added a comment - As Kan suggested, this should warn for each instance of HsftpFileSystem; this warns once for the life of the JVM and only one set of certs. If one were to connect to multiple servers w/ hsftp, only the first would be checked, +/- race conditions. Simply reading from the config and setting a member variable will work. When performing the cert expiration check for that handle (should be <= 0), set the member variable to 0 and dispense with the separate boolean flag. Since open/list are not synchronized, the member var should be volatile. The synchronization with this approach is not strictly correct; it's still possible to get multiple warnings from the same handle for multiple threads, but that's OK. Other: The Date import in HsftpFileSystem is unnecessary The expiration threshold property should include the units in which it is expressed. ssl.expiration.warn.days seems OK to me Instead of setting curTime and performing the conversion for each cert, set the threshold to curTime + days * ms/day and warn if expTime < that. The check should be disabled at the top, not the bottom of the block
        Hide
        zhiyong zhang added a comment -

        Since the warning message is not critical resource that would cause any trouble on the program running, I would think of using synchronize overkill.

        remember any type of synchronization (hardware or software) would induce some overhead.

        i re-uploaded the patch according to the comments.

        thanks.

        Show
        zhiyong zhang added a comment - Since the warning message is not critical resource that would cause any trouble on the program running, I would think of using synchronize overkill. remember any type of synchronization (hardware or software) would induce some overhead. i re-uploaded the patch according to the comments. thanks.
        Hide
        zhiyong zhang added a comment -

        removed "static" descriptor.

        Show
        zhiyong zhang added a comment - removed "static" descriptor.
        Hide
        Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12406731/HADOOP-5364.patch
        against trunk revision 770321.

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

        -1 tests included. The patch doesn't appear to include any new or modified tests.
        Please justify why no tests are needed for this patch.

        +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 warnings.

        +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

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

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

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

        Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/265/testReport/
        Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/265/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
        Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/265/artifact/trunk/build/test/checkstyle-errors.html
        Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/265/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/12406731/HADOOP-5364.patch against trunk revision 770321. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +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 warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/265/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/265/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/265/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch-vesta.apache.org/265/console This message is automatically generated.
        Hide
        Chris Douglas added a comment -

        I committed this. Thanks, Zhiyong

        Show
        Chris Douglas added a comment - I committed this. Thanks, Zhiyong
        Hide
        Hudson added a comment -

        Integrated in Hadoop-trunk #827 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/827/)
        . Add certificate expiration warning to HsftpFileSystem and HDFS proxy. Contributed by Zhiyong Zhang

        Show
        Hudson added a comment - Integrated in Hadoop-trunk #827 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/827/ ) . Add certificate expiration warning to HsftpFileSystem and HDFS proxy. Contributed by Zhiyong Zhang

          People

          • Assignee:
            zhiyong zhang
            Reporter:
            Kan Zhang
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development