You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Attila Bukor (Code Review)" <ge...@cloudera.org> on 2022/05/26 15:39:17 UTC

[kudu-CR] KUDU-3373 Key provider interface

Hello Zoltan Chovan,

I'd like you to do a code review. Please visit

    http://gerrit.cloudera.org:8080/18568

to review the following change.


Change subject: KUDU-3373 Key provider interface
......................................................................

KUDU-3373 Key provider interface

Kudu's server keys need to be encrypted on the servers, otherwise its
broken, as an attacker who can access Kudu's disks, can easily steal the
server keys used to encrypt the file keys. The cluster key, which will
be used to encrypt/decrypt the server keys, will live outside the
cluster. This commit introduces a key provider interface to
encrypt/decrypt server keys, with a reference (test-only) implementation
which uses memfrob() (a GNU C function that XORs an array with 42). A
follow-up commit will introduce a production-ready implementation that
uses Apache Ranger KMS to provide the keys.

Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
---
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/server/CMakeLists.txt
A src/kudu/server/default_key_provider-test.cc
A src/kudu/server/default_key_provider.h
A src/kudu/server/key_provider.h
8 files changed, 156 insertions(+), 6 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18568/1
-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 1
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3373 Key provider interface

Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18568 )

Change subject: KUDU-3373 Key provider interface
......................................................................


Patch Set 4: Code-Review+1


-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 May 2022 12:38:29 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3373 Key provider interface

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18568 )

Change subject: KUDU-3373 Key provider interface
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/18568/3/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/18568/3/src/kudu/fs/fs_manager.cc@467
PS3, Line 467:     // 'server_key' is a hexadecimal string and SetEncryptionKey expects bits
> As discussed separately this is a bit hard to understand to be coming from 
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 May 2022 18:16:36 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3373 Key provider interface

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18568 )

Change subject: KUDU-3373 Key provider interface
......................................................................


Patch Set 3: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 May 2022 21:47:48 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3373 Key provider interface

Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18568 )

Change subject: KUDU-3373 Key provider interface
......................................................................


Patch Set 5: Code-Review+1


-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Jun 2022 08:41:38 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3373 Key provider interface

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18568 )

Change subject: KUDU-3373 Key provider interface
......................................................................


Patch Set 5: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Jun 2022 08:16:20 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-3373 Key provider interface

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18568 )

Change subject: KUDU-3373 Key provider interface
......................................................................

KUDU-3373 Key provider interface

Kudu's server keys need to be encrypted on the servers, otherwise its
broken, as an attacker who can access Kudu's disks, can easily steal the
server keys used to encrypt the file keys. The cluster key, which will
be used to encrypt/decrypt the server keys, will live outside the
cluster. This commit introduces a key provider interface to
encrypt/decrypt server keys, with a reference (test-only) implementation
which uses memfrob() (a GNU C function that XORs an array with 42). A
follow-up commit will introduce a production-ready implementation that
uses Apache Ranger KMS to provide the keys.

Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Reviewed-on: http://gerrit.cloudera.org:8080/18568
Reviewed-by: Alexey Serbin <al...@apache.org>
Tested-by: Attila Bukor <ab...@apache.org>
Reviewed-by: Zoltan Chovan <zc...@cloudera.com>
---
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/server/CMakeLists.txt
A src/kudu/server/default_key_provider-test.cc
A src/kudu/server/default_key_provider.h
A src/kudu/server/key_provider.h
M src/kudu/tools/tool_action_common.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/test_util.cc
12 files changed, 193 insertions(+), 14 deletions(-)

Approvals:
  Alexey Serbin: Looks good to me, approved
  Attila Bukor: Verified
  Zoltan Chovan: Looks good to me, but someone else must approve

-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 6
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3373 Key provider interface

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18568 )

Change subject: KUDU-3373 Key provider interface
......................................................................


Patch Set 5: Code-Review+2

(3 comments)

http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/fs/fs_manager.cc@195
PS4, Line 195: initted_(false),
> I used the authz_provider model as inspiration. Instead of defaulting to th
That makes sense to me -- thanks for the explanation!


http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/server/key_provider.h
File src/kudu/server/key_provider.h:

http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/server/key_provider.h@32
PS4, Line 32:   // Decrypts the server key.
            :   virtual Status DecryptServerKey(const std::string& encrypted_server_key,
            :                                   std::string* server_key) = 0;
            : 
            :   // Encrypts the server key.
            :   virtual Status EncryptServerKey(const std::string& server_key,
            :                                   std::string* encrypted_server_key) = 0;
> Thought it would be worth indicating that this is the "server key" as oppos
That's totally fine with me as is: I was just trying to understand how significant was the 'Server' part.


http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/util/env.h
File src/kudu/util/env.h:

http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/util/env.h@394
PS4, Line 394: const uint8_t* key, size_t key_size
> Done
Thanks!



