Merge lp:~gmb/lpsetup/dont-blow-away-ssh-keys-bug-1018823 into lp:lpsetup

[Graham Binns]

lp-setup will no longer allow you to overwrite your existing SSH keys with new values. Instead, if it encounters an existing SSH key with a value different from the one it's trying to write, it will exit with an error.

This branch fixes the initialize step in init_host so as to prevent the user's SSH keys being overwritten with garbage if they happen to not be paying attention to what they're doing.

I couldn't find any direct tests for initialize(), so I moved the functionality that overwrites the SSH keyfiles out into a utility function and added a doctest for it. The new functionality is to raise an error if the file already exists with different contents from those we're trying to write to it (so opening the file, reading it and then writing its contents back again - our existing behaviour - shouldn't cause problems). I've also updated python-shelltoolboxes generate_ssh_keys() function to ensure that it raises an error should you try to generate new SSH keys with the same name as existing ones.

[Benji York (benji)]

This looks good.

I am a little worried that mode could be something like "w+" or "a+" and that would break the function. Perhaps an assertion that the mode is "w" or "r" and nothing else.

review: Approve (code)

[lp:~gmb/lpsetup/dont-blow-away-ssh-keys-bug-1018823]

41. By Graham Binns on 2012-07-03

Added a test to ensure mode sanity.

=== modified file 'lpsetup/subcommands/inithost.py'
--- lpsetup/subcommands/inithost.py	2012-06-28 10:33:46 +0000
+++ lpsetup/subcommands/inithost.py	2012-07-03 16:47:18 +0000
@@ -40,6 +40,55 @@
 from lpsetup.utils import call
 
 
+def write_file_contents(filename, contents, mode):
+    """Write `contents` to filename and chmod it to 0644.
+
+        >>> write_file_contents('/tmp/foo', 'Hello, world', 'w')
+        >>> print open('/tmp/foo', 'r').read()
+        Hello, world
+        <BLANKLINE>
+        >>> os.remove('/tmp/foo')
+
+    If the file already exists, `mode` is set to 'w', and the file's
+    contents differ from `contents`, write_file_contents() raises an
+    Exception.
+
+        >>> write_file_contents('/tmp/foo', 'Hello, world', 'w')
+        >>> write_file_contents(
+        ...     '/tmp/foo', 'Hello again, world', 'w') # doctest: +ELLIPSIS
+        Traceback (most recent call last):
+        Exception: File /tmp/foo already exists with different contents...
+        >>> os.remove('/tmp/foo')
+
+    If the existing file's contents don't differ from `contents`,
+    write_file_contents() will simply return.
+
+        >>> write_file_contents('/tmp/foo', 'Hello, world', 'w')
+        >>> write_file_contents(
+        ...     '/tmp/foo', 'Hello, world', 'w') # doctest: +ELLIPSIS
+        >>> os.remove('/tmp/foo')
+
+    Passing a `mode` other than "a" or "w' will cause an Exception.
+
+        >>> write_file_contents(
+        ...     '/tmp/foo', 'Hello, world', 'w+') # doctest: +ELLIPSIS
+        Traceback (most recent call last):
+        AssertionError: Mode should be 'w' or 'a' only...
+    """
+    assert mode in ('w', 'a'), "Mode should be 'w' or 'a' only."
+    if mode == 'w' and os.path.exists(filename):
+        with open(filename, 'r') as existing_file:
+            file_contents = existing_file.read()
+        if file_contents.strip() != contents.strip():
+            raise Exception(
+                "File %s already exists with different contents." % filename)
+        else:
+            return
+    with open(filename, mode) as f:
+        f.write(contents + '\n')
+    os.chmod(filename, 0644)
+
+
 def initialize(
     user, full_name, email, lpuser, private_key, public_key, valid_ssh_keys,
     ssh_key_path, feed_random):
@@ -71,9 +120,7 @@
             (auth_file, public_key, 'a'),
             (known_hosts, known_host_content, 'a'),
             ]:
-            with open(filename, mode) as f:
-                f.write(contents + '\n')
-            os.chmod(filename, 0644)
+            write_file_contents(filename, contents, mode)
         os.chmod(ssh_key_path, 0600)
         # Set up bzr and Launchpad authentication.
         call('bzr', 'whoami', formataddr([full_name, email]))