Skip to content

Add support of different key algorithm#70

Open
DXist wants to merge 30 commits intoNETWAYS:mainfrom
DXist:pkey_algorithm
Open

Add support of different key algorithm#70
DXist wants to merge 30 commits intoNETWAYS:mainfrom
DXist:pkey_algorithm

Conversation

@DXist
Copy link

@DXist DXist commented Mar 9, 2024

This PR adds support for a different key algorithm for certificate signing and usage in client/server communications.

  1. Default algorithm is RSA. I've extended molecule playbooks and included ca role configured to use Ed25519 keys.

  2. Besides I replaced passphrase passing from command argument to stdin to make sure that passphrase is not displayed in verbose output.

  3. Added more changes that I could extract into a separate PR:

  • new variables ca_dir_owner and ca_dir_group
  • ca_ca_password doesn't have default. User is required to provide it.
  • Distinguished name components like country, organization, organization unit, email are optional and omitted by default
  • common_name and subject alternative names use variables with default values and could be customized
  1. Also I've added notification handlers that run on certificate change - for regular/server/etcd/etcd server certificates.

I noticed that when CA and client certificate directory is the same on CA servers certificate push from localhost is skipped and handler is not run for client certificate update on CA host.

I've fixed it by configuring different directories for CA files and client certificates for renew and ca-renew scenarios.
This change revealed incorrect path used for crt_path.

  1. Besides I hit checksum mismatch for concurrent CA certificate fetch and added run_once argument for delegate_to tasks supposed to run only once on CA host and localhost.

  2. I simplified notAfter validation via openssl -checkvalid flag.

  3. I've fixed ansible-lint errors and enabled ansible-lint in the Github workflow.

  4. I've added support of setting ca_altnameX variables to null value and omitting subjectAltName field. Alternatively it's possible to provide custom ca_subject_alternative_name

  5. I've fixed ca.conf.j2 to include proper x509v3 extensions for CA certificate. Removed subjectAltName from it because subjectAltName makes sense for end user certificates.

  6. I've changed default value for ca_server_cert to false. My assumption is that the first end-user certificate could be configured as client or/and server.

  7. Renamed ca_client_* vars to ca_cert_* to reflect that the first certificate could be not for clientAuth purpose

  8. Added ca_cert flag to be able to skip leaf certificate generation on CA host by default.

  9. extended_key_usage is renamed to ca_extended_key_usage and become a list. It's possible to set it like this

ca_extended_key_usage:
    - clientAuth
    - serverAuth

With handlers added I recommend to use different directories for CA host files and client certificates. But I didn't change it in the default variables for backward compatibility.

@RincewindsHat
Copy link
Member

@NETWAYS/ansible-developer

@widhalmt
Copy link
Member

Wow, that's some huge contribution! Thank you so much.

I'm sifting through the changes and trying to wrap my brain around them. Usually I prefer smaller PRs but I understand that these changes are intertwined so much that it just doesn't make sense to split them up and end up in merge conflict hell.

I really love your approach to handlers. Honestly, I didn't know that this was possible and it will definitely improve not only this role but my future Ansible code as well.

I'm on it but given the sheer size of the changes it will still take some time.

Copy link
Contributor

@tbauriedel tbauriedel left a comment

Choose a reason for hiding this comment

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

thank you for the contribution! It adds a nice feature and also fixes some issues inside of the role.

I found some small things so far. Please check these again. If I have overlooked something, please let me know. CC @DXist

@widhalmt could you also have quick look at it? Just we are on the safe side and dont loose backwards compatibility.

Comment on lines +297 to 299
force: "{{ not crt_info.valid_at.check_period | default(omit) or
hostvars[ca_ca_host]['ca_ca_renewed'] | default(omit) }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
force: "{{ not crt_info.valid_at.check_period | default(omit) or
hostvars[ca_ca_host]['ca_ca_renewed'] | default(omit) }}"
force:
"{{ not crt_info.valid_at.check_period | default(omit) or
hostvars[ca_ca_host]['ca_ca_renewed'] | default(omit) }}"

Comment on lines +363 to 365
force: "{{ not crt_info.valid_at.check_period | default(omit) or
hostvars[ca_ca_host]['ca_ca_renewed'] | default(omit) }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
force: "{{ not crt_info.valid_at.check_period | default(omit) or
hostvars[ca_ca_host]['ca_ca_renewed'] | default(omit) }}"
force:
"{{ not crt_info.valid_at.check_period | default(omit) or
hostvars[ca_ca_host]['ca_ca_renewed'] | default(omit) }}"

Comment on lines +182 to 183
force: "{{ not crt_info.valid_at.check_period | default(omit) or
hostvars[ca_ca_host]['ca_ca_renewed'] | default(omit) }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
force: "{{ not crt_info.valid_at.check_period | default(omit) or
hostvars[ca_ca_host]['ca_ca_renewed'] | default(omit) }}"
force:
"{{ not crt_info.valid_at.check_period | default(omit) or
hostvars[ca_ca_host]['ca_ca_renewed'] | default(omit) }}"

path: "{{ ansible_env.HOME }}/{{ inventory_hostname }}.initial_certificate"
register: initial_handler_file

- name: Fail if initial file of on certificate change handler does not exist
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: Fail if initial file of on certificate change handler does not exist
- name: Fail if initial file of 'on certificate change' handler does not exist


- name: Fail if initial file of on certificate change handler does not exist
ansible.builtin.fail:
msg: "Failed because on certificate change handler hasn't created initial file"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
msg: "Failed because on certificate change handler hasn't created initial file"
msg: "Failed because 'on certificate change' handler hasn't created initial file"

msg: "Failed because on certificate change handler hasn't created initial file"
when: not initial_handler_file.stat.exists

- name: Register if on certificate change handler has run for renewed client certificate
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: Register if on certificate change handler has run for renewed client certificate
- name: Register if 'on certificate change' handler has run for renewed client certificate

path: "{{ ansible_env.HOME }}/{{ inventory_hostname }}.renewed_certificate"
register: renewed_handler_file

- name: Fail if renewed file of on certificate change handler does not exist
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: Fail if renewed file of on certificate change handler does not exist
- name: Fail if renewed file of 'on certificate change' handler does not exist


- name: Fail if renewed file of on certificate change handler does not exist
ansible.builtin.fail:
msg: "Failed because on certificate change handler hasn't created renewed file"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
msg: "Failed because on certificate change handler hasn't created renewed file"
msg: "Failed because 'on certificate change handler' hasn't created renewed file"

@widhalmt
Copy link
Member

widhalmt commented Aug 8, 2025

@DXist , I guess, I owe you an apology. I can't really tell you why I wasn't able to complete the review. I had it in my mind the whole time but I was blocked from proceeding with it. So finally I asked a colleague to work on it. I hope, I haven't destroyed all your enthusiasm for contributing to this role. If so, then please blame it on me but not on the project.

Copy link
Author

@DXist DXist left a comment

Choose a reason for hiding this comment

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

Rebased and removed openssl version detection.

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.

4 participants