Hello, Dmitriy. Thank you for feedback!
> 1) I suggest changing encrypt() and decrypt() method would take the same type > of parameters. > This uniformity will avoid implementors questions about why encrypt takes > byte[] but decrypt takes ByteBuffer. This is done by a reason. encrypt and decrypt methods are on hot code path, so it important to avoid unnecessary type conversion. We use ByteBuffer in places where we already operate with ByteBuffer and byte[] where we have it. Anyway, ByteBuffer can be easily converted to byte[] and vice versa. > 2) I suggest the renaming of method create() to createKey() to make it> easy > to understand what method creates.> We already discussed new SPI and approved it with reviewers. Anyway it's pretty easy to rename method, but I don't think I should do it at the moment. All other reviewers are OK with simple `create`. В Вт, 02/10/2018 в 17:34 +0300, Dmitriy Pavlov пишет: > Hi Igniters, > > I'm sorry I a little bit late to the party, but I found a couple of hours > to take a look to TDE-1 implementation > https://issues.apache.org/jira/browse/IGNITE-8485 . Nikolay kindly agreed > to let me take a look. > > Most of the change was done well and I totally agree with it. Current > implementation seems to be extendable. > > I've got several minor comments, I hope it will be fixed. > https://github.com/apache/ignite/pull/4167 > Related to tests: let's make sure we won't fail new tests and compilation > pass for all modules. I will be happy to help with failures analysis if it > is required. > > Proposals related to API > 1) I suggest changing encrypt() and decrypt() method would take the same > type of parameters. This uniformity will avoid implementors questions about > why encrypt takes byte[] but decrypt takes ByteBuffer. > 2) I suggest the renaming of method create() to createKey() to make it > easy to understand what method creates. > > > Sincerely, > Dmitriy Pavlov > > ср, 12 сент. 2018 г. в 20:34, Denis Magda <dma...@apache.org>: > > > Hello Nikolay, > > > > Excellent progress, look forward to seeing the TDE released in 2.7! > > > > -- > > Denis > > > > On Wed, Sep 12, 2018 at 2:47 AM Nikolay Izhikov <nizhi...@apache.org> > > wrote: > > > > > Hello, Denis. > > > > > > > Could you please list the limitations of Phase 1? > > > > I'm curious what won't be supported in 2.7. > > > > > > 1. We will have ability to encrypt data stored on the disk for specific > > > cache. > > > > > > - There is no limitation on API usage or something for an > > > encrypted cache. > > > - If some cache in a cache group is encrypted, all other caches > > > > in > > > this group must be encrypted. > > > - Setting up master key(standard jdk key storage) is prerequisite > > > and should be done by an administrator. > > > - `encryptionEnabled` flag setting up on cache creation and can't > > > be changed in runtime. > > > > > > 2. We won't be able to change encryption keys for existing cache(key > > > rotation procedure). > > > This will be implemented in Phase 2. > > > > > > В Вт, 11/09/2018 в 23:41 -0400, Denis Magda пишет: > > > > Nikolay, > > > > > > > > Could you please list the limitations of Phase 1? I'm curious what > > > > won't > > > be > > > > supported in 2.7. > > > > > > > > -- > > > > Denis > > > > > > > > On Tue, Sep 11, 2018 at 4:29 PM Nikolay Izhikov <nizhi...@apache.org> > > > > > > wrote: > > > > > > > > > > As I understand the plan is to get TDE Phase 1 released in 2.7, > > > > > > right? > > > > > > > > > > Yes. We will release TDE in 2.7 > > > > > > > > > > > Could you also list what needs to be done at Phase 2 and how much > > > > > > time > > > > > > > > > > it might take. > > > > > > > > > > Yes, I think Dmitry Ryabov will send Phase 2 design > > > > > > > > > > > > > > > В Вт, 11/09/2018 в 23:27 +0300, Nikolay Izhikov пишет: > > > > > > Hello, Denis. > > > > > > > > > > > > Yes, Vladimir made 2 rounds of review. > > > > > > I planning to fix all issues with implementation in a few days. > > > > > > > > > > > > > > > > > > В Вт, 11/09/2018 в 10:40 -0400, Denis Magda пишет: > > > > > > > Hi Nikolay, > > > > > > > > > > > > > > Has anybody started looking into your request? As I understand > > > > the > > > > > > > > > > plan is > > > > > > > to get TDE Phase 1 released in 2.7, right? > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89067473 > > > > > > > > > > > > > > Could you also list what needs to be done at Phase 2 and how much > > > > > > time > > > > > > > > > > it > > > > > > > might take. > > > > > > > > > > > > > > -- > > > > > > > Denis > > > > > > > > > > > > > > On Thu, Aug 9, 2018 at 8:48 AM Nikolay Izhikov < > > > > > > nizhi...@apache.org> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > Hello, Igniters. > > > > > > > > > > > > > > > > I want to share with you TDE implementation details. > > > > > > > > I think it can simplify review and acception of TDE > > > > > > implementation. > > > > > > > > This mail is copy of wiki page [1]. > > > > > > > > > > > > > > > > Please, share your thoughts. > > > > > > > > > > > > > > > > TDE is ready for review [2]. > > > > > > > > Please, let me know, who is able to make final review. > > > > > > > > > > > > > > > > This document describes some internal details of TDE.Phase 1 > > > > > > > > implementation [3]. > > > > > > > > I suggest that reader familiar with the general design > > > > described > > > in > > > > > > > > > > IEP-18 > > > > > > > > [4]. > > > > > > > > > > > > > > > > > > > > > > > > Cache group key management and node join enhancements: > > > > > > > > > > > > > > > > 1. GridEncryptionManager encapsulates all logic related > > > > > > to > > > > > > > > > > key > > > > > > > > management: > > > > > > > > a. All group encryption keys are stored in > > > > > > MetaStore. > > > > > > > > > > > > > > > > b. Joining node sends to cluster: > > > > > > > > * Master key digest. > > > > > > > > * All group keys saved locally > > > > > > (Map<Integer, > > > > > > > > byte[]>). Keys send over a network in encrypted form. > > > > > > > > > > > > > > > > c. Coordinator on new node join: > > > > > > > > * Compares new node master key digest > > > > > > with > > > > > > > > > > the > > > > > > > > local master key digest. > > > > > > > > If differs then new node join is > > > > > > rejected. > > > > > > > > > > > > > > > > * Compares local and received group > > > > keys. > > > > > > > > If some key differs then new node join > > > > is > > > > > > > > rejected. > > > > > > > > > > > > > > > > d. All server nodes: > > > > > > > > * If some of received keys are new then > > > > > > they > > > > > > > > > > save > > > > > > > > locally. > > > > > > > > > > > > > > > > 2. Dynamic cache creation: > > > > > > > > a. On server node - Encryption key is generated > > > > > > and > > > > > > > > > > added > > > > > > > > to DynamicCacheChangeRequest. > > > > > > > > > > > > > > > > b. On client node: > > > > > > > > * Prior to creation of > > > > > > > > > > DynamicCacheChangeRequest > > > > > > > > we have to get new encryption key from some server node. > > > > > > > > * New request added to solve this - > > > > > > > > GenerateEncryptionKeyRequest. > > > > > > > > > > > > > > > > > > > > > > > > WAL Record encryption: > > > > > > > > > > > > > > > > > > > > > > > > 1. New type of WAL record created – EncryptedRecord. > > > > > > > > > > > > > > > > 2. EncryptedRecord is a container that stores some > > > > > > > > WalRecordCacheGroupAware in encrypted form inside. > > > > > > > > > > > > > > > > 3. Write: > > > > > > > > Each record for an encrypted group that implements > > > > > > > > WalRecordCacheGroupAware written to WAL in encrypted form. > > > > > > > > Instead of that record we write EncryptedRecord with > > > > > > plain > > > > > > > > > > record > > > > > > > > inside(PageSnapshot, PageDeltaRecord, etc). > > > > > > > > > > > > > > > > 4. Read: There are 3 different cases on EncryptedRecord > > > > > > read: > > > > > > > > a. WAL restore – we read EncryptedRecord, > > > > decrypt > > > > > > > > > > internal > > > > > > > > record and return it. > > > > > > > > > > > > > > > > b. Offline WAL > > > > > > > > > > iteration(StandaloneWalRecordsIterator) - > > > > > > > > EncryptionSpi is null so wecan’t decrypt EncryptedRecord and > > > > just > > > > > > > > > > return it > > > > > > > > from an iterator. > > > > > > > > > > > > > > > > c. Meta storage restore process – On node start > > > > > > we > > > > > > > > > > restore > > > > > > > > MetaStore. > > > > > > > > When we do it no encryption keys are available, > > > > > > > > > > because > > > > > > > > they are stored in MetaStore. > > > > > > > > So we can’t decrypt EncryptedRecord and just > > > > > > return > > > > > > > > > > it > > > > > > > > from an iterator. > > > > > > > > We don't need decrypted record on this step to > > > > > > > > > > operate > > > > > > > > properly. > > > > > > > > > > > > > > > > > > > > > > > > Page encryption: > > > > > > > > > > > > > > > > > > > > > > > > 1. We have to write on disk pages aligned on 2^n (2kb, > > > > > > 4kb, > > > > > > > > > > etc) > > > > > > > > for gain maximum perfrormance. > > > > > > > > > > > > > > > > 2. There is a 16 byte overhead for and AES CBC mode. > > > > > > > > > > > > > > > > 3. So we have to preserve 16 bytes in page in memory to > > > > > > get > > > > > > > > encrypted page size equal to 2^n when written it to disk. > > > > > > > > > > > > > > > > 4. PageIO has many methods with pageSize parameter. > > > > > > > > So for encrypted cache groups page size is calculated > > > > as > > > > > > > > cfg.getPageSize() - 16 byte. > > > > > > > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=89067473 > > > > > > > > [2] https://github.com/apache/ignite/pull/4167 > > > > > > > > [3] https://issues.apache.org/jira/browse/IGNITE-8485 > > > > > > > > [4] > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/IGNITE/IEP-18%3A+Transparent+Data+Encryption > >
signature.asc
Description: This is a digitally signed message part
