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 10: (3 comments) http://gerrit.cloudera.org:8080/#/c/18025/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18025/3//COMMIT_MSG@30 PS3, Line 30: made to code that handles files and relies on sizes and offsets, : including in tests. : : This commit also changes the PBC tool to check if a file is encrypted : based on the encryption header instead of the file name. : > I'm not sure I understand what's the problem if returning the actual size w OnDisk() means block count * block size, so that's not really the same as file size - header size. Logical size everywhere else is also not enough, because a lot of stuff rely on the actual file size for various reasons (I go into more detail on https://gerrit.cloudera.org/c/18025/8/src/kudu/util/file_cache-test.cc#107). 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: uint64_t size; > Cool! Could you please add a comment with explaining that right in this co Done 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)); > Thank you for the explanation. I'm not sure I saw that in one of patch set PS3 was like this, in that version I add the header size to the offset on reads and writes behind the scenes. Maybe I used a bad abstraction, but I still can't really find a better one. Let's take a WAL segment as an example. It has a fixed size footer, so the way the offset is calculated is to take the file size and subtract the footer size from it. If the file size is logical, this read will be off by 64 bytes. Another example is the LBM container file, which requires blocks to be aligned to FS blocks, which are 4k. So if I simply add 64 bytes to the specified offset, nothing will be actually aligned to the FS blocks. The first block would start at offset 64, the next block could start at something like 4160 or 8256. This would make hole punching impossible, as it can only punch whole FS blocks, so for the alignments to work, LBM needs to be aware of the encryption headers. -- 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: 10 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 19:22:12 +0000 Gerrit-HasComments: Yes
