[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/20447 )
Change subject: IMPALA-13039: AES Encryption/ Decryption Support in Impala ...................................................................... Patch Set 12: (23 comments) > Uploaded patch set 12: Commit message was updated. http://gerrit.cloudera.org:8080/#/c/20447/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20447/10//COMMIT_MSG@19 PS10, Line 19: 1. AES-GCM, AES-CTR, and AES-CFB encryption functionalities and > Can you include any rationale for implementing 3 different methods. Are the Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@182 PS3, Line 182: 4_t > Not done. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@551 PS3, Line 551: std: > Not done. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@854 PS3, Line 854: f not in UTF8 m > Not done. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1076 PS3, Line 1076: ng e > Not done. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1112 PS3, Line 1112: == NUL > Not done. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1344 PS3, Line 1344: ngVa > Not done. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1624 PS3, Line 1624: le m = static_cast<double > Not done. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1625 PS3, Line 1625: le jaro_similarity = 1.0 > Not done. Done http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/exprs/string-functions-ir.cc@1832 PS10, Line 1832: // > I don't think we support random access on a single slot, so this doesn't se Done http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/exprs/string-functions-ir.cc@1840 PS10, Line 1840: > I'm not sure why this is a useful feature to add. These are added as fallbacks for gcm and were already existing in our openssl files. http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/exprs/string-functions-ir.cc@1869 PS10, Line 1869: > I'd rather first SetCipherMode, then check IsEcbMode and error. That helps I hope its fine now. http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/exprs/string-functions-ir.cc@1877 PS10, Line 1877: return StringVal::null(); > nit: unnecessary space after '('. Done http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/exprs/string-functions-ir.cc@1947 PS10, Line 1947: else { > This could use !t.IsEcbMode() Done http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/exprs/string-functions-ir.cc@1955 PS10, Line 1955: t.SetCipherMode(mode_); > Use !t.IsEcbMode() Done http://gerrit.cloudera.org:8080/#/c/20447/9/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/20447/9/be/src/util/openssl-util.cc@77 PS9, Line 77: #if OPENSSL_VERSION_NUMBER < 0x10100000L > Why did you rewrite this? It just makes the patch bigger and increases the It's just to eliminate we and us http://gerrit.cloudera.org:8080/#/c/20447/9/be/src/util/openssl-util.cc@245 PS9, Line 245: } > No need to rewrite this. removed we. http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/util/openssl-util.cc@223 PS10, Line 223: int padding_flag = padding_enabled ? 1 : 0; > This takes an int as argument. It looks like EVP_CIPHER_CTX_set_padding tre Done http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/util/openssl-util.cc@245 PS10, Line 245: } > Why do we do init twice? In the first initialization, we initialize only evpCipher and in the second initialization, we set key and iv vector. We do this because, we take variable length iv vector, hence, we have to initialize iv length in gcm mode first before initializing iv vector. http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/util/openssl-util.cc@287 PS10, Line 287: if (success != 1) { > I think we'll need to, yes. I don't think we should add a patch that disabl Done http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/util/openssl-util.cc@310 PS10, Line 310: case AES_CIPHER_MODE::AES_256_CFB: return EVP_aes_256_cfb(); > This would be better as a switch statement. Done http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/util/openssl-util.cc@402 PS10, Line 402: case AES_CIPHER_MODE::INVALID: return "INVALID"; > nit: in this case memcmp and strncmp would give the same result, and strncm Done http://gerrit.cloudera.org:8080/#/c/20447/10/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: http://gerrit.cloudera.org:8080/#/c/20447/10/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3387 PS10, Line 3387: ---- RESULTS > nit: I'd throw in an explicit round-trip test: 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: 12 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Anonymous Coward <[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-Comment-Date: Wed, 24 Jul 2024 16:58:59 +0000 Gerrit-HasComments: Yes
