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 roles and example playbook for PVC cert renewal #189

Merged
merged 7 commits into from
Aug 29, 2024

Conversation

jimright
Copy link
Contributor

@jimright jimright commented Aug 20, 2024

New roles added to support cert renewal on CDP Private Cloud:

  • tls_generate_csr: Generate CSR on each cluster host and copy back to controller
  • tls_signing: Copy CSR to the ca server, sign to generate cert
  • tls_install_certs: Copy signed certs to each cluster host; update Java keystore with new cert
  • tls_fetch_ca_certs: Used to fetch root and intermediate CA certs from the CA server

Example playbook pvc_renew_certs.yml also included to demonstrate how to use these roles.

@jimright jimright added the enhancement New feature or request label Aug 20, 2024
@jimright jimright requested a review from a team August 20, 2024 14:21
@jimright jimright force-pushed the feature/pvc_cert_renewal branch from 768046b to 1a0205c Compare August 22, 2024 13:35
Signed-off-by: Jim Enright <[email protected]>
Copy link
Member

@wmudge wmudge left a comment

Choose a reason for hiding this comment

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

Correct that you do not use tls_fetch_ca_certs in the playbook or in any other role?

An overall comment is that it feels odd to pull back the CSRs to the controller, only to push them to the CA server. Could we have each host push them to the CA server and get them signed via a delegate_to?

file_type: file
register: __csrs_to_sign
vars:
local_csrs_dir: "{{ (hostvars['localhost']['__pvc_tls_tempdir']['path'], 'csrs') | path_join }}"
Copy link
Member

Choose a reason for hiding this comment

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

Could you save yourself the vars declaration by using set_fact in the first play, which is running on localhost? (Actually, looks like the register is handling that for you already, which I thought it didn't persist outside of the play.)

local_csrs_to_sign: "{{ __csrs_to_sign.files | json_query('[*].path') | flatten }}"

- name: Play 2 - Sign the CSR
hosts: ca_server
Copy link
Member

Choose a reason for hiding this comment

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

This does not handle FreeIPA at the moment, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, the flow for renewing FreeIPA certs will be slightly different as it can happen on the cluster host with the step outlined here: https://www.freeipa.org/page/Certmonger#manually-renew-a-certificate.

# limitations under the License.
-->

# cloudera.exe.tls_fetch_ca_certs
Copy link
Member

Choose a reason for hiding this comment

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

👀

loop_control:
loop_var: dir

- ansible.builtin.set_fact:
Copy link
Member

Choose a reason for hiding this comment

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

Really should add name here and on other tasks

owner: root
mode: 0644

- name: Generate CSR
Copy link
Member

Choose a reason for hiding this comment

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

Why not use community.crypto.openssl_csr module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done as part of the latest set of commits. This then also allowed some cleanup of redundant tasks and variables.

description: "Flag to specify if the CSRs should be copied from the Ansible controller."
type: "bool"
default: true
override_old_certs:
Copy link
Member

Choose a reason for hiding this comment

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

This is a confusing parameter. If set to false, then the role will override, but if set to true, the role will back up the cert. The functionality is right; the naming is not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has simplified with the use of community.crypto.x509_certificate. I've renamed the parameter to backup_old_certs and use the parameter in the backup argument to community.crypto.x509_certificate.

roles/tls_signing/tasks/main.yml Show resolved Hide resolved

- name: Call tls_install_certs role
ansible.builtin.import_role:
name: cloudera.exetls_install_certs
Copy link
Member

Choose a reason for hiding this comment

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

Invalid reference (missing the dot separator)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot. Fixed in the latest set of commits.

owner: root
group: root

- name: Validate certificate
Copy link
Member

Choose a reason for hiding this comment

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

Potentially use the community.crypto.x509_certificate_info module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested this but it seems to not validate against the CA cert. Instead I've continued to use the openssl verify command but changed to module to ansible.builtin.command

- name: Generate a temporary PKCS12 keystore with renewed cert
community.crypto.openssl_pkcs12:
action: export
path: "{{ __temp_keystore }}"
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a tempfile location?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in the latest set of commits.

@wmudge wmudge added this to the Release 2.5.0 milestone Aug 22, 2024
Signed-off-by: Jim Enright <[email protected]>
Signed-off-by: Jim Enright <[email protected]>
Signed-off-by: Jim Enright <[email protected]>
@jimright
Copy link
Contributor Author

@wmudge - Thanks for the feedback; I've made some updates to the PR based on you comments.
Regarding your questions:

Correct that you do not use tls_fetch_ca_certs in the playbook or in any other role?

Yes! I created this role initially but found that it wasn't needed. I've kept it as it might be needed if we migrate the full TLS creation to cloudera.exe. However I'm happy to remove if you think it's better.

An overall comment is that it feels odd to pull back the CSRs to the controller, only to push them to the CA server. Could we have each host push them to the CA server and get them signed via a delegate_to?

My rationale for this was to be able to support the scenario where the CSRs are given to a third-party certificate authority in order to sign. I agree that it does make the flow a big clunky.
I will explore how we can have the both delegate_to functionality (for cases where we own the CA server) and the generate -> fetch -> sign -> fetch flows.

@wmudge wmudge merged commit 1c66512 into cloudera-labs:devel Aug 29, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request validated
Development

Successfully merging this pull request may close these issues.

2 participants