Core, Hive: source encryption keys from table metadata to prevent key… by smaheshwar-pltr · Pull Request #17404 · apache/iceberg

smaheshwar-pltr · 2026-07-29T00:25:37Z

… loss under concurrent commits

A single Table (and its shared TableOperations + EncryptionManager, e.g. via a caching catalog) can be used concurrently. On encrypted tables the mutable, shared StandardEncryptionManager could drop a snapshot's manifest-list key from encryption-keys, leaving a committed snapshot undecryptable.

Make encryption keys live only in table metadata and the manager immutable and metadata-sourced:

StandardEncryptionManager holds an immutable key snapshot. Minting a manifest-list key is a pure operation (mintManifestListKey) that returns the new key(s) instead of storing them; removes the mutating key methods.
SnapshotProducer captures the keys minted while writing the manifest list and persists them into the metadata it commits.
HiveTableOperations builds the manager on demand from a metadata's keys (memoized by metadata identity); drops the doCommit/doRefresh key merges and the mutable manager/key fields. temp(uncommittedMetadata) sources keys from the uncommitted metadata, and BaseTransaction re-points temp ops after a rebase, so staged operations resolve earlier ops' (and concurrently committed) keys.

The immutable manager serializes to Spark executors with no concurrent-mutation hazard.

… loss under concurrent commits A single Table (and its shared TableOperations + EncryptionManager, e.g. via a caching catalog) can be used concurrently. On encrypted tables the mutable, shared StandardEncryptionManager could drop a snapshot's manifest-list key from encryption-keys, leaving a committed snapshot undecryptable. Make encryption keys live only in table metadata and the manager immutable and metadata-sourced: - StandardEncryptionManager holds an immutable key snapshot. Minting a manifest-list key is a pure operation (mintManifestListKey) that returns the new key(s) instead of storing them; removes the mutating key methods. - SnapshotProducer captures the keys minted while writing the manifest list and persists them into the metadata it commits. - HiveTableOperations builds the manager on demand from a metadata's keys (memoized by metadata identity); drops the doCommit/doRefresh key merges and the mutable manager/key fields. temp(uncommittedMetadata) sources keys from the uncommitted metadata, and BaseTransaction re-points temp ops after a rebase, so staged operations resolve earlier ops' (and concurrently committed) keys. The immutable manager serializes to Spark executors with no concurrent-mutation hazard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

smaheshwar-pltr · 2026-07-29T00:26:25Z

  @Override
  public Snapshot apply() {
    refresh();
+    this.applyManifestListKeys = null;


smaheshwar-pltr · 2026-07-29T00:27:06Z

+    // Immutable so a built instance's key set is stable, which HiveTableOperations relies on to
+    // memoize its metadata-sourced encryption manager by metadata identity.


why? do we rlly need this? surely tablemetadata treats like immutable? wanna avoid large copy (per-snapshot keys)

smaheshwar-pltr · 2026-07-29T00:27:43Z

    this.testTimeShift = 0;

-    this.encryptionKeys = SerializableMap.copyOf(Maps.newLinkedHashMap());
+    Map<String, EncryptedKey> keyMap = Maps.newLinkedHashMap();


why not immutable map builder

smaheshwar-pltr · 2026-07-29T00:27:52Z

  }

