Issue32 ssh setup.3

[meejah]

Here's a bit more sketched-out version of the idea of a top-level send() and receive() API...This PR also takes out the "just invoke 'wormhole send'" part of the other PR.

[warner]

whoa, did you really get the full test suite to work on pypy? I thought the C code in pynacl was a blocker for that..

[meejah]

I think it's just building the bundled libsodium and using CFFI to call it. But yes everything passes on PyPy with the one change for Deferred __del__ to get called.

[warner]

I like it. I want to nit-pick about the implementation, though :). Could you force-push a new branch with the following changes?

• rebased to current trunk
• move the setup.py change into it's own commit
• move the cli/cli.py traceback/Failure into it's own commit
• remove the server/cli.py timing changes (I don't think it's useful to measure import timing on the server side, as it doesn't affect operational latency)
• move the wormhole.py send()/receive() changes into a new wormhole/xfer_util.py file, and into it's own commit
• last commit should be the changes to cli/cli.py that add the new commands, but the bulk of the code should live in cli/cmd_ssh.py, with just the arg-parsing and dispatch happening in cli/cli.py (the idea is that the unit tests can import cmd_ssh instead of import cli)
• also the APPID needs to be something different than wormhole send/receive, since there's no interop between wormhole send and wormhole ssh-add. Maybe lothar.com/wormhole/ssh-add. It must be the same for the two sides, of course.

I haven't looked closely at the actual .ssh/authorized_keys -touching parts yet, but we can land the stuff above and then apply more fixes later if necessary.

thanks!

[meejah]

Okay, I can mess about with git rebase soon :)

