Hadoop Common
  1. Hadoop Common
  2. HADOOP-10224

JavaKeyStoreProvider has to protect against corrupting underlying store

    Details

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

      Description

      Java keystores get corrupted at times. A key management operation that writes the store to disk could cause a corruption and all protected data would then be unaccessible.

      1. HADOOP-10224.9.patch
        15 kB
        Arun Suresh
      2. HADOOP-10224.8.patch
        13 kB
        Arun Suresh
      3. HADOOP-10224.7.patch
        13 kB
        Arun Suresh
      4. HADOOP-10224.6.patch
        13 kB
        Arun Suresh
      5. HADOOP-10224.5.patch
        13 kB
        Arun Suresh
      6. HADOOP-10224.4.patch
        12 kB
        Arun Suresh
      7. HADOOP-10224.3.patch
        12 kB
        Arun Suresh
      8. HADOOP-10224.2.patch
        7 kB
        Arun Suresh
      9. HADOOP-10224.1.patch
        6 kB
        Arun Suresh

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Mapreduce-trunk #1858 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1858/)
          HADOOP-10224. JavaKeyStoreProvider has to protect against corrupting underlying store. (asuresh via tucu) (tucu: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1616908)

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/JavaKeyStoreProvider.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/key/TestKeyProviderFactory.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-Mapreduce-trunk #1858 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1858/ ) HADOOP-10224 . JavaKeyStoreProvider has to protect against corrupting underlying store. (asuresh via tucu) (tucu: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1616908 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/JavaKeyStoreProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/key/TestKeyProviderFactory.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Hdfs-trunk #1832 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1832/)
          HADOOP-10224. JavaKeyStoreProvider has to protect against corrupting underlying store. (asuresh via tucu) (tucu: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1616908)

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/JavaKeyStoreProvider.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/key/TestKeyProviderFactory.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-Hdfs-trunk #1832 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1832/ ) HADOOP-10224 . JavaKeyStoreProvider has to protect against corrupting underlying store. (asuresh via tucu) (tucu: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1616908 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/JavaKeyStoreProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/key/TestKeyProviderFactory.java
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Hadoop-Yarn-trunk #639 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/639/)
          HADOOP-10224. JavaKeyStoreProvider has to protect against corrupting underlying store. (asuresh via tucu) (tucu: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1616908)

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/JavaKeyStoreProvider.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/key/TestKeyProviderFactory.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Hadoop-Yarn-trunk #639 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/639/ ) HADOOP-10224 . JavaKeyStoreProvider has to protect against corrupting underlying store. (asuresh via tucu) (tucu: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1616908 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/JavaKeyStoreProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/key/TestKeyProviderFactory.java
          Hide
          Hudson added a comment -

          FAILURE: Integrated in Hadoop-trunk-Commit #6040 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6040/)
          HADOOP-10224. JavaKeyStoreProvider has to protect against corrupting underlying store. (asuresh via tucu) (tucu: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1616908)

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/JavaKeyStoreProvider.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/key/TestKeyProviderFactory.java
          Show
          Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #6040 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6040/ ) HADOOP-10224 . JavaKeyStoreProvider has to protect against corrupting underlying store. (asuresh via tucu) (tucu: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1616908 ) /hadoop/common/trunk/hadoop-common-project/hadoop-common/CHANGES.txt /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/JavaKeyStoreProvider.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/key/TestKeyProviderFactory.java
          Hide
          Alejandro Abdelnur added a comment -

          Thanks Arun, nice work. Thanks Larry for reviewing it in detail. Committed to trunk.

          Show
          Alejandro Abdelnur added a comment - Thanks Arun, nice work. Thanks Larry for reviewing it in detail. Committed to trunk.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 1 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 2.0.3) warnings.

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

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4444//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4444//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/12660714/HADOOP-10224.9.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 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 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4444//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4444//console This message is automatically generated.
          Hide
          Larry McCay added a comment -

          Thank you, Arun!
          +1

          Show
          Larry McCay added a comment - Thank you, Arun! +1
          Hide
          Alejandro Abdelnur added a comment -

          +1 pending jenkins.

          Show
          Alejandro Abdelnur added a comment - +1 pending jenkins.
          Hide
          Arun Suresh added a comment -

          Larry McCay, Thank you for your detailed review and suggestions.
          Updating the patch addressing your feedback comments...

          Show
          Arun Suresh added a comment - Larry McCay , Thank you for your detailed review and suggestions. Updating the patch addressing your feedback comments...
          Hide
          Larry McCay added a comment -

          Hi Arun Suresh -

          Sorry for taking longer than I anticipated to review this.
          I understood what you were doing with the methods but something doesn't feel quite right about how similar they actually are. There are a couple things that I think would make it easier to understand:

          • javadocs on those private methods that really explain what is being done
          • the tryLoadFromPath takes a path argument but is being passed the oldPath which seems wrong from where it is being called. You have to read the method to understand that it is being passed to clean up or to fallback to. Either pass both paths as arguments, make them all member variables or somehow change the name of the method to make it more clear that it is the right argument to pass.
          • inside flush - I think you could extract a couple methods like backupOld, restoreOld or something to that effect. We are basically implementing a common pattern of backup, write-new and swap the current to new or old versions. We should be able to code to methods that are representing that pattern clearly.

          Functionally, it all seems to hold together to me.
          Given sufficient javadocs and maybe some new methods and names - I think it will be ready to go.

          Thanks!

          Show
          Larry McCay added a comment - Hi Arun Suresh - Sorry for taking longer than I anticipated to review this. I understood what you were doing with the methods but something doesn't feel quite right about how similar they actually are. There are a couple things that I think would make it easier to understand: javadocs on those private methods that really explain what is being done the tryLoadFromPath takes a path argument but is being passed the oldPath which seems wrong from where it is being called. You have to read the method to understand that it is being passed to clean up or to fallback to. Either pass both paths as arguments, make them all member variables or somehow change the name of the method to make it more clear that it is the right argument to pass. inside flush - I think you could extract a couple methods like backupOld, restoreOld or something to that effect. We are basically implementing a common pattern of backup, write-new and swap the current to new or old versions. We should be able to code to methods that are representing that pattern clearly. Functionally, it all seems to hold together to me. Given sufficient javadocs and maybe some new methods and names - I think it will be ready to go. Thanks!
          Hide
          Arun Suresh added a comment -

          Larry McCay, Thank you for the review and feedback. Will post a followup patch with your suggestions. Couple of things though :

          • .. consolidate the tryLoadFromPath and loadAndReturnPerm methods ..

            The try.. methods were intended to mark the different code paths and thereby reduce the size of the constructor making it a bit more readable. The loadAndReturnPerm method is more a utility method that is used in a couple of places. I agree it might look similar, but I feel we should keep it as it is to aid the readability.

          • I'd also like to know whether and how this has been functionally tested with concurrent activity in order to tease out all of these scenarios. I am a little concerned about _NEW and _OLDs getting stepped on by concurrent requests. Especially in KMS.

            So, The KMS is always configured with a single instance of the JKS and the flush() method is protected by a writeLock. The constructor is the only other thing that can mess up the _NEW and _OLD files, which I don't think will be called concurrently.

          Show
          Arun Suresh added a comment - Larry McCay , Thank you for the review and feedback. Will post a followup patch with your suggestions. Couple of things though : .. consolidate the tryLoadFromPath and loadAndReturnPerm methods .. The try.. methods were intended to mark the different code paths and thereby reduce the size of the constructor making it a bit more readable. The loadAndReturnPerm method is more a utility method that is used in a couple of places. I agree it might look similar, but I feel we should keep it as it is to aid the readability. I'd also like to know whether and how this has been functionally tested with concurrent activity in order to tease out all of these scenarios. I am a little concerned about _NEW and _OLDs getting stepped on by concurrent requests. Especially in KMS. So, The KMS is always configured with a single instance of the JKS and the flush() method is protected by a writeLock. The constructor is the only other thing that can mess up the _NEW and _OLD files, which I don't think will be called concurrently.
          Hide
          Larry McCay added a comment -

          This is understandably a complicated dance and it seems that you have thought through a great number (if not all) of the complexities. I am going to need a little more time to grok them all.

          In the meantime, here are my initial observations:

          • the refactoring is in the right direction but seems like it may need a little more polishing
          • I am currently trying to see whether we can consolidate the tryLoadFromPath and loadAndReturnPerm methods - they are largely the same in implementation and goals.
          • throwing exception if config file specified but not found - changes default password behavior. This may be acceptable given that an explicit action of configuring a password file was taken but misconfigured but I just wanted to make sure that we were aware that this is short-circuiting the previous behavior.
          • comment for the Need to save off the permissions in case it has to be rewritten should be moved to where that is actually being done now - or perhaps this whole block could be in a saveOffPermissions method.
          • I think that the comments in the following block are misleading and should be reworded to indicate that we are testing whether flush has completed successfully or they should moved inside the condition that proves that it hasn't:
                  if (fs.exists(path)) {
                    // flush did not proceed to completion
                    // _NEW should not exist
                    if (fs.exists(newPath)) {
                      throw new IOException(
                          String.format("Keystore not loaded due to some inconsistency "
                          + "('%s' and '%s' should not exist together)!!", path, newPath));
                    }
            

          I'd also like to know whether and how this has been functionally tested with concurrent activity in order to tease out all of these scenarios. I am a little concerned about _NEW and _OLDs getting stepped on by concurrent requests. Especially in KMS.

          Show
          Larry McCay added a comment - This is understandably a complicated dance and it seems that you have thought through a great number (if not all) of the complexities. I am going to need a little more time to grok them all. In the meantime, here are my initial observations: the refactoring is in the right direction but seems like it may need a little more polishing I am currently trying to see whether we can consolidate the tryLoadFromPath and loadAndReturnPerm methods - they are largely the same in implementation and goals. throwing exception if config file specified but not found - changes default password behavior. This may be acceptable given that an explicit action of configuring a password file was taken but misconfigured but I just wanted to make sure that we were aware that this is short-circuiting the previous behavior. comment for the Need to save off the permissions in case it has to be rewritten should be moved to where that is actually being done now - or perhaps this whole block could be in a saveOffPermissions method. I think that the comments in the following block are misleading and should be reworded to indicate that we are testing whether flush has completed successfully or they should moved inside the condition that proves that it hasn't: if (fs.exists(path)) { // flush did not proceed to completion // _NEW should not exist if (fs.exists(newPath)) { throw new IOException( String .format( "Keystore not loaded due to some inconsistency " + "('%s' and '%s' should not exist together)!!" , path, newPath)); } I'd also like to know whether and how this has been functionally tested with concurrent activity in order to tease out all of these scenarios. I am a little concerned about _NEW and _OLDs getting stepped on by concurrent requests. Especially in KMS.
          Hide
          Larry McCay added a comment -

          Reviewing in detail now - will have feedback shortly.

          Show
          Larry McCay added a comment - Reviewing in detail now - will have feedback shortly.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 1 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 2.0.3) warnings.

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

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4432//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4432//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/12659844/HADOOP-10224.8.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 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 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4432//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4432//console This message is automatically generated.
          Hide
          Larry McCay added a comment -

          Thanks, Arun Suresh! That should make it easier to read. I will take a look at the general behavior tonight. Thanks again.

          Show
          Larry McCay added a comment - Thanks, Arun Suresh ! That should make it easier to read. I will take a look at the general behavior tonight. Thanks again.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 1 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 2.0.3) 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-common:

          org.apache.hadoop.ipc.TestRPCCallBenchmark
          org.apache.hadoop.net.TestNetUtils
          org.apache.hadoop.security.TestSecurityUtil
          org.apache.hadoop.crypto.key.TestKeyProviderFactory
          org.apache.hadoop.ipc.TestRPC
          org.apache.hadoop.security.TestDoAsEffectiveUser
          org.apache.hadoop.conf.TestConfiguration
          org.apache.hadoop.ipc.TestIPC

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4429//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4429//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/12659844/HADOOP-10224.8.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 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 2.0.3) 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-common: org.apache.hadoop.ipc.TestRPCCallBenchmark org.apache.hadoop.net.TestNetUtils org.apache.hadoop.security.TestSecurityUtil org.apache.hadoop.crypto.key.TestKeyProviderFactory org.apache.hadoop.ipc.TestRPC org.apache.hadoop.security.TestDoAsEffectiveUser org.apache.hadoop.conf.TestConfiguration org.apache.hadoop.ipc.TestIPC +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4429//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4429//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/12659844/HADOOP-10224.8.patch
          against trunk revision .

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

          +1 tests included. The patch appears to include 1 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 2.0.3) 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-common:

          org.apache.hadoop.net.TestNetUtils
          org.apache.hadoop.security.TestDoAsEffectiveUser
          org.apache.hadoop.ipc.TestRPC
          org.apache.hadoop.ipc.TestRPCCallBenchmark
          org.apache.hadoop.crypto.key.TestKeyProviderFactory
          org.apache.hadoop.security.TestSecurityUtil
          org.apache.hadoop.ipc.TestIPC
          org.apache.hadoop.conf.TestConfiguration

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4427//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4427//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/12659844/HADOOP-10224.8.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 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 2.0.3) 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-common: org.apache.hadoop.net.TestNetUtils org.apache.hadoop.security.TestDoAsEffectiveUser org.apache.hadoop.ipc.TestRPC org.apache.hadoop.ipc.TestRPCCallBenchmark org.apache.hadoop.crypto.key.TestKeyProviderFactory org.apache.hadoop.security.TestSecurityUtil org.apache.hadoop.ipc.TestIPC org.apache.hadoop.conf.TestConfiguration +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4427//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4427//console This message is automatically generated.
          Hide
          Arun Suresh added a comment -

          Larry McCay, Ive re-factored the JKS constructor a bit. Hope this helps to make it a bit more readable.

          Show
          Arun Suresh added a comment - Larry McCay , Ive re-factored the JKS constructor a bit. Hope this helps to make it a bit more readable.
          Hide
          Larry McCay added a comment -

          Thanks for the patch for this - it will be helpful in both the KeyProvider and CredentialProvider APIs!

          Personally, I think that we can make that method a bit more concise and readable - consolidate the boilerplate code that verifies that the old or new keystores can be loaded, renamed and/or deleted into a common method. Also, break the other exception handling blocks into separate methods. This is just for readability and maintainability.

          I will try and dig into the actual behavior there tomorrow. Hopefully, with a more easily read version?

          Show
          Larry McCay added a comment - Thanks for the patch for this - it will be helpful in both the KeyProvider and CredentialProvider APIs! Personally, I think that we can make that method a bit more concise and readable - consolidate the boilerplate code that verifies that the old or new keystores can be loaded, renamed and/or deleted into a common method. Also, break the other exception handling blocks into separate methods. This is just for readability and maintainability. I will try and dig into the actual behavior there tomorrow. Hopefully, with a more easily read version?
          Hide
          Larry McCay added a comment -

          I will have a look tonight or tomorrow, Alejandro Abdelnur.
          Thanks for the opportunity to do so.

          Show
          Larry McCay added a comment - I will have a look tonight or tomorrow, Alejandro Abdelnur . Thanks for the opportunity to do so.
          Hide
          Alejandro Abdelnur added a comment -

          LGTM +1. Larry McCay, do you want to review the latest patch? I'll wait till WED morning to commit.

          Show
          Alejandro Abdelnur added a comment - LGTM +1. Larry McCay , do you want to review the latest patch? I'll wait till WED morning to commit.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 1 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 2.0.3) 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-common:

          org.apache.hadoop.metrics2.impl.TestMetricsSystemImpl

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4419//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4419//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/12659685/HADOOP-10224.7.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 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 2.0.3) 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-common: org.apache.hadoop.metrics2.impl.TestMetricsSystemImpl +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4419//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4419//console This message is automatically generated.
          Hide
          Arun Suresh added a comment -

          Minor re-factor

          Show
          Arun Suresh added a comment - Minor re-factor
          Hide
          Arun Suresh added a comment -

          The above test errors are un-related to the patch.

          Show
          Arun Suresh added a comment - The above test errors are un-related to the patch.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 1 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 2.0.3) 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-common:

          org.apache.hadoop.metrics2.impl.TestMetricsSystemImpl
          org.apache.hadoop.ha.TestZKFailoverControllerStress

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4413//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4413//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/12659530/HADOOP-10224.6.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 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 2.0.3) 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-common: org.apache.hadoop.metrics2.impl.TestMetricsSystemImpl org.apache.hadoop.ha.TestZKFailoverControllerStress +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4413//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4413//console This message is automatically generated.
          Hide
          Arun Suresh added a comment -

          Hi Larry McCay, I was wondering if you were ok with the patch or if you had any further suggestions with regard to this.

          Show
          Arun Suresh added a comment - Hi Larry McCay , I was wondering if you were ok with the patch or if you had any further suggestions with regard to this.
          Hide
          Arun Suresh added a comment -

          Updating patch with minor error message modifications.

          Show
          Arun Suresh added a comment - Updating patch with minor error message modifications.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 1 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 2.0.3) warnings.

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

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4404//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4404//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/12659242/HADOOP-10224.5.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 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 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4404//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4404//console This message is automatically generated.
          Hide
          Arun Suresh added a comment -

          Added one more test

          Show
          Arun Suresh added a comment - Added one more test
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 1 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 2.0.3) warnings.

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

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4403//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4403//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/12659217/HADOOP-10224.4.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 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 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4403//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4403//console This message is automatically generated.
          Hide
          Arun Suresh added a comment -

          Updated patch. thanx Alejandro Abdelnur for the reviews

          Show
          Arun Suresh added a comment - Updated patch. thanx Alejandro Abdelnur for the reviews
          Hide
          Alejandro Abdelnur added a comment -

          In the constructor, the throw new IOException("Keystore cannot be loaded !!"); should be more descriptive of the problem, why is the exception is being thrown, that CURRENT & _NEW exists, and it should not be the case, only one should be there.

          In the constructor, when loading the CURRENT and having an error other than bad password we should log that CURRENT was corrupted and we loaded OLD. We should rename CURRENT to BAD$DATE, and report that as well for an admin to delete it.

          When loading _NEW and corrupt, we should report that in the exception message clearly. Also, shouldn’t we be renaming _NEW to CURRENT here?

          "// Check if _NEW exists (incase flush had finished writing but not", typo "in case"

          "KeyStore intialized anew sucessfully" 2 typos, "KeyStore initialized anew successfully"

          isBadorWrongPassword() method, always use {}s for IF blocks.

          loadFromPath(), you don’t want to rename until you know you can load the keystore, i would do the rename outside of here.

          flush(), when renaming files verify rename is successful (boolean return value) and fail if not.

          Show
          Alejandro Abdelnur added a comment - In the constructor, the throw new IOException("Keystore cannot be loaded !!"); should be more descriptive of the problem, why is the exception is being thrown, that CURRENT & _NEW exists, and it should not be the case, only one should be there. In the constructor, when loading the CURRENT and having an error other than bad password we should log that CURRENT was corrupted and we loaded OLD. We should rename CURRENT to BAD $DATE, and report that as well for an admin to delete it. When loading _NEW and corrupt, we should report that in the exception message clearly. Also, shouldn’t we be renaming _NEW to CURRENT here? "// Check if _NEW exists (incase flush had finished writing but not", typo "in case" "KeyStore intialized anew sucessfully" 2 typos, "KeyStore initialized anew successfully" isBadorWrongPassword() method, always use {}s for IF blocks. loadFromPath() , you don’t want to rename until you know you can load the keystore, i would do the rename outside of here. flush() , when renaming files verify rename is successful (boolean return value) and fail if not.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 1 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 2.0.3) warnings.

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

          -1 core tests. The following test timeouts occurred in hadoop-common-project/hadoop-common:

          org.apache.hadoop.http.TestHttpServerLifecycle

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4397//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4397//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/12659118/HADOOP-10224.3.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 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 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. -1 core tests . The following test timeouts occurred in hadoop-common-project/hadoop-common: org.apache.hadoop.http.TestHttpServerLifecycle +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4397//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4397//console This message is automatically generated.
          Hide
          Arun Suresh added a comment -

          Updated patch with Alejandro Abdelnur's feedback suggestions

          Show
          Arun Suresh added a comment - Updated patch with Alejandro Abdelnur 's feedback suggestions
          Hide
          Arun Suresh added a comment -

          Alejandro Abdelnur, in my previous comment, for the first item, I now see why you need to throw an exception.. please ignore.

          Show
          Arun Suresh added a comment - Alejandro Abdelnur , in my previous comment, for the first item, I now see why you need to throw an exception.. please ignore.
          Hide
          Arun Suresh added a comment -

          Alejandro Abdelnur, thanx for the review..

          ..
            if (fs.exists(keyStorePath)) {
              if (fs.exists(newPath)) {
                //THROW EXCEPTION, something weird happened, admin should take care of
              }
          ..
          

          Should we actually throw an exception here ? If new exists, it would imply that the flush did not proceed to completion.. in which case, I was thinking, the JKS should restart with the last know good configuration silently. Not sure if the admin should be flagged at this point.

          Also, during startup,

          ..
          if (fs.exists(newPath) || fs.exists(oldPath)) {
                if (fs.exists(newPath)) {
                  try {
          ..
          

          I was wondering if we have to check if "new" exist ? Ideally, on startup, the JKS should be brought to one of two states : "old" state or a completely flushed state ("current"). the "new" file in my opinion is an intermediate file (and should be deleted by flush() if it proceeds to completion)

          I was also wondering if :

          if (fs.exists(newPath) || fs.exists(oldPath)) {
              //THROW EXCEPTION, something weird happened, admin should take care of
            }
          

          is needed, since we can safely assume that if the JKS has initialized properly, there should not exits a "new" and "old" file at the time of flush. no ?

          Show
          Arun Suresh added a comment - Alejandro Abdelnur , thanx for the review.. .. if (fs.exists(keyStorePath)) { if (fs.exists(newPath)) { //THROW EXCEPTION, something weird happened, admin should take care of } .. Should we actually throw an exception here ? If new exists, it would imply that the flush did not proceed to completion.. in which case, I was thinking, the JKS should restart with the last know good configuration silently. Not sure if the admin should be flagged at this point. Also, during startup, .. if (fs.exists(newPath) || fs.exists(oldPath)) { if (fs.exists(newPath)) { try { .. I was wondering if we have to check if "new" exist ? Ideally, on startup, the JKS should be brought to one of two states : "old" state or a completely flushed state ("current"). the "new" file in my opinion is an intermediate file (and should be deleted by flush() if it proceeds to completion) I was also wondering if : if (fs.exists(newPath) || fs.exists(oldPath)) { //THROW EXCEPTION, something weird happened, admin should take care of } is needed, since we can safely assume that if the JKS has initialized properly, there should not exits a "new" and "old" file at the time of flush. no ?
          Hide
          Alejandro Abdelnur added a comment -

          JavaKeyStoreProvider.java:

          • if ((pwFile != null)&&(pwdFile == null)), no need to check for pwFile not NULL here, we can be here only if it is not NULL already.

          I think we should go over all corner cases (even if not happening under normal circumstances).

          On startup should be something like:

            boolean loaded = false;
            Path keyStorePath = ....
            Path newPath = constructNewPath(path);
            Path oldPath = constructOldPath(path);
            FSPermission permission = ...
            if (fs.exists(keyStorePath)) {
              if (fs.exists(newPath)) {
                //THROW EXCEPTION, something weird happened, admin should take care of
              }
              keyStore = loadKeyStore(path, password);
              if (fs.exists(oldPath)) {
                fs.delete(oldPath);
              }
              loaded = true;
              //LOG
            } else {
              if (fs.exists(newPath) || fs.exists(oldPath)) {
                if (fs.exists(newPath)) {
                  try {
                    keyStore = loadKeyStore(newPath, password);
                    fs.rename(newPath, path);
                    fs.delete(oldPath);
                    loaded = true;
                    //LOG
                  } catch (Exception ex) {
                    //THROW EXCEPTION if password issue, we don’t want to trash the new file because of wrong password, admin should take care
                  }
                }
                if (!loaded) {
                  if (fs.exists(oldPath)) {
                    try {
                      keyStore = loadKeyStore(oldPath, password);
                      fs.rename(oldPath, path);
                      loaded = true;
                      //LOG
                    } catch (Exception ex) {
                    //THROW EXCEPTION if password issue, we don’t want to trash the new file because of wrong password, admin should take care
                    }
                  } else {
                    //LOG
                  }
                }
              } else {
                //LOG
              }
            }
            if (!loaded) {
              // creating an empty store
              keyStore = KeyStore.getInstance(SCHEME_NAME);
              OutputStream out = FileSystem.create(fs, path, permissions);
              keyStore.store(out, password);
              out.close();
              //LOG
            }
          

          On flush code should be something like:

            Path keyStorePath = ....
            Path newPath = constructNewPath(path);
            Path oldPath = constructOldPath(path);
            FSPermission permission = ...
            if (fs.exists(newPath) || fs.exists(oldPath)) {
              //THROW EXCEPTION, something weird happened, admin should take care of
            }
            fs.rename(path, oldPath);
            try {
              OutputStream out = FileSystem.create(fs, newPath, permissions);
              keyStore.store(out, password);
              out.close();
            } catch (Exception ex) {
              fs.rename(oldPath, path);
              //THROW EXCEPTION
            }
            fs.rename(newPath, path); //assert it happens else we need to revert and throw exception
            fs.delete(oldPath); //LOG WARN if does not happen.
          
          Show
          Alejandro Abdelnur added a comment - JavaKeyStoreProvider.java : if ((pwFile != null)&&(pwdFile == null)) , no need to check for pwFile not NULL here, we can be here only if it is not NULL already. I think we should go over all corner cases (even if not happening under normal circumstances). On startup should be something like: boolean loaded = false ; Path keyStorePath = .... Path newPath = constructNewPath(path); Path oldPath = constructOldPath(path); FSPermission permission = ... if (fs.exists(keyStorePath)) { if (fs.exists(newPath)) { //THROW EXCEPTION, something weird happened, admin should take care of } keyStore = loadKeyStore(path, password); if (fs.exists(oldPath)) { fs.delete(oldPath); } loaded = true ; //LOG } else { if (fs.exists(newPath) || fs.exists(oldPath)) { if (fs.exists(newPath)) { try { keyStore = loadKeyStore(newPath, password); fs.rename(newPath, path); fs.delete(oldPath); loaded = true ; //LOG } catch (Exception ex) { //THROW EXCEPTION if password issue, we don’t want to trash the new file because of wrong password, admin should take care } } if (!loaded) { if (fs.exists(oldPath)) { try { keyStore = loadKeyStore(oldPath, password); fs.rename(oldPath, path); loaded = true ; //LOG } catch (Exception ex) { //THROW EXCEPTION if password issue, we don’t want to trash the new file because of wrong password, admin should take care } } else { //LOG } } } else { //LOG } } if (!loaded) { // creating an empty store keyStore = KeyStore.getInstance(SCHEME_NAME); OutputStream out = FileSystem.create(fs, path, permissions); keyStore.store(out, password); out.close(); //LOG } On flush code should be something like: Path keyStorePath = .... Path newPath = constructNewPath(path); Path oldPath = constructOldPath(path); FSPermission permission = ... if (fs.exists(newPath) || fs.exists(oldPath)) { //THROW EXCEPTION, something weird happened, admin should take care of } fs.rename(path, oldPath); try { OutputStream out = FileSystem.create(fs, newPath, permissions); keyStore.store(out, password); out.close(); } catch (Exception ex) { fs.rename(oldPath, path); //THROW EXCEPTION } fs.rename(newPath, path); // assert it happens else we need to revert and throw exception fs.delete(oldPath); //LOG WARN if does not happen.
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 1 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 2.0.3) 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-common:

          org.apache.hadoop.ipc.TestRPCCallBenchmark
          org.apache.hadoop.net.TestNetUtils
          org.apache.hadoop.security.TestSecurityUtil
          org.apache.hadoop.crypto.key.TestKeyProviderFactory
          org.apache.hadoop.ipc.TestRPC
          org.apache.hadoop.security.TestDoAsEffectiveUser
          org.apache.hadoop.conf.TestConfiguration
          org.apache.hadoop.ipc.TestIPC

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4396//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/4396//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4396//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/12658939/HADOOP-10224.2.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 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 2.0.3) 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-common: org.apache.hadoop.ipc.TestRPCCallBenchmark org.apache.hadoop.net.TestNetUtils org.apache.hadoop.security.TestSecurityUtil org.apache.hadoop.crypto.key.TestKeyProviderFactory org.apache.hadoop.ipc.TestRPC org.apache.hadoop.security.TestDoAsEffectiveUser org.apache.hadoop.conf.TestConfiguration org.apache.hadoop.ipc.TestIPC +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4396//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/4396//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4396//console This message is automatically generated.
          Hide
          Arun Suresh added a comment -

          Updated patch to fix testcase

          Show
          Arun Suresh added a comment - Updated patch to fix testcase
          Hide
          Hadoop QA added a comment -

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

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

          +1 tests included. The patch appears to include 1 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 2.0.3) 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-common:

          org.apache.hadoop.ipc.TestRPCCallBenchmark
          org.apache.hadoop.net.TestNetUtils
          org.apache.hadoop.security.TestSecurityUtil
          org.apache.hadoop.crypto.key.TestKeyProviderFactory
          org.apache.hadoop.ipc.TestRPC
          org.apache.hadoop.security.TestDoAsEffectiveUser
          org.apache.hadoop.conf.TestConfiguration
          org.apache.hadoop.ipc.TestIPC

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

          Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4395//testReport/
          Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/4395//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html
          Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4395//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/12658926/HADOOP-10224.1.patch against trunk revision . +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 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 2.0.3) 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-common: org.apache.hadoop.ipc.TestRPCCallBenchmark org.apache.hadoop.net.TestNetUtils org.apache.hadoop.security.TestSecurityUtil org.apache.hadoop.crypto.key.TestKeyProviderFactory org.apache.hadoop.ipc.TestRPC org.apache.hadoop.security.TestDoAsEffectiveUser org.apache.hadoop.conf.TestConfiguration org.apache.hadoop.ipc.TestIPC +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/4395//testReport/ Findbugs warnings: https://builds.apache.org/job/PreCommit-HADOOP-Build/4395//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-common.html Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/4395//console This message is automatically generated.
          Hide
          Arun Suresh added a comment -

          Attaching initial patch

          Show
          Arun Suresh added a comment - Attaching initial patch
          Hide
          Alejandro Abdelnur added a comment -

          Thanks Larry.

          Show
          Alejandro Abdelnur added a comment - Thanks Larry.
          Hide
          Larry McCay added a comment -

          Please, Alejandro Abdelnur - have at it!
          Thanks for asking though.
          I was just pointing out that the two were duplicates.
          It is especially vulnerable to corruption while backing the KMS with a greater potential for concurrent access - so it needs to be addressed.

          Show
          Larry McCay added a comment - Please, Alejandro Abdelnur - have at it! Thanks for asking though. I was just pointing out that the two were duplicates. It is especially vulnerable to corruption while backing the KMS with a greater potential for concurrent access - so it needs to be addressed.
          Hide
          Alejandro Abdelnur added a comment -

          Larry McCay, are you planning to work on this JIRA anytime soon? Else, Arun Suresh or myself may have cycles to do it as we want this done by the time HDFS encryption is complete.

          Show
          Alejandro Abdelnur added a comment - Larry McCay , are you planning to work on this JIRA anytime soon? Else, Arun Suresh or myself may have cycles to do it as we want this done by the time HDFS encryption is complete.
          Hide
          Alejandro Abdelnur added a comment -

          As commented in HADOOP-10869, this could be done as follows:

          KeyProvider should flush() to a temporary file, load temporary file to verify it is healthy and then rename the temporary file to the actual file. The rename must be done in 2 steps to enable recovery on startup if the rename process was not complete:

          • rename CURRENT to OLD
          • rename NEW to CURRENT
          • delete OLD

          On start up:

          • if CURRENT exists, delete NEW & OLD
          • if CURRENT does not exist and OLD exists, rename OLD back to CURRENT and delete NEW
          Show
          Alejandro Abdelnur added a comment - As commented in HADOOP-10869 , this could be done as follows: KeyProvider should flush() to a temporary file, load temporary file to verify it is healthy and then rename the temporary file to the actual file. The rename must be done in 2 steps to enable recovery on startup if the rename process was not complete: rename CURRENT to OLD rename NEW to CURRENT delete OLD On start up: if CURRENT exists, delete NEW & OLD if CURRENT does not exist and OLD exists, rename OLD back to CURRENT and delete NEW
          Hide
          Larry McCay added a comment -

          I am thinking that the flush() needs to backup the original store to a randomized name, write the contents of the store, reload and maybe list the keys to ensure that it isn't corrupt.

          Once it has proven to not e corrupt we can delete the backup.

          Show
          Larry McCay added a comment - I am thinking that the flush() needs to backup the original store to a randomized name, write the contents of the store, reload and maybe list the keys to ensure that it isn't corrupt. Once it has proven to not e corrupt we can delete the backup.

            People

            • Assignee:
              Arun Suresh
              Reporter:
              Larry McCay
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development