[autoCAT6]

Rationale for this change

Closes #51669. CTR files with plaintext footers incorrectly declare GCM, causing encrypted-column reads to fail.

What changes are included in this PR?

• Serialize the configured file encryption algorithm directly.
• Select AES-GCM explicitly for plaintext footer signing and verification.
• Add generated GCM/CTR, footer and AAD cases, including an on-disk GCM tag check independent of reader selection.

Original report by dotnwat in #51669. The follow-up incorporates pitrou's review and adamreeve's clarification.

Are these changes tested?

• Encryption: 45 passed; metadata: 50 passed; writer: 296 passed, with 2 existing disabled tests.
• Released parquet-java 1.18.1 interoperability: 64 read outcomes and 8 raw metadata inspections matched their controls.
• clang-format 18 and git diff --check pass.

Are there any user-facing changes?

New CTR/plaintext-footer files declare the correct algorithm. No public API changes.

This PR contains a "Critical Fix": it corrects invalid encryption algorithm metadata emitted by the writer.

Was AI used for this PR?

AI was used to generate the implementation, regression tests, and PR description.

PR code and description written by:

• Human
• AI

Reviewed before submission by:

• Human

• AI

• Not reviewed

• GitHub Issue: [C++][Parquet] Plaintext-footer files written with AES_GCM_CTR_V1 record AES_GCM_V1 as the encryption algorithm and cannot be read #51669

[github-actions]

⚠️ GitHub issue #51669 has been automatically assigned in GitHub to PR creator.

[github-actions]

⚠️ GitHub issue #51669 has no components, please add labels for components.

[pitrou]

@autoCAT6 This is AI-generated, right?

Have you read our PR template at least?

[github-actions]

⚠️ GitHub issue #51669 has been automatically assigned in GitHub to PR creator.

[github-actions]

⚠️ GitHub issue #51669 has no components, please add labels for components.

[autoCAT6]

Thanks for pointing this out, @pitrou. Yes, AI was used for the implementation, regression tests, and PR description. I have updated the description to include the AI disclosure and completed the template checklist.

[pitrou]

Ok, can you please make the PR description terser so that it only keeps the salient points?

[pitrou]

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 #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 --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) {

cc @adamreeve @EnricoMi who are more knowledgeable than I am.

[pitrou]

I opened apache/parquet-testing#129 to add a file under parquet-testing.

[adamreeve]

So a proper fix looks IMHO like this:

Yes, agreed. It looks like this PR is a partial fix, it correctly sets the file metadata's encryption_algorithm, fixing the issue reported in #51669 and allowing round-tripping within this library. But the files are still incompatible with the spec due to using the file algorithm for footer signing rather than always using AES-GCM.

I also agree with dropping any modifications to signing_algorithm.aad, this should be set correctly already here:

arrow/cpp/src/parquet/encryption/encryption.cc

Lines 328 to 332 in 1413260

	algorithm_.aad.aad_file_unique = aad_file_unique_str;
	algorithm_.aad.supply_aad_prefix = supply_aad_prefix;
	if (!aad_prefix_.empty() && store_aad_prefix_in_file_) {
	algorithm_.aad.aad_prefix = aad_prefix_;
	}

[adamreeve]

I was also a bit confused about how signing and verifying the footer with AES-CTR would even work, because AES-CTR doesn't write a tag. It looks like we just assume the last 16 bytes of the encrypted data are the GCM tag when writing:

arrow/cpp/src/parquet/metadata.cc

Lines 885 to 886 in 1413260

	dst->Write(encrypted_data.data() + encrypted_len - encryption::kGcmTagLength,
	encryption::kGcmTagLength));

And then do the same when reading:

arrow/cpp/src/parquet/metadata.cc

Lines 1217 to 1218 in 1413260

	return 0 == memcmp(encrypted_buffer->data() + encrypted_len - encryption::kGcmTagLength,
	tag.data(), encryption::kGcmTagLength);

