This is a patch to automatically handle port forwarding:
xpra client -> ssh server 1 -> xpra server (ssh)
With this patch:
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:
Possible enhancements
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:
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:
- if not has_file:
+ # without this the return code is always None in WSL (and becomes zero after communicate()) + proc.wait()
+ transport.use_compression(False)
(I thought compression was disabled by default?)
auth_interactive - not sure what I am looking at
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.
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.
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.
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.
I also found a bug in the _pass dialog code.
Please submit bug fixes separately so these can be fast-tracked.
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.
Comments - not all of those are essential for merging:
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)
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)
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)
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&...
if not has_file:?
phost, pport, etc. please rename to proxy_host, proxy_port, etc
ipv6 flag is set from parsing the host value, not the proxy_host.. could that cause problems?
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:
Okay. I am working on these changes.
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.
- 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.
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.
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.
+ 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
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)
xpra.scripts.parsing.parse_URL then parse the regexp on the "proxy" option it returns
full_ssh = ssh[:] or full_ssh = list(ssh))
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.
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.getuserdoes not take any arguments, and even if it did we cannot call it unconditionally as there may not be a tty attached (we useos_util.use_tty()for cases like that)
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.
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.getuserdoes 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 likedialog_passto handle both tty and gui modes.
The WSL issue is being tracked in #2059
I found at least one bug. I am presently correcting. Will write manual page ASAP. Made other changes as requested.
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:
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:
sshpass_command discarded assignment
parse_proxy_attributes
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..)
get_username from xpra.platform instead of getpass (should end up the same)
add_ssh_proxy_args a bit
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
do_ssh_paramiko_connect_to
updated patch
Thanks! I'll start testing!
Missed from the updated patch above:
is_WSL import should be preserved
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)
Tested in windows and WSL (which is pretty similar to Linux). Everything is right with this patch:
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.
splits the connect function and the channel opening
Merged in r21188 with the following minor changes:
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:
This simplifies the code quite a bit.
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.
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.
I think this works, let's close it and create new tickets for other features.
ie: #2105 - tcp socks proxy support
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?
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.
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?
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.
I did not know about this. Thank you for fixing it!
this ticket has been moved to: https://github.com/Xpra-org/xpra/issues/2041