ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Maxim Muzafarov <mmu...@apache.org>
Subject Re: TDE Master key rotation (Phase-2)
Date Thu, 17 Oct 2019 15:22:58 GMT
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

Mime
View raw message