[SPARK-37591][SQL] Support the GCM mode by `aes_encrypt()`/`aes_decrypt()` #34852

MaxGekk · 2021-12-09T17:21:05Z

What changes were proposed in this pull request?

In the PR, I propose new AES mode for the aes_encrypt()/aes_decrypt() functions - GCM (Galois/Counter Mode) w/o padding. The aes_encrypt() function returns a binary value which consists of the following fields:

An initialization vector (IV) generated per every aes_encrypt() call using java.security.SecureRandom. The length of IV is 12 bytes.
The encrypted input.
An authentication tag that can be used to verify the integrity of the data. The length of the tag is 16 bytes.

The aes_decrypt() functions assumes that its input has the fields as showed above.

For example:

spark-sql> SELECT base64(aes_encrypt('Apache Spark', '0000111122223333', 'GCM', 'NONE'));
ak3k4wV71YJAjg6d5sUTm15Ag9fyQP0UL/ComBkBxzVvft2NBOCF+g==
spark-sql> SELECT aes_decrypt(unbase64('ak3k4wV71YJAjg6d5sUTm15Ag9fyQP0UL/ComBkBxzVvft2NBOCF+g=='), '0000111122223333', 'GCM', 'NONE');
Apache Spark

Why are the changes needed?

To achieve feature parity with other systems/frameworks, and make the migration process from them to Spark SQL easier. For example, the GCM mode is supported by:

Does this PR introduce any user-facing change?

No. The AES functions haven't been released yet.

How was this patch tested?

By running new checks:

$ build/sbt "test:testOnly org.apache.spark.sql.DataFrameFunctionsSuite"
$ build/sbt "sql/test:testOnly org.apache.spark.sql.expressions.ExpressionInfoSuite"
$ build/sbt "test:testOnly org.apache.spark.sql.MiscFunctionsSuite"

SparkQA · 2021-12-09T18:49:21Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50519/

SparkQA · 2021-12-09T19:33:56Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50519/

SparkQA · 2021-12-09T22:44:23Z

Test build #146044 has finished for PR 34852 at commit 1c3e5b2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-12-10T07:04:00Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50539/

SparkQA · 2021-12-10T07:51:14Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50539/

SparkQA · 2021-12-10T10:15:46Z

Test build #146063 has finished for PR 34852 at commit 98885d0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

sarutak · 2021-12-13T01:04:29Z

retest this please.

sarutak · 2021-12-13T01:36:26Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala

  """,
  arguments = """
    Arguments:
      * expr - The binary value to encrypt.
      * key - The passphrase to use to encrypt the data.
      * mode - Specifies which block cipher mode should be used to encrypt messages.
-               Supported modes: ECB.
+               Supported modes: ECB, GCM.


nit: Should we consistently use Supported or Valid?

sarutak · 2021-12-13T01:44:17Z

sql/core/src/test/scala/org/apache/spark/sql/MiscFunctionsSuite.scala

+      assert(encrypted.filter($"enc" === $"input").isEmpty)
+      val result = encrypted.selectExpr(
+        "CAST(aes_decrypt(enc, key, 'GCM', 'NONE') AS STRING) AS res", "input")
+      assert(!result.filter($"res" === $"input").isEmpty)


To test all the records are decrypted correctly, should we do assert(result.filter($"res" !== $"input").isEmpty) ?

Other AES tests are in DataFrameFunctionsSuite, so should we move this test to the same place?

I would rather move other tests for misc functions from DataFrameFunctionsSuite to the dedicated MiscFunctionsSuite. I was thinking of moving all AES related tests here but decided to don't do unrelated changes.

DataFrameFunctionsSuite has already had > 3800 lines. Don't think it makes sense to place new misc test there if there is a special test suite for such functions.

SparkQA · 2021-12-13T01:57:54Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50587/

SparkQA · 2021-12-13T02:43:08Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50587/

cloud-fan · 2021-12-13T05:56:31Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala

      * padding - Specifies how to pad messages whose length is not a multiple of the block size.
-                  Valid values: PKCS.
+                  Valid values: PKCS, NONE.


Shall we have a smarter way to decide the default padding? e.g. if the mode is GCM, the default padding is NONE.

We can also require the mode and padding parameter to be constant, to simplify the implementation.

We can also require the mode and padding parameter to be constant, to simplify the implementation.

I wouldn't do that because it is unnecessary restriction from my point of view. I could image an use case when data gathered from different sources and encrypted slightly differently (using different padding/modes), and process in one places. What you propose requires to somehow split input data by dataframes (or selects + unions) and process them separately. Don't see any reasons to bring such pains to users.

Shall we have a smarter way to decide the default padding? e.g. if the mode is GCM, the default padding is NONE.

I think of introducing the DEFAULT value for padding which the AES implementation substitutes by concrete value (NONE or PKCS) depending on the mode.

I think of introducing the DEFAULT value for padding

Yea this also works, and we need to document the default padding for each mode.

SparkQA · 2021-12-13T05:58:25Z

Test build #146112 has finished for PR 34852 at commit 98885d0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2021-12-13T07:50:02Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameFunctionsSuite.scala

@@ -386,7 +386,7 @@ class DataFrameFunctionsSuite extends QueryTest with SharedSparkSession {
    }

    // Unsupported AES mode and padding in decrypt
-    checkUnsupportedMode(df2.selectExpr(s"aes_decrypt(value16, '$key16', 'GSM')"))
+    checkUnsupportedMode(df2.selectExpr(s"aes_decrypt(value16, '$key16', 'GSM', 'PKCS')"))


