Zoltan Borok-Nagy 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 4: (12 comments) Thanks for working on this, the change seems pretty cool IMO. Had some comments mostly about style. 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 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@47 PS4, Line 47: fill nit: Fill, capital F http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/openssl-util-test.cc@49 PS4, Line 49: DCHECK_EQ(true, len >= 0); DCHECK_GE(len, 0); http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/openssl-util-test.cc@118 PS4, Line 118: /// 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; : vector<uint8_t> original(buffer_size); : vector<uint8_t> scratch(buffer_size); // Scratch buffer for in-place encryption. : GenerateRandomData(original.data(), buffer_size); : : // Check all the modes : AES_CIPHER_MODE modes[] = {AES_256_GCM, AES_256_CTR, AES_256_CFB}; : for (auto m : modes) { : memcpy(scratch.data(), original.data(), buffer_size); : EncryptionKey key; : key.InitializeRandom(); : key.SetCipherMode(m); : ASSERT_OK(key.Encrypt(scratch.data(), buffer_size, scratch.data())); : // Check that encryption did something : ASSERT_NE(0, memcmp(original.data(), scratch.data(), buffer_size)); : ASSERT_OK(key.Decrypt(scratch.data(), buffer_size, scratch.data())); : // Check that we get the original data back. : ASSERT_EQ(0, memcmp(original.data(), scratch.data(), 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_); : vector<uint8_t> original(buffer_size); : vector<uint8_t> scratch(buffer_size); : GenerateRandomBytes(original.data(), buffer_size); : : // Check all the modes : AES_CIPHER_MODE modes[] = {AES_256_GCM, AES_256_CTR, AES_256_CFB}; : for (auto m : modes) { : EncryptionKey key; : memcpy(scratch.data(), original.data(), buffer_size); : : key.InitializeRandom(); : key.SetCipherMode(m); : : ASSERT_OK(key.Encrypt(scratch.data(), buffer_size, scratch.data())); : // Check that encryption did something : ASSERT_NE(0, memcmp(original.data(), scratch.data(), buffer_size)); : ASSERT_OK(key.Decrypt(scratch.data(), buffer_size, scratch.data())); : // Check that we get the original data back. : ASSERT_EQ(0, memcmp(original.data(), scratch.data(), buffer_size)); : } : } These two tests are pretty similar. You could refactor the identical parts to a simple function, then call this function with different buffer sizes in a TEST_F. http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/openssl-util-test.cc@176 PS4, Line 176: if nit: If http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/openssl-util-test.cc@176 PS4, Line 176: runtim, runtime http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/openssl-util.h File be/src/util/openssl-util.h: http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/openssl-util.h@56 PS4, Line 56: enum AES_CIPHER_MODE { AES_256_CTR, AES_256_CFB, AES_256_GCM }; I think the multi-line version is more readable and more conformant to the Impala-style. http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/openssl-util.h@86 PS4, Line 86: was support since is supported since http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/openssl-util.h@145 PS4, Line 145: return nit: Return an http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/openssl-util.h@154 PS4, Line 154: tag nit: Tag 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 this comment belongs there as well. http://gerrit.cloudera.org:8080/#/c/9032/4/be/src/util/openssl-util.cc@173 PS4, Line 173: // if not supported, fall back to CFB : if (mode_ == AES_256_GCM) { : if (!CpuInfo::IsSupported(CpuInfo::PCLMULQDQ) || !SSLeay() >= OPENSSL_VERSION_1_0_1D : || !EVP_aes_256_gcm) { : LOG(WARNING) << "GCM mode is not supported, fall back to CFB."; : mode_ = AES_256_CFB; : } : } else if (mode_ == AES_256_CTR) { : if (!SSLeay() >= OPENSSL_VERSION_1_0_1 || !EVP_aes_256_ctr) { : LOG(WARNING) << "CTR mode is not supported, fall back to CFB."; : mode_ = AES_256_CFB; : } : } This part of the code is very similar to the constructor. Might worth refactoring it, or at least parts of it to private helper functions, e.g. IsGcmSupported(), IsCtrSupported(), or, GetSupportedMode(). However, the "falling chain" is different here. In the constructor it's GCM -> CTR -> CFB. Here it is GCM -> CFB, and CTR -> CFB. I don't think it is a problem until it is only used for tests, but if in the future we'll provide it as a configuration option for the users, I think it would be nice to have the same behavior here. Or, maybe in that case returning a status error would be even more preferable instead of logging and silently choosing a different cipher mode. -- 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: 4 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: Fri, 19 Jan 2018 15:51:50 +0000 Gerrit-HasComments: Yes
