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 29: (11 comments) Thanks Pranav, it looks good! http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/exprs/string-functions-ir.cc@1823 PS29, Line 1823: bool* state = reinterpret_cast<bool*>( We should check the unlikely case that 'state' is NULL. I think we could do logging if it is. http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/exprs/string-functions-ir.cc@1848 PS29, Line 1848: // Initialize key and IV. This would be better after all the checks, so after we check for ECB mode (on L1852). http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/exprs/string-functions-ir.cc@1860 PS29, Line 1860: aes_prepare Following the naming convention, this should be AesPrepare(). Also applies to AesClose() and the main functions, AesDecrypt() and AesEncrypt(). Don't forget to update the symbol names in impala_functions.py. http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/exprs/string-functions-ir.cc@1882 PS29, Line 1882: If a particular mode passed by user is not supported then the default : // mode is chosen. This applies to modes that Impala supports (members of AES_CIPHER_MODE except for INVALID) but the openssl library we use does not support. This is different from the case when a user enters a mode that Impala doesn't support (for example a mode that doesn't exist). The comment should be clear about this. http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/util/openssl-util.h File be/src/util/openssl-util.h: http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/util/openssl-util.h@156 PS29, Line 156: mode_ Now that both Initialize*() functions set this explicitly, it could start off as INVALID. http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/util/openssl-util.h@177 PS29, Line 177: // Nit: add one more / to match the other lines. http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/util/openssl-util.h@198 PS29, Line 198: Nit: remove extra space. http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/util/openssl-util.cc@198 PS29, Line 198: uint64_t next_key_num = keys_generated.Add(1); We should also check that the chosen cipher mode ('m') is compatible with the given key length - we do check it in InitializeFields(). This check could also be extracted to a function and used in both Initialize*() functions. http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/util/openssl-util.cc@208 PS29, Line 208: mode_ = m; I think the two Initialize*() functions should behave in the same way regarding the mode, so also here we should fall back to a supported mode if 'm' is not supported. http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/util/openssl-util.cc@242 PS29, Line 242: and Now we've deleted that we also support ECB for decryption, it should be mentioned here. http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/util/openssl-util.cc@346 PS29, Line 346: if (mode_ == AES_CIPHER_MODE::AES_128_ECB || mode_ == AES_CIPHER_MODE::AES_128_GCM) { The code should make sure that this is an exhaustive check, so that if someone later adds a new mode, it should lead to an error if they forget to add it here. One way to do that would be a switch statement without a 'default' case: switch (mode_) { case AES_CIPHER_MODE::AES_128_ECB: case AES_CIPHER_MODE::AES_128_GCM: { if (key_len != 16) { ... } break; } case AES_CIPHER_MODE::AES_256_ECB: case AES_CIPHER_MODE::AES_256_GCM: case AES_CIPHER_MODE::AES_256_CTR: case AES_CIPHER_MODE::AES_256_CFB: { if (key_len != 32) { ... } break; } case INVALID: { return Status("Some error."); } } -- 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: 29 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: Kurt Deschler <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Pranav Lodha <[email protected]> Gerrit-Comment-Date: Mon, 13 Jan 2025 16:16:51 +0000 Gerrit-HasComments: Yes
