Skip to content

Commit

Permalink
Clean-up: timeout-free KEX
Browse files Browse the repository at this point in the history
Do not wait synchronously until the kexInitializedFuture is fulfilled.
Instead simply submit the negotiation as a listener on that future.
It'll run once our own KEX proposal has been composed.

Waiting synchronously has the potential to block if the MINA transport
is used.

Deprecate CoreModuleProperties.KEX_PROPOSAL_SETUP_TIMEOUT; it's unused
now.
  • Loading branch information
tomaswolf committed Aug 6, 2024
1 parent c2a4760 commit 9e4e040
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -890,25 +890,36 @@ protected void doKexNegotiation() throws Exception {
sendKexInit();
break;
case BOTH:
DefaultKeyExchangeFuture initFuture;
synchronized (kexState) {
initFuture = kexInitializedFuture;
if (initFuture == null) {
initFuture = new DefaultKeyExchangeFuture(toString(), null);
kexInitializedFuture = initFuture;
}
}
// requestNewKeyExchange() is running in some other thread: wait until it has set up our own proposal.
// The timeout is a last resort only to avoid blocking indefinitely in case something goes
// catastrophically wrong somewhere; it should never be hit. If it is, an exception will be thrown.
// We are in the process of sending our own KEX_INIT. Do the negotiation once that's done.
//
// See https://issues.apache.org/jira/browse/SSHD-1197
initFuture.await(CoreModuleProperties.KEX_PROPOSAL_SETUP_TIMEOUT.getRequired(this));
break;
default:
throw new IllegalStateException("Received SSH_MSG_KEXINIT while key exchange is running");
}
// Note: we should not wait here; it might block (in particular with the MINA transport back-end).
DefaultKeyExchangeFuture initFuture;
synchronized (kexState) {
initFuture = kexInitializedFuture;
if (initFuture == null) {
initFuture = new DefaultKeyExchangeFuture(toString(), null);
kexInitializedFuture = initFuture;
}
}
initFuture.addListener(f -> {
if (f.isDone()) {
try {
performKexNegotiation();
} catch (Exception e) {
exceptionCaught(e);
}
} else {
exceptionCaught(f.getException());
}
});
}

protected void performKexNegotiation() throws Exception {
Map<KexProposalOption, String> result = negotiate();
String kexAlgorithm = result.get(KexProposalOption.ALGORITHMS);
Collection<? extends KeyExchangeFactory> kexFactories = getKeyExchangeFactories();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,14 +138,13 @@ public final class CoreModuleProperties {
= Property.bool("allow-dhg1-kex-fallback", false);

/**
* If the peer initiates a key exchange, we send our own KEX_INIT message with the proposal. This is a last-resort
* timeout for waiting until we have prepared our own KEX proposal. This timeout should actually never be hit unless
* there is a serious deadlock somewhere and the session is never closed. It should be set to a reasonably high
* value; it must be at least 5 seconds and the default is 42 seconds. If the timeout is ever hit, the key exchange
* initiated by the peer will fail.
* Unused.
*
* @deprecated since 2.14.0
*/
@Deprecated
public static final Property<Duration> KEX_PROPOSAL_SETUP_TIMEOUT
= Property.durationSec("kex-proposal-setup-timeout", Duration.ofSeconds(42), Duration.ofSeconds(5));
= Property.duration("kex-proposal-setup-timeout", Duration.ZERO);

/**
* Key used to set the heartbeat interval in milliseconds (0 to disable = default)
Expand Down

0 comments on commit 9e4e040

Please sign in to comment.