ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrey Gura <ag...@apache.org>
Subject Re: [DISCUSS] Best way to re-encrypt existing data (TDE cache key rotation).
Date Wed, 28 Oct 2020 12:09:46 GMT
Hi there,

I accidentally stumbled upon a potential performance problem in this commit.

CacheGroupMetricImpls.getPagesLeftForReencryption method contains at
least two problems:

 - Relatively major: In order to calculate a value for one metric the
method has O(N) complexity (N is number of partitions). It isn't good.
Better approach is using some precalculated or estimated value during
re-encryption process and just return this value.
 - Major: For each partition in this method PageStore.exists() will be
called. This invocation leads to N calls to the file system (may be
cached, may be not, we can't just hope). So with a default affinity
configuration this method will touch the file system 1024 times per
one metrics value calculation. Just increase dramatism and multiply
1024 on the number of cache groups existing on a node.

Finally, we have auxiliary functionality (metrics) which could affect
the whole node (and potentially cluster) behavior.

Please, fix this problem and be more careful in the future.

On Fri, Oct 23, 2020 at 12:46 PM Pavel Pereslegin <xxtern@gmail.com> wrote:
>
> Hello folks,
>
> thanks to everyone who joined the review, greatly appreciate your
> helpful comments.
>
> If there is no objection, we will merge this patch [1] shortly.
>
> [1] https://github.com/apache/ignite/pull/7941
>
> пн, 5 окт. 2020 г. в 15:30, Maksim Stepachev <maksim.stepachev@gmail.com>:
> >
> > Hi,
> >
> > I'm going to do it.
> >
> > сб, 3 окт. 2020 г. в 21:47, Alex Plehanov <plehanov.alex@gmail.com>:
> >
> > > Hello guys,
> > >
> > > I've finished the review and approved the patch.
> > > Anybody else would like to review it?
> > >
> > > пн, 28 сент. 2020 г. в 11:38, Pavel Pereslegin <xxtern@gmail.com>:
> > >
> > > > Hello, Maksim!
> > > >
> > > > I am currently working on a review notes from Alexey Plekhanov, will
> > > > let you know when I finish.
> > > >
> > > > пн, 28 сент. 2020 г. в 11:04, Maksim Stepachev <
> > > maksim.stepachev@gmail.com
> > > > >:
> > > > >
> > > > > Hi, Pavel.
> > > > >
> > > > > As I see, the ticket [
> > > https://issues.apache.org/jira/browse/IGNITE-12843
> > > > ]
> > > > > is "PATCH AVAILABLE". Is this ticket finished?
> > > > >
> > > > > чт, 13 авг. 2020 г. в 13:49, Pavel Pereslegin <xxtern@gmail.com>:
> > > > >
> > > > > > Hello all.
> > > > > >
> > > > > > I'm working on TDE cache group key rotation [1] and I have a
couple
> > > of
> > > > > > questions about partition re-encryption.
> > > > > >
> > > > > > As described in the wiki [2], the process of re-encryption at
the
> > > > > > moment consists of sequentially marking memory pages as dirty,
this
> > > > > > process looks not resource-intensive.
> > > > > > Do you think it is necessary to do this in a multithreaded mode
or
> > > > > > single thread is enough?
> > > > > > (We started testing re-encryption on dedicated servers (Xeon
E5-2680
> > > > > > 2.4Ghz, SSD Huawei ES3600P 3.2TB, ThrottlingPolicy =
> > > > > > CHECKPOINT_BUFFER_ONLY) with no speed limit and no load, as
a result,
> > > > > > single-threaded encryption loaded disk within 30%. At the same
time,
> > > > > > the total re-encryption speed was around 60 MB/s, which allows
one
> > > > > > node to re-encrypt 1 TB of data in about 5 hours, and it seems
that
> > > > > > this performance is enough.)
> > > > > >
> > > > > > The second question is about the approach to storing the
> > > re-encryption
> > > > > > status.
> > > > > > At the moment, the re-encryption status includes two parameters
- the
> > > > > > total number of pages in the partition at the time of the start
of
> > > > > > re-encryption (int) and the index of the last re-encrypted page
> > > (int).
> > > > > > These 8 bytes are stored in the metapage on the checkpoint (which
> > > > > > ensures that if the checkpoint does not happen, we will continue
the
> > > > > > process from the last page written to disk).
> > > > > > However, if multithread partition scanning does not make sense,
then
> > > > > > it seems that it is possible to change the implementation and
don't
> > > > > > change the metapage structure. Store only the "pointer" of the
> > > > > > partition (and the cache group) in the metastore and scan in
strict
> > > > > > order.
> > > > > > The approach with storing the status in the metapage of the
partition
> > > > > > seems to me more flexible, stable and has a number of advantages
over
> > > > > > the "pointer" approach:
> > > > > > 1. Since we saving the total number of pages at the re-encryption
> > > > > > startup - we will not scan extra pages that may be added to
the
> > > > > > partition later.
> > > > > > 2. We can move partitions between nodes and re-encryption should
> > > > > > continue from a certain point on the new node.
> > > > > > 3. If a partition is (re)created during cache group re-encryption,
it
> > > > > > will not be re-encrypted (since its re-encryption status will
be
> > > reset
> > > > > > and all data is encrypted with the latest encryption key after
> > > > > > (re)creation.
> > > > > >
> > > > > > Do you think single-threaded mode is enough?
> > > > > > Is it better to keep the re-encryption status in the metapage
or
> > > store
> > > > > > the "pointer" in the metastore?
> > > > > >
> > > > > > [1] https://issues.apache.org/jira/browse/IGNITE-12843
> > > > > > [2]
> > > > > >
> > > >
> > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=95652384#TDE.Phase3.Cachekeyrotation.-Backgroundre-encryption
> > > > > >
> > > > > > пт, 31 июл. 2020 г. в 11:09, Pavel Pereslegin <xxtern@gmail.com>:
> > > > > > >
> > > > > > > Hello,
> > > > > > >
> > > > > > > I'll expand the answer a bit about calculating CRC, the
problem is
> > > > not
> > > > > > > that it is calculated twice, but that now for encrypted
pages,
> > > > > > > EncryptedFileIO checks physical integrity, and FilePageStore
checks
> > > > > > > the correctness of the encryption key, but from my point
of view,
> > > it
> > > > > > > should be vice versa - the lower (delegated) FileIO should
check
> > > the
> > > > > > > physical integrity and EncryptedFileIO should check the
correctness
> > > > of
> > > > > > > the encryption key.
> > > > > > >
> > > > > > > пт, 31 июл. 2020 г. в 10:40, Pavel Pereslegin <xxtern@gmail.com>:
> > > > > > > >
> > > > > > > > Hello,
> > > > > > > >
> > > > > > > > > 10. Question - CRC is read in two places encryptionFileIO
and
> > > > > > > > > filePageStore - what should we do with this?
> > > > > > > >
> > > > > > > > We need to calculate the CRC of encrypted data, because
we may be
> > > > > > > > using the wrong encryption key to decrypt data, in
which case we
> > > > will
> > > > > > > > not understand if the physical integrity is violated
or the wrong
> > > > > > > > encryption key is used.
> > > > > > > >
> > > > > > > > > 9. Question - How do we optimize when we can
check that this
> > > > page is
> > > > > > > > > already encrypted by parallel loading? Maybe
we should do this
> > > in
> > > > > > Phase 4?
> > > > > > > >
> > > > > > > > To do this, we need to store the encryption key ID
in memory (at
> > > > > > > > least), but this is not easy to do right now without
breaking
> > > > binary
> > > > > > > > compatibility.
> > > > > > > >
> > > > > > > > > 7. Question -the current implementation does
not use the
> > > > throttling
> > > > > > that
> > > > > > > > > is implemented in PDS. Users should set the throughput
such as
> > > 5
> > > > MB
> > > > > > per
> > > > > > > > > second, but not the timeout, packet size, or
stream size.
> > > > > > > >
> > > > > > > > I've added a simple rate limiter for this.
> > > > > > > >
> > > > > > > > > 8. Question - why we add a lot of system properties?
> > > > > > > > >> Can you, please, list system properties that
should be moved
> > > to
> > > > the
> > > > > > configuration?
> > > > > > > >
> > > > > > > > It's about the background re-encryption properties,
for now, it
> > > is:
> > > > > > > > - re-encryption speed limit (in megabytes per second)
> > > > > > > > - threads count used for re-encryption
> > > > > > > > - count of pages in batch, processed under checkpoint
lock
> > > > > > > > - flag to completely disable background re-encryption
> > > > > > > >
> > > > > > > > > 11. We should remember about complicated test
scenarios with
> > > > failover
> > > > > > > >
> > > > > > > > PR contains tests for re-encryption (and key rotation)
on
> > > unstable
> > > > > > > > topology (with baseline change and without it). I'll
expand them
> > > > if I
> > > > > > > > missed some cases.
> > > > > > > >
> > > > > > > > > 13. Will re-encryption continue after the cluster
is completely
> > > > > > stopped?
> > > > > > > >
> > > > > > > > Yes, as I mentioned earlier, we save the re-encryption
status in
> > > > the
> > > > > > > > meta page of each re-encrypted partition and trigger
> > > re-encryption
> > > > on
> > > > > > > > node startup if needed (more detailed description
on the wiki).
> > > > > > > >
> > > > > > > > Thanks a lot for your comments, I am still working
on PR and
> > > > expanding
> > > > > > > > wiki documentation. I'll let you know when it will
be ready for
> > > the
> > > > > > > > review.
> > > > > > > >
> > > > > > > > вт, 28 июл. 2020 г. в 19:14, Alexey Goncharuk
<
> > > > > > alexey.goncharuk@gmail.com>:
> > > > > > > > >
> > > > > > > > > Hello Nikolay,
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > > 10. Question - CRC is read in two places
encryptionFileIO
> > > and
> > > > > > > > > > filePageStore - what should we do with this?
> > > > > > > > > >
> > > > > > > > > > filePageStore checks CRC of the encrypted
page. This required
> > > > to
> > > > > > confirm
> > > > > > > > > > the page not corrupted on the disk.
> > > > > > > > > > encryptionFileIO checks CRC of the decrypted
page(CRC itself
> > > > > > stored in the
> > > > > > > > > > encrypted data).
> > > > > > > > > > This required to be sure the decrypted page
contains correct
> > > > data
> > > > > > and not
> > > > > > > > > > replaced with some malicious content.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > I still do not see why we need CRC twice, can
you please
> > > > elaborate
> > > > > > on this
> > > > > > > > > statement? If an attacker is able to replace
the contents of an
> > > > > > encrypted
> > > > > > > > > page, it means that they have access to the encryption
key.
> > > What
> > > > will
> > > > > > > > > prevent them from calculating the CRC of malicious
content and
> > > > then
> > > > > > > > > encrypting it?
> > > > > >
> > > >
> > >

Mime
View raw message