Noemi Pap-Takacs has posted comments on this change. ( http://gerrit.cloudera.org:8080/20447 )
Change subject: IMPALA-13039: AES Encryption/ Decryption Support in Impala ...................................................................... Patch Set 13: (15 comments) Thank you for working on this! I just left a few nits and some ideas about logging and error messages. http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1810 PS13, Line 1810: ECB mode "other modes" or "CTR and CFB mode." You describe CTR and CFB later. It would be nice to list here what parameters are required for each mode. Also, encryption in ECB mode is not supported, so it is a bit confusing to list here what parameters are necessary for ECB encryption. I agree that it is useful to include a detailed description about the modes. I would put this long comment above the aes_decrypt function since that is supported for all modes. That way the comment aligns better with the code. And just a suggestion: you can switch the order of encrypt and decrypt functions like this: //long comment about all supported modes. aes_decrypt() //short comment, ECB is not supported. aes_encrypt() http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1857 PS13, Line 1857: // If user passed an empty field in mode, default AES encryption mode is chosen I think it could be useful to log this fact, and also the encryption mode used by default. Not necessarily here, but rather where the use of default mode was decided. Where is the empty mode field first encountered? We should log there once per query. (VLOG_QUERY) Or here, something like: VLOG(3) << "No AES encryption mode was specified by user. Using " << cipher_mode << " mode as default." Where 'cipher_mode' is the default encryption mode. http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1872 PS13, Line 1872: ECB mode is not supported in aes_encrypt nit: "ECB mode is not supported for encryption." http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1877 PS13, Line 1877: non ECB You could include the 'cipher_mode' in the error message. ECB encryption is not supported anyway, so it is better not to mention it to avoid confusion. http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1881 PS13, Line 1881: in case of non ECB modes This part can be deleted. It is unnecessarily confusing, since ECB is not supported for encryption. http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1903 PS13, Line 1903: st nit: 'status'. Descriptive variable names help readers understand the code. http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1906 PS13, Line 1906: encrypt nit: "encryption" http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1924 PS13, Line 1924: ECB mode Mention the other supported modes as well in the comment. See my other comment on L1810. http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1944 PS13, Line 1944: if (mode.is_null) { : cipher_mode = encryption_key.GetSupportedDefaultMode(); : } See my other comment on L1857. http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1960 PS13, Line 1960: non ECB You could include the 'cipher_mode' in the error message. http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1986 PS13, Line 1986: st nit: 'status' http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1988 PS13, Line 1988: decrypt nit: "decryption" http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util-test.cc File be/src/util/openssl-util-test.cc: http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util-test.cc@101 PS13, Line 101: // Check both CTR & CFB Comment does not align with the code. GCM should also be included in the comment. http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.h File be/src/util/openssl-util.h: http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.h@162 PS13, Line 162: /// nit: extra '///' http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.h@163 PS13, Line 163: incase nit: in case -- 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: 13 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, 26 Jul 2024 14:37:55 +0000 Gerrit-HasComments: Yes
