ignite-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vladimir Ozerov (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (IGNITE-8485) TDE - Phase-1
Date Tue, 11 Sep 2018 08:14:00 GMT

    [ https://issues.apache.org/jira/browse/IGNITE-8485?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16610255#comment-16610255

Vladimir Ozerov commented on IGNITE-8485:

Hi [~NIzhikov], my comments:
1) Styling: please look through your code and fix the following issues:
- Unused imports
- Replace abbreviations in method names with full words (as abbreviations are allowed only
for variable, not method names and class names)
- Replace "log.warn" with "U.warn"

2) I am still not satisfied with API. "AES" name is not appropriate here, because it denotes
algorithm, but instead it should denote underlying storage. Correct name would be "Keystore".
First, it will help us to add more algorithms in future while still using keystore (e.g. 3DES,
which is used by other vendors). Second, what if in future we add another implementation over
some KMS which will also use AES? How would we name? This is why "AES" should go away from
interface names.

3) Key generation for clients - please move it to {{GridEncryptionManager}}, as this is exactly
what managers and processors are created for - to manage component lifecycle, listen for events,
encapsulate related logic in a single place.

4) Currently random node is picked to send request to. Instead, random *server* node should
be used.

5) I would suggest to remove group IDs from request. First, at this point our understanding
that cache groups is a hack feature, which is likely to be removed in future in favor of tablespaces.
So it is better to avoid relying on it if possible. Second, there is no need to get existing
keys for caches at all. Because by the time you got the key from existing cache, it may get's
re-created concurrently with another key. Or key rotation may happen (in future release).
So you can never rely on what key is returned, and it should be compared with existing group
key in exchange thread during cache start. IMO all we need is the request is the number of
keys to be generated. 

6) {{GridCacheProcessor#genEncKeysAndStartCacheAfter}} - future is registered after message
is sent, which means that response may be missed (e.g. in case of long GC pause or unfavorable
context switch). Also there is no proper sync with disconnect event, meaning that you may
have hanging futures after disconnect as well. Bulletproof synchronization here looks like
this :
boolean stopped;
boolean disconnected;

onStart(...) {
    // Set all listeners

onKernalStop(...) {
    sync (mux) {
        // Set stop flag
        // Complete all futures with error

onDiscoveryMessage(...) {
    sync (mux) {
        // Iterate over registered futures, resend if possible or finish with error 

onIOMessage(...) {
    sync (mux) {
        // Generate key or complete and deregister future.

onDisconnect(...) {
    sync (mux) {
        // Set disconnect flag
        // Complete all futures with error

onReconnect(...) {
    sync (mux) {
        // Remove disconnect flag.

generateKeys(...) {
    sync (mux) {
        // Check stop and disconnect flags
        // Register future, send request

At this point it should be obvious why all this logic should be located in separate processor.

7) There is no need to throw exception in IO listener threads. All we can do here is to log
error with proper message.

> TDE - Phase-1
> -------------
>                 Key: IGNITE-8485
>                 URL: https://issues.apache.org/jira/browse/IGNITE-8485
>             Project: Ignite
>          Issue Type: Sub-task
>            Reporter: Nikolay Izhikov
>            Assignee: Nikolay Izhikov
>            Priority: Critical
>             Fix For: 2.7
> Basic support for a Transparent Data Encryption should be implemented:
> 1. Usage of standard JKS, Java Security.
> 2. Persistent Data Encryption/Decryption.

This message was sent by Atlassian JIRA

View raw message