Apache MINA sshd client: test & fix password authentication

Add tests for password and keyboard-interactive authentication.
Implement password authentication; the default provided by sshd
is non-interactive, which is not useful for JGit.

Make sure the CredentialsProvider gets reset on successive password
retrieval attempts. Otherwise it might always return the same non-
accepted password from a secure storage. (That one was discovered
by actually trying this via EGit; the JGit tests don't catch this.)

Change the default order of authentication mechanisms to prefer
password over keyboard-interactive. This is a mitigation for upstream
bug SSHD-866.[1]

Also include a fix for upstream bug SSHD-867.[2]

[1] https://issues.apache.org/jira/projects/SSHD/issues/SSHD-866
[2] https://issues.apache.org/jira/projects/SSHD/issues/SSHD-867

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

8 files changed, 264 insertions, 23 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 f9ff02b40c..aa4e4ccec3 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
@@ -44,6 +44,7 @@ knownHostsUnknownKeyPrompt=Accept and store this key, and continue connecting?
 knownHostsUnknownKeyType=Cannot read server key from known hosts file {0}; line {1}
 knownHostsUserAskCreationMsg=File {0} does not exist.
 knownHostsUserAskCreationPrompt=Create file {0} ?
+passwordPrompt=Password
 proxyCannotAuthenticate=Cannot authenticate to proxy {0}
 proxyHttpFailure=HTTP Proxy connection to {0} failed with code {1}: {2}
 proxyHttpInvalidUserName=HTTP proxy connection {0} with invalid user name; must not contain colons: {1}
diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitHostConfigEntry.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitHostConfigEntry.java
index 8e97dad4ad..a0705f25f5 100644
--- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitHostConfigEntry.java
+++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitHostConfigEntry.java
@@ -60,6 +60,22 @@ public class JGitHostConfigEntry extends HostConfigEntry {
 
 	private Map<String, List<String>> multiValuedOptions;
 
+	@Override
+	public String getProperty(String name, String defaultValue) {
+		// Upstream bug fix (SSHD-867): if there are _no_ properties at all, the
+		// super implementation returns always null even if a default value is
+		// given.
+		//
+		// See https://issues.apache.org/jira/projects/SSHD/issues/SSHD-867
+		//
+		// TODO: remove this override once we're based on sshd > 2.1.0
+		Map<String, String> properties = getProperties();
+		if (properties == null || properties.isEmpty()) {
+			return defaultValue;
+		}
+		return super.getProperty(name, defaultValue);
+	}
+
 	/**
 	 * Sets the multi-valued options.
 	 *
diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitPasswordAuthFactory.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitPasswordAuthFactory.java
new file mode 100644
index 0000000000..315d0258d6
--- /dev/null
+++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitPasswordAuthFactory.java
@@ -0,0 +1,66 @@
+/*
+ * Copyright (C) 2018, Thomas Wolf <thomas.wolf@paranor.ch>
+ * and other copyright owners as documented in the project's IP log.
+ *
+ * This program and the accompanying materials are made available
+ * under the terms of the Eclipse Distribution License v1.0 which
+ * accompanies this distribution, is reproduced below, and is
+ * available at http://www.eclipse.org/org/documents/edl-v10.php
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Eclipse Foundation, Inc. nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+package org.eclipse.jgit.internal.transport.sshd;
+
+import org.apache.sshd.client.auth.AbstractUserAuthFactory;
+import org.apache.sshd.client.auth.UserAuth;
+import org.apache.sshd.client.auth.password.UserAuthPasswordFactory;
+
+/**
+ * A customized {@link UserAuthPasswordFactory} that creates instance of
+ * {@link JGitPasswordAuthentication}.
+ */
+public class JGitPasswordAuthFactory extends AbstractUserAuthFactory {
+
+	/** The singleton {@link JGitPasswordAuthFactory}. */
+	public static final JGitPasswordAuthFactory INSTANCE = new JGitPasswordAuthFactory();
+
+	private JGitPasswordAuthFactory() {
+		super(UserAuthPasswordFactory.NAME);
+	}
+
+	@Override
+	public UserAuth create() {
+		return new JGitPasswordAuthentication();
+	}
+}
diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitPasswordAuthentication.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitPasswordAuthentication.java
new file mode 100644
index 0000000000..9a6ed39c68
--- /dev/null
+++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitPasswordAuthentication.java
@@ -0,0 +1,100 @@
+/*
+ * Copyright (C) 2018, Thomas Wolf <thomas.wolf@paranor.ch>
+ * and other copyright owners as documented in the project's IP log.
+ *
+ * This program and the accompanying materials are made available
+ * under the terms of the Eclipse Distribution License v1.0 which
+ * accompanies this distribution, is reproduced below, and is
+ * available at http://www.eclipse.org/org/documents/edl-v10.php
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Eclipse Foundation, Inc. nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+package org.eclipse.jgit.internal.transport.sshd;
+
+import java.util.concurrent.CancellationException;
+
+import org.apache.sshd.client.ClientAuthenticationManager;
+import org.apache.sshd.client.auth.keyboard.UserInteraction;
+import org.apache.sshd.client.auth.password.UserAuthPassword;
+import org.apache.sshd.client.session.ClientSession;
+
+/**
+ * A password authentication handler that uses the {@link JGitUserInteraction}
+ * to ask the user for the password. It also respects the
+ * {@code NumberOfPasswordPrompts} ssh config.
+ */
+public class JGitPasswordAuthentication extends UserAuthPassword {
+
+	private int maxAttempts;
+
+	private int attempts;
+
+	@Override
+	public void init(ClientSession session, String service) throws Exception {
+		super.init(session, service);
+		maxAttempts = Math.max(1,
+				session.getIntProperty(
+						ClientAuthenticationManager.PASSWORD_PROMPTS,
+						ClientAuthenticationManager.DEFAULT_PASSWORD_PROMPTS));
+		attempts = 0;
+	}
+
+	@Override
+	protected boolean sendAuthDataRequest(ClientSession session, String service)
+			throws Exception {
+		if (++attempts > maxAttempts) {
+			return false;
+		}
+		UserInteraction interaction = session.getUserInteraction();
+		if (!interaction.isInteractionAllowed(session)) {
+			return false;
+		}
+		String password = getPassword(session, interaction);
+		if (password == null) {
+			throw new CancellationException();
+		}
+		// sendPassword takes a buffer as first argument, but actually doesn't
+		// use it and creates its own buffer...
+		sendPassword(null, session, password, password);
+		return true;
+	}
+
+	private String getPassword(ClientSession session,
+			UserInteraction interaction) {
+		String[] results = interaction.interactive(session, null, null, "", //$NON-NLS-1$
+				new String[] { SshdText.get().passwordPrompt },
+				new boolean[] { false });
+		return (results == null || results.length == 0) ? null : results[0];
+	}
+}
diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshConfig.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshConfig.java
index 8ca9d2103f..9eced0fa7f 100644
--- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshConfig.java
+++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitSshConfig.java
@@ -105,11 +105,26 @@ public class JGitSshConfig implements HostConfigEntryResolver {
 			String username) throws IOException {
 		HostEntry entry = configFile.lookup(host, port, username);
 		JGitHostConfigEntry config = new JGitHostConfigEntry();
+		// Apache MINA conflates all keys, even multi-valued ones, in one map
+		// and puts multiple values separated by commas in one string. See
+		// the javadoc on HostConfigEntry.
+		Map<String, String> allOptions = new TreeMap<>(
+				String.CASE_INSENSITIVE_ORDER);
+		allOptions.putAll(entry.getOptions());
+		// And what if a value contains a comma??
+		entry.getMultiValuedOptions().entrySet().stream()
+				.forEach(e -> allOptions.put(e.getKey(),
+						String.join(",", e.getValue()))); //$NON-NLS-1$
+		config.setProperties(allOptions);
+		// The following is an extension from JGitHostConfigEntry
+		config.setMultiValuedOptions(entry.getMultiValuedOptions());
+		// Also make sure the underlying properties are set
 		String hostName = entry.getValue(SshConstants.HOST_NAME);
 		if (hostName == null || hostName.isEmpty()) {
 			hostName = host;
 		}
 		config.setHostName(hostName);
+		config.setProperty(SshConstants.HOST_NAME, hostName);
 		config.setHost(SshdSocketAddress.isIPv6Address(hostName) ? "" : hostName); //$NON-NLS-1$
 		String user = username != null && !username.isEmpty() ? username
 				: entry.getValue(SshConstants.USER);
@@ -117,24 +132,14 @@ public class JGitSshConfig implements HostConfigEntryResolver {
 			user = configFile.getLocalUserName();
 		}
 		config.setUsername(user);
+		config.setProperty(SshConstants.USER, user);
 		int p = port >= 0 ? port : positive(entry.getValue(SshConstants.PORT));
 		config.setPort(p >= 0 ? p : SshConstants.SSH_DEFAULT_PORT);
+		config.setProperty(SshConstants.PORT,
+				Integer.toString(config.getPort()));
 		config.setIdentities(entry.getValues(SshConstants.IDENTITY_FILE));
 		config.setIdentitiesOnly(
 				flag(entry.getValue(SshConstants.IDENTITIES_ONLY)));
-		// Apache MINA conflates all keys, even multi-valued ones, in one map
-		// and puts multiple values separated by commas in one string. See
-		// the javadoc on HostConfigEntry.
-		Map<String, String> allOptions = new TreeMap<>(
-				String.CASE_INSENSITIVE_ORDER);
-		allOptions.putAll(entry.getOptions());
-		// And what if a value contains a comma??
-		entry.getMultiValuedOptions().entrySet().stream()
-				.forEach(e -> allOptions.put(e.getKey(),
-						String.join(",", e.getValue()))); //$NON-NLS-1$
-		config.setProperties(allOptions);
-		// The following is an extension from JGitHostConfigEntry
-		config.setMultiValuedOptions(entry.getMultiValuedOptions());
 		return config;
 	}
 
diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitUserInteraction.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitUserInteraction.java
index 27380db33b..99288b7af1 100644
--- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitUserInteraction.java
+++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitUserInteraction.java
@@ -45,9 +45,13 @@ package org.eclipse.jgit.internal.transport.sshd;
 import java.net.InetSocketAddress;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
 
 import org.apache.sshd.client.auth.keyboard.UserInteraction;
 import org.apache.sshd.client.session.ClientSession;
+import org.apache.sshd.common.session.Session;
+import org.apache.sshd.common.session.SessionListener;
 import org.eclipse.jgit.transport.CredentialItem;
 import org.eclipse.jgit.transport.CredentialsProvider;
 import org.eclipse.jgit.transport.SshConstants;
@@ -62,6 +66,12 @@ public class JGitUserInteraction implements UserInteraction {
 	private final CredentialsProvider provider;
 
 	/**
+	 * We need to reset the JGit credentials provider if we have repeated
+	 * attempts.
+	 */
+	private final Map<Session, SessionListener> ongoing = new ConcurrentHashMap<>();
+
+	/**
 	 * Creates a new {@link JGitUserInteraction} for interactive password input
 	 * based on the given {@link CredentialsProvider}.
 	 *
@@ -74,13 +84,13 @@ public class JGitUserInteraction implements UserInteraction {
 
 	@Override
 	public boolean isInteractionAllowed(ClientSession session) {
-		return provider.isInteractive();
+		return provider != null && provider.isInteractive();
 	}
 
 	@Override
 	public String[] interactive(ClientSession session, String name,
 			String instruction, String lang, String[] prompt, boolean[] echo) {
-		// This is keyboard-interactive authentication
+		// This is keyboard-interactive or password authentication
 		List<CredentialItem> items = new ArrayList<>();
 		int numberOfHiddenInputs = 0;
 		for (int i = 0; i < prompt.length; i++) {
@@ -120,6 +130,19 @@ public class JGitUserInteraction implements UserInteraction {
 		}
 		URIish uri = toURI(session.getUsername(),
 				(InetSocketAddress) session.getConnectAddress());
+		// Reset the provider for this URI if it's not the first attempt and we
+		// have hidden inputs. Otherwise add a session listener that will remove
+		// itself once authenticated.
+		if (numberOfHiddenInputs > 0) {
+			SessionListener listener = ongoing.get(session);
+			if (listener != null) {
+				provider.reset(uri);
+			} else {
+				listener = new SessionAuthMarker(ongoing);
+				ongoing.put(session, listener);
+				session.addSessionListener(listener);
+			}
+		}
 		if (provider.get(uri, items)) {
 			return items.stream().map(i -> {
 				if (i instanceof CredentialItem.Password) {
@@ -166,4 +189,31 @@ public class JGitUserInteraction implements UserInteraction {
 				.setPort(port) //
 				.setUser(userName);
 	}
+
+	/**
+	 * A {@link SessionListener} that removes itself from the session when
+	 * authentication is done or the session is closed.
+	 */
+	private static class SessionAuthMarker implements SessionListener {
+
+		private final Map<Session, SessionListener> registered;
+
+		public SessionAuthMarker(Map<Session, SessionListener> registered) {
+			this.registered = registered;
+		}
+
+		@Override
+		public void sessionEvent(Session session, SessionListener.Event event) {
+			if (event == SessionListener.Event.Authenticated) {
+				session.removeSessionListener(this);
+				registered.remove(session, this);
+			}
+		}
+
+		@Override
+		public void sessionClosed(Session session) {
+			session.removeSessionListener(this);
+			registered.remove(session, this);
+		}
+	}
 }
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 d4b6593ef6..5c79f2d40e 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
@@ -56,6 +56,7 @@ public final class SshdText extends TranslationBundle {
 	/***/ public String knownHostsUnknownKeyType;
 	/***/ public String knownHostsUserAskCreationMsg;
 	/***/ public String knownHostsUserAskCreationPrompt;
+	/***/ public String passwordPrompt;
 	/***/ public String proxyCannotAuthenticate;
 	/***/ public String proxyHttpFailure;
 	/***/ public String proxyHttpInvalidUserName;
diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/transport/sshd/SshdSessionFactory.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/transport/sshd/SshdSessionFactory.java
index f5d46d3d86..4ec6f22094 100644
--- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/transport/sshd/SshdSessionFactory.java
+++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/transport/sshd/SshdSessionFactory.java
@@ -63,7 +63,6 @@ import org.apache.sshd.client.ClientBuilder;
 import org.apache.sshd.client.SshClient;
 import org.apache.sshd.client.auth.UserAuth;
 import org.apache.sshd.client.auth.keyboard.UserAuthKeyboardInteractiveFactory;
-import org.apache.sshd.client.auth.password.UserAuthPasswordFactory;
 import org.apache.sshd.client.config.hosts.HostConfigEntryResolver;
 import org.apache.sshd.client.keyverifier.ServerKeyVerifier;
 import org.apache.sshd.common.NamedFactory;
@@ -75,6 +74,7 @@ import org.eclipse.jgit.annotations.NonNull;
 import org.eclipse.jgit.errors.TransportException;
 import org.eclipse.jgit.internal.transport.sshd.CachingKeyPairProvider;
 import org.eclipse.jgit.internal.transport.sshd.GssApiWithMicAuthFactory;
+import org.eclipse.jgit.internal.transport.sshd.JGitPasswordAuthFactory;
 import org.eclipse.jgit.internal.transport.sshd.JGitPublicKeyAuthFactory;
 import org.eclipse.jgit.internal.transport.sshd.JGitSshClient;
 import org.eclipse.jgit.internal.transport.sshd.JGitSshConfig;
@@ -465,21 +465,23 @@ public class SshdSessionFactory extends SshSessionFactory implements Closeable {
 
 	/**
 	 * Gets the user authentication mechanisms (or rather, factories for them).
-	 * By default this returns gssapi-with-mic, public-key,
-	 * keyboard-interactive, and password, in that order. The order is only
-	 * significant if the ssh config does <em>not</em> set
-	 * {@code PreferredAuthentications}; if it is set, the order defined there
-	 * will be taken.
+	 * By default this returns gssapi-with-mic, public-key, password, and
+	 * keyboard-interactive, in that order. The order is only significant if the
+	 * ssh config does <em>not</em> set {@code PreferredAuthentications}; if it
+	 * is set, the order defined there will be taken.
 	 *
 	 * @return the non-empty list of factories.
 	 */
 	@NonNull
 	private List<NamedFactory<UserAuth>> getUserAuthFactories() {
+		// About the order of password and keyboard-interactive, see upstream
+		// bug https://issues.apache.org/jira/projects/SSHD/issues/SSHD-866 .
+		// Password auth doesn't have this problem.
 		return Collections.unmodifiableList(
 				Arrays.asList(GssApiWithMicAuthFactory.INSTANCE,
 						JGitPublicKeyAuthFactory.INSTANCE,
-						UserAuthKeyboardInteractiveFactory.INSTANCE,
-						UserAuthPasswordFactory.INSTANCE));
+						JGitPasswordAuthFactory.INSTANCE,
+						UserAuthKeyboardInteractiveFactory.INSTANCE));
 	}
 
 	/**