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

Add persistent private key feature #44

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

martonivan
Copy link

I - and may others - need a feature to reuse the existing private keys, for example for use-cases where the private keys are also centrally managed and distributed. I made this feature configurable by keeping the original behavior as default.

Copy link
Owner

@mprahl mprahl left a comment

Choose a reason for hiding this comment

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

We currently don't set force to true when calling the openssl_privatekey module, which means it shouldn't regenerate the private key.

Could you please provide more context for your PR?

@martonivan
Copy link
Author

You are totally right, I've played around with openssl_privatekey a bit and realized what could have misled me.

Previously I've configured a batch of servers using a small set of different domains and pre-distributed private keys and experienced that the private keys are overwritten. Other tasks later distributed the signed certificates inside the infrastructure, but it's an other question, just mentioning here to explain why the regenerated private keys caused issue for me.

You are right, force is not set to true when openssl_privatekey module is called in your module, however this is not the single case when the privatekey is regenerated. It is when any major parameter is different than the passed/defined by your module.

Since in my case not all the private keys were identical, e.g the key size is different, each of them not matching the default or explicitly configured value has been regenerated.

It would be really nice not to follow the key details in ansible too, but would be able to preliminary define that the private keys should be kept intact. I hope these above make sense. :)

README.md Outdated
@@ -92,6 +92,8 @@ Ansible 2.7+ is required for this role. If you are using an older version of Ans
* **ler53_acme_directory** - The ACME directory to use. This defaults to
`https://acme-v02.api.letsencrypt.org/directory`. This can be useful to override if you'd like to
test this role against the stage Let's Encrypt instance.
* **ler53_static_private_key** - whether or not reuse the private key. If this is `true` private key
Copy link
Owner

Choose a reason for hiding this comment

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

Could you rename this to ler53_unmanaged_private_key or somethign similar?

Copy link
Author

Choose a reason for hiding this comment

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

Implemented as you've suggested.

tasks/main.yml Outdated
size: "{{ ler53_key_size }}"
owner: "{{ ler53_cert_files_owner }}"
group: "{{ ler53_cert_files_group }}"
mode: "{{ ler53_cert_files_mode }}"
when: not private_key_path.stat.exists or not ler53_static_private_key
Copy link
Owner

Choose a reason for hiding this comment

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

I just want either the key to be managed by Ansible or not. This behavior is too complex.

Suggested change
when: not private_key_path.stat.exists or not ler53_static_private_key
when: not ler53_static_private_key

Copy link
Author

Choose a reason for hiding this comment

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

Implemented as you've suggested.

tasks/main.yml Outdated
@@ -89,19 +89,27 @@
import_tasks: virtualenv.yml
when: ansible_os_family in ['RedHat', 'Darwin']

- name: define private key path
Copy link
Owner

Choose a reason for hiding this comment

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

This can be removed after my comment on the generate the private key task.

Copy link
Author

Choose a reason for hiding this comment

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

Implemented as you've suggested.

@martonivan martonivan force-pushed the master branch 2 times, most recently from 28467e9 to fb5612b Compare January 4, 2023 15:29
README.md Outdated
@@ -92,6 +92,8 @@ Ansible 2.7+ is required for this role. If you are using an older version of Ans
* **ler53_acme_directory** - The ACME directory to use. This defaults to
`https://acme-v02.api.letsencrypt.org/directory`. This can be useful to override if you'd like to
test this role against the stage Let's Encrypt instance.
* **ler53_unmanaged_private_key** - whether or not reuse the private key. If this is `true` private key
Copy link
Owner

Choose a reason for hiding this comment

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

I think this documentation needs updating since based on the code, the key will never be managed if this is true. Is that your intention?

Copy link
Author

Choose a reason for hiding this comment

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

documentation fix has been sent

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