Skip to content

Commit

Permalink
apacheGH-533: Fix ClientUserAuthService iteration through methods
Browse files Browse the repository at this point in the history 
The ClientUserAuthService iterates through the configured authentication
methods and tries them each in order, skipping those that the server did
not list as acceptable.

However, it only set the "server acceptable" on a partial success, or on
the initial failure for the "none" request. This was wrong: if a pubkey
auth fails and the server replies with SSH_MSG_USERAUTH_FAILURE and an
acceptable methods list not containing "pubkey", then pubkey should not
continue even if it had more keys available (or more signatures to try
for an RSA key).

This case actually occurs, for instance with an XFB.Gateway SSH server
and RSA keys, if an rsa-sha2* signature is used. That server apparently
knows only ssh-rsa signatures and consequently may reply with
SSH_MSG_USERAUTH_FAILURE, partialSuccess=false, allowed=password.
(Presumably if the key was right but the signature was unknown, and the
user has only a single authorized key, and password auth is enabled
server-side.)

So reset the "server acceptable" list on any SSH_MSG_USERAUTH_FAILURE,
even if partialSuccess = false. Check whether the current method is
still allowed, and if not start over instead of continuing with the
current method.

A second problem was with the handling of multiple required
authentication methods. A server may require multiple authentications,
for instance first a pubkey auth, then a password auth, and then again
a pubkey auth with another key. Such continuations are indicated by
SSH_MSG_USERAUTH_FAILURE with partialSuccess = true and a list of
methods to try next. For pubkey,password,pubkey the client would get a
message SSH_MSG_USERAUTH_FAILURE partialSuccess=true allowed=password
after the successful first pubkey authentication, and then a message
SSH_MSG_USERAUTH_FAILURE partialSuccess=true allowed=pubkey after the
successful password log-in. On that second use of pubkey auth, the
previously successful key must not be re-tried, and there's also no
point in re-trying previously rejected keys. Thus our code cannot simply
create a new UserAuthPublicKey instance again as this would re-load the
key iterator from scratch. Instead we must remember the pubkey auth
instance once created and re-use that same instance on the second use,
so that we keep on with any remaining keys that might be available.

See also https://issues.apache.org/jira/browse/SSHD-1229, which may be
related.

Bug: apache#533
  • Loading branch information
@tomaswolf
tomaswolf committed Jul 29, 2024
1 parent 0b96a45 commit 4b6f809
Show file tree
Hide file tree
Showing 4 changed files with 462 additions and 39 deletions.
1 change: 1 addition & 0 deletions CHANGES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
## Bug Fixes

* [GH-524](https://github.com/apache/mina-sshd/issues/524) Performance improvements
* [GH-533](https://github.com/apache/mina-sshd/issues/533) Fix multi-step authentication

## New Features

Expand Down
28 changes: 26 additions & 2 deletions sshd-core/src/main/java/org/apache/sshd/client/auth/pubkey/UserAuthPublicKey.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -473,19 +473,43 @@ protected byte[] appendSignature(

@Override
public void signalAuthMethodSuccess(ClientSession session, String service, Buffer buffer) throws Exception {
PublicKeyIdentity successfulKey = current;
KeyPair identity = (successfulKey == null) ? null : successfulKey.getKeyIdentity();
current = null;
PublicKeyAuthenticationReporter reporter = session.getPublicKeyAuthenticationReporter();
if (reporter != null) {
reporter.signalAuthenticationSuccess(session, service, (current == null) ? null : current.getKeyIdentity());
reporter.signalAuthenticationSuccess(session, service, identity);
}
}

@Override
public void signalAuthMethodFailure(
ClientSession session, String service, boolean partial, List<String> serverMethods, Buffer buffer)
throws Exception {
PublicKeyIdentity keyUsed = current;
if (partial) {
// Actually a pubkey success, but we must continue with either this or another authentication method.
//
// Prevent re-use of this key if this instance of UserAuthPublicKey is used again. See OpenBSD sshd_config,
// AuthenticationMethods: "If the publickey method is listed more than once, sshd(8) verifies that keys
// that have been used successfully are not reused for subsequent authentications. For example,
// "publickey,publickey" requires successful authentication using two different public keys."
//
// https://man.openbsd.org/sshd_config#AuthenticationMethods
//
// If the successful key was an RSA key, and we succeeded with an rsa-sha2-512 signature, we might otherwise
// re-try that same key with an rsa-sha2-256 and ssh-rsa signature, which would be wrong. We have to
// continue with the next available key from the iterator.
//
// Note that if a server imposes an order on the keys used in such a case (say, it requires successful
// pubkey authentication first with key A, then with key B), it is the user's responsibility to ensure that
// the iterator has the keys in that order, for instance by specifying them in that order in "IdentityFile"
// directives in the host entry in the client-side ~/.ssh/config.
current = null;
}
PublicKeyAuthenticationReporter reporter = session.getPublicKeyAuthenticationReporter();
if (reporter != null) {
KeyPair identity = (current == null) ? null : current.getKeyIdentity();
KeyPair identity = (keyUsed == null) ? null : keyUsed.getKeyIdentity();
reporter.signalAuthenticationFailure(session, service, identity, partial, serverMethods);
}
}
Expand Down
153 changes: 116 additions & 37 deletions sshd-core/src/main/java/org/apache/sshd/client/session/ClientUserAuthService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.apache.sshd.client.auth.UserAuth;
import org.apache.sshd.client.auth.UserAuthFactory;
import org.apache.sshd.client.auth.keyboard.UserInteraction;
import org.apache.sshd.client.auth.pubkey.UserAuthPublicKey;
import org.apache.sshd.client.future.AuthFuture;
import org.apache.sshd.client.future.DefaultAuthFuture;
import org.apache.sshd.common.NamedResource;
Expand Down Expand Up @@ -66,8 +67,9 @@ public class ClientUserAuthService extends AbstractCloseable implements Service,
private final Map<String, Object> properties = new ConcurrentHashMap<>();

private String service;
private UserAuth userAuth;
private UserAuth currentUserAuth;
private int currentMethod;
private UserAuth pubkeyAuth;

private final Object initLock = new Object();
private boolean started;
Expand Down Expand Up @@ -150,6 +152,7 @@ public AuthFuture auth(String service) throws IOException {

// start from scratch
serverMethods = null;
pubkeyAuth = null;
currentMethod = 0;
clearUserAuth();

Expand Down Expand Up @@ -284,15 +287,17 @@ protected void processUserAuth(int cmd, Buffer buffer, AuthFuture authFuture) th
if (cmd == SshConstants.SSH_MSG_USERAUTH_SUCCESS) {
if (log.isDebugEnabled()) {
log.debug("processUserAuth({}) SSH_MSG_USERAUTH_SUCCESS Succeeded with {}",
session, (userAuth == null) ? "<unknown>" : userAuth.getName());
session, (currentUserAuth == null) ? "<unknown>" : currentUserAuth.getName());
}

if (userAuth != null) {
if (currentUserAuth != null) {
try {
userAuth.signalAuthMethodSuccess(session, service, buffer);
currentUserAuth.signalAuthMethodSuccess(session, service, buffer);
} finally {
clearUserAuth();
}
} else {
destroyPubkeyAuth();
}
session.setAuthenticated();
((ClientSessionImpl) session).switchToNextService();
Expand All @@ -309,65 +314,111 @@ protected void processUserAuth(int cmd, Buffer buffer, AuthFuture authFuture) th
return;
}
if (cmd == SshConstants.SSH_MSG_USERAUTH_FAILURE) {
String mths = buffer.getString();
String methods = buffer.getString();
boolean partial = buffer.getBoolean();
if (log.isDebugEnabled()) {
log.debug("processUserAuth({}) Received SSH_MSG_USERAUTH_FAILURE - partial={}, methods={}",
session, partial, mths);
session, partial, methods);
}
if (partial || (serverMethods == null)) {
serverMethods = Arrays.asList(GenericUtils.split(mths, ','));
currentMethod = 0;
if (userAuth != null) {
try {
userAuth.signalAuthMethodFailure(session, service, partial, Collections.unmodifiableList(serverMethods),
buffer);
} finally {
clearUserAuth();
List<String> allowedMethods;
if (GenericUtils.isEmpty(methods)) {
if (serverMethods == null) {
// RFC 4252 section 5.2 says that in the SSH_MSG_USERAUTH_FAILURE response
// to a 'none' request a server MAY return a list of methods. Here it didn't,
// so we just assume all methods that the client knows are fine.
//
// https://datatracker.ietf.org/doc/html/rfc4252#section-5.2
allowedMethods = new ArrayList<>(clientMethods);
} else if (partial) {
// Don't reset to an empty list; keep going with the previous methods. Sending
// a partial success without methods that may continue makes no sense and would
// be a server bug.
//
// currentUserAuth should always be set here!
if (log.isDebugEnabled()) {
log.debug(
"processUserAuth({}) : potential bug in {} server: SSH_MSG_USERAUTH_FAILURE with partial success after {} authentication, but without continuation methods",
session, session.getServerVersion(),
currentUserAuth != null ? currentUserAuth.getName() : "UNKNOWN");
}
allowedMethods = serverMethods;
} else {
allowedMethods = new ArrayList<>();
}
} else {
allowedMethods = Arrays.asList(GenericUtils.split(methods, ','));
}
if (currentUserAuth != null) {
try {
currentUserAuth.signalAuthMethodFailure(session, service, partial,
Collections.unmodifiableList(allowedMethods), buffer);
} catch (Exception e) {
clearUserAuth();
throw e;
}

// Check if the current method is still allowed.
if (allowedMethods.indexOf(currentUserAuth.getName()) < 0) {
if (currentUserAuth == pubkeyAuth) {
// Don't destroy it yet, we might still need it later on
currentUserAuth = null;
} else {
destroyUserAuth();
}
}
}
if (partial || (serverMethods == null)) {
currentMethod = 0;
}
serverMethods = allowedMethods;

tryNext(cmd, authFuture);
return;
}

if (userAuth == null) {
if (currentUserAuth == null) {
throw new IllegalStateException("Received unknown packet: " + SshConstants.getCommandMessageName(cmd));
}

if (log.isDebugEnabled()) {
log.debug("processUserAuth({}) delegate processing of {} to {}",
session, SshConstants.getCommandMessageName(cmd), userAuth.getName());
session, SshConstants.getCommandMessageName(cmd), currentUserAuth.getName());
}

buffer.rpos(buffer.rpos() - 1);
if (!userAuth.process(buffer)) {
if (!currentUserAuth.process(buffer)) {
tryNext(cmd, authFuture);
} else {
authFuture.setCancellable(userAuth.isCancellable());
authFuture.setCancellable(currentUserAuth.isCancellable());
}
}

protected void tryNext(int cmd, AuthFuture authFuture) throws Exception {
ClientSession session = getClientSession();
// Loop until we find something to try
for (boolean debugEnabled = log.isDebugEnabled();; debugEnabled = log.isDebugEnabled()) {
if (userAuth == null) {
if (currentUserAuth == null) {
if (debugEnabled) {
log.debug("tryNext({}) starting authentication mechanisms: client={}, server={}",
session, clientMethods, serverMethods);
log.debug("tryNext({}) starting authentication mechanisms: client={}, client index={}, server={}", session,
clientMethods, currentMethod, serverMethods);
}
} else if (!userAuth.process(null)) {
} else if (!currentUserAuth.process(null)) {
if (debugEnabled) {
log.debug("tryNext({}) no initial request sent by method={}", session, userAuth.getName());
log.debug("tryNext({}) no initial request sent by method={}", session, currentUserAuth.getName());
}
if (currentUserAuth == pubkeyAuth) {
// Don't destroy it yet. It might re-appear later if the server requires multiple methods.
// It doesn't have any more keys, but we don't want to re-create it from scratch and re-try
// all the keys already tried again.
currentUserAuth = null;
} else {
destroyUserAuth();
}
clearUserAuth();
currentMethod++;
} else {
if (debugEnabled) {
log.debug("tryNext({}) successfully processed initial buffer by method={}",
session, userAuth.getName());
session, currentUserAuth.getName());
}
return;
}
Expand All @@ -385,7 +436,7 @@ protected void tryNext(int cmd, AuthFuture authFuture) throws Exception {
log.debug("tryNext({}) exhausted all methods - client={}, server={}",
session, clientMethods, serverMethods);
}

clearUserAuth();
// also wake up anyone sitting in waitFor
authFuture.setException(new SshException(SshConstants.SSH2_DISCONNECT_NO_MORE_AUTH_METHODS_AVAILABLE,
"No more authentication methods available"));
Expand All @@ -398,30 +449,58 @@ protected void tryNext(int cmd, AuthFuture authFuture) throws Exception {
clearUserAuth();
return;
}
userAuth = UserAuthMethodFactory.createUserAuth(session, authFactories, method);
if (userAuth == null) {
throw new UnsupportedOperationException("Failed to find a user-auth factory for method=" + method);
if (UserAuthPublicKey.NAME.equals(method) && pubkeyAuth != null) {
currentUserAuth = pubkeyAuth;
} else {
currentUserAuth = UserAuthMethodFactory.createUserAuth(session, authFactories, method);
if (currentUserAuth == null) {
throw new UnsupportedOperationException("Failed to find a user-auth factory for method=" + method);
}
}

if (debugEnabled) {
log.debug("tryNext({}) attempting method={}", session, method);
}

userAuth.init(session, service);
authFuture.setCancellable(userAuth.isCancellable());
if (currentUserAuth != pubkeyAuth) {
currentUserAuth.init(session, service);
}
if (UserAuthPublicKey.NAME.equals(currentUserAuth.getName())) {
pubkeyAuth = currentUserAuth;
}
authFuture.setCancellable(currentUserAuth.isCancellable());
if (authFuture.isCanceled()) {
authFuture.getCancellation().setCanceled();
clearUserAuth();
return;
}
}
}

private void clearUserAuth() {
if (userAuth != null) {
if (currentUserAuth == pubkeyAuth) {
pubkeyAuth = null;
destroyUserAuth();
} else {
destroyUserAuth();
destroyPubkeyAuth();
}
}

private void destroyUserAuth() {
if (currentUserAuth != null) {
try {
currentUserAuth.destroy();
} finally {
currentUserAuth = null;
}
}
}

private void destroyPubkeyAuth() {
if (pubkeyAuth != null) {
try {
userAuth.destroy();
pubkeyAuth.destroy();
} finally {
userAuth = null;
pubkeyAuth = null;
}
}
}
Expand Down
Loading

0 comments on commit 4b6f809

Please sign in to comment.