Alexey Serbin 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: (27 comments) Just glancing over. 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@159 PS13, Line 159: "space() result", nit for here and below: if you want to update indentation in non-related code, I'd suggest you do so in a separate changelist. That's better for many reasons, including: * less conflicts if cherry-picking this patch to other branches * cleaner logical separation of changes each patch in the repository * easier to track relevant changes in the source repository * easier for the reviewers to review the code * etc. http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1346 PS13, Line 1346: nit: the indent seems to be off Also, why is this change? http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1845 PS13, Line 1845: return StringVal::null(); Any need to signal an error condition via 'ctx' in this case? http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1865 PS13, Line 1865: if(cipher_mode == AES_CIPHER_MODE::INVALID) { style nit: missing space before parenthesis http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1903 PS13, Line 1903: (uint8_t*) style nit: if using reinterpet_cast elsewhere, why not using it here? http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1912 PS13, Line 1912: (uint8_t*) ditto for the reinterpret_cast http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions.h File be/src/exprs/string-functions.h: http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions.h@105 PS13, Line 105: openssl libraries nit: OpenSSL library http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions.h@105 PS13, Line 105: impala nit: Impala http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions.h@105 PS13, Line 105: Support How does 'support'? I'd think the in-line documentation would say what particular function does, like the in-line docs for other functions in this file 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@79 PS13, Line 79: INVALID It's a bit funny: the comment says it's "enum of all the AES modes that are currently supported". What does INVALID then mean? :) http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.h@152 PS13, Line 152: contains must contain? http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.h@165 PS13, Line 165: 'in' What is 'in'? Is that actually 'data'? http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.h@181 PS13, Line 181: nit: the indent seems to be off -- if that's a continuation of the line above, it's strange that it starts with the same indent http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.h@187 PS13, Line 187: ditto http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.h@190 PS13, Line 190: It initializes key and iv nit: Initializes 'key' and 'iv' http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.h@216 PS13, Line 216: data_read refers to the output length. Why not 'out_len' then? What is the unit of length? Bytes? Would be great to clarify on that in the comment. http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.cc@77 PS13, Line 77: #if OPENSSL_VERSION_NUMBER < 0x10100000L : return SSLv23_method()->version; : #else : // OpenSSL 1.1+ doesn't allow runtime detection of the supported TLS version. It : // is assumed that the OpenSSL library linked against supports only up to TLS 1.2. : return TLS1_2_VERSION; : #endif That doesn't seem to be a part of this changelist, but just a side note: in fact, OpenSSL 1.1.1 and newer supports TLSv1.3: https://wiki.openssl.org/index.php/TLS1.3 If you want to update this piece, the code at https://github.com/apache/kudu/blob/065acfbff08ab7d77308ebe9f874d9e1d08d608d/src/kudu/security/tls_context.cc#L127-L141 could provide a reference. http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.cc@271 PS13, Line 271: *data_read += out_len; style nit: wrap this into {}, similar to the 'if' clause above? http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.cc@297 PS13, Line 297: *data_read += final_out_len; style nit: wrap this into {}, similar to the 'if' clause above? http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.cc@339 PS13, Line 339: memcpy(iv_, iv, iv_length_); nit: do you want to add DCHECK() for iv? http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.cc@344 PS13, Line 344: expr + len This is a very weird notation for a function of such signature: why not to pass (expr + len) at the call sites and eliminate the 'len' parameter? http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.cc@348 PS13, Line 348: out + len This is a very weird notation for a function of such signature if looking at what it actually does. Why not to pass (out + len) from the upper level then, not introducing the phony 'len' parameter? http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.cc@382 PS13, Line 382: case AES_CIPHER_MODE::INVALID: : return false; What is this for? Why not to handle it with 'default' case below? http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.cc@407 PS13, Line 407: case AES_CIPHER_MODE::INVALID: return "INVALID"; nit: this can be dropped since any invalid mode is already handled at line 409 http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.cc@415 PS13, Line 415: } : else if style nit: this doesn't seem to match with the rest of the style in the files in util 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: nit: remove extra spaces? http://gerrit.cloudera.org:8080/#/c/20447/13/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: http://gerrit.cloudera.org:8080/#/c/20447/13/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3481 PS13, Line 3481: STRING It seems there isn't a single case with decryption failure scenario (e.g., corrupted input data). Does it make sense to add one? Same for the encryption failure (e.g., at least cover the case with wrong encryption mode, type on the name of the cipher, etc.) -- 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 17:26:49 +0000 Gerrit-HasComments: Yes
