From 7fc2df53e71b94c9a119275d398de0989c946532 Mon Sep 17 00:00:00 2001 From: Yanzhi Xin Date: Mon, 19 Oct 2026 17:34:56 -0700 Subject: [PATCH] fix: redact Parquet key configuration Debug output --- datafusion/common/src/config.rs | 82 ++++- .../common/tests/encryption_config_debug.rs | 282 ++++++++++++++++++ datafusion/core/tests/parquet/encryption.rs | 115 ++++++- 3 files changed, 474 insertions(+), 5 deletions(-) create mode 100644 datafusion/common/tests/encryption_config_debug.rs diff --git a/datafusion/common/src/config.rs b/datafusion/common/src/config.rs index fc7202a6c5a45..9bbfffc35330f 100644 --- a/datafusion/common/src/config.rs +++ b/datafusion/common/src/config.rs @@ -3362,6 +3362,7 @@ impl ConfigField for TableParquetOptions { macro_rules! config_namespace_with_hashmap { ( + @with_derives [$($derive:path),*] $(#[doc = $struct_d:tt])* $(#[deprecated($($struct_depr:tt)*)])? // Optional struct-level deprecated attribute $vis:vis struct $struct_name:ident { @@ -3375,7 +3376,7 @@ macro_rules! config_namespace_with_hashmap { $(#[doc = $struct_d])* $(#[deprecated($($struct_depr)*)])? // Apply struct deprecation - #[derive(Debug, Clone, PartialEq)] + #[derive($($derive),*)] $vis struct $struct_name{ $( $(#[doc = $d])* @@ -3445,7 +3446,13 @@ macro_rules! config_namespace_with_hashmap { } } } - } + }; + ($($tokens:tt)*) => { + config_namespace_with_hashmap! { + @with_derives [Debug, Clone, PartialEq] + $($tokens)* + } + }; } config_namespace_with_hashmap! { @@ -3491,7 +3498,11 @@ config_namespace_with_hashmap! { } } -#[derive(Clone, Debug, PartialEq)] +/// Parquet file encryption configuration. +/// +/// Key contents are redacted from [`Debug`](std::fmt::Debug) output. Direct field +/// access and [`ConfigField`] visits still expose the configured key values. +#[derive(Clone, PartialEq)] pub struct ConfigFileEncryptionProperties { /// Should the parquet footer be encrypted /// default is true @@ -3509,6 +3520,25 @@ pub struct ConfigFileEncryptionProperties { pub store_aad_prefix: bool, } +impl fmt::Debug for ConfigFileEncryptionProperties { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("ConfigFileEncryptionProperties") + .field("encrypt_footer", &self.encrypt_footer) + .field("footer_key_as_hex", &"") + .field( + "footer_key_metadata_as_hex", + &self.footer_key_metadata_as_hex, + ) + .field( + "column_encryption_properties", + &self.column_encryption_properties, + ) + .field("aad_prefix_as_hex", &self.aad_prefix_as_hex) + .field("store_aad_prefix", &self.store_aad_prefix) + .finish() + } +} + // Setup to match EncryptionPropertiesBuilder::new() impl Default for ConfigFileEncryptionProperties { fn default() -> Self { @@ -3524,6 +3554,9 @@ impl Default for ConfigFileEncryptionProperties { } config_namespace_with_hashmap! { + @with_derives [Clone, PartialEq] + /// Per-column encryption configuration, with key contents redacted from + /// [`Debug`](std::fmt::Debug) output. pub struct ColumnEncryptionProperties { /// Per column encryption key pub column_key_as_hex: String, default = "".to_string() @@ -3532,6 +3565,15 @@ config_namespace_with_hashmap! { } } +impl fmt::Debug for ColumnEncryptionProperties { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("ColumnEncryptionProperties") + .field("column_key_as_hex", &"") + .field("column_metadata_as_hex", &self.column_metadata_as_hex) + .finish() + } +} + impl ConfigField for ConfigFileEncryptionProperties { fn visit(&self, v: &mut V, key_prefix: &str, _description: &'static str) { let key = format!("{key_prefix}.encrypt_footer"); @@ -3689,7 +3731,11 @@ impl From<&Arc> for ConfigFileEncryptionProperties { } } -#[derive(Clone, Debug, PartialEq)] +/// Parquet file decryption configuration. +/// +/// Key contents are redacted from [`Debug`](std::fmt::Debug) output. Direct field +/// access and [`ConfigField`] visits still expose the configured key values. +#[derive(Clone, PartialEq)] pub struct ConfigFileDecryptionProperties { /// Binary string to use for the parquet footer encoded in hex format pub footer_key_as_hex: String, @@ -3702,13 +3748,41 @@ pub struct ConfigFileDecryptionProperties { pub footer_signature_verification: bool, } +impl fmt::Debug for ConfigFileDecryptionProperties { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("ConfigFileDecryptionProperties") + .field("footer_key_as_hex", &"") + .field( + "column_decryption_properties", + &self.column_decryption_properties, + ) + .field("aad_prefix_as_hex", &self.aad_prefix_as_hex) + .field( + "footer_signature_verification", + &self.footer_signature_verification, + ) + .finish() + } +} + config_namespace_with_hashmap! { + @with_derives [Clone, PartialEq] + /// Per-column decryption configuration, with key contents redacted from + /// [`Debug`](std::fmt::Debug) output. pub struct ColumnDecryptionProperties { /// Per column encryption key pub column_key_as_hex: String, default = "".to_string() } } +impl fmt::Debug for ColumnDecryptionProperties { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("ColumnDecryptionProperties") + .field("column_key_as_hex", &"") + .finish() + } +} + // Setup to match DecryptionPropertiesBuilder::new() impl Default for ConfigFileDecryptionProperties { fn default() -> Self { diff --git a/datafusion/common/tests/encryption_config_debug.rs b/datafusion/common/tests/encryption_config_debug.rs new file mode 100644 index 0000000000000..c639b4f987448 --- /dev/null +++ b/datafusion/common/tests/encryption_config_debug.rs @@ -0,0 +1,282 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Public API regressions for diagnostic formatting of Parquet key configuration. + +use std::collections::HashMap; +use std::fmt::Debug; + +use datafusion_common::config::{ + ColumnDecryptionProperties, ColumnEncryptionProperties, ConfigField, + ConfigFileDecryptionProperties, ConfigFileEncryptionProperties, ParquetColumnOptions, + TableOptions, +}; + +// Synthetic, distinct AES-128 keys. No external service or credential is used. +const FOOTER_KEY: &str = "000102030405060708090a0b0c0d0e0f"; +const FIRST_COLUMN_KEY: &str = "101112131415161718191a1b1c1d1e1f"; +const SECOND_COLUMN_KEY: &str = "202122232425262728292a2b2c2d2e2f"; +const FOOTER_METADATA: &str = "footer-key-id"; +const COLUMN_METADATA: &str = "column-key-id"; +const AAD_PREFIX: &str = "file-context"; + +fn encryption_config() -> ConfigFileEncryptionProperties { + ConfigFileEncryptionProperties { + encrypt_footer: false, + footer_key_as_hex: FOOTER_KEY.to_owned(), + footer_key_metadata_as_hex: FOOTER_METADATA.to_owned(), + column_encryption_properties: HashMap::from([ + ( + "first".to_owned(), + ColumnEncryptionProperties { + column_key_as_hex: FIRST_COLUMN_KEY.to_owned(), + column_metadata_as_hex: Some(COLUMN_METADATA.to_owned()), + }, + ), + ( + "second".to_owned(), + ColumnEncryptionProperties { + column_key_as_hex: SECOND_COLUMN_KEY.to_owned(), + column_metadata_as_hex: None, + }, + ), + ]), + aad_prefix_as_hex: AAD_PREFIX.to_owned(), + store_aad_prefix: true, + } +} + +fn decryption_config() -> ConfigFileDecryptionProperties { + ConfigFileDecryptionProperties { + footer_key_as_hex: FOOTER_KEY.to_owned(), + column_decryption_properties: HashMap::from([ + ( + "first".to_owned(), + ColumnDecryptionProperties { + column_key_as_hex: FIRST_COLUMN_KEY.to_owned(), + }, + ), + ( + "second".to_owned(), + ColumnDecryptionProperties { + column_key_as_hex: SECOND_COLUMN_KEY.to_owned(), + }, + ), + ]), + aad_prefix_as_hex: AAD_PREFIX.to_owned(), + footer_signature_verification: false, + } +} + +fn assert_redacted_debug(value: &impl Debug, visible: &[&str]) { + for rendered in [format!("{value:?}"), format!("{value:#?}")] { + for secret in [FOOTER_KEY, FIRST_COLUMN_KEY, SECOND_COLUMN_KEY] { + assert!( + !rendered.contains(secret), + "key material appeared in Debug: {rendered}" + ); + } + assert!(rendered.contains(""), "{rendered}"); + for expected in visible { + assert!( + rendered.contains(expected), + "missing {expected}: {rendered}" + ); + } + } +} + +#[test] +fn file_encryption_debug_redacts_keys_and_preserves_context() { + assert_redacted_debug( + &encryption_config(), + &[ + "ConfigFileEncryptionProperties", + "encrypt_footer: false", + "footer_key_as_hex", + FOOTER_METADATA, + COLUMN_METADATA, + AAD_PREFIX, + "store_aad_prefix: true", + "first", + "second", + ], + ); +} + +#[test] +fn file_decryption_debug_redacts_keys_and_preserves_context() { + assert_redacted_debug( + &decryption_config(), + &[ + "ConfigFileDecryptionProperties", + "footer_key_as_hex", + AAD_PREFIX, + "footer_signature_verification: false", + "first", + "second", + ], + ); +} + +#[test] +fn column_encryption_debug_redacts_key_and_preserves_metadata() { + for config in encryption_config().column_encryption_properties.values() { + assert_redacted_debug( + config, + &[ + "ColumnEncryptionProperties", + "column_key_as_hex", + "column_metadata_as_hex", + ], + ); + } +} + +#[test] +fn column_decryption_debug_redacts_key() { + for config in decryption_config().column_decryption_properties.values() { + assert_redacted_debug( + config, + &["ColumnDecryptionProperties", "column_key_as_hex"], + ); + } +} + +#[test] +fn table_options_debug_redacts_nested_keys() { + let mut options = TableOptions::new(); + options.parquet.crypto.file_encryption = Some(encryption_config()); + options.parquet.crypto.file_decryption = Some(decryption_config()); + assert_redacted_debug(&options, &["TableOptions", FOOTER_METADATA, AAD_PREFIX]); + assert_redacted_debug(&options.parquet, &["TableParquetOptions", COLUMN_METADATA]); + assert_redacted_debug( + &options.parquet.crypto, + &["file_encryption", "file_decryption"], + ); +} + +#[test] +fn debug_does_not_change_key_configuration_or_entries() { + let mut options = TableOptions::new(); + for operation in ["file_encryption", "file_decryption"] { + for (suffix, value) in [ + ("footer_key_as_hex", FOOTER_KEY), + ("column_key_as_hex::first", FIRST_COLUMN_KEY), + ("column_key_as_hex::second", SECOND_COLUMN_KEY), + ] { + options + .parquet + .set(&format!("crypto.{operation}.{suffix}"), value) + .unwrap(); + } + } + let before = options.clone(); + let _ = format!("{options:?} {options:#?}"); + assert_eq!(before.parquet, options.parquet); + assert_eq!( + options + .parquet + .crypto + .file_encryption + .as_ref() + .unwrap() + .footer_key_as_hex, + FOOTER_KEY + ); + assert_eq!( + options + .parquet + .crypto + .file_decryption + .as_ref() + .unwrap() + .column_decryption_properties["second"] + .column_key_as_hex, + SECOND_COLUMN_KEY + ); + + // Explicit configuration enumeration remains a data-access API, not a log-safe view. + let entries = options.entries(); + for operation in ["file_encryption", "file_decryption"] { + for (suffix, expected) in [ + ("footer_key_as_hex", FOOTER_KEY), + ("column_key_as_hex::first", FIRST_COLUMN_KEY), + ("column_key_as_hex::second", SECOND_COLUMN_KEY), + ] { + let key = format!("parquet.crypto.{operation}.{suffix}"); + let entry = entries.iter().find(|entry| entry.key == key).unwrap(); + assert_eq!(entry.value.as_deref(), Some(expected)); + } + } +} + +#[test] +fn non_secret_column_options_keep_debug_values() { + let options = ParquetColumnOptions { + compression: Some("zstd(3)".to_owned()), + bloom_filter_enabled: Some(true), + ..Default::default() + }; + for rendered in [format!("{options:?}"), format!("{options:#?}")] { + assert!(rendered.contains("zstd(3)")); + assert!(rendered.contains("true")); + assert!(!rendered.contains("")); + } +} + +#[cfg(feature = "parquet_encryption")] +#[test] +fn debug_preserves_parquet_encryption_conversion_and_roundtrip() { + use std::sync::Arc; + + use datafusion_common::encryption::{ + FileDecryptionProperties, FileEncryptionProperties, + }; + + // Metadata and AAD are hex-encoded in configuration, unlike the diagnostic-only + // context strings used by the other tests. + let mut encrypt = encryption_config(); + encrypt.footer_key_metadata_as_hex = hex::encode(FOOTER_METADATA); + encrypt.aad_prefix_as_hex = hex::encode(AAD_PREFIX); + encrypt + .column_encryption_properties + .get_mut("first") + .unwrap() + .column_metadata_as_hex = Some(hex::encode(COLUMN_METADATA)); + encrypt + .column_encryption_properties + .get_mut("second") + .unwrap() + .column_metadata_as_hex = Some(hex::encode("second-column-key-id")); + let mut decrypt = decryption_config(); + decrypt.aad_prefix_as_hex = hex::encode(AAD_PREFIX); + + let _ = format!("{encrypt:?} {encrypt:#?} {decrypt:?} {decrypt:#?}"); + let encrypt_props = + Arc::new(FileEncryptionProperties::try_from(encrypt.clone()).unwrap()); + assert_eq!( + ConfigFileEncryptionProperties::from(&encrypt_props), + encrypt + ); + let decrypt_props = + Arc::new(FileDecryptionProperties::try_from(decrypt.clone()).unwrap()); + assert_eq!( + ConfigFileDecryptionProperties::try_from(&decrypt_props).unwrap(), + decrypt + ); +} diff --git a/datafusion/core/tests/parquet/encryption.rs b/datafusion/core/tests/parquet/encryption.rs index b7bfcefa30f34..f1351f91af4e8 100644 --- a/datafusion/core/tests/parquet/encryption.rs +++ b/datafusion/core/tests/parquet/encryption.rs @@ -24,7 +24,11 @@ use async_trait::async_trait; use datafusion::dataframe::DataFrameWriteOptions; use datafusion::datasource::listing::ListingOptions; use datafusion::prelude::{ParquetReadOptions, SessionContext}; -use datafusion_common::config::{EncryptionFactoryOptions, TableParquetOptions}; +use datafusion_common::config::{ + ColumnDecryptionProperties, ColumnEncryptionProperties, + ConfigFileDecryptionProperties, ConfigFileEncryptionProperties, + EncryptionFactoryOptions, TableParquetOptions, +}; use datafusion_common::{DataFusionError, assert_batches_sorted_eq, exec_datafusion_err}; use datafusion_datasource_parquet::ParquetFormat; use datafusion_execution::parquet_encryption::EncryptionFactory; @@ -132,6 +136,115 @@ async fn round_trip_encryption() { assert_eq!(num_rows_written, num_rows_read); } +#[tokio::test] +async fn encrypted_parquet_round_trip_with_redacted_config_debug() { + // Distinct synthetic 128-bit keys ensure the column key is needed in addition + // to the footer key. Neither key should appear in configuration diagnostics. + let footer_key = "00112233445566778899aabbccddeeff"; + let column_key = "ffeeddccbbaa99887766554433221100"; + let encryption = ConfigFileEncryptionProperties { + footer_key_as_hex: footer_key.to_string(), + column_encryption_properties: HashMap::from([( + "value".to_string(), + ColumnEncryptionProperties { + column_key_as_hex: column_key.to_string(), + ..Default::default() + }, + )]), + ..Default::default() + }; + let decryption = ConfigFileDecryptionProperties { + footer_key_as_hex: footer_key.to_string(), + column_decryption_properties: HashMap::from([( + "value".to_string(), + ColumnDecryptionProperties { + column_key_as_hex: column_key.to_string(), + }, + )]), + ..Default::default() + }; + let mut options = TableParquetOptions::default(); + options.crypto.file_encryption = Some(encryption); + options.crypto.file_decryption = Some(decryption.clone()); + let format = ParquetFormat::default().with_options(options.clone()); + for diagnostic in [ + format!("{options:?}"), + format!("{options:#?}"), + format!("{format:?}"), + format!("{format:#?}"), + ] { + assert!(!diagnostic.contains(footer_key), "{diagnostic}"); + assert!(!diagnostic.contains(column_key), "{diagnostic}"); + } + + let ids: ArrayRef = Arc::new(Int32Array::from(vec![Some(1), Some(2), None])); + let values: ArrayRef = + Arc::new(StringArray::from(vec![Some("alpha"), None, Some("gamma")])); + let batch = RecordBatch::try_from_iter(vec![("id", ids), ("value", values)]).unwrap(); + let ctx = SessionContext::new(); + let df = ctx.read_batch(batch).unwrap(); + let tempdir = TempDir::new().unwrap(); + let path = tempdir.path().join("encrypted.parquet"); + let path = path.to_str().unwrap(); + df.write_parquet( + path, + DataFrameWriteOptions::new().with_single_file_output(true), + Some(options), + ) + .await + .unwrap(); + + let read_ctx = SessionContext::new(); + let batches = read_parquet_test_data( + path, + &read_ctx, + ParquetReadOptions::default().file_decryption_properties(decryption.clone()), + ) + .await; + assert_batches_sorted_eq!( + [ + "+----+-------+", + "| id | value |", + "+----+-------+", + "| 1 | alpha |", + "| 2 | |", + "| | gamma |", + "+----+-------+", + ], + &batches + ); + + // A readable footer must not make the encrypted column readable with an + // incorrect column key. Accept failure during either metadata or data reads. + let mut wrong_decryption = decryption; + wrong_decryption + .column_decryption_properties + .get_mut("value") + .unwrap() + .column_key_as_hex = "102132435465768798a9bacbdcedfe0f".to_string(); + let wrong_key_result = async { + let df = SessionContext::new() + .read_parquet( + path, + ParquetReadOptions::default() + .file_decryption_properties(wrong_decryption), + ) + .await?; + df.collect().await + } + .await; + assert!(wrong_key_result.is_err()); + + let missing_key_result = async { + let df = SessionContext::new() + .read_parquet(path, ParquetReadOptions::default()) + .await?; + df.collect().await + } + .await; + assert!(missing_key_result.is_err()); +} + #[tokio::test] async fn round_trip_parquet_with_encryption_factory() { let ctx = SessionContext::new();