[
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)
