Merge lp:~frankban/lpsetup/remove-ssh-args into lp:lpsetup

[Francesco Banconi]

Removed ssh private and public key arguments.

== Changes ==

Removed ssh private and public key arguments: updated subcommands and tests accordingly.

The handler *handle_ssh_keys* now just adds to namespace the names *ssh_key_path* and *valid_ssh_keys*, and raises an error if a key (of the ssh key pair) is found but not the other.

The integration test `test_install_lxc.py` pass.

[Brad Crittenden (bac)]

Looks good. Thanks Francesco.

review: Approve (code)

[Launchpad QA Bot (lpqabot)]

The attempt to merge lp:~frankban/lpsetup/remove-ssh-args into lp:lpsetup failed. Below is the output from the failed tests.

./lpsetup/handlers.py
     179: E111 indentation is not a multiple of four

[Francesco Banconi (frankban)]

review: Approve

[Launchpad QA Bot (lpqabot)]

The attempt to merge lp:~frankban/lpsetup/remove-ssh-args into lp:lpsetup failed. Below is the output from the failed tests.

nose.plugins.cover: ERROR: Coverage not available: unable to import coverage module
.........F.......................usage: nosetests [-h] {init-repo,help} ...
nosetests: error: argument user can not be omitted if the script is run as root.
Eusage: nosetests [-h] {init-repo,help} ...
nosetests: error: argument user can not be omitted if the script is run as root.
Eusage: nosetests [-h] {init-repo,help} ...
nosetests: error: argument user can not be omitted if the script is run as root.
Eusage: nosetests init-repo [-h]
                           [-s {fetch,setup_bzr_locations} [{fetch,setup_bzr_locations} ...]]
                           [--skip-steps {fetch,setup_bzr_locations} [{fetch,setup_bzr_locations} ...]]
                           [-y] [--dry-run] [--source SOURCE] [--use-http]
                           [--branch-name BRANCH_NAME]
                           [--checkout-name CHECKOUT_NAME] [-r REPOSITORY]
                           [--no-checkout]
nosetests init-repo: error: argument user can not be omitted if the script is run as root.
Eusage: nosetests init-repo [-h]
                           [-s {fetch,setup_bzr_locations} [{fetch,setup_bzr_locations} ...]]
                           [--skip-steps {fetch,setup_bzr_locations} [{fetch,setup_bzr_locations} ...]]
                           [-y] [--dry-run] [--source SOURCE] [--use-http]
                           [--branch-name BRANCH_NAME]
                           [--checkout-name CHECKOUT_NAME] [-r REPOSITORY]
                           [--no-checkout]
nosetests init-repo: error: argument user can not be omitted if the script is run as root.
Eusage: nosetests init-repo [-h]
                           [-s {fetch,setup_bzr_locations} [{fetch,setup_bzr_locations} ...]]
                           [--skip-steps {fetch,setup_bzr_locations} [{fetch,setup_bzr_locations} ...]]
                           [-y] [--dry-run] [--source SOURCE] [--use-http]
                           [--branch-name BRANCH_NAME]
                           [--checkout-name CHECKOUT_NAME] [-r REPOSITORY]
                           [--no-checkout]
nosetests init-repo: error: argument user can not be omitted if the script is run as root.
E...usage: nosetests init-host [-h]
                           [-s {initialize,setup_home,initialize_lxc,setup_apt} [{initialize,setup_home,initialize_lxc,setup_apt} ...]]
                           [--skip-steps {initialize,setup_home,initialize_lxc,setup_apt} [{initialize,setup_home,initialize_lxc,setup_apt} ...]]
                           [-y] [--dry-run] [-u USER] [-E EMAIL]
                           [-f FULL_NAME] [-l LPUSER] [-S SSH_KEY_NAME]
nosetests init-host: error: argument user can not be omitted if the script is run as root.
E.usage: nosetests [-h] {update,help} ...
nosetests: error: argument user can not be omitted if the script is run as root.
Eusage: nosetests [-h] {update,help} ...
nosetests: error: argument user can not be omitted if the script is run as root.
Eusage: nosetests [-h] {update,help} ...
nosetests: error: argument user can not ...

=== modified file 'lpsetup/handlers.py'
--- lpsetup/handlers.py	2012-07-31 09:41:24 +0000
+++ lpsetup/handlers.py	2012-08-06 15:14:31 +0000
@@ -168,72 +168,17 @@
     The namespace must contain *home_dir* and *ssh_key_name*.
     It should be invoked after handle_user.
 
-    Keys contained in the namespace are escaped::
-
-        >>> import argparse
-        >>> private = r'PRIVATE\nKEY'
-        >>> public = r'PUBLIC\nKEY'
-        >>> namespace = argparse.Namespace(
-        ...     private_key=private, public_key=public,
-        ...     ssh_key_name='id_rsa', home_dir='/tmp/')
-        >>> handle_ssh_keys(namespace)
-        >>> namespace.private_key == private.decode('string-escape')
-        True
-        >>> namespace.public_key == public.decode('string-escape')
-        True
-        >>> namespace.valid_ssh_keys
-        True
-
-    After this handler is called, the ssh key path is present as an attribute
-    of the namespace::
-
-        >>> namespace.ssh_key_path
-        '/tmp/.ssh/id_rsa'
-
-    Keys are None if they are not provided and can not be found in the
-    current home directory::
-
-        >>> namespace = argparse.Namespace(
-        ...     private_key=None, public_key=None, ssh_key_name='id_rsa',
-        ...     home_dir='/tmp/__does_not_exists__')
-        >>> handle_ssh_keys(namespace) # doctest: +ELLIPSIS
-        >>> print namespace.private_key
-        None
-        >>> print namespace.public_key
-        None
-        >>> namespace.valid_ssh_keys
-        False
-
-    If only one of private_key and public_key is provided, a
-    ValidationError will be raised.
-
-        >>> namespace = argparse.Namespace(
-        ...     private_key=private, public_key=None, ssh_key_name='id_rsa',
-        ...     home_dir='/tmp/__does_not_exists__')
-        >>> handle_ssh_keys(namespace) # doctest: +ELLIPSIS
-        Traceback (most recent call last):
-        ValidationError: arguments private-key...
+    This handler adds *ssh_key_path* and *valid_ssh_keys* to the namespace.
     """
-    namespace.valid_ssh_keys = True
     namespace.ssh_key_path = os.path.join(
         namespace.home_dir, '.ssh', namespace.ssh_key_name)
-    for attr, path in (
-        ('private_key', namespace.ssh_key_path),
-        ('public_key', namespace.ssh_key_path + '.pub')):
-        value = getattr(namespace, attr, None)
-        if value:
-            setattr(namespace, attr, value.decode('string-escape'))
-        else:
-            try:
-                value = open(path).read()
-            except IOError:
-                value = None
-                namespace.valid_ssh_keys = False
-            setattr(namespace, attr, value)
-    if bool(namespace.private_key) != bool(namespace.public_key):
+    namespace.valid_ssh_keys = os.path.isfile(namespace.ssh_key_path)
+    public_key_path = namespace.ssh_key_path + '.pub'
+    # Exits with an error if only one key of the ssh pair is found.
+    if namespace.valid_ssh_keys != os.path.isfile(public_key_path):
         raise ValidationError(
-            "arguments private-key and public-key: "
-            "both must be provided or neither must be provided.")
+            'ssh private and public keys {0}(.pub): both must exist '
+            'or neither must exist.'.format(namespace.ssh_key_path))
 
 
 def handle_directories(namespace):

=== modified file 'lpsetup/subcommands/inithost.py'
--- lpsetup/subcommands/inithost.py	2012-07-30 12:04:54 +0000
+++ lpsetup/subcommands/inithost.py	2012-08-06 15:14:31 +0000
@@ -99,7 +99,7 @@
     os.chmod(filename, 0644)
 
 
-def setup_ssh(ssh_dir, private_key, public_key, valid_ssh_keys, ssh_key_path):
+def setup_ssh(ssh_dir, valid_ssh_keys, ssh_key_path):
     """Set up the user's `.ssh` directory.
 
     Also ensure that SSH is configured correctly for the user.
