Add Parquet Modular decryption (read) support + `encryption` flag #6637

rok · 2024-10-28T20:51:18Z

Which issue does this PR close?

This PR is based on branch and an internal patch and aims to provide basic modular decryption support. Partially closes #3511. We decided to split encryption work into a separate PR.

Rationale for this change

See Parquet Modular Encryption support #3511.

What changes are included in this PR?

This introduces AesGcmV1 cypher decryption to ArrowReaderMetadata and ParquetRecordBatchReader. Introduced classes and functions are tested on sample files from parquet-dataset.

Are there any user-facing changes?

Several new classes and method parameters are introduced. If project is compiled without encryption flag changes are not breaking. If encryption flag is on some methods and constructors (e.g. ParquetMetaData::new) will require new parameters which would be a breaking change.

rok · 2024-10-28T20:53:17Z

Currently this is a rough rebase of work done by @ggershinsky. As ParquetMetaDataReader is now available some refactoring will be required.

etseidl · 2024-10-28T22:50:19Z

As ParquetMetaDataReader is now available some refactoring will be required.

@rok let me know if you want any help shoehorning this into ParquetMetaDataReader.

brainslush · 2024-11-15T09:29:23Z

Is there any help, input or contribution needed here?

rok · 2024-11-21T00:15:17Z

Thanks for the offer @etseidl & @brainslush! I'm making some progress and would definitely appreciate a review! I'll ping once I push.

rok · 2024-12-04T13:30:31Z

As ParquetMetaDataReader is now available some refactoring will be required.

@rok let me know if you want any help shoehorning this into ParquetMetaDataReader.

@etseidl could you please do a quick pass to say if this makes sense in respect to ParquetMetaDataReader?
I'll continue with data decryption.

etseidl

Only looking at the metadata bits for now...looks good to me so far. Just a few minor nits. Thanks @rok!

parquet/src/file/footer.rs

parquet/src/file/metadata/reader.rs

parquet/src/file/serialized_reader.rs

parquet/src/arrow/arrow_reader/mod.rs

…encryption module later and we'll have to name it encrypt to not clash with the super module name (encryption). It would be odd to have sub modules called encrypt and decryption.

…are provided or where encyption is disabled but file is encrypted.

.github/workflows/parquet.yml

rok · 2025-03-10T17:42:34Z

Thanks for the review @adamreeve, I think I addressed all the points, feel free to do another pass.

alamb

Thank you for this @rok -- I think it is quite close.

I went over the PR quite carefully and only real concern is the API in AsyncFileReader::get_metadata_with_encryption -- I left comments inline. Everything else is minor / could be done as some follow on clean up PR I think

The CI failure here
https://github.com/apache/arrow-rs/actions/runs/13770783611/job/38509760112?pr=6637

Appears unrelated to the changes in this PR as it happens for me on main as well

andrewlamb@Andrews-MacBook-Pro-2:~/Software/arrow-rs/object_store$ cargo msr verify
error: no such command: `msr`

	Did you mean `msrv`?

	View all installed commands with `cargo --list`
	Find a package to install `msr` with `cargo search cargo-msr`
andrewlamb@Andrews-MacBook-Pro-2:~/Software/arrow-rs/object_store$ cargo msrv verify
  [Meta]   cargo-msrv 0.17.1

Compatibility Check #1: Rust 1.64.0
  [FAIL]   Is incompatible

  ╭──────────────────────────────────────────────────────────────────────────────────────────────────────╮
  │ error: failed to parse lock file at: /Users/andrewlamb/Software/arrow-rs/object_store/Cargo.lock     │
  │                                                                                                      │
  │ Caused                                                                                               │
  │ by:                                                                                                  │
  │   lock file version `4` was found, but this version of Cargo does not understand this lock file, per │
  │ haps Cargo needs to be updated?                                                                      │
  │                                                                                                      │
  ╰──────────────────────────────────────────────────────────────────────────────────────────────────────╯



Crate source was found to be incompatible with Rust version '1.64.0' specified as MSRV in the Cargo manifest located at '/Users/andrewlamb/Software/arrow-rs/object_store/Cargo.toml'andrewlamb@Andrews-MacBook-Pro-2:~/Software/arrow-rs/object_store$

I also made a PR to test this didn't inadvertently break things downstream and it looks good to me:

WIP: test parquet modular encryption support datafusion#15133

parquet/src/arrow/async_reader/mod.rs

alamb · 2025-03-10T20:06:24Z

parquet/src/arrow/async_reader/mod.rs

