cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Robert Stupp (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CASSANDRA-9945) Add transparent data encryption core classes
Date Fri, 31 Jul 2015 18:21:05 GMT

    [ https://issues.apache.org/jira/browse/CASSANDRA-9945?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14649594#comment-14649594
] 

Robert Stupp commented on CASSANDRA-9945:
-----------------------------------------

The code looks good so far. Some comments on the patch.

{{CipherFactory#buildCipher}} calls {{Cipher.getInstance}} for every encryption. {{Cipher.getInstance}}
is a somewhat expensive operation. Using a Cipher-per-thread (ThreadLocal) would be nicer
IMO. I ran a [quick’n’dirty microbench|https://gist.github.com/snazy/7839a7fdcf25dabafd4b].
It’s not about ms (at least not for AES).

Instead of forcing users to modify the JRE/JDK, we could provide [bouncycastle JCE|https://www.bouncycastle.org/documentation.html]
(MIT license) - or even allow people to use it. This would just require a slight extension
to {{TransparentDataEncryptionOptions}} to add a {{String provider}} and pass it to {{Cipher.getInstance(transformation,
provider)}}.

Can you elaborate a bit how the {{Key}} cache is to be used in {{EncryptionContext}}?
I guess you need it when the encryption key for commit logs and/or sstables is changed.
Will there be any way to change the encryption keys e.g. via ”nodetool refresh_tde_keystore“?

A unit test covering especially the {{getEncryptor}}+{{getDecryptor}} methods in {{EncryptionContext}}
would be great. Maybe also comparing data going all the way (clear-text -> encryption ->
decryption -> clear-text).

CipherFactory.secureRandom seems to be used multi-threaded. I assume {{SecureRandom}} is not
thread-safe. Maybe put a {{SecureRandom}} (and Cipher instances) in a {{ThreadLocal}}?

Nits:
* DatabaseDescriptor: typo in the comment of {{encryptionContext}} - seems you were missing
a ”git add” in the terminal? ;)
* EncryptionOptions: the check {{if (tdeOptions.enabled)}} in the catch-clause is redundant
and can be omitted
* Unused method {{EncryptionContext.toHeaderParameters}} (at least in this commit)
* CipherFactory.getDecryptor: the assertion (or the message) is wrong. Should it be iv.length
> 0 ?

> Add transparent data encryption core classes
> --------------------------------------------
>
>                 Key: CASSANDRA-9945
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-9945
>             Project: Cassandra
>          Issue Type: Improvement
>            Reporter: Jason Brown
>            Assignee: Jason Brown
>              Labels: encryption
>             Fix For: 3.0 beta 1
>
>
> This patch will add the core infrastructure classes necessary for transparent data encryption
(file-level encryption), as required for CASSANDRA-6018 and CASSANDRA-9633.  The phrase "transparent
data encryption", while not the most aesthetically pleasing, seems to be used throughout the
database industry (Oracle, SQLQServer, Datastax Enterprise) to describe file level encryption,
so we'll go with that, as well. 



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message