So the "tag" is actually just the last bytes of the AES-CTR encrypted data and the round trip succeeds

[github-actions]

⚠️ GitHub issue #51669 has been automatically assigned in GitHub to PR creator.

[github-actions]

⚠️ GitHub issue #51669 has no components, please add labels for components.

[autoCAT6]

Thanks, updated. I shortened the description and simplified the algorithm copy. Signing and both verification paths now explicitly use GCM.

I checked the old code too: metadata=true already selects GCM, and SignedFooterEncrypt rejects CTR. The new footer tag test also passes on the previous head. So the behavior fix is the metadata value; the GCM calls just make the signing rule clear.

C++ tests and the Java 1.18.1 checks pass.

[adamreeve]

I checked the old code too: metadata=true already selects GCM, and SignedFooterEncrypt rejects CTR. The new footer tag test also passes on the previous head. So the behavior fix is the metadata value; the GCM calls just make the signing rule clear.

It took me a while to parse this AI speak, but I think this is saying that changing file_decryptor->algorithm() to ParquetCipher::AES_GCM_V1 isn't actually necessary. We end up doing AES-GCM by passing metadata=True, which I missed:

arrow/cpp/src/parquet/encryption/encryption_internal.cc

Line 76 in 1413260

if (metadata || (ParquetCipher::AES_GCM_V1 == alg_id)) {

Passing the AES_GCM_V1 algorithm explicitly makes things clearer. But this is more evidence for a tidy up being necessary (#45525).

I'm happy to create a new parquet-testing file for this. I think we should generate it from this PR branch, then verify it can be read in arrow-go and parquet-java (which are the only other implementations that support AES-CTR I believe).

[autoCAT6]

Thanks, that sounds good. I checked files from the current PR head with arrow-go v18.8.0 too. The CTR file with a plaintext footer reads correctly with signature verification enabled. I'll rerun both readers against the new parquet-testing fixture once it's ready.

[adamreeve]

Can you please check the build failures @autoCAT6:

FAILED: src/parquet/CMakeFiles/parquet-encryption-test.dir/encryption/write_configurations_test.cc.o 
/usr/bin/ccache /usr/bin/clang++-18 -DADDRESS_SANITIZER -DARROW_HAVE_RUNTIME_AVX2 -DARROW_HAVE_RUNTIME_AVX512 -DARROW_HAVE_RUNTIME_BMI2 -DARROW_HAVE_RUNTIME_SSE4_2 -DARROW_HAVE_SSE4_2 -DARROW_UBSAN -DPARQUET_THRIFT_VERSION_MAJOR=0 -DPARQUET_THRIFT_VERSION_MINOR=19 -DSIMDJSON_DEVELOPMENT_CHECKS -DSIMDJSON_EXCEPTIONS=0 -DSIMDJSON_THREADS_ENABLED=1 -I/build/cpp/src -I/arrow/cpp/src -isystem /build/cpp/_deps/xsimd-src/include -isystem /arrow/cpp/thirdparty/flatbuffers/include -isystem /build/cpp/_deps/simdjson-src/include -Qunused-arguments -fcolor-diagnostics  -Wall -Wextra -Wdocumentation -DARROW_WARN_DOCUMENTATION -Wshorten-64-to-32 -Wno-missing-braces -Wno-unused-parameter -Wno-constant-logical-operand -Wno-return-stack-address -Wdate-time -Wno-unknown-warning-option -Wno-pass-failed -msse4.2  -fsanitize=address -DADDRESS_SANITIZER -fsanitize=undefined -fno-sanitize=alignment,vptr,function,float-divide-by-zero -fno-sanitize-recover=all -fsanitize-coverage=pc-table,inline-8bit-counters,edge,no-prune,trace-cmp,trace-div,trace-gep -fsanitize-blacklist=/arrow/cpp/build-support/sanitizer-disallowed-entries.txt -g -Werror -O0 -ggdb -g1 -std=c++20 -fPIE   -fsanitize-coverage=pc-table,inline-8bit-counters,edge,no-prune,trace-cmp,trace-div,trace-gep -DGTEST_HAS_PTHREAD=1 -MD -MT src/parquet/CMakeFiles/parquet-encryption-test.dir/encryption/write_configurations_test.cc.o -MF src/parquet/CMakeFiles/parquet-encryption-test.dir/encryption/write_configurations_test.cc.o.d -o src/parquet/CMakeFiles/parquet-encryption-test.dir/encryption/write_configurations_test.cc.o -c /arrow/cpp/src/parquet/encryption/write_configurations_test.cc
/arrow/cpp/src/parquet/encryption/write_configurations_test.cc:292:62: error: implicit conversion loses integer precision: 'std::size_t' (aka 'unsigned long') to 'int32_t' (aka 'int') [-Werror,-Wshorten-64-to-32]
  291 |     auto encryptor = AesEncryptor::Make(ParquetCipher::AES_GCM_V1,
      |                      ~~~~~~~~~~~~
  292 |                                         kFooterEncryptionKey.size(), /*metadata=*/true,
      |                                         ~~~~~~~~~~~~~~~~~~~~~^~~~~~

FAILED: [code=4294967295] debug/parquet-encryption-test.exe 
C:\Windows\system32\cmd.exe /C "cd . && "C:\Program Files\Git\usr\bin\cmake.exe" -E vs_link_exe --msvc-ver=1944 --intdir=src\parquet\CMakeFiles\parquet-encryption-test.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100261~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100261~1.0\x64\mt.exe --manifests  -- C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1444~1.352\bin\Hostx64\x64\link.exe /nologo @CMakeFiles\parquet-encryption-test.rsp  /out:debug\parquet-encryption-test.exe /implib:debug\parquet-encryption-test.lib /pdb:debug\parquet-encryption-test.pdb /version:0.0 /machine:x64  /NODEFAULTLIB:LIBCMT /debug /INCREMENTAL /subsystem:console && cd ."
LINK Pass 1: command "C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1444~1.352\bin\Hostx64\x64\link.exe /nologo @CMakeFiles\parquet-encryption-test.rsp /out:debug\parquet-encryption-test.exe /implib:debug\parquet-encryption-test.lib /pdb:debug\parquet-encryption-test.pdb /version:0.0 /machine:x64 /NODEFAULTLIB:LIBCMT /debug /INCREMENTAL /subsystem:console /MANIFEST /MANIFESTFILE:src\parquet\CMakeFiles\parquet-encryption-test.dir/intermediate.manifest src\parquet\CMakeFiles\parquet-encryption-test.dir/manifest.res" failed (exit code 1120) with the following output:
unity_0_cxx.cxx.obj : error LNK2019: unresolved external symbol "class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > __cdecl parquet::encryption::CreateFooterAad(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &)" (?CreateFooterAad@encryption@parquet@@YA?AV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@AEBV34@@Z) referenced in function "protected: void __cdecl parquet::encryption::test::TestGeneratedEncryptionConfiguration::CheckPlaintextFooterSignature(void)" (?CheckPlaintextFooterSignature@TestGeneratedEncryptionConfiguration@test@encryption@parquet@@IEAAXXZ)

debug\parquet-encryption-test.exe : fatal error LNK1120: 1 unresolved externals

Because you're calling CreateFooterAad from a test it will need to be annotated with PARQUET_EXPORT. Or maybe it's OK to use an arbitrary AAD in the test to avoid needing this?

[autoCAT6]

Thanks, both issues are fixed. The key length now has an explicit int32_t conversion, and the test constructs the same footer AAD locally from the file AAD and footer module byte, so CreateFooterAad does not need to be exported.

The test translation unit passes -Werror=shorten-64-to-32, its object no longer references CreateFooterAad, and all 45 encryption tests pass against the shared libraries on macOS. The on-disk GCM tag check is unchanged. Windows still needs CI confirmation.