Skip to content

Commit cd05b41

Browse files
feat: Making From<ConfigFileDecryptionProperties/ConfigFileEncryptionProperties> conversions fallible with TryFrom (#21985)
## Which issue does this PR close? - Closes #21974 . ## Rationale for this change The conversions from `ConfigFileDecryptionProperties` to `FileDecryptionProperties` and from `ConfigFileEncryptionProperties` to `FileEncryptionProperties` currently use From, but the implementations contain several unwrap() and expect() calls which can cause panics for invalid or wrong inputs. Instead of panics we can gracefully return `DataFusionError::Confoguration` errors. ## What changes are included in this PR? ``` rust impl From<ConfigFileEncryptionProperties> for FileEncryptionProperties ``` becomes ``` rust impl TryFrom<ConfigFileEncryptionProperties> for FileEncryptionProperties ``` Similarly ```rust impl From<ConfigFileDecryptionProperties> for FileDecryptionProperties ``` becomes ```rust impl TryFrom<ConfigFileDecryptionProperties> for FileDecryptionProperties ``` These are header changes with accompanied implementation changes ## Are these changes tested? Yes tests are added in `datafusion/common/src/config.rs` ## Are there any user-facing changes? Yes this is an user facing change. And upgradation guide is added in `54.0.0.md` --------- Co-authored-by: Kumar Ujjawal <ujjawalpathak6@gmail.com>
1 parent e4f43ff commit cd05b41

4 files changed

Lines changed: 228 additions & 28 deletions

File tree

‎datafusion/common/src/config.rs‎

Lines changed: 182 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -2862,28 +2862,42 @@ impl ConfigField for ConfigFileEncryptionProperties {
28622862
}
28632863

28642864
#[cfg(feature = "parquet_encryption")]
2865-
impl From<ConfigFileEncryptionProperties> for FileEncryptionProperties {
2866-
fn from(val: ConfigFileEncryptionProperties) -> Self {
2865+
impl TryFrom<ConfigFileEncryptionProperties> for FileEncryptionProperties {
2866+
type Error = DataFusionError;
2867+
2868+
fn try_from(val: ConfigFileEncryptionProperties) -> Result<Self> {
28672869
let mut fep = FileEncryptionProperties::builder(
2868-
hex::decode(val.footer_key_as_hex).unwrap(),
2870+
hex::decode(val.footer_key_as_hex)
2871+
.map_err(|e| {
2872+
DataFusionError::Configuration(format!("Unable to decode hex footer key from ConfigFileEncryptionProperties: {e}"))
2873+
})?,
28692874
)
28702875
.with_plaintext_footer(!val.encrypt_footer)
28712876
.with_aad_prefix_storage(val.store_aad_prefix);
28722877

28732878
if !val.footer_key_metadata_as_hex.is_empty() {
28742879
fep = fep.with_footer_key_metadata(
28752880
hex::decode(&val.footer_key_metadata_as_hex)
2876-
.expect("Invalid footer key metadata"),
2881+
.map_err(|e| {
2882+
DataFusionError::Configuration(format!("Unable to decode hex footer key metadata from ConfigFileEncryptionProperties: {e}"))
2883+
})?,
28772884
);
28782885
}
28792886

28802887
for (column_name, encryption_props) in val.column_encryption_properties.iter() {
28812888
let encryption_key = hex::decode(&encryption_props.column_key_as_hex)
2882-
.expect("Invalid column encryption key");
2889+
.map_err(|e| {
2890+
DataFusionError::Configuration(format!("Unable to decode hex encryption key for column {column_name}: {e}"))
2891+
})?;
28832892
let key_metadata = encryption_props
28842893
.column_metadata_as_hex
28852894
.as_ref()
2886-
.map(|x| hex::decode(x).expect("Invalid column metadata"));
2895+
.map(hex::decode)
2896+
.transpose()
2897+
.map_err(|e| {
2898+
DataFusionError::Configuration(format!("Unable to decode hex column metadata for column {column_name}: {e}"))
2899+
})?;
2900+
28872901
match key_metadata {
28882902
Some(key_metadata) => {
28892903
fep = fep.with_column_key_and_metadata(
@@ -2899,11 +2913,18 @@ impl From<ConfigFileEncryptionProperties> for FileEncryptionProperties {
28992913
}
29002914

29012915
if !val.aad_prefix_as_hex.is_empty() {
2902-
let aad_prefix: Vec<u8> =
2903-
hex::decode(&val.aad_prefix_as_hex).expect("Invalid AAD prefix");
2916+
let aad_prefix: Vec<u8> = hex::decode(&val.aad_prefix_as_hex).map_err(|e| {
2917+
DataFusionError::Configuration(format!(
2918+
"Unable to decode hex AAD prefix from ConfigFileEncryptionProperties: {e}"
2919+
))
2920+
})?;
29042921
fep = fep.with_aad_prefix(aad_prefix);
29052922
}
2906-
Arc::unwrap_or_clone(fep.build().unwrap())
2923+
Ok(Arc::unwrap_or_clone(fep.build().map_err(|e| {
2924+
DataFusionError::Configuration(format!(
2925+
"Could not build FileEncryptionProperties: {e}"
2926+
))
2927+
})?))
29072928
}
29082929
}
29092930

@@ -3019,36 +3040,54 @@ impl ConfigField for ConfigFileDecryptionProperties {
30193040
}
30203041

30213042
#[cfg(feature = "parquet_encryption")]
3022-
impl From<ConfigFileDecryptionProperties> for FileDecryptionProperties {
3023-
fn from(val: ConfigFileDecryptionProperties) -> Self {
3043+
impl TryFrom<ConfigFileDecryptionProperties> for FileDecryptionProperties {
3044+
type Error = DataFusionError;
3045+
3046+
fn try_from(val: ConfigFileDecryptionProperties) -> Result<Self> {
30243047
let mut column_names: Vec<&str> = Vec::new();
30253048
let mut column_keys: Vec<Vec<u8>> = Vec::new();
30263049

30273050
for (col_name, decryption_properties) in val.column_decryption_properties.iter() {
3051+
let column_key = hex::decode(&decryption_properties.column_key_as_hex).map_err(|e| {
3052+
DataFusionError::Configuration(format!
3053+
("Could not decode hex column key from ConfigFileDecryptionProperties for column name {col_name}: {e}."))
3054+
})?;
30283055
column_names.push(col_name.as_str());
3029-
column_keys.push(
3030-
hex::decode(&decryption_properties.column_key_as_hex)
3031-
.expect("Invalid column decryption key"),
3032-
);
3056+
column_keys.push(column_key);
30333057
}
30343058

3035-
let mut fep = FileDecryptionProperties::builder(
3036-
hex::decode(val.footer_key_as_hex).expect("Invalid footer key"),
3037-
)
3038-
.with_column_keys(column_names, column_keys)
3039-
.unwrap();
3059+
let footer_key = hex::decode(val.footer_key_as_hex).map_err(|e| {
3060+
DataFusionError::Configuration(format!(
3061+
"Could not decode hex footer key from ConfigFileDecryptionProperties: {e}."
3062+
))
3063+
})?;
3064+
3065+
let mut fep = FileDecryptionProperties::builder(footer_key)
3066+
.with_column_keys(column_names, column_keys)
3067+
.map_err(|e| {
3068+
DataFusionError::Configuration(format!(
3069+
"Could not set column keys on FileDecryptionPropertiesBuilder: {e}."
3070+
))
3071+
})?;
30403072

30413073
if !val.footer_signature_verification {
30423074
fep = fep.disable_footer_signature_verification();
30433075
}
30443076

30453077
if !val.aad_prefix_as_hex.is_empty() {
3046-
let aad_prefix =
3047-
hex::decode(&val.aad_prefix_as_hex).expect("Invalid AAD prefix");
3078+
let aad_prefix = hex::decode(&val.aad_prefix_as_hex).map_err(|e| {
3079+
DataFusionError::Configuration(format!(
3080+
"Could not decode hex AAD prefix from ConfigFileDecryptionProperties: {e}."
3081+
))
3082+
})?;
30483083
fep = fep.with_aad_prefix(aad_prefix);
30493084
}
30503085

3051-
Arc::unwrap_or_clone(fep.build().unwrap())
3086+
Ok(Arc::unwrap_or_clone(fep.build().map_err(|e| {
3087+
DataFusionError::Configuration(format!(
3088+
"Could not build FileDecryptionProperties: {e}."
3089+
))
3090+
})?))
30523091
}
30533092
}
30543093

@@ -3586,13 +3625,13 @@ mod tests {
35863625
let config_encrypt =
35873626
ConfigFileEncryptionProperties::from(&file_encryption_properties);
35883627
let encryption_properties_built =
3589-
Arc::new(FileEncryptionProperties::from(config_encrypt.clone()));
3628+
Arc::new(FileEncryptionProperties::try_from(config_encrypt.clone()).unwrap());
35903629
assert_eq!(file_encryption_properties, encryption_properties_built);
35913630

35923631
let config_decrypt =
35933632
ConfigFileDecryptionProperties::try_from(&decryption_properties).unwrap();
35943633
let decryption_properties_built =
3595-
Arc::new(FileDecryptionProperties::from(config_decrypt.clone()));
3634+
Arc::new(FileDecryptionProperties::try_from(config_decrypt.clone()).unwrap());
35963635
assert_eq!(decryption_properties, decryption_properties_built);
35973636

35983637
///////////////////////////////////////////////////////////////////////////////////
@@ -3678,6 +3717,124 @@ mod tests {
36783717
);
36793718
}
36803719

3720+
#[cfg(feature = "parquet_encryption")]
3721+
#[test]
3722+
fn parquet_encryption_invalid_hex_errors_encryption() {
3723+
use crate::config::ColumnEncryptionProperties;
3724+
use crate::config::ConfigFileEncryptionProperties;
3725+
use parquet::encryption::encrypt::FileEncryptionProperties;
3726+
use std::collections::HashMap;
3727+
3728+
let valid_footer_key_as_hex = hex::encode(b"0123456789012345");
3729+
3730+
let mut enc = ConfigFileEncryptionProperties {
3731+
encrypt_footer: true,
3732+
footer_key_as_hex: valid_footer_key_as_hex.clone(),
3733+
footer_key_metadata_as_hex: String::new(),
3734+
column_encryption_properties: HashMap::new(),
3735+
aad_prefix_as_hex: String::new(),
3736+
store_aad_prefix: false,
3737+
};
3738+
3739+
// Encryption: invalid footer key hex
3740+
enc.footer_key_as_hex = "not_hex".to_string();
3741+
let err = FileEncryptionProperties::try_from(enc.clone())
3742+
.unwrap_err()
3743+
.to_string();
3744+
assert!(err.contains("Unable to decode hex footer key"));
3745+
enc.footer_key_as_hex = valid_footer_key_as_hex.clone();
3746+
3747+
// Encryption: invalid footer key metadata hex
3748+
enc.footer_key_metadata_as_hex = "zz".to_string();
3749+
let err = FileEncryptionProperties::try_from(enc.clone())
3750+
.unwrap_err()
3751+
.to_string();
3752+
assert!(err.contains("Unable to decode hex footer key metadata"));
3753+
enc.footer_key_metadata_as_hex = String::new();
3754+
3755+
// Encryption: invalid column key hex
3756+
enc.column_encryption_properties.insert(
3757+
"col1".to_string(),
3758+
ColumnEncryptionProperties {
3759+
column_key_as_hex: "bad".to_string(),
3760+
column_metadata_as_hex: None,
3761+
},
3762+
);
3763+
let err = FileEncryptionProperties::try_from(enc.clone())
3764+
.unwrap_err()
3765+
.to_string();
3766+
assert!(err.contains("Unable to decode hex encryption key for column col1"));
3767+
enc.column_encryption_properties.clear();
3768+
3769+
// Encryption: invalid column metadata hex
3770+
enc.column_encryption_properties.insert(
3771+
"col1".to_string(),
3772+
ColumnEncryptionProperties {
3773+
column_key_as_hex: hex::encode(b"1234567890123450"),
3774+
column_metadata_as_hex: Some("zz".to_string()),
3775+
},
3776+
);
3777+
let err = FileEncryptionProperties::try_from(enc.clone())
3778+
.unwrap_err()
3779+
.to_string();
3780+
assert!(err.contains("Unable to decode hex column metadata for column col1"));
3781+
enc.column_encryption_properties.clear();
3782+
3783+
// Encryption: invalid AAD prefix hex
3784+
enc.aad_prefix_as_hex = "zz".to_string();
3785+
let err = FileEncryptionProperties::try_from(enc.clone())
3786+
.unwrap_err()
3787+
.to_string();
3788+
assert!(err.contains("Unable to decode hex AAD prefix"));
3789+
}
3790+
3791+
#[cfg(feature = "parquet_encryption")]
3792+
#[test]
3793+
fn parquet_encryption_invalid_hex_errors_decryption() {
3794+
use crate::config::ColumnDecryptionProperties;
3795+
use crate::config::ConfigFileDecryptionProperties;
3796+
use parquet::encryption::decrypt::FileDecryptionProperties;
3797+
use std::collections::HashMap;
3798+
3799+
let valid_footer_key_as_hex = hex::encode(b"0123456789012345");
3800+
3801+
let mut dec = ConfigFileDecryptionProperties {
3802+
footer_key_as_hex: valid_footer_key_as_hex.clone(),
3803+
column_decryption_properties: HashMap::new(),
3804+
aad_prefix_as_hex: String::new(),
3805+
footer_signature_verification: true,
3806+
};
3807+
3808+
// Decryption: invalid column key hex
3809+
dec.column_decryption_properties.insert(
3810+
"col1".to_string(),
3811+
ColumnDecryptionProperties {
3812+
column_key_as_hex: "bad".to_string(),
3813+
},
3814+
);
3815+
let err = FileDecryptionProperties::try_from(dec.clone())
3816+
.unwrap_err()
3817+
.to_string();
3818+
assert!(err.contains("Could not decode hex column key"));
3819+
assert!(err.contains("col1"));
3820+
dec.column_decryption_properties.clear();
3821+
3822+
// Decryption: invalid footer key hex
3823+
dec.footer_key_as_hex = "bad".to_string();
3824+
let err = FileDecryptionProperties::try_from(dec.clone())
3825+
.unwrap_err()
3826+
.to_string();
3827+
assert!(err.contains("Could not decode hex footer key"));
3828+
dec.footer_key_as_hex = valid_footer_key_as_hex;
3829+
3830+
// Decryption: invalid AAD prefix hex
3831+
dec.aad_prefix_as_hex = "zz".to_string();
3832+
let err = FileDecryptionProperties::try_from(dec.clone())
3833+
.unwrap_err()
3834+
.to_string();
3835+
assert!(err.contains("Could not decode hex AAD prefix"));
3836+
}
3837+
36813838
#[cfg(feature = "parquet_encryption")]
36823839
#[test]
36833840
fn parquet_encryption_factory_config() {

‎datafusion/datasource-parquet/src/file_format.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,7 @@ async fn get_file_decryption_properties(
308308
file_path: &Path,
309309
) -> Result<Option<Arc<FileDecryptionProperties>>> {
310310
Ok(match &options.crypto.file_decryption {
311-
Some(cfd) => Some(Arc::new(FileDecryptionProperties::from(cfd.clone()))),
311+
Some(cfd) => Some(Arc::new(FileDecryptionProperties::try_from(cfd.clone())?)),
312312
None => match &options.crypto.factory_id {
313313
Some(factory_id) => {
314314
let factory =
@@ -1284,7 +1284,7 @@ async fn set_writer_encryption_properties(
12841284
if let Some(file_encryption_properties) = parquet_opts.crypto.file_encryption {
12851285
// Encryption properties have been specified directly
12861286
return Ok(builder.with_file_encryption_properties(Arc::new(
1287-
FileEncryptionProperties::from(file_encryption_properties),
1287+
FileEncryptionProperties::try_from(file_encryption_properties)?,
12881288
)));
12891289
} else if let Some(encryption_factory_id) = &parquet_opts.crypto.factory_id.as_ref() {
12901290
// Encryption properties will be generated by an encryption factory

‎datafusion/datasource-parquet/src/source.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -543,7 +543,8 @@ impl FileSource for ParquetSource {
543543
.crypto
544544
.file_decryption
545545
.clone()
546-
.map(FileDecryptionProperties::from)
546+
.map(FileDecryptionProperties::try_from)
547+
.transpose()?
547548
.map(Arc::new);
548549

549550
let coerce_int96 = self

‎docs/source/library-user-guide/upgrading/54.0.0.md‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -447,6 +447,48 @@ let config_decryption_properties = ConfigFileDecryptionProperties::try_from(&dec
447447
See [#21602](https://github.com/apache/datafusion/issues/21602) and
448448
[PR #21603](https://github.com/apache/datafusion/pull/21603) for details.
449449

450+
### Conversion from `ConfigFileEncryptionProperties` / `ConfigFileDecryptionProperties` is now fallible
451+
452+
Previously, `datafusion_common::config::ConfigFileEncryptionProperties` and
453+
`datafusion_common::config::ConfigFileDecryptionProperties` implemented infallible
454+
conversions into Parquet's encryption/decryption types (via `From` / `Into`).
455+
These conversions may need to decode hex-encoded keys and other configuration values, which can fail.
456+
457+
They now use `TryFrom` / `TryInto` and return a `Result`:
458+
459+
- `impl TryFrom<ConfigFileEncryptionProperties> for parquet::encryption::encrypt::FileEncryptionProperties`
460+
- `impl TryFrom<ConfigFileDecryptionProperties> for parquet::encryption::decrypt::FileDecryptionProperties`
461+
462+
**Migration guide:**
463+
464+
Replace `from()` / `into()` with `try_from()` / `try_into()` and handle the resulting `Result`.
465+
466+
**Before:**
467+
468+
```rust,ignore
469+
let file_encryption_properties: FileEncryptionProperties = config_encryption_properties.into();
470+
// or
471+
let file_decryption_properties = FileDecryptionProperties::from(config_decryption_properties);
472+
```
473+
474+
(
475+
where `config_encryption_properties` is a `ConfigFileEncryptionProperties` and
476+
`config_decryption_properties` is a `ConfigFileDecryptionProperties`
477+
)
478+
479+
**After:**
480+
481+
```rust,ignore
482+
let file_encryption_properties: FileEncryptionProperties =
483+
config_encryption_properties.try_into()?;
484+
// or
485+
let file_decryption_properties =
486+
FileDecryptionProperties::try_from(config_decryption_properties)?;
487+
```
488+
489+
See [#21974](https://github.com/apache/datafusion/issues/21974) and
490+
[PR #21985](https://github.com/apache/datafusion/pull/21985) for details.
491+
450492
### `approx_percentile_cont`, `approx_percentile_cont_with_weight`, `approx_median` now coerce to floats
451493

452494
The type signatures of `approx_percentile_cont`, `approx_percentile_cont_with_weight`, and

0 commit comments

Comments
 (0)