@@ -107,28 +107,24 @@
     # Set up the user's ssh directory.  The ssh key must be associated
     # with the lpuser's Launchpad account.
     mkdirs(ssh_dir)
-    # Generate user ssh keys if none are supplied.
+    # Generate user ssh keys if they don't exist.
     pub_key_path = ssh_key_path + '.pub'
     if not valid_ssh_keys:
         generate_ssh_keys(ssh_key_path)
-        private_key = open(ssh_key_path).read()
-        public_key = open(pub_key_path).read()
-    auth_file = os.path.join(ssh_dir, 'authorized_keys')
+    # Write the user's public key to `authorized_keys` so that the user can
+    # non-interactively ssh to the container.
+    authorized_keys = os.path.join(ssh_dir, 'authorized_keys')
+    authorized_keys_contents = open(pub_key_path).read()
+    # Add bazaar.launchpad.net to `known_hosts`.
     known_hosts = os.path.join(ssh_dir, 'known_hosts')
-    known_host_content = run(
+    known_host_contents = run(
         'ssh-keyscan', '-t', 'rsa', 'bazaar.launchpad.net')
     files_to_write = [
-        (auth_file, public_key, 'a'),
-        (known_hosts, known_host_content, 'a'),
+        (authorized_keys, authorized_keys_contents, 'a'),
+        (known_hosts, known_host_contents, 'a'),
         ]
-    if valid_ssh_keys:
-        # We only write out the SSH keys if we haven't had to
-        # generate them further up this function.
-        files_to_write.append((ssh_key_path, private_key, 'w'))
-        files_to_write.append((pub_key_path, public_key, 'w'))
     for filename, contents, mode in files_to_write:
         write_file_contents(filename, contents, mode, header=get_file_header())
-    os.chmod(ssh_key_path, 0600)
 
 
 def initialize_base(user):
@@ -182,9 +178,7 @@
 """.format(modules=LP_APACHE_MODULES, hosts=HOSTS_FILE)
 
 
-def setup_home(
-    user, full_name, email, lpuser, private_key, public_key, valid_ssh_keys,
-    ssh_key_path):
+def setup_home(user, full_name, email, lpuser, valid_ssh_keys, ssh_key_path):
     """Initialize the user home directory.
 
     This is a separate step for several reasons::
@@ -198,9 +192,7 @@
     """
     with su(user) as env:
         # Set up the `.ssh` directory.
-        setup_ssh(
-            os.path.join(env.home, '.ssh'), private_key, public_key,
-            valid_ssh_keys, ssh_key_path)
+        setup_ssh(os.path.join(env.home, '.ssh'), valid_ssh_keys, ssh_key_path)
 
         # Set up bzr and Launchpad authentication.
         call('bzr', 'whoami', formataddr([full_name, email]))
@@ -272,7 +264,7 @@
 
     setup_home_step = (setup_home,
          'user', 'full_name', 'email', 'lpuser',
-         'private_key', 'public_key', 'valid_ssh_keys', 'ssh_key_path')
+         'valid_ssh_keys', 'ssh_key_path')
 
     initialize_lxc_step = (initialize_lxc, )
 
@@ -321,20 +313,6 @@
                  'check out dependencies. If not provided, the system '
                  'user name is used.')
         parser.add_argument(
-            '-v', '--private-key',
-            help='The SSH private key for the Launchpad user (without '
-                 'passphrase). If this argument is omitted and a keypair is '
-                 'not found in the home directory of the system user a new '
-                 'SSH keypair will be generated and the checkout of the '
-                 'Launchpad code will use HTTP rather than bzr+ssh.')
-        parser.add_argument(
-            '-b', '--public-key',
-            help='The SSH public key for the Launchpad user. '
-                 'If this argument is omitted and a keypair is not found '
-                 'in the home directory of the system user a new SSH '
-                 'keypair will be generated and the checkout of the '
-                 'Launchpad code will use HTTP rather than bzr+ssh.')
-        parser.add_argument(
             '-S', '--ssh-key-name', default=SSH_KEY_NAME,
             help='{0} [DEFAULT={1}]'.format(
                 'The ssh key name used to connect to Launchpad.',

=== modified file 'lpsetup/subcommands/initlxc.py'
--- lpsetup/subcommands/initlxc.py	2012-07-31 12:35:51 +0000
+++ lpsetup/subcommands/initlxc.py	2012-08-06 15:14:31 +0000
@@ -208,7 +208,7 @@
 
 
 def inithost_in_lxc(lxc_name, ssh_key_path, user, email, full_name, lpuser,
-                    private_key, public_key, ssh_key_name, home_dir):
+                    ssh_key_name, home_dir):
     """Prepare the Launchpad environment inside an LXC."""
     # Use ssh to call this script from inside the container.
     args = ['init-host', '--yes', '-u', user, '-E', email, '-f', full_name,
@@ -244,7 +244,7 @@
         'lpsetup_branch')
     inithost_in_lxc_step = (inithost_in_lxc,
         'lxc_name', 'ssh_key_path', 'user', 'email', 'full_name', 'lpuser',
-        'private_key', 'public_key', 'ssh_key_name', 'home_dir')
+        'ssh_key_name', 'home_dir')
     stop_lxc_step = (stop_lxc,
         'lxc_name', 'ssh_key_path')
 

=== modified file 'lpsetup/tests/subcommands/test_inithost.py'
--- lpsetup/tests/subcommands/test_inithost.py	2012-07-18 10:53:15 +0000
+++ lpsetup/tests/subcommands/test_inithost.py	2012-08-06 15:14:31 +0000
@@ -21,7 +21,7 @@
 initialize_step = (inithost.initialize, ['user'])
 setup_home_step = (
     inithost.setup_home, ['user', 'full_name', 'email', 'lpuser',
-    'private_key', 'public_key', 'valid_ssh_keys', 'ssh_key_path',
+    'valid_ssh_keys', 'ssh_key_path',
     ])
 initialize_lxc_step = (inithost.initialize_lxc, [])
 setup_apt_step = (inithost.setup_apt, [])
@@ -32,12 +32,9 @@
     email = get_random_string()
     full_name = get_random_string() + '@example.com'
     lpuser = get_random_string()
-    private_key = get_random_string()
-    public_key = get_random_string()
     ssh_key_name = get_random_string()
-    return (
-        '-u', user, '-E', email, '-f', full_name, '-l', lpuser,
-        '-v', private_key, '-b', public_key, '-S', ssh_key_name)
+    return ('-u', user, '-E', email, '-f', full_name, '-l', lpuser,
+            '-S', ssh_key_name)
 
 
 class InithostSmokeTest(StepsBasedSubCommandTestMixin, unittest.TestCase):
@@ -112,7 +109,7 @@
             '/tmp/foo', 'Hello, world!', 'a+')
 
 
-class SetupSSHTestCase(unittest.TestCase):
+class SetupSSHTest(unittest.TestCase):
     """Tests for inithost.setup_ssh()."""
 
     def setUp(self):
@@ -122,45 +119,36 @@
         self.ssh_dir = tempfile.mkdtemp()
         self.addCleanup(shutil.rmtree, self.ssh_dir)
         self.addCleanup(os.remove, self.temp_filename)
-        self.addCleanup(os.remove, self.temp_filename + ".pub")
-
-    def test_setup_ssh_writes_to_ssh_key_path(self):
-        # If inithost.setup_ssh() is told that the keys it has
-        # been passed are valid, it will write them out to the provided
-        # ssh_key_path.
-        public_key = "Public"
-        private_key = "Private"
-        inithost.setup_ssh(
-            self.ssh_dir, private_key, public_key, True, self.temp_filename)
-        with open(self.temp_filename + '.pub', 'r') as pub_file:
-            self.assertIn(public_key, pub_file.read())
-        with open(self.temp_filename, 'r') as priv_file:
-            self.assertIn(private_key, priv_file.read())
-
-    def test_setup_ssh_generates_keys_if_not_passed(self):
-        # If SSH keys aren't passed to setup_ssh(), the function
-        # will generate some.
-        inithost.setup_ssh(
-            self.ssh_dir, None, None, False, self.temp_filename)
+        self.addCleanup(os.remove, self.temp_filename + '.pub')
+
+    def test_setup_ssh_generates_keys_if_not_present(self):
+        # If SSH keys do not exist, the function will generate some.
+        inithost.setup_ssh(self.ssh_dir, False, self.temp_filename)
         self.assertTrue(os.path.exists(self.temp_filename))
-        self.assertTrue(os.path.exists(self.temp_filename + ".pub"))
-
-    def test_setup_ssh_generates_other_files(self):
-        # setup_ssh() also generates an authorized_keys file and a
-        # known_hosts file in the ssh dir.
-        inithost.setup_ssh(
-            self.ssh_dir, None, None, False, self.temp_filename)
-        self.assertTrue(
-            os.path.exists(os.path.join(self.ssh_dir, 'authorized_keys')))
-        self.assertTrue(
-            os.path.exists(os.path.join(self.ssh_dir, 'known_hosts')))
+        self.assertTrue(os.path.exists(self.temp_filename + '.pub'))
+
+    def test_setup_ssh_generates_authorized_keys(self):
+        # setup_ssh() also generates an authorized_keys file containing
+        # the public key.
+        inithost.setup_ssh(self.ssh_dir, False, self.temp_filename)
+        authorized_keys = os.path.join(self.ssh_dir, 'authorized_keys')
+        self.assertTrue(os.path.exists(authorized_keys))
+        public_key = open(self.temp_filename + '.pub').read()
+        self.assertIn(public_key, open(authorized_keys).read())
+
+    def test_setup_ssh_generates_known_hosts(self):
+        # setup_ssh() also generates a `known_host` file containing
+        # 'bazaar.launchpad.net'.
+        inithost.setup_ssh(self.ssh_dir, False, self.temp_filename)
+        known_hosts = os.path.join(self.ssh_dir, 'known_hosts')
+        self.assertTrue(os.path.exists(known_hosts))
+        self.assertIn('bazaar.launchpad.net', open(known_hosts).read())
 
     def test_setup_ssh_creates_ssh_dir(self):
         # If the ssh_dir passed to setup_ssh() doesn't exist, it
         # will be created.
         shutil.rmtree(self.ssh_dir)
-        inithost.setup_ssh(
-            self.ssh_dir, None, None, False, self.temp_filename)
+        inithost.setup_ssh(self.ssh_dir, False, self.temp_filename)
         self.assertTrue(os.path.exists(self.ssh_dir))
         self.assertTrue(os.path.isdir(self.ssh_dir))
 

=== modified file 'lpsetup/tests/subcommands/test_initlxc.py'
--- lpsetup/tests/subcommands/test_initlxc.py	2012-07-31 09:38:14 +0000
+++ lpsetup/tests/subcommands/test_initlxc.py	2012-08-06 15:14:31 +0000
@@ -30,7 +30,7 @@
 inithost_in_lxc_step = (
     initlxc.inithost_in_lxc,
     ['lxc_name', 'ssh_key_path', 'user', 'email', 'full_name', 'lpuser',
-     'private_key', 'public_key', 'ssh_key_name',  'home_dir'])
+     'ssh_key_name',  'home_dir'])
 stop_lxc_step = (initlxc.stop_lxc, ['lxc_name', 'ssh_key_path'])
 
 

=== modified file 'lpsetup/tests/test_handlers.py'
--- lpsetup/tests/test_handlers.py	2012-07-31 09:41:24 +0000
+++ lpsetup/tests/test_handlers.py	2012-08-06 15:14:31 +0000
@@ -228,53 +228,52 @@
 
 class HandleSSHKeysTest(HandlersTestMixin, unittest.TestCase):
 
-    home_dir = '/tmp/__does_not_exist__'
-    private = r'PRIVATE\nKEY'
-    public = r'PUBLIC\nKEY'
-    ssh_key_name = 'id_rsa'
+    ssh_key_name = 'my_id_rsa'
 
-    def test_key_escaping(self):
-        # Ensure the keys contained in the namespace are correctly escaped.
-        namespace = argparse.Namespace(
-            private_key=self.private, public_key=self.public,
+    def setUp(self):
+        self.home_dir = tempfile.mkdtemp()
+        self.addCleanup(shutil.rmtree, self.home_dir)
+        self.namespace = argparse.Namespace(
             ssh_key_name=self.ssh_key_name, home_dir=self.home_dir)
-        handle_ssh_keys(namespace)
-        self.assertEqual(
-            self.private.decode('string-escape'),
-            namespace.private_key)
-        self.assertEqual(
-            self.public.decode('string-escape'),
-            namespace.public_key)
-        self.assertTrue(namespace.valid_ssh_keys)
+
+    def get_ssh_key_path(self):
+        return os.path.join(self.home_dir, '.ssh', self.ssh_key_name)
+
+    def write_key(self, path):
+        directory = os.path.dirname(path)
+        if not os.path.exists(directory):
+            os.mkdir(os.path.dirname(path))
+        with open(path, 'w') as f:
+            f.write('KEY CONTENTS')
 
     def test_ssh_key_path_in_namespace(self):
         # After the handler is called, the ssh key path is present
         # as an attribute of the namespace.
-        namespace = argparse.Namespace(
-            private_key=self.private, public_key=self.public,
-            ssh_key_name=self.ssh_key_name, home_dir=self.home_dir)
-        handle_ssh_keys(namespace)
-        expected = self.home_dir + '/.ssh/id_rsa'
-        self.assertEqual(expected, namespace.ssh_key_path)
-
-    def test_no_keys(self):
-        # Keys are None if they are not provided and can not be found in the
-        # current home directory.
-        namespace = argparse.Namespace(
-            private_key=None, ssh_key_name=self.ssh_key_name,
-            home_dir=self.home_dir)
-        handle_ssh_keys(namespace)
-        self.assertIsNone(namespace.private_key)
-        self.assertIsNone(namespace.public_key)
-        self.assertFalse(namespace.valid_ssh_keys)
+        handle_ssh_keys(self.namespace)
+        self.assertEqual(self.get_ssh_key_path(), self.namespace.ssh_key_path)
+
+    def test_invalid_keys(self):
+        # Keys are marked as invalid if they can not be found in the
+        # current home directory.
+        handle_ssh_keys(self.namespace)
+        self.assertFalse(self.namespace.valid_ssh_keys)
+
+    def test_valid_keys(self):
+        # Keys are marked as valid if they are correctly found in the
+        # current home directory.
+        ssh_key_path = self.get_ssh_key_path()
+        self.write_key(ssh_key_path)
+        self.write_key(ssh_key_path + '.pub')
+        handle_ssh_keys(self.namespace)
+        self.assertTrue(self.namespace.valid_ssh_keys)
 
     def test_only_one_key(self):
-        # Ensure a `ValidationError` is raised if only one key is provided.
-        namespace = argparse.Namespace(
-            private_key=self.private, public_key=None,
-            ssh_key_name=self.ssh_key_name, home_dir=self.home_dir)
-        with self.assertNotValid('private-key'):
-            handle_ssh_keys(namespace)
+        # Ensure the handler raises a ValidationError if only one key
+        # of the private/public pair is found.
+        ssh_key_path = self.get_ssh_key_path()
+        self.write_key(ssh_key_path)
+        with self.assertNotValid(ssh_key_path):
+            handle_ssh_keys(self.namespace)
 
 
 class HandleTargetFromRepositoryTest(unittest.TestCase):