GH-43535: [C++] support the AWS S3 SSE-C encryption #43601

ripplehang · 2024-08-07T08:40:57Z

Rationale for this change

What changes are included in this PR?

Extend S3Options to support server-side encryption with customer-provided keys (SSE-C).

Are these changes tested?

Yes

Are there any user-facing changes?

yes, the new member sse_customer_key added for S3Option

GitHub Issue: [C++] Support the SSE-C(server-side encryption with customer-provided keys) for the AWS s3 filesystem read and write #43535

github-actions · 2024-08-07T08:41:26Z

⚠️ GitHub issue #43535 has been automatically assigned in GitHub to PR creator.

ripplehang · 2024-08-22T08:35:15Z

@pitrou @mapleFU @felipecrv can you help to review the PR, Thanks

pitrou · 2024-08-22T08:45:42Z

I took a quick look and here are some assorted thoughts:

there are no unit tests
only the encryption key needs to be on S3Options, not the MD5 and algorithm string (which can be computed on the fly)

pitrou · 2024-08-22T08:50:27Z

Also, for the other readers wondering, I've found a useful (though possibly outdated?) comparison of S3 encryption options here:
https://www.linkedin.com/pulse/delusion-s3-encryption-benefits-ravi-ivaturi/

ripplehang · 2024-08-23T08:32:42Z

I took a quick look and here are some assorted thoughts:

@pitrou Thanks for your review, for #1, i would try to add ut for the change, for #2, yes, actually ONLY the Key is needed, that's why i only expose one SetSSECKey function, and three Get function, and private the new added datamember.
do you suggest to only add one datamember for S3Options? but if there is only one memeber, we may needs to calculate everywhere?

ripplehang · 2024-08-23T11:10:48Z

@pitrou I've submitted another commit to only expose the ssec-key in S3Options, and then calculate the MD5 every time on the fly, can you help to review whether there are other comments? I would try to add the ut if there is no interface change required, Thanks

ripplehang · 2024-08-27T07:37:47Z

@pitrou Can you help to review the PR again?Thanks

ripplehang · 2024-08-30T09:06:02Z

@kou @mapleFU can you help to review the change, Thanks

mapleFU

I'll take a careful around this weekend

mapleFU · 2024-08-30T09:23:50Z

cpp/src/arrow/filesystem/util_internal.h

