sshd: implement ssh config PubkeyAcceptedAlgorithms

Apache MINA sshd 2.6.0 appears to use only the first appropriate
public key signature algorithm for a particular key. See [1]. For
RSA keys, that is rsa-sha2-512. This breaks authentication at servers
that only know the older (and deprecated) ssh-rsa algorithm.

With PubkeyAcceptedAlgorithms, users can re-order algorithms in
the ssh config file per host, if needed. Setting

  PubkeyAcceptedAlgorithms ^ssh-rsa

will put "ssh-rsa" at the front of the list of algorithms, and then
authentication at such servers with RSA keys works again.

[1] https://issues.apache.org/jira/browse/SSHD-1105

Bug: 572056
Change-Id: I86c3b93f05960c68936e80642965815926bb2532
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

4 files changed, 90 insertions, 41 deletions

diff --git a/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties b/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties
index f810fd40e4..16b5738332 100644
--- a/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties
+++ b/org.eclipse.jgit.ssh.apache/resources/org/eclipse/jgit/internal/transport/sshd/SshdText.properties
@@ -5,8 +5,7 @@ configInvalidPath=Invalid path in ssh config key {0}: {1}
 configInvalidPattern=Invalid pattern in ssh config key {0}: {1}
 configInvalidPositive=Ssh config entry {0} must be a strictly positive number but is ''{1}''
 configInvalidProxyJump=Ssh config, host ''{0}'': Cannot parse ProxyJump ''{1}''
-configNoKnownHostKeyAlgorithms=No implementations for any of the algorithms ''{0}'' given in HostKeyAlgorithms in the ssh config; using the default.
-configNoRemainingHostKeyAlgorithms=Ssh config removed all host key algorithms: HostKeyAlgorithms ''{0}''
+configNoKnownAlgorithms=Ssh config ''{0}'' ''{1}'' resulted in empty list (none known, or all known removed); using default.
 configProxyJumpNotSsh=Non-ssh URI in ProxyJump ssh config
 configProxyJumpWithPath=ProxyJump ssh config: jump host specification must not have a path
 ftpCloseFailed=Closing the SFTP channel failed
diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java
index 66713ba632..8183a92b9f 100644
--- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java
+++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitClientSession.java
@@ -21,6 +21,7 @@ import java.security.PublicKey;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -45,6 +46,7 @@ import org.eclipse.jgit.fnmatch.FileNameMatcher;
 import org.eclipse.jgit.internal.transport.sshd.proxy.StatefulProxyConnector;
 import org.eclipse.jgit.transport.CredentialsProvider;
 import org.eclipse.jgit.transport.SshConstants;
+import org.eclipse.jgit.util.StringUtils;
 
 /**
  * A {@link org.apache.sshd.client.session.ClientSession ClientSession} that can
@@ -201,48 +203,23 @@ public class JGitClientSession extends ClientSessionImpl {
 	@Override
 	protected String resolveAvailableSignaturesProposal(
 			FactoryManager manager) {
-		Set<String> defaultSignatures = new LinkedHashSet<>();
-		defaultSignatures.addAll(getSignatureFactoriesNames());
+		List<String> defaultSignatures = getSignatureFactoriesNames();
 		HostConfigEntry config = resolveAttribute(
 				JGitSshClient.HOST_CONFIG_ENTRY);
-		String hostKeyAlgorithms = config
+		String algorithms = config
 				.getProperty(SshConstants.HOST_KEY_ALGORITHMS);
-		if (hostKeyAlgorithms != null && !hostKeyAlgorithms.isEmpty()) {
-			char first = hostKeyAlgorithms.charAt(0);
-			switch (first) {
-			case '+':
-				// Additions make not much sense -- it's either in
-				// defaultSignatures already, or we have no implementation for
-				// it. No point in proposing it.
-				return String.join(",", defaultSignatures); //$NON-NLS-1$
-			case '-':
-				// This takes wildcard patterns!
-				removeFromList(defaultSignatures,
-						SshConstants.HOST_KEY_ALGORITHMS,
-						hostKeyAlgorithms.substring(1));
-				if (defaultSignatures.isEmpty()) {
-					// Too bad: user config error. Warn here, and then fail
-					// later.
-					log.warn(format(
-							SshdText.get().configNoRemainingHostKeyAlgorithms,
-							hostKeyAlgorithms));
-				}
-				return String.join(",", defaultSignatures); //$NON-NLS-1$
-			default:
-				// Default is overridden -- only accept the ones for which we do
-				// have an implementation.
-				List<String> newNames = filteredList(defaultSignatures,
-						hostKeyAlgorithms);
-				if (newNames.isEmpty()) {
-					log.warn(format(
-							SshdText.get().configNoKnownHostKeyAlgorithms,
-							hostKeyAlgorithms));
-					// Use the default instead.
-				} else {
-					return String.join(",", newNames); //$NON-NLS-1$
+		if (!StringUtils.isEmptyOrNull(algorithms)) {
+			List<String> result = modifyAlgorithmList(defaultSignatures,
+					algorithms, SshConstants.HOST_KEY_ALGORITHMS);
+			if (!result.isEmpty()) {
+				if (log.isDebugEnabled()) {
+					log.debug(SshConstants.HOST_KEY_ALGORITHMS + ' ' + result);
 				}
-				break;
+				return String.join(",", result); //$NON-NLS-1$
 			}
+			log.warn(format(SshdText.get().configNoKnownAlgorithms,
+					SshConstants.HOST_KEY_ALGORITHMS,
+					algorithms));
 		}
 		// No HostKeyAlgorithms; using default -- change order to put existing
 		// keys first.
@@ -262,11 +239,67 @@ public class JGitClientSession extends ClientSessionImpl {
 				}
 			}
 			reordered.addAll(defaultSignatures);
+			if (log.isDebugEnabled()) {
+				log.debug(SshConstants.HOST_KEY_ALGORITHMS + ' ' + reordered);
+			}
 			return String.join(",", reordered); //$NON-NLS-1$
 		}
+		if (log.isDebugEnabled()) {
+			log.debug(
+					SshConstants.HOST_KEY_ALGORITHMS + ' ' + defaultSignatures);
+		}
 		return String.join(",", defaultSignatures); //$NON-NLS-1$
 	}
 
+	/**
+	 * Modifies a given algorithm list according to a list from the ssh config,
+	 * including remove ('-') and reordering ('^') operators. Addition ('+') is
+	 * not handled since we have no way of adding dynamically implementations,
+	 * and the defaultList is supposed to contain all known implementations
+	 * already.
+	 *
+	 * @param defaultList
+	 *            to modify
+	 * @param fromConfig
+	 *            telling how to modify the {@code defaultList}, must not be
+	 *            {@code null} or empty
+	 * @param overrideKey
+	 *            ssh config key; used for logging
+	 * @return the modified list or {@code null} if {@code overrideKey} is not
+	 *         set
+	 */
+	public List<String> modifyAlgorithmList(List<String> defaultList,
+			String fromConfig, String overrideKey) {
+		Set<String> defaults = new LinkedHashSet<>();
+		defaults.addAll(defaultList);
+		switch (fromConfig.charAt(0)) {
+		case '+':
+			// Additions make not much sense -- it's either in
+			// defaultList already, or we have no implementation for
+			// it. No point in proposing it.
+			return defaultList;
+		case '-':
+			// This takes wildcard patterns!
+			removeFromList(defaults, overrideKey, fromConfig.substring(1));
+			return new ArrayList<>(defaults);
+		case '^':
+			// Specified entries go to the front of the default list
+			List<String> allSignatures = filteredList(defaults,
+					fromConfig.substring(1));
+			Set<String> atFront = new HashSet<>(allSignatures);
+			for (String sig : defaults) {
+				if (!atFront.contains(sig)) {
+					allSignatures.add(sig);
+				}
+			}
+			return allSignatures;
+		default:
+			// Default is overridden -- only accept the ones for which we do
+			// have an implementation.
+			return filteredList(defaults, fromConfig);
+		}
+	}
+
 	private void removeFromList(Set<String> current, String key,
 			String patterns) {
 		for (String toRemove : patterns.split("\\s*,\\s*")) { //$NON-NLS-1$
diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshClient.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshClient.java
index 74455dc808..071e1979d3 100644
--- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshClient.java
+++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshClient.java
@@ -267,6 +267,24 @@ public class JGitSshClient extends SshClient {
 		session.setUsername(username);
 		session.setConnectAddress(address);
 		session.setHostConfigEntry(hostConfig);
+		// Set signature algorithms for public key authentication
+		String pubkeyAlgos = hostConfig
+				.getProperty(SshConstants.PUBKEY_ACCEPTED_ALGORITHMS);
+		if (!StringUtils.isEmptyOrNull(pubkeyAlgos)) {
+			List<String> signatures = getSignatureFactoriesNames();
+			signatures = session.modifyAlgorithmList(signatures, pubkeyAlgos,
+					SshConstants.PUBKEY_ACCEPTED_ALGORITHMS);
+			if (!signatures.isEmpty()) {
+				if (log.isDebugEnabled()) {
+					log.debug(SshConstants.PUBKEY_ACCEPTED_ALGORITHMS + ' '
+							+ signatures);
+				}
+				session.setSignatureFactoriesNames(signatures);
+			} else {
+				log.warn(format(SshdText.get().configNoKnownAlgorithms,
+						SshConstants.PUBKEY_ACCEPTED_ALGORITHMS, pubkeyAlgos));
+			}
+		}
 		if (session.getCredentialsProvider() == null) {
 			session.setCredentialsProvider(getCredentialsProvider());
 		}
diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java
index 13bb3ebe75..4c4ff5949d 100644
--- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java
+++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/SshdText.java
@@ -25,8 +25,7 @@ public final class SshdText extends TranslationBundle {
 	/***/ public String configInvalidPattern;
 	/***/ public String configInvalidPositive;
 	/***/ public String configInvalidProxyJump;
-	/***/ public String configNoKnownHostKeyAlgorithms;
-	/***/ public String configNoRemainingHostKeyAlgorithms;
+	/***/ public String configNoKnownAlgorithms;
 	/***/ public String configProxyJumpNotSsh;
 	/***/ public String configProxyJumpWithPath;
 	/***/ public String ftpCloseFailed;