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>

org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF

@@ -14,6 +14,7 @@ Import-Package: org.apache.sshd.client.config.hosts;version="[2.6.0,2.7.0)",
  org.apache.sshd.common.helpers;version="[2.6.0,2.7.0)",
  org.apache.sshd.common.keyprovider;version="[2.6.0,2.7.0)",
  org.apache.sshd.common.session;version="[2.6.0,2.7.0)",
+ org.apache.sshd.common.signature;version="[2.6.0,2.7.0)",
  org.apache.sshd.common.util.net;version="[2.6.0,2.7.0)",
  org.apache.sshd.common.util.security;version="[2.6.0,2.7.0)",
  org.apache.sshd.core;version="[2.6.0,2.7.0)",

org.eclipse.jgit.ssh.apache.test/build.properties

@@ -3,3 +3,5 @@ output.. = bin/
 bin.includes = META-INF/,\
                .,\
                plugin.properties
+additional.bundles = org.apache.log4j,\
+                     org.slf4j.binding.log4j12

org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java

@@ -47,7 +47,9 @@ import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.api.errors.TransportException;
 import org.eclipse.jgit.junit.ssh.SshTestBase;
 import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.transport.RemoteSession;
 import org.eclipse.jgit.transport.SshSessionFactory;
+import org.eclipse.jgit.transport.URIish;
 import org.eclipse.jgit.util.FS;
 import org.junit.Test;
 import org.junit.experimental.theories.Theories;
@@ -232,64 +234,89 @@ public class ApacheSshTest extends SshTestBase {
 	}
 
 	/**
-	 * Creates a simple proxy server. Accepts only publickey authentication from
-	 * the given user with the given key, allows all forwardings. Adds the
-	 * proxy's host key to {@link #knownHosts}.
+	 * Creates a simple SSH server without git setup.
 	 *
 	 * @param user
 	 *            to accept
 	 * @param userKey
 	 *            public key of that user at this server
-	 * @param report
-	 *            single-element array to report back the forwarded address.
-	 * @return the started server
+	 * @return the {@link SshServer}, not yet started
 	 * @throws Exception
 	 */
-	private SshServer createProxy(String user, File userKey,
-			SshdSocketAddress[] report) throws Exception {
-		SshServer proxy = SshServer.setUpDefaultServer();
+	private SshServer createServer(String user, File userKey) throws Exception {
+		SshServer srv = SshServer.setUpDefaultServer();
 		// Give the server its own host key
 		KeyPairGenerator generator = KeyPairGenerator.getInstance("RSA");
 		generator.initialize(2048);
 		KeyPair proxyHostKey = generator.generateKeyPair();
-		proxy.setKeyPairProvider(
+		srv.setKeyPairProvider(
 				session -> Collections.singletonList(proxyHostKey));
 		// Allow (only) publickey authentication
-		proxy.setUserAuthFactories(Collections.singletonList(
+		srv.setUserAuthFactories(Collections.singletonList(
 				ServerAuthenticationManager.DEFAULT_USER_AUTH_PUBLIC_KEY_FACTORY));
 		// Install the user's public key
 		PublicKey userProxyKey = AuthorizedKeyEntry
 				.readAuthorizedKeys(userKey.toPath()).get(0)
 				.resolvePublicKey(null, PublicKeyEntryResolver.IGNORING);
-		proxy.setPublickeyAuthenticator(
+		srv.setPublickeyAuthenticator(
 				(userName, publicKey, session) -> user.equals(userName)
 						&& KeyUtils.compareKeys(userProxyKey, publicKey));
-		// Allow forwarding
-		proxy.setForwardingFilter(new StaticDecisionForwardingFilter(true) {
+		return srv;
+	}
 
-			@Override
-			protected boolean checkAcceptance(String request, Session session,
-					SshdSocketAddress target) {
-				report[0] = target;
-				return super.checkAcceptance(request, session, target);
-			}
-		});
-		proxy.start();
+	/**
+	 * Writes the server's host key to our knownhosts file.
+	 *
+	 * @param srv to register
+	 * @throws Exception
+	 */
+	private void registerServer(SshServer srv) throws Exception {
 		// Add the proxy's host key to knownhosts
 		try (BufferedWriter writer = Files.newBufferedWriter(
 				knownHosts.toPath(), StandardCharsets.US_ASCII,
 				StandardOpenOption.WRITE, StandardOpenOption.APPEND)) {
 			writer.append('\n');
 			KnownHostHashValue.appendHostPattern(writer, "localhost",
-					proxy.getPort());
+					srv.getPort());
 			writer.append(',');
 			KnownHostHashValue.appendHostPattern(writer, "127.0.0.1",
-					proxy.getPort());
+					srv.getPort());
 			writer.append(' ');
 			PublicKeyEntry.appendPublicKeyEntry(writer,
-					proxyHostKey.getPublic());
+					srv.getKeyPairProvider().loadKeys(null).iterator().next().getPublic());
 			writer.append('\n');
 		}
+	}
+
+	/**
+	 * Creates a simple proxy server. Accepts only publickey authentication from
+	 * the given user with the given key, allows all forwardings. Adds the
+	 * proxy's host key to {@link #knownHosts}.
+	 *
+	 * @param user
+	 *            to accept
+	 * @param userKey
+	 *            public key of that user at this server
+	 * @param report
+	 *            single-element array to report back the forwarded address.
+	 * @return the started server
+	 * @throws Exception
+	 */
+	private SshServer createProxy(String user, File userKey,
+			SshdSocketAddress[] report) throws Exception {
+		SshServer proxy = createServer(user, userKey);
+		// Allow forwarding
+		proxy.setForwardingFilter(new StaticDecisionForwardingFilter(true) {
+
+			@Override
+			protected boolean checkAcceptance(String request, Session session,
+					SshdSocketAddress target) {
+				report[0] = target;
+				return super.checkAcceptance(request, session, target);
+			}
+		});
+		proxy.start();
+		registerServer(proxy);
 		return proxy;
 	}
 
@@ -606,4 +633,35 @@ public class ApacheSshTest extends SshTestBase {
 			}
 		}
 	}
+
+	/**
+	 * Tests that one can log in to an old server that doesn't handle
+	 * rsa-sha2-512 if one puts ssh-rsa first in the client's list of public key
+	 * signature algorithms.
+	 *
+	 * @see <a href="https://bugs.eclipse.org/bugs/show_bug.cgi?id=572056">bug
+	 *      572056</a>
+	 * @throws Exception
+	 *             on failure
+	 */
+	@Test
+	public void testConnectAuthSshRsaPubkeyAcceptedAlgorithms()
+			throws Exception {
+		try (SshServer oldServer = createServer(TEST_USER, publicKey1)) {
+			oldServer.setSignatureFactoriesNames("ssh-rsa");
+			oldServer.start();
+			registerServer(oldServer);
+			installConfig("Host server", //
+					"HostName localhost", //
+					"Port " + oldServer.getPort(), //
+					"User " + TEST_USER, //
+					"IdentityFile " + privateKey1.getAbsolutePath(), //
+					"PubkeyAcceptedAlgorithms ^ssh-rsa");
+			RemoteSession session = getSessionFactory().getSession(
+					new URIish("ssh://server/doesntmatter"), null, FS.DETECTED,
+					10000);
+			assertNotNull(session);
+			session.disconnect();
+		}
+	}
 }

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

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$

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());
 		}

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;

org.eclipse.jgit/src/org/eclipse/jgit/transport/SshConstants.java

@@ -114,6 +114,14 @@ public final class SshConstants {
 	/** Key in an ssh config file. */
 	public static final String PREFERRED_AUTHENTICATIONS = "PreferredAuthentications";
 
+	/**
+	 * Key in an ssh config file; defines signature algorithms for public key
+	 * authentication as a comma-separated list.
+	 *
+	 * @since 5.11
+	 */
+	public static final String PUBKEY_ACCEPTED_ALGORITHMS = "PubkeyAcceptedAlgorithms";
+
 	/** Key in an ssh config file. */
 	public static final String PROXY_COMMAND = "ProxyCommand";