feat(encryption) [5/N] Support encryption: Encryption Manager by xanderbailey · Pull Request #2383 · apache/iceberg-rust

xanderbailey · 2026-04-28T22:29:52Z

Which issue does this PR close?

Part of #2034

What changes are included in this PR?

Stacked on #2340. Adds EncryptionManager — handles two-layer envelope encryption (master key → KEK → DEK) for Iceberg tables.

New files

encryption_manager.rs — EncryptionManager with typed_builder construction:
- encrypt() / decrypt() — AGS1 stream file encryption
- generate_native_key_metadata() — generates StandardKeyMetadata (DEK + AAD prefix) for Parquet Modular Encryption (PME). Callers pass this directly to the Parquet writer's FileEncryptionProperties and serialize it for manifest storage.
- wrap_key_metadata() / unwrap_key_metadata() — KEK envelope wrapping for manifest list key metadata
- KEK lifecycle: creation, rotation (730-day NIST SP 800-57 lifespan), caching (1-hour TTL via moka)
encrypted_io.rs — EncryptedInputFile, EncryptedOutputFile wrappers for transparent AGS1 stream encrypt/decrypt on read/write

Design decisions

No NativeEncryptedInputFile / NativeEncryptedOutputFile wrappers — For PME, StandardKeyMetadata already carries the DEK and AAD prefix. Wrapper types that just bundle an InputFile/OutputFile with the same data are unnecessary indirection. Readers deserialize StandardKeyMetadata directly from key_metadata bytes; writers receive (OutputFile, &StandardKeyMetadata) separately.
generate_native_key_metadata() is sync — unlike AGS1 encryption, PME key generation doesn't touch KMS. It just generates random bytes locally.

How this differs from Java's `StandardEncryptionManager`

KEK management is explicit: Java's addManifestListKeyMetadata() mutates an internal map and callers need to downcast to StandardEncryptionManager to access the keys. Our wrap_key_metadata() returns (wrapped_key, Option<new_kek>) directly. no hidden mutation, no downcasting. Java
No EncryptionUtil grab-bag: Java needs a static utility class (decryptManifestListKeyMetadata, encryptManifestListKeyMetadata, etc.) because the interface is too narrow. Here those are just methods on EncryptionManager.
table_key_id enforced at compile time: Required via typed_builder, can't forget it. Unencrypted tables use Option<EncryptionManager> instead of Java's PlaintextEncryptionManager.
Config values match Java exactly: KEK lifespan (730 days), cache TTL (1 hour), AAD prefix length (16 bytes) are all hardcoded.

Notes on usage for how this is used:
load_manifest_list does:

                let unwrapped_km = em
                    .unwrap_key_metadata(encrypted_key, table_metadata.encryption_keys_map())
                    .await?;
                let input = file_io.new_input(&self.manifest_list)?;
                em.decrypt(input, &unwrapped_km).await?.read().await?

load_manifest does:

        let avro = match &self.key_metadata {
            Some(km) if !km.is_empty() => {
                let em = encryption_manager.ok_or_else(|| {
                    Error::new(
                        ErrorKind::FeatureUnsupported,
                        "Encrypted manifest but no EncryptionManager configured on table",
                    )
                })?;
                let input = file_io.new_input(&self.manifest_path)?;
                em.decrypt(input, km).await?.read().await?
            }
            _ => file_io.new_input(&self.manifest_path)?.read().await?,
        };

        let (metadata, mut entries) = Manifest::try_from_avro_bytes(&avro)?;

Are these changes tested?

Yes

xanderbailey · 2026-04-29T09:04:49Z

+        default = moka::future::Cache::builder().time_to_live(DEFAULT_CACHE_TTL).build(),
+        setter(skip)
+    )]
+    kek_cache: moka::future::Cache<String, SensitiveBytes>,


Put a cache here for the same reason Java does, KMS is not aware of the key_id and since we populate the cache when we create_kek it would be hard to do this purely within a CachingKmsService

mbutrovich · 2026-05-05T18:46:23Z

