Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20447 )
Change subject: IMPALA-13039: AES Encryption/ Decryption Support in Impala ...................................................................... Patch Set 22: (4 comments) http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util-test.cc File be/src/util/openssl-util-test.cc: http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util-test.cc@67 PS22, Line 67: // Check all the modes We should also test the 128 bit modes, and also ECB. See also L102. http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util.cc@197 PS22, Line 197: void EncryptionKey::InitializeRandom() { Some unit tests in openssl-util-test.cc are failing because this function doesn't set 'key_length_' and 'iv_length_'. These variables are added by this patch, and are set in InitializeFields(), but not here. This function could also take 'key_len' and 'iv_len' as parameters. On the other hand, starting from this change, key length is dependent on the operation mode, so maybe setting the operation mode could also be part of initialization, instead of a separate SetCipherMode() method. http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util.cc@231 PS22, Line 231: ECB Here, in the EncryptionKey class, ECB is not disabled for encryption. It is not necessarily bad, the main thing is that end users of Impala shouldn't use it, not developers. But the comment should be correct. http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util.cc@291 PS22, Line 291: (out_len == nullptr ? offset : *out_len), The offset here should not depend on whether the user provided a non-null pointer for 'out_len'. If it is NULL, the current code is incorrect. Instead, in the loop at L267, we should also accumulate an 'output_offset', similarly to 'offset' (which could be renamed to 'input_offset'). We should use 'output_offset' here. In the loop, 'out_len' doesn't need to be updated, it's enough to set "if (out_len != nullptr) *out_len = output_offset + final_out_len;" at L300. -- To view, visit http://gerrit.cloudera.org:8080/20447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3902f2b1d95da4d06995cbd687e79c48e16190c9 Gerrit-Change-Number: 20447 Gerrit-PatchSet: 22 Gerrit-Owner: Pranav Lodha <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Pranav Lodha <[email protected]> Gerrit-Comment-Date: Fri, 25 Oct 2024 14:13:32 +0000 Gerrit-HasComments: Yes
