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?

yes, the new member sse_customer_key tls_ca_file_path, tls_ca_dir_path and tls_verify_certificates added for 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-10-16T04:41:09Z

@kou @pitrou Can you help to review the PR again, Thanks a lot.

kou

+1

cpp/src/arrow/filesystem/s3_internal.h

cpp/src/arrow/filesystem/s3_test_util.cc

cpp/src/arrow/filesystem/s3fs.h

cpp/src/arrow/filesystem/s3fs_test.cc

kou · 2024-10-16T05:22:27Z

Could you update the PR description to reflect the current implementation?

ripplehang · 2024-10-17T01:41:56Z

@kou do you think we can merge the PR now. Suppose those requested changes from @pitrou should also be resolved.

kou · 2024-10-17T01:59:31Z

I'll wait for re-review from pitrou in this week.
I'll merge this in the next week if nobody reviews this.

…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

pitrou · 2024-10-17T14:52:33Z

cpp/src/arrow/filesystem/s3fs_test.cc

+class TestS3FSHTTP : public TestS3FS {
+ public:
+  void SetUp() override {
+    enable_tls_if_supported_ = false;


Why was this necessary? This looks like a weird workaround, can there at least be an explanation in comments?

I've temporarily disabled this workaround here: a69961b

We'll see how CI goes

It seems it can occur sporadically even in non-stress tests:
https://github.com/ursacomputing/crossbow/actions/runs/11388191999/job/31684559075#step:7:3791

[ RUN ] TestS3FS.OpenOutputStreamMetadata /arrow/cpp/src/arrow/filesystem/s3fs_test.cc:593: Failure Failed 'output->Close()' failed with IOError: When completing multiple part upload for key 'mdfile1' in bucket 'bucket': AWS Error NO_SUCH_UPLOAD during CompleteMultipartUpload operation: The specified multipart upload does not exist. The upload ID may be invalid, or the upload may have been aborted or completed. /arrow/cpp/src/arrow/filesystem/s3fs.cc:1830 CleanupIfFailed(FinishPartUploadAfterFlush()) [ FAILED ] TestS3FS.OpenOutputStreamMetadata (215 ms)

@kou Do you know if we were seeing this error before switching to HTTPS for Minio-based tests?

I think this may have to do with forcing "localhost" for TLS certificate check purposes: in some cases, a port number might be reused?

What we should perhaps do is:

switch all tests to HTTP except the SSE customer key tests

use "localhost" only for HTTPS tests, otherwise use GetListenAddress() without a host argument.

@pitrou Fine, i've done the related change in the latest commit. please help to review again.

pitrou · 2024-10-17T15:29:48Z

@github-actions crossbow submit -g cpp

github-actions · 2024-10-17T15:32:21Z

Revision: a69961b

Submitted crossbow builds: ursacomputing/crossbow @ actions-cc413ca29c

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

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

ripplehang · 2024-10-21T03:00:51Z

@pitrou is it possible to help review the change again, Thanks.

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

kou approved these changes Oct 16, 2024

View reviewed changes

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

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Oct 16, 2024

Hang Zheng and others added 13 commits October 17, 2024 16:45

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

853960c

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

c9a9597

always start the minio server with tls for ut

4345c32

switch the benchmark case also to https

add498a

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

e708a0d

…face to set the ca for windows

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

2874181

…add the SSEC related function after 1.9.201

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

5f662b7

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

refine code according to review comments

3a2b318

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

41ed275

…the host could success during the TLS handshake

force to use http for some cases

ec3b7f2

try to fix the crash on arm64 platform

c55b8f0

refine the SSE-C words in comments

33962a8

Fix CopyFile and add test (+ nits)

3fce2f5

pitrou force-pushed the support_ssec_for_aws_s3 branch from 50627c2 to 3fce2f5 Compare October 17, 2024 14:50

pitrou reviewed Oct 17, 2024

View reviewed changes

Temporarily disable workaround

a69961b

Hang Zheng added 2 commits October 17, 2024 21:44

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

0afad6f

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

set the correct address for the minio start args

c18343f

ripplehang requested a review from pitrou October 18, 2024 03:31

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 Oct 16, 2024

kou left a comment

kou commented Oct 16, 2024

ripplehang commented Oct 17, 2024 •

edited

Loading

kou commented Oct 17, 2024

pitrou Oct 17, 2024

pitrou Oct 17, 2024

pitrou Oct 17, 2024

pitrou Oct 17, 2024

ripplehang Oct 18, 2024

pitrou commented Oct 17, 2024

github-actions bot commented Oct 17, 2024

ripplehang commented Oct 21, 2024

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 Oct 16, 2024

kou left a comment

Choose a reason for hiding this comment

kou commented Oct 16, 2024

ripplehang commented Oct 17, 2024 • edited Loading

kou commented Oct 17, 2024

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

pitrou commented Oct 17, 2024

github-actions bot commented Oct 17, 2024

ripplehang commented Oct 21, 2024

ripplehang commented Aug 7, 2024 •

edited

Loading

ripplehang commented Oct 17, 2024 •

edited

Loading