Repository: ansible/ansible
Pull request: openssl_pkcs12: Add idempotency checks
##### SUMMARY
Adds idempotency checks to the openssl_pkcs12 module and related tests. 
Also decoupled `parse` and `generate` from the file write, as they are now used for multiple stuff that don't require the file to be written to disk.
Fixes #53221

##### ISSUE TYPE
- Bugfix Pull Request

##### COMPONENT NAME
openssl_pkcs12



Problem statement:
openssl_pkcs12: idempotency check is not working
##### SUMMARY
The idempotency check of `openssl_pkcs12` is totally broken. It currently only checks whether the output file is there and has the correct permissions, and nothing else. There is code for a test which checks whether the private key is readable (with the provided passphrase), but that code is never executed (since `()` is missing in the `return` line).

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
openssl_pkcs12

##### ANSIBLE VERSION
```paste below
2.7.x
```

Hints from issue discussion:
Files identified in the description:
None

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
!component =lib/ansible/modules/crypto/openssl_pkcs12.py
Files identified in the description:
* [lib/ansible/modules/crypto/openssl_pkcs12.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/crypto/openssl_pkcs12.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @MarkusTeufelberger @Shaps @Spredzy @Xyon @gdelpierre @puiterwijk @resmo
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
Having a look at this.

Commit message: Added changelog and porting guide

Patch to review:
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 00000000000000..c575f999280f0c
--- /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 4681e3458d0d1c..485a9f09a386f5 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,12 @@ 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 ``win_domain_membership`` module will no longer automatically join a host in a domain that already has an account
+  with the same name. Set ``allow_existing_computer_account=yes`` to override this check and go back to the original
+  behaviour.
+
+* 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 41ed70313983b5..77c980b95bfd22 100644
--- a/lib/ansible/modules/crypto/openssl_pkcs12.py
+++ b/lib/ansible/modules/crypto/openssl_pkcs12.py
@@ -29,10 +29,11 @@
         type: str
         default: export
         choices: [ export, parse ]
-    ca_certificates:
+    other_certificates:
         description:
-            - List of CA certificate to include.
+            - List of other certificates to include.
         type: list
+        aliases: [ ca_certificates ]
     certificate_path:
         description:
             - The path to read certificates and private keys from.
@@ -201,7 +202,7 @@ def __init__(self, module):
             module.check_mode
         )
         self.action = module.params['action']
-        self.ca_certificates = module.params['ca_certificates']
+        self.ca_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,48 @@ def _check_pkey_passphrase():
         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.ca_certificates is not None):
+                expected_ca_certs = [crypto.dump_certificate(crypto.FILETYPE_PEM,
+                                                             ca_cert) for ca_cert in self.pkcs12.get_ca_certificates()]
+                if set(pkcs12_other_certificates) != set(expected_ca_certs):
+                    return False
+            elif bool(pkcs12_other_certificates) != bool(self.ca_certificates):
+                return False
+
+            if pkcs12_privatekey:
+                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,7 +296,6 @@ def dump(self):
 
     def generate(self, module):
         """Generate PKCS#12 file archive."""
-
         self.pkcs12 = crypto.PKCS12()
 
         if self.ca_certificates:
@@ -278,20 +319,14 @@ def generate(self, module):
             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 +338,29 @@ def parse(self, module):
                                           p12.get_privatekey())
             crt = crypto.dump_certificate(crypto.FILETYPE_PEM,
                                           p12.get_certificate())
+            ca_certs = []
+            if p12.get_ca_certificates() is not None:
+                ca_certs = [crypto.dump_certificate(crypto.FILETYPE_PEM,
+                                                    ca_cert) for ca_cert in p12.get_ca_certificates()]
 
-            crypto_utils.write_file(module, b'%s%s' % (pkey, crt))
+            friendly_name = p12.get_friendlyname()
+
+            return (pkey, crt, ca_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 +410,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, ca_certs, friendly_name = pkcs12.parse()
+                    dump_content = '%s%s%s' % (to_native(pkey), to_native(cert), to_native(b''.join(ca_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 543186100214db..492724fa3994b6 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 da88d0c7b7e00d..37d6d72118a952 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:


Code review comments:
diff_hunk: @@ -303,17 +338,29 @@ def parse(self, module):
                                           p12.get_privatekey())
             crt = crypto.dump_certificate(crypto.FILETYPE_PEM,
                                           p12.get_certificate())
+            ca_certs = []
path: lib/ansible/modules/crypto/openssl_pkcs12.py
text: @user1:
I guess this should also be renamed?

diff_hunk: @@ -333,6 +333,12 @@ 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 ``win_domain_membership`` module will no longer automatically join a host in a domain that already has an account
path: docs/docsite/rst/porting_guides/porting_guide_2.8.rst
text: @user1:
This shouldn't be in this PR.