Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17251 )
Change subject: [encryption] Add encryption support to Env ...................................................................... Patch Set 7: (9 comments) http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env-test.cc File src/kudu/util/env-test.cc: http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env-test.cc@1291 PS7, Line 1291: ASSERT_OK(unencrpyted->ReadV(13, results)); : ASSERT_NE(result1, "This text"); : ASSERT_NE(result2, " is slightly longer"); It would be nice to comment on the intention of these actions. Are those only to make sure that non-encrypted files have different alignment vs encrypted? Or the most important thing is to make sure the contents in the file is encrypted? BTW, if that's only the latter, maybe reading data via Read() and searching for sub-string might be a better way to verify the contents of an encrypted file? http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env-test.cc@1314 PS7, Line 1314: } Does it make sense to add a test case where the reading from the file is performed till an EOF condition? http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env-test.cc@1325 PS7, Line 1325: size_t size1 = 10 Why not to try reading the whole contents till EOF in this case? It would be nice to add a small blurb to explain why. http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env-test.cc@1348 PS7, Line 1348: } BTW, what happens if trying to read the data till EOF? Does it work as expected and what's 'expected' in this case? http://gerrit.cloudera.org:8080/#/c/17251/2/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/17251/2/src/kudu/util/env_posix.cc@382 PS2, Line 382: ui > Unfortunately, I don't think I can use that macro here, as it's in security I see. Probably, at some point we might move the common piece out of security into security_util or something, if it's makes any sense. BTW, do you know whether these EVP_EncryptXXX function add anything to the OpenSSL's error stack? I guess they do not, but just to make sure there not need to http://gerrit.cloudera.org:8080/#/c/17251/2/src/kudu/util/env_posix.cc@567 PS2, Line 567: > I'm not sure I understand what that would look like. Anyway, I wouldn't wan Yep, I guess it's too early to optimize, but I was just curious whether you thought/tried any alternative here. The current approach looks fine as of now, of course. http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env_posix.cc@400 PS7, Line 400: EVP_EncryptUpdate nit: does it make sense to add DCHECK_LE(out_length, kEncryptionBlockSize) ? http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env_posix.cc@417 PS7, Line 417: DCHECK_EQ(out_length, cleartext[i].size()); nit: maybe, also add DCHECK_LE(out_length, ciphertext[i].size()) ? http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env_posix.cc@586 PS7, Line 586: RETURN_NOT_OK(DoEncryptV(kDummyEncryptionKey, offset, data, encrypted_data)); What happens with the memory allocated for 'encrypted_buf' when DoEncryptV() returns non-OK here? Would it be leaked? -- To view, visit http://gerrit.cloudera.org:8080/17251 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id5a81b5736a0c1970818dbbc4a234480476b8d41 Gerrit-Change-Number: 17251 Gerrit-PatchSet: 7 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: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 19 Apr 2021 21:57:49 +0000 Gerrit-HasComments: Yes