+    #[cfg(feature = "encryption")]
+    fn get_metadata_with_encryption(
+        &mut self,
+        file_decryption_properties: Option<FileDecryptionProperties>,


I don't understand this new API and it seems overly specific to the encryption feature.

It seems like the need is to pass file_decryption_properties to the underlying ParquetMetadataReader but there are other potential options on ArrowReaderOptionsthat might be useful.

What if instead we added a new method like this:

/// Provides asynchronous access to the [`ParquetMetaData`] of a parquet file, /// allowing fine-grained control over how metadata is sourced, in particular allowing /// for caching, pre-fetching, catalog metadata, etc... /// /// By default calls `get_metadata()` fn get_metadata_with_options(&mut self, options: &ArrowReaderOptions) -> BoxFuture<'_, Result<Arc<ParquetMetaData>>> { self.get_metadata() }

I think that would

Be backwards compatible

Permit access to the encryption options necessary to decrype the data

Allow us eventually to clean up ArrowReaderMetadata::load_async function (in some other PR)

As a follow on PR we could potentially deprecate get_metadata and direct people to implement get_metadata_with_options

That does indeed seem like the better API! Changed in 90434d6

I've pushed some follow up changes (with Rok's blessing) to continue this refactor and fix the async metadata loading with an ObjectStore. I think this is addressed now.

alamb · 2025-03-10T20:16:34Z

parquet/src/arrow/async_reader/mod.rs

@@ -853,6 +929,10 @@ struct InMemoryRowGroup<'a> {
    offset_index: Option<&'a [OffsetIndexMetaData]>,
    column_chunks: Vec<Option<Arc<ColumnChunkData>>>,
    row_count: usize,
+    #[cfg(feature = "encryption")]
+    row_group_ordinal: usize,


Minor: I think row_group_idx would be a name that is more consistent with the rest of this crate (and it is used as the local variable name above too)

Changed to row_group_idx.
I don't have a strong preference, but parquet encryption spec uses ordinal while here we do indeed use idx.

also don't have a strong preference -- as long as the code is consistent with something (the spec would be fine) I am ok with either

parquet/src/arrow/async_reader/mod.rs

parquet/src/arrow/async_reader/store.rs

alamb · 2025-03-10T20:20:24Z

parquet/src/encryption/decrypt.rs

