Allow the client to initiate the key exchange

[geoffstewart]

Seems that connecting to some servers just hangs and times out with both server and client waiting on key exchange. I've read of Cisco switches and I've experienced it with AudioCodes SBC, as well.

This PR offers the ability to initiate the key exchange explicitly for these servers that sit and wait.

This PR may solve #767 .

[Shafqat-Ali]

How can I use this code in my application? Because I'm unable to modify ConnectionInfo.cs file because I installed nuget package in my application.

[geoffstewart]

@Shafqat-Ali You can clone my forked repo, checkout the branch and build your own version of SSH.NET.

[Shafqat-Ali]

@Shafqat-Ali You can clone my forked repo, checkout the branch and build your own version of SSH.NET.

@geoffstewart thanks, man. I appreciate your help

[likeMyCoffee]

Seems to be another solution to my pr #841

[BoronBGP]

This solution doesnt stop the client from sending the key exchange message again after recieving the key exchange init from the server. This is easily fixed by adding a condition in KeyExchange.Start to prevent duplicate messages. Once tested with this condition added I was succesfully able to connect to a cisco router while initiating a key exchange.

e.g.

publicvirtualvoidStart(Sessionsession,KeyExchangeInitMessagemessage){Session=session;// Send key exchange if server initiatedif(!session.ConnectionInfo.InitiateKeyExchange){SendMessage(session.ClientInitMessage);}// Determine encryption algorithmvarclientEncryptionAlgorithmName=(frombinsession.ConnectionInfo.Encryptions.Keysfromainmessage.EncryptionAlgorithmsClientToServerwherea==bselecta).FirstOrDefault();
...

[geoffstewart]

Interesting. Without your proposed fix, were you unable to connect to a cisco router? In other words, you needed your change in order for the connection to work?

[BoronBGP]

I was unable to connect to the Cisco Router without the fix. On wireshark it appeared that without the proposed fix the key exchange would be sent from the client twice and then this would trigger the end of the connection which would lead to this being repeatedly retried until timeout

[geoffstewart]

OK, I'll look at applying the change to this PR. Thanks!

[scott-xu]

@geoffstewart Would you mind adding some unit tests for the change?

[scott-xu]

StyleCop issue needs to be fixed.
Refer ConnectionInfo line 88 please.

[Rob-Hague]

I think _keyExchangeInProgress = true; needs to be added just before calling SendMessage(ClientInitMessage);

The logic should also be duplicated into Session.ConnectAsync()

Could/should this be done unconditionally?

[geoffstewart]

@Rob-Hague I will add the _keyExchangeInProgress = true to my change and duplicate logic in ConnectAsync().

As for doing this unconditionally, I implemented it with a condition because I figured it would negatively affect how connections with other servers would behave. I'd have to defer to others on that, though.

[Rob-Hague]

I have run packet captures against several servers using WinSCP (which is based on PuTTY AFAIK) and OpenSSH as clients, and concluded that neither waits for the server KEXINIT message before sending the client KEXINIT.

Based on this and my interpretation of the RFC, I don't think there is any reason for the library to wait for the server KEXINIT message, and I think we should enable the client KEXINIT sending unconditionally.

However, there is currently a problem (unconditional or not) which is that if we receive a server KEXINIT before having sent the client KEXINIT, it is possible that we will continue the key exchange procedure and send e.g. SSH_MSG_KEX_ECDH_INIT before we have sent the KEXINIT. This is causing errors in the integration tests locally.

To fix, I think we need another wait handle in Session e.g. _clientKeyExchangeInitSent which is set after SendMessage(ClientInitMessage); and must be waited on in OnKeyExchangeInitReceived before continuing the key exchange procedure.

What do you think?

[Rob-Hague]

OK... I had not applied the changes to ConnectAsync which explains the failures. So the problem is for now only theoretical 🙂 But possibly worth some more thought

[WojciechNagorski]

@Rob-Hague I agree that there has to be a better solution than this PR. Using the ssh command no one sets this kind of setting. This PR is like a workaround rather than a normal solution. Would you like propose some PR?

[likeMyCoffee]

@Rob-Hague I agree that there has to be a better solution than this PR. Using the ssh command no one sets this kind of setting. This PR is like a workaround rather than a normal solution. Would you like propose some PR?

Look I've commented before that I've already solved this problem check out pull request #841

The problem is that the initiation order of fixed in this code. The code adheres to the RFC where Cisco does not.

I was watching this thread to see if you were making a more elegant solution but I see you hit the wall ☺️

[Rob-Hague]

This PR is like a workaround rather than a normal solution. Would you like propose some PR?

@WojciechNagorski I think it would just take a couple of changes to this PR, if @geoffstewart is happy to make them (if not, I can try using this PR as a basis). But I first want to think a bit more about it.

Look I've commented before that I've already solved this problem check out pull request #841

Cool. I think we should still initiate the client KEXINIT without waiting for the server, whether or not it fixes the Cisco problem. If it does, then great. If not, then we need something in addition like #841.

[likeMyCoffee]

The most elegant solution would be to give the client x seconds before the server initiates. This would keep it kind of in the RFC, without extra parameters.

Then add a parameter for server first or client first, for situations where many quick connections are needed.

This would be a slightly more complex solution...

[WojciechNagorski]

I write "workaround" because no other SSH client provides such a parameter in the configuration depending on the server manufacturer ;)

