Support encrypted table operations and server-side purge by hkwi · Pull Request #5060 · apache/polaris

hkwi · 2026-07-15T05:50:51Z

Summary

This draft adds Polaris-side Iceberg table encryption support for table operations and asynchronous server-side purge.

Initialize and own an Iceberg KeyManagementClient from catalog properties.
Build a metadata-aware EncryptionManager and EncryptingFileIO for encrypted tables.
Use uncommitted table metadata while a transaction writes manifests, and persist the wrapped keys generated by the encryption manager in table metadata at commit.
Carry a structured, trusted encryption context into asynchronous cleanup tasks so server-side purge can read encrypted metadata, manifest lists, and manifests before deleting table files.
Close task-owned KMS and FileIO resources on both successful and failed initialization paths.

Related to #2829, which tracks the broader Iceberg encryption support discussion.

Motivation and scope

An Iceberg client can write an encrypted table, but Polaris also executes Iceberg I/O in server-side paths such as purge. Those paths need an encryption manager capable of reading encrypted manifest metadata; otherwise the existing cleanup handlers cannot enumerate and delete all files in an encrypted table.

This change uses Iceberg's existing catalog-property contract (encryption.kms-type / encryption.kms-impl and implementation-specific properties). The Polaris server and an Iceberg client may use the same KMS implementation and settings, but they are configured independently and only need to resolve the same table key ID. An out-of-tree KeyManagementClient can be used when its implementation is present on the Polaris runtime classpath.

This draft does not vend KMS configuration or credentials through the REST Catalog API, define key rotation, or attempt to cover all possible REST semantics for encrypted tables.

Implementation notes

Temporary table operations resolve encryption from the transaction's uncommitted metadata instead of the outer operation's committed metadata cache.
The encryption manager remains owned by the outer table operations instance so wrapped keys registered while writing manifests survive until the final metadata commit.
Cached managers are bound to the table key ID and DEK length. Plaintext results are not cached, and incompatible metadata fails fast rather than reusing a stale manager.
Newly committed metadata is exposed through the local operations cache only after the Polaris catalog create/update succeeds.
Cleanup tasks store KMS configuration and wrapped keys in one structured internal encryption context. That context is not passed to FileIO initialization.

Tests

The targeted tests pass for both NoSQL in-memory and relational catalog implementations, covering:

encrypted create transactions and encrypted manifest/manifest-list output;
encrypted table write, read, refresh, reload, and server-side purge;
metadata-bound encryption manager reuse and mismatch rejection;
structured cleanup-context serialization and isolation from FileIO properties;
encrypted task FileIO round trips and resource closing; and
preservation of the current metadata cache after a failed catalog commit.

Commands run:

./gradlew :polaris-runtime-service:test <targeted encryption, purge, transaction, and commit-failure tests>
./gradlew :polaris-core:spotlessJavaCheck :polaris-runtime-service:spotlessJavaCheck

This PR does not enable or change coverage tooling.

Checklist

🛡️ Don't disclose security issues! (contact security@apache.org)
🔗 Clearly explained why the changes are needed, or linked related issues: Related to Iceberg encryption support #2829
🧪 Added/updated tests with good coverage, or manually tested (and explained how)
💡 Added comments for complex logic
🧾 Updated CHANGELOG.md (if needed; deferred while this is a draft)
📚 Updated documentation in site/content/in-dev/unreleased (if needed; deferred while this is a draft)

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

dimas-b

Thanks for your contribution, @hkwi !

Given that it's a major change, would you mind starting a corresponding discussion on the dev ML for visibility?

This is just a preliminary review from my side, but in general the PR looks pretty solid.

dimas-b · 2026-07-15T22:07:02Z

      }

