Accumulo
  1. Accumulo
  2. ACCUMULO-2713

Instance secret written out with other configuration items to RFiles and WALogs when encryption is turned on

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.5.1
    • Fix Version/s: 1.6.0
    • Component/s: None
    • Labels:

      Description

      The encryption at rest feature records configuration information in order to encrypted RFiles and WALogs so that if the configuration changes, the files can be read back. The code that does this recording hovers up all the "instance.*" entries, and does not pick out the instance.secret as a special one not to write. Thus the instance secret goes into each file in the clear, which is non-ideal to say the least.

      Patch forthcoming.

        Activity

        Hide
        John Vines added a comment -

        Expanded to test for all sensitive properties and check the encryptedOutputStream which is used by DfsLogger

        Show
        John Vines added a comment - Expanded to test for all sensitive properties and check the encryptedOutputStream which is used by DfsLogger
        Hide
        ASF subversion and git services added a comment -

        Commit fbbe472a304ac9dd25850f80de65479d3b19e3a6 in accumulo's branch refs/heads/master from John Vines
        [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=fbbe472 ]

        ACCUMULO-2713 checking for all sensitive and raw output stream for walogs

        Show
        ASF subversion and git services added a comment - Commit fbbe472a304ac9dd25850f80de65479d3b19e3a6 in accumulo's branch refs/heads/master from John Vines [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=fbbe472 ] ACCUMULO-2713 checking for all sensitive and raw output stream for walogs
        Hide
        ASF subversion and git services added a comment -

        Commit fbbe472a304ac9dd25850f80de65479d3b19e3a6 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from John Vines
        [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=fbbe472 ]

        ACCUMULO-2713 checking for all sensitive and raw output stream for walogs

        Show
        ASF subversion and git services added a comment - Commit fbbe472a304ac9dd25850f80de65479d3b19e3a6 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from John Vines [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=fbbe472 ] ACCUMULO-2713 checking for all sensitive and raw output stream for walogs
        Hide
        Sean Busbey added a comment -
        • If this also leaked in the WAL, we should check in both the RFile test and the WALog test
        • per Christopher's suggestion, we should expand to keep out all annotated Sensitive properties
        Show
        Sean Busbey added a comment - If this also leaked in the WAL, we should check in both the RFile test and the WALog test per Christopher's suggestion, we should expand to keep out all annotated Sensitive properties
        Hide
        John Vines added a comment -

        Added patch with test

        Show
        John Vines added a comment - Added patch with test
        Hide
        ASF subversion and git services added a comment -

        Commit 6138a80f0b3252abbb1ac65e5c267f5ff7514ff6 in accumulo's branch refs/heads/master from John Vines
        [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=6138a80 ]

        ACCUMULO-2713 fixing Michael Allen's patch and adding test

        Show
        ASF subversion and git services added a comment - Commit 6138a80f0b3252abbb1ac65e5c267f5ff7514ff6 in accumulo's branch refs/heads/master from John Vines [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=6138a80 ] ACCUMULO-2713 fixing Michael Allen's patch and adding test
        Hide
        ASF subversion and git services added a comment -

        Commit 6138a80f0b3252abbb1ac65e5c267f5ff7514ff6 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from John Vines
        [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=6138a80 ]

        ACCUMULO-2713 fixing Michael Allen's patch and adding test

        Show
        ASF subversion and git services added a comment - Commit 6138a80f0b3252abbb1ac65e5c267f5ff7514ff6 in accumulo's branch refs/heads/1.6.0-SNAPSHOT from John Vines [ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=6138a80 ] ACCUMULO-2713 fixing Michael Allen's patch and adding test
        Hide
        Christopher Tubbs added a comment -

        Re: the patch, sensitive properties are labeled with an annotation. It'd be best to remove any instance properties with that @Sensitive annotation, rather than just a specific known one, to prevent regression. However, I'm having a difficult time understanding why CryptoModuleParameters includes instance properties in the first place. Perhaps they can just be removed entirely. I can't find where they would be used. Can anybody help me out here?

        Show
        Christopher Tubbs added a comment - Re: the patch, sensitive properties are labeled with an annotation. It'd be best to remove any instance properties with that @Sensitive annotation, rather than just a specific known one, to prevent regression. However, I'm having a difficult time understanding why CryptoModuleParameters includes instance properties in the first place. Perhaps they can just be removed entirely. I can't find where they would be used. Can anybody help me out here?
        Hide
        Alex Moundalexis added a comment -

        Simply put, fix it for 1.6.0:

        • community hat: it's the correct move
        • customer-awaiting-1.6.0 hat: another X days isn't going to matter

        It looks extremely poor for a security-minded distribution to cut a release with a known security flaw, moreso given recent vulnerabilities in the media. That is not the image we want to project to the community, nor code we want to provide to the same, whether the feature is experimental or new or old.

        My non-binding $0.02...

        Show
        Alex Moundalexis added a comment - Simply put, fix it for 1.6.0: community hat: it's the correct move customer-awaiting-1.6.0 hat: another X days isn't going to matter It looks extremely poor for a security-minded distribution to cut a release with a known security flaw, moreso given recent vulnerabilities in the media. That is not the image we want to project to the community, nor code we want to provide to the same, whether the feature is experimental or new or old. My non-binding $0.02...
        Hide
        Sean Busbey added a comment -

        I consider this a blocker. I will -1 any impacted release that does not include it. I will encourage others to do the same.

        We're supposed to be the conservative choice for security. Wether or not the feature is experimental is immaterial. Experimental should not mean "causes a known compromise of system security."

        Show
        Sean Busbey added a comment - I consider this a blocker. I will -1 any impacted release that does not include it. I will encourage others to do the same. We're supposed to be the conservative choice for security. Wether or not the feature is experimental is immaterial. Experimental should not mean "causes a known compromise of system security."
        Hide
        Christopher Tubbs added a comment -

        The crypto properties are still labeled as such.

        Show
        Christopher Tubbs added a comment - The crypto properties are still labeled as such.
        Hide
        John Vines added a comment -

        I don't consider it experimental in 1.6

        Show
        John Vines added a comment - I don't consider it experimental in 1.6
        Hide
        John Vines added a comment - - edited

        1.5 might not be affected, actually

        Show
        John Vines added a comment - - edited 1.5 might not be affected, actually
        Hide
        Christopher Tubbs added a comment -

        Isn't encryption at rest an experimental feature? I'm not sure this warrants a blocker.

        Show
        Christopher Tubbs added a comment - Isn't encryption at rest an experimental feature? I'm not sure this warrants a blocker.
        Hide
        Sean Busbey added a comment -

        can someone describe how impacted 1.5 is?

        Show
        Sean Busbey added a comment - can someone describe how impacted 1.5 is?
        Hide
        John Vines added a comment -

        There was a first pass at encryption in 1.5

        Show
        John Vines added a comment - There was a first pass at encryption in 1.5
        Hide
        Sean Busbey added a comment -

        I didn't htink the encryption stuff was in the 1.5 branch?

        Show
        Sean Busbey added a comment - I didn't htink the encryption stuff was in the 1.5 branch?
        Hide
        Michael Allen added a comment -

        Sure. The patch was off of the 1.5.1 branch, BTW, so I guess files moved in the 1.6.0 timeframe.

        Show
        Michael Allen added a comment - Sure. The patch was off of the 1.5.1 branch, BTW, so I guess files moved in the 1.6.0 timeframe.
        Hide
        Sean Busbey added a comment -

        Michael, can you include a test please?

        Show
        Sean Busbey added a comment - Michael, can you include a test please?
        Hide
        John Vines added a comment -

        git apply --check /tmp/Dont-write-instance-secret-to-RFiles.patch
        error: patch failed: core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleFactory.java:258
        error: core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleFactory.java: patch does not apply

        Show
        John Vines added a comment - git apply --check /tmp/Dont-write-instance-secret-to-RFiles.patch error: patch failed: core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleFactory.java:258 error: core/src/main/java/org/apache/accumulo/core/security/crypto/CryptoModuleFactory.java: patch does not apply
        Hide
        Mike Drob added a comment -

        Instead of blocking 1.6.0, can we STRONGLY inform people that they should not be using encryption at rest, and then fix this in 1.6.1?

        Show
        Mike Drob added a comment - Instead of blocking 1.6.0, can we STRONGLY inform people that they should not be using encryption at rest, and then fix this in 1.6.1?
        Hide
        Michael Allen added a comment -

        Attached patch.

        Show
        Michael Allen added a comment - Attached patch.

          People

          • Assignee:
            John Vines
            Reporter:
            Michael Allen
          • Votes:
            1 Vote for this issue
            Watchers:
            10 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development