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 support for SSE-C encryption #1217

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

noonedeadpunk
Copy link

Changes implement 2 new flags --sse-customer-key and
--sse-copy-source-customer-key that can be used by user to
provide a key for server side encryption.

Once these options are set extra headers are added to request
accordingly to SSE-C specification [1]

This PR squashes and rebases on current master changes
implemented by @jheller

[1] https://docs.aws.amazon.com/AmazonS3/latest/userguide/specifying-s3-c-encryption.html
Closes-Bug: #352

@noonedeadpunk
Copy link
Author

This is WIP - while put and copy seems to work properly, get does not. Also patch missing tests atm. Fill fix this soon.

Changes implement 2 new flags --sse-customer-key and
--sse-copy-source-customer-key that can be used by user to
provide a key for server side encryption.

Once these options are set extra headers are added to request
accordingly to SSE-C specification [1]

We also ensure that object_get respects provided extra_headers

This PR squashes and rebases on current master changes
implemented by @jheller

[1] https://docs.aws.amazon.com/AmazonS3/latest/userguide/specifying-s3-c-encryption.html
In order to test out server-side encryption we need to use secure connection
fisrt. This way we generate self-signed certificates for minio.
@noonedeadpunk
Copy link
Author

Sorry for spamming - I thought it would be easier to test out locally, but eventually some things that were passing locally were failing in CI.

Now I believe patch is in usable shape so you're welcome to review it!

@@ -8,28 +8,30 @@

from __future__ import absolute_import, division

Copy link
Author

Choose a reason for hiding this comment

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

I've just sorted imports - can revert them back

@fviard
Copy link
Contributor

fviard commented Oct 29, 2021

Sorry for spamming - I thought it would be easier to test out locally, but eventually some things that were passing locally were failing in CI.

Now I believe patch is in usable shape so you're welcome to review it!

Don't worry, it is always appreciated that you take the time to be sure that it is all good :-)

s3cmd Outdated
cfg.sync_checks.remove("md5")
except Exception:
pass
if cfg.sync_checks.count("mtime") == 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that you can simply do:
if "mtime" not in cfg.sync_checks:

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, wasn't changing that part from fork. Changed

S3/S3.py Outdated
if self.config.sse_customer_key:
md5s = md5()
sse_customer_key = self.config.sse_customer_key.encode()
md5s.update(sse_customer_key)
Copy link
Contributor

Choose a reason for hiding this comment

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

(Same for other similar blocks)
You can give the value to hash directly to the md5 like md5(sse_customer_key).
So, I think that it will be cleaner if you pack everything in a single line and avoid temporary vars like:
b64(md5(key).digest())

Copy link
Author

Choose a reason for hiding this comment

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

Done

S3/S3.py Outdated
key_encoded = b64encode(sse_customer_key)
headers["x-amz-server-side-encryption-customer-algorithm"] = "AES256"
headers["x-amz-server-side-encryption-customer-key"] = key_encoded.decode()
headers["x-amz-server-side-encryption-customer-key-md5"] = md5_encoded.decode()
Copy link
Contributor

Choose a reason for hiding this comment

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

I put the comment here, but same for all decode() and encode():
Even if the effect will be the same, it would be better to use the encode_to_s3() and decode_from_s3() instead.

Copy link
Author

Choose a reason for hiding this comment

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

Done

mkdir -p ~/minio_tmp
mkdir -p ~/.minio/certs
cd ~/.minio/certs
~/cache/certgen -ca -host "localhost,127.0.0.1,172.17.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you add 172.17.0.1? localhost is not enough for github actions?

Copy link
Author

Choose a reason for hiding this comment

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

Nah, it's not needed. it's just one of IPs minio listens on in CI, so just added all IPs while testing. Changed.

@fviard fviard added this to the 2.3.0 milestone Oct 31, 2021
@fviard
Copy link
Contributor

fviard commented Oct 31, 2021

Thank you for your contribution.
I will need a little bit more time to review, but it looks very nice to me so far.

Your addition of "https" for the CI with certgen is a very good idea, i did not know about this tool.
Would it be possible that you extract this from this PR to put it in a separated one as it is a different subject?
Also, there is a little bit that surprise me, you don't setup the path to the certificates for Minio.
Does it use automatically certs generated with the same name from ~/.minio/certs forlder?

Also, to spare a few seconds of "cert" generation for nothing, maybe you can "cache" the generated certificates with certgen to avoid doing the expensive generation operation each time.

With this commit we reflect on the review process and improve code
and testing process.
@noonedeadpunk
Copy link
Author

@fviard Um, yes, I can extract SSL from this PR. Another good thing to extract would be fixing of extra-headers that are currently not applied for object get.

But tbh I don't know how in github I can make series of patches. Ie, for this PR to pass CI, it should be based on top of the one with SSL. I dunno how to submit such series based on top of each other in github (super easy thing to do in gerrit :D)
So to make these separate PR and all of them working and passing CI sound to me close to impossible or I just lack knowledge:( Unless you can quick merge it and I then will be able to just rebase on master...

Regarding ~/.minio/certs - that is default path and is loaded automatically. Eventually certio is developed by minio as well, so they make it super easy to use: https://docs.min.io/docs/how-to-secure-access-to-minio-server-with-tls.html

@noonedeadpunk
Copy link
Author

Also te be fair, seems like my patch is kind of duplicating #1083

@salanki
Copy link

salanki commented Sep 9, 2022

Any movement on this one?

@fviard fviard modified the milestones: 2.3.0, 2.4.0 Oct 2, 2022
@fviard fviard modified the milestones: 2.4.0, 2.5.0 Dec 6, 2023
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.

4 participants