feat(encryption) [6/N] streaming block encryption by xanderbailey · Pull Request #3969 · apache/iceberg-python

xanderbailey · 2026-09-15T08:48:29Z

Matching Ciphers.streamBlockAAD and based on the spec's definition as defined by https://iceberg.apache.org/gcm-stream-spec/#encryption-algorithm

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

xanderbailey · 2026-09-18T13:07:58Z

@kevinjqliu ready for review when you get the time! Thanks!

mbutrovich

@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.

mbutrovich · 2026-09-22T20:37:52Z

+be reordered or moved between files. Byte-compatible with Java's `AesGcmInputStream` and
+`AesGcmOutputStream`, and with iceberg-rust.


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.

I wonder if https://github.com/apache/iceberg-verification is the best place for these fixtures to land?

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

mbutrovich · 2026-09-22T20:38:05Z

+MAX_BLOCKS = 2 ** (8 * BLOCK_INDEX_LENGTH) - 1
+
+
+def stream_block_aad(aad_prefix: bytes | None, block_index: int) -> bytes:


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?

This one is still open at the head commit. For reference, apache/iceberg-rust#3236 also narrows MIN_STREAM_LENGTH to pub(crate), so iceberg-rust keeps both of these format details out of its public API. The tests can import _-prefixed names, so making them private doesn't cost any coverage.

mbutrovich · 2026-09-22T20:38:18Z

+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.
+    """
+


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.

The docstrings on calculate_plaintext_length and from_encrypted_length cover the first part, thanks. Could you still file the issue for the reader failing on a missing file_length and link it here? Without it, nothing tracks that requirement until the reader PR shows up.

mbutrovich · 2026-09-22T20:38:27Z

+    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)


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?

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

Thanks for filing apache/iceberg#18219. iceberg-rust main still accepts a header-only stream: calculate_plaintext_length returns 0 for a stream length of 0. The MIN_STREAM_LENGTH check is in apache/iceberg-rust#3236, which is still open. So the README's "as does iceberg-rust" isn't true of any released or merged Rust code yet.

The bigger question is that the spec's form of an empty file is the header alone, since the last block must have a non-zero length. With this change, PyIceberg refuses the one encoding the spec defines. The trusted file_length already rules out truncation, so accepting 8 bytes on read doesn't open an attack. Could the reader accept both forms (header only, and header plus one empty block) until #18219 settles which one writers produce? That keeps the writer question open for the output stream PR without making PyIceberg reject spec-compliant files. The module docstring (line 26) and the README (lines 36-40) would then describe the discrepancy and link #18219, instead of stating Java's behavior as the format.

Was optimistically hoping the rust PR would merge before this one

mbutrovich · 2026-09-22T20:38:37Z

+        layout.block_index_for(plaintext_offset)
+
+
+@pytest.mark.parametrize("plaintext_length", [1, 100, PLAIN_BLOCK_SIZE, PLAIN_BLOCK_SIZE + 7, 2 * PLAIN_BLOCK_SIZE])


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.

The Java empty form is now decrypted through empty.ags1 in test_decrypts_a_java_written_stream, which covers the second half. The first half depends on the empty-stream thread. Today build_stream(b"") produces a header-only stream that from_encrypted_length rejects, so the test helper that stands in for an output stream can't round-trip an empty file. If the reader accepts both forms, 0 belongs in this list. If it doesn't, build_stream should write the empty block so the helper matches what the reader accepts.

mbutrovich

@xanderbailey thanks for adding the Java fixtures and the trusted-length docstring. The new comments are about the empty-stream decision on read, a duplicated length calculation, and the fixture README, which is failing markdown-link-check.

mbutrovich · 2026-09-23T17:11:55Z

+        plaintext_length = calculate_plaintext_length(encrypted_length)
+        stream_length = encrypted_length - GCM_STREAM_HEADER_LENGTH
+        full_blocks, cipher_bytes_in_last_block = divmod(stream_length, CIPHER_BLOCK_SIZE)
+        if cipher_bytes_in_last_block == 0:
+            num_blocks, last_cipher_block_size = full_blocks, CIPHER_BLOCK_SIZE
+        else:
+            num_blocks, last_cipher_block_size = full_blocks + 1, cipher_bytes_in_last_block
+
+        if num_blocks > MAX_BLOCKS:
+            raise ValueError(f"AGS1 streams hold at most {MAX_BLOCKS} blocks, but {encrypted_length} bytes needs {num_blocks}")
+
+        return cls(plaintext_length=plaintext_length, num_blocks=num_blocks, last_cipher_block_size=last_cipher_block_size)


from_encrypted_length repeats the stream_length and divmod work that calculate_plaintext_length just did (lines 91-92), so the block layout rule lives in two places. Could from_encrypted_length do the validation and the divmod once, and derive plaintext_length as (num_blocks - 1) * PLAIN_BLOCK_SIZE + last_cipher_block_size - BLOCK_OVERHEAD? calculate_plaintext_length can then return Ags1Layout.from_encrypted_length(encrypted_length).plaintext_length, or go away if the public API thread ends with only Ags1Layout. It also means the MAX_BLOCKS check applies to both entry points, where today calculate_plaintext_length accepts lengths that from_encrypted_length rejects.

mbutrovich · 2026-09-23T17:12:19Z

+| `empty.ags1` | 36 B | 0 B | Java writes an 8 byte header **plus one empty block** for an empty file, not a bare header |
+| `partial-block.ags1` | 136 B | 100 B | Header, nonce/tag layout, and a single short block |
+| `partial-block-no-aad.ags1` | 136 B | 100 B | The same stream with a null AAD prefix, so the block index alone is the AAD |
+| `aligned-multi-block.ags1` | 2097216 B | 2 MiB | Two full blocks: the little-endian block index in each block's AAD, and that a block-aligned write appends **no** trailing empty block |


MANIFEST.in has recursive-include tests *, so these fixtures ship in every sdist, and they add about 2 MiB to a tests/ directory that is 4.8 MB today. They also stay in git history after #4010 moves them to iceberg-verification. Is that the trade-off the maintainers want, or should the fixtures land in iceberg-verification first and be fetched from there? @kevinjqliu, what do you think?

mbutrovich · 2026-09-23T17:12:34Z

+<!-- markdown-link-check-disable-next-line -->
+```bash
+V=1.11.0
+for a in iceberg-core iceberg-api iceberg-bundled-guava; do
+  curl -sfLO "https://repo1.maven.org/maven2/org/apache/iceberg/$a/$V/$a-$V.jar"


This README fails the markdown-link-check job on the head commit: the templated URL https://repo1.maven.org/maven2/org/apache/iceberg/$a/$V/$a-$V.jar returns 404 (run). disable-next-line only covers line 66, the opening fence, and the URL is on line 69. Wrapping the block with  and  should fix it.

feat(encryption) [6/N] streaming encryption

20d65e3

mbutrovich reviewed Sep 22, 2026

View reviewed changes

xanderbailey mentioned this pull request Sep 23, 2026

Add AES GCM streaming encryption test cases apache/iceberg-verification#11

Open

xanderbailey added 2 commits September 23, 2026 09:25

doc strings

bffd261

test fictures

f5baaa5

This was referenced Sep 23, 2026

Move AES GCM fixtures to Iceberg-verification #4010

Open

AES GCM spec vs implementation discrepancy apache/iceberg#18219

Open

mbutrovich reviewed Sep 23, 2026

View reviewed changes

		be reordered or moved between files. Byte-compatible with Java's `AesGcmInputStream` and
		`AesGcmOutputStream`, and with iceberg-rust.

		MAX_BLOCKS = 2 ** (8 * BLOCK_INDEX_LENGTH) - 1


		def stream_block_aad(aad_prefix: bytes \| None, block_index: int) -> bytes:

		layout.block_index_for(plaintext_offset)


		@pytest.mark.parametrize("plaintext_length", [1, 100, PLAIN_BLOCK_SIZE, PLAIN_BLOCK_SIZE + 7, 2 * PLAIN_BLOCK_SIZE])

Conversation

xanderbailey commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

Uh oh!

xanderbailey commented Sep 18, 2026

Uh oh!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

xanderbailey commented Sep 15, 2026 •

edited

Loading