+/// \param expect_input_key_size, default 32
+/// \return true if the decode and calculate MD5 success, otherwise return false
+ARROW_EXPORT
+bool CalculateSSECKeyMD5(const std::string& base64_encoded_key, std::string& md5_result,


Would you mind change to Result<std::string>?

ok, I would try to refine the return value for the function.

mapleFU · 2024-08-30T09:24:43Z

cpp/src/arrow/filesystem/util_internal.cc

+  // Decode the Base64-encoded key to get the raw binary key
+  Aws::Utils::ByteBuffer rawKey =
+      Aws::Utils::HashingUtils::Base64Decode(base64_encoded_key);


how do this handles error if base64_encoded_key is not valid?

From https://github.com/aws/aws-sdk-cpp/blob/ac2da09e6930e3988d1289717e2df5d4b7408f17/src/aws-cpp-sdk-core/source/utils/base64/Base64.cpp#L91-L121, seems this aws util API didn't validate the input properly,but directly calculate the output size and allocate the buffer, so i add the check to ensure every character is the valid character here.
Meanwhile, I also add the related Unit test for the CalculateSSECKeyMD5 to test the different input value.

mapleFU · 2024-08-30T09:25:06Z

cpp/src/arrow/filesystem/util_internal.cc

+
+  // the key needs to be // 256 bits(32 bytes)according to
+  // https://docs.aws.amazon.com/AmazonS3/latest/userguide/ServerSideEncryptionCustomerKeys.html#specifying-s3-c-encryption
+  if (rawKey.GetLength() != expect_input_key_size) {


When would this not equal to 256?

the expect_input_key_size is exposed via S3Options, so it's possible for this value to be set as any length.

kou

Could you fix style by pre-commit run --show-diff-on-failure --color=always --all?

kou · 2024-08-31T09:20:21Z

cpp/src/arrow/filesystem/filesystem_test.cc

+  ASSERT_FALSE(CalculateSSECKeyMD5("").ok());  // invalid base64
+  ASSERT_FALSE(CalculateSSECKeyMD5("%^H").ok());  // invalid base64
+  ASSERT_FALSE(CalculateSSECKeyMD5("INVALID").ok());  // invalid base64
+  ASSERT_FALSE(CalculateSSECKeyMD5("MTIzNDU2Nzg5").ok());  // invalid, the input key size not match


Could you use ASSERT_RAISES_WITH_MESSAGES() instead?

cpp/src/arrow/filesystem/filesystem_test.cc

cpp/src/arrow/filesystem/s3fs.cc

kou · 2024-08-31T09:28:37Z

cpp/src/arrow/filesystem/s3fs_test.cc

@@ -443,6 +443,7 @@ class TestS3FS : public S3TestMixin {
    // Most tests will create buckets
    options_.allow_bucket_creation = true;
    options_.allow_bucket_deletion = true;
+    options_.sse_customer_key = "1WH9aTJ0+Tn0NLbTMHZn9aCW3Li3ViAdBsoIldPCREw=";


Why is this needed?

I tend to cover the workflow with sse_customer_key nonempty.

If we always set sse_custom_key, we can't test no sse_custom_key case.
Can we add a test that uses sse_custom_key instead?

Indeed, and ideally the SSE-C encryption test would write a file with the encryption key, and check that trying to read it with another key (or with no key at all) fails.

cpp/src/arrow/filesystem/util_internal.cc

ripplehang · 2024-09-20T09:08:56Z

@kou Can you help to review and help to trigger the CI again? Meanwhile, I've rebased the main branch,thanks

ripplehang · 2024-09-20T10:07:01Z

Could you rebase on main?

Could you check S3 filesystem related CI failures?

@kou The reason for the failure is, the minio test server is start with http, while the SSEC-Key would require https.
do you know why the minio server is started with http rather than https? is there any concern for the self-signed certificate?
I tend to start the minio server with self-signed certificate for the new test case only, and keep the current http case unchanged, do you think it's acceptable?Thanks

pitrou · 2024-09-20T11:07:42Z

I think it's ok to switch the tests to access Minio using HTTPS with a self-signed certificate.

pitrou · 2024-09-20T11:08:03Z

do you know why the minio server is started with http rather than https?

Just because it was easier :-) No particular reason otherwise.

kou · 2024-09-20T20:21:03Z

cpp/src/arrow/filesystem/s3fs_test.cc

+  sse_customer_key[31] = '\xFA';  // non-ASCII
+  std::string sse_customer_key_string(sse_customer_key, sizeof(sse_customer_key));
+  ASSERT_OK_AND_ASSIGN(auto md5, CalculateSSECustomerKeyMD5(sse_customer_key_string))
+  ASSERT_STREQ(md5.c_str(), "97FTa6lj0hE7lshKdBy61g==");  // valid case


Suggested change

ASSERT_STREQ(md5.c_str(), "97FTa6lj0hE7lshKdBy61g=="); // valid case

ASSERT_EQ(md5, "97FTa6lj0hE7lshKdBy61g=="); // valid case

kou · 2024-09-20T20:28:30Z

We can always use HTTPS Minio.
Can we prepare a self-signed certificate with portable way? (Do we need openssl command for it?)

kou · 2024-09-20T20:35:28Z

FYI: I created https://github.com/apache/arrow-flight-sql-postgresql/blob/main/dev/prepare-tls.sh for other Arrow related project but this is not for Windows...

pitrou · 2024-09-20T21:06:36Z

There is no need to generate a self-signed cert everytime, it can just be stored in the repository.

kou · 2024-09-21T05:32:40Z

In general, it's not a good idea.
If we add it to this repository, someone may report it a security vulnerability. We need to explain that it's not danger.
FYI: GH-6399

(If we don't have another approach, we can use the approach.)

pitrou · 2024-09-21T09:02:50Z

If someone is silly enough to report a self-signed certificate used for testing purposes as a security vulnerability, then they deserve to be ignored.

CPython has stored a ton of fake certificates in its repository for decades and we don't get any complaints:
https://github.com/python/cpython/tree/main/Lib/test/certdata

github-actions bot added Component: C++ awaiting review Awaiting review labels Aug 7, 2024

ripplehang force-pushed the support_ssec_for_aws_s3 branch from 7e1f200 to 1134b07 Compare August 9, 2024 04:11

ripplehang force-pushed the support_ssec_for_aws_s3 branch 3 times, most recently from bb2e024 to 539fa7c Compare August 27, 2024 06:00

mapleFU reviewed Aug 30, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review awaiting review Awaiting review and removed awaiting review Awaiting review awaiting committer review Awaiting committer review labels Aug 30, 2024

ripplehang requested a review from mapleFU August 30, 2024 11:28

ripplehang force-pushed the support_ssec_for_aws_s3 branch 3 times, most recently from de05328 to 8649388 Compare August 30, 2024 12:24

kou changed the title ~~GH-43535:[C++] support the AWS S3 SSE-C encryption~~ GH-43535: [C++] support the AWS S3 SSE-C encryption Aug 31, 2024

kou reviewed Aug 31, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting review Awaiting review and removed awaiting review Awaiting review labels Aug 31, 2024

github-actions bot added Component: C# Component: Ruby Component: GLib Component: Documentation labels Sep 20, 2024

ripplehang force-pushed the support_ssec_for_aws_s3 branch from d5db532 to e2f6ec1 Compare September 20, 2024 08:09

github-actions bot removed Component: R Component: Java Component: Parquet Component: Python Component: C# Component: Ruby Component: GLib Component: Documentation labels Sep 20, 2024

ripplehang force-pushed the support_ssec_for_aws_s3 branch from e2f6ec1 to f019bc5 Compare September 20, 2024 08:15

apacheGH-43535: [C++] support the AWS S3 SSE-C encryption

954176b

ripplehang force-pushed the support_ssec_for_aws_s3 branch from f019bc5 to 954176b Compare September 20, 2024 08:31

lidavidm removed their request for review September 20, 2024 08:41

ripplehang requested a review from kou September 20, 2024 09:07

kou reviewed Sep 20, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-43535: [C++] support the AWS S3 SSE-C encryption #43601

GH-43535: [C++] support the AWS S3 SSE-C encryption #43601

ripplehang commented Aug 7, 2024 •

edited

Loading

github-actions bot commented Aug 7, 2024

ripplehang commented Aug 22, 2024

pitrou commented Aug 22, 2024

pitrou commented Aug 22, 2024

ripplehang commented Aug 23, 2024

ripplehang commented Aug 23, 2024

ripplehang commented Aug 27, 2024

ripplehang commented Aug 30, 2024

mapleFU left a comment

mapleFU Aug 30, 2024

ripplehang Aug 30, 2024

mapleFU Aug 30, 2024

ripplehang Aug 30, 2024

mapleFU Aug 30, 2024

ripplehang Aug 30, 2024

kou left a comment

kou Aug 31, 2024

kou Aug 31, 2024

ripplehang Sep 3, 2024

kou Sep 10, 2024

pitrou Sep 10, 2024

ripplehang commented Sep 20, 2024

ripplehang commented Sep 20, 2024

pitrou commented Sep 20, 2024

pitrou commented Sep 20, 2024

kou Sep 20, 2024

kou commented Sep 20, 2024

kou commented Sep 20, 2024

pitrou commented Sep 20, 2024

kou commented Sep 21, 2024

pitrou commented Sep 21, 2024

	ASSERT_STREQ(md5.c_str(), "97FTa6lj0hE7lshKdBy61g=="); // valid case
	ASSERT_EQ(md5, "97FTa6lj0hE7lshKdBy61g=="); // valid case

GH-43535: [C++] support the AWS S3 SSE-C encryption #43601

Are you sure you want to change the base?

GH-43535: [C++] support the AWS S3 SSE-C encryption #43601

Conversation

ripplehang commented Aug 7, 2024 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Aug 7, 2024

ripplehang commented Aug 22, 2024

pitrou commented Aug 22, 2024

pitrou commented Aug 22, 2024

ripplehang commented Aug 23, 2024

ripplehang commented Aug 23, 2024

ripplehang commented Aug 27, 2024

ripplehang commented Aug 30, 2024

mapleFU left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kou left a 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

Choose a reason for hiding this comment

ripplehang commented Sep 20, 2024

ripplehang commented Sep 20, 2024

pitrou commented Sep 20, 2024

pitrou commented Sep 20, 2024

Choose a reason for hiding this comment

kou commented Sep 20, 2024

kou commented Sep 20, 2024

pitrou commented Sep 20, 2024

kou commented Sep 21, 2024

pitrou commented Sep 21, 2024

ripplehang commented Aug 7, 2024 •

edited

Loading