Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add OBJECTSTORE_S3_SSE_C_KEY environment variable to use S3 SSE-C encryption (server side encryption) #2098

Closed
wants to merge 3 commits into from

Conversation

jessebot
Copy link

@jessebot jessebot commented Nov 7, 2023

Thanks to everyone who maintains this repo!

I recently learned I could pass in env vars for using S3 as the primary object store, but I was missing the encryption key.

This PR would add an environment variable called OBJECTSTORE_S3_SSE_C_KEY to the docs and if set, it will be used to provide the sse_c_key argument to the $CONFIG array in s3.config.php. If the user doesn't provide OBJECTSTORE_S3_SSE_C_KEY, we don't set ss3_c_key, because it has no default.

It's mentioned in the docs here:

Nextcloud supports server side encryption, also known as SSE-C, with compatible S3 bucket provider. The encryption and decryption happens on the S3 bucket side with a key provided by the Nextcloud server.

The key can be specified with the sse_c_key parameter which needs to be provided as a base64 encoded string with a maximum length of 32 bytes. A random key could be generated using the the following command:

openssl rand 32 | base64

…FIG array

Signed-off-by: JesseBot <jessebot@linux.com>
…README.md

Signed-off-by: JesseBot <jessebot@linux.com>
@joshtrichards
Copy link
Member

Hi @jessebot - Thanks for including the matching README update. :-)

This variable should get _FILE (Docker secrets) support too if we're going to do it. Feel up for adding it? Can be modeled after the others at the same spot in the code.

@@ -180,6 +180,7 @@ To use an external S3 compatible object store as primary storage, set the follow
- `OBJECTSTORE_S3_LEGACYAUTH` (default: `false`): Not required for AWS S3
- `OBJECTSTORE_S3_OBJECT_PREFIX` (default: `urn:oid:`): Prefix to prepend to the fileid
- `OBJECTSTORE_S3_AUTOCREATE` (default: `true`): Create the container if it does not exist
- `OBJECTSTORE_S3_SSE_C_KEY`: Server side encryption key - must be a base64 encoded string with a maximum length of 32 bytes
Copy link
Author

@jessebot jessebot Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- `OBJECTSTORE_S3_SSE_C_KEY`: Server side encryption key - must be a base64 encoded string with a maximum length of 32 bytes
- `OBJECTSTORE_S3_SSE_C_KEY`: _Optional_ Server side encryption key - must be a base64 encoded string with a maximum length of 32 bytes

We might want to include the word optional, so users know it is not required.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing wording in the README is (not set by default) - not _Optional_. Could you please add it to your PR?

@jessebot
Copy link
Author

jessebot commented Nov 8, 2023

Hi @jessebot - Thanks for including the matching README update. :-)

This variable should get _FILE (Docker secrets) support too if we're going to do it. Feel up for adding it? Can be modeled after the others at the same spot in the code.

No problem :) I can take a look the docker secrets now

Signed-off-by: jessebot <jessebot@linux.com>
@jessebot
Copy link
Author

jessebot commented Nov 8, 2023

@joshtrichards I've added the OBJECTSTORE_S3_SSE_C_KEY_FILE env var support as well. Let me know if I can help with anything else :)

@jessebot
Copy link
Author

do these jobs normally pass? @joshtrichards They look like they're failing on not having the hostname set. I took a quick peak at the github action workflows for these and couldn't find where we set any sort of env vars. Happy to update them if you can point me in the right direction.

@jessebot
Copy link
Author

please let me know if there's anything I can do to help more this forward.

Copy link
Contributor

@pathob pathob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jessebot , I'm not a maintainer here, but highly interested in this change, so I would like to help moving it forward. I've added two suggestions that might help get this merged. It would be great if you could make these two small adjustments. Thank you!

Comment on lines +26 to +31
if (getenv('OBJECTSTORE_S3_SSE_C_KEY_FILE') && file_exists(getenv('OBJECTSTORE_S3_SSE_C_KEY_FILE'))) {
$CONFIG['objectstore']['arguments']['sse_c_key'] = trim(file_get_contents(getenv('OBJECTSTORE_S3_SSE_C_KEY_FILE'));
} elseif (getenv('OBJECTSTORE_S3_SSE_C_KEY')) {
$CONFIG['objectstore']['arguments']['sse_c_key'] = getenv('OBJECTSTORE_S3_SSE_C_KEY');
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this block should go to the end of the file since OBJECTSTORE_S3_KEY_FILE and OBJECTSTORE_S3_SECRET_FILE have much more relevance than this.

@@ -180,6 +180,7 @@ To use an external S3 compatible object store as primary storage, set the follow
- `OBJECTSTORE_S3_LEGACYAUTH` (default: `false`): Not required for AWS S3
- `OBJECTSTORE_S3_OBJECT_PREFIX` (default: `urn:oid:`): Prefix to prepend to the fileid
- `OBJECTSTORE_S3_AUTOCREATE` (default: `true`): Create the container if it does not exist
- `OBJECTSTORE_S3_SSE_C_KEY`: Server side encryption key - must be a base64 encoded string with a maximum length of 32 bytes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing wording in the README is (not set by default) - not _Optional_. Could you please add it to your PR?

@@ -23,6 +23,12 @@
)
);

if (getenv('OBJECTSTORE_S3_SSE_C_KEY_FILE') && file_exists(getenv('OBJECTSTORE_S3_SSE_C_KEY_FILE'))) {
$CONFIG['objectstore']['arguments']['sse_c_key'] = trim(file_get_contents(getenv('OBJECTSTORE_S3_SSE_C_KEY_FILE'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried to run it... there is a third closing bracket ) missing at the end

@J0WI
Copy link
Contributor

J0WI commented Jan 25, 2024

closing in favour of #2151

@J0WI J0WI closed this Jan 25, 2024
@jessebot
Copy link
Author

Hi @jessebot , I'm not a maintainer here, but highly interested in this change, so I would like to help moving it forward. I've added two suggestions that might help get this merged. It would be great if you could make these two small adjustments. Thank you!

Sorry for missing this, but thank you for carrying the torch to the finish line! 🙏 Glad to see the feature implemented :)

@jessebot jessebot deleted the patch-1 branch April 30, 2024 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants