Xpra: Ticket #2041: 2hop ssh patch

This is a patch to automatically handle port forwarding:

xpra client -> ssh server 1 -> xpra server (ssh)

With this patch:

  1. a client can connect to an XPRA server through an ssh server in the middle.
  1. paramiko supports a "keyboard interactive" mode to deal servers requiring several prompts for things like, say a RSA SecureID value.
  1. I've fixed a small bug on the "_dialog" code for paramiko.

I do need to test it more thoroughly. I've not tried Apple at all yet, nor have I tried paramiko with a Tectia server. I will do that once I hear back regarding whether or not this can be included.

I hope it can be included. Without this patch in the mainline XPRA will be harder for me to deploy on my servers. I'm willing to put in effort to bring it up to your standards whatever those may be.

If it can be included in the main XPRA source code I am willing to:

  1. maintain it for the foreseeable future
  2. change it to bring it up to your standards
  3. enhance it as required.

Possible enhancements

  1. OpenSSH client support. Right now I've only done Putty and Paramiko. However, since Paramiko seems works fine in windows, I suspect that, ultimately, it will be enabled by default, in which case Paramiko might be enough.
  1. Right now the "key" field only works for the proxy server and it only works with putty (no Paramiko). The Paramiko code will find "id_rsa" and "id_dsa" normally, so it was less pressing. I can add a field for XPRA server key and enable it for Paramiko.
  1. It might make sense to combine my "SSH -> SSH" mode with "SSH". This could be done pretty easily with a checkbox enabling/disabling the proxy. I imagine then that SSH -> SSH would become "SSH" and the proxy code would be check-boxed "off" by default.
  1. I've not added "proxying" support to the command line tool. I didn't bother because none of my users will be doing command line (I suspect). I can add that too, if requested.


Thu, 15 Nov 2018 21:10:56 GMT - Nathan Hallquist: attachment set


Fri, 16 Nov 2018 17:36:53 GMT - Nathan Hallquist:

I've found a few bugs related to test code I put in but never took out. I'll be update it shortly.

Replying to Nathan Hallquist:

This is a patch to automatically handle port forwarding:


Fri, 16 Nov 2018 18:37:24 GMT - Antoine Martin: owner changed

I've skimmed the patch briefly. It looks pretty good considering how awful the launcher code is to begin with!

This new mode should probably be supported from the command line, just like all the other modes, something like:

xpra attach ssh+ssh://host:port/?phost=...&..

Sadly, some of the parsing code is already duplicated between main and the launcher...

There are one or two things I'm unsure about:

(I thought compression was disabled by default?)

Sounds like this should be changed. RFCs don't change, implementations do.

I'll look again at your updated patch when I find the time.


Tue, 20 Nov 2018 18:09:29 GMT - Nathan Hallquist:

I just did an update and see the ssh.py got a new SSHProxyCommandConnection. Is this something that replaces what I'm trying to do, or is it something in addition? From what I can tell it executes an external "command" whereas I'm using Paramiko to forward Paramiko.

I'm not sure I understand everything, but I do not think it is equivalent to what I did.

I am planning to debug what I did today. I'll try to merge what I've done back in.

Anyway I'll try to follow the coding style in SSHProxyCommandConnect (and the other code) more closely. I'm very new to python.


Wed, 21 Nov 2018 04:51:13 GMT - Antoine Martin:

