Repository: impala Updated Branches: refs/heads/2.x a84053ddf -> 2c893f46b
IMPALA-6892: CheckHashAndDecrypt() includes file and host The error text with AES-GCM enabled looks like: Error reading 44 bytes from scratch file '/tmp/impala-scratch/0:0_d43635d0-8f55-485e-8899-907af289ac86' on backend tarmstrong-box:22000 at offset 0: verification of read data failed. OpenSSL error in EVP_DecryptFinal: 139634997483216:error:0607C083:digital envelope routines:EVP_CIPHER_CTX_ctrl:no cipher set:evp_enc.c:610: 139634997483216:error:0607C083:digital envelope routines:EVP_CIPHER_CTX_ctrl:no cipher set:evp_enc.c:610: 139634997483216:error:0607C083:digital envelope routines:EVP_CIPHER_CTX_ctrl:no cipher set:evp_enc.c:610: 139634997483216:error:0607C083:digital envelope routines:EVP_CIPHER_CTX_ctrl:no cipher set:evp_enc.c:610: Testing: Added a backend test to exercise the code path and verify the error code. Change-Id: I0652d6cdfbb4e543dd0ca46b7cc65edc4e41a2d8 Reviewed-on: http://gerrit.cloudera.org:8080/10204 Reviewed-by: Tim Armstrong <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> Reviewed-on: http://gerrit.cloudera.org:8080/10232 Project: http://git-wip-us.apache.org/repos/asf/impala/repo Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/2c893f46 Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/2c893f46 Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/2c893f46 Branch: refs/heads/2.x Commit: 2c893f46bd630b42b96ade7694cb166515f20ef3 Parents: a84053d Author: Tim Armstrong <[email protected]> Authored: Wed Apr 25 11:32:30 2018 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Fri Apr 27 19:47:38 2018 +0000 ---------------------------------------------------------------------- be/src/exec/exec-node.cc | 2 -- be/src/runtime/tmp-file-mgr-test.cc | 53 ++++++++++++++++++++++++++++++ be/src/runtime/tmp-file-mgr.cc | 15 +++++++-- common/thrift/generate_error_codes.py | 4 +++ 4 files changed, 70 insertions(+), 4 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/impala/blob/2c893f46/be/src/exec/exec-node.cc ---------------------------------------------------------------------- diff --git a/be/src/exec/exec-node.cc b/be/src/exec/exec-node.cc index c94f38c..8553459 100644 --- a/be/src/exec/exec-node.cc +++ b/be/src/exec/exec-node.cc @@ -68,8 +68,6 @@ using strings::Substitute; -DECLARE_int32(be_port); -DECLARE_string(hostname); DEFINE_bool_hidden(enable_partitioned_hash_join, true, "Deprecated - has no effect"); DEFINE_bool_hidden(enable_partitioned_aggregation, true, "Deprecated - has no effect"); http://git-wip-us.apache.org/repos/asf/impala/blob/2c893f46/be/src/runtime/tmp-file-mgr-test.cc ---------------------------------------------------------------------- diff --git a/be/src/runtime/tmp-file-mgr-test.cc b/be/src/runtime/tmp-file-mgr-test.cc index a61f844..318fdbe 100644 --- a/be/src/runtime/tmp-file-mgr-test.cc +++ b/be/src/runtime/tmp-file-mgr-test.cc @@ -31,6 +31,7 @@ #include "service/fe-support.h" #include "testutil/gtest-util.h" #include "util/condition-variable.h" +#include "util/cpu-info.h" #include "util/filesystem-util.h" #include "util/metrics.h" @@ -169,6 +170,9 @@ class TmpFileMgrTest : public ::testing::Test { while (cb_counter_ < val) cb_cv_.Wait(lock); } + /// Implementation of TestBlockVerification(), which is run with different environments. + void TestBlockVerification(); + ObjectPool obj_pool_; scoped_ptr<MetricGroup> metrics_; // Owned by 'obj_pool_'. @@ -507,6 +511,55 @@ TEST_F(TmpFileMgrTest, TestEncryptionDuringCancellation) { file_group.Close(); test_env_->TearDownQueries(); } + +TEST_F(TmpFileMgrTest, TestBlockVerification) { + TestBlockVerification(); +} + +TEST_F(TmpFileMgrTest, TestBlockVerificationGcmDisabled) { + // Disable AES-GCM to test that errors from non-AES-GCM verification are also correct. + CpuInfo::TempDisable t1(CpuInfo::PCLMULQDQ); + TestBlockVerification(); +} + +void TmpFileMgrTest::TestBlockVerification() { + FLAGS_disk_spill_encryption = true; + TUniqueId id; + TmpFileMgr::FileGroup file_group(test_env_->tmp_file_mgr(), io_mgr(), profile_, id); + string data = "the quick brown fox jumped over the lazy dog"; + MemRange data_mem_range(reinterpret_cast<uint8_t*>(&data[0]), data.size()); + + // Start a write in flight, which should encrypt the data and write it to disk. + unique_ptr<TmpFileMgr::WriteHandle> handle; + WriteRange::WriteDoneCallback callback = + bind(mem_fn(&TmpFileMgrTest::SignalCallback), this, _1); + ASSERT_OK(file_group.Write(data_mem_range, callback, &handle)); + string file_path = handle->TmpFilePath(); + + WaitForWrite(handle.get()); + WaitForCallbacks(1); + + // Modify the data in the scratch file and check that a read error occurs. + FILE* file = fopen(file_path.c_str(), "rb+"); + fputc('?', file); + fclose(file); + vector<uint8_t> tmp; + tmp.resize(data.size()); + Status read_status = file_group.Read(handle.get(), MemRange(tmp.data(), tmp.size())); + LOG(INFO) << read_status.GetDetail(); + EXPECT_EQ(TErrorCode::SCRATCH_READ_VERIFY_FAILED, read_status.code()) + << read_status.GetDetail(); + + // Modify the data in memory. Restoring the data should fail. + data[0] = '?'; + Status restore_status = file_group.RestoreData(move(handle), data_mem_range); + LOG(INFO) << restore_status.GetDetail(); + EXPECT_EQ(TErrorCode::SCRATCH_READ_VERIFY_FAILED, restore_status.code()) + << restore_status.GetDetail(); + + file_group.Close(); + test_env_->TearDownQueries(); +} } int main(int argc, char** argv) { http://git-wip-us.apache.org/repos/asf/impala/blob/2c893f46/be/src/runtime/tmp-file-mgr.cc ---------------------------------------------------------------------- diff --git a/be/src/runtime/tmp-file-mgr.cc b/be/src/runtime/tmp-file-mgr.cc index 5adfcab..b44a5c4 100644 --- a/be/src/runtime/tmp-file-mgr.cc +++ b/be/src/runtime/tmp-file-mgr.cc @@ -608,15 +608,26 @@ Status TmpFileMgr::WriteHandle::EncryptAndHash(MemRange buffer) { Status TmpFileMgr::WriteHandle::CheckHashAndDecrypt(MemRange buffer) { DCHECK(FLAGS_disk_spill_encryption); + DCHECK(write_range_ != nullptr); SCOPED_TIMER(encryption_timer_); // GCM mode will verify the integrity by itself if (!key_.IsGcmMode()) { if (!hash_.Verify(buffer.data(), buffer.len())) { - return Status("Block verification failure"); + return Status(TErrorCode::SCRATCH_READ_VERIFY_FAILED, buffer.len(), + write_range_->file(), GetBackendString(), write_range_->offset()); } } - return key_.Decrypt(buffer.data(), buffer.len(), buffer.data()); + Status decrypt_status = key_.Decrypt(buffer.data(), buffer.len(), buffer.data()); + if (!decrypt_status.ok()) { + // Treat decryption failing as a verification failure, but include extra info from + // the decryption status. + Status result_status(TErrorCode::SCRATCH_READ_VERIFY_FAILED, buffer.len(), + write_range_->file(), GetBackendString(), write_range_->offset()); + result_status.MergeStatus(decrypt_status); + return result_status; + } + return Status::OK(); } string TmpFileMgr::WriteHandle::DebugString() { http://git-wip-us.apache.org/repos/asf/impala/blob/2c893f46/common/thrift/generate_error_codes.py ---------------------------------------------------------------------- diff --git a/common/thrift/generate_error_codes.py b/common/thrift/generate_error_codes.py index fc47b2a..7775ef6 100755 --- a/common/thrift/generate_error_codes.py +++ b/common/thrift/generate_error_codes.py @@ -359,6 +359,10 @@ error_codes = ( ("LIB_VERSION_MISMATCH", 117, "The library $0 last modified time $1 does not match the expected last " "modified time $2. Run 'refresh functions <db name>'."), + + ("SCRATCH_READ_VERIFY_FAILED", 118, "Error reading $0 bytes from scratch file '$1' " + "on backend $2 at offset $3: verification of read data failed."), + ) import sys
