[WIP] Add Parquet decryption support for Hive tables #23583

amoghmargoor · 2024-09-26T14:00:24Z

Description

Adds support to read Hive tables with encrypted Parquet files.
PS: This PR is work in progress and we are adding tests to it.

Additional context and related issues

Parquet added support for encryption https://parquet.apache.org/docs/file-format/data-pages/encryption/. Spark also added support to read and write tables with parquet encrypted files. In this PR we are adding support to read Hive tables with encrypted Parquet files with Trino.

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

ggershinsky · 2024-09-30T10:49:34Z

Thanks @amoghmargoor . A high-level comment first. This patch is big; I think it can be split into smaller patches that are easier to process.

the classes under crypto/keytools package handle key management. They can moved into a separate PR. This PR can focus on the basic PME layer that is given an encryption key via the reader API, and doesn't manage the key storage etc.
many classes in the crypto package are identical to the same classes in the Apache parquet-java artifact. Can we re-use them?

amoghmargoor · 2024-10-08T09:48:18Z

Thanks @amoghmargoor . A high-level comment first. This patch is big; I think it can be split into smaller patches that are easier to process.

the classes under crypto/keytools package handle key management. They can moved into a separate PR. This PR can focus on the basic PME layer that is given an encryption key via the reader API, and doesn't manage the key storage etc.

many classes in the crypto package are identical to the same classes in the Apache parquet-java artifact. Can we re-use them?

Ok I will try to split it. Regarding crypto package I think some jars were removed or decoupled as a process of removing hadoop dependency. Do we have any hadoop dependency in parquet-java ?

ggershinsky · 2024-10-08T11:28:59Z

I think the dependency on Apache Hadoop jars is "provided" in the parquet-hadoop pom, so maybe they won't be fetched during the Trino build. Most of the basic crypto package classes (unrelated to factories) don't need Apache Hadoop artifacts. Unless there are some other cross-dependencies, we might be able to re-use those classes.

electrum · 2024-10-17T18:00:28Z

lib/trino-parquet/src/main/java/io/trino/parquet/EncryptionUtils.java

+        if (trinoParquetCryptoConfig.getCryptoFactoryClass() == null) {
+            return Optional.empty();
+        }
+        final Class<?> foundClass = TrinoCryptoConfigurationUtil.getClassFromConfig(


We don’t do this kind of dynamic class loading based on class names from config. Everything should be configured statically.

electrum · 2024-10-17T18:35:41Z

The last time I looked, the Parquet crypto classes rely on Hadoop Configuration. Also, they’re mixed together in a JAR with other Hadoop-requiring classes, so we don’t include parquet-hadoop in Trino.

sopel39 · 2024-10-28T16:58:52Z

The last time I looked, the Parquet crypto classes rely on Hadoop Configuration. Also, they’re mixed together in a JAR with other Hadoop-requiring classes, so we don’t include parquet-hadoop in Trino.

@electrum all crypto classes are forked into io.trino.parquet.crypto package. There is no dependency on parquet-hadoop from trino-parquet.

sopel39 · 2024-10-28T16:56:53Z

plugin/trino-hive/pom.xml

@@ -379,6 +379,21 @@
            <scope>runtime</scope>
        </dependency>

+        <!-- Below two dependencies are not needed when parquet-kms-apple includes jackson dependency -->


we should remove this or update the comment

sopel39 · 2024-10-28T16:57:23Z

lib/trino-parquet/pom.xml

        <dependency>
            <groupId>com.google.errorprone</groupId>
            <artifactId>error_prone_annotations</artifactId>
            <optional>true</optional>
        </dependency>
-


unrelated change, undo

electrum · 2024-10-28T22:13:38Z

Sorry for the confusion. I was answering these comments:

many classes in the crypto package are identical to the same classes in the Apache parquet-java artifact. Can we re-use them?

Most of the basic crypto package classes (unrelated to factories) don't need Apache Hadoop artifacts. Unless there are some other cross-dependencies, we might be able to re-use those classes.

sopel39 · 2024-11-05T16:45:05Z

lib/trino-filesystem/pom.xml

+    <build>
+        <plugins>
+            <plugin>
+                <groupId>org.basepom.maven</groupId>


do we still need these?

sopel39 · 2024-11-05T16:45:20Z

lib/trino-orc/pom.xml

@@ -208,6 +208,16 @@
                    </excludes>
                </configuration>
            </plugin>
+            <plugin>
+                <groupId>org.basepom.maven</groupId>


remove. ORC shouldn't be affected

sopel39 · 2024-11-05T16:45:34Z

lib/trino-parquet/pom.xml

@@ -13,12 +13,25 @@
    <description>Trino - Parquet file format support</description>

    <dependencies>
+        <dependency>
+            <groupId>com.fasterxml.jackson.core</groupId>


are these required?

sopel39 · 2024-11-05T16:50:50Z

lib/trino-parquet/src/main/java/io/trino/parquet/Page.java

@@ -26,4 +28,6 @@ public int getUncompressedSize()
    {
        return uncompressedSize;
    }
+
+    public abstract Slice getSlice();


nit: this should be a separate commit

sopel39 · 2024-11-05T16:53:44Z

lib/trino-parquet/src/main/java/io/trino/parquet/EncryptionUtils.java

+import java.lang.reflect.InvocationTargetException;
+import java.util.Optional;
+
+public class EncryptionUtils


this should be final

ggershinsky · 2024-11-07T06:50:04Z

all crypto classes are forked into io.trino.parquet.crypto package. There is no dependency on parquet-hadoop from trino-parquet.

If no dependency on parquet-hadoop, then sure, it's fine to fork the crypto classes. They are quite stable in the Apache Parquet-java repo, have not been modified in many years (besides code style changes). The only differences in Trino will be related to the config, file path and file system objects, I'll review those parts.

ggershinsky · 2024-11-12T08:35:26Z

...no-parquet/src/main/java/io/trino/parquet/crypto/keytools/TrinoHadoopFSKeyMaterialStore.java

+import java.io.IOException;
+import java.util.Map;
+
+public class TrinoHadoopFSKeyMaterialStore


might want to rename it, this class doesn't use HadoopFS

ggershinsky · 2024-11-12T08:57:10Z

lib/trino-parquet/src/main/java/io/trino/parquet/ParquetReaderOptions.java

+    public boolean isUniformEncryption()
+    {
+        return parquetReaderEncryptionOptions.uniformEncryption;
+    }
+
+    public boolean isEncryptionParameterChecked()
+    {
+        return parquetReaderEncryptionOptions.encryptionParameterChecked;
+    }
+
+    public String getFailsafeEncryptionKeyId()
+    {
+        return parquetReaderEncryptionOptions.failsafeEncryptionKeyId;
+    }
+
+    public String getEncryptionColumnKeys()
+    {
+        return parquetReaderEncryptionOptions.columnKeys;
+    }
+
+    public String getEncryptionFooterKeyId()


these parameters are write-only, not required for reading.
(except for isEncryptionParameterChecked)

ggershinsky · 2024-11-12T08:59:50Z

lib/trino-parquet/src/main/java/io/trino/parquet/ParquetReaderOptions.java

+    public String[] getEncryptionVersionedKeyList()
+    {
+        return parquetReaderEncryptionOptions.versionedKeyList;
+    }
+
+    public String[] getEncryptionKeyList()
+    {
+        return parquetReaderEncryptionOptions.keyList;
+    }
+
+    public String getEncryptionKeyFile()
+    {
+        return parquetReaderEncryptionOptions.keyFile;
+    }
+
+    public boolean isEncryptionEnvironmentKeys()


these are custom parameters for one particular KMS client

ggershinsky · 2024-11-12T09:13:02Z

The only differences in Trino will be related to the config, file path and file system objects, I'll review those parts.

Had a look at these. The file path and file system parts are fine.

Regarding the config (encryption parameters) - this is related to the general challenge of supporting custom KMS systems. There is basically an infinite number of key managers out there (each public cloud has one; plus open-source KMS's; plus org private KMS systems), so ideally we'd have a dynamic loading mechanism for custom KMS client classes, and a map-like config object for supplying custom encryption parameters for these custom classes.

Otherwise, the users would have to add their custom KMS client class (and perform custom modifications of the ParquetReaderOptions.java) in their Trino fork.

github-actions · 2024-12-03T17:03:18Z

This pull request has gone a while without any activity. Tagging for triage help: @mosabua

sopel39 · 2024-12-10T09:04:23Z

I had a discussion with @electrum regarding KMS. The best would be to have one or two predefined KMS and let people contribute more to OSS or override binding if they need company specific customization

ggershinsky · 2024-12-10T12:34:05Z

I had a discussion with @electrum regarding KMS. The best would be to have one or two predefined KMS and let people contribute more to OSS or override binding if they need company specific customization

SGTM

sopel39 · 2024-12-18T14:52:48Z

Superceeded by #24517

cla-bot bot added the cla-signed label Sep 26, 2024

amoghmargoor changed the title ~~Add Parquet decryption support for Hive tables~~ [WIP] Add Parquet decryption support for Hive tables Sep 26, 2024

github-actions bot added hudi Hudi connector iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector labels Sep 26, 2024

amoghmargoor mentioned this pull request Sep 26, 2024

Trino Parquet Column Encryption #9383

Open

amoghmargoor force-pushed the master-pme-oss branch 2 times, most recently from f9d182e to 25e6fd1 Compare September 26, 2024 14:45

Add Parquet decryption support for Hive tables

cdb6cba

amoghmargoor force-pushed the master-pme-oss branch from 25e6fd1 to cdb6cba Compare September 27, 2024 08:35

electrum reviewed Oct 17, 2024

View reviewed changes

sopel39 reviewed Oct 28, 2024

View reviewed changes

sopel39 mentioned this pull request Nov 5, 2024

Support Parquet Modular Encryption for Trino #20069

Open

sopel39 reviewed Nov 6, 2024

View reviewed changes

ggershinsky reviewed Nov 12, 2024

View reviewed changes

github-actions bot added the stale label Dec 3, 2024

github-actions bot removed the stale label Dec 10, 2024

sopel39 closed this Dec 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add Parquet decryption support for Hive tables #23583

[WIP] Add Parquet decryption support for Hive tables #23583

amoghmargoor commented Sep 26, 2024 •

edited

Loading

ggershinsky commented Sep 30, 2024

amoghmargoor commented Oct 8, 2024

ggershinsky commented Oct 8, 2024

electrum Oct 17, 2024

electrum commented Oct 17, 2024 •

edited

Loading

sopel39 commented Oct 28, 2024

sopel39 Oct 28, 2024

sopel39 Oct 28, 2024

electrum commented Oct 28, 2024

sopel39 Nov 5, 2024

sopel39 Nov 5, 2024

sopel39 Nov 5, 2024

sopel39 Nov 5, 2024

sopel39 Nov 5, 2024

ggershinsky commented Nov 7, 2024

ggershinsky Nov 12, 2024

ggershinsky Nov 12, 2024 •

edited

Loading

ggershinsky Nov 12, 2024

ggershinsky commented Nov 12, 2024

github-actions bot commented Dec 3, 2024

sopel39 commented Dec 10, 2024

ggershinsky commented Dec 10, 2024

sopel39 commented Dec 18, 2024

[WIP] Add Parquet decryption support for Hive tables #23583

[WIP] Add Parquet decryption support for Hive tables #23583

Conversation

amoghmargoor commented Sep 26, 2024 • edited Loading

Description

Additional context and related issues

Release notes

ggershinsky commented Sep 30, 2024

amoghmargoor commented Oct 8, 2024

ggershinsky commented Oct 8, 2024

Choose a reason for hiding this comment

electrum commented Oct 17, 2024 • edited Loading

sopel39 commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electrum commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggershinsky commented Nov 7, 2024

Choose a reason for hiding this comment

ggershinsky Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggershinsky commented Nov 12, 2024

github-actions bot commented Dec 3, 2024

sopel39 commented Dec 10, 2024

ggershinsky commented Dec 10, 2024

sopel39 commented Dec 18, 2024

amoghmargoor commented Sep 26, 2024 •

edited

Loading

electrum commented Oct 17, 2024 •

edited

Loading

ggershinsky Nov 12, 2024 •

edited

Loading