The changes to ssh come from r21034 (#1937).

It loads the ~/.ssh/config and executes the proxy command that may be specified there. The command is wrapped in a channel object.

It would be nice to re-use the same code, there is no reason why we can't daisy chain paramiko channels.


Wed, 21 Nov 2018 05:15:20 GMT - Nathan Hallquist:

I've successfully merged my patch back in according to the style of the new patch (as I understand it).

I also found a bug in the _pass dialog code. The "confirm" button was not "activating" the text entry field causing that "_pass" dialog to not send the password to stdout. The fix will be included in my patch when I post it in a day or two.

Before I post the patch I think it is vital that I add some additional error handling to my code.


Wed, 21 Nov 2018 05:47:12 GMT - Antoine Martin:

I also found a bug in the _pass dialog code.

Please submit bug fixes separately so these can be fast-tracked.


Wed, 21 Nov 2018 14:45:07 GMT - Antoine Martin: component changed


Thu, 22 Nov 2018 21:46:27 GMT - Nathan Hallquist:

Here's the next version.

I've separated out the other bug fixes as you requested.

It is working nicely for me. I hope it works for you. I will make any changes you request.

In a future iteration intend to enable the key field on paramiko and add a key field to the final destination host. I imagine this is useful for screen sharing (owner of the session adds the guests public key to an authorized file), but I've not tried screen sharing yet in XPRA so I'm not sure how that works.

Tortoise plink won't work except with the most recent versions of tortoise plink because "-nc" is pretty new.

patch


Thu, 22 Nov 2018 21:46:59 GMT - Nathan Hallquist: attachment set


Fri, 23 Nov 2018 09:57:55 GMT - Antoine Martin:

Comments - not all of those are essential for merging:

Minor changes merged:


Sat, 24 Nov 2018 04:30:21 GMT - Nathan Hallquist:

Okay. I am working on these changes.


Tue, 27 Nov 2018 04:24:43 GMT - Nathan Hallquist:

Still working on this. I've got openssh, plink, and paramiko all working. Will do some testing tomorrow.

One question about passing in the password on the command line for plink: in UNIX the command line is exposed to all other users typically; is it different in windows? It seems to be different, but I cannot find a definitive answer.


Tue, 27 Nov 2018 04:46:53 GMT - Nathan Hallquist:

I did this early on. When you asked why, I had forgotten why. I couldn't even tell whether it was a mistake or not.

After looking at it, I have determined that I probably meant to also get rid of the line after that, namely, the "reset_errors()". I was intended to have XPRA highlight the required fields at startup (and not surprise me later).

I didn't intended to submit that change with the patch.

I can undo the change, or fix the change by removing the "reset_errors()" line too (and submitting it as a separate ticket). Whichever you want.


Tue, 27 Nov 2018 04:52:56 GMT - Antoine Martin:

I can undo the change, or fix the change by removing the "reset_errors()" line too (and submitting it as a separate ticket). Whichever you want.

I don't understand the reasoning for removing that line and the one preceding it.

I believe that the idea here is to only hide the errors when the form is blank, and not when it is populated by a session file. That seems reasonable, I think. It gives users a chance to populate the form before behind told that there are mistakes in it - similar to what you would get on most web forms.


Thu, 29 Nov 2018 03:34:44 GMT - Nathan Hallquist:

Here's the next iteration of the patch.

I think I've done everything in the list except for splitting do_ssh_paramiko_connect_to().

If splitting is a condition for having the patch accepted, then I'll do it.

At this stage, I've tested plink and paramiko pretty thoroughly in windows with different username/password/port inputs (and non-inputs).

Tomorrow I will test openssh and paramiko on linux launcher.

I will also carefully test the command line on both linux and windows with different username/password/port combinations.

For the command line I've decided to adopt

?proxy=ssh://user:password@host:port

My command line parser is based on a regex rule that checks for two things (1) the presence of ?proxy=... at the end and (2) ?proxy= having the right format. If (1) is there with an unrecognizd wrong format in (2) XPRA prints an error and just hands the description to the rest of the parser. If (2) matches XPRA deletes the proxy portion from the description and passes it to the rest of the XPRA parser.

I am imagining down the line that it might be interesting to use ?proxy=ssh:// on, for instance, a tcp:// connection.


Thu, 29 Nov 2018 03:36:50 GMT - Nathan Hallquist: attachment set


Thu, 29 Nov 2018 06:11:26 GMT - Antoine Martin:

Because of how python evaluates it:

>>> True or True and not True
True

Just a thought, in the future we may want to let the user choose the ssh backend (paramiko, openssh, plink) - hard to do since the whole command line should be editable (at least for openssh and plink), but that's for another ticket.


Thu, 29 Nov 2018 09:01:46 GMT - Nathan Hallquist:

Thanks very much for the feedback!

I hadn't meant to upload the getuser thing yet, as I hadn't tested it at all. (Big Oops!). My apologies. Now I am wondering about how to do this right.

In p2 I have already altered the launcher so that it won't connect without a user. I had, therefore, intended to only interactively get a user when on command line mode, which I presume is always TTY.

In UNIX I am pretty sure, then, that there will be a TTY. However, I don't really understand how MINGW Python works with command line in windows, so I've been just trying things and sticking with whatever works. My plan (when I wrote the getuser line) had been to just guess at the code (my first guess was the getuser); try to run it, which I hadn't done yet; and then see what happens when I test the command line tomorrow, which is how I missed this flaw.

Maybe I should alter the command line to require a username like I've done in the launcher? If I do that, though I may break some script somewhere, so I am hesitant to do a thing like that.

What is the best way, then, to gather interactively gather a username on TTY? Alternatively, should I alter the command line parser to fail unless the usernames is set there?


Thu, 29 Nov 2018 09:12:42 GMT - Antoine Martin:

The username should not be mandatory: we can fallback to the current local username when it isn't specified, that's what ssh does by default. getpass.getuser does not use the tty or ask the user anything, it just looks it up (env vars). If you really want to ask the user to specify one, you should probably use something like dialog_pass to handle both tty and gui modes.


Thu, 29 Nov 2018 17:49:18 GMT - Nathan Hallquist:

I think I badly misread the getuser documentation (I want to blame the late hour of my last reply). I really regret that you had to reply to my inanity. I'm embarrassed.

What you say is absolutely the right thing on the command line interface. The username should be assumed to be the client username.

In the UI what I've done is mostly similar. Even though I require that the field be filled in, the the field *starts out* filled in with the output from xpra.platform.info.get_username() (so the user would have to decided to clear it to get a blank field). I think that this behavior is almost equivalent to the command line behavior that I just mentioned.

The slight difference is (in my opinion) desirable: On windows my username doesn't match my username on the server that I am logging into. My windows username has a space in it, and I think that that is pretty typical. When, due to a bad username, my attempt to login fails my IP gets blocked (my server runs OSSEC). I would bet that lots of people have similarly configured server and would appreciate having the UI thusly steer them away from such mistakes. I think as I've done things the UI protects the user by *displaying* the default value while providing the benefits of sensible defaults.

If you want me to allow for the launcher entry fields to be blank, I *will* change it to do whatever you prefer. Just say the word!

Thanks again for all your help!

Replying to Antoine Martin:

The username should not be mandatory: we can fallback to the current local username when it isn't specified, that's what ssh does by default. getpass.getuser does not use the tty or ask the user anything, it just looks it up (env vars). If you really want to ask the user to specify one, you should probably use something like dialog_pass to handle both tty and gui modes.


Fri, 30 Nov 2018 10:41:50 GMT - Antoine Martin:

The WSL issue is being tracked in #2059


Sat, 01 Dec 2018 04:14:41 GMT - Nathan Hallquist:

I found at least one bug. I am presently correcting. Will write manual page ASAP. Made other changes as requested.


Tue, 04 Dec 2018 00:19:31 GMT - Nathan Hallquist:

I believe this latest iteration of my patch, p3 does what you've requested. If I have forgotten to do something it was not intentional. I've not tried to update the wiki yet, but I've updated the manual page in the patch.

There are some outstanding issues. However, I think they all existed prior to my work. I hope that XPRA can accept my patch first and then let me address these outstanding issues later. They are:

  1. Currently the UI accepts a key only for plink. I omitted Paramiko and OpenSSH support because, unlike plink, they automatically search ~/.ssh like a reasonable naive user would expect them to. In the future I intend to add a file selector to choose a key for the XPRA server just like I have already for the proxy server. When I get around to this I will add support for Paramiko and OpenSSH key selection as well. I want for users to be able to do screen sharing using public keys. At LSTC, for instance, users would come into the SSH relay machine using their own account and then hop to the XPRA server using a public key.
  1. Sshpass doesn't work right with multiple "keyboard-interactive" prompts. This may be hopeless. Plink, OpenSSH with askpass, and Paramiko are all fine.
  1. Sshpass freaks out if the host key is unknown.
  1. The launcher should always set XPRA_NOTTY=1. I think many users will not expect the password prompts to show up in the spawning window.
  1. When doing a proxy from the launcher with plink, plink's password prompt pops up to the front as expected for the proxy server authentication but it doesn't reliably get the focus on the destination prompt. In fact, it often gets buried.
  1. The Paramiko client doesn't do the "AUTH_NONE" first to find out the supported methods. On my servers this can cause some problems. Some servers only support publickey auth. I would like for Paramiko to detect that and bail if there is no key. Right now it will happily ask for passwords even though no password mechanisms are supported by the server. Similarly, Paramiko will try and *retry* password auth even if password auth is not supported. I intend to fix this one ASAP.
  1. When I've time I'd like to write up a "howto" document for environments like ours. I'm running XPRA servers from systemd and have a locked-down single-point of entry. I really like this configuration a lot and I think others might like it too.

Tue, 04 Dec 2018 00:19:47 GMT - Nathan Hallquist: attachment set


Tue, 04 Dec 2018 21:00:18 GMT - Antoine Martin:

It was easier and clearer for me to make the changes rather than to go through another round of review, please see the updated patch - untested, does that work for you? Changes:

Other changes still needed / useful:


Tue, 04 Dec 2018 21:01:14 GMT - Antoine Martin: attachment set

updated patch


Tue, 04 Dec 2018 21:05:07 GMT - Nathan Hallquist:

Thanks! I'll start testing!


Wed, 05 Dec 2018 05:22:29 GMT - Antoine Martin:

Missed from the updated patch above:


Fri, 07 Dec 2018 04:12:35 GMT - Nathan Hallquist:

Been busy last few days. I will get this done tomorrow. (Just letting you know I've not disappeared; I'm very excited about getting this done)


Fri, 07 Dec 2018 23:33:47 GMT - Nathan Hallquist:

Tested in windows and WSL (which is pretty similar to Linux). Everything is right with this patch:

  1. I fixed a bug having to do with sshpass (when it's present)
  1. I fixed some UI bugs having to do with sshpass is missing
  1. I by hiding hboxes instead of entry fields I make the vertical spacing more uniform
  1. My patch fixes the is_WSL problem
  1. My patch makes "ssh" the default mode again (sorry about this!)
  1. fixed a bug in the command line parser having to do with port numbers
  1. fixed a bug having to do with calculation of is_putty and is_paramiko

I am very sorry for not understanding how you wanted the variable names to look; I do not intend to create needless work for you. I went through every line of the diff and I have tried to absorb the changes so that I follow XPRA conventions more closely on future submissions.


Fri, 07 Dec 2018 23:34:47 GMT - Nathan Hallquist: attachment set


Sat, 08 Dec 2018 04:43:52 GMT - Antoine Martin: attachment set

splits the connect function and the channel opening


Sat, 08 Dec 2018 04:45:35 GMT - Antoine Martin:

Merged in r21188 with the following minor changes:


I am very sorry for not understanding how you wanted the variable names to look; I do not intend to create needless work for you.

No problem at all. I am being particularly facetious on this patch submission because this file was already quite hard to read so I am trying to make sure it doesn't get worse!


Please take a look at the untested patch above. This is the approach I would much prefer:

This simplifies the code quite a bit.


Sun, 09 Dec 2018 20:21:22 GMT - Nathan Hallquist:

The split patch works in WSL and in Windows. I think it's fine. Looks like the other patches are in subversion. I'm really happy about that.

I have a lot of enhancement and fixes in mind for this feature. Should I open up new tickets for each one as I start them, or should I just keep adding to this ticket?

First on the list is to clean up sshpass code a little and enhance its error reporting and maybe have it fallback to askpass. From a user's point of view I think the present sshpass code could be pretty confusing.


Sun, 09 Dec 2018 20:27:23 GMT - Antoine Martin:

The split patch works in WSL and in Windows. I think it's fine.

Thanks, applied in r21189.

I have a lot of enhancement and fixes in mind for this feature. Should I open up new tickets for each one as I start them, or should I just keep adding to this ticket?

If the changes are small or just bug fixes for the changes from this ticket then we can keep it here, otherwise let's create new tickets.


Tue, 15 Jan 2019 05:54:17 GMT - Antoine Martin: status changed; resolution set

I think this works, let's close it and create new tickets for other features.

ie: #2105 - tcp socks proxy support


Tue, 15 Jan 2019 16:35:30 GMT - Nathan Hallquist:

I have several enhancements and bug fixes planned for this. I thought that I would add them to this ticket. Should I open up new tickets?

Would you like me to open up tickets now and do them when I have time or is it better to open them when I'm ready to start?


Tue, 15 Jan 2019 17:17:23 GMT - Antoine Martin:

Should I open up new tickets?

Yes please.

Would you like me to open up tickets now and do them when I have time or is it better to open them when I'm ready to start?

Sooner is better, then we can start adding ideas, linking to other tickets, etc.


Sun, 20 Jan 2019 06:48:43 GMT - Antoine Martin:

pylint is complaining:

hostport_match = re.match("(?P<host>[^:]+)($|:(?P<port>\d+)$)", hostport)
PyLint: Anomalous backslash in string: '\d'. String constant might be missing an r prefix.

I think the warning is correct - but how does it work without?


Mon, 21 Jan 2019 14:45:29 GMT - Antoine Martin:

nvm, it's some python automagic with backslash:

$ python3
>>> "(?P<host>[^:]+)($|:(?P<port>\d+)$)"
'(?P<host>[^:]+)($|:(?P<port>\\d+)$)'

r21444 switches to an 'r' string so this is more explicit now.


Mon, 21 Jan 2019 17:59:31 GMT - Nathan Hallquist:

I did not know about this. Thank you for fixing it!


Sat, 23 Jan 2021 05:40:34 GMT - migration script:

this ticket has been moved to: https://github.com/Xpra-org/xpra/issues/2041