Bugfix 956 #961
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,8 @@ | |
| IAM_ACCESS_POINT_ROLE_POLICY = "targetDatasetAccessControlPolicy" | ||
| DATAALL_ALLOW_OWNER_SID = "AllowAllToAdmin" | ||
| DATAALL_ACCESS_POINT_KMS_DECRYPT_SID = "DataAll-Access-Point-KMS-Decrypt" | ||
| DATAALL_ACCESS_POINT_ENABLE_PIVOT_ROLE_PERMISSIONS_SID = "DataAll-Access-Point-Enable-Pivot-Role-Permissions" | ||
|
|
||
|
|
||
|
|
||
| class S3AccessPointShareManager: | ||
|
|
@@ -331,14 +333,24 @@ def update_dataset_bucket_key_policy(self): | |
| ) | ||
| key_alias = f"alias/{self.dataset.KmsAlias}" | ||
| kms_client = KmsClient(self.source_account_id, self.source_environment.region) | ||
| kms_key_id = kms_client.get_key_id_using_list_aliases(key_alias) | ||
| existing_policy = kms_client.get_key_policy(kms_key_id) | ||
| target_requester_arn = IAM.get_role_arn_by_name(self.target_account_id, self.target_requester_IAMRoleName) | ||
|
|
||
| if existing_policy: | ||
| existing_policy = json.loads(existing_policy) | ||
| counter = count() | ||
| statements = {item.get("Sid", next(counter)): item for item in existing_policy.get("Statement", {})} | ||
|
|
||
| if DATAALL_ACCESS_POINT_ENABLE_PIVOT_ROLE_PERMISSIONS_SID in statements.keys(): | ||
| logger.info( | ||
| f'KMS key policy already contains share statement {DATAALL_ACCESS_POINT_ENABLE_PIVOT_ROLE_PERMISSIONS_SID}') | ||
| else: | ||
| logger.info( | ||
| f'KMS key policy does not contain statement {DATAALL_ACCESS_POINT_ENABLE_PIVOT_ROLE_PERMISSIONS_SID}, generating a new one') | ||
| statements[DATAALL_ACCESS_POINT_ENABLE_PIVOT_ROLE_PERMISSIONS_SID] \ | ||
| = self.generate_enable_pivot_role_permissions_policy_statement(self.dataset_account_id) | ||
|
|
||
| if DATAALL_ACCESS_POINT_KMS_DECRYPT_SID in statements.keys(): | ||
| logger.info( | ||
| f'KMS key policy contains share statement {DATAALL_ACCESS_POINT_KMS_DECRYPT_SID}, ' | ||
|
|
@@ -353,12 +365,14 @@ def update_dataset_bucket_key_policy(self): | |
| statements[DATAALL_ACCESS_POINT_KMS_DECRYPT_SID] = (self.generate_default_kms_decrypt_policy_statement | ||
| (target_requester_arn)) | ||
| existing_policy["Statement"] = list(statements.values()) | ||
|
|
||
| else: | ||
| logger.info('KMS key policy does not contain any statements, generating a new one') | ||
| existing_policy = { | ||
| "Version": "2012-10-17", | ||
| "Statement": [ | ||
| self.generate_default_kms_decrypt_policy_statement(target_requester_arn), | ||
| self.generate_enable_pivot_role_permissions_policy_statement(self.dataset_account_id) | ||
| ] | ||
| } | ||
| kms_client.put_key_policy( | ||
|
|
@@ -474,7 +488,7 @@ def delete_dataset_bucket_key_policy( | |
| ) | ||
| key_alias = f"alias/{dataset.KmsAlias}" | ||
| kms_client = KmsClient(dataset.AwsAccountId, dataset.region) | ||
| kms_key_id = kms_client.get_key_id_using_list_aliases(key_alias) | ||
| existing_policy = json.loads(kms_client.get_key_policy(kms_key_id)) | ||
| target_requester_arn = IAM.get_role_arn_by_name(self.target_account_id, self.target_requester_IAMRoleName) | ||
| counter = count() | ||
|
|
@@ -485,6 +499,8 @@ def delete_dataset_bucket_key_policy( | |
| principal_list.remove(f"{target_requester_arn}") | ||
| if len(principal_list) == 0: | ||
| statements.pop(DATAALL_ACCESS_POINT_KMS_DECRYPT_SID) | ||
| if DATAALL_ACCESS_POINT_KMS_DECRYPT_SID not in statements.keys(): | ||
|
There was a problem hiding this comment. do we need this if statement? If there are no principals in We may need to keep There was a problem hiding this comment. I am popping both Statements if principal list is 0 in the next revision. There was a problem hiding this comment. testing the above while processing a bucket share worked with this PR, I did receive Do we see any issue with checking if These are the required permissions we already implicitly give to the pivotRole in IAM with our conditional statements... I do not see an issue with doing as such but let me know your thoughts @anushka-singh @dlpzx There was a problem hiding this comment. If we do want to also keep the remove logic then can we provide a default value when we do
Otherwise we may run into There was a problem hiding this comment. In my opinion it makes sense to not remove it, because the pivot role access to the key is similar to a configuration requirement of a dataset that we apply in a lazy mode. If we keep it we avoid issues with simultaneous revoke/shares that create and delete it and with Malformedpolicy errors There was a problem hiding this comment. Okay I agree! In the remove functions, I will not pop |
||
| statements.pop(DATAALL_ACCESS_POINT_ENABLE_PIVOT_ROLE_PERMISSIONS_SID) | ||
| else: | ||
| statements[DATAALL_ACCESS_POINT_KMS_DECRYPT_SID]["Principal"]["AWS"] = principal_list | ||
| existing_policy["Statement"] = list(statements.values()) | ||
|
|
@@ -510,7 +526,7 @@ def handle_share_failure(self, error: Exception) -> None: | |
| self.target_folder, self.share, self.target_environment | ||
| ) | ||
|
|
||
| def handle_revoke_failure(self, error: Exception) -> bool: | ||
| """ | ||
| Handles share failure by raising an alarm to alarmsTopic | ||
| Returns | ||
|
|
@@ -526,6 +542,7 @@ def handle_revoke_failure(self, error: Exception) -> None: | |
| DatasetAlarmService().trigger_revoke_folder_sharing_failure_alarm( | ||
| self.target_folder, self.share, self.target_environment | ||
| ) | ||
| return True | ||
|
|
||
| @staticmethod | ||
| def generate_default_kms_decrypt_policy_statement(target_requester_arn): | ||
|
|
@@ -541,6 +558,29 @@ def generate_default_kms_decrypt_policy_statement(target_requester_arn): | |
| "Resource": "*" | ||
| } | ||
|
|
||
| @staticmethod | ||
| def generate_enable_pivot_role_permissions_policy_statement(dataset_account_id): | ||
| return { | ||
| "Sid": f"{DATAALL_ACCESS_POINT_ENABLE_PIVOT_ROLE_PERMISSIONS_SID}", | ||
| "Effect": "Allow", | ||
| "Principal": { | ||
| "AWS": [ | ||
| f"arn:aws:iam::{dataset_account_id}:role/dataallPivotRole" | ||
noah-paige marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ] | ||
| }, | ||
| "Action": [ | ||
| "kms:Decrypt", | ||
| "kms:Encrypt", | ||
| "kms:GenerateDataKey*", | ||
| "kms:PutKeyPolicy", | ||
| "kms:GetKeyPolicy", | ||
| "kms:ReEncrypt*", | ||
| "kms:TagResource", | ||
| "kms:UntagResource" | ||
| ], | ||
| "Resource": "*" | ||
| } | ||
|
|
||
| def add_target_arn_to_statement_principal(self, statement, target_requester_arn): | ||
| principal_list = self.get_principal_list(statement) | ||
| if f"{target_requester_arn}" not in principal_list: | ||
|
|
||
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.
One thing I am realizing is that if a dataset has a folder share and a bucket share then there will be 2 statements enabling PivotRole Permissions in the Key Policy
Would be curious to see if there's a way to not have duplicate here but also understand having them separate is far easier to manage in terms of checking if exists and removing if no more folder and/or bucket shares
... something to think about but deploying now to test as is and confirm the above suspicion
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 had the same thought.
Here is how I think about it: Eventually we will move to having refactored code where we just have 1 file with all these functions consolidated in it. Currently there is a lot of common code between access point and bucket share that can be pulled out into a separate class. At that time, we can have just one of these pivot role auto create permissions in place. Let me know what you think!
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.
Something like a
share_managers/kms_policy_utilssimilar to theshare_manager_utils? We could have a single function that handlesupdate_dataset_bucket_key_policyfor both. I like that it reduces the places where we modify the policy to a single oneThere 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.
@dlpzx I 100% agree we should consolidate the functions into a single utils file. However, I want to do that as a part of clean up outside of this PR. If thats okay with you, I can keep update_dataset_bucket_key_policy in both access point and bucket share for now and we can extract common functions out as a part of different issue I will create.
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.
In the new revision I have used KMSPivotRolePermissions as Noah advised in both the share managers.