You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "sweisdb (via GitHub)" <gi...@apache.org> on 2023/04/26 22:58:25 UTC
[GitHub] [spark] sweisdb opened a new pull request, #40970: [SPARK-43290][SQL] Adds IV and AAD support to aes_encrypt/aes_decrypt
sweisdb opened a new pull request, #40970:
URL: https://github.com/apache/spark/pull/40970
### What changes were proposed in this pull request?
This change adds support for optional IV and AAD fields to `aes_encrypt` and `aes_decrypt`. This allows callers to specify their own initialization vector values for some specific use cases, and to take advantage of AES-GCM's authenticated additional data optional input.
### Why are the changes needed?
There are some use cases where callers to aes_encrypt may want to provide initialization vectors (IVs) or additional authenticated data (AAD). The most common cases will be:
1. Ensuring that ciphertext matches values that have been encrypted by external tools. In those cases, the caller will need to provide an identical IV value.
2. For AES-CBC mode, there are some cases where callers want to generate deterministic encrypted output.
3. For AES-GCM mode, providing AAD fields allows callers to bind additional data to an encrypted ciphertext so that it can only be decrypted by a caller providing the same value. This is often used to enforce some context.
### Does this PR introduce _any_ user-facing change?
Not yet. This change adds support to the underlying implementation, but does not yet update the SQL support to include the new parameters.
### How was this patch tested?
All existing unit tests still pass and new tests in `ExpressionImplUtilsSuite` exercise the new code paths:
```
build/sbt "sql/test:testOnly org.apache.spark.sql.DataFrameFunctionsSuite"
build/sbt "sql/test:testOnly org.apache.spark.sql.catalyst.expressions.ExpressionImplUtilsSuite"
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #40970: [SPARK-43290][SQL] Adds IV and AAD support to aes_encrypt/aes_decrypt
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #40970:
URL: https://github.com/apache/spark/pull/40970#issuecomment-1556111345
> I want to land this first.
ok. Let's modify PR's title and its description according to your actual changes.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #40970: [SPARK-43290][SQL] Adds IV and AAD support to aes_encrypt/aes_decrypt
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #40970:
URL: https://github.com/apache/spark/pull/40970#discussion_r1199716559
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -5627,4 +5642,4 @@
"Failed to get block <blockId>, which is not a shuffle block"
]
}
-}
+}
Review Comment:
Please, remove the unnecessary changes.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #40970: [SPARK-43290][SQL] Adds IV and AAD support to aes_encrypt/aes_decrypt
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #40970:
URL: https://github.com/apache/spark/pull/40970#issuecomment-1550789731
> This change adds support for optional IV and AAD fields to aes_encrypt and aes_decrypt
@sweisdb Looking at the constructors of the `AesEncrypt` and `AesDecrypt` expressions, they still don't support new parameters. Are you going to update the expressions, correct?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #40970: [SPARK-43290][SQL] Adds AES IV and AAD support to ExpressionImplUtils
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #40970:
URL: https://github.com/apache/spark/pull/40970#issuecomment-1557847010
+1, LGTM. Merging to master.
Thank you, @sweisdb.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sweisdb commented on a diff in pull request #40970: [WIP][SPARK-43290][SQL] Adds IV and AAD support to aes_encrypt/aes_decrypt
Posted by "sweisdb (via GitHub)" <gi...@apache.org>.
sweisdb commented on code in PR #40970:
URL: https://github.com/apache/spark/pull/40970#discussion_r1179789828
##########
sql/core/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtilsSuite.scala:
##########
@@ -0,0 +1,260 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.expressions
+
+import org.apache.spark.{SparkFunSuite, SparkRuntimeException}
+import org.apache.spark.unsafe.types.UTF8String
+
+class ExpressionImplUtilsSuite extends SparkFunSuite {
+ private val b64decoder = java.util.Base64.getDecoder
+ private val b64encoder = java.util.Base64.getEncoder
+
+ case class TestCase(
+ plaintext: String,
+ key: String,
+ base64CiphertextExpected: String,
+ mode: String,
+ padding: String = "Default",
+ ivHex: String = null,
Review Comment:
Done
##########
sql/core/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtilsSuite.scala:
##########
@@ -0,0 +1,260 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.expressions
+
+import org.apache.spark.{SparkFunSuite, SparkRuntimeException}
+import org.apache.spark.unsafe.types.UTF8String
+
+class ExpressionImplUtilsSuite extends SparkFunSuite {
+ private val b64decoder = java.util.Base64.getDecoder
+ private val b64encoder = java.util.Base64.getEncoder
+
+ case class TestCase(
+ plaintext: String,
+ key: String,
+ base64CiphertextExpected: String,
+ mode: String,
+ padding: String = "Default",
+ ivHex: String = null,
+ aad: String = null,
+ expectedErrorClass: String = null) {
+ val plaintextBytes = plaintext.getBytes("UTF-8")
+ val keyBytes = key.getBytes("UTF-8")
+ val utf8mode = UTF8String.fromString(mode)
+ val utf8Padding = UTF8String.fromString(padding)
+ val deterministic = mode.equalsIgnoreCase("ECB") || (ivHex != null)
+ val ivBytes = if (ivHex == null) null else Hex.unhex(ivHex.getBytes("UTF-8"))
+ val aadBytes = if (aad == null) null else aad.getBytes("UTF-8")
+ }
+
+ val testCases = Seq(
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop",
+ "4Hv0UKCx6nfUeAoPZo1z+w==",
+ "ECB"),
+ TestCase("Spark",
+ "abcdefghijklmnop12345678",
+ "NeTYNgA+PCQBN50DA//O2w==",
+ "ECB"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "9J3iZbIxnmaG+OIA9Amd+A==",
+ "ECB"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "+MgyzJxhusYVGWCljk7fhhl6C6oUqWmtdqoaG93KvhY=",
+ "CBC"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "y5la3muiuxN2suj6VsYXB+0XUFjtrUD0/zv5eDafsA3U",
+ "GCM"),
+ TestCase(
+ "This message is longer than a single AES block and should work fine.",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "agUfTbLT8KPsqbAmQn/YdpohvxqX5bBsfFjtxE5UwqvO6EWSUVy" +
+ "jeDA6r30XyS0ARebsBgXKSExaAVZ40NMgDLQa6/o9pieYwLT5YXI7flU=",
+ "ECB"),
+ TestCase(
+ "This message is longer than a single AES block and should work fine.",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "cxUKNdlZa/6hT6gdhp46OThPcdNONdBwJj/Ctl6z4gWVKfcA6DE" +
+ "lJg84LbkueIifjNOTloduKgidk9G9a4BDsn0NjlGLUeG8GH1moPWb/+knBC7oT/OOA06W6rJXudDo",
+ "CBC"),
+ TestCase(
+ "This message is longer than a single AES block and should work fine.",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "73B0tHM3F7bvmG7yIZB9vMKnzHyuCYjD9PzAI7NJ+kDBWtaFO22" +
+ "n2cKlkNcCzr45a4Uol+sNtQwQAV7iRhBdt6YmXoviemyXJWOZ89G279SgxabaomEIyN/HZwenxeN4",
+ "GCM")
+ )
+
+ test("AesDecrypt Only") {
+ testCases.map(decOnlyCase)
+ }
+
+ test("AesEncrypt and AesDecrypt") {
+ testCases.map(encDecCase)
+ }
+
+ val ivAadTestCases = Seq(
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "AAAAAAAAAAAAAAAAAAAAAPSd4mWyMZ5mhvjiAPQJnfg=",
+ "CBC",
+ ivHex = "00000000000000000000000000000000"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "AAAAAAAAAAAAAAAAQiYi+sRNYDAOTjdSEcYBFsAWPL1f",
+ "GCM",
+ ivHex = "000000000000000000000000"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "AAAAAAAAAAAAAAAAQiYi+sTLm7KD9UcZ2nlRdYDe/PX4",
+ "GCM",
+ ivHex = "000000000000000000000000",
+ aad = "This is an AAD mixed into the input"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "AAAAAAAAAAAAAAAAQiYi+sTLm7KD9UcZ2nlRdYDe/PX4",
+ "GCM",
+ aad = "This is an AAD mixed into the input")
+ )
+
+ test("AesDecrypt only with IVs or AADs") {
+ ivAadTestCases.map(decOnlyCase)
+ }
+
+ test("AesEncrypt and AesDecrypt with IVs or AADs") {
+ ivAadTestCases.map(encDecCase)
+ }
+
+ def decOnlyCase(t: TestCase): Unit = {
+ val expectedBytes = b64decoder.decode(t.base64CiphertextExpected)
+ val decryptedBytes = ExpressionImplUtils.aesDecrypt(
+ expectedBytes,
+ t.keyBytes,
+ t.utf8mode,
+ t.utf8Padding,
+ t.aadBytes
+ )
+ val decryptedString = new String(decryptedBytes)
+ assert(decryptedString == t.plaintext)
+ }
+
+ def encDecCase(t: TestCase): Unit = {
+ val ciphertextBytes = ExpressionImplUtils.aesEncrypt(
+ t.plaintextBytes,
+ t.keyBytes,
+ t.utf8mode,
+ t.utf8Padding,
+ t.ivBytes,
+ t.aadBytes
+ )
+ val ciphertextBase64 = b64encoder.encodeToString(ciphertextBytes)
+ val decryptedBytes = ExpressionImplUtils.aesDecrypt(
+ ciphertextBytes,
+ t.keyBytes,
+ t.utf8mode,
+ t.utf8Padding,
+ t.aadBytes
+ )
+ val decryptedString = new String(decryptedBytes)
+ assert(decryptedString == t.plaintext)
+ if (t.deterministic) {
+ assert(t.base64CiphertextExpected == ciphertextBase64)
+ }
+ }
+
+ val unsupportedErrorCases = Seq(
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "unused",
+ "ECB",
+ ivHex = "0000000000000000",
+ expectedErrorClass = "UNSUPPORTED_FEATURE.AES_MODE_IV"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "unused",
+ "ECB",
+ aad = "ECB does not support AAD mode",
+ expectedErrorClass = "UNSUPPORTED_FEATURE.AES_MODE_AAD"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "unused",
+ "CBC",
+ ivHex = "0000000000",
+ expectedErrorClass = "INVALID_PARAMETER_VALUE.AES_IV_LENGTH"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "unused",
+ "GCM",
+ ivHex = "0000000000",
+ expectedErrorClass = "INVALID_PARAMETER_VALUE.AES_IV_LENGTH"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "unused",
+ "GCM",
+ padding = "PKCS",
+ expectedErrorClass = "UNSUPPORTED_FEATURE.AES_MODE"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "unused",
+ "CBC",
+ aad = "CBC doesn't support AADs",
+ expectedErrorClass = "UNSUPPORTED_FEATURE.AES_MODE_AAD")
+ )
+
+ test("AesEncrypt unsupported errors") {
+ unsupportedErrorCases.foreach { t =>
+ val e1 = intercept[SparkRuntimeException] {
+ encDecCase(t)
+ }
+ assert(e1.isInstanceOf[SparkRuntimeException])
+ assert(e1.getErrorClass == t.expectedErrorClass)
+ }
+ }
+
+ val corruptedCiphertexts = Seq(
+ // This is truncated
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "+MgyzJxhusYVGWCljk7fhhl6C6oUqWmtdqoaG93=",
+ "CBC",
+ expectedErrorClass = "INVALID_PARAMETER_VALUE.AES_CRYPTO_ERROR"),
+ // The ciphertext is corrupted
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "y5la3muiuxN2suj6VsYXB+1XUFjtrUD0/zv5eDafsA3U",
+ "GCM",
+ expectedErrorClass = "INVALID_PARAMETER_VALUE.AES_CRYPTO_ERROR"),
+ // Valid ciphertext, wrong AAD
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "AAAAAAAAAAAAAAAAQiYi+sTLm7KD9UcZ2nlRdYDe/PX4",
+ "GCM",
+ aad = "The ciphertext is valid, but the AAD is wrong",
+ expectedErrorClass = "INVALID_PARAMETER_VALUE.AES_CRYPTO_ERROR")
+ )
+
+ test("AesEncrypt Expected Errors") {
+ corruptedCiphertexts.foreach { t =>
+ val e1 = intercept[SparkRuntimeException] {
+ decOnlyCase(t)
+ }
+ assert(e1.isInstanceOf[SparkRuntimeException])
+ assert(e1.getErrorClass == t.expectedErrorClass)
+ }
Review Comment:
Done
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sweisdb commented on pull request #40970: [SPARK-43290][SQL] Adds IV and AAD support to aes_encrypt/aes_decrypt
Posted by "sweisdb (via GitHub)" <gi...@apache.org>.
sweisdb commented on PR #40970:
URL: https://github.com/apache/spark/pull/40970#issuecomment-1551795385
@MaxGekk I am planning to doing the user-facing SQL expression changes in a followup to make each change more simple. I want to land this first.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sweisdb commented on pull request #40970: [SPARK-43290][SQL] Adds AES IV and AAD support to ExpressionImplUtilsSuite
Posted by "sweisdb (via GitHub)" <gi...@apache.org>.
sweisdb commented on PR #40970:
URL: https://github.com/apache/spark/pull/40970#issuecomment-1557535698
I updated the description to clarify that this change is just for `ExpressionImplUtilsSuite` and doesn't expose the user-facing changes in `aes_encrypt` and `aes_decrypt` yet.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sweisdb commented on pull request #40970: [SPARK-43290][SQL] Adds IV and AAD support to aes_encrypt/aes_decrypt
Posted by "sweisdb (via GitHub)" <gi...@apache.org>.
sweisdb commented on PR #40970:
URL: https://github.com/apache/spark/pull/40970#issuecomment-1553299644
There is an unrelated "sql - other tests" failing. I'm going to rebase and start everything over again.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #40970: [SPARK-43290][SQL] Adds AES IV and AAD support to ExpressionImplUtilsSuite
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #40970:
URL: https://github.com/apache/spark/pull/40970#issuecomment-1557556926
> this change is just for ExpressionImplUtilsSuite
> Adds AES IV and AAD support to ExpressionImplUtilsSuite
@sweisdb Please, replace ExpressionImplUtilsSuite by ExpressionImplUtils. *Suite is the suffix of test suites but you changed not only tests.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sweisdb commented on a diff in pull request #40970: [SPARK-43290][SQL] Adds IV and AAD support to aes_encrypt/aes_decrypt
Posted by "sweisdb (via GitHub)" <gi...@apache.org>.
sweisdb commented on code in PR #40970:
URL: https://github.com/apache/spark/pull/40970#discussion_r1200741253
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -5627,4 +5642,4 @@
"Failed to get block <blockId>, which is not a shuffle block"
]
}
-}
+}
Review Comment:
This class is generated by `SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "core/testOnly *SparkThrowableSuite -- -t \"Error classes are correctly formatted\""` to pass one of the unit tests which was checking that the output was properly formatted.
I can manually revert this to add a newline at the end of the file, but if it causes the test to fail, I'll need to go back.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sweisdb commented on pull request #40970: [SPARK-43290][SQL] Adds IV and AAD support to aes_encrypt/aes_decrypt
Posted by "sweisdb (via GitHub)" <gi...@apache.org>.
sweisdb commented on PR #40970:
URL: https://github.com/apache/spark/pull/40970#issuecomment-1546250478
I rebased on master, which moves the location of the test suite. I ended up squashing commits by mistake and force pushed a single commit.
`build/sbt "catalyst/test:testOnly org.apache.spark.sql.catalyst.expressions.ExpressionImplUtilsSuite"`
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #40970: [SPARK-43290][SQL] Adds IV and AAD support to aes_encrypt/aes_decrypt
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #40970:
URL: https://github.com/apache/spark/pull/40970#discussion_r1192959026
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1051,6 +1051,16 @@
"expects a binary value with 16, 24 or 32 bytes, but got <actualLength> bytes."
]
},
+ "AES_IV_LENGTH" : {
+ "message" : [
+ "supports 16-byte CBC IVs and 12-byte GCM IVs, but got <actualLength> bytes for <mode>."
+ ]
+ },
+ "AES_SALTED_MAGIC" : {
Review Comment:
Is it used somewhere? If not, please, remove it.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sweisdb commented on pull request #40970: [SPARK-43290][SQL] Adds IV and AAD support to aes_encrypt/aes_decrypt
Posted by "sweisdb (via GitHub)" <gi...@apache.org>.
sweisdb commented on PR #40970:
URL: https://github.com/apache/spark/pull/40970#issuecomment-1546223599
> @sweisdb Could you fix the build errors:
>
> ```
> /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtilsSuite.scala:41:67: not found: value ivHex
> [error] val deterministic: Boolean = mode.equalsIgnoreCase("ECB") || (ivHex != null)
> [error] ^
> [error]
> ```
Oops, sorry. I think I merged this in and didn't re-run tests after. I pushed a fix.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #40970: [SPARK-43290][SQL] Adds IV and AAD support to aes_encrypt/aes_decrypt
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #40970:
URL: https://github.com/apache/spark/pull/40970#issuecomment-1549417703
@sweisdb Could you fix the code style issues:
```
Checkstyle checks failed at following occurrences:
Error: src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:[56] (sizes) LineLength: Line is longer than 100 characters (found 114).
Error: src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:[117] (sizes) LineLength: Line is longer than 100 characters (found 121).
Error: src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:[118] (sizes) LineLength: Line is longer than 100 characters (found 102).
Error: src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:[121] (sizes) LineLength: Line is longer than 100 characters (found 110).
Error: src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtils.java:[122] (sizes) LineLength: Line is longer than 100 characters (found 104).
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sweisdb commented on a diff in pull request #40970: [SPARK-43290][SQL] Adds IV and AAD support to aes_encrypt/aes_decrypt
Posted by "sweisdb (via GitHub)" <gi...@apache.org>.
sweisdb commented on code in PR #40970:
URL: https://github.com/apache/spark/pull/40970#discussion_r1194059698
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1051,6 +1051,16 @@
"expects a binary value with 16, 24 or 32 bytes, but got <actualLength> bytes."
]
},
+ "AES_IV_LENGTH" : {
+ "message" : [
+ "supports 16-byte CBC IVs and 12-byte GCM IVs, but got <actualLength> bytes for <mode>."
+ ]
+ },
+ "AES_SALTED_MAGIC" : {
Review Comment:
I think this was re-added by mistake when I merged this on top of SPARK-43286. My mistake.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #40970: [WIP][SPARK-43290][SQL] Adds IV and AAD support to aes_encrypt/aes_decrypt
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #40970:
URL: https://github.com/apache/spark/pull/40970#discussion_r1179405552
##########
sql/core/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtilsSuite.scala:
##########
@@ -0,0 +1,260 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.expressions
+
+import org.apache.spark.{SparkFunSuite, SparkRuntimeException}
+import org.apache.spark.unsafe.types.UTF8String
+
+class ExpressionImplUtilsSuite extends SparkFunSuite {
+ private val b64decoder = java.util.Base64.getDecoder
+ private val b64encoder = java.util.Base64.getEncoder
+
+ case class TestCase(
+ plaintext: String,
+ key: String,
+ base64CiphertextExpected: String,
+ mode: String,
+ padding: String = "Default",
+ ivHex: String = null,
+ aad: String = null,
+ expectedErrorClass: String = null) {
+ val plaintextBytes = plaintext.getBytes("UTF-8")
+ val keyBytes = key.getBytes("UTF-8")
+ val utf8mode = UTF8String.fromString(mode)
+ val utf8Padding = UTF8String.fromString(padding)
+ val deterministic = mode.equalsIgnoreCase("ECB") || (ivHex != null)
+ val ivBytes = if (ivHex == null) null else Hex.unhex(ivHex.getBytes("UTF-8"))
+ val aadBytes = if (aad == null) null else aad.getBytes("UTF-8")
+ }
+
+ val testCases = Seq(
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop",
+ "4Hv0UKCx6nfUeAoPZo1z+w==",
+ "ECB"),
+ TestCase("Spark",
+ "abcdefghijklmnop12345678",
+ "NeTYNgA+PCQBN50DA//O2w==",
+ "ECB"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "9J3iZbIxnmaG+OIA9Amd+A==",
+ "ECB"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "+MgyzJxhusYVGWCljk7fhhl6C6oUqWmtdqoaG93KvhY=",
+ "CBC"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "y5la3muiuxN2suj6VsYXB+0XUFjtrUD0/zv5eDafsA3U",
+ "GCM"),
+ TestCase(
+ "This message is longer than a single AES block and should work fine.",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "agUfTbLT8KPsqbAmQn/YdpohvxqX5bBsfFjtxE5UwqvO6EWSUVy" +
+ "jeDA6r30XyS0ARebsBgXKSExaAVZ40NMgDLQa6/o9pieYwLT5YXI7flU=",
+ "ECB"),
+ TestCase(
+ "This message is longer than a single AES block and should work fine.",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "cxUKNdlZa/6hT6gdhp46OThPcdNONdBwJj/Ctl6z4gWVKfcA6DE" +
+ "lJg84LbkueIifjNOTloduKgidk9G9a4BDsn0NjlGLUeG8GH1moPWb/+knBC7oT/OOA06W6rJXudDo",
+ "CBC"),
+ TestCase(
+ "This message is longer than a single AES block and should work fine.",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "73B0tHM3F7bvmG7yIZB9vMKnzHyuCYjD9PzAI7NJ+kDBWtaFO22" +
+ "n2cKlkNcCzr45a4Uol+sNtQwQAV7iRhBdt6YmXoviemyXJWOZ89G279SgxabaomEIyN/HZwenxeN4",
+ "GCM")
+ )
+
+ test("AesDecrypt Only") {
+ testCases.map(decOnlyCase)
+ }
+
+ test("AesEncrypt and AesDecrypt") {
+ testCases.map(encDecCase)
+ }
+
+ val ivAadTestCases = Seq(
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "AAAAAAAAAAAAAAAAAAAAAPSd4mWyMZ5mhvjiAPQJnfg=",
+ "CBC",
+ ivHex = "00000000000000000000000000000000"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "AAAAAAAAAAAAAAAAQiYi+sRNYDAOTjdSEcYBFsAWPL1f",
+ "GCM",
+ ivHex = "000000000000000000000000"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "AAAAAAAAAAAAAAAAQiYi+sTLm7KD9UcZ2nlRdYDe/PX4",
+ "GCM",
+ ivHex = "000000000000000000000000",
+ aad = "This is an AAD mixed into the input"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "AAAAAAAAAAAAAAAAQiYi+sTLm7KD9UcZ2nlRdYDe/PX4",
+ "GCM",
+ aad = "This is an AAD mixed into the input")
+ )
+
+ test("AesDecrypt only with IVs or AADs") {
+ ivAadTestCases.map(decOnlyCase)
+ }
+
+ test("AesEncrypt and AesDecrypt with IVs or AADs") {
+ ivAadTestCases.map(encDecCase)
+ }
+
+ def decOnlyCase(t: TestCase): Unit = {
+ val expectedBytes = b64decoder.decode(t.base64CiphertextExpected)
+ val decryptedBytes = ExpressionImplUtils.aesDecrypt(
+ expectedBytes,
+ t.keyBytes,
+ t.utf8mode,
+ t.utf8Padding,
+ t.aadBytes
+ )
+ val decryptedString = new String(decryptedBytes)
+ assert(decryptedString == t.plaintext)
+ }
+
+ def encDecCase(t: TestCase): Unit = {
+ val ciphertextBytes = ExpressionImplUtils.aesEncrypt(
+ t.plaintextBytes,
+ t.keyBytes,
+ t.utf8mode,
+ t.utf8Padding,
+ t.ivBytes,
+ t.aadBytes
+ )
+ val ciphertextBase64 = b64encoder.encodeToString(ciphertextBytes)
+ val decryptedBytes = ExpressionImplUtils.aesDecrypt(
+ ciphertextBytes,
+ t.keyBytes,
+ t.utf8mode,
+ t.utf8Padding,
+ t.aadBytes
+ )
+ val decryptedString = new String(decryptedBytes)
+ assert(decryptedString == t.plaintext)
+ if (t.deterministic) {
+ assert(t.base64CiphertextExpected == ciphertextBase64)
+ }
+ }
+
+ val unsupportedErrorCases = Seq(
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "unused",
+ "ECB",
+ ivHex = "0000000000000000",
+ expectedErrorClass = "UNSUPPORTED_FEATURE.AES_MODE_IV"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "unused",
+ "ECB",
+ aad = "ECB does not support AAD mode",
+ expectedErrorClass = "UNSUPPORTED_FEATURE.AES_MODE_AAD"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "unused",
+ "CBC",
+ ivHex = "0000000000",
+ expectedErrorClass = "INVALID_PARAMETER_VALUE.AES_IV_LENGTH"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "unused",
+ "GCM",
+ ivHex = "0000000000",
+ expectedErrorClass = "INVALID_PARAMETER_VALUE.AES_IV_LENGTH"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "unused",
+ "GCM",
+ padding = "PKCS",
+ expectedErrorClass = "UNSUPPORTED_FEATURE.AES_MODE"),
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "unused",
+ "CBC",
+ aad = "CBC doesn't support AADs",
+ expectedErrorClass = "UNSUPPORTED_FEATURE.AES_MODE_AAD")
+ )
+
+ test("AesEncrypt unsupported errors") {
+ unsupportedErrorCases.foreach { t =>
+ val e1 = intercept[SparkRuntimeException] {
+ encDecCase(t)
+ }
+ assert(e1.isInstanceOf[SparkRuntimeException])
+ assert(e1.getErrorClass == t.expectedErrorClass)
+ }
+ }
+
+ val corruptedCiphertexts = Seq(
+ // This is truncated
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "+MgyzJxhusYVGWCljk7fhhl6C6oUqWmtdqoaG93=",
+ "CBC",
+ expectedErrorClass = "INVALID_PARAMETER_VALUE.AES_CRYPTO_ERROR"),
+ // The ciphertext is corrupted
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "y5la3muiuxN2suj6VsYXB+1XUFjtrUD0/zv5eDafsA3U",
+ "GCM",
+ expectedErrorClass = "INVALID_PARAMETER_VALUE.AES_CRYPTO_ERROR"),
+ // Valid ciphertext, wrong AAD
+ TestCase(
+ "Spark",
+ "abcdefghijklmnop12345678ABCDEFGH",
+ "AAAAAAAAAAAAAAAAQiYi+sTLm7KD9UcZ2nlRdYDe/PX4",
+ "GCM",
+ aad = "The ciphertext is valid, but the AAD is wrong",
+ expectedErrorClass = "INVALID_PARAMETER_VALUE.AES_CRYPTO_ERROR")
+ )
+
+ test("AesEncrypt Expected Errors") {
+ corruptedCiphertexts.foreach { t =>
+ val e1 = intercept[SparkRuntimeException] {
+ decOnlyCase(t)
+ }
+ assert(e1.isInstanceOf[SparkRuntimeException])
+ assert(e1.getErrorClass == t.expectedErrorClass)
+ }
Review Comment:
Could you use `checkError()` like:
```scala
checkError(
exception = intercept[SparkRuntimeException] {
decOnlyCase(t)
},
errorClass = t.expectedErrorClass)
```
##########
sql/core/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionImplUtilsSuite.scala:
##########
@@ -0,0 +1,260 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.expressions
+
+import org.apache.spark.{SparkFunSuite, SparkRuntimeException}
+import org.apache.spark.unsafe.types.UTF8String
+
+class ExpressionImplUtilsSuite extends SparkFunSuite {
+ private val b64decoder = java.util.Base64.getDecoder
+ private val b64encoder = java.util.Base64.getEncoder
+
+ case class TestCase(
+ plaintext: String,
+ key: String,
+ base64CiphertextExpected: String,
+ mode: String,
+ padding: String = "Default",
+ ivHex: String = null,
Review Comment:
nit: can't you follow Scala style and avoid `null`s? Like:
```scala
ivHex: Option[String] = None,
...
val ivBytes = ivHex.map(iv => Hex.unhex(iv.getBytes("UTF-8")))
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sweisdb commented on pull request #40970: [SPARK-43290][SQL] Adds AES IV and AAD support to ExpressionImplUtils
Posted by "sweisdb (via GitHub)" <gi...@apache.org>.
sweisdb commented on PR #40970:
URL: https://github.com/apache/spark/pull/40970#issuecomment-1557580752
> > this change is just for ExpressionImplUtilsSuite
> > Adds AES IV and AAD support to ExpressionImplUtilsSuite
>
> @sweisdb Please, replace ExpressionImplUtilsSuite by ExpressionImplUtils. *Suite is the suffix of test suites but you changed not only tests.
Doh. Done. Thank you.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk closed pull request #40970: [SPARK-43290][SQL] Adds AES IV and AAD support to ExpressionImplUtils
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #40970: [SPARK-43290][SQL] Adds AES IV and AAD support to ExpressionImplUtils
URL: https://github.com/apache/spark/pull/40970
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org