[S3] Add option to specify an SSE-C customer provided key

[juliusknorr]

• Document sse_c_key to provide the 256-bit encryption key for Amazon S3 to use to encrypt or decrypt your data.
  • Requires a base64 encoded value e.g. generated with openssl rand 32 | base64
  • 'sse_c_key' => 'o9d3Q9tHcPMv6TIpH53MSXaUmY91YheZRwuIhwCFRSs=',
  • Add documentation for using SSE-C on S3 primary storage documentation#9414
• Needs more testing, especially large file upload, copy

https://docs.aws.amazon.com/AmazonS3/latest/userguide/ServerSideEncryptionCustomerKeys.html#specifying-s3-c-encryption

[PVince81]

@CarlSchwan @artonge @icewind1991 please review/test

[artonge]

Code looks good, but waiting for the documentation to properly test it :)

[juliusknorr]

We only have s3 primary storage documented in the portal so I'd extend the docs over there once merged.

However this can be setup by just adding the additional sse_c_key parameter with a 32 byte random string as a secret for testing to the config.php, e.g.:

'objectstore' =>
		array (
			'class' => 'OC\\Files\\ObjectStore\\S3',
			'arguments' =>
			array (
				'bucket' => 'mughuxiji2ooy0Oa5nie0eiX',
				'key' => 'Q3AM3UQ867SPQQA43P2F',
				'secret' => 'zuf+tfteSlswRu7BJ86wekitnifILbZam1KYY3TG',
				'hostname' => 'play.min.io',
				'port' => '9443',
				'use_ssl' => true,
				'use_path_style' => true,
				'autocreate' => true,
				'verify_bucket_exists' => true,
				'sse_c_key' => 'yu2Xieyoozoqueeree2aeLee6CohYeim',
			),
		)

[github-actions]

Possible performance regression detected

Show Output

1 queries added

≠ /remote.php/dav/files/test with 1 queries removed
  - UPDATE "oc_preferences" SET "configvalue" = :dcValue1 WHERE ("userid" = :dcValue2) AND ("appid" = :dcValue3) AND ("configkey" = :dcValue4)
≠ /remote.php/dav/files/test/test.txt with 1 queries added
  + UPDATE "oc_preferences" SET "configvalue" = :dcValue1 WHERE ("userid" = :dcValue2) AND ("appid" = :dcValue3) AND ("configkey" = :dcValue4)
= /remote.php/dav/files/test/many_files
= /remote.php/dav/files/test/new_file.txt
≠ /remote.php/dav/files/test/new_file.txt with 1 queries added
  + UPDATE "oc_preferences" SET "configvalue" = :dcValue1 WHERE ("userid" = :dcValue2) AND ("appid" = :dcValue3) AND ("configkey" = :dcValue4)

[xangelix]

would it not be better to follow the more common standard of base64 encoding the key?
also, sorry not familiar with the codebase overall, but any hope of following this up with SSE-C support to the external storage module? I've been dying for it

[juliusknorr]

The AWS SDK is taking care of base64 encoding the key when sending it over:
https://github.com/aws/aws-sdk-php/blob/15f6b434b92826a321191bf5b72820934d970580/src/S3/SSECMiddleware.php#L66

It is already possible do so something like:

'sse_c_key' => base64_decode('eXUyWGlleW9vem9xdWVlcmVlMmFlTGVlNkNvaFllaW0=')

But I agree that we can probable push people to using a wider key-space than ASCII by enforcing to use a base64 encoded one and suggesting to use openssl rand 32 | base64 to generate one.

[juliusknorr]

also, sorry not familiar with the codebase overall, but any hope of following this up with SSE-C support to the external storage module? I've been dying for it

Out of scope of this PR but might be feasible as a follow up.

[ir0nhide]

@juliushaertl thank you working on this feature.
I tested your code in a local test setup and found out, that copy operations were not working. After looking more into the details i found out, that for S3 copyObject requests, it is also required to send CopySourceSSEC*-headers.

With the following changes, i got copy operations with your SSE-C implementation working:

S3ConnectionTrait.php:

protected function getSSECParameters($copy = false): array {
	$key = $this->getSSECKey();

	if ($key === null) {
			return [];
	}

	$rawKey = base64_decode($key);
	if ($copy) {
		return [
			'CopySourceSSECustomerAlgorithm' => 'AES256',
			'CopySourceSSECustomerKey' => $rawKey,
			'CopySourceSSECustomerKeyMD5' => md5($rawKey, true)
		];
	}
	return [
		'SSECustomerAlgorithm' => 'AES256',
		'SSECustomerKey' => $rawKey,
		'SSECustomerKeyMD5' => md5($rawKey, true)
	];
}