-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Jun 2022 20:13:17 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3373 Key provider interface

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18568 )

Change subject: KUDU-3373 Key provider interface
......................................................................


Patch Set 4:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/fs/fs_manager.cc@195
PS4, Line 195: key_provider_(new DefaultKeyProvider())
BTW, is there a case when 'key_provider_' would be a nullptr wrapper?  In other words, why to have 'key_provider_' with semantics of a pointer instead of simply having DefaultKeyProvider instance?


http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/fs/fs_manager.cc@691
PS4, Line 691: clear_server_key
nit: maybe, plain_server_key would be a better name here (not sure I understand what 'clear' stands for here -- maybe I'm missing something)


http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/fs/fs_manager.cc@693
PS4, Line 693: key_provider_->EncryptServerKey(clear_server_key, metadata->mutable_server_key());
Should this be wrapped into RETURN_NOT_OK() as well?


http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/server/default_key_provider.h
File src/kudu/server/default_key_provider.h:

http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/server/default_key_provider.h@38
PS4, Line 38: memfrob(encrypted_server_key->data(), server_key.length());
Does this compile on macOS?


http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/server/key_provider.h
File src/kudu/server/key_provider.h:

http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/server/key_provider.h@32
PS4, Line 32:   // Decrypts the server key.
            :   virtual Status DecryptServerKey(const std::string& encrypted_server_key,
            :                                   std::string* server_key) = 0;
            : 
            :   // Encrypts the server key.
            :   virtual Status EncryptServerKey(const std::string& server_key,
            :                                   std::string* encrypted_server_key) = 0;
naming nit: maybe, drop 'Server' from the names?  It would be just 'DecryptKey()' and 'EncryptKey()' then.  Or there is something substantial behind having 'Server' as the part of method names?


http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/util/env.h
File src/kudu/util/env.h:

http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/util/env.h@394
PS4, Line 394: size_t key_size, const uint8_t* key
nit: it's not exactly part of this changelist, but maybe it's not too late to change the order of the parameters to swap them?  Usually, the data blob comes first and then comes the size in signatures like this.



-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Tue, 31 May 2022 22:19:54 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3373 Key provider interface

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/18568

to look at the new patch set (#3).

Change subject: KUDU-3373 Key provider interface
......................................................................

KUDU-3373 Key provider interface

Kudu's server keys need to be encrypted on the servers, otherwise its
broken, as an attacker who can access Kudu's disks, can easily steal the
server keys used to encrypt the file keys. The cluster key, which will
be used to encrypt/decrypt the server keys, will live outside the
cluster. This commit introduces a key provider interface to
encrypt/decrypt server keys, with a reference (test-only) implementation
which uses memfrob() (a GNU C function that XORs an array with 42). A
follow-up commit will introduce a production-ready implementation that
uses Apache Ranger KMS to provide the keys.

Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
---
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/server/CMakeLists.txt
A src/kudu/server/default_key_provider-test.cc
A src/kudu/server/default_key_provider.h
A src/kudu/server/key_provider.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
10 files changed, 171 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18568/3
-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3373 Key provider interface

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Zoltan Chovan, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/18568

to look at the new patch set (#2).

Change subject: KUDU-3373 Key provider interface
......................................................................

KUDU-3373 Key provider interface

Kudu's server keys need to be encrypted on the servers, otherwise its
broken, as an attacker who can access Kudu's disks, can easily steal the
server keys used to encrypt the file keys. The cluster key, which will
be used to encrypt/decrypt the server keys, will live outside the
cluster. This commit introduces a key provider interface to
encrypt/decrypt server keys, with a reference (test-only) implementation
which uses memfrob() (a GNU C function that XORs an array with 42). A
follow-up commit will introduce a production-ready implementation that
uses Apache Ranger KMS to provide the keys.

Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
---
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/server/CMakeLists.txt
A src/kudu/server/default_key_provider-test.cc
A src/kudu/server/default_key_provider.h
A src/kudu/server/key_provider.h
8 files changed, 156 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18568/2
-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 2
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3373 Key provider interface

Posted by "Zoltan Chovan (Code Review)" <ge...@cloudera.org>.
Zoltan Chovan has posted comments on this change. ( http://gerrit.cloudera.org:8080/18568 )

Change subject: KUDU-3373 Key provider interface
......................................................................


Patch Set 3: Code-Review+1

(1 comment)

other than some readability stuff mentioned in the comment, it looks good to me

http://gerrit.cloudera.org:8080/#/c/18568/3/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/18568/3/src/kudu/fs/fs_manager.cc@467
PS3, Line 467:     env_->SetEncryptionKey(server_key.length() * 4,
As discussed separately this is a bit hard to understand to be coming from length / 2 * 8 due to the hex encoding.
Maybe an explanation comment would make it easier to read.
Also it might be helpful to add an explanation comment to SetEncryptionKey() that the key_size is the number of bits.



-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 May 2022 18:46:20 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3373 Key provider interface

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/18568

to look at the new patch set (#4).

Change subject: KUDU-3373 Key provider interface
......................................................................

KUDU-3373 Key provider interface

Kudu's server keys need to be encrypted on the servers, otherwise its
broken, as an attacker who can access Kudu's disks, can easily steal the
server keys used to encrypt the file keys. The cluster key, which will
be used to encrypt/decrypt the server keys, will live outside the
cluster. This commit introduces a key provider interface to
encrypt/decrypt server keys, with a reference (test-only) implementation
which uses memfrob() (a GNU C function that XORs an array with 42). A
follow-up commit will introduce a production-ready implementation that
uses Apache Ranger KMS to provide the keys.

Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
---
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/server/CMakeLists.txt
A src/kudu/server/default_key_provider-test.cc
A src/kudu/server/default_key_provider.h
A src/kudu/server/key_provider.h
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
10 files changed, 174 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18568/4
-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3373 Key provider interface

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has removed a vote on this change.

Change subject: KUDU-3373 Key provider interface
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 3
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3373 Key provider interface

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18568 )

Change subject: KUDU-3373 Key provider interface
......................................................................


Patch Set 4:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/fs/fs_manager.cc@195
PS4, Line 195: key_provider_(new DefaultKeyProvider())
> BTW, is there a case when 'key_provider_' would be a nullptr wrapper?  In o
I used the authz_provider model as inspiration. Instead of defaulting to the DefaultKeyProvider implementation, which would only provide a false sense of security, I'm planning to default to nullptr, and fail to open the FS if a key provider is not configured, but a server_key is. This way, we could prevent running a server with an unencrypted server key by mistake.


http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/fs/fs_manager.cc@691
PS4, Line 691: clear_server_key
> nit: maybe, plain_server_key would be a better name here (not sure I unders
Done


http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/fs/fs_manager.cc@693
PS4, Line 693: key_provider_->EncryptServerKey(clear_server_key, metadata->mutable_server_key());
> Should this be wrapped into RETURN_NOT_OK() as well?
Done


http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/server/default_key_provider.h
File src/kudu/server/default_key_provider.h:

http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/server/default_key_provider.h@38
PS4, Line 38: memfrob(encrypted_server_key->data(), server_key.length());
> Does this compile on macOS?
It doesn't, thanks for bringing this up :)


http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/server/key_provider.h
File src/kudu/server/key_provider.h:

http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/server/key_provider.h@32
PS4, Line 32:   // Decrypts the server key.
            :   virtual Status DecryptServerKey(const std::string& encrypted_server_key,
            :                                   std::string* server_key) = 0;
            : 
            :   // Encrypts the server key.
            :   virtual Status EncryptServerKey(const std::string& server_key,
            :                                   std::string* encrypted_server_key) = 0;
> naming nit: maybe, drop 'Server' from the names?  It would be just 'Decrypt
Thought it would be worth indicating that this is the "server key" as opposed to the cluster key or the file key. If you think it's an unnecessary distinction, I can drop it.


http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/util/env.h
File src/kudu/util/env.h:

http://gerrit.cloudera.org:8080/#/c/18568/4/src/kudu/util/env.h@394
PS4, Line 394: size_t key_size, const uint8_t* key
> nit: it's not exactly part of this changelist, but maybe it's not too late 
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 4
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 Jun 2022 19:07:22 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-3373 Key provider interface

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Zoltan Chovan, Alexey Serbin, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/18568

to look at the new patch set (#5).

Change subject: KUDU-3373 Key provider interface
......................................................................

KUDU-3373 Key provider interface

Kudu's server keys need to be encrypted on the servers, otherwise its
broken, as an attacker who can access Kudu's disks, can easily steal the
server keys used to encrypt the file keys. The cluster key, which will
be used to encrypt/decrypt the server keys, will live outside the
cluster. This commit introduces a key provider interface to
encrypt/decrypt server keys, with a reference (test-only) implementation
which uses memfrob() (a GNU C function that XORs an array with 42). A
follow-up commit will introduce a production-ready implementation that
uses Apache Ranger KMS to provide the keys.

Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
---
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/server/CMakeLists.txt
A src/kudu/server/default_key_provider-test.cc
A src/kudu/server/default_key_provider.h
A src/kudu/server/key_provider.h
M src/kudu/tools/tool_action_common.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/test_util.cc
12 files changed, 193 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/68/18568/5
-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>

[kudu-CR] KUDU-3373 Key provider interface

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has removed a vote on this change.

Change subject: KUDU-3373 Key provider interface
......................................................................


Removed Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/18568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ie6ccc05fb991f0fd5cbcd8a49f5b23286d1094ac
Gerrit-Change-Number: 18568
Gerrit-PatchSet: 5
Gerrit-Owner: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>