feat(encryption) [6/N] streaming block encryption - #3969
xanderbailey wants to merge 3 commits into
Conversation
|
@kevinjqliu ready for review when you get the time! Thanks! |
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
@xanderbailey thanks for this. The comments are about spec compliance on the read side (trusted file length and empty files), how much of this should be public before a consumer exists, and cross-client test coverage.
| be reordered or moved between files. Byte-compatible with Java's `AesGcmInputStream` and | ||
| `AesGcmOutputStream`, and with iceberg-rust. |
There was a problem hiding this comment.
Only the header is checked against Java's bytes so far. Could the cross-client fixture issue I asked for on #3968 include AGS1 files written by Java's AesGcmOutputStream: an empty file, a single partial block, and a block-aligned multi-block file? The issue should also list the close-path tests from apache/iceberg-rust#2286 for the output stream PR, so a block-aligned write doesn't add a trailing empty block.
There was a problem hiding this comment.
I wonder if https://github.com/apache/iceberg-verification is the best place for these fixtures to land?
There was a problem hiding this comment.
Okay I've actually got claude to generate test cases using the java 1.11 jars and checked them into this PR. I'll open a ticket in this repo also to track moving them
| MAX_BLOCKS = 2 ** (8 * BLOCK_INDEX_LENGTH) - 1 | ||
|
|
||
|
|
||
| def stream_block_aad(aad_prefix: bytes | None, block_index: int) -> bytes: |
There was a problem hiding this comment.
This PR has the format primitives but no encrypting or decrypting stream yet, so stream_block_aad, calculate_plaintext_length, and Ags1Layout become public API with no consumer. iceberg-rust keeps stream_block_aad pub(crate). This is the same question as MemoryKeyManagementClient on #3968. Could you prefix these with _ until the reader and writer land, or say in the PR description which upcoming PR consumes them and why they need to be public?
| class Ags1Layout: | ||
| """Where each block of an AGS1 stream sits, derived from the encrypted file length. | ||
|
|
||
| Only the final block may hold less than `PLAIN_BLOCK_SIZE` of plaintext, so the layout | ||
| follows from the encrypted length alone, without reading the stream. | ||
| """ | ||
|
|
There was a problem hiding this comment.
Could the docstring say that encrypted_length must be the trusted length from StandardKeyMetadata.file_length (key_metadata.py#L52), never a file system stat? The spec's File length section requires this. Otherwise an attacker can drop whole trailing blocks, and every remaining block still authenticates. Java deprecated the AesGcmInputFile constructor without a length because it's "not safe". apache/iceberg-rust#3236 now makes a missing file_length a hard error on read, because Java can't read files written without it. Please also file an issue under #3222 so the reader PR fails when file_length is None, and link it here.
| def from_encrypted_length(cls, encrypted_length: int) -> Ags1Layout: | ||
| """Derive the layout of an AGS1 stream that occupies `encrypted_length` bytes.""" | ||
| plaintext_length = calculate_plaintext_length(encrypted_length) | ||
| stream_length = encrypted_length - GCM_STREAM_HEADER_LENGTH | ||
| if stream_length == 0: | ||
| return cls(plaintext_length=0, num_blocks=0, last_cipher_block_size=0) |
There was a problem hiding this comment.
Accepting a header-only stream here follows the spec, which says the last block has a non-zero length, so an empty plaintext has no blocks. Java disagrees in both directions. AesGcmOutputStream encrypts one empty block on close for an empty file (the currentBlockIndex != 0 guard only skips the trailing block once a block exists). AesGcmInputFile rejects anything shorter than MIN_STREAM_LENGTH, the header plus one empty block. Accepting both forms on read seems right to me. On write, a PyIceberg writer that follows the spec for an empty file would produce 8 bytes that Java refuses to open. Could you open an issue on apache/iceberg about the discrepancy and link it here, so the output stream PR has a settled answer on which form to write?
There was a problem hiding this comment.
Rust and Java now actually check this MIN_STREAM_LENGTH and I think we should do the same here so I've actually made that change. Have opened an issue and I might do a mailing list about this apache/iceberg#18219
| layout.block_index_for(plaintext_offset) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("plaintext_length", [1, 100, PLAIN_BLOCK_SIZE, PLAIN_BLOCK_SIZE + 7, 2 * PLAIN_BLOCK_SIZE]) |
There was a problem hiding this comment.
Could 0 be added to these cases? build_stream(b"") produces the header-only stream the spec describes, and nothing round-trips an empty plaintext yet. Java's empty-file form (header plus one empty block) is already covered as a layout case, but not decrypted, so a round-trip test that builds that form with one empty block would pin it too.
This branch has not been deployed
2 participants
Matching
Ciphers.streamBlockAADand based on the spec's definition as defined by https://iceberg.apache.org/gcm-stream-spec/#encryption-algorithmRationale for this change
Are these changes tested?
Are there any user-facing changes?