Jump channel

[JoostJM]

Implements the option of connecting to a SSH server through another SSH server (jump-host).
This is achieved through a new ProxyConnector class: SshConnector.

In addition, refactor the structure of ConnectionInfo objects, improving the configuration of proxy connections.
Proxy connections are now defined using ConnectionInfo objects, which are set to the ProxyConnection property of a ConnectionInfo object. This allows for specification of multiple proxies through which to connect.

As SSH requires different configuration compared to other proxy types, create new sub-interfaces of the IConnectionInfo interface.

Update tests to reflect new interface.

[JoostJM]

Related to #852.

[JoostJM]

A number of tests appear to be failing due to issues with the 8122 port used in tests. It looks like that port is already being used in AppVeyor. Commit 8f0da78 seems to fix this for most of the Connection tests, but the error occurs more often throughout the other tests.

[JoostJM]

Fixed failing tests. All changes in testing were due to the following reasons:

• Many of the tests used port 8122 to simulate connections, either by setting up a listener or expecting it to fail due to no listener on that port. However, port 8122 is already bound on appveyor, which causes either the listener to be unable to bind the socket, or producing an unexpected result in other tests, as the socket can be connected to.
  • This error is fixed by either using dynamic port assignment, or using port 8121.
• Timeout test set the timeout to a random small number. However, when testing proxy timeout, this should also be set on the ProxyConnectionInfo, which is a new object defined in this PR to allow connecting through multiple proxies.
  • This error is fixed by setting the timeout of proxyconnections to the same small value assigned to the connection info objects.
• The new interface requires a service factory object to be passed to the constructor of connector objects, allowing these objects to instantiate proxyconnector objects. In testing, these objects are replaced by Mock<> objects.
  • This error is fixed by implementing Mock<> objects for ServiceFactory where required.

[jophippe]

Hi, I'm testing this PR to merge it into the Visual Studio fork of SSH.NET, and I'm running into a hanging issue. I'm using this code:

var connectionInfo = new ConnectionInfo(Destination, 22, "root", ProxyTypes.Ssh,
    new ConnectionInfo(Proxy, "admin", new PasswordAuthenticationMethod("admin", Password)),
    new PasswordAuthenticationMethod("root", Password));

using (var client = new SftpClient(connectionInfo))
{
    client.Connect();
    var dirs = client.ListDirectory("/");
    foreach (var dir in dirs)
    {
        Console.WriteLine(dir.FullName);
    }
    client.Disconnect();
}

and the program hangs on the client.Connect() line. When I debug, it looks like the Session creates an SshConnector, then the SshConnector creates a Session, then that Session creates an SshConnector, and that SshConnector creates a Session, and so on until the AuthenticationConnection check causes the program to hang. Is there something I'm doing wrong?

[JoostJM]

@jophippe, My apologies. The updates I implemented to make it fit better with the more recent changes to SSH net introduced a bug. Before, I instantiated the connector by also passing the proxy connection info to the constructor (and used it there to instantiate the proxy session required for the jump). In the update, I moved that part to the Connect function, but forgot to use the ProxyConnectionInfo, using the ConnectionInfo instead (pointing to the ultimate destination). This caused an infinite recursion, as each instantiation of the Session tries to make a new SSH Connector. I fixed the error now.

[benrobot]

Any progress on this pull request? I would like to start using this feature.

[JoostJM]

This PR is/was ready for merging, but pending a review by @drieseng. I use it myself by checking out this branch. All checks passed.

[WojciechNagorski]

@JoostJM Amazing job! As soon as we release the .NET Standard version I'll want to test this PR. However, the branch needs to merge with the master because there are many conflicts.

[WojciechNagorski]

I will review this PR. This feature would be great for next version. But you should merge this PR to develop branch, not the master. The best option for you will be to create a new PR from the develop branch.

[JoostJM]

I'll merge with the develop branch as well. First I still need to finish up reviewing the merges of the tests (source code merge with master is done).

[JoostJM]

@WojciechNagorski, the merge with develop is complete. Besides the new features, this also contains some adjustments to the tests, removing the use of port 8122 as much as possible (using dynamic assignment of ports), and using 8121 elsewhere.
I implemented this as in the past, I ran into issues with tests in appveyor (it was already using port 8122 for something else).

[Rob-Hague]

Thanks for working on this @JoostJM

This branch is currently 140 files and 1600+ lines changed, with breaking changes on top. It is hard to really tell but my impression is that many of the changes are unrelated to this feature. I think the review process is going to be much easier for everyone if this PR is made as small as possible, so that it just contains changes which are necessary. For example, an improvement like the dynamic port assignment can go into a separate PR; a change like the port 8122 is not something we experience in appveyor right now, so is it related to this PR or is it now unnecessary?

Doing it this way will mean a reviewer does not have to find such a large block of free time to dedicate to the review, which is honestly hard to be motivated for.

I appreciate the branch could be a small tree at this age, so you may not be motivated yourself to do that. And maybe @WojciechNagorski has some different thoughts, that's just my 2c

[Rob-Hague]

What's going on here? It's a bit unnerving

[JoostJM]

Following the structure in SSH.NET, it would currently be OK to specify ProxyTypes.Ssh, but provinding a ProxyConnectionInfo instance (intended for HTTP and SOCKS proxy connections) as the proxyConnection parameter in the ConnectionInfo constructor. Therefore, I add a check here to ensure the provided proxyConnection can be used to start an SSH session (to the jump server). Still, it may be a better idea to move this check inside the constructor of ConnectionInfo.

