feat(encryption): add StandardEncryptionManager

[hectar-glitches]

Summary

Adds 'StandardEncryptionManager', a KMS-backed 'EncryptionManager' implementation for generic (format-agnostic) file envelope encryption, e.g. manifests, manifest lists, and Puffin statistics files. This follows on from #1447, which merged the 'EncryptionManager'/'KeyManagementClient' interfaces, 'PlaintextEncryptionManager', and the in-memory KMS, and #1493, which added the KMS catalog-property registry ('kms-type'). As discussed in #1289, this is the piece everything downstream ('EncryptingFileIO', Parquet native encryption, manifest/manifest-list encryption, Puffin blob encryption) needs before it can do real encryption, so it's the next slice in the proposed split.

It uses a 'KeyManagementClient' to generate/wrap a fresh AES-256 data encryption key (DEK) per file, splits each file into fixed-size plaintext blocks (default 64KB), and seals each block independently with AES-GCM using a unique nonce (per-file random prefix combined with the block index). Since blocks are sealed independently, decrypted files support random access ('Seek'/'ReadAt') without decrypting the whole file, which is required since 'icebergio.File' mandates 'io.ReaderAt'/'io.Seeker'. Everything needed to decrypt (wrapped DEK, key ID, nonce prefix, block size, plaintext length) is persisted as a small JSON blob in the opaque per-file 'key_metadata' already carried by manifest entries. It fails closed, consistent with 'PlaintextEncryptionManager': it requires a non-empty 'keyID' on write ('ErrKeyIDRequired') and non-empty key metadata on read ('ErrKeyMetadataRequired'), rather than silently no-op'ing.

Testing covers round trips (sub-block, multi-block, exact block-size multiple, empty file), random access ('ReadAt' spanning a block boundary, 'Seek' plus sequential read), tamper detection (a flipped ciphertext byte fails with 'ErrAuthenticationFailed'), and error paths (empty keyID, empty key metadata, unknown KMS key ID, malformed key metadata). Verified with 'go vet', 'go test -race -count=2', and 'golangci-lint' (0 issues).

This PR is the standalone encryption engine only, it does not yet wire 'StandardEncryptionManager' into 'EncryptingFileIO' or any table/catalog code path, that follow-up work is tracked in #1289.

[tanmayrauth]

Engine works. One heads-up: this isn't the AGS1 stream format Java's StandardEncryptionManager produces, so encrypted files won't interop with Java — worth a quick call on whether that matters here. Plus two small code nits. Not
blocking. Details inline.

[tanmayrauth]

Nit: NewStandardEncryptionManager doesn't validate blockSize, so a bad WithBlockSize only surfaces at write time — WithBlockSize(-1) panics slice bounds out of range [:-1] on the first Write, and WithBlockSize(0) spins forever here (len(f.buf) == f.blockSize is 0 == 0 every iteration, so p is never consumed). A blockSize > 0 guard in NewEncryptedOutputFile (which already returns an error) rejects it early.

[hectar-glitches]

okay, the new 'NewEncryptedOutputFile' now rejects blockSize <= 0 with 'ErrInvalidBlockSize' before generating a DEK, so both 'WithBlockSize(-1)' (panic) and 'WithBlockSize(0)' (infinite loop) fail closed at construction time instead. Added tests for both cases.

[tanmayrauth]

Minor robustness nit: NewDecryptedInputFile checks the version but doesn't validate the rest of the decoded metadata. A well-formed-JSON key_metadata with "block-size":0 and non-zero "plaintext-length" builds fine here, then the first Read/ReadAt panics with integer divide by zero (numBlocks at :379, curOff / int64(f.blockSize) at :423). Not reachable from this manager's own output today, but it's untrusted input on a crypto read path — better to fail closed than panic. Validate meta.BlockSize > 0, meta.PlaintextLength >= 0, and len(meta.NoncePrefix) == 4 right after the version check (:198) and return a wrapped error. The existing "malformed key metadata" test only covers invalid JSON, so add a well-formed-but-block-size-0 case.

[hectar-glitches]

agreed. Added validation right after the version check: block-size must be positive, plaintext-length must be non-negative, and nonce-prefix must be exactly 4 bytes, all wrapped in a new 'ErrInvalidKeyMetadata'. Added the well-formed-but-block-size-0 case you mentioned, plus negative-plaintext-length and bad-nonce-prefix-length cases.

[tanmayrauth]

Heads-up on the format, not a blocker — a call worth making before this gets wired in, since the format becomes a breaking change once reads/writes depend on it.

Java's StandardEncryptionManager (same name) produces the AGS1 stream via AesGcmOutputStream: "AGS1" header + block size, 1 MiB blocks, an inline random 12-byte nonce per block, AAD = fileAadPrefix ‖ block ordinal, and Avro StandardKeyMetadata {encryption_key, aad_prefix, file_length}. This produces a different format — no header, 64 KB blocks, derived nonce, nil AAD, JSON key_metadata — so a Java reader can't read a file written here and vice versa.

Is cross-engine interop of encrypted metadata a goal? For a table encrypted by one engine and read by another it has to be, but if the intent is an iceberg-go-only format for now, this is fine as-is. One thing to note either way: there's no fileAadPrefix hook here, which is what Java uses to bind an AAD (e.g. the manifest-list case); adding that later is easier before the format is locked in. Flagging for the design discussion in #1289, not asking for changes in this PR.

[hectar-glitches]

I think cross-engine interop isn't a goal for this PR as this is intentionally an iceberg-go-only envelope format for now (no header, 64 KB blocks, derived nonce, nil AAD, JSON key_metadata), not the AGS1 stream Java's StandardEncryptionManager produces.

