-
Notifications
You must be signed in to change notification settings - Fork 342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Provide support for AWS S3 Public Access Blocking #171
Changes from 2 commits
e95e81a
322f4be
f5a21c0
7dcf2a1
78f228e
3bcabc9
95e4b56
276a0f0
98092b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -89,6 +89,13 @@ | |||||||||||||
description: KMS master key ID to use for the default encryption. This parameter is allowed if encryption is aws:kms. If | ||||||||||||||
not specified then it will default to the AWS provided KMS key. | ||||||||||||||
type: str | ||||||||||||||
public_access: | ||||||||||||||
description: | ||||||||||||||
- Configure public access block for S3 bucket | ||||||||||||||
- supported keys [ 'BlockPublicAcls', 'IgnorePublicAcls', 'BlockPublicPolicy', 'RestrictPublicBuckets' ] | ||||||||||||||
- allowed values 'true/false' | ||||||||||||||
- keys that are not explicitely defined defaults to 'false' | ||||||||||||||
type: dict | ||||||||||||||
extends_documentation_fragment: | ||||||||||||||
- amazon.aws.aws | ||||||||||||||
- amazon.aws.ec2 | ||||||||||||||
|
@@ -153,6 +160,17 @@ | |||||||||||||
name: mys3bucket | ||||||||||||||
state: present | ||||||||||||||
encryption: "aws:kms" | ||||||||||||||
|
||||||||||||||
# Create a bucket with custom public policy block configuration | ||||||||||||||
- amazon.aws.s3_bucket: | ||||||||||||||
name: mys3bucket | ||||||||||||||
state: present | ||||||||||||||
public_access: | ||||||||||||||
BlockPublicAcls: true | ||||||||||||||
IgnorePublicAcls: true | ||||||||||||||
## keys == 'false' can be ommited, undefined keys defaults to 'false' | ||||||||||||||
# BlockPublicPolicy: false | ||||||||||||||
# RestrictPublicBuckets: false | ||||||||||||||
''' | ||||||||||||||
|
||||||||||||||
import json | ||||||||||||||
|
@@ -188,6 +206,7 @@ def create_or_update_bucket(s3_client, module, location): | |||||||||||||
versioning = module.params.get("versioning") | ||||||||||||||
encryption = module.params.get("encryption") | ||||||||||||||
encryption_key_id = module.params.get("encryption_key_id") | ||||||||||||||
public_access = sanitize_public_access_parameter(module.params.get("public_access")) | ||||||||||||||
changed = False | ||||||||||||||
result = {} | ||||||||||||||
|
||||||||||||||
|
@@ -356,6 +375,21 @@ def create_or_update_bucket(s3_client, module, location): | |||||||||||||
|
||||||||||||||
result['encryption'] = current_encryption | ||||||||||||||
|
||||||||||||||
# Public access configuration | ||||||||||||||
try: | ||||||||||||||
current_public_access = get_bucket_public_access(s3_client, name) | ||||||||||||||
except (ClientError, BotoCoreError) as err_public_access: | ||||||||||||||
module.fail_json_aws(err_public_access, msg="Failed to get bucket public access configuration") | ||||||||||||||
|
||||||||||||||
if public_access is not None: | ||||||||||||||
if current_public_access == public_access: | ||||||||||||||
result['public_access_block'] = current_public_access | ||||||||||||||
else: | ||||||||||||||
put_bucket_public_access(s3_client, name, public_access) | ||||||||||||||
changed = True | ||||||||||||||
result['public_access_block'] = public_access | ||||||||||||||
|
||||||||||||||
# Module exit | ||||||||||||||
module.exit_json(changed=changed, name=name, **result) | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
|
@@ -487,6 +521,14 @@ def delete_bucket(s3_client, bucket_name): | |||||||||||||
pass | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
@AWSRetry.exponential_backoff(max_delay=120, catch_extra_error_codes=['NoSuchBucket', 'OperationAborted']) | ||||||||||||||
def put_bucket_public_access(s3_client, bucket_name, public_acces): | ||||||||||||||
''' | ||||||||||||||
Put new public access block to S3 bucket | ||||||||||||||
''' | ||||||||||||||
s3_client.put_public_access_block(Bucket=bucket_name, PublicAccessBlockConfiguration=public_acces) | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def wait_policy_is_applied(module, s3_client, bucket_name, expected_policy, should_fail=True): | ||||||||||||||
for dummy in range(0, 12): | ||||||||||||||
try: | ||||||||||||||
|
@@ -580,6 +622,36 @@ def get_current_bucket_tags_dict(s3_client, bucket_name): | |||||||||||||
return boto3_tag_list_to_ansible_dict(current_tags) | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def get_bucket_public_access(s3_client, bucket_name): | ||||||||||||||
''' | ||||||||||||||
Get current bucket public access block | ||||||||||||||
''' | ||||||||||||||
try: | ||||||||||||||
current_public_access = s3_client.get_public_access_block(Bucket=bucket_name) | ||||||||||||||
return current_public_access['PublicAccessBlockConfiguration'] | ||||||||||||||
except is_boto3_error_code('NoSuchPublicAccessBlockConfiguration'): | ||||||||||||||
return {} | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def sanitize_public_access_parameter(public_access_block): | ||||||||||||||
''' | ||||||||||||||
Sanitize public access block - make sure that only supported keys are defined with proper values | ||||||||||||||
''' | ||||||||||||||
sanitized_block = {'BlockPublicAcls': False, 'IgnorePublicAcls': False, 'BlockPublicPolicy': False, 'RestrictPublicBuckets': False} | ||||||||||||||
|
||||||||||||||
if public_access_block is not None: | ||||||||||||||
for key in public_access_block: | ||||||||||||||
if str(key) in sanitized_block: | ||||||||||||||
val = str(public_access_block[key]).lower() | ||||||||||||||
if val == 'true': | ||||||||||||||
sanitized_block[key] = True | ||||||||||||||
else: | ||||||||||||||
sanitized_block[key] = False | ||||||||||||||
return sanitized_block | ||||||||||||||
else: | ||||||||||||||
return(None) | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def paginated_list(s3_client, **pagination_params): | ||||||||||||||
pg = s3_client.get_paginator('list_objects_v2') | ||||||||||||||
for page in pg.paginate(**pagination_params): | ||||||||||||||
|
@@ -691,7 +763,8 @@ def main(): | |||||||||||||
versioning=dict(type='bool'), | ||||||||||||||
ceph=dict(default=False, type='bool'), | ||||||||||||||
encryption=dict(choices=['none', 'AES256', 'aws:kms']), | ||||||||||||||
encryption_key_id=dict() | ||||||||||||||
encryption_key_id=dict(), | ||||||||||||||
public_access=dict(type='dict') | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This can be converted to the format used by boto3 using snake_dict_to_camel_dict (and back again using camel_dict_to_snake_dict) these functions can be found in ansible.module_utils.common.dict_transformations There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, that will be much better. I was not aware about this 'dict in dict'. |
||||||||||||||
) | ||||||||||||||
|
||||||||||||||
required_by = dict( | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ dotted | |
tags | ||
encryption_kms | ||
encryption_sse | ||
public_access | ||
|
||
[all:vars] | ||
ansible_connection=local | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
--- | ||
- module_defaults: | ||
group/aws: | ||
aws_access_key: "{{ aws_access_key }}" | ||
aws_secret_key: "{{ aws_secret_key }}" | ||
security_token: "{{ security_token | default(omit) }}" | ||
region: "{{ aws_region }}" | ||
block: | ||
|
||
# ============================================================ | ||
|
||
- name: 'Create a simple bucket with public access block configuration' | ||
s3_bucket: | ||
name: '{{ bucket_name }}' | ||
state: present | ||
public_access: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please also add a test to the 'basic' suite to make sure your PR doesn't change the default behaviour. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think I get this. Can you please explain in further detail? I don't see any file named basic in tests/. I've followed what's already in place for 'encryption_kms' test. It's in inventory and in tasks/. Thanks in advance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simplest version would be to tweak https://github.com/ansible-collections/amazon.aws/blob/main/tests/integration/targets/s3_bucket/roles/s3_bucket/tasks/simple.yml and make sure that the returned values for public_access match those of Amazon's defaults if you didn't pass public_access. The intention is to ensure that in adding the new public_access option you've not changed the default behaviour when someone creates a bucket. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, makes perfect sense now:) TY. |
||
BlockPublicAcls: true | ||
BlockPublicPolicy: true | ||
IgnorePublicAcls: true | ||
RestrictPublicBuckets: true | ||
register: output | ||
|
||
- name: 'Re-configure public access block configuration' | ||
s3_bucket: | ||
name: '{{ bucket_name }}' | ||
state: present | ||
public_access: | ||
BlockPublicAcls: true | ||
BlockPublicPolicy: false | ||
IgnorePublicAcls: true | ||
RestrictPublicBuckets: false | ||
register: output | ||
|
||
- assert: | ||
that: | ||
- output.changed | ||
- output.public_access_block | ||
- not output.public_access_block.BlockPublicPolicy | ||
- not output.public_access_block.RestrictPublicBuckets | ||
|
||
- name: 'Re-configure public access block configuration (idempotency)' | ||
s3_bucket: | ||
name: '{{ bucket_name }}' | ||
state: present | ||
public_access: | ||
BlockPublicAcls: true | ||
BlockPublicPolicy: false | ||
IgnorePublicAcls: true | ||
RestrictPublicBuckets: false | ||
register: output | ||
|
||
- assert: | ||
that: | ||
- output is not changed | ||
- output.public_access_block | ||
- not output.public_access_block.BlockPublicPolicy | ||
- not output.public_access_block.RestrictPublicBuckets | ||
|
||
# ============================================================ | ||
|
||
- name: Delete testing s3 bucket | ||
s3_bucket: | ||
name: '{{ bucket_name }}' | ||
state: absent | ||
register: output | ||
|
||
- assert: | ||
that: | ||
- output.changed | ||
|
||
# ============================================================ | ||
always: | ||
- name: Ensure all buckets are deleted | ||
s3_bucket: | ||
name: '{{ bucket_name }}' | ||
state: absent | ||
ignore_errors: yes |
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.
This would be better handled as "suboptions" on the parameter, will show how below.