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 2020/08/07 14:52:38 UTC
[mina-sshd] branch master updated (56087bb -> a966070)
This is an automated email from the ASF dual-hosted git repository.
lgoldstein pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git.
from 56087bb [SSHD-1004] Mark retired security settings as deprecated
new b848f76 [SSHD-1050] Fixed race condition in AuthFuture if exception caught before authentication started
new 3d28075 [SSHD-1050] Fix race conditions with early access to AuthFuture
new 4fc3e04 [SSHD-1050] Store only the 1st error that occurs before first authentication
new a966070 [SSHD-1050] Make first error before first auth attempt sticky
The 4 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
CHANGES.md | 1 +
.../sshd/client/session/ClientSessionImpl.java | 95 +++++++++++++++-------
.../sshd/client/session/ClientSessionTest.java | 73 +++++++++++++++--
3 files changed, 134 insertions(+), 35 deletions(-)
[mina-sshd] 03/04: [SSHD-1050] Store only the 1st error that occurs
before first authentication
Posted by lg...@apache.org.
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 4fc3e04743987e807087c555187ae1a5d6a1410c
Author: Lyor Goldstein <lg...@apache.org>
AuthorDate: Thu Aug 6 18:05:37 2020 +0300
[SSHD-1050] Store only the 1st error that occurs before first
authentication
---
.../sshd/client/session/ClientSessionImpl.java | 57 +++++++++-------------
1 file changed, 24 insertions(+), 33 deletions(-)
diff --git a/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java b/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
index 9bc68a9..a7c07c9 100644
--- a/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
+++ b/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
@@ -19,18 +19,17 @@
package org.apache.sshd.client.session;
import java.io.IOException;
-import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
import java.util.HashMap;
-import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
import org.apache.sshd.client.ClientFactoryManager;
import org.apache.sshd.client.future.AuthFuture;
@@ -60,11 +59,8 @@ public class ClientSessionImpl extends AbstractClientSession {
*/
private volatile AuthFuture authFuture;
- /** Records exceptions before there is an authFuture. */
- private List<Throwable> earlyErrors = new ArrayList<>();
-
- /** Guards setting an earlyError and the authFuture together. */
- private final Object errorLock = new Object();
+ /** Also guards setting an earlyError and the authFuture together. */
+ private final AtomicReference<Throwable> authErrorHolder = new AtomicReference<>();
/**
* For clients to store their own metadata
@@ -134,33 +130,27 @@ public class ClientSessionImpl extends AbstractClientSession {
ClientUserAuthService authService = getUserAuthService();
String serviceName = nextServiceName();
- List<Throwable> errors;
+ Throwable earlyError;
AuthFuture future;
// Guard both getting early errors and setting authFuture
- synchronized (errorLock) {
+ synchronized (authErrorHolder) {
future = ValidateUtils.checkNotNull(
authService.auth(serviceName), "No auth future generated by service=%s", serviceName);
- errors = earlyErrors;
- earlyErrors = null;
+ earlyError = authErrorHolder.getAndSet(null);
authFuture = future;
}
- if (errors != null && !errors.isEmpty()) {
- Iterator<Throwable> iter = errors.iterator();
- Throwable first = iter.next();
- iter.forEachRemaining(t -> {
- if (t != first && t != null) {
- first.addSuppressed(t);
- }
- });
- future.setException(first);
+
+ if (earlyError != null) {
+ future.setException(earlyError);
if (log.isDebugEnabled()) {
- log.debug("auth({}) early exception type={}: {}", //$NON-NLS-1$
- this, first.getClass().getSimpleName(),
- first.getMessage());
+ log.debug("auth({}) early exception type={}: {}",
+ this, earlyError.getClass().getSimpleName(),
+ earlyError.getMessage());
}
- // Or throw directly:
- // throw new IOException(first.getMessage(), first);
+ // TODO consider throw directly:
+ // throw new IOException(earlyError.getMessage(), earlyError);
}
+
return future;
}
@@ -184,22 +174,23 @@ public class ClientSessionImpl extends AbstractClientSession {
protected void signalAuthFailure(Throwable t) {
AuthFuture future = authFuture;
+ boolean firstError = false;
if (future == null) {
- synchronized (errorLock) {
- if (earlyErrors != null) {
- earlyErrors.add(t);
- }
+ synchronized (authErrorHolder) {
+ // save only the 1st newly signaled exception
+ firstError = authErrorHolder.compareAndSet(null, t);
future = authFuture;
}
}
+
if (future != null) {
future.setException(t);
}
+
if (log.isDebugEnabled()) {
- boolean signalled = future != null && t == future.getException();
- log.debug("signalAuthFailure({}) type={}, signalled={}: {}", this,
- t.getClass().getSimpleName(), Boolean.valueOf(signalled),
- t.getMessage());
+ boolean signalled = (future != null) && (t == future.getException());
+ log.debug("signalAuthFailure({}) type={}, signalled={}, first={}: {}",
+ this, t.getClass().getSimpleName(), signalled, firstError, t.getMessage());
}
}
[mina-sshd] 01/04: [SSHD-1050] Fixed race condition in AuthFuture
if exception caught before authentication started
Posted by lg...@apache.org.
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 b848f767cc132aa18ef42989ef979d6f7e7eb57e
Author: Lyor Goldstein <lg...@apache.org>
AuthorDate: Tue Aug 4 11:01:03 2020 +0300
[SSHD-1050] Fixed race condition in AuthFuture if exception caught before authentication started
---
CHANGES.md | 1 +
.../sshd/client/future/InitialAuthFuture.java | 52 +++++++++++++++++++
.../sshd/client/session/ClientSessionImpl.java | 16 ++++--
.../sshd/client/session/ClientSessionTest.java | 59 +++++++++++++++++++---
4 files changed, 119 insertions(+), 9 deletions(-)
diff --git a/CHANGES.md b/CHANGES.md
index 089fa2b..0dbba59 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -38,3 +38,4 @@ or `-key-file` command line option.
* [SSHD-1039](https://issues.apache.org/jira/browse/SSHD-1039) Fix support for some basic options in ssh/sshd cli.
* [SSHD-1047](https://issues.apache.org/jira/browse/SSHD-1047) Support for SSH jumps.
* [SSHD-1048](https://issues.apache.org/jira/browse/SSHD-1048) Wrap instead of rethrow IOException in Future.
+* [SSHD-1050](https://issues.apache.org/jira/browse/SSHD-1050) Fixed race condition in AuthFuture if exception caught before authentication started
\ No newline at end of file
diff --git a/sshd-core/src/main/java/org/apache/sshd/client/future/InitialAuthFuture.java b/sshd-core/src/main/java/org/apache/sshd/client/future/InitialAuthFuture.java
new file mode 100644
index 0000000..375fdb4
--- /dev/null
+++ b/sshd-core/src/main/java/org/apache/sshd/client/future/InitialAuthFuture.java
@@ -0,0 +1,52 @@
+/*
+ * 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.client.future;
+
+import org.apache.sshd.common.future.SshFutureListener;
+
+/**
+ * A special future used until the authentication service replaces it with a "real" one. This future serves as
+ * a placeholder for any exceptions caught until authentication starts. It is special in one other way - it has no
+ * listeners attached to it and expects no authentication success or failure - only exceptions.
+ *
+ * @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
+ */
+public class InitialAuthFuture extends DefaultAuthFuture {
+ public InitialAuthFuture(Object id, Object lock) {
+ super(id, lock);
+ }
+
+ @Override
+ public AuthFuture addListener(SshFutureListener<AuthFuture> listener) {
+ throw new UnsupportedOperationException("Not allowed to add listeners to this future");
+ }
+
+ @Override
+ protected void notifyListener(SshFutureListener<AuthFuture> l) {
+ if (l != null) {
+ throw new UnsupportedOperationException("No listeners expected for this future");
+ }
+ }
+
+ @Override
+ public void setAuthed(boolean authed) {
+ throw new UnsupportedOperationException("No authentication expected for this future");
+ }
+}
diff --git a/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java b/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
index 21e0631..bc8a9bd 100644
--- a/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
+++ b/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
@@ -32,7 +32,7 @@ import java.util.concurrent.TimeUnit;
import org.apache.sshd.client.ClientFactoryManager;
import org.apache.sshd.client.future.AuthFuture;
-import org.apache.sshd.client.future.DefaultAuthFuture;
+import org.apache.sshd.client.future.InitialAuthFuture;
import org.apache.sshd.common.Service;
import org.apache.sshd.common.ServiceFactory;
import org.apache.sshd.common.SshConstants;
@@ -85,8 +85,7 @@ public class ClientSessionImpl extends AbstractClientSession {
nextServiceFactory = null;
}
- authFuture = new DefaultAuthFuture(ioSession.getRemoteAddress(), futureLock);
- authFuture.setAuthed(false);
+ authFuture = new InitialAuthFuture(ioSession.getRemoteAddress(), futureLock);
signalSessionCreated(ioSession);
@@ -124,8 +123,19 @@ public class ClientSessionImpl extends AbstractClientSession {
ClientUserAuthService authService = getUserAuthService();
synchronized (sessionLock) {
String serviceName = nextServiceName();
+ // SSHD-1050 - check if need to propagate any previously caught exceptions
+ Throwable caught = (authFuture instanceof InitialAuthFuture) ? authFuture.getException() : null;
authFuture = ValidateUtils.checkNotNull(
authService.auth(serviceName), "No auth future generated by service=%s", serviceName);
+ if (caught != null) {
+ /*
+ * Safe to do under session lock despite SSHD-916 since
+ * the newly generated future has no associated listeners
+ * attached to it yet
+ */
+ signalAuthFailure(authFuture, caught);
+ }
+
return authFuture;
}
}
diff --git a/sshd-core/src/test/java/org/apache/sshd/client/session/ClientSessionTest.java b/sshd-core/src/test/java/org/apache/sshd/client/session/ClientSessionTest.java
index ed0addf..d9e022f 100644
--- a/sshd-core/src/test/java/org/apache/sshd/client/session/ClientSessionTest.java
+++ b/sshd-core/src/test/java/org/apache/sshd/client/session/ClientSessionTest.java
@@ -26,15 +26,21 @@ import java.rmi.ServerException;
import java.util.concurrent.atomic.AtomicInteger;
import org.apache.sshd.client.SshClient;
+import org.apache.sshd.client.future.AuthFuture;
import org.apache.sshd.common.AttributeRepository;
import org.apache.sshd.common.AttributeRepository.AttributeKey;
import org.apache.sshd.common.session.Session;
import org.apache.sshd.common.session.SessionListener;
+import org.apache.sshd.core.CoreModuleProperties;
import org.apache.sshd.server.SshServer;
+import org.apache.sshd.server.auth.keyboard.KeyboardInteractiveAuthenticator;
+import org.apache.sshd.server.auth.pubkey.AcceptAllPublickeyAuthenticator;
import org.apache.sshd.util.test.BaseTestSupport;
+import org.apache.sshd.util.test.BogusPasswordAuthenticator;
import org.apache.sshd.util.test.CommandExecutionHelper;
import org.apache.sshd.util.test.CoreTestSupportUtils;
import org.junit.AfterClass;
+import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.FixMethodOrder;
import org.junit.Test;
@@ -83,10 +89,17 @@ public class ClientSessionTest extends BaseTestSupport {
}
}
+ @Before
+ public void setUp() {
+ sshd.setPasswordAuthenticator(BogusPasswordAuthenticator.INSTANCE);
+ sshd.setPublickeyAuthenticator(AcceptAllPublickeyAuthenticator.INSTANCE);
+ sshd.setKeyboardInteractiveAuthenticator(KeyboardInteractiveAuthenticator.NONE);
+ }
+
@Test
public void testDefaultExecuteCommandMethod() throws Exception {
- final String expectedCommand = getCurrentTestName() + "-CMD";
- final String expectedResponse = getCurrentTestName() + "-RSP";
+ String expectedCommand = getCurrentTestName() + "-CMD";
+ String expectedResponse = getCurrentTestName() + "-RSP";
sshd.setCommandFactory((session, command) -> new CommandExecutionHelper(command) {
private boolean cmdProcessed;
@@ -116,8 +129,8 @@ public class ClientSessionTest extends BaseTestSupport {
@Test
public void testExceptionThrownIfRemoteStderrWrittenTo() throws Exception {
- final String expectedCommand = getCurrentTestName() + "-CMD";
- final String expectedErrorMessage = getCurrentTestName() + "-ERR";
+ String expectedCommand = getCurrentTestName() + "-CMD";
+ String expectedErrorMessage = getCurrentTestName() + "-ERR";
sshd.setCommandFactory((session, command) -> new CommandExecutionHelper(command) {
private boolean cmdProcessed;
@@ -161,8 +174,8 @@ public class ClientSessionTest extends BaseTestSupport {
@Test
public void testExceptionThrownIfNonZeroExitStatus() throws Exception {
- final String expectedCommand = getCurrentTestName() + "-CMD";
- final int expectedErrorCode = 7365;
+ String expectedCommand = getCurrentTestName() + "-CMD";
+ int expectedErrorCode = 7365;
sshd.setCommandFactory((session, command) -> new CommandExecutionHelper(command) {
private boolean cmdProcessed;
@@ -237,4 +250,38 @@ public class ClientSessionTest extends BaseTestSupport {
client.removeSessionListener(listener);
}
}
+
+ @Test // SSHD-1050
+ public void testAuthGetsNotifiedOnLongPreamble() throws Exception {
+ int limit = CoreModuleProperties.MAX_IDENTIFICATION_SIZE.getRequired(sshd);
+ String line = getClass().getCanonicalName() + "#" + getCurrentTestName();
+ StringBuilder sb = new StringBuilder(limit + line.length());
+ while (sb.length() <= limit) {
+ if (sb.length() > 0) {
+ sb.append(CoreModuleProperties.SERVER_EXTRA_IDENT_LINES_SEPARATOR);
+ }
+ sb.append(line);
+ }
+ CoreModuleProperties.SERVER_EXTRA_IDENTIFICATION_LINES.set(sshd, sb.toString());
+
+ try (ClientSession session = client.connect(getCurrentTestName(), TEST_LOCALHOST, port)
+ .verify(CONNECT_TIMEOUT)
+ .getSession()) {
+ session.addPasswordIdentity(getCurrentTestName());
+ // Give time to the client to signal the overflow in server identification
+ Thread.sleep(AUTH_TIMEOUT.toMillis() / 2L);
+
+ AuthFuture future = session.auth();
+ assertTrue("Auth not completed on time", future.await(AUTH_TIMEOUT));
+ assertTrue("No auth result", future.isDone());
+ assertFalse("Unexpected auth success", future.isSuccess());
+ assertTrue("Auth not marked as failed", future.isFailure());
+
+ Throwable exception = future.getException();
+ String message = exception.getLocalizedMessage();
+ assertTrue("Invalid exception message: " + message, message.contains("too many header lines"));
+ } finally {
+ CoreModuleProperties.SERVER_EXTRA_IDENTIFICATION_LINES.set(sshd, null);
+ }
+ }
}
[mina-sshd] 02/04: [SSHD-1050] Fix race conditions with early
access to AuthFuture
Posted by lg...@apache.org.
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 3d280750c6dd817c63948d4c10a4ad991d901eb3
Author: Thomas Wolf <th...@paranor.ch>
AuthorDate: Tue Aug 4 21:04:59 2020 +0200
[SSHD-1050] Fix race conditions with early access to AuthFuture
Make ClientSessionImpl.authFuture volatile since it may be accessed by
different threads without synchronization.
Remove the initial dummy AuthFuture that served only to record an early
exception, and to provide a correct session state in waitFor() when KEX
was done but auth() not called yet.
Record exceptions from signalAuthFailure() in a list if there's no
AuthFuture yet. Introduce a lock for guarding the authFuture field and
this new list of exceptions together.
The access in updateCurrentSessionState() must remain unsynchronized
because it occurs while the futureLock is held. Synchronizing there
on errorLock might introduce a lock inversion between that access in
updateCurrentSessionState() (lock order futureLock-errorLock), but
the call to authService.auth() inside ClientSessionImpl.auth() has the
potential to also access the futureLock is there was a previous future,
which might give the lock order errorLock-futureLock.
Listeners futures are always called outside the future's lock, so even
if a listener calls auth() or signalAuthFailure(), this will
not cause a lock inversion.
---
.../sshd/client/future/InitialAuthFuture.java | 52 -----------
.../sshd/client/session/ClientSessionImpl.java | 103 +++++++++++++--------
2 files changed, 64 insertions(+), 91 deletions(-)
diff --git a/sshd-core/src/main/java/org/apache/sshd/client/future/InitialAuthFuture.java b/sshd-core/src/main/java/org/apache/sshd/client/future/InitialAuthFuture.java
deleted file mode 100644
index 375fdb4..0000000
--- a/sshd-core/src/main/java/org/apache/sshd/client/future/InitialAuthFuture.java
+++ /dev/null
@@ -1,52 +0,0 @@
-/*
- * 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.client.future;
-
-import org.apache.sshd.common.future.SshFutureListener;
-
-/**
- * A special future used until the authentication service replaces it with a "real" one. This future serves as
- * a placeholder for any exceptions caught until authentication starts. It is special in one other way - it has no
- * listeners attached to it and expects no authentication success or failure - only exceptions.
- *
- * @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
- */
-public class InitialAuthFuture extends DefaultAuthFuture {
- public InitialAuthFuture(Object id, Object lock) {
- super(id, lock);
- }
-
- @Override
- public AuthFuture addListener(SshFutureListener<AuthFuture> listener) {
- throw new UnsupportedOperationException("Not allowed to add listeners to this future");
- }
-
- @Override
- protected void notifyListener(SshFutureListener<AuthFuture> l) {
- if (l != null) {
- throw new UnsupportedOperationException("No listeners expected for this future");
- }
- }
-
- @Override
- public void setAuthed(boolean authed) {
- throw new UnsupportedOperationException("No authentication expected for this future");
- }
-}
diff --git a/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java b/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
index bc8a9bd..9bc68a9 100644
--- a/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
+++ b/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
@@ -19,11 +19,13 @@
package org.apache.sshd.client.session;
import java.io.IOException;
+import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
import java.util.HashMap;
+import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
@@ -32,7 +34,6 @@ import java.util.concurrent.TimeUnit;
import org.apache.sshd.client.ClientFactoryManager;
import org.apache.sshd.client.future.AuthFuture;
-import org.apache.sshd.client.future.InitialAuthFuture;
import org.apache.sshd.common.Service;
import org.apache.sshd.common.ServiceFactory;
import org.apache.sshd.common.SshConstants;
@@ -50,7 +51,20 @@ import org.apache.sshd.common.util.buffer.Buffer;
* @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
*/
public class ClientSessionImpl extends AbstractClientSession {
- private AuthFuture authFuture;
+
+ /**
+ * The authentication future created by the last call to {@link #auth()}; {@code null} before the first call to
+ * {@link #auth()}.
+ *
+ * Volatile because of unsynchronized access in {@link #updateCurrentSessionState(Collection)}.
+ */
+ private volatile AuthFuture authFuture;
+
+ /** Records exceptions before there is an authFuture. */
+ private List<Throwable> earlyErrors = new ArrayList<>();
+
+ /** Guards setting an earlyError and the authFuture together. */
+ private final Object errorLock = new Object();
/**
* For clients to store their own metadata
@@ -85,8 +99,6 @@ public class ClientSessionImpl extends AbstractClientSession {
nextServiceFactory = null;
}
- authFuture = new InitialAuthFuture(ioSession.getRemoteAddress(), futureLock);
-
signalSessionCreated(ioSession);
/*
@@ -121,63 +133,73 @@ public class ClientSessionImpl extends AbstractClientSession {
}
ClientUserAuthService authService = getUserAuthService();
- synchronized (sessionLock) {
- String serviceName = nextServiceName();
- // SSHD-1050 - check if need to propagate any previously caught exceptions
- Throwable caught = (authFuture instanceof InitialAuthFuture) ? authFuture.getException() : null;
- authFuture = ValidateUtils.checkNotNull(
+ String serviceName = nextServiceName();
+ List<Throwable> errors;
+ AuthFuture future;
+ // Guard both getting early errors and setting authFuture
+ synchronized (errorLock) {
+ future = ValidateUtils.checkNotNull(
authService.auth(serviceName), "No auth future generated by service=%s", serviceName);
- if (caught != null) {
- /*
- * Safe to do under session lock despite SSHD-916 since
- * the newly generated future has no associated listeners
- * attached to it yet
- */
- signalAuthFailure(authFuture, caught);
+ errors = earlyErrors;
+ earlyErrors = null;
+ authFuture = future;
+ }
+ if (errors != null && !errors.isEmpty()) {
+ Iterator<Throwable> iter = errors.iterator();
+ Throwable first = iter.next();
+ iter.forEachRemaining(t -> {
+ if (t != first && t != null) {
+ first.addSuppressed(t);
+ }
+ });
+ future.setException(first);
+ if (log.isDebugEnabled()) {
+ log.debug("auth({}) early exception type={}: {}", //$NON-NLS-1$
+ this, first.getClass().getSimpleName(),
+ first.getMessage());
}
-
- return authFuture;
+ // Or throw directly:
+ // throw new IOException(first.getMessage(), first);
}
+ return future;
}
@Override
public void exceptionCaught(Throwable t) {
- signalAuthFailure(authFuture, t);
+ signalAuthFailure(t);
super.exceptionCaught(t);
}
@Override
protected void preClose() {
- signalAuthFailure(authFuture, new SshException("Session is being closed"));
+ signalAuthFailure(new SshException("Session is being closed"));
super.preClose();
}
@Override
protected void handleDisconnect(int code, String msg, String lang, Buffer buffer) throws Exception {
- signalAuthFailure(authFuture, new SshException(code, msg));
+ signalAuthFailure(new SshException(code, msg));
super.handleDisconnect(code, msg, lang, buffer);
}
- protected void signalAuthFailure(AuthFuture future, Throwable t) {
- boolean signalled = false;
- if (future != null) {
- /*
- * SSHD-916 - do not synchronize on the session lock since the result of setting an exception may be an
- * exchange of messages due to one of the registered listeners. If this is the case then a deadlock will
- * occur since the session's message handling loop also tries to lock the session lock - but on another
- * thread.
- */
- synchronized (future) {
- if (!future.isDone()) {
- future.setException(t);
- signalled = true;
+ protected void signalAuthFailure(Throwable t) {
+ AuthFuture future = authFuture;
+ if (future == null) {
+ synchronized (errorLock) {
+ if (earlyErrors != null) {
+ earlyErrors.add(t);
}
+ future = authFuture;
}
}
-
+ if (future != null) {
+ future.setException(t);
+ }
if (log.isDebugEnabled()) {
- log.debug("signalAuthFailure({}) type={}, signalled={}: {}",
- this, t.getClass().getSimpleName(), signalled, t.getMessage());
+ boolean signalled = future != null && t == future.getException();
+ log.debug("signalAuthFailure({}) type={}, signalled={}: {}", this,
+ t.getClass().getSimpleName(), Boolean.valueOf(signalled),
+ t.getMessage());
}
}
@@ -319,8 +341,11 @@ public class ClientSessionImpl extends AbstractClientSession {
if (isAuthenticated()) { // authFuture.isSuccess()
state.add(ClientSessionEvent.AUTHED);
}
- if (KexState.DONE.equals(kexState.get()) && authFuture.isFailure()) {
- state.add(ClientSessionEvent.WAIT_AUTH);
+ if (KexState.DONE.equals(kexState.get())) {
+ AuthFuture future = authFuture;
+ if (future == null || future.isFailure()) {
+ state.add(ClientSessionEvent.WAIT_AUTH);
+ }
}
return state;
[mina-sshd] 04/04: [SSHD-1050] Make first error before first auth
attempt sticky
Posted by lg...@apache.org.
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 a966070d8b793a00f0e479d984f2e431fcee229c
Author: Lyor Goldstein <lg...@apache.org>
AuthorDate: Thu Aug 6 18:33:54 2020 +0300
[SSHD-1050] Make first error before first auth attempt sticky
---
.../sshd/client/session/ClientSessionImpl.java | 13 ++++++++-
.../sshd/client/session/ClientSessionTest.java | 32 ++++++++++++++++------
2 files changed, 35 insertions(+), 10 deletions(-)
diff --git a/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java b/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
index a7c07c9..72c41ae 100644
--- a/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
+++ b/sshd-core/src/main/java/org/apache/sshd/client/session/ClientSessionImpl.java
@@ -59,6 +59,7 @@ public class ClientSessionImpl extends AbstractClientSession {
*/
private volatile AuthFuture authFuture;
+ private final AtomicReference<Throwable> beforeAuthErrorHolder = new AtomicReference<>();
/** Also guards setting an earlyError and the authFuture together. */
private final AtomicReference<Throwable> authErrorHolder = new AtomicReference<>();
@@ -136,7 +137,13 @@ public class ClientSessionImpl extends AbstractClientSession {
synchronized (authErrorHolder) {
future = ValidateUtils.checkNotNull(
authService.auth(serviceName), "No auth future generated by service=%s", serviceName);
- earlyError = authErrorHolder.getAndSet(null);
+ // If have an error before the 1st auth then make it "sticky"
+ Throwable beforeAuthError = beforeAuthErrorHolder.get();
+ if (authFuture != null) {
+ earlyError = authErrorHolder.getAndSet(beforeAuthError);
+ } else {
+ earlyError = beforeAuthError;
+ }
authFuture = future;
}
@@ -180,6 +187,10 @@ public class ClientSessionImpl extends AbstractClientSession {
// save only the 1st newly signaled exception
firstError = authErrorHolder.compareAndSet(null, t);
future = authFuture;
+ // save in special location errors before the 1st auth attempt
+ if (future == null) {
+ beforeAuthErrorHolder.compareAndSet(null, t);
+ }
}
}
diff --git a/sshd-core/src/test/java/org/apache/sshd/client/session/ClientSessionTest.java b/sshd-core/src/test/java/org/apache/sshd/client/session/ClientSessionTest.java
index d9e022f..d6c4cf2 100644
--- a/sshd-core/src/test/java/org/apache/sshd/client/session/ClientSessionTest.java
+++ b/sshd-core/src/test/java/org/apache/sshd/client/session/ClientSessionTest.java
@@ -252,7 +252,16 @@ public class ClientSessionTest extends BaseTestSupport {
}
@Test // SSHD-1050
- public void testAuthGetsNotifiedOnLongPreamble() throws Exception {
+ public void testAuthGetsNotifiedIfErrorBeforeFirstAuth() throws Exception {
+ testEarlyErrorAuthAttempts(1);
+ }
+
+ @Test // SSHD-1050
+ public void testSecondAuthNotifiedAfterEarlyError() throws Exception {
+ testEarlyErrorAuthAttempts(3);
+ }
+
+ private void testEarlyErrorAuthAttempts(int maxAttempts) throws Exception {
int limit = CoreModuleProperties.MAX_IDENTIFICATION_SIZE.getRequired(sshd);
String line = getClass().getCanonicalName() + "#" + getCurrentTestName();
StringBuilder sb = new StringBuilder(limit + line.length());
@@ -271,15 +280,20 @@ public class ClientSessionTest extends BaseTestSupport {
// Give time to the client to signal the overflow in server identification
Thread.sleep(AUTH_TIMEOUT.toMillis() / 2L);
- AuthFuture future = session.auth();
- assertTrue("Auth not completed on time", future.await(AUTH_TIMEOUT));
- assertTrue("No auth result", future.isDone());
- assertFalse("Unexpected auth success", future.isSuccess());
- assertTrue("Auth not marked as failed", future.isFailure());
+ for (int index = 1; index <= maxAttempts; index++) {
+ String authId = "Auth " + index + "/" + maxAttempts;
+ outputDebugMessage("%s(%s)", getCurrentTestName(), authId);
+
+ AuthFuture future = session.auth();
+ assertTrue(authId + " not completed on time", future.await(AUTH_TIMEOUT));
+ assertTrue(authId + " has no result", future.isDone());
+ assertFalse(authId + " unexpected success", future.isSuccess());
+ assertTrue(authId + " not marked as failed", future.isFailure());
- Throwable exception = future.getException();
- String message = exception.getLocalizedMessage();
- assertTrue("Invalid exception message: " + message, message.contains("too many header lines"));
+ Throwable exception = future.getException();
+ String message = exception.getMessage();
+ assertTrue(authId + " invalid exception message: " + message, message.contains("too many header lines"));
+ }
} finally {
CoreModuleProperties.SERVER_EXTRA_IDENTIFICATION_LINES.set(sshd, null);
}