Skip to content

Conversation

@smaheshwar-pltr
Copy link
Contributor

@smaheshwar-pltr smaheshwar-pltr commented Jun 3, 2025

This PR implements client-side support for REST catalog encryption. With it, clients interacting with a REST catalog can read and write encrypted data.

It is similar to #13066, that integrates encryption with the Hive catalog.

cc @rdblue @RussellSpitzer @ggershinsky

Comment on lines 54 to 57
// TODO(smaheshwar): This test is taken from https://github.com/apache/iceberg/pull/13066, with the
// exception of testCtas, but adapted for the REST catalog. When that merges, we can directly use
// those tests for the REST catalog as well by adding to the parameters method there, to have a
// single test class for table encryption.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Highlighting this - this file is largely taken from #13066

@smaheshwar-pltr smaheshwar-pltr marked this pull request as ready for review June 3, 2025 21:24
: Integer.parseInt(dekLength);
}

// Force re-creation of encryptingFileIO and encryptionManager
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@github-actions
Copy link

github-actions bot commented Jul 7, 2025

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jul 7, 2025
@smaheshwar-pltr
Copy link
Contributor Author

(Bump to remove staleness)

@github-actions github-actions bot removed the stale label Jul 8, 2025
@github-actions
Copy link

github-actions bot commented Aug 7, 2025

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Aug 7, 2025
@smaheshwar-pltr
Copy link
Contributor Author

(Bump to remove staleness)

@github-actions github-actions bot removed the stale label Aug 8, 2025
@github-actions
Copy link

github-actions bot commented Sep 8, 2025

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 8, 2025
@smaheshwar-pltr
Copy link
Contributor Author

(Bump to remove staleness)

* @param dataKeyLength length of data encryption key (16/24/32 bytes)
* @param kmsClient Client of KMS used to wrap/unwrap keys in envelope encryption
*/
public StandardEncryptionManager(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could deprecate the old constructor

import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.TestTemplate;

// TODO(smaheshwar-pltr): This test is taken from https://github.com/apache/iceberg/pull/13066, with
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a CTAS test to the ones authored by @ggershinsky here. Perhaps there's a world in which this PR merges first in which case can undo this comment. There are also review comments on these tests in #13066 that are potentially relevant

@smaheshwar-pltr
Copy link
Contributor Author

Given #7770 is merged, curious for thoughts on this PR.
REST integration sounds on the cards. Also happy for this PR to be superseded if other folks have worked on it.

cc @huaxingao @RussellSpitzer @ggershinsky @rdblue

@RussellSpitzer
Copy link
Member

Could you elaborate on the api additions? I think it would help to have some more description on the general direction of this or

@huaxingao
Copy link
Contributor

@smaheshwar-pltr Could you please resolve the conflicts?

@ggershinsky
Copy link
Contributor

@huaxingao @smaheshwar-pltr Our team has a person who works on encryption with the REST catalog. If @smaheshwar-pltr does not object, we can follow up on this patch.

return encryptionManager;
}

