diff options
author | HÃ¥kon Hallingstad <hakon.hallingstad@gmail.com> | 2022-03-28 17:09:16 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-03-28 17:09:16 +0200 |
commit | 0c6c20f84b33d8e32465acfc0c0c14443fa01228 (patch) | |
tree | e23aa49530c600e9543eeeff327146e448cf494b | |
parent | 0ad0999caf32c04110390fef838b01d14474cd3c (diff) | |
parent | d150e94da2cc4a421d3ec57be660378cdbfb82c1 (diff) |
Merge pull request #21857 from vespa-engine/revert-21841-freva/secure-directory-stream
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); |