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

Implementing check to identify CMK errors (s3) #59

Merged
merged 3 commits into from
Jun 29, 2023

Conversation

alanprot
Copy link
Contributor

Some bucket like s3 and Azure implementations allows encrypting the block storage with customer managed keys and the permissions to this keys can be revoked anytime.

This PR is creating a extra method (IsCustomerManagedKeyError) when the key permission got revoked by the key owner and so the upstream can handle it appropriately (cortex,thanos)

Related to: cortexproject/cortex#5420

As a first step only the s3 implementation is identifying such errors but i can follow up with the azure one.

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

Changes

  • Add IsCustomerManagedKeyError method on the bucket interface.
  • Implement IsCustomerManagedKeyError on the s3 bucket implementation.

Signed-off-by: Alan Protasio <alanprot@gmail.com>
@alanprot alanprot force-pushed the IsKeyAccessDeniedErr branch from dbff2d5 to 5ba7eb9 Compare June 23, 2023 01:35
Signed-off-by: Alan Protasio <alanprot@gmail.com>
@alanprot
Copy link
Contributor Author

@GiedriusS @fpetkovski Thoughts?

I wanna use to identify when the key access was revoked so we can return an well defined error from store gateway: on cortex: cortexproject/cortex#5420

@@ -85,6 +85,9 @@ type BucketReader interface {
// IsObjNotFoundErr returns true if error means that object is not found. Relevant to Get operations.
IsObjNotFoundErr(err error) bool

// IsCustomerManagedKeyError returns true if the permissions for key used to encrypt the object was revoked.
IsCustomerManagedKeyError(err error) bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we return a newly defined ErrInvalidCustomerManagedKey error and use errors.Cause to identify it? This way a client does not need an instance of the bucket in order to distinguish between error types.

Copy link
Contributor

Choose a reason for hiding this comment

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

I missed that we already have IsObjNotFoundErr, so I guess this should also be fine.

Copy link
Contributor

@fpetkovski fpetkovski left a comment

Choose a reason for hiding this comment

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

Looks good to me, but would be nice for someone with more knowledge of the codebase to take a look as well.

@@ -85,6 +85,9 @@ type BucketReader interface {
// IsObjNotFoundErr returns true if error means that object is not found. Relevant to Get operations.
IsObjNotFoundErr(err error) bool

// IsCustomerManagedKeyError returns true if the permissions for key used to encrypt the object was revoked.
IsCustomerManagedKeyError(err error) bool
Copy link
Contributor

Choose a reason for hiding this comment

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

I missed that we already have IsObjNotFoundErr, so I guess this should also be fine.

providers/s3/s3.go Outdated Show resolved Hide resolved
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Nice thanks!

Signed-off-by: Alan Protasio <alanprot@gmail.com>
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.

3 participants