Thanks @xanderbailey! Here's my summary of reviewing with Claude:

Correctness

1. Missing KEY_TIMESTAMP on a KEK should fail-fast, not silently pass None AAD

encryption_manager.rs:500-503 (in decrypt_dek) and the symmetric wrap site (:350-352):
let aad = kek.properties().get(KEK_CREATED_AT_PROPERTY).map(|ts| ts.as_bytes());
Java asserts Preconditions.checkState(keyEncryptionKeyTimestamp != null, "Key encryption key must be timestamped") (EncryptionUtil.java:161-162). Here a KEK missing the timestamp silently passes None on both wrap and unwrap — the pair roundtrips, but the tampering defense is gone. And if one side has the property and the other doesn't, you get a generic AES-GCM auth failure instead of a clear "corrupted KEK" error.

Suggest: require the property in both paths and return ErrorKind::DataInvalid with a clear message if it's missing. create_kek always sets it, so the only way to hit this is a malformed table — which is exactly the case we want to surface loudly.

2. Manager doesn't internalize newly created KEKs — deviates from the RFC's write flow

RFC §"Write Path", step 2.e: "Store as EncryptedKey (encrypted_by_id = kek_id) in encryption manager."

Today wrap_key_metadata takes &self, encryption_keys is frozen at builder time, and the new KEK is returned to the caller to persist. In the single-manifest-list-per-commit flow this is fine — but it does mean the RFC and implementation disagree.

Two reasonable resolutions:

Update the RFC wording to reflect the current design (caller persists the returned new KEK) — I actually like this better, it's a cleaner ownership story than Java's hidden mutation.

Or use interior mutability (e.g. ArcSwap<HashMap<...>> / RwLock<...>) so the manager can be reused across calls in one commit.

Either way, worth resolving with the RFC before this lands so [6/N] onward doesn't have to walk back either side.

API / naming

3. wrap_key_metadata return type

The Option<EncryptedKey> in (EncryptedKey, Option<EncryptedKey>) has subtle "persist-this-or-lose-it" semantics that's easy to miss at call sites. A named struct makes it self-documenting:
pub struct WrappedKeyMetadata {
    pub entry: EncryptedKey,
    pub new_kek: Option<EncryptedKey>, // Some only when a KEK was created/rotated
}
4. Public constant name

KEK_CREATED_AT_PROPERTY holds the value "KEY_TIMESTAMP" — which matches Java. But the Rust identifier diverges from Java's KEY_TIMESTAMP constant. Since this is public API, matching Java's identifier name makes cross-reading easier.

5. EncryptedInputFile / EncryptedOutputFile are AGS1-only

Names suggest generality, docstring says "AGS1 stream-encrypted". Java disambiguates with AesGcmInputFile vs NativeEncryptionInputFile. Since the RFC also has PME/native paths coming, consider AesGcmStreamInputFile / AesGcmStreamOutputFile to leave room.

6. RFC/impl naming drift

RFC §"EncryptionManager" lists encrypt_native() / generate_dek(); the PR has generate_native_key_metadata() (returning the full StandardKeyMetadata). Which way should the RFC move? Worth aligning in one place.

Behavioral divergences from Java (not spec-mandated, just worth a comment)

7. find_active_kek selection order

max_by_key(timestamp) picks the newest unexpired KEK. Java picks the first unexpired one by LinkedHashMap insertion order (StandardEncryptionManager.java:162-170). The Rust choice is deterministic (HashMap iteration isn't, so max_by_key is necessary) and arguably more correct. A one-line comment explaining the intentional divergence would help.

8. KEK key_id format

Rust: Uuid::new_v4().to_string(). Java: 16 random bytes, Base64-encoded (generateKeyId() at StandardEncryptionManager.java:228-232). Spec doesn't constrain format. Interop works since both sides just compare strings, but worth noting for tables that might be read by Java tooling.

Tests

9. Missing coverage

AAD tampering: mutate a KEK's KEY_TIMESTAMP property in table metadata, assert unwrap_key_metadata fails. This exercises the tampering defense the timestamp-as-AAD scheme is designed to catch.

Missing-timestamp rejection: once chore: Setup project layout #1 above is fixed, add a test asserting a KEK without KEY_TIMESTAMP fails wrap and unwrap with a clear error.

wrap_key_metadata twice on same manager with empty initial encryption_keys: currently test_kek_reuse_when_not_expired constructs a second manager with the first KEK pre-populated, which is exactly the path that works. Testing the same-manager case would pin down whichever decision comes out of Design of Serialization/Deserialization #2.

Nits

encryption_manager.rs:498 comment says "The Iceberg Spec uses the KEK timestamp as AAD". The spec doesn't mandate this — it's a Java implementation choice that we're matching. Reword as "matches Java's wrap scheme" to avoid overclaiming the spec.

crypto.rs:180-186 — TryFrom<SensitiveBytes> for SecureKey is fine, but wrap_dek_with_kek / unwrap_dek_with_kek (encryption_manager.rs:530, 542) clone the SensitiveBytes just to call try_from, duplicating zeroing key material into a second buffer. A TryFrom<&SensitiveBytes> impl (or a &SensitiveBytes AsRef-style path) would avoid the clone.

decrypt_dek wraps the inner error with "Failed to unwrap key metadata with KEK..." — the caller (unwrap_key_metadata) is already named for this context, so the wrapper message is mostly redundant. Not worth changing on its own, but consider when you touch that error path.

Confirm uuid was added to Cargo.toml as part of this PR (not visible in the diff).

xanderbailey · 2026-05-05T20:28:30Z

Missing KEY_TIMESTAMP on a KEK should fail-fast, not silently pass None AAD
encryption_manager.rs:500-503 (in decrypt_dek) and the symmetric wrap site (:350-352):

let aad = kek.properties().get(KEK_CREATED_AT_PROPERTY).map(|ts| ts.as_bytes());
Java asserts Preconditions.checkState(keyEncryptionKeyTimestamp != null, "Key encryption key must be timestamped") (EncryptionUtil.java:161-162). Here a KEK missing the timestamp silently passes None on both wrap and unwrap — the pair roundtrips, but the tampering defense is gone. And if one side has the property and the other doesn't, you get a generic AES-GCM auth failure instead of a clear "corrupted KEK" error.

Suggest: require the property in both paths and return ErrorKind::DataInvalid with a clear message if it's missing. create_kek always sets it, so the only way to hit this is a malformed table — which is exactly the case we want to surface loudly.

7f0ffcc

xanderbailey · 2026-05-05T20:29:59Z

EncryptedInputFile / EncryptedOutputFile are AGS1-only
Names suggest generality, docstring says "AGS1 stream-encrypted". Java disambiguates with AesGcmInputFile vs NativeEncryptionInputFile. Since the RFC also has PME/native paths coming, consider AesGcmStreamInputFile / AesGcmStreamOutputFile to leave room.

We don't have NativeEncryptionInputFile / NativeEncryptionOutputFile so I think this is maybe fine? WDYT?

xanderbailey · 2026-05-05T20:31:11Z

find_active_kek selection order
max_by_key(timestamp) picks the newest unexpired KEK. Java picks the first unexpired one by LinkedHashMap insertion order (StandardEncryptionManager.java:162-170). The Rust choice is deterministic (HashMap iteration isn't, so max_by_key is necessary) and arguably more correct. A one-line comment explaining the intentional divergence would help.

Do we think we actually need a comment here? The code seems self documenting here?

xanderbailey · 2026-05-05T20:39:49Z

KEK key_id format
Rust: Uuid::new_v4().to_string(). Java: 16 random bytes, Base64-encoded (generateKeyId() at StandardEncryptionManager.java:228-232). Spec doesn't constrain format. Interop works since both sides just compare strings, but worth noting for tables that might be read by Java tooling.

I think UUID is fine here maybe? both are 16 bytes and random

mbutrovich · 2026-05-05T20:56:27Z

KEK key_id format
Rust: Uuid::new_v4().to_string(). Java: 16 random bytes, Base64-encoded (generateKeyId() at StandardEncryptionManager.java:228-232). Spec doesn't constrain format. Interop works since both sides just compare strings, but worth noting for tables that might be read by Java tooling.

I think UUID is fine here maybe? both are 16 bytes and random

Agreed.

mbutrovich · 2026-05-05T20:56:45Z

find_active_kek selection order
max_by_key(timestamp) picks the newest unexpired KEK. Java picks the first unexpired one by LinkedHashMap insertion order (StandardEncryptionManager.java:162-170). The Rust choice is deterministic (HashMap iteration isn't, so max_by_key is necessary) and arguably more correct. A one-line comment explaining the intentional divergence would help.

Do we think we actually need a comment here? The code seems self documenting here?

Agreed.

mbutrovich · 2026-05-05T20:57:06Z

EncryptedInputFile / EncryptedOutputFile are AGS1-only
Names suggest generality, docstring says "AGS1 stream-encrypted". Java disambiguates with AesGcmInputFile vs NativeEncryptionInputFile. Since the RFC also has PME/native paths coming, consider AesGcmStreamInputFile / AesGcmStreamOutputFile to leave room.

We don't have NativeEncryptionInputFile / NativeEncryptionOutputFile so I think this is maybe fine? WDYT?

Yeah, seems fine.

blackmwk

Thanks @xanderbailey for this pr!

blackmwk · 2026-05-06T08:06:26Z

+        default = moka::future::Cache::builder().time_to_live(DEFAULT_CACHE_TTL).build(),
+        setter(skip)
+    )]
+    kek_cache: moka::future::Cache<String, SensitiveBytes>,


Please don't use fully qualified name here, use them in imports as much as possible.

blackmwk · 2026-05-06T08:17:33Z

For simplicity should we just rename this module to io?

blackmwk · 2026-05-06T08:17:54Z

Should we just rename this file to manager.rs?

blackmwk · 2026-05-06T09:20:07Z

+    table_key_id: String,
+    /// All encryption keys from table metadata (KEKs and wrapped key metadata entries).
+    #[builder(default)]
+    encryption_keys: HashMap<String, EncryptedKey>,


nit: For collections like this, we could use mutations in TypedBuilder so that user facing api is like add_encryption_key(EncryptedKey) .

blackmwk · 2026-05-06T09:23:57Z

+/// Transparently decrypts on read.
+pub struct EncryptedInputFile {
+    inner: InputFile,
+    decryptor: Arc<AesGcmFileDecryptor>,


I think we should just store StandardKeyMetadata here?

And just use that to construct the the FileRead when we need it?

The AesGcmFileDecryptor is the thing we actually need at read time so is it a good idea to just construct it once?

blackmwk · 2026-05-06T09:27:00Z

+    }
+
+    /// Returns the key metadata bytes (for storage in manifest/data files).
+    pub fn key_metadata(&self) -> &[u8] {


We should return StandardKeyMetadata

blackmwk · 2026-05-06T09:28:50Z

+    /// Returns a [`StandardKeyMetadata`] containing a fresh DEK and AAD prefix.
+    /// The caller should pass this to the Parquet writer to configure
+    /// `FileEncryptionProperties`, and serialize it for storage in the manifest.
+    pub fn generate_native_key_metadata(&self) -> Result<StandardKeyMetadata> {


Let's make it mod private first/

blackmwk · 2026-05-06T09:29:46Z

+
+    /// Decrypt an encrypted input file, returning an [`EncryptedInputFile`]
+    /// that transparently decrypts on read.
+    pub fn decrypt(&self, input: InputFile, key_metadata: &[u8]) -> Result<EncryptedInputFile> {


Suggested change

pub fn decrypt(&self, input: InputFile, key_metadata: &[u8]) -> Result<EncryptedInputFile> {

pub fn decrypt(&self, input: EncryptedInputeFile) -> Result<EncryptedInputFile> {

This seems cyclical to me no?

blackmwk · 2026-05-06T09:31:03Z

+    /// write) or the existing KEK expired (rotation). When `Some`, the
+    /// caller must also persist this KEK in table metadata so that future
+    /// `unwrap_key_metadata` calls can find it.
+    pub async fn wrap_key_metadata(


It's deprecated in java, remove it.

I think wrapKey and unwrapKey are deprecated. wrap_key_metadata maps to EncryptionUtil.encryptManifestListKeyMetadata() and unwrap_key_metadata maps to EncryptionUtil.decryptManifestListKeyMetadata() but I've tried to avoid the EncryptionUtil grab-bag in my implementation.

blackmwk · 2026-05-06T09:31:14Z

+    /// Given an `EncryptedKey` entry (from a manifest list or snapshot) and
+    /// the full map of encryption keys from `TableMetadata`, returns the
+    /// unwrapped key metadata bytes (e.g. serialized `StandardKeyMetadata`).
+    pub async fn unwrap_key_metadata(


xanderbailey changed the title ~~feat(encryption) [4/N] Support encryption: Encryption Manager~~ feat(encryption) [5/N] Support encryption: Encryption Manager Apr 28, 2026

em

74f9e2f

xanderbailey force-pushed the xb/encryption_manager branch from b7fe819 to 74f9e2f Compare April 29, 2026 08:44

xanderbailey commented Apr 29, 2026

View reviewed changes

move native key material

0351241

xanderbailey marked this pull request as draft April 29, 2026 20:16

remove native io types

1ab67ce

xanderbailey marked this pull request as ready for review April 29, 2026 21:15

mbutrovich self-requested a review May 5, 2026 18:01

timestamp is not optional

7f0ffcc

fmt

864d3ff

blackmwk reviewed May 6, 2026

View reviewed changes

xanderbailey added 2 commits May 6, 2026 10:46

rename

d83b7c1

import moke cache

32fdd3a

xanderbailey force-pushed the xb/encryption_manager branch from 5311801 to 32fdd3a Compare May 6, 2026 09:50

xanderbailey added 3 commits May 6, 2026 11:19

fmt

a5bc5f6

builder method

78be2ac

generate_native_key_metadata crate pub

6309d0b

	pub fn decrypt(&self, input: InputFile, key_metadata: &[u8]) -> Result<EncryptedInputFile> {
	pub fn decrypt(&self, input: EncryptedInputeFile) -> Result<EncryptedInputFile> {

Conversation

xanderbailey commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

New files

Design decisions

How this differs from Java's StandardEncryptionManager

Are these changes tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbutrovich commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Correctness

1. Missing KEY_TIMESTAMP on a KEK should fail-fast, not silently pass None AAD

2. Manager doesn't internalize newly created KEKs — deviates from the RFC's write flow

API / naming

3. wrap_key_metadata return type

4. Public constant name

5. EncryptedInputFile / EncryptedOutputFile are AGS1-only

6. RFC/impl naming drift

Behavioral divergences from Java (not spec-mandated, just worth a comment)

7. find_active_kek selection order

8. KEK key_id format

Tests

9. Missing coverage

Nits

Uh oh!

xanderbailey commented May 5, 2026

Uh oh!

xanderbailey commented May 5, 2026

Uh oh!

xanderbailey commented May 5, 2026

Uh oh!

xanderbailey commented May 5, 2026

Uh oh!

mbutrovich commented May 5, 2026

Uh oh!

mbutrovich commented May 5, 2026

Uh oh!

mbutrovich commented May 5, 2026

Uh oh!

blackmwk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xanderbailey commented Apr 28, 2026 •

edited

Loading

How this differs from Java's `StandardEncryptionManager`

mbutrovich commented May 5, 2026 •

edited

Loading

1. Missing `KEY_TIMESTAMP` on a KEK should fail-fast, not silently pass `None` AAD

3. `wrap_key_metadata` return type

5. `EncryptedInputFile` / `EncryptedOutputFile` are AGS1-only

7. `find_active_kek` selection order

8. KEK `key_id` format