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 role to support CRL #5

Merged
merged 6 commits into from
Jul 24, 2017
Merged

Refactor role to support CRL #5

merged 6 commits into from
Jul 24, 2017

Conversation

msheiny
Copy link
Contributor

@msheiny msheiny commented Jul 21, 2017

Did some tweaking to allow running this role under a persistent CA environment which is necessary to support things like CRL, and serial number tracking.

Previous design was assuming you were storing a CA in ansible vault and
just spinning off CSRs remotely, pulling those back and signing them, then
ignoring the local environment completely (besides private CA key purge).
This was all well and good but does not play nicely with an environment
where you are tracking CA attributes such as serial numbers and MORE
importantly revocation data.

This big commit tries to rework the tasks to allow such a scenario. It
doesn't set that up but if it detects you are trying to run under a
long-running CA folder (by looking for a crl.pem ), a different task will
kick off that will allow you to sign a CSR and also continue track that
data locally.

@msheiny msheiny requested a review from conorsch July 21, 2017 21:08
msheiny added 4 commits July 21, 2017 17:09
Previous design was assuming you were storing a CA in ansible vault and just
spinning off CSRs remotely, pulling those back and signing them, then ignoring
the local environment completely (besides private CA key purge). This was all
well and good but does not play nicely with an environment where you are
tracking CA attributes such as serial numbers and MORE importantly revocation
data.

This big commit tries to rework the tasks to allow such a scenario. It doesn't
set that up but if it detects you are trying to run under a long-running CA
folder (by looking for a crl.pem ), a different task will kick off that will
allow you to sign a CSR and also continue track that data locally.

PHEW isn't openssl fun!!?  :|
Helps when you are trying to utilize tags to call out specific bits for testing.
tasks/main.yml Outdated
content: "{{ openssl_node_ca_cert_sign.ca_pub_key }}"
register: copy_ca_certs

- name: Update system CA store
command: update-ca-certificates
when: copy_ca_certs.changed
when:
- copy_ca_certs.changed
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: should be copy_ca_certs|changed

Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Clean implementation and good documentation of changes. Requested edit for |changed filter use.

Also consider using the tempfile Ansible module for managing the temporary directories. Not blocking merge for that, though.

I've made this good a few times in the past where it comes to bite me. The
changed filter provides better error handling in say, the situation where the
changed field doesnt even exist :)
@msheiny
Copy link
Contributor Author

msheiny commented Jul 24, 2017

Clean implementation and good documentation of changes. Requested edit for |changed filter use.

✔️ Addressed that in 8f24aae. Good 👁 !

Also consider using the tempfile Ansible module for managing the temporary directories. Not blocking merge for that, though.

That's a good call. I made a ticket to address that in #7. Thanks for not blocking on that yet. I'd like to get CI wired up ( #6 ) before I do that to make sure I don't have any regressions.

@conorsch
Copy link
Contributor

I made a ticket to address that in #7.

I ❤️ rainy-day issues!

@conorsch conorsch merged commit fe0c642 into master Jul 24, 2017
@msheiny msheiny deleted the CRLSupport branch July 27, 2017 21:58
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