Add support for SSE-C encryption #1217
Conversation
noonedeadpunk
force-pushed
the
sse-c
branch
9 times, most recently
from
2f4aefe
to
f41d74f
Compare
|
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
noonedeadpunk
force-pushed
the
sse-c
branch
12 times, most recently
from
fd536f7
to
08cf139
Compare
In order to test out server-side encryption we need to use secure connection fisrt. This way we generate self-signed certificates for minio.
|
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 | |||
|
|
|||
There was a problem hiding this comment.
I've just sorted imports - can revert them back
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: |
There was a problem hiding this comment.
I think that you can simply do:
if "mtime" not in cfg.sync_checks:
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
(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())
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() |
There was a problem hiding this comment.
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.
.github/workflows/test.yml
Outdated
| mkdir -p ~/minio_tmp | ||
| mkdir -p ~/.minio/certs | ||
| cd ~/.minio/certs | ||
| ~/cache/certgen -ca -host "localhost,127.0.0.1,172.17.0.1" |
There was a problem hiding this comment.
Why do you add 172.17.0.1? localhost is not enough for github actions?
There was a problem hiding this comment.
Nah, it's not needed. it's just one of IPs minio listens on in CI, so just added all IPs while testing. Changed.
|
Thank you for your contribution. Your addition of "https" for the CI with certgen is a very good idea, i did not know about this tool. 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.
|
@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) 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 |
|
Also te be fair, seems like my patch is kind of duplicating #1083 |
|
Any movement on this one? |
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