You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "rok (via GitHub)" <gi...@apache.org> on 2025/06/04 01:09:28 UTC
[PR] When writing parquet plaintext footer files footer_signing_key_metadata is not included, encryption alghoritm is always written in footer [arrow-rs]
rok opened a new pull request, #7600:
URL: https://github.com/apache/arrow-rs/pull/7600
# Which issue does this PR close?
Closes #7599.
# Rationale for this change
Written plaintext footer file will not include `footer_signing_key_metadata`, see proposed test for reproduction.
Written encrypted non-plaintext footer files shouldn't include `encryption_algorithm`, see proposed test for reproduction.
# What changes are included in this PR?
`footer_signing_key_metadata` is now included in plaintext footer file and `encryption_algorithm` is not included in the footer if footer is non-plaintext.
# Are there any user-facing changes?
This doesn't change user facing API.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Include footer key metadata when writing encrypted Parquet with a plaintext footer [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7600:
URL: https://github.com/apache/arrow-rs/pull/7600
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] When writing parquet plaintext footer files footer_signing_key_metadata is not included, encryption alghoritm is always written in footer [arrow-rs]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #7600:
URL: https://github.com/apache/arrow-rs/pull/7600#discussion_r2126975773
##########
parquet/tests/encryption/encryption.rs:
##########
@@ -256,6 +256,72 @@ fn test_non_uniform_encryption_plaintext_footer_with_key_retriever() {
verify_encryption_test_file_read(file, decryption_properties);
}
+#[test]
+fn test_uniform_encryption_plaintext_footer_with_key_retriever() {
+ let test_data = arrow::util::test_util::parquet_test_data();
+ let path = format!("{test_data}/encrypt_columns_plaintext_footer.parquet.encrypted");
+ let file = File::open(path).unwrap();
+ let temp_file = tempfile::tempfile().unwrap();
+
+ let key_retriever = TestKeyRetriever::new()
+ .with_key("kf".to_owned(), b"0123456789012345".to_vec())
+ .with_key("kc1".to_owned(), b"1234567890123450".to_vec())
+ .with_key("kc2".to_owned(), b"1234567890123451".to_vec());
+
+ let encryption_properties = FileEncryptionProperties::builder(b"0123456789012345".to_vec())
Review Comment:
Indeed, moved things into a more logical order and added comments.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] When writing parquet plaintext footer files footer_signing_key_metadata is not included, encryption alghoritm is always written in footer [arrow-rs]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #7600:
URL: https://github.com/apache/arrow-rs/pull/7600#discussion_r2126976711
##########
parquet/tests/encryption/encryption.rs:
##########
@@ -256,6 +256,72 @@ fn test_non_uniform_encryption_plaintext_footer_with_key_retriever() {
verify_encryption_test_file_read(file, decryption_properties);
}
+#[test]
+fn test_uniform_encryption_plaintext_footer_with_key_retriever() {
+ let test_data = arrow::util::test_util::parquet_test_data();
+ let path = format!("{test_data}/encrypt_columns_plaintext_footer.parquet.encrypted");
+ let file = File::open(path).unwrap();
+ let temp_file = tempfile::tempfile().unwrap();
+
+ let key_retriever = TestKeyRetriever::new()
+ .with_key("kf".to_owned(), b"0123456789012345".to_vec())
+ .with_key("kc1".to_owned(), b"1234567890123450".to_vec())
+ .with_key("kc2".to_owned(), b"1234567890123451".to_vec());
+
+ let encryption_properties = FileEncryptionProperties::builder(b"0123456789012345".to_vec())
+ .with_footer_key_metadata("kf".into())
+ .with_column_key_and_metadata("double_field", b"1234567890123450".to_vec(), b"kc1".into())
+ .with_column_key_and_metadata("float_field", b"1234567890123451".to_vec(), b"kc2".into())
+ .with_plaintext_footer(true)
+ .build()
+ .unwrap();
+
+ let decryption_properties =
+ FileDecryptionProperties::with_key_retriever(Arc::new(key_retriever))
+ .build()
+ .unwrap();
+
+ let options = ArrowReaderOptions::default()
+ .with_file_decryption_properties(decryption_properties.clone());
+ let metadata = ArrowReaderMetadata::load(&file, options.clone()).unwrap();
+
+ let builder = ParquetRecordBatchReaderBuilder::try_new_with_options(file, options).unwrap();
+ let batch_reader = builder.build().unwrap();
+ let batches = batch_reader
+ .collect::<parquet::errors::Result<Vec<RecordBatch>, _>>()
+ .unwrap();
+
+ let props = WriterProperties::builder()
+ .with_file_encryption_properties(encryption_properties)
+ .build();
+
+ let mut writer = ArrowWriter::try_new(
+ temp_file.try_clone().unwrap(),
+ metadata.schema().clone(),
+ Some(props),
+ )
+ .unwrap();
+ for batch in batches {
+ writer.write(&batch).unwrap();
+ }
+
+ writer.close().unwrap();
+
+ let key_retriever = TestKeyRetriever::new()
Review Comment:
Oh, wasn't aware of that mechanism. Done.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] When writing parquet plaintext footer files footer_signing_key_metadata is not included, encryption alghoritm is always written in footer [arrow-rs]
Posted by "adamreeve (via GitHub)" <gi...@apache.org>.
adamreeve commented on code in PR #7600:
URL: https://github.com/apache/arrow-rs/pull/7600#discussion_r2125257704
##########
parquet/src/file/metadata/writer.rs:
##########
@@ -635,11 +636,17 @@ impl MetadataObjectWriter {
}
}
- fn get_footer_encryption_algorithm(&self) -> Option<EncryptionAlgorithm> {
- if let Some(file_encryptor) = &self.file_encryptor {
- return Some(Self::encryption_algorithm_from_encryptor(file_encryptor));
+ fn get_plaintext_footer_metadata(&self) -> (Option<EncryptionAlgorithm>, Option<Vec<u8>>) {
Review Comment:
Nitpicking but I think this should have something encryption related in the name even though it's already quite long... How about `get_plaintext_footer_crypto_metadata`? Or it could be `get_footer_crypto_metadata` and have a comment that the footer only contains encryption metadata when using plaintext footers?
##########
parquet/tests/encryption/encryption.rs:
##########
@@ -256,6 +256,72 @@ fn test_non_uniform_encryption_plaintext_footer_with_key_retriever() {
verify_encryption_test_file_read(file, decryption_properties);
}
+#[test]
+fn test_uniform_encryption_plaintext_footer_with_key_retriever() {
+ let test_data = arrow::util::test_util::parquet_test_data();
+ let path = format!("{test_data}/encrypt_columns_plaintext_footer.parquet.encrypted");
+ let file = File::open(path).unwrap();
+ let temp_file = tempfile::tempfile().unwrap();
+
+ let key_retriever = TestKeyRetriever::new()
+ .with_key("kf".to_owned(), b"0123456789012345".to_vec())
+ .with_key("kc1".to_owned(), b"1234567890123450".to_vec())
+ .with_key("kc2".to_owned(), b"1234567890123451".to_vec());
+
+ let encryption_properties = FileEncryptionProperties::builder(b"0123456789012345".to_vec())
Review Comment:
This test is a bit confusing to follow. Eg. `temp_file` and these encryption properties aren't used until later when we rewrite the data. Could we move them down and define them closer to where they're used, and maybe add some brief comments about what's going on?
##########
parquet/tests/encryption/encryption.rs:
##########
@@ -256,6 +256,72 @@ fn test_non_uniform_encryption_plaintext_footer_with_key_retriever() {
verify_encryption_test_file_read(file, decryption_properties);
}
+#[test]
+fn test_uniform_encryption_plaintext_footer_with_key_retriever() {
+ let test_data = arrow::util::test_util::parquet_test_data();
+ let path = format!("{test_data}/encrypt_columns_plaintext_footer.parquet.encrypted");
+ let file = File::open(path).unwrap();
+ let temp_file = tempfile::tempfile().unwrap();
+
+ let key_retriever = TestKeyRetriever::new()
+ .with_key("kf".to_owned(), b"0123456789012345".to_vec())
+ .with_key("kc1".to_owned(), b"1234567890123450".to_vec())
+ .with_key("kc2".to_owned(), b"1234567890123451".to_vec());
+
+ let encryption_properties = FileEncryptionProperties::builder(b"0123456789012345".to_vec())
+ .with_footer_key_metadata("kf".into())
+ .with_column_key_and_metadata("double_field", b"1234567890123450".to_vec(), b"kc1".into())
+ .with_column_key_and_metadata("float_field", b"1234567890123451".to_vec(), b"kc2".into())
+ .with_plaintext_footer(true)
+ .build()
+ .unwrap();
+
+ let decryption_properties =
+ FileDecryptionProperties::with_key_retriever(Arc::new(key_retriever))
+ .build()
+ .unwrap();
+
+ let options = ArrowReaderOptions::default()
+ .with_file_decryption_properties(decryption_properties.clone());
+ let metadata = ArrowReaderMetadata::load(&file, options.clone()).unwrap();
+
+ let builder = ParquetRecordBatchReaderBuilder::try_new_with_options(file, options).unwrap();
+ let batch_reader = builder.build().unwrap();
+ let batches = batch_reader
+ .collect::<parquet::errors::Result<Vec<RecordBatch>, _>>()
+ .unwrap();
+
+ let props = WriterProperties::builder()
+ .with_file_encryption_properties(encryption_properties)
+ .build();
+
+ let mut writer = ArrowWriter::try_new(
+ temp_file.try_clone().unwrap(),
+ metadata.schema().clone(),
+ Some(props),
+ )
+ .unwrap();
+ for batch in batches {
+ writer.write(&batch).unwrap();
+ }
+
+ writer.close().unwrap();
+
+ let key_retriever = TestKeyRetriever::new()
Review Comment:
Rather than creating a new `key_retriever` here, we could create it inside an `Arc` initially and then clone 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] When writing parquet plaintext footer files footer_signing_key_metadata is not included, encryption alghoritm is always written in footer [arrow-rs]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #7600:
URL: https://github.com/apache/arrow-rs/pull/7600#discussion_r2126972583
##########
parquet/src/file/metadata/writer.rs:
##########
@@ -635,11 +636,17 @@ impl MetadataObjectWriter {
}
}
- fn get_footer_encryption_algorithm(&self) -> Option<EncryptionAlgorithm> {
- if let Some(file_encryptor) = &self.file_encryptor {
- return Some(Self::encryption_algorithm_from_encryptor(file_encryptor));
+ fn get_plaintext_footer_metadata(&self) -> (Option<EncryptionAlgorithm>, Option<Vec<u8>>) {
Review Comment:
Good point! Went with `get_plaintext_footer_crypto_metadata` and comment:
```rust
// Only plaintext footers may contain encryption algorithm and footer key metadata.
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org