Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys ...................................................................... Patch Set 8: (8 comments) http://gerrit.cloudera.org:8080/#/c/18025/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18025/4//COMMIT_MSG@29 PS4, Line 29: 64- > I actually ended up using 64 bytes after all. It's not needed for alignment Maybe I'm missing it -- in env_posix I only see the headers using 40: 7 for magic, 1 for the algorithm, and 32 for the file key. Is there another 24 used elsewhere? http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/18025/4/src/kudu/fs/log_block_manager.cc@749 PS4, Line 749: > what's wrong with the spacing here? Whitespace after initializer lists aren't treated the same with regards to subsequent lines: https://google.github.io/styleguide/cppguide.html#Constructor_Initializer_Lists http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/tools/tool_action_pbc.cc File src/kudu/tools/tool_action_pbc.cc: http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/tools/tool_action_pbc.cc@107 PS8, Line 107: if (!env->IsEncryptionEnabled()) { : return false; : } This seems like it doesn't do the right thing -- just because 'env' isn't encrypted doesn't mean the file was written on an unencrypted env, right? Would it make sense to set FLAGS_encrypt_data_at_rest=true here in a flag saver and then check if we can open the file? I suppose it only matters in cases where the env with which we read is different from the env in which we write. But that's possible (though problematic) e.g. in the case a user incorrectly starts without encryption args, when the data is already encrypted, and vice versa. http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@491 PS8, Line 491: //memcpy(ciphertext[i].mutable_data(), cleartext[i].data(), cleartext[i].size()); nit: remove? http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@777 PS8, Line 777: // As the keys are encrypted in ECB mode which requires padding, we need : // 32 bytes to encrypt and write a 192-bit key. nit: I'm pretty sure this comment is explaining why we are using 32 _instead of_ 24, as the key size below is specified. Can you explicitly call that out? It's not obvious without reading the ReadEncryptionHeader code http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@800 PS8, Line 800: 56 nit: define this as a constexpr constant with a kVariableName. Also why 56? Mind adding a comment? http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@827 PS8, Line 827: *eh) nit: seems kind of confusing to be passing this as an argument, giving we're not decrypting anything in this readv call. http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_posix.cc@833 PS8, Line 833: case 0: nit: consider assigning values to these enums and using those instead? May be less error prone. i.e. enum class EncryptionAlgorithm { AES128CTR = 0, ... }; -- To view, visit http://gerrit.cloudera.org:8080/18025 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idb1282c117271fda63a8cc54c00add7cc96dcffd Gerrit-Change-Number: 18025 Gerrit-PatchSet: 8 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[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, 12 Jan 2022 06:30:23 +0000 Gerrit-HasComments: Yes
