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 28: (8 comments) http://gerrit.cloudera.org:8080/#/c/20447/28/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/28/be/src/exprs/string-functions-ir.cc@1811 PS28, Line 1811: Nit: too much indentation in this function: it should be 2 spaces. http://gerrit.cloudera.org:8080/#/c/20447/28/be/src/exprs/string-functions-ir.cc@1814 PS28, Line 1814: throw std::logic_error("Expression cannot be NULL."); Why don't we allow the expression to be NULL? I think it makes sense to simply return NULL in this case. If we do that, it may be simpler to leave this part out of this function and put it back directly into aes_encrypt() and aes_decrypt(). http://gerrit.cloudera.org:8080/#/c/20447/28/be/src/exprs/string-functions-ir.cc@1822 PS28, Line 1822: throw std::logic_error("AES only supports 128 and 256 bit key lengths."); There could still be a mismatch between the chosen AES mode and the key length. We should check those in a pair. Unifying InitializeFields with SetCipherMode, the unified function could be a good place to do that. http://gerrit.cloudera.org:8080/#/c/20447/28/be/src/udf/udf.h File be/src/udf/udf.h: http://gerrit.cloudera.org:8080/#/c/20447/28/be/src/udf/udf.h@238 PS28, Line 238: void SetLoggerFlag(); > Have you verified each UDF gets a unique FunctionContext? I agree we should use Set/GetFunctionState(). The log flooding problem is specific to aes_encrypt/decrypt, so we shouldn't add a new method to FunctionContext because of this. Instead, aes_encrypt() and aes_decrypt() should retrieve the function state from the FunctionContext and use it. Although it is quite old, you can have a look at this change for an example: https://gerrit.cloudera.org/#/c/5776/, especially the string-functions-ir.cc and function-registry/impala_functions.py files. http://gerrit.cloudera.org:8080/#/c/20447/28/be/src/util/openssl-util-test.cc File be/src/util/openssl-util-test.cc: http://gerrit.cloudera.org:8080/#/c/20447/28/be/src/util/openssl-util-test.cc@93 PS28, Line 93: return; Don't return here, that will quit the entire function, leaving anything, e.g. AES_CIPHER_MODE::AES_128_GCM untested. Use 'continue' instead. http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/util/openssl-util.h File be/src/util/openssl-util.h: http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/util/openssl-util.h@176 PS24, Line 176: In other modes, > Nit: "In other modes" is no longer needed. Please address this comment. 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(int key_len, int iv_len) { > SetCipherMode() is not new to this change, it existed in the base code as w It's true it's not new, I missed it. But if it throws errors in other files, it's not difficult change it there too. I just checked it and it's only used in the tests and string-functions-ir.cc, so it's very easy to modify it. http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util.cc@231 PS22, Line 231: > Yes, ECB should be included. It won't cause confusion because this class su Please address this comment. -- 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: 28 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: Kurt Deschler <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Pranav Lodha <[email protected]> Gerrit-Comment-Date: Wed, 27 Nov 2024 15:53:13 +0000 Gerrit-HasComments: Yes
