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

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``. "

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
 =======
 

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):

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'

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: