From adbf0935e105819f6b8f65325013b6def6205f18 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Wed, 18 Oct 2017 22:45:36 +0200 Subject: [PATCH] Ensure that ~ in ssh config is replaced before Jsch sees it Do tilde replacement for values from the ssh config file that are file names in all cases to make sure that they are already replaced when Jsch tries to get the values. Previously, OpenSshConfig did tilde replacement only for the IdentityFile in the JGit-facing "Host" interface and left the replacement in the Jsch-facing "Config" interface to Jsch. But on Windows the JGit notion of what should be used to replace the tilde differs from Jsch's replacement. Jsch always replaces the tilde by the value of the system property "user.home", whereas JGit also considers some environment variables like %HOME%. This can lead to rather surprising failures as in the case of bug 526175 where %HOME% != user.home. Prior to commit 9d24470 (i.e.,prior to JGit 4.9.0) this problem never occurred because Jsch was completely unaware of the ssh config file and all host and IdentityFile handling happened exclusively in JGit. Bug: 526175 Change-Id: I1511699664ffea07cb58ed751cfdb79b15e3a99e Signed-off-by: Thomas Wolf --- .../jgit/transport/OpenSshConfigTest.java | 25 ++++++----- .../eclipse/jgit/transport/OpenSshConfig.java | 45 ++++++++++++++----- 2 files changed, 49 insertions(+), 21 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 3eb049758..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 @@ -323,11 +323,12 @@ public class OpenSshConfigTest extends RepositoryTestCase { @Test public void testListValueMultiple() throws Exception { - // Tilde expansion doesn't occur within the parser + // Tilde expansion occurs within the parser config("Host orcz\nUserKnownHostsFile \"~/foo/ba z\" /foo/bar \n"); final ConfigRepository.Config c = osc.getConfig("orcz"); assertNotNull(c); - assertArrayEquals(new Object[] { "~/foo/ba z", "/foo/bar" }, + assertArrayEquals(new Object[] { new File(home, "foo/ba z").getPath(), + "/foo/bar" }, c.getValues("UserKnownHostsFile")); } @@ -371,8 +372,9 @@ public class OpenSshConfigTest extends RepositoryTestCase { // Host does tilde replacement assertEquals(new File(home, "foo/ba z"), f); final ConfigRepository.Config c = h.getConfig(); - // Config doesn't - assertArrayEquals(new Object[] { "~/foo/ba z", "/foo/bar" }, + // Config does tilde replacement, too + assertArrayEquals(new Object[] { new File(home, "foo/ba z").getPath(), + "/foo/bar" }, c.getValues("IdentityFile")); } @@ -386,8 +388,9 @@ public class OpenSshConfigTest extends RepositoryTestCase { // Host does tilde replacement assertEquals(new File(home, "foo/ba z"), f); final ConfigRepository.Config c = h.getConfig(); - // Config doesn't - assertArrayEquals(new Object[] { "~/foo/ba z", "/foo/bar", "/foo/baz" }, + // Config does tilde replacement, too + assertArrayEquals(new Object[] { new File(home, "foo/ba z").getPath(), + "/foo/bar", "/foo/baz" }, c.getValues("IdentityFile")); } @@ -397,7 +400,7 @@ public class OpenSshConfigTest extends RepositoryTestCase { final Host h = osc.lookup("repo.or.cz"); assertNotNull(h); assertEquals(new File(home, "foo/bar"), h.getIdentityFile()); - assertArrayEquals(new Object[] { "~/foo/bar" }, + assertArrayEquals(new Object[] { new File(home, "foo/bar").getPath() }, h.getConfig().getValues("IdentityFile")); } @@ -407,7 +410,8 @@ public class OpenSshConfigTest extends RepositoryTestCase { final Host h = osc.lookup("repo.or.cz"); assertNotNull(h); assertEquals(new File(home, "foo/bar"), h.getIdentityFile()); - assertArrayEquals(new Object[] { "~/foo/bar", "/foo/baz" }, + assertArrayEquals(new Object[] { new File(home, "foo/bar").getPath(), + "/foo/baz" }, h.getConfig().getValues("IdentityFile")); } @@ -417,12 +421,13 @@ public class OpenSshConfigTest extends RepositoryTestCase { final Host h1 = osc.lookup("repo.or.cz"); assertNotNull(h1); assertEquals(new File(home, "foo/bar"), h1.getIdentityFile()); - assertArrayEquals(new Object[] { "~/foo/bar", "/foo/baz" }, + assertArrayEquals(new Object[] { new File(home, "foo/bar").getPath(), + "/foo/baz" }, h1.getConfig().getValues("IdentityFile")); final Host h2 = osc.lookup("orcz"); assertNotNull(h2); assertEquals(new File(home, "foo/bar"), h2.getIdentityFile()); - assertArrayEquals(new Object[] { "~/foo/bar" }, + assertArrayEquals(new Object[] { new File(home, "foo/bar").getPath() }, h2.getConfig().getValues("IdentityFile")); } 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 b5b532dff..67a7db99f 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/OpenSshConfig.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/OpenSshConfig.java @@ -352,6 +352,17 @@ public class OpenSshConfig implements ConfigRepository { return Boolean.FALSE; } + private static File toFile(String path, File home) { + if (path.startsWith("~/")) { //$NON-NLS-1$ + return new File(home, path.substring(2)); + } + File ret = new File(path); + if (ret.isAbsolute()) { + return ret; + } + return new File(home, path); + } + private static int positive(final String value) { if (value != null) { try { @@ -730,25 +741,48 @@ public class OpenSshConfig implements ConfigRepository { return result; } + private List replaceTilde(List values, File home) { + List result = new ArrayList<>(values.size()); + for (String value : values) { + result.add(toFile(value, home).getPath()); + } + return result; + } + protected void substitute(String originalHostName, File home) { Replacer r = new Replacer(originalHostName, home); if (multiOptions != null) { List values = multiOptions.get("IDENTITYFILE"); //$NON-NLS-1$ if (values != null) { values = substitute(values, "dhlru", r); //$NON-NLS-1$ + values = replaceTilde(values, home); multiOptions.put("IDENTITYFILE", values); //$NON-NLS-1$ } values = multiOptions.get("CERTIFICATEFILE"); //$NON-NLS-1$ if (values != null) { values = substitute(values, "dhlru", r); //$NON-NLS-1$ + values = replaceTilde(values, home); multiOptions.put("CERTIFICATEFILE", values); //$NON-NLS-1$ } } + if (listOptions != null) { + List values = listOptions.get("GLOBALKNOWNHOSTSFILE"); //$NON-NLS-1$ + if (values != null) { + values = replaceTilde(values, home); + listOptions.put("GLOBALKNOWNHOSTSFILE", values); //$NON-NLS-1$ + } + values = listOptions.get("USERKNOWNHOSTSFILE"); //$NON-NLS-1$ + if (values != null) { + values = replaceTilde(values, home); + listOptions.put("USERKNOWNHOSTSFILE", values); //$NON-NLS-1$ + } + } if (options != null) { // HOSTNAME already done in Replacer constructor String value = options.get("IDENTITYAGENT"); //$NON-NLS-1$ if (value != null) { value = r.substitute(value, "dhlru"); //$NON-NLS-1$ + value = toFile(value, home).getPath(); options.put("IDENTITYAGENT", value); //$NON-NLS-1$ } } @@ -909,17 +943,6 @@ public class OpenSshConfig implements ConfigRepository { } } - private File toFile(String path, File home) { - if (path.startsWith("~/")) { //$NON-NLS-1$ - return new File(home, path.substring(2)); - } - File ret = new File(path); - if (ret.isAbsolute()) { - return ret; - } - return new File(home, path); - } - Config getConfig() { return config; }