From 6a4eb60ba6182e14360770356b4fb2f2f74cb64b Mon Sep 17 00:00:00 2001 From: alice-shang Date: Thu, 8 Oct 2026 20:57:59 +0900 Subject: [PATCH 1/6] feat: add standard encryption key metadata StandardKeyMetadata is the per-file key metadata of the standard Iceberg encryption scheme: the data encryption key, the AAD prefix (file ID) and, for AES GCM streams, the encrypted file length. It is stored in the key_metadata field of data, delete and manifest files, and (encrypted) in the table metadata encryption-keys for manifest lists. The serialized form is Java's: a schema version byte (1) followed by an Avro binary record. Tests check byte-for-byte compatibility with vectors written by Java (checked in under test/resources/encryption/vectors, excluded from the text-fixing pre-commit hooks). Co-Authored-By: Claude Opus 5.5 --- .pre-commit-config.yaml | 5 +- src/iceberg/CMakeLists.txt | 1 + .../encryption/standard_key_metadata.cc | 143 ++++++++++++++++++ .../encryption/standard_key_metadata.h | 68 +++++++++ src/iceberg/test/CMakeLists.txt | 2 + src/iceberg/test/encryption_test_util.h | 48 ++++++ .../encryption/vectors/aad_prefix.bin | 1 + .../resources/encryption/vectors/ags1_0.bin | Bin 0 -> 36 bytes .../resources/encryption/vectors/ags1_1.bin | Bin 0 -> 37 bytes .../resources/encryption/vectors/ags1_100.bin | Bin 0 -> 136 bytes .../test/resources/encryption/vectors/kek.bin | 1 + .../encryption/vectors/kek_timestamp.txt | 1 + .../test/resources/encryption/vectors/key.bin | 1 + .../encryption/vectors/key_metadata.bin | Bin 0 -> 37 bytes .../vectors/key_metadata_with_length.bin | 1 + .../manifest_list_key_metadata_encrypted.bin | 1 + .../encryption/vectors/master_key_keyA.bin | 1 + .../test/standard_key_metadata_test.cc | 82 ++++++++++ 18 files changed, 354 insertions(+), 2 deletions(-) create mode 100644 src/iceberg/encryption/standard_key_metadata.cc create mode 100644 src/iceberg/encryption/standard_key_metadata.h create mode 100644 src/iceberg/test/encryption_test_util.h create mode 100644 src/iceberg/test/resources/encryption/vectors/aad_prefix.bin create mode 100644 src/iceberg/test/resources/encryption/vectors/ags1_0.bin create mode 100644 src/iceberg/test/resources/encryption/vectors/ags1_1.bin create mode 100644 src/iceberg/test/resources/encryption/vectors/ags1_100.bin create mode 100644 src/iceberg/test/resources/encryption/vectors/kek.bin create mode 100644 src/iceberg/test/resources/encryption/vectors/kek_timestamp.txt create mode 100644 src/iceberg/test/resources/encryption/vectors/key.bin create mode 100644 src/iceberg/test/resources/encryption/vectors/key_metadata.bin create mode 100644 src/iceberg/test/resources/encryption/vectors/key_metadata_with_length.bin create mode 100644 src/iceberg/test/resources/encryption/vectors/manifest_list_key_metadata_encrypted.bin create mode 100644 src/iceberg/test/resources/encryption/vectors/master_key_keyA.bin create mode 100644 src/iceberg/test/standard_key_metadata_test.cc diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index fbc3e3347b..fa11263772 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -25,9 +25,10 @@ repos: rev: v5.0.0 hooks: - id: trailing-whitespace - exclude: ^src/iceberg/catalog/hive/gen-cpp/ + # Encryption test vectors and fixtures are binary files written by Java. + exclude: ^src/iceberg/(catalog/hive/gen-cpp|test/resources/encryption)/ - id: end-of-file-fixer - exclude: ^src/iceberg/catalog/hive/gen-cpp/ + exclude: ^src/iceberg/(catalog/hive/gen-cpp|test/resources/encryption)/ - id: check-yaml - id: check-added-large-files exclude: ^src/iceberg/catalog/hive/gen-cpp/ diff --git a/src/iceberg/CMakeLists.txt b/src/iceberg/CMakeLists.txt index 717f1da4b2..c283b8af9d 100644 --- a/src/iceberg/CMakeLists.txt +++ b/src/iceberg/CMakeLists.txt @@ -33,6 +33,7 @@ set(ICEBERG_SOURCES deletes/position_delete_index.cc deletes/position_delete_range_consumer.cc deletes/roaring_position_bitmap.cc + encryption/standard_key_metadata.cc expression/aggregate.cc expression/binder.cc expression/evaluator.cc diff --git a/src/iceberg/encryption/standard_key_metadata.cc b/src/iceberg/encryption/standard_key_metadata.cc new file mode 100644 index 0000000000..2a94eda2c0 --- /dev/null +++ b/src/iceberg/encryption/standard_key_metadata.cc @@ -0,0 +1,143 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/encryption/standard_key_metadata.h" + +#include "iceberg/util/macros.h" + +namespace iceberg { + +namespace { + +void WriteLong(int64_t value, std::vector& out) { + // Avro zig-zag varint encoding + auto n = (static_cast(value) << 1) ^ static_cast(value >> 63); + while (n & ~0x7FULL) { + out.push_back(static_cast((n & 0x7F) | 0x80)); + n >>= 7; + } + out.push_back(static_cast(n)); +} + +void WriteBytes(std::span bytes, std::vector& out) { + WriteLong(static_cast(bytes.size()), out); + out.insert(out.end(), bytes.begin(), bytes.end()); +} + +class Decoder { + public: + explicit Decoder(std::span buffer) : buffer_(buffer) {} + + Result ReadLong() { + uint64_t n = 0; + int shift = 0; + while (true) { + if (pos_ >= buffer_.size()) { + return Invalid("Truncated key metadata"); + } + if (shift > 63) { + return Invalid("Invalid varint in key metadata"); + } + uint8_t b = buffer_[pos_++]; + n |= static_cast(b & 0x7F) << shift; + if ((b & 0x80) == 0) { + break; + } + shift += 7; + } + return static_cast((n >> 1) ^ -(n & 1)); + } + + Result> ReadBytes() { + ICEBERG_ASSIGN_OR_RAISE(auto length, ReadLong()); + if (length < 0 || static_cast(length) > buffer_.size() - pos_) { + return Invalid("Invalid bytes length in key metadata: {}", length); + } + std::vector bytes(buffer_.begin() + pos_, buffer_.begin() + pos_ + length); + pos_ += length; + return bytes; + } + + /// \brief Read the branch index of a ["null", T] union; true if T is present. + Result ReadOptionalBranch() { + ICEBERG_ASSIGN_OR_RAISE(auto branch, ReadLong()); + if (branch != 0 && branch != 1) { + return Invalid("Invalid union branch in key metadata: {}", branch); + } + return branch == 1; + } + + private: + std::span buffer_; + size_t pos_ = 0; +}; + +} // namespace + +Result StandardKeyMetadata::Parse(std::span buffer) { + if (buffer.empty()) { + return Invalid("Empty key metadata"); + } + if (buffer[0] != kV1) { + return NotSupported("Unsupported key metadata schema version: {}", + static_cast(buffer[0])); + } + + Decoder decoder(buffer.subspan(1)); + StandardKeyMetadata metadata; + ICEBERG_ASSIGN_OR_RAISE(metadata.encryption_key, decoder.ReadBytes()); + ICEBERG_ASSIGN_OR_RAISE(auto has_aad_prefix, decoder.ReadOptionalBranch()); + if (has_aad_prefix) { + ICEBERG_ASSIGN_OR_RAISE(metadata.aad_prefix, decoder.ReadBytes()); + } + ICEBERG_ASSIGN_OR_RAISE(auto has_file_length, decoder.ReadOptionalBranch()); + if (has_file_length) { + ICEBERG_ASSIGN_OR_RAISE(metadata.file_length, decoder.ReadLong()); + } + // Trailing bytes may come from a newer schema version with extra fields; ignore them, + // as Java does. + return metadata; +} + +std::vector StandardKeyMetadata::Serialize() const { + std::vector out; + out.push_back(kV1); + WriteBytes(encryption_key, out); + if (aad_prefix.has_value()) { + WriteLong(1, out); + WriteBytes(*aad_prefix, out); + } else { + WriteLong(0, out); + } + if (file_length.has_value()) { + WriteLong(1, out); + WriteLong(*file_length, out); + } else { + WriteLong(0, out); + } + return out; +} + +StandardKeyMetadata StandardKeyMetadata::WithFileLength(int64_t length) const { + StandardKeyMetadata copy = *this; + copy.file_length = length; + return copy; +} + +} // namespace iceberg diff --git a/src/iceberg/encryption/standard_key_metadata.h b/src/iceberg/encryption/standard_key_metadata.h new file mode 100644 index 0000000000..ebad2d80a2 --- /dev/null +++ b/src/iceberg/encryption/standard_key_metadata.h @@ -0,0 +1,68 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +/// \file iceberg/encryption/standard_key_metadata.h +/// Key metadata of the standard Iceberg encryption scheme. + +#include +#include +#include +#include + +#include "iceberg/iceberg_export.h" +#include "iceberg/result.h" + +namespace iceberg { + +/// \brief Per-file key metadata of the standard encryption scheme. +/// +/// Stored in the `key_metadata` field of data files, delete files and manifest files, +/// and (encrypted by a key encryption key) in the table metadata `encryption-keys` for +/// manifest lists. +/// +/// The serialized form is compatible with Java's StandardKeyMetadata: a one-byte schema +/// version (1) followed by an Avro binary encoded record with the fields +/// `encryption_key: bytes`, `aad_prefix: union{null, bytes}` and +/// `file_length: union{null, long}`. +struct ICEBERG_EXPORT StandardKeyMetadata { + static constexpr uint8_t kV1 = 1; + + /// \brief The data encryption key (16, 24 or 32 bytes). + std::vector encryption_key; + /// \brief The AAD prefix (file ID) used to bind ciphertext to this file. + std::optional> aad_prefix; + /// \brief The encrypted (stored) file length, required to read AES GCM streams. + std::optional file_length; + + /// \brief Parse serialized key metadata. + static Result Parse(std::span buffer); + + /// \brief Serialize to the version-prefixed Avro binary form. + std::vector Serialize() const; + + /// \brief Return a copy with the given encrypted file length. + StandardKeyMetadata WithFileLength(int64_t length) const; + + friend bool operator==(const StandardKeyMetadata&, + const StandardKeyMetadata&) = default; +}; + +} // namespace iceberg diff --git a/src/iceberg/test/CMakeLists.txt b/src/iceberg/test/CMakeLists.txt index 292132bb6d..a86bc9087b 100644 --- a/src/iceberg/test/CMakeLists.txt +++ b/src/iceberg/test/CMakeLists.txt @@ -116,6 +116,8 @@ add_iceberg_test(expression_test residual_evaluator_test.cc strict_metrics_evaluator_test.cc) +add_iceberg_test(encryption_test SOURCES standard_key_metadata_test.cc) + add_iceberg_test(json_serde_test SOURCES json_serde_test.cc diff --git a/src/iceberg/test/encryption_test_util.h b/src/iceberg/test/encryption_test_util.h new file mode 100644 index 0000000000..e628ddb23e --- /dev/null +++ b/src/iceberg/test/encryption_test_util.h @@ -0,0 +1,48 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#pragma once + +#include +#include +#include +#include +#include + +#include "iceberg/test/test_resource.h" + +namespace iceberg { + +/// \brief Read a binary file under test/resources/encryption/vectors, written by +/// Java's EncryptionVectors with fixed keys. +inline std::vector ReadEncryptionVector(const std::string& name) { + std::ifstream in(GetResourcePath("encryption/vectors/" + name), std::ios::binary); + return {std::istreambuf_iterator(in), std::istreambuf_iterator()}; +} + +/// \brief Deterministic bytes, the same as Java's EncryptionVectors.bytes(). +inline std::vector VectorBytes(size_t length, int seed) { + std::vector bytes(length); + for (size_t i = 0; i < length; ++i) { + bytes[i] = static_cast(static_cast(i) * 31 + seed); + } + return bytes; +} + +} // namespace iceberg diff --git a/src/iceberg/test/resources/encryption/vectors/aad_prefix.bin b/src/iceberg/test/resources/encryption/vectors/aad_prefix.bin new file mode 100644 index 0000000000..2238d09635 --- /dev/null +++ b/src/iceberg/test/resources/encryption/vectors/aad_prefix.bin @@ -0,0 +1 @@ +!@_~8Wv \ No newline at end of file diff --git a/src/iceberg/test/resources/encryption/vectors/ags1_0.bin b/src/iceberg/test/resources/encryption/vectors/ags1_0.bin new file mode 100644 index 0000000000000000000000000000000000000000..546abbca3f90facd87a668f4ccf54c779d9cb4b9 GIT binary patch literal 36 scmZ>C4>n|A5MY>eEx1R!(AC_$t*rn3l{+&;KTe*T6Xv~6<^P>60P+$L82|tP literal 0 HcmV?d00001 diff --git a/src/iceberg/test/resources/encryption/vectors/ags1_1.bin b/src/iceberg/test/resources/encryption/vectors/ags1_1.bin new file mode 100644 index 0000000000000000000000000000000000000000..869262b903ecab8066b8808d2f702b453972e24d GIT binary patch literal 37 tcmZ>C4>n|A5McOm?|}v@0;DIc0OtgtIiN?ExU09QAfW4#U!|zjrqyU^S0@TWW q4zr{GOqSy@p=!bMx1d183tUABw>8>q1J2c1&`0?UogaxW8_Prsi$VSX literal 0 HcmV?d00001 diff --git a/src/iceberg/test/resources/encryption/vectors/kek.bin b/src/iceberg/test/resources/encryption/vectors/kek.bin new file mode 100644 index 0000000000..34ff2fe210 --- /dev/null +++ b/src/iceberg/test/resources/encryption/vectors/kek.bin @@ -0,0 +1 @@ +"A`9Xw \ No newline at end of file diff --git a/src/iceberg/test/resources/encryption/vectors/kek_timestamp.txt b/src/iceberg/test/resources/encryption/vectors/kek_timestamp.txt new file mode 100644 index 0000000000..e8a48eb620 --- /dev/null +++ b/src/iceberg/test/resources/encryption/vectors/kek_timestamp.txt @@ -0,0 +1 @@ +1791455219054 \ No newline at end of file diff --git a/src/iceberg/test/resources/encryption/vectors/key.bin b/src/iceberg/test/resources/encryption/vectors/key.bin new file mode 100644 index 0000000000..da230c0bf7 --- /dev/null +++ b/src/iceberg/test/resources/encryption/vectors/key.bin @@ -0,0 +1 @@ + ?^}7Vu \ No newline at end of file diff --git a/src/iceberg/test/resources/encryption/vectors/key_metadata.bin b/src/iceberg/test/resources/encryption/vectors/key_metadata.bin new file mode 100644 index 0000000000000000000000000000000000000000..23ae0d6ea6ee381113d90f7e38d2fa103ad83aa3 GIT binary patch literal 37 tcmZQnU{tV=tDUp^)=vrZu+k};FEJ@FDLTa0&E0eRm!w5_+0-qU82}A;4}<^! literal 0 HcmV?d00001 diff --git a/src/iceberg/test/resources/encryption/vectors/key_metadata_with_length.bin b/src/iceberg/test/resources/encryption/vectors/key_metadata_with_length.bin new file mode 100644 index 0000000000..70ae902780 --- /dev/null +++ b/src/iceberg/test/resources/encryption/vectors/key_metadata_with_length.bin @@ -0,0 +1 @@ +  ?^}7Vu !@_~8Wvږ \ No newline at end of file diff --git a/src/iceberg/test/resources/encryption/vectors/manifest_list_key_metadata_encrypted.bin b/src/iceberg/test/resources/encryption/vectors/manifest_list_key_metadata_encrypted.bin new file mode 100644 index 0000000000..d365484485 --- /dev/null +++ b/src/iceberg/test/resources/encryption/vectors/manifest_list_key_metadata_encrypted.bin @@ -0,0 +1 @@ +W܍%X$ሰ. }}ְz{a*N#إC3jkۨ.: \ No newline at end of file diff --git a/src/iceberg/test/resources/encryption/vectors/master_key_keyA.bin b/src/iceberg/test/resources/encryption/vectors/master_key_keyA.bin new file mode 100644 index 0000000000..7cf2dead9a --- /dev/null +++ b/src/iceberg/test/resources/encryption/vectors/master_key_keyA.bin @@ -0,0 +1 @@ +0123456789012345 \ No newline at end of file diff --git a/src/iceberg/test/standard_key_metadata_test.cc b/src/iceberg/test/standard_key_metadata_test.cc new file mode 100644 index 0000000000..bc374ce566 --- /dev/null +++ b/src/iceberg/test/standard_key_metadata_test.cc @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "iceberg/encryption/standard_key_metadata.h" + +#include + +#include "iceberg/test/encryption_test_util.h" +#include "iceberg/test/matchers.h" + +namespace iceberg { + +TEST(StandardKeyMetadataTest, ParseJavaVectors) { + auto key = ReadEncryptionVector("key.bin"); + auto aad_prefix = ReadEncryptionVector("aad_prefix.bin"); + ASSERT_EQ(key, VectorBytes(16, 1)); + ASSERT_EQ(aad_prefix, VectorBytes(16, 2)); + + auto with_length = ReadEncryptionVector("key_metadata_with_length.bin"); + ICEBERG_UNWRAP_OR_FAIL(auto parsed, StandardKeyMetadata::Parse(with_length)); + EXPECT_EQ(parsed.encryption_key, key); + EXPECT_EQ(parsed.aad_prefix, aad_prefix); + EXPECT_EQ(parsed.file_length, 1234567); + // Serialization is byte-for-byte identical to Java + EXPECT_EQ(parsed.Serialize(), with_length); + + auto without_length = ReadEncryptionVector("key_metadata.bin"); + ICEBERG_UNWRAP_OR_FAIL(auto parsed2, StandardKeyMetadata::Parse(without_length)); + EXPECT_EQ(parsed2.encryption_key, key); + EXPECT_FALSE(parsed2.file_length.has_value()); + EXPECT_EQ(parsed2.Serialize(), without_length); + EXPECT_EQ(parsed2.WithFileLength(1234567).Serialize(), with_length); +} + +TEST(StandardKeyMetadataTest, RoundTrip) { + StandardKeyMetadata metadata{.encryption_key = VectorBytes(32, 7)}; + ICEBERG_UNWRAP_OR_FAIL(auto parsed, StandardKeyMetadata::Parse(metadata.Serialize())); + EXPECT_EQ(parsed, metadata); + + // Large and negative-looking lengths survive the zig-zag varint encoding + for (int64_t length : {int64_t{0}, int64_t{63}, int64_t{64}, int64_t{1} << 40}) { + auto with_length = metadata.WithFileLength(length); + ICEBERG_UNWRAP_OR_FAIL(auto round_trip, + StandardKeyMetadata::Parse(with_length.Serialize())); + EXPECT_EQ(round_trip, with_length); + } +} + +TEST(StandardKeyMetadataTest, InvalidInput) { + EXPECT_THAT(StandardKeyMetadata::Parse(std::vector{}), + IsError(ErrorKind::kInvalid)); + // Unknown schema version + EXPECT_THAT(StandardKeyMetadata::Parse(std::vector{2, 0, 0, 0}), + IsError(ErrorKind::kNotSupported)); + // Key length beyond the buffer + EXPECT_THAT(StandardKeyMetadata::Parse(std::vector{1, 0x20, 1, 2}), + IsError(ErrorKind::kInvalid)); + // Invalid union branch + EXPECT_THAT(StandardKeyMetadata::Parse(std::vector{1, 0x02, 9, 0x04}), + IsError(ErrorKind::kInvalid)); + // Truncated optional fields + EXPECT_THAT(StandardKeyMetadata::Parse(std::vector{1, 0x02, 9}), + IsError(ErrorKind::kInvalid)); +} + +} // namespace iceberg From 7b952746012ddde851258bc615f747a0eb2a6579 Mon Sep 17 00:00:00 2001 From: alice-shang Date: Fri, 9 Oct 2026 10:16:34 +0900 Subject: [PATCH 2/6] fix: use std::cmp_greater for the key metadata bytes length check Co-Authored-By: Claude Opus 5.5 --- src/iceberg/encryption/standard_key_metadata.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/iceberg/encryption/standard_key_metadata.cc b/src/iceberg/encryption/standard_key_metadata.cc index 2a94eda2c0..9698de2256 100644 --- a/src/iceberg/encryption/standard_key_metadata.cc +++ b/src/iceberg/encryption/standard_key_metadata.cc @@ -19,6 +19,8 @@ #include "iceberg/encryption/standard_key_metadata.h" +#include + #include "iceberg/util/macros.h" namespace iceberg { @@ -66,7 +68,7 @@ class Decoder { Result> ReadBytes() { ICEBERG_ASSIGN_OR_RAISE(auto length, ReadLong()); - if (length < 0 || static_cast(length) > buffer_.size() - pos_) { + if (length < 0 || std::cmp_greater(length, buffer_.size() - pos_)) { return Invalid("Invalid bytes length in key metadata: {}", length); } std::vector bytes(buffer_.begin() + pos_, buffer_.begin() + pos_ + length); From 722a6f1427ca8c865737dc5ac63d1381c343b73f Mon Sep 17 00:00:00 2001 From: alice-shang Date: Fri, 9 Oct 2026 12:05:36 +0900 Subject: [PATCH 3/6] test: cover key metadata varint boundaries and malformed input Round trip the extreme and negative file lengths, distinguish an empty AAD prefix from an absent one, and reject negative byte lengths, truncated varints and varints longer than ten bytes (as Java's Avro decoder does). Also clarify why trailing bytes are ignored. --- .../encryption/standard_key_metadata.cc | 3 +- .../test/standard_key_metadata_test.cc | 30 +++++++++++++++++-- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/src/iceberg/encryption/standard_key_metadata.cc b/src/iceberg/encryption/standard_key_metadata.cc index 9698de2256..3fceb21c48 100644 --- a/src/iceberg/encryption/standard_key_metadata.cc +++ b/src/iceberg/encryption/standard_key_metadata.cc @@ -112,8 +112,7 @@ Result StandardKeyMetadata::Parse(std::span if (has_file_length) { ICEBERG_ASSIGN_OR_RAISE(metadata.file_length, decoder.ReadLong()); } - // Trailing bytes may come from a newer schema version with extra fields; ignore them, - // as Java does. + // Trailing bytes are ignored, matching Java, which decodes a single record. return metadata; } diff --git a/src/iceberg/test/standard_key_metadata_test.cc b/src/iceberg/test/standard_key_metadata_test.cc index bc374ce566..d7e959ab43 100644 --- a/src/iceberg/test/standard_key_metadata_test.cc +++ b/src/iceberg/test/standard_key_metadata_test.cc @@ -19,6 +19,8 @@ #include "iceberg/encryption/standard_key_metadata.h" +#include + #include #include "iceberg/test/encryption_test_util.h" @@ -53,13 +55,26 @@ TEST(StandardKeyMetadataTest, RoundTrip) { ICEBERG_UNWRAP_OR_FAIL(auto parsed, StandardKeyMetadata::Parse(metadata.Serialize())); EXPECT_EQ(parsed, metadata); - // Large and negative-looking lengths survive the zig-zag varint encoding - for (int64_t length : {int64_t{0}, int64_t{63}, int64_t{64}, int64_t{1} << 40}) { + // Zig-zag varint boundaries, including negative values and ten-byte encodings + for (int64_t length : + {int64_t{0}, int64_t{63}, int64_t{64}, int64_t{1} << 40, int64_t{-1}, + std::numeric_limits::min(), std::numeric_limits::max()}) { auto with_length = metadata.WithFileLength(length); ICEBERG_UNWRAP_OR_FAIL(auto round_trip, StandardKeyMetadata::Parse(with_length.Serialize())); EXPECT_EQ(round_trip, with_length); } + + // A present but empty AAD prefix is distinct from an absent one + StandardKeyMetadata empty_aad{.encryption_key = VectorBytes(16, 7), + .aad_prefix = std::vector{}}; + ICEBERG_UNWRAP_OR_FAIL(auto parsed_empty, + StandardKeyMetadata::Parse(empty_aad.Serialize())); + ASSERT_TRUE(parsed_empty.aad_prefix.has_value()); + EXPECT_TRUE(parsed_empty.aad_prefix->empty()); + ICEBERG_UNWRAP_OR_FAIL(auto parsed_null, + StandardKeyMetadata::Parse(metadata.Serialize())); + EXPECT_FALSE(parsed_null.aad_prefix.has_value()); } TEST(StandardKeyMetadataTest, InvalidInput) { @@ -77,6 +92,17 @@ TEST(StandardKeyMetadataTest, InvalidInput) { // Truncated optional fields EXPECT_THAT(StandardKeyMetadata::Parse(std::vector{1, 0x02, 9}), IsError(ErrorKind::kInvalid)); + // Negative key length (zig-zag 0x01 is -1) + EXPECT_THAT(StandardKeyMetadata::Parse(std::vector{1, 0x01, 9}), + IsError(ErrorKind::kInvalid)); + // Truncated varint + EXPECT_THAT(StandardKeyMetadata::Parse(std::vector{1, 0x80}), + IsError(ErrorKind::kInvalid)); + // Varint longer than ten bytes, rejected as by Java's Avro decoder + std::vector long_varint{1}; + long_varint.insert(long_varint.end(), 10, 0x80); + long_varint.push_back(0x01); + EXPECT_THAT(StandardKeyMetadata::Parse(long_varint), IsError(ErrorKind::kInvalid)); } } // namespace iceberg From 32bd22ecd780baac76e07fd4206ed506a2049444 Mon Sep 17 00:00:00 2001 From: alice-shang Date: Fri, 9 Oct 2026 12:08:23 +0900 Subject: [PATCH 4/6] test: inline the Java key metadata vectors Embed the two key metadata vectors serialized by Iceberg Java in the test instead of checking in binary files. The other vectors belong to later steps; this also drops the pre-commit exclusion that was only needed for binary files. --- .pre-commit-config.yaml | 5 +- src/iceberg/test/encryption_test_util.h | 48 ------------------ .../encryption/vectors/aad_prefix.bin | 1 - .../resources/encryption/vectors/ags1_0.bin | Bin 36 -> 0 bytes .../resources/encryption/vectors/ags1_1.bin | Bin 37 -> 0 bytes .../resources/encryption/vectors/ags1_100.bin | Bin 136 -> 0 bytes .../test/resources/encryption/vectors/kek.bin | 1 - .../encryption/vectors/kek_timestamp.txt | 1 - .../test/resources/encryption/vectors/key.bin | 1 - .../encryption/vectors/key_metadata.bin | Bin 37 -> 0 bytes .../vectors/key_metadata_with_length.bin | 1 - .../manifest_list_key_metadata_encrypted.bin | 1 - .../encryption/vectors/master_key_keyA.bin | 1 - .../test/standard_key_metadata_test.cc | 42 +++++++++++---- 14 files changed, 35 insertions(+), 67 deletions(-) delete mode 100644 src/iceberg/test/encryption_test_util.h delete mode 100644 src/iceberg/test/resources/encryption/vectors/aad_prefix.bin delete mode 100644 src/iceberg/test/resources/encryption/vectors/ags1_0.bin delete mode 100644 src/iceberg/test/resources/encryption/vectors/ags1_1.bin delete mode 100644 src/iceberg/test/resources/encryption/vectors/ags1_100.bin delete mode 100644 src/iceberg/test/resources/encryption/vectors/kek.bin delete mode 100644 src/iceberg/test/resources/encryption/vectors/kek_timestamp.txt delete mode 100644 src/iceberg/test/resources/encryption/vectors/key.bin delete mode 100644 src/iceberg/test/resources/encryption/vectors/key_metadata.bin delete mode 100644 src/iceberg/test/resources/encryption/vectors/key_metadata_with_length.bin delete mode 100644 src/iceberg/test/resources/encryption/vectors/manifest_list_key_metadata_encrypted.bin delete mode 100644 src/iceberg/test/resources/encryption/vectors/master_key_keyA.bin diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index fa11263772..fbc3e3347b 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -25,10 +25,9 @@ repos: rev: v5.0.0 hooks: - id: trailing-whitespace - # Encryption test vectors and fixtures are binary files written by Java. - exclude: ^src/iceberg/(catalog/hive/gen-cpp|test/resources/encryption)/ + exclude: ^src/iceberg/catalog/hive/gen-cpp/ - id: end-of-file-fixer - exclude: ^src/iceberg/(catalog/hive/gen-cpp|test/resources/encryption)/ + exclude: ^src/iceberg/catalog/hive/gen-cpp/ - id: check-yaml - id: check-added-large-files exclude: ^src/iceberg/catalog/hive/gen-cpp/ diff --git a/src/iceberg/test/encryption_test_util.h b/src/iceberg/test/encryption_test_util.h deleted file mode 100644 index e628ddb23e..0000000000 --- a/src/iceberg/test/encryption_test_util.h +++ /dev/null @@ -1,48 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -#pragma once - -#include -#include -#include -#include -#include - -#include "iceberg/test/test_resource.h" - -namespace iceberg { - -/// \brief Read a binary file under test/resources/encryption/vectors, written by -/// Java's EncryptionVectors with fixed keys. -inline std::vector ReadEncryptionVector(const std::string& name) { - std::ifstream in(GetResourcePath("encryption/vectors/" + name), std::ios::binary); - return {std::istreambuf_iterator(in), std::istreambuf_iterator()}; -} - -/// \brief Deterministic bytes, the same as Java's EncryptionVectors.bytes(). -inline std::vector VectorBytes(size_t length, int seed) { - std::vector bytes(length); - for (size_t i = 0; i < length; ++i) { - bytes[i] = static_cast(static_cast(i) * 31 + seed); - } - return bytes; -} - -} // namespace iceberg diff --git a/src/iceberg/test/resources/encryption/vectors/aad_prefix.bin b/src/iceberg/test/resources/encryption/vectors/aad_prefix.bin deleted file mode 100644 index 2238d09635..0000000000 --- a/src/iceberg/test/resources/encryption/vectors/aad_prefix.bin +++ /dev/null @@ -1 +0,0 @@ -!@_~8Wv \ No newline at end of file diff --git a/src/iceberg/test/resources/encryption/vectors/ags1_0.bin b/src/iceberg/test/resources/encryption/vectors/ags1_0.bin deleted file mode 100644 index 546abbca3f90facd87a668f4ccf54c779d9cb4b9..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 36 scmZ>C4>n|A5MY>eEx1R!(AC_$t*rn3l{+&;KTe*T6Xv~6<^P>60P+$L82|tP diff --git a/src/iceberg/test/resources/encryption/vectors/ags1_1.bin b/src/iceberg/test/resources/encryption/vectors/ags1_1.bin deleted file mode 100644 index 869262b903ecab8066b8808d2f702b453972e24d..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 37 tcmZ>C4>n|A5McOm?|}v@0;DIc0OtgtIiN?ExU09QAfW4#U!|zjrqyU^S0@TWW q4zr{GOqSy@p=!bMx1d183tUABw>8>q1J2c1&`0?UogaxW8_Prsi$VSX diff --git a/src/iceberg/test/resources/encryption/vectors/kek.bin b/src/iceberg/test/resources/encryption/vectors/kek.bin deleted file mode 100644 index 34ff2fe210..0000000000 --- a/src/iceberg/test/resources/encryption/vectors/kek.bin +++ /dev/null @@ -1 +0,0 @@ -"A`9Xw \ No newline at end of file diff --git a/src/iceberg/test/resources/encryption/vectors/kek_timestamp.txt b/src/iceberg/test/resources/encryption/vectors/kek_timestamp.txt deleted file mode 100644 index e8a48eb620..0000000000 --- a/src/iceberg/test/resources/encryption/vectors/kek_timestamp.txt +++ /dev/null @@ -1 +0,0 @@ -1791455219054 \ No newline at end of file diff --git a/src/iceberg/test/resources/encryption/vectors/key.bin b/src/iceberg/test/resources/encryption/vectors/key.bin deleted file mode 100644 index da230c0bf7..0000000000 --- a/src/iceberg/test/resources/encryption/vectors/key.bin +++ /dev/null @@ -1 +0,0 @@ - ?^}7Vu \ No newline at end of file diff --git a/src/iceberg/test/resources/encryption/vectors/key_metadata.bin b/src/iceberg/test/resources/encryption/vectors/key_metadata.bin deleted file mode 100644 index 23ae0d6ea6ee381113d90f7e38d2fa103ad83aa3..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 37 tcmZQnU{tV=tDUp^)=vrZu+k};FEJ@FDLTa0&E0eRm!w5_+0-qU82}A;4}<^! diff --git a/src/iceberg/test/resources/encryption/vectors/key_metadata_with_length.bin b/src/iceberg/test/resources/encryption/vectors/key_metadata_with_length.bin deleted file mode 100644 index 70ae902780..0000000000 --- a/src/iceberg/test/resources/encryption/vectors/key_metadata_with_length.bin +++ /dev/null @@ -1 +0,0 @@ -  ?^}7Vu !@_~8Wvږ \ No newline at end of file diff --git a/src/iceberg/test/resources/encryption/vectors/manifest_list_key_metadata_encrypted.bin b/src/iceberg/test/resources/encryption/vectors/manifest_list_key_metadata_encrypted.bin deleted file mode 100644 index d365484485..0000000000 --- a/src/iceberg/test/resources/encryption/vectors/manifest_list_key_metadata_encrypted.bin +++ /dev/null @@ -1 +0,0 @@ -W܍%X$ሰ. }}ְz{a*N#إC3jkۨ.: \ No newline at end of file diff --git a/src/iceberg/test/resources/encryption/vectors/master_key_keyA.bin b/src/iceberg/test/resources/encryption/vectors/master_key_keyA.bin deleted file mode 100644 index 7cf2dead9a..0000000000 --- a/src/iceberg/test/resources/encryption/vectors/master_key_keyA.bin +++ /dev/null @@ -1 +0,0 @@ -0123456789012345 \ No newline at end of file diff --git a/src/iceberg/test/standard_key_metadata_test.cc b/src/iceberg/test/standard_key_metadata_test.cc index d7e959ab43..4a50e781e6 100644 --- a/src/iceberg/test/standard_key_metadata_test.cc +++ b/src/iceberg/test/standard_key_metadata_test.cc @@ -20,21 +20,45 @@ #include "iceberg/encryption/standard_key_metadata.h" #include +#include #include -#include "iceberg/test/encryption_test_util.h" #include "iceberg/test/matchers.h" namespace iceberg { +namespace { + +/// \brief Deterministic test bytes: byte i is (i * 31 + seed). +std::vector TestBytes(size_t length, int seed) { + std::vector bytes(length); + for (size_t i = 0; i < length; ++i) { + bytes[i] = static_cast(static_cast(i) * 31 + seed); + } + return bytes; +} + +// Serialized by Iceberg Java's StandardKeyMetadata, with encryption key +// TestBytes(16, 1), AAD prefix TestBytes(16, 2) and no file length. +const std::vector kJavaKeyMetadata = { + 0x01, 0x20, 0x01, 0x20, 0x3f, 0x5e, 0x7d, 0x9c, 0xbb, 0xda, 0xf9, 0x18, 0x37, + 0x56, 0x75, 0x94, 0xb3, 0xd2, 0x02, 0x20, 0x02, 0x21, 0x40, 0x5f, 0x7e, 0x9d, + 0xbc, 0xdb, 0xfa, 0x19, 0x38, 0x57, 0x76, 0x95, 0xb4, 0xd3, 0x00}; + +// The same, with file length 1234567. +const std::vector kJavaKeyMetadataWithLength = { + 0x01, 0x20, 0x01, 0x20, 0x3f, 0x5e, 0x7d, 0x9c, 0xbb, 0xda, 0xf9, 0x18, 0x37, 0x56, + 0x75, 0x94, 0xb3, 0xd2, 0x02, 0x20, 0x02, 0x21, 0x40, 0x5f, 0x7e, 0x9d, 0xbc, 0xdb, + 0xfa, 0x19, 0x38, 0x57, 0x76, 0x95, 0xb4, 0xd3, 0x02, 0x8e, 0xda, 0x96, 0x01}; + +} // namespace + TEST(StandardKeyMetadataTest, ParseJavaVectors) { - auto key = ReadEncryptionVector("key.bin"); - auto aad_prefix = ReadEncryptionVector("aad_prefix.bin"); - ASSERT_EQ(key, VectorBytes(16, 1)); - ASSERT_EQ(aad_prefix, VectorBytes(16, 2)); + auto key = TestBytes(16, 1); + auto aad_prefix = TestBytes(16, 2); - auto with_length = ReadEncryptionVector("key_metadata_with_length.bin"); + const auto& with_length = kJavaKeyMetadataWithLength; ICEBERG_UNWRAP_OR_FAIL(auto parsed, StandardKeyMetadata::Parse(with_length)); EXPECT_EQ(parsed.encryption_key, key); EXPECT_EQ(parsed.aad_prefix, aad_prefix); @@ -42,7 +66,7 @@ TEST(StandardKeyMetadataTest, ParseJavaVectors) { // Serialization is byte-for-byte identical to Java EXPECT_EQ(parsed.Serialize(), with_length); - auto without_length = ReadEncryptionVector("key_metadata.bin"); + const auto& without_length = kJavaKeyMetadata; ICEBERG_UNWRAP_OR_FAIL(auto parsed2, StandardKeyMetadata::Parse(without_length)); EXPECT_EQ(parsed2.encryption_key, key); EXPECT_FALSE(parsed2.file_length.has_value()); @@ -51,7 +75,7 @@ TEST(StandardKeyMetadataTest, ParseJavaVectors) { } TEST(StandardKeyMetadataTest, RoundTrip) { - StandardKeyMetadata metadata{.encryption_key = VectorBytes(32, 7)}; + StandardKeyMetadata metadata{.encryption_key = TestBytes(32, 7)}; ICEBERG_UNWRAP_OR_FAIL(auto parsed, StandardKeyMetadata::Parse(metadata.Serialize())); EXPECT_EQ(parsed, metadata); @@ -66,7 +90,7 @@ TEST(StandardKeyMetadataTest, RoundTrip) { } // A present but empty AAD prefix is distinct from an absent one - StandardKeyMetadata empty_aad{.encryption_key = VectorBytes(16, 7), + StandardKeyMetadata empty_aad{.encryption_key = TestBytes(16, 7), .aad_prefix = std::vector{}}; ICEBERG_UNWRAP_OR_FAIL(auto parsed_empty, StandardKeyMetadata::Parse(empty_aad.Serialize())); From 80ef731854b685ea7a7a9fc5f9fa6237ce9cf52c Mon Sep 17 00:00:00 2001 From: shangxinli Date: Thu, 15 Oct 2026 07:18:27 -0700 Subject: [PATCH 5/6] build: include standard_key_metadata.h in the C++20 public-header check --- src/iceberg/cpp20_compatibility_internal.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/iceberg/cpp20_compatibility_internal.h b/src/iceberg/cpp20_compatibility_internal.h index 1b719f1b24..359e5ddff8 100644 --- a/src/iceberg/cpp20_compatibility_internal.h +++ b/src/iceberg/cpp20_compatibility_internal.h @@ -49,6 +49,7 @@ #include "iceberg/deletes/position_delete_range_consumer.h" #include "iceberg/deletes/roaring_position_bitmap.h" #include "iceberg/encryption/encrypted_key.h" +#include "iceberg/encryption/standard_key_metadata.h" #include "iceberg/exception.h" #include "iceberg/expression/aggregate.h" #include "iceberg/expression/binder.h" From 2aff7072a0a44bf52582baa5611efbc66343f261 Mon Sep 17 00:00:00 2001 From: shangxinli Date: Thu, 15 Oct 2026 07:20:56 -0700 Subject: [PATCH 6/6] fix: decode union indices with Avro's int rules Avro encodes a union branch index as an int, and Java's BinaryDecoder.readIndex delegates to readInt, which caps the varint at five bytes. ReadLong accepted up to ten, so a six-byte encoding of branch zero parsed here but is rejected by Java. --- .../encryption/standard_key_metadata.cc | 26 ++++++++++++++++++- .../test/standard_key_metadata_test.cc | 7 +++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/iceberg/encryption/standard_key_metadata.cc b/src/iceberg/encryption/standard_key_metadata.cc index 3fceb21c48..18f2d92ea2 100644 --- a/src/iceberg/encryption/standard_key_metadata.cc +++ b/src/iceberg/encryption/standard_key_metadata.cc @@ -66,6 +66,27 @@ class Decoder { return static_cast((n >> 1) ^ -(n & 1)); } + /// \brief Read a zig-zag varint, limited to the five bytes of an Avro int. + Result ReadInt() { + uint32_t n = 0; + int shift = 0; + while (true) { + if (pos_ >= buffer_.size()) { + return Invalid("Truncated key metadata"); + } + if (shift > 28) { + return Invalid("Invalid int varint in key metadata"); + } + uint8_t b = buffer_[pos_++]; + n |= static_cast(b & 0x7F) << shift; + if ((b & 0x80) == 0) { + break; + } + shift += 7; + } + return static_cast((n >> 1) ^ -(n & 1)); + } + Result> ReadBytes() { ICEBERG_ASSIGN_OR_RAISE(auto length, ReadLong()); if (length < 0 || std::cmp_greater(length, buffer_.size() - pos_)) { @@ -77,8 +98,11 @@ class Decoder { } /// \brief Read the branch index of a ["null", T] union; true if T is present. + /// + /// Avro encodes the branch index as an int, so this rejects the longer + /// encodings that ReadLong accepts, as Java's readIndex does. Result ReadOptionalBranch() { - ICEBERG_ASSIGN_OR_RAISE(auto branch, ReadLong()); + ICEBERG_ASSIGN_OR_RAISE(auto branch, ReadInt()); if (branch != 0 && branch != 1) { return Invalid("Invalid union branch in key metadata: {}", branch); } diff --git a/src/iceberg/test/standard_key_metadata_test.cc b/src/iceberg/test/standard_key_metadata_test.cc index 4a50e781e6..dd83c1ce34 100644 --- a/src/iceberg/test/standard_key_metadata_test.cc +++ b/src/iceberg/test/standard_key_metadata_test.cc @@ -127,6 +127,13 @@ TEST(StandardKeyMetadataTest, InvalidInput) { long_varint.insert(long_varint.end(), 10, 0x80); long_varint.push_back(0x01); EXPECT_THAT(StandardKeyMetadata::Parse(long_varint), IsError(ErrorKind::kInvalid)); + // Union index encoded in six bytes: a valid long but not a valid int, so + // Java's readIndex rejects it even though it decodes to branch zero. The + // trailing 0x00 is a well-formed file_length branch, so the record is + // otherwise complete and only the index encoding makes it invalid. + EXPECT_THAT(StandardKeyMetadata::Parse(std::vector{ + 1, 0x02, 9, 0x80, 0x80, 0x80, 0x80, 0x80, 0x00, 0x00}), + IsError(ErrorKind::kInvalid)); } } // namespace iceberg