From 8438aba0b5223da61fb61fe9924a90c7895f153a Mon Sep 17 00:00:00 2001 From: David Kainz Date: Thu, 13 Jun 2019 16:18:38 +0200 Subject: [PATCH 01/11] openssh_keypair: bugfix make regenerating keypairs via force possible / add invalid file handling --- lib/ansible/modules/crypto/openssh_keypair.py | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/lib/ansible/modules/crypto/openssh_keypair.py b/lib/ansible/modules/crypto/openssh_keypair.py index 06722b86ce45fe..20d397f43de495 100644 --- a/lib/ansible/modules/crypto/openssh_keypair.py +++ b/lib/ansible/modules/crypto/openssh_keypair.py @@ -181,7 +181,7 @@ def generate(self, module): try: self.changed = True - module.run_command(args) + module.run_command(args, data="y") proc = module.run_command([module.get_bin_path('ssh-keygen', True), '-lf', self.path]) self.fingerprint = proc[1].split() pubkey = module.run_command([module.get_bin_path('ssh-keygen', True), '-yf', self.path]) @@ -202,6 +202,25 @@ def _check_state(): if _check_state(): proc = module.run_command([module.get_bin_path('ssh-keygen', True), '-lf', self.path]) + if not proc[0] == 0: + if not os.path.isdir(self.path) and os.access(self.path, os.W_OK): + if not self.force: + module.fail_json( + msg='%s seems not to be a valid key file. If you want this module to overwrite ' + 'the file at %s please specify the force parameter.' % (self.path, self.path)) + else: + return False + elif not os.path.isdir(self.path) and not os.access(self.path, os.W_OK): + if not self.force: + module.fail_json( + msg='%s seems not to be write-able. If you want this module to (re)generate the ' + 'keypair anyways please specify the force parameter.' % (self.path)) + else: + os.remove(self.path) + return False + else: + module.fail_json(msg='%s is a directory. Please specify a path to a file.' % (self.path)) + fingerprint = proc[1].split() pubkey = module.run_command([module.get_bin_path('ssh-keygen', True), '-yf', self.path]) pubkey = pubkey[1].strip('\n') From f210f5598b7094dc858135cfc48eb8a87c7c5cc2 Mon Sep 17 00:00:00 2001 From: David Kainz Date: Sun, 16 Jun 2019 21:58:44 +0200 Subject: [PATCH 02/11] openssh_keypair: change permissions of read-only file instead of deleting it for regeneration; add changelog fragment --- ...nssh_keypair-fix-make-regenerating-valid-files-possible.yml | 2 ++ lib/ansible/modules/crypto/openssh_keypair.py | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) create mode 100644 changelogs/fragments/57801-openssh_keypair-fix-make-regenerating-valid-files-possible.yml diff --git a/changelogs/fragments/57801-openssh_keypair-fix-make-regenerating-valid-files-possible.yml b/changelogs/fragments/57801-openssh_keypair-fix-make-regenerating-valid-files-possible.yml new file mode 100644 index 00000000000000..13ccdde2ae52dd --- /dev/null +++ b/changelogs/fragments/57801-openssh_keypair-fix-make-regenerating-valid-files-possible.yml @@ -0,0 +1,2 @@ +bugfixes: + - openssh_keypair - make regeneration of valid keypairs with force possible, add better handling for invalid files diff --git a/lib/ansible/modules/crypto/openssh_keypair.py b/lib/ansible/modules/crypto/openssh_keypair.py index 20d397f43de495..18aa2442f9f701 100644 --- a/lib/ansible/modules/crypto/openssh_keypair.py +++ b/lib/ansible/modules/crypto/openssh_keypair.py @@ -116,6 +116,7 @@ ''' import os +import stat import errno from ansible.module_utils.basic import AnsibleModule @@ -216,7 +217,7 @@ def _check_state(): msg='%s seems not to be write-able. If you want this module to (re)generate the ' 'keypair anyways please specify the force parameter.' % (self.path)) else: - os.remove(self.path) + os.chmod(self.path, stat.S_IWUSR + stat.S_IRUSR) return False else: module.fail_json(msg='%s is a directory. Please specify a path to a file.' % (self.path)) From 64ba2451f567dde3f56e5a2eeb5509ef0a5888b5 Mon Sep 17 00:00:00 2001 From: David Kainz Date: Fri, 21 Jun 2019 15:36:11 +0200 Subject: [PATCH 03/11] address review feedbak, refactor --- lib/ansible/modules/crypto/openssh_keypair.py | 24 +++++++------------ 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/lib/ansible/modules/crypto/openssh_keypair.py b/lib/ansible/modules/crypto/openssh_keypair.py index 18aa2442f9f701..d230574104470a 100644 --- a/lib/ansible/modules/crypto/openssh_keypair.py +++ b/lib/ansible/modules/crypto/openssh_keypair.py @@ -181,6 +181,8 @@ def generate(self, module): args.extend(['-C', ""]) try: + if os.path.exists(self.path) and not os.access(self.path, os.W_OK): + os.chmod(self.path, stat.S_IWUSR + stat.S_IRUSR) self.changed = True module.run_command(args, data="y") proc = module.run_command([module.get_bin_path('ssh-keygen', True), '-lf', self.path]) @@ -204,23 +206,15 @@ def _check_state(): if _check_state(): proc = module.run_command([module.get_bin_path('ssh-keygen', True), '-lf', self.path]) if not proc[0] == 0: - if not os.path.isdir(self.path) and os.access(self.path, os.W_OK): - if not self.force: - module.fail_json( + if os.path.isdir(self.path): + module.fail_json(msg='%s is a directory. Please specify a path to a file.' % (self.path)) + + if os.access(self.path, os.W_OK) and not self.force: + module.fail_json( msg='%s seems not to be a valid key file. If you want this module to overwrite ' - 'the file at %s please specify the force parameter.' % (self.path, self.path)) - else: - return False - elif not os.path.isdir(self.path) and not os.access(self.path, os.W_OK): - if not self.force: - module.fail_json( - msg='%s seems not to be write-able. If you want this module to (re)generate the ' - 'keypair anyways please specify the force parameter.' % (self.path)) - else: - os.chmod(self.path, stat.S_IWUSR + stat.S_IRUSR) - return False + 'the file at %s please specify the force parameter.' % (self.path, self.path)) else: - module.fail_json(msg='%s is a directory. Please specify a path to a file.' % (self.path)) + return False fingerprint = proc[1].split() pubkey = module.run_command([module.get_bin_path('ssh-keygen', True), '-yf', self.path]) From afe2cbd4ed94d2566a4c7f3849b3df3a1673644f Mon Sep 17 00:00:00 2001 From: David Kainz Date: Fri, 21 Jun 2019 16:36:10 +0200 Subject: [PATCH 04/11] add integration tests for bigfixes --- .../targets/openssh_keypair/tasks/main.yml | 40 +++++++++++++++++++ .../openssh_keypair/tests/validate.yml | 18 +++++++++ 2 files changed, 58 insertions(+) diff --git a/test/integration/targets/openssh_keypair/tasks/main.yml b/test/integration/targets/openssh_keypair/tasks/main.yml index e81f620962f6e5..25116c63a94268 100644 --- a/test/integration/targets/openssh_keypair/tasks/main.yml +++ b/test/integration/targets/openssh_keypair/tasks/main.yml @@ -27,4 +27,44 @@ path: '{{ output_dir }}/privatekey5' register: publickey_gen +- name: Generate privatekey6 + openssh_keypair: + path: '{{ output_dir }}/privatekey6' + type: rsa + +- name: Regenerate privatekey6 via force + openssh_keypair: + path: '{{ output_dir }}/privatekey6' + type: rsa + force: yes + register: output_regenerated_via_force + +- name: Create broken key + copy: + dest: '{{ item }}' + content: '' + mode: '0700' + loop: + - '{{ output_dir }}/privatekeybroken' + - '{{ output_dir }}/privatekeybroken.pub' + +- name: Regenerate broken key + openssh_keypair: + path: '{{ output_dir }}/privatekeybroken' + type: rsa + register: output_broken + +- name: Generate read-only private key + openssh_keypair: + path: '{{ output_dir }}/privatekeyreadonly' + type: rsa + mode: '0200' + +- name: Regenerate read-only private key via force + openssh_keypair: + path: '{{ output_dir }}/privatekeyreadonly' + type: rsa + force: yes + register: output_read_only + - import_tasks: ../tests/validate.yml diff --git a/test/integration/targets/openssh_keypair/tests/validate.yml b/test/integration/targets/openssh_keypair/tests/validate.yml index 47fa6259ff8fdd..266665a4548c7c 100644 --- a/test/integration/targets/openssh_keypair/tests/validate.yml +++ b/test/integration/targets/openssh_keypair/tests/validate.yml @@ -1,3 +1,4 @@ +--- - name: Log privatekey1 return values debug: var: privatekey1_result @@ -73,3 +74,20 @@ assert: that: - "publickey_gen.public_key == lookup('file', output_dir ~ '/privatekey5.pub').strip('\n')" + +- name: Verify that privatekey6 will be regenerated via force + assert: + that: + - output_regenerated_via_force is changed + + +- name: Verify that broken key will be regenerated + assert: + that: + - output_broken is changed + + +- name: Verify that read-only key will be regenerated + assert: + that: + - output_read_only is changed \ No newline at end of file From 09d7f9a26e930a4920237122d2d3ba83d7b6f9c2 Mon Sep 17 00:00:00 2001 From: David Kainz Date: Fri, 21 Jun 2019 16:37:54 +0200 Subject: [PATCH 05/11] linter: fix indent --- lib/ansible/modules/crypto/openssh_keypair.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ansible/modules/crypto/openssh_keypair.py b/lib/ansible/modules/crypto/openssh_keypair.py index d230574104470a..e98539e7e7f6a4 100644 --- a/lib/ansible/modules/crypto/openssh_keypair.py +++ b/lib/ansible/modules/crypto/openssh_keypair.py @@ -212,7 +212,7 @@ def _check_state(): if os.access(self.path, os.W_OK) and not self.force: module.fail_json( msg='%s seems not to be a valid key file. If you want this module to overwrite ' - 'the file at %s please specify the force parameter.' % (self.path, self.path)) + 'the file at %s please specify the force parameter.' % (self.path, self.path)) else: return False From 277afbb3f9bb6450058d20cd449442959d6ec914 Mon Sep 17 00:00:00 2001 From: David Kainz Date: Fri, 21 Jun 2019 16:46:06 +0200 Subject: [PATCH 06/11] fixup integration tests: use force when regenerating an invalid file --- test/integration/targets/openssh_keypair/tasks/main.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/test/integration/targets/openssh_keypair/tasks/main.yml b/test/integration/targets/openssh_keypair/tasks/main.yml index 25116c63a94268..c2c32d7144cfd5 100644 --- a/test/integration/targets/openssh_keypair/tasks/main.yml +++ b/test/integration/targets/openssh_keypair/tasks/main.yml @@ -52,6 +52,7 @@ openssh_keypair: path: '{{ output_dir }}/privatekeybroken' type: rsa + force: yes register: output_broken - name: Generate read-only private key From ff9dfd59fa4da337ba8890c547d01de1c1e01c10 Mon Sep 17 00:00:00 2001 From: David Kainz Date: Fri, 21 Jun 2019 17:09:27 +0200 Subject: [PATCH 07/11] linter: fix indent --- lib/ansible/modules/crypto/openssh_keypair.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/ansible/modules/crypto/openssh_keypair.py b/lib/ansible/modules/crypto/openssh_keypair.py index e98539e7e7f6a4..1567b5b7865836 100644 --- a/lib/ansible/modules/crypto/openssh_keypair.py +++ b/lib/ansible/modules/crypto/openssh_keypair.py @@ -211,8 +211,8 @@ def _check_state(): if os.access(self.path, os.W_OK) and not self.force: module.fail_json( - msg='%s seems not to be a valid key file. If you want this module to overwrite ' - 'the file at %s please specify the force parameter.' % (self.path, self.path)) + msg='%s seems not to be a valid key file. If you want this module to overwrite ' + 'the file at %s please specify the force parameter.' % (self.path, self.path)) else: return False From 8d8f319ae2050c57de9bf8b88949135d0ad0fdac Mon Sep 17 00:00:00 2001 From: David Kainz Date: Mon, 24 Jun 2019 13:05:48 +0200 Subject: [PATCH 08/11] openssh_keypair: address review feedback --- ..._keypair-fix-make-regenerating-valid-files-possible.yml | 2 +- lib/ansible/modules/crypto/openssh_keypair.py | 7 +------ test/integration/targets/openssh_keypair/tasks/main.yml | 1 - 3 files changed, 2 insertions(+), 8 deletions(-) diff --git a/changelogs/fragments/57801-openssh_keypair-fix-make-regenerating-valid-files-possible.yml b/changelogs/fragments/57801-openssh_keypair-fix-make-regenerating-valid-files-possible.yml index 13ccdde2ae52dd..461a32f344363b 100644 --- a/changelogs/fragments/57801-openssh_keypair-fix-make-regenerating-valid-files-possible.yml +++ b/changelogs/fragments/57801-openssh_keypair-fix-make-regenerating-valid-files-possible.yml @@ -1,2 +1,2 @@ bugfixes: - - openssh_keypair - make regeneration of valid keypairs with force possible, add better handling for invalid files + - openssh_keypair - make regeneration of valid keypairs with the ```force``` option possible, add better handling for invalid files diff --git a/lib/ansible/modules/crypto/openssh_keypair.py b/lib/ansible/modules/crypto/openssh_keypair.py index 1567b5b7865836..3fa3dad42864cb 100644 --- a/lib/ansible/modules/crypto/openssh_keypair.py +++ b/lib/ansible/modules/crypto/openssh_keypair.py @@ -209,12 +209,7 @@ def _check_state(): if os.path.isdir(self.path): module.fail_json(msg='%s is a directory. Please specify a path to a file.' % (self.path)) - if os.access(self.path, os.W_OK) and not self.force: - module.fail_json( - msg='%s seems not to be a valid key file. If you want this module to overwrite ' - 'the file at %s please specify the force parameter.' % (self.path, self.path)) - else: - return False + return False fingerprint = proc[1].split() pubkey = module.run_command([module.get_bin_path('ssh-keygen', True), '-yf', self.path]) diff --git a/test/integration/targets/openssh_keypair/tasks/main.yml b/test/integration/targets/openssh_keypair/tasks/main.yml index c2c32d7144cfd5..25116c63a94268 100644 --- a/test/integration/targets/openssh_keypair/tasks/main.yml +++ b/test/integration/targets/openssh_keypair/tasks/main.yml @@ -52,7 +52,6 @@ openssh_keypair: path: '{{ output_dir }}/privatekeybroken' type: rsa - force: yes register: output_broken - name: Generate read-only private key From 1b46482e81b2d2d6f8781545ffadbaf2a0afcc54 Mon Sep 17 00:00:00 2001 From: David Kainz Date: Mon, 24 Jun 2019 13:51:06 +0200 Subject: [PATCH 09/11] openssh_keypair: fixup, remove backtick --- ...enssh_keypair-fix-make-regenerating-valid-files-possible.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/fragments/57801-openssh_keypair-fix-make-regenerating-valid-files-possible.yml b/changelogs/fragments/57801-openssh_keypair-fix-make-regenerating-valid-files-possible.yml index 461a32f344363b..19308e129c2757 100644 --- a/changelogs/fragments/57801-openssh_keypair-fix-make-regenerating-valid-files-possible.yml +++ b/changelogs/fragments/57801-openssh_keypair-fix-make-regenerating-valid-files-possible.yml @@ -1,2 +1,2 @@ bugfixes: - - openssh_keypair - make regeneration of valid keypairs with the ```force``` option possible, add better handling for invalid files + - openssh_keypair - make regeneration of valid keypairs with the ``force`` option possible, add better handling for invalid files From da138ef3cd68b5f63579c7404647083ea544b021 Mon Sep 17 00:00:00 2001 From: David Kainz Date: Mon, 24 Jun 2019 17:08:26 +0200 Subject: [PATCH 10/11] openssh_keypair: address review feedback --- lib/ansible/modules/crypto/openssh_keypair.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ansible/modules/crypto/openssh_keypair.py b/lib/ansible/modules/crypto/openssh_keypair.py index 3fa3dad42864cb..7e3ca33c713931 100644 --- a/lib/ansible/modules/crypto/openssh_keypair.py +++ b/lib/ansible/modules/crypto/openssh_keypair.py @@ -204,7 +204,7 @@ def _check_state(): return os.path.exists(self.path) if _check_state(): - proc = module.run_command([module.get_bin_path('ssh-keygen', True), '-lf', self.path]) + proc = module.run_command([module.get_bin_path('ssh-keygen', True), '-lf', self.path], check_rc=False) if not proc[0] == 0: if os.path.isdir(self.path): module.fail_json(msg='%s is a directory. Please specify a path to a file.' % (self.path)) From cf009c5b0e8aba318f3f588c89adc22eb9954ea8 Mon Sep 17 00:00:00 2001 From: Felix Fontein Date: Mon, 24 Jun 2019 21:11:58 +0200 Subject: [PATCH 11/11] Only pass 'y' into stdin of ssh-keygen when file exists. --- lib/ansible/modules/crypto/openssh_keypair.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/ansible/modules/crypto/openssh_keypair.py b/lib/ansible/modules/crypto/openssh_keypair.py index 7e3ca33c713931..61c8513eb13cfd 100644 --- a/lib/ansible/modules/crypto/openssh_keypair.py +++ b/lib/ansible/modules/crypto/openssh_keypair.py @@ -184,7 +184,10 @@ def generate(self, module): if os.path.exists(self.path) and not os.access(self.path, os.W_OK): os.chmod(self.path, stat.S_IWUSR + stat.S_IRUSR) self.changed = True - module.run_command(args, data="y") + stdin_data = None + if os.path.exists(self.path): + stdin_data = 'y' + module.run_command(args, data=stdin_data) proc = module.run_command([module.get_bin_path('ssh-keygen', True), '-lf', self.path]) self.fingerprint = proc[1].split() pubkey = module.run_command([module.get_bin_path('ssh-keygen', True), '-yf', self.path])