-
Notifications
You must be signed in to change notification settings - Fork 331
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
Introduce support of GCS encryption for both CMEK and CSEK #7608
Introduce support of GCS encryption for both CMEK and CSEK #7608
Conversation
Hey @emulatorchen , thank you for your contribution! I should of reviewed sooner, it took me a bit longer since I needed to ramp up my gcp/kms knowledge. @Jonathan-Rosenberg and myself were debating some CSEK/CMEK questions. As we're both not very familiar with gcp, I wonder if you could help us with answering the below:
|
Really appreciate the review! It's my honor to have those critical questions being addressed.
The reason I am making it in lakeFS level is because there's already a S3 reference in the lakeFS level that I thought it will be easier for you to accept it so that we can meet the internal need first. Then I would like look for a possible solution as a multi-tenant proposal, and they should be able to exist in lakeFS just cannot be configured at the same time. But we all know that it will be more complicated, especially when we are providing an universal UI/API/CLI to the operation of different storage options, different keys will require not just the permission but also the key management in lakeFS itself or we may have issues in CSEK(ex. not able to preview in UI, similar reason I dropped the support of PreSignedURL when CSEK enabled) or the key of the bucket is not using the default key of the project. And now I implementing CSEK is just providing an option, we will consider more on CMEK. The reason is that CMEK can actually be enabled almost without implementation in lakeFS as long as the key is the default key of the project and being proper authenticated. On the other hand, CSEK is more like a thing that user can have more control when there's risk happened. So the implementation is to consider the case we would like the CMEK being used as an internal protection for now, and I am happy to remove CSEK if you think it's risky. |
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.
Thank you for the elaborated response and the PR!
I have added some comments, so lets get through them and continue on.
pkg/block/factory/build.go
Outdated
if params.ServerSideEncryptionCustomerSupplied != nil { | ||
opts = append(opts, gs.WithServerSideEncryptionCustomerSupplied(params.ServerSideEncryptionCustomerSupplied)) | ||
} | ||
if params.ServerSideEncryptionKmsKeyID != "" { | ||
opts = append(opts, gs.WithServerSideEncryptionKmsKeyID(params.ServerSideEncryptionKmsKeyID)) | ||
} |
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.
Please use an empty switch
case to make it more idiomatic and emphasize that there can be only one config type.
pkg/config/config.go
Outdated
if c.Blockstore.GS.ServerSideEncryptionCustomerSupplied != "" { | ||
v, err := hex.DecodeString(c.Blockstore.GS.ServerSideEncryptionCustomerSupplied) | ||
if err != nil || len(v) != GcpAESKeyLength { | ||
logging.ContextUnavailable().WithError(err).Fatalf("Value of customer-supplied server side encryption is not a valid %d bytes AES key", GcpAESKeyLength) |
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.
please return an error instead. Logging is operated using a middleware. It will be fatal if the block parameters aren't returned properly (i.e. when an error is returned)
pkg/config/config.go
Outdated
} | ||
customerSuppliedKey = v | ||
if c.Blockstore.GS.ServerSideEncryptionKmsKeyID != "" { | ||
logging.ContextUnavailable().Fatal("Setting both kms and customer supplied encryption will result failure when reading/writing object") |
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.
same
pkg/config/config.go
Outdated
@@ -486,17 +489,35 @@ func (c *Config) BlockstoreLocalParams() (blockparams.Local, error) { | |||
return params, nil | |||
} | |||
|
|||
const ( | |||
GcpAESKeyLength = 32 |
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.
GcpAESKeyLength = 32 | |
gcpAESKeyLength = 32 |
pkg/block/gs/adapter.go
Outdated
Ideally we assume all the objects should be encrypted with AES key at the very beginning | ||
But it can also be the case that some of existing objects are created before the key is introduced | ||
So we determined the object and see if it's encrypted. But we don't support multiple keys for | ||
individual objects, so it will generate the error when an improper key is supplied. | ||
|
||
We don't check the key from KMS as it will be decrypted as long as the service account with proper permissions |
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.
Ideally we assume all the objects should be encrypted with AES key at the very beginning | |
But it can also be the case that some of existing objects are created before the key is introduced | |
So we determined the object and see if it's encrypted. But we don't support multiple keys for | |
individual objects, so it will generate the error when an improper key is supplied. | |
We don't check the key from KMS as it will be decrypted as long as the service account with proper permissions | |
prepareEncryptedReadHandle will assume that all of the objects are encrypted with an AES key at the very beginning. | |
It may be that some existing objects were created before the key was introduced, so objects will be examined and checked for encryption. | |
Multiple keys for individual objects aren't supported! An error will be generated if an improper key is supplied. | |
Keys are not validated in KMS. You should use a proper service account with permissions to access them. |
pkg/block/gs/adapter.go
Outdated
h := (*storage.ObjectHandle)(o) | ||
att, err := h.Attrs(ctx) | ||
if err == nil { | ||
a.log(ctx).Debug("Object has attribute customerKeySHA256 means it is encrypted by Customer-Supplied key: ", att.CustomerKeySHA256) |
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 this log should be inside the if clause below.
Also, change to lower case (Object
-> object
)
pkg/block/gs/adapter.go
Outdated
} | ||
} | ||
// Assume no decryption needed when attrs is not found | ||
a.log(ctx).Infof("Object Attrs get error %s", err) |
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.
lower case as well.
change to debug please
pkg/block/gs/adapter.go
Outdated
return h | ||
} | ||
|
||
type storageObjectHandle storage.ObjectHandle |
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.
Please explain the reasoning behind this type. Why not use functions (instead of methods) that accept storage.ObjectHandle
as parameters?
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.
Thank you once more!
Since you introduce a new type that wraps another, the methods and functions should use this type instead of the wrapped one. I gave you some comments regarding that, but in general, since the methods of the wrapped struct (storage.ObjectHandle
) can be called over the wrapping type, you can use the wrapping type only. This will create consistency and a single way of using the methods.
Other than that LGTM!
pkg/block/gs/adapter.go
Outdated
/* | ||
Ideally we assume all the objects should be encrypted with AES key at the very beginning | ||
It may be that some existing objects were created before the key is was introduced, so objects will be examined and checked for encryption. | ||
Multiple keys for individual objects aren't supported! An error will be generated if an improper key is supplied. | ||
Keys in KMS will not be validated. You should use a proper service account with permissions to access them. | ||
*/ |
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.
comments related to methods/functions in Go begin with the method/function's name. Please rephrase that.
pkg/block/gs/adapter.go
Outdated
return c | ||
} | ||
|
||
func (o *storageObjectHandle) newComposer(a *Adapter, srcs ...*storage.ObjectHandle) (*storage.ObjectHandle, *storage.Composer) { |
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.
just as you did with newCopier
, please change newComposer
, and newWriter
to return pointers to the structs they create
pkg/block/gs/adapter.go
Outdated
|
||
func (o *storageObjectHandle) prepareWriteHandle(a *Adapter) *storageObjectHandle { | ||
if a.ServerSideEncryptionCustomerSupplied != nil { | ||
return &storageObjectHandle{o.Key(a.ServerSideEncryptionCustomerSupplied)} |
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.
return &storageObjectHandle{o.Key(a.ServerSideEncryptionCustomerSupplied)} | |
o.ObjectHandle = o.Key(a.ServerSideEncryptionCustomerSupplied) |
pkg/block/gs/adapter.go
Outdated
*storage.ObjectHandle | ||
} | ||
|
||
func (o *storageObjectHandle) prepareWriteHandle(a *Adapter) *storageObjectHandle { |
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.
This is classic for the with
prefix:
func (o *storageObjectHandle) prepareWriteHandle(a *Adapter) *storageObjectHandle { | |
func (o *storageObjectHandle) withWriteHandle(a *Adapter) *storageObjectHandle { |
pkg/block/gs/adapter.go
Outdated
Multiple keys for individual objects aren't supported! An error will be generated if an improper key is supplied. | ||
Keys in KMS will not be validated. You should use a proper service account with permissions to access them. | ||
*/ | ||
func (o *storageObjectHandle) prepareReadHandle(ctx context.Context, a *Adapter) *storage.ObjectHandle { |
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.
prepareReadHandle
and prepareWriteHandle
should return the same type for consistency.
Please return a *storageObjectHandle
in both.
pkg/block/gs/adapter.go
Outdated
if err == nil { | ||
a.log(ctx).Debug("object has attribute customerKeySHA256 means it is encrypted by Customer-Supplied key: ", att.CustomerKeySHA256) | ||
if a.ServerSideEncryptionCustomerSupplied != nil && att.CustomerKeySHA256 != "" { | ||
return o.Key(a.ServerSideEncryptionCustomerSupplied) |
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.
return o.Key(a.ServerSideEncryptionCustomerSupplied) | |
o.ObjectHandle = o.Key(a.ServerSideEncryptionCustomerSupplied) |
pkg/block/gs/adapter.go
Outdated
} | ||
// Assume no decryption needed when attrs is not found | ||
a.log(ctx).Debugf("object Attrs get error %w", err) | ||
return o.ObjectHandle |
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.
return o.ObjectHandle | |
return o |
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.
Thank you @emulatorchen.
Your contribution is highly appreciated 🙌
We should also add docs to https://github.com/treeverse/lakeFS/blob/master/docs/howto/deploy/gcp.md, but this could be made on a different PR.
@emulatorchen you would need to sign the CLA in order to contribute... |
…everse#7609) * Use existing 6MiB file for S3 multipart copy test on Azure ADLS2 Fixes treeverse#7485: it just takes too long to upload some _new_ data there. * [bug] Use correct path for existing 6 MiB object on ADLS gen2
* Configurable persistence of friendly name to KV * Update docs and sample config with new property
* [Docs] Improve consent visibility * Optimize resource loading for a better user experience (LCP)
* Bump ubuntu version for actions * Fix changes * test * test2 * test3 * test4 * Good for now --------- Co-authored-by: Nir Ozery <nir.ozery@treeverse.io>
…reeverse#7708) * Document the correct Spark client version (0.13.0, maybe assembled) * [CR] [bug] Fix tab heading Tab headings don't support `` `...` `` so don't use that there. Also add words how to use the assembled JAR with spark-shell and friends. * [bug] Avoid indentation in <div>-tabs notation
* make sure creds are refreshed before expiry * small fix * small fix * small fix
* Task: Create Rust API Client * Add linguist-generated * CR Fixes
* lakectl - Support Retries
* Changelog v1.21.0
* Remove linked addresses from KV * CR Fixes * Add changelog * Change delimiter and encoding * More fixes * More fixes 2 * More More Fixes * Update esti/lakectl_util.go Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io> * Fix azure regex --------- Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>
* Add a section about the Cloud Scalability Model * Update lakefs-cloud.md * Apply suggestions from code review Co-authored-by: N-o-Z <ozery.nir@gmail.com> * Update lakefs-cloud.md --------- Co-authored-by: N-o-Z <ozery.nir@gmail.com>
* Organize Enterprise docs * PR fixes * Fix Enterprise docs * idp
* Update lakectl abuse to use set/link physical address * CR Fixes * Fix test
* Update CHANGELOG.md * Update CHANGELOG.md Co-authored-by: itaiad200 <itaiad200@gmail.com> --------- Co-authored-by: itaiad200 <itaiad200@gmail.com>
* Support presigned image URLs in markdown * Migrate from react-markdown to rehype-react + remark-rehype
* Fix background staging token behaviour * fix
* Change MaxConnectionsPerHost DDB * Change config param name
* Add Mirroring to the list of lakeFS Cloud features * add deprecation notice for Unity Delta sharing
d969f50
to
daf8869
Compare
I think I just screwed up this PR when I tried to fix email on my commits, I will just create another PR for it. |
Closes #(Insert issue number closed by this PR)
Change Description
Background
Provide the support of GCS encryption for both CMEK and CSEK
New Feature
Issue link: #7557
Testing Details
How were the changes tested?
Breaking Change?
Does this change break any existing functionality? (API, CLI, Clients)
No breaking change as there's no API changed, only the server configuration is required
Additional info
Logs, outputs, screenshots of changes if applicable (CLI / GUI changes)
Contact Details
By GitHub account