yellow team mailing list archive
-
yellow team
-
Mailing list archive
-
Message #01061
[Merge] lp:~frankban/lpsetup/remove-ssh-args into lp:lpsetup
Francesco Banconi has proposed merging lp:~frankban/lpsetup/remove-ssh-args into lp:lpsetup.
Requested reviews:
Yellow Squad (yellow)
Related bugs:
Bug #1033454 in lpsetup: "lpsetup: delete private and public key arguments and logic from inithost."
https://bugs.launchpad.net/lpsetup/+bug/1033454
For more details, see:
https://code.launchpad.net/~frankban/lpsetup/remove-ssh-args/+merge/118330
== 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.
--
https://code.launchpad.net/~frankban/lpsetup/remove-ssh-args/+merge/118330
Your team Yellow Squad is requested to review the proposed merge of lp:~frankban/lpsetup/remove-ssh-args into lp:lpsetup.
=== modified file 'lpsetup/handlers.py'
--- lpsetup/handlers.py 2012-07-31 09:41:24 +0000
+++ lpsetup/handlers.py 2012-08-06 10:43:18 +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):
- raise ValidationError(
- "arguments private-key and public-key: "
- "both must be provided or neither must be provided.")
+ 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(
+ '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 10:43:18 +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 10:43:18 +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 10:43:18 +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 10:43:18 +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 10:43:18 +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):
