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 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? 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: Probably, the encryption stuff is not one of those things which might be affected, but anyways: I found that sometimes it makes sense to check how that works at nodes with inferior hardware to help pin-pointing implicit assumptions in test scenarios. In that context, does it makes sense to run the suite using dist-test with encryption enabled? 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(sizeof(PhysicalEntry) * entry_in For the RWFile::Read() and RWFile::Write() below: does it make sense to hide the offset behind the API, i.e. introducing a class derived from RWFile which takes care of the offset? 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: al string in What's the idea behind add 'const' to the size_t output type which is returned by value anyways? I'm not sure this makes much sense. http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env.h@398 PS8, Line 398: Drop const? http://gerrit.cloudera.org:8080/#/c/18025/8/src/kudu/util/env.h@698 PS8, Line 698: Is it expected to have a non-const implementation of this method in any of the derived classes? If not, consider making this method constant. 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 overwritten. Instead, does it seem feasible to specify proper options at corresponding call sites? 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(0, data)); Instead of passing around the information on the enc header size and offset, did you consider an alternative to introduce a new sub-type of RWFile (say, EncryptedRWFile), so that NewWritableFile() would return a proper instance via the shared pointer? Since Read() and Write() are virtual methods in RWFile class, those might be overridden as necessary in EncryptedRWFile, so it would not be necessary to pass around the size of the encrypted header on the upper level. Same approach might be used for RandomAccessFile and others. What do you think? -- 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: 3 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 03:21:58 +0000 Gerrit-HasComments: Yes
