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

s3_bucket: add encryption capabilities to the module #38538

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
8 participants
@julienvey
Contributor

julienvey commented Apr 10, 2018

SUMMARY

Add parameters to enable default encryption on a s3 bucket

  • encryption: AES256 or aws:kms
  • encryption_key_id: kms key id when aws:kms is used

Fixes #35110

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

s3_bucket

ANSIBLE VERSION
2.6.0
ADDITIONAL INFORMATION

I've added integration test for AES256. For aws:kms, I have tested it on my account, but since ansible does not provide a module to create kms key I was not able to add it to the integration test suite.

@ansibot

This comment has been minimized.

Contributor

ansibot commented Apr 10, 2018

@julienvey julienvey force-pushed the julienvey:s3_bucket_encrypt branch to 13d76f1 Apr 11, 2018

@julienvey

This comment has been minimized.

Contributor

julienvey commented Apr 11, 2018

@s-hertel Sorry to bother you once again, but this PR will also need a ci update :(

I have updated the storage-policy.json to reflect the necessary changes. the ListAllMyBuckets seems to be already present, otherwise the current test suite would have failed.

@s-hertel

Looks good @julienvey. I have a pull request open for CI.

@@ -510,11 +583,13 @@ def main():
state=dict(default='present', type='str', choices=['present', 'absent']),
tags=dict(required=False, default=None, type='dict'),
versioning=dict(default=None, type='bool'),
ceph=dict(default='no', type='bool')
ceph=dict(default='no', type='bool'),
encryption=dict(type='str', default=None, choices=['none', 'AES256', 'aws:kms']),

This comment has been minimized.

@s-hertel

s-hertel Apr 12, 2018

Contributor

type='str' and default=None are defaults, so you can omit those.

ceph=dict(default='no', type='bool')
ceph=dict(default='no', type='bool'),
encryption=dict(type='str', default=None, choices=['none', 'AES256', 'aws:kms']),
encryption_key_id=dict(type='str')

This comment has been minimized.

@s-hertel

s-hertel Apr 12, 2018

Contributor

Same here.

@@ -68,6 +68,14 @@
description:
- Whether versioning is enabled or disabled (note that once versioning is enabled, it can only be suspended)
type: bool
encryption:
description:
- Describes the default server-side encryption to apply to new objects in the bucket. If none is specified, the default encryption will be applied.

This comment has been minimized.

@s-hertel

s-hertel Apr 12, 2018

Contributor

People might misinterpret which none you mean. You might want to note that 'none' as a string is necessary to remove the server-side encryption.

@@ -244,8 +254,35 @@ def create_or_update_bucket(s3_client, module, location):
current_tags_dict = tags
changed = True
# Encryption
try:
current_encryption = get_bucket_encryption(s3_client, name)

This comment has been minimized.

@s-hertel

s-hertel Apr 12, 2018

Contributor

You probably want to check if s3_client has the appropriate attribute first, and only call this if it is the case (and only fail if it is not the case and the user has provided the encryption option).

hasattr(s3_client, "get_bucket_encryption")

Right now if a user has an older version of botocore this will fail with an AttributeError:

The full traceback is:
Traceback (most recent call last):
  File "/tmp/ansible_s3862i/ansible_module_s3_bucket.py", line 633, in <module>
    main()
  File "/tmp/ansible_s3862i/ansible_module_s3_bucket.py", line 627, in main
    create_or_update_bucket(s3_client, module, location)
  File "/tmp/ansible_s3862i/ansible_module_s3_bucket.py", line 259, in create_or_update_bucket
    current_encryption = get_bucket_encryption(s3_client, name)
  File "/tmp/ansible_s3862i/ansible_modlib.zip/ansible/module_utils/cloud.py", line 153, in retry_func
AttributeError: 'S3' object has no attribute 'get_bucket_encryption'
@julienvey

This comment has been minimized.

Contributor

julienvey commented Apr 13, 2018

@s-hertel Updates done

module.exit_json(changed=changed, name=name, versioning=versioning_return_value,
requester_pays=requester_pays, policy=current_policy, tags=current_tags_dict)
requester_pays=requester_pays, policy=current_policy, tags=current_tags_dict, encryption=current_encryption)

This comment has been minimized.

@s-hertel

s-hertel Apr 16, 2018

Contributor

current_encryption will only exist if hasattr(s3_client, "get_bucket_encryption") is True. You could add an else on line 267 to default the value.

description:
- Describes the default server-side encryption to apply to new objects in the bucket.
If the argument is not specified, the default encryption will be applied when creating the bucket.
In order to remove the server-side encryption, the encryption needs to be set to 'none' explicitely.

This comment has been minimized.

@s-hertel

s-hertel Apr 16, 2018

Contributor

s/explicitely/explicitly

time.sleep(5)
else:
return encryption
module.fail_json(msg="Bucket encryption failed to apply in the excepted time")

This comment has been minimized.

@s-hertel

s-hertel Apr 16, 2018

Contributor

I haven't run the looped tests on this PR to see how frequent the failures are, but each time I've rerun shippable it has been unstable. I think the allotted wait time might need to be increased for encryption. Also s/excepted/accepted? In other places too.

As this pattern is done in a number of places, would it make sense to turn the pattern into a wrapper that would take the method, method params, and the expected value?

This comment has been minimized.

@willthames

willthames Apr 17, 2018

Contributor

I suspect s/excepted/expected/

@ansibot ansibot added the stale_ci label Apr 25, 2018

@mattclay

This comment has been minimized.

Member

mattclay commented May 2, 2018

Tests in CI were unstable: https://app.shippable.com/github/ansible/ansible/runs/61466/67/tests

test/integration/targets/s3_bucket/tasks/main.yml:138:

failure: Bucket versioning failed to apply in the excepted time

I'll restart the tests to see if it occurs again.

)
)
module = AnsibleAWSModule(argument_spec=argument_spec)
module = AnsibleAWSModule(argument_spec=argument_spec, required_if=[['encryption', 'aws:kms', ['encryption_key_id']]])

This comment has been minimized.

@ryansb

ryansb May 3, 2018

Contributor

I think we also want to require encryption be specified anytime encryption_key_id is passed. That way users won't accidentally pass the key_id and assume that it's being used when it may not be.

@ansibot ansibot added needs_revision and removed core_review labels May 3, 2018

@ansibot ansibot added the stale_ci label May 11, 2018

@ansibot ansibot added the affects_2.6 label May 19, 2018

@wimnat

This comment has been minimized.

Contributor

wimnat commented Aug 10, 2018

@julienvey I tried to create a PR back to you for the review comments here but GitHub failed to show your fork up for some reason. You can check out by changes here https://github.com/wimnat/ansible/tree/s3_encryption

If you could merge these changes that'd be great - I'd love to see this merged.

@stefanhorning

This comment has been minimized.

Contributor

stefanhorning commented Nov 28, 2018

Also would like to see this added. @wimnat can't you just open a new PR with your branch? Like this https://github.com/ansible/ansible/compare/devel...wimnat:s3_encryption?expand=1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment