Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump channel #954

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Jump channel #954

wants to merge 8 commits into from

Conversation

JoostJM
Copy link

@JoostJM JoostJM commented Apr 28, 2022

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.

Allow specification of jumphosts. Sessions opened using this configuration will connect to the remote host through the specified jump servers.
Rewrite some interfaces to allow this new type of ProxyConnection
Additionally, store and use information on proxy connections as a IConnectionInfo object "ProxyConnection" in the IConnectionInfo object. This allows the user to specify multiple proxies to connect through before making the final connection to the target (which will be done using a "DirectConnector" instance).

Update tests to reflect new interface
@JoostJM
Copy link
Author

JoostJM commented Apr 28, 2022

Related to #852.

connectionInfo is allowed to be of type other than ProxyConnectionInfo.
Due to new interface with proxy connections, ensure the correct Mock objects are used and setup.
When testing connectors and proxy connectors, initialize (proxy) servers first with port set to 0 (resulting in dynamic assignment of the port number). Then use the actual port number to initialize the connection info.

This prevents failing test due to already occupied ports.
@JoostJM
Copy link
Author

JoostJM commented Apr 28, 2022

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.

Port 8122 is already bound in AppVeyor and therefore causes unexpected errors. Use dynamic port assignment (using port 0) where possible, port 8121 otherwise.

Addtionally, add mock setup for IConnector.Dispose() where these mocks are used.
Due to the new style of defining proxy connections, connection timeout should be set for each connection seperately.

Set the connection timeout on the proxy connection test (HTTP, SOCKS4 and SOCKS5) to reflect this.
@JoostJM
Copy link
Author

JoostJM commented May 11, 2022

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
Copy link

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
Copy link
Author

JoostJM commented May 27, 2022

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

The SSH connector erroneously started a connection to the final destination host, instead of the intended proxy (or jump) host. Fix this error to prevent an infinite recursion.
@benrobot
Copy link

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

@JoostJM
Copy link
Author

JoostJM commented Jul 31, 2023

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants