Generate SSH keys if none exist.

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://code.launchpad.net/~makyo/juju-quickstart/ssh-2-create-keys/+merge/198135

(do not edit description out of merge proposal)

[matthew.scott, 2013-12-06 23:15:24]

Please take a look.

[frankban, 2013-12-09 10:45:41]

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#n...
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#n...
quickstart/tests/test_app.py:239:
mock_call.assert_called_with('/usr/bin/ssh-keygen',
assert_called_once_with? Cheap double check.

[matthew.scott, 2013-12-09 17:15:57]

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#n...
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#n...
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.

[matthew.scott, 2013-12-09 18:12:34]

Please take a look.

[frankban, 2013-12-09 18:26:06]

LGTM thank you!

[matthew.scott, 2013-12-09 18:38:11]

*** 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