From 5343627c338a6ae124fb3088a417fdfd903dbcec Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 6 Apr 2021 14:55:33 +0200 Subject: [PATCH 01/11] Add permission to upload ACL in ExtraArgs --- docs/amazon.aws.aws_s3_module.rst | 2 +- plugins/modules/aws_s3.py | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/docs/amazon.aws.aws_s3_module.rst b/docs/amazon.aws.aws_s3_module.rst index 642bac916ee..e870d1d1c01 100644 --- a/docs/amazon.aws.aws_s3_module.rst +++ b/docs/amazon.aws.aws_s3_module.rst @@ -460,7 +460,7 @@ Parameters Default:
["private"]
-
This option lets the user set the canned permissions on the object/bucket that are created. The permissions that can be set are private, public-read, public-read-write, authenticated-read for a bucket or private, public-read, public-read-write, aws-exec-read, authenticated-read, bucket-owner-read, bucket-owner-full-control for an object. Multiple permissions can be specified as a list.
+
This option lets the user set the canned permissions on the object/bucket that are created. The permissions that can be set are private, public-read, public-read-write, authenticated-read for a bucket or private, public-read, public-read-write, aws-exec-read, authenticated-read, bucket-owner-read, bucket-owner-full-control for an object. Multiple permissions can be specified as a list, yet only the first one is used when first uploading the object(s).
diff --git a/plugins/modules/aws_s3.py b/plugins/modules/aws_s3.py index b0ec5d33895..bbe4b64903b 100644 --- a/plugins/modules/aws_s3.py +++ b/plugins/modules/aws_s3.py @@ -531,6 +531,13 @@ def upload_s3file(module, s3, bucket, obj, expiry, metadata, encrypt, headers, s else: extra['Metadata'][option] = metadata[option] + if module.params.get('permission'): + permissions = module.params['permission'] + if isinstance(permissions, str): + extra['ACL'] = permissions + elif isinstance(permissions, list): + extra['ACL'] = permissions[0] + if 'ContentType' not in extra: content_type = None if src is not None: From a4ec9888f5716e41a25f04a4fe91a6dc7dfc1842 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 7 Apr 2021 14:42:32 +0200 Subject: [PATCH 02/11] Add docs to aws_s3 docstring --- docs/amazon.aws.aws_s3_module.rst | 2 +- plugins/modules/aws_s3.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/amazon.aws.aws_s3_module.rst b/docs/amazon.aws.aws_s3_module.rst index e870d1d1c01..642bac916ee 100644 --- a/docs/amazon.aws.aws_s3_module.rst +++ b/docs/amazon.aws.aws_s3_module.rst @@ -460,7 +460,7 @@ Parameters Default:
["private"]
-
This option lets the user set the canned permissions on the object/bucket that are created. The permissions that can be set are private, public-read, public-read-write, authenticated-read for a bucket or private, public-read, public-read-write, aws-exec-read, authenticated-read, bucket-owner-read, bucket-owner-full-control for an object. Multiple permissions can be specified as a list, yet only the first one is used when first uploading the object(s).
+
This option lets the user set the canned permissions on the object/bucket that are created. The permissions that can be set are private, public-read, public-read-write, authenticated-read for a bucket or private, public-read, public-read-write, aws-exec-read, authenticated-read, bucket-owner-read, bucket-owner-full-control for an object. Multiple permissions can be specified as a list.
diff --git a/plugins/modules/aws_s3.py b/plugins/modules/aws_s3.py index bbe4b64903b..b0eebaf0ce0 100644 --- a/plugins/modules/aws_s3.py +++ b/plugins/modules/aws_s3.py @@ -78,7 +78,8 @@ - This option lets the user set the canned permissions on the object/bucket that are created. The permissions that can be set are C(private), C(public-read), C(public-read-write), C(authenticated-read) for a bucket or C(private), C(public-read), C(public-read-write), C(aws-exec-read), C(authenticated-read), C(bucket-owner-read), - C(bucket-owner-full-control) for an object. Multiple permissions can be specified as a list. + C(bucket-owner-full-control) for an object. Multiple permissions can be specified as a list; although only the first one + will be used during the initial upload of the file default: ['private'] type: list elements: str From f00cb89f2934a0b905dfd5d7a9089b6f807a4d29 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 7 Apr 2021 14:53:40 +0200 Subject: [PATCH 03/11] Update CHANGELOG --- CHANGELOG.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 2428b5aaa47..38e55c5c3b3 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -24,6 +24,7 @@ Minor Changes Bugfixes -------- +- aws_s3 - Fix upload permission when an S3 bucket ACL policy requires a particular canned ACL - ec2_vol - a creation or update now returns a structure with an up to date list of tags (https://github.com/ansible-collections/amazon.aws/pull/241). v1.3.0 From 5d0ac189fe344a2c2f3e3772ff2697183fbe9c26 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 7 Apr 2021 14:54:34 +0200 Subject: [PATCH 04/11] Revert "Update CHANGELOG" This reverts commit f00cb89f2934a0b905dfd5d7a9089b6f807a4d29. --- CHANGELOG.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 38e55c5c3b3..2428b5aaa47 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -24,7 +24,6 @@ Minor Changes Bugfixes -------- -- aws_s3 - Fix upload permission when an S3 bucket ACL policy requires a particular canned ACL - ec2_vol - a creation or update now returns a structure with an up to date list of tags (https://github.com/ansible-collections/amazon.aws/pull/241). v1.3.0 From 5ec1939ac588570e0d629e5b83aadfc1d95cdd06 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 7 Apr 2021 14:55:56 +0200 Subject: [PATCH 05/11] Add bugfix CHANGELOG fragment --- changelogs/fragments/318-s3-upload-acl.yml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changelogs/fragments/318-s3-upload-acl.yml diff --git a/changelogs/fragments/318-s3-upload-acl.yml b/changelogs/fragments/318-s3-upload-acl.yml new file mode 100644 index 00000000000..897c6953f1e --- /dev/null +++ b/changelogs/fragments/318-s3-upload-acl.yml @@ -0,0 +1,2 @@ +bugfixes: +- aws_s3 - Fix upload permission when an S3 bucket ACL policy requires a particular canned ACL From 7bb20b60ddf818ef434d7c8f931c53c301b55d33 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 7 Apr 2021 14:57:49 +0200 Subject: [PATCH 06/11] Add pr link to changelog fragment --- changelogs/fragments/318-s3-upload-acl.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/fragments/318-s3-upload-acl.yml b/changelogs/fragments/318-s3-upload-acl.yml index 897c6953f1e..19326ceb315 100644 --- a/changelogs/fragments/318-s3-upload-acl.yml +++ b/changelogs/fragments/318-s3-upload-acl.yml @@ -1,2 +1,2 @@ bugfixes: -- aws_s3 - Fix upload permission when an S3 bucket ACL policy requires a particular canned ACL +- aws_s3 - Fix upload permission when an S3 bucket ACL policy requires a particular canned ACL (https://github.com/ansible-collections/amazon.aws/pull/318) From d38de0d30c30b16b5fa24616d35d0bd3521ab8e4 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 7 Apr 2021 15:13:42 +0200 Subject: [PATCH 07/11] wip: create task for testing bucket ACL upload --- .../integration/targets/aws_s3/tasks/main.yml | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/integration/targets/aws_s3/tasks/main.yml b/tests/integration/targets/aws_s3/tasks/main.yml index 5d811ce1775..0e303a07378 100644 --- a/tests/integration/targets/aws_s3/tasks/main.yml +++ b/tests/integration/targets/aws_s3/tasks/main.yml @@ -526,6 +526,38 @@ - result is not changed when: ansible_system == 'Linux' or ansible_distribution == 'MacOSX' + - name: test upload to ACL-restricted bucket + block: + - name: make a bucket to upload the file + aws_s3: + bucket: "{{ bucket_name }}_with_acl" + mode: create + permission: bucket-owner-full-control + + # This should fail + - name: fail to upload the file to the bucket + aws_s3: + bucket: "{{ bucket_name }}" + mode: put + src: "{{ tmpdir.path }}/largefile" + object: multipart.txt + permission: private + delay: 3 + until: "result.msg == 'PUT operation complete'" + register: result + + # This should succeed + - name: fail to upload the file to the bucket + aws_s3: + bucket: "{{ bucket_name }}" + mode: put + src: "{{ tmpdir.path }}/largefile" + object: multipart.txt + permission: bucket-owner-full-control + delay: 3 + until: "result.msg == 'PUT operation complete'" + register: result + - name: create an object from static content aws_s3: bucket: "{{ bucket_name }}" From 0824b149ca616bc65b50b34f518deed63a522b02 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Thu, 8 Apr 2021 23:03:10 +0200 Subject: [PATCH 08/11] Create s3 bucket during ACL test with policy template --- .../targets/aws_s3/defaults/main.yml | 2 ++ .../integration/targets/aws_s3/tasks/main.yml | 31 +++++++++++++++---- .../targets/aws_s3/templates/policy.json.j2 | 21 +++++++++++++ 3 files changed, 48 insertions(+), 6 deletions(-) create mode 100644 tests/integration/targets/aws_s3/templates/policy.json.j2 diff --git a/tests/integration/targets/aws_s3/defaults/main.yml b/tests/integration/targets/aws_s3/defaults/main.yml index eb7dd2d3712..d1dabdb3de5 100644 --- a/tests/integration/targets/aws_s3/defaults/main.yml +++ b/tests/integration/targets/aws_s3/defaults/main.yml @@ -1,3 +1,5 @@ --- # defaults file for s3 bucket_name: '{{resource_prefix}}' + +bucket_name_acl: '{{ bucket_name }}_with_acl' diff --git a/tests/integration/targets/aws_s3/tasks/main.yml b/tests/integration/targets/aws_s3/tasks/main.yml index 0e303a07378..3df9b03642d 100644 --- a/tests/integration/targets/aws_s3/tasks/main.yml +++ b/tests/integration/targets/aws_s3/tasks/main.yml @@ -528,16 +528,35 @@ - name: test upload to ACL-restricted bucket block: + - name: make a bucket to upload the file - aws_s3: - bucket: "{{ bucket_name }}_with_acl" - mode: create - permission: bucket-owner-full-control + s3_bucket: + name: "{{ bucket_name_acl }}" + state: present + policy: " {{ lookup('template', 'policy.json') }}" + requester_pays: yes + versioning: yes + register: output + + - assert: + that: + - output is changed + - output.name == '{{ bucket_name_acl }}' + - output.requester_pays + - output.versioning.MfaDelete == 'Disabled' + - output.versioning.Versioning == 'Enabled' + - output.tags.example == 'tag1' + - output.tags.another == 'tag2' + - output.policy.Statement[0].Action == 's3:PutObject' + - output.policy.Statement[0].Effect == 'Allow' + - output.policy.Statement[0].Principal == '*' + - output.policy.Statement[0].Resource == 'arn:aws:s3:::{{ bucket_name }}/*' + #- output.policy.Statement[0].Condition.StringEquals. == 'AddPerm' # This should fail - name: fail to upload the file to the bucket aws_s3: - bucket: "{{ bucket_name }}" + bucket: "{{ bucket_name_acl }}" mode: put src: "{{ tmpdir.path }}/largefile" object: multipart.txt @@ -549,7 +568,7 @@ # This should succeed - name: fail to upload the file to the bucket aws_s3: - bucket: "{{ bucket_name }}" + bucket: "{{ bucket_name_acl }}" mode: put src: "{{ tmpdir.path }}/largefile" object: multipart.txt diff --git a/tests/integration/targets/aws_s3/templates/policy.json.j2 b/tests/integration/targets/aws_s3/templates/policy.json.j2 new file mode 100644 index 00000000000..210833d8801 --- /dev/null +++ b/tests/integration/targets/aws_s3/templates/policy.json.j2 @@ -0,0 +1,21 @@ +{ + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "Only allow writes to my bucket with bucket owner full control", + "Effect": "Allow", + "Principal": "*", + "Action": [ + "s3:PutObject" + ], + "Resource": [ + "arn:aws:s3:::{{ bucket_name }}/*" + ], + "Condition": { + "StringEquals": { + "s3:x-amz-acl": "bucket-owner-full-control" + } + } + } + ] +} From 522da9b60cd319805427f1ea94378df8bf2153f3 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Fri, 9 Apr 2021 06:46:35 +0200 Subject: [PATCH 09/11] Integration test fixups --- .../targets/aws_s3/defaults/main.yml | 5 +- .../integration/targets/aws_s3/tasks/main.yml | 105 ++++++++++-------- .../targets/aws_s3/templates/policy.json.j2 | 2 +- 3 files changed, 61 insertions(+), 51 deletions(-) diff --git a/tests/integration/targets/aws_s3/defaults/main.yml b/tests/integration/targets/aws_s3/defaults/main.yml index d1dabdb3de5..67d026de087 100644 --- a/tests/integration/targets/aws_s3/defaults/main.yml +++ b/tests/integration/targets/aws_s3/defaults/main.yml @@ -1,5 +1,4 @@ --- # defaults file for s3 -bucket_name: '{{resource_prefix}}' - -bucket_name_acl: '{{ bucket_name }}_with_acl' +bucket_name: '{{ resource_prefix }}' +bucket_name_acl: '{{ bucket_name }}-with-acl' diff --git a/tests/integration/targets/aws_s3/tasks/main.yml b/tests/integration/targets/aws_s3/tasks/main.yml index 3df9b03642d..c5a31726cb9 100644 --- a/tests/integration/targets/aws_s3/tasks/main.yml +++ b/tests/integration/targets/aws_s3/tasks/main.yml @@ -526,56 +526,48 @@ - result is not changed when: ansible_system == 'Linux' or ansible_distribution == 'MacOSX' - - name: test upload to ACL-restricted bucket - block: + - name: make a bucket with the bucket-owner-full-control ACL + s3_bucket: + name: "{{ bucket_name_acl }}" + state: present + policy: "{{ lookup('template', 'policy.json.j2') }}" + requester_pays: yes + versioning: yes + register: bucket_with_policy - - name: make a bucket to upload the file - s3_bucket: - name: "{{ bucket_name_acl }}" - state: present - policy: " {{ lookup('template', 'policy.json') }}" - requester_pays: yes - versioning: yes - register: output + - assert: + that: + - bucket_with_policy is changed - - assert: - that: - - output is changed - - output.name == '{{ bucket_name_acl }}' - - output.requester_pays - - output.versioning.MfaDelete == 'Disabled' - - output.versioning.Versioning == 'Enabled' - - output.tags.example == 'tag1' - - output.tags.another == 'tag2' - - output.policy.Statement[0].Action == 's3:PutObject' - - output.policy.Statement[0].Effect == 'Allow' - - output.policy.Statement[0].Principal == '*' - - output.policy.Statement[0].Resource == 'arn:aws:s3:::{{ bucket_name }}/*' - #- output.policy.Statement[0].Condition.StringEquals. == 'AddPerm' - - # This should fail - - name: fail to upload the file to the bucket - aws_s3: - bucket: "{{ bucket_name_acl }}" - mode: put - src: "{{ tmpdir.path }}/largefile" - object: multipart.txt - permission: private - delay: 3 - until: "result.msg == 'PUT operation complete'" - register: result + - name: fail to upload the file to the bucket with an ACL + aws_s3: + bucket: "{{ bucket_name_acl }}" + mode: put + src: "{{ tmpdir.path }}/upload.txt" + object: file-with-permissions.txt + permission: private + ignore_nonexistent_bucket: True + register: upload_private + ignore_errors: True - # This should succeed - - name: fail to upload the file to the bucket - aws_s3: - bucket: "{{ bucket_name_acl }}" - mode: put - src: "{{ tmpdir.path }}/largefile" - object: multipart.txt - permission: bucket-owner-full-control - delay: 3 - until: "result.msg == 'PUT operation complete'" - register: result + # XXX Doesn't fail... + # - assert: + # that: + # - upload_private is failed + + - name: upload the file to the bucket with an ACL + aws_s3: + bucket: "{{ bucket_name_acl }}" + mode: put + src: "{{ tmpdir.path }}/upload.txt" + object: file-with-permissions.txt + permission: bucket-owner-full-control + ignore_nonexistent_bucket: True + register: upload_owner + + - assert: + that: + - upload_owner is changed - name: create an object from static content aws_s3: @@ -690,9 +682,22 @@ - delete.txt - delete_encrypt.txt - delete_encrypt_kms.txt + - multipart.txt - put-content.txt - put-template.txt - put-binary.txt + - foo/bar/baz + - foo/bar + - foo + ignore_errors: yes + + - name: remove uploaded files (bucket with ACL) + aws_s3: + bucket: "{{ bucket_name_acl }}" + mode: delobj + object: "{{ item }}" + loop: + - file-with-permissions.txt ignore_errors: yes - name: delete temporary files @@ -712,3 +717,9 @@ bucket: "{{ bucket_name + '.bucket' }}" mode: delete ignore_errors: yes + + - name: delete the acl bucket + aws_s3: + bucket: "{{ bucket_name_acl }}" + mode: delete + ignore_errors: yes diff --git a/tests/integration/targets/aws_s3/templates/policy.json.j2 b/tests/integration/targets/aws_s3/templates/policy.json.j2 index 210833d8801..7f2c85a676e 100644 --- a/tests/integration/targets/aws_s3/templates/policy.json.j2 +++ b/tests/integration/targets/aws_s3/templates/policy.json.j2 @@ -9,7 +9,7 @@ "s3:PutObject" ], "Resource": [ - "arn:aws:s3:::{{ bucket_name }}/*" + "arn:aws:s3:::{{ bucket_name_acl }}/*" ], "Condition": { "StringEquals": { From c9f79dbe3eb9bfbbde6d5f4319dec112f030c9b0 Mon Sep 17 00:00:00 2001 From: Mark Chappell Date: Fri, 9 Apr 2021 14:42:22 +0200 Subject: [PATCH 10/11] Make sure bucket isn't completely public --- tests/integration/targets/aws_s3/tasks/main.yml | 8 ++++++++ tests/integration/targets/aws_s3/templates/policy.json.j2 | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/integration/targets/aws_s3/tasks/main.yml b/tests/integration/targets/aws_s3/tasks/main.yml index c5a31726cb9..68a796083ae 100644 --- a/tests/integration/targets/aws_s3/tasks/main.yml +++ b/tests/integration/targets/aws_s3/tasks/main.yml @@ -8,6 +8,14 @@ region: "{{ aws_region }}" block: + - name: get ARN of calling user + aws_caller_info: + register: aws_caller_info + + - name: register account id + set_fact: + aws_account: "{{ aws_caller_info.account }}" + - name: Create temporary directory tempfile: state: directory diff --git a/tests/integration/targets/aws_s3/templates/policy.json.j2 b/tests/integration/targets/aws_s3/templates/policy.json.j2 index 7f2c85a676e..4af2e0713b1 100644 --- a/tests/integration/targets/aws_s3/templates/policy.json.j2 +++ b/tests/integration/targets/aws_s3/templates/policy.json.j2 @@ -4,7 +4,7 @@ { "Sid": "Only allow writes to my bucket with bucket owner full control", "Effect": "Allow", - "Principal": "*", + "Principal": { "AWS":"{{ aws_account }}" }, "Action": [ "s3:PutObject" ], From 7fe54a775bfbbf86d4abd67bcc3b4311b92e1669 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 21 Apr 2021 10:12:42 +0200 Subject: [PATCH 11/11] Remove versioning from test bucket config --- tests/integration/targets/aws_s3/tasks/main.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/integration/targets/aws_s3/tasks/main.yml b/tests/integration/targets/aws_s3/tasks/main.yml index 68a796083ae..e44f80934cc 100644 --- a/tests/integration/targets/aws_s3/tasks/main.yml +++ b/tests/integration/targets/aws_s3/tasks/main.yml @@ -539,8 +539,6 @@ name: "{{ bucket_name_acl }}" state: present policy: "{{ lookup('template', 'policy.json.j2') }}" - requester_pays: yes - versioning: yes register: bucket_with_policy - assert: