Merge ~smoser/cloud-init:fix/1677205-eol-on-sshd_config into cloud-init:master

[Scott Moser]

set_passwords: Add newline to end of sshd config, only restart if updated.

This admittedly does a fairly extensive re-factor to simply add a newline
to the end of sshd_config.

It makes the ssh_config updating portion of set_passwords more testable
and adds tests for that.

The new function is in 'update_ssh_config_lines' which allows you
to update a config with multiple changes even though only a single one
is currently used.

We also only restart the ssh daemon now if a change was made to the
config file. Before it was always restarted if the user specified
a value for ssh_pwauth other than 'unchanged'.

Thanks to Lorens Kockum for initial diagnosis and patch.

LP: #1677205

see commit message

[Scott Moser (smoser)]

Increases test coverage:
 * ssh_util from 60 -> 70%
 * cc_set_passwords: 6% -> 27%

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:3c15d5bfaa7929c129277fbb36d28af958b88a5a
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1006/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1006/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:6229047dea8b7d690fc705c9c43073c53fe72757
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1007/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1007/rebuild

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:f388b171bcbeb5757ce0f519d9be1edf87b341a1
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1010/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1010/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

This is fine. One comment w.r.t the boolean construction. Take that or ignore.

review: Approve

[Scott Moser (smoser)]

Your use of timeit is a bit flawed. You really need to use 'setup'
in almost all cases. otherwise you're testing both the construction
of the list and the evaluation of your condition in each loop. Obviously
if the construction of the list takes 10x the test then your results
will not be meaningful.

So I tried..

>>> timeit.timeit('True if a else False', setup='a=list(range(0,100))', number=number)
0.048113107681274414
>>> timeit.timeit('len(a)', setup='a=list(range(0,100))', number=number)
0.054379940032958984
>>> timeit.timeit('len(a) == 0', setup='a=list(range(0,100))', number=number)
0.06807184219360352
>>> timeit.timeit('bool(a)', setup='a=list(range(0,100))', number=number)
0.11591291427612305

That is mind-boggling.
How can 'True if a else False' *not* call bool(a) in order to evaluate 'if a'?

Even crazier...
Here we define 'mybool' a regular old function and take the function overhead but still do the 'True if a else False'.
*That* is faster than bool(a) when 'a' is an empty list bug does get slower when 'a' is a larger list.

>>> timeit.timeit('mybool(a)', setup='a=[]\ndef mybool(b):\n return True if b else False', number=number)
0.09648299217224121
>>> timeit.timeit('bool(a)', setup='a=[]\ndef mybool(b):\n return True if b else False', number=number)
0.12272787094116211
>>> timeit.timeit('mybool(a)', setup='a=list(range(0,300))\ndef mybool(b):\n return True if b else False', number=number)
0.09898996353149414
>>> timeit.timeit('bool(a)', setup='a=list(range(0,300))\ndef mybool(b):\n return True if b else False', number=number)
0.11946702003479004

[Scott Moser (smoser)]

I think the most obvious implementation of "return True if the list has entries and False otherwise" is in order of most to least obvious:

return len(changed) == 0
return bool(changed)
return True if changed else False

The fact that bool(changed) is "so slow" is unfortunate.

I think I'll go with len(changed) == 0

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:5ff0a5d4479d59b69d1e5cb104522671f5488243
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1031/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1031/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:770101c0b5a9e7455fbd97a04c7164b1b213eb5d
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1032/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1032/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

> Your use of timeit is a bit flawed. You really need to use 'setup'
> in almost all cases. otherwise you're testing both the construction
> of the list and the evaluation of your condition in each loop. Obviously
> if the construction of the list takes 10x the test then your results
> will not be meaningful.

I'm well aware of construction costs, however, given the size of list it won't have
a significant effect on the outcome.

>
> So I tried..
>
> >>> timeit.timeit('True if a else False', setup='a=list(range(0,100))',
> number=number)
> 0.048113107681274414
> >>> timeit.timeit('len(a)', setup='a=list(range(0,100))', number=number)
> 0.054379940032958984
> >>> timeit.timeit('len(a) == 0', setup='a=list(range(0,100))', number=number)
> 0.06807184219360352
> >>> timeit.timeit('bool(a)', setup='a=list(range(0,100))', number=number)
> 0.11591291427612305