[likeMyCoffee]

Just for the record, as my memory serves me, when SSH.net identifies first the Cisco device will never respond or identify. This causes a race hazard between SSH.net, on whom talks first. SSH.net first means a connection is never made because Cisco will never identify itself.

So IMHO it's more of an identification order issue...

[geoffstewart]

I'm far from an expert, but I'm happy to continue working on this PR to get something that works.

So, in the current implementation (not the PR), it looks like SSH.NET sends protocol version exchange and then waits for messages from the server; including KEXINIT.

In looking at another client implmentation (OpenSSH), it looks like they do the protocol version exchange and then directly send the kexinit instead of waiting. https://github.com/openssh/openssh-portable/blob/26f3f3bbc69196d908cad6558c8c7dc5beb8d74a/sshconnect.c#L1561.

I know it was suggested above to just always send the ClientInitMessage but perhaps it is as simple as that?

[Rob-Hague]

I know it was suggested above to just always send the ClientInitMessage but perhaps it is as simple as that?

That's my belief, with the caveat that we need to be able to respond to key re-exchange events, which are also SSH_MSG_KEXINIT messages. So, there is some decision necessary at the point of receiving a KEXINIT as to whether we need to send one back: if it is the initial key exchange, then the answer is no (we've already sent) but if it's a key re-exchange, then we do need to send one.

[likeMyCoffee]

Hope I'm not being redundant or to chatty here but here's the SSH RFC I've referred to before
https://datatracker.ietf.org/doc/html/rfc4253

In 4.2 the connection is explained.

Identification is sent by the client and then the KEXINIT.
Both sides may identify themselves.
Cisco wants to be the first to identify or it will stop responding, breaking the RFC (what I figured out with wireshark).

As this issue states it hangs on KEXINIT, I believe it's because the identification order caused the Cisco to stop responding making it hang on the next action.

The identification problem with Cisco needs to be solved so that KEXINIT is accepted. I don't think there is an actual key exchange problem, it's just where it gets stuck.

my 2 cts

[expyram]

Pulled 2023.0.0 today, and reintegrated #841 manually. It's still the only way I can get SSH.NET to connect reliably and repeatably to my Cisco router, so I hope a successful PR will evolve from this.

[Rob-Hague]

@likeMyCoffee@expyram Thanks. Let's now focus this PR on the key exchange and continue discussion relating to identification over in #841.

In the meantime, if one of you could demonstrate the identification problem with a packet capture, ideally a failure case with SSH.NET and a successful case with a different client, and post it over there (as a screenshot or otherwise) that would be great.

[Rob-Hague]

So to summarise my thoughts here, we can

1. make this unconditional
2. add _keyExchangeInProgress = true before the SendMessage(ClientInitMessage); call
3. move it above line 651 (_ = ThreadAbstraction.ExecuteThreadLongRunning(MessageListener);). This will ensure we have sent the client init before we continue the key exchange
4. Add a parameter bool sendClientInitMessage to IKeyExchange.Start which guards SendMessage(session.ClientInitMessage); in KeyExchange.Start
5. Add a private bool _initialKeyExchangeCompleted; (or similar) member to Session, which is used to decide the sendClientInitMessage parameter in Session.OnKeyExchangeInitReceived.

As for testing, if we make it unconditional then we are covered already 🙂

[geoffstewart]

I've had very little time to look at pushing this idea forward, unfortunately. But I'm happy to see @Rob-Hague created #1274 to keep this rolling.

[WojciechNagorski]

It's done in #1274

[WojciechNagorski]

The 2023.0.1 version has been released to Nuget: https://www.nuget.org/packages/SSH.NET/2023.0.1

[expyram]

I put 2023.0.1 into production today, and the new connection logic is working well with my Cisco routers. Your efforts are greatly appreciated.