Hello Igniters! Nikolay almost finished PR review. Does anyone else want to look at the changes? [1]
I implemented master key change management through Java API and JMX. I created the issue [2] to implement change through control.sh that I will do after the merge first one. [1] https://github.com/apache/ignite/pull/6937 [2] https://issues.apache.org/jira/browse/IGNITE-12475 пт, 18 окт. 2019 г. в 15:18, Nikolay Izhikov <nizhi...@apache.org>: > > Hello, Nikita. > > Thank you. > > I will take a look. shortly. > > чт, 17 окт. 2019 г. в 18:23, Maxim Muzafarov <mmu...@apache.org>: > > > Nikita, > > > > > Can we include it into a 2.8 release scope? > > I think it is possible since the release scope freeze date has not > > happened yet. > > > > On Thu, 17 Oct 2019 at 17:36, Nikita Amelchev <nsamelc...@gmail.com> > > wrote: > > > > > > Hi, Igniters! > > > > > > I have implemented the master key change process [1] for TDE as > > > described in the design [2]. > > > > > > I have prepared PR [3] and created the Upsource review branch [4]. > > > > > > Could anyone take a look at my changes? > > > > > > Can we include it into a 2.8 release scope? > > > > > > [1] https://issues.apache.org/jira/browse/IGNITE-12186 > > > [2] > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=95652381 > > > [3] https://github.com/apache/ignite/pull/6937 > > > [4] https://reviews.ignite.apache.org/ignite/review/IGNT-CR-1067 > > > > > > пн, 23 сент. 2019 г. в 17:13, Nikolay Izhikov <nizhi...@apache.org>: > > > > > > > > Hello, Nikita. > > > > > > > > > A node creates the ChangeMasterKeyMessage message and sent it by > > discovery as a custom event. > > > > > The goal is to verify that all nodes have the same master key. > > > > ... > > > > > The ChangeMasterKeyFinishMessage action message is sent by discovery > > as a custom event. > > > > > New master key id. > > > > > > > > 1. We should store locally new key id and new key hash after > > processing of ChangeMasterKeyMessage > > > > 2. We should send new key hash in ChangeMasterKeyFinishMessage > > > > 3. We should ensure that both ChangeMasterKeyMessage and > > ChangeMasterKeyFinishMessage contains the same key id and key hash before > > executing a change process. > > > > > > > > I think we should rename: > > > > > > > > > Node left during key rotation process(was not starting re-encrypt > > cache keys) > > > > > > > > Node was down during key rotation. ChangeMasterKeyRecord was not found. > > > > > > > > > Node left during key rotation process(was starting re-encrypt cache > > keys) > > > > > > > > Node was down during key rotation. ChangeMasterKeyRecord found. > > > > > > > > We should add a description of changes in the cluster join process. > > > > A node should not try to join to the cluster before the process of > > ChangeMasterKeyRecord. > > > > > > > > It's not clear for me how we differ two cases: > > > > > > > > 1. ChangeMasterKeyRecord found in WAL and key rotation was finished > > successfully. > > > > 2. ChangeMasterKeyRecord found in WAL and key rotation was NOT > > finished successfully. > > > > > > > > > Meta storage will store master key id. > > > > > > > > We should add that key id from metastorage has a higher priority to > > key id from IgniteConfiguration. > > > > > > > > > > > > В Пт, 20/09/2019 в 14:06 +0300, Nikita Amelchev пишет: > > > > > Nikolay, > > > > > > > > > > you are right in many ways. I updated the design on the wiki. [1] > > > > > > > > > > [1] > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=95652381 > > > > > > > > > > пт, 20 сент. 2019 г. в 13:49, Nikolay Izhikov <nizhi...@apache.org>: > > > > > > > > > > > > Nikita > > > > > > > > > > > > > I suggested the implementation where the encryption manager is > > > > > > > responsible for storing the master key id. > > > > > > > > > > > > I don't think it's a right proposal. > > > > > > > > > > > > 1. EncryptionSpi implementation becomes more complicated. > > Developer of it should be aware of Ignite deployment scenarious, etc. > > > > > > Imagine implementation when EncryptionSpi send master key id to > > some external storage over network(it's happen in Discovery thread) > > > > > > > > > > > > 2. Implementation would be duplicate features(saving master key id) > > > > > > > > > > > > 3. We already store cache keys in metastore. For me it's a native > > approach to store master key id in the same place. > > > > > > > > > > > > What do you think? > > > > > > > > > > > > > > > > > > В Пт, 20/09/2019 в 13:39 +0300, Nikita Amelchev пишет: > > > > > > > Nikolay, > > > > > > > > > > > > > > because I suggested the implementation where the encryption > > manager is > > > > > > > responsible for storing the master key id. > > > > > > > To implement this logic in the EncryptionSpi, we will need to > > > > > > > introduce the methods look like this: > > > > > > > > > > > > > > setMasterKeyId(String masterKeyId) // Sets "current" master key > > id > > > > > > > String getMasterKeyId() // Gets "current" master key id > > > > > > > > > > > > > > Follow methods will work with master key that setted by previous > > method: > > > > > > > > > > > > > > byte[] masterKeyDigest() > > > > > > > byte[] encryptKey(Serializable key) > > > > > > > Serializable decryptKey(byte[] key) > > > > > > > > > > > > > > If such implementation is more reasonable, I will do so. > > > > > > > > > > > > > > пт, 20 сент. 2019 г. в 13:04, Nikolay Izhikov < > > nizhi...@apache.org>: > > > > > > > > > > > > > > > > Why do we need "defaultMasterKeyId" instead of *current* > > master key id that can be obtained with > > `KeystoreEncryptionSpi#getMasterKeyName()`? > > > > > > > > > > > > > > > > В Пт, 20/09/2019 в 12:56 +0300, Nikita Amelchev пишет: > > > > > > > > > Nikolay, > > > > > > > > > > > > > > > > > > Thanks for the proposal, I like it. > > > > > > > > > > > > > > > > > > The GridEncryptionManager will control the process of master > > key > > > > > > > > > rotation, so we should provide him master key id at startup. > > Seems we > > > > > > > > > should get it from some configuration for encryption. > > > > > > > > > > > > > > > > > > I suggest just adding the String defaultMasterKeyId() method > > into the > > > > > > > > > EncryptionSpi interface. This method gets default master key > > id used > > > > > > > > > on first cluster start. > > > > > > > > > > > > > > > > > > The specific implementation will be responsible for setting > > this value. > > > > > > > > > > > > > > > > > > What do you think? > > > > > > > > > > > > > > > > > > пт, 20 сент. 2019 г. в 10:44, Nikolay Izhikov < > > nizhi...@apache.org>: > > > > > > > > > > > > > > > > > > > > Hello, Nikita > > > > > > > > > > > > > > > > > > > > > IgniteConfiguration: New methods will be added to the > > IgniteConfiguration: > > > > > > > > > > > public IgniteConfiguration > > setEncryptionMasterKeyId(String masterKeyId) - sets master key id. > > > > > > > > > > > public String getEncryptionMasterKeyId() > > > > > > > > > > > > > > > > > > > > We don't need it in the IgniteConfiguration. > > > > > > > > > > > > > > > > > > > > As you may know, we already have > > KeystoreEncryptionSpi#setMasterKeyName. > > > > > > > > > > Seems, we should add it to the EncryptionSpi itself. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > В Ср, 18/09/2019 в 22:25 +0300, Nikita Amelchev пишет: > > > > > > > > > > > Nikolay, thanks for participating. > > > > > > > > > > > > > > > > > > > > > > I have supplemented the design and clarify these > > moments. [1] > > > > > > > > > > > > > > > > > > > > > > [1] > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=95652381 > > > > > > > > > > > > > > > > > > > > > > ср, 18 сент. 2019 г. в 16:48, Nikolay Izhikov < > > nizhi...@apache.org>: > > > > > > > > > > > > > > > > > > > > > > > > Hello, Nikita. > > > > > > > > > > > > > > > > > > > > > > > > Thanks for starting this discussion. > > > > > > > > > > > > > > > > > > > > > > > > 1. We should add prerequisites for "master key > > rotation process" in design. > > > > > > > > > > > > Seems, it should be, "New master key available to > > EncryptionSPI for each server node". > > > > > > > > > > > > > > > > > > > > > > > > 2. Please, use code formatting in wiki. It's make > > reading easier. > > > > > > > > > > > > > > > > > > > > > > > > 3. Please, clarify java API proposal. What will be > > changed and how. > > > > > > > > > > > > AFAIK we need to change EncryptionSPI, this should be > > covered in design. > > > > > > > > > > > > > > > > > > > > > > > > 4. Please, clarify new CLI commands. > > > > > > > > > > > > AFAIK we should have 2 command: > > > > > > > > > > > > > > > > > > > > > > > > 1. Start regular master key rotation process. > > > > > > > > > > > > 2. Start local master key rotation process > > during node recovery(for the case when key changed while node was down). > > > > > > > > > > > > > > > > > > > > > > > > В Ср, 18/09/2019 в 16:09 +0300, Nikita Amelchev пишет: > > > > > > > > > > > > > Hi, Igniters. > > > > > > > > > > > > > > > > > > > > > > > > > > I'm going to implement the ability to rotate the > > master encryption key > > > > > > > > > > > > > (TDE Phase 2). [1] > > > > > > > > > > > > > Master key rotation required in case of it > > compromising or at the end > > > > > > > > > > > > > of crypto period(key validity period). I prepared > > the design. [2] > > > > > > > > > > > > > > > > > > > > > > > > > > In briefly, master keys will be identified by String > > masterKeyId. The > > > > > > > > > > > > > concept of the masterKeyId will be added to the > > cache keys encryption > > > > > > > > > > > > > process in EncryptionSpi. > > > > > > > > > > > > > > > > > > > > > > > > > > Users can configure master key id in > > IgniteConfiguration and will be > > > > > > > > > > > > > able to manage the key rotation process from java > > API, JMX, CLI: > > > > > > > > > > > > > - ignite.encryption().changeMasterKey(String > > masterKeyId) - starts > > > > > > > > > > > > > master key rotation process. > > > > > > > > > > > > > - String ignite.encryption().getMasterKeyId() - > > gets current master key id. > > > > > > > > > > > > > > > > > > > > > > > > > > Any thoughts? > > > > > > > > > > > > > > > > > > > > > > > > > > [1] > > https://issues.apache.org/jira/browse/IGNITE-12186 > > > > > > > > > > > > > [2] > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=95652381 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > Best wishes, > > > Amelchev Nikita > > -- Best wishes, Amelchev Nikita
