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 19: (15 comments) http://gerrit.cloudera.org:8080/#/c/20447/19//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20447/19//COMMIT_MSG@31 PS19, Line 31: G The space before "GCM" shouldn't be removed. http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@1936 PS17, Line 1936: > What do you mean? The same as on L2058. If AAD is not included in this change, it shouldn't be mentioned for either encryption or decryption. http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@2058 PS17, Line 2058: > It'll come in a separate patch as is stated in the future steps, because it If it comes in a separate change, it shouldn't be included in the comment in this change. Applies also to aes_encrypt(). http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@2134 PS17, Line 2134: > I don't think we can replace it, it leads to an error as well. In what cases does it lead to an error? http://gerrit.cloudera.org:8080/#/c/20447/19/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/19/be/src/exprs/string-functions-ir.cc@1842 PS19, Line 1842: Unneeded empty line. http://gerrit.cloudera.org:8080/#/c/20447/19/be/src/exprs/string-functions-ir.cc@1875 PS19, Line 1875: ModeToString This is not informative, this will invariably output "INVALID". We should include the mode string that was entered by the user in the error message - the variable 'mode' instead of 'cipher_mode'. It should be something like 'Invalid AES mode: "mode_entered_by_user"'. Use the Substitute() function instead of the string concatenation operator. Applies also to aes_encrypt(). http://gerrit.cloudera.org:8080/#/c/20447/19/be/src/exprs/string-functions-ir.cc@1883 PS19, Line 1883: ctx->SetError(("IV vector required for " + As Michael said, prefer Substitute() for concatenation, not operator+(). Applies also to L1956 and L1968. http://gerrit.cloudera.org:8080/#/c/20447/19/be/src/exprs/string-functions-ir.cc@1923 PS19, Line 1923: Unneeded empty line. http://gerrit.cloudera.org:8080/#/c/20447/19/be/src/exprs/string-functions.h File be/src/exprs/string-functions.h: http://gerrit.cloudera.org:8080/#/c/20447/19/be/src/exprs/string-functions.h@106 PS19, Line 106: , The param name 'ctx' should be named also here. http://gerrit.cloudera.org:8080/#/c/20447/15/be/src/exprs/string-functions.h File be/src/exprs/string-functions.h: http://gerrit.cloudera.org:8080/#/c/20447/15/be/src/exprs/string-functions.h@138 PS15, Line 138: static void RegexpPrepare(FunctionContext*, FunctionContext::FunctionStateScope); > Done This has gone back to the version without capitalisation in PS19, it should be changed back to the correct state. Also, add a full stop (".") at the end of the line. http://gerrit.cloudera.org:8080/#/c/20447/19/be/src/util/openssl-util.h File be/src/util/openssl-util.h: http://gerrit.cloudera.org:8080/#/c/20447/19/be/src/util/openssl-util.h@140 PS19, Line 140: // Unneeded slashes. http://gerrit.cloudera.org:8080/#/c/20447/19/be/src/util/openssl-util.h@184 PS19, Line 184: || mode_ == AES_CIPHER_MODE::AES_128_GCM; Add 4 more spaces because it is the continuation of the previous line. Applies also to L190. http://gerrit.cloudera.org:8080/#/c/20447/19/be/src/util/openssl-util.h@196 PS19, Line 196: gcm_tag The variable name has an underscore at the end. Also, put it inside single quotes. Applies also to L198. http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.cc@227 PS17, Line 227: AES key, and t > Done I asked to go back to the original text, but also to add changes where appropriate - this latter part is missing. For example: - now we can also use a 128 bit key - we have added new cipher modes - ECB does not support arbitrary length In addition, missing spaces could also be corrected. http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.cc@405 PS17, Line 405: } > I've added a check for length which does the same thing. Now all the names of the modes have the same length but it is not guaranteed that it will always be like this. Using the approach I described in the previous commit is more future-proof, and additionally - it expresses the intent more clearly, making the code more easier to understand - extracting the mode names into static constants and using them also in ModeToString() makes sure we're using the same values in both functions and signals this fact to the reader. If you want the comparison to be case-insensitive, you could try boost::iequals() (I haven't tried it). -- 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: 19 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: Tue, 03 Sep 2024 16:39:15 +0000 Gerrit-HasComments: Yes
