Cassandra
  1. Cassandra
  2. CASSANDRA-3051

On Disk Compression breaks SSL Encryption

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Fix Version/s: 1.0.0
    • Component/s: None
    • Labels:
      None
    • Environment:

      Trunk

      Description

      Encryption depends on FileStreamTask.write [1] protected member to be called because the SSLFileStreamTask.write overrides this to write back to the server.

      When enabled, compression circumvents the call and the client does not communicate using an SSL socket back to the server.

      [1]
      protected long write(FileChannel fc, Pair<Long, Long> section, long length, long bytesTransferred) throws IOException

      1. CASSANDRA-3051-v2.patch
        16 kB
        Pavel Yaskevich
      2. CASSANDRA-3051.patch
        14 kB
        Pavel Yaskevich

        Activity

        Gavin made changes -
        Workflow patch-available, re-open possible [ 12752953 ] reopen-resolved, no closed status, patch-avail, testing [ 12755632 ]
        Gavin made changes -
        Workflow no-reopen-closed, patch-avail [ 12627493 ] patch-available, re-open possible [ 12752953 ]
        Hide
        Hudson added a comment -

        Integrated in Cassandra #1056 (See https://builds.apache.org/job/Cassandra/1056/)
        Fix streaming over SSL when compressed SSTable involved
        patch by Pavel Yaskevich; reviewed by Jonathan Ellis for CASSANDRA-3051

        xedin : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1163207
        Files :

        • /cassandra/trunk/CHANGES.txt
        • /cassandra/trunk/src/java/org/apache/cassandra/io/compress/CompressedRandomAccessReader.java
        • /cassandra/trunk/src/java/org/apache/cassandra/net/MessagingService.java
        • /cassandra/trunk/src/java/org/apache/cassandra/security/streaming/SSLFileStreamTask.java
        • /cassandra/trunk/src/java/org/apache/cassandra/streaming/FileStreamTask.java
        Show
        Hudson added a comment - Integrated in Cassandra #1056 (See https://builds.apache.org/job/Cassandra/1056/ ) Fix streaming over SSL when compressed SSTable involved patch by Pavel Yaskevich; reviewed by Jonathan Ellis for CASSANDRA-3051 xedin : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1163207 Files : /cassandra/trunk/CHANGES.txt /cassandra/trunk/src/java/org/apache/cassandra/io/compress/CompressedRandomAccessReader.java /cassandra/trunk/src/java/org/apache/cassandra/net/MessagingService.java /cassandra/trunk/src/java/org/apache/cassandra/security/streaming/SSLFileStreamTask.java /cassandra/trunk/src/java/org/apache/cassandra/streaming/FileStreamTask.java
        Pavel Yaskevich made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Reviewer gdusbabek jbellis
        Resolution Fixed [ 1 ]
        Hide
        Pavel Yaskevich added a comment -

        Committed.

        Show
        Pavel Yaskevich added a comment - Committed.
        Hide
        Jonathan Ellis added a comment -

        +1

        Show
        Jonathan Ellis added a comment - +1
        Pavel Yaskevich made changes -
        Attachment CASSANDRA-3051-v2.patch [ 12492216 ]
        Hide
        Pavel Yaskevich added a comment -

        removed writeHeader method, seek and flush are done once per section.

        Show
        Pavel Yaskevich added a comment - removed writeHeader method, seek and flush are done once per section.
        Hide
        Jonathan Ellis added a comment -
        • feels like we lose more than we gain by making writeHeader/write separate methods. they aren't really self-contained so you have to keep the context they were called in, around mentally. and if they were in-line, it would be obvious that you don't need to re-seek for each call to write().
        • comments in write() don't really add much to what the code says, imo
        • is flushing with each chunk necessary? seems like that would harm performance
        Show
        Jonathan Ellis added a comment - feels like we lose more than we gain by making writeHeader/write separate methods. they aren't really self-contained so you have to keep the context they were called in, around mentally. and if they were in-line, it would be obvious that you don't need to re-seek for each call to write(). comments in write() don't really add much to what the code says, imo is flushing with each chunk necessary? seems like that would harm performance
        Pavel Yaskevich made changes -
        Attachment CASSANDRA-3051.patch [ 12492154 ]
        Hide
        Pavel Yaskevich added a comment -

        CompressedRandomAccessReader.transfer method was removed with special casing for compressed files, SocketChannel based transfer changed to DataOuputStream based to unify SSL and normal modes. SSLFileStreamTask removed as unused.

        Show
        Pavel Yaskevich added a comment - CompressedRandomAccessReader.transfer method was removed with special casing for compressed files, SocketChannel based transfer changed to DataOuputStream based to unify SSL and normal modes. SSLFileStreamTask removed as unused.
        Pavel Yaskevich made changes -
        Attachment CASSANDRA-3051.patch [ 12492082 ]
        Hide
        Pavel Yaskevich added a comment -

        I figured out the problem - SSLSocket always returns null on getChannel even on current code, I will refactor FileStreamingTask to support DataOutputStream instead of SocketChannel to unify normal and SSL transfers.

        Show
        Pavel Yaskevich added a comment - I figured out the problem - SSLSocket always returns null on getChannel even on current code, I will refactor FileStreamingTask to support DataOutputStream instead of SocketChannel to unify normal and SSL transfers.
        Hide
        Pavel Yaskevich added a comment -

        Sure

        Show
        Pavel Yaskevich added a comment - Sure
        Hide
        Jonathan Ellis added a comment -

        Pavel, can you try to set up local SSL w/ a ccm cluster based on Gary's instructions to verify?

        Show
        Jonathan Ellis added a comment - Pavel, can you try to set up local SSL w/ a ccm cluster based on Gary's instructions to verify?
        Hide
        Pavel Yaskevich added a comment -

        You misunderstood that, it is not breaking SSL it was just special cased in FileStreamTask so it wasn't using ssl socket. This patch removes special casing for SSL streaming by creating ssl socket directly in FileStreamTask if encryption options were set.

        Show
        Pavel Yaskevich added a comment - You misunderstood that, it is not breaking SSL it was just special cased in FileStreamTask so it wasn't using ssl socket. This patch removes special casing for SSL streaming by creating ssl socket directly in FileStreamTask if encryption options were set.
        Hide
        Gary Dusbabek added a comment - - edited

        Pavel, I tested SSL prior to committing the feature.

        I was under the impression that this ticket exists because compression, when enabled, breaks SSL. The implication is that it was working prior, else the ticket would be something more like "SSL is broken."

        Show
        Gary Dusbabek added a comment - - edited Pavel, I tested SSL prior to committing the feature. I was under the impression that this ticket exists because compression, when enabled, breaks SSL. The implication is that it was working prior, else the ticket would be something more like "SSL is broken."
        Hide
        Pavel Yaskevich added a comment -

        Following your logic - person who was working on ssl should've done that at first place, there is no guarantee that it works in the current state. I'm not pushing things forward just wondering why testing wasn't done before.

        Show
        Pavel Yaskevich added a comment - Following your logic - person who was working on ssl should've done that at first place, there is no guarantee that it works in the current state. I'm not pushing things forward just wondering why testing wasn't done before.
        Hide
        Gary Dusbabek added a comment -

        Not that I know of. If someone wants to write one it would flesh out these basic steps:

        1. follow the steps for generating a keystore and a trust store here: http://download.oracle.com/javase/6/docs/technotes/guides/security/jsse/JSSERefGuide.html#CreateKeystore
        2. plug those files into encryption_options in cassandra.yaml
        3. make sure encryption_options.internode_encryption = all in the yaml.

        I was mostly raising a voice of caution against committing code backed up by statements like "I don't see anh reason why it won't..." This is usually a prelude to something like "we need to quickly release a new version because XYZ broke streaming." Just sayin'.

        Show
        Gary Dusbabek added a comment - Not that I know of. If someone wants to write one it would flesh out these basic steps: follow the steps for generating a keystore and a trust store here: http://download.oracle.com/javase/6/docs/technotes/guides/security/jsse/JSSERefGuide.html#CreateKeystore plug those files into encryption_options in cassandra.yaml make sure encryption_options.internode_encryption = all in the yaml. I was mostly raising a voice of caution against committing code backed up by statements like "I don't see anh reason why it won't..." This is usually a prelude to something like "we need to quickly release a new version because XYZ broke streaming." Just sayin'.
        Hide
        Pavel Yaskevich added a comment -

        No, unfortunately I haven't found any info about how to do that so you are welcome to test if you can...

        Show
        Pavel Yaskevich added a comment - No, unfortunately I haven't found any info about how to do that so you are welcome to test if you can...
        Hide
        Jonathan Ellis added a comment -

        do we have a "ssl howto" somewhere? I was hoping it would be in cassandra.yaml by encryption_options but no. Or at least, not sufficiently "for dummies" for me.

        Show
        Jonathan Ellis added a comment - do we have a "ssl howto" somewhere? I was hoping it would be in cassandra.yaml by encryption_options but no. Or at least, not sufficiently "for dummies" for me.
        Hide
        Gary Dusbabek added a comment -

        wait, you tested it locally first, right? It's not difficult to set up a streaming environment locally.

        Show
        Gary Dusbabek added a comment - wait, you tested it locally first, right? It's not difficult to set up a streaming environment locally.
        Hide
        Pavel Yaskevich added a comment -

        I don't see anh reason why it won't but can't write a test because SSL is treacky with it's stores...

        Show
        Pavel Yaskevich added a comment - I don't see anh reason why it won't but can't write a test because SSL is treacky with it's stores...
        Hide
        Jonathan Ellis added a comment -

        lgtm, +1 if it actually works

        Show
        Jonathan Ellis added a comment - lgtm, +1 if it actually works
        Pavel Yaskevich made changes -
        Status In Progress [ 3 ] Patch Available [ 10002 ]
        Reviewer gdusbabek
        Fix Version/s 1.0 [ 12316349 ]
        Pavel Yaskevich made changes -
        Attachment CASSANDRA-3051.patch [ 12492082 ]
        Hide
        Pavel Yaskevich added a comment -

        Removed SSLFileStreamTask and added EncryptionOptions to the constructor of the FileStreamTask.

        Rebased with latest trunk (last commit 0a4b1667bee674f7c0a22057cbdab97e368a20d1)

        Show
        Pavel Yaskevich added a comment - Removed SSLFileStreamTask and added EncryptionOptions to the constructor of the FileStreamTask. Rebased with latest trunk (last commit 0a4b1667bee674f7c0a22057cbdab97e368a20d1)
        Pavel Yaskevich made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Pavel Yaskevich made changes -
        Link This issue is blocked by CASSANDRA-3015 [ CASSANDRA-3015 ]
        Pavel Yaskevich made changes -
        Field Original Value New Value
        Link This issue is blocked by CASSANDRA-3015 [ CASSANDRA-3015 ]
        Benjamin Coverston created issue -

          People

          • Assignee:
            Pavel Yaskevich
            Reporter:
            Benjamin Coverston
            Reviewer:
            Jonathan Ellis
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development