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

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

Rationale for this change

server-side encryption with customer-provided keys is an important security feature for aws s3, it's useful when user want to manager the encryption key themselves, say, they don't want the data to be exposed to the aws system admin, and ensure the object is safe even the ACCESS_KEY and SECRET_KEY is somehow leaked.
Some comparison of S3 encryption options :
https://www.linkedin.com/pulse/delusion-s3-encryption-benefits-ravi-ivaturi/

What changes are included in this PR?

Add the sse_customer_key member for S3Options to support server-side encryption with customer-provided keys (SSE-C keys).
- The sse_customer_key was expected to be 256 bits (32 bytes) according to aws doc
- The sse_customer_key was expected to be the raw key rather than base64 encoded value, arrow would calculate the base64 and MD5 on the fly.
- By default the sse_customer_key is empty, and when the sse_customer_key is empty, there is no impact on the existing workflow. When the sse_customer_key is configured, it would require the aws sdk version to newer than 1.9.201.
Add the tls_ca_file_path, tls_ca_dir_path and tls_verify_certificates members for S3Options.
- the tls_ca_file_path, tls_ca_dir_path member for S3Options would override the value configured by arrow::fs::FileSystemGlobalOptions.
- for s3, according to aws sdk doc, the tls_ca_file_path and tls_ca_dir_path only take effect in Linux, in order to support connect to the the storage server like minio with self-signed certificates on non-linux platform, we expose the tls_verify_certificates.
Refine the unit test to start the minio server with self-signed certificate on linux platform, so the unit test could cover the https case on linux, and http case on non-linux platform.

Are these changes tested?

Yes

Are there any user-facing changes?

Only additional members to S3Options.

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-11-01T01:15:47Z

@kou Do you think we could merge the pr since there is no response from @pitrou for two weeks.

kou · 2024-11-03T02:36:48Z

I'll merge this in the next week if nobody objects it.

…face to set the ca for windows

…add the SSEC related function after 1.9.201

… in linux as the minio with mingw could NOT work well

…the host could success during the TLS handshake

…r http mionio server, rollback to use the random host ip

pitrou

I will merge if CI is green, thanks for the updates @ripplehang !

pitrou · 2024-11-03T16:08:55Z

@github-actions crossbow submit -g cpp

pitrou · 2024-11-03T17:10:11Z

@github-actions crossbow submit -g cpp

github-actions · 2024-11-03T17:12:53Z

Revision: ce67a27

Submitted crossbow builds: ursacomputing/crossbow @ actions-7d5b001eaa

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-20.04-cuda-11.2.2
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

conbench-apache-arrow · 2024-11-04T02:22:42Z

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 00e7c65.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them.

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

Hang Zheng and others added 17 commits November 3, 2024 16:35

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

17bd705

support to start the minio test server with https, and default use https

3c7a583

always start the minio server with tls for ut

e668e62

switch the benchmark case also to https

d51a77f

install the self-signed certificate for windows, as there is no inter…

d83f0b2

…face to set the ca for windows

according to aws/aws-sdk-cpp@7989e6f, CompleteMultipartUploadRequest …

b1fba0c

…add the SSEC related function after 1.9.201

expose the tls option on the s3options,only start the minio with tls…

8235c87

… in linux as the minio with mingw could NOT work well

refine code according to review comments

4eb9ace

using the fixed localhost as the endpoint, so that the client verify …

a27bdd1

…the host could success during the TLS handshake

force to use http for some cases

bb1a5a5

try to fix the crash on arm64 platform

c7a3c85

refine the SSE-C words in comments

7fb4e90

Fix CopyFile and add test (+ nits)

8f15323

Temporarily disable workaround

7be0aa2

only enable the https for ssec related unit test cases, meanwhile, fo…

8ae7014

…r http mionio server, rollback to use the random host ip

set the correct address for the minio start args

20bf5df

Various cleanups

452185c

pitrou force-pushed the support_ssec_for_aws_s3 branch from c18343f to 452185c Compare November 3, 2024 16:07

pitrou approved these changes Nov 3, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

Avoid mutating global state, to fix TSAN failure

ce67a27

pitrou merged commit 00e7c65 into apache:main Nov 3, 2024
38 of 39 checks passed

pitrou removed the awaiting merge Awaiting merge label Nov 3, 2024

pitrou mentioned this pull request Nov 3, 2024

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

Closed

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 by pitrou

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 Nov 1, 2024

kou commented Nov 3, 2024

pitrou left a comment

pitrou commented Nov 3, 2024

This comment was marked as outdated.

pitrou commented Nov 3, 2024

github-actions bot commented Nov 3, 2024

conbench-apache-arrow bot commented Nov 4, 2024

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

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

Conversation

ripplehang commented Aug 7, 2024 • edited by pitrou 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 Nov 1, 2024

kou commented Nov 3, 2024

pitrou left a comment

Choose a reason for hiding this comment

pitrou commented Nov 3, 2024

This comment was marked as outdated.

pitrou commented Nov 3, 2024

github-actions bot commented Nov 3, 2024

conbench-apache-arrow bot commented Nov 4, 2024

ripplehang commented Aug 7, 2024 •

edited by pitrou

Loading