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 13: Verified+1 (10 comments) > Patch Set 13: Verified-1 > > Build Failed > > http://jenkins.kudu.apache.org/job/kudu-gerrit/25006/ : FAILURE 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: of it. > Well, I guess the regular way should work there, like Thanks, I'll try that. http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@662 PS11, Line 662: Should only be called : // when a block is fully written, > nit: mind updating this? Done http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@749 PS11, Line 749: auto en > nit: auto or size_t? Done http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@750 PS11, Line 750: // If we have an encryption heade > Please add a comment describing why this is here. Done http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@937 PS11, Line 937: } > nit: const? Done http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@1095 PS11, Line 1095: ritten. > I'm curious how this changed? What in this patch results in 0-length record We have some tests that create empty blocks, like TestMetadataTruncation and TestReuseBlockIds. We always had these tests, but until now record->offset() was also 0. Now record->offset() is 4096 due to alignments, and *data_file_size is 64. http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/fs/log_block_manager.cc@1318 PS11, Line 1318: int64_t off = std::max(preallocated_offset_, block_start_offset); > Can you add a comment for this too? 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@827 PS8, Line 827: m, 1 > One alternative might be passing 'eh' as const pointer and removing the 'de Yea that's a good idea, thank you. I don't know why I didn't think of that. http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/18025/11/src/kudu/util/env_posix.cc@243 PS11, Line 243: AES128ECB = 0xFD, : AES192ECB = 0XFE, : AES256ECB = 0xFF, > nit: I'd also expect just adding some bit (e.g. the MSB) for ECB vs CTR, li These are special values that should never be written to an encryption header. The 0x03-0xFC range is reserved for future use if we ever want to add support for some other encryption ciphers. 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)); > Looking through this again, it seems like whenever we deal with Read() and Yea, the OffsetFromEnd approach has occurred to me too, but I discarded it because of LBM. -- 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: 13 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: Tue, 25 Jan 2022 21:45:52 +0000 Gerrit-HasComments: Yes
