openssh_* modules: check return code on ssh(-keygen) invocations; fail 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.

plugins/module_utils/openssh/backends/common.py

@@ -127,7 +127,7 @@ class OpensshModule(object):
         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 @@ class KeygenCommand(object):
     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 @@ class KeypairBackendOpensshBin(KeypairBackend):
         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 @@ class Certificate(OpensshModule):
         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,