-
Notifications
You must be signed in to change notification settings - Fork 336
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
Support key version with KMS names to correctly recognize when to skip re-encryption #1705
base: master
Are you sure you want to change the base?
Conversation
… when to skip re-encryption
gslib/utils/encryption_helper.py
Outdated
@@ -34,7 +34,8 @@ | |||
lambda: re.compile('projects/([^/]+)/' | |||
'locations/([a-zA-Z0-9_-]{1,63})/' | |||
'keyRings/([a-zA-Z0-9_-]{1,63})/' | |||
'cryptoKeys/([a-zA-Z0-9_-]{1,63})$')) | |||
'cryptoKeys/([a-zA-Z0-9_-]{1,63})/' | |||
'cryptoKeyVersions/([0-9]{1,63})$')) |
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.
Looks like the ValidateCMEK function seems to be getting used at multiple places and not every time we pass a key with CryptoKeyVersions (e.g. if you are reading it from boto config)?. I have a feeling that this might break other workflows.
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.
You're right, it will break a lot of things that rely on the CryptoKeyWrapper
class.
Thanks for the PR! Can you please verify some of the other use cases that rely on this function? |
Since this bug has to do specifically with |
The metadata received from
GetObjectMetadata()
at https://github.com/GoogleCloudPlatform/gsutil/blob/master/gslib/commands/rewrite.py#LL399C1-L403C5 is used to determine if the object's current encryption is CSEK or SMEK, and compared withdest_encryption_kms_key
:The issue here is that GetObjectMetadata() returns the fully-qualified KMS key name, including the version number. Here is sample debugging output from one of my local runs:
encryption_unchanged
will thus never beTrue
for objects with CMEK, and therewrite -k
command will always re-encrypt the object despite having the same KMS key.To reproduce:
$ gsutil cp ./mysecretfile.txt gs://my-bucket
rewrite
to encrypt it with CMEK.storage.objects.rewrite
gets called as expected:storage.objects.rewrite
called again:Contrast this with a user-generated CSEK instead, where it skips the rewrite second time round:
Modifying
ValidateCMEK()
to accept version numbers as part of the KMS name fixes this. Public docs already mention using the "fully-qualified" KMS key name, which includes the version number anyway.