Skip to content
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

Refactor openssl_privatekey module, move add openssl_privatekey_pipe module #119

Merged

Conversation

felixfontein
Copy link
Contributor

SUMMARY

openssl_privatekey_pipe is providing almost the same functionality than openssl_privatekey, except that it does not operate on disk. An existing private key can (optionally) be passed into the module (for idempotency checks, or for conversion), and the new/converted/unchanged private key is returned, but not written to disk.

This allows to store key data in vaults without having to write the unprotected key to disk (for example with the community.sops collection).

For this, I moved the main code from openssl_privatekey to plugins/module_utils/crypto/module_backends/privatekey.py, and the corresponding documentation to plugins/doc_fragments/module_privatekey.py.

I'm not 100% sure about the _pipe suffix. If someone has a better idea, feel free to mention it :)

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

openssl_privatekey
openssl_privatekey_pipe

@MarkusTeufelberger
Copy link
Contributor

Isn't this functionality (ephemeral state) something that's more likely part of an action plugin? Also visible in logs or not, this will always transmit the private key material to the Ansible controller and the target host. It seems to me that this is something that you shouldn't run on remote hosts anyways (you only might want to assign the task to the host to get some context/host_vars).

Implementing this as action plugin however is a LOT more pain than one might initially expect, just as a fair warning.

I don't have a better name than the _pipe suffix (maybe _ephemeral? Harder to spell correctly though and not much more correct...).

Maybe there's also the option of adding a content field to openssl_privatekey and the option to only return the key in the module output instead to get a similar functionality and stay in line with openssl_privatekey_info?

@felixfontein
Copy link
Contributor Author

@MarkusTeufelberger I'd think the module will be mostly used for localhost (i.e. delegate_to: localhost); maybe I should add that to the example to make this clearer.

Making it an action plugin is a good idea in this case, though I wanted to do the same extension to other modules as well, most prominently openssl_csr, and for that an action plugin would be very limiting, since the private key input both makes sense as content and a file. You could execute openssl_csr_pipe on a remote host (which has the private key stored on disk), without having to write the .csr to disk.

About adding a content field to openssl_privatekey: that's possible, but very painful to implement, since the module is built around the idea of operating on a file on disk. I think having another module (or action plugin) specialized on this use-case is better.

I don't have a better name than the _pipe suffix (maybe _ephemeral? Harder to spell correctly though and not much more correct...).

I agree that _ephemeral is harder to spell correctly, so let's avoid that. On the other hand, it would be a good training exercise for me, so I will hopefully misspell it less often in the future ;)

@felixfontein felixfontein changed the title Refactor openssl_privatekey module, move add openssl_privatekey_pipe module [WIP] Refactor openssl_privatekey module, move add openssl_privatekey_pipe module Oct 9, 2020
@felixfontein
Copy link
Contributor Author

I'll look into changing this to an action plugin later. I think it should not be that hard, with a proper bit of framework code for argument validation. Fortunately most of that has already been moved out ot AnsibleModule into generic validation functionality in https://github.com/ansible/ansible/tree/devel/lib/ansible/module_utils/common/

@felixfontein felixfontein changed the title [WIP] Refactor openssl_privatekey module, move add openssl_privatekey_pipe module Refactor openssl_privatekey module, move add openssl_privatekey_pipe module Oct 10, 2020
@felixfontein
Copy link
Contributor Author

I've changed the module to an action plugin. For that, I've added a small framework (mostly copy'n'pasta from ansible.module_utils.basic) which provides similar argument validation facilities as AnsibleModule.

@felixfontein
Copy link
Contributor Author

ready_for_review

Copy link
Contributor

@MarkusTeufelberger MarkusTeufelberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find much, maybe a few ideas to consider or things to clarify. Hope it helps, I think it can also be merged as-is.

plugins/module_utils/action_module.py Show resolved Hide resolved
plugins/action/openssl_privatekey_pipe.py Show resolved Hide resolved
plugins/module_utils/crypto/module_backends/privatekey.py Outdated Show resolved Hide resolved
plugins/modules/openssl_privatekey.py Outdated Show resolved Hide resolved
plugins/modules/openssl_privatekey_pipe.py Show resolved Hide resolved
@MarkusTeufelberger
Copy link
Contributor

Looks good to me, thanks for all the work!

/shipit

@felixfontein felixfontein merged commit 3c21079 into ansible-collections:main Oct 28, 2020
@felixfontein felixfontein deleted the openssl_privatekey_pipe branch October 28, 2020 20:53
@felixfontein
Copy link
Contributor Author

@MarkusTeufelberger thanks a lot for reviewing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants