RFC for table encryption by xanderbailey · Pull Request #2183 · apache/iceberg-rust

xanderbailey · 2026-02-26T16:48:07Z

Which issue does this PR close?

RFC for table encryption
Part of: #2034
Rough draft with some of the key parts: #2042

Closes #.

What changes are included in this PR?

Are these changes tested?

mbutrovich · 2026-02-27T21:21:43Z

Not specific feedback for the RFC, just sharing some context links and previous discussion that was helpful in getting the EncryptionFactory design into DataFusion that interfaces with the parquet crate. Comet makes good use of this design with a custom EncryptionFactory that uses JNI to interface with Spark-based custom KMSs.

apache/datafusion#16779
apache/datafusion#15216 (comment)

Also tagging @ggershinsky in case he has any cycles to read this. His guidance was instrumental on the DataFusion and Arrow-rs PME work, as well as Iceberg Java encryption implementation.

ggershinsky · 2026-03-03T08:31:51Z

Thanks, I'll be glad to have a look.

ggershinsky · 2026-03-04T08:01:22Z

docs/rfcs/0003_table_encryption.md

+```
+Master Key (in KMS)
+  └── wraps → KEK (Key Encryption Key) — stored in table metadata as EncryptedKey
+        └── wraps → DEK (Data Encryption Key) — stored in StandardKeyMetadata per file


some DEKs (those for manifest list files) are also stored in table metadata as EncryptedKey. These DEKs are indeed packaged in a StandardKeyMetadata (along with AAD prefix and file length). The serialized StandardKeyMetadata is encrypted/wrapped by the KEK, and stored in the table metadata / encrypted_keys structure.

The manifest file DEKs are packaged in StandardKeyMetadata, and stored as-is (without encryption) in manifest list files. The latter are encrypted then.

The data file DEKs are packaged in StandardKeyMetadata, and stored as-is (without encryption) in manifest files. The latter are encrypted then.

ggershinsky · 2026-03-04T08:02:59Z

docs/rfcs/0003_table_encryption.md

+
+- **Master keys** live in the KMS and never leave it
+- **KEKs** are wrapped by the master key and stored in `TableMetadata.encryption_keys`
+- **DEKs** are wrapped by a KEK and stored per-file in `StandardKeyMetadata`


Only manifest list DEKs are wrapped by a KEK. Other DEKs are encrypted in the parent files, by the parent DEKs

ggershinsky · 2026-03-04T08:04:45Z

docs/rfcs/0003_table_encryption.md

+          │
+          ▼
+  load_manifest_list(file_io, table_metadata)
+    1. Look up encryption_key_id in table_metadata.encryption_keys


Also need to unwrap the KEK (via a KMS client)

ggershinsky · 2026-03-04T08:06:34Z

docs/rfcs/0003_table_encryption.md

+       a. file_io.new_encrypted_output(path) → AGS1-encrypting OutputFile
+       b. em.wrap_key_metadata() → EncryptedKey for table metadata
+       c. Store key_id on Snapshot.encryption_key_id
+    3. Table updates include AddEncryptionKey for new KEKs


Also need to wrap the KEK (via a KMS client)

xanderbailey · 2026-03-04T09:47:47Z

Thanks for taking a look @ggershinsky I've tried to fill in some of the details here

blackmwk

Thanks @xanderbailey for this pr, generally LGTM, left some suggestions.

blackmwk · 2026-03-06T02:31:08Z

docs/rfcs/0003_table_encryption.md

+
+### Iceberg Spec: Encryption
+
+The [Iceberg table spec](https://iceberg.apache.org/spec/#table-metadata) defines encryption


This link may be a better reference.

blackmwk · 2026-03-10T01:08:28Z

docs/rfcs/0003_table_encryption.md

+│   ├── kms/
+│   │   ├── mod.rs
+│   │   └── in_memory.rs             # InMemoryKms (testing only)
+│   └── integration_tests.rs         # End-to-end encryption round-trip tests


In rust we usually put integration tests in tests dir, next to src dir.

blackmwk · 2026-03-10T01:17:28Z

docs/rfcs/0003_table_encryption.md

+
+```rust
+#[async_trait]
+pub trait EncryptionManager: Debug + Send + Sync {


Do we really need this to be a trait? I think a no-op kms client would be enough?

Yeah we actually don't need this to be a trait the standard encryption manager is the only one we need

blackmwk · 2026-03-10T01:23:04Z

docs/rfcs/0003_table_encryption.md

+`InputFile` and `OutputFile` are enums with three variants each:
+
+```rust
+pub enum InputFile {


I think we this enum should sth as following:

pub enum EncryptedInputFile { Encrypted { key_metadata: KeyMetadata inner: InputFile }, NativeedEncryted { key_material: NativeKeyMaterial, inner: InputFile } }

I think this requires a lot more changes to the current code since encrypted inputs and outputs won't be handled transparently. Or is the suggestion to have InputFile as an enum of Plain, EncryptedInputFile and EncryptedInputFile is itself an enum?

I find it a little difficult to imagine what it will look like given different choice? Could you give raise some examples? I'm even thinking if we actually need an enum. InputFile, EncryptedInputFile and NativeEncryptedInputFile are used in differenent cases. Let's use an example, ManifestReader, if we remove the enum, what we need is to add an extra api for read(EncryptedInputFile). This is acceptable to me, some we have a FileRead trait to abstract out the descryption process.

I'm leaning towards the example snippet @blackmwk above and I don't think we should expand Input/OutputFile to an enum. EncryptedFile should be a wrapper of Input/OutputFile rather than a variation.

By the same logic, having a wrapper Storage like EncryptionStorage may not be ideal. For encryption, we only need to (un)wrap Input/OutputFile in the FileIO level

I think we can do this if you both think this is better but this requires some reasonable surgery on structs like ManifestListWriter which today takes an OutputFile and will now need to take a Box<dyn FileWrite> and ManifestWriter will need Box<dyn FileWrite> and a location.

Okay I tried it out and it's actually not too bad. If you're both happy with the changes I mentioned above then I think this is a good approach.

blackmwk · 2026-03-10T01:24:29Z

docs/rfcs/0003_table_encryption.md

+// Via FileIOBuilder extension (works with RestCatalog and any extension-aware catalog)
+let file_io = FileIOBuilder::new("s3")
+    .with_prop("s3.region", "us-east-1")
+    .with_extension(encryption_manager)


This has been removed.

blackmwk · 2026-03-10T01:33:27Z

docs/rfcs/0003_table_encryption.md

+    .build()?;
+
+// Or via convenience method on FileIO
+let file_io = file_io.with_encryption_manager(encryption_manager);


blackmwk · 2026-03-10T01:35:09Z

docs/rfcs/0003_table_encryption.md

+// Option A: EncryptionManager on the catalog
+let catalog = GlueCatalogBuilder::default()
+    .with_storage_factory(Arc::new(OpenDalStorageFactory::S3))
+    .with_encryption_manager(encryption_manager)


I think what we actually need from user is KmsClientFactory?

Oh interesting, where they can provide a way to construct a number of KMS clients?

I mean the code to be as following:

let catalog = GlueCatalogBuilder::default() .with_storage_factory(Arc::new(OpenDalStorageFactory::S3)) .with_kms_client_factory(new AwsKmsClientFactory())

CTTY · 2026-03-12T23:05:11Z

docs/rfcs/0003_table_encryption.md

+    .load("my_catalog", props)
+    .await?;
+
+// Option B: Wrapping StorageFactory


I prefer option A due to https://github.com/apache/iceberg-rust/pull/2183/changes#r2927906969

RFC for table encryption

c79f8ed

xanderbailey mentioned this pull request Feb 26, 2026

feat(encryption) [1/N] Support encryption: Add crypto for AES-GCM #2026

Open

mbutrovich self-requested a review February 27, 2026 16:04

xanderbailey added 2 commits February 27, 2026 19:10

update

4eef7de

Update with kms client registration

b375682

ggershinsky reviewed Mar 4, 2026

View reviewed changes

address comments

24bc3f2

blackmwk reviewed Mar 10, 2026

View reviewed changes

CTTY reviewed Mar 12, 2026

View reviewed changes


		### Iceberg Spec: Encryption

		The [Iceberg table spec](https://iceberg.apache.org/spec/#table-metadata) defines encryption

Conversation

xanderbailey commented Feb 26, 2026

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Uh oh!

mbutrovich commented Feb 27, 2026

Uh oh!

ggershinsky commented Mar 3, 2026

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 Mar 4, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants