Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17251 )
Change subject: [encryption] Add encryption support to Env ...................................................................... Patch Set 8: (10 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: : // Treating it as an unencrypted file should yield garbage and not contain the : // cleartext written to the file. > It would be nice to comment on the intention of these actions. Are those o Done http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env-test.cc@1314 PS7, Line 1314: ASSERT_OK(env_->NewRandomAccessFile(random_opts, kFile, &random)); > Does it make sense to add a test case where the reading from the file is pe Done 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: atus DoEncryptV(co > I see. Probably, at some point we might move the common piece out of secur I didn't see any mention of it in the official docs, but in one example on the openssl wiki I found that they use ERR_get_error() after EVP_* function failures, so ended up moving openssl_util into util in a separate commit and changing these to use the OPENSSL_RET_NOT_OK macro. 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@224 PS7, Line 224: // key infra is in place. > Do we want to file a bug to track this which is linked to parent/epic JIRA Done http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env_posix.cc@380 PS7, Line 380: nd > Nit: it's Done http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env_posix.cc@397 PS7, Line 397: unsigned char scratch_clear[kEncryptionBlockSize]; : unsigned char scratch_cipher[kEncryptionBlockSize]; : > I'm not familiar with the OpenSSL encryption APIs but should these be zero I don't think they need to be. EVP_EncryptUpdate() encrypts scratch_clear and writes the ciphertext into scratch_cipher. These are not used ever, it's only to advance the internal counter to the correct offset. It doesn't matter if it encrypts garbage or 0s, but this way we save the cost of initializing these arrays. http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env_posix.cc@400 PS7, Line 400: SL_RET_NOT_OK(EVP > nit: does it make sense to add Done http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env_posix.cc@417 PS7, Line 417: DCHECK_LE(out_length, ciphertext[i].size()); > nit: maybe, also add Done http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env_posix.cc@579 PS7, Line 579: EAN > I got a bit confused between idx and i below. Could you consider renaming o Done http://gerrit.cloudera.org:8080/#/c/17251/7/src/kudu/util/env_posix.cc@586 PS7, Line 586: bytes_req += data[i].size(); > Perhaps the scoped_cleanup below needs to move above around L579. The scoped cleanup needs to be in an outer scope as the encrypted_data is used outside this scope. It's true that this would leak encrypted_buf though, so changed RETURN_NOT_OK to RETURN_NOT_OK_EVAL and delete[] it there. -- 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: 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: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 09 Sep 2021 18:22:37 +0000 Gerrit-HasComments: Yes
