pitrou commented on PR #54884: URL: https://github.com/apache/arrow/pull/54884#issuecomment-6342493385
I'm not an expert on this and the Parquet encryption code is convoluted, but I think this PR is wrong (and so was the previous attempt in https://github.com/apache/arrow/pull/51693/). Basically: * the `encryption_algorithm` recorded in the plaintext footer is the algorithm used for encrypting file metadata and data (it can be either GCM or CTR) * the *signing algorithm* for the plaintext footer is always GCM regardless of the `encryption_algorithm` So a proper fix looks IMHO like this: ```diff diff --git a/cpp/src/parquet/encryption/internal_file_encryptor.cc b/cpp/src/parquet/encryption/internal_file_encryptor.cc index 23dbfe34d3..4ab221fd55 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<Encryptor> 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/metadata.cc b/cpp/src/parquet/metadata.cc index 61a111fc0c..fa0c80442a 100644 --- a/cpp/src/parquet/metadata.cc +++ b/cpp/src/parquet/metadata.cc @@ -1202,8 +1202,10 @@ bool FileMetaData::VerifySignature(std::span<const uint8_t> 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<int>(key.size()), /*metadata=*/true, + ParquetCipher::AES_GCM_V1, static_cast<int>(key.size()), /*metadata=*/true, /*write_length=*/false); std::shared_ptr<Buffer> encrypted_buffer = @@ -2136,16 +2138,8 @@ class FileMetaDataBuilder::FileMetaDataBuilderImpl { // if plaintext footer, set footer signing algorithm 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; - } - signing_algorithm.algorithm = ParquetCipher::AES_GCM_V1; - - metadata_->__set_encryption_algorithm(ToThrift(signing_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) { ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