If (or when) cross-engine interop becomes a requirement (which I agree it eventually needs to be, for tables written by one engine and read by another), we'd need to either adopt AGS1 directly or add a pluggable format layer. I'll bring this up on #1289.

[laskoviymishka]

Really nice first cut — the per-file DEK envelope model, block-level AEAD for random access, and the fail-closed sentinel errors are all the right shape, and the round-trip and tamper tests are a good baseline.

I'd hold it before merging, though. tanmayrauth's already got the big interop/format call open for #1289 (AGS1 header, Avro key_metadata, AAD binding, block size) plus the blockSize/metadata validation nits — I'm deferring to those rather than re-raising, so this is about what's new on top.

The one I'd want fixed first is on the read path: readBlock discards a short read, so on a real backend (S3, local fs) the final partial block comes back as (n, io.EOF) and we hand a zero-padded buffer to aead.Open. The last block of every file whose size isn't a block multiple fails authentication — and the in-memory test's bytes.Reader fills the whole slice, which is exactly why the suite doesn't catch it. It needs a test backed by a reader that returns (n, io.EOF).

The second is the write/close state handling. written is incremented before the block is actually flushed, and closed is set before the final flush runs, so a mid-write flush failure leaves the writer in a state where PlaintextLength overcounts and a retried Close returns nil — the caller thinks the file succeeded when it's silently truncated. I'd advance written only on a successful flush and make the writer sticky on error.

A few smaller things I'd want before merge:

• fail closed in readBlock on bad decoded metadata (building on tanmayrauth's validation ask — the negative-length / out-of-range idx case too)
• validate DEK length at construction the way we're adding for blockSize
• make the DEK-per-file freshness invariant explicit (or widen the nonce prefix), since the whole nonce-uniqueness argument rests on it

Rest is nits inline. Once the read-path and write-state issues are sorted, happy to take another pass.

[laskoviymishka]

This discards a short read, and I think it breaks the last block of every file whose size isn't a block multiple on a real backend.

On S3 or the local fs, ReadAt fills a partial final block and returns (n, io.EOF) with n < len(ciphertext). We drop the EOF here, so aead.Open gets a buffer that's zero-padded past n and fails auth — which then surfaces as ErrAuthenticationFailed, not truncation. The in-memory test file never hits this because bytes.Reader fills the whole slice, so the suite is green while real reads fail.

I'd capture n and slice ciphertext = ciphertext[:n] before Open, and treat a genuinely short non-final block as a distinct truncation error rather than letting it fall through to auth-failed. Worth a test backed by a reader that returns (n, io.EOF) so we actually catch this class.

[laskoviymishka]

We bump f.written before the block is actually flushed, and I think that corrupts the file on a mid-write flush failure.

If flushBlock fails, we reset f.buf but leave f.written counting the bytes that never reached the writer, so Close records a PlaintextLength longer than the ciphertext actually holds — the reader then computes wrong block boundaries and fails to auth the final block. The return total - len(p) on the same path also reports buffered-but-unflushed bytes as written, which an io.Writer caller treats as durable.

I'd only advance written after a successful flush, and make the writer sticky (an f.err) so a failed flush poisons subsequent Writes rather than continuing on a half-written stream. While we're here, Write has no guard against use after Close — a Write after Close happily appends through the closed FileWriter. wdyt?

[laskoviymishka]

closed gets set before the final flush, so a Close that fails its flush can't be retried.

The second Close hits the if f.closed { return nil } guard and returns nil without re-flushing, so the caller sees success on a file that never finished writing (and KeyMetadata stays empty). The flush-failure path also drops the FileWriter.Close() error via _ =.

I'd set closed = true only on the success path (or track a sticky f.err and return it on every subsequent Close). A failWriter stub that errors on the Nth block would let us assert Close errors, KeyMetadata() is nil, and a repeat Close still errors.

[laskoviymishka]

agreed with tanmayrauth's call to validate the decoded metadata right after this version check — I'd add one belt-and-suspenders guard on top.

Even with the constructor validation, readBlock itself does make([]byte, plainLen+overhead) with no floor on plainLen and no check that idx is in [0, numBlocks()). It's the actual crypto read path taking numbers derived from untrusted metadata, so I'd have it fail closed there too rather than relying solely on the constructor. Cheap insurance against a future caller that constructs a standardInputFile some other way.

[laskoviymishka]

This is safe today, but the safety is entirely load-bearing on the DEK being fresh per file, and the 4-byte prefix leaves less room than I'd like.

The nonce is 4 random bytes + 8-byte block index, so uniqueness holds only because a new DEK is generated per file — the moment anyone adds DEK caching or reuse, prefix+index collides and GCM nonce reuse is catastrophic. Right now that invariant lives implicitly in the code, not in a comment.

I'd either widen the prefix to 8 bytes to buy margin, or at minimum make the invariant explicit at the generation site ("security relies on the DEK being unique per file; do not reuse a DEK across files") so a future change doesn't quietly break it. wdyt?

[laskoviymishka]

just adding a concrete data point to tanmayrauth's #1289 interop flag, not asking for anything here.

For whoever picks up that discussion, here's the exact byte-level gap vs Java's AesGcmOutputStream: no 8-byte "AGS1" + block-size header, block layout is ciphertext‖tag with the nonce derived rather than Java's inline random 12-byte nonce, AAD is nil vs Java's fileAadPrefix‖ordinal, 64 KiB blocks vs 1 MiB, and key_metadata is our JSON struct vs Java's Avro StandardKeyMetadata. So the formats are byte-incompatible in both directions today.

Two things that lean toward resolving it sooner rather than later: the type reuses the name StandardEncryptionManager, which is the canonical cross-engine impl elsewhere, so a reader reasonably assumes interop and gets silently-unreadable tables; and only the DEK is wrapped here — the nonce prefix, block size, and plaintext length ride in cleartext JSON, where Java envelope-encrypts the whole blob. Both are inputs for #1289, not changes for this PR.

[laskoviymishka]

small stdlib-conformance thing: a zero-length read on an empty file returns io.EOF here where bytes.Reader/strings.Reader return (0, nil).

With plaintextLength 0, off >= f.plaintextLength is 0 >= 0, so ReadAt(nil, 0) reports io.EOF. A caller probing an empty file (a valid zero-row manifest) via a zero-length ReadAt would misread it as an error. I'd add if len(p) == 0 { return 0, nil } ahead of the offset check.

[laskoviymishka]

doc nit: ReadAt is stateless and safe for concurrent use (and io.ReaderAt documents that contract), but Read/Seek mutate f.pos with no synchronization. Same type mixing both is a bit of a trap via io.Copy. Worth a comment that Read/Seek aren't concurrent-safe while ReadAt is.

[laskoviymishka]

tiny consistency nit: this wraps with %v while the NewGCM error just below uses %w. errors.Is(ErrInvalidKeyLength) still works, but %v drops the inner aes error from the chain. I'd use %w for both.

[laskoviymishka]

nit: the copy buffer is a fixed 32 KiB regardless of blockSize, so with a larger block we do extra Write round-trips through the buffering. make([]byte, f.blockSize) (or max(32*1024, f.blockSize)) lines it up with the block boundary.

[zeroshade]

Thanks for taking this on — the envelope model, the KeyManagementClient integration, the fail-closed sentinels, and the random-access design are all sound and should survive largely intact. The round-trip works and the code is clean. My concerns are about the on-disk format and about hardening against untrusted key metadata.

The format question is the one that matters most

The on-disk layout doesn't conform to the Iceberg AES GCM Stream spec (format/gcm-stream-spec.md), so files written here are not readable by Java or PyIceberg, and vice versa:

Spec (AGS1)	This PR
4-byte magic 41 47 53 31	none
4-byte LE BlockLength header	block size only in key metadata
12-byte RBG nonce stored per block in the file	derived from 4-byte prefix + block index
AAD = aadPrefix || blockSeqNum (4-byte LE)	no AAD
File length from a trusted source	plaintext-length in unauthenticated metadata

Key metadata also deviates: Java's StandardKeyMetadata is a 1-byte version + Avro record {encryption_key: bytes, aad_prefix: bytes?, file_length: long?}, where this PR uses custom JSON (which is also ~3-4x larger, and it's stored per manifest entry).

