Make the file tree search for the SSH executable opt-in (#1)

On non-linux build agents it is likely that the file tree search is done. This means a serious performance hit for build agent startup and a big security issue if you run untrusted builds. Due to that now you have to explicitly configure the SSH executable to 'auto' to enable the file tree scan if you really want it.
Vampire · Aug 15, 2019 · 7c7259f · 7c7259f
1 parent 45ebf7b
commit 7c7259f
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 24 deletions.
diff --git a/README.md b/README.md
@@ -63,13 +63,26 @@ Requirements
 The plugin needs the "SSH Keys Manager" plugin - which is shipped with TeamCity - to be available and enabled.
 
 Additionally, on the build agent the `ssh` client tool needs to be installed. It can be available on the path. If it is
-not, some common places in the filesystem are searched for the executable. If it is not found, as last option, the whole
-filesystem is searched for an executable called `ssh`, which is tested for being an `ssh` client executable. If the
-`ssh` client is available but is not found, a different than the automatically found one should be used, or the
-filesystem scan should not be done, it can be configured on the build agent as a build agent property in
-`conf/buildAgent.properties`, as a system property, both with the name `ssh.executable`, or as environment variable with
-name `SSH_EXECUTABLE` in that order. The value should either be an executable name available on the path or an absolute
-path.
+not, some common places in the filesystem are searched for the executable. If the `ssh` client is available but is not
+found or a different than the automatically found one should be used, it can be configured on the build agent as a build
+agent property in `conf/buildAgent.properties`, as a system property, both with the name `ssh.executable`, or as
+environment variable with name `SSH_EXECUTABLE` in that order. The value should either be an executable name available
+on the path or an absolute path. Alternatively the value can be set to `auto`. In this case the whole filesystem is
+searched for an executable called `ssh` with an optional file extension, which is tested for being an `ssh` client
+executable.
+
+**Warnings for usage of `auto`**
+
+- _**Security:**_ If `auto` is used, the whole file tree is scanned for a file which then is executed. If the build
+  agent system is not trustworthy, or you are running untrusted builds that could leave a file called accordingly, this
+  could maliciously steal your SSH key. Do **not** use this functionality if your build agent system is not trustworthy
+  or you are running untrusted builds like pull requests from arbitrary people, but configure the SSH executable
+  directly.
+
+- _**Performance:**_ If `auto` is used, the whole file tree is scanned for a file, which is a very expensive operation
+  in terms of performance. If you use this functionality, the startup of your build agents will be significantly delayed
+  due to this file tree search. Do **not** use this functionality if this is an issue for you, but configure the SSH
+  executable directly.
 
 
 

diff --git a/agent/src/main/java/net/kautler/teamcity/ssh_tunnel/agent/SshDetector.java b/agent/src/main/java/net/kautler/teamcity/ssh_tunnel/agent/SshDetector.java
@@ -79,10 +79,21 @@ public void beforeAgentConfigurationLoaded(@NotNull BuildAgent agent) {
                 () -> Stream.of("ssh", "/usr/local/bin/ssh", "/usr/sbin/ssh", "/sbin/ssh", "/usr/bin/ssh", "/bin/ssh")
                         .filter(SshDetector::sshAvailableAtPath)
                         .findAny()
-                        .orElse(null),
-                () -> {
-                    LOG.warn("SSH executable not found in PATH or hard-coded paths. " +
+                        .orElse(null))
+                .map(Supplier::get)
+                .filter(Objects::nonNull)
+                .findFirst()
+                .map(possibleSshExecutable -> {
+                    if (!possibleSshExecutable.equals("auto")) {
+                        return possibleSshExecutable;
+                    }
+                    LOG.warn("SSH executable is configured to value 'auto'. " +
                                     "Going to search the whole file tree for an SSH executable. " +
+                                    "This might constitute a serious security risk if malicious builds " +
+                                    "leave a fake ssh executable on the file system that could steal your " +
+                                    "SSH key. Do not enable this if you run untrusted builds. " +
+                                    "Additionally this is a pretty heavy operation and will " +
+                                    "significantly slow down the agent startup. " +
                                     "To prevent this or if the wrong one is found, " +
                                     "you can configure the path to the SSH executable " +
                                     "using an agent configuration property or system property named '{}' " +
@@ -121,26 +132,24 @@ && sshAvailableAtPath(file.toString())) {
 
                                         @Override
                                         public FileVisitResult visitFileFailed(Path file, IOException exc) {
-                                            if (exc != null) {
-                                                if (LOG.isDebugEnabled()) {
-                                                    LOG.info("Exception during walking the file tree for '{}' at {}", rootDirectory, file, exc);
-                                                } else {
-                                                    LOG.debug("Exception during walking the file tree for '{}' at {}: {}", rootDirectory, file, exc.getMessage());
-                                                }
-                                            }
+                                            logVisitException(file, exc);
                                             return CONTINUE;
                                         }
 
                                         @Override
                                         public FileVisitResult postVisitDirectory(Path dir, IOException exc) {
+                                            logVisitException(dir, exc);
+                                            return CONTINUE;
+                                        }
+
+                                        private void logVisitException(Path path, IOException exc) {
                                             if (exc != null) {
                                                 if (LOG.isDebugEnabled()) {
-                                                    LOG.info("Exception during walking the file tree for '{}' at {}", rootDirectory, dir, exc);
+                                                    LOG.info("Exception during walking the file tree for '{}' at {}", rootDirectory, path, exc);
                                                 } else {
-                                                    LOG.debug("Exception during walking the file tree for '{}' at {}: {}", rootDirectory, dir, exc.getMessage());
+                                                    LOG.debug("Exception during walking the file tree for '{}' at {}: {}", rootDirectory, path, exc.getMessage());
                                                 }
                                             }
-                                            return CONTINUE;
                                         }
                                     });
                                     return sshExecutable.get().map(Stream::of).orElseGet(Stream::empty);
@@ -154,11 +163,11 @@ public FileVisitResult postVisitDirectory(Path dir, IOException exc) {
                                 }
                             })
                             .findAny()
-                            .orElse(null);
+                            .orElseGet(() -> {
+                                LOG.info("SSH executable is configured to value 'auto', but no SSH executable was found in the file tree");
+                                return null;
+                            });
                 })
-                .map(Supplier::get)
-                .filter(Objects::nonNull)
-                .findFirst()
                 .ifPresent(sshExecutable -> agent.getConfiguration().addConfigurationParameter(SSH_EXECUTABLE_CONFIGURATION_PARAMETER_NAME, sshExecutable));
     }