Merge branch 'stable-5.11'

* stable-5.11:
  Refactor CommitCommand to improve readability
  CommitCommand: fix formatting
  CommitCommand: remove unncessary comment
  Ensure post-commit hook is called after index lock was released
  sshd: try all configured signature algorithms for a key
  sshd: modernize ssh config file parsing
  sshd: implement ssh config PubkeyAcceptedAlgorithms

Change-Id: Ic3235ffd84c9d7537a1fe5ff4f216578e6e26724
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

3 files changed, 124 insertions, 25 deletions

diff --git a/org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF b/org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF
index 5030f2c172..2a116523e7 100644
--- a/org.eclipse.jgit.ssh.apache.test/META-INF/MANIFEST.MF
+++ b/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)",
diff --git a/org.eclipse.jgit.ssh.apache.test/build.properties b/org.eclipse.jgit.ssh.apache.test/build.properties
index 9ffa0caf78..406c5a768f 100644
--- a/org.eclipse.jgit.ssh.apache.test/build.properties
+++ b/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
diff --git a/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java b/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java
index 97f97f9028..c56d2307c6 100644
--- a/org.eclipse.jgit.ssh.apache.test/tst/org/eclipse/jgit/transport/sshd/ApacheSshTest.java
+++ b/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,73 @@ 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();
+		}
+	}
+
+	/**
+	 * Tests that one can log in to an old server that knows only the ssh-rsa
+	 * signature algorithm. The client has by default the list of signature
+	 * algorithms for RSA as "rsa-sha2-512,rsa-sha2-256,ssh-rsa". It should try
+	 * all three with the single key configured, and finally succeed.
+	 * <p>
+	 * The re-ordering mechanism (see
+	 * {@link #testConnectAuthSshRsaPubkeyAcceptedAlgorithms()}) is still
+	 * important; servers may impose a penalty (back-off delay) for subsequent
+	 * attempts with signature algorithms unknown to the server. So a user
+	 * connecting to such a server and noticing delays may still want to put
+	 * ssh-rsa first in the list for that host.
+	 * </p>
+	 *
+	 * @see <a href="https://bugs.eclipse.org/bugs/show_bug.cgi?id=572056">bug
+	 *      572056</a>
+	 * @throws Exception
+	 *             on failure
+	 */
+	@Test
+	public void testConnectAuthSshRsa() 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());
+			RemoteSession session = getSessionFactory().getSession(
+					new URIish("ssh://server/doesntmatter"), null, FS.DETECTED,
+					10000);
+			assertNotNull(session);
+			session.disconnect();
+		}
+	}
 }