[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

Are these changes tested?

Yes, unit test and on AES-256 encrypted Parquet files.

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?

mbutrovich · 2026-01-27T20:18:25Z

parquet/src/encryption/ciphers.rs

 use crate::errors::Result;
 use crate::file::metadata::HeapSize;
-use ring::aead::{AES_128_GCM, Aad, LessSafeKey, NonceSequence, UnboundKey};
+use ring::aead::{AES_128_GCM, Aad, LessSafeKey, NonceSequence, UnboundKey, Algorithm};


Did this have cargo fmt run on it? The order looks off to me.

mbutrovich · 2026-01-27T20:19:20Z

parquet/src/encryption/decrypt.rs

    keys: DecryptionKeys,
    aad_prefix: Option<Vec<u8>>,
    footer_signature_verification: bool,
+    algorithm: &'static Algorithm


Why static?

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

parquet/src/encryption/ciphers.rs

 }

 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

parquet/src/file/metadata/mod.rs


+        #[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

parquet/src/encryption/ciphers.rs

+    #[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

parquet/src/encryption/decrypt.rs

    }

+    /// 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

parquet/src/encryption/ciphers.rs

+    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

parquet/src/encryption/ciphers.rs

+    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

parquet/src/encryption/decrypt.rs

    }

+    /// 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.

adamreeve · 2026-02-11T21:45:44Z

parquet/tests/encryption/encryption_async.rs

@@ -115,7 +115,7 @@ async fn test_misspecified_encryption_keys() {

    // Too short footer key
    check_for_error(


Could you please add some higher level tests for round-tripping files with larger key sizes? It might be beneficial to other Parquet implementations too if you could add files that use larger keys to the parquet-testing repository.

@adamreeve Will do, thanks!

Allow more encryption algorithms

c017804

github-actions bot 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

Open

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

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 algorithms Feb 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

		@@ -115,7 +115,7 @@ async fn test_misspecified_encryption_keys() {

		// Too short footer key
		check_for_error(

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Feb 3, 2026

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamreeve Feb 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamreeve Feb 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hsiang-c commented Jan 16, 2026 •

edited

Loading

adamreeve Feb 11, 2026 •

edited

Loading

adamreeve Feb 11, 2026 •

edited

Loading