+// decrypt parquet modules (data pages, dictionary pages, etc.).
+#[derive(Debug, Clone)]
+pub(crate) struct CryptoContext {
+    pub(crate) row_group_ordinal: usize,


stylistically, as before I think using idx or index for row group and column group would be more consistent with the existing code but it is not a major item

See #6637 (comment). I'm ok with changing page_ordinal and column_ordinal if that's better for readability.

adamreeve

Thanks Rok, yes all my review comments were addressed, I've just left a few small follow up comments.

parquet/src/arrow/arrow_reader/mod.rs

parquet/src/encryption/decrypt.rs

parquet/src/file/metadata/reader.rs

Co-authored-by: Adam Reeve <[email protected]>

…Change row_group_ordinal to row_group_idx.

…ader load

parquet/src/encryption/mod.rs

alamb · 2025-03-11T18:28:16Z

👁️

github-actions bot added the parquet Changes to the parquet crate label Oct 28, 2024

brainslush mentioned this pull request Nov 18, 2024

Add modular parquet de-/encryption pola-rs/polars#19858

Open

zubieta mentioned this pull request Nov 21, 2024

Client side encryption for cloud storage openobserve/openobserve#5115

Open

rok force-pushed the decryption-basics-fork branch 2 times, most recently from fe488b3 to d263510 Compare November 23, 2024 23:06

etseidl reviewed Dec 4, 2024

View reviewed changes

parquet/src/file/footer.rs Show resolved Hide resolved

parquet/src/file/footer.rs Outdated Show resolved Hide resolved

parquet/src/file/metadata/reader.rs Outdated Show resolved Hide resolved

rok force-pushed the decryption-basics-fork branch from f90d8b4 to 29d55eb Compare December 16, 2024 23:51

rok commented Dec 16, 2024

View reviewed changes

parquet/src/file/serialized_reader.rs Outdated Show resolved Hide resolved

adamreeve reviewed Dec 17, 2024

View reviewed changes

parquet/src/arrow/arrow_reader/mod.rs Outdated Show resolved Hide resolved

adamreeve mentioned this pull request Dec 19, 2024

[C++][Parquet] Encryption test files are generated with invalid repetition levels apache/arrow#45073

Closed

adamreeve force-pushed the decryption-basics-fork branch from 27e77ad to 7db06cc Compare December 20, 2024 02:15

rok force-pushed the decryption-basics-fork branch from a4105d5 to 3e7646d Compare January 9, 2025 21:59

rok force-pushed the decryption-basics-fork branch 4 times, most recently from deedba9 to 951f2fa Compare January 21, 2025 20:35

rok changed the title ~~Parquet Modular Encryption support~~ Parquet Modular decryption support Jan 21, 2025

rok force-pushed the decryption-basics-fork branch 2 times, most recently from f6b9e88 to 23375d1 Compare January 23, 2025 18:17

adamreeve force-pushed the decryption-basics-fork branch 3 times, most recently from 7f94e39 to 177d826 Compare January 24, 2025 02:46

adamreeve mentioned this pull request Jan 24, 2025

AES-CTR support briansmith/ring#2282

Closed

rok force-pushed the decryption-basics-fork branch 2 times, most recently from ac4ac21 to 3241425 Compare February 5, 2025 10:01

rok force-pushed the decryption-basics-fork branch from 9bdf7f0 to a0cea3e Compare March 10, 2025 14:35

Review feedback

7e9e7aa

rok force-pushed the decryption-basics-fork branch from a0cea3e to 7e9e7aa Compare March 10, 2025 14:39

rok added 3 commits March 10, 2025 15:49

Add new CI check

c4737a4

Rename decryption module to decrypt. This is because we'll introduce …

99e2b6d

…encryption module later and we'll have to name it encrypt to not clash with the super module name (encryption). It would be odd to have sub modules called encrypt and decryption.

Fix failing where encryption is enabled but no decryption properties …

d9e597c

…are provided or where encyption is disabled but file is encrypted.

rok force-pushed the decryption-basics-fork branch from d5f2605 to d9e597c Compare March 10, 2025 17:19

rok commented Mar 10, 2025

View reviewed changes

.github/workflows/parquet.yml Show resolved Hide resolved

This was referenced Mar 10, 2025

Parquet Modular Encryption support #3511

Open

WIP: test parquet modular encryption support apache/datafusion#15133

Draft

rok pushed a commit to rok/arrow-rs that referenced this pull request Mar 10, 2025

Rebased on apache/pull/6637

f6ff1d7

alamb reviewed Mar 10, 2025

View reviewed changes

adamreeve reviewed Mar 10, 2025

View reviewed changes

parquet/src/arrow/arrow_reader/mod.rs Outdated Show resolved Hide resolved

parquet/src/arrow/arrow_reader/mod.rs Outdated Show resolved Hide resolved

parquet/src/encryption/decrypt.rs Outdated Show resolved Hide resolved

parquet/src/file/metadata/reader.rs Outdated Show resolved Hide resolved

rok and others added 5 commits March 11, 2025 01:13

Apply suggestions from code review

cc27421

Co-authored-by: Adam Reeve <[email protected]>

get_metadata_with_encryption -> get_metadata_with_options

90434d6

Use ParquetMetaData instead of RowGroupMetaData in InMemoryRowGroup. …

5273244

…Change row_group_ordinal to row_group_idx.

Review feedback

d8fd4f2

Fixes

67b8cb9

rok force-pushed the decryption-basics-fork branch from 9531491 to 67b8cb9 Compare March 11, 2025 01:05

adamreeve added 2 commits March 11, 2025 15:31

Continue refactor away from encryption specific APIs and fix async re…

3491930

…ader load

Add default get_metadata_with_options implementation

07ad9dd

adamreeve approved these changes Mar 11, 2025

View reviewed changes

Minor tidy ups and test fix

e58ae7c

adamreeve force-pushed the decryption-basics-fork branch from f33c96c to e58ae7c Compare March 11, 2025 03:08

adamreeve reviewed Mar 11, 2025

View reviewed changes

parquet/src/encryption/mod.rs Outdated Show resolved Hide resolved

rok pushed a commit to rok/arrow-rs that referenced this pull request Mar 11, 2025

Rebased on apache/pull/6637

2917399

Update parquet/src/encryption/mod.rs

65f9bc4

rok requested a review from alamb March 11, 2025 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Parquet Modular decryption (read) support + `encryption` flag #6637

Add Parquet Modular decryption (read) support + `encryption` flag #6637

rok commented Oct 28, 2024 •

edited by alamb

Loading

rok commented Oct 28, 2024

etseidl commented Oct 28, 2024

brainslush commented Nov 15, 2024

rok commented Nov 21, 2024

rok commented Dec 4, 2024

etseidl left a comment

rok commented Mar 10, 2025

alamb left a comment

alamb Mar 10, 2025

rok Mar 11, 2025

adamreeve Mar 11, 2025 •

edited

Loading

alamb Mar 10, 2025

rok Mar 11, 2025

alamb Mar 11, 2025

alamb Mar 10, 2025

rok Mar 11, 2025 •

edited

Loading

adamreeve left a comment

alamb commented Mar 11, 2025

Add Parquet Modular decryption (read) support + encryption flag #6637

Are you sure you want to change the base?

Add Parquet Modular decryption (read) support + encryption flag #6637

Conversation

rok commented Oct 28, 2024 • edited by alamb Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

rok commented Oct 28, 2024

etseidl commented Oct 28, 2024

brainslush commented Nov 15, 2024

rok commented Nov 21, 2024

rok commented Dec 4, 2024

etseidl left a comment

Choose a reason for hiding this comment

rok commented Mar 10, 2025

alamb left a comment

Choose a reason for hiding this comment

alamb Mar 10, 2025

Choose a reason for hiding this comment

rok Mar 11, 2025

Choose a reason for hiding this comment

adamreeve Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

alamb Mar 10, 2025

Choose a reason for hiding this comment

rok Mar 11, 2025

Choose a reason for hiding this comment

alamb Mar 11, 2025

Choose a reason for hiding this comment

alamb Mar 10, 2025

Choose a reason for hiding this comment

rok Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

adamreeve left a comment

Choose a reason for hiding this comment

alamb commented Mar 11, 2025

Add Parquet Modular decryption (read) support + `encryption` flag #6637

Add Parquet Modular decryption (read) support + `encryption` flag #6637

rok commented Oct 28, 2024 •

edited by alamb

Loading

adamreeve Mar 11, 2025 •

edited

Loading

rok Mar 11, 2025 •

edited

Loading