Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20447 )
Change subject: IMPALA-12418: Include crypto functions supported by Hive ...................................................................... Patch Set 6: (22 comments) http://gerrit.cloudera.org:8080/#/c/20447/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20447/6//COMMIT_MSG@25 PS6, Line 25: Testing: The patch is thouroughly tested and the tests are included in Let's do a test run on CentOS 7 to ensure we retain support for older OpenSSL versions, and Ubuntu 22 to check for any issues with OpenSSL 3. http://gerrit.cloudera.org:8080/#/c/20447/6//COMMIT_MSG@27 PS6, Line 27: Change-Id: I3902f2b1d95da4d06995cbd687e79c48e16190c9 nit: I normally see an empty line before Change-Id. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@160 PS6, Line 160: "space() result", These lines should still be indented 4 spaces. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1347 PS6, Line 1347: StringValue::FromStringVal(key), &result)) { This line should still be indented. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1820 PS6, Line 1820: // in AES_ECB always takes blocks of 128 bits sizes so, it should be padded to 128 Please clarify how this impacts the length required to be allocated for output. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1826 PS6, Line 1826: EVP_CIPHER_CTX* cipher_ctx = EVP_CIPHER_CTX_new(); This function doesn't exist in OpenSSL 1.0.2. It appears it was added in 1.1.0. I think we need to use EVP_CIPHER_CTX_init as shown in https://www.openssl.org/docs/man1.0.2/man3/EVP_EncryptInit_ex.html. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1829 PS6, Line 1829: EVP_CIPHER_CTX_free(cipher_ctx); We shouldn't call free on a nullptr. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1834 PS6, Line 1834: if(!EVP_EncryptInit_ex(cipher_ctx, cipher, nullptr, key, nullptr)) { EVP_EncryptInit_ex is considered a legacy function in https://www.openssl.org/docs/man3.2/man3/EVP_EncryptInit_ex.html. However the ex2 variant was only added in OpenSSL 3.0. Please document that we're using the older version to retain support for OpenSSL 1.0.2. Also note that we don't provide an IV because ECB mode can't use one. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1842 PS6, Line 1842: if(!EVP_EncryptUpdate(cipher_ctx, output, &len, input, input_len)) { Let's add a DCHECK that len >= input_len and len <= the length we require be allocated for output (probably helpful to make that a utility function). If we need to, we can delay DCHECK until after EVP_EncryptFinal_ex, since that encrypts any partial block. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1848 PS6, Line 1848: int cipher_len = 0; nit: why not int cipher_len = len; http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1865 PS6, Line 1865: const unsigned char* key, unsigned char* output, const EVP_CIPHER* cipher) { Please document what length we require allocated for output here. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1867 PS6, Line 1867: EVP_CIPHER_CTX* cipher_ctx = EVP_CIPHER_CTX_new(); See comment at line 1826. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1870 PS6, Line 1870: EVP_CIPHER_CTX_free(cipher_ctx); See comment at line 1829. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1875 PS6, Line 1875: if(!EVP_DecryptInit_ex(cipher_ctx, cipher, nullptr, key, nullptr)) { Same comment as line 1834. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1899 PS6, Line 1899: cipher_len += len; Let's also add DCHECKs that cipher_len contains the full results and does not exceed the bounds of the 'output' buffer. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1913 PS6, Line 1913: int output_len = input.len + (16 - input.len % 16); Let's use EVP_CIPHER_block_size to determine this after selecting cipher. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1934 PS6, Line 1934: memcpy(input_vec, input.ptr, input.len); Why do we have input_vec, and copy to it? http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1935 PS6, Line 1935: memcpy(key_vec, key.ptr, key.len); Why do we have key_vec? http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1936 PS6, Line 1936: // DCHECK(ret_size > output_len); Remove commented out code. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1938 PS6, Line 1938: ctx->SetError("AES encryption output return size is not expected size"); If we encounter this, won't we have already encountered an error and SetError? Would this overwrite the previous error details? http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1952 PS6, Line 1952: int output_len = input.len + (16 - input.len % 16); Let's use EVP_CIPHER_block_size to determine this. http://gerrit.cloudera.org:8080/#/c/20447/6/be/src/exprs/string-functions-ir.cc@1971 PS6, Line 1971: int ret_size = decrypt_ecb(ctx, input.ptr, input.len, key.ptr, output_vec, cipher); We need to handle where ret_size == -1 (decrypt_ecb errored). -- 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: 6 Gerrit-Owner: Anonymous Coward <[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-Comment-Date: Fri, 19 Jan 2024 17:59:44 +0000 Gerrit-HasComments: Yes
