[parquet] Allow more encryption algorithms by hsiang-c · Pull Request #9203 · apache/arrow-rs

hsiang-c · 2026-01-16T07:20:16Z

Which issue does this PR close?

Closes[Parquet] Support other encryption/decryption key size #9202.

Rationale for this change

Iceberg spec supports AES key sizes of 128, 192 and 256 bits. Iceberg Rust depends on arrow-rs for Parquet I/O, I'd like to start supporting AES 256 with this PR.

What changes are included in this PR?

RingGcmBlockEncryptor and RingGcmBlockDecryptor will pick AES-128 or AES-256 based on key size
Refactor encryption_async.rs and encryption.rs to test both AES-128 and AES-256 encrypted parquet files

Are these changes tested?

Yes, unit test and on AES-256 encrypted Parquet files defined in https://github.com/apache/parquet-testing/tree/master/data/aes256

Are there any user-facing changes?

No

xanderbailey

When I exposed quote style here I exported the underlying QuoteStyle is that something we might want to do here also for Algorithm?

hsiang-c · 2026-01-23T23:42:04Z

@xanderbailey

Thank you for the review, I took a look at your PR. You made QuoteStyle part of Writer and expose as part of the API.

Do you mean you'd like to make Algorithm a field of RingGcmBlockDecryptor and RingGcmBlockEncryptor and expose them instead of importing it from ring::aead?

alamb · 2026-02-03T22:07:21Z

Thanks @hsiang-c and @mbutrovich -- looks like there are some CI failures on this PR.

Also, it seems like we should have some tests for the new features

alamb

Thanks @hsiang-c

FYI @adamreeve and @rok as the original authors of the parquet encryption feature

Perhaps you have some time to review this PR?

