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.

apache/iceberg-rust#3236 has merged, so the Rust reader now fails when key metadata has no file_length (io.rs#L85-L89, tested by test_missing_file_length_is_rejected). I don't see a PyIceberg issue for the same check yet. Could you open one under #3222 and link it here, so the reader PR picks it 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

Let me have a think about this for a moment

Was optimistically hoping the rust PR would merge before this one

This has now merged

On accepting both forms, I'd rather not, and I don't think it costs us spec compliance in any way that's observable. AGS1 only ever wraps Avro (manifests, manifest lists) or Puffin, and both mandate a magic prefix - it would only move the error from "Invalid AGS1 stream: expected at least 36 bytes" to an opaque Avro header failure one layer up. I'd rather fail at the layer that can name the problem. WDYT?

That works for me. Failing at the layer that can name the problem is a good reason, and matching Java and iceberg-rust (now that #3236 merged) keeps the three readers consistent until apache/iceberg#18219 settles it.

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.

With the reader requiring one block, build_stream(b"") at the head commit still returns the 8 byte header, and _Ags1Layout.from_encrypted_length(8) raises Invalid AGS1 stream: expected at least 36 bytes, got 8. Could build_stream iterate over range(0, len(plaintext), _PLAIN_BLOCK_SIZE) or [0], and 0 go into the test_layout_describes_a_real_stream cases? The num_blocks assertion there would become max(1, -(-plaintext_length // _PLAIN_BLOCK_SIZE)). I tried that locally and the empty case produces a 36 byte stream with one block. It pins the form the output stream PR has to write.

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: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?

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

xanderbailey added 4 commits September 24, 2026 12:41

make everything private

5f3f799

read me fix

77410a6

truncation

c89ba0b

comments

731e995

		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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

xanderbailey Sep 24, 2026 •

edited

Loading

xanderbailey Sep 24, 2026 •

edited

Loading