From bea3b46678c0b53ed7212a1190c4a8bb8b9002a4 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Fri, 12 Jan 2018 12:51:22 +0100 Subject: [PATCH] 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 --- .../jgit/transport/OpenSshConfigTest.java | 3 +- .../transport/JschConfigSessionFactory.java | 101 +++++++++++++++++- .../eclipse/jgit/transport/OpenSshConfig.java | 60 +---------- 3 files changed, 103 insertions(+), 61 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/OpenSshConfigTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/OpenSshConfigTest.java index 8ce3f0f14..d604751fe 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/OpenSshConfigTest.java +++ b/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 diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/JschConfigSessionFactory.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/JschConfigSessionFactory.java index eadfd69b5..cdd39ccbf 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/JschConfigSessionFactory.java +++ b/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 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. + *

+ * 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); + } + } + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/OpenSshConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/OpenSshConfig.java index 79fab0129..b5d509923 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/OpenSshConfig.java +++ b/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); - } - - } }