Cassandra
  1. Cassandra
  2. CASSANDRA-1567

Provide configurable encryption support for internode communication

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Fix Version/s: 0.8 beta 1
    • Component/s: Core
    • Labels:
      None

      Description

      Provide the option to encrypt internode communication. The initial thought is to use JSSE (http://download.oracle.com/javase/6/docs/technotes/guides/security/jsse/JSSERefGuide.html) to wrap the existing ServerSocket & Sockets. This will only be an optional configuration and not enabled by default. The defaults would be TLS V1, RSA 1024-bit keys for handshake and SSL_RSA_WITH_RC4_128_MD5 as the cipher suite. Although this can be made configurable if the need arises.

      1. 1567-v4.patch
        31 kB
        Nirmal Ranganathan
      2. 1567-v3.patch
        33 kB
        Nirmal Ranganathan
      3. 0004-setReuseAddress-before-bind-and-docs.patch
        4 kB
        Gary Dusbabek
      4. 0002-Configurable-internode-encryption-option-V2.patch
        30 kB
        Nirmal Ranganathan
      5. 0003-Default-Key-and-Certificate-for-internode-SSL-V2.patch
        4 kB
        Nirmal Ranganathan
      6. 0002-Configurable-internode-encryption-option.patch
        24 kB
        Nirmal Ranganathan
      7. 0003-Default-Key-and-Certificate-for-internode-SSL.patch
        4 kB
        Nirmal Ranganathan

        Activity

        Nirmal Ranganathan created issue -
        Hide
        Nirmal Ranganathan added a comment -

        working code. Need to update the configuration for keystores

        Show
        Nirmal Ranganathan added a comment - working code. Need to update the configuration for keystores
        Nirmal Ranganathan made changes -
        Field Original Value New Value
        Attachment 0001-Adding-SSL-versions-for-streaming-classes.patch [ 12456402 ]
        Attachment 0002-Configurable-internode-encryption-option.patch [ 12456403 ]
        Attachment 0003-Default-Key-and-Certificate-for-internode-SSL.patch [ 12456404 ]
        Hide
        Stu Hood added a comment -
        • For 0001, I would really like to see an A(bstract)StreamableSocket rather than complete duplication of the Stream classes
        • Rather than a boolean, the internode_encryption setting should probably be an enum, to leave room to add conditional encryption based on zones returned by the snitch
        • The SSL settings in JVM_OPTS should be disabled by default, and need a comment linking to a place to get more information about the keystore and truststore files (probably the 'Creating Keystores' section of the link in the description)

        Sorry for the long delayed review: Thanks a ton for tackling this!

        Show
        Stu Hood added a comment - For 0001, I would really like to see an A(bstract)StreamableSocket rather than complete duplication of the Stream classes Rather than a boolean, the internode_encryption setting should probably be an enum, to leave room to add conditional encryption based on zones returned by the snitch The SSL settings in JVM_OPTS should be disabled by default, and need a comment linking to a place to get more information about the keystore and truststore files (probably the 'Creating Keystores' section of the link in the description) Sorry for the long delayed review: Thanks a ton for tackling this!
        Stu Hood made changes -
        Reviewer stuhood
        Hide
        Stu Hood added a comment -

        Also, can we add a startup message that indicates the encryption mode being used?

        Show
        Stu Hood added a comment - Also, can we add a startup message that indicates the encryption mode being used?
        Nirmal Ranganathan made changes -
        Attachment 0001-Adding-SSL-versions-for-streaming-classes.patch [ 12456402 ]
        Nirmal Ranganathan made changes -
        Attachment 0002-Configurable-internode-encryption-option.patch [ 12456403 ]
        Nirmal Ranganathan made changes -
        Hide
        Nirmal Ranganathan added a comment -

        For 0001, I would really like to see an A(bstract)StreamableSocket rather than complete duplication of the Stream classes

        Done

        Rather than a boolean, the internode_encryption setting should probably be an enum, to leave room to add conditional encryption based on zones returned by the snitch

        Updated to use an enum, just (all, none) for now.

        The SSL settings in JVM_OPTS should be disabled by default, and need a comment linking to a place to get more information about the keystore and truststore files (probably the 'Creating Keystores' section of the link in the description)

        Having those properties in should not be a problem. We can provide a wiki page on how to get everything setup.

        Show
        Nirmal Ranganathan added a comment - For 0001, I would really like to see an A(bstract)StreamableSocket rather than complete duplication of the Stream classes Done Rather than a boolean, the internode_encryption setting should probably be an enum, to leave room to add conditional encryption based on zones returned by the snitch Updated to use an enum, just (all, none) for now. The SSL settings in JVM_OPTS should be disabled by default, and need a comment linking to a place to get more information about the keystore and truststore files (probably the 'Creating Keystores' section of the link in the description) Having those properties in should not be a problem. We can provide a wiki page on how to get everything setup.
        Stu Hood made changes -
        Assignee Nirmal Ranganathan [ rnirmal ] Stu Hood [ stuhood ]
        Hide
        Nirmal Ranganathan added a comment -

        There's one more change I'm going to add, and will hopefully have it out soon, rebased and all. I'm going to make the default cipher suite we use to AES_128/256 with SHA.

        Show
        Nirmal Ranganathan added a comment - There's one more change I'm going to add, and will hopefully have it out soon, rebased and all. I'm going to make the default cipher suite we use to AES_128/256 with SHA.
        Hide
        Stu Hood added a comment -

        Nirmal mentioned that he was going to do a bit more refactoring of this one before calling it reviewable again.

        Show
        Stu Hood added a comment - Nirmal mentioned that he was going to do a bit more refactoring of this one before calling it reviewable again.
        Stu Hood made changes -
        Assignee Stu Hood [ stuhood ] Nirmal Ranganathan [ rnirmal ]
        Jonathan Ellis made changes -
        Fix Version/s 0.8 [ 12314820 ]
        Fix Version/s 0.7.1 [ 12315199 ]
        Jonathan Ellis made changes -
        Fix Version/s 0.7.1 [ 12315199 ]
        Fix Version/s 0.8 [ 12314820 ]
        Jonathan Ellis made changes -
        Comment [ bumping to 0.8 for dependency on THRIFT-106 which is in (will be in?) Thrift 0.6 ]
        Hide
        Nirmal Ranganathan added a comment -

        So for this I think we'll go with just all internode encryption with AES_128/256 in an either/or situation. Either all your cluster node transfers is encrypted or not. Based on if there's demand to have just cross DC encrypted we can update it at that point and if users want to configure encryption options.

        Show
        Nirmal Ranganathan added a comment - So for this I think we'll go with just all internode encryption with AES_128/256 in an either/or situation. Either all your cluster node transfers is encrypted or not. Based on if there's demand to have just cross DC encrypted we can update it at that point and if users want to configure encryption options.
        Hide
        Jonathan Ellis added a comment -

        Where did patch 0001 go? Was it committed separately?

        Show
        Jonathan Ellis added a comment - Where did patch 0001 go? Was it committed separately?
        Hide
        Nirmal Ranganathan added a comment -

        During the last update I combined 0001 & 0002. So everything is in 0002 and 0003 is a sample keystore and env config. yet to upload the latest changes, but what's there works

        Show
        Nirmal Ranganathan added a comment - During the last update I combined 0001 & 0002. So everything is in 0002 and 0003 is a sample keystore and env config. yet to upload the latest changes, but what's there works
        Hide
        Jonathan Ellis added a comment -

        make the default cipher suite we use to AES_128/256 with SHA

        This looks like all that needs to be done to close out this ticket. That and probably a fairly hairy rebase.

        Show
        Jonathan Ellis added a comment - make the default cipher suite we use to AES_128/256 with SHA This looks like all that needs to be done to close out this ticket. That and probably a fairly hairy rebase.
        Jonathan Ellis made changes -
        Assignee Nirmal Ranganathan [ rnirmal ] Pavel Yaskevich [ xedin ]
        Hide
        Nirmal Ranganathan added a comment -

        Attaching rebased versions (-V2) with the latest updates. I've tested it a bit and seems to work fine. Would be nice to test it out a little more.

        Show
        Nirmal Ranganathan added a comment - Attaching rebased versions (-V2) with the latest updates. I've tested it a bit and seems to work fine. Would be nice to test it out a little more.
        Nirmal Ranganathan made changes -
        Attachment 0002-Configurable-internode-encryption-option-V2.patch [ 12468048 ]
        Attachment 0003-Default-Key-and-Certificate-for-internode-SSL-V2.patch [ 12468049 ]
        Hide
        Pavel Yaskevich added a comment -

        Nirmal Ranganathan: Can the latest 002 and 003 patches be considered as complete solution?

        Show
        Pavel Yaskevich added a comment - Nirmal Ranganathan: Can the latest 002 and 003 patches be considered as complete solution?
        Hide
        Nirmal Ranganathan added a comment -

        Yes it can be considered as patch ready. Pavel it will be great if you can review it too.

        Show
        Nirmal Ranganathan added a comment - Yes it can be considered as patch ready. Pavel it will be great if you can review it too.
        Hide
        Pavel Yaskevich added a comment -

        Great! I will review it too. Can you please change status to Patch Available?

        Show
        Pavel Yaskevich added a comment - Great! I will review it too. Can you please change status to Patch Available?
        Nirmal Ranganathan made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Pavel Yaskevich added a comment -

        Minor notes:

        1. Clean code style of the method definitions (e.g. DatabaseDescriptor methods sizeMemtableThroughput, sizeMemtableOperations)
        2. remove space in line "bytesRead += buf. limit();" of SSLIncomingStreamReader class

        Everything else looks good.

        Show
        Pavel Yaskevich added a comment - Minor notes: 1. Clean code style of the method definitions (e.g. DatabaseDescriptor methods sizeMemtableThroughput, sizeMemtableOperations) 2. remove space in line "bytesRead += buf. limit();" of SSLIncomingStreamReader class Everything else looks good.
        Jonathan Ellis made changes -
        Assignee Pavel Yaskevich [ xedin ] Nirmal Ranganathan [ rnirmal ]
        Hide
        Gary Dusbabek added a comment - - edited

        Am I missing something big, or does this only encrypt stream communications?

        EDIT: nm. Stu pointed out the parts that I missed.

        Show
        Gary Dusbabek added a comment - - edited Am I missing something big, or does this only encrypt stream communications? EDIT: nm. Stu pointed out the parts that I missed.
        Hide
        Stu Hood added a comment -

        Nirmal, thanks again for your work here: this will really be a killer feature.

        • Could we add a class level javadoc to SSLFileStreamTask to indicate that it exists because FileStreamTask uses sendFile?

        +1 aside from that.

        Show
        Stu Hood added a comment - Nirmal, thanks again for your work here: this will really be a killer feature. Could we add a class level javadoc to SSLFileStreamTask to indicate that it exists because FileStreamTask uses sendFile? +1 aside from that.
        Nirmal Ranganathan made changes -
        Attachment 0002-Configurable-internode-encryption-option-V2.patch [ 12468048 ]
        Nirmal Ranganathan made changes -
        Hide
        Nirmal Ranganathan added a comment -

        Pavel: I've updated based on your notes, btw sizeMemtableThroughput and sizeMemtableOperations shouldn't be there in the first place, I think it got left behind in one of my rebases, removed it now.

        StuHood: Thanks for looking through it, I've added comments to explain why SSLSocket/SSLServerSocket cannot encrypt data transferred using FileChannel.transferTo/transferFrom.

        Gary: As StuHood mentioned that's the reason the additional SSL versions. But all internode data is getting encrypted.

        Show
        Nirmal Ranganathan added a comment - Pavel: I've updated based on your notes, btw sizeMemtableThroughput and sizeMemtableOperations shouldn't be there in the first place, I think it got left behind in one of my rebases, removed it now. StuHood: Thanks for looking through it, I've added comments to explain why SSLSocket/SSLServerSocket cannot encrypt data transferred using FileChannel.transferTo/transferFrom. Gary: As StuHood mentioned that's the reason the additional SSL versions. But all internode data is getting encrypted.
        Hide
        Gary Dusbabek added a comment -
        • setReuseAddress happens before bind.
        • added link to keytool docs in cassandra.yaml.
        Show
        Gary Dusbabek added a comment - setReuseAddress happens before bind. added link to keytool docs in cassandra.yaml.
        Gary Dusbabek made changes -
        Hide
        Nirmal Ranganathan added a comment -

        Updated the patch to cleanly apply on 0.7 branch and trunk

        Show
        Nirmal Ranganathan added a comment - Updated the patch to cleanly apply on 0.7 branch and trunk
        Nirmal Ranganathan made changes -
        Attachment 1567-v3.patch [ 12468147 ]
        Nirmal Ranganathan made changes -
        Attachment 1567-v4.patch [ 12468759 ]
        Hide
        Gary Dusbabek added a comment -

        Moving to 0.8 based on the "shorter release schedule" thread on -dev.

        Show
        Gary Dusbabek added a comment - Moving to 0.8 based on the "shorter release schedule" thread on -dev.
        Gary Dusbabek made changes -
        Fix Version/s 0.8 [ 12314820 ]
        Fix Version/s 0.7.1 [ 12315199 ]
        Hide
        Gary Dusbabek added a comment -

        committed. excellent work!

        Show
        Gary Dusbabek added a comment - committed. excellent work!
        Gary Dusbabek made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Hudson added a comment -

        Integrated in Cassandra #678 (See https://hudson.apache.org/hudson/job/Cassandra/678/)
        configurable internode encryption. patch by rnirmal, reviewed by gdusbabek. CASSANDRA-1567

        Show
        Hudson added a comment - Integrated in Cassandra #678 (See https://hudson.apache.org/hudson/job/Cassandra/678/ ) configurable internode encryption. patch by rnirmal, reviewed by gdusbabek. CASSANDRA-1567
        Hide
        Jonathan Ellis added a comment -

        where did we document how to use this?

        Show
        Jonathan Ellis added a comment - where did we document how to use this?
        Hide
        Nirmal Ranganathan added a comment -

        Haven't documented yet, just the info in the conf file for now, since it was moved to 0.8 release. I haven't looked at the wiki recently, if we have sections or docs for 0.8 release, I'll add this with a note.

        Show
        Nirmal Ranganathan added a comment - Haven't documented yet, just the info in the conf file for now, since it was moved to 0.8 release. I haven't looked at the wiki recently, if we have sections or docs for 0.8 release, I'll add this with a note.
        Gavin made changes -
        Workflow no-reopen-closed, patch-avail [ 12522038 ] patch-available, re-open possible [ 12752457 ]
        Gavin made changes -
        Workflow patch-available, re-open possible [ 12752457 ] reopen-resolved, no closed status, patch-avail, testing [ 12755367 ]

          People

          • Assignee:
            Nirmal Ranganathan
            Reporter:
            Nirmal Ranganathan
            Reviewer:
            Stu Hood
          • Votes:
            1 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development