GH-51349: [C++][Parquet] Add flag to control pass-through of KMS URLs from key material

[adamreeve]

Rationale for this change

Addresses #51349

What changes are included in this PR?

• Adds a new read_kms_url member to DecryptionConfiguration and a parameter with the same name to rotate_master_keys. Both are false by default.
• Adds these to the PyArrow bindings.

Are these changes tested?

Yes, this includes new unit tests.

Are there any user-facing changes?

Yes, this adds a new user-facing option. Users that relied on this behaviour previously will now need to opt-in and enable the flag.

• GitHub Issue: [C++][Parquet] KMS URL pass-through should be configurable #51349

[github-actions]

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

[github-actions]

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

[Copilot]

🟡 Changes recommended

Critical caching and public API compatibility issues, plus a test expectation mismatch, remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds opt-in read_kms_url support for Parquet KMS URL pass-through during decryption and master-key rotation, exposed through C++ and PyArrow.

Changes:

• Adds the configuration and rotation parameters, defaulting to false.
• Updates C++/PyArrow bindings and documentation.
• Adds KMS request mocks and C++/Python tests.

File summaries

File	Summary
python/pyarrow/tests/parquet/test_encryption.py	Adds Python behavior coverage.
python/pyarrow/src/arrow/python/parquet_encryption.h	Updates binding declarations.
python/pyarrow/src/arrow/python/parquet_encryption.cc	Implements binding changes.
python/pyarrow/includes/libparquet_encryption.pxd	Updates Cython declarations.
python/pyarrow/_parquet_encryption.pyx	Exposes the Python API option.
docs/source/python/parquet/parquet_encryption.rst	Documents the new option.
cpp/src/parquet/encryption/test_in_memory_kms.h	Records KMS client creation requests.
cpp/src/parquet/encryption/key_toolkit.h	Adds the rotation parameter. Critical (1 vote): preserve compatibility with the previous signature using a default or overload.
cpp/src/parquet/encryption/key_toolkit.cc	Implements rotation propagation.
cpp/src/parquet/encryption/key_management_test.cc	Adds C++ coverage. Moderate (1 vote): align expectations with normalized default configuration values.
cpp/src/parquet/encryption/file_key_unwrapper.h	Adds URL-reading state.
cpp/src/parquet/encryption/file_key_unwrapper.cc	Selects KMS URLs. Critical (3 votes): prevent cached clients from bypassing a later read_kms_url=false setting.
cpp/src/parquet/encryption/crypto_factory.h	Adds public configuration and API parameters.
cpp/src/parquet/encryption/crypto_factory.cc	Propagates the configuration through the factory.

Review details

Suppressed comments (1)

cpp/src/parquet/encryption/key_management_test.cc:571

• FileKeyWrapper normalizes an empty KmsConnectionConfig with SetDefaultIfEmpty() before creating the writer client, so the first CreationRequests() entry is (DEFAULT, DEFAULT), not empty strings. The analogous expectations in python/pyarrow/tests/parquet/test_encryption.py have the same mismatch, so update both tests or assert the normalized values.

  EXPECT_EQ(requests[0].kms_instance_id, "");
  EXPECT_EQ(requests[0].kms_instance_url, "");

• Files reviewed: 14/14 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Critical test assertions fail, and KMS client caching can ignore embedded URLs during decryption or rotation.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

cpp/src/parquet/encryption/file_key_unwrapper.cc:153

• When read_kms_url_ is enabled, this URL is selected from key material, but KeyToolkit::GetKmsClient caches clients by access token and kms_instance_id only. A reused CryptoFactory can therefore return a client created earlier with the default (or another file's) URL, so reading the same file first with read_kms_url=False can make a later True read ignore the embedded URL. Include the URL in the cache key or otherwise bypass/invalidate the cache for URL-sensitive clients, and cover reuse across reads.

    if (read_kms_url_) {
      kms_instance_url = key_material.kms_instance_url();

cpp/src/parquet/encryption/key_toolkit.cc:70

• During rotation, FileKeyWrapper has already created and cached a client before this flag is used. If the rotation config supplies the same instance ID but omits the URL, the cache (keyed by token and instance ID, not URL) returns the wrapper's DEFAULT-URL client, so read_kms_url=True silently ignores the URL in the existing key material. The cache identity or the wrapper/unwrapper client setup must be adjusted so the material URL is actually honored.

  if (read_kms_url) {
    file_key_unwrapper.EnableReadingKmsUrl();
  }

• Files reviewed: 14/14 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

ID-only rotation configurations can reuse a client created with an empty URL, preventing read_kms_url=True from taking effect.

Review details

Suppressed comments (1)

cpp/src/parquet/encryption/key_toolkit.cc:70

• When the rotation config supplies a KMS instance ID but omits only the URL, the wrapper is constructed before the existing footer key is unwrapped and caches a client for that ID using the empty URL. The unwrapper then fills kms_instance_url from the file, but the shared cache returns the already-created client, so read_kms_url=True has no effect for this valid configuration. Add a regression test for an ID-only rotation config and avoid reusing the pre-existing client when the URL is resolved from key material.

  if (read_kms_url) {
    file_key_unwrapper.EnableReadingKmsUrl();
  }

• Files reviewed: 14/14 changed files
• Comments generated: 0 new
• Review effort level: Lite

[EnricoMi]

LGTM! Straight-forward, mainly wiring through the flag, thoroughly tested.

[wgtmac]

I haven't checked the test change and python code. The fix itself looks good to me.