Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20447 )
Change subject: IMPALA-13039: AES Encryption/ Decryption Support in Impala ...................................................................... Patch Set 24: (12 comments) Some nits, and some spots I think we need to return after SetError. http://gerrit.cloudera.org:8080/#/c/20447/24//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20447/24//COMMIT_MSG@40 PS24, Line 40: Future Steps: nit: If we actually plan to do these, I'd put them in JIRA. Otherwise maybe mention this as: "intentionally omitted from implementation" because we don't anticipate needing them. http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/exprs/string-functions-ir.cc@1813 PS24, Line 1813: // If a particular mode passed by user is not supported then by default nit: "not supported then the default mode is chosen". "by default the default" is a bit redundant. http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/exprs/string-functions-ir.cc@1841 PS24, Line 1841: // encryption and decryption, and it can operate with various feedback sizes, offering It's not clear how to control "feedback size". It's also not clear to me what "flexibility in implementation" means, that seems like fluff. http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/exprs/string-functions-ir.cc@1857 PS24, Line 1857: ctx->SetError("AES only supports 128 and 256 bits key length"); nit: I think "bit key lengths" is better grammar. http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/exprs/string-functions-ir.cc@1876 PS24, Line 1876: (reinterpret_cast<const char*>(mode.ptr), mode.len)).c_str()); Should this return? I'm not sure why we'd continue after setting an error. http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/exprs/string-functions-ir.cc@1958 PS24, Line 1958: (reinterpret_cast<const char*>(mode.ptr), mode.len)).c_str()); Same question, should this return since we set an error? http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/util/openssl-util-test.cc File be/src/util/openssl-util-test.cc: http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/util/openssl-util-test.cc@67 PS24, Line 67: // Check all the modes nit: this isn't really all the modes, since it doesn't include ECB. Can we add some special handling to do decryption-only ECB tests? http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/util/openssl-util-test.cc@79 PS24, Line 79: m == AES_CIPHER_MODE::AES_256_CFB || m == AES_CIPHER_MODE::AES_256_ECB) { ECB is never used. http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/util/openssl-util.cc@202 PS24, Line 202: RAND_bytes(key_, sizeof(key_)); nit: we could optimize this a bit by using a template to have separate 16-byte and 32-byte implementations, so we don't use more space or generate more random bytes than needed. But we don't need to hold this feature up for it. http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/util/openssl-util.cc@232 PS24, Line 232: // Start encryption/decryption. We use a 128/ 256-bit AES key and support GCM, CTR nit: try to be consistent, either "128/256-bit" or "128 / 256-bit". Looks like elsewhere you did "128/256-bit" (without spaces). http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/util/openssl-util.cc@237 PS24, Line 237: // well-optimized(instruction level parallelism) with hardware acceleration nit: add a space after "well-optimized" http://gerrit.cloudera.org:8080/#/c/20447/24/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: http://gerrit.cloudera.org:8080/#/c/20447/24/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3291 PS24, Line 3291: # AES encryption/ decryption examples: nit: "encryption/decryption" fits our normal style better. -- 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: 24 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, 01 Nov 2024 21:00:20 +0000 Gerrit-HasComments: Yes