That matches what I found.

>
> That is mind-boggling.
> How can 'True if a else False' *not* call bool(a) in order to evaluate 'if a'?

https://docs.python.org/3/library/stdtypes.html#truth

>
> Even crazier...
> Here we define 'mybool' a regular old function and take the function overhead
> but still do the 'True if a else False'.
> *That* is faster than bool(a) when 'a' is an empty list bug does get slower
> when 'a' is a larger list.
>
> >>> timeit.timeit('mybool(a)', setup='a=[]\ndef mybool(b):\n return True if b
> else False', number=number)
> 0.09648299217224121
> >>> timeit.timeit('bool(a)', setup='a=[]\ndef mybool(b):\n return True if b
> else False', number=number)
> 0.12272787094116211
> >>> timeit.timeit('mybool(a)', setup='a=list(range(0,300))\ndef mybool(b):\n
> return True if b else False', number=number)
> 0.09898996353149414
> >>> timeit.timeit('bool(a)', setup='a=list(range(0,300))\ndef mybool(b):\n
> return True if b else False', number=number)
> 0.11946702003479004

Indeed; I suspect the cost of allocating memory for bool() where that's not used
in any other case. A boolean is a subclass of integers and requires 24 bytes of memory.

[Chad Smith (chad.smith)]

An upstream commit landed for this bug.

To view that commit see the following URL:
https://git.launchpad.net/cloud-init/commit/?id=4952a854

diff --git a/cloudinit/config/cc_set_passwords.py b/cloudinit/config/cc_set_passwords.py
index bb24d57..5ef9737 100755
--- a/cloudinit/config/cc_set_passwords.py
+++ b/cloudinit/config/cc_set_passwords.py
@@ -68,16 +68,57 @@ import re
 import sys
 
 from cloudinit.distros import ug_util
-from cloudinit import ssh_util
+from cloudinit import log as logging
+from cloudinit.ssh_util import update_ssh_config
 from cloudinit import util
 
 from string import ascii_letters, digits
 
+LOG = logging.getLogger(__name__)
+
 # We are removing certain 'painful' letters/numbers
 PW_SET = (''.join([x for x in ascii_letters + digits
                    if x not in 'loLOI01']))
 
 
+def handle_ssh_pwauth(pw_auth, service_cmd=None, service_name="ssh"):
+    """Apply sshd PasswordAuthentication changes.
+
+    @param pw_auth: config setting from 'pw_auth'.
+                    Best given as True, False, or "unchanged".
+    @param service_cmd: The service command list (['service'])
+    @param service_name: The name of the sshd service for the system.
+
+    @return: None"""
+    cfg_name = "PasswordAuthentication"
+    if service_cmd is None:
+        service_cmd = ["service"]
+
+    if util.is_true(pw_auth):
+        cfg_val = 'yes'
+    elif util.is_false(pw_auth):
+        cfg_val = 'no'
+    else:
+        bmsg = "Leaving ssh config '%s' unchanged." % cfg_name
+        if pw_auth is None or pw_auth.lower() == 'unchanged':
+            LOG.debug("%s ssh_pwauth=%s", bmsg, pw_auth)
+        else:
+            LOG.warning("%s Unrecognized value: ssh_pwauth=%s", bmsg, pw_auth)
+        return
+
+    updated = update_ssh_config({cfg_name: cfg_val})
+    if not updated:
+        LOG.debug("No need to restart ssh service, %s not updated.", cfg_name)
+        return
+
+    if 'systemctl' in service_cmd:
+        cmd = list(service_cmd) + ["restart", service_name]
+    else:
+        cmd = list(service_cmd) + [service_name, "restart"]
+    util.subp(cmd)
+    LOG.debug("Restarted the ssh daemon.")
+
+
 def handle(_name, cfg, cloud, log, args):
     if len(args) != 0:
         # if run from command line, and give args, wipe the chpasswd['list']
@@ -170,65 +211,9 @@ def handle(_name, cfg, cloud, log, args):
             if expired_users:
                 log.debug("Expired passwords for: %s users", expired_users)
 
-    change_pwauth = False
-    pw_auth = None
-    if 'ssh_pwauth' in cfg:
-        if util.is_true(cfg['ssh_pwauth']):
-            change_pwauth = True
-            pw_auth = 'yes'
-        elif util.is_false(cfg['ssh_pwauth']):
-            change_pwauth = True
-            pw_auth = 'no'
-        elif str(cfg['ssh_pwauth']).lower() == 'unchanged':
-            log.debug('Leaving auth line unchanged')
-            change_pwauth = False
-        elif not str(cfg['ssh_pwauth']).strip():
-            log.debug('Leaving auth line unchanged')
-            change_pwauth = False
-        elif not cfg['ssh_pwauth']:
-            log.debug('Leaving auth line unchanged')
-            change_pwauth = False
-        else:
-            msg = 'Unrecognized value %s for ssh_pwauth' % cfg['ssh_pwauth']
-            util.logexc(log, msg)
-
-    if change_pwauth:
-        replaced_auth = False
-
-        # See: man sshd_config
-        old_lines = ssh_util.parse_ssh_config(ssh_util.DEF_SSHD_CFG)
-        new_lines = []
-        i = 0
-        for (i, line) in enumerate(old_lines):
-            # Keywords are case-insensitive and arguments are case-sensitive
-            if line.key == 'passwordauthentication':
-                log.debug("Replacing auth line %s with %s", i + 1, pw_auth)
-                replaced_auth = True
-                line.value = pw_auth
-            new_lines.append(line)
-
-        if not replaced_auth:
-            log.debug("Adding new auth line %s", i + 1)
-            replaced_auth = True
-            new_lines.append(ssh_util.SshdConfigLine('',
-                                                     'PasswordAuthentication',
-                                                     pw_auth))
-
-        lines = [str(l) for l in new_lines]
-        util.write_file(ssh_util.DEF_SSHD_CFG, "\n".join(lines),
-                        copy_mode=True)
-
-        try:
-            cmd = cloud.distro.init_cmd  # Default service
-            cmd.append(cloud.distro.get_option('ssh_svcname', 'ssh'))
-            cmd.append('restart')
-            if 'systemctl' in cmd:  # Switch action ordering
-                cmd[1], cmd[2] = cmd[2], cmd[1]
-            cmd = filter(None, cmd)  # Remove empty arguments
-            util.subp(cmd)
-            log.debug("Restarted the ssh daemon")
-        except Exception:
-            util.logexc(log, "Restarting of the ssh daemon failed")
+    handle_ssh_pwauth(
+        cfg.get('ssh_pwauth'), service_cmd=cloud.distro.init_cmd,
+        service_name=cloud.distro.get_option('ssh_svcname', 'ssh'))
 
     if len(errors):
         log.debug("%s errors occured, re-raising the last one", len(errors))
diff --git a/cloudinit/config/tests/test_set_passwords.py b/cloudinit/config/tests/test_set_passwords.py
new file mode 100644
index 0000000..b051ec8
--- /dev/null
+++ b/cloudinit/config/tests/test_set_passwords.py
@@ -0,0 +1,71 @@
+# This file is part of cloud-init. See LICENSE file for license information.
+
+import mock
+
+from cloudinit.config import cc_set_passwords as setpass
+from cloudinit.tests.helpers import CiTestCase
+from cloudinit import util
+
+MODPATH = "cloudinit.config.cc_set_passwords."
+
+
+class TestHandleSshPwauth(CiTestCase):
+    """Test cc_set_passwords handling of ssh_pwauth in handle_ssh_pwauth."""
+
+    with_logs = True
+
+    @mock.patch(MODPATH + "util.subp")
+    def test_unknown_value_logs_warning(self, m_subp):
+        setpass.handle_ssh_pwauth("floo")
+        self.assertIn("Unrecognized value: ssh_pwauth=floo",
+                      self.logs.getvalue())
+        m_subp.assert_not_called()
+
+    @mock.patch(MODPATH + "update_ssh_config", return_value=True)
+    @mock.patch(MODPATH + "util.subp")
+    def test_systemctl_as_service_cmd(self, m_subp, m_update_ssh_config):
+        """If systemctl in service cmd: systemctl restart name."""
+        setpass.handle_ssh_pwauth(
+            True, service_cmd=["systemctl"], service_name="myssh")
+        self.assertEqual(mock.call(["systemctl", "restart", "myssh"]),
+                         m_subp.call_args)
+
+    @mock.patch(MODPATH + "update_ssh_config", return_value=True)
+    @mock.patch(MODPATH + "util.subp")
+    def test_service_as_service_cmd(self, m_subp, m_update_ssh_config):
+        """If systemctl in service cmd: systemctl restart name."""
+        setpass.handle_ssh_pwauth(
+            True, service_cmd=["service"], service_name="myssh")
+        self.assertEqual(mock.call(["service", "myssh", "restart"]),
+                         m_subp.call_args)
+
+    @mock.patch(MODPATH + "update_ssh_config", return_value=False)
+    @mock.patch(MODPATH + "util.subp")
+    def test_not_restarted_if_not_updated(self, m_subp, m_update_ssh_config):
+        """If config is not updated, then no system restart should be done."""
+        setpass.handle_ssh_pwauth(True)
+        m_subp.assert_not_called()
+        self.assertIn("No need to restart ssh", self.logs.getvalue())
+
+    @mock.patch(MODPATH + "update_ssh_config", return_value=True)
+    @mock.patch(MODPATH + "util.subp")
+    def test_unchanged_does_nothing(self, m_subp, m_update_ssh_config):
+        """If 'unchanged', then no updates to config and no restart."""
+        setpass.handle_ssh_pwauth(
+            "unchanged", service_cmd=["systemctl"], service_name="myssh")
+        m_update_ssh_config.assert_not_called()
+        m_subp.assert_not_called()
+
+    @mock.patch(MODPATH + "util.subp")
+    def test_valid_change_values(self, m_subp):
+        """If value is a valid changen value, then update should be called."""
+        upname = MODPATH + "update_ssh_config"
+        optname = "PasswordAuthentication"
+        for value in util.FALSE_STRINGS + util.TRUE_STRINGS:
+            optval = "yes" if value in util.TRUE_STRINGS else "no"
+            with mock.patch(upname, return_value=False) as m_update:
+                setpass.handle_ssh_pwauth(value)
+                m_update.assert_called_with({optname: optval})
+        m_subp.assert_not_called()
+
+# vi: ts=4 expandtab
diff --git a/cloudinit/ssh_util.py b/cloudinit/ssh_util.py
index 882517f..73c3177 100644
--- a/cloudinit/ssh_util.py
+++ b/cloudinit/ssh_util.py
@@ -279,24 +279,28 @@ class SshdConfigLine(object):
 
 
 def parse_ssh_config(fname):
+    if not os.path.isfile(fname):
+        return []
+    return parse_ssh_config_lines(util.load_file(fname).splitlines())
+
+
+def parse_ssh_config_lines(lines):
     # See: man sshd_config
     # The file contains keyword-argument pairs, one per line.
     # Lines starting with '#' and empty lines are interpreted as comments.
     # Note: key-words are case-insensitive and arguments are case-sensitive
-    lines = []
-    if not os.path.isfile(fname):
-        return lines
-    for line in util.load_file(fname).splitlines():
+    ret = []
+    for line in lines:
         line = line.strip()
         if not line or line.startswith("#"):
-            lines.append(SshdConfigLine(line))
+            ret.append(SshdConfigLine(line))
             continue
         try:
             key, val = line.split(None, 1)
         except ValueError:
             key, val = line.split('=', 1)
-        lines.append(SshdConfigLine(line, key, val))
-    return lines
+        ret.append(SshdConfigLine(line, key, val))
+    return ret
 
 
 def parse_ssh_config_map(fname):
@@ -310,4 +314,56 @@ def parse_ssh_config_map(fname):
         ret[line.key] = line.value
     return ret
 
+
+def update_ssh_config(updates, fname=DEF_SSHD_CFG):
+    """Read fname, and update if changes are necessary.
+
+    @param updates: dictionary of desired values {Option: value}
+    @return: boolean indicating if an update was done."""
+    lines = parse_ssh_config(fname)
+    changed = update_ssh_config_lines(lines=lines, updates=updates)
+    if changed:
+        util.write_file(
+            fname, "\n".join([str(l) for l in lines]) + "\n", copy_mode=True)
+    return len(changed) != 0
+
+
+def update_ssh_config_lines(lines, updates):
+    """Update the ssh config lines per updates.
+
+    @param lines: array of SshdConfigLine.  This array is updated in place.
+    @param updates: dictionary of desired values {Option: value}
+    @return: A list of keys in updates that were changed."""
+    found = set()
+    changed = []
+
+    # Keywords are case-insensitive and arguments are case-sensitive
+    casemap = dict([(k.lower(), k) for k in updates.keys()])
+
+    for (i, line) in enumerate(lines, start=1):
+        if not line.key:
+            continue
+        if line.key in casemap:
+            key = casemap[line.key]
+            value = updates[key]
+            found.add(key)
+            if line.value == value:
+                LOG.debug("line %d: option %s already set to %s",
+                          i, key, value)
+            else:
+                changed.append(key)
+                LOG.debug("line %d: option %s updated %s -> %s", i,
+                          key, line.value, value)
+                line.value = value
+
+    if len(found) != len(updates):
+        for key, value in updates.items():
+            if key in found:
+                continue
+            changed.append(key)
+            lines.append(SshdConfigLine('', key, value))
+            LOG.debug("line %d: option %s added with %s",
+                      len(lines), key, value)
+    return changed
+
 # vi: ts=4 expandtab
diff --git a/tests/unittests/test_sshutil.py b/tests/unittests/test_sshutil.py
index 4c62c8b..73ae897 100644
--- a/tests/unittests/test_sshutil.py
+++ b/tests/unittests/test_sshutil.py
@@ -4,6 +4,7 @@ from mock import patch
 
 from cloudinit import ssh_util
 from cloudinit.tests import helpers as test_helpers
+from cloudinit import util
 
 
 VALID_CONTENT = {
@@ -56,7 +57,7 @@ TEST_OPTIONS = (
     'user \"root\".\';echo;sleep 10"')
 
 
-class TestAuthKeyLineParser(test_helpers.TestCase):
+class TestAuthKeyLineParser(test_helpers.CiTestCase):
 
     def test_simple_parse(self):
         # test key line with common 3 fields (keytype, base64, comment)
@@ -126,7 +127,7 @@ class TestAuthKeyLineParser(test_helpers.TestCase):
         self.assertFalse(key.valid())
 
 
-class TestUpdateAuthorizedKeys(test_helpers.TestCase):
+class TestUpdateAuthorizedKeys(test_helpers.CiTestCase):
 
     def test_new_keys_replace(self):
         """new entries with the same base64 should replace old."""
@@ -168,7 +169,7 @@ class TestUpdateAuthorizedKeys(test_helpers.TestCase):
         self.assertEqual(expected, found)
 
 
-class TestParseSSHConfig(test_helpers.TestCase):
+class TestParseSSHConfig(test_helpers.CiTestCase):
 
     def setUp(self):
         self.load_file_patch = patch('cloudinit.ssh_util.util.load_file')
@@ -235,4 +236,94 @@ class TestParseSSHConfig(test_helpers.TestCase):
         self.assertEqual('foo', ret[0].key)
         self.assertEqual('bar', ret[0].value)
 
+
+class TestUpdateSshConfigLines(test_helpers.CiTestCase):
+    """Test the update_ssh_config_lines method."""
+    exlines = [
+        "#PasswordAuthentication yes",
+        "UsePAM yes",
+        "# Comment line",
+        "AcceptEnv LANG LC_*",
+        "X11Forwarding no",
+    ]
+    pwauth = "PasswordAuthentication"
+
+    def check_line(self, line, opt, val):
+        self.assertEqual(line.key, opt.lower())
+        self.assertEqual(line.value, val)
+        self.assertIn(opt, str(line))
+        self.assertIn(val, str(line))
+
+    def test_new_option_added(self):
+        """A single update of non-existing option."""
+        lines = ssh_util.parse_ssh_config_lines(list(self.exlines))
+        result = ssh_util.update_ssh_config_lines(lines, {'MyKey': 'MyVal'})
+        self.assertEqual(['MyKey'], result)
+        self.check_line(lines[-1], "MyKey", "MyVal")
+
+    def test_commented_out_not_updated_but_appended(self):
+        """Implementation does not un-comment and update lines."""
+        lines = ssh_util.parse_ssh_config_lines(list(self.exlines))
+        result = ssh_util.update_ssh_config_lines(lines, {self.pwauth: "no"})
+        self.assertEqual([self.pwauth], result)
+        self.check_line(lines[-1], self.pwauth, "no")
+
+    def test_single_option_updated(self):
+        """A single update should have change made and line updated."""
+        opt, val = ("UsePAM", "no")
+        lines = ssh_util.parse_ssh_config_lines(list(self.exlines))
+        result = ssh_util.update_ssh_config_lines(lines, {opt: val})
+        self.assertEqual([opt], result)
+        self.check_line(lines[1], opt, val)
+
+    def test_multiple_updates_with_add(self):
+        """Verify multiple updates some added some changed, some not."""
+        updates = {"UsePAM": "no", "X11Forwarding": "no", "NewOpt": "newval",
+                   "AcceptEnv": "LANG ADD LC_*"}
+        lines = ssh_util.parse_ssh_config_lines(list(self.exlines))
+        result = ssh_util.update_ssh_config_lines(lines, updates)
+        self.assertEqual(set(["UsePAM", "NewOpt", "AcceptEnv"]), set(result))
+        self.check_line(lines[3], "AcceptEnv", updates["AcceptEnv"])
+
+    def test_return_empty_if_no_changes(self):
+        """If there are no changes, then return should be empty list."""
+        updates = {"UsePAM": "yes"}
+        lines = ssh_util.parse_ssh_config_lines(list(self.exlines))
+        result = ssh_util.update_ssh_config_lines(lines, updates)
+        self.assertEqual([], result)
+        self.assertEqual(self.exlines, [str(l) for l in lines])
+
+    def test_keycase_not_modified(self):
+        """Original case of key should not be changed on update.
+        This behavior is to keep original config as much intact as can be."""
+        updates = {"usepam": "no"}
+        lines = ssh_util.parse_ssh_config_lines(list(self.exlines))
+        result = ssh_util.update_ssh_config_lines(lines, updates)
+        self.assertEqual(["usepam"], result)
+        self.assertEqual("UsePAM no", str(lines[1]))
+
+
+class TestUpdateSshConfig(test_helpers.CiTestCase):
+    cfgdata = '\n'.join(["#Option val", "MyKey ORIG_VAL", ""])
+
+    def test_modified(self):
+        mycfg = self.tmp_path("ssh_config_1")
+        util.write_file(mycfg, self.cfgdata)
+        ret = ssh_util.update_ssh_config({"MyKey": "NEW_VAL"}, mycfg)
+        self.assertTrue(ret)
+        found = util.load_file(mycfg)
+        self.assertEqual(self.cfgdata.replace("ORIG_VAL", "NEW_VAL"), found)
+        # assert there is a newline at end of file (LP: #1677205)
+        self.assertEqual('\n', found[-1])
+
+    def test_not_modified(self):
+        mycfg = self.tmp_path("ssh_config_2")
+        util.write_file(mycfg, self.cfgdata)
+        with patch("cloudinit.ssh_util.util.write_file") as m_write_file:
+            ret = ssh_util.update_ssh_config({"MyKey": "ORIG_VAL"}, mycfg)
+        self.assertFalse(ret)
+        self.assertEqual(self.cfgdata, util.load_file(mycfg))
+        m_write_file.assert_not_called()
+
+
 # vi: ts=4 expandtab