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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Wed, 01 Jun 2022 19:07:22 +0000 Gerrit-HasComments: Yes
