openssh_* modules: check return code on ssh(-keygen) invocations; fai…

…l if comment cannot be updated (#646)

* Check return code on ssh(-keygen) invocations.

* openssh_cert: only check for errors if certificate should be present and module is not in check mode.

* Handle rc check for _get_private_key().

* Add changelog fragment.

* Only pass -o for comment updating when necessary.

* Now fails if comment cannot be updated.

This was silently ignored in the past.

* Avoid failing operation.

changelogs/fragments/646-openssh-rc.yml

@@ -0,0 +1,5 @@
+bugfixes:
+  - "openssh_cert, openssh_keypair - the modules ignored return codes of ``ssh`` and ``ssh-keygen`` in some cases (https://github.com/ansible-collections/community.crypto/issues/645, https://github.com/ansible-collections/community.crypto/pull/646)."
+  - "openssh_keypair - fix comment updating for OpenSSH before 6.5 (https://github.com/ansible-collections/community.crypto/pull/646)."
+minor_changes:
+  - "openssh_keypair - fail when comment cannot be updated (https://github.com/ansible-collections/community.crypto/pull/646)."

plugins/module_utils/openssh/backends/common.py

@@ -127,7 +127,7 @@ def _get_ssh_version(self):
         ssh_bin = self.module.get_bin_path('ssh')
         if not ssh_bin:
             return ""
-        return parse_openssh_version(self.module.run_command([ssh_bin, '-V', '-q'])[2].strip())
+        return parse_openssh_version(self.module.run_command([ssh_bin, '-V', '-q'], check_rc=True)[2].strip())
 
     @_restore_all_on_failure
     def _safe_secure_move(self, sources_and_destinations):
@@ -208,14 +208,18 @@ def get_matching_public_key(self, private_key_path, **kwargs):
     def get_private_key(self, private_key_path, **kwargs):
         return self._run_command([self._bin_path, '-l', '-f', private_key_path], **kwargs)
 
-    def update_comment(self, private_key_path, comment, **kwargs):
+    def update_comment(self, private_key_path, comment, force_new_format=True, **kwargs):
         if os.path.exists(private_key_path) and not os.access(private_key_path, os.W_OK):
             try:
                 os.chmod(private_key_path, stat.S_IWUSR + stat.S_IRUSR)
             except (IOError, OSError) as e:
                 raise e("The private key at %s is not writeable preventing a comment update" % private_key_path)
 
-        return self._run_command([self._bin_path, '-q', '-o', '-c', '-C', comment, '-f', private_key_path], **kwargs)
+        command = [self._bin_path, '-q']
+        if force_new_format:
+            command.append('-o')
+        command.extend(['-c', '-C', comment, '-f', private_key_path])
+        return self._run_command(command, **kwargs)
 
 
 class PrivateKey(object):

plugins/module_utils/openssh/backends/keypair_backend.py

@@ -323,23 +323,27 @@ def __init__(self, module):
         self.ssh_keygen = KeygenCommand(self.module)
 
     def _generate_keypair(self, private_key_path):
-        self.ssh_keygen.generate_keypair(private_key_path, self.size, self.type, self.comment)
+        self.ssh_keygen.generate_keypair(private_key_path, self.size, self.type, self.comment, check_rc=True)
 
     def _get_private_key(self):
-        private_key_content = self.ssh_keygen.get_private_key(self.private_key_path)[1]
+        rc, private_key_content, err = self.ssh_keygen.get_private_key(self.private_key_path, check_rc=False)
+        if rc != 0:
+            raise ValueError(err)
         return PrivateKey.from_string(private_key_content)
 
     def _get_public_key(self):
-        public_key_content = self.ssh_keygen.get_matching_public_key(self.private_key_path)[1]
+        public_key_content = self.ssh_keygen.get_matching_public_key(self.private_key_path, check_rc=True)[1]
         return PublicKey.from_string(public_key_content)
 
     def _private_key_readable(self):
-        rc, stdout, stderr = self.ssh_keygen.get_matching_public_key(self.private_key_path)
+        rc, stdout, stderr = self.ssh_keygen.get_matching_public_key(self.private_key_path, check_rc=False)
         return not (rc == 255 or any_in(stderr, 'is not a public key file', 'incorrect passphrase', 'load failed'))
 
     def _update_comment(self):
         try:
-            self.ssh_keygen.update_comment(self.private_key_path, self.comment)
+            ssh_version = self._get_ssh_version() or "7.8"
+            force_new_format = LooseVersion('6.5') <= LooseVersion(ssh_version) < LooseVersion('7.8')
+            self.ssh_keygen.update_comment(self.private_key_path, self.comment, force_new_format=force_new_format, check_rc=True)
         except (IOError, OSError) as e:
             self.module.fail_json(msg=to_native(e))
 

plugins/modules/openssh_cert.py

@@ -497,7 +497,10 @@ def _result(self):
         if self.state != 'present':
             return {}
 
-        certificate_info = self.ssh_keygen.get_certificate_info(self.path)[1]
+        certificate_info = self.ssh_keygen.get_certificate_info(
+            self.path,
+            check_rc=self.state == 'present' and not self.module.check_mode,
+        )[1]
 
         return {
             'type': self.type,

tests/integration/targets/openssh_keypair/tests/options.yml

@@ -100,8 +100,10 @@
     comment: "test_modified@comment"
     backend: "{{ backend }}"
   register: modified_comment_output
+  ignore_errors: true
 
 - name: "({{ backend }}) Assert comment preserved public key - comment"
+  when: modified_comment_output is succeeded
   assert:
     that:
       - comment_output.public_key == modified_comment_output.public_key
@@ -111,9 +113,17 @@
   assert:
     that:
       - modified_comment_output.comment == 'test_modified@comment'
+      - modified_comment_output is succeeded
   # Support for updating comments for key types other than rsa1 was added in OpenSSH 7.2
   when: not (backend == 'opensshbin' and openssh_version is version('7.2', '<'))
 
+- name: "({{ backend }}) Assert comment not changed - comment"
+  assert:
+    that:
+      - modified_comment_output is failed
+  # Support for updating comments for key types other than rsa1 was added in OpenSSH 7.2
+  when: backend == 'opensshbin' and openssh_version is version('7.2', '<')
+
 - name: "({{ backend }}) Remove key - comment"
   openssh_keypair:
     path: "{{ remote_tmp_dir }}/comment"

tests/integration/targets/openssh_keypair/tests/regenerate.yml

@@ -329,22 +329,25 @@
     that:
       - result is changed
 
-- name: "({{ backend }}) Regenerate - adjust comment"
-  openssh_keypair:
-    path: '{{ remote_tmp_dir }}/regenerate-a-{{ item }}'
-    type: dsa
-    size: 1024
-    comment: test comment
-    regenerate: '{{ item }}'
-    backend: "{{ backend }}"
-  loop: "{{ regenerate_values }}"
-  register: result
-- assert:
-    that:
-      - result is changed
-      # for all values but 'always', the key should not be regenerated.
-      # verify this by comparing fingerprints:
-      - result.results[0].fingerprint == result.results[1].fingerprint
-      - result.results[0].fingerprint == result.results[2].fingerprint
-      - result.results[0].fingerprint == result.results[3].fingerprint
-      - result.results[0].fingerprint != result.results[4].fingerprint
+# Support for updating comments for key types other than rsa1 was added in OpenSSH 7.2
+- when: not (backend == 'opensshbin' and openssh_version is version('7.2', '<'))
+  block:
+  - name: "({{ backend }}) Regenerate - adjust comment"
+    openssh_keypair:
+      path: '{{ remote_tmp_dir }}/regenerate-a-{{ item }}'
+      type: dsa
+      size: 1024
+      comment: test comment
+      regenerate: '{{ item }}'
+      backend: "{{ backend }}"
+    loop: "{{ regenerate_values }}"
+    register: result
+  - assert:
+      that:
+        - result is changed
+        # for all values but 'always', the key should not be regenerated.
+        # verify this by comparing fingerprints:
+        - result.results[0].fingerprint == result.results[1].fingerprint
+        - result.results[0].fingerprint == result.results[2].fingerprint
+        - result.results[0].fingerprint == result.results[3].fingerprint
+        - result.results[0].fingerprint != result.results[4].fingerprint