ggershinsky commented on code in PR #13225:
URL: https://github.com/apache/iceberg/pull/13225#discussion_r2480301530
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -166,7 +208,67 @@ public void commit(TableMetadata base, TableMetadata
metadata) {
@Override
public FileIO io() {
- return io;
+ if (encryptionKeyId == null) {
+ return io;
+ }
+
+ if (encryptingFileIO == null) {
+ encryptingFileIO = EncryptingFileIO.combine(io, encryption());
+ }
+
+ return encryptingFileIO;
+ }
+
+ @Override
+ public EncryptionManager encryption() {
+ if (encryptionManager != null) {
+ return encryptionManager;
+ }
+
+ if (encryptionKeyId != null) {
+ if (kmsClient == null) {
+ throw new RuntimeException(
+ "Cant create encryption manager, because key management client is
not set");
+ }
+
+ Map<String, String> tableProperties = Maps.newHashMap();
+ tableProperties.put(TableProperties.ENCRYPTION_TABLE_KEY,
encryptionKeyId);
+ tableProperties.put(
+ TableProperties.ENCRYPTION_DEK_LENGTH,
String.valueOf(encryptionDekLength));
+ encryptionManager =
+ EncryptionUtil.createEncryptionManager(
+ encryptedKeysFromMetadata, tableProperties, kmsClient);
+ } else {
+ return PlaintextEncryptionManager.instance();
+ }
+
+ return encryptionManager;
+ }
+
+ private void encryptionPropsFromMetadata(TableMetadata metadata) {
Review Comment:
Yep, technically the AES GCM Stream can be used for encryption of the
metadata.json file. But I wouldn't require (or even recommend) this in the REST
spec, for the following reasons:
- risk of losing a table
- security (and potentially implementation) overkill: all we need is keeping
the metadata in a safe db/storage, isolated from the data storage; don't have
to encrypt it.
- unnecessary micromanagement: custom catalogs can do whatever, as long as
the API spec and security requirements are met.
Having said that - if some catalog implementation decides to continue
keeping the metadata.json in data storage and using it for REST calls, but
applies the AES GCM Stream to encrypt it there (investing extra effort in
reliable key storage, so tables are not lost; building a key access control;
and keeping the json file IDs / AAD prefixes isolated from data storage, to
prevent replay attacks) - then yes, I think this would work, and might even be
a reasonable solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
