wjones127 commented on code in PR #6230:
URL: https://github.com/apache/arrow-rs/pull/6230#discussion_r1718611372


##########
object_store/src/aws/builder.rs:
##########
@@ -813,6 +827,16 @@ impl AmazonS3Builder {
         self
     }
 
+    /// Use SSE-C for server side encryption.
+    pub fn with_ssec_encryption(mut self, customer_key_base64: impl 
Into<String>) -> Self {

Review Comment:
   I'm confused, should this parameter already be base64 encoded? I ask because 
internally it looks like you are encoding it?
   ```suggestion
       pub fn with_ssec_encryption(mut self, customer_key: impl Into<String>) 
-> Self {
   ```



##########
object_store/src/aws/builder.rs:
##########
@@ -813,6 +827,16 @@ impl AmazonS3Builder {
         self
     }
 
+    /// Use SSE-C for server side encryption.
+    pub fn with_ssec_encryption(mut self, customer_key_base64: impl 
Into<String>) -> Self {
+        self.encryption_type = 
Some(ConfigValue::Parsed(S3EncryptionType::SseC));
+        if let Some(customer_key_64) = customer_key_base64.into().into() {
+            self.encryption_customer_key_base64 =
+                Some(BASE64_STANDARD.encode(customer_key_64.clone()));
+        }

Review Comment:
   Why is this conditional? Seems like you could just do this:
   ```suggestion
           self.encryption_customer_key_base64 =
               Some(BASE64_STANDARD.encode(customer_key.into()));
   ```



##########
object_store/CONTRIBUTING.md:
##########
@@ -101,6 +101,52 @@ export AWS_SERVER_SIDE_ENCRYPTION=aws:kms:dsse
 cargo test --features aws
 ```
 
+#### SSE-C Encryption tests
+
+Unfortunately, localstack does not support SSE-C encryption 
(https://github.com/localstack/localstack/issues/11356).
+
+We will use 
[MinIO](https://min.io/docs/minio/container/operations/server-side-encryption.html)
 to test SSE-C encryption.
+
+First, create a self-signed certificate to enable HTTPS for MinIO, as SSE-C 
requires HTTPS.
+
+```shell
+mkdir ~/certs
+cd ~/certs
+openssl genpkey -algorithm RSA -out private.key
+openssl req -new -key private.key -out request.csr -subj 
"/C=US/ST=State/L=City/O=Organization/OU=Unit/CN=example.com/emailAddress=em...@example.com"
+openssl x509 -req -days 365 -in request.csr -signkey private.key -out 
public.crt
+rm request.csr
+```
+
+Second, start MinIO with the self-signed certificate.
+
+```shell
+docker run -d \
+  -p 9000:9000 \
+  --name minio \
+  -v ${HOME}/certs:/root/.minio/certs \
+  -e "MINIO_ROOT_USER=minio" \
+  -e "MINIO_ROOT_PASSWORD=minio123" \
+  minio/minio server /data
+```
+
+Create a test bucket.
+
+```shell
+export AWS_ACCESS_KEY_ID=minio
+export AWS_SECRET_ACCESS_KEY=minio123

Review Comment:
   ```suggestion
   export AWS_BUCKET_NAME=test-bucket
   export AWS_ACCESS_KEY_ID=minio
   export AWS_SECRET_ACCESS_KEY=minio123
   export AWS_ENDPOINT=https://localhost:9000
   ```



##########
object_store/src/aws/builder.rs:
##########
@@ -813,6 +827,16 @@ impl AmazonS3Builder {
         self
     }
 
+    /// Use SSE-C for server side encryption.

Review Comment:
   Should probably say whether it should already be base64 encoded.



##########
object_store/src/aws/mod.rs:
##########
@@ -605,4 +606,71 @@ mod tests {
             store.delete(location).await.unwrap();
         }
     }
+
+    /// See CONTRIBUTING.md for the MinIO setup for this test.
+    #[tokio::test]
+    async fn test_s3_ssec_encryption_with_minio() {
+        if std::env::var("TEST_S3_SSEC_ENCRYPTION").is_err() {
+            eprintln!("Skipping S3 SSE-C encryption test");
+            return;
+        }
+        eprintln!("Running S3 SSE-C encryption test");
+
+        let customer_key = "1234567890abcdef1234567890abcdef";
+        let expected_md5 = "JMwgiexXqwuPqIPjYFmIZQ==";
+
+        let store = AmazonS3Builder::new()
+            .with_bucket_name("test-bucket")
+            .with_access_key_id("minio")
+            .with_secret_access_key("minio123")
+            .with_ssec_encryption(customer_key)
+            .with_endpoint("https://localhost:9000";)

Review Comment:
   Instead of hard coding these values in the binary, could we take them from 
environment variables? That way we can just change the environment variables to 
point them at an AWS S3 bucket to re-use the test.
   ```suggestion
           let store = AmazonS3Builder::from_env()
               .with_ssec_encryption(customer_key)
   ```



-- 
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-unsubscr...@arrow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to