Skip to content

feat: add AES GCM cipher and AES GCM Stream (AGS1) files - #1030

Merged
wgtmac merged 4 commits into
apache:mainfrom
shangxinli:enc/04-aes-gcm-stream
Oct 21, 2026
Merged

wgtmac merged 4 commits into
apache:mainfrom
shangxinli:enc/04-aes-gcm-stream

Conversation

@shangxinli

Copy link
Copy Markdown
Contributor

Summary

Step 4 of #988. Adds the crypto primitives of table encryption to the bundle library, built when ICEBERG_ENCRYPTION is ON. No behavior change for existing builds: everything here is new, and the default (ICEBERG_ENCRYPTION=OFF) compiles none of it.

  • AesGcmCipher (OpenSSL EVP): AES-GCM with a random 12-byte nonce, in the nonce | ciphertext | tag layout of Java's Ciphers. Key lengths 16/24/32, AAD optional, every OpenSSL return code checked including EVP_DecryptFinal_ex, so a failed tag check is always an error rather than unauthenticated plaintext.
  • AesGcmInputFile / AesGcmOutputFile: AES GCM Stream files per format/gcm-stream-spec.md, the format used for manifests, manifest lists, Avro data files and Puffin files. 1 MiB plaintext blocks, header AGS1 + little-endian block size, block AAD = file AAD prefix followed by the block index as a little-endian int32, and only the last block may be partial. The reader is seekable and takes the encrypted length from the key metadata rather than the file, so truncation is detected; the writer reports the plaintext Position() and the encrypted StoredLength().

aes_gcm_stream.h is the first public header that requires ICEBERG_ENCRYPTION while sharing a directory with unconditional ones (encrypted_key.h, standard_key_metadata.h). ci/scripts/generate_public_headers.py only classifies whole directories, so it grows a small per-header override table, and cpp20_compatibility_test gains a matching ICEBERG_PUBLIC_HEADERS_ENCRYPTION define.

Java parity

Ported against core/src/main/java/org/apache/iceberg/encryption/{Ciphers,AesGcmInputStream,AesGcmOutputStream,AesGcmInputFile,AesGcmOutputFile}.java. Two places where the Java semantics are not obvious from the C++ base classes and are deliberately matched:

  • AesGcmOutputStream::Position() keeps reporting the plaintext length after Close(). Closing flushes the final block, which resets the in-block position, so the value is captured first — Java does the same with its finalPosition field.
  • An empty stream is written as one empty block, while a stream whose length is an exact multiple of the block size gets no trailing empty block.

StandardKeyMetadata records the stored length for every encrypted file because Java needs it for AGS1 reads; that part landed in #991.

Test plan

  • crypto_test passes. The tests decrypt AGS1 streams of 0, 1 and 100 bytes and a KEK-encrypted manifest-list key metadata record, all produced by Iceberg Java, so a wire-format divergence fails rather than round-tripping against itself. Following feat: add standard encryption key metadata #991, the fixtures are inlined byte arrays rather than checked-in binaries.
  • Round trips across block boundaries (0, 1, block-1, block, block+1, 2*block+17) with write chunks that straddle blocks, plus random access across a boundary.
  • Wrong key, wrong AAD prefix, tampered ciphertext, truncated ciphertext, bad magic and a too-short stream all return errors.
  • cpp20_compatibility_test passes with ICEBERG_ENCRYPTION=ON.
  • pre-commit clean, including the generated public-header check.

Verified that the Position()-after-Close() assertion fails without its fix (reports 1048576 for a 100-byte stream).

Add the crypto primitives of table encryption to the bundle library, built
when ICEBERG_ENCRYPTION is ON:

- AesGcmCipher (OpenSSL EVP): AES-GCM with a random 12-byte nonce and the
  nonce | ciphertext | tag layout of Java's Ciphers.
- AesGcmInputFile / AesGcmOutputFile: AES GCM Stream files
  (format/gcm-stream-spec.md), used for manifests, manifest lists, Avro data
  files and Puffin files. 1 MiB plaintext blocks, block AAD = file AAD prefix
  followed by the block index. The reader is seekable, takes the encrypted
  length from trusted key metadata, and verifies every block. The writer
  reports the plaintext Position() and the encrypted StoredLength().

aes_gcm_stream.h is the first public header that needs ICEBERG_ENCRYPTION
while sharing a directory with unconditional ones, so the public-header
generator grows a per-header override and the C++20 compatibility test a
matching ICEBERG_PUBLIC_HEADERS_ENCRYPTION define.

Tests decrypt AGS1 streams and KEK-encrypted key metadata produced by Java,
and cover multi-block round trips, random access and tampering.
@shangxinli
shangxinli force-pushed the enc/04-aes-gcm-stream branch from 341ee25 to 046e105 Compare October 16, 2026 19:09

@wgtmac wgtmac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More broadly, this may be a good time to add a small arithmetic/math utility, similar to Arrow C++ checked arithmetic helpers, for checked add/multiply and range conversions. The same utility can then be reused by AGS1 length derivation, read-range validation, and later encryption metadata paths instead of each caller implementing its own overflow checks.

Comment thread src/iceberg/encryption/aes_gcm_stream.cc
AesGcmStream::PlaintextLength now returns a Result and rejects a length
shorter than an empty stream, one implying more blocks than the int32
block index in the block AAD can address, and one whose last block is too
short to hold a nonce and a tag. Java bounds the block count with
Math.toIntExact in the AesGcmInputStream constructor; the C++ side only
caught it when a block was decrypted, so AesGcmInputFile::Size() could
return a length that could never be read back.

@wgtmac wgtmac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new length validation addresses the earlier Size/Open concern. I also need to correct my earlier multiplication-overflow example: for a valid nonnegative encrypted length, num_full_blocks * kPlainBlockSize cannot overflow because plaintext blocks are smaller than cipher blocks. That specific argument was incorrect. A general checked-arithmetic utility remains an optional follow-up; the actual remaining range-addition issue and two other findings are attached inline.

Comment thread src/iceberg/encryption/aes_gcm_stream.cc
Comment thread src/iceberg/encryption/aes_gcm_stream.h Outdated
Comment thread src/iceberg/encryption/aes_gcm_stream.cc Outdated
shangxinli and others added 2 commits October 19, 2026 08:09
…heck

An empty AGS1 file still carries one block with a nonce and a tag, which the
writer emits like Java does, but the reader stopped at a plaintext length of
zero and never authenticated it. A wrong key, a wrong AAD prefix, a corrupt
tag or a file truncated behind its trusted length was accepted as an empty
file. Java authenticates that block on the first read; an empty stream is
never read from, so it is authenticated when the stream is opened.

ReadFully added the output length to the position before bounding it, so a
position near int64 max was signed overflow before Seek could reject it. The
position is now bounded first and the length compared against the remainder.

AesGcmStream::PlaintextLength is defined out of line in a bundle source built
with hidden visibility, so it needs ICEBERG_BUNDLE_EXPORT to link from outside
the shared bundle. The static-first tests did not cover this.
Read header patterns and optional groups from the CMake installation rules so AGS1 headers are installed only with the encryption bundle.

Rename the AES-GCM source, label OpenSSL null arguments, and document the Java-compatible format constraints.

@wgtmac wgtmac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! Let's move forward.

@wgtmac
wgtmac merged commit 4f195d0 into apache:main Oct 21, 2026
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants