ec2_key - fix security vulnerability #1704
Conversation
abikouo
changed the title
abikouo
changed the title
abikouo
added
backport-4
Docs Build 📝Thank you for contribution!✨ This PR has been merged and your docs changes will be incorporated when they are next published. |
|
@abikouo This cannot be backported because it adds new features where version_added is 7.0.0 and a new module with the same version_added. |
@alinabuzachis I added the label for the backport PR to be created once this is merged (to see the impacts). Indeed the backport will be performed manually |
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 3m 42s |
ansibullbot
added
bug
|
@abikouo Oh, I saw the changelog now. If it is a breaking_change we cannot backport as far as I know. |
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 10s |
There was a problem hiding this comment.
I don't like this approach.
To avoid the value turning up in the logs you can already use no_log and register, if anything this is a vulnerability in playbooks using the default behaviour, rather than the module:
- ec2_key: {...}
no_log: True
register: ec2_key_result
Defaulting to writing out a key is going to result in confusion because it will write it out on the 'host' side rather than the 'controller' side. However, if you then want to connect to an instance using this key you'll usually need it on the 'controller' side.
What I would suggest instead:
- Document that
no_logshould generally be used (unfortunately Ansible doesn't have support for 'secret' return values yet for this to be handled in the same way that it is for input values) - Add
file_nameas a new optional parameter. When used it'll write out the key to that file, here we can also document that this is written out 'host' side rather than 'controller' side. This becomes a new feature, which we can backport to stable-5 and stable-6
In theory we could then change the default behaviour in main to default to writing to a file, but I still don't think this is necessary.
|
recheck |
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 5m 21s |
I actually didn't realize register would still work with no_log, TIL. We'll definitely talk this over! |
|
Telling users to use |
@tremble thanks for the feedback!! |
abikouo
force-pushed
the
ec2_key_CVE-2023-4237
branch
from
b21fcb8 to
0566e19
Compare
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 5m 12s |
Start sketching out unit tests
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
… defining path to a file to store private key when creating new key pair
GomathiselviS
force-pushed
the
ec2_key_CVE-2023-4237
branch
from
cd0b977 to
61e45d7
Compare
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 4m 48s |
|
recheck |
Add ec2_key_info module SUMMARY Separated the info module from PR ansible-collections#1704 ISSUE TYPE New Module Pull Request COMPONENT NAME ADDITIONAL INFORMATION Reviewed-by: Alina Buzachis Reviewed-by: Helen Bailey <[email protected]> Reviewed-by: GomathiselviS Reviewed-by: Mike Graves <[email protected]>
Backport to stable-6: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply 1a077fb on top of patchback/backports/stable-6/1a077fb3a15241db8964dc086d3b15370bbd1e4a/pr-1704 Backporting merged PR #1704 into main
🤖 @patchback |
* WIP: begin swapping out private key data for key file Start sketching out unit tests * ec2_key - update module return private_key and add new parameter path defining path to a file to store private key when creating new key pair * fix changelog file * fix additional sanity issues * ec2_key - Remove breaking_change feature, add new parameter to save private key in * Code review updates * Refactoring * Remove key_info * Doc fixes * Uncomment call to ec2_key_info in tests * Add key_info runtume.yml --------- Co-authored-by: Jill Rouleau <[email protected]> Co-authored-by: GomathiselviS <[email protected]> (cherry picked from commit 1a077fb)
[manual backport stable-6] ec2_key - fix security vulnerability (#1704) WIP: begin swapping out private key data for key file Start sketching out unit tests ec2_key - update module return private_key and add new parameter path defining path to a file to store private key when creating new key pair fix changelog file fix additional sanity issues ec2_key - Remove breaking_change feature, add new parameter to save private key in Code review updates Refactoring Remove key_info Doc fixes Uncomment call to ec2_key_info in tests Add key_info runtume.yml Co-authored-by: Jill Rouleau [email protected] Co-authored-by: GomathiselviS [email protected] (cherry picked from commit 1a077fb) SUMMARY ISSUE TYPE Bugfix Pull Request Docs Pull Request Feature Pull Request New Module Pull Request COMPONENT NAME ADDITIONAL INFORMATION Reviewed-by: GomathiselviS
v8.4.0 Major Changes ------------- fortinet.fortios - Improve the document for adding notes and examples in Q&A for modules using Integer number as the mkey. Minor Changes ------------- amazon.aws - cloudformation - Add support for ``disable_rollback`` to update stack operation (ansible-collections/amazon.aws#1681). - ec2_key - add support for new parameter ``file_name`` to save private key in when new key is created by AWS. When this option is provided the generated private key will be removed from the module return (ansible-collections/amazon.aws#1704). ansible.netcommon - Add a new cliconf plugin ``default`` that can be used when no cliconf plugin is found for a given network_os. This plugin only supports ``get()``. (ansible-collections/ansible.netcommon#569) - httpapi - Add additional option ``ca_path``, ``client_cert``, ``client_key``, and ``http_agent`` that are available in open_url but not to httpapi. (ansible-collections/ansible.netcommon#528) - telnet - add crlf option to send CRLF instead of just LF (ansible-collections/ansible.netcommon#440). ansible.utils - Add ipcut filter plugin.(ansible-collections/ansible.utils#251) - Add ipv6form filter plugin.(ansible-collections/ansible.utils#230) arista.eos - Add support for overridden operation in bgp_global resource module.
|
This is CVE-2023-4237 |
|
That CVE seems to be mis-filed on GitHub, since the issue does not affect the |
|
@felixfontein Care to reassign ? |
|
I'd rather leave that to the collection maintainers since they know which versions of the collection are affected. |
Depends-On: ansible/ansible-zuul-jobs#1816
SUMMARY
"When creating a new keypair the ec2_key module prints out the private key directly to the standard output. This makes it unusable in any kind of public workflow.
The only reasonable implementation to address this will be to write the private key to a file. This is what other modules (such as community.crypto.openssh_keypair) that work with ssh keys do.
This fix consists in :
private_keyto store path to a file containing private key instead of private key directlypathto define the path where the private key will be stored when creating a new key pair.ISSUE TYPE
COMPONENT NAME
ec2_keyCo-authored-by: @jillr