You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "lriggs (via GitHub)" <gi...@apache.org> on 2025/03/07 01:25:04 UTC
[PR] [C++][Gandiva] GH-45693 Fix aes_encrypt/decrypt algorithm selection. [arrow]
lriggs opened a new pull request, #45695:
URL: https://github.com/apache/arrow/pull/45695
* GH-45693 updated aes_encrypt/decrypt
### Rationale for this change
aes encrypt was only using a 128 bit algorithm which allowed limited cipher length and provided no warning when the cipher provided was longer than that which is supported.
### What changes are included in this PR?
Update the c++ Gandiva functions to support 128, 192, 254 bits and corresponding cipher lengths. Throw an error if its incompatible.
### Are these changes tested?
Yes, unit test and tested at Dremio.
### Are there any user-facing changes?
decryption may not work if the previously used encryption method used a truncated cipher.
Fixes GH-45693
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45693: [C++][Gandiva] Fix aes_encrypt/decrypt algorithm selection [arrow]
Posted by "lriggs (via GitHub)" <gi...@apache.org>.
lriggs commented on code in PR #45695:
URL: https://github.com/apache/arrow/pull/45695#discussion_r1992340654
##########
cpp/src/gandiva/encrypt_utils.cc:
##########
@@ -16,22 +16,24 @@
// under the License.
#include "gandiva/encrypt_utils.h"
+#include <string.h>
Review Comment:
We do now after adding to_string.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45693: [C++][Gandiva] Fix aes_encrypt/decrypt algorithm selection [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #45695:
URL: https://github.com/apache/arrow/pull/45695#issuecomment-2724145788
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 2fe4da09bf01a1ad8694dc3a989e8140bc2b596f.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/38765738039) has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45693: [C++][Gandiva] Fix aes_encrypt/decrypt algorithm selection. [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45695:
URL: https://github.com/apache/arrow/pull/45695#issuecomment-2707551734
:warning: GitHub issue #45693 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [C++][Gandiva] GH-45693 Fix aes_encrypt/decrypt algorithm selection. [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #45695:
URL: https://github.com/apache/arrow/pull/45695#issuecomment-2705303618
<!--
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.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - Contributing Overview](https://arrow.apache.org/docs/developers/overview.html)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45693: [C++][Gandiva] Fix aes_encrypt/decrypt algorithm selection [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #45695:
URL: https://github.com/apache/arrow/pull/45695#discussion_r1988161604
##########
cpp/src/gandiva/encrypt_utils.cc:
##########
@@ -16,22 +16,24 @@
// under the License.
#include "gandiva/encrypt_utils.h"
+#include <string.h>
Review Comment:
Do we really need this?
##########
cpp/src/gandiva/encrypt_utils.cc:
##########
@@ -87,4 +90,17 @@ int32_t aes_decrypt(const char* ciphertext, int32_t ciphertext_len, const char*
EVP_CIPHER_CTX_free(de_ctx);
return plaintext_len;
}
+
+const EVP_CIPHER* get_cipher_algo(int32_t key_length) {
+ switch (key_length) {
+ case 16:
+ return EVP_aes_128_ecb();
+ case 24:
+ return EVP_aes_192_ecb();
+ case 32:
+ return EVP_aes_256_ecb();
+ default:
+ throw std::runtime_error("unsupported key length");
Review Comment:
How about showing the given key length too for easy to debug?
##########
cpp/src/gandiva/gdv_function_stubs.cc:
##########
@@ -318,21 +316,32 @@ const char* gdv_fn_aes_encrypt(int64_t context, const char* data, int32_t data_l
return "";
}
+ int64_t kAesBlockSize = 0;
+ if (key_data_len == 16 || key_data_len == 24 || key_data_len == 32) {
+ kAesBlockSize = static_cast<int64_t>(key_data_len);
+ } else {
+ gdv_fn_context_set_error_msg(context, "invalid key length");
Review Comment:
How about showing the given key length for easy to debug?
##########
cpp/src/gandiva/encrypt_utils_test.cc:
##########
@@ -20,36 +20,40 @@
#include <gtest/gtest.h>
TEST(TestShaEncryptUtils, TestAesEncryptDecrypt) {
- // 8 bytes key
- auto* key = "1234abcd";
Review Comment:
We drop support for 8 bytes key, right?
(Or 8 bytes key didn't work?)
##########
cpp/src/gandiva/encrypt_utils.h:
##########
@@ -28,13 +28,15 @@ namespace gandiva {
**/
GANDIVA_EXPORT
int32_t aes_encrypt(const char* plaintext, int32_t plaintext_len, const char* key,
- unsigned char* cipher);
+ int32_t key_len, unsigned char* cipher);
/**
* Decrypt data using aes algorithm
**/
GANDIVA_EXPORT
int32_t aes_decrypt(const char* ciphertext, int32_t ciphertext_len, const char* key,
- unsigned char* plaintext);
+ int32_t key_len, unsigned char* plaintext);
+
+const EVP_CIPHER* get_cipher_algo(int32_t key_length);
Review Comment:
Can we define this in `encrypt_utils.cc` with anonymous namespace?
##########
cpp/src/gandiva/gdv_function_stubs.cc:
##########
@@ -349,24 +358,35 @@ const char* gdv_fn_aes_decrypt(int64_t context, const char* data, int32_t data_l
return "";
}
+ int64_t kAesBlockSize = 0;
+ if (key_data_len == 16 || key_data_len == 24 || key_data_len == 32) {
+ kAesBlockSize = static_cast<int64_t>(key_data_len);
+ } else {
+ gdv_fn_context_set_error_msg(context, "invalid key length");
Review Comment:
ditto.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45693: [C++][Gandiva] Fix aes_encrypt/decrypt algorithm selection [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #45695:
URL: https://github.com/apache/arrow/pull/45695#discussion_r1992510944
##########
cpp/src/gandiva/encrypt_utils.cc:
##########
@@ -87,4 +90,17 @@ int32_t aes_decrypt(const char* ciphertext, int32_t ciphertext_len, const char*
EVP_CIPHER_CTX_free(de_ctx);
return plaintext_len;
}
+
+const EVP_CIPHER* get_cipher_algo(int32_t key_length) {
+ switch (key_length) {
+ case 16:
+ return EVP_aes_128_ecb();
+ case 24:
+ return EVP_aes_192_ecb();
+ case 32:
+ return EVP_aes_256_ecb();
+ default:
+ throw std::runtime_error("unsupported key length");
Review Comment:
@lriggs Could you update this too?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45693: [C++][Gandiva] Fix aes_encrypt/decrypt algorithm selection [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #45695:
URL: https://github.com/apache/arrow/pull/45695
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45693: [C++][Gandiva] Fix aes_encrypt/decrypt algorithm selection [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #45695:
URL: https://github.com/apache/arrow/pull/45695#discussion_r1994578129
##########
cpp/src/gandiva/encrypt_utils.cc:
##########
@@ -17,21 +17,41 @@
#include "gandiva/encrypt_utils.h"
+#include <sstream>
#include <stdexcept>
+namespace {
+const EVP_CIPHER* get_cipher_algo(int32_t key_length) {
+ switch (key_length) {
+ case 16:
+ return EVP_aes_128_ecb();
+ case 24:
+ return EVP_aes_192_ecb();
+ case 32:
+ return EVP_aes_256_ecb();
+ default: {
+ std::ostringstream oss;
+ oss << "unsupported key length: " << key_length;
+ throw std::runtime_error(oss.str());
+ }
+ }
+}
+} // namespace
+
namespace gandiva {
GANDIVA_EXPORT
int32_t aes_encrypt(const char* plaintext, int32_t plaintext_len, const char* key,
- unsigned char* cipher) {
+ int32_t key_len, unsigned char* cipher) {
int32_t cipher_len = 0;
int32_t len = 0;
EVP_CIPHER_CTX* en_ctx = EVP_CIPHER_CTX_new();
+ const EVP_CIPHER* cipher_algo = get_cipher_algo(key_len);
if (!en_ctx) {
throw std::runtime_error("could not create a new evp cipher ctx for encryption");
}
- if (!EVP_EncryptInit_ex(en_ctx, EVP_aes_128_ecb(), nullptr,
+ if (!EVP_EncryptInit_ex(en_ctx, cipher_algo, nullptr,
reinterpret_cast<const unsigned char*>(key), nullptr)) {
throw std::runtime_error("could not initialize evp cipher ctx for encryption");
Review Comment:
This is not related to this PR but it seems that we need to call `EVP_CIPHER_CTX_free(en_ctx)` on error.
(We can work on this as a follow-up task. We can use RAII for 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45693: [C++][Gandiva] Fix aes_encrypt/decrypt algorithm selection [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #45695:
URL: https://github.com/apache/arrow/pull/45695#discussion_r1992510090
##########
cpp/src/gandiva/encrypt_utils_test.cc:
##########
@@ -20,36 +20,40 @@
#include <gtest/gtest.h>
TEST(TestShaEncryptUtils, TestAesEncryptDecrypt) {
- // 8 bytes key
- auto* key = "1234abcd";
Review Comment:
OK. Could you add the "This PR includes breaking changes to public APIs. (If there are any breaking changes to public APIs, please explain which changes are breaking. If not, you can remove this.)" part in the pull request template to the PR description?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45693: [C++][Gandiva] Fix aes_encrypt/decrypt algorithm selection [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #45695:
URL: https://github.com/apache/arrow/pull/45695#discussion_r1992510483
##########
cpp/src/gandiva/encrypt_utils.cc:
##########
@@ -16,22 +16,42 @@
// under the License.
#include "gandiva/encrypt_utils.h"
+#include <sstream>
Review Comment:
```suggestion
#include <sstream>
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45693: [C++][Gandiva] Fix aes_encrypt/decrypt algorithm selection. [arrow]
Posted by "lriggs (via GitHub)" <gi...@apache.org>.
lriggs commented on PR #45695:
URL: https://github.com/apache/arrow/pull/45695#issuecomment-2707686918
@github-actions autotune
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45693: [C++][Gandiva] Fix aes_encrypt/decrypt algorithm selection [arrow]
Posted by "lriggs (via GitHub)" <gi...@apache.org>.
lriggs commented on code in PR #45695:
URL: https://github.com/apache/arrow/pull/45695#discussion_r1994480748
##########
cpp/src/gandiva/encrypt_utils.cc:
##########
@@ -87,4 +90,17 @@ int32_t aes_decrypt(const char* ciphertext, int32_t ciphertext_len, const char*
EVP_CIPHER_CTX_free(de_ctx);
return plaintext_len;
}
+
+const EVP_CIPHER* get_cipher_algo(int32_t key_length) {
+ switch (key_length) {
+ case 16:
+ return EVP_aes_128_ecb();
+ case 24:
+ return EVP_aes_192_ecb();
+ case 32:
+ return EVP_aes_256_ecb();
+ default:
+ throw std::runtime_error("unsupported key length");
Review Comment:
I did.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-45693: [C++][Gandiva] Fix aes_encrypt/decrypt algorithm selection [arrow]
Posted by "lriggs (via GitHub)" <gi...@apache.org>.
lriggs commented on code in PR #45695:
URL: https://github.com/apache/arrow/pull/45695#discussion_r1992348776
##########
cpp/src/gandiva/encrypt_utils_test.cc:
##########
@@ -20,36 +20,40 @@
#include <gtest/gtest.h>
TEST(TestShaEncryptUtils, TestAesEncryptDecrypt) {
- // 8 bytes key
- auto* key = "1234abcd";
Review Comment:
Dropping support since it doesn't match the reference implementations of openssl and hive.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org