From 44dcb85736434b533198137c5e2725e27b8d7153 Mon Sep 17 00:00:00 2001 From: dlpzx <71252798+dlpzx@users.noreply.github.com> Date: Mon, 19 Feb 2024 15:43:44 +0100 Subject: [PATCH] Fix: add sharing guardrails drop permissions (#1055) ### Feature or Bugfix - Feature - Bugfix ### Detail This PR should be tested and reviewed after #1016 is merged. data.all pivot role is a Data Lake admin in Lake Formation. However, to drop tables and databases "DROP" permissions are required, even for Data Lake Admins. In order for the revoke processes to work correctly, we need to ensure that these permissions are granted to the pivot role for data sharing glue resources (shared database and resource link tables) in all situations. One of this scenarios is the migration from manual to auto-created pivot roles and vice-versa as reported in #1053 . Other cases involve manual actions on existing resource links. To avoid any potential issue and make the revoke more robust, this PR explicitly grants DROP permissions on tables before deleting them. And grants "ALL" permissions on the shared_db database. Tested locally: - Create 2 environments with manually created pivot role and create, submit and approve a share request with tables. - [X] Change the configuration to use auto-created pivot roles and revoke the share. The tables are revoked successfully ### Relates - #1053 ### Security Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --- .../share_managers/lf_share_manager.py | 15 +++++++++++ .../lakeformation_process_share.py | 2 ++ .../datasets/tasks/test_lf_share_manager.py | 25 +++++++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/backend/dataall/modules/dataset_sharing/services/share_managers/lf_share_manager.py b/backend/dataall/modules/dataset_sharing/services/share_managers/lf_share_manager.py index 94e629238..b3f2eab95 100644 --- a/backend/dataall/modules/dataset_sharing/services/share_managers/lf_share_manager.py +++ b/backend/dataall/modules/dataset_sharing/services/share_managers/lf_share_manager.py @@ -202,6 +202,21 @@ def grant_pivot_role_all_database_permissions_to_shared_database(self) -> True: ) return True + def grant_pivot_role_drop_permissions_to_resource_link_table(self, table: DatasetTable) -> True: + """ + Grants 'DROP' Lake Formation permissions to pivot role to the resource link table in target account + :param table: DatasetTable + :return: True if it is successful + """ + self.lf_client_in_target.grant_permissions_to_table( + principals=[SessionHelper.get_delegation_role_arn(self.target_environment.AwsAccountId)], + database_name=self.shared_db_name, + table_name=table.GlueTableName, + catalog_id=self.target_environment.AwsAccountId, + permissions=['DROP'] + ) + return True + def grant_principals_database_permissions_to_shared_database(self) -> True: """ Grants 'DESCRIBE' Lake Formation permissions to share principals to the shared database in target account diff --git a/backend/dataall/modules/dataset_sharing/services/share_processors/lakeformation_process_share.py b/backend/dataall/modules/dataset_sharing/services/share_processors/lakeformation_process_share.py index 1406e6eaf..f60a57e02 100644 --- a/backend/dataall/modules/dataset_sharing/services/share_processors/lakeformation_process_share.py +++ b/backend/dataall/modules/dataset_sharing/services/share_processors/lakeformation_process_share.py @@ -155,6 +155,7 @@ def process_revoked_shares(self) -> bool: '##### Starting Revoking tables #######' ) success = True + self.grant_pivot_role_all_database_permissions_to_shared_database() for table in self.revoked_tables: share_item = ShareObjectRepository.find_sharable_item( self.session, self.share.shareUri, table.tableUri @@ -182,6 +183,7 @@ def process_revoked_shares(self) -> bool: if (self.is_new_share and not other_table_shares_in_env) or not self.is_new_share: warn('self.is_new_share will be deprecated in v2.6.0', DeprecationWarning, stacklevel=2) + self.grant_pivot_role_drop_permissions_to_resource_link_table(table) self.delete_resource_link_table_in_shared_database(table) if not other_table_shares_in_env: diff --git a/tests/modules/datasets/tasks/test_lf_share_manager.py b/tests/modules/datasets/tasks/test_lf_share_manager.py index 54a39ca85..cb7c48de6 100644 --- a/tests/modules/datasets/tasks/test_lf_share_manager.py +++ b/tests/modules/datasets/tasks/test_lf_share_manager.py @@ -445,6 +445,31 @@ def test_grant_principals_permissions_to_resource_link_table( permissions=['DESCRIBE'] ) + +def test_grant_pivot_role_drop_permissions_to_resource_link_table( + processor_with_mocks, + table1: DatasetTable, + target_environment: Environment, + mocker +): + processor, lf_client, glue_client = processor_with_mocks + mocker.patch( + "dataall.base.aws.sts.SessionHelper.get_delegation_role_arn", + return_value="arn:role", + ) + # When + processor.grant_pivot_role_drop_permissions_to_resource_link_table(table1) + # Then + lf_client.grant_permissions_to_table.assert_called_once() + lf_client.grant_permissions_to_table.assert_called_with( + principals=["arn:role"], + database_name=processor.shared_db_name, + table_name=table1.GlueTableName, + catalog_id=target_environment.AwsAccountId, + permissions=['DROP'] + ) + + def test_grant_principals_permissions_to_table_in_target( processor_with_mocks, table1: DatasetTable,