cloud-init eats final EOL of sshd_config

[Lorens Kockum]

cloud-init reads in sshd_config, and then writes it out again, but eats the final newline.

It also changes the file mode to 644, which I don't like since the original was 600, but I haven't yet found a decent way to change that.

While I'm at it, spellcheck in comment.

HAND

diff --git a/cloudinit/config/cc_set_passwords.py b/cloudinit/config/cc_set_passwords.py
index eb0bdab0..ed30a9d9 100755
--- a/cloudinit/config/cc_set_passwords.py
+++ b/cloudinit/config/cc_set_passwords.py
@@ -215,7 +215,7 @@ def handle(_name, cfg, cloud, log, args):
                                                      pw_auth))

         lines = [str(l) for l in new_lines]
- util.write_file(ssh_util.DEF_SSHD_CFG, "\n".join(lines))
+ util.write_file(ssh_util.DEF_SSHD_CFG, "\n".join(lines)+"\n")

         try:
             cmd = cloud.distro.init_cmd # Default service
diff --git a/cloudinit/util.py b/cloudinit/util.py
index 33019579..a8e84da8 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -1691,7 +1691,7 @@ def chmod(path, mode):
 def write_file(filename, content, mode=0o644, omode="wb"):
     """
     Writes a file with the given content and sets the file mode as specified.
- Resotres the SELinux context if possible.
+ Restores the SELinux context if possible.

     @param filename: The full path of the file to write.
     @param content: The content to write to the file.

[Scott Moser (smoser)]

I think the change for the permissions would best happen in write_file.
We'd like to say: use existing mode if the file exists, otherwise use X.

maybe an argument: use_existing_mode=True
that would use the existing mode if the file exixsted, otherwise would use 'mode'.
adn default that to False (for backwards compat).

A simpler change for *just this* looks like:
--- a/cloudinit/config/cc_set_passwords.py
+++ b/cloudinit/config/cc_set_passwords.py
@@ -215,7 +215,13 @@ def handle(_name, cfg, cloud, log, args):
                                                      pw_auth))

         lines = [str(l) for l in new_lines]
- util.write_file(ssh_util.DEF_SSHD_CFG, "\n".join(lines))
+ import os, stat
+ mode = 0o600
+ if os.path.exists(ssh_util.DEF_SSHD_CFG):
+ mode = stat.S_IMODE(os.lstat(ssh_util.DEF_SSHD_CFG).st_mode)
+
+ util.write_file(ssh_util.DEF_SSHD_CFG, "\n".join(lines) + "\n",
+ mode=mode)

         try:
             cmd = cloud.distro.init_cmd # Default service

[Lorens Kockum (lorensk)]

Putting a default of 0600 is good (that is the value recommended by CIS, for example).

Attaching an even more elaborate patch that simply doesn't touch the file if not necessary.

One could also modify write_file to not modify the mode of existing files, but if if hasn't been necessary yet it's probably not worth the work.

FYI I found this while automating security audits.

[Lorens Kockum (lorensk)]

I see that

commit 721348a622a660b65acfdf7fdf53203b47f80748
Author: Lars Kellogg-Stedman

has added copy_mode to util.write_file, which is nice, and which has been pushed as an update to Xenial.

diff --git a/cloudinit/config/cc_set_passwords.py b/cloudinit/config/cc_set_passwords.py
index eb0bdab0..bb24d57f 100755
--- a/cloudinit/config/cc_set_passwords.py
+++ b/cloudinit/config/cc_set_passwords.py
@@ -215,7 +215,8 @@ def handle(_name, cfg, cloud, log, args):
                                                      pw_auth))

         lines = [str(l) for l in new_lines]
- util.write_file(ssh_util.DEF_SSHD_CFG, "\n".join(lines))
+ util.write_file(ssh_util.DEF_SSHD_CFG, "\n".join(lines),
+ copy_mode=True)

         try:
             cmd = cloud.distro.init_cmd # Default service

This handles the changed mode but it doesn't change the fact that cloud-init strips the newline on the last line. Aside from needlessly modifying the file, it leaves it non-POSIX-compliant (lines in a text file end with a newline).

Just inserting ."\n" to get

        util.write_file(ssh_util.DEF_SSHD_CFG, "\n".join(lines)."\n",
                        copy_mode=True)

would fix the problem.

Attaching diff.

[Lorens Kockum (lorensk)]

It surprises and pains me that such an obvious bug with such a trivial fix (provided by the reporter, even) is not fixed more than one year later. I have not seen any obvious github-like way of making a pull request, is that something I can do to contribute?

[Scott Moser (smoser)]

Lorens,
Sorry for slow reply.
http://cloudinit.readthedocs.io/en/latest/topics/hacking.html

that describes how to contribute to cloud-init.

You can follow that all the way and put up a merge proposal.
Or if you just sign the contributors agreement, I can grab your patch from here.

[Scott Moser (smoser)]

Lorens,
I've put up
 https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/343246
It accomplishes most of what you were after I think.
Your feedback would be appreciated there.

[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

[Chad Smith (chad.smith)]

This bug is believed to be fixed in cloud-init in version 18.2-27-g6ef92c98-0ubuntu1~18.04.1. If this is still a problem for you, please make a comment and set the state back to New

Thank you.

[Scott Moser (smoser)]

This bug is believed to be fixed in cloud-init in version 18.3. If this is still a problem for you, please make a comment and set the state back to New

Thank you.

[James Falcon (falcojr)]

Tracked in Github Issues as https://github.com/canonical/cloud-init/issues/2846