Obviously, it would be good to have more eyes on the ~/.ssh/*-touching parts since that's the meat of this thing ;)

[warner]

Let's see:

• we could bikeshed on the existing command names (wormhole ssh-add, wormhole ssh-send) a bit: use a subcommand (wormhole ssh add), use send/receive to make it parallel with file send/receive, maybe find some synonyms (I like "add", but I don't know what the other side should use)
• it'd be nice if the ssh commands accepted all the same arguments as send/receive (--verify, -0, etc)
• I'm tempted to have the receiving side specify a username, and have it find the matching authorized_keys file. I've got two tentative reasons. 1: if the file doesn't exist already, then .ssh might not exist either, so we need to mkdir that, and we need to set the permissions on the new authorized_keys to make ssh happy, so we need more than just the target filename. 2: there are two main use cases here: either you're logged into your own account (maybe via a temporary password) and you're adding an ssh key from your own machine (so you're the one who will be running ssh-send), or you're logged in as root and you're setting up somebody else's account, in which case we want to make sure it's hard to give root access to your new user by mistake (which would happen if you forgot to provide --auth-file and it defaulted to ~/.ssh/authorized_keys). So if we made username a required positional argument, then that'd make it super-obvious whose shell you're handing out access to
• on the sending side, I'd like to find all the .pub files, and have the user choose between them. Or, if there's more than one, then abort. Maybe require a posarg with the right .pub file. I try to use Ed25519 pubkeys, so I don't even have an id_rsa.pub, feels like I don't want that to be hard-coded. I'm not too worried about getting confirmation on the sending side (you aren't giving up any authority by revealing your pubkey), so if there's only one pubkey in the ~/.ssh directory, I'd say send it without confirmation. Not sure whether to have a "select number from list" picker if there's multiple keys, or require a posarg (with the full path to the .pub file) when there are mutiple ones. For sure a bare wormhole send-ssh should Just Work when there's only one pubkey.
• the utility function.. should that include a hook to signal an ACK from the far side? making it a send-and-receive utility instead of an unacked send? I don't know if we can use this utility for wormhole send --text, but if so, we'd need an ACK there. (I think wormhole send [file] is going to need much more than this utility, as it grows Transit negotiation and stuff).

[meejah]

As per #32, a name like wormhole ssh copy-id was suggested (to be moar-similar to "ssh copy-id")

[meejah]

--verify is already a top-level option -- should -0 be as well? Should that be re-named something more obviously bad, like --insecure-no-code or something?

[meejah]

I agree about root stuff. How about: if you appear to be root then:

• never copy/install keys (or maybe install a key, but only if --allow-root-access or something?)
• ...unless a --user= option being specified

(Basically I guess that is just "if you're root, you have to specify --user= and it can't be root). Or, just bail out right away with "don't run this as root; switch to the target user first".

[meejah]

For .pub handing, what about:

• if there's only one .pub key (and we're not root) send it
• if there's more than on .pub key, tell the user to use --auth-file to specify the file to use
• if there are zero .pub keys (or no ~/.ssh) tell the user to read about ssh-keygen and try later.

[warner]

• should we consider "invite"/"accept" as the verb pair? something which speaks to the power being granted rather than the (relatively less important) direction of data transfer?
• I moved --verify and -0 to be subcommand arguments already (see CommonArgs in src/wormhole/cli/cli.py), along with -c=/--code-length=, --tor, --hide-progress (although that's file-transfer -specific), and --no-listen (which is both file-transfer -specific and is a debug argument that I'd like to hide from the --help output).
• I kinda like the "YOLO" brevity implied by -0, but I guess it couldn't hurt to add a long-form name that's more.. discouraging, so if someone is demonstrating it in a slide deck or something, they can point out what it really does. Maybe -0/--unauthenticated? Or -0/--yolo? The help text must explain what is really does, of course.

Re --allow-root-access and --user= .. I guess the question is: what is the majority use case going to be? If we think that "root adds a new account" is the majority, then a mandatory user= positional argument seems promising, so the recommended setup recipe can be:

# adduser --disabled-password warner
# wormhole ssh invite warner
 -> CODE

And if you're logging into your own account and want to add a new key (maybe you used a password the first time, or maybe you logged in from one host and want to add the pubkey of a second host), you do the same:

% wormhole ssh invite warner
 -> CODE

If we think the add-key-to-my-own-account is more likely, then we'd not require the posarg, default to modifying your own .ssh/authorized_keys, add a way to point at the file or the user, and add extra protection against root modifying root's keys.

Re pubkeys: yeah, that plan sounds fine. The only thing I'd add would be to print out the list of discovered pubkeys, if there's more than one. And I think I'd be ok with leaving out a special check to avoid sending root's pubkey (doesn't seem dangerous to me). I might call it -p=/--pub-key= instead of authkey, although I could be swayed otherwise ("pub" matches the source file, "auth" matches the dest file, "key" matches the "ssh-keygen" tool that creates them).

For extra credit we could make it interactive (print a list, assign a number to each, ask the user for the number), but maybe that's too much work.

Unlike text/file sending, the sender doesn't need to confirm the transfer. But the receiver does (at least if --verify is enabled), since it's the receiver who's granting authority by accepting the transfer. Not sure how that might affect the common send/receive utilities.. there might need to be a mode flag that says which direction the verification needs to travel.

[meejah]

Okay, I force-pushed after rebasing and doing the things in #57 (comment)

[meejah]

okay, i made some of the cleanups in the last couple messages from @warner in the last commit on this branch. I moved to "wormhole ssh accept/invite" sub-sub commands, --user, changed method-names, more permissions checks, ask user which key (if there's multiple keys), ...

[codecov-io]

Current coverage is 86.57% (diff: 46.15%)

Merging #57 into master will decrease coverage by 0.36%

@@             master        #57   diff @@
==========================================
  Files            18         18          
  Lines          2694       2718    +24   
  Methods           0          0          
  Messages          0          0          
  Branches        379        380     +1   
==========================================
+ Hits           2342       2353    +11   
- Misses          257        270    +13   
  Partials         95         95          

Powered by Codecov. Last update afa123a...fe29b31

[warner]

Looks good: I made a few tweaks, and I have a list of additional fixes to make later, but I'm going to land this now. Thanks!