Add S3 SSE KMS key and bucketkey encryption by tsdicloud · Pull Request #26899 · nextcloud/server

tsdicloud · 2021-05-06T15:27:10Z

This is a replacement pull request for #26001 due to diverse rebase problems on the original request.

It also contains fixes for

'verify_bucket_exists' instead of 'autocreate' in S3 config [S3] 'verify_bucket_exists' instead of 'autocreate' in S3 config #26093
fclose(): supplied resource is not a valid stream resource [S3] fclose(): supplied resource is not a valid stream resource #23373

Let´s try again. Sorry for the unconventional retry...

Signed-off-by: Bernd.Rederlechner@t-systems.com <bernd.rederlechner@t-systems.com>

juliusknorr

The current commit seems to revert the additional mimetype parameter of the writeObject method which was added in #26575 with that change adjusted, tests should also pass.

Looks fine otherwise from my side 👍

tsdicloud · 2021-05-07T08:11:53Z

I am working on the incompatibility and run unit test for it.

Signed-off-by: Bernd.Rederlechner@t-systems.com <bernd.rederlechner@t-systems.com>

tsdicloud · 2021-05-12T16:58:08Z

I cannot identify the php-cs-check problem. My dev setup does not show any fix for the corresponding files. Please help!

blizzz · 2021-05-12T22:08:01Z

I cannot identify the php-cs-check problem. My dev setup does not show any fix for the corresponding files. Please help!

That's unrelated to your code – a change sneaked in into master. If you rebase to the current, it won't appear anymore.

kesselb · 2021-05-17T15:09:25Z

+ $encrypt_state = $this->connection->getBucketEncryption([
+ 'Bucket' => $this->bucket,
+ ]);
+ return;


Suggested change
return;

kesselb · 2021-05-17T15:36:50Z