-      // We diverge from `BaseMetastoreTableOperations` in the below code block
+      if (null == existingLocation) {


Will this conflict with #5057? 🤔

Thanks for flagging this. I merged the current main, including #5057, without force-pushing. The conflict was in doCommit. I kept the MetadataWriteResult/writeSucceeded cleanup flow from #5057 unchanged and removed the writer-side encryption changes from that path. The remaining LocalIcebergCatalog change only preserves encryption context for the server-side purge task, so the two changes no longer overlap semantically.

flyrain · 2026-07-17T17:34:34Z

Thanks @hkwi for working on it. The scope sounds good to me.

flyrain · 2026-07-17T17:40:16Z

    closeableGroup.addCloseable(metricsReporter());
+    if (properties.containsKey(CatalogProperties.ENCRYPTION_KMS_TYPE)
+        || properties.containsKey(CatalogProperties.ENCRYPTION_KMS_IMPL)) {
+      keyManagementClient = EncryptionUtil.createKmsClient(properties);


I think we should document how the KMS client obtains credentials for the KMS store, especially when multiple KMS stores are used. Not a blocker, though. Either a follow up or documenting it in this PR would be nice.

Thanks. I added a code comment at the dynamic KMS load site explaining that this follows the same structural pattern as CatalogUtil.loadFileIO: the configured KeyManagementClient is instantiated from trusted catalog properties, and credential resolution is delegated to that implementation. The cleanup task keeps those catalog KMS properties and the wrapped table keys in a task-only envelope rather than merging them into FileIO/storage properties. If a custom client selects among multiple KMS stores, that selection and credential handling remain the responsibility of that KeyManagementClient. Broader deployment documentation can follow separately.

FileIO uses the credential vending to get the storage credentials. We don't have a counterpart for KMS credentials. I'm confused how it can work. Here is a WIP PR to add KMS credentials, apache/iceberg#17155. cc @singhpk234

Thanks, I agree that there is currently no generic REST counterpart for vending KMS credentials.

Iceberg #17155 is relevant when a REST client relies on access delegation: the catalog would need to vend scoped KMS credentials along with storage credentials. The server-side purge path in this PR has a different credential boundary. The KeyManagementClient runs inside the Polaris cleanup worker, so its implementation can obtain server-side credentials from the worker's deployment environment, for example through workload identity or a local credential proxy, without putting credentials in catalog properties.

I put together a runnable example of the latter model:

https://github.com/hkwi/compose-demo-polaris-server-side-purge

It uses MinIO/S3 and Vault Transit. The engine and Polaris are configured with the same KMS proxy URI, but isolated network aliases route them to separate environment-local proxies. Each proxy owns its own scoped Vault token; the Java KMS client receives no token. The engine-side proxy permits encrypt and decrypt, while the Polaris-side proxy permits decrypt only.

The demo writes encrypted data, manifest list, and manifest files, verifies their AGS1 headers, and then verifies that Polaris server-side purge removes all of them.

This demonstrates one working credential model without REST KMS credential vending. I see Iceberg #17155 as complementary for clients that use REST KMS access delegation, rather than as a prerequisite for this server-side purge path. The demo intentionally covers a single KMS store and does not claim generic multiple-store credential routing.

dimas-b · 2026-07-18T20:13:51Z


-      String newLocation = writeNewMetadataIfRequired(base == null, metadata);
+      // SnapshotProducer may already have used this manager to generate and register encryption
+      // keys while writing encrypted manifests. Recreating it here would discard those keys


Polaris Servers never write manifests, IIRC... or did I miss it?

You are right: Polaris should not write manifests. I removed the writer-side EncryptionManager/FileIO initialization and the doCommit key propagation from LocalIcebergCatalog. The only server-side KMS path left is in cleanup tasks, where Polaris reads and decrypts manifest lists and manifests previously written by the engine so that purge can enumerate files to delete. I also added a comment at task creation to make this ownership boundary explicit.

dimas-b · 2026-07-18T20:18:04Z

+
+    @Override
+    public EncryptionManager encryption() {
+      return encryption(currentMetadata);


Why is this override necessary? IIRC, Polaris only writes metadata files, which are not encrypted? 🤔

Agreed. That override was only supporting writer-side encryption and is no longer necessary, so I removed it along with the writer-side encryption manager. Polaris continues to write the plaintext metadata JSON using the normal FileIO. A separate encryption-aware FileIO is reconstructed only by cleanup tasks to read encrypted manifest lists and manifests during server-side purge.

dimas-b · 2026-07-18T20:23:29Z

+              .withProperty(TableProperties.FORMAT_VERSION, "3")
+              .withProperty(TableProperties.ENCRYPTION_TABLE_KEY, PolarisTestKms.MASTER_KEY_NAME)
+              .createTransaction();
+      transaction.table().newFastAppend().appendFile(FILE_A).commit();


This test appears to require more from LocalIcebergCatalog than what is necessary for usual Polaris operation.

Would it be possible to convert this to an integration test similar to RestCatalogRustFSSpecialIT?

Encrypted writes would normally be performed by the "engine" (represented by test) code, which Polaris should only handle purges. WDYT?

Agreed. I replaced the LocalIcebergCatalog test with a REST integration test in RestCatalogFileIntegrationTest. The test-side client/engine writes encrypted data, manifest lists, and manifests, verifies their raw AGS1 headers, commits through REST, requests server-side purge, and verifies deletion. The Polaris production path only handles the purge. Because the corresponding Iceberg REST client encryption support in apache/iceberg#13225 is still pending, the test uses a test-only TableOperations adapter to model the engine-side behavior.

…server-purge-squashed # Conflicts: # runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java

hkwi · 2026-07-21T04:57:03Z

Thanks for the preliminary review. I pushed an update that merges the current main and addresses the inline feedback: writer-side encryption was removed from LocalIcebergCatalog, the remaining KMS use is scoped to reading encrypted manifest lists/manifests during server-side purge, and the test was moved to a REST integration test where the test-side engine performs encrypted writes. Validation completed: format and compileAll passed; PolarisEncryptionUtilTest, TaskFileIOSupplierTest, and RestCatalogFileIntegrationTest.encryptedTableWrittenByClientCanBePurgedByServer passed. I will handle the requested dev mailing-list discussion separately; no ML post has been made yet.

flyrain · 2026-07-21T21:06:04Z

+      // EncryptingFileIO closes an EncryptionManager when it is Closeable. The subclass preserves
+      // StandardEncryptionManager's type because Iceberg uses that type to decrypt manifest-list
+      // key metadata, while also closing the task-owned KMS client.
+      FileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);


The KMS client leaks if the raw FileIO throws during close(). Iceberg 1.11.0 closes its delegate first, so that exception prevents it from reaching this closeable encryption manager. We will need to fix EncryptingFileIO.close() upstream(Iceberg) to use try/finally or equivalent suppression logic. Not a blocker.

Thanks for catching this. I agree that this is an Iceberg lifecycle issue rather than something specific to the Polaris cleanup path.

I opened the following upstream fix:

apache/iceberg#17329

It updates EncryptingFileIO.close() to use try-with-resources so that a closeable EncryptionManager is still closed when the delegate FileIO throws. If both close operations fail, the EncryptionManager exception is retained as a suppressed exception. The upstream CI checks are passing.

Since this was marked as non-blocking and the lifecycle is owned by EncryptingFileIO, I have not added a Polaris-specific workaround.

flyrain · 2026-07-21T22:30:32Z

    closeableGroup.addCloseable(metricsReporter());
+    if (properties.containsKey(CatalogProperties.ENCRYPTION_KMS_TYPE)
+        || properties.containsKey(CatalogProperties.ENCRYPTION_KMS_IMPL)) {
+      keyManagementClient = EncryptionUtil.createKmsClient(properties);


FileIO uses the credential vending to get the storage credentials. We don't have a counterpart for KMS credentials. I'm confused how it can work. Here is a WIP PR to add KMS credentials, apache/iceberg#17155. cc @singhpk234

hkwi · 2026-07-22T05:07:51Z

As an update to the requested dev mailing-list discussion, I posted:

[DISCUSS] Current state of Iceberg table encryption in Polaris

It summarizes the responsibility boundary between engine-side encrypted writes, Polaris server-side purge, and REST KMS credential vending, together with the related Iceberg and Polaris work.

dimas-b

LGTM with a few minor comments 👍

dimas-b · 2026-07-22T15:14:37Z

+
+    if (metadata == null
+        || metadata.properties().get(TableProperties.ENCRYPTION_TABLE_KEY) == null) {
+      return;


If we return here after the remove call on line 56, it will contradict the logic implied by the method name 🤷

If the remove is truly necessary, please rename the method. Suggestion: setupCleanupTaskEncryptionProperties()

The remove is necessary because taskProperties starts with user-configurable table metadata properties, while ENCRYPTION_CONTEXT is a reserved internal task field.

We need to discard any table-controlled value even when the table is not encrypted, so that the task field is either absent or populated by Polaris from trusted catalog settings and wrapped keys.

Since the method may remove the field without adding it again, I agree that the current name is misleading. I renamed it to setupCleanupTaskEncryptionProperties().

dimas-b · 2026-07-22T15:17:41Z

+    // merged into the FileIO and storage properties already resolved for the cleanup task.
+    CleanupTaskEncryptionContext context =
+        new CleanupTaskEncryptionContext(
+            Map.copyOf(catalogProperties),


The copy is probably redundant, given that the object is discarded right after serialization, but it makes the reader think that the object may be passed around, which would make the copy necessary.

Agreed. CleanupTaskEncryptionContext already makes a defensive copy of kmsProperties in its compact constructor, so the Map.copyOf call here was redundant.

I now pass catalogProperties directly; the context still retains an immutable snapshot.

dimas-b · 2026-07-22T15:24:43Z

-    Map<String, String> properties = new HashMap<>(internalProperties);
+    Map<String, String> taskProperties = task.getInternalPropertiesAsMap();
+    Map<String, String> fileIOProperties = new HashMap<>(taskProperties);
+    fileIOProperties.remove(PolarisTaskConstants.ENCRYPTION_CONTEXT);


Why is this remove necessary? Just trying to understand 🤔

Task properties serve two purposes here: they carry the settings used to initialize the raw FileIO, as well as the task-only encryption context used to construct the encryption-aware wrapper.

The encryption context contains the catalog KMS settings and wrapped table keys, so it should not be passed to FileIOFactory as an ordinary FileIO/storage property. Only the filtered copy is used for raw FileIO initialization; the original taskProperties retain the context and are passed to encryptTaskFileIO() below.

I added a comment here to make this two-stage use explicit.

dimas-b · 2026-07-22T15:28:47Z

+        .hasMessageContaining("Missing encryption context")
+        .satisfies(failure -> assertThat(failure.getSuppressed()).containsExactly(closeFailure));
+
+    verify(fileIO).close();


nit: this is probably redundant given the assert on line 70.

Agreed. The suppressed-exception assertion already demonstrates that fileIO.close() was invoked, so I removed the redundant explicit verification.

Support encrypted table operations and server-side purge

999271b

github-project-automation Bot added this to Basic Kanban Board Jul 15, 2026

github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Jul 15, 2026

hkwi marked this pull request as ready for review July 15, 2026 08:39

Copilot AI review requested due to automatic review settings July 15, 2026 08:39

Copilot AI reviewed Jul 15, 2026

dimas-b reviewed Jul 15, 2026

View reviewed changes

flyrain reviewed Jul 17, 2026

View reviewed changes

dimas-b reviewed Jul 18, 2026

View reviewed changes

hkwi mentioned this pull request Jul 21, 2026

Support server-side purge for encrypted tables hkwi/polaris#2

Draft

6 tasks

hkwi added 2 commits July 21, 2026 13:27

Merge remote-tracking branch 'origin/main' into feat/encrypted-table-…

66cc661

…server-purge-squashed # Conflicts: # runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java

Address review feedback for encrypted purge

c00fdf9

hkwi mentioned this pull request Jul 21, 2026

Iceberg encryption support #2829

Open

flyrain reviewed Jul 21, 2026

View reviewed changes

hkwi mentioned this pull request Jul 22, 2026

API: Close encryption manager after FileIO close failure apache/iceberg#17329

Open

dimas-b reviewed Jul 22, 2026

View reviewed changes

Address minor encrypted purge review comments

13abdb8

This was referenced Jul 24, 2026

feat: verify encrypted table metadata integrity hkwi/polaris#3

Draft

feat: enforce trusted Iceberg metadata pointers hkwi/polaris#4

Draft

Uh oh!

Conversation

hkwi commented Jul 15, 2026

Summary

Motivation and scope

Implementation notes

Tests

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment

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!

flyrain commented Jul 17, 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!

hkwi commented Jul 21, 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!

hkwi commented Jul 22, 2026

Uh oh!

dimas-b left a comment

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!

4 participants