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/01/14 17:42:54 UTC

[PR] Add crypto for AES-GCM [iceberg-rust]

xanderbailey opened a new pull request, #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026

   Add Core Encryption Primitives for Iceberg Encryption Support
   
   ## Summary
   
   This PR introduces the foundational cryptographic primitives needed for implementing encryption in iceberg-rust, providing AES-GCM encryption operations that match the Java implementation's behavior and data format.
   
    ## Motivation
   
   Iceberg's Java implementation supports table-level encryption to protect sensitive data at rest. To achieve feature parity and ensure interoperability between Java and Rust implementations, we need to build encryption support from the ground up. This PR provides the core cryptographic operations that will serve as the foundation for the complete encryption feature.
   
    ## Changes
   
     New Module: encryption
   
     Added a new encryption module with core AES-GCM cryptographic operations:
   
     - encryption/crypto.rs - Core encryption implementation
       - EncryptionAlgorithm enum supporting AES-128-GCM and AES-256-GCM
       - SecureKey struct with automatic memory zeroization for security
       - AesGcmEncryptor providing encrypt/decrypt operations with AAD support
   
     Key Features
   
     1. Java-Compatible Format: Ciphertext format matches Java's implementation exactly:
     [12-byte nonce][encrypted data][16-byte GCM authentication tag]
     1. This ensures files encrypted by Java can be decrypted by Rust and vice versa.
     2. Secure Key Handling: Uses the zeroize crate to automatically clear encryption keys from memory when dropped, preventing key material from lingering in memory.
     3. Additional Authenticated Data (AAD): Full support for AAD to ensure integrity of associated metadata that isn't encrypted.
     4. Comprehensive Testing: 8 tests covering:
       - Round-trip encryption/decryption for both AES-128 and AES-256
       - AAD validation
       - Empty plaintext handling
       - Tamper detection
       - Format compatibility verification
   
     Dependencies Added
   
     - aes-gcm = "0.10" - Industry-standard AES-GCM implementation
     - zeroize = "1.7" - Secure memory cleanup for encryption keys
   
     Compatibility
   
     This implementation directly corresponds to Java's https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/encryption/Ciphers.java:
   
     | Java Class                  | Rust Implementation                      |
     |-----------------------------|------------------------------------------|
     | Ciphers.AesGcmEncryptor     | AesGcmEncryptor::encrypt()               |
     | Ciphers.AesGcmDecryptor     | AesGcmEncryptor::decrypt()               |
     | EncryptionAlgorithm.AES_GCM | EncryptionAlgorithm::Aes128Gcm/Aes256Gcm |
   
     Testing
   
     Future Work
   
     This PR is the first in a series to implement full encryption support. Upcoming PRs will add:
   
     1. Table properties for encryption configuration
     2. Key management interfaces (KeyManagementClient trait)
     3. EncryptionManager implementation
     4. Native Parquet encryption integration
     5. AWS KMS support
     6. Integration with Table and FileIO
   
     Review Notes
   
     - This PR is intentionally minimal and self-contained
     - No existing code paths are modified - this is purely additive
     - The module is public but won't be used until future PRs wire it up
     - Format compatibility with Java has been prioritized to ensure interoperability
   
   
   ## 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.
   -->
   
   - Closes #.
   
   ## 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?
   Yes
   <!--
   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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2819122906


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,383 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use rand::RngCore;
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,

Review Comment:
   Correct, the crate has both! Designed to be extended once the arrow PR merges 



-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2819122906


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,383 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use rand::RngCore;
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,

Review Comment:
   Correct, the crate has both! 



-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2978520299


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,533 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::U12;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Aes256Gcm, AesGcm, Nonce};
+use zeroize::Zeroizing;
+
+/// AES-192-GCM with 96-bit nonce. Not provided by `aes-gcm` but constructible
+/// from the underlying primitives, same as `Aes128Gcm` and `Aes256Gcm`.
+type Aes192Gcm = AesGcm<aes_gcm::aes::Aes192, U12>;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] and [`Display`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);
+
+impl SensitiveBytes {
+    /// Wraps the given bytes as sensitive material.
+    pub fn new(bytes: impl Into<Box<[u8]>>) -> Self {
+        Self(Zeroizing::new(bytes.into()))
+    }
+
+    /// Returns the underlying bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.0
+    }
+
+    /// Returns the number of bytes.
+    #[allow(dead_code)] // Encryption work is ongoing so currently unused
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+
+    /// Returns `true` if the byte slice is empty.
+    #[allow(dead_code)] // Encryption work is ongoing so currently unused
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+}
+
+impl fmt::Debug for SensitiveBytes {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "[{} bytes REDACTED]", self.0.len())
+    }
+}
+
+impl fmt::Display for SensitiveBytes {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "[{} bytes REDACTED]", self.0.len())
+    }
+}
+
+/// Supported AES key sizes for AES-GCM encryption.
+///
+/// The Iceberg spec supports 128, 192, and 256-bit keys for AES-GCM.
+/// See: <https://iceberg.apache.org/gcm-stream-spec/#goals>
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum AesKeySize {
+    /// 128-bit AES key (16 bytes)
+    Bits128 = 128,
+    /// 192-bit AES key (24 bytes)
+    Bits192 = 192,
+    /// 256-bit AES key (32 bytes)
+    Bits256 = 256,
+}
+
+impl AesKeySize {
+    /// Returns the key length in bytes for this key size.
+    pub fn key_length(&self) -> usize {
+        match self {
+            Self::Bits128 => 16,
+            Self::Bits192 => 24,
+            Self::Bits256 => 32,
+        }
+    }
+
+    /// Returns the key size for a given DEK length in bytes.
+    ///
+    /// Matches Java's `encryption.data-key-length` property semantics:
+    /// 16 → 128-bit, 24 → 192-bit, 32 → 256-bit.
+    pub fn from_key_length(len: usize) -> Result<Self> {
+        match len {
+            16 => Ok(Self::Bits128),
+            24 => Ok(Self::Bits192),
+            32 => Ok(Self::Bits256),
+            _ => Err(Error::new(
+                ErrorKind::FeatureUnsupported,
+                format!("Unsupported data key length: {len} (must be 16, 24, or 32)"),
+            )),
+        }
+    }
+}
+
+impl FromStr for AesKeySize {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self> {
+        match s {
+            "128" | "AES_GCM_128" | "AES128_GCM" => Ok(Self::Bits128),
+            "192" | "AES_GCM_192" | "AES192_GCM" => Ok(Self::Bits192),
+            "256" | "AES_GCM_256" | "AES256_GCM" => Ok(Self::Bits256),
+            _ => Err(Error::new(
+                ErrorKind::FeatureUnsupported,
+                format!("Unsupported AES key size: {s}"),
+            )),
+        }
+    }
+}
+
+/// A secure encryption key that zeroes its memory on drop.
+pub struct SecureKey {
+    key: SensitiveBytes,
+    key_size: AesKeySize,
+}
+
+impl SecureKey {
+    /// Creates a new secure key with the specified key size.
+    ///
+    /// # Errors
+    /// Returns an error if the key length doesn't match the key size requirements.
+    pub fn new(key: &[u8], key_size: AesKeySize) -> Result<Self> {

Review Comment:
   I think we could infer 
   ```suggestion
       pub fn new(key: &[u8]) -> Result<Self> {
   ```
   I think we could infer `AesKeySize` from key length?



##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,533 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::U12;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Aes256Gcm, AesGcm, Nonce};
+use zeroize::Zeroizing;
+
+/// AES-192-GCM with 96-bit nonce. Not provided by `aes-gcm` but constructible
+/// from the underlying primitives, same as `Aes128Gcm` and `Aes256Gcm`.
+type Aes192Gcm = AesGcm<aes_gcm::aes::Aes192, U12>;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] and [`Display`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);

Review Comment:
   ```suggestion
   struct SensitiveBytes(Zeroizing<Box<[u8]>>);
   ```
   
   I think we are expecting to expose `SecureKey` as public api, and we should hide this?



-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2819127004


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,383 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use rand::RngCore;
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,
+}
+
+impl EncryptionAlgorithm {
+    /// Returns the key length in bytes for this algorithm.
+    pub fn key_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as KeySizeUser>::KeySize::USIZE,
+        }
+    }
+
+    /// Returns the nonce/IV length in bytes for this algorithm.
+    pub fn nonce_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as AeadCore>::NonceSize::USIZE,
+        }
+    }
+
+    /// Returns the string identifier for this algorithm.
+    pub fn as_str(&self) -> &'static str {
+        match self {
+            Self::Aes128Gcm => "AES_GCM_128",
+        }
+    }
+}
+
+impl FromStr for EncryptionAlgorithm {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self> {
+        match s {
+            "AES_GCM_128" | "AES128_GCM" => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported encryption algorithm: {s}"),
+            )),
+        }
+    }
+}
+
+/// A secure encryption key that zeroes its memory on drop.
+pub struct SecureKey {
+    key: Zeroizing<Vec<u8>>,

Review Comment:
   Tried to follow best practices here



-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#issuecomment-4051649153

   > @blackmwk how would you feel about starting to get some parts of the implementation merged whilst we work out some final details of the RFC?
   
   Hi, @xanderbailey Yeah, we could move on with small part that we reached consensus.


-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2965274621


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,383 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use rand::RngCore;
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,

Review Comment:
   It's not used in java but java's Cipher class can handle just the length because the underlying library is happy to accept a key length which is not true for us so we need to dispatch somewhere, I guess my design had this dispatch on creation but your suggestion is to do it at encrypt / decrypt time which is totally fine also. Happy to make this change. Thanks for iterating with me.



-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk merged PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026


-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2902427898


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,458 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);

