Xianda Ke 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 6: (4 comments) > Patch Set 6: Published edit on patch set 5. Thank Zoltan for your comments. The duplicated logic are removed. I prefer the "falling back" option. if we provide conf option to the user. If user want to use GCM mode but still there are some 'old' nodes do NOT support pclmulqdq instruction in the cluster. The "falling back" option doesn't block the user. Zoltan, any comments? http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/cpu-info.h File be/src/util/cpu-info.h: http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/cpu-info.h@37 PS4, Line 37: = (1 << 1); : static const int64_t SSE4_1 = (1 << 2); : static const int64_t SSE4_2 = (1 << 3); : static const int64_t POPCNT = (1 << 4); : static const int64_t AVX = (1 << 5); : static const int64_t AVX2 = (1 << 6); : static const int64_t PCLMULQDQ = > please align the assignment operators fixed http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/openssl-util-test.cc File be/src/util/openssl-util-test.cc: http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/openssl-util-test.cc@118 PS4, Line 118: : /// Test that encryption and decryption work in-place. : TEST_F(OpenSSLUtilTest, EncryptInPlace) { : const int buffer_size = 1024 * 1024; : TestEncryptionDecryption(buffer_size); : } : : /// Test that encryption works with buffer lengths that don't fit in a 32-bit integer. : TEST_F(OpenSSLUtilTest, EncryptInPlaceHugeBuffer) { : const int64_t buffer_size = 3 * 1024L * 1024L * 1024L; : TestEncryptionDecryption(buffer_size); : } : : /// Test that encryption works with arbitrary-length buffer : TEST_F(OpenSSLUtilTest, EncryptArbitraryLength) { : std::uniform_int_distribution<uint64_t> dis(0, 1024 * 1024); : const int buffer_size = dis(rng_); : TestEncryptionDecryption(buffer_size); : } : : /// Test integrity in GCM mode : TEST_F(OpenSSLUtilTest, GcmIntegrity) { : const int buffer_size = 1024 * 1024; : vector<uint8_t> buffer(buffer_size); : : EncryptionKey key; : key.InitializeRandom(); : key.SetCipherMode(AES_256_GCM); : : // If GCM mode is supported at runtime : if (key.IsGcmMode()) { : GenerateRandomData(buffer.data(), buffer_size); : ASSERT_OK(key.Encrypt(buffer.data(), buffer_size, buffer.data())); : : // tamper the data : ++buffer[0]; : Status s = key.Decrypt(buffer.data(), buffer_size, buffer.data()); : EXPECT_STR_CONTAINS(s.GetDetail(), "EVP_DecryptFinal"); : } : } : : /// Test basic integrity hash functionality. : TEST_F(OpenSSLUtilTest, IntegrityHash) { : const int buffer_size = 1024 * 1024; : vector<uint8_t> buf1(buffer_size); : vector<uint8_t> buf1_copy(buffer_size); : vector<uint8_t> buf2(buffer_size); : > These two tests are pretty similar. You could refactor the identical parts three functions are refactored http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/openssl-util.cc@162 PS4, Line 162: // use weak symbol to avoid compiling error on OpenSSL 1.0.0 environment > The weak symbol test is now in the constructor of EncryptionKey, I think th refactored http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/openssl-util.cc@173 PS4, Line 173: LOG(WARNING) << "This mode is not supported, fall back to the default mode."; : mode_ = GetSupportedDefaultMode(); : } : } : : bool EncryptionKey::IsModeSupported(AES_CIPHER_MODE m) const { : switch (m) { : case AES_256_GCM: : return (CpuInfo::IsSupported(CpuInfo::PCLMULQDQ) : && SSLeay() >= OPENSSL_VERSION_1_0_1D && EVP_aes_256_gcm); : : case AES_256_CTR: : > This part of the code is very similar to the constructor. good suggestion. Thanks a lot! duplicated logic is refactored. "falling chain" is same now. I prefer the "falling back" option. if we provide conf option to the user. If user want to use GCM mode but still there are some 'old' nodes do NOT support pclmulqdq instruction in the cluster. The "falling back" option doesn't block the user. Zoltan, any comments? -- 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: 6 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: Mon, 22 Jan 2018 03:30:16 +0000 Gerrit-HasComments: Yes
