Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/9032 )
Change subject: IMPALA-6219: Use AES-GCM for spill-to-disk encryption ...................................................................... Patch Set 9: (5 comments) http://gerrit.cloudera.org:8080/#/c/9032/9/be/src/util/openssl-util-test.cc File be/src/util/openssl-util-test.cc: http://gerrit.cloudera.org:8080/#/c/9032/9/be/src/util/openssl-util-test.cc@149 PS9, Line 149: if (key.IsGcmMode()) Shouldn't this be a DCHECK? http://gerrit.cloudera.org:8080/#/c/9032/9/be/src/util/openssl-util.h File be/src/util/openssl-util.h: http://gerrit.cloudera.org:8080/#/c/9032/9/be/src/util/openssl-util.h@51 PS9, Line 51: /// OpenSSL 1.0.1 : #define OPENSSL_VERSION_1_0_1 0x1000100fL : : /// OpenSSL 1.0.1d : #define OPENSSL_VERSION_1_0_1D 0x1000104fL : : /// If not defined at compile time, define them manually : /// see: openssl/evp.h : #ifndef EVP_CIPH_GCM_MODE : #define EVP_CTRL_GCM_SET_IVLEN 0x9 : #define EVP_CTRL_GCM_GET_TAG 0x10 : #define EVP_CTRL_GCM_SET_TAG 0x11 : #endif : : extern "C" { : ATTRIBUTE_WEAK : const EVP_CIPHER* EVP_aes_256_ctr(); : : ATTRIBUTE_WEAK : const EVP_CIPHER* EVP_aes_256_gcm(); : } These can be moved to the .cc file ? http://gerrit.cloudera.org:8080/#/c/9032/9/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/9032/9/be/src/util/openssl-util.cc@188 PS9, Line 188: nit: whitespace http://gerrit.cloudera.org:8080/#/c/9032/9/be/src/util/openssl-util.cc@189 PS9, Line 189: /// not ideal but is OK to use SSLeay() for GCM mode here. Add "... but is OK to use SSLeay() for GCM mode here since in the worst case, we will be using AES_256_CTR in a system that supports AES_256_GCM". http://gerrit.cloudera.org:8080/#/c/9032/9/be/src/util/openssl-util.cc@183 PS9, Line 183: /// It becomes a bit tricky for GCM mode, because GCM mode is enabled since : /// OpenSSL 1.0.1, but the tag validation only works since 1.0.1d. We have to : /// make sure that OpenSSL version >= 1.0.1d for GCM. So we need SSLeay(). Note : /// that SSLeay() may return the compiling version on certain platforms if it : /// was built against an older version(see: IMPALA-6418). In this case, it will : /// return false, and EncryptionKey will try to fall back to CTR mode, so it is : /// not ideal but is OK to use SSLeay() for GCM mode here. I would move this comment inside the case AES_256_GCM: block. -- To view, visit http://gerrit.cloudera.org:8080/9032 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1ea87b82a8897ee8bfa187715ac1c52883790d24 Gerrit-Change-Number: 9032 Gerrit-PatchSet: 9 Gerrit-Owner: Xianda Ke <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Xianda Ke <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 25 Jan 2018 18:00:00 +0000 Gerrit-HasComments: Yes