Review Comment:
   Love this 💯 



##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,383 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use rand::RngCore;
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,

Review Comment:
   >  once the arrow PR merges
   
   Not a huge deal, but I wonder whether you can save a bit of extra review churn by filling that part in already and having the pre-existing `FeatureUnsupported` error in this PR?
   
   https://github.com/apache/iceberg-rust/blob/854f66d371a59d218498de72b02db7d3282f2643/crates/iceberg/src/error.rs#L59-L62
   
   As I say, this is also probably quite easy to do in a follow-up too once it does merge. So not a huge deal 👍 



##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,458 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);
+
+impl SensitiveBytes {
+    /// Wraps the given bytes as sensitive material.
+    pub fn new(bytes: impl Into<Box<[u8]>>) -> Self {
+        Self(Zeroizing::new(bytes.into()))
+    }
+
+    /// Returns the underlying bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.0
+    }
+
+    /// Returns the number of bytes.
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+
+    /// Returns `true` if the byte slice is empty.
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+}
+
+impl fmt::Debug for SensitiveBytes {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "[{} bytes REDACTED]", self.0.len())
+    }
+}
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,
+}
+
+impl EncryptionAlgorithm {
+    /// Returns the key length in bytes for this algorithm.
+    pub fn key_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as KeySizeUser>::KeySize::USIZE,
+        }
+    }
+
+    /// Returns the nonce/IV length in bytes for this algorithm.
+    pub fn nonce_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as AeadCore>::NonceSize::USIZE,
+        }
+    }
+
+    /// Returns the string identifier for this algorithm.
+    pub fn as_str(&self) -> &'static str {
+        match self {
+            Self::Aes128Gcm => "AES_GCM_128",
+        }
+    }
+
+    /// Returns the algorithm for a given DEK length in bytes.
+    ///
+    /// Matches Java's `encryption.data-key-length` property semantics:
+    /// 16 → AES-128-GCM.
+    pub fn from_key_length(len: usize) -> Result<Self> {
+        match len {
+            16 => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported data key length: {len} (must be 16)"),
+            )),
+        }
+    }
+}
+
+impl FromStr for EncryptionAlgorithm {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self> {
+        match s {
+            "AES_GCM_128" | "AES128_GCM" => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported encryption algorithm: {s}"),

Review Comment:
   This might be better mapped to the [`FeatureUnsupported` error](https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/error.rs#L59-L62), considering the message is also about an unsupported algorithm. Rather than invalid data (although I do think this works anyway)
   
   



-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#issuecomment-3917138835

   Thanks for the review! I'm new to contributing to iceberg, what's the convention for getting a committer to review?


-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#issuecomment-3917977502

   > Thanks for the review! I'm new to contributing to iceberg, what's the convention for getting a committer to review?
   
   We need an Iceberg committer, most likely one of the folks working more closely on the Iceberg-rust repo (@Xuanwo and @liurenjie1024). However, with Lunar New Year I think they'll be slow to respond for a little bit.


-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2928537053


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,383 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use rand::RngCore;
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,

Review Comment:
   Do we really need this enum? I think iceberg currently only support AES_GCM, and the enum is actully not used anyware.



##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,458 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);

Review Comment:
   +1



##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,458 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);
+
+impl SensitiveBytes {
+    /// Wraps the given bytes as sensitive material.
+    pub fn new(bytes: impl Into<Box<[u8]>>) -> Self {
+        Self(Zeroizing::new(bytes.into()))
+    }
+
+    /// Returns the underlying bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.0
+    }
+
+    /// Returns the number of bytes.
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+
+    /// Returns `true` if the byte slice is empty.
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+}
+
+impl fmt::Debug for SensitiveBytes {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "[{} bytes REDACTED]", self.0.len())
+    }
+}
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,
+}
+
+impl EncryptionAlgorithm {
+    /// Returns the key length in bytes for this algorithm.
+    pub fn key_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as KeySizeUser>::KeySize::USIZE,
+        }
+    }
+
+    /// Returns the nonce/IV length in bytes for this algorithm.
+    pub fn nonce_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as AeadCore>::NonceSize::USIZE,
+        }
+    }
+
+    /// Returns the string identifier for this algorithm.
+    pub fn as_str(&self) -> &'static str {
+        match self {
+            Self::Aes128Gcm => "AES_GCM_128",
+        }
+    }
+
+    /// Returns the algorithm for a given DEK length in bytes.
+    ///
+    /// Matches Java's `encryption.data-key-length` property semantics:
+    /// 16 → AES-128-GCM.
+    pub fn from_key_length(len: usize) -> Result<Self> {
+        match len {
+            16 => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported data key length: {len} (must be 16)"),
+            )),
+        }
+    }
+}
+
+impl FromStr for EncryptionAlgorithm {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self> {
+        match s {
+            "AES_GCM_128" | "AES128_GCM" => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported encryption algorithm: {s}"),
+            )),
+        }
+    }
+}
+
+/// A secure encryption key that zeroes its memory on drop.
+pub struct SecureKey {
+    key: SensitiveBytes,
+    algorithm: EncryptionAlgorithm,
+}
+
+impl SecureKey {
+    /// Creates a new secure key with the specified algorithm.
+    ///
+    /// # Errors
+    /// Returns an error if the key length doesn't match the algorithm requirements.
+    pub fn new(key: &[u8], algorithm: EncryptionAlgorithm) -> Result<Self> {
+        if key.len() != algorithm.key_length() {
+            return Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!(
+                    "Invalid key length for {:?}: expected {} bytes, got {}",
+                    algorithm,
+                    algorithm.key_length(),
+                    key.len()
+                ),
+            ));
+        }
+        Ok(Self {
+            key: SensitiveBytes::new(key),
+            algorithm,
+        })
+    }
+
+    /// Generates a new random key for the specified algorithm.
+    pub fn generate(algorithm: EncryptionAlgorithm) -> Self {
+        let mut key = vec![0u8; algorithm.key_length()];
+        OsRng.fill_bytes(&mut key);
+        Self {
+            key: SensitiveBytes::new(key),
+            algorithm,
+        }
+    }
+
+    /// Returns the encryption algorithm for this key.
+    pub fn algorithm(&self) -> EncryptionAlgorithm {
+        self.algorithm
+    }
+
+    /// Returns the key bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.key.as_bytes()
+    }
+}
+
+enum CipherImpl {
+    Aes128Gcm(Aes128Gcm),
+}
+
+/// AES-GCM encryptor for encrypting and decrypting data.
+///
+/// The cipher is initialized once at construction time.
+pub struct AesGcmEncryptor {
+    cipher: CipherImpl,

Review Comment:
   It's a little odd. It's called AesGcmEncryptor, but it has enum for support inside. I checked java's code, and I think currently iceberg only support AesGcm? I'm thinking we only need need a struct 
   `AesGcmCipher`, which could be used for do encryption/decrpytion.



-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2930525467


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,458 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);
+
+impl SensitiveBytes {
+    /// Wraps the given bytes as sensitive material.
+    pub fn new(bytes: impl Into<Box<[u8]>>) -> Self {
+        Self(Zeroizing::new(bytes.into()))
+    }
+
+    /// Returns the underlying bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.0
+    }
+
+    /// Returns the number of bytes.
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+
+    /// Returns `true` if the byte slice is empty.
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+}
+
+impl fmt::Debug for SensitiveBytes {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "[{} bytes REDACTED]", self.0.len())
+    }
+}
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,
+}
+
+impl EncryptionAlgorithm {
+    /// Returns the key length in bytes for this algorithm.
+    pub fn key_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as KeySizeUser>::KeySize::USIZE,
+        }
+    }
+
+    /// Returns the nonce/IV length in bytes for this algorithm.
+    pub fn nonce_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as AeadCore>::NonceSize::USIZE,
+        }
+    }
+
+    /// Returns the string identifier for this algorithm.
+    pub fn as_str(&self) -> &'static str {
+        match self {
+            Self::Aes128Gcm => "AES_GCM_128",
+        }
+    }
+
+    /// Returns the algorithm for a given DEK length in bytes.
+    ///
+    /// Matches Java's `encryption.data-key-length` property semantics:
+    /// 16 → AES-128-GCM.
+    pub fn from_key_length(len: usize) -> Result<Self> {
+        match len {
+            16 => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported data key length: {len} (must be 16)"),
+            )),
+        }
+    }
+}
+
+impl FromStr for EncryptionAlgorithm {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self> {
+        match s {
+            "AES_GCM_128" | "AES128_GCM" => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported encryption algorithm: {s}"),
+            )),
+        }
+    }
+}
+
+/// A secure encryption key that zeroes its memory on drop.
+pub struct SecureKey {
+    key: SensitiveBytes,
+    algorithm: EncryptionAlgorithm,
+}
+
+impl SecureKey {
+    /// Creates a new secure key with the specified algorithm.
+    ///
+    /// # Errors
+    /// Returns an error if the key length doesn't match the algorithm requirements.
+    pub fn new(key: &[u8], algorithm: EncryptionAlgorithm) -> Result<Self> {
+        if key.len() != algorithm.key_length() {
+            return Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!(
+                    "Invalid key length for {:?}: expected {} bytes, got {}",
+                    algorithm,
+                    algorithm.key_length(),
+                    key.len()
+                ),
+            ));
+        }
+        Ok(Self {
+            key: SensitiveBytes::new(key),
+            algorithm,
+        })
+    }
+
+    /// Generates a new random key for the specified algorithm.
+    pub fn generate(algorithm: EncryptionAlgorithm) -> Self {
+        let mut key = vec![0u8; algorithm.key_length()];
+        OsRng.fill_bytes(&mut key);
+        Self {
+            key: SensitiveBytes::new(key),
+            algorithm,
+        }
+    }
+
+    /// Returns the encryption algorithm for this key.
+    pub fn algorithm(&self) -> EncryptionAlgorithm {
+        self.algorithm
+    }
+
+    /// Returns the key bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.key.as_bytes()
+    }
+}
+
+enum CipherImpl {
+    Aes128Gcm(Aes128Gcm),
+}
+
+/// AES-GCM encryptor for encrypting and decrypting data.
+///
+/// The cipher is initialized once at construction time.
+pub struct AesGcmEncryptor {
+    cipher: CipherImpl,

Review Comment:
   I can add this once https://github.com/apache/arrow-rs/pull/9203 has merged and we bump arrow.



-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2963447737


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,458 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);
+
+impl SensitiveBytes {
+    /// Wraps the given bytes as sensitive material.
+    pub fn new(bytes: impl Into<Box<[u8]>>) -> Self {
+        Self(Zeroizing::new(bytes.into()))
+    }
+
+    /// Returns the underlying bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.0
+    }
+
+    /// Returns the number of bytes.
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+
+    /// Returns `true` if the byte slice is empty.
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+}
+
+impl fmt::Debug for SensitiveBytes {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "[{} bytes REDACTED]", self.0.len())
+    }
+}
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,
+}
+
+impl EncryptionAlgorithm {
+    /// Returns the key length in bytes for this algorithm.
+    pub fn key_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as KeySizeUser>::KeySize::USIZE,
+        }
+    }
+
+    /// Returns the nonce/IV length in bytes for this algorithm.
+    pub fn nonce_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as AeadCore>::NonceSize::USIZE,
+        }
+    }
+
+    /// Returns the string identifier for this algorithm.
+    pub fn as_str(&self) -> &'static str {
+        match self {
+            Self::Aes128Gcm => "AES_GCM_128",
+        }
+    }
+
+    /// Returns the algorithm for a given DEK length in bytes.
+    ///
+    /// Matches Java's `encryption.data-key-length` property semantics:
+    /// 16 → AES-128-GCM.
+    pub fn from_key_length(len: usize) -> Result<Self> {
+        match len {
+            16 => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported data key length: {len} (must be 16)"),
+            )),
+        }
+    }
+}
+
+impl FromStr for EncryptionAlgorithm {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self> {
+        match s {
+            "AES_GCM_128" | "AES128_GCM" => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported encryption algorithm: {s}"),
+            )),
+        }
+    }
+}
+
+/// A secure encryption key that zeroes its memory on drop.
+pub struct SecureKey {
+    key: SensitiveBytes,
+    algorithm: EncryptionAlgorithm,
+}
+
+impl SecureKey {
+    /// Creates a new secure key with the specified algorithm.
+    ///
+    /// # Errors
+    /// Returns an error if the key length doesn't match the algorithm requirements.
+    pub fn new(key: &[u8], algorithm: EncryptionAlgorithm) -> Result<Self> {
+        if key.len() != algorithm.key_length() {
+            return Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!(
+                    "Invalid key length for {:?}: expected {} bytes, got {}",
+                    algorithm,
+                    algorithm.key_length(),
+                    key.len()
+                ),
+            ));
+        }
+        Ok(Self {
+            key: SensitiveBytes::new(key),
+            algorithm,
+        })
+    }
+
+    /// Generates a new random key for the specified algorithm.
+    pub fn generate(algorithm: EncryptionAlgorithm) -> Self {
+        let mut key = vec![0u8; algorithm.key_length()];
+        OsRng.fill_bytes(&mut key);
+        Self {
+            key: SensitiveBytes::new(key),
+            algorithm,
+        }
+    }
+
+    /// Returns the encryption algorithm for this key.
+    pub fn algorithm(&self) -> EncryptionAlgorithm {
+        self.algorithm
+    }
+
+    /// Returns the key bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.key.as_bytes()
+    }
+}
+
+enum CipherImpl {
+    Aes128Gcm(Aes128Gcm),
+}
+
+/// AES-GCM encryptor for encrypting and decrypting data.
+///
+/// The cipher is initialized once at construction time.
+pub struct AesGcmEncryptor {
+    cipher: CipherImpl,

Review Comment:
   Sorry, I'm not quite convinced, in `aes_gcm` both and `Aes128Gsm` and `Aes256Gsm` are just type aliases. With the enum in https://github.com/apache/iceberg-rust/pull/2026/changes#r2957736592, we could do following:
   ```
   match AesKeySize {
     Bit128 => encrypt<Aes, Nounce128>(...)
   Bit256 => encrypt<Aes, Nounce256>(...)
   }
   fn encrypt<Aes, NouunceSize> (...) {
       ...
   }
   ```



##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,383 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use rand::RngCore;
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,

Review Comment:
   I'm still quite confused because I don't see the usage of `EncryptionAlgorithm` in java. If we have to add this, I'm thinking sth as following:
   ```
   pub enum AesKeySize {
      Bits128  = 128,
      Bits192  = 192,
      Bits256 = 256
   }
   ```
   
   And this could be used in following.



##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,383 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use rand::RngCore;
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,

Review Comment:
   Please correct me if I'm wrong, and point a source where it's used in java codebase.



-- 
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] [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#issuecomment-3751733204

   You are correct by the looks of it! https://github.com/apache/arrow-rs/blob/main/parquet/src/encryption/ciphers.rs


-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2906263554


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,458 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);
+
+impl SensitiveBytes {
+    /// Wraps the given bytes as sensitive material.
+    pub fn new(bytes: impl Into<Box<[u8]>>) -> Self {
+        Self(Zeroizing::new(bytes.into()))
+    }
+
+    /// Returns the underlying bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.0
+    }
+
+    /// Returns the number of bytes.
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+
+    /// Returns `true` if the byte slice is empty.
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+}
+
+impl fmt::Debug for SensitiveBytes {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "[{} bytes REDACTED]", self.0.len())
+    }
+}
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,
+}
+
+impl EncryptionAlgorithm {
+    /// Returns the key length in bytes for this algorithm.
+    pub fn key_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as KeySizeUser>::KeySize::USIZE,
+        }
+    }
+
+    /// Returns the nonce/IV length in bytes for this algorithm.
+    pub fn nonce_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as AeadCore>::NonceSize::USIZE,
+        }
+    }
+
+    /// Returns the string identifier for this algorithm.
+    pub fn as_str(&self) -> &'static str {
+        match self {
+            Self::Aes128Gcm => "AES_GCM_128",
+        }
+    }
+
+    /// Returns the algorithm for a given DEK length in bytes.
+    ///
+    /// Matches Java's `encryption.data-key-length` property semantics:
+    /// 16 → AES-128-GCM.
+    pub fn from_key_length(len: usize) -> Result<Self> {
+        match len {
+            16 => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported data key length: {len} (must be 16)"),
+            )),
+        }
+    }
+}
+
+impl FromStr for EncryptionAlgorithm {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self> {
+        match s {
+            "AES_GCM_128" | "AES128_GCM" => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported encryption algorithm: {s}"),

Review Comment:
   Yeah I wasn't sure about this one, it could be an algorithm unsupported by the spec or an algorithm unsupported by iceberg-rs and I wasn't sure how to distinguish these. 



-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2929810745


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,383 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use rand::RngCore;
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,

Review Comment:
   We will support 256 bit as soon is we get support in the parquet reader / writer so I wanted to trivially be able to extend this.



-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2937760471


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,458 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);
+
+impl SensitiveBytes {
+    /// Wraps the given bytes as sensitive material.
+    pub fn new(bytes: impl Into<Box<[u8]>>) -> Self {
+        Self(Zeroizing::new(bytes.into()))
+    }
+
+    /// Returns the underlying bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.0
+    }
+
+    /// Returns the number of bytes.
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+
+    /// Returns `true` if the byte slice is empty.
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+}
+
+impl fmt::Debug for SensitiveBytes {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "[{} bytes REDACTED]", self.0.len())
+    }
+}
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,
+}
+
+impl EncryptionAlgorithm {
+    /// Returns the key length in bytes for this algorithm.
+    pub fn key_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as KeySizeUser>::KeySize::USIZE,
+        }
+    }
+
+    /// Returns the nonce/IV length in bytes for this algorithm.
+    pub fn nonce_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as AeadCore>::NonceSize::USIZE,
+        }
+    }
+
+    /// Returns the string identifier for this algorithm.
+    pub fn as_str(&self) -> &'static str {
+        match self {
+            Self::Aes128Gcm => "AES_GCM_128",
+        }
+    }
+
+    /// Returns the algorithm for a given DEK length in bytes.
+    ///
+    /// Matches Java's `encryption.data-key-length` property semantics:
+    /// 16 → AES-128-GCM.
+    pub fn from_key_length(len: usize) -> Result<Self> {
+        match len {
+            16 => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported data key length: {len} (must be 16)"),
+            )),
+        }
+    }
+}
+
+impl FromStr for EncryptionAlgorithm {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self> {
+        match s {
+            "AES_GCM_128" | "AES128_GCM" => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported encryption algorithm: {s}"),
+            )),
+        }
+    }
+}
+
+/// A secure encryption key that zeroes its memory on drop.
+pub struct SecureKey {
+    key: SensitiveBytes,
+    algorithm: EncryptionAlgorithm,
+}
+
+impl SecureKey {
+    /// Creates a new secure key with the specified algorithm.
+    ///
+    /// # Errors
+    /// Returns an error if the key length doesn't match the algorithm requirements.
+    pub fn new(key: &[u8], algorithm: EncryptionAlgorithm) -> Result<Self> {
+        if key.len() != algorithm.key_length() {
+            return Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!(
+                    "Invalid key length for {:?}: expected {} bytes, got {}",
+                    algorithm,
+                    algorithm.key_length(),
+                    key.len()
+                ),
+            ));
+        }
+        Ok(Self {
+            key: SensitiveBytes::new(key),
+            algorithm,
+        })
+    }
+
+    /// Generates a new random key for the specified algorithm.
+    pub fn generate(algorithm: EncryptionAlgorithm) -> Self {
+        let mut key = vec![0u8; algorithm.key_length()];
+        OsRng.fill_bytes(&mut key);
+        Self {
+            key: SensitiveBytes::new(key),
+            algorithm,
+        }
+    }
+
+    /// Returns the encryption algorithm for this key.
+    pub fn algorithm(&self) -> EncryptionAlgorithm {
+        self.algorithm
+    }
+
+    /// Returns the key bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.key.as_bytes()
+    }
+}
+
+enum CipherImpl {
+    Aes128Gcm(Aes128Gcm),
+}
+
+/// AES-GCM encryptor for encrypting and decrypting data.
+///
+/// The cipher is initialized once at construction time.
+pub struct AesGcmEncryptor {
+    cipher: CipherImpl,

Review Comment:
   What I want to say is that other encryption algorithms (e.g. aes gcm 256) are format specific, and currently in iceberg itself, we only need to keep `AesGcmEncryptor`. The support for format specific algorithm should be in format specific module like arrow. 
   In my opinion, in this pr we only need to keep `AesGcmEncryptor`, and remove other components.



-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#issuecomment-3865073507

   @mbutrovich are you still okay to review this? Keen to start getting stuff merged! 


-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#issuecomment-3951301764

   I think is ready for review from a committer @Xuanwo @blackmwk. @xanderbailey has a number of PRs queued up to get encryption support going in iceberg-rust.


-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2818892342


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,383 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use rand::RngCore;
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,
+}
+
+impl EncryptionAlgorithm {
+    /// Returns the key length in bytes for this algorithm.
+    pub fn key_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as KeySizeUser>::KeySize::USIZE,
+        }
+    }
+
+    /// Returns the nonce/IV length in bytes for this algorithm.
+    pub fn nonce_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as AeadCore>::NonceSize::USIZE,
+        }
+    }
+
+    /// Returns the string identifier for this algorithm.
+    pub fn as_str(&self) -> &'static str {
+        match self {
+            Self::Aes128Gcm => "AES_GCM_128",
+        }
+    }
+}
+
+impl FromStr for EncryptionAlgorithm {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self> {
+        match s {
+            "AES_GCM_128" | "AES128_GCM" => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported encryption algorithm: {s}"),
+            )),
+        }
+    }
+}
+
+/// A secure encryption key that zeroes its memory on drop.
+pub struct SecureKey {
+    key: Zeroizing<Vec<u8>>,

Review Comment:
   Good, clears it.



##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,383 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use rand::RngCore;
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,

Review Comment:
   We're trying to get 256 added to Arrow-rs, this approach should work with that?



##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,383 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use rand::RngCore;
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,
+}
+
+impl EncryptionAlgorithm {
+    /// Returns the key length in bytes for this algorithm.
+    pub fn key_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as KeySizeUser>::KeySize::USIZE,

Review Comment:
   Same question about 256 for these following functions.



-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2954420689


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,458 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);
+
+impl SensitiveBytes {
+    /// Wraps the given bytes as sensitive material.
+    pub fn new(bytes: impl Into<Box<[u8]>>) -> Self {
+        Self(Zeroizing::new(bytes.into()))
+    }
+
+    /// Returns the underlying bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.0
+    }
+
+    /// Returns the number of bytes.
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+
+    /// Returns `true` if the byte slice is empty.
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+}
+
+impl fmt::Debug for SensitiveBytes {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "[{} bytes REDACTED]", self.0.len())
+    }
+}
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,
+}
+
+impl EncryptionAlgorithm {
+    /// Returns the key length in bytes for this algorithm.
+    pub fn key_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as KeySizeUser>::KeySize::USIZE,
+        }
+    }
+
+    /// Returns the nonce/IV length in bytes for this algorithm.
+    pub fn nonce_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as AeadCore>::NonceSize::USIZE,
+        }
+    }
+
+    /// Returns the string identifier for this algorithm.
+    pub fn as_str(&self) -> &'static str {
+        match self {
+            Self::Aes128Gcm => "AES_GCM_128",
+        }
+    }
+
+    /// Returns the algorithm for a given DEK length in bytes.
+    ///
+    /// Matches Java's `encryption.data-key-length` property semantics:
+    /// 16 → AES-128-GCM.
+    pub fn from_key_length(len: usize) -> Result<Self> {
+        match len {
+            16 => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported data key length: {len} (must be 16)"),
+            )),
+        }
+    }
+}
+
+impl FromStr for EncryptionAlgorithm {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self> {
+        match s {
+            "AES_GCM_128" | "AES128_GCM" => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported encryption algorithm: {s}"),
+            )),
+        }
+    }
+}
+
+/// A secure encryption key that zeroes its memory on drop.
+pub struct SecureKey {
+    key: SensitiveBytes,
+    algorithm: EncryptionAlgorithm,
+}
+
+impl SecureKey {
+    /// Creates a new secure key with the specified algorithm.
+    ///
+    /// # Errors
+    /// Returns an error if the key length doesn't match the algorithm requirements.
+    pub fn new(key: &[u8], algorithm: EncryptionAlgorithm) -> Result<Self> {
+        if key.len() != algorithm.key_length() {
+            return Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!(
+                    "Invalid key length for {:?}: expected {} bytes, got {}",
+                    algorithm,
+                    algorithm.key_length(),
+                    key.len()
+                ),
+            ));
+        }
+        Ok(Self {
+            key: SensitiveBytes::new(key),
+            algorithm,
+        })
+    }
+
+    /// Generates a new random key for the specified algorithm.
+    pub fn generate(algorithm: EncryptionAlgorithm) -> Self {
+        let mut key = vec![0u8; algorithm.key_length()];
+        OsRng.fill_bytes(&mut key);
+        Self {
+            key: SensitiveBytes::new(key),
+            algorithm,
+        }
+    }
+
+    /// Returns the encryption algorithm for this key.
+    pub fn algorithm(&self) -> EncryptionAlgorithm {
+        self.algorithm
+    }
+
+    /// Returns the key bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.key.as_bytes()
+    }
+}
+
+enum CipherImpl {
+    Aes128Gcm(Aes128Gcm),
+}
+
+/// AES-GCM encryptor for encrypting and decrypting data.
+///
+/// The cipher is initialized once at construction time.
+pub struct AesGcmEncryptor {
+    cipher: CipherImpl,

Review Comment:
   The aes_gcm crate uses distinct types for Aes128Gcm and Aes256Gcm - they're different generic instantiations, not runtime-configurable. A key_length: usize field can't select between them at the type level, so we'd still need branching logic somewhere. The CipherImpl enum resolves this dispatch once at construction time rather than on every encrypt/decrypt call which seems more idiomatic to me?



-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "jdockerty (via GitHub)" <gi...@apache.org>.
jdockerty commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2902433361


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,458 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);
+
+impl SensitiveBytes {
+    /// Wraps the given bytes as sensitive material.
+    pub fn new(bytes: impl Into<Box<[u8]>>) -> Self {
+        Self(Zeroizing::new(bytes.into()))
+    }
+
+    /// Returns the underlying bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.0
+    }
+
+    /// Returns the number of bytes.
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+
+    /// Returns `true` if the byte slice is empty.
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+}
+
+impl fmt::Debug for SensitiveBytes {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "[{} bytes REDACTED]", self.0.len())
+    }
+}
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,
+}
+
+impl EncryptionAlgorithm {
+    /// Returns the key length in bytes for this algorithm.
+    pub fn key_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as KeySizeUser>::KeySize::USIZE,
+        }
+    }
+
+    /// Returns the nonce/IV length in bytes for this algorithm.
+    pub fn nonce_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as AeadCore>::NonceSize::USIZE,
+        }
+    }
+
+    /// Returns the string identifier for this algorithm.
+    pub fn as_str(&self) -> &'static str {
+        match self {
+            Self::Aes128Gcm => "AES_GCM_128",
+        }
+    }
+
+    /// Returns the algorithm for a given DEK length in bytes.
+    ///
+    /// Matches Java's `encryption.data-key-length` property semantics:
+    /// 16 → AES-128-GCM.
+    pub fn from_key_length(len: usize) -> Result<Self> {
+        match len {
+            16 => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported data key length: {len} (must be 16)"),
+            )),
+        }
+    }
+}
+
+impl FromStr for EncryptionAlgorithm {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self> {
+        match s {
+            "AES_GCM_128" | "AES128_GCM" => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported encryption algorithm: {s}"),

Review Comment:
   This might be better mapped to the [`FeatureUnsupported` error](https://github.com/apache/iceberg-rust/blob/main/crates/iceberg/src/error.rs#L59-L62), considering the message is also about an unsupported algorithm. Rather than invalid data (although I do think `DataInvalid` works anyway)
   
   



-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#issuecomment-4045333680

   @blackmwk how would you feel about starting to get some parts of the implementation merged whilst we work out some final details of the RFC? 


-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2929821042


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,458 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);
+
+impl SensitiveBytes {
+    /// Wraps the given bytes as sensitive material.
+    pub fn new(bytes: impl Into<Box<[u8]>>) -> Self {
+        Self(Zeroizing::new(bytes.into()))
+    }
+
+    /// Returns the underlying bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.0
+    }
+
+    /// Returns the number of bytes.
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+
+    /// Returns `true` if the byte slice is empty.
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+}
+
+impl fmt::Debug for SensitiveBytes {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "[{} bytes REDACTED]", self.0.len())
+    }
+}
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,
+}
+
+impl EncryptionAlgorithm {
+    /// Returns the key length in bytes for this algorithm.
+    pub fn key_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as KeySizeUser>::KeySize::USIZE,
+        }
+    }
+
+    /// Returns the nonce/IV length in bytes for this algorithm.
+    pub fn nonce_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as AeadCore>::NonceSize::USIZE,
+        }
+    }
+
+    /// Returns the string identifier for this algorithm.
+    pub fn as_str(&self) -> &'static str {
+        match self {
+            Self::Aes128Gcm => "AES_GCM_128",
+        }
+    }
+
+    /// Returns the algorithm for a given DEK length in bytes.
+    ///
+    /// Matches Java's `encryption.data-key-length` property semantics:
+    /// 16 → AES-128-GCM.
+    pub fn from_key_length(len: usize) -> Result<Self> {
+        match len {
+            16 => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported data key length: {len} (must be 16)"),
+            )),
+        }
+    }
+}
+
+impl FromStr for EncryptionAlgorithm {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self> {
+        match s {
+            "AES_GCM_128" | "AES128_GCM" => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported encryption algorithm: {s}"),
+            )),
+        }
+    }
+}
+
+/// A secure encryption key that zeroes its memory on drop.
+pub struct SecureKey {
+    key: SensitiveBytes,
+    algorithm: EncryptionAlgorithm,
+}
+
+impl SecureKey {
+    /// Creates a new secure key with the specified algorithm.
+    ///
+    /// # Errors
+    /// Returns an error if the key length doesn't match the algorithm requirements.
+    pub fn new(key: &[u8], algorithm: EncryptionAlgorithm) -> Result<Self> {
+        if key.len() != algorithm.key_length() {
+            return Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!(
+                    "Invalid key length for {:?}: expected {} bytes, got {}",
+                    algorithm,
+                    algorithm.key_length(),
+                    key.len()
+                ),
+            ));
+        }
+        Ok(Self {
+            key: SensitiveBytes::new(key),
+            algorithm,
+        })
+    }
+
+    /// Generates a new random key for the specified algorithm.
+    pub fn generate(algorithm: EncryptionAlgorithm) -> Self {
+        let mut key = vec![0u8; algorithm.key_length()];
+        OsRng.fill_bytes(&mut key);
+        Self {
+            key: SensitiveBytes::new(key),
+            algorithm,
+        }
+    }
+
+    /// Returns the encryption algorithm for this key.
+    pub fn algorithm(&self) -> EncryptionAlgorithm {
+        self.algorithm
+    }
+
+    /// Returns the key bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.key.as_bytes()
+    }
+}
+
+enum CipherImpl {
+    Aes128Gcm(Aes128Gcm),
+}
+
+/// AES-GCM encryptor for encrypting and decrypting data.
+///
+/// The cipher is initialized once at construction time.
+pub struct AesGcmEncryptor {
+    cipher: CipherImpl,

Review Comment:
   Yes, the additional option is to use 256 which is still AES-GCM but different length keys.



-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2939335716


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,458 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);
+
+impl SensitiveBytes {
+    /// Wraps the given bytes as sensitive material.
+    pub fn new(bytes: impl Into<Box<[u8]>>) -> Self {
+        Self(Zeroizing::new(bytes.into()))
+    }
+
+    /// Returns the underlying bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.0
+    }
+
+    /// Returns the number of bytes.
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+
+    /// Returns `true` if the byte slice is empty.
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+}
+
+impl fmt::Debug for SensitiveBytes {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "[{} bytes REDACTED]", self.0.len())
+    }
+}
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,
+}
+
+impl EncryptionAlgorithm {
+    /// Returns the key length in bytes for this algorithm.
+    pub fn key_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as KeySizeUser>::KeySize::USIZE,
+        }
+    }
+
+    /// Returns the nonce/IV length in bytes for this algorithm.
+    pub fn nonce_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as AeadCore>::NonceSize::USIZE,
+        }
+    }
+
+    /// Returns the string identifier for this algorithm.
+    pub fn as_str(&self) -> &'static str {
+        match self {
+            Self::Aes128Gcm => "AES_GCM_128",
+        }
+    }
+
+    /// Returns the algorithm for a given DEK length in bytes.
+    ///
+    /// Matches Java's `encryption.data-key-length` property semantics:
+    /// 16 → AES-128-GCM.
+    pub fn from_key_length(len: usize) -> Result<Self> {
+        match len {
+            16 => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported data key length: {len} (must be 16)"),
+            )),
+        }
+    }
+}
+
+impl FromStr for EncryptionAlgorithm {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self> {
+        match s {
+            "AES_GCM_128" | "AES128_GCM" => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported encryption algorithm: {s}"),
+            )),
+        }
+    }
+}
+
+/// A secure encryption key that zeroes its memory on drop.
+pub struct SecureKey {
+    key: SensitiveBytes,
+    algorithm: EncryptionAlgorithm,
+}
+
+impl SecureKey {
+    /// Creates a new secure key with the specified algorithm.
+    ///
+    /// # Errors
+    /// Returns an error if the key length doesn't match the algorithm requirements.
+    pub fn new(key: &[u8], algorithm: EncryptionAlgorithm) -> Result<Self> {
+        if key.len() != algorithm.key_length() {
+            return Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!(
+                    "Invalid key length for {:?}: expected {} bytes, got {}",
+                    algorithm,
+                    algorithm.key_length(),
+                    key.len()
+                ),
+            ));
+        }
+        Ok(Self {
+            key: SensitiveBytes::new(key),
+            algorithm,
+        })
+    }
+
+    /// Generates a new random key for the specified algorithm.
+    pub fn generate(algorithm: EncryptionAlgorithm) -> Self {
+        let mut key = vec![0u8; algorithm.key_length()];
+        OsRng.fill_bytes(&mut key);
+        Self {
+            key: SensitiveBytes::new(key),
+            algorithm,
+        }
+    }
+
+    /// Returns the encryption algorithm for this key.
+    pub fn algorithm(&self) -> EncryptionAlgorithm {
+        self.algorithm
+    }
+
+    /// Returns the key bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.key.as_bytes()
+    }
+}
+
+enum CipherImpl {
+    Aes128Gcm(Aes128Gcm),
+}
+
+/// AES-GCM encryptor for encrypting and decrypting data.
+///
+/// The cipher is initialized once at construction time.
+pub struct AesGcmEncryptor {
+    cipher: CipherImpl,

Review Comment:
   The algorithm selection isn't just for Parquet data files - it's also needed for encrypting metadata files (manifests, manifest lists) at the core Iceberg layer, which never goes through the arrow/format module. The `encryption.data-key-length` table property controls DEK generation for all encrypted files uniformly, so the core crate needs to know whether it's 128 or 256.



-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2939337816


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,458 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::Unsigned;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, KeySizeUser, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Key, Nonce};
+use zeroize::Zeroizing;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);
+
+impl SensitiveBytes {
+    /// Wraps the given bytes as sensitive material.
+    pub fn new(bytes: impl Into<Box<[u8]>>) -> Self {
+        Self(Zeroizing::new(bytes.into()))
+    }
+
+    /// Returns the underlying bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.0
+    }
+
+    /// Returns the number of bytes.
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+
+    /// Returns `true` if the byte slice is empty.
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+}
+
+impl fmt::Debug for SensitiveBytes {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "[{} bytes REDACTED]", self.0.len())
+    }
+}
+
+/// Supported encryption algorithm.
+/// Currently only AES-128-GCM is supported as it's the only algorithm
+/// compatible with arrow-rs Parquet encryption.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum EncryptionAlgorithm {
+    /// AES-128 in GCM mode
+    Aes128Gcm,
+}
+
+impl EncryptionAlgorithm {
+    /// Returns the key length in bytes for this algorithm.
+    pub fn key_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as KeySizeUser>::KeySize::USIZE,
+        }
+    }
+
+    /// Returns the nonce/IV length in bytes for this algorithm.
+    pub fn nonce_length(&self) -> usize {
+        match self {
+            Self::Aes128Gcm => <Aes128Gcm as AeadCore>::NonceSize::USIZE,
+        }
+    }
+
+    /// Returns the string identifier for this algorithm.
+    pub fn as_str(&self) -> &'static str {
+        match self {
+            Self::Aes128Gcm => "AES_GCM_128",
+        }
+    }
+
+    /// Returns the algorithm for a given DEK length in bytes.
+    ///
+    /// Matches Java's `encryption.data-key-length` property semantics:
+    /// 16 → AES-128-GCM.
+    pub fn from_key_length(len: usize) -> Result<Self> {
+        match len {
+            16 => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported data key length: {len} (must be 16)"),
+            )),
+        }
+    }
+}
+
+impl FromStr for EncryptionAlgorithm {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self> {
+        match s {
+            "AES_GCM_128" | "AES128_GCM" => Ok(Self::Aes128Gcm),
+            _ => Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!("Unsupported encryption algorithm: {s}"),
+            )),
+        }
+    }
+}
+
+/// A secure encryption key that zeroes its memory on drop.
+pub struct SecureKey {
+    key: SensitiveBytes,
+    algorithm: EncryptionAlgorithm,
+}
+
+impl SecureKey {
+    /// Creates a new secure key with the specified algorithm.
+    ///
+    /// # Errors
+    /// Returns an error if the key length doesn't match the algorithm requirements.
+    pub fn new(key: &[u8], algorithm: EncryptionAlgorithm) -> Result<Self> {
+        if key.len() != algorithm.key_length() {
+            return Err(Error::new(
+                ErrorKind::DataInvalid,
+                format!(
+                    "Invalid key length for {:?}: expected {} bytes, got {}",
+                    algorithm,
+                    algorithm.key_length(),
+                    key.len()
+                ),
+            ));
+        }
+        Ok(Self {
+            key: SensitiveBytes::new(key),
+            algorithm,
+        })
+    }
+
+    /// Generates a new random key for the specified algorithm.
+    pub fn generate(algorithm: EncryptionAlgorithm) -> Self {
+        let mut key = vec![0u8; algorithm.key_length()];
+        OsRng.fill_bytes(&mut key);
+        Self {
+            key: SensitiveBytes::new(key),
+            algorithm,
+        }
+    }
+
+    /// Returns the encryption algorithm for this key.
+    pub fn algorithm(&self) -> EncryptionAlgorithm {
+        self.algorithm
+    }
+
+    /// Returns the key bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.key.as_bytes()
+    }
+}
+
+enum CipherImpl {
+    Aes128Gcm(Aes128Gcm),
+}
+
+/// AES-GCM encryptor for encrypting and decrypting data.
+///
+/// The cipher is initialized once at construction time.
+pub struct AesGcmEncryptor {
+    cipher: CipherImpl,

Review Comment:
   Sorry if I have misunderstood your point here!



-- 
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] [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#issuecomment-3751588429

   This is awesome! I will take a look since I did a lot of the PME support in Comet. Regarding AES-256-GCM support, last I looked Arrow-rs' Parquet reader only supported 128. Is that not the case anymore?


-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "hsiang-c (via GitHub)" <gi...@apache.org>.
hsiang-c commented on PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#issuecomment-3758556640

   @xanderbailey Thank you for the work on encryption support.
   
   Regarding arror-rs, I removed the hardcoded AES-128 constraint w/ https://github.com/apache/arrow-rs/pull/9203, hope that helps a bit. Because we're using PME with AES-256 
   
   cc @mbutrovich


-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#issuecomment-3758880594

   This helps a bunch thanks! 


-- 
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) [1/N] Support encryption: Add crypto for AES-GCM [iceberg-rust]

Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2026:
URL: https://github.com/apache/iceberg-rust/pull/2026#discussion_r2980082038


