gaborkaszab commented on code in PR #16527:
URL: https://github.com/apache/iceberg/pull/16527#discussion_r3921801463


##########
format/encryption-spec.md:
##########
@@ -0,0 +1,77 @@
+---
+title: "Encryption Spec"
+---
+<!--
+ - Licensed to the Apache Software Foundation (ASF) under one or more
+ - contributor license agreements.  See the NOTICE file distributed with
+ - this work for additional information regarding copyright ownership.
+ - The ASF licenses this file to You under the Apache License, Version 2.0
+ - (the "License"); you may not use this file except in compliance with
+ - the License.  You may obtain a copy of the License at
+ -
+ -   http://www.apache.org/licenses/LICENSE-2.0
+ -
+ - Unless required by applicable law or agreed to in writing, software
+ - distributed under the License is distributed on an "AS IS" BASIS,
+ - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ - See the License for the specific language governing permissions and
+ - limitations under the License.
+ -->
+
+# Iceberg Encryption Spec
+
+This document specifies the standard encryption scheme for Apache Iceberg 
tables (v3 and above). It defines the interoperable, cross-implementation 
binary format for per-file encryption key material referenced by the 
`key_metadata` fields in the [table spec](spec.md), along with the key 
hierarchy used to manage those keys.
+
+Encrypted key material is tracked in two places:
+
+* The `key_metadata` fields in [manifest entries](spec.md#manifests), 
[manifest list entries](spec.md#manifest-lists), and [statistics 
files](spec.md#table-metadata-fields) store the per-file key material.
+* The table metadata [`encryption-keys`](spec.md#encryption-keys) list tracks 
the key hierarchy used to protect that per-file material.

Review Comment:
   If I'm not mistaken `encrypted-keys` in table metadata is not to protect the 
key_metadata fields listed above.



##########
format/encryption-spec.md:
##########
@@ -0,0 +1,77 @@
+---
+title: "Encryption Spec"
+---
+<!--
+ - Licensed to the Apache Software Foundation (ASF) under one or more
+ - contributor license agreements.  See the NOTICE file distributed with
+ - this work for additional information regarding copyright ownership.
+ - The ASF licenses this file to You under the Apache License, Version 2.0
+ - (the "License"); you may not use this file except in compliance with
+ - the License.  You may obtain a copy of the License at
+ -
+ -   http://www.apache.org/licenses/LICENSE-2.0
+ -
+ - Unless required by applicable law or agreed to in writing, software
+ - distributed under the License is distributed on an "AS IS" BASIS,
+ - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ - See the License for the specific language governing permissions and
+ - limitations under the License.
+ -->
+
+# Iceberg Encryption Spec
+
+This document specifies the standard encryption scheme for Apache Iceberg 
tables (v3 and above). It defines the interoperable, cross-implementation 
binary format for per-file encryption key material referenced by the 
`key_metadata` fields in the [table spec](spec.md), along with the key 
hierarchy used to manage those keys.
+
+Encrypted key material is tracked in two places:
+
+* The `key_metadata` fields in [manifest entries](spec.md#manifests), 
[manifest list entries](spec.md#manifest-lists), and [statistics 
files](spec.md#table-metadata-fields) store the per-file key material.
+* The table metadata [`encryption-keys`](spec.md#encryption-keys) list tracks 
the key hierarchy used to protect that per-file material.
+
+## Standard Key Metadata
+
+The `key_metadata` field in manifest entries stores per-file encryption key 
material as a binary blob. To enable cross-implementation interoperability, the 
standard encryption scheme defines the following binary format for this field:

Review Comment:
   I think the spec's purpose is cross-implementation interoperability, no need 
to spell it out: "To enable cross-implementation interoperability". I think we 
can remove this part of the sentence and just say that the this is the scheme 
for `key_metadata`



##########
format/encryption-spec.md:
##########
@@ -0,0 +1,77 @@
+---
+title: "Encryption Spec"
+---
+<!--
+ - Licensed to the Apache Software Foundation (ASF) under one or more
+ - contributor license agreements.  See the NOTICE file distributed with
+ - this work for additional information regarding copyright ownership.
+ - The ASF licenses this file to You under the Apache License, Version 2.0
+ - (the "License"); you may not use this file except in compliance with
+ - the License.  You may obtain a copy of the License at
+ -
+ -   http://www.apache.org/licenses/LICENSE-2.0
+ -
+ - Unless required by applicable law or agreed to in writing, software
+ - distributed under the License is distributed on an "AS IS" BASIS,
+ - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ - See the License for the specific language governing permissions and
+ - limitations under the License.
+ -->
+
+# Iceberg Encryption Spec
+
+This document specifies the standard encryption scheme for Apache Iceberg 
tables (v3 and above). It defines the interoperable, cross-implementation 
binary format for per-file encryption key material referenced by the 
`key_metadata` fields in the [table spec](spec.md), along with the key 
hierarchy used to manage those keys.
+
+Encrypted key material is tracked in two places:
+
+* The `key_metadata` fields in [manifest entries](spec.md#manifests), 
[manifest list entries](spec.md#manifest-lists), and [statistics 
files](spec.md#table-metadata-fields) store the per-file key material.

Review Comment:
   `key_metadata` in table stats files is going to be deprecated here: 
https://github.com/apache/iceberg/pull/17533
   Listing where the field is found now might not age well if the list changes 
by time.



##########
format/encryption-spec.md:
##########
@@ -0,0 +1,77 @@
+---
+title: "Encryption Spec"
+---
+<!--
+ - Licensed to the Apache Software Foundation (ASF) under one or more
+ - contributor license agreements.  See the NOTICE file distributed with
+ - this work for additional information regarding copyright ownership.
+ - The ASF licenses this file to You under the Apache License, Version 2.0
+ - (the "License"); you may not use this file except in compliance with
+ - the License.  You may obtain a copy of the License at
+ -
+ -   http://www.apache.org/licenses/LICENSE-2.0
+ -
+ - Unless required by applicable law or agreed to in writing, software
+ - distributed under the License is distributed on an "AS IS" BASIS,
+ - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ - See the License for the specific language governing permissions and
+ - limitations under the License.
+ -->
+
+# Iceberg Encryption Spec
+
+This document specifies the standard encryption scheme for Apache Iceberg 
tables (v3 and above). It defines the interoperable, cross-implementation 
binary format for per-file encryption key material referenced by the 
`key_metadata` fields in the [table spec](spec.md), along with the key 
hierarchy used to manage those keys.
+
+Encrypted key material is tracked in two places:
+
+* The `key_metadata` fields in [manifest entries](spec.md#manifests), 
[manifest list entries](spec.md#manifest-lists), and [statistics 
files](spec.md#table-metadata-fields) store the per-file key material.
+* The table metadata [`encryption-keys`](spec.md#encryption-keys) list tracks 
the key hierarchy used to protect that per-file material.
+
+## Standard Key Metadata
+
+The `key_metadata` field in manifest entries stores per-file encryption key 
material as a binary blob. To enable cross-implementation interoperability, the 
standard encryption scheme defines the following binary format for this field:

Review Comment:
   I'd rather remove reference to the table format such as mentioning that 
manifest entries have a `key-metadata` field. I think that in the encryption 
spec we should be more general and not assuming that which table metadata 
structs have what fields.



##########
format/encryption-spec.md:
##########
@@ -0,0 +1,77 @@
+---
+title: "Encryption Spec"
+---
+<!--
+ - Licensed to the Apache Software Foundation (ASF) under one or more
+ - contributor license agreements.  See the NOTICE file distributed with
+ - this work for additional information regarding copyright ownership.
+ - The ASF licenses this file to You under the Apache License, Version 2.0
+ - (the "License"); you may not use this file except in compliance with
+ - the License.  You may obtain a copy of the License at
+ -
+ -   http://www.apache.org/licenses/LICENSE-2.0
+ -
+ - Unless required by applicable law or agreed to in writing, software
+ - distributed under the License is distributed on an "AS IS" BASIS,
+ - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ - See the License for the specific language governing permissions and
+ - limitations under the License.
+ -->
+
+# Iceberg Encryption Spec
+
+This document specifies the standard encryption scheme for Apache Iceberg 
tables (v3 and above). It defines the interoperable, cross-implementation 
binary format for per-file encryption key material referenced by the 
`key_metadata` fields in the [table spec](spec.md), along with the key 
hierarchy used to manage those keys.
+
+Encrypted key material is tracked in two places:
+
+* The `key_metadata` fields in [manifest entries](spec.md#manifests), 
[manifest list entries](spec.md#manifest-lists), and [statistics 
files](spec.md#table-metadata-fields) store the per-file key material.
+* The table metadata [`encryption-keys`](spec.md#encryption-keys) list tracks 
the key hierarchy used to protect that per-file material.
+
+## Standard Key Metadata
+
+The `key_metadata` field in manifest entries stores per-file encryption key 
material as a binary blob. To enable cross-implementation interoperability, the 
standard encryption scheme defines the following binary format for this field:
+
+```
+VersionByte Payload
+```
+
+where:
+
+* `VersionByte` is a single byte indicating the key metadata schema version. 
Currently, the only valid version is `0x01`.
+* `Payload` is an Avro binary-encoded record (not a container file — only the 
raw binary encoding of the fields) using the schema for the given version.
+
+The Avro schema for version 1 is a record with the following fields, in order:
+
+| Field name | Avro type | Required | Description |
+|---|---|---|---|
+| **`encryption_key`** | `bytes` | _required_ | The data encryption key (DEK) 
for this file. Must be 16, 24, or 32 bytes (corresponding to AES-128, AES-192, 
or AES-256). |
+| **`aad_prefix`** | `bytes` | _optional_ | Random AAD prefix used for 
encryption integrity protection. For [AES GCM Stream](gcm-stream-spec.md) 
files, the prefix is combined with a block index to form the per-block AAD. For 
[Parquet modular 
encryption](https://parquet.apache.org/docs/file-format/data-pages/encryption/),
 the prefix is passed as the AAD prefix parameter, which is combined with a 
module AAD suffix to form the full AAD for each Parquet module. |
+| **`file_length`** | `long` | _optional_ | The encrypted file length in 
bytes. Required for [AES GCM Stream](gcm-stream-spec.md) encrypted files to 
detect truncation attacks (see [AES GCM Stream file 
length](gcm-stream-spec.md#file-length)). Not set for Parquet encrypted files. |
+
+The usage of the `encryption_key` and `aad_prefix` fields depends on the file 
format:
+
+* **AES GCM Stream files**:
+  - Manifest lists
+  - Manifests
+  - Avro data files
+  - Puffin files
+
+  The `encryption_key` is used directly as the AES-GCM key. The `aad_prefix` 
is combined with a 4-byte little-endian block index to form the AAD for each 
cipher block, as described in the [AES GCM Stream AAD 
section](gcm-stream-spec.md#additional-authenticated-data). The `file_length` 
field stores the encrypted file length for truncation detection.
+
+* **Parquet encrypted files**: The `encryption_key` and `aad_prefix` are 
provided to Parquet readers and writers, which delegate encryption to the 
[Parquet modular 
encryption](https://parquet.apache.org/docs/file-format/data-pages/encryption/) 
format.
+
+### Encryption Key Hierarchy
+
+The standard encryption scheme uses a two-tier key hierarchy tracked in the 
table metadata [`encryption-keys`](spec.md#encryption-keys) list:

Review Comment:
   This gives the impression that all encryption related information should go 
into this structure. I think that what is true instead is that there are 2 
different way: 1) raw key-metadata stored per file as described in the above 
sections, or 2) key-id driven encrypted key-metadata stored together (where 
sharing between encrypted files is possible).
   I think we should articulate this more.
   
   Also, for 2) it's essential to have a KMS. Might make sense to dedicate a 
sort paragraph to describe what it is and what assumptions we might have.



##########
format/encryption-spec.md:
##########
@@ -0,0 +1,77 @@
+---
+title: "Encryption Spec"
+---
+<!--
+ - Licensed to the Apache Software Foundation (ASF) under one or more
+ - contributor license agreements.  See the NOTICE file distributed with
+ - this work for additional information regarding copyright ownership.
+ - The ASF licenses this file to You under the Apache License, Version 2.0
+ - (the "License"); you may not use this file except in compliance with
+ - the License.  You may obtain a copy of the License at
+ -
+ -   http://www.apache.org/licenses/LICENSE-2.0
+ -
+ - Unless required by applicable law or agreed to in writing, software
+ - distributed under the License is distributed on an "AS IS" BASIS,
+ - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ - See the License for the specific language governing permissions and
+ - limitations under the License.
+ -->
+
+# Iceberg Encryption Spec
+
+This document specifies the standard encryption scheme for Apache Iceberg 
tables (v3 and above). It defines the interoperable, cross-implementation 
binary format for per-file encryption key material referenced by the 
`key_metadata` fields in the [table spec](spec.md), along with the key 
hierarchy used to manage those keys.
+
+Encrypted key material is tracked in two places:
+
+* The `key_metadata` fields in [manifest entries](spec.md#manifests), 
[manifest list entries](spec.md#manifest-lists), and [statistics 
files](spec.md#table-metadata-fields) store the per-file key material.
+* The table metadata [`encryption-keys`](spec.md#encryption-keys) list tracks 
the key hierarchy used to protect that per-file material.
+
+## Standard Key Metadata
+
+The `key_metadata` field in manifest entries stores per-file encryption key 
material as a binary blob. To enable cross-implementation interoperability, the 
standard encryption scheme defines the following binary format for this field:
+
+```
+VersionByte Payload
+```
+
+where:
+
+* `VersionByte` is a single byte indicating the key metadata schema version. 
Currently, the only valid version is `0x01`.
+* `Payload` is an Avro binary-encoded record (not a container file — only the 
raw binary encoding of the fields) using the schema for the given version.
+
+The Avro schema for version 1 is a record with the following fields, in order:
+
+| Field name | Avro type | Required | Description |
+|---|---|---|---|
+| **`encryption_key`** | `bytes` | _required_ | The data encryption key (DEK) 
for this file. Must be 16, 24, or 32 bytes (corresponding to AES-128, AES-192, 
or AES-256). |
+| **`aad_prefix`** | `bytes` | _optional_ | Random AAD prefix used for 
encryption integrity protection. For [AES GCM Stream](gcm-stream-spec.md) 
files, the prefix is combined with a block index to form the per-block AAD. For 
[Parquet modular 
encryption](https://parquet.apache.org/docs/file-format/data-pages/encryption/),
 the prefix is passed as the AAD prefix parameter, which is combined with a 
module AAD suffix to form the full AAD for each Parquet module. |
+| **`file_length`** | `long` | _optional_ | The encrypted file length in 
bytes. Required for [AES GCM Stream](gcm-stream-spec.md) encrypted files to 
detect truncation attacks (see [AES GCM Stream file 
length](gcm-stream-spec.md#file-length)). Not set for Parquet encrypted files. |
+
+The usage of the `encryption_key` and `aad_prefix` fields depends on the file 
format:
+
+* **AES GCM Stream files**:

Review Comment:
   For a regular reader "AES GCM Stream files" might not mean much in an 
Iceberg spec. Taking a look at the list below, isn't this AVRO files and Puffin 
files?



##########
format/encryption-spec.md:
##########
@@ -0,0 +1,77 @@
+---
+title: "Encryption Spec"
+---
+<!--
+ - Licensed to the Apache Software Foundation (ASF) under one or more
+ - contributor license agreements.  See the NOTICE file distributed with
+ - this work for additional information regarding copyright ownership.
+ - The ASF licenses this file to You under the Apache License, Version 2.0
+ - (the "License"); you may not use this file except in compliance with
+ - the License.  You may obtain a copy of the License at
+ -
+ -   http://www.apache.org/licenses/LICENSE-2.0
+ -
+ - Unless required by applicable law or agreed to in writing, software
+ - distributed under the License is distributed on an "AS IS" BASIS,
+ - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ - See the License for the specific language governing permissions and
+ - limitations under the License.
+ -->
+
+# Iceberg Encryption Spec
+
+This document specifies the standard encryption scheme for Apache Iceberg 
tables (v3 and above). It defines the interoperable, cross-implementation 
binary format for per-file encryption key material referenced by the 
`key_metadata` fields in the [table spec](spec.md), along with the key 
hierarchy used to manage those keys.
+
+Encrypted key material is tracked in two places:
+
+* The `key_metadata` fields in [manifest entries](spec.md#manifests), 
[manifest list entries](spec.md#manifest-lists), and [statistics 
files](spec.md#table-metadata-fields) store the per-file key material.
+* The table metadata [`encryption-keys`](spec.md#encryption-keys) list tracks 
the key hierarchy used to protect that per-file material.
+
+## Standard Key Metadata
+
+The `key_metadata` field in manifest entries stores per-file encryption key 
material as a binary blob. To enable cross-implementation interoperability, the 
standard encryption scheme defines the following binary format for this field:
+
+```
+VersionByte Payload
+```
+
+where:
+
+* `VersionByte` is a single byte indicating the key metadata schema version. 
Currently, the only valid version is `0x01`.
+* `Payload` is an Avro binary-encoded record (not a container file — only the 
raw binary encoding of the fields) using the schema for the given version.
+
+The Avro schema for version 1 is a record with the following fields, in order:
+
+| Field name | Avro type | Required | Description |
+|---|---|---|---|
+| **`encryption_key`** | `bytes` | _required_ | The data encryption key (DEK) 
for this file. Must be 16, 24, or 32 bytes (corresponding to AES-128, AES-192, 
or AES-256). |
+| **`aad_prefix`** | `bytes` | _optional_ | Random AAD prefix used for 
encryption integrity protection. For [AES GCM Stream](gcm-stream-spec.md) 
files, the prefix is combined with a block index to form the per-block AAD. For 
[Parquet modular 
encryption](https://parquet.apache.org/docs/file-format/data-pages/encryption/),
 the prefix is passed as the AAD prefix parameter, which is combined with a 
module AAD suffix to form the full AAD for each Parquet module. |
+| **`file_length`** | `long` | _optional_ | The encrypted file length in 
bytes. Required for [AES GCM Stream](gcm-stream-spec.md) encrypted files to 
detect truncation attacks (see [AES GCM Stream file 
length](gcm-stream-spec.md#file-length)). Not set for Parquet encrypted files. |
+
+The usage of the `encryption_key` and `aad_prefix` fields depends on the file 
format:
+
+* **AES GCM Stream files**:
+  - Manifest lists
+  - Manifests
+  - Avro data files
+  - Puffin files
+
+  The `encryption_key` is used directly as the AES-GCM key. The `aad_prefix` 
is combined with a 4-byte little-endian block index to form the AAD for each 
cipher block, as described in the [AES GCM Stream AAD 
section](gcm-stream-spec.md#additional-authenticated-data). The `file_length` 
field stores the encrypted file length for truncation detection.
+
+* **Parquet encrypted files**: The `encryption_key` and `aad_prefix` are 
provided to Parquet readers and writers, which delegate encryption to the 
[Parquet modular 
encryption](https://parquet.apache.org/docs/file-format/data-pages/encryption/) 
format.
+
+### Encryption Key Hierarchy
+
+The standard encryption scheme uses a two-tier key hierarchy tracked in the 
table metadata [`encryption-keys`](spec.md#encryption-keys) list:
+
+1. **Key Encryption Keys (KEKs):** Entries where `encrypted-by-id` equals the 
table's encryption key ID (configured via `encryption.key-id`). The 
`encrypted-key-metadata` contains the KEK wrapped by the KMS and is opaque to 
Iceberg — its format is determined by the KMS provider. KEK entries must 
include a `KEY_TIMESTAMP` property recording the creation time in milliseconds 
since epoch; this timestamp is used as the AAD when encrypting manifest list 
key metadata.
+
+2. **Manifest List Keys:** Entries where `encrypted-by-id` references a KEK. 
The `encrypted-key-metadata` contains the Standard Key Metadata (defined above) 
encrypted with AES GCM using the referenced unwrapped KEK. The ciphertext 
format is:

Review Comment:
   Saying "Manifest List" is too narrow IMO. In V4, root manifest files, and as 
a future enhancement, stat files could also store their encryption metadata 
within this list.



##########
format/spec.md:
##########
@@ -731,7 +664,7 @@ The `data_file` struct consists of the following fields:
     | _optional_ | _optional_ |            | ~~**`111  distinct_counts`**~~    
| `map<123: int, 124: long>`                                                  | 
**Deprecated. Do not write.** |
     | _optional_ | _optional_ | _optional_ | **`125  lower_bounds`**           
| `map<126: int, 127: binary>`                                                | 
Map from column id to lower bound in the column serialized as binary [1]. Each 
value must be less than or equal to all non-null, non-NaN values in the column 
for the file [2] |
     | _optional_ | _optional_ | _optional_ | **`128  upper_bounds`**           
| `map<129: int, 130: binary>`                                                | 
Map from column id to upper bound in the column serialized as binary [1]. Each 
value must be greater than or equal to all non-null, non-Nan values in the 
column for the file [2] |
-    | _optional_ | _optional_ | _optional_ | **`131  key_metadata`**           
| `binary`                                                                    | 
Implementation-specific key metadata for encryption |
+    | _optional_ | _optional_ | _optional_ | **`131  key_metadata`**           
| `binary`                                                                    | 
Per-file encryption key metadata. See [Standard Key 
Metadata](encryption-spec.md#standard-key-metadata) for the interoperable 
format used by the standard encryption scheme. |

Review Comment:
   I think this should be enough:
   "Per-file encryption key metadata. See [Standard Key 
Metadata](encryption-spec.md#standard-key-metadata)."
   Same goes for other occurrences.



-- 
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]

Reply via email to