Worth noting that aad_prefix and file_length are precisely the two fields whose absence causes the truncation vulnerability below — so adopting the spec format would fix the security issue, the nonce fragility, and interop in a single move.

Given that, I'd suggest settling the format direction on #1289 before iterating further here, since it determines whether the block layout, nonce derivation, and metadata encoding all get rewritten anyway.

Correcting an earlier review comment

An earlier comment on this PR suggests the discarded ReadAt byte count breaks the last partial block. I checked this against a real os.File and it does not — the buffer is sized exactly to the remaining bytes, and a multi-block-plus-tail round trip returns err=nil, got=40, want=40. Please don't restructure around that comment. (There is still a smaller issue on that line, noted inline.)

Test coverage

Round trips (sub-block, multi-block, exact multiple, empty), ReadAt across a boundary, Seek+read, single-byte tamper, and the metadata-validation error paths are all covered well.

The most valuable additions, roughly in priority order:

1. Truncation / length tampering — the attack described inline is currently untested.
2. Adversarial metadata — the huge-value cases that take the process down.
3. Wrong-key decryption — nothing currently unwraps with a different KEK, which is the most obvious crypto failure path.
4. Write-path error injection via a FileWriter that fails on flush or close — the absence of this is why the two Close/Write issues below went unnoticed.
5. Known-answer vectors with a stubbed RNG, to catch format drift.
6. Cross-implementation fixtures from Java/PyIceberg.
7. The 64 KiB default block size is never round-tripped (tests use 16 bytes).

[zeroshade]

Blocking: no AAD means a file can be silently truncated.

Seal passes nil for additional data here, and Open does the same at line 424. Combined with plaintext-length living in unauthenticated key metadata, an attacker who can edit a manifest entry can shorten plaintext-length to a block boundary and truncate the ciphertext — every remaining block still authenticates, so decryption succeeds silently.

I verified this: a 4-block file with plaintext-length edited to 32 and the ciphertext cut at a block boundary decrypted with err = nil and returned 32 bytes of valid plaintext. No error surfaced anywhere.

The spec's AAD construction (aadPrefix || blockSequenceNumber) exists specifically to prevent this, along with block swapping within and between files.

Suggested fix: bind an AAD prefix (file identifier) plus the block index into the AAD on both Seal and Open, and validate the ciphertext length against plaintext-length before the first Open.

[zeroshade]

Matching nil AAD on the decrypt side (see the note on line 310).

Related: trailing garbage appended past the final block is currently accepted silently, because nothing binds the file's total length into what gets authenticated.

[zeroshade]

Blocking: unguarded overflow on attacker-controlled values.

f.plaintextLength + int64(f.blockSize) - 1 has no overflow check, and both operands come from key metadata that NewDecryptedInputFile only checks for > 0 / >= 0 (line 215). With block-size = MaxInt64 this wraps negative, numBlocks() returns 0, and blockPlainLen then falls through to return int64(f.blockSize) — which panics in readBlock with makeslice: len out of range.

Suggested fix: use overflow-safe arithmetic here, and cross-check plaintextLength against the underlying file's Stat().Size() at construction.

[zeroshade]

Blocking: unbounded allocation from untrusted metadata — takes the process down.

plainLen derives from the attacker-controlled block-size / plaintext-length, so this make is effectively unbounded. With block-size = plaintext-length = 2^40 I got fatal error: runtime: out of memory, which is an unrecoverable runtime throw — not a panic a caller can recover from. One malicious manifest entry kills the process.

Suggested fix: cap BlockSize at something sane (64 MiB would be generous) during metadata validation.

[zeroshade]

These two checks are the only validation applied to attacker-controlled key metadata, and > 0 / >= 0 isn't enough — both values remain free up to MaxInt64, which drives the overflow at line 397 and the allocation at line 419.

Suggested additions here: an upper bound on BlockSize, and a cross-check of PlaintextLength against the underlying file size.

[zeroshade]

Write after Close silently appends more sealed blocks and returns nil, while KeyMetadata() still describes the shorter file — so the trailing blocks are unreadable but no error is raised.

Suggested fix: reject writes on a closed file with fs.ErrClosed.

[zeroshade]

Not blocking, but worth hardening.

The 4-byte random per-file prefix concatenated with the 8-byte block index is unique only under the invariant that every file gets a fresh DEK. That invariant is real today but neither enforced nor documented, and it's exactly what a future key-cache or rotation-interval optimization would break — Java caches unwrapped keys, for reference.

The consequence is unusually severe for GCM: nonce reuse leaks the plaintext XOR and the GHASH authentication key. With only 4 random bytes, roughly 2^16 files sharing a DEK gives ~50% collision odds.

Adopting the spec's per-block stored 12-byte RBG nonce would remove the invariant entirely. Short of that, widening the prefix and documenting the fresh-DEK requirement at the type would help.

(crypto/rand is used correctly throughout, return values are checked, and I confirmed 16 concurrent ReadAt calls are race-clean under -race.)

[zeroshade]

Two small things here.

The ReadAt byte count is discarded. A short read fails safe — authentication catches it — but it surfaces as ErrAuthenticationFailed, which sends anyone debugging a truncated file down the wrong path. Using io.ReadFull semantics and a distinct truncation error would be clearer.

To be explicit, since an earlier review comment raised this: the discarded count does not break the final partial block. The buffer is sized to exactly the remaining bytes, and I confirmed against a real os.File that a multi-block-plus-tail round trip returns err=nil, got=40, want=40.

[zeroshade]

Not blocking, but a significant read cost: there's no block cache, so every Read re-decrypts its entire block.

Measured: 5000 one-byte Read calls triggered 5000 full 64 KiB block decryptions — about 320 MB of AES to read 5 KB. Even a 4 KiB buffered reader shows ~4x amplification.

Caching the most recently decrypted block is only a few lines. One caveat: a naive cache would break the parallel-use guarantee that ReadAt is documented to provide, so it needs guarding.

[zeroshade]

WithDEKLength and WithBlockSize accept invalid values silently — errors only surface later at NewEncryptedOutputFile, and for dekLength = 0 only after a wasted KMS round trip.

Validating at construction would fail faster and closer to the mistake.

[rambleraptor]

@hectar-glitches are you able to make the changes on this PR? I'd love to move forward with encryption support!

[zeroshade]

