-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
#946: Implement SSE-KMS and SSE-C for AWS S3 #2170
Conversation
Signed-off-by: Alexander Laties <[email protected]>
Signed-off-by: Alexander Laties <[email protected]>
Signed-off-by: Alexander Laties <[email protected]>
Signed-off-by: Alexander Laties <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! One suggestion really.
// NOTE: If both sse_c_key and sse_kms_id are set, we throw an error, as it's unclear what the operator wants. | ||
// See https://docs.aws.amazon.com/AmazonS3/latest/dev/ServerSideEncryptionCustomerKeys.html | ||
// for more details on the SSE-S3, SSE-KMS, and SSE-C options. | ||
SSEEncryptionLegacy bool `yaml:"encrypt_sse"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want something like this, but what about, since we are in struct make it more generic? The problem with this approach is that the relation if user will see ALL three of them is not really sure.
It should be clear that you can either set sse_s3
, see_c
or see_kms
if possible... (:
Or at least let's create SSE SSEConfig
field and put all of those in such struct. I think that would work. Also let's remove legacy field and add CHANGELOG item for removal of this. What do you think?
This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions. |
It seems like this feature should work but probably not in my environment. During attempts to upload the new block into AWS S3 store or to read |
Implements #946
Changes
Added support for parsing and passing along AWS S3 SSE-KMS and SSE-C mode configuration details. Seemed pretty easy.
Adds the following config vars:
sse_s3
- essentially the newencrypt_sse
, enables SSE-S3 mode.sse_kms_id
- enables SSE-KMS mode when set.sse_kms_context
- optional map of string keys and values for the KMS Context.sse_c_key
- enables SSE-C mode when set. expects a filepath.encrypt_sse
is kept as an alias forsse_s3
, as that's whatencrypt_sse
currently does, and it seems silly to break valid configuration files from prior to this change for little reason.Note that KMS and C mode both supersede and override SSE-S3 mode currently. This allows for syntax like:
to work as expected.
KMS and C mode are mutually exclusive, and trying to enable both currently throws an error.
E.G.
Verification
Haven't yet really... would like some help from @jujugrrr to validate the code does what I think it does before going any further really.