S3ObjectTrait.php:

public function copyObject($from, $to) {
	$this->getConnection()->copy($this->getBucket(), $from, $this->getBucket(), $to, 'private', [
			'params' => $this->getSSECParameters() + $this->getSSECParameters(true)
	]);
}

[juliusknorr]

Thanks for testing and good catch with the copy command @ir0nhide

I managed to get my test setup for this up and running again, so I added the mentioned changes and rebased to latest master. Ready for review.

[juliusknorr]

@artonge I needed to do a bit of manual setup for testing this locally as minio requires to take care of ssl termination on its own.

Steps for testing with [nextcloud-docker-dev environments]

• Generate a TLS key pair with "minio" as CN
  • openssl req -nodes -new -x509 -keyout private.key -out public.crt and put it in ./data/minio/ssl

Patch for nextcloud-docker-dev

diff --git a/data/additional.config.php b/data/additional.config.php
index 20ca0e7..59e9aa0 100644
--- a/data/additional.config.php
+++ b/data/additional.config.php
@@ -22,10 +22,11 @@ if ($primary === 'minio') {
                                'secret' => 'nextcloud',
                                'hostname' => 'minio',
                                'port' => '9000',
-                               'use_ssl' => false,
+                               'use_ssl' => true,
                                'use_path_style' => true,
                                'autocreate' => true,
                                'verify_bucket_exists' => true,
+                               'sse_c_key' => 'o9d3Q9tHcPMv6TIpH53MSXaUmY91YheZRwuIhwCFRSs=',
                        ),
                )
        ];
diff --git a/docker-compose.yml b/docker-compose.yml
index 4ddd9c2..9cadde1 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -536,14 +536,20 @@ services:

   minio:
     image: minio/minio
+    ports:
+      - 9120:9000
+      - 9121:9001
     environment:
       VIRTUAL_HOST: minio${DOMAIN_SUFFIX}
       VIRTUAL_PORT: 9001
+      VIRTUAL_PROTO: https
       MINIO_ROOT_USER: nextcloud
       MINIO_ROOT_PASSWORD: nextcloud
-      MINIO_BROWSER_REDIRECT_URL: http://minio${DOMAIN_SUFFIX}
+      MINIO_BROWSER_REDIRECT_URL: https://minio:9121
+      MINIO_HTTP_TRACE: /dev/stdout
     volumes:
       - objectstorage_minio:/data
+      - ./data/minio/ssl:/root/.minio/certs
     command: server /data --console-address :9001

   elasticsearch:

Patch for server to disable ssl verification on s3 primary storage

diff --git a/lib/private/Files/ObjectStore/S3ConnectionTrait.php b/lib/private/Files/ObjectStore/S3ConnectionTrait.php
index 35c7bdd28d4..998f2c09f3c 100644
--- a/lib/private/Files/ObjectStore/S3ConnectionTrait.php
+++ b/lib/private/Files/ObjectStore/S3ConnectionTrait.php
@@ -132,7 +132,7 @@ trait S3ConnectionTrait {
                        'signature_provider' => \Aws\or_chain([self::class, 'legacySignatureProvider'], ClientResolver::_default_signature_provider()),
                        'csm' => false,
                        'use_arn_region' => false,
-                       'http' => ['verify' => $this->getCertificateBundlePath()],
+                       'http' => ['verify' => false],
                        'use_aws_shared_config_files' => false,
                ];
                if ($this->getProxy()) {
diff --git a/lib/private/Files/ObjectStore/S3ObjectTrait.php b/lib/private/Files/ObjectStore/S3ObjectTrait.php
index bd9905c5fc9..8d2a98912a2 100644
--- a/lib/private/Files/ObjectStore/S3ObjectTrait.php
+++ b/lib/private/Files/ObjectStore/S3ObjectTrait.php
@@ -79,6 +79,9 @@ trait S3ObjectTrait {
                                        'cafile' => $bundle
                                ];
                        }
+                       $opts['ssl'] = [
+                               'verify_peer'   => false
+                       ];

                        if ($this->getProxy()) {
                                $opts['http']['proxy'] = $this->getProxy();

[juliusknorr]

@artonge @icewind1991 Kind ping for another review here :)

[artonge]

Didn't re-review the code, but tested and work fine. Tried big file upload, rename, and download.