Revert "Use SecureDirectoryStream in container FS"

2 files changed, 16 insertions, 98 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/fs/ContainerFileSystemProvider.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/fs/ContainerFileSystemProvider.java
index 843503d1cbf..5405a5acd61 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/fs/ContainerFileSystemProvider.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/fs/ContainerFileSystemProvider.java
@@ -18,7 +18,6 @@ import java.nio.file.LinkOption;
 import java.nio.file.OpenOption;
 import java.nio.file.Path;
 import java.nio.file.ProviderMismatchException;
-import java.nio.file.SecureDirectoryStream;
 import java.nio.file.attribute.BasicFileAttributeView;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.nio.file.attribute.FileAttribute;
@@ -29,7 +28,6 @@ import java.nio.file.attribute.PosixFileAttributes;
 import java.nio.file.attribute.UserPrincipal;
 import java.nio.file.spi.FileSystemProvider;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.Iterator;
 import java.util.Map;
 import java.util.Set;
@@ -80,12 +78,10 @@ class ContainerFileSystemProvider extends FileSystemProvider {
     @Override
     public SeekableByteChannel newByteChannel(Path path, Set<? extends OpenOption> options, FileAttribute<?>... attrs) throws IOException {
         Path pathOnHost = pathOnHost(path);
-        try (SecureDirectoryStream<Path> sds = leafDirectoryStream(pathOnHost)) {
-            boolean existedBefore = Files.exists(pathOnHost);
-            SeekableByteChannel seekableByteChannel = sds.newByteChannel(pathOnHost.getFileName(), addNoFollow(options), attrs);
-            if (!existedBefore) fixOwnerToContainerRoot(toContainerPath(path));
-            return seekableByteChannel;
-        }
+        boolean existedBefore = Files.exists(pathOnHost);
+        SeekableByteChannel seekableByteChannel = provider(pathOnHost).newByteChannel(pathOnHost, options, attrs);
+        if (!existedBefore) fixOwnerToContainerRoot(toContainerPath(path));
+        return seekableByteChannel;
     }
 
     @Override
@@ -113,16 +109,14 @@ class ContainerFileSystemProvider extends FileSystemProvider {
     public void copy(Path source, Path target, CopyOption... options) throws IOException {
         // Only called when both 'source' and 'target' have 'this' as the FS provider
         Path targetPathOnHost = pathOnHost(target);
-        provider(targetPathOnHost).copy(pathOnHost(source), targetPathOnHost, addNoFollow(options));
+        provider(targetPathOnHost).copy(pathOnHost(source), targetPathOnHost, options);
     }
 
     @Override
     public void move(Path source, Path target, CopyOption... options) throws IOException {
         // Only called when both 'source' and 'target' have 'this' as the FS provider
         Path targetPathOnHost = pathOnHost(target);
-        try (SecureDirectoryStream<Path> sds = leafDirectoryStream(targetPathOnHost)) {
-            sds.move(pathOnHost(source), sds, targetPathOnHost.getFileName());
-        }
+        provider(targetPathOnHost).move(pathOnHost(source), targetPathOnHost, options);
     }
 
     @Override
@@ -173,12 +167,12 @@ class ContainerFileSystemProvider extends FileSystemProvider {
         if (!type.isAssignableFrom(PosixFileAttributeView.class)) return null;
         Path pathOnHost = pathOnHost(path);
         FileSystemProvider provider = pathOnHost.getFileSystem().provider();
-        if (type == BasicFileAttributeView.class) // Basic view doesn't have owner/group fields, forward to base FS provider
-            return provider.getFileAttributeView(pathOnHost, type, addNoFollow(options));
+        if (type == BasicFileAttributeView.class) // Basic view doesnt have owner/group fields, forward to base FS provider
+            return provider.getFileAttributeView(pathOnHost, type, options);
 
-        PosixFileAttributeView view = provider.getFileAttributeView(pathOnHost, PosixFileAttributeView.class, addNoFollow(options));
+        PosixFileAttributeView view = provider.getFileAttributeView(pathOnHost, PosixFileAttributeView.class, options);
         return (V) new ContainerPosixFileAttributeView(view,
-                uncheck(() -> new ContainerPosixFileAttributes(readAttributes(path, "unix:*", addNoFollow(options)))));
+                uncheck(() -> new ContainerPosixFileAttributes(readAttributes(path, "unix:*", options))));
     }
 
     @Override
@@ -187,10 +181,10 @@ class ContainerFileSystemProvider extends FileSystemProvider {
         if (!type.isAssignableFrom(PosixFileAttributes.class)) throw new UnsupportedOperationException();
         Path pathOnHost = pathOnHost(path);
         if (type == BasicFileAttributes.class)
-            return pathOnHost.getFileSystem().provider().readAttributes(pathOnHost, type, addNoFollow(options));
+            return pathOnHost.getFileSystem().provider().readAttributes(pathOnHost, type, options);
 
         // Non-basic requests need to be upgraded to unix:* to get owner,group,uid,gid fields, which are then re-mapped
-        return (A) new ContainerPosixFileAttributes(readAttributes(path, "unix:*", addNoFollow(options)));
+        return (A) new ContainerPosixFileAttributes(readAttributes(path, "unix:*", options));
     }
 
     @Override
@@ -198,9 +192,9 @@ class ContainerFileSystemProvider extends FileSystemProvider {
         Path pathOnHost = pathOnHost(path);
         int index = attributes.indexOf(':');
         if (index < 0 || attributes.startsWith("basic:"))
-            return provider(pathOnHost).readAttributes(pathOnHost, attributes, addNoFollow(options));
+            return provider(pathOnHost).readAttributes(pathOnHost, attributes, options);
 
-        Map<String, Object> attrs = new HashMap<>(provider(pathOnHost).readAttributes(pathOnHost, "unix:*", addNoFollow(options)));
+        Map<String, Object> attrs = new HashMap<>(provider(pathOnHost).readAttributes(pathOnHost, "unix:*", options));
         int uid = userPrincipalLookupService.userIdInContainer((int) attrs.get("uid"));
         int gid = userPrincipalLookupService.groupIdInContainer((int) attrs.get("gid"));
         attrs.put("uid", uid);
@@ -213,7 +207,7 @@ class ContainerFileSystemProvider extends FileSystemProvider {
     @Override
     public void setAttribute(Path path, String attribute, Object value, LinkOption... options) throws IOException {
         Path pathOnHost = pathOnHost(path);
-        provider(pathOnHost).setAttribute(pathOnHost, attribute, fixAttributeValue(attribute, value), addNoFollow(options));
+        provider(pathOnHost).setAttribute(pathOnHost, attribute, fixAttributeValue(attribute, value), options);
     }
 
     private Object fixAttributeValue(String attribute, Object value) {
@@ -244,17 +238,6 @@ class ContainerFileSystemProvider extends FileSystemProvider {
         setAttribute(path, "unix:gid", path.user().gid(), LinkOption.NOFOLLOW_LINKS);
     }
 
-    private SecureDirectoryStream<Path> leafDirectoryStream(Path pathOnHost) throws IOException {
-        Path containerRoot = containerFs.containerRootOnHost();
-        SecureDirectoryStream<Path> sds = ((SecureDirectoryStream<Path>) Files.newDirectoryStream(containerRoot));
-        for (int i = containerRoot.getNameCount(); i < pathOnHost.getNameCount() - 1; i++) {
-            SecureDirectoryStream<Path> next = sds.newDirectoryStream(pathOnHost.getName(i), LinkOption.NOFOLLOW_LINKS);
-            sds.close();
-            sds = next;
-        }
-        return sds;
-    }
-
     private class ContainerDirectoryStream implements DirectoryStream<Path> {
         private final DirectoryStream<Path> hostDirectoryStream;
         private final UnixUser user;
@@ -287,6 +270,7 @@ class ContainerFileSystemProvider extends FileSystemProvider {
         }
     }
 
+
     static ContainerPath toContainerPath(Path path) {
         return cast(path, ContainerPath.class);
     }
@@ -303,27 +287,4 @@ class ContainerFileSystemProvider extends FileSystemProvider {
     private static FileSystemProvider provider(Path path) {
         return path.getFileSystem().provider();
     }
-
-    private static Set<? extends OpenOption> addNoFollow(Set<? extends OpenOption> options) {
-        if (options.contains(LinkOption.NOFOLLOW_LINKS)) return options;
-        Set<OpenOption> copy = new HashSet<>(options);
-        copy.add(LinkOption.NOFOLLOW_LINKS);
-        return copy;
-    }
-
-    private static LinkOption[] addNoFollow(LinkOption... options) {
-        if (Set.of(options).contains(LinkOption.NOFOLLOW_LINKS)) return options;
-        LinkOption[] copy = new LinkOption[options.length + 1];
-        System.arraycopy(options, 0, copy, 0, options.length);
-        copy[options.length] = LinkOption.NOFOLLOW_LINKS;
-        return copy;
-    }
-
-    private static CopyOption[] addNoFollow(CopyOption... options) {
-        if (Set.of(options).contains(LinkOption.NOFOLLOW_LINKS)) return options;
-        CopyOption[] copy = new CopyOption[options.length + 1];
-        System.arraycopy(options, 0, copy, 0, options.length);
-        copy[options.length] = LinkOption.NOFOLLOW_LINKS;
-        return copy;
-    }
 }
diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/fs/ContainerFileSystemTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/fs/ContainerFileSystemTest.java
index 07ef0b80d91..b5f2ef41a1a 100644
--- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/fs/ContainerFileSystemTest.java
+++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/task/util/fs/ContainerFileSystemTest.java
@@ -19,8 +19,6 @@ import java.nio.file.StandardOpenOption;
 import java.util.Map;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertThrows;
 
 /**
  * @author freva
@@ -142,47 +140,6 @@ class ContainerFileSystemTest {
         assertOwnership(source, 0, 0, 10000, 11000);
     }
 
-    @Test
-    public void disallow_operations_on_symlink() throws IOException {
-        Path destination = fileSystem.getPath("/dir/file");
-        Files.createDirectories(destination.getParent());
-
-        ContainerPath link = containerFs.getPath("/link");
-        Files.createSymbolicLink(link, destination);
-
-        // Cannot write file via symlink
-        assertThrows(IOException.class, () -> Files.writeString(link, "hello"));
-
-        assertOwnership(link, 0, 0, 10_000, 11_000);
-        Files.setAttribute(link, "unix:uid", 10); // This succeeds because attribute is set on the link (destination does not exist)
-        assertFalse(Files.exists(destination));
-        assertOwnership(link, 10, 0, 10_010, 11_000);
-    }
-
-    @Test
-    public void disallow_operations_on_parent_symlink() throws IOException {
-        Path destination = fileSystem.getPath("/dir/sub/folder");
-        Files.createDirectories(destination.getParent());
-
-        // Create symlink /some/dir/link -> /dir/sub
-        ContainerPath link = containerFs.getPath("/some/dir/link");
-        Files.createDirectories(link.getParent());
-        Files.createSymbolicLink(link, destination.getParent());
-
-        { // Cannot write file via symlink
-            ContainerPath file = link.resolve("file");
-            assertThrows(IOException.class, () -> Files.writeString(file, "hello"));
-            Files.writeString(file.pathOnHost(), "hello"); // Writing through host FS works
-        }
-
-        { // Cannot move via symlink
-            ContainerPath file = containerFs.getPath("/file");
-            Files.writeString(file, "world");
-            assertThrows(IOException.class, () -> Files.move(file, link.resolve("dest")));
-            Files.move(file.pathOnHost(), link.resolve("dest").pathOnHost()); // Moving through host FS works
-        }
-    }
-
     private static void assertOwnership(ContainerPath path, int contUid, int contGid, int hostUid, int hostGid) throws IOException {
         assertOwnership(path, contUid, contGid);
         assertOwnership(path.pathOnHost(), hostUid, hostGid);