Avoid crash in check mode (#243)

* Do not let AnsibleModule crash when setting permissions on not yet existing files in check mode. * Add tests. * Fix bugs.
2021-06-02 16:44:26 +02:00 · 2021-06-02 16:44:26 +02:00 · 376d7cde12
parent a466df9c52
commit 376d7cde12
21 changed files with 181 additions and 18 deletions
--- a/changelogs/fragments/243-permission-check-crash.yml
+++ b/changelogs/fragments/243-permission-check-crash.yml
@ -0,0 +1,4 @@
+bugfixes:
+- "various modules - prevent crashes when modules try to set attributes on not yet existing files in check mode.
+  This will be fixed in ansible-core 2.12, but it is not backported to every Ansible version we support
+  (https://github.com/ansible-collections/community.crypto/issue/242, https://github.com/ansible-collections/community.crypto/pull/243)."
--- a/plugins/module_utils/crypto/support.py
+++ b/plugins/module_utils/crypto/support.py
@ -371,6 +371,8 @@ class OpenSSLObject(object):

        def _check_perms(module):
            file_args = module.load_file_common_arguments(module.params)
+            if module.check_file_absent_if_check_mode(file_args['path']):
+                return False
            return not module.set_fs_attributes_if_different(file_args, False)

        if not perms_required:
--- a/plugins/module_utils/io.py
+++ b/plugins/module_utils/io.py
@ -92,7 +92,8 @@ def write_file(module, content, default_mode=None, path=None):
        # Move tempfile to final destination
        module.atomic_move(tmp_name, file_args['path'])
        # Try to update permissions again
-        module.set_fs_attributes_if_different(file_args, False)
+        if not module.check_file_absent_if_check_mode(file_args['path']):
+            module.set_fs_attributes_if_different(file_args, False)
    except Exception as e:
        try:
            os.remove(tmp_name)
--- a/plugins/module_utils/openssh/backends/keypair_backend.py
+++ b/plugins/module_utils/openssh/backends/keypair_backend.py
@ -207,6 +207,8 @@ class KeypairBackend(object):
        file_args = self.module.load_file_common_arguments(self.module.params)
        if public_key:
            file_args['path'] = file_args['path'] + '.pub'
+        if self.module.check_file_absent_if_check_mode(file_args['path']):
+            return True
        return self.module.set_fs_attributes_if_different(file_args, False)

    @property
--- a/plugins/modules/openssh_cert.py
+++ b/plugins/modules/openssh_cert.py
@ -364,7 +364,9 @@ class Certificate(object):
                module.fail_json(msg="%s" % to_native(e))

        file_args = module.load_file_common_arguments(module.params)
-        if module.set_fs_attributes_if_different(file_args, False):
+        if module.check_file_absent_if_check_mode(file_args['path']):
+            self.changed = True
+        elif module.set_fs_attributes_if_different(file_args, False):
            self.changed = True

    def convert_to_datetime(self, module, timestring):
--- a/plugins/modules/openssl_csr.py
+++ b/plugins/modules/openssl_csr.py
@ -286,7 +286,10 @@ class CertificateSigningRequestModule(OpenSSLObject):
            self.changed = True

        file_args = module.load_file_common_arguments(module.params)
-        self.changed = module.set_fs_attributes_if_different(file_args, self.changed)
+        if module.check_file_absent_if_check_mode(file_args['path']):
+            self.changed = True
+        else:
+            self.changed = module.set_fs_attributes_if_different(file_args, self.changed)

    def remove(self, module):
        self.module_backend.set_existing(None)
--- a/plugins/modules/openssl_dhparam.py
+++ b/plugins/modules/openssl_dhparam.py
@ -221,9 +221,9 @@ class DHParameterBase(object):
    def _check_fs_attributes(self, module):
        """Checks (and changes if not in check mode!) fs attributes"""
        file_args = module.load_file_common_arguments(module.params)
-        attrs_changed = module.set_fs_attributes_if_different(file_args, False)
-
-        return not attrs_changed
+        if module.check_file_absent_if_check_mode(file_args['path']):
+            return False
+        return not module.set_fs_attributes_if_different(file_args, False)

    def dump(self):
        """Serialize the object into a dictionary."""
--- a/plugins/modules/openssl_pkcs12.py
+++ b/plugins/modules/openssl_pkcs12.py
@ -733,7 +733,9 @@ def main():
                    changed = True

            file_args = module.load_file_common_arguments(module.params)
-            if module.set_fs_attributes_if_different(file_args, changed):
+            if module.check_file_absent_if_check_mode(file_args['path']):
+                changed = True
+            elif module.set_fs_attributes_if_different(file_args, changed):
                changed = True
        else:
            if module.check_mode:
--- a/plugins/modules/openssl_privatekey.py
+++ b/plugins/modules/openssl_privatekey.py
@ -214,7 +214,10 @@ class PrivateKeyModule(OpenSSLObject):
            self.changed = True

        file_args = module.load_file_common_arguments(module.params)
-        self.changed = module.set_fs_attributes_if_different(file_args, self.changed)
+        if module.check_file_absent_if_check_mode(file_args['path']):
+            self.changed = True
+        else:
+            self.changed = module.set_fs_attributes_if_different(file_args, self.changed)

    def remove(self, module):
        self.module_backend.set_existing(None)
--- a/plugins/modules/openssl_publickey.py
+++ b/plugins/modules/openssl_publickey.py
@ -338,7 +338,9 @@ class PublicKey(OpenSSLObject):
            backend=self.backend,
        )
        file_args = module.load_file_common_arguments(module.params)
-        if module.set_fs_attributes_if_different(file_args, False):
+        if module.check_file_absent_if_check_mode(file_args['path']):
+            self.changed = True
+        elif module.set_fs_attributes_if_different(file_args, False):
            self.changed = True

    def check(self, module, perms_required=True):
--- a/plugins/modules/x509_certificate.py
+++ b/plugins/modules/x509_certificate.py
@ -472,7 +472,10 @@ class GenericCertificate(OpenSSLObject):
            self.changed = True

        file_args = module.load_file_common_arguments(module.params)
-        self.changed = module.set_fs_attributes_if_different(file_args, self.changed)
+        if module.check_file_absent_if_check_mode(file_args['path']):
+            self.changed = True
+        else:
+            self.changed = module.set_fs_attributes_if_different(file_args, self.changed)

    def check(self, module, perms_required=True):
        """Ensure the resource is in its desired state."""
--- a/plugins/modules/x509_crl.py
+++ b/plugins/modules/x509_crl.py
@ -710,7 +710,9 @@ class CRL(OpenSSLObject):
            self.changed = True

        file_args = self.module.load_file_common_arguments(self.module.params)
-        if self.module.set_fs_attributes_if_different(file_args, False):
+        if self.module.check_file_absent_if_check_mode(file_args['path']):
+            self.changed = True
+        elif self.module.set_fs_attributes_if_different(file_args, False):
            self.changed = True

    def dump(self, check_mode=False):
--- a/tests/integration/targets/openssl_dhparam/tasks/impl.yml
+++ b/tests/integration/targets/openssl_dhparam/tasks/impl.yml
@ -1,6 +1,15 @@
 ---
 # The tests for this module generate unsafe parameters for testing purposes;
 # otherwise tests would be too slow. Use sizes of at least 2048 in production!
+- name: "[{{ select_crypto_backend }}] Generate parameter (check mode)"
+  openssl_dhparam:
+    size: 768
+    path: '{{ output_dir }}/dh768.pem'
+    select_crypto_backend: "{{ select_crypto_backend }}"
+    return_content: yes
+  check_mode: true
+  register: dhparam_check
+
 - name: "[{{ select_crypto_backend }}] Generate parameter"
  openssl_dhparam:
    size: 768
@ -9,6 +18,15 @@
    return_content: yes
  register: dhparam

+- name: "[{{ select_crypto_backend }}] Don't regenerate parameters with no change (check mode)"
+  openssl_dhparam:
+    size: 768
+    path: '{{ output_dir }}/dh768.pem'
+    select_crypto_backend: "{{ select_crypto_backend }}"
+    return_content: yes
+  check_mode: true
+  register: dhparam_changed_check
+
 - name: "[{{ select_crypto_backend }}] Don't regenerate parameters with no change"
  openssl_dhparam:
    size: 768
--- a/tests/integration/targets/openssl_dhparam/tests/validate.yml
+++ b/tests/integration/targets/openssl_dhparam/tests/validate.yml
@ -26,6 +26,9 @@
 - name: "[{{ select_crypto_backend }}] Check if changed works correctly"
  assert:
    that:
+      - dhparam_check is changed
+      - dhparam is changed
+      - dhparam_changed_check is not changed
      - dhparam_changed is not changed
      - dhparam_changed_512 is not changed
      - dhparam_changed_to_512 is changed
--- a/tests/integration/targets/openssl_pkcs12/tasks/impl.yml
+++ b/tests/integration/targets/openssl_pkcs12/tasks/impl.yml
@ -1,4 +1,16 @@
 - block:
+  - name: "({{ select_crypto_backend }}) Generate PKCS#12 file (check mode)"
+    openssl_pkcs12:
+      select_crypto_backend: '{{ select_crypto_backend }}'
+      path: '{{ output_dir }}/ansible.p12'
+      friendly_name: abracadabra
+      privatekey_path: '{{ output_dir }}/ansible_pkey1.pem'
+      certificate_path: '{{ output_dir }}/ansible1.crt'
+      state: present
+      return_content: true
+    check_mode: true
+    register: p12_standard_check
+
  - name: "({{ select_crypto_backend }}) Generate PKCS#12 file"
    openssl_pkcs12:
      select_crypto_backend: '{{ select_crypto_backend }}'
@ -10,6 +22,18 @@
      return_content: true
    register: p12_standard

+  - name: "({{ select_crypto_backend }}) Generate PKCS#12 file again, idempotency (check mode)"
+    openssl_pkcs12:
+      select_crypto_backend: '{{ select_crypto_backend }}'
+      path: '{{ output_dir }}/ansible.p12'
+      friendly_name: abracadabra
+      privatekey_path: '{{ output_dir }}/ansible_pkey1.pem'
+      certificate_path: '{{ output_dir }}/ansible1.crt'
+      state: present
+      return_content: true
+    check_mode: true
+    register: p12_standard_idempotency_check
+
  - name: "({{ select_crypto_backend }}) Generate PKCS#12 file again, idempotency"
    openssl_pkcs12:
      select_crypto_backend: '{{ select_crypto_backend }}'
--- a/tests/integration/targets/openssl_pkcs12/tests/validate.yml
+++ b/tests/integration/targets/openssl_pkcs12/tests/validate.yml
@ -14,17 +14,20 @@
 - name: '({{ select_crypto_backend }}) Validate PKCS#12 (assert)'
  assert:
    that:
+      - p12_standard_check is changed
+      - p12_standard is changed
      - p12.stdout_lines[2].split(':')[-1].strip() == 'abracadabra'
      - p12_standard.mode == '0400'
-      - p12_no_pkey.changed
+      - p12_no_pkey is changed
      - p12_validate_no_pkey.stdout_lines[-1] == '-----END CERTIFICATE-----'
-      - p12_force.changed
+      - p12_force is changed
      - p12_force_and_mode.mode == '0644' and p12_force_and_mode.changed
-      - p12_dumped.changed
-      - not p12_standard_idempotency.changed
-      - not p12_multiple_certs_idempotency.changed
-      - not p12_dumped_idempotency.changed
-      - not p12_dumped_check_mode.changed
+      - p12_dumped is changed
+      - p12_standard_idempotency is not changed
+      - p12_standard_idempotency_check is not changed
+      - p12_multiple_certs_idempotency is not changed
+      - p12_dumped_idempotency is not changed
+      - p12_dumped_check_mode is not changed
      - "'www1.' in p12_validate_multi_certs.stdout"
      - "'www2.' in p12_validate_multi_certs.stdout"
      - "'www3.' in p12_validate_multi_certs.stdout"
--- a/tests/integration/targets/openssl_privatekey/tasks/impl.yml
+++ b/tests/integration/targets/openssl_privatekey/tasks/impl.yml
@ -1,4 +1,12 @@
 ---
+- name: "({{ select_crypto_backend }}) Generate privatekey1 - standard (check mode)"
+  openssl_privatekey:
+    path: '{{ output_dir }}/privatekey1.pem'
+    select_crypto_backend: '{{ select_crypto_backend }}'
+    return_content: yes
+  check_mode: true
+  register: privatekey1_check
+
 - name: "({{ select_crypto_backend }}) Generate privatekey1 - standard"
  openssl_privatekey:
    path: '{{ output_dir }}/privatekey1.pem'
@ -6,6 +14,14 @@
    return_content: yes
  register: privatekey1

+- name: "({{ select_crypto_backend }}) Generate privatekey1 - standard (idempotence, check mode)"
+  openssl_privatekey:
+    path: '{{ output_dir }}/privatekey1.pem'
+    select_crypto_backend: '{{ select_crypto_backend }}'
+    return_content: yes
+  check_mode: true
+  register: privatekey1_idempotence_check
+
 - name: "({{ select_crypto_backend }}) Generate privatekey1 - standard (idempotence)"
  openssl_privatekey:
    path: '{{ output_dir }}/privatekey1.pem'
--- a/tests/integration/targets/openssl_privatekey/tests/validate.yml
+++ b/tests/integration/targets/openssl_privatekey/tests/validate.yml
@ -5,6 +5,9 @@
 - name: "({{ select_crypto_backend }}) Validate privatekey1 idempotency and content returned"
  assert:
    that:
+      - privatekey1_check is changed
+      - privatekey1 is changed
+      - privatekey1_idempotence_check is not changed
      - privatekey1_idempotence is not changed
      - privatekey1.privatekey == lookup('file', output_dir ~ '/privatekey1.pem', rstrip=False)
      - privatekey1.privatekey == privatekey1_idempotence.privatekey
--- a/tests/integration/targets/openssl_publickey/tasks/impl.yml
+++ b/tests/integration/targets/openssl_publickey/tasks/impl.yml
@ -4,6 +4,15 @@
    path: '{{ output_dir }}/privatekey.pem'
    size: '{{ default_rsa_key_size }}'

+- name: "({{ select_crypto_backend }}) Generate publickey - PEM format (check mode)"
+  openssl_publickey:
+    path: '{{ output_dir }}/publickey.pub'
+    privatekey_path: '{{ output_dir }}/privatekey.pem'
+    select_crypto_backend: '{{ select_crypto_backend }}'
+    return_content: yes
+  check_mode: true
+  register: publickey_check
+
 - name: "({{ select_crypto_backend }}) Generate publickey - PEM format"
  openssl_publickey:
    path: '{{ output_dir }}/publickey.pub'
@ -12,6 +21,15 @@
    return_content: yes
  register: publickey

+- name: "({{ select_crypto_backend }}) Generate publickey - PEM format (check mode, idempotence)"
+  openssl_publickey:
+    path: '{{ output_dir }}/publickey.pub'
+    privatekey_path: '{{ output_dir }}/privatekey.pem'
+    select_crypto_backend: '{{ select_crypto_backend }}'
+    return_content: yes
+  check_mode: true
+  register: publickey_check2
+
 - name: "({{ select_crypto_backend }}) Generate publickey - PEM format (idempotence)"
  openssl_publickey:
    path: '{{ output_dir }}/publickey.pub'
@ -20,6 +38,14 @@
    return_content: yes
  register: publickey_idempotence

+- name: "({{ select_crypto_backend }}) Verify check mode"
+  assert:
+    that:
+      - publickey_check is changed
+      - publickey is changed
+      - publickey_check2 is not changed
+      - publickey_idempotence is not changed
+
 - name: "({{ select_crypto_backend }}) Generate publickey - OpenSSH format"
  openssl_publickey:
    path: '{{ output_dir }}/publickey-ssh.pub'
--- a/tests/integration/targets/x509_certificate/tasks/ownca.yml
+++ b/tests/integration/targets/x509_certificate/tasks/ownca.yml
@ -35,6 +35,17 @@
    - 'CA:TRUE'
    basic_constraints_critical: yes

+- name: (OwnCA, {{select_crypto_backend}}) Generate selfsigned CA certificate (check mode)
+  x509_certificate:
+    path: '{{ output_dir }}/ca_cert.pem'
+    csr_path: '{{ output_dir }}/ca_csr.csr'
+    privatekey_path: '{{ output_dir }}/ca_privatekey.pem'
+    provider: selfsigned
+    selfsigned_digest: sha256
+    select_crypto_backend: '{{ select_crypto_backend }}'
+  check_mode: true
+  register: result_check_mode
+
 - name: (OwnCA, {{select_crypto_backend}}) Generate selfsigned CA certificate
  x509_certificate:
    path: '{{ output_dir }}/ca_cert.pem'
@ -43,6 +54,13 @@
    provider: selfsigned
    selfsigned_digest: sha256
    select_crypto_backend: '{{ select_crypto_backend }}'
+  register: result
+
+- name: (OwnCA, {{select_crypto_backend}}) Verify changed
+  assert:
+    that:
+      - result_check_mode is changed
+      - result is changed

 - name: (OwnCA, {{select_crypto_backend}}) Generate selfsigned CA certificate (privatekey passphrase)
  x509_certificate:
--- a/tests/integration/targets/x509_crl/tasks/impl.yml
+++ b/tests/integration/targets/x509_crl/tasks/impl.yml
@ -20,6 +20,27 @@
  check_mode: yes
  register: crl_1_check

+- name: Create CRL 1 (check mode)
+  x509_crl:
+    path: '{{ output_dir }}/ca-crl1.crl'
+    privatekey_path: '{{ output_dir }}/ca.key'
+    issuer:
+      CN: Ansible
+    last_update: 20191013000000Z
+    next_update: 20191113000000Z
+    revoked_certificates:
+      - path: '{{ output_dir }}/cert-1.pem'
+        revocation_date: 20191013000000Z
+      - path: '{{ output_dir }}/cert-2.pem'
+        revocation_date: 20191013000000Z
+        reason: key_compromise
+        reason_critical: yes
+        invalidity_date: 20191012000000Z
+      - serial_number: 1234
+        revocation_date: 20191001000000Z
+  check_mode: true
+  register: crl_1_check
+
 - name: Create CRL 1
  x509_crl:
    path: '{{ output_dir }}/ca-crl1.crl'
@ -40,6 +61,11 @@
        revocation_date: 20191001000000Z
  register: crl_1

+- assert:
+    that:
+      - crl_1_check is changed
+      - crl_1 is changed
+
 - name: Retrieve CRL 1 infos
  x509_crl_info:
    path: '{{ output_dir }}/ca-crl1.crl'