Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,7 @@ public class PolarisTaskConstants {
public static final String TASK_DATA = "data";
public static final String TASK_TYPE = "taskType";
public static final String STORAGE_LOCATION = "storageLocation";

/** Serialized encryption context used by an Iceberg cleanup task. */
public static final String ENCRYPTION_CONTEXT = "encryptionContext";
}
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,12 @@
import org.apache.iceberg.catalog.Namespace;
import org.apache.iceberg.catalog.SupportsNamespaces;
import org.apache.iceberg.catalog.TableIdentifier;
import org.apache.iceberg.encryption.EncryptingFileIO;
import org.apache.iceberg.encryption.EncryptionManager;
import org.apache.iceberg.encryption.EncryptionUtil;
import org.apache.iceberg.encryption.KeyManagementClient;
import org.apache.iceberg.encryption.PlaintextEncryptionManager;
import org.apache.iceberg.encryption.StandardEncryptionManager;
import org.apache.iceberg.exceptions.AlreadyExistsException;
import org.apache.iceberg.exceptions.BadRequestException;
import org.apache.iceberg.exceptions.CommitFailedException;
Expand Down Expand Up @@ -136,6 +141,7 @@
import org.apache.polaris.service.catalog.common.CatalogUtils;
import org.apache.polaris.service.catalog.common.LocationUtils;
import org.apache.polaris.service.catalog.io.FileIOFactory;
import org.apache.polaris.service.catalog.io.PolarisEncryptionUtil;
import org.apache.polaris.service.catalog.io.StorageAccessConfigProvider;
import org.apache.polaris.service.catalog.validation.IcebergPropertiesValidation;
import org.apache.polaris.service.events.EventAttributeMap;
Expand Down Expand Up @@ -191,6 +197,7 @@ public class LocalIcebergCatalog extends BaseMetastoreViewCatalog
private FileIO catalogFileIO;
private CloseableGroup closeableGroup;
private Map<String, String> tableDefaultProperties;
private KeyManagementClient keyManagementClient;

private final String catalogName;
private final long catalogId;
Expand Down Expand Up @@ -321,6 +328,11 @@ public void initialize(String name, Map<String, String> properties) {

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

closeableGroup.addCloseable(keyManagementClient);
}
closeableGroup.setSuppressCloseFailure(true);

tableDefaultProperties =
Expand Down Expand Up @@ -579,6 +591,8 @@ public boolean dropTable(TableIdentifier tableIdentifier, boolean purge) {
clone.put(CatalogProperties.FILE_IO_IMPL, ioImplClassName);
clone.putAll(properties);
clone.put(PolarisTaskConstants.STORAGE_LOCATION, lastMetadata.location());
PolarisEncryptionUtil.addCleanupTaskEncryptionProperties(
clone, catalogProperties, lastMetadata);
return clone;
})
.orElse(Map.of());
Expand Down Expand Up @@ -1621,6 +1635,26 @@ public ViewBuilder withLocation(String newLocation) {
}
}

private record TableEncryptionConfig(String tableKeyId, int dataKeyLength) {
private static @Nullable TableEncryptionConfig from(TableMetadata metadata) {
if (metadata == null) {
return null;
}

String tableKeyId = metadata.properties().get(TableProperties.ENCRYPTION_TABLE_KEY);
if (tableKeyId == null) {
return null;
}

return new TableEncryptionConfig(
tableKeyId,
PropertyUtil.propertyAsInt(
metadata.properties(),
TableProperties.ENCRYPTION_DEK_LENGTH,
TableProperties.ENCRYPTION_DEK_LENGTH_DEFAULT));
}
}

/**
* An implementation of {@link TableOperations} that integrates with {@link LocalIcebergCatalog}.
* Much of this code was originally copied from {@link
Expand All @@ -1635,6 +1669,9 @@ public class BasePolarisTableOperations extends PolarisOperationsBase<TableMetad
private final boolean makeMetadataCurrentOnCommit;

private FileIO tableFileIO;
private EncryptionManager encryptionManager;
private TableEncryptionConfig encryptionManagerConfig;
private EncryptingFileIO encryptingFileIO;

BasePolarisTableOperations(
FileIO defaultFileIO,
Expand Down Expand Up @@ -1670,6 +1707,7 @@ public TableMetadata refresh() {
currentMetadata = null;
currentMetadataLocation = null;
version = -1;
resetEncryptionState();
throw e;
}
return current();
Expand Down Expand Up @@ -1707,7 +1745,65 @@ public void commit(TableMetadata base, TableMetadata metadata) {

@Override
public FileIO io() {
return tableFileIO;
return io(currentMetadata);
}

private FileIO io(TableMetadata metadata) {
Preconditions.checkState(
tableFileIO != null, "FileIO has not been initialized for table %s", fullTableName);

EncryptionManager manager = encryption(metadata);
if (manager == PlaintextEncryptionManager.instance()) {
return tableFileIO;
}

if (encryptingFileIO == null) {
encryptingFileIO = EncryptingFileIO.combine(tableFileIO, manager);
}
return encryptingFileIO;
}

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

}

private EncryptionManager encryption(TableMetadata metadata) {
TableEncryptionConfig requestedConfig = TableEncryptionConfig.from(metadata);
if (encryptionManager != null) {
// A successful new-table commit may leave currentMetadata null until the next refresh, but
// transaction cleanup still calls the public io() method. Preserve the manager that was
// bound to the committed metadata in that case. A non-null plaintext metadata object is a
// real configuration change and is rejected below.
if (metadata == null) {
return encryptionManager;
}

Preconditions.checkState(
requestedConfig != null && requestedConfig.equals(encryptionManagerConfig),
"Cannot reuse encryption manager for table %s: initialized with %s but metadata requires %s",
fullTableName,
encryptionManagerConfig,
requestedConfig == null ? "plaintext" : requestedConfig);
return encryptionManager;
}

// Do not cache the plaintext singleton. In particular, io() may be called before metadata is
// loaded for a new table, and that must not prevent temporary operations from initializing
// encryption from uncommitted metadata later.
if (requestedConfig == null) {
return PlaintextEncryptionManager.instance();
}

encryptionManager = PolarisEncryptionUtil.encryptionManager(metadata, keyManagementClient);
encryptionManagerConfig = requestedConfig;
return encryptionManager;
}

private void resetEncryptionState() {
encryptionManager = null;
encryptionManagerConfig = null;
encryptingFileIO = null;
}

@Override
Expand Down Expand Up @@ -1771,7 +1867,10 @@ public void doRefresh() {
resolvedEntities,
new HashMap<>(tableDefaultProperties),
Set.of(PolarisStorageActions.READ, PolarisStorageActions.LIST));
return TableMetadataParser.read(fileIO, metadataLocation);
TableMetadata metadata = TableMetadataParser.read(fileIO, metadataLocation);
tableFileIO = fileIO;
resetEncryptionState();
return metadata;
});
if (polarisEventDispatcher.hasListeners(PolarisEventType.AFTER_REFRESH_TABLE)) {
polarisEventDispatcher.dispatch(
Expand Down Expand Up @@ -1850,7 +1949,23 @@ public void doCommit(TableMetadata base, TableMetadata metadata) {
PolarisStorageActions.WRITE,
PolarisStorageActions.LIST));

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

// before they are persisted in the new table metadata. A new-table commit has no current
// metadata and therefore still initializes the manager from the proposed metadata.
encryption(metadata);
encryptingFileIO = null;

TableMetadata tableMetadata = metadata;
if (encryptionManager instanceof StandardEncryptionManager) {
TableMetadata.Builder builder = TableMetadata.buildFrom(metadata);
for (var entry : EncryptionUtil.encryptionKeys(encryptionManager).entrySet()) {
builder.addEncryptionKey(entry.getValue());
}
tableMetadata = builder.build();
}

String newLocation = writeNewMetadataIfRequired(base == null, tableMetadata);
String oldLocation = base == null ? null : base.metadataFileLocation();

// TODO: Consider using the entity from doRefresh() directly to do the conflict detection
Expand Down Expand Up @@ -1923,21 +2038,22 @@ public void doCommit(TableMetadata base, TableMetadata metadata) {
tableIdentifier, oldLocation, newLocation, existingLocation);
}

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this conflict with #5057? 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

createTableLike(tableIdentifier, entity);
} else {
updateTableLike(tableIdentifier, entity);
}

// We diverge from `BaseMetastoreTableOperations` in the below code block. Only expose the
// new metadata through this operations instance after the catalog commit has succeeded.
if (makeMetadataCurrentOnCommit) {
currentMetadata =
TableMetadata.buildFrom(metadata)
TableMetadata.buildFrom(tableMetadata)
.withMetadataLocation(newLocation)
.discardChanges()
.build();
currentMetadataLocation = newLocation;
}

if (null == existingLocation) {
createTableLike(tableIdentifier, entity);
} else {
updateTableLike(tableIdentifier, entity);
}
}

private boolean requestedTableLocationsChanged(TableMetadata base, TableMetadata metadata) {
Expand Down Expand Up @@ -1995,12 +2111,12 @@ public LocationProvider locationProvider() {

@Override
public FileIO io() {
return BasePolarisTableOperations.this.io();
return BasePolarisTableOperations.this.io(uncommittedMetadata);
}

@Override
public EncryptionManager encryption() {
return BasePolarisTableOperations.this.encryption();
return BasePolarisTableOperations.this.encryption(uncommittedMetadata);
}

@Override
Expand All @@ -2018,7 +2134,7 @@ protected String writeNewMetadataIfRequired(boolean newTable, TableMetadata meta

protected String writeNewMetadata(TableMetadata metadata, int newVersion) {
String newTableMetadataFilePath = newTableMetadataFilePath(metadata, newVersion);
OutputFile newMetadataLocation = io().newOutputFile(newTableMetadataFilePath);
OutputFile newMetadataLocation = io(metadata).newOutputFile(newTableMetadataFilePath);

// write the new metadata
// use overwrite to avoid negative caching in S3. this is safe because the metadata location
Expand Down
Loading