nimrodg-shell: hardening

* Add dependency on org.apache.commons:commons-exec:1.3
  - Safer, more-robust process code.
  - Can remove our copied version of StringUtils#quoteArgument().
* Add ShellUtils#newByteChannelSafe(), attempts to safely create
  a byte channel with the correct permissions.

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<PosixFilePermission> perms, Instant timestamp) throws IOException {
+	public void upload(String destPath, byte[] bytes, Set<PosixFilePermission> 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<PosixFilePermission> 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));
 	}
 

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.+'

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<PosixFilePermission> perms, Instant timestamp) throws IOException {
+	public void upload(String destPath, byte[] bytes, Set<PosixFilePermission> 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));
 	}
 

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<Path> 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<PosixFilePermission> perms, Instant timestamp) throws IOException {
+    public void upload(String destPath, byte[] bytes, Set<PosixFilePermission> 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())) {
 

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<PosixFilePermission> perms, Instant timestamp) throws IOException;
+	void upload(String destPath, byte[] bytes, Set<PosixFilePermission> perms, Instant timestamp) throws IOException;
 
-	default void upload(String destPath, Path path, Collection<PosixFilePermission> perms, Instant timestamp) throws IOException {
+	default void upload(String destPath, Path path, Set<PosixFilePermission> perms, Instant timestamp) throws IOException {
 		upload(destPath, Files.readAllBytes(path), perms, timestamp);
 	}