alamb · 2026-02-11T19:43:19Z

 }

 impl RingGcmBlockDecryptor {
+ #[allow(dead_code)]


why do we need to allow dead code? Maybe we should remove it instead?

alamb · 2026-02-11T19:46:26Z


+ #[cfg(feature = "encryption")]
+ let expected_size_with_decryptor = 3080;
+ #[cfg(not(feature = "encryption"))]


Why is this needed? The whole test is gated by
#[cfg(feature = "encryption")]fn test_memory_size_with_decryptor(){
it seems like you just need to update the 3072 to 3080 to reflect the additional size?

alamb · 2026-02-11T19:47:07Z

+ #[test]
+ fn test_round_trip_with_incorrect_key_length() {
+ let key = [0u8; 16];
+ assert!(RingGcmBlockEncryptor::new_with_algorithm(&CHACHA20_POLY1305, &key).is_err());


It isn't clear to me that CHACHA is a valid Parquet encryption: https://github.com/apache/parquet-format/blob/master/Encryption.md :

alamb · 2026-02-11T19:48:13Z

 }

+ /// The AEAD decryption algorithm to be used.
+ pub fn with_algorithm(mut self, algorithm: &'static Algorithm) -> Self {


this effectively makes Algorithm a part of the public API I think -- this might be ok, but it also means we would not be able to change the underlying crypto library without breaking the API in the future

Yes I don't like this and I don't think it's necessary, see my comment on new_with_algorithm. This also makes it possible for users to easily create Parquet files that aren't compliant with the spec by using a non-standard algorithm.
Changing the crypto library is something we might potentially want to do to be able to add AES_GCM_CTR support for example (#7258 (comment)).
I think if we want to support new algorithms like AES_GCM_CTR in the future the user should provide an enum value from a supported set of algorithms like is done in the C++ implementation, but we can infer the key length from the provided key so this isn't necessary for this change.

adamreeve

Thanks for addressing this @hsiang-c! I've left a few comments

adamreeve · 2026-02-11T21:35:29Z

+ pub(crate) fn new_with_algorithm(
+ algorithm: &'static Algorithm,
+ key_bytes: &[u8],
+ ) -> Result<Self> {


I don't think there's any need to add this new constructor, we can reuse the existing new method above and change the algorithm based on the length of the key.
This is also a bit semantically confusing as you could create a new "RingGcmBlockDecryptor" with a non-GCM algorithm.

adamreeve · 2026-02-11T21:38:54Z

+ pub(crate) fn new_with_algorithm(
+ algorithm: &'static Algorithm,
+ key_bytes: &[u8],
+ ) -> Result<Self> {


As above, I think we should reuse the existing new method.

adamreeve · 2026-02-11T21:42:50Z

 }

+ /// The AEAD decryption algorithm to be used.
+ pub fn with_algorithm(mut self, algorithm: &'static Algorithm) -> Self {


Yes I don't like this and I don't think it's necessary, see my comment on new_with_algorithm. This also makes it possible for users to easily create Parquet files that aren't compliant with the spec by using a non-standard algorithm.
Changing the crypto library is something we might potentially want to do to be able to add AES_GCM_CTR support for example (#7258 (comment)).
I think if we want to support new algorithms like AES_GCM_CTR in the future the user should provide an enum value from a supported set of algorithms like is done in the C++ implementation, but we can infer the key length from the provided key so this isn't necessary for this change.

tom-s-powell · 2026-02-18T10:07:25Z

Curious if there's a rough timeline on getting this change in?

tom-s-powell

Any chance that this'll get reviewed?

adamreeve · 2026-02-25T20:24:49Z

Any chance that this'll get reviewed?

This isn't ready for another review yet. The tests are currently failing because they need apache/parquet-testing#102. I'm happy to review this again once @hsiang-c says it's ready.

alamb · 2026-02-27T19:49:56Z

Marking as draft as I think this PR is no longer waiting on feedback and I am trying to make it easier to find PRs in need of review. Please mark it as ready for review when it is ready for another look

hsiang-c · 2026-05-04T18:30:07Z

@adamreeve I reverted some unnecessary tests and refactored the test data a bit, please take a look when you have time, thank you.

adamreeve

Thanks @hsiang-c, the test changes look good to me. This is still a bit complicated by the int64_field.list.int64_field issue, but I think that's OK for now.

hsiang-c · 2026-05-05T18:46:45Z

Thanks @adamreeve I'll fix the test data in parquet-testing and remove int64_field.list.int64_field in the tests.

alamb · 2026-05-07T18:02:40Z

Marking as draft as I think this PR is no longer waiting on feedback and I am trying to make it easier to find PRs in need of review. Please mark it as ready for review when it is ready for another look

Or if you prefer to merge it now, let me know and we can do that

hsiang-c · 2026-05-14T20:25:20Z

@alamb This is ready for your review, thank you.

alamb

looks good to me -- thnks @adamreeve and @hsiang-c

# Which issue does this PR close?  - Closesapache#9202. # Rationale for this change  - Iceberg [spec](https://iceberg.apache.org/gcm-stream-spec/#encryption-algorithm) supports AES key sizes of 128, 192 and 256 bits. Iceberg Rust depends on `arrow-rs` for Parquet I/O, I'd like to start supporting AES 256 with this PR. # What changes are included in this PR?  - `RingGcmBlockEncryptor` and `RingGcmBlockDecryptor` will pick AES-128 or AES-256 based on key size - Refactor `encryption_async.rs` and `encryption.rs` to test both AES-128 and AES-256 encrypted parquet files # Are these changes tested?  Yes, unit test and on AES-256 encrypted Parquet files defined in https://github.com/apache/parquet-testing/tree/master/data/aes256 # Are there any user-facing changes?  No

@hsiang-c

…10351) - Part of #10349 - Fixes#9202 in 58.x releases This PR: - Backports #9203 from @hsiang-c to the `58_maintenance` line Co-authored-by: hsiang-c <137842490+hsiang-c@users.noreply.github.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

…#2878) ## Which issue does this PR close? - Closes #. Depends on the arrow-rs 58.4.0 release tracked in apache/arrow-rs#10349. ## What changes are included in this PR? - Bump `arrow`/`parquet` deps from `58` to `58.4`. arrow-rs 58.4.0 adds AES-256 for Parquet modular encryption (58.3.0 was AES-128 only), landed via apache/arrow-rs#9203. - Add an AES-256 encrypted-Parquet read test. Refactored the existing 128-bit test into a shared `assert_encrypted_parquet_roundtrip(key)` helper with `_aes_128` and `_aes_256` callers. Note: 192-bit is not supported for data files. arrow-rs uses `ring`, which has no AES-192-GCM. Can revisit later. ## Are these changes tested? Yes. Unit test `test_read_encrypted_parquet_aes_256` writes and reads back a 256-bit encrypted Parquet file. It fails on arrow-rs 58.3.0 (`ring` rejects the 32-byte key) and passes on 58.4.0.

Allow more encryption algorithms
c017804

github-actionsBot added the parquet Changes to the parquet crate label Jan 16, 2026

hsiang-c mentioned this pull request Jan 16, 2026
feat(encryption) [1/N] Support encryption: Add crypto for AES-GCM apache/iceberg-rust#2026
Merged

xanderbailey reviewed Jan 16, 2026
View reviewed changes

Merge branch 'main' into allow_more_enc_algo
1d12eb4

hsiang-c marked this pull request as ready for review January 23, 2026 07:27

hsiang-c added 2 commits January 27, 2026 11:20

Expose new API
e8da4b9

Fix API
3e1b8a7

mbutrovich reviewed Jan 27, 2026
View reviewed changes

Comment threadparquet/src/encryption/ciphers.rs Outdated

mbutrovich reviewed Jan 27, 2026
View reviewed changes

Comment threadparquet/src/encryption/decrypt.rs Outdated

hsiang-c added 2 commits February 4, 2026 20:36

Fix format and clippy
1f1f0da

Document new api
92b4094

alamb changed the title ~~Allow more encryption algorithms~~[parquet] Allow more encryption algorithmsFeb 11, 2026

alamb reviewed Feb 11, 2026
View reviewed changes

adamreeve reviewed Feb 11, 2026
View reviewed changes

hsiang-c added 2 commits February 13, 2026 16:52

Pick algorithm by key size
12c1888

Remove unnecessary tests
3ec6b70

hsiang-c marked this pull request as draft February 14, 2026 00:58

hsiang-c mentioned this pull request Feb 19, 2026
Added AES-256 encrypted parquet generated from parquet-java apache/parquet-testing#102
Merged

Round-trip tests w/ AES-256 encrypted parquet
deec81e

hsiang-c marked this pull request as ready for review February 19, 2026 00:52

hsiang-cand others added 2 commits February 18, 2026 17:26

Merge branch 'main' into allow_more_enc_algo
35b482f

Fix comment
f55fe3d

tom-s-powell reviewed Feb 25, 2026
View reviewed changes

rok reviewed Feb 25, 2026
View reviewed changes

Comment threadparquet/tests/encryption/encryption_util.rs Outdated

hsiang-cand others added 6 commits May 1, 2026 16:06

Revert tests
1b3f2f7

Fix comments
5b8b244

Merge branch 'apache:main' into allow_more_enc_algo
96ad92d

Extract hardcoded strings to constants
f414b3e

Clean up
4336eed

Revert uncessary changes
44caecc

hsiang-c force-pushed the allow_more_enc_algo branch from 2fe8a54 to 44caeccCompare May 4, 2026 17:41

hsiang-c added 2 commits May 4, 2026 11:17

Extract common data
12d768d

Restore one test
06c6b73

hsiang-c marked this pull request as ready for review May 4, 2026 18:28

adamreeve approved these changes May 5, 2026
View reviewed changes

hsiang-c mentioned this pull request May 11, 2026
Use the LIST 3-level structure apache/parquet-testing#106
Merged

hsiang-cand others added 3 commits May 11, 2026 14:12

Remove the workaround
a91f2f9

Update test data
ec912ab

Merge branch 'main' into allow_more_enc_algo
28f3ab2

alamb approved these changes May 15, 2026
View reviewed changes

xanderbailey mentioned this pull request May 17, 2026
Release arrow-rs / parquet Major version 59.0.0 (May 2026) #9110
Closed
12 tasks

alamb merged commit accb1cf into apache:mainMay 20, 2026
17 checks passed

nibix mentioned this pull request Jun 16, 2026
[RFC] Cryptographic design decisions for integration of Parquet Modular Encryption opensearch-project/OpenSearch#22183
Open

adamreeve mentioned this pull request Jul 1, 2026
Fix panic on short wrapped keys and silently ignored DataFusion options G-Research/parquet-key-management-rs#43
Merged

This was referenced Jul 15, 2026
Release arrow-rs / parquet Minor/Patch version 58.4.0 (July 2026) #10349
Closed
[58_maintenance] [parquet] Allow more encryption algorithms (#9203) #10351
Merged

mbutrovich mentioned this pull request Jul 22, 2026
feat(encryption): support AES-256 keys for reading Parquet data files apache/iceberg-rust#2878
Merged

Uh oh!

Conversation

hsiang-c commented Jan 16, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

xanderbailey left a comment

Choose a reason for hiding this comment

Uh oh!

hsiang-c commented Jan 23, 2026

Uh oh!

Uh oh!

Uh oh!

alamb commented Feb 3, 2026

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alambFeb 11, 2026

Choose a reason for hiding this comment

Uh oh!

alambFeb 11, 2026

Choose a reason for hiding this comment

Uh oh!

alambFeb 11, 2026

Choose a reason for hiding this comment

Uh oh!

alambFeb 11, 2026

Choose a reason for hiding this comment

Uh oh!

adamreeveFeb 11, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamreeve left a comment

Choose a reason for hiding this comment

Uh oh!

adamreeveFeb 11, 2026

Choose a reason for hiding this comment

Uh oh!

adamreeveFeb 11, 2026

Choose a reason for hiding this comment

Uh oh!

adamreeveFeb 11, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tom-s-powell commented Feb 18, 2026

Uh oh!

tom-s-powell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adamreeve commented Feb 25, 2026

Uh oh!

alamb commented Feb 27, 2026

Uh oh!

hsiang-c commented May 4, 2026

Uh oh!

adamreeve left a comment

Choose a reason for hiding this comment

Uh oh!

hsiang-c commented May 5, 2026

Uh oh!

alamb commented May 7, 2026

Uh oh!

hsiang-c commented May 14, 2026

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

hsiang-c commented Jan 16, 2026•
edited
Loading

adamreeveFeb 11, 2026•
edited
Loading

adamreeveFeb 11, 2026•
edited
Loading