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 25: (15 comments) > Uploaded patch set 25. 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: Testing: The patch is thouroughly tested and the tests are included in > nit: If we actually plan to do these, I'd put them in JIRA. Otherwise maybe Okay, will create jiras for 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 not supported then the default > nit: "not supported then the default mode is chosen". "by default the defau Done 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 wh We can alter the number of feedback bits by using the FeedbackSize method. That is what brings flexibility in implementation. 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 bit key lengths"); > nit: I think "bit key lengths" is better grammar. Done 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. Done http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/exprs/string-functions-ir.cc@1958 PS24, Line 1958: ctx->SetError(Substitute("Invalid AES 'mode': $0", StringPiece > Same question, should this return since we set an error? Done http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util-test.cc File be/src/util/openssl-util-test.cc: http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util-test.cc@67 PS22, Line 67: > These are unit tests for the class EncryptionKey. We've added new functiona Done 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: > I think we should also test ECB encryption if the EncryptionKey class suppo Done http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/util/openssl-util-test.cc@79 PS24, Line 79: > See my reply to L67. Done http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util.cc@197 PS22, Line 197: void EncryptionKey::InitializeRandom(int key_len, int iv_len) { > Have you considered this? SetCipherMode() is not new to this change, it existed in the base code as well. it might throw errors in other files if we modify it. 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@232 PS24, Line 232: const char* err_context = encrypt ? "encrypting" : "decrypting"; > nit: try to be consistent, either "128/256-bit" or "128 / 256-bit". Looks l Done http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/util/openssl-util.cc@237 PS24, Line 237: int padding_flag = padding_enabled ? 1 : 0; > nit: add a space after "well-optimized" Done http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/util/openssl-util.cc@266 PS24, Line 266: } > I don't think this works well with ECB. If we have for example two chunks, Done http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/util/openssl-util.cc@275 PS24, Line 275: to sm > Should be 'output_offset'. Done http://gerrit.cloudera.org:8080/#/c/20447/24/be/src/util/openssl-util.cc@277 PS24, Line 277: fset > Should be 'output_offset'. 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: 25 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: Wed, 13 Nov 2024 19:23:36 +0000 Gerrit-HasComments: Yes
