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

Adds SSE-KMS and SSE-C config to S3 Objstore #3064

Merged
merged 13 commits into from
Aug 25, 2020
Merged

Adds SSE-KMS and SSE-C config to S3 Objstore #3064

merged 13 commits into from
Aug 25, 2020

Conversation

jalev
Copy link
Contributor

@jalev jalev commented Aug 23, 2020

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Adds S3 SSE via a sse_config block. It's a reimplementation of #2170 since it looks like it was abandoned. It solves #946.

There are 3 types of SSE you can use: SSE-S3, SSE-KMS, and SSE-C.

  sse_config:
    # Whether or not we use SSE at all. This should be set at the very minimum to enable SSE-S3.
    enabled: false
    # If someone wants to use their KMS key, we let them set that up by passing in kms_key_id. This sets up SSE-KMS
    kms_key_id: ""
    # kms_encryption_context provides additional variables to use for encryption/decryption. It doesn't need to be set as AWS provides a default encryption context, but you _can_ set it if you want something more.
    kms_encryption_context: {}
    # encryption_key is what it says on the tin. This sets up SSE-C.
    encryption_key: ""

Verification

I can verify if this works when I get back to work. We're currently running into 403 errors from not providing a KMS key ID, so when we don't get 403s I'll know it works.

Signed-off-by: James Bach <james.bach@transferwise.com>
Signed-off-by: James Bach <james.bach@transferwise.com>
@jalev jalev changed the title Adds SSE config to S3 Objstore Adds SSE-KMS and SSE-C config to S3 Objstore Aug 23, 2020
Signed-off-by: James Bach <james.bach@transferwise.com>
Signed-off-by: James Bach <james.bach@transferwise.com>
Signed-off-by: James Bach <james.bach@transferwise.com>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Amazing, this is what we need. I have minor suggestions though, otherwise LGTM!

Thanks for helping here and reimplementing old PR 🤗

docs/storage.md Outdated Show resolved Hide resolved
Signed-off-by: James Bach <james.bach@transferwise.com>
Signed-off-by: James Bach <james.bach@transferwise.com>
Signed-off-by: James Bach <james.bach@transferwise.com>
@jalev
Copy link
Contributor Author

jalev commented Aug 24, 2020

Prior to KMS configuration:

level=info ts=2020-08-24T09:44:57.119043821Z caller=http.go:81 service=http/server component=compact msg="internal server shutdown" err="error executing compaction: compaction: group 0@13480198280331570948: upload of 01EGFXMP8HXRQRX2WEPB7E317Q failed: upload meta file to debug dir: upload file /var/thanos/compact/compact/0@13480198280331570948/01EGFXMP8HXRQRX2WEPB7E317Q/meta.json as debug/metas/01EGFXMP8HXRQRX2WEPB7E317Q.json: upload s3 object: Access Denied"
level=info ts=2020-08-24T09:44:57.119069085Z caller=intrumentation.go:66 msg="changing probe status" status=not-healthy reason="error executing compaction: compaction: group 0@13480198280331570948: upload of 01EGFXMP8HXRQRX2WEPB7E317Q failed: upload meta file to debug dir: upload file /var/thanos/compact/compact/0@13480198280331570948/01EGFXMP8HXRQRX2WEPB7E317Q/meta.json as debug/metas/01EGFXMP8HXRQRX2WEPB7E317Q.json: upload s3 object: Access Denied"

So, for testing (with KMS), I use the the following policy that lets Thanos actually use the KMS key + upload to buckets:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "VisualEditor0",
            "Effect": "Allow",
            "Action": [
                "s3:PutObject",
                "s3:GetObject",
                "s3:AbortMultipartUpload",
                "s3:ListBucket",
                "s3:DeleteObject",
                "s3:PutObjectAcl",
                "s3:ListMultipartUploadParts"
            ],
            "Resource": [
                "arn:aws:s3:::secret-bucket",
                "arn:aws:s3:::secret-bucket/*"
            ]
        },
        {
            "Sid": "VisualEditor1",
            "Effect": "Allow",
            "Action": [
                "kms:Encrypt",
                "kms:GenerateDataKey",
                "kms:Decrypt"
            ],
            "Resource": [
                "arn:aws:kms:eu-central-1:1234567890abc:key/12345678-12ab-34cd-56ef-1234567890ab"
            ]
        }
    ]
}

And configure the key (using custom helm chart, ignore that but take note of the sse_config):

  service:
    args:
      - "compact"
      - "--log.level=debug"
      - "--log.format=logfmt"
      - "--http-address=0.0.0.0:10902"
      - |
        --objstore.config=
          type: S3
          config:
            bucket: secret-bucket
            endpoint: s3.eu-central-1.amazonaws.com
            sse_config:
              type: SSE-KMS
              kms_key_id: 12345678-12ab-34cd-56ef-1234567890ab
      - "--data-dir=/var/thanos/compact"
      - "--consistency-delay=30m"
      - "--retention.resolution-raw=0d"
      - "--retention.resolution-5m=0d"
      - "--retention.resolution-1h=0d"
      - "--block-sync-concurrency=20"
      - "--compact.concurrency=1"
      - "--wait"
