feat(encryption) [7/N] Read encrypted manifest list by xanderbailey · Pull Request #2453 · apache/iceberg-rust

xanderbailey · 2026-05-14T14:31:51Z

Which issue does this PR close?

Working towards Support client-side encryption #2034

What changes are included in this PR?

Wires encryption support into the Table type so that encrypted manifest lists can be read. Adds an EncryptionManager accessor on Table and ensures it is supplied wherever manifest lists are loaded from snapshots.

Also threads the EncryptionManager through purge_table / drop_table_data so that dropping an encrypted table can read its manifest lists and clean up referenced files correctly.

A design decision was made whilst reviewing the RFC to keep the EM out of FileIo and pass it around separately which turns out to be much cleaner.

Note that we aren't exposing any sort of configuration for the KMS when building the tables in the catalogs. That will come later. That PR is deliberately coming later to minimise the public surface area whilst this feature is under development.

Also note that StaticTable doesn't have the KMS wired in yet since we'd need to merge the KmsFactory to make that code path valid. Similar story is true for wiring this through the catalogs.

Are these changes tested?

Yes

xanderbailey · 2026-05-14T14:32:13Z

    io: &FileIO,
    metadata: &TableMetadata,
    metadata_location: Option<&str>,
+    encryption_manager: Option<&EncryptionManager>,


public api change here

xanderbailey · 2026-05-14T14:32:44Z

        &self,
        file_io: &FileIO,
        table_metadata: &TableMetadata,
+        encryption_manager: Option<&EncryptionManager>,


public api change here.

We had some discussion before, and I think we should add sth like ManifestListLoader to avoid such api changes.

The api should be as following:

impl Table { pub fn manifest_list_loader(&self, s: &Snapshot) -> Result<ManifestListLoader> { ... } }

With this approach, we could hide all table related constructs like Runtime, Cache, FileIO to avoid api changes in future.

+1 it would be nice to encapsulate these public apis now rather than scramble when Iceberg-Rust eventually preps for its 1.0 release. @blackmwk do you feel strongly about adding a manifest_list_loader method in this PR or a future one?

I've added the loader in this PR, let me know what you think!

I prefer to move it into a standalone pr.

Will move the manifest list reader to it's own PR now

xanderbailey · 2026-05-14T14:34:21Z

+///
+/// Returns `Ok(None)` if the property is not set. Returns an error if the
+/// property is set but no [`KeyManagementClient`] was provided.
+fn maybe_configure_encryption(


Wasn't exactly sure if this should live here or if it's a concern of the EM?

hsiang-c · 2026-05-14T22:42:52Z

    metadata_location: Option<String>,
    metadata: Option<TableMetadataRef>,
    identifier: Option<TableIdent>,
+    kms_client: Option<Arc<dyn KeyManagementClient>>,


Thanks for working on the encryption support @xanderbailey, this is really great.

I think TableMetadata::table_properties has all the information we need to construct either a KeyManagementClient or a EncryptionManager. Do you think we could build them as part of the builder instead of passing KeyManagementClient in the API?

Thanks for taking a look! In Java's case the kms is configured at the catalog level (CatalogProperties.ENCRYPTION_KMS_*) and I think we should do the same, the catalog will take some KmsFactory and construct the KMS for the table? WDYT?

Sounds good to me, thanks @xanderbailey.

I think it is closer to the encryption() method in Java's HiveTableOperations.java

hsiang-c · 2026-05-19T03:05:27Z

+    pub const PROPERTY_ENCRYPTION_KEY_ID: &str = "encryption.key-id";
+
+    /// Property key for the encryption data encryption key (DEK) length in bytes.
+    pub const PROPERTY_ENCRYPTION_DATA_KEY_LENGTH: &str = "encryption.data-key-length";


Thanks for adding it. I think I can get rid of the additional argument for data key length In introduced in #2466

mbutrovich

Thanks @xanderbailey! First initial pass, trying to refer back to RFC and spec. I think this looks right:

The "EM lives on Table, passed explicitly, FileIO stays encryption-unaware" shape is exactly the decision in RFC #2183 thread.
TableBuilder::maybe_configure_encryption is the function the RFC literally names (RFC §"Catalog Integration", Step 2) with the same three-step logic.
v3-only enforcement matches the format spec (Appendix E) and RFC §"Format Version Requirement".
Putting kms_client(...) directly on TableBuilder (rather than the RFC's catalog-level KmsClientFactory) is acknowledged as interim in the PR description.
decrypt_manifest_list_key_metadata(key_id: &str) -> StandardKeyMetadata is the signature negotiated in #2383.
Snapshot key-id is optional in v3 per spec.md §Snapshots, so (None, _) reading via an EM-equipped table is spec-valid.

mbutrovich · 2026-05-20T16:25:11Z

+        let manifest_list_content = match (&self.encryption_key_id, encryption_manager) {
+            (Some(_), None) => {
+                return Err(Error::new(
+                    ErrorKind::FeatureUnsupported,


Can we do ErrorKind::PreconditionFailed or ErrorKind::Unexpected here?

Precondition seems correct

mbutrovich · 2026-05-20T16:26:49Z

+                let input = file_io.new_input(&self.manifest_list)?;
+                EncryptedInputFile::new(input, key_metadata).read().await?
+            }
+            (None, _) => file_io.new_input(&self.manifest_list)?.read().await?,


It's non-obvious that there could be an intentionally unused EncryptionManager here, since a table could have unencrypted snapshots in its history. Maybe worth a one-line comment so someone doesn't change this to (None, None).

Double checked and java does the same https://github.com/apache/iceberg/blob/main/api/src/main/java/org/apache/iceberg/encryption/EncryptingFileIO.java#L120-L125

This would be nice small place to improve on what Java does, even if its in a follow up comment only PR.

mbutrovich · 2026-05-20T16:27:17Z

+
+    use bytes::Bytes;
+
+    use crate::encryption::kms::{KeyManagementClient, MemoryKeyManagementClient};


Can we hoist these up near 539?

mbutrovich · 2026-05-20T16:27:49Z

+
+    let kms_client = kms_client.ok_or_else(|| {
+        Error::new(
+            ErrorKind::FeatureUnsupported,


Same ErrorKind feedback as before. Whatever we pick should be consistent.

mbutrovich · 2026-05-20T16:29:33Z

 /// Data files within manifests are only deleted if the `gc.enabled` table
 /// property is `true` (the default), to avoid corrupting other tables that
 /// may share the same data files.
 pub async fn drop_table_data(


Do we have a test for this code path, even just to make sure future signature changes don't break reading manifest lists?

I was going to add more tests here when we had all the decryption stuff in, else I'd have to awkwardly construct an encrypted manifest list with unencrypted manifest files which seemed a little wonky to me. WDYT?

I guess we can defer as long as end up with a test for the codepath. I know it's hard when the features comes in pieces like this.

blackmwk · 2026-05-19T02:53:03Z

        &self,
        file_io: &FileIO,
        table_metadata: &TableMetadata,
+        encryption_manager: Option<&EncryptionManager>,


We had some discussion before, and I think we should add sth like ManifestListLoader to avoid such api changes.

blackmwk · 2026-05-21T06:59:52Z

        &self,
        file_io: &FileIO,
        table_metadata: &TableMetadata,
+        encryption_manager: Option<&EncryptionManager>,


The api should be as following:

impl Table { pub fn manifest_list_loader(&self, s: &Snapshot) -> Result<ManifestListLoader> { ... } }

With this approach, we could hide all table related constructs like Runtime, Cache, FileIO to avoid api changes in future.

blackmwk · 2026-05-21T07:05:46Z

+        ));
+    }
+
+    let kms_client = kms_client.ok_or_else(|| {


Is this correct? Why user must provide a kms_client for table property?

If a table_key_id is present (encryption.key-id) then the the user must provide a KMS, the property is telling us that the table is encrypted. If we weren't to fail loudly here I think users would end up with some cryptic deserialize error when they try and read an encrypted manifest list.

Sounds reasonable to me.

We should log a warning if encryption key is not set, but kms client is provided.

xanderbailey · 2026-05-21T09:20:02Z

Thanks for the review @blackmwk. I've introduced the ManifestListLoader a3f165e and made load_manifest_list pub crate. Note that the original load_manifest_list is still used directly by the object cache and in the catalog utils, I think both of these make sense to leave?

xanderbailey · 2026-05-27T05:55:09Z

Thanks so much for your reviews so far @blackmwk, would you mind taking another quick look here when you've got a minute?

Kurtiscwright · 2026-05-28T21:17:36Z

        &self,
        file_io: &FileIO,
        table_metadata: &TableMetadata,
+        encryption_manager: Option<&EncryptionManager>,


+1 it would be nice to encapsulate these public apis now rather than scramble when Iceberg-Rust eventually preps for its 1.0 release. @blackmwk do you feel strongly about adding a manifest_list_loader method in this PR or a future one?

Kurtiscwright · 2026-05-28T21:18:57Z

+                let input = file_io.new_input(&self.manifest_list)?;
+                EncryptedInputFile::new(input, key_metadata).read().await?
+            }
+            (None, _) => file_io.new_input(&self.manifest_list)?.read().await?,


This would be nice small place to improve on what Java does, even if its in a follow up comment only PR.

Kurtiscwright · 2026-05-28T21:30:37Z

    pub cdc_norm_level: i32,
+    /// The master key id used to encrypt this table's manifest list and data
+    /// files. `None` if `encryption.key-id` is not set.
+    pub encryption_key_id: Option<String>,


Code style nit pick: we could utilize Rust's match and type system if we had a TableProperties without this and a separate struct for EncryptedTableProperties. The encrypted struct would look like

struct EncryptedTableProperties { /// The master key id used to encrypt this table's manifest list and data /// files. `None` if `encryption.key-id` is not set. pub encryption_key_id: Option<String>, ...TableProperties, }

Then when you need to check to use encrypted vs non-encrypted code paths you would just utilize match and check the type of the table's properties.

I don't know if ...TableProperties is valid rust is it? Do you mean copy all of the fields out and have an enum for table properties? Seems like adding a new struct here would just be added indirection, WDYT?

blackmwk · 2026-05-30T03:37:26Z

 }

+/// Loads a [`ManifestList`] for a snapshot.
+pub struct ManifestListLoader<'a> {


This should be moved to manifest_list module.

Sorry, I made a mistake, we already have a ManifestListWriter, maybe we should call this ManifestListReader?

blackmwk · 2026-05-30T03:38:32Z

+    /// Loads and returns the [`ManifestList`] for this snapshot.
+    pub async fn load(&self) -> Result<ManifestList> {
+        self.snapshot
+            .load_manifest_list(self.file_io, self.table_metadata, self.encryption_manager)


We should move the logic of loading manifest list into this method, and deprecate the load_manifest_list in Snapshot.

blackmwk · 2026-05-30T03:51:21Z

+
+    // Encryption is a v3 feature: `encryption-keys` table metadata and the
+    // snapshot `key-id` field are introduced in format version 3.
+    if metadata.format_version() < FormatVersion::V3 {


This logic is odd to me. I think the correct one should be like:

if format_version < v3 { return Ok(None) } // Now we are v3 if encryption_key_id is none { return None }

Happy to make that change, I was trying to test the invariants more explicitly, i.e if we end up with a table with encryption_key_id and you're not V3 then we should shout loudly. But I'm happy with your suggestion

blackmwk · 2026-05-30T03:53:06Z

+        ));
+    }
+
+    let kms_client = kms_client.ok_or_else(|| {


Sounds reasonable to me.

blackmwk · 2026-05-30T03:53:46Z

+        ));
+    }
+
+    let kms_client = kms_client.ok_or_else(|| {


We should log a warning if encryption key is not set, but kms client is provided.

blackmwk · 2026-05-30T03:55:14Z

        &self,
        file_io: &FileIO,
        table_metadata: &TableMetadata,
+        encryption_manager: Option<&EncryptionManager>,


I prefer to move it into a standalone pr.

xanderbailey · 2026-05-30T11:19:48Z

Have moved ManifestListReader out to #2541 which we can merged before this.

xanderbailey commented May 14, 2026

View reviewed changes

hsiang-c reviewed May 14, 2026

View reviewed changes

hsiang-c reviewed May 19, 2026

View reviewed changes

xanderbailey force-pushed the xb/encryption-7-table-wiring branch from d2d19e7 to 3bb548e Compare May 19, 2026 11:20

mbutrovich suggested changes May 20, 2026

View reviewed changes

blackmwk reviewed May 21, 2026

View reviewed changes

xanderbailey force-pushed the xb/encryption-7-table-wiring branch 2 times, most recently from 296ccbf to f3d4a9b Compare May 21, 2026 09:33

Kurtiscwright reviewed May 28, 2026

View reviewed changes

blackmwk reviewed May 30, 2026

View reviewed changes

chore(refactor): add manifest list reader

85a2a67

xanderbailey added 14 commits May 30, 2026 12:19

fmt

0bcc80a

public-api-lock

3fd956b

Read encrypted manifest lists

df5300d

wire in purge_table

9a93a51

fmt

371aca0

key_size

4415ba6

Whoops

88052dd

fix tests

90c6d46

Use precondition error

aaaa71f

hoist test functions and imports

b159391

fix tests

1bb18cd

fix different test

e2466bc

Add manifest list loader

bf6ac34

fix lock

2626710

xanderbailey force-pushed the xb/encryption-7-table-wiring branch from f3d4a9b to 2626710 Compare May 30, 2026 12:33

xanderbailey added 2 commits May 30, 2026 13:34

revert cargo change

67c94f6

comments

8710d4f


		use bytes::Bytes;

		use crate::encryption::kms::{KeyManagementClient, MemoryKeyManagementClient};

Conversation

xanderbailey commented May 14, 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?

Are these changes tested?

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 May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

xanderbailey commented May 21, 2026

Uh oh!

xanderbailey commented May 27, 2026

Uh oh!

xanderbailey commented May 14, 2026 •

edited

Loading

xanderbailey May 15, 2026 •

edited

Loading