##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,533 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::U12;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Aes256Gcm, AesGcm, Nonce};
+use zeroize::Zeroizing;
+
+/// AES-192-GCM with 96-bit nonce. Not provided by `aes-gcm` but constructible
+/// from the underlying primitives, same as `Aes128Gcm` and `Aes256Gcm`.
+type Aes192Gcm = AesGcm<aes_gcm::aes::Aes192, U12>;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] and [`Display`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);

Review Comment:
   [092c25a](https://github.com/apache/iceberg-rust/pull/2026/commits/092c25a26bfa59b5bba918d4c5d39c460032ebe1)



##########
crates/iceberg/src/encryption/crypto.rs:
##########
@@ -0,0 +1,533 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Core cryptographic operations for Iceberg encryption.
+
+use std::fmt;
+use std::str::FromStr;
+
+use aes_gcm::aead::generic_array::typenum::U12;
+use aes_gcm::aead::rand_core::RngCore;
+use aes_gcm::aead::{Aead, AeadCore, KeyInit, OsRng, Payload};
+use aes_gcm::{Aes128Gcm, Aes256Gcm, AesGcm, Nonce};
+use zeroize::Zeroizing;
+
+/// AES-192-GCM with 96-bit nonce. Not provided by `aes-gcm` but constructible
+/// from the underlying primitives, same as `Aes128Gcm` and `Aes256Gcm`.
+type Aes192Gcm = AesGcm<aes_gcm::aes::Aes192, U12>;
+
+use crate::{Error, ErrorKind, Result};
+
+/// Wrapper for sensitive byte data (encryption keys, DEKs, etc.) that:
+/// - Zeroizes memory on drop
+/// - Redacts content in [`Debug`] and [`Display`] output
+/// - Provides only `&[u8]` access via [`as_bytes()`](Self::as_bytes)
+/// - Uses `Box<[u8]>` (immutable boxed slice) since key bytes never grow
+///
+/// Use this type for any struct field that holds plaintext key material.
+/// Because its [`Debug`] impl always prints `[N bytes REDACTED]`, structs
+/// containing `SensitiveBytes` can safely derive or implement `Debug`
+/// without risk of leaking key material.
+#[derive(Clone, PartialEq, Eq)]
+pub struct SensitiveBytes(Zeroizing<Box<[u8]>>);
+
+impl SensitiveBytes {
+    /// Wraps the given bytes as sensitive material.
+    pub fn new(bytes: impl Into<Box<[u8]>>) -> Self {
+        Self(Zeroizing::new(bytes.into()))
+    }
+
+    /// Returns the underlying bytes.
+    pub fn as_bytes(&self) -> &[u8] {
+        &self.0
+    }
+
+    /// Returns the number of bytes.
+    #[allow(dead_code)] // Encryption work is ongoing so currently unused
+    pub fn len(&self) -> usize {
+        self.0.len()
+    }
+
+    /// Returns `true` if the byte slice is empty.
+    #[allow(dead_code)] // Encryption work is ongoing so currently unused
+    pub fn is_empty(&self) -> bool {
+        self.0.is_empty()
+    }
+}
+
+impl fmt::Debug for SensitiveBytes {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "[{} bytes REDACTED]", self.0.len())
+    }
+}
+
+impl fmt::Display for SensitiveBytes {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "[{} bytes REDACTED]", self.0.len())
+    }
+}
+
+/// Supported AES key sizes for AES-GCM encryption.
+///
+/// The Iceberg spec supports 128, 192, and 256-bit keys for AES-GCM.
+/// See: <https://iceberg.apache.org/gcm-stream-spec/#goals>
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum AesKeySize {
+    /// 128-bit AES key (16 bytes)
+    Bits128 = 128,
+    /// 192-bit AES key (24 bytes)
+    Bits192 = 192,
+    /// 256-bit AES key (32 bytes)
+    Bits256 = 256,
+}
+
+impl AesKeySize {
+    /// Returns the key length in bytes for this key size.
+    pub fn key_length(&self) -> usize {
+        match self {
+            Self::Bits128 => 16,
+            Self::Bits192 => 24,
+            Self::Bits256 => 32,
+        }
+    }
+
+    /// Returns the key size for a given DEK length in bytes.
+    ///
+    /// Matches Java's `encryption.data-key-length` property semantics:
+    /// 16 → 128-bit, 24 → 192-bit, 32 → 256-bit.
+    pub fn from_key_length(len: usize) -> Result<Self> {
+        match len {
+            16 => Ok(Self::Bits128),
+            24 => Ok(Self::Bits192),
+            32 => Ok(Self::Bits256),
+            _ => Err(Error::new(
+                ErrorKind::FeatureUnsupported,
+                format!("Unsupported data key length: {len} (must be 16, 24, or 32)"),
+            )),
+        }
+    }
+}
+
+impl FromStr for AesKeySize {
+    type Err = Error;
+
+    fn from_str(s: &str) -> Result<Self> {
+        match s {
+            "128" | "AES_GCM_128" | "AES128_GCM" => Ok(Self::Bits128),
+            "192" | "AES_GCM_192" | "AES192_GCM" => Ok(Self::Bits192),
+            "256" | "AES_GCM_256" | "AES256_GCM" => Ok(Self::Bits256),
+            _ => Err(Error::new(
+                ErrorKind::FeatureUnsupported,
+                format!("Unsupported AES key size: {s}"),
+            )),
+        }
+    }
+}
+
+/// A secure encryption key that zeroes its memory on drop.
+pub struct SecureKey {
+    key: SensitiveBytes,
+    key_size: AesKeySize,
+}
+
+impl SecureKey {
+    /// Creates a new secure key with the specified key size.
+    ///
+    /// # Errors
+    /// Returns an error if the key length doesn't match the key size requirements.
+    pub fn new(key: &[u8], key_size: AesKeySize) -> Result<Self> {

Review Comment:
   [092c25a](https://github.com/apache/iceberg-rust/pull/2026/commits/092c25a26bfa59b5bba918d4c5d39c460032ebe1)



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