aws_ssm connection: add SSE encryption parameters. #763
Conversation
ansibullbot
added
WIP
There was a problem hiding this comment.
this is pretty nice! couple of inline suggestions (so far), and you'll need to add a changelog fragment as well.
plugins/connection/aws_ssm.py
Outdated
| @@ -554,14 +566,31 @@ def _file_transport_command(self, in_path, out_path, ssm_action): | |||
|
|
|||
| profile_name = self.get_option('profile') | |||
|
|
|||
| put_args = dict() | |||
| put_headers = dict() | |||
| if self.get_option('bucket_sse_mode') and self.get_option('bucket_sse_mode') in {'AES256', 'aws:kms'}: | |||
There was a problem hiding this comment.
| if self.get_option('bucket_sse_mode') and self.get_option('bucket_sse_mode') in {'AES256', 'aws:kms'}: | |
| if self.get_option('bucket_sse_mode'): |
In plugins (as opposed to modules), the option documentation is used not only for generating docs, but also during runtime by the built-in plugin machinery (called Config Manager), and so the choices: section is already enforced by the time you retrieve the value here.
There was a problem hiding this comment.
Noted & changed!
plugins/connection/aws_ssm.py
Outdated
| put_command_headers = "-Headers @{" + \ | ||
| "; ".join(["'%s' = '%s'" % (h, v) for h, v in put_headers.items()]) + "} " | ||
| put_command = "Invoke-WebRequest -Method PUT %s-InFile '%s' -Uri '%s' -UseBasicParsing" % ( | ||
| put_command_headers, in_path, | ||
| self._get_url('put_object', self.get_option('bucket_name'), s3_path, 'PUT', profile_name, put_args)) |
There was a problem hiding this comment.
| put_command_headers = "-Headers @{" + \ | |
| "; ".join(["'%s' = '%s'" % (h, v) for h, v in put_headers.items()]) + "} " | |
| put_command = "Invoke-WebRequest -Method PUT %s-InFile '%s' -Uri '%s' -UseBasicParsing" % ( | |
| put_command_headers, in_path, | |
| self._get_url('put_object', self.get_option('bucket_name'), s3_path, 'PUT', profile_name, put_args)) | |
| put_command_headers = "; ".join(["'%s' = '%s'" % (h, v) for h, v in put_headers.items()]) | |
| put_command = "Invoke-WebRequest -Method PUT -Headers @{ %s } -InFile '%s' -Uri '%s' -UseBasicParsing" % ( | |
| put_command_headers, in_path, | |
| self._get_url('put_object', self.get_option('bucket_name'), s3_path, 'PUT', profile_name, **put_args)) |
Somewhat opinionated, but it's ok to pass empty headers as in Invoke-WebRequest -Headers @{} so I think this reads a little better.
There was a problem hiding this comment.
Noted & changed! Additionally, since empty put_headers work in all cases, I removed the corresponding test & empty string default value for put_command_headers.
plugins/connection/aws_ssm.py
Outdated
| @@ -506,11 +516,13 @@ def _flush_stderr(self, subprocess): | |||
|
|
|||
| return stderr | |||
|
|
|||
| def _get_url(self, client_method, bucket_name, out_path, http_method, profile_name): | |||
| def _get_url(self, client_method, bucket_name, out_path, http_method, profile_name, extra_args={}): | |||
There was a problem hiding this comment.
| def _get_url(self, client_method, bucket_name, out_path, http_method, profile_name, extra_args={}): | |
| def _get_url(self, client_method, bucket_name, out_path, http_method, profile_name, **extra_args): |
sanity tests (pylint) flagged {} as a dangerous default value. Changed to kwargs style.
There was a problem hiding this comment.
extra_args here are not intended to be variable length kwargs but to correspond to boto's ExtraArgs parameter used in a number of S3 methods esp. upload_fileobj
Kept a standard optional single parameter with default =None, and used named arguments for extra_args/ExtraArgs for clarity.
|
@fh-maxime-froment this PR contains the following merge commits: Please rebase your branch to remove these commits. |
ansibullbot
added
merge_commit
fh-maxime-froment
force-pushed
the
wip-connection-ssm-encrypted-uploads
branch
from
b2a7c35 to
e8e7c2e
Compare
ansibullbot
removed
merge_commit
fh-maxime-froment
changed the title
|
Thanks very much @fh-maxime-froment! Would you be able to add some examples to the plugin showing how to use the new options? I took a quick pass a some integration tests after we talked on irc today @briantist, I do think that all the necessary IAM permissions should be there. I thought that this should cover it but I've definitely missed a step. I don't have a ton of experience using SSM though so maybe something will stand out that I overlooked? That gets me a failure on the ping test, however the console shows that the key is created: |
Great idea; examples in the docs should be very doable!
Thank you for this @jillr ! Quoting the diff so I can add diff highlighting, but I'll put it in an expand box. Integration diff proposal
There's a comment here that may end up being the reason why it doesn't work: #127 (comment) When we were first testing this out, we couldn't get it to work with an encrypted bucket either. That comment mentions an up to 24 hour period before the pre-signed URL works correctly. It seemed like a long shot, but to our surprise, waiting a day actually worked for us! That's bad news for CI though.. unless it's possible to just have this bucket exist rather than be created/destroyed during the test run? (perhaps it could be cleared at the beginning and end of each test run to prevent it filling up?). |
|
Thanks a lot @jillr and @briantist ! Concerning the integration test: besides the issue mentioned above by @briantist, I believe that the KMS key id input should either be an ID or an alias prefixed with Could you try with this instead?
For a newly created bucket, it may still not work because of the issue mentioned by @briantist.
But that's a bit of scope for this PR and should probably be made optional as the global url otherwise returned by |
Add the following parameters to aws_ssm.py connection plugin: * ansible_aws_ssm_bucket_sse_mode * ansible_aws_ssm_bucket_sse_kms_key_id
…rameters Co-authored-by: Brian Scholer <[email protected]>
fh-maxime-froment
force-pushed
the
wip-connection-ssm-encrypted-uploads
branch
from
f8d9d0b to
5c85d1d
Compare
I'll apply the patches mentioned from both of you and get that committed. |
|
@jillr so I applied your changes to the integration tests (and Maxime's suggestion), and maybe I'm not reading it right, but it looks like all the cloud integration tests were skipped? How do they actually get run? |
@jillr I've updated the tests to stop creating an S3 bucket and to instead use the one that already exists (532ccd6). But still seeing AccessDenied errors when the tests try to execute; I can't tell where to fix that though. |
|
I couldn't get these tests running locally without re-adding Apart of that, using another S3 encrypted bucket, the PR seems to work correctly. I ran the integration tests and also tried to check any change in the S3 bucket using the console @jillr I guess there is a problem with bucket permissions @briantist. Since @jillr is now on PTO, when they will come back (next week) will set it up. |
|
recheck |
|
@briantist I've disabled these tests in CI. We've manually tested the plugin itself and are confident that the actual code here is good. Once the merge conflict is resolved (the result of me disabling the tests, my apologies) I'm happy to merge this PR. @fh-maxime-froment I appreciate the patience you have given us, and for adding this feature! |
|
@jillr thanks a lot! conflict has been resolved |
|
@jillr @briantist @tremble @alinabuzachis Thank you all so much for following up on testing this PR! |
|
Since |
![ansible-zuul[bot]](https://avatars.githubusercontent.com/in/27800?s=60&v=4){kind=small}
Backport to stable-2: 💚 backport PR created✅ Backport PR branch: Backported as #823 🤖 @patchback |
aws_ssm connection: add SSE encryption parameters. SUMMARY Add the following parameters to aws_ssm.py connection plugin: ansible_aws_ssm_bucket_sse_mode ansible_aws_ssm_bucket_sse_kms_key_id ISSUE TYPE Feature Pull Request COMPONENT NAME aws_ssm connection plugin ADDITIONAL INFORMATION This allows the connection plugin to work when encryption parameters are required for uploads on the file transfer bucket by policy / SCP (see here for an example). Reviewed-by: Brian Scholer <None> Reviewed-by: Maxime <None> Reviewed-by: Jill R <None> Reviewed-by: Markus Bergholz <[email protected]> Reviewed-by: None <None> (cherry picked from commit 08f95cc)
aws_ssm connection: add SSE encryption parameters. SUMMARY Add the following parameters to aws_ssm.py connection plugin: ansible_aws_ssm_bucket_sse_mode ansible_aws_ssm_bucket_sse_kms_key_id ISSUE TYPE Feature Pull Request COMPONENT NAME aws_ssm connection plugin ADDITIONAL INFORMATION This allows the connection plugin to work when encryption parameters are required for uploads on the file transfer bucket by policy / SCP (see here for an example). Reviewed-by: Brian Scholer <None> Reviewed-by: Maxime <None> Reviewed-by: Jill R <None> Reviewed-by: Markus Bergholz <[email protected]> Reviewed-by: None <None> (cherry picked from commit 08f95cc) Co-authored-by: Maxime <[email protected]>
Backport to stable-3: 💚 backport PR created✅ Backport PR branch: Backported as #900 🤖 @patchback |
aws_ssm connection: add SSE encryption parameters. SUMMARY Add the following parameters to aws_ssm.py connection plugin: ansible_aws_ssm_bucket_sse_mode ansible_aws_ssm_bucket_sse_kms_key_id ISSUE TYPE Feature Pull Request COMPONENT NAME aws_ssm connection plugin ADDITIONAL INFORMATION This allows the connection plugin to work when encryption parameters are required for uploads on the file transfer bucket by policy / SCP (see here for an example). Reviewed-by: Brian Scholer <None> Reviewed-by: Maxime <None> Reviewed-by: Jill R <None> Reviewed-by: Markus Bergholz <[email protected]> Reviewed-by: None <None> (cherry picked from commit 08f95cc)
[PR #763/08f95cc6 backport][stable-3] aws_ssm connection: add SSE encryption parameters. This is a backport of PR #763 as merged into main (08f95cc). SUMMARY Add the following parameters to aws_ssm.py connection plugin: ansible_aws_ssm_bucket_sse_mode ansible_aws_ssm_bucket_sse_kms_key_id ISSUE TYPE Feature Pull Request COMPONENT NAME aws_ssm connection plugin ADDITIONAL INFORMATION This allows the connection plugin to work when encryption parameters are required for uploads on the file transfer bucket by policy / SCP (see here for an example).
ec2_instance incr version added to 3.3.0 for added parameters SUMMARY CI problems in backport-3 ansible-collections#721 And we don't won't to stop 3.2.0 release. Let's try to put/backport it for the next release. ISSUE TYPE Docs Pull Request COMPONENT NAME ec2_instance Reviewed-by: Gonéri Le Bouder <[email protected]> Reviewed-by: Mark Chappell <None>
…ctions#852) ec2_instance: metadata_options version_added increased SUMMARY After CI troubles in the past, we've forget to backport this feature. incr from 3.2.0 to 3.3.0 ansible-collections#763 failed backport 3 PR for 3.2.0 release ansible-collections#721 initial implementation for 3.2.0 ansible-collections#715 therefore no changelog fragment is necessary. ISSUE TYPE Feature Pull Request COMPONENT NAME ec2_instance ADDITIONAL INFORMATION Reviewed-by: Jill R <None> Reviewed-by: Alina Buzachis <None> Reviewed-by: Mark Chappell <None>
SUMMARY
Add the following parameters to aws_ssm.py connection plugin:
ISSUE TYPE
COMPONENT NAME
aws_ssm connection plugin
ADDITIONAL INFORMATION
This allows the connection plugin to work when encryption parameters are required for uploads on the file transfer bucket by policy / SCP (see here for an example).