private void encryptionPropsFromMetadata(TableMetadata metadata) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this method applied on the TableMetadata that is fetched directly from the REST catalog, and not from the metadata.json file? Both are possible, but the former must override (and check) the latter, to protect against the key removal and other attacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this method will always be applied on a metadata field of a LoadTableResponse object received directly from the REST catalog (its only usage within this class is as such, and you can check the constructor usages within RESTSessionCatalog to confirm that the metadata coming in from there is as such too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question (not sure if there have been discussions here or if your team have thoughts): we want the key ID to come from the REST catalog service directly for security reasons.

It's typical for REST catalogs to provide metadata that corresponds to the metadata file in storage and not modify it apart from that. Given this, would it be preferable to have this field returned within the LoadTableResponse itself, to encourage catalogs to track it explicitly?

The concrete proposal here might be: ENCRYPTION_TABLE_KEY and ENCRYPTION_DEK_LENGTH become properties on the LoadTableResponse's config (mentioned in the REST spec here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Well I see two scenarios when thinking about this:

  1. metadata.json is something that both the server and the clients can read (although clients wouldn't need to, given they get the metadata with the LoadTableResponse )
  2. metadata.json can only be accessed on the server side and clients are not given FS credentials (either vended or not) to reach it

For case (1) I totally agree, we can't rely on just metadata.json to store these encryption properties, and the catalog should store it separately too, and eventually populating (i.e. doing the override logic referred by @ggershinsky) the properties in the LoadTableResponse to be created.
For case (2) I'm not 100% sure, but still leaning toward the catalog taking on this responsibility.

Either way, for the client side there's not much we can do other than recommending clients to consider the metadata from LoadTableResponse only. The rest (no pun intended) is on the server side to be decided and will be implementation-specific. For this code snippet above, irrelevant IMHO.

Let me know your thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Afaik, HMS is not optimized for JSON storage. But maybe someone in the community will take on storing the full metadata object there, to improve table security. Having only the table properties is barely sufficient. I think we should recommend REST catalogs for encrypted tables.

It's not. But would storing a hash suffice? If so we could generate the hash of the whole JSON content and store it via an additional (Hive) table property. Then during table loading we can verify that the TableMetadata we just read in from a potentially untrusted storage (and yet metadata.json is not encrypted) is original or has been tampered with.

Copy link
Contributor

Choose a reason for hiding this comment

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

One aspect of having an encrypted metadata.json is when the table schema is also considered a sensitive piece of information. I haven't found this in the discussions but do you know if this has ever been considered @ggershinsky ?

Copy link
Contributor

Choose a reason for hiding this comment

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

would storing a hash suffice? If so we could generate the hash of the whole JSON content and store it via an additional (Hive) table property. Then during table loading we can verify that the TableMetadata we just read in from a potentially untrusted storage (and yet metadata.json is not encrypted) is original or has been tampered with.

I think it's a good idea

Copy link
Contributor

Choose a reason for hiding this comment

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

One aspect of having an encrypted metadata.json is when the table schema is also considered a sensitive piece of information. I haven't found this in the discussions but do you know if this has ever been considered

Not sure. Though, it should be possible to have a REST implementation that hides the metadata.json file from the storage.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a good idea

Sounds good, I can take this on and will produce a PR shortly.

Not sure. Though, it should be possible to have a REST implementation that hides the metadata.json file from the storage.

Yes, with REST that's true, I just meant it in a general sense, e.g. it's not currently possible to hide the schema of an encrypted table with Hive catalog. It may just be one more thing to note/document as a limitation of encryption wrt. Hive catalog - just merely wanted to highlight this though.

@ggershinsky
Copy link
Contributor

Also, it would be good to refactor (if possible) a code common to this PR and to #13066 , so that other catalogs will be able to re-use it.

@github-actions github-actions bot added the build label Oct 27, 2025
@ggershinsky
Copy link
Contributor

@huaxingao @smaheshwar-pltr Our team has a person who works on encryption with the REST catalog. If @smaheshwar-pltr does not object, we can follow up on this patch.

Sorry for lack of responses, was out of office for a bit.

@ggershinsky, I'm happy to continue with this PR unless the approaches significantly differ, in which case happy to also hand it off or accept a review here! 🙏

Sure, no problem. I think the approach is similar.

private EncryptingFileIO encryptingFileIO;
private String tableKeyId;
private int encryptionDekLength;
private List<EncryptedKey> encryptedKeysFromMetadata;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also factor in the #14427 changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll need to think about whether those changes are needed here actually, given the metadata file equality check in updateCurrentMetadata (test passes).

I suspect it's best to have such changes so when there's new metadata, the old keys are kept around. Will have to think if there's a case for this though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies for the delay here, there is a case for this and this PR now needs those changes - it's concurrent replace transactions instead of append transactions. I have the full patch here, which I will rebase onto this PR once I've run some final tests

append.appendFile(dataFiles.get(0));

// append to the table in the meantime. use a separate load to avoid shared operations
validationCatalog.loadTable(tableIdent).newFastAppend().appendFile(dataFiles.get(0)).commit();
Copy link
Contributor

Choose a reason for hiding this comment

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

@szlta , could you have a quick look at this addition?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, this does look good to me

encryptedKeysFromMetadata = metadata.encryptionKeys();

Map<String, String> tableProperties = metadata.properties();
if (tableKeyId == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we always get tableKeyId in case there is a key rotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose we can always get it from the properties here though I suspect automatic key rotation often keeps key ID the same so perhaps reasonable to require that (otherwise would imagine you'd need tasks to commit new metadata with changed properties on rotation), cc @ggershinsky curious for thoughts / if there've been discussions on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

if the tableKeyId == null (here and below), it just means the table is not encrypted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the question more generally is whether encryption-related properties should be refreshed. On second thought and looking at the Hive implementation, I've made the change to do so. (This method is used differently to the similar one for Hive)

The question of encryption property changes on the "temporary" table operations of an encrypted table (used e.g. during a transaction, or when you replace a table "as select") feels more delicate, and I believe the current stance is not to respect those changes (the Hive implementation doesn't). So, the temp operations on this class similarly do not use the uncommitted metadata's properties.

List<MetadataUpdate> updates;

TableMetadata metadataToCommit = metadata;
if (encryption() instanceof StandardEncryptionManager) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to add this update to the metadata before we call commit ?
per API contract we should try commit metadata

* Replace the base table metadata with a new version.
*
* <p>This method should implement and document atomicity guarantees.
*
* <p>Implementations must check that the base metadata is current to avoid overwriting updates.
* Once the atomic commit operation succeeds, implementations must not perform any operations that
* may fail because failure in this method cannot be distinguished from commit failure.
*
* <p>Implementations must throw a {@link
* org.apache.iceberg.exceptions.CommitStateUnknownException} in cases where it cannot be
* determined if the commit succeeded or failed. For example if a network partition causes the
* confirmation of the commit to be lost, the implementation should throw a
* CommitStateUnknownException. This is important because downstream users of this API need to
* know whether they can clean up the commit or not, if the state is unknown then it is not safe
* to remove any files. All other exceptions will be treated as if the commit has failed.
*
* @param base table metadata on which changes were based
* @param metadata new table metadata with updates
*/

Copy link
Contributor

@ggershinsky ggershinsky Nov 9, 2025

Choose a reason for hiding this comment

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

A couple of points,

  1. The stated requirement is to "check that the base metadata is current to avoid overwriting updates". This is met. We add a missing field to the new metadata after the check.
  2. There are lots of calls to commit from various classes. Moving this outside will be technically hard (if it works at all).

if (base != null) {
Set<String> removedProps =
base.properties().keySet().stream()
.filter(key -> !metadata.properties().containsKey(key))
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: for consistency ?

Suggested change
.filter(key -> !metadata.properties().containsKey(key))
.filter(key -> !metadataToCommit.properties().containsKey(key))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks - eacbfe7

Comment on lines 203 to 211
if (removedProps.contains(TableProperties.ENCRYPTION_TABLE_KEY)) {
throw new RuntimeException("Cannot remove key in encrypted table");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be an IllegalState ? instead of RTE ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @ggershinsky perhaps similar situation to #13225 (comment).

Thoughts on IllegalArgumentException? (If you alter this property and try to commit, IllegalArgumentException maybe makes sense?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll be ok with any decision on this; will add to the HiveTO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with IllegalArgumentException but open to change. Also, I put up #14659.

Comment on lines 344 to 348
String dekLength = tableProperties.get(TableProperties.ENCRYPTION_DEK_LENGTH);
encryptionDekLength =
(dekLength == null)
? TableProperties.ENCRYPTION_DEK_LENGTH_DEFAULT
: Integer.parseInt(dekLength);
Copy link
Contributor

Choose a reason for hiding this comment

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

can use PropertyUtils ?

Copy link
Contributor Author

@smaheshwar-pltr smaheshwar-pltr Nov 8, 2025

Choose a reason for hiding this comment

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

Agree, cc @ggershinsky maybe similar situation to #13225 (comment).

(Happy to think / put up these patches if it'd help, but the original code is @ggershinsky's so will wait 🙏)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I'm working on an "encryption clean up" patch, will add this to the todo list.

return encryptionManager;
}

private void encryptionPropsFromMetadata(TableMetadata metadata) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you everyone for great discussions !

There is one more perspective I wanna share, which IMHO we should consider

That metadata.json should be consistent to what we have done in other catalogs, one of the reasons to have a metadata.json is to have portability between different catalogs for example one can migrate from hive to REST and REST to hive, but if we store part of metadata in different place and plan to return in /config, imho it will become a concern to portability

Also please consider there are REST server implementations out there who cache the whole metadata.json in memory / db to avoid lookup to remote store.

In rest we can add requirement for an update that we can't drop it / meanwhile server can also double assert, if there is a concern for malicious clients and the key update or drop, then an implicit trust relation between catalog and client is required which means a catalog can reject the whole update if it doesn't trust the client ? if the concern is server tampering stuff, then a lot of things go at toss ? we requires similar trust relation for other works as wells :

  • DEFINER views
  • Access Decision Exchange

@ggershinsky
Copy link
Contributor

I wonder if #14465 could affect safety of encrypted tables, and if yes, how this can be handled..

@XJDKC
Copy link
Member

XJDKC commented Nov 6, 2025

I wonder if #14465 could affect safety of encrypted tables, and if yes, how this can be handled..

IIUC, the encryption key must be accessible to the query engine in order for it to decrypt and encrypt the data. Could you please help me understand why allowing a custom builder would raise security concerns?

From my perspective, even without that PR, users could still copy the code and replace the default TableOperations implementation. The PR just simply introduces a generic interface to make such customization cleaner and more maintainable.

@ggershinsky
Copy link
Contributor

Thanks @XJDKC , it's likely just a matter of documenting the new interface to make sure the users are aware of the security aspects of the REST TO (if they plan to use table encryption).

why allowing a custom builder would raise security concerns?

Maybe its ok, but we need to check the risk for metadata integrity (if broken, can be used for data leaks and other attacks), as discussed in this PR comments - making sure the client gets the metadata from the REST server, and not from the metadata.json file.
I'll have a look at the 14465 details to see if there are other security implications.

@smaheshwar-pltr
Copy link
Contributor Author

Thanks @XJDKC , it's likely just a matter of documenting the new interface to make sure the users are aware of the security aspects of the REST TO (if they plan to use table encryption).

Maybe its ok, but we need to check the risk for metadata integrity (if broken, can be used for data leaks and other attacks), as discussed in this PR comments - making sure the client gets the metadata from the REST server, and not from the metadata.json file.

Thanks both for the discussions here.

I don't see anything concerning with the motivation behind the other PR. If a a REST client wanted to read from the metadata JSON in storage, they can do so regardless after calling loadTable - so enabling custom operations builders doesn't enable but facilitates custom client behaviour.

As such, I suspect the documentation should be at the spec level to advise clients if required, possibly depending on the corresponding REST spec discussions / conclusions (#14486 / https://lists.apache.org/thread/0nn11o4xf1nmw68d4px33sxw5tzzmgbo).

@ggershinsky
Copy link
Contributor

ggershinsky commented Nov 9, 2025

I'm also fine with the motivation. Regarding the support for encrypted tables, I have two concerns,

  1. Unrelated to REST. What if the encryption.key-id table property is set (https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/TableProperties.java#L391) but a custom TO implementation ignores it. Do the users expect a table to be encrypted if the encryption.key-id is set? Should the implementors of custom TOs validate them by running the Iceberg unitests (inc TestTableEncryption)?
  2. Related to REST. The standard RESTableIOperations, built in Iceberg, is verified (in this PR) to be safe wrt metadata access. A custom TO replacement can behave differently of course.

I think both concerns can be addressed by adding a few lines to the RESTOperationsBuilder javadoc API comments (not to the REST spec, because the first point is unrelated, and the second is related only to client impl, not the server impl)

@huaxingao
Copy link
Contributor

@smaheshwar-pltr I have merged the encryption clean up PR. Could you please rebase?

@smaheshwar-pltr smaheshwar-pltr force-pushed the rest-encrypt-on-main-cherry-pick branch from eacbfe7 to ed1165b Compare November 22, 2025 17:23
@smaheshwar-pltr smaheshwar-pltr changed the title REST encryption integration Encryption for REST catalog Nov 22, 2025
@smaheshwar-pltr smaheshwar-pltr force-pushed the rest-encrypt-on-main-cherry-pick branch from ed1165b to da894ba Compare November 22, 2025 18:04
@smaheshwar-pltr smaheshwar-pltr force-pushed the rest-encrypt-on-main-cherry-pick branch 2 times, most recently from c7f7e65 to 49114e7 Compare November 22, 2025 21:48
@smaheshwar-pltr smaheshwar-pltr force-pushed the rest-encrypt-on-main-cherry-pick branch from 49114e7 to 11ff326 Compare November 22, 2025 21:55
@github-actions github-actions bot added the hive label Nov 22, 2025

if (removedProps.contains(TableProperties.ENCRYPTION_TABLE_KEY)) {
throw new RuntimeException("Cannot remove key in encrypted table");
throw new RuntimeException("Cannot remove encryption key ID from an encrypted table");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Wanted to make this more descriptive on the REST side, changed here too for Hive in this PR so tests pass)

TableMetadata.Builder builder = TableMetadata.buildFrom(metadata);
for (Map.Entry<String, EncryptedKey> entry :
EncryptionUtil.encryptionKeys(encryption()).entrySet()) {
builder.addEncryptionKey(entry.getValue());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This adds the required REST updates to the metadata to be committed

Comment on lines +82 to +87
// keys loaded from the latest metadata
private Optional<List<EncryptedKey>> encryptedKeysFromMetadata = Optional.empty();

// keys added to EM (e.g. as a result of a FileAppend) but not committed into the latest metadata
// yet
private Optional<List<EncryptedKey>> encryptedKeysPending = Optional.empty();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The flow here felt a bit complicated to me, so I put up #14752 separately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants