Revert handling of ssh IdentityFile to pre-4.9 behavior

Jsch caches keys (aka identities) specified in ~/.ssh/config via
IndentityFile only for the current Jsch Session. This results in
multiple password prompts for successive sessions.

Do the handling of IdentityFile exclusively in JGit, as it was before
4.9. JGit uses different Jsch instances per host and caches the
IdentityFile there, allowing it to be re-used in different sessions
for the same host.

* Add comments to explain this.
* Move the JschBugFixingConfig from OpenSshConfig to
  JschConfigSessionFactory to have all these Jsch work-arounds
  in one place.
* Make that config hide the IdentityFile config from Jsch to avoid
  that Jsch overrides the JGit behavior.

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

org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/OpenSshConfigTest.java

@@ -343,7 +343,8 @@ public class OpenSshConfigTest extends RepositoryTestCase {
 		assertEquals(h1.getConnectionAttempts(), h2.getConnectionAttempts());
 		final ConfigRepository.Config c = osc.getConfig("orcz");
 		assertNotNull(c);
-		assertSame(h1.getConfig(), h2.getConfig());
+		assertSame(c, h1.getConfig());
+		assertSame(c, h2.getConfig());
 	}
 
 	@Test

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

@@ -59,7 +59,9 @@ import java.net.ConnectException;
 import java.net.UnknownHostException;
 import java.text.MessageFormat;
 import java.util.HashMap;
+import java.util.Locale;
 import java.util.Map;
+import java.util.concurrent.TimeUnit;
 
 import org.eclipse.jgit.errors.TransportException;
 import org.eclipse.jgit.internal.JGitText;
@@ -67,6 +69,7 @@ import org.eclipse.jgit.util.FS;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import com.jcraft.jsch.ConfigRepository;
 import com.jcraft.jsch.JSch;
 import com.jcraft.jsch.JSchException;
 import com.jcraft.jsch.Session;
@@ -89,6 +92,14 @@ public abstract class JschConfigSessionFactory extends SshSessionFactory {
 	private static final Logger LOG = LoggerFactory
 			.getLogger(JschConfigSessionFactory.class);
 
+	/**
+	 * We use different Jsch instances for hosts that have an IdentityFile
+	 * configured in ~/.ssh/config. Jsch by default would cache decrypted keys
+	 * only per session, which results in repeated password prompts. Using
+	 * different Jsch instances, we can cache the keys on these instances so
+	 * that they will be re-used for successive sessions, and thus the user is
+	 * prompted for a key password only once while Eclipse runs.
+	 */
 	private final Map<String, JSch> byIdentityFile = new HashMap<>();
 
 	private JSch defaultJSch;
@@ -297,7 +308,8 @@ public abstract class JschConfigSessionFactory extends SshSessionFactory {
 		if (defaultJSch == null) {
 			defaultJSch = createDefaultJSch(fs);
 			if (defaultJSch.getConfigRepository() == null) {
-				defaultJSch.setConfigRepository(config);
+				defaultJSch.setConfigRepository(
+						new JschBugFixingConfigRepository(config));
 			}
 			for (Object name : defaultJSch.getIdentityNames())
 				byIdentityFile.put((String) name, defaultJSch);
@@ -378,4 +390,91 @@ public abstract class JschConfigSessionFactory extends SshSessionFactory {
 			}
 		}
 	}
+
+	private static class JschBugFixingConfigRepository
+			implements ConfigRepository {
+
+		private final ConfigRepository base;
+
+		public JschBugFixingConfigRepository(ConfigRepository base) {
+			this.base = base;
+		}
+
+		@Override
+		public Config getConfig(String host) {
+			return new JschBugFixingConfig(base.getConfig(host));
+		}
+
+		/**
+		 * A {@link com.jcraft.jsch.ConfigRepository.Config} that transforms
+		 * some values from the config file into the format Jsch 0.1.54 expects.
+		 * This is a work-around for bugs in Jsch.
+		 * <p>
+		 * Additionally, this config hides the IdentityFile config entries from
+		 * Jsch; we manage those ourselves. Otherwise Jsch would cache passwords
+		 * (or rather, decrypted keys) only for a single session, resulting in
+		 * multiple password prompts for user operations that use several Jsch
+		 * sessions.
+		 */
+		private static class JschBugFixingConfig implements Config {
+
+			private static final String[] NO_IDENTITIES = {};
+
+			private final Config real;
+
+			public JschBugFixingConfig(Config delegate) {
+				real = delegate;
+			}
+
+			@Override
+			public String getHostname() {
+				return real.getHostname();
+			}
+
+			@Override
+			public String getUser() {
+				return real.getUser();
+			}
+
+			@Override
+			public int getPort() {
+				return real.getPort();
+			}
+
+			@Override
+			public String getValue(String key) {
+				String k = key.toUpperCase(Locale.ROOT);
+				if ("IDENTITYFILE".equals(k)) { //$NON-NLS-1$
+					return null;
+				}
+				String result = real.getValue(key);
+				if (result != null) {
+					if ("SERVERALIVEINTERVAL".equals(k) //$NON-NLS-1$
+							|| "CONNECTTIMEOUT".equals(k)) { //$NON-NLS-1$
+						// These values are in seconds. Jsch 0.1.54 passes them
+						// on as is to java.net.Socket.setSoTimeout(), which
+						// expects milliseconds. So convert here to
+						// milliseconds.
+						try {
+							int timeout = Integer.parseInt(result);
+							result = Long.toString(
+									TimeUnit.SECONDS.toMillis(timeout));
+						} catch (NumberFormatException e) {
+							// Ignore
+						}
+					}
+				}
+				return result;
+			}
+
+			@Override
+			public String[] getValues(String key) {
+				String k = key.toUpperCase(Locale.ROOT);
+				if ("IDENTITYFILE".equals(k)) { //$NON-NLS-1$
+					return NO_IDENTITIES;
+				}
+				return real.getValues(key);
+			}
+		}
+	}
 }

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

@@ -59,7 +59,6 @@ import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.TimeUnit;
 
 import org.eclipse.jgit.errors.InvalidPatternException;
 import org.eclipse.jgit.fnmatch.FileNameMatcher;
@@ -972,7 +971,7 @@ public class OpenSshConfig implements ConfigRepository {
 	@Override
 	public Config getConfig(String hostName) {
 		Host host = lookup(hostName);
-		return new JschBugFixingConfig(host.getConfig());
+		return host.getConfig();
 	}
 
 	@Override
@@ -981,61 +980,4 @@ public class OpenSshConfig implements ConfigRepository {
 		return "OpenSshConfig [home=" + home + ", configFile=" + configFile
 				+ ", lastModified=" + lastModified + ", state=" + state + "]";
 	}
-
-	/**
-	 * A {@link com.jcraft.jsch.ConfigRepository.Config} that transforms some
-	 * values from the config file into the format Jsch 0.1.54 expects. This is
-	 * a work-around for bugs in Jsch.
-	 */
-	private static class JschBugFixingConfig implements Config {
-
-		private final Config real;
-
-		public JschBugFixingConfig(Config delegate) {
-			real = delegate;
-		}
-
-		@Override
-		public String getHostname() {
-			return real.getHostname();
-		}
-
-		@Override
-		public String getUser() {
-			return real.getUser();
-		}
-
-		@Override
-		public int getPort() {
-			return real.getPort();
-		}
-
-		@Override
-		public String getValue(String key) {
-			String result = real.getValue(key);
-			if (result != null) {
-				String k = key.toUpperCase(Locale.ROOT);
-				if ("SERVERALIVEINTERVAL".equals(k) //$NON-NLS-1$
-						|| "CONNECTTIMEOUT".equals(k)) { //$NON-NLS-1$
-					// These values are in seconds. Jsch 0.1.54 passes them on
-					// as is to java.net.Socket.setSoTimeout(), which expects
-					// milliseconds. So convert here to milliseconds...
-					try {
-						int timeout = Integer.parseInt(result);
-						result = Long
-								.toString(TimeUnit.SECONDS.toMillis(timeout));
-					} catch (NumberFormatException e) {
-						// Ignore
-					}
-				}
-			}
-			return result;
-		}
-
-		@Override
-		public String[] getValues(String key) {
-			return real.getValues(key);
-		}
-
-	}
 }