From 8d0fe9935c8bd9ac3b7b2cbc8caefe99bfc4f936 Mon Sep 17 00:00:00 2001 From: HemangChothani Date: Thu, 21 May 2020 18:49:30 +0530 Subject: [PATCH 1/3] fix(storage): fix upload object with bucket cmek enabled --- google/cloud/storage/blob.py | 10 ++++++++-- tests/system/test_system.py | 24 ++++++++++++++++++++++++ tests/unit/test_blob.py | 25 +++++++++++++++++++++++-- 3 files changed, 55 insertions(+), 4 deletions(-) diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py index 9567c1096..0588e231d 100644 --- a/google/cloud/storage/blob.py +++ b/google/cloud/storage/blob.py @@ -1280,7 +1280,10 @@ def _do_multipart_upload( if self.user_project is not None: name_value_pairs.append(("userProject", self.user_project)) - if self.kms_key_name is not None: + if ( + self.kms_key_name is not None + and "cryptoKeyVersions" not in self.kms_key_name + ): name_value_pairs.append(("kmsKeyName", self.kms_key_name)) if predefined_acl is not None: @@ -1417,7 +1420,10 @@ def _initiate_resumable_upload( if self.user_project is not None: name_value_pairs.append(("userProject", self.user_project)) - if self.kms_key_name is not None: + if ( + self.kms_key_name is not None + and "cryptoKeyVersions" not in self.kms_key_name + ): name_value_pairs.append(("kmsKeyName", self.kms_key_name)) if predefined_acl is not None: diff --git a/tests/system/test_system.py b/tests/system/test_system.py index c96b10ce1..ee26fc97b 100644 --- a/tests/system/test_system.py +++ b/tests/system/test_system.py @@ -1966,6 +1966,30 @@ def test_rewrite_rotate_csek_to_cmek(self): self.assertEqual(dest.download_as_string(), source_data) + def test_upload_new_blob_w_bucket_cmek_enabled(self): + blob_name = "test-blob" + payload = b"DEADBEEF" + alt_payload = b"NEWDEADBEEF" + + kms_key_name = self._kms_key_name() + self.bucket.default_kms_key_name = kms_key_name + self.bucket.patch() + self.assertEqual(self.bucket.default_kms_key_name, kms_key_name) + + blob = self.bucket.blob(blob_name) + blob.upload_from_string(payload) + # We don't know the current version of the key. + self.assertTrue(blob.kms_key_name.startswith(kms_key_name)) + + blob.upload_from_string(alt_payload, if_generation_match=blob.generation) + self.case_blobs_to_delete.append(blob) + + self.assertEqual(blob.download_as_string(), alt_payload) + + self.bucket.default_kms_key_name = None + self.bucket.patch() + self.assertIsNone(self.bucket.default_kms_key_name) + class TestRetentionPolicy(unittest.TestCase): def setUp(self): diff --git a/tests/unit/test_blob.py b/tests/unit/test_blob.py index 94822b93a..0a5bfd9d8 100644 --- a/tests/unit/test_blob.py +++ b/tests/unit/test_blob.py @@ -1525,7 +1525,7 @@ def _do_multipart_success( if predefined_acl is not None: qs_params.append(("predefinedAcl", predefined_acl)) - if kms_key_name is not None: + if kms_key_name is not None and "cryptoKeyVersions" not in kms_key_name: qs_params.append(("kmsKeyName", kms_key_name)) if if_generation_match is not None: @@ -1579,6 +1579,17 @@ def test__do_multipart_upload_with_kms(self, mock_get_boundary): ) self._do_multipart_success(mock_get_boundary, kms_key_name=kms_resource) + @mock.patch(u"google.resumable_media._upload.get_boundary", return_value=b"==0==") + def test__do_multipart_upload_with_kms_with_version(self, mock_get_boundary): + kms_resource = ( + "projects/test-project-123/" + "locations/us/" + "keyRings/test-ring/" + "cryptoKeys/test-key" + "cryptoKeyVersions/1" + ) + self._do_multipart_success(mock_get_boundary, kms_key_name=kms_resource) + @mock.patch(u"google.resumable_media._upload.get_boundary", return_value=b"==0==") def test__do_multipart_upload_with_retry(self, mock_get_boundary): self._do_multipart_success(mock_get_boundary, num_retries=8) @@ -1685,7 +1696,7 @@ def _initiate_resumable_helper( if predefined_acl is not None: qs_params.append(("predefinedAcl", predefined_acl)) - if kms_key_name is not None: + if kms_key_name is not None and "cryptoKeyVersions" not in kms_key_name: qs_params.append(("kmsKeyName", kms_key_name)) if if_generation_match is not None: @@ -1770,6 +1781,16 @@ def test__initiate_resumable_upload_with_kms(self): ) self._initiate_resumable_helper(kms_key_name=kms_resource) + def test__initiate_resumable_upload_with_kms_with_version(self): + kms_resource = ( + "projects/test-project-123/" + "locations/us/" + "keyRings/test-ring/" + "cryptoKeys/test-key" + "cryptoKeyVersions/1" + ) + self._initiate_resumable_helper(kms_key_name=kms_resource) + def test__initiate_resumable_upload_without_chunk_size(self): self._initiate_resumable_helper(blob_chunk_size=None) From ba92f581c3d741afe4f5ca080f02324446433ada Mon Sep 17 00:00:00 2001 From: HemangChothani Date: Fri, 22 May 2020 13:56:45 +0530 Subject: [PATCH 2/3] fix(storage): add comments for condition used --- google/cloud/storage/blob.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py index 0588e231d..c5f9da168 100644 --- a/google/cloud/storage/blob.py +++ b/google/cloud/storage/blob.py @@ -1280,6 +1280,10 @@ def _do_multipart_upload( if self.user_project is not None: name_value_pairs.append(("userProject", self.user_project)) + # When cmek is enabled for bucket blob.metadata stores the value of + # kmsKeyName with version information whereas upload method expect + # information without version so this condition ignores the kmsKeyName + # which has cryptoKeyVersions. if ( self.kms_key_name is not None and "cryptoKeyVersions" not in self.kms_key_name @@ -1420,6 +1424,10 @@ def _initiate_resumable_upload( if self.user_project is not None: name_value_pairs.append(("userProject", self.user_project)) + # When cmek is enabled for bucket blob.metadata stores the value of + # kmsKeyName with version information whereas upload method expect + # information without version so this condition ignores the kmsKeyName + # which has cryptoKeyVersions. if ( self.kms_key_name is not None and "cryptoKeyVersions" not in self.kms_key_name From 71935adb1c3d1439ca9a65e85be77d4e2662272d Mon Sep 17 00:00:00 2001 From: HemangChothani Date: Fri, 22 May 2020 17:08:17 +0530 Subject: [PATCH 3/3] fix(storage): nit --- google/cloud/storage/blob.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/google/cloud/storage/blob.py b/google/cloud/storage/blob.py index c5f9da168..ab50a8f72 100644 --- a/google/cloud/storage/blob.py +++ b/google/cloud/storage/blob.py @@ -1280,10 +1280,11 @@ def _do_multipart_upload( if self.user_project is not None: name_value_pairs.append(("userProject", self.user_project)) - # When cmek is enabled for bucket blob.metadata stores the value of - # kmsKeyName with version information whereas upload method expect - # information without version so this condition ignores the kmsKeyName - # which has cryptoKeyVersions. + # When a Customer Managed Encryption Key is used to encrypt Cloud Storage object + # at rest, object resource metadata will store the version of the Key Management + # Service cryptographic material. If a Blob instance with KMS Key metadata set is + # used to upload a new version of the object then the existing kmsKeyName version + # value can't be used in the upload request and the client instead ignores it. if ( self.kms_key_name is not None and "cryptoKeyVersions" not in self.kms_key_name @@ -1424,10 +1425,11 @@ def _initiate_resumable_upload( if self.user_project is not None: name_value_pairs.append(("userProject", self.user_project)) - # When cmek is enabled for bucket blob.metadata stores the value of - # kmsKeyName with version information whereas upload method expect - # information without version so this condition ignores the kmsKeyName - # which has cryptoKeyVersions. + # When a Customer Managed Encryption Key is used to encrypt Cloud Storage object + # at rest, object resource metadata will store the version of the Key Management + # Service cryptographic material. If a Blob instance with KMS Key metadata set is + # used to upload a new version of the object then the existing kmsKeyName version + # value can't be used in the upload request and the client instead ignores it. if ( self.kms_key_name is not None and "cryptoKeyVersions" not in self.kms_key_name