From 5a1367ccbcb66166bcff8d62194c4e491332dd53 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Wed, 2 Jun 2021 13:05:26 +0200 Subject: [PATCH 1/3] Do not let AnsibleModule crash when setting permissions on not yet existing files in check mode. --- changelogs/fragments/243-permission-check-crash.yml | 4 ++++ plugins/module_utils/crypto/support.py | 2 ++ plugins/module_utils/io.py | 3 ++- plugins/module_utils/openssh/backends/keypair_backend.py | 2 ++ plugins/modules/openssh_cert.py | 4 +++- plugins/modules/openssl_csr.py | 5 ++++- plugins/modules/openssl_dhparam.py | 6 +++--- plugins/modules/openssl_pkcs12.py | 4 +++- plugins/modules/openssl_privatekey.py | 5 ++++- plugins/modules/openssl_publickey.py | 4 +++- plugins/modules/x509_certificate.py | 5 ++++- plugins/modules/x509_crl.py | 4 +++- 12 files changed, 37 insertions(+), 11 deletions(-) create mode 100644 changelogs/fragments/243-permission-check-crash.yml diff --git a/changelogs/fragments/243-permission-check-crash.yml b/changelogs/fragments/243-permission-check-crash.yml new file mode 100644 index 000000000..e24cc81ff --- /dev/null +++ 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)." diff --git a/plugins/module_utils/crypto/support.py b/plugins/module_utils/crypto/support.py index 0a3828e79..c65ccdab5 100644 --- a/plugins/module_utils/crypto/support.py +++ b/plugins/module_utils/crypto/support.py @@ -371,6 +371,8 @@ def _check_state(): 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: diff --git a/plugins/module_utils/io.py b/plugins/module_utils/io.py index debc499a2..c1c2643a0 100644 --- 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) diff --git a/plugins/module_utils/openssh/backends/keypair_backend.py b/plugins/module_utils/openssh/backends/keypair_backend.py index ce4a7ef66..6f8613b56 100644 --- a/plugins/module_utils/openssh/backends/keypair_backend.py +++ b/plugins/module_utils/openssh/backends/keypair_backend.py @@ -207,6 +207,8 @@ def _permissions_changed(self, public_key=False): 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 diff --git a/plugins/modules/openssh_cert.py b/plugins/modules/openssh_cert.py index 80a20e186..6ec7a5b9d 100644 --- a/plugins/modules/openssh_cert.py +++ b/plugins/modules/openssh_cert.py @@ -364,7 +364,9 @@ def generate(self, module): 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): diff --git a/plugins/modules/openssl_csr.py b/plugins/modules/openssl_csr.py index e9d375acb..48808cace 100644 --- a/plugins/modules/openssl_csr.py +++ b/plugins/modules/openssl_csr.py @@ -286,7 +286,10 @@ def generate(self, module): 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) diff --git a/plugins/modules/openssl_dhparam.py b/plugins/modules/openssl_dhparam.py index 484f7fbba..b7d620dda 100644 --- a/plugins/modules/openssl_dhparam.py +++ b/plugins/modules/openssl_dhparam.py @@ -221,9 +221,9 @@ def _check_params_valid(self, module): 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.""" diff --git a/plugins/modules/openssl_pkcs12.py b/plugins/modules/openssl_pkcs12.py index aee76ca74..26e1bea6b 100644 --- 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']): + self.changed = True + elif module.set_fs_attributes_if_different(file_args, changed): changed = True else: if module.check_mode: diff --git a/plugins/modules/openssl_privatekey.py b/plugins/modules/openssl_privatekey.py index b364a7547..37ca9ee00 100644 --- a/plugins/modules/openssl_privatekey.py +++ b/plugins/modules/openssl_privatekey.py @@ -214,7 +214,10 @@ def generate(self, module): 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) diff --git a/plugins/modules/openssl_publickey.py b/plugins/modules/openssl_publickey.py index a9b1c5968..47e87eb46 100644 --- a/plugins/modules/openssl_publickey.py +++ b/plugins/modules/openssl_publickey.py @@ -338,7 +338,9 @@ def generate(self, module): 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): diff --git a/plugins/modules/x509_certificate.py b/plugins/modules/x509_certificate.py index fb6fc2f5f..89422c135 100644 --- a/plugins/modules/x509_certificate.py +++ b/plugins/modules/x509_certificate.py @@ -472,7 +472,10 @@ def generate(self, module): 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.""" diff --git a/plugins/modules/x509_crl.py b/plugins/modules/x509_crl.py index eeb54e392..5652ccb98 100644 --- a/plugins/modules/x509_crl.py +++ b/plugins/modules/x509_crl.py @@ -710,7 +710,9 @@ def generate(self): 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 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): From 1ab02bc5f39b8d9f13ce2906031d76e14e0fda39 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Wed, 2 Jun 2021 13:21:20 +0200 Subject: [PATCH 2/3] Add tests. --- .../targets/openssl_dhparam/tasks/impl.yml | 18 +++++++++++++ .../openssl_dhparam/tests/validate.yml | 3 +++ .../targets/openssl_pkcs12/tasks/impl.yml | 24 +++++++++++++++++ .../targets/openssl_pkcs12/tests/validate.yml | 17 +++++++----- .../targets/openssl_privatekey/tasks/impl.yml | 16 ++++++++++++ .../openssl_privatekey/tests/validate.yml | 3 +++ .../targets/openssl_publickey/tasks/impl.yml | 26 +++++++++++++++++++ .../targets/x509_certificate/tasks/ownca.yml | 18 +++++++++++++ .../targets/x509_crl/tasks/impl.yml | 26 +++++++++++++++++++ 9 files changed, 144 insertions(+), 7 deletions(-) diff --git a/tests/integration/targets/openssl_dhparam/tasks/impl.yml b/tests/integration/targets/openssl_dhparam/tasks/impl.yml index d367436de..fcc26df55 100644 --- 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 diff --git a/tests/integration/targets/openssl_dhparam/tests/validate.yml b/tests/integration/targets/openssl_dhparam/tests/validate.yml index a9ed03ef3..9e717614b 100644 --- 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 diff --git a/tests/integration/targets/openssl_pkcs12/tasks/impl.yml b/tests/integration/targets/openssl_pkcs12/tasks/impl.yml index bd878b8b1..abb134e02 100644 --- 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 }}' diff --git a/tests/integration/targets/openssl_pkcs12/tests/validate.yml b/tests/integration/targets/openssl_pkcs12/tests/validate.yml index f3c524825..9f71a4938 100644 --- 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" diff --git a/tests/integration/targets/openssl_privatekey/tasks/impl.yml b/tests/integration/targets/openssl_privatekey/tasks/impl.yml index 6fb8def8d..8608935ba 100644 --- 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' diff --git a/tests/integration/targets/openssl_privatekey/tests/validate.yml b/tests/integration/targets/openssl_privatekey/tests/validate.yml index 5672d927b..bac06554a 100644 --- 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 diff --git a/tests/integration/targets/openssl_publickey/tasks/impl.yml b/tests/integration/targets/openssl_publickey/tasks/impl.yml index cfe930fda..51091075f 100644 --- 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' diff --git a/tests/integration/targets/x509_certificate/tasks/ownca.yml b/tests/integration/targets/x509_certificate/tasks/ownca.yml index 3feea15f1..7657caa98 100644 --- 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: diff --git a/tests/integration/targets/x509_crl/tasks/impl.yml b/tests/integration/targets/x509_crl/tasks/impl.yml index 77803d169..a4fd262ec 100644 --- 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' From 89b3a4cb0582cf4d31755380eb649cf304aa11f2 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Wed, 2 Jun 2021 13:22:39 +0200 Subject: [PATCH 3/3] Fix bugs. --- plugins/modules/openssl_pkcs12.py | 2 +- plugins/modules/x509_crl.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/modules/openssl_pkcs12.py b/plugins/modules/openssl_pkcs12.py index 26e1bea6b..b52b965e3 100644 --- a/plugins/modules/openssl_pkcs12.py +++ b/plugins/modules/openssl_pkcs12.py @@ -734,7 +734,7 @@ def main(): file_args = module.load_file_common_arguments(module.params) if module.check_file_absent_if_check_mode(file_args['path']): - self.changed = True + changed = True elif module.set_fs_attributes_if_different(file_args, changed): changed = True else: diff --git a/plugins/modules/x509_crl.py b/plugins/modules/x509_crl.py index 5652ccb98..532d86adc 100644 --- a/plugins/modules/x509_crl.py +++ b/plugins/modules/x509_crl.py @@ -710,7 +710,7 @@ def generate(self): self.changed = True file_args = self.module.load_file_common_arguments(self.module.params) - if module.check_file_absent_if_check_mode(file_args['path']): + 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