Skip to content

Commit

Permalink
Fix: add sharing guardrails drop permissions (data-dot-all#1055)
Browse files Browse the repository at this point in the history
### Feature or Bugfix
- Feature
- Bugfix

### Detail
This PR should be tested and reviewed after data-dot-all#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 data-dot-all#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
- data-dot-all#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.
  • Loading branch information
dlpzx authored Feb 19, 2024
1 parent b6449d1 commit 44dcb85
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
25 changes: 25 additions & 0 deletions tests/modules/datasets/tasks/test_lf_share_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 44dcb85

Please sign in to comment.