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

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

Rationale for this change

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

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?

oh thanks for flagging this. im not a fan of checking in fixtures into the repo. and if we must, we should exclude them from the packaged artifacts.

are all of these .ags1 files necessary? I dont see them in the iceberg-rust repo. Can we just generate them in the tests? Or perhaps inline the specific content in the tests?

ykisana · 2026-10-01T03:41:28Z

Made a PR to update spec based on this discussion: apache/iceberg#18335

xanderbailey · 2026-10-05T18:44:21Z

@kevinjqliu are you able to give a review here?

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

4 open issues

_MAX_BLOCKS is currently set to the maximum block index representable by 4 bytes (2^32 - 1),… · New
_MAX_BLOCKS is currently set to the maximum block index representable by 4 bytes (2^32 - 1),… · New
block_index.to_bytes(4, ...) will raise OverflowError for negative values or values > 2^32-1,… · New
This table uses double leading pipes (||), which doesn’t render as a standard Markdown table (it… · New

What changed in this PR

Adds AGS1 (Iceberg GCM stream) format primitives and cross-client fixtures to support streaming AES-GCM block encryption compatible with the Iceberg spec / Java implementation.

Changes:

Introduces pyiceberg.encryption.stream with AGS1 header/AAD helpers and encrypted-length-derived block layout.
Adds a comprehensive pytest suite covering header decoding, AAD encoding, layout derivation, and Java-produced fixture decryption.
Documents and provides a Java generator for AGS1 binary fixtures used in cross-client tests.

File	Description
`pyiceberg/encryption/stream.py`	Implements AGS1 stream constants, header encode/decode, AAD derivation, and `_Ags1Layout` for block sizing/offsets.
`tests/encryption/test_stream.py`	Adds tests for stream primitives and validates decryption against Java-written fixture files.
`tests/encryption/ags1/README.md`	Documents the fixture set, parameters, and explains empty-stream behavior differences across implementations.
`tests/encryption/ags1/GenerateAgs1Fixtures.java`	Utility to regenerate fixtures using Iceberg Java’s `AesGcmOutputStream` behavior.

_{Review effort: Lite}

+_BLOCK_OVERHEAD = AesGcmCipher.NONCE_LENGTH + AesGcmCipher.TAG_LENGTH
+_CIPHER_BLOCK_SIZE = _PLAIN_BLOCK_SIZE + _BLOCK_OVERHEAD
+_BLOCK_INDEX_LENGTH = 4
+_MAX_BLOCKS = 2 ** (8 * _BLOCK_INDEX_LENGTH) - 1


+        if num_blocks > _MAX_BLOCKS:
+            raise ValueError(f"AGS1 streams hold at most {_MAX_BLOCKS} blocks, but {encrypted_length} bytes needs {num_blocks}")


+    Args:
+        aad_prefix (bytes | None): The file's AAD prefix, from its key metadata.
+        block_index (int): The zero-based index of the block within the stream.
+    """


+| File | Encrypted size | Plaintext | Pins |
+| --- | --- | --- | --- |
+| `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 |


kevinjqliu

Thanks for the PR!

A couple of comments, please take these with a grain of salt 😄

Test fixtures

my preference is to not check in binary for tests, they make release artifacts messy. one of the criterias during release is "theres no binaries in this artifact" 🤷

I think we can just inline them in the tests, like so

Details

JAVA_STREAMS = {
    # An empty file is the header plus one empty block, not a bare header.
    "empty": bytes.fromhex("4147533100001000178c6b4fd72e166bc1667356c865baed13878ed51c4340f6426501ec"),
    "partial-block": bytes.fromhex(
        "4147533100001000c02998e75683447a486f7dd3dd6c9f8a2a10fbfc956b15fdb35611534fe8d1414c81b17a477b914b"
        "fa0b515a5060cff94301feb0e85535402a2a2966214e20274635d4c19ae525d4da042ac51ed7db0c6698c731df0ba637"
        "4aff8b8d5c05297b8896abbfabe09c3ce1a2389c46d14718c17b0a3e33a4ddebc05a11b18ec9c576"
    ),
    # Written with a null AAD prefix, so the block index alone is the AAD.
    "partial-block-no-aad": bytes.fromhex(
        "4147533100001000117d00679bfa455f29880fe9dcd8d2ae9b8a7e81d40dbce6061631524e5ec61cbac9914317e7e60f"
        "dff52b32a779b3ae5339ea8859a40b3454a986a7f36b62f6e46699b036b6abe2ae043a86aee0ac5873066bab2420802c"
        "f970eede664723b455a224b99a5866c862c81640393168355e2acbe90e677a6dbaadab9ffa5cf3dd"
    ),
}

We can mention how to reproduce these in the README file, and also move the java file into it.

That leaves us with just stream.py, test_stream.py, and README.md which contains all the setup and repro.

Public API

These are all private functions and classes now. Perhaps we can define the public api. For example, encrypt_ags1 / decrypt_ags1 to replace _Ags1Layout. Something that will help with eventually using the pyiceberg-core (rust) functions

decrypt_ags1 can potentially help us enforce file_length on decrypt (#4011).

This isnt blocking, we can follow up too

kevinjqliu · 2026-10-21T20:45:19Z

+
+
+@dataclass(frozen=True)
+class _Ags1Layout:


thanks for adhering to python conventions 😄

kevinjqliu · 2026-10-21T20:48:31Z

im a bit concerned about checking in a java file thats not used at all. i think its just mentioned in the README. Perhaps we can just embed the file in the README itself... as collapsed details, like so:

Details

what do you think?

Have removed

kevinjqliu · 2026-10-21T20:50:43Z

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


oh thanks for flagging this. im not a fan of checking in fixtures into the repo. and if we must, we should exclude them from the packaged artifacts.

are all of these .ags1 files necessary? I dont see them in the iceberg-rust repo. Can we just generate them in the tests? Or perhaps inline the specific content in the tests?

kevinjqliu · 2026-10-21T20:56:00Z

these are all "private" functions and classes, which is fine to merge.

I think we should start thinking about defining the correct api surface and interface so that we can easily take the pyiceberg-core encryption features

xanderbailey · 2026-10-21T21:40:54Z

I’ll rethink the test artefacts, maybe even just remove them for the most part, you’re right this isn’t how we did it in rust, it was based on Matt’s suggestion. I think really we want these to come from the validations repo instead so I’ll remove them for now and just include unit tests

kevinjqliu · 2026-10-21T21:53:07Z

Actually Matt has a good point about leaving these as private functions for now. We can use _encrypt_ags1/_decrypt_ags1 until the manifest reader lands.

btw i pushed up a branch with my review iterations, feel free to take a look and take any (or none) of the proposed changes 😄
main...kevinjqliu:iceberg-python:kevinjqliu-review-pr-3969

Drop the checked-in AGS1 binaries and Java generator so no binaries ship in the sdist; cross-client fixtures will come from iceberg-verification (apache#4010).

mbutrovich · 2026-10-21T22:27:28Z

it was based on Matt’s suggestion.

whoa I got thrown under the bus! 😛

kevinjqliu · 2026-10-21T22:29:12Z

btw i created a tracking issue on the iceberg-rust side about pyiceberg-core integration: apache/iceberg-rust#3506

xanderbailey · 2026-10-21T22:31:22Z

Sorry Matt, I wasn't throwing shade! I think this intention here is good! But Kevin makes some good points about having the fixtures in the repo. WDYT?

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

kevinjqliu requested a balanced review from Copilot October 21, 2026 20:38

Copilot AI reviewed Oct 21, 2026

View reviewed changes

kevinjqliu reviewed Oct 21, 2026

View reviewed changes

xanderbailey added 2 commits October 21, 2026 17:58

replace java fixtures with unit tests

1b09754

Drop the checked-in AGS1 binaries and Java generator so no binaries ship in the sdist; cross-client fixtures will come from iceberg-verification (apache#4010).

take kevin's changes

c31523b

kevinjqliu mentioned this pull request Oct 21, 2026

Expose encryption in pyiceberg-core apache/iceberg-rust#3506

Open

1 of 6 tasks

Copilot stopped reviewing on behalf of kevinjqliu due to an error October 21, 2026 22:37
The job was not acquired by a runner even after multiple attempts.

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

		if num_blocks > _MAX_BLOCKS:
		raise ValueError(f"AGS1 streams hold at most {_MAX_BLOCKS} blocks, but {encrypted_length} bytes needs {num_blocks}")



		@dataclass(frozen=True)
		class _Ags1Layout:

Conversation

xanderbailey commented Sep 15, 2026 • edited by kevinjqliu 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ykisana commented Oct 1, 2026

Uh oh!

xanderbailey commented Oct 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Test fixtures

Public API

Uh oh!

xanderbailey commented Sep 15, 2026 •

edited by kevinjqliu

Loading

xanderbailey Sep 24, 2026 •

edited

Loading

xanderbailey Sep 24, 2026 •

edited

Loading

xanderbailey commented Oct 21, 2026 •

edited

Loading