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.

Sign up for GitHub

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

Jump to bottom

objstore: add experimental encryption wrapper #46

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

objstore: add experimental encryption wrapper #46

wants to merge 1 commit into from

Conversation

MichaHoffmann
Copy link
Contributor

@MichaHoffmann MichaHoffmann commented Feb 18, 2023 •
edited
Loading

This PR attempts to add an encryption wrapper that defers to github.com/minio/sio for encryption and decryption. This is useful when attempting to use Thanos in environments that need PCI DSS compliance.

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

Changes

  • added optional encryption wrapper to buckets

Verification

  • passes the acceptance tests
  • built thanos with this fork and set up receiver, querier, store and pushed metrics into receiver using telegraf. querier pointing at store eventually showed the correct metrics, i verified that the contents of object storage are encrypted too.

@MichaHoffmann MichaHoffmann force-pushed the mhoffm-add-experimental-encryption-wrapper branch 7 times, most recently from 129a4cc to dde5f47 Compare February 18, 2023 17:42
@MichaHoffmann MichaHoffmann marked this pull request as ready for review February 18, 2023 17:55
@MichaHoffmann MichaHoffmann force-pushed the mhoffm-add-experimental-encryption-wrapper branch 7 times, most recently from 60868e9 to dbf50f1 Compare February 18, 2023 21:59
@MichaHoffmann
Copy link
Contributor Author

README.md changes were automatically created by make lint

@MichaHoffmann MichaHoffmann force-pushed the mhoffm-add-experimental-encryption-wrapper branch 5 times, most recently from 375a48d to 573a56d Compare February 19, 2023 22:02
GiedriusS
GiedriusS reviewed Feb 20, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
@MichaHoffmann MichaHoffmann force-pushed the mhoffm-add-experimental-encryption-wrapper branch 3 times, most recently from b59025d to 89368db Compare February 20, 2023 16:31
@MichaHoffmann MichaHoffmann requested review from bwplotka and GiedriusS and removed request for bwplotka and GiedriusS February 20, 2023 17:10
MichaHoffmann
MichaHoffmann commented Feb 20, 2023
View reviewed changes
objstore.go Outdated Show resolved Hide resolved
@MichaHoffmann MichaHoffmann force-pushed the mhoffm-add-experimental-encryption-wrapper branch 2 times, most recently from 0306227 to d773f93 Compare February 20, 2023 18:27
@alanprot
Copy link
Contributor

Idk what block storage you are using but cortex has s3 with sse... maybe is worth looking at it:

https://github.com/cortexproject/cortex/blob/master/pkg/storage/bucket/sse_bucket_client.go

@MichaHoffmann
Copy link
Contributor Author

Idk what block storage you are using but cortex has s3 with sse... maybe is worth looking at it:

https://github.com/cortexproject/cortex/blob/master/pkg/storage/bucket/sse_bucket_client.go

I wanted to add client side encryption for situations where server side encryption is not an option though

GiedriusS
GiedriusS reviewed May 3, 2023
View reviewed changes
objstore.go Outdated
if err != nil {
return nil, errors.Wrap(err, "unable to fetch salt")
}
defer saltReader.Close()
Copy link
Member

Choose a reason for hiding this comment

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

I believe that we also need to exhaust the reader so that keep-alive connections would be properly maintained.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

GiedriusS
GiedriusS reviewed May 3, 2023
View reviewed changes
objstore.go Outdated
if err != nil {
return 0, err
}
defer rc.Close()
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

@MichaHoffmann MichaHoffmann force-pushed the mhoffm-add-experimental-encryption-wrapper branch from d773f93 to d79d0af Compare May 4, 2023 10:51
@MichaHoffmann MichaHoffmann force-pushed the mhoffm-add-experimental-encryption-wrapper branch from d79d0af to 30eccd1 Compare May 4, 2023 10:53
GiedriusS
GiedriusS reviewed May 15, 2023
View reviewed changes
objstore.go
if err != nil {
return nil, errors.Wrap(err, "unable to fetch salt")
}
defer metaReader.Close()
Copy link
Member

Choose a reason for hiding this comment

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

Let's use: https://github.com/efficientgo/tools/blob/6b7392939a606553331e012253354849d7f9df92/core/pkg/logerrcapture/do.go#L44

GiedriusS
GiedriusS reviewed May 15, 2023
View reviewed changes
client/factory.go
if len(key) != 32 {
return nil, errors.New("decoded key must have size 32")
}
bucket = objstore.BucketWithEncryption(bucket, key)
Copy link
Member

Choose a reason for hiding this comment

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

Let's pass the logger here so that we could log a message in case Close() and/or exhaustion fails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GiedriusS GiedriusS GiedriusS left review comments

@bwplotka bwplotka Awaiting requested review from bwplotka

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MichaHoffmann @alanprot @GiedriusS @bwplotka