Pranav Lodha has posted comments on this change. ( http://gerrit.cloudera.org:8080/20447 )
Change subject: IMPALA-13039: AES Encryption/ Decryption Support in Impala ...................................................................... Patch Set 18: (32 comments) 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@1866 PS13, Line 1866: ncryptionKey > Also include the mode string (which turned out to be invalid) in the log. Will add it in the next patch 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: > Where can AAD be added? What do you mean? http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@1991 PS17, Line 1991: nput > Yes, for static methods we should use the explicit static syntax. Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@1993 PS17, Line 1993: yption_key.En > ditto Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@1995 PS17, Line 1995: > ditto Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@2058 PS17, Line 2058: > Where can AAD be provided? It'll come in a separate patch as is stated in the future steps, because it introduces overloading which will increase the size of this patch unnecessarily. http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@2059 PS17, Line 2059: > expect Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@2087 PS17, Line 2087: > Decryption would be better. Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@2088 PS17, Line 2088: > style nit (if applicable to Impala's code): consider an explicit notation o Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@2090 PS17, Line 2090: > style nit: ditto Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@2094 PS17, Line 2094: > nit: missing a space after 'if' and before the parenthesis Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@2124 PS17, Line 2124: > The conditional is only needed for the 'len' parameter, so this could be Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@2134 PS17, Line 2134: > Now that we've confirmed that the additional AES_BLOCK_SIZE is only needed I don't think we can replace it, it leads to an error as well. http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions.h File be/src/exprs/string-functions.h: http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions.h@139 PS17, Line 139: o > nit: remove an extra space? Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions.h@140 PS17, Line 140: r > ditto Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions.h@142 PS17, Line 142: p > dito Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions.h@142 PS17, Line 142: > ditto Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.h File be/src/util/openssl-util.h: http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.h@71 PS17, Line 71: . > Use "." instead of ",". Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.h@71 PS17, Line 71: E > Capital "E": "Enum". Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.h@139 PS17, Line 139: /// plaintext and the AAD, and GCM also protects the confidentiality of the > Now, compared to the original state, we're missing "and GCM also protects t Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.h@159 PS17, Line 159: /// buffers must not overlap. In GCM mode, the hash tag is kept internally (in the > 'out_len' should be mentioned in the comment: Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.h@194 PS17, Line 194: void InitializeFields(const > nit: could this be Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.h@195 PS17, Line 195: > nit: could this be 'const uint8_t*'? Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.h@195 PS17, Line 195: > Why do you call it 'expr'? 'tag' would be better. Don't forget to rename it Done 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 > What other modes? In the original comment only CTR is mentioned. Also, what Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.cc@375 PS17, Line 375: case AES_CIPHER_MODE::AES_128_ > nit: this line might be removed Invalid and default serve different purposes. http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.cc@404 PS17, Line 404: return "INVALID"; > From the usability and ease-of-use perspective, does it make sense to use c Done http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.cc@405 PS17, Line 405: } > We should also add a test that prefixes of valid modes are not accepted. I've added a check for length which does the same thing. http://gerrit.cloudera.org:8080/#/c/20447/13/common/function-registry/impala_functions.py File common/function-registry/impala_functions.py: http://gerrit.cloudera.org:8080/#/c/20447/13/common/function-registry/impala_functions.py@520 PS13, Line 520: [[ > The extra spaces should be removed. Done http://gerrit.cloudera.org:8080/#/c/20447/17/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: http://gerrit.cloudera.org:8080/#/c/20447/17/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3456 PS17, Line 3456: '' > Shouldn't this be under the "mode not specified" queries after L3585?. Done http://gerrit.cloudera.org:8080/#/c/20447/17/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3586 PS17, Line 3586: > Sorry, not the key but the encryption mode. Why isn't the empty string inva Done http://gerrit.cloudera.org:8080/#/c/20447/17/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3601 PS17, Line 3601: ==== > We should test the AES modes explicitly. The fallback mode may change in th Done -- 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: 18 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 11:54:54 +0000 Gerrit-HasComments: Yes