-  private SecureRandom workerRNG() {
+  private synchronized SecureRandom workerRNG() {


smaheshwar-pltr · 2026-07-29T00:28:21Z

-    String keyEncryptionKeyTimestamp =
-        encryptionKeys.get(keyEncryptionKeyID).properties().get(KEY_TIMESTAMP);
+  /**
+   * Mints a manifest-list key for the given key metadata without storing it. The returned {@link


what is this "mint" wording throughout? is it standard are u sure? i don't love its sound

smaheshwar-pltr · 2026-07-29T00:29:37Z

+   * Mints a manifest-list key for the given key metadata without storing it. The returned {@link
+   * MintedKeys} must be persisted into table metadata by the caller to stay decryptable.
+   */
+  public MintedKeys mintManifestListKey(NativeEncryptionKeyMetadata keyMetadata) {


does this have to be on this class? feels a bit weird and confusing to me to have this method here. is there a better design?

smaheshwar-pltr · 2026-07-29T00:29:58Z

    return ByteBuffer.wrap(newKey);
  }

+  /** The keys minted by a single {@link #mintManifestListKey} call, for the caller to persist. */


again, can't help but feel design could be better here

smaheshwar-pltr · 2026-07-29T00:30:34Z

+  }
+
+  @Test
+  public void testConcurrentMintingIsSafeAndDoesNotMutateKeys() throws InterruptedException {


why? why would it ever mint keys?

test reads silly to me wdyt?

should we not be using standard testing patterns on the SEM to test meaningfully

smaheshwar-pltr · 2026-07-29T00:32:10Z

  @Override
  public FileIO io() {
+    // Avoid current() (which may refresh) on the unencrypted fast path.
+    return tableKeyId == null ? fileIO : io(current());


u sure it's fine to call current() in io / encryption? just feels like design smell here

smaheshwar-pltr · 2026-07-29T00:32:55Z

+    // Use plain fileIO, not io(): metadata isn't envelope-encrypted and io() would re-enter
+    // refresh.


hmm don't love this

smaheshwar-pltr · 2026-07-29T00:33:04Z


-      tableKeyId = tableKeyIdFromHMS;
-      encryptionDekLength =
+      // Keys aren't cached here; encryption() rebuilds the manager from metadata on demand.


what does this comment give us

smaheshwar-pltr · 2026-07-29T00:33:40Z

+    // Encryption keys are already persisted into the metadata by SnapshotProducer.
+    TableMetadata tableMetadata = metadata;


this is silly both can be tiched

smaheshwar-pltr · 2026-07-29T00:34:38Z

+        // Source keys from the uncommitted metadata so staged transaction operations can read
+        // snapshots produced by earlier operations before the transaction commits.


maybe mention encryption here. also, justify encryptionPropsFromMetadata which feels mutating-y above

smaheshwar-pltr · 2026-07-29T00:34:49Z

-              encryptedKeys, encryptionProperties, keyManagementClient);
-    } else {
-      return PlaintextEncryptionManager.instance();
+    synchronized (managerCacheLock) {


u sure we need locking here

remember smaheshwar-pltr#10 PR description where there's no sync in table operations but that's fine. also think abt how that pr desc applies to this pr in general

smaheshwar-pltr · 2026-07-29T00:37:07Z

  }

+  @TestTemplate
+  public void testSharedTableTransactionInterleavedWithDirectCommit() {


changes in this file read weird to me, shouldn't this be the tests in smaheshwar-pltr#14 smaheshwar-pltr#15 instead? i feel like those tests are very useful, maybe this new one is too or the same as one of the prev one. but concurrent append test w encryption is almost essential to me

smaheshwar-pltr · 2026-07-29T00:37:20Z

+    // A single shared Table (and its EncryptionManager) drives both a staged transaction and a
+    // direct commit. With a mutable shared manager, the direct commit's key could be dropped from
+    // metadata, leaving its snapshot undecryptable. The metadata-sourced manager keeps every key.


smaheshwar-pltr · 2026-07-29T00:38:27Z

+
+    transaction.commitTransaction();
+
+    // Reading forces decryption of every snapshot's manifest list, including the direct commit's.


this is just completely wrong. reading only reads the current manifest list. this is unacceptable correctness bug in your code. i've lost trust in your iteration process so you must now review everything fact checking everything. that's what happens when i catch a huge oversight like this. not acceptable at all

smaheshwar-pltr · 2026-07-29T00:39:13Z

+  }
+
+  /**
+   * Creates a manager whose immutable key set is exactly {@code keys}, as if sourced from metadata.


"as if sourced from metadata." is silly

smaheshwar-pltr · 2026-07-29T00:39:42Z

          @Override
          public EncryptionManager encryption() {
-            return encryptionManager;
+            // Metadata-sourced: keys live in metadata, persisted by SnapshotProducer at commit.


smaheshwar-pltr · 2026-07-29T00:39:56Z

+            List<EncryptedKey> keys = current() == null ? List.of() : current().encryptionKeys();
+            return EncryptionTestHelpers.createEncryptionManager(keys);


optional of nullable + or else -> one line?

smaheshwar-pltr · 2026-07-29T00:40:20Z

+    // Reading the staged snapshot's manifest list decrypts it, proving its key was persisted into
+    // metadata even though the snapshot is not a branch head.


slop comment, improve it.

smaheshwar-pltr · 2026-07-29T00:40:58Z

  }

+  /** The keys minted by a single {@link #mintManifestListKey} call, for the caller to persist. */
+  public static class MintedKeys {


do we need the verboseness in these classes btw? we don't have records available

smaheshwar-pltr · 2026-07-29T00:41:24Z

+      // Re-point temp ops at the refreshed metadata so a metadata-sourced encryption manager can
+      // resolve keys for the concurrently-committed snapshots the refresh pulled in.


don't like this wording, too AI

smaheshwar-pltr · 2026-07-29T00:42:06Z

+/**
+ * An immutable {@link EncryptionManager} for standard (envelope) encryption whose keys are sourced
+ * from table metadata. Minting a manifest-list key is a pure operation ({@link
+ * #mintManifestListKey}) that returns the new key(s) for the caller to persist into metadata rather
+ * than storing them, so instances are safe to share across concurrent commits and to serialize.
+ */


smaheshwar-pltr · 2026-07-29T00:43:55Z

+    - code: "java.method.removed"
+      old: "method java.lang.String org.apache.iceberg.encryption.StandardEncryptionManager::addManifestListKeyMetadata(org.apache.iceberg.encryption.NativeEncryptionKeyMetadata)"
+      justification: "Replaced by pure StandardEncryptionManager.mintManifestListKey;\
+        \ the immutable, metadata-sourced manager no longer stores minted keys"


this is significant. what should we actually do here. i didn't realise StandardEncryptionManager was public so this whole PR might be infeasible....

can u look at github history to work out what the standard procedure is for this stuff? do we need to go through deprecation or cycle or are we just screwed here given we're changing a public class? can u multiple precedences for this with for / against?

i'm unconvinced we can "just do this"

smaheshwar-pltr · 2026-07-29T00:44:54Z


-  private ManifestFile writeAndReadEncryptedManifestList(EncryptionManager em) throws IOException {
+  /**
+   * Writes an encrypted manifest list with a manager built from {@code metadataKeys}, persists the


i've not audited these tests but having seen ur other tests, i'm highly skeptical your tests in this PR are good. please do an audit for software quality with good-testing-patterns

github-actions Bot added spark core hive labels Jul 29, 2026

smaheshwar-pltr commented Jul 29, 2026

View reviewed changes

		// Immutable so a built instance's key set is stable, which HiveTableOperations relies on to
		// memoize its metadata-sourced encryption manager by metadata identity.

		// Use plain fileIO, not io(): metadata isn't envelope-encrypted and io() would re-enter
		// refresh.

		// Encryption keys are already persisted into the metadata by SnapshotProducer.
		TableMetadata tableMetadata = metadata;

		// Source keys from the uncommitted metadata so staged transaction operations can read
		// snapshots produced by earlier operations before the transaction commits.


		transaction.commitTransaction();

		// Reading forces decryption of every snapshot's manifest list, including the direct commit's.

		List<EncryptedKey> keys = current() == null ? List.of() : current().encryptionKeys();
		return EncryptionTestHelpers.createEncryptionManager(keys);

		// Reading the staged snapshot's manifest list decrypts it, proving its key was persisted into
		// metadata even though the snapshot is not a branch head.

		// Re-point temp ops at the refreshed metadata so a metadata-sourced encryption manager can
		// resolve keys for the concurrently-committed snapshots the refresh pulled in.

Uh oh!

Conversation

smaheshwar-pltr commented Jul 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant