diff --git a/nimrodg-resources/src/test/java/au/edu/uq/rcc/nimrodg/resource/TestShell.java b/nimrodg-resources/src/test/java/au/edu/uq/rcc/nimrodg/resource/TestShell.java index 409898d2d..eef00e304 100644 --- a/nimrodg-resources/src/test/java/au/edu/uq/rcc/nimrodg/resource/TestShell.java +++ b/nimrodg-resources/src/test/java/au/edu/uq/rcc/nimrodg/resource/TestShell.java @@ -14,10 +14,9 @@ import java.nio.file.attribute.PosixFilePermission; import java.security.PublicKey; import java.time.Instant; -import java.util.Collection; -import java.util.HashSet; import java.util.List; import java.util.Optional; +import java.util.Set; public class TestShell implements RemoteShell { @@ -60,18 +59,10 @@ public CommandResult runCommand(String[] args, byte[] stdin) throws IOException } @Override - public void upload(String destPath, byte[] bytes, Collection perms, Instant timestamp) throws IOException { + public void upload(String destPath, byte[] bytes, Set perms, Instant timestamp) throws IOException { Path dp = fs.getPath(destPath); Files.write(dp, bytes); - Files.setPosixFilePermissions(dp, new HashSet<>(perms)); - Files.setLastModifiedTime(dp, FileTime.from(timestamp)); - } - - @Override - public void upload(String destPath, Path path, Collection perms, Instant timestamp) throws IOException { - Path dp = fs.getPath(destPath); - Files.copy(fs.getPath(path.toString()), dp); - Files.setPosixFilePermissions(dp, new HashSet<>(perms)); + Files.setPosixFilePermissions(dp, perms); Files.setLastModifiedTime(dp, FileTime.from(timestamp)); } diff --git a/nimrodg-shell/build.gradle b/nimrodg-shell/build.gradle index 9ff6804cd..861c227cf 100644 --- a/nimrodg-shell/build.gradle +++ b/nimrodg-shell/build.gradle @@ -5,6 +5,7 @@ dependencies { implementation group: 'org.apache.sshd', name: 'sshd-core', version: '2.+' implementation group: 'org.apache.sshd', name: 'sshd-scp', version: '2.+' + implementation group: 'org.apache.commons', name: 'commons-exec', version: '1.3' /* sshd-core has this as an "optional" dependency, so gradle doesn't pull it. */ implementation group: 'net.i2p.crypto', name: 'eddsa', version: '0.+' diff --git a/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/LocalShell.java b/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/LocalShell.java index 0c187fcbf..01f07b178 100644 --- a/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/LocalShell.java +++ b/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/LocalShell.java @@ -19,20 +19,22 @@ */ package au.edu.uq.rcc.nimrodg.shell; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.channels.ByteChannel; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; import java.nio.file.attribute.FileTime; import java.nio.file.attribute.PosixFilePermission; import java.time.Instant; -import java.util.Collection; import java.util.Collections; -import java.util.HashSet; import java.util.Map; +import java.util.Set; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; public class LocalShell implements RemoteShell { @@ -44,10 +46,13 @@ public CommandResult runCommand(String[] args, byte[] stdin) throws IOException } @Override - public void upload(String destPath, byte[] bytes, Collection perms, Instant timestamp) throws IOException { + public void upload(String destPath, byte[] bytes, Set perms, Instant timestamp) throws IOException { Path path = Paths.get(destPath); - Files.write(path, bytes); - Files.setPosixFilePermissions(path, new HashSet<>(perms)); + + try(ByteChannel c = ShellUtils.newByteChannelSafe(path, perms)) { + c.write(ByteBuffer.wrap(bytes)); + } + Files.setLastModifiedTime(path, FileTime.from(timestamp)); } diff --git a/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/OpenSSHClient.java b/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/OpenSSHClient.java index b99180828..ba8940795 100644 --- a/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/OpenSSHClient.java +++ b/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/OpenSSHClient.java @@ -19,32 +19,33 @@ */ package au.edu.uq.rcc.nimrodg.shell; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.net.URI; import java.net.URISyntaxException; +import java.nio.ByteBuffer; +import java.nio.channels.ByteChannel; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; -import java.nio.file.StandardOpenOption; import java.nio.file.attribute.PosixFilePermission; import java.time.Instant; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.EnumSet; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - /** * A wrapper around OpenSSH. This entire class is a glorious hack. */ @@ -97,7 +98,11 @@ public OpenSSHClient(URI uri, Path workDir, Optional privateKey, Optional< if(privateKey.isPresent()) { /* The key may be on another filesystem, so make a "local" copy of it. */ this.privateKey = Optional.of(workDir.resolve(String.format("openssh-%d-key", (long)uri.hashCode() & 0xFFFFFFFFL))); - copyPrivateKey(privateKey.get(), this.privateKey.get()); + + try(ByteChannel c = ShellUtils.newByteChannelSafe(this.privateKey.get(), EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE))) { + c.write(ByteBuffer.wrap(Files.readAllBytes(privateKey.get()))); + } + ssh.add("-i"); ssh.add(this.privateKey.get().toString()); } else { @@ -246,7 +251,7 @@ private String readNextLine(InputStream is) throws IOException { } @Override - public void upload(String destPath, byte[] bytes, Collection perms, Instant timestamp) throws IOException { + public void upload(String destPath, byte[] bytes, Set perms, Instant timestamp) throws IOException { int operms = ShellUtils.permissionsToInt(perms); this.runSsh(new String[]{ @@ -322,29 +327,6 @@ public void close() throws IOException { ShellUtils.doProcessOneshot(closeArgs, LOGGER); } - private static void copyPrivateKey(Path src, Path dest) throws IOException { - Files.deleteIfExists(dest); - Files.createFile(dest); - - try { - Files.setPosixFilePermissions(dest, EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE)); - } catch(UnsupportedOperationException e) { - LOGGER.warn("Unable to secure private key, POSIX permissions not supported", e); - } - - try(OutputStream os = Files.newOutputStream(dest, StandardOpenOption.TRUNCATE_EXISTING, StandardOpenOption.WRITE)) { - os.write(Files.readAllBytes(src)); - } - - /* - try(ByteChannel c = Files.newByteChannel(dest, - EnumSet.of(StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE), - PosixFilePermissions.asFileAttribute(perms))) { - c.write(ByteBuffer.wrap(Files.readAllBytes(src))); - } - */ - } - public static void main(String[] args) throws IOException { try(OpenSSHClient c = new OpenSSHClient(URI.create("ssh://0.0.0.0"), Paths.get("/tmp"), Optional.empty(), Optional.empty(), Map.of())) { diff --git a/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/RemoteShell.java b/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/RemoteShell.java index fc19076fe..6ed09d98a 100644 --- a/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/RemoteShell.java +++ b/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/RemoteShell.java @@ -25,11 +25,8 @@ import java.nio.file.Path; import java.nio.file.attribute.PosixFilePermission; import java.time.Instant; -import java.util.Collection; -import java.util.HashMap; import java.util.Map; -import java.util.regex.Matcher; -import java.util.regex.Pattern; +import java.util.Set; /** * A small interface around a remote system. Used so there can be multiple transports, i.e. Mina SSHD and a local ssh @@ -58,9 +55,9 @@ default CommandResult runCommand(String... args) throws IOException { CommandResult runCommand(String[] args, byte[] stdin) throws IOException; - void upload(String destPath, byte[] bytes, Collection perms, Instant timestamp) throws IOException; + void upload(String destPath, byte[] bytes, Set perms, Instant timestamp) throws IOException; - default void upload(String destPath, Path path, Collection perms, Instant timestamp) throws IOException { + default void upload(String destPath, Path path, Set perms, Instant timestamp) throws IOException { upload(destPath, Files.readAllBytes(path), perms, timestamp); } diff --git a/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/ShellUtils.java b/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/ShellUtils.java index bd0cbc53e..7ca279c80 100644 --- a/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/ShellUtils.java +++ b/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/ShellUtils.java @@ -1,18 +1,31 @@ package au.edu.uq.rcc.nimrodg.shell; +import org.apache.commons.exec.CommandLine; +import org.apache.commons.exec.DefaultExecutor; +import org.apache.commons.exec.ExecuteException; +import org.apache.commons.exec.Executor; +import org.apache.commons.exec.PumpStreamHandler; +import org.apache.commons.exec.util.StringUtils; import org.apache.sshd.common.config.keys.AuthorizedKeyEntry; import org.apache.sshd.common.config.keys.PublicKeyEntryResolver; import org.slf4j.Logger; -import java.io.BufferedInputStream; -import java.io.BufferedOutputStream; +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; import java.io.IOException; import java.net.URI; +import java.nio.channels.ByteChannel; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.OpenOption; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.nio.file.attribute.FileAttribute; import java.nio.file.attribute.PosixFilePermission; +import java.nio.file.attribute.PosixFilePermissions; import java.security.GeneralSecurityException; import java.security.PublicKey; -import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; @@ -20,10 +33,15 @@ import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.StringTokenizer; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.stream.Collectors; +/** + * Various utility functions. + * + * Some of these are thin wrappers around commons-exec to avoid exposing the dependency. + */ @SuppressWarnings("OptionalUsedAsFieldOrParameterType") public class ShellUtils { @@ -44,21 +62,16 @@ public static Optional getUriUser(Optional uri) { } public static RemoteShell.CommandResult doProcessOneshot(Process p, String[] args, byte[] input) throws IOException { - BufferedOutputStream stdin = new BufferedOutputStream(p.getOutputStream()); - BufferedInputStream stdout = new BufferedInputStream(p.getInputStream()); - BufferedInputStream stderr = new BufferedInputStream(p.getErrorStream()); + ByteArrayInputStream stdin = new ByteArrayInputStream(input); + ByteArrayOutputStream stdout = new ByteArrayOutputStream(); + ByteArrayOutputStream stderr = new ByteArrayOutputStream(); - /* TODO: Do this properly in threads to avoid blocking. */ - if(input.length > 0) { - stdin.write(input); - } - stdin.close(); - - byte[] out = stdout.readAllBytes(); - byte[] err = stderr.readAllBytes(); + PumpStreamHandler psh = new PumpStreamHandler(stdout, stderr, stdin); + psh.setProcessOutputStream(p.getInputStream()); + psh.setProcessErrorStream(p.getErrorStream()); + psh.setProcessInputStream(p.getOutputStream()); - String output = new String(out, StandardCharsets.UTF_8); - String error = new String(err, StandardCharsets.UTF_8).trim(); + psh.start(); while(p.isAlive()) { try { @@ -67,30 +80,57 @@ public static RemoteShell.CommandResult doProcessOneshot(Process p, String[] arg /* nop */ } } - return new RemoteShell.CommandResult(buildEscapedCommandLine(args), p.exitValue(), output, error); - } - public static RemoteShell.CommandResult doProcessOneshot(Process p, String[] args) throws IOException { - return doProcessOneshot(p, args, new byte[0]); + psh.stop(); + + return new RemoteShell.CommandResult( + buildEscapedCommandLine(args), + p.exitValue(), + stdout.toString(StandardCharsets.UTF_8), + stderr.toString(StandardCharsets.UTF_8) + ); } - public static RemoteShell.CommandResult doProcessOneshot(String[] args, byte[] input, Logger logger) throws IOException { - ProcessBuilder pb = new ProcessBuilder(args); - pb.redirectOutput(ProcessBuilder.Redirect.PIPE); - pb.redirectError(ProcessBuilder.Redirect.PIPE); - pb.redirectInput(ProcessBuilder.Redirect.PIPE); + public static RemoteShell.CommandResult doProcessOneshot(CommandLine cmd, byte[] input, Logger logger) throws IOException { + String cmdline = buildEscapedCommandLine(cmd.toStrings()); + logger.trace("Executing command: {}", cmdline); - logger.trace("Executing command: {}", buildEscapedCommandLine(args)); + ByteArrayInputStream stdin = new ByteArrayInputStream(input); + ByteArrayOutputStream stdout = new ByteArrayOutputStream(); + ByteArrayOutputStream stderr = new ByteArrayOutputStream(); - Process p = pb.start(); + Executor exec = new DefaultExecutor(); + exec.setStreamHandler(new PumpStreamHandler(stdout, stderr, stdin)); + exec.setExitValues(null); + + int rv; try { - return doProcessOneshot(p, args, input); - } catch(IOException e) { - logger.error(new String(p.getErrorStream().readAllBytes(), StandardCharsets.UTF_8)); - throw e; - } finally { - p.destroyForcibly(); + rv = exec.execute(cmd); + } catch(ExecuteException e) { + Throwable t = e.getCause(); + if(t != null) { + if(t instanceof IOException) { + throw (IOException)t; + } + throw new IOException(t); + } + rv = e.getExitValue(); + } + + return new RemoteShell.CommandResult(cmdline, rv, stdout.toString(StandardCharsets.UTF_8), stderr.toString(StandardCharsets.UTF_8)); + } + + public static RemoteShell.CommandResult doProcessOneshot(String[] args, byte[] input, Logger logger) throws IOException { + if(args.length == 0) { + throw new IllegalArgumentException(); + } + + CommandLine cmd = new CommandLine(args[0]); + for(int i = 1; i < args.length; ++i) { + cmd.addArgument(args[i]); } + + return doProcessOneshot(cmd, input, logger); } public static RemoteShell.CommandResult doProcessOneshot(String[] args, Logger logger) throws IOException { @@ -98,20 +138,11 @@ public static RemoteShell.CommandResult doProcessOneshot(String[] args, Logger l } public static String buildEscapedCommandLine(List args) { - return buildEscapedCommandLine(args.toArray(new String[args.size()])); + return args.stream().map(ShellUtils::quoteArgument).collect(Collectors.joining(" ")); } public static String buildEscapedCommandLine(String... args) { - StringBuilder sb = new StringBuilder(); - - for(int i = 0; i < args.length; ++i) { - sb.append(quoteArgument(args[i])); - if(i != args.length - 1) { - sb.append(' '); - } - } - - return sb.toString(); + return Arrays.stream(args).map(ShellUtils::quoteArgument).collect(Collectors.joining(" ")); } private static final Pattern ENV_PATTERN = Pattern.compile("^([A-Za-z_][A-Za-z0-9_+]*)=(.*)$"); @@ -227,126 +258,34 @@ public static int permissionsToInt(Collection perms) { return operms; } - /** - * Put quotes around the given String if necessary. - *

- * If the argument doesn't include spaces or quotes, return it as is. If it contains double quotes, use single - * quotes - else surround the argument by double quotes. - *

- * - * @param argument the argument to be quoted - * @return the quoted argument - * @throws IllegalArgumentException If argument contains both types of quotes - *

- * Ripped from Apache commons-exec - * https://github.com/apache/commons-exec/blob/trunk/src/main/java/org/apache/commons/exec/util/StringUtils.java - *

- * NOTICE: Apache Commons Exec Copyright 2005-2016 The Apache Software Foundation - *

- * This product includes software developed at The Apache Software Foundation (http://www.apache.org/). - */ public static String quoteArgument(final String argument) { + return StringUtils.quoteArgument(argument); + } - final String SINGLE_QUOTE = "\'"; - final String DOUBLE_QUOTE = "\""; - - String cleanedArgument = argument.trim(); - - // strip the quotes from both ends - while(cleanedArgument.startsWith(SINGLE_QUOTE) || cleanedArgument.startsWith(DOUBLE_QUOTE)) { - cleanedArgument = cleanedArgument.substring(1); - } + public static String[] translateCommandline(String toProcess) { + return CommandLine.parse(toProcess).toStrings(); + } - while(cleanedArgument.endsWith(SINGLE_QUOTE) || cleanedArgument.endsWith(DOUBLE_QUOTE)) { - cleanedArgument = cleanedArgument.substring(0, cleanedArgument.length() - 1); - } + /* Sheer and utter paranoia. */ + public static ByteChannel newByteChannelSafe(Path path, Set perms) throws IOException { + Set opts = Set.of(StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE); - final StringBuilder buf = new StringBuilder(); - if(cleanedArgument.contains(DOUBLE_QUOTE)) { - if(cleanedArgument.contains(SINGLE_QUOTE)) { - throw new IllegalArgumentException("Can't handle single and double quotes in same argument"); - } - return buf.append(SINGLE_QUOTE).append(cleanedArgument).append(SINGLE_QUOTE).toString(); - } else if(cleanedArgument.contains(SINGLE_QUOTE) || cleanedArgument.contains(" ")) { - return buf.append(DOUBLE_QUOTE).append(cleanedArgument).append(DOUBLE_QUOTE).toString(); + FileAttribute[] atts; + if(path.getFileSystem().supportedFileAttributeViews().contains("posix")) { + atts = new FileAttribute[]{ PosixFilePermissions.asFileAttribute(perms) }; } else { - return cleanedArgument; + /* TODO: Convert perms to what they should be. */ + atts = new FileAttribute[0]; } - } - - /** - * Crack a command line. - * - * @param toProcess the command line to process. - * @return the command line broken into strings. An empty or null toProcess parameter results in a zero sized array. - *

- * Taken from https://github.com/apache/ant/blob/master/src/main/org/apache/tools/ant/types/Commandline.java - * Revision 790e27474ff11b42f1d3f355fa8b0d34be10e321 Changed to throw IllegalArgumentException instead of - * BuildException - *

- * NOTICE: Apache Ant Copyright 1999-2018 The Apache Software Foundation - *

- * This product includes software developed at The Apache Software Foundation (http://www.apache.org/). - */ - public static String[] translateCommandline(String toProcess) { - if(toProcess == null || toProcess.isEmpty()) { - //no command? no string - return new String[0]; - } - // parse with a simple finite state machine - - final int normal = 0; - final int inQuote = 1; - final int inDoubleQuote = 2; - int state = normal; - final StringTokenizer tok = new StringTokenizer(toProcess, "\"\' ", true); - final ArrayList result = new ArrayList<>(); - final StringBuilder current = new StringBuilder(); - boolean lastTokenHasBeenQuoted = false; - - while(tok.hasMoreTokens()) { - String nextTok = tok.nextToken(); - switch(state) { - case inQuote: - if("\'".equals(nextTok)) { - lastTokenHasBeenQuoted = true; - state = normal; - } else { - current.append(nextTok); - } - break; - case inDoubleQuote: - if("\"".equals(nextTok)) { - lastTokenHasBeenQuoted = true; - state = normal; - } else { - current.append(nextTok); - } - break; - default: - if("\'".equals(nextTok)) { - state = inQuote; - } else if("\"".equals(nextTok)) { - state = inDoubleQuote; - } else if(" ".equals(nextTok)) { - if(lastTokenHasBeenQuoted || current.length() > 0) { - result.add(current.toString()); - current.setLength(0); - } - } else { - current.append(nextTok); - } - lastTokenHasBeenQuoted = false; - break; - } - } - if(lastTokenHasBeenQuoted || current.length() > 0) { - result.add(current.toString()); - } - if(state == inQuote || state == inDoubleQuote) { - throw new IllegalArgumentException("unbalanced quotes in " + toProcess); - } - return result.toArray(new String[result.size()]); + /* + * Explicitly delete the file, as Files.newByteChannel() doesn't apply the attributes if it does. + * Also, an adversary may already have an open file handle. + * + * There is a *slight* race here, but Java doesn't provide a way to do this atomically. + * If the race does happen, and someone's created a file in the same place, this will throw. + */ + Files.deleteIfExists(path); + return Files.newByteChannel(path, opts, atts); } } diff --git a/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/SshdClient.java b/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/SshdClient.java index a0f00a068..e2c526bfa 100644 --- a/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/SshdClient.java +++ b/nimrodg-shell/src/main/java/au/edu/uq/rcc/nimrodg/shell/SshdClient.java @@ -48,9 +48,9 @@ import java.time.Instant; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collection; import java.util.EnumSet; import java.util.List; +import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; @@ -101,7 +101,6 @@ public SshdClient(URI uri, PublicKey[] hostKeys, KeyPair keyPair) throws IOExcep client = ClientBuilder.builder() .serverKeyVerifier((ClientSession arg0, SocketAddress arg1, PublicKey key) -> { - for(int i = 0; i < hostKeys.length; ++i) { if(hostKeys[i].equals(key)) { return true; @@ -164,7 +163,7 @@ public CommandResult runCommand(String[] args, byte[] stdin) throws IOException } @Override - public void upload(String destPath, byte[] bytes, Collection perms, Instant timestamp) throws IOException { + public void upload(String destPath, byte[] bytes, Set perms, Instant timestamp) throws IOException { long ms = timestamp.getEpochSecond(); scp.upload(bytes, destPath, perms, new ScpTimestamp(ms, ms));