Encrypted v3 tables: make encryption keys metadata-only (immutable EncryptionManager)#12
Draft
smaheshwar-pltr wants to merge 3 commits into
Draft
Encrypted v3 tables: make encryption keys metadata-only (immutable EncryptionManager)#12smaheshwar-pltr wants to merge 3 commits into
smaheshwar-pltr wants to merge 3 commits into
Conversation
Make StandardEncryptionManager immutable and sourced from table metadata;
minting becomes a pure operation that returns keys instead of storing them.
- StandardEncryptionManager: remove the mutable key map and all synchronized
guards over it. Constructor stores an immutable SerializableMap snapshot of
the metadata keys. addManifestListKeyMetadata -> mintManifestListKey, a pure
function returning MintedKeys{manifestListKey, newKeyEncryptionKey}. KEK
rotation (find-unexpired-else-mint) no longer stores; a freshly minted KEK is
returned so the caller can persist it. Lazy caches (unwrappedKeyCache, RNG)
stay transient with lock-free/DCL init.
- ManifestListWriter: mint via the pure API, cache the MintedKeys (idempotent
toManifestListFile), expose manifestListKeys().
- SnapshotProducer: capture the writer's minted keys in apply() and persist
them in addEncryptionKeys(); no longer reads keys back out of the manager.
- HiveTableOperations: delete the per-ops encryptionManager/encryptingFileIO/
encryptedKeys fields, encryptionLock, and refresh rebuild. Build the immutable
manager on demand from a metadata's keys; temp() sources uncommitted metadata
so staged transaction reads see prior-op keys. Read metadata JSON with plain
fileIO to avoid io()->current() refresh re-entrancy.
- BaseTransaction: on rebase after a concurrent commit, re-point temp ops at the
refreshed metadata so the metadata-sourced manager can resolve pulled-in keys.
- Tests: rewrite TestManifestListEncryption rotation + TestStandardEncryption-
ManagerConcurrency for the pure/immutable model; EncryptionTestHelpers gains a
from-keys factory.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address findings from codex / code-review / good-testing-patterns: - StandardEncryptionManager.encryptionKeys(): return an immutable (unmodifiable) view instead of the live SerializableMap, restoring the pre-redesign guarantee that the manager's key set cannot be mutated through the accessor. Revert the DCL on unwrappedKeyCache()/workerRNG() back to plain synchronized to match the convention set by the preceding review commit (fields stay transient volatile). - HiveTableOperations: memoize the on-demand manager by metadata identity so repeated io()/encryption() calls on the same metadata reuse one manager (and its KMS unwrap cache) instead of rebuilding it and re-copying the key map on every scan-planning/commit call. Simplify the null-metadata handling. - ManifestListWriter: correct the stale 'idempotent' comment. - TestManifestListEncryption: split the monolithic rotation test into three behavior-named tests (fresh-mint round trip, KEK reuse before rotation, KEK rotation after lifespan) that assert directly on the MintedKeys result rather than manually re-counting keys; drop the obscure reduce-throwing helper. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
codex flagged that HiveTableOperations' metadata-identity manager memoization relies on 'same TableMetadata instance == same keys', but the constructor stored the encryption-keys list by reference, so the invariant was not enforced. Defensively copy the list into an ImmutableList in the TableMetadata constructor (matching how build() already wraps snapshots/refs/etc.), making the key set of any TableMetadata instance immutable and the memoization sound. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Encryption keys for a v3 table already live durably in
TableMetadata.encryption-keys, butStandardEncryptionManagerkept a second, mutable, per-TableOperationscopy and minted new manifest-list keys (MLKs) and key-encryption-keys (KEKs) into that transient copy during commit. Keeping the two homes in sync is the root of the concurrency bugs the base branch patched — a non-thread-safe key map, keys lost when a refresh swapped the manager between mint and commit, and an unsynchronized refresh/rebuild — each of which needed its own lock/volatile/re-attach machinery.This removes the second home: keys live only in metadata, so concurrency-safety and refresh-consistency fall out by construction instead of by patching.
Changes
StandardEncryptionManageris now an immutable projection of the metadata key set — no mutable key map, nosynchronized.mintManifestListKey(...)is a pure function returningMintedKeys{ manifestListKey, newKeyEncryptionKey }; it stores nothing.SnapshotProducercaptures the minted keys from the manifest-list writer and persists them (MLK + KEK when freshly minted) into the committed metadata viaaddEncryptionKey.HiveTableOperations: deleted theencryptionManager/encryptingFileIO/encryptedKeysfields, theencryptionLock, and the rebuild-on-refresh dance; the manager is built on demand fromcurrent().encryptionKeys()(memoized by metadata identity).BaseTransactionre-points staged (temp) ops at the rebased metadata so a later op can read an earlier staged snapshot's keys;TableMetadatanow holds an immutable key list (makes the memoization sound);doRefreshreads the (unencrypted) metadata JSON via the plainFileIOto avoid anio()→current()re-entrancy.Trade-offs (called out honestly)
apply()(pre-commit) no longer yields a self-readable encrypted snapshot — an encrypted snapshot's key exists only once its metadata is committed. No caller relies on this; it is a deliberate contract narrowing.EncryptionManagerinterface still needs aninstanceof StandardEncryptionManagerfor manifest-list-key ops. A fuller split (pure crypto primitive + separate key-lifecycle service) was prototyped and judged not worth the extra API surface: its main payoff — keeping the key hierarchy off executors — doesn't hold, since metadata-table scans (all_manifests/manifests) decrypt manifest lists on executors.Tests
Behavioral bar green: concurrent shared-
Tableappends, append/replace transactions, "manifest-list key persisted to metadata", and plaintext + read paths. KEK-rotation tests were rewritten around the pure-mint result, and the manager concurrency test around the pure-mint contract (there is no shared mutable map left to race).🤖 Generated with Claude Code