Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20447 )
Change subject: IMPALA-13039: AES Encryption/ Decryption Support in Impala ...................................................................... Patch Set 22: (8 comments) 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 like "The GCM, CTR and CFB modes expect ...". 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 as defined in openssl library. I don't think it's true based on the code. http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/exprs/string-functions-ir.cc@1864 PS22, Line 1864: empty Should be "NULL" - the empty string is invalid. 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. 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 for encrypting and decrypting a The original (before the first patch set) was: "The key and initialization vector (IV) required to encrypt and decrypt a buffer of data. This should be regenerated for each buffer of data." The original contained to sentences. The first one describes that this class stores the key and the IV for encryption. The second notes that it should be regenerated for each buffer. In the new version, the sentences are merged and the meaning of the first sentence is lost. I don't think this needs to be reworded. 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? http://gerrit.cloudera.org:8080/#/c/20447/22/be/src/util/openssl-util.h@176 PS22, Line 176: The 'out' buffer must contain sufficient length to hold : /// the extra padding block in case of ECB mode We've now determined that there is not extra block in case of ECB DEcryption, only for ENcryption. 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@276 PS22, Line 276: *out_len Is it a valid use case if '*out_len' is not 0 at this point? If not, why not just set it to 'output_len'? See also around L291, if '*out_len' started out as non-zero, wouldn't we index incorrectly? -- 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: 22 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, 25 Oct 2024 12:38:42 +0000 Gerrit-HasComments: Yes