[Rob-Hague]

These should all stay (no need to break anyone here), they can forward to the other ConnectionInfo if it exists

[JoostJM]

I can return these if you want. I removed them to prevent confusion. Now, both SSH and HTTP/SOCKS connections may now be used as a proxy connection. However, SSH connection authentication is handled differently from HTTP/SOCKS proxies, with the latter keeping the username and password in the ConnectionInfo instance, but SSH inside one or more child AuthenticationMethod. Having the ProxyUsername and ProxyPassword may suggest these would also represent the respective fields inside a password authentication for SSH, but this is then not the case.

[Rob-Hague]

On a more practical note

1. Why do we need all the extra ConnectionInfo types and changes? Could we not just pass an existing ConnectionInfo to the constructor of another one?

// ssh -J user1@host1 user2@host2

var conn1 = new ConnectionInfo("host1", "user1", new PrivateKeyFile(...));
var conn2 = new ConnectionInfo("host2", "user2", new PrivateKeyFile(...), proxyConnection: conn1)

new SftpClient(conn2) // etc...

2. Why is JumpChannel necessary? It looks very much like ForwardedPortLocal, which is how I understand jump host works. So can we just use ForwardedPortLocal?

[JoostJM]

On a more practical note

1. Why do we need all the extra ConnectionInfo types and changes? Could we not just pass an existing ConnectionInfo to the constructor of another one?

// ssh -J user1@host1 user2@host2

var conn1 = new ConnectionInfo("host1", "user1", new PrivateKeyFile(...));
var conn2 = new ConnectionInfo("host2", "user2", new PrivateKeyFile(...), proxyConnection: conn1)

new SftpClient(conn2) // etc...

Do you mean why I now have distinct connection info classes for proxy HTTP/SOCKS connections and SSH connections?
For this I have 2 reasons:

1. The topmost ConnectionInfo should always be a SSH-specific connection info, as the final connection should yield a SSH connection.
2. HTTPS/SOCKS connections require an user and password to be set in the connection info object, but these are not required for SSH connections, so my thought was to keep them out of the ConnectionInfo class.

2. Why is JumpChannel necessary? It looks very much like ForwardedPortLocal, which is how I understand jump host works. So can we just use ForwardedPortLocal?

It's true that you could use ForwardedPortLocal to set up a tunnel to be used as a jump channel, allowing you to achieve similar functionality. IMO, this PR represents an improvement for 2 reasons:

1. Configuring and using it is now more intuitive, as only connection info needs to be provide, without having to worry about instantiating (and ultimatly disposing) the required tunnels.
2. Using JumpChannel is more secure, as the ForwardedPortLocal listener is only listening until the first socket is connected. That way, no other sockets (potentially from different programs running on the host machine) may subvert the tunnel existing to the jump host.

Finally, changing the configuration of proxies from properties in the ConnectionInfo class to child ConnectionInfo instances allows defining multiple jump hosts to hop through.

[JoostJM]

Thanks for working on this @JoostJM

This branch is currently 140 files and 1600+ lines changed, with breaking changes on top. It is hard to really tell but my impression is that many of the changes are unrelated to this feature. I think the review process is going to be much easier for everyone if this PR is made as small as possible, so that it just contains changes which are necessary. For example, an improvement like the dynamic port assignment can go into a separate PR; a change like the port 8122 is not something we experience in appveyor right now, so is it related to this PR or is it now unnecessary?

Doing it this way will mean a reviewer does not have to find such a large block of free time to dedicate to the review, which is honestly hard to be motivated for.

I appreciate the branch could be a small tree at this age, so you may not be motivated yourself to do that. And maybe @WojciechNagorski has some different thoughts, that's just my 2c

It's true that the functional changes in this PR are not as extensive as the number of files and lines changed would suggest. This is mainly due to the fact that for any SSH proxy, it needed to refactor how proxies are defined.
This in turn resulted in internal breaking changes, though externally the breaking changes would be limited (just the removal of the proxy(...) properties in ConnectionInfo class. E.g. the current constructors for ConnectionInfo are preserved, and the new functionality is exposed through a new constructor.
These internal breaking changes required significant rewriting of the tests. Some of these rewrites may now be redundant (such as the use of dynamically assigned ports where possible and switching to 8121 instead of 8122 everywhere else). I can remove these latter changes if you want. For now, I let them in as I considered them to be more robust than the hardcoded ports and I already did the work.

I agree that it makes this PR quite daunting to review in one go. If it helps, I can split it up into 3 (successive?) PRs:

1. The change to ConnectionInfo, moving proxy specification to child instances of ConnectionInfo. This also adds the option of defining multiple proxyconnection to connect through successively.
2. The addition of the SshConnector proxy connector and the associated JumpChannel.
3. Dynamic assignment of ports in the tests (this can potentially be a first PR, or removed entirely)

[JoostJM]

@Rob-Hague, @WojciechNagorski, what do you think about my suggestions? I'd rather not wait too long, or I'll need to spend again a lot of time getting everything up-to-date with the master branch...

[Rob-Hague]

I am sorry, I have not yet found a large enough chunk of time to consider this problem properly. In general I would favour simplicity unless there is proven demand otherwise.

I have a couple of other things to get through beforehand. As you know, the library does not move very fast so there are unlikely to be lots of conflicts any time soon :-)