xanderbailey commented on code in PR #2183:
URL: https://github.com/apache/iceberg-rust/pull/2183#discussion_r3129641613
##########
docs/rfcs/0003_table_encryption.md:
##########
@@ -410,56 +455,67 @@ block to its position in the stream to prevent reordering
attacks.
random-access reads. **`AesGcmFileWrite`** implements `FileWrite` for
transparent AGS1
encryption with block buffering.
-### InputFile and OutputFile Enums
+### EncryptedInputFile and EncryptedOutputFile Wrappers
-`InputFile` and `OutputFile` are enums with three variants each:
+Rather than adding encryption variants to `InputFile`/`OutputFile` (which
would change the
+public API of those core types), encryption uses dedicated wrapper types.
`EncryptedInputFile`
+and `EncryptedOutputFile` wrap a plain `InputFile`/`OutputFile` and add
transparent
+encryption/decryption. The plain `InputFile`/`OutputFile` types remain
unchanged.
```rust
-pub enum InputFile {
- /// Standard unencrypted file.
- Plain { storage: Arc<dyn Storage>, path: String },
-
- /// AGS1 stream-encrypted file. Transparent decryption on read.
- Encrypted { storage: Arc<dyn Storage>, path: String, decryptor:
Arc<FileDecryptor> },
-
- /// Parquet Modular Encryption. Raw reads; Parquet reader handles
decryption.
- NativeEncrypted { storage: Arc<dyn Storage>, path: String, key_material:
NativeKeyMaterial },
+/// Wraps a plain InputFile with decryption capabilities.
+pub enum EncryptedInputFile {
+ /// AGS1 stream-encrypted file. Decrypted transparently on read.
+ Encrypted {
+ inner: InputFile,
+ decryptor: Arc<AesGcmFileDecryptor>,
+ },
+ /// Parquet Modular Encryption. The Parquet reader handles decryption.
+ NativeEncrypted {
+ inner: InputFile,
+ key_material: NativeKeyMaterial,
+ },
}
-pub enum OutputFile {
- Plain { storage: Arc<dyn Storage>, path: String },
- Encrypted { storage: Arc<dyn Storage>, path: String, encryptor:
Arc<FileEncryptor> },
- NativeEncrypted { storage: Arc<dyn Storage>, path: String, key_material:
NativeKeyMaterial },
+/// Wraps a plain OutputFile with encryption capabilities.
+pub enum EncryptedOutputFile {
+ /// AGS1 stream-encrypted output. Encrypted transparently on write.
+ Encrypted {
+ inner: OutputFile,
+ key_metadata: Box<[u8]>,
+ encryptor: Arc<AesGcmFileEncryptor>,
+ },
+ /// Parquet Modular Encryption. The Parquet writer handles encryption.
+ NativeEncrypted {
+ inner: OutputFile,
+ key_metadata: Box<[u8]>,
+ key_material: NativeKeyMaterial,
+ },
}
```
+Both wrappers delegate standard operations (`location()`, `exists()`,
`read()`, `reader()`,
+`write()`, `writer()`) to the inner file, with `Encrypted` variants
transparently
+encrypting/decrypting via `AesGcmFileRead`/`AesGcmFileWrite`. `into_inner()`
recovers
+the underlying plain file.
+
`NativeKeyMaterial` carries the plaintext DEK and AAD prefix for Parquet's
`FileEncryptionProperties` / `FileDecryptionProperties`.
-Common operations (`location()`, `exists()`, `read()`, `reader()`, `write()`,
`writer()`)
-delegate to the appropriate variant, with `Encrypted` variants transparently
encrypting/decrypting
-via `AesGcmFileRead`/`AesGcmFileWrite`.
-
-#### Adaptation for Storage Trait RFC
-
-Once RFC 0002 merges, `InputFile` will hold `Arc<dyn Storage>` instead of
`Operator`. This is
-already the case in this implementation — the enum structure is stable. Only
the underlying
-`Storage` trait implementation may change.
+This wrapper approach means `ManifestReader` and `ManifestListWriter` accept
the encrypted
+wrapper types (or `Box<dyn FileWrite>`) where encryption is needed, rather
than requiring
+changes to the `InputFile`/`OutputFile` enums themselves.
### FileIO Integration
-The `EncryptionManager` is stored as a type-safe `FileIOBuilder` extension.
This integrates
-naturally with catalogs that support extensions (e.g.
`RestCatalog.with_file_io_extension()`):
+The `EncryptionManager` is attached to `FileIO` via a convenience method. The
encryption
Review Comment:
`EncryptingFileIO` would mean more code changes since each call site would
now need to take `EncryptingFileIO` which seems wrong also somehow?
I'm going to see if I can move `EncryptionManager` out of `FileIo`
altogether and just have it on the `Table` and pass it down where it's needed.
I'll report back when I have a better idea of how this looks
--
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]
