2hop ssh patch

[totaam]

Issue migrated from trac ticket # 2041

component: network | priority: major | resolution: fixed

2018-11-15 21:10:17: nathan_lstc created the issue

---

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.

2. paramiko supports a "keyboard interactive" mode to deal servers requiring several prompts for things like, say a RSA SecureID value.

3. 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

a. 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.

b. 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.

c. 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.

d. 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.

[totaam]

2018-11-15 21:10:56: nathan_lstc uploaded file patch_16 (27.7 KiB)

[totaam]

2018-11-16 17:36:53: nathan_lstc commented

---

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 [#2041 Nathan Hallquist]:

This is a patch to automatically handle port forwarding:

[totaam]

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:

• why remove this check?

-        if not has_file:

• looks like this is a bug fix?

+        # without this the return code is always None in WSL (and becomes zero after communicate())
+        proc.wait()

• do we want this by default?

+        transport.use_compression(False)

(I thought compression was disabled by default?)

• auth_interactive - not sure what I am looking at
• per the RFC we probably should do none first always and read off the supported methods, however, the current code seems to work fine with OpenSSH
  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.

[totaam]

2018-11-20 18:09:29: nathan_lstc commented

---

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.

[totaam]

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.

[totaam]

2018-11-21 05:15:20: nathan_lstc commented

---

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.

[totaam]

I also found a bug in the _pass dialog code.

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

[totaam]

2018-11-22 21:46:28: nathan_lstc commented

---

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

[totaam]

2018-11-22 21:46:59: nathan_lstc uploaded file p.txt (27.2 KiB)

[totaam]

Comments - not all of those are essential for merging:

• the names of the attributes are inconsistent: sshproxy_ssh_port vs proxy_username - let's drop the ssh prefix, this could be re-used later for connecting via a TCP or HTTP proxy (ie: proxy_port, proxy_password)
• we should support openssh, either with ProxyCommand or ProxyJump (or both)
• padding_label and spacer_scb stuff should be replaced with a gtk.Alignment (no need to keep a reference to them either)
• we're going to need man page updates and updates to the wiki page: SSH
• try to minimize whitespace changes (found one: strict host key check for SSL and SSH) and don't put more than one empty line in code or between methods (save 2 for between classes or occasionally between methods)
• all connection modes should be supported as plain URL strings with xpra attach URL so the code should be moved to add_ssh_args - not sure what that would look like for ssh via proxy though, maybe something like ssh+ssh://username:password@host:sshport/?proxy_host=hostname&proxy_port=22&...
• why remove if not has_file:?
• phost, pport, etc. please rename to proxy_host, proxy_port, etc
• the ipv6 flag is set from parsing the host value, not the proxy_host.. could that cause problems?
• rather than overloading do_ssh_paramiko_connect_to, maybe split it so we only do authentication and return the transport, then use another method for either running the xpra-proxy-command, or opening the port forward

Minor changes merged:

• r21091 ensure we always select an active mode
• r21092 do key auth first

[totaam]

2018-11-24 04:30:22: nathan_lstc commented

---

Okay. I am working on these changes.

[totaam]

2018-11-27 04:24:43: nathan_lstc commented

---

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.

[totaam]

2018-11-27 04:46:54: nathan_lstc commented

---

• why remove if not has_file:?

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.

[totaam]

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.

[totaam]

2018-11-29 03:34:45: nathan_lstc commented

---

Here's the next iteration of the [/attachment/ticket/2041/p2.txt 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.

[totaam]

2018-11-29 03:36:50: nathan_lstc uploaded file p2.txt (33.1 KiB)

[totaam]

• this needs some parentheses:

+        if ssh or sshtossh and not port:

Because of how python evaluates it:

> True or True and not True
True

• FILE_CHOOSER_ACTION_SAVE looks like the wrong constant since we're looking for an existing file, right? See GTK FileChooser Action Constants
• WSL: is there an environment variable we can use to detect it and only call proc.wait() in this case?
• getpass.getuser does not take any arguments, and even if it did we cannot call it unconditionally as there may not be a tty attached (we use os_util.use_tty() for cases like that)
• I'm not sure about the parsing of the proxy value on the URI: maybe we should do a first pass using xpra.scripts.parsing.parse_URL then parse the regexp on the "proxy" option it returns
• python does lists by reference, full_ssh # ssh was probably a bug - the "ssh" attribute was actually unused, so r21139 removes it. (FYI: to keep "ssh", the pythonic way would have been: full_sshssh[:] or full_ssh = list(ssh))
• and don't forget the man page updates, etc

---

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.

[totaam]

2018-11-29 09:01:46: nathan_lstc commented

---

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?

• getpass.getuser does not take any arguments, and even if it did we cannot call it unconditionally as there may not be a tty attached (we use os_util.use_tty() for cases like that)

[totaam]

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.

[totaam]

2018-11-29 17:49:18: nathan_lstc commented

---

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 [comment:17 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.

[totaam]

The WSL issue is being tracked in #2059

[totaam]

2018-12-01 04:14:42: nathan_lstc commented

---

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

[totaam]

2018-12-04 00:19:31: nathan_lstc commented

---

I believe this latest iteration of my patch, [/attachment/ticket/2041/p3.txt 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.

2. Sshpass doesn't work right with multiple "keyboard-interactive" prompts. This may be hopeless. Plink, OpenSSH with askpass, and Paramiko are all fine.

3. Sshpass freaks out if the host key is unknown.

4. 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.

5. 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.

6. 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.

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

[totaam]

2018-12-04 00:19:47: nathan_lstc uploaded file p3.txt (36.5 KiB)

[totaam]

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:

• minor import style tweak
• minor indentation issues (missing whitespace, extra space, etc)
• logging message cut&pasted wrong: "auth_interactive"
• re-do WSL workaround which was removed by the patch
• sshpass_command discarded assignment
• moved proxy attribute parsing to a function: parse_proxy_attributes
• re-order proxy attributes to make them consistent with non-proxy attributes: host, port, username, password (and found that proxy_ssh_port_entry was assigned twice that way)
• is_putty and is_paramiko need to be updated every time the ssh command is updated, not just initially - not sure I like having those two extra attributes, but anyway... (a backend enum might be better here..)
• used get_username from xpra.platform instead of getpass (should end up the same)
• renamed "proxy_ssh_port" to "proxy_port" for consistency / clearer and so that "port" can be reused for other types of proxy
• rename "proxy_pkey_path" to "proxy_key": we could potentially support inlined base64 encoded keys (like we do for ssl certs) and 'p' is not descriptive
• renamed "_l" to "_label" for consistency
• used "%s" string formatting rather than string concatenation
• tidy up add_ssh_proxy_args a bit
• make error messages more helpful: show what part of the string parsing failed

Other changes still needed / useful:

• proxy_pkey_path_browse_clicked we should support openssh and paramiko proxy ssh keys
• nostrict_host_check should only be shown for openssh mode
• constify modes?
• split do_ssh_paramiko_connect_to

[totaam]

2018-12-04 21:01:15: antoine uploaded file ssh-2hop-v4.patch (35.4 KiB)

updated patch

[totaam]

2018-12-04 21:05:08: nathan_lstc commented

---

Thanks! I'll start testing!

[totaam]

Missed from the updated patch above:

• should not change the launcher default mode
• is_WSL import should be preserved

[totaam]

2018-12-07 04:12:35: nathan_lstc commented

---

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)

[totaam]

2018-12-07 23:33:47: nathan_lstc commented

---

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)

2. I fixed some UI bugs having to do with sshpass is missing

3. I by hiding hboxes instead of entry fields I make the vertical spacing more uniform

4. My patch fixes the is_WSL problem

5. My patch makes "ssh" the default mode again (sorry about this!)

6. fixed a bug in the command line parser having to do with port numbers

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

[totaam]

2018-12-07 23:34:48: nathan_lstc uploaded file ssh-2hop-v4.1.patch (6.9 KiB)

[totaam]

2018-12-08 04:43:52: antoine uploaded file ssh-2hop-splitconnect.patch (4.4 KiB)

splits the connect function and the channel opening

[totaam]

2018-12-08 04:45:35: antoine commented

---

Merged in r21188 with the following minor changes:

• use lowercase for 'xpra' as per the rest of the man page
• leave default launcher mode as 'tcp', not 'ssh' (My patch makes "ssh" the default mode again (sorry about this!))
• validate(): get the mode once and re-use it: mode = self.mode_combo.get_active_text().lower()

---

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:

• one function only deals with creating the connection and authenticating
• another function deals with running the remote-xpra command
  This simplifies the code quite a bit.

[totaam]

2018-12-09 20:21:23: nathan_lstc commented

---

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.

[totaam]

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.

[totaam]

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

ie: #2105 - tcp socks proxy support

[totaam]

2019-01-15 16:35:31: nathan_lstc commented

---

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?

[totaam]

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.

[totaam]

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?

[totaam]

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.

[totaam]

2019-01-21 17:59:32: nathan_lstc commented

---

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

[totaam]

Note: the custom parsing code using regular expression has been replaced with the standard urllib.parse: 82b0698 - more in #3599.