Adds support to S3 server side encryption using AWS KMS #3651

lucasvmiguel · 2021-01-06T11:50:46Z

What this PR does: Adds support to S3 server side encryption using KMS.

Which issue(s) this PR fixes:
Fixes #3437

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]
Manually tests to verify if S3 SSE is working
- Data stored without SSE
- Data stored with deprecated SSE (s3.sse-encryption=true)
- Data stored with S3 SSE (sse-config.type=SSE-S3)
- Data store with S3 KMS (sse-config.type=SSE-KMS)

TIP: Just to help future folks that want to use this feature (because I lost sometime to understand how I'd fetch the data back from the S3 bucket). You basically have to create the following IAM policy and attach to an IAM role.

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "VisualEditor0",
            "Effect": "Allow",
            "Action": [
                "kms:Decrypt",
                "s3:GetObject"
            ],
            "Resource": [
                "<AWS KMS KEY ARN>",
                "<AWS S3 BUCKET ARN>/*"
            ]
        }
    ]
}

pracucci

Thanks for working on this! I haven't review it properly. Just leaving a comment about a little thing I've noticed and then will do a proper review.

pkg/chunk/aws/s3_storage_client.go

lucasvmiguel · 2021-01-11T14:35:47Z

Manual tests performed

bucket name	params	result
cortex-test-sse	none	stored without SSE
cortex-test-sse-2	s3.sse-encryption=true	stored encrypted data with SSE-S3
cortex-test-sse-3	s3.sse-config.type=SSE-S3	stored encrypted data with SSE-S3
cortex-test-sse-4	s3.sse-config.type=SSE-KMS and s3.sse-config.kms-key-id=SOME_KEY	stored encrypted data with SSE-KMS

Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com>

pracucci

Good job @lucasvmiguel and sorry for the extremely delay in the review! Overall LGTM, just left few comments to polish it a bit before merging. Thanks!

pkg/chunk/aws/s3_storage_client.go

pracucci · 2021-01-19T14:10:06Z

pkg/chunk/aws/s3_storage_client.go

@@ -100,7 +108,10 @@ func (cfg *S3Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
 	f.StringVar(&cfg.AccessKeyID, prefix+"s3.access-key-id", "", "AWS Access Key ID")
 	f.StringVar(&cfg.SecretAccessKey, prefix+"s3.secret-access-key", "", "AWS Secret Access Key")
 	f.BoolVar(&cfg.Insecure, prefix+"s3.insecure", false, "Disable https on s3 connection.")
-	f.BoolVar(&cfg.SSEEncryption, prefix+"s3.sse-encryption", false, "Enable AES256 AWS Server Side Encryption")
+	f.BoolVar(&cfg.SSEEncryption, prefix+"s3.sse-encryption", false, "Enable AWS Server Side Encryption [Deprecated: Use sse-config instead. if s3.sse-encryption is enabled, it assumes sse-config SSE-S3 type]")
+	f.StringVar(&cfg.SSEConfig.Type, prefix+"s3.sse-config.type", "", "Enable AWS Server Side Encryption. Only SSE-S3 and SSE-KMS are supported")


We're used to define RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) on the receiver config. I would suggest you to do the same on SSEConfig, passing prefix+"s3.sse." as prefix to SSEConfig.RegisterFlagsWithPrefix().

pracucci · 2021-01-19T14:11:23Z

pkg/chunk/aws/s3_storage_client.go

@@ -100,7 +108,10 @@ func (cfg *S3Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
 	f.StringVar(&cfg.AccessKeyID, prefix+"s3.access-key-id", "", "AWS Access Key ID")
 	f.StringVar(&cfg.SecretAccessKey, prefix+"s3.secret-access-key", "", "AWS Secret Access Key")
 	f.BoolVar(&cfg.Insecure, prefix+"s3.insecure", false, "Disable https on s3 connection.")
-	f.BoolVar(&cfg.SSEEncryption, prefix+"s3.sse-encryption", false, "Enable AES256 AWS Server Side Encryption")
+	f.BoolVar(&cfg.SSEEncryption, prefix+"s3.sse-encryption", false, "Enable AWS Server Side Encryption [Deprecated: Use sse-config instead. if s3.sse-encryption is enabled, it assumes sse-config SSE-S3 type]")


Please add also a // TODO Remove in Cortex 1.9.0 (because we guarantee deprecated config to be removed at least after 2 minor releases).

pracucci · 2021-01-19T14:20:52Z

pkg/chunk/aws/sse_config.go

+)
+
+// SSEEncryptionConfig configures server side encryption (SSE)
+type SSEEncryptionConfig struct {


Why do we need this when we already have SSEConfig?

SSEConfig is the struct responsible to receive the config from the CLI or a config file. After SSEConfig is filled with the values provided by the user, a SSEEncryptionConfig struct will be created to be worked inside the application. In the end, SSEEncryptionConfig will have different values than SSEConfig.

pracucci · 2021-01-19T14:22:31Z

pkg/chunk/aws/sse_config.go

+	KMSKeyID             *string
+	KMSEncryptionContext *string


I'm not a big fan of pointers in config structs. You can achieve the same assuming empty string means not configured (which should be fine in this context).

AWS lib uses string pointers several times in there. Removing string pointers from sse_config.go (SSEEncryptionConfig) means that I will have to treat that on the file s3_storage_client.go, which has more logic that should have.

pracucci · 2021-01-19T14:27:45Z

pkg/chunk/aws/s3_storage_client.go

@@ -72,6 +72,7 @@ type S3Config struct {
 	SSEEncryption    bool       `yaml:"sse_encryption"`
 	HTTPConfig       HTTPConfig `yaml:"http_config"`
 	SignatureVersion string     `yaml:"signature_version"`
+	SSEConfig        SSEConfig  `yaml:"sse_config"`


In recent times we stopped appending _config to sub-config structs cause it's a bit redundant. I would remove it from here too.

Suggested change

SSEConfig SSEConfig `yaml:"sse_config"`

SSEConfig SSEConfig `yaml:"sse"`

If we decide to not use _config, we won't be compatible with Thanos config.

I don't mind to remove _config, I just thought that we wanted the config interface exactly the same as Thanos. (to not "break" any future migration)

We're not 1:1 compatible with Thanos even in the blocks storage. It's perfectly fine. We just wanna let configure the same things, so that when we'll add KMS support to blocks storage too we can keep the same config struct.

pracucci · 2021-01-19T14:33:24Z

CHANGELOG.md

@@ -2,6 +2,11 @@

 ## master / unreleased

+
+* [FEATURE] Adds support to S3 server side encryption using KMS. The following config fields have been added. #3651


We're used to group CHANGELOG entries together by type. Please move this entry below, under the group of [FEATURE].

Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com>

pracucci · 2021-01-22T11:07:32Z

CHANGELOG.md

@@ -24,13 +24,17 @@
  * `-cluster.peer-timeout` in favor of `-alertmanager.cluster.peer-timeout`
 * [CHANGE] Blocks storage: the default value of `-blocks-storage.bucket-store.sync-interval` has been changed from `5m` to `15m`. #3724
 * [FEATURE] Querier: Queries can be federated across multiple tenants. The tenants IDs involved need to be specified separated by a `|` character in the `X-Scope-OrgID` request header. This is an experimental feature, which can be enabled by setting `-tenant-federation.enabled=true` on all Cortex services. #3250
-* [ENHANCEMENT] Allow specifying JAEGER_ENDPOINT instead of sampling server or local agent port. #3682
+* [FEATURE] Adds support to S3 server side encryption using KMS. Deprecated `<prefix>.s3.sse-encryption`, you should use the following config fields that have been added. #3651


We've just released 1.7.0-rc.0. Could you rebase master and move the CHANGELOG entry under the master / unreleased section, please?

pracucci · 2021-01-22T11:07:37Z

CHANGELOG.md

 * [FEATURE] Alertmanager: introduced the experimental option `-alertmanager.sharding-enabled` to shard tenants across multiple Alertmanager instances. This feature is still under heavy development and its usage is discouraged. The following new metrics are exported by the Alertmanager: #3664
  * `cortex_alertmanager_ring_check_errors_total`
  * `cortex_alertmanager_sync_configs_total`
  * `cortex_alertmanager_sync_configs_failed_total`
  * `cortex_alertmanager_tenants_discovered`
  * `cortex_alertmanager_tenants_owned`
+* [ENHANCEMENT] Allow specifying JAEGER_ENDPOINT instead of sampling server or local agent port. #3682


We've just released 1.7.0-rc.0. Could you rebase master and move the CHANGELOG entry under the master / unreleased section, please?

Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com>

pracucci

Thanks for the follow up! We're very close! I left last comments.

Please mention that S3 server-side encryption is experimental in docs/configuration/v1-guarantees.md (there's a list of experimental features). This will allow us to introduce config breaking changes in the near future if we find out any issue (or improvement).

pracucci · 2021-01-26T16:10:56Z

CHANGELOG.md

@@ -2,6 +2,10 @@

 ## master / unreleased

+* [FEATURE] Adds support to S3 server side encryption using KMS. Deprecated `<prefix>.s3.sse-encryption`, you should use the following config fields that have been added. #3651


Suggested change

* [FEATURE] Adds support to S3 server side encryption using KMS. Deprecated `<prefix>.s3.sse-encryption`, you should use the following config fields that have been added. #3651

* [FEATURE] Adds support to S3 server side encryption using KMS. Deprecated `-<prefix>.s3.sse-encryption`, you should use the following CLI flags that have been added. #3651

pracucci · 2021-01-26T16:11:15Z

CHANGELOG.md

@@ -2,6 +2,10 @@

 ## master / unreleased

+* [FEATURE] Adds support to S3 server side encryption using KMS. Deprecated `<prefix>.s3.sse-encryption`, you should use the following config fields that have been added. #3651
+  - `<prefix>.s3.sse.type`


Suggested change

- `<prefix>.s3.sse.type`

- `-<prefix>.s3.sse.type`

pracucci · 2021-01-26T16:11:21Z

CHANGELOG.md

@@ -2,6 +2,10 @@

 ## master / unreleased

+* [FEATURE] Adds support to S3 server side encryption using KMS. Deprecated `<prefix>.s3.sse-encryption`, you should use the following config fields that have been added. #3651
+  - `<prefix>.s3.sse.type`
+  - `<prefix>.s3.sse.kms-key-id`


Suggested change

- `<prefix>.s3.sse.kms-key-id`

- `-<prefix>.s3.sse.kms-key-id`

pracucci · 2021-01-26T16:11:27Z

CHANGELOG.md

+* [FEATURE] Adds support to S3 server side encryption using KMS. Deprecated `<prefix>.s3.sse-encryption`, you should use the following config fields that have been added. #3651
+  - `<prefix>.s3.sse.type`
+  - `<prefix>.s3.sse.kms-key-id`
+  - `<prefix>.s3.sse.kms-encryption-context`


Suggested change

- `<prefix>.s3.sse.kms-encryption-context`

- `-<prefix>.s3.sse.kms-encryption-context`

pracucci · 2021-01-26T16:22:17Z

pkg/chunk/aws/sse_config.go

+type SSEConfig struct {
+	Type                 string     `yaml:"type"`
+	KMSKeyID             string     `yaml:"kms_key_id"`
+	KMSEncryptionContext chunk.Tags `yaml:"kms_encryption_context"`


I don't think we should use chunk.Tags here. I would simplify it just taking in input the context json as a string. Moreover, I would clarify in the CLI flag description that it expects a JSON as a string.

pracucci · 2021-01-26T16:25:56Z

pkg/chunk/aws/sse_config_test.go

+			},
+		},
+		{
+			name: "Test invalid SEE type",


Suggested change

name: "Test invalid SEE type",

name: "Test invalid SSE type",

pracucci · 2021-01-26T16:26:24Z

pkg/chunk/aws/sse_config_test.go

+			},
+		},
+		{
+			name: "Test SSE encryption with SSEKMS type with context",


This test is duplicated from above.

Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com>

…e-3437

pracucci

Thanks for the follow up! I have a last question about context parsing.

pkg/chunk/aws/sse_config.go

CHANGELOG.md

Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com>

pstibrany

LGTM, thank you! Please take a look at final comments before merging.

pstibrany · 2021-01-28T08:20:25Z

pkg/chunk/aws/sse_config.go

+}
+
+// NewSSEParsedConfig creates a struct to configure server side encryption (SSE)
+func NewSSEParsedConfig(sseType string, kmsKeyID string, kmsEncryptionContext string) (*SSEParsedConfig, error) {


Nit: Perhaps this should have been a method on SSEConfig or take it as a parameter?

pkg/chunk/aws/sse_config.go

pstibrany · 2021-01-28T08:35:55Z

pkg/chunk/aws/sse_config_test.go

+func TestNewSSEParsedConfig(t *testing.T) {
+	kmsKeyID := "test"
+	kmsEncryptionContext := `{"a": "bc", "b": "cd"}`
+	parsedKMSEncryptionContext := "eyJhIjoiYmMiLCJiIjoiY2QifQ=="


Suggested change

parsedKMSEncryptionContext := "eyJhIjoiYmMiLCJiIjoiY2QifQ=="

parsedKMSEncryptionContext = base64.StdEncoding.EncodeToString([]byte(`{"a":"bc","b":"cd"}`)) // compact form of kmsEncryptionContext

Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com>

…t/cortex#3651) * Adds support to S3 server side encryption using AWS KMS Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com> * refatored based on PR review Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com> * small refactor Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com> * rebased master Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com> * rebased master correctly Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com> * added new line Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com> * refactored Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com> * reordered changelog Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com> * refactored NewSSEParsedConfig Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com> * removed unused struct Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com> Co-authored-by: Lucas Vieira <lucas.vieira@ASSUMESEEQUITE.workdayinternal.com>

pull-request-size bot added the size/L label Jan 6, 2021

lucasvmiguel force-pushed the issue-3437 branch from 2749d74 to eaf5f8a Compare January 6, 2021 12:15

lucasvmiguel changed the title ~~(WIP) Adds support to S3 server side encryption using KMS. #~~ Adds support to S3 server side encryption using AWS KMS Jan 7, 2021

lucasvmiguel changed the title ~~Adds support to S3 server side encryption using AWS KMS~~ (WIP) Adds support to S3 server side encryption using AWS KMS Jan 7, 2021

pracucci reviewed Jan 7, 2021

View reviewed changes

pkg/chunk/aws/s3_storage_client.go Outdated Show resolved Hide resolved

lucasvmiguel force-pushed the issue-3437 branch 2 times, most recently from 1df3429 to 7e6f625 Compare January 7, 2021 16:28

pull-request-size bot added size/XL and removed size/L labels Jan 11, 2021

pull-request-size bot added size/L and removed size/XL labels Jan 11, 2021

lucasvmiguel force-pushed the issue-3437 branch from 0411e31 to 46fcdc0 Compare January 11, 2021 15:01

lucasvmiguel changed the title ~~(WIP) Adds support to S3 server side encryption using AWS KMS~~ Adds support to S3 server side encryption using AWS KMS Jan 11, 2021

lucasvmiguel force-pushed the issue-3437 branch from b9c7f82 to 683c61d Compare January 11, 2021 17:01

Adds support to S3 server side encryption using AWS KMS

0925ad6

Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com>

lucasvmiguel force-pushed the issue-3437 branch from 683c61d to 0925ad6 Compare January 11, 2021 18:03

lucasvmiguel requested a review from pracucci January 12, 2021 13:40

Merge branch 'master' into issue-3437

192b1ab

pracucci reviewed Jan 19, 2021

View reviewed changes

lucasvmiguel added 2 commits January 19, 2021 15:56

refatored based on PR review

3fdf51c

Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com>

small refactor

183895d

Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com>

lucasvmiguel requested a review from pracucci January 19, 2021 16:23

lucasvmiguel added 2 commits January 20, 2021 09:35

Merge branch 'master' into issue-3437

c9d03a8

Merge branch 'master' into issue-3437

ea13344

pracucci reviewed Jan 22, 2021

View reviewed changes

lucasvmiguel added 3 commits January 22, 2021 11:27

Merge remote-tracking branch 'upstream/master' into issue-3437

61db766

rebased master

1132d86

Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com>

rebased master correctly

cf146e5

Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com>

added new line

8ca51e3

Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com>

lucasvmiguel requested a review from pracucci January 25, 2021 14:00

Merge branch 'master' into issue-3437

590c0c8

pracucci reviewed Jan 26, 2021

View reviewed changes

lucasvmiguel added 2 commits January 27, 2021 10:20

refactored

62082bb

Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com>

Merge branch 'issue-3437' of github.com:lucasvmiguel/cortex into issu…

2972428

…e-3437

lucasvmiguel requested a review from pracucci January 27, 2021 10:43

pracucci reviewed Jan 27, 2021

View reviewed changes

pkg/chunk/aws/sse_config.go Show resolved Hide resolved

CHANGELOG.md Show resolved Hide resolved

reordered changelog

bffcc7d

Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com>

pracucci approved these changes Jan 27, 2021

View reviewed changes

pstibrany approved these changes Jan 28, 2021

View reviewed changes

lucasvmiguel added 2 commits January 28, 2021 09:48

refactored NewSSEParsedConfig

2536269

Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com>

removed unused struct

93ec9e5

Signed-off-by: Lucas Miguel <lucasvieira.dev@gmail.com>

pstibrany merged commit f639b18 into cortexproject:master Jan 28, 2021

pracucci mentioned this pull request Feb 10, 2021

Add S3 KMS support to blocks storage client #3810

Merged

3 tasks

saswatamcode mentioned this pull request Apr 1, 2022

Instrument service with metrics observatorium/rules-objstore#9

Merged

	SSEConfig SSEConfig `yaml:"sse_config"`
	SSEConfig SSEConfig `yaml:"sse"`

		@@ -2,6 +2,11 @@

		## master / unreleased


		* [FEATURE] Adds support to S3 server side encryption using KMS. The following config fields have been added. #3651

		@@ -2,6 +2,10 @@

		## master / unreleased

		* [FEATURE] Adds support to S3 server side encryption using KMS. Deprecated `<prefix>.s3.sse-encryption`, you should use the following config fields that have been added. #3651

	- `<prefix>.s3.sse.kms-key-id`
	- `-<prefix>.s3.sse.kms-key-id`

	- `<prefix>.s3.sse.kms-encryption-context`
	- `-<prefix>.s3.sse.kms-encryption-context`

	name: "Test invalid SEE type",
	name: "Test invalid SSE type",

	parsedKMSEncryptionContext := "eyJhIjoiYmMiLCJiIjoiY2QifQ=="
	parsedKMSEncryptionContext = base64.StdEncoding.EncodeToString([]byte(`{"a":"bc","b":"cd"}`)) // compact form of kmsEncryptionContext

Adds support to S3 server side encryption using AWS KMS #3651

Adds support to S3 server side encryption using AWS KMS #3651

Uh oh!

Conversation

lucasvmiguel commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lucasvmiguel commented Jan 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lucasvmiguel commented Jan 6, 2021 •

edited

Loading

lucasvmiguel commented Jan 11, 2021 •

edited

Loading