PowerDNS / simpleblob

MIT License
4 stars 5 forks source link

Add SSE-C support #52

Open udf2457 opened 1 year ago

udf2457 commented 1 year ago

Ref. https://github.com/PowerDNS/lightningstream/issues/63

Similar to Content-MD5 this looks like one where where the MinIO lib does the heavy lifting (minio.GetObjectOptions -> opts.ServerSideEncryption, and similar for PutObjectOptions).

Edit: This is now ready to go, see PR #58

wojas commented 1 year ago

I think having some kind of way to use "server-side encryption with customer-provided keys" (SSE-C) would be great, but we need to get the implementation right.

When adding encryption, there are several considerations to keep in mind:

  1. Using a static key (like in #58) allows the server or attacker to decrypt any file after only intercepting a single request.
  2. Therefore every file needs to use a unique key, which can be derived from a secret password/key and known unique salt (e.g. the filename).
  3. What happens when the user changes the key in the configuration? Is there a way to detect this instead of returning garbage for old files?
  4. What happens when a user later enables encryption on a bucket that already contains unencrypted files?
  5. Is there a way to performs key rollovers? This is typically the second question a customer asks us.

MinIO provides an example of how to use SSE-C with PBKDF: https://github.com/minio/minio-go/blob/master/examples/s3/put-encrypted-object.go

Is there also a way to address key changes/rollover?

udf2457 commented 1 year ago

@wojas Thanks for your reply, I am aware of most of the issues raised.

However as a quick initial"TL;DR", my thinking here was that the "threat model" in terms of PowerDNS backed simpleblob was more against "curious minds" than anything more sinsiter, which is why I ultimately proposed a simple static key.

But I hear what you are saying and I will provide a different version. I do have some ideas.

wojas commented 1 year ago

It would be nice to add a use of SSE-C to the backend, but it should then at least be a decent implementation that provides actual security. A false sense of security can be worse than no security. I would not want someone to put sensitive data in a public cluster under the assumption that it has solid encryption that makes it safe.

wojas commented 1 year ago

Reopening the issue, as the feature is welcome if the points mentioned above are considered.

udf2457 commented 1 year ago

I'll be open and honest honest from my side because I have been thinking about this ....

A thin-wrapper around Go stdlib encryption yielded accusations of "DIY crypto" which IMHO were unjustified as it was a mere wrapper that did not change the crypto behaviour.

Therefore I am not sure how I can propose anything sensible in relation to per-object key-derivation and/or key-rotation without facing similar accusations of "DIY crypto" even though I would be proposing to do nothing more than a thin-wrapper around a pre-existing KDF function, for example.

TL;DR You put me in a "catch 22" situation. You want me to propose crypto related enhancements, but are taking away the tools to enable me to do so.

So therefore I am going to cede my seat on this one and leave it until such time as someone from the Go community who is deemed more worthy comes along, e.g. @FiloSottile for example.

I wish you all the very best with simpleblob but the road ends here for me.

wojas commented 1 year ago

A thin-wrapper around Go stdlib encryption yielded accusations of "DIY crypto" which IMHO were unjustified as it was a mere wrapper that did not change the crypto behaviour.

The code in question was using low level encryption functions, that in itself does not make a secure use of cryptography. The only supporting documentation for your use of these functions was the "Simple encryption helper" comment.

The library you were using ("golang.org/x/crypto/chacha20poly1305") is not really part of the stdlib, but of the 'x' repository that "is a namespace for other packages. Packages under x have looser compatibility requirements. Packages in this namespace may contain backwards incompatible changes within the same major version." It is not clear that it was intended to be an all-round solution for this use case.

If the methodology was explained somewhere on a public URL and deemed safe, you could have linked to it in a comment.

It would be better to use a higher level library vetted for this purpose instead of an underlying library.

Therefore I am not sure how I can propose anything sensible in relation to per-object key-derivation and/or key-rotation without facing similar accusations of "DIY crypto" even though I would be proposing to do nothing more than a thin-wrapper around a pre-existing KDF function, for example.

The S3 API offer a parameter to pass a key, but that does not mean that reusing a single static key is the intended use of this parameter. The MinIO Go client offers an example that I linked to above that shows how to actually use it with the object name as the salt.

TL;DR You put me in a "catch 22" situation. You want me to propose crypto related enhancements, but are taking away the tools to enable me to do so.

Rotation is more about practical deployment scenarios than cryptography algorithms. If you can detect that the key you provided on the first attempt is incorrect, you can fallback to a previous key.

So therefore I am going to cede my seat on this one and leave it until such time as someone from the Go community who is deemed more worthy comes along, e.g. FiloSottile for example.

This is fine, you are not required to do this. Keeping the issue open allows others to pick it up.

I wish you all the very best with simpleblob but the road ends here for me.

I'm sorry you feel that way. Thanks for the contributions that you have made.

udf2457 commented 1 year ago

Regarding /x/crypto will just leave a link to this Reddit comment from @FiloSottile here.

I offer no comment on the rest.

tbaumann commented 7 months ago

Missing cryptography is also pretty much standing between me and using lightningstream on a public cloud. (I need the geo-redundancy)

However, server side encryption is categorically not an option. I have honestly more trust in https://github.com/PowerDNS/simpleblob/pull/59 (or perhaps something like that with less obscure API like NaCL, since the original PR is abandoned)