shall we test GCM?

SparkQA · 2021-12-13T09:09:39Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50604/

SparkQA · 2021-12-13T10:32:13Z

Test build #146128 has finished for PR 34852 at commit dffeec7.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-12-13T10:57:00Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50606/

SparkQA · 2021-12-13T11:37:19Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/50606/

MaxGekk · 2021-12-13T12:42:11Z

Seems Linters, licenses, dependencies and documentation generation is flaky (this PR https://github.com/monkeyboy123/spark/actions/runs/1570703624 has the same issue)

Error in loadNamespace(x) : there is no package called ‘pkgdown’
Calls: loadNamespace -> withRestarts -> withOneRestart -> doWithOneRestart
Execution halted
                    ------------------------------------------------
      Jekyll 4.2.1   Please append `--trace` to the `build` command 
                     for any additional information or backtrace. 
                    ------------------------------------------------

I am going to ignore the issue in R, and merge this PR since other GAs passed successfully.

MaxGekk · 2021-12-13T12:59:05Z

Merging to master. Thank you, @sarutak @gengliangwang and @cloud-fan for review.

SparkQA · 2021-12-13T15:10:16Z

Test build #146131 has finished for PR 34852 at commit fc29610.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Initial implementation of GCM

1c3e5b2

github-actions bot added the SQL label Dec 9, 2021

Add tests to MiscFunctionsSuite

98885d0

MaxGekk marked this pull request as ready for review December 10, 2021 06:27

MaxGekk changed the title ~~[WIP][SPARK-37591][SQL] Support the GCM mode by aes_encrypt()/aes_decrypt()~~ [SPARK-37591][SQL] Support the GCM mode by aes_encrypt()/aes_decrypt() Dec 10, 2021

MaxGekk requested review from sarutak, cloud-fan, gengliangwang and HyukjinKwon December 10, 2021 06:28

sarutak reviewed Dec 13, 2021

View reviewed changes

cloud-fan reviewed Dec 13, 2021

View reviewed changes

MaxGekk added 3 commits December 13, 2021 09:15

Supported modes -> Valid modes

624fa2d

Add a check

b84a377

Support the DEFAULT padding

3f0cc7c

cloud-fan reviewed Dec 13, 2021

View reviewed changes

cloud-fan approved these changes Dec 13, 2021

View reviewed changes

Test GCM too

dffeec7

gengliangwang approved these changes Dec 13, 2021

View reviewed changes

sarutak approved these changes Dec 13, 2021

View reviewed changes

Re-gen sql-expression-schema.md

fc29610

MaxGekk closed this in 5e4d664 Dec 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-37591][SQL] Support the GCM mode by `aes_encrypt()`/`aes_decrypt()` #34852

[SPARK-37591][SQL] Support the GCM mode by `aes_encrypt()`/`aes_decrypt()` #34852

MaxGekk commented Dec 9, 2021 •

edited

SparkQA commented Dec 9, 2021

SparkQA commented Dec 9, 2021

SparkQA commented Dec 9, 2021

SparkQA commented Dec 10, 2021

SparkQA commented Dec 10, 2021

SparkQA commented Dec 10, 2021

sarutak commented Dec 13, 2021

sarutak Dec 13, 2021

cloud-fan Dec 13, 2021

sarutak Dec 13, 2021

sarutak Dec 13, 2021

MaxGekk Dec 13, 2021

MaxGekk Dec 13, 2021

SparkQA commented Dec 13, 2021

SparkQA commented Dec 13, 2021

cloud-fan Dec 13, 2021

MaxGekk Dec 13, 2021

cloud-fan Dec 13, 2021

SparkQA commented Dec 13, 2021

cloud-fan Dec 13, 2021

SparkQA commented Dec 13, 2021

SparkQA commented Dec 13, 2021

SparkQA commented Dec 13, 2021

SparkQA commented Dec 13, 2021

MaxGekk commented Dec 13, 2021

MaxGekk commented Dec 13, 2021

SparkQA commented Dec 13, 2021

[SPARK-37591][SQL] Support the GCM mode by aes_encrypt()/aes_decrypt() #34852

[SPARK-37591][SQL] Support the GCM mode by aes_encrypt()/aes_decrypt() #34852

Conversation

MaxGekk commented Dec 9, 2021 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

SparkQA commented Dec 9, 2021

SparkQA commented Dec 9, 2021

SparkQA commented Dec 9, 2021

SparkQA commented Dec 10, 2021

SparkQA commented Dec 10, 2021

SparkQA commented Dec 10, 2021

sarutak commented Dec 13, 2021

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

Choose a reason for hiding this comment

SparkQA commented Dec 13, 2021

SparkQA commented Dec 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 13, 2021

Choose a reason for hiding this comment

SparkQA commented Dec 13, 2021

SparkQA commented Dec 13, 2021

SparkQA commented Dec 13, 2021

SparkQA commented Dec 13, 2021

MaxGekk commented Dec 13, 2021

MaxGekk commented Dec 13, 2021

SparkQA commented Dec 13, 2021

[SPARK-37591][SQL] Support the GCM mode by `aes_encrypt()`/`aes_decrypt()` #34852

[SPARK-37591][SQL] Support the GCM mode by `aes_encrypt()`/`aes_decrypt()` #34852

MaxGekk commented Dec 9, 2021 •

edited