@rambleraptor Thanks for the interest! Status: the review feedback (chiefly that the on-disk layout needs to conform to the Iceberg AES GCM Stream spec — AGS1 magic, per-block stored nonces, block-sequence AAD, and Java's StandardKeyMetadata Avro form — so files interop with Java/PyIceberg) hasn't been addressed yet. If @hectar-glitches doesn't have bandwidth, a fresh PR building on this one with the format fixed would be very welcome — happy to review promptly.

_{Posted by an AI-assisted tool on behalf of maintainer zeroshade.}

[hectar-glitches]

@rambleraptor @zeroshade on it! sorry for the delay

[zeroshade]

Thanks for the hardening update. Several prior issues are fixed, but the implementation still has two blocking format/robustness problems and two major I/O error-contract problems that need to be resolved before merge. Details and reproductions are inline.

Blocking findings

• The encrypted file and key metadata remain incompatible with Iceberg AES GCM Stream (AGS1), and the missing AAD still allows coordinated ciphertext/length truncation to decrypt silently.
• Untrusted block-size metadata remains unbounded and feeds overflow-prone arithmetic and allocation sizes.

Major findings

• Write still reports bytes from a block whose flush failed; the added regression test does not assert the returned count.
• A direct underlying Close error is not sticky, can be masked by retry, and exposes finalized metadata before the output has closed successfully.

The update does fix short-read reporting, sticky flush errors, post-close writes, configured DEK validation, zero-length reads, and several range checks.

Validation performed: go test ./encryption -count=1, go test -race ./encryption -count=1, go vet ./encryption, git diff --check, and a clean merge probe against current main all pass. Focused probes reproduce the three behavioral failures described inline. Per SECURITY-THREAT-MODEL.md, the malformed-metadata issue is classified as robustness rather than an Iceberg Go security-boundary vulnerability.

---

This review was drafted by an AI-assisted tool and
confirmed by an Apache Iceberg Go maintainer. After you've
addressed the points above and pushed an update, an Apache Iceberg Go
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.

More on how Apache Iceberg Go handles maintainer review:
contributing-docs/05_pull_requests.rst.

[zeroshade]

Blocking: this is still not Iceberg AES GCM Stream (AGS1). The stream omits the required AGS1 magic and little-endian block-length header, blocks do not carry their required random 12-byte nonce, Seal/Open use nil AAD rather than aadPrefix || uint32LE(blockIndex), and key metadata is custom JSON rather than Iceberg StandardKeyMetadata. This is incompatible with Java/PyIceberg and leaves truncation undetected: I reproduced changing plaintext-length from 64 to 32 and truncating after two 16-byte blocks; decryption returned 32 bytes without error. Please implement the specified framing/AAD and add a known-vector or cross-implementation test.

[zeroshade]

Blocking robustness: block-size remains an unbounded metadata-controlled allocation size. Positivity alone permits values such as 1 << 40; construction succeeds and the first read reaches make([]byte, blockSize+overhead). The ceiling-division and physical-offset calculations can overflow as well. Please impose a supported maximum, use overflow-safe arithmetic (1 + (length-1)/blockSize plus checked multiplication/addition), and reject metadata whose implied physical layout is impossible. Under the repository threat model this is malformed-input robustness, not an in-scope security vulnerability.

[zeroshade]

Major: the previous Write count finding is still outstanding. Bytes are removed from p before flushBlock succeeds, so with four-byte blocks and a writer that fails on its second call, Write([]byte("aaaabbbb")) returns n=8, err!=nil although only the first block reached storage and the writer is permanently poisoned. Track the accepted count at the last successful flush boundary and extend the new failure test to assert n == 4.

[zeroshade]

Major: direct underlying Close failures are not sticky. This branch returns the error without assigning f.err; keyMetadata has already been published and closed is still false. A fail-once writer therefore makes the first Close fail and the second return nil, and Write is accepted between those calls with stale metadata. I reproduced that retry behavior. Keep this error sticky and avoid publishing final metadata until the underlying close succeeds; add a fail-first-Close regression test.

[laskoviymishka]

Thanks for the update, this is a lot better than round 1, and my headline call from last time (the readBlock short read) was just wrong on my part. The buffer's sized to the remaining bytes and the (n, io.EOF) case is handled, so os.File and the real backends are fine, and the shortReadFile test covers it now. That, plus readBlock failing closed on bad decoded metadata and out-of-range cases, the DEK length validation, and the nonce-uniqueness argument holding up (fresh DEK per file plus block index), clears everything I was originally gating on.

I'm leaving this as a comment rather than a hold, but to be clear it isn't merge-ready from my side yet either. The write/close pair I raised last round is still open, and it lines up with zeroshade's two majors:

• Close doesn't make an underlying-close failure sticky, so KeyMetadata() hands back a finalized blob for a file that never committed, and a retried Close returns nil and masks the original error.
• Write still counts a block whose flush failed, so the returned n overcounts what actually landed, and the regression test discards the count. Details inline on both.

And zeroshade's two blockers still stand and matter more than anything on my side: the missing AAD plus the unauthenticated plaintext-length let someone with catalog write access truncate a file and have it decrypt silently, and block-size comes off untrusted metadata unbounded and feeds overflow-prone offset math and allocation sizing.

So I'm dropping my formal hold, but the write/close pair and zeroshade's two blockers all need to land before this merges. Get those in and I'm a yes.

[laskoviymishka]

This is the other half of the write/close note from last round, still open on the close path. We assign f.keyMetadata before calling f.FileWriter.Close(), and when that underlying Close fails we return the error but never set f.err. So a caller that reads KeyMetadata() after a failed Close gets a fully-populated blob for a file that never committed, and a retried Close sails past the f.err guard, calls the underlying Close a second time, and returns nil, masking the original failure. I'd set f.err on the underlying-close failure and clear keyMetadata (or gate KeyMetadata() on f.closed), so the failure sticks and the metadata only surfaces once the file's actually down. wdyt?

[laskoviymishka]

The count side of that same write concern. When a block flush fails mid-Write we return total - len(p), but the failed block is still sitting in f.buf, drained out of p and never written. So Write([]byte("aaaabbbb")) with blockSize=4 and the second flush failing returns (8, err) when only 4 bytes actually landed, and io.Copy accumulates that count before it checks the error. I'd return total - len(p) - len(f.buf) so we only count what was flushed. The regression test discards the count (_, err = out.Write(...)), which is why this slips through, so worth asserting n there too.

[laskoviymishka]

block-size comes off untrusted JSON and we only check <= 0. With no upper bound it feeds two bad spots: physicalOffset does f.blockSize + f.aead.Overhead() as an int add before the int64 cast, so a large decoded block-size overflows to a negative offset we then hand to ReadAt; and readBlock sizes make([]byte, wantLen) off it, so a 1 GiB block-size means a 1 GiB allocation per block read, an OOM any malformed manifest entry can trigger. I'd add an upper bound here (a few tens of MiB) and do the offset add in int64. Fails closed the way the rest of the read path does now.

[laskoviymishka]

Small one: crypto/rand.Read always returns (n, nil) since Go 1.20, so this error check (and the one on the nonce prefix at ~205) is dead. kms.go in this package uses io.ReadFull(rand.Reader, buf); matching that here keeps the check live and the crypto path consistent.

[zeroshade]

Fourth review round. I re-verified each outstanding item against head 53e1953 rather than re-reading cold, using an independent decoder and instrumented stubs. CI is green across all 15 checks; encryption tests pass under -race; gofmt clean; golangci-lint reports 0 issues.

Previously-blocking items now genuinely fixed

AGS1 stream conformance (my 2026-08-18 blocker) — resolved at the stream level, and I confirmed it the hard way. I wrote a from-scratch spec-conformant decoder (raw crypto/cipher GCM, none of this package's code) that parses the AGS1 magic + little-endian block length, splits nonce‖ciphertext‖tag, rebuilds aadPrefix ‖ uint32LE(index), and it decrypted the writer's output exactly. Byte layout checks out too: 32 B plaintext at 16 B blocks → 96 B on disk = 8 + 2*(12+16+16). The framing, nonce placement, and AAD construction now match https://iceberg.apache.org/gcm-stream-spec/. Nice work — this was the hard part.

Also confirmed fixed: Write returning the flushed-only count (n=4, not 8, on the second-block failure); sticky f.err on underlying-Close failure with keyMetadata published only after that succeeds; bounded block size with checked mul/add arithmetic; short-read surfacing as ErrCiphertextTooShort rather than a misleading auth failure.

Remaining blocker

Key metadata is still bespoke JSON, so end-to-end interop still doesn't exist. (encryption/standard_manager.go:140)

The stream is now readable by a conformant reader — but only if that reader is handed the DEK and AAD prefix out of band. Those live in:

{"v":1,"key-id":"kek-1","wrapped-key":"...","aad-prefix":"...","plaintext-length":32}

Java's StandardEncryptionManager — the same name, which is the whole problem — reads an Avro-encoded StandardKeyMetadata. A Java or PyIceberg reader gets a perfectly valid AGS1 stream it cannot open. That's the other half of the 08-18 blocker, and the requested known-answer vector / cross-implementation test still isn't there.

Either match the Avro encoding, or rename the type so nobody infers interop from it. My preference is matching, since the stream work is already done and this is the last piece.

Related, and worth a deliberate decision either way: Java envelope-encrypts the whole key-metadata blob, whereas here aad-prefix, plaintext-length, and the block framing ride in cleartext.

Major

The underlying FileWriter is never closed after a poisoned flush. (encryption/standard_manager.go:522)

New finding this round. Close() returns at the f.err != nil guard before reaching f.FileWriter.Close(), so once a mid-write flush fails there is no path that closes the underlying writer — a leaked fd or connection on every failed write. An instrumented stub confirms closes == 0. TestStandardEncryptionManager_FlushFailurePoisonsWriterAndClose misses it because memFileWriter.Close is a no-op.

Fix: track whether the underlying writer has already been closed and close it once on the poisoned path too; give the test's stub a counting Close so this stays covered.

Read amplification is unchanged from the last round. (encryption/standard_manager.go:682)

5000 one-byte Read calls trigger 5001 full block decryptions — roughly 327 MB of AES to deliver 5 KB. Still not blocking, but manifests are the target workload here, so it will show up in practice. A most-recently-decrypted-block cache is a few lines. As noted previously, it needs guarding to preserve the documented concurrent-ReadAt property.

Minor / unaddressed from prior rounds

• nil KMS still panics (:205) with a nil-pointer dereference, while the doc comment states "kms must not be nil". Either check it or panic explicitly with a clear message.
• rand.Read vs the package convention (:248, :262, :475). kms.go uses io.ReadFull(rand.Reader, …); these three sites use rand.Read, whose error is unreachable. Raised on 08-25, still open.
• Unauthenticated header block size (:351). Inflating the header field to 128 MiB is accepted at open; the read then allocates 128 MiB before authentication rejects it. It fails closed, and under the repository threat model this is malformed-input robustness rather than an in-scope vulnerability — but the writer knows the block size, so persisting it in key metadata and requiring the header to match would remove the untrusted allocation outright.
• ErrOutputFileClosed could wrap fs.ErrClosed so errors.Is(err, fs.ErrClosed) works for callers.

Two behaviours to close out explicitly rather than leave ambiguous

Both are spec-consistent, but I cited the first as a repro in an earlier blocker, so it shouldn't just go quiet:

• Truncation still succeeds when plaintext-length is edited alongside the ciphertext (err=nil, 32 of 64 bytes returned). The AGS1 spec's "File length" note delegates exactly this to a trusted length source, and this repository's threat model treats catalog-supplied metadata as trusted (Boundary 2) — so it is by design. Please say so explicitly in the thread so it reads as a decision rather than an oversight.
• Trailing garbage appended past the final block is silently ignored, for the same reason.

Rebase

Please rebase onto main. The branch is 249 commits behind and does not build standalone (table/arrow_scanner.go:1027: undefined: internal); CI is green only because it tests the merge result, so the branch state itself is not actually being verified.

Nit

The PR description says block size is persisted in key_metadata; it is in the stream header instead. Worth correcting so the description matches the format.

---

Overall the crypto core is in good shape and the AGS1 work is real — I verified it independently rather than taking the description at its word. The naming/interop question and the writer leak are what I am holding on.

[zeroshade]

Three of my four items from 08-28 are properly fixed and I mutation-verified each one. The crypto core is in good shape and I re-verified it from scratch rather than trusting the description. What's holding this is the naming/interop decision, a concurrency regression introduced by the read-amplification fix, and a few tests that assert less than they appear to.

Blocking — the interop/naming question is still unanswered, and there's now a second, harder reason Java can't read these files

encryption/standard_manager.go:135-169, type name at :188.

What changed since 08-28 is a comment at :137-141 arguing the table spec makes key_metadata "implementation-specific". I checked that citation and it is accurate — format/spec.md:736 and :1027 both say exactly that, so I'll stop framing this as a spec violation. But it doesn't answer the ask, which was interop or a rename, and neither happened. Java's reader calls StandardKeyMetadata.parse() unconditionally on a 1-byte version + Avro {encryption_key, aad_prefix, file_length}; handed this JSON it throws.

The new finding this round is independent of the metadata argument. AesGcmInputStream.validateHeader() hard-asserts plainBlockSize == Ciphers.PLAIN_BLOCK_SIZE, where PLAIN_BLOCK_SIZE = 1024 * 1024 is a compile-time constant that also sizes cipherBlockBuffer and all the block-boundary math. So Java rejects any AGS1 stream whose header block length isn't exactly 1 MiB. This PR's default is 64 KiB (:47):

blockSize=65536   -> header=65536,   Java validateHeader accepts: false
blockSize=1048576 -> header=1048576, Java validateHeader accepts: true

So even with the key metadata fixed, the default configuration produces files Java refuses at byte 4. (PyIceberg is moot — it has no AGS1 implementation; key_metadata is a passthrough field there.) The known-answer vector / cross-implementation fixture still isn't present either.

Two acceptable exits, and the cheap one is genuinely cheap:

1. Rename (e.g. GoEnvelopeEncryptionManager) and add a paragraph on the type stating plainly that the byte stream is AGS1-conformant but the key metadata is Go-specific, so files are not readable by Java. Nobody infers interop, and the stream work stands on its own.
2. Match: default blockSize to 1 MiB and emit 1-byte version ‖ Avro{encryption_key, aad_prefix, file_length}, with a fixture from Java as the regression test.

My preference is still (2) since the hard part is done, but (1) unblocks this today. What I can't sign off on is a type named StandardEncryptionManager whose files the canonical StandardEncryptionManager cannot read, with nothing saying so.

Major

1. standard_manager.go:694-744 — the new block cache holds cacheMu across the underlying ReadAt, so concurrent random access is now fully serialized including remote I/O.

readBlock takes f.cacheMu at :695 with defer Unlock, then does f.underlying.ReadAt at :722 — a network round-trip on S3 — and aead.Open at :734, all under the lock. Before this round readBlock had no lock, so N concurrent ReadAt calls ran in parallel. The type still documents "ReadAt is stateless and safe for concurrent use" (:617), and it is still safe — but it is no longer concurrent. With a stub whose ReadAt sleeps 20 ms, 16 goroutines each reading a distinct block:

elapsed = 322.99ms   (serial floor 320ms, parallel floor 20ms)   underlying reads = 17

A ~16× throughput regression on exactly the workload this manager targets, introduced by the fix for the sequential case. Fix: don't hold the lock across I/O — consult the cache under the lock, release, read+decrypt, then re-take only to publish. A lost race just decrypts the same block twice and is harmless:

f.cacheMu.Lock()
if f.cacheValid && f.cacheIdx == idx { blk := f.cacheBlock; f.cacheMu.Unlock(); return blk, nil }
f.cacheMu.Unlock()
// … validate, ReadAt, Open — no lock held …
f.cacheMu.Lock(); f.cacheIdx, f.cacheBlock, f.cacheValid = idx, plaintext, true; f.cacheMu.Unlock()

An atomic.Pointer[cachedBlock] over an immutable {idx, block} removes the mutex outright. Either way, please add a test asserting concurrent ReadAt actually overlaps — the documented property has never had one.

2. standard_manager_test.go:250 and :258 — both key-metadata validation tests are tautologies. Neither fixture supplies "block-size", so meta.BlockSize decodes to 0 and the check at :315 fires first. Both then assert only the shared ErrInvalidKeyMetadata sentinel, so they're green for the wrong reason:

NegativePlaintextLength (as written):      err = …block-size must be positive…, got 0
EmptyAADPrefix          (as written):      err = …block-size must be positive…, got 0
NegativePlaintextLength (+ block-size:16): err = …plaintext-length must be non-negative, got -1
EmptyAADPrefix          (+ block-size:16): err = …aad-prefix must not be empty

Mutation-confirmed: deleting the PlaintextLength < 0 check (:318-320) leaves its test passing; deleting the len(AADPrefix) == 0 check (:321-323) leaves its test passing.

Fix: add "block-size":16 to both fixtures and assert on the message (or give the three checks distinct sentinels). Related: there is no test for the block-size check itself, and that's a regression — @tanmayrauth asked for a well-formed-but-block-size:0 case on 07-31 and the 08-03 reply said it was added, but it isn't in the current head. Please restore it plus an over-StandardMaxBlockSize case. The whole 3-check validation block currently has one accidental line of coverage.

What I verified independently

• From-scratch AGS1 decoder using only crypto/aes + crypto/cipher and the spec text — no code from this package. It decrypted the writer's output exactly for 11 (blockSize, size) combinations: bs=16/{0,1,15,16,17,32,100}, bs=4/13, bs=1/5, bs=7/63, bs=65536/70000. Physical size byte-exact at 8 + plaintext + nblocks*28 every time. AAD suffix endianness cross-checked against both the spec text and Java's Ciphers.streamBlockAAD — they agree.
• Nonce uniqueness: 40 files × 25 blocks → 1000/1000 distinct nonces. Fresh RBG per block, stored inline, so the old "uniqueness is load-bearing on DEK freshness" fragility is gone entirely.
• AAD binding: block 1's ciphertext fails under block 0's AAD and under a mutated prefix, opens only under its own — verified with raw GCM outside the package.
• Exhaustive random access: 3 block sizes × 11 file sizes, every (offset, length) pair in [0, size+2]² → 22,431 ReadAt assertions, all correct in bytes, content and EOF semantics, plus Seek(0)+io.ReadAll parity.
• Tamper matrix: cross-file block splice, duplicate-block replay at another index, and swapped nonces all → ErrAuthenticationFailed; bare truncation → ErrCiphertextTooShort: block 2: read 0 of 44; wrong KEK rejected at construction; header inflated to 128 MiB rejected before any allocation; files of 0/1/4/7 bytes → ErrInvalidStreamHeader.
• go build/vet/gofmt/golangci-lint on ./encryption/... clean, 0 issues; go test and -race pass (27 tests); CI 15/15.

Minor

• :571-578, :596-601 — the two sticky-error paths behave correctly but have no tests; the fail-first-Close regression test I asked for on 08-18 is still missing, and no writer in the suite ever errors from Close. Mutation-confirmed: removing f.err = … from either path leaves the entire suite green. I verified the behaviour by hand and it's right (Close#1 and Close#2 both return the same error, closes = 1, KeyMetadata() = nil, post-Close Write rejected). Two ~15-line tests lock both down — without them, with the mutation applied, Close#2 on a bytes.Buffer-backed writer returns nil and publishes metadata for a file that never committed.
• :467-473 — the underlyingClosed once-only guard is unreachable; every caller sets f.err first and both Write and Close return at that guard. Removing it entirely is behaviourally unobservable. Harmless defence in depth, but the comment at :453-455 implies it's load-bearing.
• :702-708 — readBlock's idx range check is likewise unreachable via the public API (ReadAt rejects off >= plaintextLength at :753). Fine as insurance; worth a // defensive: marker.
• :727 reuses ErrCiphertextTooShort, which kms.go:45 documents as being about a wrapped key or encrypted payload shorter than the nonce. A caller can't distinguish a malformed KMS blob from a short block read; a distinct ErrBlockTruncated would.
• Single-entry cache thrashes on non-local access: 300 round-robin one-byte ReadAt across 3 blocks → 300 decryptions (ideal 3). Manifest reads are mostly sequential so this is probably fine, but a 2–4 entry ring would be nearly free.
• The cache retains one full decrypted block (up to 128 MiB) for the input file's lifetime. Negligible at the 64 KiB default; worth a word in the doc.
• :285-287 — if the header write fails, NewEncryptedOutputFile returns without closing writer, while Write/Close now do take ownership of closing on failure. Defensible (no EncryptedOutputFile was handed out) but inconsistent.
• No coverage for ReadFrom or Stat(). I probed both and they're correct — ReadFrom reports n=4 (flushed bytes only) with closes=1 and a sticky error on failure; Stat() overrides Size() with the plaintext length.
• The 64 KiB default block size is still never round-tripped — every test using the default is an error-path test; the cache test uses 4096 and the rest use 16.
• numBlocks() is recomputed twice per readBlock (:702, and inside blockPlainLen at :657).

Prior items

1. Bespoke JSON key metadata / shared name → Still open (above).
2. FileWriter never closed after a poisoned flush → Fixed. closeUnderlyingIgnoringError (:467-473) is called from all three failure paths, the test stub got a counting Close, and it asserts closes == 1. Mutation-verified: removing the call from Write's failure path fails with expected: 1, actual: 0. Also confirmed via ReadFrom.
3. Read amplification → Fixed for sequential access. 5000 one-byte Read calls on 64 KiB blocks now cost 1 decryption, down from 5001. Mutation-verified: disabling the cache fails the new test with "5000" is not less than or equal to "2" — a real, tight assertion. But the guarding introduced Major 1, and non-local access is unimproved.
4. The four minors → all Fixed, each mutation-verified: nil KMS now panics with a clear message and a test; io.ReadFull(rand.Reader, …) at all three sites, matching kms.go; the header block size is now persisted in trusted key metadata and the header value only compared, never used to size an allocation (a 128 MiB inflated header is rejected with stream header block length 134217728 does not match key metadata block-size 16) — fixed exactly as suggested; ErrOutputFileClosed wraps fs.ErrClosed.
5. Truncation / trailing garbage close-out → Partially fixed. Both are now documented in code as spec-delegated, citing the AGS1 "File length" note, and I reconfirmed both behaviours are unchanged. But nothing was posted in the PR thread — there's no reply from you after 2026-08-18 on any thread — so the record still reads as an oversight rather than a decision. One comment closes it.
6. Rebase → Still open, 251 commits behind, and go build ./... fails at table/arrow_scanner.go:1027. Softening my earlier framing though: main hasn't touched encryption/ since the merge base and this PR adds only two new files there, so the merge is trivially clean and ./encryption/... builds and tests standalone. Hygiene, not hidden risk.

Carry-forward: block-size upper bound + int64 offset math (@laskoviymishka 08-25) → Fixed (:315, checked mul/add at :669-684). Write count → Fixed, mutation-verified. KeyMetadata() after a failed underlying Close → Fixed in code, untested. block-size:0 metadata test → regressed out of the suite. Known-answer vectors and cross-implementation fixtures → Still open.

Description

The description was never updated for the AGS1 rewrite and is now wrong about the crypto — specifically about the thing that was the blocker:

• "seals each block independently with AES-GCM using a unique nonce (per-file random prefix combined with the block index)" — no longer true; each block carries a fresh 12-byte RBG nonce stored inline (:515-521). This matters more than a usual description nit: a reader who trusts it will think the old derived-nonce scheme is still in place and re-raise the nonce-reuse fragility the rewrite actually eliminated.
• "Everything needed to decrypt (wrapped DEK, key ID, nonce prefix, block size, plaintext length)" — it's an AAD prefix (:160), a different thing serving a different purpose. Worth noting it's 16 bytes, matching Java's ENCRYPTION_AAD_LENGTH_DEFAULT.
• AGS1 conformance isn't mentioned at all — the headline change across four review rounds is absent.
• The testing paragraph predates the current suite (no mention of the block-reorder test, the three header-validation tests, the truncated-backend test, the write/close failure test, or the block-cache test).

Given the interop decision, the description is also the right place to state plainly which readers can and cannot open these files.

---

To be clear about where this stands: the wire format, nonce generation, AAD binding and random access are all correct, and the three fixes this round are real and mutation-resistant. It's the naming/interop decision, the concurrency cost of the cache fix, and a handful of tests that assert less than they appear to.

---

This review was drafted by an AI-assisted tool and confirmed by an Iceberg Go maintainer. The findings cite the project's review criteria; if you think one is mis-applied, please reply and a maintainer will weigh in.