Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18025 )
Change subject: [security] KUDU-3316 Add encrypted file keys ...................................................................... Patch Set 8: (3 comments) 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() > I don't think I got a proper answer there. You mentioned that you started OK, it seems at https://gerrit.cloudera.org/c/18025/3/COMMIT_MSG#35 there is a discussion on how to draw a line where to use logical vs physical offsets. 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; > When we copy a file, it doesn't make sense to decrypt and re-encrypt it wit Cool! Could you please add a comment with explaining that right in this code? It could help with better understanding the code. 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)); > That's effectively what the original approach was (hide the header size eve Thank you for the explanation. I'm not sure I saw that in one of patch sets published for this review item. Did I miss it somehow? I didn't see the PS for the approach you referred, but I guess that the ton of hack were mostly due to not very good abstraction or a bug somewhere. So my ask is to try re-evaluate your original approach, if possible. If you are sure it's not going to work that way, I guess I'm OK with the current approach of passing the offsets in many places at the higher level. -- 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 18:23:47 +0000 Gerrit-HasComments: Yes
