Bugfix: ssh port override for #1116

[memetb]

This issue fixes #1116.

It is a direct fix that maintains backward compatibility, however it introduces an outcome that breaks the principle of least astonishment: the port specified in the query string will take precedence over the port specified in any configured targets along the way (e.g. client -> bastion1 -> bastion2 -> host).

My opinion is that config should have priority over query string, however, this behaviour is not backwards compatible.

@dmacvicar : will need your input on this.

[memetb]

@mhtr fyi

[scabala]

I think that it is not breaking principle of least astonishment because, IIRC specifying port on commandline takes precedence than port specification in config file. I do not know, however, how does it work with libvirt's uri string.

[memetb]

@scabala can you reproduce this?

From man page. It does contradict itself a little bit... It is a bit bureaucratically worded...

ssh(1) obtains configuration data from the following sources in
     the following order:

           1.   command-line options
           2.   user's configuration file (~/.ssh/config)
           3.   system-wide configuration file (/etc/ssh/ssh_config)

     Unless noted otherwise, for each parameter, the first obtained           <--- "universal" expectation
     value will be used.  The configuration files contain sections
     separated by Host specifications, and that section is only
     applied for hosts that match one of the patterns given in the
     specification.  The matched host name is usually the one given on
     the command line (see the CanonicalizeHostname option for
     exceptions).

     Since the first obtained value for each parameter is used, more
     host-specific declarations should be given near the beginning of
     the file, and general defaults at the end.

     The file contains keyword-argument pairs, one per line.  Lines
     starting with ‘#’ and empty lines are interpreted as comments.
     Arguments may optionally be enclosed in double quotes (") in
     order to represent arguments containing spaces.  Configuration
     options may be separated by whitespace or optional whitespace and
     exactly one ‘=’; the latter format is useful to avoid the need to
     quote whitespace when specifying configuration options using the
     ssh, scp, and sftp -o option.

But contrast with:

ProxyJump
     Specifies one or more jump proxies as either
     [user@]host[:port] or an ssh URI.  Multiple proxies may
     be separated by comma characters and will be visited
     sequentially.  Setting this option will cause ssh(1) to
     connect to the target host by first making a ssh(1)
     connection to the specified ProxyJump host and then
     establishing a TCP forwarding to the ultimate target from
     there.  Setting the host to none disables this option
     entirely.

     Note that this option will compete with the ProxyCommand
     option - whichever is specified first will prevent later
     instances of the other from taking effect.

     Note also that the configuration for the destination host
     (either supplied via the command-line or the
     configuration file) is not generally applied to jump        <------------- note of the "otherwise"
     hosts.  ~/.ssh/config should be used if specific
     configuration is required for jump hosts.

I also did a quick test on my local machine and it behaves as I expect:

memet@machine:~$ head ~/.ssh/config | tail -n+2
UseRoaming no

Host fake-jump
        Port 2222
        HostName 192.168.50.165

Host bogey
        Port 2222
        HostName localhost
        ProxyJump fake-jump
memet@machine:~$ ssh fake-jump
ssh: connect to host 192.168.50.165 port 2222: Connection refused
memet@machine:~$ ssh fake-jump -p 22 
# success
memet@machine:~$
logout
Connection to 192.168.50.165 closed.
memet@machine:~$ ssh bogey
ssh: connect to host 192.168.50.165 port 2222: Connection refused
kex_exchange_identification: Connection closed by remote host
Connection closed by UNKNOWN port 65535
memet@machine:~$ ssh bogey -p 22
ssh: connect to host 192.168.50.165 port 2222: Connection refused
kex_exchange_identification: Connection closed by remote host
Connection closed by UNKNOWN port 65535

[scabala]

I was writing from memory but what from example you provided it looks like it works as I wrote - commandline has priority over config.
When connecting to fake-jump, port from config is 2222 and it does not work, only when overridden by port 22 from commandline.

Second example does not work because IMO it first tries to port 2222 on fake-jump which IMO is fine - I do not know of any way to override this from commandline.

Now when I read your description in PR it is more clear to me what you meant. I must say I was lost a bit - this SSH handling part grew significantly. Yeah, I guess that might indeed be a little surprising to user but I think this is can be dealt with some little documentation.

[memetb]

Please see TL;DR at end. The answer is trivial: we honor sshconfig manpage rules.

it looks like it works as I wrote - commandline has priority over config.

I'm not sure about that, I've re-done the same output for more explicit clarity. You can try the below stanzas locally too...

memet@machine:~$ head -n 16 ~/.ssh/config | tail -n+2
UseRoaming no

Host fake-jump
        Port 2222
        HostName 192.168.50.165 # this is my local DHCP address

Host bogey-1
        Port 2222
        HostName localhost
        ProxyJump fake-jump

Host bogey-2
        Port 2222
        HostName localhost

memet@machine:~$ ssh bogey-2    # FAILS WITHOUT OVERRIDE
ssh: connect to host localhost port 2222: Connection refused
memet@machine:~$ ssh bogey-2 -p 22  # SUCCEEDS WITH FLAG
Linux machine 6.1.0-22-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.1.94-1 (2024-06-21) x86_64
# SUCCESS ...
memet@machine:~$ ssh fake-jump # THIS FAILS WITHOUT OVERRIDE
ssh: connect to host 192.168.50.165 port 2222: Connection refused
memet@machine:~$ ssh fake-jump -p 22 # THIS SUCCEEDS WITH FLAG
Linux machine 6.1.0-22-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.1.94-1 (2024-06-21) x86_64
# SUCCESS ...
memet@machine:~$
logout
Connection to 192.168.50.165 closed.
memet@machine:~$ ssh bogey-1 # THIS FAILS WITHOUT OVERRIDE
ssh: connect to host 192.168.50.165 port 2222: Connection refused
kex_exchange_identification: Connection closed by remote host
Connection closed by UNKNOWN port 65535
memet@machine:~$ ssh bogey-1 -p 22 # THIS STILL FAILS WITH FLAG 
ssh: connect to host 192.168.50.165 port 2222: Connection refused 
kex_exchange_identification: Connection closed by remote host
Connection closed by UNKNOWN port 65535
memet@machine:~$ ssh bogey-1 -p 22 -v
OpenSSH_9.2p1 Debian-2+deb12u2, OpenSSL 3.0.13 30 Jan 2024
debug1: Reading configuration data /home/memet/.ssh/config
debug1: /home/memet/.ssh/config line 2: Deprecated option "useroaming"
debug1: /home/memet/.ssh/config line 8: Applying options for bogey-1
debug1: Reading configuration data /etc/ssh/ssh_config
debug1: /etc/ssh/ssh_config line 19: Applying options for *
debug1: Setting implicit ProxyCommand from ProxyJump: ssh -v -W '[%h]:%p' fake-jump
debug1: Executing proxy command: exec ssh -v -W '[localhost]:22' fake-jump
debug1: identity file /home/memet/.ssh/****** type 0
debug1: identity file /home/memet/.ssh/******-cert type -1
debug1: Local version string SSH-2.0-OpenSSH_9.2p1 Debian-2+deb12u2
OpenSSH_9.2p1 Debian-2+deb12u2, OpenSSL 3.0.13 30 Jan 2024
debug1: Reading configuration data /home/memet/.ssh/config
debug1: /home/memet/.ssh/config line 2: Deprecated option "useroaming"
debug1: /home/memet/.ssh/config line 4: Applying options for fake-jump
debug1: Reading configuration data /etc/ssh/ssh_config
debug1: /etc/ssh/ssh_config line 19: Applying options for *
debug1: Connecting to 192.168.50.165 [192.168.50.165] port 2222.
debug1: connect to address 192.168.50.165 port 2222: Connection refused
ssh: connect to host 192.168.50.165 port 2222: Connection refused
kex_exchange_identification: Connection closed by remote host
Connection closed by UNKNOWN port 65535

I can connect to bogey-2 via -p override, I can connect to fake-jump via -p override, but when I do -p override on bogey-1, it does NOT override the port on fake-jump. You can see the connection fails on the first hop.

Second example does not work because IMO it first tries to port 2222 on fake-jump which IMO is fine - I do not know of any way to override this from commandline.

Yes, this is what the man page says will happen as the exception, not the rule. Specifically for JumpHost.

Now when I read your description in PR it is more clear to me what you meant. I must say I was lost a bit - this SSH handling part grew significantly.

It didn't "grow" so much as it moved beyond trivial handling. The last release had an outdated sshconfig package (v0.0.0) and it only used the "User" parameter.

Besides, without sshconfig, bastion hosts were not possible and this is a big no-go for my (any?) corporate environment.

All we need to finalize is how to be backward compatible while maintaining good hygiene moving forward...

TL;DR I just realized that there is no backward compatibility issue at all. v0.7.6 simply did not support proxy jumps, therefore nobody has production code with proxy jumps in them that can break due to this change.

[mhtr]

@memetb hi
Thanks for your fix, it worked for me. But now I have another issue 1121

[dmacvicar]

There is no need to be backward compatibility and go with the behavior closer to ssh/libvirt.

We are not in 1.0 yet. Lets just mention it in the release notes.

[memetb]

Please disregard this comment: there is a bug, but not where I thought.

As per net/url documentation here

Host        [string](https://pkg.go.dev/builtin#string)    // host or host:port (see Hostname and Port methods)

the Host member can be either host or host:port, and therefore we should not make use of .Host but instead rely on .Hostname() when getting the hostname.

---

@dmacvicar I've found a bug here. It's subtle but it was affecting this PR. I will not fix this bug in this PR and instead fix it in its own issue and associated PR.

The bug is here:

// The name passed to the remote virConnectOpen function is formed **by removing
// transport, hostname, port number, username and extra parameters** from the remote URI
// unless the name option is specified.

It appears that when a port name is specified with the virsh connection string format, the URL.Parse incorrectly puts "hostname:port" as the name of the server. As a result, this fails in all sorts of ways because the target now becomes "servername:port" instead of "servername".

As I said, I will open this as a new issue and associated pr.