Encryption integration and test #5544

ggershinsky · 2022-08-16T06:28:44Z

Initially, only for Hive catalogs

pvary · 2023-01-16T10:48:13Z

core/src/main/java/org/apache/iceberg/encryption/DefaultEncryptionManagerFactory.java

+    Map<String, String> tableProperties = tableMetadata.properties();
+    String fileFormat =
+        PropertyUtil.propertyAsString(tableProperties, DEFAULT_FILE_FORMAT, "parquet");
+    if (!fileFormat.equals("parquet")) {


FileFormat.fromString should be used to match the general usage pattern.
In other cases we accept upper case PARQUET too, so I think we should accept it here too.

SGTM, thanks for the suggestion.

pvary · 2023-01-16T10:49:48Z

core/src/main/java/org/apache/iceberg/encryption/DefaultEncryptionManagerFactory.java

+    // Important: put catalog properties after table properties. Former overrides the latter.
+    encryptionProperties.putAll(catalogPropertyMap);
+
+    String tableKeyId = System.getProperty(ICEBERG_ENCRYPTION_TABLE_KEY);


This seems odd to me.
Do we rely on System properties?

This is basically a hack that allows to address a challenge of using different keys for different tables - in those cases where the table properties are not trusted. I'd say the purpose is somewhere between a "temporary solution" and a "discussion trigger" :). In the long run, we might have table properties safely kept in catalogs (eg the REST catalog), but we need a solution in a shorter term.

rdblue · 2023-02-27T18:26:28Z

core/src/main/java/org/apache/iceberg/encryption/EncryptionManagerFactory.java

+   * @param properties catalog properties
+   */
+  default void initialize(Map<String, String> properties) {}
+  /**


Missing whitespace.

rdblue · 2023-09-10T21:23:35Z

core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java

        fileIOImpl == null
            ? new HadoopFileIO(conf)
            : CatalogUtil.loadFileIO(fileIOImpl, properties, conf);
+    this.encryptionManagerFactory = new DefaultEncryptionManagerFactory();


Is the intent for this to be a StandardEncryptionManagerFactory?

rdblue · 2023-09-10T21:24:31Z

core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java

            ? new HadoopFileIO(conf)
            : CatalogUtil.loadFileIO(fileIOImpl, properties, conf);
+    this.encryptionManagerFactory = new DefaultEncryptionManagerFactory();
+    this.encryptionManagerFactory.initialize(properties);


This pattern is typically used for dynamically loaded implementations. Since this is part of the catalog, I think it makes sense to just pass the properties to the encryption manager constructor.

pvary · 2023-10-18T11:58:42Z

@ggershinsky: Do we want to add Flink tests alongside the Spark tests as well?

ggershinsky · 2023-10-18T12:49:58Z

The current PR sequence develops the encryption tools using Spark as a usecase. But once the toolset converges, we'll certainly need the Flink support and unitests.

ggershinsky · 2024-04-24T05:57:06Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

+      return encryptingFileIO;
+    }
+
+    encryptingFileIO = EncryptingFileIO.combine(fileIO, encryption());


maybe no need to do this if encryption() is PlaintextEncryptionManager.instance() . Then we won't have to fix the tests with Hadoop config cast.

ggershinsky · 2024-04-24T05:57:50Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

+      return encryptionManager;
+    }
+
+    if (keyManagementClient == null) {


what if tableKeyID is not null? check and throw an exception

ggershinsky · 2024-05-06T11:03:14Z

core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java

+      FileIO io = io();
+      Preconditions.checkArgument(
+          io instanceof EncryptingFileIO,
+          "Cannot encrypt table metadata because the fileIO (%s) does not "
+              + "implement EncryptingFileIO",
+          io.getClass());
+      EncryptingFileIO encryptingIO = (EncryptingFileIO) io();
+      EncryptedOutputFile newEncryptedMetadataFile =
+          encryptingIO.newEncryptingOutputFile(newTableMetadataFilePath);
+
+      if (newEncryptedMetadataFile.keyMetadata() == null
+          || newEncryptedMetadataFile.keyMetadata().buffer() == null) {
+        throw new IllegalStateException("Null key metadata in encrypted table");
+      }
+
+      newMetadataFile = newEncryptedMetadataFile.encryptingOutputFile();
+      EncryptionManager encryptionManager = encryptingIO.encryptionManager();
+
+      Preconditions.checkArgument(
+          encryptionManager instanceof StandardEncryptionManager,
+          "Cannot encrypt table metadata because the encryption manager (%s) does not "
+              + "implement StandardEncryptionManager",
+          encryptionManager.getClass());
+      NativeEncryptionKeyMetadata keyMetadata =
+          (NativeEncryptionKeyMetadata) newEncryptedMetadataFile.keyMetadata();
+      ByteBuffer metadataEncryptionKey = keyMetadata.encryptionKey();
+      // Wrap (encrypt) metadata file key
+      ByteBuffer wrappedEncryptionKey =
+          ((StandardEncryptionManager) encryptionManager).wrapKey(metadataEncryptionKey);
+
+      ByteBuffer metadataAADPrefix = keyMetadata.aadPrefix();
+      wrappedMetadataKey =
+          Base64.getEncoder()
+              .encodeToString(
+                  EncryptionUtil.createKeyMetadata(wrappedEncryptionKey, metadataAADPrefix)
+                      .buffer()
+                      .array());


move this logic into a util class (eg EncryptionUtil or TableMetadataParser), so it can be re-used by other catalogs.

ggershinsky · 2024-05-06T16:26:46Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestTableEncryption.java

+  }
+
+  private void checkMetadataFileEncryption(InputFile file) throws IOException {
+    SeekableInputStream stream = file.newStream();


RussellSpitzer · 2024-05-20T17:20:52Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

      HiveOperationsBase.validateTableIsIceberg(table, fullName);

      metadataLocation = table.getParameters().get(METADATA_LOCATION_PROP);
+      // TODO do we need to lock/unlock Hive table, to get all 3 params in one atomic operation?


We probably don't need to lock but we do want to retry if we fail because we got non-synchronized values here.

Co-Authored-By: Jian Tang <[email protected]> rename to default encr mng factory test fix for: CTAS produces unencrypted files sync with design check file format in table props use new kms interface refactor common KMS client for unitests refactor update move factory to another PR update

ggershinsky · 2024-06-17T13:40:15Z

core/src/main/java/org/apache/iceberg/TableMetadataParser.java


+      if (node.has(SNAPSHOT_KEK)) {
+        String wrappedKeyEncryptionKey = JsonUtil.getString(SNAPSHOT_KEK, node);
+        if (wrappedKeyEncryptionKey != null) {


no need in this check

hsiang-c · 2024-06-24T08:00:31Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

+    }
+
+    if (encryptionKeyId == null) {
+      encryptionPropsFromHms();


This LGTM now.

The previous approach (i.e. call the dekLength() in BaseMetastoreTableOperations) will only set encryptionDekLength on the write path (the writeNewMetadataFile method). Therefore, Hive Catalog readers get the default -1 value.

rdblue · 2024-07-12T22:21:43Z

.palantir/revapi.yml

+    - code: "java.class.defaultSerializationChanged"
+      old: "class org.apache.iceberg.encryption.EncryptingFileIO"
+      new: "class org.apache.iceberg.encryption.EncryptingFileIO"
+      justification: "Decrypting input mist receive length"


rdblue · 2024-07-12T22:24:21Z

.palantir/revapi.yml

+      justification: "Decrypting input mist receive length"
+    - code: "java.method.removed"
+      old: "method org.apache.iceberg.io.InputFile org.apache.iceberg.encryption.EncryptingFileIO::newDecryptingInputFile(java.lang.String,\
+        \ java.nio.ByteBuffer)"


I agree that we want to remove this, but we can't make changes to API like this until a new major release. Can you please deprecate the method instead and make a note that it will be removed in 2.0.0?

rdblue · 2024-07-12T22:27:25Z

api/src/main/java/org/apache/iceberg/encryption/EncryptingFileIO.java

+
+    InputFile inputFile = io.newInputFile(path, length);
+
+    if (inputFile.getLength() != length) {


This should not call inputFile.getLength because it will either return the length passed in above and is useless (newInputFile(path, length)) or it will make a call to the underlying storage (needless HEAD request). Don't we already catch cases where the file has been truncated?

Hm. I don't see a test in TestGcmStreams so we should probably add one that validates truncated streams specifically.

I don't see a test in TestGcmStreams so we should probably add one that validates truncated streams specifically.

I'm not sure how. If a file is truncated by exactly 1 block, GCM Streams won't detect that. The upper layer (Avro or Json readers, etc) might detect that or might not, it's not guaranteed. That's why we've added an explicit requirement in the GCM Stream spec to take the length from a trusted source. This is "out of band" from the spec point of view, meaning that we must make sure the length comes from a parent metadata (and not from the file system) everywhere in Iceberg where we decrypt a stream.

However, FileIO.newInputFile(path, length) implementations are custom; some of them simply ignore the length parameter - and then indeed send a file system request upon InputFile.getLength() call. But we can prevent a security breach by making this check (if (inputFile.getLength() != length)). In most cases, this won't trigger a HEAD request, because most of FileIO implementations don't ignore the length parameter in newInputFile(path, length) and store it. For those few that do ignore it, we need to verify the file length wasn't truncated in the file system.

rdblue · 2024-07-12T22:29:19Z

core/src/main/java/org/apache/iceberg/TableMetadataParser.java

      TableMetadata metadata, OutputFile outputFile, boolean overwrite) {
    boolean isGzip = Codec.fromFileName(outputFile.location()) == Codec.GZIP;
    OutputStream stream = overwrite ? outputFile.createOrOverwrite() : outputFile.create();
+


Nit: unnecessary whitespace change.

rdblue · 2024-07-13T00:02:01Z

core/src/main/java/org/apache/iceberg/TableMetadataParser.java


  public static TableMetadata read(FileIO io, InputFile file) {
+    EncryptionManager encryption =
+        (io instanceof EncryptingFileIO) ? ((EncryptingFileIO) io).encryptionManager() : null;


There should be no need for most of the changes in this file after my recommendations on #7770.

ggershinsky · 2024-08-01T06:21:57Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java

+  private String encryptionKeyId;
+  private int encryptionDekLength;
+
+  /** Tests only */


I'll remove this comment.

ggershinsky · 2024-08-15T07:06:08Z

core/src/main/java/org/apache/iceberg/TableMetadata.java

    return snapshots;
  }

+  public void setKekCache(Map<String, KeyEncryptionKey> kekCache) {


I'll change this to withKekCache, making the implementation similar to withUUID (via new Builder).

github-actions · 2024-09-19T00:14:12Z

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.

hsiang-c · 2024-09-19T03:19:05Z

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java


+    if (catalogProperties.containsKey(CatalogProperties.ENCRYPTION_KMS_IMPL)) {
+      this.keyManagementClient = EncryptionUtil.createKmsClient(properties);
+      this.writerKekTimeout =


Wanna keep this PR active

(nit) Maybe it make sense to add unit to the variable name, i.e. writerKekTimeoutMs? For example, in LockManagers, we have

this.heartbeatIntervalMs = PropertyUtil.propertyAsLong( properties, CatalogProperties.LOCK_HEARTBEAT_INTERVAL_MS, CatalogProperties.LOCK_HEARTBEAT_INTERVAL_MS_DEFAULT);

nastra · 2024-12-03T11:25:17Z

@ggershinsky is this PR still active? Could you please rebase and fix the merge conflicts? I'll review this afterwards

ggershinsky · 2024-12-03T12:31:45Z

Thanks @nastra . This PR is heavily dependent on #7770 , so can be properly rebased after the latter is merged. Both PRs are active, awaiting a progress in the 7770.

github-actions · 2025-01-16T00:14:58Z

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 · 2025-01-30T00:14:30Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

ggershinsky · 2025-05-15T12:02:12Z

replaced with #13066

github-actions bot added core spark labels Aug 16, 2022

ggershinsky mentioned this pull request Aug 16, 2022

Core: Apply encryption basics to writers and readers #2640

Closed

pvary reviewed Jan 16, 2023

View reviewed changes

ggershinsky changed the title ~~Encryption integration and test~~ DON'T MERGE Encryption integration and test Feb 7, 2023

github-actions bot added the hive label Feb 7, 2023

rdblue reviewed Feb 27, 2023

View reviewed changes

ggershinsky changed the title ~~DON'T MERGE Encryption integration and test~~ Encryption integration and test Aug 14, 2023

rdblue reviewed Sep 10, 2023

View reviewed changes

ggershinsky force-pushed the integrate-and-test branch from 6bfdf67 to 9902ef6 Compare September 20, 2023 07:15

ggershinsky mentioned this pull request Sep 21, 2023

Deliver key metadata to parquet encryption #6762

Closed

ggershinsky force-pushed the integrate-and-test branch from 5d43ebf to 2a9ca87 Compare April 21, 2024 07:01

github-actions bot added the API label Apr 21, 2024

ggershinsky commented Apr 24, 2024

View reviewed changes

ggershinsky commented May 6, 2024

View reviewed changes

RussellSpitzer reviewed May 20, 2024

View reviewed changes

ggershinsky added 9 commits May 26, 2024 09:49

address review comments

2a7c804

cleanup

bef73dc

enable null table metadata for old unitests

4bb958f

remove encryption factory

636c233

update

4547417

Metadata file encryption

c2b355c

spotlessApply

7a61a2e

clean up

a59e3f5

ggershinsky force-pushed the integrate-and-test branch from 57c1a4b to 3cbc0c2 Compare May 26, 2024 06:50

ggershinsky added 4 commits May 28, 2024 14:16

comments

9d9a522

wrap snapshot key encryption key in KMS

839b9cd

old return type in metadata write

a077b63

remove unnneeded change

e04a390

ggershinsky commented Jun 17, 2024

View reviewed changes

hsiang-c reviewed Jun 24, 2024

View reviewed changes

rdblue reviewed Jul 12, 2024

View reviewed changes

rdblue reviewed Jul 13, 2024

View reviewed changes

kek cache

3afe781

ggershinsky commented Aug 1, 2024

View reviewed changes

ggershinsky added 2 commits August 4, 2024 14:24

address review comments

453a8b9

visible for testing

bd62067

ggershinsky commented Aug 15, 2024

View reviewed changes

github-actions bot added the stale label Sep 19, 2024

hsiang-c reviewed Sep 19, 2024

View reviewed changes

github-actions bot removed the stale label Sep 21, 2024

github-actions bot added the stale label Jan 16, 2025

github-actions bot closed this Jan 30, 2025

ggershinsky mentioned this pull request Feb 10, 2025

Parquet: Support parquet modular encryption #2639

Merged

smaheshwar-pltr mentioned this pull request Jun 3, 2025

Encryption for REST catalog #13225

Open


		InputFile inputFile = io.newInputFile(path, length);

		if (inputFile.getLength() != length) {

Encryption integration and test #5544

Encryption integration and test #5544

Uh oh!

Conversation

ggershinsky commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

pvary commented Oct 18, 2023

Uh oh!

ggershinsky commented Oct 18, 2023

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!

ggershinsky Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

github-actions bot commented Sep 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nastra commented Dec 3, 2024

Uh oh!

ggershinsky commented Dec 3, 2024

Uh oh!

github-actions bot commented Jan 16, 2025

Uh oh!

github-actions bot commented Jan 30, 2025

Uh oh!

ggershinsky commented May 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ggershinsky commented Aug 16, 2022 •

edited

Loading

ggershinsky Aug 1, 2024 •

edited

Loading