API, Core, Spark 4.1: Add key_metadata and encrypted write support for StatisticsFile

[gaborkaszab]

No description provided.

[gaborkaszab]

Notes:

• The spec already contains key metadata for statistics file
• The currently no encryption is used for the table statistics Puffin file on the write path even if configured for the table
• Currently, there is no reader that reads into the Puffin file (NDV is retrieved from the metadata rather than from the sketch) but this might change in the future. But now, no prod reader adjustment was required

Potential follow-up: do the same changes for partition statistics files. Note, a spec change is required for that as key_metadata is not specified atm.

[gaborkaszab]

cc @ggershinsky @RussellSpitzer

[ggershinsky]

In general, SGTM; it'd be good to have all table files encrypted and tamper-proofed.
Will this patch be able to read files produced by Iceberg 1.11? Maybe it should check the key_metadata field, and if empty, read the file as usual without decryption?
(it might be working ok already as is, but just to verify)

[ggershinsky]

Thinking more about this, there could be a problem. If StatisticFile structs are kept in the metadata.json file (and not in a manifest/list file), the encryption key etc would be exposed in the key_metadata since the metadata is not encrypted. We might need to encrypt the key_metadata with a key encryption key (like with manifest list key_metadata) handling the AADs, or (less ideally) with the master key via the KMS.

[ggershinsky]

and the encrypted key_metadata is best kept in the TableMetadata encrypted_keys list, since we need a pointer to the wrapping key.
Which doesn't fit the spec..

[RussellSpitzer]

Yeah I think we have an issue here. I don't think we can just put the key metadata in plain text in metadata.json ?

[ggershinsky]

Yep, the key metadata keeps the encryption key in the open.. It can be encrypted by a "key encryption key" - like we do for the snapshot (manifest list file) keys. But then we have a storage issue, because we also need a pointer to the "key encryption key". For snapshots, we don't have akey_metadata field - instead, we keep a key-id that points to the encrypted-keys table entry (and there, we have the key encryption key id)

A (technically) straightforward solution would be to add a key-id field to the statistics struct (and deprecate the key_metadatafield), but I don't know how practical it is wrt the spec process, V4 etc.

There might be less straightforward solutions that don't require spec changes, but they either are looking not great (like packing everything needed inside the key_metadata in a custom format) or have substantial performance implications (like calling KMS for each file/snapshot, instead of using a cached key encryption key). The latter also has some security issues.

[gaborkaszab]

Thanks for pointing this out @ggershinsky and @RussellSpitzer !
I wasn't entirely sure how these key_metadata fields work, but after reading your comments I agree we can't keep them in the metadata.json. I'll explore the approach involving encryption-keys and key-id.