Skip to content

Commit

Permalink
Scope down dataset sharing requester IAM role managed IAM policy S3 p…
Browse files Browse the repository at this point in the history
…ermissions (#1280)

### Feature or Bugfix
- Bugfix

### Detail
This PR will address the issue - 1226

### Relates


### 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)? N/A
  - Is the input sanitized? N/A
- What precautions are you taking before deserializing the data you
consume? N/A
  - Is injection prevented by parametrizing queries? N/A
  - Have you ensured no `eval` or similar functions are used? N/A
- Does this PR introduce any functionality or component that requires
authorization? N/A
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
N/A
  - Are you logging failed auth attempts? N/A
- Are you using or adding any cryptographic features? N/A
  - Do you use a standard proven implementations? N/A
- Are the used keys controlled by the customer? Where are they stored?
N/A
- Are you introducing any new policies/roles/users? No
- Have you used the least-privilege principle? How?Yes, restricted the
s3:* permissions to s3 readonly permissions


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

---------

Co-authored-by: Mourya Darivemula <mouryacd@amazon.com>
  • Loading branch information
mourya-33 and Mourya Darivemula authored Jun 3, 2024
1 parent 1a4a27d commit 3557b98
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,15 @@ def add_missing_resources_to_policy_statement(
index = self._get_statement_by_sid(policy_document, statement_sid)
if index is None:
log.info(f'{statement_sid} does NOT exists for Managed policy {policy_name} ' f'creating statement...')
policy_actions = (
[f'{resource_type}:List*', f'{resource_type}:Describe*', f'{resource_type}:GetObject']
if resource_type == 's3'
else [f'{resource_type}:*']
)
additional_policy = {
'Sid': statement_sid,
'Effect': 'Allow',
'Action': [f'{resource_type}:*'],
'Action': policy_actions,
'Resource': target_resources,
}
policy_document['Statement'].append(additional_policy)
Expand Down Expand Up @@ -209,7 +214,7 @@ def _update_policy_resources_from_inline_policy(self, policy, statement_sid, exi
additional_policy = {
'Sid': f'{statement_sid}S3',
'Effect': 'Allow',
'Action': ['s3:*', 's3:List*', 's3:Describe*'],
'Action': ['s3:List*', 's3:Describe*', 's3:GetObject'],
'Resource': existing_s3,
}
policy['Statement'].append(additional_policy)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ def test_grant_target_role_access_policy_test_empty_policy(
{
'Sid': f'{IAM_S3_ACCESS_POINTS_STATEMENT_SID}S3',
'Effect': 'Allow',
'Action': ['s3:*'],
'Action': ['s3:List*', 's3:Describe*', 's3:GetObject'],
'Resource': [
f'arn:aws:s3:::{location1.S3BucketName}',
f'arn:aws:s3:::{location1.S3BucketName}/*',
Expand Down
8 changes: 6 additions & 2 deletions tests/modules/datasets/tasks/test_s3_bucket_share_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,9 @@ def test_grant_s3_iam_access_with_no_policy(mocker, dataset2, share2_manager):
assert len(iam_policy['Statement'][kms_index]['Resource']) == 1
assert (
f'arn:aws:s3:::{dataset2.S3BucketName}' in iam_policy['Statement'][s3_index]['Resource']
and 's3:*' in iam_policy['Statement'][s3_index]['Action']
and 's3:List*' in iam_policy['Statement'][s3_index]['Action']
and 's3:Describe*' in iam_policy['Statement'][s3_index]['Action']
and 's3:GetObject' in iam_policy['Statement'][s3_index]['Action']
)
assert (
f'arn:aws:kms:{dataset2.region}:{dataset2.AwsAccountId}:key/kms-key'
Expand Down Expand Up @@ -519,7 +521,9 @@ def test_grant_s3_iam_access_with_empty_policy(mocker, dataset2, share2_manager)
assert len(iam_policy['Statement'][kms_index]['Resource']) == 1
assert (
f'arn:aws:s3:::{dataset2.S3BucketName}' in iam_policy['Statement'][s3_index]['Resource']
and 's3:*' in iam_policy['Statement'][s3_index]['Action']
and 's3:List*' in iam_policy['Statement'][s3_index]['Action']
and 's3:Describe*' in iam_policy['Statement'][s3_index]['Action']
and 's3:GetObject' in iam_policy['Statement'][s3_index]['Action']
)
assert (
f'arn:aws:kms:{dataset2.region}:{dataset2.AwsAccountId}:key/kms-key'
Expand Down

0 comments on commit 3557b98

Please sign in to comment.