laskoviymishka commented on code in PR #3236:
URL: https://github.com/apache/iceberg-rust/pull/3236#discussion_r4044955201
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -492,7 +492,20 @@ impl<'a> SnapshotProducer<'a> {
manifest_list_writer.add_manifests(new_manifests.into_iter())?;
let writer_next_row_id = manifest_list_writer.next_row_id();
- manifest_list_writer.close().await?;
+ let file_metadata = manifest_list_writer.close().await?;
+ let encryption_key_id = if let Some(key_metadata) = key_metadata {
+ Some(
+ self.table
+ .encryption_manager()
+ .expect("Encryption manager must be present when key
metadata exists")
Review Comment:
This is the `.expect()` from round one — still a panic on the commit path.
The invariant holds today (we only set `key_metadata` in the `Some(em)` arm),
but a second `encryption_manager()` lookup after the `await` is exactly the
kind of thing a later refactor quietly breaks, and then a commit panics instead
of erroring.
Rather than re-look-it-up, I'd capture the manager in the match arm up top
and reuse it here — hold `Some((em.clone(),
encrypted_output.key_metadata().clone()))` — then
`em.encrypt_manifest_list_key_metadata(&key_metadata.with_file_length(file_metadata.size)).await?`.
That drops the `.expect()` and the redundant second call in one go.
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -746,6 +746,7 @@ impl ArrowAsyncFileWriter for AsyncFileWriter {
self.0
.close()
.await
+ .map(|_| ())
Review Comment:
This `.map(|_| ())` is where encrypted data files fall out of the new
scheme: `close()` now hands back the ciphertext size, but the Arrow adaptor
drops it, so a data file's `StandardKeyMetadata` never gets `file_length` and
the truncation check can't fire on read the way it now does for manifests.
I don't think we need to solve it here — the `ArrowAsyncFileWriter` surface
makes it awkward — but I'd leave a `// TODO(encryption): wire file_length once
ArrowAsyncFileWriter can surface it` or open a tracking issue so the obligation
doesn't get lost. wdyt?
##########
crates/iceberg/src/encryption/io.rs:
##########
@@ -138,8 +153,8 @@ impl EncryptedOutputFile {
)))
}
- /// Write bytes to file (transparently encrypted).
- pub async fn write(&self, bs: Bytes) -> Result<()> {
+ /// Write bytes to the file and return its encrypted size.
+ pub async fn write(&self, bs: Bytes) -> Result<FileMetadata> {
Review Comment:
Thanks for having `write()` return the size — that's the piece I wanted. The
sharp edge from last round is still here though: `key_metadata()` keeps
returning length-less metadata, so every call site has to remember the
`output.key_metadata().clone().with_file_length(file_metadata.size).encode()`
dance, and the one that forgets writes a file that only fails at read time.
All four call sites do it right today, so this isn't blocking. But I'd like
the type to make it hard to get wrong — either `write()` returns
`(FileMetadata, StandardKeyMetadata)` with the length already stamped, or a
small `WriteSummary` that vends `into_key_metadata()`. wdyt?
##########
crates/iceberg/src/encryption/key_metadata.rs:
##########
@@ -90,7 +90,7 @@ impl StandardKeyMetadata {
self.aad_prefix.as_deref()
}
- /// Returns the optional file length.
+ /// Returns the encrypted file length in bytes, required for AGS1 files.
Review Comment:
Small thing: the doc now says "required for AGS1 files" but the return is
still `Option<u64>`, which reads as a contradiction. The field is genuinely
optional in the spec schema and absent on anything written before this PR —
it's the AGS1 read path that now hard-requires it.
Maybe "Returns the encrypted file length. Must be set before opening an AGS1
reader; absent on files written before this change." Keeps the `Option` honest.
##########
crates/iceberg/src/encryption/stream.rs:
##########
@@ -71,8 +71,7 @@ pub const GCM_STREAM_MAGIC: [u8; 4] = *b"AGS1";
pub const GCM_STREAM_HEADER_LENGTH: u32 = 8;
/// Minimum valid AGS1 stream length (header + one empty block).
-#[cfg(test)]
-pub const MIN_STREAM_LENGTH: u32 = GCM_STREAM_HEADER_LENGTH + NONCE_LENGTH +
GCM_TAG_LENGTH;
+pub(crate) const MIN_STREAM_LENGTH: u32 = GCM_STREAM_HEADER_LENGTH +
NONCE_LENGTH + GCM_TAG_LENGTH;
Review Comment:
Now that this is `pub(crate)`, one gap worth closing while we're here:
`EncryptedInputFile::encrypted_length()` rejects anything below
`MIN_STREAM_LENGTH`, but `AesGcmFileRead::new()` is public and takes the length
directly with no such check. Construct it with `stream_length ==
GCM_STREAM_HEADER_LENGTH` and `num_blocks` is 0, so reads return empty bytes
with no GCM verification at all.
I'd push the `< MIN_STREAM_LENGTH` check down into `new()` so `num_blocks >=
1` holds by construction — then the guard inside `read()` becomes unreachable
rather than a live bypass for direct callers. Not blocking, but it closes the
gap for the public entry point.
##########
crates/iceberg/src/encryption/stream.rs:
##########
@@ -640,6 +651,68 @@ mod tests {
assert!(result.is_empty());
}
+ #[tokio::test]
+ async fn test_short_ciphertext_read_is_rejected() {
+ struct ShortRead;
+
+ #[async_trait::async_trait]
+ impl FileRead for ShortRead {
+ async fn read(&self, range: Range<u64>) -> Result<Bytes> {
+ Ok(Bytes::from(vec![0; (range.end - range.start - 1) as
usize]))
Review Comment:
Tiny thing in the helper: `range.end - range.start - 1` is unsaturated
`u64`, so a zero-length range panics in debug and wraps to a giant allocation
in release. It doesn't fire with today's ranges, but since it's shaped like a
real `FileRead`, `let len = (range.end - range.start).saturating_sub(1) as
usize;` avoids the trap if it's ever reused.
--
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]
