You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by GitBox <gi...@apache.org> on 2021/07/12 19:53:02 UTC
[GitHub] [mina-sshd] tomaswolf opened a new pull request #201: [SSHD-1197] Fix race condition in KEX
tomaswolf opened a new pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201
There was a window in AbstractSession.requestNewKeyExchange()
during which the KEX state was set already to INIT, but the
caller's proposal not set yet. If the peer had also decided to
start a new key exchange, it was possible that the peer's KEX_INIT
message arrived and was handled in that window and then proceeded
with wrong or uninitialized data.
KEX negotiation must start only when both proposals are indeed
available. Thus wait when having received a KEX_INIT from the peer
until our own proposal has been prepared before continuing with the
negotiation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on a change in pull request #201: [SSHD-1197] Fix race condition in KEX
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on a change in pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201#discussion_r670204650
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##########
@@ -109,6 +109,16 @@
*/
public static final String SESSION = "org.apache.sshd.session";
+ /**
+ * A last-resort timeout for waiting after having received a KEX_INIT message from the peer 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.
+ *
+ * @see <a href="https://issues.apache.org/jira/browse/SSHD-1197">SSHD-1197</a>
+ * @see #doKexNegotiation()
+ */
+ private static final Duration KEX_PROPOSAL_SETUP_TIMEOUT = Duration.ofSeconds(42);
Review comment:
I've been thinking more about this -- I'll add it with validation for now. Reason: we are indeed waiting until sendKexInit has sent the buffer (or rather, has queued it for sending on the IOSession). But I'm still not happy with that whole KEX implementation. In particular, this having to wait until the buffer is sent should not be necessary, but with the current implementation is unavoidable because of the `ReservedSessionMessagesHandler.sendKexInitRequest` hook added for SSHD-1097, which can modify the buffer and even might send it on its own.
That hook was added recently, and is used only in that "endless tarpit" thing, where it actually only suppresses writing the buffer. If that were changed to something that could only say "don't send the message", I could extract that whole KEX state machine in a separate class and it would even possible to do an implementation that didn't have to wait at all for this race condition case handled here.
I wouldn't add a full explanation of how KEX works, that's explained in the RFCs. But the new timeout property will have a comment explaining what it is for.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #201: [SSHD-1197] Fix race condition in KEX
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201#discussion_r670119533
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##########
@@ -109,6 +109,16 @@
*/
public static final String SESSION = "org.apache.sshd.session";
+ /**
+ * A last-resort timeout for waiting after having received a KEX_INIT message from the peer 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.
+ *
+ * @see <a href="https://issues.apache.org/jira/browse/SSHD-1197">SSHD-1197</a>
+ * @see #doKexNegotiation()
+ */
+ private static final Duration KEX_PROPOSAL_SETUP_TIMEOUT = Duration.ofSeconds(42);
Review comment:
>>> If we make that configurable, users can very easily shoot themselves in the foot by setting a (very) low value, like 0.
I understand your concern, but personally I believe in total user responsibility - I don't like being told by someone that "they know better", so I am reluctant to impose my opinion on others (BTW, this is also why I always insist that all our internal variables and/or methods be accessible either via getters or as public/protected members).
In this context, the same could be said for *all* our configuration values - users can shoot themselves in the foot by choosing a wrong or conflicting value. However, I believe this is a good thing - they will learn... IMO, we are not supposed to protect our users from mistakes that occur because they do not bother to read the documentation or understand the consequences of their actions. In this case, perhaps we could do something that would satisfy both our concerns:
```java
Duration waitTime = CoreModuleProperties.KEX_PROPOSAL_SETUP_TIMEOUT.getRequired(this);
// SOME_MIN_VALUE is also publicly defined next to KEX_PROPOSAL_SETUP_TIMEOUT
ValidateUtils.checkTrue(waitTime.toMillis() >= SOME_MIN_VALUE, "Value below threshold: %d < %d", waitTime, SOME_MIN_VALUE);
```
>> Lengthening this should never be necessary. The thread that is preparing this side's proposal is already running.
While it seems reasonable, we don't know what weird usages our users might encounter or what unforeseen circumstances. Therefore, I believe we should give them the flexibility to decide for themselves.
Bottom line - I trust your judgement, and if you still feel that a constant value is more appropriate I will not object to it. In any case, let's also update the README (perhaps add a KEX section) and explain this mechanism so that if there are future questions or suspicions of bugs around this we will have a clear picture of the implementation and the choice of either a constant or configurable timeout value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #201: [SSHD-1197] Fix race condition in KEX
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201#discussion_r668995415
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##########
@@ -109,6 +109,16 @@
*/
public static final String SESSION = "org.apache.sshd.session";
+ /**
+ * A last-resort timeout for waiting after having received a KEX_INIT message from the peer 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.
+ *
+ * @see <a href="https://issues.apache.org/jira/browse/SSHD-1197">SSHD-1197</a>
+ * @see #doKexNegotiation()
+ */
+ private static final Duration KEX_PROPOSAL_SETUP_TIMEOUT = Duration.ofSeconds(42);
Review comment:
Please use a `CoreModuleProperties#KEX_PROPOSAL_SETUP_TIMEOUT` as initially suggested so that users can decide to shorten/lengthen this value instead of it being hardcoded
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #201: [SSHD-1197] Fix race condition in KEX
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201#discussion_r670119533
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##########
@@ -109,6 +109,16 @@
*/
public static final String SESSION = "org.apache.sshd.session";
+ /**
+ * A last-resort timeout for waiting after having received a KEX_INIT message from the peer 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.
+ *
+ * @see <a href="https://issues.apache.org/jira/browse/SSHD-1197">SSHD-1197</a>
+ * @see #doKexNegotiation()
+ */
+ private static final Duration KEX_PROPOSAL_SETUP_TIMEOUT = Duration.ofSeconds(42);
Review comment:
>>> If we make that configurable, users can very easily shoot themselves in the foot by setting a (very) low value, like 0.
I understand your concern, but personally I believe in total user responsibility - I don't like being told by someone that "they know better", so I am reluctant to impose my opinion on others (BTW, this is also why I always insist that all our internal variables and/or methods be accessible either via getters or as public/protected members).
In this context, the same could be said for *all* our configuration values - users can shoot themselves in the foot by choosing a wrong or conflicting value. However, I believe this is a good thing - they will learn... IMO, we are not supposed to protect our users from mistakes that occur because they do not bother to read the documentation or understand the consequences of their actions. In this case, perhaps we could do something that would satisfy both our concerns:
```java
Duration waitTime = CoreModuleProperties.KEX_PROPOSAL_SETUP_TIMEOUT.getRequired(this);
ValidateUtils.checkTrue(waitTime.toMillis() > SOME_MIN_VALUE, "Value below threshold: %d < %d", waitTime, SOME_MIN_VALUE);
```
>> Lengthening this should never be necessary. The thread that is preparing this side's proposal is already running.
While it seems reasonable, we don't know what weird usages our users might encounter or what unforeseen circumstances. Therefore, I believe we should give them the flexibility to decide for themselves.
Bottom line - I trust your judgement, and if you still feel that a constant value is more appropriate I will not object to it. In any case, let's also update the README (perhaps add a KEX section) and explain this mechanism so that if there are future questions or suspicions of bugs around this we will have a clear picture of the implementation and the choice of either a constant or configurable timeout value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #201: [SSHD-1197] Fix race condition in KEX
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201#discussion_r670289418
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##########
@@ -109,6 +109,16 @@
*/
public static final String SESSION = "org.apache.sshd.session";
+ /**
+ * A last-resort timeout for waiting after having received a KEX_INIT message from the peer 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.
+ *
+ * @see <a href="https://issues.apache.org/jira/browse/SSHD-1197">SSHD-1197</a>
+ * @see #doKexNegotiation()
+ */
+ private static final Duration KEX_PROPOSAL_SETUP_TIMEOUT = Duration.ofSeconds(42);
Review comment:
Sounds reasonable to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #201: [SSHD-1197] Fix race condition in KEX
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201#discussion_r670119533
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##########
@@ -109,6 +109,16 @@
*/
public static final String SESSION = "org.apache.sshd.session";
+ /**
+ * A last-resort timeout for waiting after having received a KEX_INIT message from the peer 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.
+ *
+ * @see <a href="https://issues.apache.org/jira/browse/SSHD-1197">SSHD-1197</a>
+ * @see #doKexNegotiation()
+ */
+ private static final Duration KEX_PROPOSAL_SETUP_TIMEOUT = Duration.ofSeconds(42);
Review comment:
>>> If we make that configurable, users can very easily shoot themselves in the foot by setting a (very) low value, like 0.
I understand your concern, but personally I believe in total user responsibility - I don't like being told by someone that "they know better", so I am reluctant to impose my opinion on others (BTW, this is also why I always insist that all our internal variables and/or methods be accessible either via getters or as public/protected members).
In this context, the same could be said for *all* our configuration values - users can shoot themselves in the foot by choosing a wrong or conflicting value. However, I believe this is a good thing - they will learn... IMO, we are not supposed to protect our users from mistakes that occur because they do not bother to read the documentation or understand the consequences of their actions. In this case, perhaps we could do something that would satisfy both our concerns:
```java
Duration waitTime = CoreModuleProperties.KEX_PROPOSAL_SETUP_TIMEOUT.getRequired(this);
ValidateUtils.checkTrue(waitTime.toMillis() > 0L, "Infinite/Undefined value N/A: %d", waitTime);
```
>> Lengthening this should never be necessary. The thread that is preparing this side's proposal is already running.
While it seems reasonable, we don't know what weird usages our users might encounter or what unforeseen circumstances. Therefore, I believe we should give them the flexibility to decide for themselves.
Bottom line - I trust your judgement, and if you still feel that a constant value is more appropriate I will not object to it. In any case, let's also update the README (perhaps add a KEX section) and explain this mechanism so that if there are future questions or suspicions of bugs around this we will have a clear picture of the implementation and the choice of either a constant or configurable timeout value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #201: [SSHD-1197] Fix race condition in KEX
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201#discussion_r668893454
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##########
@@ -696,7 +702,18 @@ protected void handleKexInit(Buffer buffer) throws Exception {
protected void doKexNegotiation() throws Exception {
if (kexState.compareAndSet(KexState.DONE, KexState.RUN)) {
sendKexInit();
- } else if (!kexState.compareAndSet(KexState.INIT, KexState.RUN)) {
+ } else if (kexState.compareAndSet(KexState.INIT, KexState.RUN)) {
+ DefaultKeyExchangeFuture initFuture;
+ synchronized (kexState) {
+ initFuture = kexInitializedFuture;
+ if (initFuture == null) {
+ initFuture = new DefaultKeyExchangeFuture(toString(), kexLock);
+ kexInitializedFuture = initFuture;
+ }
+ }
+
+ initFuture.await();
Review comment:
I am not comfortable with an infinite wait - just in case some deadlock/bug occurs. Let's define a `CoreModuleProperties#MAX_KEX_INIT_WAIT_TIMEOUT` and use it here (I think ~30 seconds as default value should do, but you are welcome to use some other value). If it expires then throw an exception (which will close the session(.
##########
File path: sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpTransferTest.java
##########
@@ -23,22 +23,39 @@
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Path;
+import java.util.ArrayList;
import java.util.Arrays;
import java.util.Date;
+import java.util.List;
import org.apache.sshd.client.session.ClientSession;
+import org.apache.sshd.core.CoreModuleProperties;
import org.apache.sshd.sftp.client.fs.SftpFileSystem;
import org.junit.FixMethodOrder;
import org.junit.Test;
+import org.junit.runner.RunWith;
import org.junit.runners.MethodSorters;
+import org.junit.runners.Parameterized;
@FixMethodOrder(MethodSorters.NAME_ASCENDING)
+@RunWith(Parameterized.class)
public class SftpTransferTest extends AbstractSftpClientTestSupport {
private static final int BUFFER_SIZE = 8192;
- public SftpTransferTest() throws IOException {
+ private final long rekeyBlockSize;
+
+ public SftpTransferTest(long rekeyBlockSize) throws IOException {
super();
Review comment:
Please remove the `super()` call - the coding convention is that if there is no other statement in the constructor, then we call `super()` explicitly. Otherwise (i.e., more statements) we drop it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #201: [SSHD-1197] Fix race condition in KEX
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201#discussion_r670119533
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##########
@@ -109,6 +109,16 @@
*/
public static final String SESSION = "org.apache.sshd.session";
+ /**
+ * A last-resort timeout for waiting after having received a KEX_INIT message from the peer 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.
+ *
+ * @see <a href="https://issues.apache.org/jira/browse/SSHD-1197">SSHD-1197</a>
+ * @see #doKexNegotiation()
+ */
+ private static final Duration KEX_PROPOSAL_SETUP_TIMEOUT = Duration.ofSeconds(42);
Review comment:
>>> If we make that configurable, users can very easily shoot themselves in the foot by setting a (very) low value, like 0.
I understand your concern, but personally I believe in total user responsibility - I don't like being told by someone that "they know better", so I am reluctant to impose my opinion on others (BTW, this is also why I always insist that all our internal variables and/or methods be accessible either via getters or as public/protected members).
In this context, the same could be said for *all* our configuration values - users can shoot themselves in the foot by choosing a wrong or conflicting value. However, I believe this is a good thing - they will learn... IMO, we are not supposed to protect our users from mistakes that occur because they do not bother to read the documentation or understand the consequences of their actions. In this case, perhaps we could do something that would satisfy both our concerns:
```java
Duration waitTime = CoreModuleProperties.KEX_PROPOSAL_SETUP_TIMEOUT.getRequired(this);
long waitMillis = waitTime.toMillis() ;
// SOME_MIN_VALUE is also publicly defined next to KEX_PROPOSAL_SETUP_TIMEOUT
ValidateUtils.checkTrue(waitMillis >= SOME_MIN_VALUE, "Value below threshold: %d < %d", waitMillis , SOME_MIN_VALUE);
future.await(waitMillis);
```
>> Lengthening this should never be necessary. The thread that is preparing this side's proposal is already running.
While it seems reasonable, we don't know what weird usages our users might encounter or what unforeseen circumstances. Therefore, I believe we should give them the flexibility to decide for themselves.
Bottom line - I trust your judgement, and if you still feel that a constant value is more appropriate I will not object to it. In any case, let's also update the README (perhaps add a KEX section) and explain this mechanism so that if there are future questions or suspicions of bugs around this we will have a clear picture of the implementation and the choice of either a constant or configurable timeout value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on a change in pull request #201: [SSHD-1197] Fix race condition in KEX
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on a change in pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201#discussion_r668998134
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##########
@@ -109,6 +109,16 @@
*/
public static final String SESSION = "org.apache.sshd.session";
+ /**
+ * A last-resort timeout for waiting after having received a KEX_INIT message from the peer 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.
+ *
+ * @see <a href="https://issues.apache.org/jira/browse/SSHD-1197">SSHD-1197</a>
+ * @see #doKexNegotiation()
+ */
+ private static final Duration KEX_PROPOSAL_SETUP_TIMEOUT = Duration.ofSeconds(42);
Review comment:
Hmmm. If we make that configurable, users can very easily shoot themselves in the foot by setting a (very) low value, like 0. I don't like that. Lengthening this should never be necessary. The thread that is preparing this side's proposal is already running.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #201: [SSHD-1197] Fix race condition in KEX
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201#discussion_r670119533
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##########
@@ -109,6 +109,16 @@
*/
public static final String SESSION = "org.apache.sshd.session";
+ /**
+ * A last-resort timeout for waiting after having received a KEX_INIT message from the peer 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.
+ *
+ * @see <a href="https://issues.apache.org/jira/browse/SSHD-1197">SSHD-1197</a>
+ * @see #doKexNegotiation()
+ */
+ private static final Duration KEX_PROPOSAL_SETUP_TIMEOUT = Duration.ofSeconds(42);
Review comment:
>>> If we make that configurable, users can very easily shoot themselves in the foot by setting a (very) low value, like 0.
I understand your concern, but personally I believe in total user responsibility - I don't like being told by someone that "they know better", so I am reluctant to impose my opinion on others (BTW, this is also why I always insist that all our internal variables and/or methods be accessible either via getters or as public/protected members).
In this context, the same could be said for *all* our configuration values - users can shoot themselves in the foot by choosing a wrong or conflicting value. However, I believe this is a good thing - they will learn... IMO, we are not supposed to protect our users from mistakes that occur because they do not bother to read the documentation or understand the consequences of their actions. In this case, perhaps we could do something that would satisfy both our concerns:
```java
Duration waitTime = CoreModuleProperties.KEX_PROPOSAL_SETUP_TIMEOUT.getRequired(this);
// SOME_MIN_VALUE is also publicly defined next to KEX_PROPOSAL_SETUP_TIMEOUT
ValidateUtils.checkTrue(waitTime.toMillis() > SOME_MIN_VALUE, "Value below threshold: %d < %d", waitTime, SOME_MIN_VALUE);
```
>> Lengthening this should never be necessary. The thread that is preparing this side's proposal is already running.
While it seems reasonable, we don't know what weird usages our users might encounter or what unforeseen circumstances. Therefore, I believe we should give them the flexibility to decide for themselves.
Bottom line - I trust your judgement, and if you still feel that a constant value is more appropriate I will not object to it. In any case, let's also update the README (perhaps add a KEX section) and explain this mechanism so that if there are future questions or suspicions of bugs around this we will have a clear picture of the implementation and the choice of either a constant or configurable timeout value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on a change in pull request #201: [SSHD-1197] Fix race condition in KEX
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on a change in pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201#discussion_r668991711
##########
File path: sshd-sftp/src/test/java/org/apache/sshd/sftp/client/SftpTransferTest.java
##########
@@ -23,22 +23,39 @@
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Path;
+import java.util.ArrayList;
import java.util.Arrays;
import java.util.Date;
+import java.util.List;
import org.apache.sshd.client.session.ClientSession;
+import org.apache.sshd.core.CoreModuleProperties;
import org.apache.sshd.sftp.client.fs.SftpFileSystem;
import org.junit.FixMethodOrder;
import org.junit.Test;
+import org.junit.runner.RunWith;
import org.junit.runners.MethodSorters;
+import org.junit.runners.Parameterized;
@FixMethodOrder(MethodSorters.NAME_ASCENDING)
+@RunWith(Parameterized.class)
public class SftpTransferTest extends AbstractSftpClientTestSupport {
private static final int BUFFER_SIZE = 8192;
- public SftpTransferTest() throws IOException {
+ private final long rekeyBlockSize;
+
+ public SftpTransferTest(long rekeyBlockSize) throws IOException {
super();
Review comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf merged pull request #201: [SSHD-1197] Fix race condition in KEX
Posted by GitBox <gi...@apache.org>.
tomaswolf merged pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on pull request #201: [SSHD-1197] Fix race condition in KEX
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201#issuecomment-878484411
@lgoldstein , do you agree with this way of fixing this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] lgoldstein commented on a change in pull request #201: [SSHD-1197] Fix race condition in KEX
Posted by GitBox <gi...@apache.org>.
lgoldstein commented on a change in pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201#discussion_r668893454
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##########
@@ -696,7 +702,18 @@ protected void handleKexInit(Buffer buffer) throws Exception {
protected void doKexNegotiation() throws Exception {
if (kexState.compareAndSet(KexState.DONE, KexState.RUN)) {
sendKexInit();
- } else if (!kexState.compareAndSet(KexState.INIT, KexState.RUN)) {
+ } else if (kexState.compareAndSet(KexState.INIT, KexState.RUN)) {
+ DefaultKeyExchangeFuture initFuture;
+ synchronized (kexState) {
+ initFuture = kexInitializedFuture;
+ if (initFuture == null) {
+ initFuture = new DefaultKeyExchangeFuture(toString(), kexLock);
+ kexInitializedFuture = initFuture;
+ }
+ }
+
+ initFuture.await();
Review comment:
I am not comfortable with an infinite wait - just in case some deadlock/bug occurs. Let's define a `CoreModuleProperties#MAX_KEX_INIT_WAIT_TIMEOUT` and use it here (I think ~30 seconds as default value should do, but you are welcome to use some other value). If it expires then throw an exception (which will close the session).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
[GitHub] [mina-sshd] tomaswolf commented on a change in pull request #201: [SSHD-1197] Fix race condition in KEX
Posted by GitBox <gi...@apache.org>.
tomaswolf commented on a change in pull request #201:
URL: https://github.com/apache/mina-sshd/pull/201#discussion_r668992125
##########
File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
##########
@@ -696,7 +702,18 @@ protected void handleKexInit(Buffer buffer) throws Exception {
protected void doKexNegotiation() throws Exception {
if (kexState.compareAndSet(KexState.DONE, KexState.RUN)) {
sendKexInit();
- } else if (!kexState.compareAndSet(KexState.INIT, KexState.RUN)) {
+ } else if (kexState.compareAndSet(KexState.INIT, KexState.RUN)) {
+ DefaultKeyExchangeFuture initFuture;
+ synchronized (kexState) {
+ initFuture = kexInitializedFuture;
+ if (initFuture == null) {
+ initFuture = new DefaultKeyExchangeFuture(toString(), kexLock);
+ kexInitializedFuture = initFuture;
+ }
+ }
+
+ initFuture.await();
Review comment:
await(timeout) throws an exception already if it times out. Added a timeout.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org