Mailing-List: contact commits-help@cassandra.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cassandra.apache.org
Date: Fri, 31 Jul 2015 18:21:05 +0000 (UTC)
From: "Robert Stupp (JIRA)" <jira@apache.org>
To: commits@cassandra.apache.org
Message-ID: <JIRA.12850396.1438290118000.342340.1438366865064@Atlassian.JIRA>
In-Reply-To: <JIRA.12850396.1438290118000@Atlassian.JIRA>
References: <JIRA.12850396.1438290118000@Atlassian.JIRA>
 <JIRA.12850396.1438290118062@arcas>
Subject: [jira] [Commented] (CASSANDRA-9945) Add transparent data encryption
 core classes
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable


    [ https://issues.apache.org/jira/browse/CASSANDRA-9945?page=3Dcom.atlas=
sian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=3D=
14649594#comment-14649594 ]=20

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

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

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

Instead of forcing users to modify the JRE/JDK, we could provide [bouncycas=
tle 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 {{Encryption=
Context}}?
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 =E2=80=9Dnodet=
ool refresh_tde_keystore=E2=80=9C?

A unit test covering especially the {{getEncryptor}}+{{getDecryptor}} metho=
ds 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 {{Secu=
reRandom}} is not thread-safe. Maybe put a {{SecureRandom}} (and Cipher ins=
tances) in a {{ThreadLocal}}?

Nits:
* DatabaseDescriptor: typo in the comment of {{encryptionContext}} - seems =
you were missing a =E2=80=9Dgit add=E2=80=9D in the terminal? ;)
* EncryptionOptions: the check {{if (tdeOptions.enabled)}} in the catch-cla=
use is redundant and can be omitted
* Unused method {{EncryptionContext.toHeaderParameters}} (at least in this =
commit)
* CipherFactory.getDecryptor: the assertion (or the message) is wrong. Shou=
ld 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 transpa=
rent data encryption (file-level encryption), as required for CASSANDRA-601=
8 and CASSANDRA-9633.  The phrase "transparent data encryption", while not =
the most aesthetically pleasing, seems to be used throughout the database i=
ndustry (Oracle, SQLQServer, Datastax Enterprise) to describe file level en=
cryption, so we'll go with that, as well.=20


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