ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nikita Amelchev <nsamelc...@gmail.com>
Subject Re: TDE Master key rotation (Phase-2)
Date Tue, 24 Dec 2019 18:54:13 GMT
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 <nizhikov@apache.org>:
>
> Hello, Nikita.
>
> Thank you.
>
> I will take a look. shortly.
>
> чт, 17 окт. 2019 г. в 18:23, Maxim Muzafarov <mmuzaf@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 <nsamelchev@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 <nizhikov@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 <nizhikov@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 <
> > nizhikov@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 <
> > nizhikov@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 <
> > nizhikov@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

Mime
View raw message