+ $logger = \OC::$server->getLogger();
+
+ try {
+ $encrypt_state = $this->connection->getBucketEncryption([


$encrypt_state is unused.

MorrisJobke · 2021-05-17T17:58:06Z

 <?php
 /**
 * @copyright Copyright (c) 2016 Robin Appelman <robin@icewind.nl>
- *


This will most likely be re-added by the license script. So better not remove it in the first place to keep the changeset clean.

MorrisJobke · 2021-05-17T17:58:34Z

 }

- $this->id = 'amazon::' . $params['bucket'];
+ $this->id = 'amazon::'.$params['bucket'];


This is most likely why the code style CI job fails. We keep spaces around operators in our code style.

MorrisJobke · 2021-05-17T17:58:46Z

 $this->uploadPartSize = !isset($params['uploadPartSize']) ? 524288000 : $params['uploadPartSize'];
 $params['region'] = empty($params['region']) ? 'eu-west-1' : $params['region'];
- $params['hostname'] = empty($params['hostname']) ? 's3.' . $params['region'] . '.amazonaws.com' : $params['hostname'];
+ $params['hostname'] = empty($params['hostname']) ? 's3.'.$params['region'].'.amazonaws.com' : $params['hostname'];


Same code style issue.

MorrisJobke · 2021-05-17T18:06:00Z

+ $params['autocreate'] = !isset($params['autocreate']) ? false : $params['autocreate'];
+
+ // this avoid at least the hash lookups for each read/weite operation
+ if (isset($params['ssekmsbucketkeyid'])) {


This should be documented in "admin_manual/configuration_files/primary_storage.rst" of the documentation.

MorrisJobke · 2021-05-17T18:06:22Z

+ // this avoid at least the hash lookups for each read/weite operation
+ if (isset($params['ssekmsbucketkeyid'])) {
+ $this->sseKmsBucketKeyId = $params['ssekmsbucketkeyid'];
+ } elseif (isset($params['ssekmskeyid'])) {


This should be documented in "admin_manual/configuration_files/primary_storage.rst" of the documentation.

MorrisJobke · 2021-05-17T18:07:30Z

+ * @throws \Exception if bucket creation fails
+ */
+	protected function createNewBucket() {
+ $logger = \OC::$server->getLogger();


I don't fully like that approach but as this is a trait I have no other good idea how to properly inject this dependency. Maybe @ChristophWurst has an idea, otherwise this should be fine.

MorrisJobke · 2021-05-17T18:07:43Z

+	protected function createNewBucket() {
+ $logger = \OC::$server->getLogger();
+ try {
+ $logger->info('Bucket "'.$this->bucket.'" does not exist - creating it.', ['app' => 'objectstore']);


Suggested change
$logger->info('Bucket "'.$this->bucket.'" does not exist - creating it.', ['app' => 'objectstore']);
$logger->info('Bucket "' . $this->bucket . '" does not exist - creating it.', ['app' => 'objectstore']);

MorrisJobke · 2021-05-17T18:07:58Z

+ try {
+ $logger->info('Bucket "'.$this->bucket.'" does not exist - creating it.', ['app' => 'objectstore']);
+ if (!$this->connection::isBucketDnsCompatible($this->bucket)) {
+ throw new \Exception('The bucket will not be created because the name is not dns compatible, please correct it: '.$this->bucket);


Suggested change
thrownew \Exception('The bucket will not be created because the name is not dns compatible, please correct it: '.$this->bucket);
thrownew \Exception('The bucket will not be created because the name is not dns compatible, please correct it: ' . $this->bucket);

MorrisJobke · 2021-05-17T18:08:18Z

+ 'level' => ILogger::DEBUG,
+ 'app' => 'objectstore',
+ ]);
+ throw new \Exception('Creation of bucket "'.$this->bucket.'" failed. '.$e->getMessage());


Suggested change
thrownew \Exception('Creation of bucket "'.$this->bucket.'" failed. '.$e->getMessage());
thrownew \Exception('Creation of bucket "' . $this->bucket . '" failed. ' . $e->getMessage());

MorrisJobke · 2021-05-17T18:08:37Z

+ return;
+ } catch (S3Exception $e) {
+ try {
+ $logger->info('Bucket key for "'.$this->bucket.'" is not set - adding it.', ['app' => 'objectstore']);


Suggested change
$logger->info('Bucket key for "'.$this->bucket.'" is not set - adding it.', ['app' => 'objectstore']);
$logger->info('Bucket key for "' . $this->bucket . '" is not set - adding it.', ['app' => 'objectstore']);

MorrisJobke · 2021-05-17T18:08:58Z

+ 'level' => ILogger::DEBUG,
+ 'app' => 'objectstore',
+ ]);
+ throw new \Exception('Putting configured bucket key to "'.$this->bucket.'" failed. '.$e->getMessage());


Suggested change
thrownew \Exception('Putting configured bucket key to "'.$this->bucket.'" failed. '.$e->getMessage());
thrownew \Exception('Putting configured bucket key to "' . $this->bucket . '" failed. ' . $e->getMessage());

MorrisJobke · 2021-05-17T18:09:09Z


 $scheme = (isset($this->params['use_ssl']) && $this->params['use_ssl'] === false) ? 'http' : 'https';
- $base_url = $scheme . '://' . $this->params['hostname'] . ':' . $this->params['port'] . '/';
+ $base_url = $scheme.'://'.$this->params['hostname'].':'.$this->params['port'].'/';


Same code style issue.

MorrisJobke · 2021-05-17T18:09:15Z

 $logger = \OC::$server->getLogger();
- $logger->debug('Bucket "' . $this->bucket . '" This bucket name is not dns compatible, it may contain invalid characters.',
-['app' => 'objectstore']);
+ $logger->debug('Bucket "'.$this->bucket.'" This bucket name is not dns compatible, it may contain invalid characters.',


Same code style issue.

MorrisJobke · 2021-05-17T18:10:04Z

 <?php
 /**
 * @copyright Copyright (c) 2017 Robin Appelman <robin@icewind.nl>
- *


Same as above

MorrisJobke

Mostly code style nitpicks. Beside that it looks good 👍

tsdicloud · 2021-05-17T18:19:51Z

Thanks for all the code hints. We will change this soon. But we found today a conceptual issue with bucket keys and default keys that may are used as a replacement. Also, we don´t need to send kms:id on read. Thus, we also need a little re-work of functionality and handling. Update will follow,.

blizzz · 2021-06-16T13:33:35Z

Moving to 23 as we're in freeze and the last beta is just ahead

juliusknorr · 2021-08-26T07:05:45Z

@tsdicloud I took over your changes from the nextmcloud repo and rebase them since the multipart upload optimization was already merged in #27877. Let's continue over there.

juliusknorr · 2021-08-27T10:02:28Z

New pull request is in #28601

Add S3 SSE KMS key and bucketked encryption
cafb437
Signed-off-by: Bernd.Rederlechner@t-systems.com <bernd.rederlechner@t-systems.com>

tsdicloud force-pushed the s3ssekms branch from 0f7a939 to cafb437Compare May 6, 2021 15:28

tsdicloud changed the title ~~Add S3 SSE KMS key and bucketked encryption~~Add S3 SSE KMS key and bucketkey encryptionMay 6, 2021

PVince81 requested review from icewind1991, juliusknorr and rullzer May 6, 2021 16:19

juliusknorr reviewed May 7, 2021
View reviewed changes

Comment threadlib/private/Files/ObjectStore/S3ObjectTrait.php Outdated

juliusknorr reviewed May 7, 2021
View reviewed changes

Comment threadlib/private/Files/ObjectStore/S3ObjectTrait.php Outdated

juliusknorr reviewed May 7, 2021
View reviewed changes

juliusknorr requested a review from MorrisJobke May 7, 2021 07:51

juliusknorr added 2. developing Work in progress enhancement feature: object storage labels May 7, 2021

juliusknorr added this to the Nextcloud 22 milestone May 7, 2021

Keep miemtype handling for writeObject
f139d2b
Signed-off-by: Bernd.Rederlechner@t-systems.com <bernd.rederlechner@t-systems.com>

tsdicloud force-pushed the s3ssekms branch from 72058c1 to f139d2bCompare May 7, 2021 08:54

Fix syntax problem
6b3f4ff
Signed-off-by: Bernd.Rederlechner@t-systems.com <bernd.rederlechner@t-systems.com>

tsdicloud force-pushed the s3ssekms branch from a4c6b42 to 6b3f4ffCompare May 7, 2021 12:48

juliusknorr reviewed May 11, 2021
View reviewed changes

Comment threadlib/private/Files/ObjectStore/S3ConnectionTrait.php

tsdicloud added 3 commits May 12, 2021 13:52

Fix decision tree for sending KMS key ref
f7981c1
Signed-off-by: Bernd.Rederlechner@t-systems.com <bernd.rederlechner@t-systems.com>

Use correct class reference in config
23c41dc
Signed-off-by: Bernd.Rederlechner@t-systems.com <bernd.rederlechner@t-systems.com>

Fix small syntax problem
86bcc3d
Signed-off-by: Bernd.Rederlechner@t-systems.com <bernd.rederlechner@t-systems.com>

juliusknorr added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 17, 2021

juliusknorr approved these changes May 17, 2021
View reviewed changes

kesselb reviewed May 17, 2021
View reviewed changes

MorrisJobke reviewed May 17, 2021
View reviewed changes

MorrisJobke requested changes May 17, 2021
View reviewed changes

This was referenced May 20, 2021
22 beta1 #26953
Merged
22.0.0 beta 2 #27111
Merged

blizzz mentioned this pull request Jun 2, 2021
22.0.0 beta 3 #27342
Merged
57 tasks

MorrisJobke mentioned this pull request Jun 10, 2021
22.0.0 Beta 4 #27458
Merged
59 tasks

blizzz mentioned this pull request Jun 16, 2021
22.0.0 beta 5 #27525
Merged
45 tasks

blizzz modified the milestones: Nextcloud 22, Nextcloud 23 Jun 16, 2021

juliusknorr mentioned this pull request Aug 26, 2021
Add S3 SSE KMS key and bucket key encryption #28601
Closed
5 tasks

juliusknorr closed this Aug 26, 2021

tsdicloud deleted the s3ssekms branch July 6, 2023 08:34

	$logger->info('Bucket "'.$this->bucket.'" does not exist - creating it.', ['app' => 'objectstore']);
	$logger->info('Bucket "' . $this->bucket . '" does not exist - creating it.', ['app' => 'objectstore']);

	thrownew \Exception('The bucket will not be created because the name is not dns compatible, please correct it: '.$this->bucket);
	thrownew \Exception('The bucket will not be created because the name is not dns compatible, please correct it: ' . $this->bucket);

	thrownew \Exception('Creation of bucket "'.$this->bucket.'" failed. '.$e->getMessage());
	thrownew \Exception('Creation of bucket "' . $this->bucket . '" failed. ' . $e->getMessage());

	$logger->info('Bucket key for "'.$this->bucket.'" is not set - adding it.', ['app' => 'objectstore']);
	$logger->info('Bucket key for "' . $this->bucket . '" is not set - adding it.', ['app' => 'objectstore']);

	thrownew \Exception('Putting configured bucket key to "'.$this->bucket.'" failed. '.$e->getMessage());
	thrownew \Exception('Putting configured bucket key to "' . $this->bucket . '" failed. ' . $e->getMessage());

Uh oh!

Uh oh!

Conversation

tsdicloud commented May 6, 2021

Uh oh!

Uh oh!

Uh oh!

juliusknorr left a comment

Choose a reason for hiding this comment

Uh oh!

tsdicloud commented May 7, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tsdicloud commented May 12, 2021

Uh oh!

blizzz commented May 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

tsdicloud commented May 17, 2021

Uh oh!

blizzz commented Jun 16, 2021

Uh oh!

juliusknorr commented Aug 26, 2021

Uh oh!

juliusknorr commented Aug 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tsdicloud commented May 7, 2021•
edited
Loading