You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mina.apache.org by lg...@apache.org on 2019/06/17 03:58:35 UTC
[mina-sshd] 01/02: [SSHD-921] Do no send session disconnect message
due to timeout expiration if already done so.
This is an automated email from the ASF dual-hosted git repository.
lgoldstein pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
commit 95e826e5b6d4ec678c513462c735d87b176c41cf
Author: Lyor Goldstein <lg...@apache.org>
AuthorDate: Wed Jun 5 14:33:11 2019 +0300
[SSHD-921] Do no send session disconnect message due to timeout expiration if already done so.
---
CHANGES.md | 2 +
.../common/session/helpers/TimeoutIndicator.java | 74 ++++++++++++++++++++++
.../org/apache/sshd/common/session/Session.java | 14 +---
.../common/session/SessionDisconnectHandler.java | 4 +-
.../sshd/common/session/helpers/SessionHelper.java | 60 ++++++++++--------
.../java/org/apache/sshd/server/ServerTest.java | 27 ++++----
6 files changed, 132 insertions(+), 49 deletions(-)
diff --git a/CHANGES.md b/CHANGES.md
index fe3fa8b..5eab448 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -138,3 +138,5 @@ and therefore closing all currently tracked file/directory handles.
* [SSHD-916](https://issues.apache.org/jira/browse/SSHD-916) - Avoid locking the session lock when signalling client session authentication failure.
* [SSHD-917](https://issues.apache.org/jira/browse/SSHD-917) - Add support for SSH2 public key file format.
+
+* [SSHD-921](https://issues.apache.org/jira/browse/SSHD-921) - Do not send session disconnect message due to timeout expiration if already done so.
diff --git a/sshd-common/src/main/java/org/apache/sshd/common/session/helpers/TimeoutIndicator.java b/sshd-common/src/main/java/org/apache/sshd/common/session/helpers/TimeoutIndicator.java
new file mode 100644
index 0000000..4e95269
--- /dev/null
+++ b/sshd-common/src/main/java/org/apache/sshd/common/session/helpers/TimeoutIndicator.java
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sshd.common.session.helpers;
+
+/**
+ * Used to convey information about an expired timeout
+ *
+ * @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
+ */
+public class TimeoutIndicator {
+ /**
+ * Timeout status.
+ */
+ public enum TimeoutStatus {
+ NoTimeout,
+ AuthTimeout,
+ IdleTimeout
+ }
+
+ public static final TimeoutIndicator NONE = new TimeoutIndicator(TimeoutStatus.NoTimeout, 0L, 0L);
+
+ private final TimeoutStatus status;
+ private final long thresholdValue;
+ private final long expiredValue;
+
+ /**
+ * @param status The expired timeout type (if any)
+ * @param thresholdValue The configured timeout value
+ * @param expiredValue The actual value that cause the timeout
+ */
+ public TimeoutIndicator(TimeoutStatus status, long thresholdValue, long expiredValue) {
+ this.status = status;
+ this.thresholdValue = thresholdValue;
+ this.expiredValue = expiredValue;
+ }
+
+ public TimeoutStatus getStatus() {
+ return status;
+ }
+
+ public long getThresholdValue() {
+ return thresholdValue;
+ }
+
+ public long getExpiredValue() {
+ return expiredValue;
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName()
+ + "[status=" + getStatus()
+ + ", threshold=" + getThresholdValue()
+ + ", expired=" + getExpiredValue()
+ + "]";
+ }
+}
diff --git a/sshd-core/src/main/java/org/apache/sshd/common/session/Session.java b/sshd-core/src/main/java/org/apache/sshd/common/session/Session.java
index ee55a26..2478727 100644
--- a/sshd-core/src/main/java/org/apache/sshd/common/session/Session.java
+++ b/sshd-core/src/main/java/org/apache/sshd/common/session/Session.java
@@ -42,6 +42,7 @@ import org.apache.sshd.common.io.PacketWriter;
import org.apache.sshd.common.kex.KexFactoryManager;
import org.apache.sshd.common.kex.KeyExchange;
import org.apache.sshd.common.mac.MacInformation;
+import org.apache.sshd.common.session.helpers.TimeoutIndicator;
import org.apache.sshd.common.util.buffer.Buffer;
/**
@@ -75,15 +76,6 @@ public interface Session
boolean isServerSession();
/**
- * Timeout status.
- */
- enum TimeoutStatus {
- NoTimeout,
- AuthTimeout,
- IdleTimeout
- }
-
- /**
* Retrieves current cipher information - <B>Note:</B> may change if
* key re-exchange executed
*
@@ -245,9 +237,9 @@ public interface Session
/**
* Check if timeout has occurred.
*
- * @return the timeout status, never {@code null}
+ * @return the timeout status - never {@code null}
*/
- TimeoutStatus getTimeoutStatus();
+ TimeoutIndicator getTimeoutStatus();
/**
* @return Timeout value in milliseconds for communication
diff --git a/sshd-core/src/main/java/org/apache/sshd/common/session/SessionDisconnectHandler.java b/sshd-core/src/main/java/org/apache/sshd/common/session/SessionDisconnectHandler.java
index 76a0d85..5f0d063 100644
--- a/sshd-core/src/main/java/org/apache/sshd/common/session/SessionDisconnectHandler.java
+++ b/sshd-core/src/main/java/org/apache/sshd/common/session/SessionDisconnectHandler.java
@@ -24,7 +24,7 @@ import java.util.Map;
import org.apache.sshd.common.Service;
import org.apache.sshd.common.kex.KexProposalOption;
-import org.apache.sshd.common.session.Session.TimeoutStatus;
+import org.apache.sshd.common.session.helpers.TimeoutIndicator;
import org.apache.sshd.common.util.buffer.Buffer;
import org.apache.sshd.server.ServerFactoryManager;
@@ -46,7 +46,7 @@ public interface SessionDisconnectHandler {
* @throws IOException If failed to handle the event
*/
default boolean handleTimeoutDisconnectReason(
- Session session, TimeoutStatus timeoutStatus)
+ Session session, TimeoutIndicator timeoutStatus)
throws IOException {
return false;
}
diff --git a/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/SessionHelper.java b/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/SessionHelper.java
index 3ae0e45..5e91e32 100644
--- a/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/SessionHelper.java
+++ b/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/SessionHelper.java
@@ -21,7 +21,6 @@ package org.apache.sshd.common.session.helpers;
import java.io.IOException;
import java.net.SocketAddress;
import java.nio.charset.StandardCharsets;
-import java.util.AbstractMap.SimpleImmutableEntry;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
@@ -64,6 +63,7 @@ import org.apache.sshd.common.session.SessionContext;
import org.apache.sshd.common.session.SessionDisconnectHandler;
import org.apache.sshd.common.session.SessionListener;
import org.apache.sshd.common.session.UnknownChannelReferenceHandler;
+import org.apache.sshd.common.session.helpers.TimeoutIndicator.TimeoutStatus;
import org.apache.sshd.common.util.GenericUtils;
import org.apache.sshd.common.util.Invoker;
import org.apache.sshd.common.util.ValidateUtils;
@@ -100,7 +100,7 @@ public abstract class SessionHelper extends AbstractKexFactoryManager implements
// Session timeout measurements
private long authTimeoutStart = System.currentTimeMillis();
private long idleTimeoutStart = System.currentTimeMillis();
- private final AtomicReference<TimeoutStatus> timeoutStatus = new AtomicReference<>(TimeoutStatus.NoTimeout);
+ private final AtomicReference<TimeoutIndicator> timeoutStatus = new AtomicReference<>(TimeoutIndicator.NONE);
private ReservedSessionMessagesHandler reservedSessionMessagesHandler;
private SessionDisconnectHandler sessionDisconnectHandler;
@@ -221,34 +221,44 @@ public abstract class SessionHelper extends AbstractKexFactoryManager implements
* Checks whether the session has timed out (both auth and idle timeouts are checked).
* If the session has timed out, a DISCONNECT message will be sent.
*
+ * @return An indication whether timeout has been detected
* @throws IOException If failed to check
* @see #checkAuthenticationTimeout(long, long)
* @see #checkIdleTimeout(long, long)
*/
- protected void checkForTimeouts() throws IOException {
+ protected TimeoutIndicator checkForTimeouts() throws IOException {
if ((!isOpen()) || isClosing() || isClosed()) {
if (log.isDebugEnabled()) {
log.debug("checkForTimeouts({}) session closing", this);
- return;
+ return TimeoutIndicator.NONE;
+ }
+ }
+
+ // If already detected a timeout don't check again
+ TimeoutIndicator result = timeoutStatus.get();
+ TimeoutStatus status = (result == null) ? TimeoutStatus.NoTimeout : result.getStatus();
+ if ((status != null) && (status != TimeoutStatus.NoTimeout)) {
+ if (log.isDebugEnabled()) {
+ log.debug("checkForTimeouts({}) already detected {}", this, result);
}
+ return result;
}
long now = System.currentTimeMillis();
- Map.Entry<TimeoutStatus, String> result =
- checkAuthenticationTimeout(now, getAuthTimeout());
+ result = checkAuthenticationTimeout(now, getAuthTimeout());
if (result == null) {
result = checkIdleTimeout(now, getIdleTimeout());
}
- TimeoutStatus status = (result == null) ? TimeoutStatus.NoTimeout : result.getKey();
+ status = (result == null) ? TimeoutStatus.NoTimeout : result.getStatus();
if ((status == null) || TimeoutStatus.NoTimeout.equals(status)) {
- return;
+ return TimeoutIndicator.NONE;
}
SessionDisconnectHandler handler = getSessionDisconnectHandler();
- if ((handler != null) && handler.handleTimeoutDisconnectReason(this, status)) {
+ if ((handler != null) && handler.handleTimeoutDisconnectReason(this, result)) {
if (log.isDebugEnabled()) {
- log.debug("checkForTimeouts({}) cancel {} due to handler intervention", this, status);
+ log.debug("checkForTimeouts({}) cancel {} due to handler intervention", this, result);
}
switch(status) {
@@ -261,15 +271,19 @@ public abstract class SessionHelper extends AbstractKexFactoryManager implements
default: // ignored
}
- return;
+
+ return TimeoutIndicator.NONE;
}
if (log.isDebugEnabled()) {
- log.debug("checkForTimeouts({}) disconnect - reason={}", this, status);
+ log.debug("checkForTimeouts({}) disconnect - reason={}", this, result);
}
- timeoutStatus.set(status);
- disconnect(SshConstants.SSH2_DISCONNECT_PROTOCOL_ERROR, result.getValue());
+ timeoutStatus.set(result);
+
+ disconnect(SshConstants.SSH2_DISCONNECT_PROTOCOL_ERROR,
+ "Detected " + status + " after " + result.getExpiredValue() + "/" + result.getThresholdValue() + " ms.");
+ return result;
}
@Override
@@ -289,16 +303,14 @@ public abstract class SessionHelper extends AbstractKexFactoryManager implements
*
* @param now The current time in millis
* @param authTimeoutMs The configured timeout in millis - if non-positive then no timeout
- * @return A {@link SimpleImmutableEntry} specifying the timeout status and disconnect reason
+ * @return A {@link TimeoutIndicator} specifying the timeout status and disconnect reason
* message if timeout expired, {@code null} or {@code NoTimeout} if no timeout occurred
* @see #getAuthTimeout()
*/
- protected SimpleImmutableEntry<TimeoutStatus, String> checkAuthenticationTimeout(long now, long authTimeoutMs) {
+ protected TimeoutIndicator checkAuthenticationTimeout(long now, long authTimeoutMs) {
long authDiff = now - getAuthTimeoutStart();
if ((!isAuthenticated()) && (authTimeoutMs > 0L) && (authDiff > authTimeoutMs)) {
- return new SimpleImmutableEntry<>(
- TimeoutStatus.AuthTimeout,
- "Session has timed out waiting for authentication after " + authTimeoutMs + " ms.");
+ return new TimeoutIndicator(TimeoutStatus.AuthTimeout, authTimeoutMs, authDiff);
} else {
return null;
}
@@ -314,16 +326,14 @@ public abstract class SessionHelper extends AbstractKexFactoryManager implements
*
* @param now The current time in millis
* @param idleTimeoutMs The configured timeout in millis - if non-positive then no timeout
- * @return A {@link SimpleImmutableEntry} specifying the timeout status and disconnect reason
+ * @return A {@link TimeoutIndicator} specifying the timeout status and disconnect reason
* message if timeout expired, {@code null} or {@code NoTimeout} if no timeout occurred
* @see #getIdleTimeout()
*/
- protected SimpleImmutableEntry<TimeoutStatus, String> checkIdleTimeout(long now, long idleTimeoutMs) {
+ protected TimeoutIndicator checkIdleTimeout(long now, long idleTimeoutMs) {
long idleDiff = now - getIdleTimeoutStart();
if ((idleTimeoutMs > 0L) && (idleDiff > idleTimeoutMs)) {
- return new SimpleImmutableEntry<>(
- TimeoutStatus.IdleTimeout,
- "User session has timed out idling after " + idleTimeoutMs + " ms.");
+ return new TimeoutIndicator(TimeoutStatus.IdleTimeout, idleTimeoutMs, idleDiff);
} else {
return null;
}
@@ -337,7 +347,7 @@ public abstract class SessionHelper extends AbstractKexFactoryManager implements
}
@Override
- public TimeoutStatus getTimeoutStatus() {
+ public TimeoutIndicator getTimeoutStatus() {
return timeoutStatus.get();
}
diff --git a/sshd-core/src/test/java/org/apache/sshd/server/ServerTest.java b/sshd-core/src/test/java/org/apache/sshd/server/ServerTest.java
index c9ee121..ae3ce35 100644
--- a/sshd-core/src/test/java/org/apache/sshd/server/ServerTest.java
+++ b/sshd-core/src/test/java/org/apache/sshd/server/ServerTest.java
@@ -60,12 +60,13 @@ import org.apache.sshd.common.channel.WindowClosedException;
import org.apache.sshd.common.io.IoSession;
import org.apache.sshd.common.kex.KexProposalOption;
import org.apache.sshd.common.session.Session;
-import org.apache.sshd.common.session.Session.TimeoutStatus;
import org.apache.sshd.common.session.SessionContext;
import org.apache.sshd.common.session.SessionDisconnectHandler;
import org.apache.sshd.common.session.SessionListener;
import org.apache.sshd.common.session.helpers.AbstractConnectionService;
import org.apache.sshd.common.session.helpers.AbstractSession;
+import org.apache.sshd.common.session.helpers.TimeoutIndicator;
+import org.apache.sshd.common.session.helpers.TimeoutIndicator.TimeoutStatus;
import org.apache.sshd.common.util.GenericUtils;
import org.apache.sshd.common.util.MapEntryUtils.NavigableMapBuilder;
import org.apache.sshd.common.util.OsUtils;
@@ -202,14 +203,15 @@ public class ServerTest extends BaseTestSupport {
final long testAuthTimeout = TimeUnit.SECONDS.toMillis(4L);
PropertyResolverUtils.updateProperty(sshd, FactoryManager.AUTH_TIMEOUT, testAuthTimeout);
- AtomicReference<TimeoutStatus> timeoutHolder = new AtomicReference<>();
+ AtomicReference<TimeoutIndicator> timeoutHolder = new AtomicReference<>(TimeoutIndicator.NONE);
sshd.setSessionDisconnectHandler(new SessionDisconnectHandler() {
@Override
- public boolean handleTimeoutDisconnectReason(Session session, TimeoutStatus timeoutStatus)
+ public boolean handleTimeoutDisconnectReason(Session session, TimeoutIndicator timeoutStatus)
throws IOException {
outputDebugMessage("Session %s timeout reported: %s", session, timeoutStatus);
- TimeoutStatus prev = timeoutHolder.getAndSet(timeoutStatus);
- if (prev != null) {
+
+ TimeoutIndicator prev = timeoutHolder.getAndSet(timeoutStatus);
+ if (prev != TimeoutIndicator.NONE) {
throw new StreamCorruptedException("Multiple timeout disconnects: " + timeoutStatus + " / " + prev);
}
return false;
@@ -235,23 +237,24 @@ public class ServerTest extends BaseTestSupport {
client.stop();
}
- assertSame("Mismatched timeout status reported", TimeoutStatus.AuthTimeout, timeoutHolder.getAndSet(null));
+ TimeoutIndicator status = timeoutHolder.getAndSet(null);
+ assertSame("Mismatched timeout status reported", TimeoutIndicator.TimeoutStatus.AuthTimeout, status.getStatus());
}
@Test
public void testIdleTimeout() throws Exception {
final long testIdleTimeout = 2500L;
PropertyResolverUtils.updateProperty(sshd, FactoryManager.IDLE_TIMEOUT, testIdleTimeout);
- AtomicReference<TimeoutStatus> timeoutHolder = new AtomicReference<>();
+ AtomicReference<TimeoutIndicator> timeoutHolder = new AtomicReference<>(TimeoutIndicator.NONE);
CountDownLatch latch = new CountDownLatch(1);
TestEchoShell.latch = new CountDownLatch(1);
sshd.setSessionDisconnectHandler(new SessionDisconnectHandler() {
@Override
- public boolean handleTimeoutDisconnectReason(Session session, TimeoutStatus timeoutStatus)
+ public boolean handleTimeoutDisconnectReason(Session session, TimeoutIndicator timeoutStatus)
throws IOException {
outputDebugMessage("Session %s timeout reported: %s", session, timeoutStatus);
- TimeoutStatus prev = timeoutHolder.getAndSet(timeoutStatus);
- if (prev != null) {
+ TimeoutIndicator prev = timeoutHolder.getAndSet(timeoutStatus);
+ if (prev != TimeoutIndicator.NONE) {
throw new StreamCorruptedException("Multiple timeout disconnects: " + timeoutStatus + " / " + prev);
}
return false;
@@ -326,7 +329,9 @@ public class ServerTest extends BaseTestSupport {
assertTrue("Session latch not signalled in time", latch.await(1L, TimeUnit.SECONDS));
assertTrue("Shell latch not signalled in time", TestEchoShell.latch.await(1L, TimeUnit.SECONDS));
- assertSame("Mismatched timeout status", TimeoutStatus.IdleTimeout, timeoutHolder.getAndSet(null));
+
+ TimeoutIndicator status = timeoutHolder.getAndSet(null);
+ assertSame("Mismatched timeout status", TimeoutStatus.IdleTimeout, status.getStatus());
}
/*