level=debug ts=2020-08-24T10:33:14.97343253Z caller=objstore.go:158 group="0@{cluster=\"secret-environment.transferwise.com\", prometheus=\"prometheus\", prometheus_shard=\"0\", replica=\"prometheus-k8s-shard-0-1.secret-environment.transferwise.com\"}" groupKey=0@11189359859295756877 msg="uploaded file" from=/var/thanos/compact/compact/0@11189359859295756877/01EGG0DS7MV3TDG8HGNTVA4W3F/meta.json dst=debug/metas/01EGG0DS7MV3TDG8HGNTVA4W3F.json bucket="tracing: secret-bucket"
level=info ts=2020-08-24T10:33:16.778498888Z caller=fetcher.go:453 component=block.BaseFetcher msg="successfully synchronized block metadata" duration=244.381563ms cached=168 returned=168 partial=1
level=debug ts=2020-08-24T10:33:16.801469782Z caller=objstore.go:158 group="0@{cluster=\"secret-environment.transferwise.com\", prometheus=\"prometheus\", prometheus_shard=\"0\", replica=\"prometheus-k8s-shard-0-1.secret-environment.transferwise.com\"}" groupKey=0@11189359859295756877 msg="uploaded file" from=/var/thanos/compact/compact/0@11189359859295756877/01EGG0DS7MV3TDG8HGNTVA4W3F/chunks/000001 dst=01EGG0DS7MV3TDG8HGNTVA4W3F/chunks/000001 bucket="tracing: secret-bucket"
level=debug ts=2020-08-24T10:33:18.556145359Z caller=objstore.go:158 group="0@{cluster=\"secret-environment.transferwise.com\", prometheus=\"prometheus\", prometheus_shard=\"0\", replica=\"prometheus-k8s-shard-0-1.secret-environment.transferwise.com\"}" groupKey=0@11189359859295756877 msg="uploaded file" from=/var/thanos/compact/compact/0@11189359859295756877/01EGG0DS7MV3TDG8HGNTVA4W3F/chunks/000002 dst=01EGG0DS7MV3TDG8HGNTVA4W3F/chunks/000002 bucket="tracing: secret-bucket"
level=debug ts=2020-08-24T10:33:20.047301162Z caller=objstore.go:158 group="0@{cluster=\"secret-environment.transferwise.com\", prometheus=\"prometheus\", prometheus_shard=\"0\", replica=\"prometheus-k8s-shard-0-1.secret-environment.transferwise.com\"}" groupKey=0@11189359859295756877 msg="uploaded file" from=/var/thanos/compact/compact/0@11189359859295756877/01EGG0DS7MV3TDG8HGNTVA4W3F/index dst=01EGG0DS7MV3TDG8HGNTVA4W3F/index bucket="tracing: secret-bucket"
level=debug ts=2020-08-24T10:33:20.101614981Z caller=objstore.go:158 group="0@{cluster=\"secret-environment.transferwise.com\", prometheus=\"prometheus\", prometheus_shard=\"0\", replica=\"prometheus-k8s-shard-0-1.secret-environment.transferwise.com\"}" groupKey=0@11189359859295756877 msg="uploaded file" from=/var/thanos/compact/compact/0@11189359859295756877/01EGG0DS7MV3TDG8HGNTVA4W3F/meta.json dst=01EGG0DS7MV3TDG8HGNTVA4W3F/meta.json bucket="tracing: secret-bucket"

And to test with an unhappy case of a misconfigured key:

level=error ts=2020-08-24T10:52:58.150471275Z caller=compact.go:382 msg="retriable error" err="compaction: group 0@11189359859295756877: upload of 01EGG1HVYM3329P0P5BQCNVD7X failed: upload meta file to debug dir: upload file /var/thanos/compact/compact/0@11189359859295756877/01EGG1HVYM3329P0P5BQCNVD7X/meta.json as debug/metas/01EGG1HVYM3329P0P5BQCNVD7X.json: upload s3 object: Key 'arn:aws:kms:eu-central-1:1234567890abc:key/12345678-12ab-34cd-56ef-BROKENKEYOHNO' does not exist"

I should add the policy I used to the documentation.

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Amazing. Small suggestions only and happy to merge this! 🤗

```yaml

---
sse_config:
Copy link
Member

Choose a reason for hiding this comment

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

👍

pkg/objstore/s3/s3.go Outdated Show resolved Hide resolved
pkg/objstore/s3/s3.go Outdated Show resolved Hide resolved
Signed-off-by: James Bach <james.bach@transferwise.com>
Signed-off-by: James Bach <james.bach@transferwise.com>
Signed-off-by: James Bach <james.bach@transferwise.com>
Signed-off-by: James Bach <james.bach@transferwise.com>
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for your patience! 💪

pkg/objstore/s3/s3.go Outdated Show resolved Hide resolved
@@ -173,8 +192,32 @@ func NewBucketWithConfig(logger log.Logger, config Config, component string) (*B
client.SetAppInfo(fmt.Sprintf("thanos-%s", component), fmt.Sprintf("%s (%s)", version.Version, runtime.Version()))

var sse encrypt.ServerSide
if config.SSEEncryption {
sse = encrypt.NewSSE()
if config.SSEConfig.Type != "" {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this we can have just empty case for ""

Not a blocker (:

Signed-off-by: James Bach <james.bach@transferwise.com>
@jalev
Copy link
Contributor Author

jalev commented Aug 24, 2020

Going to wait for tests to run and then ready to merge 👌

@bwplotka
Copy link
Member

OMG link checking job is too annoying, let's kill it for now.... Related issue: #3060

@bwplotka bwplotka merged commit 61b8382 into thanos-io:master Aug 25, 2020
@bwplotka
Copy link
Member

Thanks!

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

We are missing an important things here.

encrypt_sse: true should fail and not be silently ignored. This will ensure users will not get surprised. Will add that change in separate PR (:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants