Watch for SSH key creation

Watch for SSH key creation

Allow the user to create SSH keys in another term/window, watch for creation and continue when available.

To test: make check
To QA:
1. Move ~/.ssh to a back up
2. Run quickstart - select w when prompted to watch
3. Create ssh keys as instructed in another term/window
4. Quickstart should continue
5. Move ~/.ssh back; other options should work as expected.

https://code.launchpad.net/~makyo/juju-quickstart/ssh-3-watch-keys/+merge/198851

(do not edit description out of merge proposal)

[matthew.scott, 2013-12-13 00:21:05]

Please take a look.

[gary.poster, 2013-12-13 01:49:45]

Wording suggestions.  Didn't look other than that for now.

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

https://codereview.appspot.com/39610049/diff/1/quickstart/app.py#newcode99
quickstart/app.py:99: 'Would you like quickstart to create them for you? Note
that '
What do you think of something like this?  Please feel free to polish or
counter-propose.

----

No SSH keys were found in ~/.ssh

To proceed, quickstart can automatically create keys for you [a], or it can give
you a command to run yourself [s] in another terminal or window to create the
keys.

Note that, if you want quickstart to create your key, the underlying program
(ssh-keygen) will ask for a passphrase, but quickstart will not have access to
it.

Automatically create keys [a], create the keys yourself [s], or cancel [C]?

https://codereview.appspot.com/39610049/diff/1/quickstart/app.py#newcode112
quickstart/app.py:112: raise ProgramExit('Please run this command and follow the
'
If you would like to create the keys yourself, please run this command and then
re-run quickstart:

ssh-keygen -b 4096 -t rsa

https://codereview.appspot.com/39610049/diff/1/quickstart/app.py#newcode120
quickstart/app.py:120: return utils.call('/usr/bin/find', ssh_dir, '-name',
'id_*.pub')[0]
We can wait for Windows compatibility, but it would be nice if quickstart used
built in Python tools rather than find.  <shrug> for now.

[gary.poster, 2013-12-13 13:51:45]

What you've added looks good to me, with the changes I've suggested, but QA bad.

------------------------------------------------
Continue [y/N] or watch for new keys [w]? w
Please run this command in another terminal or window and follow the
instructions it produces; quickstart will continue when keys are generated. ^C
to quit.

ssh-keygen -b 4096 -t rsa

.
.
.
.
.
.
.
.
.
.
.
bootstrapping the ec2 environment (type: ec2)
juju-quickstart: error: ERROR error parsing environment "ec2": no public ssh
keys found
------------------------------------------------

Do we need to start (restart) an agent?

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

https://codereview.appspot.com/39610049/diff/1/quickstart/app.py#newcode112
quickstart/app.py:112: raise ProgramExit('Please run this command and follow the
'
On 2013/12/13 01:49:45, gary.poster wrote:
> If you would like to create the keys yourself, please run this command 

, follow its instructions,

> and then
> re-run quickstart:
> 
> ssh-keygen -b 4096 -t rsa

https://codereview.appspot.com/39610049/diff/1/quickstart/app.py#newcode120
quickstart/app.py:120: return utils.call('/usr/bin/find', ssh_dir, '-name',
'id_*.pub')[0]
On 2013/12/13 01:49:45, gary.poster wrote:
> We can wait for Windows compatibility, but it would be nice if quickstart used
> built in Python tools rather than find.  <shrug> for now.
That is: please leave as is for now unless you find yourself really motivated
and it looks really fast and easy.

https://codereview.appspot.com/39610049/diff/1/quickstart/app.py#newcode130
quickstart/app.py:130: print('.')
I think this would be much better.  OK with you?

print('.', end="")

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

https://codereview.appspot.com/39610049/diff/1/quickstart/tests/test_app.py#n...
quickstart/tests/test_app.py:245: def test_watch(self, mock_print, mock_sleep):
Nice test.

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

Thanks for the comments.  Will re-propose after fixing QA issue.

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

https://codereview.appspot.com/39610049/diff/1/quickstart/app.py#newcode99
quickstart/app.py:99: 'Would you like quickstart to create them for you? Note
that '
On 2013/12/13 01:49:45, gary.poster wrote:
> What do you think of something like this?  Please feel free to polish or
> counter-propose.
> 
> ----
> 
> No SSH keys were found in ~/.ssh
> 
> To proceed, quickstart can automatically create keys for you [a], or it can
give
> you a command to run yourself [s] in another terminal or window to create the
> keys.
> 
> Note that, if you want quickstart to create your key, the underlying program
> (ssh-keygen) will ask for a passphrase, but quickstart will not have access to
> it.
> 
> Automatically create keys [a], create the keys yourself [s], or cancel [C]?

Done.

https://codereview.appspot.com/39610049/diff/1/quickstart/app.py#newcode112
quickstart/app.py:112: raise ProgramExit('Please run this command and follow the
'
On 2013/12/13 01:49:45, gary.poster wrote:
> If you would like to create the keys yourself, please run this command and
then
> re-run quickstart:
> 
> ssh-keygen -b 4096 -t rsa

Done.

https://codereview.appspot.com/39610049/diff/1/quickstart/app.py#newcode120
quickstart/app.py:120: return utils.call('/usr/bin/find', ssh_dir, '-name',
'id_*.pub')[0]
On 2013/12/13 13:51:45, gary.poster wrote:
> On 2013/12/13 01:49:45, gary.poster wrote:
> > We can wait for Windows compatibility, but it would be nice if quickstart
used
> > built in Python tools rather than find.  <shrug> for now.
> That is: please leave as is for now unless you find yourself really motivated
> and it looks really fast and easy.

This is encompassed by ssh keys 2.5 - this find is fragile and does not a)
verify that the keys are valid for ssh, or b) take into account that keys can be
stored elsewhere with an ssh config.  Talked with Francesco, leaving it as an
increment.

https://codereview.appspot.com/39610049/diff/1/quickstart/app.py#newcode130
quickstart/app.py:130: print('.')
On 2013/12/13 13:51:45, gary.poster wrote:
> I think this would be much better.  OK with you?
> 
> print('.', end="")

Done.

[matthew.scott, 2013-12-13 21:46:25]

Please take a look.

[gary.poster, 2013-12-13 22:52:55]

Other than needing to add some docstrings, this looks good, but the QA is still
bad.

Note I am using a passphrase when I QA.

I was able to get it to work when I did the following.

(1) In the other terminal, also *add* the new key

ssh-keygen -b 4096 -t rsa && ssh-agent && ssh-add

(2) In the code, I did a time.sleep(10) after it saw the key in order to wait
for me to type the password in.  This is obviously not ideal.

Note that I am pretty sure that create_ssh_keys should run ssh-add also.

Ideally, I think the script would run ssh-agent and ssh-add.  In quick hacks, I
couldn't get this to work.  I started debugging and it might be that it needed
some time in between the call to ssh-agent and ssh-add. I'm not sure.  Note that
ssh-add needs to be interactive, so utils.call can't work as is.  We need stdin
and stdout to be used.

Sorry it is still not working for me.

https://codereview.appspot.com/39610049/diff/20001/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/39610049/diff/20001/quickstart/app.py#newcode119
quickstart/app.py:119: def check_ssh_keys():
docstring please

https://codereview.appspot.com/39610049/diff/20001/quickstart/app.py#newcode124
quickstart/app.py:124: def watch_for_ssh_keys():
docstring please

[matthew.scott, 2013-12-13 23:31:57]

Putting this branch on hold, then.  Thanks for the QA, Gary.  We need to find a
check_ssh_keys method that actually checks if keys are available to the agent,
which will be the case once the user enters the password (no matter how it
works: automatic creation or manual).  I'll add the docstrings and push, then
start researching tonight/this weekend on SSH solutions.

Thanks again

[gary.poster, 2013-12-14 03:12:41]

Cool.

I could very well be wrong, but I think that the script should start the agent
and add the key.  Any thing else seems like it could easily go wrong and hang
forever.

That's what I was trying to get to work, briefly.  It's what the other code path
needs, and it seems like it is not working yet either.

Gary

> On Dec 13, 2013, at 6:31 PM, matthew.scott@canonical.com wrote:
> 
> Putting this branch on hold, then.  Thanks for the QA, Gary.  We need to
> find a check_ssh_keys method that actually checks if keys are available
> to the agent, which will be the case once the user enters the password
> (no matter how it works: automatic creation or manual).  I'll add the
> docstrings and push, then start researching tonight/this weekend on SSH
> solutions.
> 
> Thanks again
> 
> https://codereview.appspot.com/39610049/

[matthew.scott, 2013-12-17 03:24:33]

Please take a look.

[frankban, 2013-12-17 17:22:38]

This branch looks very good Matthew, thank you for this work and for
investigating this ssh stuff!
I wrote a lot of comments, but many of them are just nice to
have/minors/do-whit-them-what-you-want.
LGTM with (possible) changes.
QA ok with some comments (about the initial agent check and the SIGINT
handling).

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode98
quickstart/app.py:98: utils.start_ssh_agent()
It seems an OSError raised from this call would crash the application. Also,
this always starts the agent, even if the user has one already running. Do we
have a way to avoid that? This could also be tackled in another card.

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode108
quickstart/app.py:108: 'keys yourself [s], or cancel [C]? ')
Maybe ^C here too? <shrug>

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode114
quickstart/app.py:114: raise ProgramExit('If you would like to create the keys
yourself, '
This is displayed as an error, but I am not sure this is an error.
sys.exit(msg) could be used instead.

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode120
quickstart/app.py:120: def check_ssh_keys():
Very nice commented function, thank you.

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode124
quickstart/app.py:124: in some way.  Raise OSError (via utils.start_ssh_agent)
if we couldn't
I don't see the start_ssh_agent call here. Is this still true?

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode131
quickstart/app.py:131: retcode, output, error = utils.call('/usr/bin/ssh-add',
'-l')
error does not seem to be used here and can be safely replaced with _

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode150
quickstart/app.py:150: raise ProgramExit(msg.format(error).encode('utf-8'))
You can avoid encoding ProgramExit messages <shrug>.

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode164
quickstart/app.py:164: while not check_ssh_keys():
^C produces a traceback. It's not that important, but we can easily avoid it
with something like:

    try:
        while not check_ssh_keys():
            print('.', end='')
            sys.stdout.flush()
            time.sleep(3)
    except KeyboardInterrupt:
        sys.exit('\nquitting')

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode166
quickstart/app.py:166: sys.stdout.flush()
Interesting, why is this required? A comment would be nice.

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode185
quickstart/app.py:185: if retcode > 0:
Is it ok if retcode is < 0? Can this happen?
If not, we can just check "if retcode:".

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode188
quickstart/app.py:188: if retcode > 0:
Same as above.

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

https://codereview.appspot.com/39610049/diff/40001/quickstart/tests/test_app....
quickstart/tests/test_app.py:240: class TestCheckSSHKeys(
Nice tests!

[matthew.scott, 2013-12-17 21:07:32]

Please take a look.

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode98
quickstart/app.py:98: utils.start_ssh_agent()
On 2013/12/17 17:22:39, frankban wrote:
> It seems an OSError raised from this call would crash the application. Also,
> this always starts the agent, even if the user has one already running. Do we
> have a way to avoid that? This could also be tackled in another card.

Oops, will catch the error.

Through testing, even if another agent is running, quickstart can only use that
agent for authentication, not for adding keys (and so ssh-add will always fail
with rc 2, "Cannot contact agent").  We can investigate this part further in
another card, though, if there's a way around it.

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode108
quickstart/app.py:108: 'keys yourself [s], or cancel [C]? ')
On 2013/12/17 17:22:39, frankban wrote:
> Maybe ^C here too? <shrug>

Done.

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode114
quickstart/app.py:114: raise ProgramExit('If you would like to create the keys
yourself, '
On 2013/12/17 17:22:39, frankban wrote:
> This is displayed as an error, but I am not sure this is an error.
> sys.exit(msg) could be used instead.

Done.

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode124
quickstart/app.py:124: in some way.  Raise OSError (via utils.start_ssh_agent)
if we couldn't
On 2013/12/17 17:22:39, frankban wrote:
> I don't see the start_ssh_agent call here. Is this still true? 

Nope, good catch.

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode131
quickstart/app.py:131: retcode, output, error = utils.call('/usr/bin/ssh-add',
'-l')
On 2013/12/17 17:22:39, frankban wrote:
> error does not seem to be used here and can be safely replaced with _

Done.

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode164
quickstart/app.py:164: while not check_ssh_keys():
On 2013/12/17 17:22:39, frankban wrote:
> ^C produces a traceback. It's not that important, but we can easily avoid it
> with something like:
> 
>     try:
>         while not check_ssh_keys():
>             print('.', end='')
>             sys.stdout.flush()
>             time.sleep(3)
>     except KeyboardInterrupt:
>         sys.exit('\nquitting')

Done.

https://codereview.appspot.com/39610049/diff/40001/quickstart/app.py#newcode166
quickstart/app.py:166: sys.stdout.flush()
On 2013/12/17 17:22:39, frankban wrote:
> Interesting, why is this required? A comment would be nice.

Done.

With end being empty, stdout's buffer won't flush until after it reaches a
certain number of bytes.

[rharding, 2013-12-18 18:40:01]

QA:

The text is too long and should wrap. I had the text wrapping mid-word because
of the length. I think a standard 78ish for terminal work would be really nice.

http://uploads.mitechie.com/lp/quickstart.png

A list of options would read cleaner.

"Quickstart can

[a] Automatically create keys
[s] Provide you commands to create keys yourself
[C] Cancel Quickstart
"

There was a big lack of warning that it was going on into bootstrap and such
after the key creation. Maybe a pause/continue makes sense there?

Trying manual instructions:

What's the .? Is it doing something? I thought it was waiting for me to enter a
command?

Again, the instructions but up. It's not clear what's part of this step and the
last bit of info.

Once I did enter the command, the ...... wasn't cleared before the
'bootstraping' log line so I got. 

........................bootstrapping the azure environment (type: azure)



Trying to cancel:

I get a traceback during a ctrl-c to kill. It should be caught/exit cleanly. 

If I start to automatically create, but cancel when it asks me for a passphrase
it also dumps a traceback at me.



I'd propose something that looks to the user more like:

http://paste.ubuntu.com/6595673/

Note the use of more usual opposites of automatic/manual, indentation to help
distinguish commands to run manually as a callout, etc.

[rharding, 2013-12-19 00:58:01]

The QA was LGTM with the comments and discussion points in my previous message.

[matthew.scott, 2013-12-19 03:33:07]

*** Submitted:

Watch for SSH key creation

Allow the user to create SSH keys in another term/window, watch for creation and
continue when available.

To test: make check
To QA:
1. Move ~/.ssh to a back up
2. Run quickstart - select w when prompted to watch
3. Create ssh keys as instructed in another term/window
4. Quickstart should continue
5. Move ~/.ssh back; other options should work as expected.

R=gary.poster, frankban, rharding
CC=
https://codereview.appspot.com/39610049