You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "xanderbailey (via GitHub)" <gi...@apache.org> on 2026/06/04 08:56:19 UTC
[PR] feat(encryption) [8/N] Add decryption support for parquet data-files [iceberg-rust]
xanderbailey opened a new pull request, #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Working towards https://github.com/apache/iceberg-rust/issues/2034
We're currently not able to decrypt PME encrypted files, all that's needed for this PR is to wire through the key_metadata from the manifest entries into the scan tasks and then configure the parquet reader with the correct encryption key and aad.
It's important to note that no encryption manager is needed here since the key_metadata on the manifest files contain the plaintext keys.
## What changes are included in this PR?
<!--
Provide a summary of the modifications in this PR. List the main changes such as new features, bug fixes, refactoring, or any other updates.
-->
## Are these changes tested?
Tests follow the pattern on manually writing an encrypted parquet file using the raw arrow-rs writer and then using the FileScanTask and iceberg reader to read that tmp file.
I looked at using the [parquet testing repo](https://github.com/apache/parquet-testing) but the encrypted files there all use column based encryption which isn't supported by the spec. I'll separately track adding some files there that we can use here but the testing strategy here is consistent with how Java does things so I think this should be sufficient for us to continue.
<!--
Specify what test covers (unit test, integration test, etc.).
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3414614254
##########
crates/iceberg/src/arrow/delete_file_loader.rs:
##########
@@ -165,4 +171,169 @@ mod tests {
assert_eq!(result.len(), 1);
}
+
+ fn write_encrypted_parquet(path: &str, batch: &arrow_array::RecordBatch, key: &[u8]) {
Review Comment:
`write_encrypted_parquet` is copy-pasted, and the delete-loader copy drops the `aad_prefix` param. Share one helper so the delete path also gets AAD coverage?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3420913822
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -118,6 +118,13 @@ pub struct FileScanTask {
/// Whether this scan task should treat column names as case-sensitive when binding predicates.
pub case_sensitive: bool,
+
+ /// Key metadata for encrypted data files (Parquet Modular Encryption).
+ /// When present, the reader uses this to build `FileDecryptionProperties`.
+ #[serde(default)]
+ #[serde(skip_serializing_if = "Option::is_none")]
+ #[builder(default)]
+ pub key_metadata: Option<Box<[u8]>>,
Review Comment:
Thank you! There are other places in Spark and/or Comet where we serialize credentials and the only thing we can do is suggest that users enable secure communication channels (SSL, encryption, etc.).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3576795626
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -431,14 +437,44 @@ impl ArrowReader {
)
.with_parquet_read_options(parquet_read_options);
- let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, Default::default())
+ let arrow_reader_options = Self::build_arrow_reader_options(key_metadata)?;
+
+ let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, arrow_reader_options)
.await
.map_err(|e| {
Error::new(ErrorKind::Unexpected, "Failed to load Parquet metadata").with_source(e)
})?;
Ok((reader, arrow_metadata))
}
+
+ /// Builds `ArrowReaderOptions`, adding `FileDecryptionProperties` when
+ /// key metadata is present for Parquet Modular Encryption.
+ fn build_arrow_reader_options(key_metadata: Option<&[u8]>) -> Result<ArrowReaderOptions> {
Review Comment:
I agree with @xanderbailey we could postpone the dynamic resolution approach until we feel necessary.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3577048877
##########
crates/iceberg/src/encryption/key_metadata.rs:
##########
@@ -97,7 +97,17 @@ impl StandardKeyMetadata {
/// Decodes from Java-compatible format.
pub fn decode(bytes: &[u8]) -> Result<Self> {
- _serde::StandardKeyMetadataV1::decode(bytes).map(Self::from)
+ let metadata = _serde::StandardKeyMetadataV1::decode(bytes).map(Self::from)?;
+ // Validate the DEK is a usable AES key (16/24/32 bytes) up front, so a
+ // malformed key surfaces a clear error.
+ SecureKey::new(metadata.encryption_key.as_bytes()).map_err(|e| {
Review Comment:
I think it's okay since `decode` is the trust boundary where an "attacker" would try and use incorrect standard metadata. I'm happy to add it to `new` and `From` but I don't think it's required. I can create an issue to do it as a follow-up but it would require returning `Result` which is a break and a small refactor. WDYT?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#issuecomment-4978074377
cc @mbutrovich To see if there is other concerns.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3414612059
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -638,6 +675,222 @@ mod tests {
}
}
+ fn write_encrypted_parquet(
+ path: &str,
+ batch: &RecordBatch,
+ key: &[u8],
+ aad_prefix: Option<&[u8]>,
+ ) {
+ let mut builder = FileEncryptionProperties::builder(key.to_vec());
+ if let Some(aad) = aad_prefix {
+ builder = builder.with_aad_prefix(aad.to_vec());
+ }
+ let encryption_properties = builder.build().unwrap();
+
+ let props = WriterProperties::builder()
+ .set_compression(Compression::SNAPPY)
+ .with_file_encryption_properties(encryption_properties)
+ .build();
+
+ let file = File::create(path).unwrap();
+ let mut writer = ArrowWriter::try_new(file, batch.schema(), Some(props)).unwrap();
+ writer.write(batch).expect("Writing batch");
+ writer.close().unwrap();
+ }
+
+ #[tokio::test]
+ async fn test_read_encrypted_parquet() {
+ let encryption_key = b"0123456789abcdef";
+ let aad_prefix = b"my-table-uuid!!";
+
+ let schema = Arc::new(
+ Schema::builder()
+ .with_schema_id(1)
+ .with_fields(vec![
+ NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(),
+ ])
+ .build()
+ .unwrap(),
+ );
+
+ let arrow_schema = Arc::new(ArrowSchema::new(vec![
+ Field::new("id", DataType::Int32, false).with_metadata(HashMap::from([(
+ PARQUET_FIELD_ID_META_KEY.to_string(),
+ "1".to_string(),
+ )])),
+ ]));
+
+ let tmp_dir = TempDir::new().unwrap();
+ let table_location = tmp_dir.path().to_str().unwrap().to_string();
+ let file_io = FileIO::new_with_fs();
+
+ let id_data = Arc::new(Int32Array::from(vec![10, 20, 30])) as ArrayRef;
+ let batch = RecordBatch::try_new(arrow_schema.clone(), vec![id_data]).unwrap();
+
+ let file_path = format!("{table_location}/encrypted.parquet");
+ write_encrypted_parquet(&file_path, &batch, encryption_key, Some(aad_prefix));
+
+ let key_metadata = crate::encryption::StandardKeyMetadata::new(encryption_key)
+ .with_aad_prefix(aad_prefix)
+ .encode()
+ .unwrap();
+
+ let reader = ArrowReaderBuilder::new(file_io, Runtime::current()).build();
+
+ let task = FileScanTask::builder()
+ .with_file_size_in_bytes(std::fs::metadata(&file_path).unwrap().len())
+ .with_start(0)
+ .with_length(0)
+ .with_data_file_path(file_path)
+ .with_data_file_format(DataFileFormat::Parquet)
+ .with_schema(schema)
+ .with_project_field_ids(vec![1])
+ .with_case_sensitive(false)
+ .with_key_metadata(Some(key_metadata))
+ .build();
+
+ let tasks = Box::pin(futures::stream::iter(vec![Ok(task)])) as FileScanTaskStream;
+ let batches: Vec<RecordBatch> = reader
+ .read(tasks)
+ .unwrap()
+ .stream()
+ .try_collect()
+ .await
+ .unwrap();
+
+ assert_eq!(batches.len(), 1);
+ let ids = batches[0]
+ .column(0)
+ .as_any()
+ .downcast_ref::<Int32Array>()
+ .unwrap();
+ assert_eq!(ids.values(), &[10, 20, 30]);
+ }
+
+ #[tokio::test]
+ async fn test_read_encrypted_parquet_without_key_metadata_fails() {
+ let encryption_key = b"0123456789abcdef";
+
+ let schema = Arc::new(
+ Schema::builder()
+ .with_schema_id(1)
+ .with_fields(vec![
+ NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(),
+ ])
+ .build()
+ .unwrap(),
+ );
+
+ let arrow_schema = Arc::new(ArrowSchema::new(vec![
+ Field::new("id", DataType::Int32, false).with_metadata(HashMap::from([(
+ PARQUET_FIELD_ID_META_KEY.to_string(),
+ "1".to_string(),
+ )])),
+ ]));
+
+ let tmp_dir = TempDir::new().unwrap();
+ let table_location = tmp_dir.path().to_str().unwrap().to_string();
+ let file_io = FileIO::new_with_fs();
+
+ let id_data = Arc::new(Int32Array::from(vec![1, 2, 3])) as ArrayRef;
+ let batch = RecordBatch::try_new(arrow_schema.clone(), vec![id_data]).unwrap();
+
+ let file_path = format!("{table_location}/encrypted_no_key.parquet");
+ write_encrypted_parquet(&file_path, &batch, encryption_key, None);
+
+ let reader = ArrowReaderBuilder::new(file_io, Runtime::current()).build();
+
+ let task = FileScanTask::builder()
+ .with_file_size_in_bytes(std::fs::metadata(&file_path).unwrap().len())
+ .with_start(0)
+ .with_length(0)
+ .with_data_file_path(file_path)
+ .with_data_file_format(DataFileFormat::Parquet)
+ .with_schema(schema)
+ .with_project_field_ids(vec![1])
+ .with_case_sensitive(false)
+ .build();
+
+ let tasks = Box::pin(futures::stream::iter(vec![Ok(task)])) as FileScanTaskStream;
+ let result: Result<Vec<RecordBatch>, _> =
+ reader.read(tasks).unwrap().stream().try_collect().await;
+
+ let err = result.unwrap_err();
+ assert_eq!(err.kind(), crate::ErrorKind::Unexpected);
+ let err_str = format!("{err}");
+ assert!(
+ err_str.contains("encrypted footer"),
Review Comment:
We would need to update this if `parquet` ever words that error differently. Nothing worth changing, just noting it as a future maintenance spot.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3414948337
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -431,14 +437,44 @@ impl ArrowReader {
)
.with_parquet_read_options(parquet_read_options);
- let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, Default::default())
+ let arrow_reader_options = Self::build_arrow_reader_options(key_metadata)?;
+
+ let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, arrow_reader_options)
.await
.map_err(|e| {
Error::new(ErrorKind::Unexpected, "Failed to load Parquet metadata").with_source(e)
})?;
Ok((reader, arrow_metadata))
}
+
+ /// Builds `ArrowReaderOptions`, adding `FileDecryptionProperties` when
+ /// key metadata is present for Parquet Modular Encryption.
+ fn build_arrow_reader_options(key_metadata: Option<&[u8]>) -> Result<ArrowReaderOptions> {
Review Comment:
Ah this is very interesting, I opened a thread https://github.com/apache/iceberg/pull/16527/changes and https://lists.apache.org/list?dev@iceberg.apache.org:lte=1M:[DISCUSS]%20Specifying%20the%20encryption%20key%20metadata%20formats%20for%20cross-implementation%20interop to actually close this gap in the spec. It would appear to me to be the case that adding specific implementations here would beak a table's ability to interop with different engines / clients and hence I have suggested closing that gap.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3585362389
##########
crates/iceberg/src/encryption/key_metadata.rs:
##########
@@ -97,7 +97,17 @@ impl StandardKeyMetadata {
/// Decodes from Java-compatible format.
pub fn decode(bytes: &[u8]) -> Result<Self> {
- _serde::StandardKeyMetadataV1::decode(bytes).map(Self::from)
+ let metadata = _serde::StandardKeyMetadataV1::decode(bytes).map(Self::from)?;
+ // Validate the DEK is a usable AES key (16/24/32 bytes) up front, so a
+ // malformed key surfaces a clear error.
+ SecureKey::new(metadata.encryption_key.as_bytes()).map_err(|e| {
Review Comment:
I think we should add it since as a library the boundary should be api, and it would be difficult to predicate how users may use it. I'm fine with doing it in a follow up issue.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3415038377
##########
crates/iceberg/src/scan/context.rs:
##########
@@ -136,6 +136,7 @@ impl ManifestEntryContext {
// TODO: Extract name_mapping from table metadata property "schema.name-mapping.default"
.with_name_mapping(None)
.with_case_sensitive(self.case_sensitive)
+ .with_key_metadata(self.manifest_entry.data_file.key_metadata().map(Box::from))
Review Comment:
The metadata here is very small (max 32 byte keys + aad) so it's a pretty small cost and hard to avoid with the current setup unfortunately.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3414716454
##########
crates/iceberg/src/scan/context.rs:
##########
@@ -136,6 +136,7 @@ impl ManifestEntryContext {
// TODO: Extract name_mapping from table metadata property "schema.name-mapping.default"
.with_name_mapping(None)
.with_case_sensitive(self.case_sensitive)
+ .with_key_metadata(self.manifest_entry.data_file.key_metadata().map(Box::from))
Review Comment:
`map(Box::from)` copies the key-metadata slice into a fresh allocation for every file on the scan path. Fine if intended, just flagging it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3414598284
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -118,6 +118,13 @@ pub struct FileScanTask {
/// Whether this scan task should treat column names as case-sensitive when binding predicates.
pub case_sensitive: bool,
+
+ /// Key metadata for encrypted data files (Parquet Modular Encryption).
+ /// When present, the reader uses this to build `FileDecryptionProperties`.
+ #[serde(default)]
+ #[serde(skip_serializing_if = "Option::is_none")]
+ #[builder(default)]
+ pub key_metadata: Option<Box<[u8]>>,
Review Comment:
`FileScanTask` derives `Serialize`, so with `StandardKeyMetadata` the plaintext DEK serializes into the plan when a task ships to a worker. `skip_serializing_if` (line 125) only covers the `None` case. Is that an acceptable trust boundary here, or worth a field doc note? (A pluggable key-resolution hook in the reader would also address it, since the task would then carry only a key reference rather than the DEK.)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3416627826
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -431,14 +437,44 @@ impl ArrowReader {
)
.with_parquet_read_options(parquet_read_options);
- let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, Default::default())
+ let arrow_reader_options = Self::build_arrow_reader_options(key_metadata)?;
+
+ let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, arrow_reader_options)
.await
.map_err(|e| {
Error::new(ErrorKind::Unexpected, "Failed to load Parquet metadata").with_source(e)
})?;
Ok((reader, arrow_metadata))
}
+
+ /// Builds `ArrowReaderOptions`, adding `FileDecryptionProperties` when
+ /// key metadata is present for Parquet Modular Encryption.
+ fn build_arrow_reader_options(key_metadata: Option<&[u8]>) -> Result<ArrowReaderOptions> {
Review Comment:
Claude an I riffed for a bit to see what this might look like https://github.com/xanderbailey/iceberg-rust/pull/2/changes and it's not a crazy change to add in but I'm disinclined to add it into this PR since it changes the scope of this PR by quite a bit. WDYT?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3415007751
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -431,14 +437,44 @@ impl ArrowReader {
)
.with_parquet_read_options(parquet_read_options);
- let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, Default::default())
+ let arrow_reader_options = Self::build_arrow_reader_options(key_metadata)?;
+
+ let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, arrow_reader_options)
.await
.map_err(|e| {
Error::new(ErrorKind::Unexpected, "Failed to load Parquet metadata").with_source(e)
})?;
Ok((reader, arrow_metadata))
}
+
+ /// Builds `ArrowReaderOptions`, adding `FileDecryptionProperties` when
+ /// key metadata is present for Parquet Modular Encryption.
+ fn build_arrow_reader_options(key_metadata: Option<&[u8]>) -> Result<ArrowReaderOptions> {
+ match key_metadata {
+ Some(km) => {
+ let standard_key_metadata = StandardKeyMetadata::decode(km)?;
+ let mut builder = FileDecryptionProperties::builder(
+ standard_key_metadata.encryption_key().as_bytes().to_vec(),
Review Comment:
[a1cda69](https://github.com/apache/iceberg-rust/pull/2584/commits/a1cda69eb66ebbe9dc42228808587cd888b0fd8f)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3576732658
##########
crates/iceberg/src/arrow/mod.rs:
##########
@@ -44,3 +44,38 @@ pub use partition_value_calculator::*;
/// Record batch partition splitter for partitioned tables
pub mod record_batch_partition_splitter;
pub use record_batch_partition_splitter::*;
+
+#[cfg(test)]
+pub(crate) mod test_utils {
Review Comment:
Please move this to a standalone file mode `test_utils`
##########
crates/iceberg/src/encryption/key_metadata.rs:
##########
@@ -97,7 +97,17 @@ impl StandardKeyMetadata {
/// Decodes from Java-compatible format.
pub fn decode(bytes: &[u8]) -> Result<Self> {
- _serde::StandardKeyMetadataV1::decode(bytes).map(Self::from)
+ let metadata = _serde::StandardKeyMetadataV1::decode(bytes).map(Self::from)?;
+ // Validate the DEK is a usable AES key (16/24/32 bytes) up front, so a
+ // malformed key surfaces a clear error.
+ SecureKey::new(metadata.encryption_key.as_bytes()).map_err(|e| {
Review Comment:
If this is required, should we check in all constructor? Like `new` and `From` implementation.
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -431,14 +437,44 @@ impl ArrowReader {
)
.with_parquet_read_options(parquet_read_options);
- let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, Default::default())
+ let arrow_reader_options = Self::build_arrow_reader_options(key_metadata)?;
+
+ let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, arrow_reader_options)
.await
.map_err(|e| {
Error::new(ErrorKind::Unexpected, "Failed to load Parquet metadata").with_source(e)
})?;
Ok((reader, arrow_metadata))
}
+
+ /// Builds `ArrowReaderOptions`, adding `FileDecryptionProperties` when
+ /// key metadata is present for Parquet Modular Encryption.
+ fn build_arrow_reader_options(key_metadata: Option<&[u8]>) -> Result<ArrowReaderOptions> {
Review Comment:
I agree with @xanderbailey we could postpone the dynamic resolution approach when necessary.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#issuecomment-4929338011
@blackmwk I think this should be ready for a review when you get the chance!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3414713299
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -431,14 +437,44 @@ impl ArrowReader {
)
.with_parquet_read_options(parquet_read_options);
- let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, Default::default())
+ let arrow_reader_options = Self::build_arrow_reader_options(key_metadata)?;
+
+ let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, arrow_reader_options)
.await
.map_err(|e| {
Error::new(ErrorKind::Unexpected, "Failed to load Parquet metadata").with_source(e)
})?;
Ok((reader, arrow_metadata))
}
+
+ /// Builds `ArrowReaderOptions`, adding `FileDecryptionProperties` when
+ /// key metadata is present for Parquet Modular Encryption.
+ fn build_arrow_reader_options(key_metadata: Option<&[u8]>) -> Result<ArrowReaderOptions> {
+ match key_metadata {
+ Some(km) => {
+ let standard_key_metadata = StandardKeyMetadata::decode(km)?;
+ let mut builder = FileDecryptionProperties::builder(
+ standard_key_metadata.encryption_key().as_bytes().to_vec(),
Review Comment:
The decoded DEK is passed straight to `FileDecryptionProperties::builder(key)`. A malformed key currently surfaces as arrow-rs's generic build/decrypt error. Would an explicit check that the key is a valid AES length (16/24/32 bytes), returning a clear `iceberg::Error`, be worth adding? That is a real invariant with a better message than the downstream failure.
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -431,14 +437,44 @@ impl ArrowReader {
)
.with_parquet_read_options(parquet_read_options);
- let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, Default::default())
+ let arrow_reader_options = Self::build_arrow_reader_options(key_metadata)?;
+
+ let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, arrow_reader_options)
.await
.map_err(|e| {
Error::new(ErrorKind::Unexpected, "Failed to load Parquet metadata").with_source(e)
})?;
Ok((reader, arrow_metadata))
}
+
+ /// Builds `ArrowReaderOptions`, adding `FileDecryptionProperties` when
+ /// key metadata is present for Parquet Modular Encryption.
+ fn build_arrow_reader_options(key_metadata: Option<&[u8]>) -> Result<ArrowReaderOptions> {
+ match key_metadata {
+ Some(km) => {
+ let standard_key_metadata = StandardKeyMetadata::decode(km)?;
Review Comment:
`StandardKeyMetadata.file_length` is parsed but unused; the size used for reading comes from `task.file_size_in_bytes`. That matches Java, where `fileLength()` has no consumer on the native-encryption path (it is for AGS1 stream decryption, not PME data files), so this looks correct. Not worth asserting `file_length == file_size_in_bytes`: `file_length` is optional and the spec does not guarantee the two are equal.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3416476568
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -118,6 +118,13 @@ pub struct FileScanTask {
/// Whether this scan task should treat column names as case-sensitive when binding predicates.
pub case_sensitive: bool,
+
+ /// Key metadata for encrypted data files (Parquet Modular Encryption).
+ /// When present, the reader uses this to build `FileDecryptionProperties`.
+ #[serde(default)]
+ #[serde(skip_serializing_if = "Option::is_none")]
+ #[builder(default)]
+ pub key_metadata: Option<Box<[u8]>>,
Review Comment:
[add docs for trust boundry](https://github.com/apache/iceberg-rust/pull/2584/commits/b02c2813508610c318161478db9bb0930ac00e76)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3415934611
##########
crates/iceberg/src/scan/task.rs:
##########
@@ -118,6 +118,13 @@ pub struct FileScanTask {
/// Whether this scan task should treat column names as case-sensitive when binding predicates.
pub case_sensitive: bool,
+
+ /// Key metadata for encrypted data files (Parquet Modular Encryption).
+ /// When present, the reader uses this to build `FileDecryptionProperties`.
+ #[serde(default)]
+ #[serde(skip_serializing_if = "Option::is_none")]
+ #[builder(default)]
+ pub key_metadata: Option<Box<[u8]>>,
Review Comment:
[Spark](https://github.com/apache/iceberg/blob/main/spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/source/SparkInputPartition.java#L34-L36)
Has `BaseFileScanTask` [link](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/BaseFileScanTask.java#L28) which has [this](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/BaseContentScanTask.java#L31) which contains raw dek metadata. It's a very good flag though so I'll add a comment.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3414594167
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -431,14 +437,44 @@ impl ArrowReader {
)
.with_parquet_read_options(parquet_read_options);
- let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, Default::default())
+ let arrow_reader_options = Self::build_arrow_reader_options(key_metadata)?;
+
+ let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, arrow_reader_options)
.await
.map_err(|e| {
Error::new(ErrorKind::Unexpected, "Failed to load Parquet metadata").with_source(e)
})?;
Ok((reader, arrow_metadata))
}
+
+ /// Builds `ArrowReaderOptions`, adding `FileDecryptionProperties` when
+ /// key metadata is present for Parquet Modular Encryption.
+ fn build_arrow_reader_options(key_metadata: Option<&[u8]>) -> Result<ArrowReaderOptions> {
Review Comment:
Resolution is hardcoded to `StandardKeyMetadata::decode(km)` -> plaintext DEK (line 456-458). Worth noting the spec defines `key_metadata` as "implementation-specific key metadata for encryption" (field 131), and `StandardKeyMetadata` is the reference implementation's format rather than a spec-mandated one. So baking in that single decode path means non-standard key-management schemes (for example, where `key_metadata` is a key reference resolved from a KMS at read time rather than an embedded DEK) cannot plug in.
Would it be worth keeping the `StandardKeyMetadata` path as the default but exposing the resolution step as a trait, so other implementations can supply their own? Roughly:
```rust
#[async_trait]
pub trait FileDecryptionHandler: Send + Sync {
async fn file_decryption_properties(&self, key_metadata: &[u8]) -> Result<FileDecryptionProperties>;
}
```
One reason to make it `async`: a scheme that resolves the DEK from a KMS at read time (where `key_metadata` carries a wrapped DEK plus a key id, not a plaintext DEK) needs a network round-trip during resolution. arrow-rs has a lower-level `parquet::encryption::decrypt::KeyRetriever`, but its `retrieve_key(&self, key_metadata) -> Result<Vec<u8>>` is synchronous, so it cannot perform an async unwrap inline. The general path is therefore to resolve the key (async) and hand arrow-rs an explicit-key `FileDecryptionProperties` via `builder(dek)` (exactly what this PR already does at line 457), with the default `StandardKeyMetadata` impl just being the synchronous no-KMS case.
Iceberg Java structures it this way: `EncryptionManager` is an interface, with `StandardEncryptionManager` as the spec-compliant default, selected per table and pluggable via the `encryption.kms-impl` catalog property. Worth noting the seam there is at the manager / `decrypt` level, not only at the KMS client, because an alternative scheme may differ in both the `key_metadata` layout and the unwrap step, not just the KMS backend.
Is a hook like this already planned for a later PR, or intentionally deferred? Mainly want to avoid the concrete decode path becoming hard to retrofit once the rest of the series builds on it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3416693917
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -431,14 +437,44 @@ impl ArrowReader {
)
.with_parquet_read_options(parquet_read_options);
- let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, Default::default())
+ let arrow_reader_options = Self::build_arrow_reader_options(key_metadata)?;
+
+ let arrow_metadata = ArrowReaderMetadata::load_async(&mut reader, arrow_reader_options)
.await
.map_err(|e| {
Error::new(ErrorKind::Unexpected, "Failed to load Parquet metadata").with_source(e)
})?;
Ok((reader, arrow_metadata))
}
+
+ /// Builds `ArrowReaderOptions`, adding `FileDecryptionProperties` when
+ /// key metadata is present for Parquet Modular Encryption.
+ fn build_arrow_reader_options(key_metadata: Option<&[u8]>) -> Result<ArrowReaderOptions> {
Review Comment:
It also doesn't look like java allows for this kind of plug-ability (yet) https://github.com/apache/iceberg/blob/8d0508308cd068778897c06d3a802740ed4305a6/parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java#L796-L805
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#discussion_r3576992137
##########
crates/iceberg/src/arrow/mod.rs:
##########
@@ -44,3 +44,38 @@ pub use partition_value_calculator::*;
/// Record batch partition splitter for partitioned tables
pub mod record_batch_partition_splitter;
pub use record_batch_partition_splitter::*;
+
+#[cfg(test)]
+pub(crate) mod test_utils {
Review Comment:
[move test utils to their own file](https://github.com/apache/iceberg-rust/pull/2584/commits/7cd9e26835ba1f6b92cc669b98d289e10495548a)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk merged PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#issuecomment-4989416382
Follow up PR for standard key metadata validation is here https://github.com/apache/iceberg-rust/pull/2834
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [9/N] Read encrypted parquet data-files [iceberg-rust]
Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on PR #2584:
URL: https://github.com/apache/iceberg-rust/pull/2584#issuecomment-4986120410
Thanks @xanderbailey
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org