Merge lp:~makyo/juju-quickstart/ssh-2-create-keys into lp:juju-quickstart

[Madison Scott-Clary]

Generate SSH keys if none exist.

This is step 2: provide the user with the option to let Quickstart generate SSH keys.

Note: this is now an incremental branch, with better checks on ssh identities in the future; there's a card to represent this on the kanban board.

To test: make check

to QA:
1. mv ~/.ssh ~/.ssh.old
2a. .venv/bin/python juju-quickstart # should ask if you want to generate; say no, should exit
2b. .venv/bin/python juju-quickstart # should ask if you want to generate; say yes, shoud prompt for password, then continue, generating a new key
3. mv ~/.ssh.old ~/.ssh
4. ssh-agent
5. ssh-add
6. .venv/bin/python juju-quickstart # should bootstrap

https://codereview.appspot.com/36080044/

[Madison Scott-Clary (makyo)]

Reviewers: mp+198135_code.launchpad.net,

Message:
Please take a look.

Description:
Generate SSH keys if none exist.

This is step 2: provide the user with the option to let Quickstart
generate SSH keys.

To test: make check

to QA:
1. mv ~/.ssh ~/.ssh.old
2a. .venv/bin/python juju-quickstart # should ask if you want to
generate; say no, should exit
2b. .venv/bin/python juju-quickstart # should ask if you want to
generate; say yes, shoud prompt for password, then continue, generating
a new key
3. mv ~/.ssh.old ~/.ssh
4. ssh-agent
5. ssh-add
6. .venv/bin/python juju-quickstart # should bootstrap

https://code.launchpad.net/~makyo/juju-quickstart/ssh-2-create-keys/+merge/198135

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/36080044/

Affected files (+108, -20 lines):
   A [revision details]
   M quickstart/app.py
   M quickstart/tests/test_app.py

[Francesco Banconi (frankban)]

LGTM with (possible) changes, thank you!

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py#newcode101
quickstart/app.py:101: if len(create_keys) > 0 and
create_keys[0].lower() == 'y':
This can be written just as:
if create_keys.lower() == 'y':

In general, for sequences (like strings), use the fact that if they are
empty they are also False, e.g.:
use "if seq" instead of "if len(seq) [> 0]".

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py#newcode109
quickstart/app.py:109: 'ssh-add')
Thinking about this in retrospective, I am not sure we need the user to
have an ssh agent set up. It seems all we need are ssh keys, so IIUC the
return codes you described, a 130 could still be ok for us. What do you
think? Feel free to disagree.

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py#newcode121
quickstart/app.py:121: retcode, _, error =
utils.call('/usr/bin/ssh-keygen',
Here, if the user decides to continue, we just generate ssh keys without
starting the agent. This is ok IMHO, but then we don't want to require
the agent on the next quickstart run.

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py#newcode128
quickstart/app.py:128: raise ProgramExit('Error generating ssh key!
{}'.format(error))
All the other quickstart messages are lower cased: we might want to
change this, but for now let's follow that pattern.

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py#newcode129
quickstart/app.py:129: print('A new SSH key was generated in
{}'.format(key_file))
Ditto.

https://codereview.appspot.com/36080044/diff/1/quickstart/tests/test_app.py
File quickstart/tests/test_app.py (right):

https://codereview.appspot.com/36080044/diff/1/quickstart/tests/test_app.py#newcode197
quickstart/tests/test_app.py:197: mock_call.assert_has_calls([
assert_called_with? <shrug>

https://codereview.appspot.com/36080044/diff/1/quickstart/tests/test_app.py#newcode239
quickstart/tests/test_app.py:239:
mock_call.assert_called_with('/usr/bin/ssh-keygen',
assert_called_once_with? Cheap double check.

https://codereview.appspot.com/36080044/

[Madison Scott-Clary (makyo)]

Thanks for the review; need to switch check for keys as noted below, but
everything else is done.

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py#newcode101
quickstart/app.py:101: if len(create_keys) > 0 and
create_keys[0].lower() == 'y':
On 2013/12/09 10:45:41, frankban wrote:
> This can be written just as:
> if create_keys.lower() == 'y':

> In general, for sequences (like strings), use the fact that if they
are empty
> they are also False, e.g.:
> use "if seq" instead of "if len(seq) [> 0]".

Done.

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py#newcode109
quickstart/app.py:109: 'ssh-add')
On 2013/12/09 10:45:41, frankban wrote:
> Thinking about this in retrospective, I am not sure we need the user
to have an
> ssh agent set up. It seems all we need are ssh keys, so IIUC the
return codes
> you described, a 130 could still be ok for us. What do you think? Feel
free to
> disagree.

I ran into that with this branch, and agree with you. I don't think we
need to start the agent. The problem I was running into in the original
branch is that an agent was already running as part of working with bzr.
  Going to switch to just keygen and use a different test, not `ssh-add
-l`; I was under the impression that would list keys regardless of agent
status, but testing from a tty, that's not the case.

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py#newcode121
quickstart/app.py:121: retcode, _, error =
utils.call('/usr/bin/ssh-keygen',
On 2013/12/09 10:45:41, frankban wrote:
> Here, if the user decides to continue, we just generate ssh keys
without
> starting the agent. This is ok IMHO, but then we don't want to require
the agent
> on the next quickstart run.

Agreed, but see note about the current ssh-add -l test.

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py#newcode128
quickstart/app.py:128: raise ProgramExit('Error generating ssh key!
{}'.format(error))
On 2013/12/09 10:45:41, frankban wrote:
> All the other quickstart messages are lower cased: we might want to
change this,
> but for now let's follow that pattern.

Done.

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py#newcode129
quickstart/app.py:129: print('A new SSH key was generated in
{}'.format(key_file))
On 2013/12/09 10:45:41, frankban wrote:
> Ditto.

Done.

https://codereview.appspot.com/36080044/diff/1/quickstart/tests/test_app.py
File quickstart/tests/test_app.py (right):

https://codereview.appspot.com/36080044/diff/1/quickstart/tests/test_app.py#newcode197
quickstart/tests/test_app.py:197: mock_call.assert_has_calls([
On 2013/12/09 10:45:41, frankban wrote:
> assert_called_with? <shrug>

Done.

https://codereview.appspot.com/36080044/diff/1/quickstart/tests/test_app.py#newcode239
quickstart/tests/test_app.py:239:
mock_call.assert_called_with('/usr/bin/ssh-keygen',
On 2013/12/09 10:45:41, frankban wrote:
> assert_called_once_with? Cheap double check.

Done.

https://codereview.appspot.com/36080044/

[lp:~makyo/juju-quickstart/ssh-2-create-keys]

31. By Madison Scott-Clary on 2013-12-09

Responding to review.

32. By Madison Scott-Clary on 2013-12-09

Switching to find from ssh-add

[Madison Scott-Clary (makyo)]

Please take a look.

https://codereview.appspot.com/36080044/

[Francesco Banconi (frankban)]

LGTM thank you!

https://codereview.appspot.com/36080044/

[Madison Scott-Clary (makyo)]

*** Submitted:

Generate SSH keys if none exist.

This is step 2: provide the user with the option to let Quickstart
generate SSH keys.

Note: this is now an incremental branch, with better checks on ssh
identities in the future; there's a card to represent this on the kanban
board.

To test: make check

to QA:
1. mv ~/.ssh ~/.ssh.old
2a. .venv/bin/python juju-quickstart # should ask if you want to
generate; say no, should exit
2b. .venv/bin/python juju-quickstart # should ask if you want to
generate; say yes, shoud prompt for password, then continue, generating
a new key
3. mv ~/.ssh.old ~/.ssh
4. ssh-agent
5. ssh-add
6. .venv/bin/python juju-quickstart # should bootstrap

R=frankban
CC=
https://codereview.appspot.com/36080044

https://codereview.appspot.com/36080044/

=== modified file 'quickstart/app.py'
--- quickstart/app.py	2013-12-06 17:18:22 +0000
+++ quickstart/app.py	2013-12-09 18:12:20 +0000
@@ -24,6 +24,7 @@
 import functools
 import json
 import logging
+import os
 import time
 
 import jujuclient
@@ -88,18 +89,48 @@
 
 
 def ensure_ssh_keys():
-    retcode = utils.call('/usr/bin/ssh-add', '-l')[0]
-    if retcode > 0:
-        # The return code will be 130 if no agent was found; it will be 127 if
-        # no keys were found.  The return code will be 0 (with an output of
-        # keys) if everything was successful.
-        raise ProgramExit('No ssh keys were found.\n'
-                          'Please run these commands and follow the '
-                          'instructions they produce before running '
-                          'quickstart again:\n\n'
-                          'ssh-keygen -t rsa\n'
-                          'ssh-agent\n'
-                          'ssh-add')
+    """Ensure that SSH keys are available.
+
+    Allow the user to let quickstart create SSH keys, or quit by raising a
+    ProgramExit if they would like to create the key themselves.
+    """
+    ssh_dir = os.path.join(os.path.expanduser('~'), '.ssh')
+    retcode = utils.call('/usr/bin/find', ssh_dir, '-name', 'id_*.pub')[0]
+    if retcode > 0:
+        print('No SSH keys were found in ~/.ssh\n\n'
+              'Would you like quickstart to create them for you? Note that '
+              'this will create a new SSH key with the passphrase you provide '
+              'available to your SSH agent. ssh-keygen will ask for a '
+              'passphrase, but juju quickstart will not have access to it.\n'
+              'Alternatively, you may create the keys on your own.\n')
+        create_keys = raw_input('Continue [y/N]? ')
+        if create_keys.lower() == 'y':
+            create_ssh_keys()
+        else:
+            raise ProgramExit('Please run these commands and follow the '
+                              'instructions they produce before running '
+                              'quickstart again:\n\n'
+                              'ssh-keygen -b 4096 -t rsa')
+
+
+def create_ssh_keys():
+    """Create SSH keys for the user
+
+    Raises ProgramExit if the keys were not created successfully.
+
+    NB: this involves user interaction for entering the passphrase; this may
+    have to change if creating SSH keys takes place in the urwid interface.
+    """
+    key_file = os.path.join(os.path.expanduser('~'), '.ssh', 'id_juju_rsa')
+    retcode, _, error = utils.call('/usr/bin/ssh-keygen',
+                                   '-q',  # silent
+                                   '-b', '4096',  # 4096 bytes
+                                   '-t', 'rsa',  # RSA type
+                                   '-C', 'Generated with Juju Quickstart',
+                                   '-f', key_file)
+    if retcode > 0:
+        raise ProgramExit('error generating ssh key!  {}'.format(error))
+    print('a new ssh key was generated in {}'.format(key_file))
 
 
 def ensure_environments():

=== modified file 'quickstart/tests/test_app.py'
--- quickstart/tests/test_app.py	2013-12-06 17:18:22 +0000
+++ quickstart/tests/test_app.py	2013-12-09 18:12:20 +0000
@@ -20,6 +20,7 @@
 
 from contextlib import contextmanager
 import json
+import os
 import unittest
 
 import jujuclient
@@ -176,29 +177,75 @@
             self.assertEqual(3, mock_call.call_count)
 
 
[e-mail]_print
 class TestEnsureSSHKeys(
         helpers.CallTestsMixin, ProgramExitTestsMixin, unittest.TestCase):
 
-    def test_success(self):
+    print_msg = 'No SSH keys were found in ~/.ssh\n\n' \
+                'Would you like quickstart to create them for you? Note ' \
+                'that this will create a new SSH key with the passphrase ' \
+                'you provide available to your SSH agent. ssh-keygen will ' \
+                'ask for a passphrase, but juju quickstart will not have ' \
+                'access to it.\nAlternatively, you may create the keys on ' \
+                'your own.\n'
+    ssh_key_find = ('/usr/bin/find',
+                    os.path.join(os.path.expanduser('~'), '.ssh'),
+                    '-name', 'id_*.pub')
+
+    def patch_raw_input(self, return_value='N'):
+        mock_raw_input = mock.Mock(return_value=return_value)
+        return mock.patch('__builtin__.raw_input', mock_raw_input)
+
+    def test_success(self, mock_print):
         with self.patch_call(0) as mock_call:
             app.ensure_ssh_keys()
-        mock_call.assert_has_calls([
-            mock.call('/usr/bin/ssh-add', '-l')
-        ])
+        mock_call.assert_called_with(*self.ssh_key_find)
 
-    def test_failure(self):
-        msg = 'No ssh keys were found.\n' \
-              'Please run these commands and follow the instructions they ' \
+    def test_failure_no_keygen(self, mock_print):
+        msg = 'Please run these commands and follow the instructions they ' \
               'produce before running quickstart again:\n\n' \
-              'ssh-keygen -t rsa\n' \
-              'ssh-agent\n' \
-              'ssh-add'
+              'ssh-keygen -b 4096 -t rsa'
         with self.assert_program_exit(msg):
             with self.patch_call(130) as mock_call:
-                app.ensure_ssh_keys()
-        mock_call.assert_has_calls([
-            mock.call('/usr/bin/ssh-add', '-l')
-        ])
+                with self.patch_raw_input() as mock_raw_input:
+                    app.ensure_ssh_keys()
+        mock_call.assert_called_with(*self.ssh_key_find)
+        mock_print.assert_has_calls([mock.call(self.print_msg)])
+        self.assertTrue(mock_raw_input.called)
+
+    def test_failure_with_keygen(self, mock_print):
+        with self.patch_call(130) as mock_call:
+            with self.patch_raw_input(return_value='Y') as mock_raw_input:
+                with mock.patch('quickstart.app.create_ssh_keys') \
+                        as mock_create_ssh_keys:
+                    app.ensure_ssh_keys()
+        mock_call.assert_called_with(*self.ssh_key_find)
+        mock_print.assert_has_calls([mock.call(self.print_msg)])
+        self.assertTrue(mock_raw_input.called)
+        self.assertTrue(mock_create_ssh_keys.called)
+
+
[e-mail]_print
+class TestCreateSSHKeys(
+        helpers.CallTestsMixin, ProgramExitTestsMixin, unittest.TestCase):
+
+    def test_success(self, mock_print):
+        key_file = os.path.join(os.path.expanduser('~'), '.ssh', 'id_juju_rsa')
+        with self.patch_call(0) as mock_call:
+            app.create_ssh_keys()
+        mock_call.assert_called_once_with('/usr/bin/ssh-keygen',
+                                          '-q', '-b', '4096', '-t', 'rsa',
+                                          '-C',
+                                          'Generated with Juju Quickstart',
+                                          '-f', key_file)
+        mock_print.assert_called_with('a new ssh key was generated in {}'
+                                      .format(key_file))
+
+    def test_failure(self, mock_print):
+        with self.assert_program_exit('error generating ssh key!  Oh no!'):
+            with self.patch_call(1, error='Oh no!') as mock_call:
+                app.create_ssh_keys()
+        self.assertTrue(mock_call.called)
 
 
 @helpers.mock_print