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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
