You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "mfellner (via GitHub)" <gi...@apache.org> on 2026/10/11 19:12:43 UTC
[I] Commits to v1/v2 tables with encryption.key-id write plaintext files [iceberg-rust]
mfellner opened a new issue, #3402:
URL: https://github.com/apache/iceberg-rust/issues/3402
Related to #2411 (v3 tracking). Related to #2034, #2626 and #3018.
## Apache Iceberg Rust version
main at 050ddfda8. #3018 is contained in the `v0.11.0-rc.1` and `v0.11.0-rc.2` tags.
## Describe the bug
On a v2 table with the table property `encryption.key-id` set, commits succeed and the manifest list and manifests are written unencrypted. No error is returned.
The property can be set through the public catalog API: `create_table` accepts it, and `update_table_properties` sets it on an existing v2 table. The reproduction below uses the in-memory catalog.
On main:
- `EncryptionManager::from_table_metadata` returns `Ok(None)` for format versions below v3 before it reads `encryption.key-id` (`crates/iceberg/src/encryption/manager.rs:113-115`).
- The snapshot writer uses the plaintext writer when there is no encryption manager (`crates/iceberg/src/transaction/snapshot.rs:455`).
- `Transaction::commit` has no encryption check. In v0.10.1 it returned `FeatureUnsupported` for any table with `encryption.key-id` set (`crates/iceberg/src/transaction/mod.rs:184` at tag `v0.10.1`). #3018 removed that check.
- The build-time skip for pre-v3 tables is asserted by `table_builder_skips_encryption_on_pre_v3_table` (`crates/iceberg/src/table.rs:569`). That test was added in #2453, before the commit guard in #2626 was merged. No test covers a commit after such a build.
- The doc comment on `Table::encryption_manager` says a manager is present iff `encryption.key-id` is set and a KMS client was supplied (`crates/iceberg/src/table.rs:267-268`). For a v2 table with both set, it returns `None` (checked with a table-level build, not in the snippet below).
- `TableMetadataBuilder::add_encryption_key` accepts a key on v2 metadata (`crates/iceberg/src/spec/table_metadata_builder.rs:1050`). The v1/v2 metadata struct has no `encryption-keys` field (`crates/iceberg/src/spec/table_metadata.rs:849`), so the key is not in the serialized metadata.
In apache/iceberg `main`, `EncryptionUtil.checkCompatibility` throws `Invalid properties for v%s` for `encryption.key-id` and `encryption.data-key-length` when the format version is below 3 (`core/src/main/java/org/apache/iceberg/encryption/EncryptionUtil.java:207-219`). `TableMetadata.newTableMetadata` calls it (`core/src/main/java/org/apache/iceberg/TableMetadata.java:147`).
The Iceberg spec lists `encryption-keys` and the snapshot `key-id` as v3 changes (Appendix E, Version 3).
## To Reproduce
Add these imports to the `tests` module of `crates/iceberg/src/transaction/append.rs` at main (050ddfda8):
```rust
use crate::spec::{FormatVersion, NestedField, PrimitiveType, Schema, Type};
use crate::transaction::ApplyTransactionAction;
use crate::{Catalog, NamespaceIdent, TableCreation};
```
Then add the following tests to the same module and run `cargo test -p iceberg --lib repro_`.
```rust
fn repro_v2_metadata(with_key_id: bool) -> TableMetadata {
let mut metadata: TableMetadata = serde_json::from_str(
&fs::read_to_string(format!(
"{}/testdata/table_metadata/TableMetadataV2ValidMinimal.json",
env!("CARGO_MANIFEST_DIR")
))
.unwrap(),
)
.unwrap();
if with_key_id {
metadata
.properties
.insert("encryption.key-id".into(), "master-1".into());
}
metadata
}
fn repro_catalog_v2_table_creation() -> TableCreation {
TableCreation::builder()
.name("test1".to_string())
.schema(
Schema::builder()
.with_fields(vec![
NestedField::required(1, "foo", Type::Primitive(PrimitiveType::Int)).into(),
])
.build()
.unwrap(),
)
.build()
}
fn repro_data_file(table: &Table) -> DataFile {
DataFileBuilder::default()
.content(DataContentType::Data)
.file_path("test/repro_catalog.parquet".to_string())
.file_format(DataFileFormat::Parquet)
.file_size_in_bytes(100)
.record_count(1)
.partition_spec_id(table.metadata().default_partition_spec_id())
.partition(Struct::empty())
.build()
.unwrap()
}
async fn repro_assert_plaintext_manifest_list(table: &Table) {
let snapshot = table.metadata().current_snapshot().unwrap();
assert_eq!(snapshot.encryption_key_id(), None);
let list_bytes = table
.file_io()
.new_input(snapshot.manifest_list())
.unwrap()
.read()
.await
.unwrap();
assert_eq!(&list_bytes[..4], b"Obj\x01");
}
#[tokio::test]
async fn repro_catalog_v2_create_with_encryption_key_id_commits_plaintext() {
let catalog = crate::memory::tests::new_memory_catalog().await;
let namespace = NamespaceIdent::from_strs(["ns1"]).unwrap();
catalog
.create_namespace(&namespace, HashMap::new())
.await
.unwrap();
let mut creation = repro_catalog_v2_table_creation();
creation
.properties
.insert("encryption.key-id".to_string(), "master-1".to_string());
let table = catalog.create_table(&namespace, creation).await.unwrap();
assert_eq!(table.metadata().format_version(), FormatVersion::V2);
let tx = Transaction::new(&table);
let action = tx.fast_append().add_data_files(vec![repro_data_file(&table)]);
let tx = action.apply(tx).unwrap();
let committed = tx.commit(&catalog).await.unwrap();
repro_assert_plaintext_manifest_list(&committed).await;
}
#[tokio::test]
async fn repro_catalog_v2_set_encryption_key_id_then_commit_plaintext() {
let catalog = crate::memory::tests::new_memory_catalog().await;
let namespace = NamespaceIdent::from_strs(["ns1"]).unwrap();
catalog
.create_namespace(&namespace, HashMap::new())
.await
.unwrap();
let table = catalog
.create_table(&namespace, repro_catalog_v2_table_creation())
.await
.unwrap();
let tx = Transaction::new(&table);
let tx = tx
.update_table_properties()
.set("encryption.key-id".to_string(), "master-1".to_string())
.apply(tx)
.unwrap();
let table = tx.commit(&catalog).await.unwrap();
assert_eq!(
table.metadata().properties().get("encryption.key-id"),
Some(&"master-1".to_string())
);
let tx = Transaction::new(&table);
let action = tx.fast_append().add_data_files(vec![repro_data_file(&table)]);
let tx = action.apply(tx).unwrap();
let committed = tx.commit(&catalog).await.unwrap();
repro_assert_plaintext_manifest_list(&committed).await;
}
#[test]
fn repro_v2_add_encryption_key_is_dropped_on_serialization() {
let key = crate::spec::EncryptedKey::builder()
.key_id("kek-1")
.encrypted_key_metadata(vec![1, 2, 3, 4])
.build();
let built = repro_v2_metadata(false)
.into_builder(None)
.add_encryption_key(key.clone())
.build()
.unwrap();
assert_eq!(built.metadata.encryption_key("kek-1"), Some(&key));
let json = serde_json::to_string(&built.metadata).unwrap();
let round_tripped: TableMetadata = serde_json::from_str(&json).unwrap();
assert_eq!(round_tripped.encryption_key("kek-1"), None);
}
```
Output on 050ddfda8:
```
running 3 tests
test transaction::append::tests::repro_v2_add_encryption_key_is_dropped_on_serialization ... ok
test transaction::append::tests::repro_catalog_v2_create_with_encryption_key_id_commits_plaintext ... ok
test transaction::append::tests::repro_catalog_v2_set_encryption_key_id_then_commit_plaintext ... ok
test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 1928 filtered out; finished in 0.02s
```
Controls run on the same commit, not in the snippet: a table built from `testdata/table_metadata/TableMetadataV1Valid.json` (format version 1) with `encryption.key-id` added also writes Avro manifest lists and manifests. The same commit on the existing v3 fixture (`make_encrypted_table()`) writes an `AGS1` manifest list and records a snapshot `encryption_key_id`.
## Expected behavior
A commit to a v1/v2 table with `encryption.key-id` set should not write plaintext manifest lists or manifests. Either of these would address it:
- reject `encryption.key-id` and `encryption.data-key-length` on v1/v2 tables, as Java does at table creation, or
- return an error when the table is built or committed, instead of taking the unencrypted path.
`TableMetadataBuilder::add_encryption_key` on v1/v2 metadata should not report success for a key that is not persisted.
Either direction also changes `table_builder_skips_encryption_on_pre_v3_table`, so I would appreciate maintainer guidance on which behaviour is intended.
## Willingness to contribute
I would be willing to contribute a fix with guidance from the Iceberg community (any contribution would be agent-assisted and I would disclose that).
Found during an agent-assisted audit of v3 support (see #2411). The reproduction above was run against main at 050ddfda8.
--
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.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: [I] Commits to v1/v2 tables with encryption.key-id write plaintext files [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on issue #3402:
URL: https://github.com/apache/iceberg-rust/issues/3402#issuecomment-6126872505
Encryption isn't supported in pre-v3 spec. We did have this discussion https://github.com/apache/iceberg-rust/pull/2453#discussion_r3328741702 and I would personally be happier with it throwing but happy to hear what @blackmwk thinks 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