From eb3b2de3e344fb5b53b122bbbd901c8ffa3b442f Mon Sep 17 00:00:00 2001 From: Yanzhi Xin Date: Tue, 20 Oct 2026 20:53:59 -0700 Subject: [PATCH 1/2] GH-51669: [C++][Parquet] Preserve cipher in plaintext footer metadata --- .../encryption/write_configurations_test.cc | 128 ++++++++++++++++++ cpp/src/parquet/metadata.cc | 14 +- 2 files changed, 133 insertions(+), 9 deletions(-) diff --git a/cpp/src/parquet/encryption/write_configurations_test.cc b/cpp/src/parquet/encryption/write_configurations_test.cc index 0af0278b2604..f1b95cc49c9b 100644 --- a/cpp/src/parquet/encryption/write_configurations_test.cc +++ b/cpp/src/parquet/encryption/write_configurations_test.cc @@ -19,6 +19,9 @@ #include +#include +#include + #include #include "parquet/column_reader.h" @@ -195,6 +198,131 @@ TEST_F(TestEncryptionConfiguration, EncryptTwoColumnsAndFooterUseAES_GCM_CTR) { "tmp_encrypt_columns_and_footer_ctr.parquet.encrypted")); } +enum class AadPrefixMode { None, Stored, External }; + +class TestGeneratedEncryptionConfiguration + : public ::testing::TestWithParam< + std::tuple> { + public: + void SetUp() override { + PARQUET_ASSIGN_OR_THROW(temp_dir_, temp_data_dir()); + file_path_ = temp_dir_->path().ToString() + "algorithm_metadata.parquet.encrypted"; + + std::map> columns; + columns[kDoubleFieldName] = + ColumnEncryptionProperties::WithColumnKey(kColumnEncryptionKey1, "kc1"); + columns[kFloatFieldName] = + ColumnEncryptionProperties::WithColumnKey(kColumnEncryptionKey2, "kc2"); + columns[kBooleanFieldName] = ColumnEncryptionProperties::Unencrypted(); + + FileEncryptionProperties::Builder builder(kFooterEncryptionKey); + builder.footer_key_metadata("kf") + ->encrypted_columns(std::move(columns)) + ->algorithm(std::get<0>(GetParam())); + if (std::get<1>(GetParam())) { + builder.set_plaintext_footer(); + } + if (std::get<2>(GetParam()) != AadPrefixMode::None) { + builder.aad_prefix(kFileName); + } + if (std::get<2>(GetParam()) == AadPrefixMode::External) { + builder.disable_aad_prefix_storage(); + } + FileEncryptor().EncryptFile(file_path_, builder.build()); + } + + protected: + enum class Keys { All, MissingColumn, WrongFooter }; + enum class AadPrefix { Match, Missing, Wrong }; + + std::shared_ptr MakeDecryptionProperties( + Keys keys = Keys::All, AadPrefix prefix = AadPrefix::Match) { + auto retriever = std::make_shared(); + retriever->PutKey("kf", keys == Keys::WrongFooter ? SecureString(kNewFooterMasterKey) + : kFooterEncryptionKey); + if (keys != Keys::MissingColumn) { + retriever->PutKey("kc1", kColumnEncryptionKey1); + } + retriever->PutKey("kc2", kColumnEncryptionKey2); + + FileDecryptionProperties::Builder builder; + builder.key_retriever(std::move(retriever)); + if (std::get<2>(GetParam()) != AadPrefixMode::None && prefix != AadPrefix::Missing) { + builder.aad_prefix(prefix == AadPrefix::Wrong ? "incorrect-prefix" : kFileName); + } + return builder.build(); + } + + std::unique_ptr OpenReader( + std::shared_ptr decryption_properties) { + auto reader_properties = default_reader_properties(); + reader_properties.file_decryption_properties(std::move(decryption_properties)); + return ParquetFileReader::OpenFile(file_path_, false, reader_properties); + } + + void ReadFirstSecretValue(std::shared_ptr properties) { + auto reader = OpenReader(std::move(properties)); + auto column = reader->RowGroup(0)->Column(5); + auto* double_reader = static_cast(column.get()); + double value; + int64_t values_read; + double_reader->ReadBatch(1, nullptr, nullptr, &value, &values_read); + } + + std::unique_ptr temp_dir_; + std::string file_path_; +}; + +TEST_P(TestGeneratedEncryptionConfiguration, AlgorithmMetadataAndRoundTrip) { + const auto cipher = std::get<0>(GetParam()); + const bool plaintext_footer = std::get<1>(GetParam()); + const auto prefix_mode = std::get<2>(GetParam()); + auto reader = OpenReader(MakeDecryptionProperties()); + const auto metadata = reader->metadata(); + if (plaintext_footer) { + ASSERT_TRUE(metadata->is_encryption_algorithm_set()); + const auto algorithm = metadata->encryption_algorithm(); + ASSERT_EQ(cipher, algorithm.algorithm); + EXPECT_FALSE(algorithm.aad.aad_file_unique.empty()); + EXPECT_EQ(prefix_mode == AadPrefixMode::External, algorithm.aad.supply_aad_prefix); + EXPECT_EQ(prefix_mode == AadPrefixMode::Stored ? kFileName : "", + algorithm.aad.aad_prefix); + } else { + // An encrypted footer declares the algorithm in FileCryptoMetaData instead. + EXPECT_FALSE(metadata->is_encryption_algorithm_set()); + } + reader->Close(); + + FileDecryptor decryptor; + // Verify every physical column and all row groups through sync and async readers. + ASSERT_NO_THROW(decryptor.DecryptFile(file_path_, MakeDecryptionProperties())); + if (prefix_mode == AadPrefixMode::Stored) { + ASSERT_NO_THROW(decryptor.DecryptFile( + file_path_, MakeDecryptionProperties(Keys::All, AadPrefix::Missing))); + } + if (plaintext_footer) { + // This reads only public columns and does not authenticate the keyless footer. + ASSERT_NO_THROW(decryptor.DecryptFile(file_path_, nullptr)); + } + EXPECT_THROW(ReadFirstSecretValue(MakeDecryptionProperties(Keys::MissingColumn)), + std::out_of_range); + EXPECT_THROW(OpenReader(MakeDecryptionProperties(Keys::WrongFooter)), ParquetException); + if (prefix_mode == AadPrefixMode::External) { + EXPECT_THROW(OpenReader(MakeDecryptionProperties(Keys::All, AadPrefix::Missing)), + ParquetException); + EXPECT_THROW(OpenReader(MakeDecryptionProperties(Keys::All, AadPrefix::Wrong)), + ParquetException); + } +} + +INSTANTIATE_TEST_SUITE_P( + GeneratedFiles, TestGeneratedEncryptionConfiguration, + ::testing::Combine(::testing::Values(ParquetCipher::AES_GCM_V1, + ParquetCipher::AES_GCM_CTR_V1), + ::testing::Bool(), + ::testing::Values(AadPrefixMode::None, AadPrefixMode::Stored, + AadPrefixMode::External))); + // Set temp_dir before running the write/read tests. The encrypted files will // be written/read from this directory. void TestEncryptionConfiguration::SetUpTestCase() { diff --git a/cpp/src/parquet/metadata.cc b/cpp/src/parquet/metadata.cc index 61a111fc0c34..9c74dd3fa99b 100644 --- a/cpp/src/parquet/metadata.cc +++ b/cpp/src/parquet/metadata.cc @@ -2133,19 +2133,15 @@ class FileMetaDataBuilder::FileMetaDataBuilderImpl { metadata_->column_orders.resize(schema_->num_columns(), column_order); metadata_->__isset.column_orders = true; - // if plaintext footer, set footer signing algorithm + // Plaintext footers declare the file encryption algorithm and signing key metadata. auto file_encryption_properties = properties_->file_encryption_properties(); if (file_encryption_properties && !file_encryption_properties->encrypted_footer()) { - EncryptionAlgorithm signing_algorithm; - EncryptionAlgorithm algo = file_encryption_properties->algorithm(); - signing_algorithm.aad.aad_file_unique = algo.aad.aad_file_unique; - signing_algorithm.aad.supply_aad_prefix = algo.aad.supply_aad_prefix; - if (!algo.aad.supply_aad_prefix) { - signing_algorithm.aad.aad_prefix = algo.aad.aad_prefix; + EncryptionAlgorithm algorithm = file_encryption_properties->algorithm(); + if (algorithm.aad.supply_aad_prefix) { + algorithm.aad.aad_prefix.clear(); } - signing_algorithm.algorithm = ParquetCipher::AES_GCM_V1; - metadata_->__set_encryption_algorithm(ToThrift(signing_algorithm)); + metadata_->__set_encryption_algorithm(ToThrift(algorithm)); const std::string& footer_signing_key_metadata = file_encryption_properties->footer_key_metadata(); if (footer_signing_key_metadata.size() > 0) { From 5e7a7e6e7c6adfe551e2ad7e9e92cfa19c25c39f Mon Sep 17 00:00:00 2001 From: Yanzhi Xin Date: Thu, 22 Oct 2026 17:16:47 -0700 Subject: [PATCH 2/2] GH-51669: Make plaintext footer GCM signing explicit Simplify file algorithm serialization and verify generated plaintext footer tags directly against AES-GCM. Apply the footer signing and metadata simplification suggested in PR review. --- .../encryption/internal_file_encryptor.cc | 4 +- .../encryption/write_configurations_test.cc | 44 ++++++++++++++++++- cpp/src/parquet/metadata.cc | 20 ++++----- 3 files changed, 56 insertions(+), 12 deletions(-) diff --git a/cpp/src/parquet/encryption/internal_file_encryptor.cc b/cpp/src/parquet/encryption/internal_file_encryptor.cc index 23dbfe34d393..4ab221fd55a7 100644 --- a/cpp/src/parquet/encryption/internal_file_encryptor.cc +++ b/cpp/src/parquet/encryption/internal_file_encryptor.cc @@ -68,7 +68,9 @@ std::shared_ptr InternalFileEncryptor::GetFooterSigningEncryptor() { return footer_signing_encryptor_; } - ParquetCipher::type algorithm = properties_->algorithm().algorithm; + // A plaintext footer is always signed with AES-GCM, regardless of the algorithm + // configured for encryption. + const auto algorithm = ParquetCipher::AES_GCM_V1; std::string footer_aad = encryption::CreateFooterAad(properties_->file_aad()); const SecureString& footer_signing_key = properties_->footer_key(); auto aes_encryptor = GetMetaAesEncryptor(algorithm, footer_signing_key.size()); diff --git a/cpp/src/parquet/encryption/write_configurations_test.cc b/cpp/src/parquet/encryption/write_configurations_test.cc index f1b95cc49c9b..df1be59c71f3 100644 --- a/cpp/src/parquet/encryption/write_configurations_test.cc +++ b/cpp/src/parquet/encryption/write_configurations_test.cc @@ -21,11 +21,16 @@ #include #include +#include #include +#include "arrow/util/endian.h" +#include "arrow/util/ubsan.h" + #include "parquet/column_reader.h" #include "parquet/column_writer.h" +#include "parquet/encryption/encryption_internal.h" #include "parquet/encryption/test_encryption_util.h" #include "parquet/file_reader.h" #include "parquet/file_writer.h" @@ -228,7 +233,9 @@ class TestGeneratedEncryptionConfiguration if (std::get<2>(GetParam()) == AadPrefixMode::External) { builder.disable_aad_prefix_storage(); } - FileEncryptor().EncryptFile(file_path_, builder.build()); + auto properties = builder.build(); + file_aad_ = properties->file_aad(); + FileEncryptor().EncryptFile(file_path_, std::move(properties)); } protected: @@ -260,6 +267,37 @@ class TestGeneratedEncryptionConfiguration return ParquetFileReader::OpenFile(file_path_, false, reader_properties); } + void CheckPlaintextFooterSignature() { + PARQUET_ASSIGN_OR_THROW(auto file, ::arrow::io::ReadableFile::Open(file_path_)); + PARQUET_ASSIGN_OR_THROW(auto file_size, file->GetSize()); + ASSERT_GT(file_size, 8); + PARQUET_ASSIGN_OR_THROW(auto trailer, file->ReadAt(file_size - 8, 8)); + ASSERT_EQ(trailer->size(), 8); + const auto footer_size = ::arrow::bit_util::FromLittleEndian( + ::arrow::util::SafeLoadAs(trailer->data())); + constexpr auto signature_size = kNonceLength + kGcmTagLength; + ASSERT_GT(footer_size, signature_size); + ASSERT_LE(footer_size, file_size - 8); + PARQUET_ASSIGN_OR_THROW(auto footer, + file->ReadAt(file_size - 8 - footer_size, footer_size)); + ASSERT_EQ(footer->size(), footer_size); + const auto footer_span = footer->span_as(); + const auto metadata = footer_span.first(footer_size - signature_size); + const auto nonce = footer_span.subspan(metadata.size(), kNonceLength); + const auto tag = footer_span.last(kGcmTagLength); + const auto aad = CreateFooterAad(file_aad_); + + // Verify the on-disk tag with GCM explicitly, independently of reader selection. + auto encryptor = AesEncryptor::Make(ParquetCipher::AES_GCM_V1, + kFooterEncryptionKey.size(), /*metadata=*/true, + /*write_length=*/false); + std::vector encrypted(encryptor->CiphertextLength(metadata.size())); + encryptor->SignedFooterEncrypt(metadata, kFooterEncryptionKey.as_span(), + str2span(aad), nonce, encrypted); + EXPECT_EQ(std::vector(tag.begin(), tag.end()), + std::vector(encrypted.end() - kGcmTagLength, encrypted.end())); + } + void ReadFirstSecretValue(std::shared_ptr properties) { auto reader = OpenReader(std::move(properties)); auto column = reader->RowGroup(0)->Column(5); @@ -271,12 +309,16 @@ class TestGeneratedEncryptionConfiguration std::unique_ptr temp_dir_; std::string file_path_; + std::string file_aad_; }; TEST_P(TestGeneratedEncryptionConfiguration, AlgorithmMetadataAndRoundTrip) { const auto cipher = std::get<0>(GetParam()); const bool plaintext_footer = std::get<1>(GetParam()); const auto prefix_mode = std::get<2>(GetParam()); + if (plaintext_footer) { + CheckPlaintextFooterSignature(); + } auto reader = OpenReader(MakeDecryptionProperties()); const auto metadata = reader->metadata(); if (plaintext_footer) { diff --git a/cpp/src/parquet/metadata.cc b/cpp/src/parquet/metadata.cc index 9c74dd3fa99b..2bc871a8c865 100644 --- a/cpp/src/parquet/metadata.cc +++ b/cpp/src/parquet/metadata.cc @@ -823,9 +823,11 @@ class FileMetaData::FileMetaDataImpl { const SecureString& key = file_decryptor_->GetFooterKey(); const std::string& aad = encryption::CreateFooterAad(file_decryptor_->file_aad()); - auto aes_encryptor = encryption::AesEncryptor::Make(file_decryptor_->algorithm(), - static_cast(key.size()), - true, false /*write_length*/); + // A plaintext footer is always signed with AES-GCM, regardless of the algorithm + // configured for encryption. + auto aes_encryptor = encryption::AesEncryptor::Make( + ParquetCipher::AES_GCM_V1, static_cast(key.size()), /*metadata=*/true, + /*write_length=*/false); std::shared_ptr encrypted_buffer = AllocateBuffer( file_decryptor_->pool(), aes_encryptor->CiphertextLength( @@ -1202,8 +1204,10 @@ bool FileMetaData::VerifySignature(std::span serialized_metadata, const SecureString& key = file_decryptor->GetFooterKey(); const std::string& aad = encryption::CreateFooterAad(file_decryptor->file_aad()); + // A plaintext footer is always signed with AES-GCM, regardless of the algorithm + // configured for encryption. auto aes_encryptor = encryption::AesEncryptor::Make( - file_decryptor->algorithm(), static_cast(key.size()), /*metadata=*/true, + ParquetCipher::AES_GCM_V1, static_cast(key.size()), /*metadata=*/true, /*write_length=*/false); std::shared_ptr encrypted_buffer = @@ -2136,12 +2140,8 @@ class FileMetaDataBuilder::FileMetaDataBuilderImpl { // Plaintext footers declare the file encryption algorithm and signing key metadata. auto file_encryption_properties = properties_->file_encryption_properties(); if (file_encryption_properties && !file_encryption_properties->encrypted_footer()) { - EncryptionAlgorithm algorithm = file_encryption_properties->algorithm(); - if (algorithm.aad.supply_aad_prefix) { - algorithm.aad.aad_prefix.clear(); - } - - metadata_->__set_encryption_algorithm(ToThrift(algorithm)); + metadata_->__set_encryption_algorithm( + ToThrift(file_encryption_properties->algorithm())); const std::string& footer_signing_key_metadata = file_encryption_properties->footer_key_metadata(); if (footer_signing_key_metadata.size() > 0) {