From 0303ea2bfa7d52701afe8016195ca19c13e4f29b Mon Sep 17 00:00:00 2001 From: Andrea Tartaglia Date: Wed, 10 Apr 2019 11:43:08 +0100 Subject: [PATCH] openssl_pkcs12: Add idempotency checks (#54633) * Added idempotency logic to openssl_pkcs12 Also decoupled the 'parse' and 'generate' function from the file write as they are now used in different places that do not need the file to be written to disk. * Added idempotency tests for openssl_pkcs12 Also adds a new test for pkcs12 files with multiple certificates * Regenerate if parsed file is invalid * pkcs12_other_certificates check was wrong * Updated ca_certificates to other_certificates ca_certificates is left as an alias to other_certificates; friendlyname depends on private key, so it will be ignored while checking for idempotency if the pkey is not set; idempotency check only checks for correct certs in the stack * use different keys for different certs * Added other_certificates in module docs * Added changelog and porting guide * removed unrelated porting guide entry * renamed ca_cert* occurrence with other_cert --- ...4633-openssl_pkcs12_idempotency_fixes.yaml | 2 + .../rst/porting_guides/porting_guide_2.8.rst | 2 + lib/ansible/modules/crypto/openssl_pkcs12.py | 100 +++++++++++++----- .../targets/openssl_pkcs12/tasks/impl.yml | 80 ++++++++++++-- .../targets/openssl_pkcs12/tests/validate.yml | 15 +-- 5 files changed, 163 insertions(+), 36 deletions(-) create mode 100644 changelogs/fragments/54633-openssl_pkcs12_idempotency_fixes.yaml diff --git a/changelogs/fragments/54633-openssl_pkcs12_idempotency_fixes.yaml b/changelogs/fragments/54633-openssl_pkcs12_idempotency_fixes.yaml new file mode 100644 index 00000000000..c575f999280 --- /dev/null +++ b/changelogs/fragments/54633-openssl_pkcs12_idempotency_fixes.yaml @@ -0,0 +1,2 @@ +minor_changes: +- "openssl_pkcs12 - Fixed idempotency checks, the module will regenerate the pkcs12 file if any of the parameters differ from the ones in the file. The ``ca_certificates`` parameter has been renamed to ``other_certificates``. " diff --git a/docs/docsite/rst/porting_guides/porting_guide_2.8.rst b/docs/docsite/rst/porting_guides/porting_guide_2.8.rst index 4681e3458d0..64307a08f63 100644 --- a/docs/docsite/rst/porting_guides/porting_guide_2.8.rst +++ b/docs/docsite/rst/porting_guides/porting_guide_2.8.rst @@ -333,6 +333,8 @@ Noteworthy module changes * The ``win_dsc`` module will now validate the input options for a DSC resource. In previous versions invalid options would be ignored but are now not. +* The ``openssl_pkcs12`` module will now regenerate the pkcs12 file if there are differences between the file on disk and the parameters passed to the module. + Plugins ======= diff --git a/lib/ansible/modules/crypto/openssl_pkcs12.py b/lib/ansible/modules/crypto/openssl_pkcs12.py index 41ed7031398..983247bcfbc 100644 --- a/lib/ansible/modules/crypto/openssl_pkcs12.py +++ b/lib/ansible/modules/crypto/openssl_pkcs12.py @@ -29,10 +29,11 @@ options: type: str default: export choices: [ export, parse ] - ca_certificates: + other_certificates: description: - - List of CA certificate to include. + - List of other certificates to include. Pre 2.8 this parameter was called C(ca_certificates) type: list + aliases: [ ca_certificates ] certificate_path: description: - The path to read certificates and private keys from. @@ -111,7 +112,7 @@ EXAMPLES = r''' friendly_name: raclette privatekey_path: /opt/certs/keys/key.pem certificate_path: /opt/certs/cert.pem - ca_certificates: /opt/certs/ca.pem + other_certificates: /opt/certs/ca.pem state: present - name: Change PKCS#12 file permission @@ -121,7 +122,7 @@ EXAMPLES = r''' friendly_name: raclette privatekey_path: /opt/certs/keys/key.pem certificate_path: /opt/certs/cert.pem - ca_certificates: /opt/certs/ca.pem + other_certificates: /opt/certs/ca.pem state: present mode: '0600' @@ -133,7 +134,7 @@ EXAMPLES = r''' friendly_name: raclette privatekey_path: /opt/certs/keys/key.pem certificate_path: /opt/certs/cert.pem - ca_certificates: /opt/certs/ca.pem + other_certificates: /opt/certs/ca.pem state: present mode: '0600' force: yes @@ -201,7 +202,7 @@ class Pkcs(crypto_utils.OpenSSLObject): module.check_mode ) self.action = module.params['action'] - self.ca_certificates = module.params['ca_certificates'] + self.other_certificates = module.params['other_certificates'] self.certificate_path = module.params['certificate_path'] self.friendly_name = module.params['friendly_name'] self.iter_size = module.params['iter_size'] @@ -237,7 +238,50 @@ class Pkcs(crypto_utils.OpenSSLObject): if not state_and_perms: return state_and_perms - return _check_pkey_passphrase + if os.path.exists(self.path) and module.params['action'] == 'export': + dummy = self.generate(module) + self.src = self.path + try: + pkcs12_privatekey, pkcs12_certificate, pkcs12_other_certificates, pkcs12_friendly_name = self.parse() + except crypto.Error: + return False + if (pkcs12_privatekey is not None) and (self.privatekey_path is not None): + expected_pkey = crypto.dump_privatekey(crypto.FILETYPE_PEM, + self.pkcs12.get_privatekey()) + if pkcs12_privatekey != expected_pkey: + return False + elif bool(pkcs12_privatekey) != bool(self.privatekey_path): + return False + + if (pkcs12_certificate is not None) and (self.certificate_path is not None): + + expected_cert = crypto.dump_certificate(crypto.FILETYPE_PEM, + self.pkcs12.get_certificate()) + if pkcs12_certificate != expected_cert: + return False + elif bool(pkcs12_certificate) != bool(self.certificate_path): + return False + + if (pkcs12_other_certificates is not None) and (self.other_certificates is not None): + expected_other_certs = [crypto.dump_certificate(crypto.FILETYPE_PEM, + other_cert) for other_cert in self.pkcs12.get_ca_certificates()] + if set(pkcs12_other_certificates) != set(expected_other_certs): + return False + elif bool(pkcs12_other_certificates) != bool(self.other_certificates): + return False + + if pkcs12_privatekey: + # This check is required because pyOpenSSL will not return a firendly name + # if the private key is not set in the file + if ((self.pkcs12.get_friendlyname() is not None) and (pkcs12_friendly_name is not None)): + if self.pkcs12.get_friendlyname() != pkcs12_friendly_name: + return False + elif bool(self.pkcs12.get_friendlyname()) != bool(pkcs12_friendly_name): + return False + else: + return False + + return _check_pkey_passphrase() def dump(self): """Serialize the object into a dictionary.""" @@ -254,13 +298,12 @@ class Pkcs(crypto_utils.OpenSSLObject): def generate(self, module): """Generate PKCS#12 file archive.""" - self.pkcs12 = crypto.PKCS12() - if self.ca_certificates: - ca_certs = [crypto_utils.load_certificate(ca_cert) for ca_cert - in self.ca_certificates] - self.pkcs12.set_ca_certificates(ca_certs) + if self.other_certificates: + other_certs = [crypto_utils.load_certificate(other_cert) for other_cert + in self.other_certificates] + self.pkcs12.set_ca_certificates(other_certs) if self.certificate_path: self.pkcs12.set_certificate(crypto_utils.load_certificate( @@ -278,20 +321,14 @@ class Pkcs(crypto_utils.OpenSSLObject): except crypto_utils.OpenSSLBadPassphraseError as exc: raise PkcsError(exc) - if self.backup: - self.backup_file = module.backup_local(self.path) - crypto_utils.write_file( - module, - self.pkcs12.export(self.passphrase, self.iter_size, self.maciter_size), - 0o600 - ) + return self.pkcs12.export(self.passphrase, self.iter_size, self.maciter_size) def remove(self, module): if self.backup: self.backup_file = module.backup_local(self.path) super(Pkcs, self).remove(module) - def parse(self, module): + def parse(self): """Read PKCS#12 file.""" try: @@ -303,17 +340,29 @@ class Pkcs(crypto_utils.OpenSSLObject): p12.get_privatekey()) crt = crypto.dump_certificate(crypto.FILETYPE_PEM, p12.get_certificate()) + other_certs = [] + if p12.get_ca_certificates() is not None: + other_certs = [crypto.dump_certificate(crypto.FILETYPE_PEM, + other_cert) for other_cert in p12.get_ca_certificates()] - crypto_utils.write_file(module, b'%s%s' % (pkey, crt)) + friendly_name = p12.get_friendlyname() + + return (pkey, crt, other_certs, friendly_name) except IOError as exc: raise PkcsError(exc) + def write(self, module, content, mode=None): + """Write the PKCS#12 file.""" + if self.backup: + self.backup_file = module.backup_local(self.path) + crypto_utils.write_file(module, content, mode) + def main(): argument_spec = dict( action=dict(type='str', default='export', choices=['export', 'parse']), - ca_certificates=dict(type='list', elements='path'), + other_certificates=dict(type='list', elements='path', aliases=['ca_certificates']), certificate_path=dict(type='path'), force=dict(type='bool', default=False), friendly_name=dict(type='str', aliases=['name']), @@ -363,10 +412,13 @@ def main(): if module.params['action'] == 'export': if not module.params['friendly_name']: module.fail_json(msg='Friendly_name is required') - pkcs12.generate(module) + pkcs12_content = pkcs12.generate(module) + pkcs12.write(module, pkcs12_content, 0o600) changed = True else: - pkcs12.parse(module) + pkey, cert, other_certs, friendly_name = pkcs12.parse() + dump_content = '%s%s%s' % (to_native(pkey), to_native(cert), to_native(b''.join(other_certs))) + pkcs12.write(module, to_bytes(dump_content)) file_args = module.load_file_common_arguments(module.params) if module.set_fs_attributes_if_different(file_args, changed): diff --git a/test/integration/targets/openssl_pkcs12/tasks/impl.yml b/test/integration/targets/openssl_pkcs12/tasks/impl.yml index 54318610021..492724fa399 100644 --- a/test/integration/targets/openssl_pkcs12/tasks/impl.yml +++ b/test/integration/targets/openssl_pkcs12/tasks/impl.yml @@ -1,21 +1,48 @@ --- - block: - - name: 'Generate privatekey with' + - name: 'Generate privatekey' openssl_privatekey: path: "{{ output_dir }}/ansible_pkey.pem" - - name: 'Generate CSR with' + - name: 'Generate privatekey2' + openssl_privatekey: + path: "{{ output_dir }}/ansible_pkey2.pem" + + - name: 'Generate privatekey3' + openssl_privatekey: + path: "{{ output_dir }}/ansible_pkey3.pem" + + - name: 'Generate CSR' openssl_csr: path: "{{ output_dir }}/ansible.csr" privatekey_path: "{{ output_dir }}/ansible_pkey.pem" commonName: 'www.ansible.com' + - name: 'Generate CSR 2' + openssl_csr: + path: "{{ output_dir }}/ansible2.csr" + privatekey_path: "{{ output_dir }}/ansible_pkey2.pem" + commonName: 'www2.ansible.com' + + - name: 'Generate CSR 3' + openssl_csr: + path: "{{ output_dir }}/ansible3.csr" + privatekey_path: "{{ output_dir }}/ansible_pkey3.pem" + commonName: 'www3.ansible.com' + - name: 'Generate certificate' openssl_certificate: - path: "{{ output_dir }}/ansible.crt" - privatekey_path: "{{ output_dir }}/ansible_pkey.pem" - csr_path: "{{ output_dir }}/ansible.csr" + path: "{{ output_dir }}/{{ item.name }}.crt" + privatekey_path: "{{ output_dir }}/{{ item.pkey }}" + csr_path: "{{ output_dir }}/{{ item.name }}.csr" provider: selfsigned + loop: + - name: ansible + pkey: ansible_pkey.pem + - name: ansible2 + pkey: ansible_pkey2.pem + - name: ansible3 + pkey: ansible_pkey3.pem - name: 'Generate PKCS#12 file' openssl_pkcs12: @@ -26,6 +53,15 @@ state: present register: p12_standard + - name: 'Generate PKCS#12 file again, idempotency' + openssl_pkcs12: + path: "{{ output_dir }}/ansible.p12" + friendly_name: 'abracadabra' + privatekey_path: "{{ output_dir }}/ansible_pkey.pem" + certificate_path: "{{ output_dir }}/ansible.crt" + state: present + register: p12_standard_idempotency + - name: 'Generate PKCS#12 file (force)' openssl_pkcs12: path: "{{ output_dir }}/ansible.p12" @@ -54,6 +90,37 @@ action: 'parse' state: 'present' + - name: 'Generate PKCS#12 file with multiple certs' + openssl_pkcs12: + path: "{{ output_dir }}/ansible_multi_certs.p12" + friendly_name: 'abracadabra' + privatekey_path: "{{ output_dir }}/ansible_pkey.pem" + certificate_path: "{{ output_dir }}/ansible.crt" + ca_certificates: + - "{{ output_dir }}/ansible2.crt" + - "{{ output_dir }}/ansible3.crt" + state: present + register: p12_multiple_certs + + - name: 'Generate PKCS#12 file with multiple certs, again (idempotency)' + openssl_pkcs12: + path: "{{ output_dir }}/ansible_multi_certs.p12" + friendly_name: 'abracadabra' + privatekey_path: "{{ output_dir }}/ansible_pkey.pem" + certificate_path: "{{ output_dir }}/ansible.crt" + ca_certificates: + - "{{ output_dir }}/ansible2.crt" + - "{{ output_dir }}/ansible3.crt" + state: present + register: p12_multiple_certs_idempotency + + - name: 'Dump PKCS#12 with multiple certs' + openssl_pkcs12: + src: "{{ output_dir }}/ansible_multi_certs.p12" + path: "{{ output_dir }}/ansible_parse_multi_certs.pem" + action: 'parse' + state: 'present' + - name: Generate privatekey with password openssl_privatekey: path: '{{ output_dir }}/privatekeypw.pem' @@ -163,10 +230,11 @@ - name: 'Delete PKCS#12 file' openssl_pkcs12: state: absent - path: '{{ output_dir }}/ansible.p12' + path: '{{ output_dir }}/{{ item }}.p12' loop: - 'ansible' - 'ansible_no_pkey' + - 'ansible_multi_certs' - 'ansible_pw1' - 'ansible_pw2' - 'ansible_pw3' diff --git a/test/integration/targets/openssl_pkcs12/tests/validate.yml b/test/integration/targets/openssl_pkcs12/tests/validate.yml index da88d0c7b7e..37d6d72118a 100644 --- a/test/integration/targets/openssl_pkcs12/tests/validate.yml +++ b/test/integration/targets/openssl_pkcs12/tests/validate.yml @@ -1,9 +1,3 @@ ---- -- name: 'Install pexpect' - pip: - name: 'pexpect' - state: 'present' - - name: 'Validate PKCS#12' command: "openssl pkcs12 -info -in {{ output_dir }}/ansible.p12 -nodes -passin pass:''" register: p12 @@ -12,6 +6,10 @@ command: "openssl pkcs12 -info -in {{ output_dir }}/ansible_no_pkey.p12 -nodes -passin pass:''" register: p12_validate_no_pkey +- name: 'Validate PKCS#12 with multiple certs' + shell: "openssl pkcs12 -info -in {{ output_dir }}/ansible_multi_certs.p12 -nodes -passin pass:'' | grep subject" + register: p12_validate_multi_certs + - name: 'Validate PKCS#12 (assert)' assert: that: @@ -21,6 +19,11 @@ - p12_validate_no_pkey.stdout_lines[-1] == '-----END CERTIFICATE-----' - p12_force.changed - p12_force_and_mode.mode == '0644' and p12_force_and_mode.changed + - not p12_standard_idempotency.changed + - not p12_multiple_certs_idempotency.changed + - "'www.' in p12_validate_multi_certs.stdout" + - "'www2.' in p12_validate_multi_certs.stdout" + - "'www3.' in p12_validate_multi_certs.stdout" - name: Check passphrase on private key assert: