Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20447 )
Change subject: [WIP] IMPALA-13039: AES Encryption/ Decryption Support in Impala ...................................................................... Patch Set 10: (13 comments) http://gerrit.cloudera.org:8080/#/c/20447/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20447/10//COMMIT_MSG@19 PS10, Line 19: 1. AES-GCM, AES-CTR, and AES-CFB encryption functionalities and Can you include any rationale for implementing 3 different methods. Are there performance trade-offs around them? Which do we choose as a default? It looks like this may mostly be about OpenSSL features provided. So mostly we'd assume AES-GCM is the default; please mention the others are fallbacks and - if possible - note which OpenSSL versions might not support GCM. http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/exprs/string-functions-ir.cc@1832 PS10, Line 1832: // for use in situations where random access to ciphertext is required. I don't think we support random access on a single slot, so this doesn't seem to provide any benefit over GCM. http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/exprs/string-functions-ir.cc@1840 PS10, Line 1840: // flexibility in implementation. I'm not sure why this is a useful feature to add. http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/exprs/string-functions-ir.cc@1869 PS10, Line 1869: if (mode_ == AES_CIPHER_MODE::AES_128_ECB I'd rather first SetCipherMode, then check IsEcbMode and error. That helps contain where we need to keep track of what ECB modes exist. http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/exprs/string-functions-ir.cc@1877 PS10, Line 1877: if ( iv.is_null) { nit: unnecessary space after '('. http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/exprs/string-functions-ir.cc@1947 PS10, Line 1947: if (mode_ != AES_CIPHER_MODE::AES_128_ECB This could use !t.IsEcbMode() http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/exprs/string-functions-ir.cc@1955 PS10, Line 1955: if (mode_ != AES_CIPHER_MODE::AES_128_ECB Use !t.IsEcbMode() http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/util/openssl-util.cc@223 PS10, Line 223: ScopedEVPCipherCtx ctx(padding_enabled); This takes an int as argument. It looks like EVP_CIPHER_CTX_set_padding treats 0 as disabled, 1 as enabled. Can you make the conversion explicit (either here or in ScopedEVPCipherCtx)? http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/util/openssl-util.cc@245 PS10, Line 245: success = encrypt ? EVP_EncryptInit_ex(ctx.ctx, nullptr, nullptr, key_, iv_): Why do we do init twice? http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/util/openssl-util.cc@287 PS10, Line 287: // supported, uncommenting it will result in an error. I don't think we can accept this patch without handling errors here. http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/util/openssl-util.cc@310 PS10, Line 310: if (mode_ == AES_CIPHER_MODE::AES_256_CTR) return EVP_aes_256_ctr(); This would be better as a switch statement. http://gerrit.cloudera.org:8080/#/c/20447/10/be/src/util/openssl-util.cc@402 PS10, Line 402: if (memcmp("AES_256_GCM", str, len) == 0 ) { nit: in this case memcmp and strncmp would give the same result, and strncmp makes more sense to me. http://gerrit.cloudera.org:8080/#/c/20447/10/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: http://gerrit.cloudera.org:8080/#/c/20447/10/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3387 PS10, Line 3387: STRING nit: I'd throw in an explicit round-trip test: aes_decrypt(aes_encrypt(...), ...) I know it's essentially covered by the case above, but helps avoid accidental partial test changes. -- 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: 10 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Comment-Date: Wed, 10 Jul 2024 22:10:28 +0000 Gerrit-HasComments: Yes
