Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-533: Fix ClientUserAuthService iteration through methods #547

Merged
merged 1 commit into from
Jul 29, 2024
Merged

GH-533: Fix ClientUserAuthService iteration through methods #547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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