Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys ...................................................................... Patch Set 8: (15 comments) > Patch Set 3: > > (7 comments) > > I took a quick look and since I see the size of the encryption is still > floating around, may be it makes sense to re-evaluate the approach I already > pointed at https://gerrit.cloudera.org/#/c/18025/3/src/kudu/util/env.h@194 ? > > What do you think? What do you mean the by the size floating around and how would your suggestion address that? 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- > Maybe I'm missing it -- in env_posix I only see the headers using 40: 7 for Yea we right-pad it here: https://gerrit.cloudera.org/c/18025/4/src/kudu/util/env_posix.cc#800 Seems I forgot to add this to the EncryptionHeader comment though. http://gerrit.cloudera.org:8080/#/c/18025/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18025/8//COMMIT_MSG@36 PS8, Line 36: manually with encryption enabled > Probably, the encryption stuff is not one of those things which might be af I don't think it would be necessary, but I guess i wouldn't hurt. Is there a way to pass env vars to dist-test? http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/consensus/log_index.cc File src/kudu/consensus/log_index.cc: http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/consensus/log_index.cc@124 PS8, Line 124: Read(file_->GetEncryptionHeaderSize() > For the RWFile::Read() and RWFile::Write() below: does it make sense to hid Answered this on https://gerrit.cloudera.org/c/18025/8/src/kudu/util/file_cache-test.cc#107 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: > Whitespace after initializer lists aren't treated the same with regards to Done 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 e This is only the pbc tool though, this code is never part of any servers. If the file was written by an encrypted server, we won't be able to decrypt it with encryption disabled anyway, so this check should still be okay. This condition is there basically to avoid reading the file twice if we don't have to. http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env.h File src/kudu/util/env.h: http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env.h@378 PS8, Line 378: const size_t > What's the idea behind add 'const' to the size_t output type which is retur Done http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env.h@398 PS8, Line 398: const > Drop const? Done http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env.h@698 PS8, Line 698: IsEncrypted() > Is it expected to have a non-const implementation of this method in any of Done 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? Done 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 _instea Sorry, I thought the 192-bit key part makes it clear enough. 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. Done 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'r Yea, I agree this is weird, do you have a suggestion for a cleaner approach? 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 Done http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_util.cc File src/kudu/util/env_util.cc: http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env_util.cc@250 PS8, Line 250: opts.is_sensitive = false; > This looks a bit strange: WritableFileOptions are unconditionally overwritt When we copy a file, it doesn't make sense to decrypt and re-encrypt it with a different key. Treating the file as insensitive allows us to copy encrypted files as-is. http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc File src/kudu/util/file_cache-test.cc: http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/file_cache-test.cc@103 PS8, Line 103: unique_ptr<RWFile> f; : RWFileOptions opts; : opts.is_sensitive = true; : RETURN_NOT_OK(env_->NewRWFile(opts, name, &f)); : RETURN_NOT_OK(f->Write(f->GetEncryptionHeaderSize(), data)); > Instead of passing around the information on the enc header size and offset That's effectively what the original approach was (hide the header size everywhere), but that leads to a lot of different confusion. As Andrew pointed out, it's hard to draw the line where we use physical vs logical size: https://gerrit.cloudera.org/c/18025/3//COMMIT_MSG#35 It also required a ton of hacks. We also rely on specific offsets a lot, we don't just start writing to files at 0, but we sometimes need to read the footer of a file, so we need to be able to calculate the offset where the footer begins based on the file size. LBM relies on FS blocks for hole punching, so all blocks in a container file need to be aligned. Hiding the file size would mess with these alignments. -- 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 17:33:55 +0000 Gerrit-HasComments: Yes
