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 23: (12 comments) > Patch Set 21: > > (3 comments) > > Thanks for the new patch sets! > I have one concern regarding the default mode. I asked you to log the fact > which mode was chosen as default, but I think it would be safer to not have a > default mode at all. Users should always provide explicitly what encryption > mode they are using to be aware what mode was used to encrypt their data. Hi, I had brought it up with Kurt and he has replied on the discussion thread as well. I hope that solves the concern. http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/exprs/string-functions-ir.cc@1810 PS22, Line 1810: expression, key, AES mode and iv vector > These are the same for CTR and CFB on the next line, it could be merged lik I've kept it separate as of now as it'll have aad added to it as well in the next patch(future steps). http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/exprs/string-functions-ir.cc@1813 PS22, Line 1813: If a particular mode passed by user is not supported then by default : // the default mode is chosen. > I don't think it's true based on the code. Done http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/exprs/string-functions-ir.cc@1864 PS22, Line 1864: NULL" > Should be "NULL" - the empty string is invalid. Done http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/exprs/string-functions-ir.cc@1922 PS22, Line 1922: // GCM mode expects expression, key, AES mode and iv vector. > See L1810. I've kept it separate as of now as it'll have aad added to it as well in the next patch(future steps). 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: // Check all the modes > We should also test the 128 bit modes, and also ECB. See also L102. Have added for 128 bit mode. For ECB we might have to add a new function. I think the end to end tests should be sufficient, but if these are necessary, I can add them in the next patch. http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util.h File be/src/util/openssl-util.h: http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util.h@133 PS22, Line 133: /// The key and initialization vector (IV) required to encrypt and decrypt a buffer of > The original (before the first patch set) was: Done http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util.h@139 PS22, Line 139: arbitrary-length ciphertexts > Except for ECB, isn't it? Done http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util.h@176 PS22, Line 176: In other modes, 'out' buffer should be : /// at least as big as the length of the 'data' > We've now determined that there is not extra block in case of ECB DEcryptio 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) { > Some unit tests in openssl-util-test.cc are failing because this function d Done http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util.cc@231 PS22, Line 231: > Here, in the EncryptionKey class, ECB is not disabled for encryption. It is So should I include ECB in the comment for encryption? Wouldn't that lead to confusion. http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util.cc@276 PS22, Line 276: > Is it a valid use case if '*out_len' is not 0 at this point? If not, why no Addressed http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util.cc@291 PS22, Line 291: // Finalize encryption or decryption. > The offset here should not depend on whether the user provided a non-null p 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: 23 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: Thu, 31 Oct 2024 09:17:23 +0000 Gerrit-HasComments: Yes
