known_hosts: support --diff (#20349)

* known_hosts: support --diff

* known_hosts: support --diff also without --check

* Add unit tests and fix incorrect diff in one corner case

Tests are good!

* Refactor for readability

* Python 3 compat

* More Python 3 compat

* Add an integration test for known_hosts

* Handle ssh-keygen -HF returning non-zero exit code

AFAICT this is a bug in ssh-keygen in some newer OpenSSH versions
(>= 6.4 probably; see commit dd9d5cc670):
when you invoke ssh-keygen with -H and -F <host> options, it always
returns exit code 1.  This is because in ssh-keygen.c there's a function
do_known_hosts() which calls

  exit (find_host && !ctx.found_key);

at the end, and find_host is 1 (because we passed -F on the command line),
but ctx.found_key is always 0.  Why is found_key always 0?  Because the
callback passed to hostkeys_foreach(), which is known_hosts_hash(),
never bothers to set found_key to 1.

* This test does not need root

* Avoid ssh-ed25519 keys in sample known_hosts file

Older versions of OpenSSH do not like them and ssh-keygen -HF
aborts with an error when it sees such keys:

  line 5 invalid key: example.net...
  /root/ansible_testing/known_hosts is not a valid known_hosts file.

* Fix Python 3 errors

Specifically, the default mode of tempfile.NamedTemporaryFile is 'w+b',
which means Python 3 wants us to write bytes objects to it -- but the
keys we have are all unicode strings.

lib/ansible/modules/system/known_hosts.py

@@ -119,12 +119,15 @@ def enforce_state(module, params):
 
     found,replace_or_add,found_line,key=search_for_host_key(module,host,key,hash_host,path,sshkeygen)
 
+    params['diff'] = compute_diff(path, found_line, replace_or_add, state, key)
+
     #We will change state if found==True & state!="present"
     #or found==False & state=="present"
     #i.e found XOR (state=="present")
     #Alternatively, if replace is true (i.e. key present, and we must change it)
     if module.check_mode:
-        module.exit_json(changed = replace_or_add or (state=="present") != found)
+        module.exit_json(changed = replace_or_add or (state=="present") != found,
+                         diff=params['diff'])
 
     #Now do the work.
 
@@ -145,7 +148,7 @@ def enforce_state(module, params):
                 module.fail_json(msg="Failed to read %s: %s" % \
                                      (path,str(e)))
         try:
-            outf=tempfile.NamedTemporaryFile(dir=os.path.dirname(path))
+            outf = tempfile.NamedTemporaryFile(mode='w+', dir=os.path.dirname(path))
             if inf is not None:
                 for line_number, line in enumerate(inf):
                     if found_line==(line_number + 1) and (replace_or_add or state=='absent'):
@@ -188,7 +191,7 @@ def sanity_check(module,host,key,sshkeygen):
     #The approach is to write the key to a temporary file,
     #and then attempt to look up the specified host in that file.
     try:
-        outf=tempfile.NamedTemporaryFile()
+        outf = tempfile.NamedTemporaryFile(mode='w+')
         outf.write(key)
         outf.flush()
     except IOError:
@@ -240,7 +243,7 @@ def search_for_host_key(module,host,key,hash_host,path,sshkeygen):
 
     sshkeygen_command.insert(1,'-H')
     rc,stdout,stderr=module.run_command(sshkeygen_command,check_rc=False)
-    if rc!=0: #something went wrong
+    if rc not in (0, 1) or stderr != '': #something went wrong
         module.fail_json(msg="ssh-keygen failed to hash host (rc=%d,stdout='%s',stderr='%s')" % (rc,stdout,stderr))
     hashed_lines=stdout.split('\n')
 
@@ -294,6 +297,30 @@ def normalize_known_hosts_key(key):
         d['key']=k[2]
     return d
 
+def compute_diff(path, found_line, replace_or_add, state, key):
+    diff = {
+        'before_header': path,
+        'after_header': path,
+        'before': '',
+        'after': '',
+    }
+    try:
+        inf = open(path, "r")
+    except IOError:
+        e = get_exception()
+        if e.errno == errno.ENOENT:
+            diff['before_header'] = '/dev/null'
+    else:
+        diff['before'] = inf.read()
+        inf.close()
+    lines = diff['before'].splitlines(1)
+    if (replace_or_add or state == 'absent') and found_line is not None and 1 <= found_line <= len(lines):
+        del lines[found_line - 1]
+    if state == 'present' and (replace_or_add or found_line is None):
+        lines.append(key)
+    diff['after'] = ''.join(lines)
+    return diff
+
 def main():
 
     module = AnsibleModule(

test/integration/non_destructive.yml

@@ -35,3 +35,4 @@
     - { role: mount, tags: [test_mount, needs_root, needs_privileged]}
     - { role: include_vars, tags: test_include_vars }
     - { role: sefcontext, tags: [test_sefcontext, needs_root]}
+    - { role: known_hosts, tags: test_known_hosts }

test/integration/targets/known_hosts/aliases

@@ -0,0 +1 @@
+posix/ci/group2

test/integration/targets/known_hosts/defaults/main.yml

@@ -0,0 +1,3 @@
+---
+example_org_rsa_key: >
+  example.org ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAAAglyZmHHWskQ9wkh8LYbIqzvg99/oloneH7BaZ02ripJUy/2Zynv4tgUfm9fdXvAb1XXCEuTRnts9FBer87+voU0FPRgx3CfY9Sgr0FspUjnm4lqs53FIab1psddAaS7/F7lrnjl6VqBtPwMRQZG7qlml5uogGJwYJHxX0PGtsdoTJsM=

test/integration/targets/known_hosts/files/existing_known_hosts

@@ -0,0 +1,5 @@
+example.com ssh-dss AAAAB3NzaC1kc3MAAACBALT8YHxZ59d8yX4oQNPbpdK9AMPRQGKFY9X13S2fp4UMPijiB3ETxU1bAyVTjTbsoag065naFt13aIVl+u0MDPfMuYgVJFEorAZkDlBixvT25zpKyQhI4CtHhZ9Y9YWug4xLqSaFUYEPO31Bie7k8xRfDwsHtzTRPp/0zRURwARHAAAAFQDLx2DZMm3cR8cZtbq4zdSvkXLh0wAAAIAalkQYziu2b5dDRQMiFpDLpPdbymyVhDMmRKnXwAB1+dhGyJLGvfe0xO+ibqGXMp1aZ1iC3a/vHTpYKDVqKIIpFg5r0fxAcAZkJR0aRC8RDxW/IclbIliETD71osIT8I47OFc7vAVCWP8JbV3ZYzR+i98WUkmZ4/ZUzsDl2gi7WAAAAIAsdTGwAo4Fs784TdP2tIHCqxAIz2k4tWmZyeRmXkH5K/P1o9XSh3RNxvFKK7BY6dQK+h9jLunMBs0SCzhMoTcXaJq331kmLJltjq5peo0PnLGnQz5pas0PD7p7gb+soklmHoVp7J2oMC/U4N1Rxr6g9sv8Rpsf1PTPDT3sEbze6A== root@freezer
+|1|d71/U7CbOH3Su+d2zxlbmiNfXtI=|g2YSPAVoK7bmg16FCOOPKTZe2BM= ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAq2A7hRGmdnm9tUDbO9IDSwBK6TbQa+PXYPCPy6rbTrTtw7PHkccKrpp0yVhp5HdEIcKr6pLlVDBfOLX9QUsyCOV0wzfjIJNlGEYsdlLJizHhbn2mUjvSAHQqZETYP81eFzLQNnPHt4EVVUh7VfDESU84KezmD5QlWpXLmvU31/yMf+Se8xhHTvKSCZIFImWwoG6mbUoWf9nzpIoaSjB+weqqUUmpaaasXVal72J+UX2B+2RPW3RcT0eOzQgqlJL3RKrTJvdsjE3JEAvGq3lGHSZXy28G3skua2SmVi/w4yCE6gbODqnTWlg7+wC604ydGXA8VJiS5ap43JXiUFFAaQ==
+|1|L0TqxOhAVh6mLZ2lbHdTv3owun0=|vn0La5pbHNxin3XzQQdvaOulvVU= ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBCNLCAA/SjVF3jkmlAlkgh+GtZdgxtusHaK66fcA7XSgCpQOdri1dGmND6pQDGwsxiKMy4Ou1GB2DR4N0G9T5E8=
+|1|WPo7yAOdlQKLSuRatNJCmDoga0k=|D/QybGglKokWuEQUe9Okpy5uSh0= ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBCNLCAA/SjVF3jkmlAlkgh+GtZdgxtusHaK66fcA7XSgCpQOdri1dGmND6pQDGwsxiKMy4Ou1GB2DR4N0G9T5E8=
+# example.net ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIM6OSqweGdPdQ/metQaf738AdN3P+itYp1AypOTgXkyj root@localhost

test/integration/targets/known_hosts/meta/main.yml

@@ -0,0 +1,2 @@
+dependencies:
+  - prepare_tests

test/integration/targets/known_hosts/tasks/main.yml

@@ -0,0 +1,169 @@
+# test code for the known_hosts module
+# (c) 2017, Marius Gedminas <marius@gedmin.as>
+
+# This file is part of Ansible
+#
+# Ansible is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Ansible is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Ansible.  If not, see <http://www.gnu.org/licenses/>.
+
+- name: copy an existing file in place
+  copy: src=existing_known_hosts dest="{{output_dir|expanduser}}/known_hosts"
+
+# test addition
+
+- name: add a new host in check mode
+  check_mode: yes
+  known_hosts:
+    name: example.org
+    key: "{{ example_org_rsa_key }}"
+    state: present
+    path: "{{output_dir|expanduser}}/known_hosts"
+  register: diff
+
+- name: assert that the diff looks as expected (the key was added at the end)
+  assert:
+    that:
+    - 'diff.changed'
+    - 'diff.diff.before_header == diff.diff.after_header == output_dir|expanduser + "/known_hosts"'
+    - 'diff.diff.after.splitlines()[:-1] == diff.diff.before.splitlines()'
+    - 'diff.diff.after.splitlines()[-1] == example_org_rsa_key.strip()'
+
+- name: add a new host
+  known_hosts:
+    name: example.org
+    key: "{{ example_org_rsa_key }}"
+    state: present
+    path: "{{output_dir|expanduser}}/known_hosts"
+  register: result
+
+- name: get the file content
+  shell: cat "{{output_dir|expanduser}}/known_hosts"
+  register: known_hosts
+
+- name: assert that the key was added and ordering preserved
+  assert:
+    that:
+    - 'result.changed'
+    - 'known_hosts.stdout_lines[0].startswith("example.com")'
+    - 'known_hosts.stdout_lines[4].startswith("# example.net")'
+    - 'known_hosts.stdout_lines[-1].strip() == example_org_rsa_key.strip()'
+
+# test idempotence of addition
+
+- name: add the same host in check mode
+  check_mode: yes
+  known_hosts:
+    name: example.org
+    key: "{{ example_org_rsa_key }}"
+    state: present
+    path: "{{output_dir|expanduser}}/known_hosts"
+  register: check
+
+- name: assert that no changes were expected
+  assert:
+    that:
+    - 'not check.changed'
+    - 'check.diff.before == check.diff.after'
+
+- name: add the same host
+  known_hosts:
+    name: example.org
+    key: "{{ example_org_rsa_key }}"
+    state: present
+    path: "{{output_dir|expanduser}}/known_hosts"
+  register: result
+
+- name: get the file content
+  shell: cat "{{output_dir|expanduser}}/known_hosts"
+  register: known_hosts_v2
+
+- name: assert that no changes happened
+  assert:
+    that:
+    - 'not result.changed'
+    - 'result.diff.before == result.diff.after'
+    - 'known_hosts.stdout == known_hosts_v2.stdout'
+
+# test removal
+
+- name: remove the host in check mode
+  check_mode: yes
+  known_hosts:
+    name: example.org
+    key: "{{ example_org_rsa_key }}"
+    state: absent
+    path: "{{output_dir|expanduser}}/known_hosts"
+  register: diff
+
+- name: assert that the diff looks as expected (the key was removed)
+  assert:
+    that:
+    - 'diff.diff.before_header == diff.diff.after_header == output_dir|expanduser + "/known_hosts"'
+    - 'diff.diff.before.splitlines()[-1] == example_org_rsa_key.strip()'
+    - 'diff.diff.after.splitlines() == diff.diff.before.splitlines()[:-1]'
+
+- name: remove the host
+  known_hosts:
+    name: example.org
+    key: "{{ example_org_rsa_key }}"
+    state: absent
+    path: "{{output_dir|expanduser}}/known_hosts"
+  register: result
+
+- name: get the file content
+  shell: cat "{{output_dir|expanduser}}/known_hosts"
+  register: known_hosts_v3
+
+- name: assert that the key was removed and ordering preserved
+  assert:
+    that:
+    - 'result.changed'
+    - '"example.org" not in known_hosts_v3.stdout'
+    - 'known_hosts_v3.stdout_lines[0].startswith("example.com")'
+    - 'known_hosts_v3.stdout_lines[-1].startswith("# example.net")'
+
+# test idempotence of removal
+
+- name: remove the same host in check mode
+  check_mode: yes
+  known_hosts:
+    name: example.org
+    key: "{{ example_org_rsa_key }}"
+    state: absent
+    path: "{{output_dir|expanduser}}/known_hosts"
+  register: check
+
+- name: assert that no changes were expected
+  assert:
+    that:
+    - 'not check.changed'
+    - 'check.diff.before == check.diff.after'
+
+- name: remove the same host
+  known_hosts:
+    name: example.org
+    key: "{{ example_org_rsa_key }}"
+    state: absent
+    path: "{{output_dir|expanduser}}/known_hosts"
+  register: result
+
+- name: get the file content
+  shell: cat "{{output_dir|expanduser}}/known_hosts"
+  register: known_hosts_v4
+
+- name: assert that no changes happened
+  assert:
+    that:
+    - 'not result.changed'
+    - 'result.diff.before == result.diff.after'
+    - 'known_hosts_v3.stdout == known_hosts_v4.stdout'

test/units/modules/system/test_known_hosts.py

@@ -0,0 +1,112 @@
+import os
+import tempfile
+
+from ansible.compat.tests import unittest
+from ansible.module_utils._text import to_bytes
+
+from ansible.modules.system.known_hosts import compute_diff
+
+
+class KnownHostsDiffTestCase(unittest.TestCase):
+
+    def _create_file(self, content):
+        tmp_file = tempfile.NamedTemporaryFile(prefix='ansible-test-', suffix='-known_hosts', delete=False)
+        tmp_file.write(to_bytes(content))
+        tmp_file.close()
+        self.addCleanup(os.unlink, tmp_file.name)
+        return tmp_file.name
+
+    def test_no_existing_file(self):
+        path = tempfile.mktemp(prefix='ansible-test-', suffix='-known_hosts')
+        key = 'example.com ssh-rsa AAAAetc\n'
+        diff = compute_diff(path, found_line=None, replace_or_add=False, state='present', key=key)
+        self.assertEqual(diff, {
+            'before_header': '/dev/null',
+            'after_header': path,
+            'before': '',
+            'after': 'example.com ssh-rsa AAAAetc\n',
+        })
+
+    def test_key_addition(self):
+        path = self._create_file(
+            'two.example.com ssh-rsa BBBBetc\n'
+        )
+        key = 'one.example.com ssh-rsa AAAAetc\n'
+        diff = compute_diff(path, found_line=None, replace_or_add=False, state='present', key=key)
+        self.assertEqual(diff, {
+            'before_header': path,
+            'after_header': path,
+            'before':
+                'two.example.com ssh-rsa BBBBetc\n',
+            'after':
+                'two.example.com ssh-rsa BBBBetc\n'
+                'one.example.com ssh-rsa AAAAetc\n',
+        })
+
+    def test_no_change(self):
+        path = self._create_file(
+            'one.example.com ssh-rsa AAAAetc\n'
+            'two.example.com ssh-rsa BBBBetc\n'
+        )
+        key = 'one.example.com ssh-rsa AAAAetc\n'
+        diff = compute_diff(path, found_line=1, replace_or_add=False, state='present', key=key)
+        self.assertEqual(diff, {
+            'before_header': path,
+            'after_header': path,
+            'before':
+                'one.example.com ssh-rsa AAAAetc\n'
+                'two.example.com ssh-rsa BBBBetc\n',
+            'after':
+                'one.example.com ssh-rsa AAAAetc\n'
+                'two.example.com ssh-rsa BBBBetc\n',
+        })
+
+    def test_key_change(self):
+        path = self._create_file(
+            'one.example.com ssh-rsa AAAaetc\n'
+            'two.example.com ssh-rsa BBBBetc\n'
+        )
+        key = 'one.example.com ssh-rsa AAAAetc\n'
+        diff = compute_diff(path, found_line=1, replace_or_add=True, state='present', key=key)
+        self.assertEqual(diff, {
+            'before_header': path,
+            'after_header': path,
+            'before':
+                'one.example.com ssh-rsa AAAaetc\n'
+                'two.example.com ssh-rsa BBBBetc\n',
+            'after':
+                'two.example.com ssh-rsa BBBBetc\n'
+                'one.example.com ssh-rsa AAAAetc\n',
+        })
+
+    def test_key_removal(self):
+        path = self._create_file(
+            'one.example.com ssh-rsa AAAAetc\n'
+            'two.example.com ssh-rsa BBBBetc\n'
+        )
+        key = 'one.example.com ssh-rsa AAAAetc\n'
+        diff = compute_diff(path, found_line=1, replace_or_add=False, state='absent', key=key)
+        self.assertEqual(diff, {
+            'before_header': path,
+            'after_header': path,
+            'before':
+                'one.example.com ssh-rsa AAAAetc\n'
+                'two.example.com ssh-rsa BBBBetc\n',
+            'after':
+                'two.example.com ssh-rsa BBBBetc\n',
+        })
+
+    def test_key_removal_no_change(self):
+        path = self._create_file(
+            'two.example.com ssh-rsa BBBBetc\n'
+        )
+        key = 'one.example.com ssh-rsa AAAAetc\n'
+        diff = compute_diff(path, found_line=None, replace_or_add=False, state='absent', key=key)
+        self.assertEqual(diff, {
+            'before_header': path,
+            'after_header': path,
+            'before':
+                'two.example.com ssh-rsa BBBBetc\n',
+            'after':
+                'two.example.com ssh-rsa BBBBetc\n',
+        })