Merge pull request #22220 from vespa-engine/revert-22195-revert-22183-revert-22177-freva/revert

Reapply 2: "Set default permissions"

3 files changed, 75 insertions, 2 deletions

diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java
index 61e777a9576..75977da369c 100644
--- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java
+++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java
@@ -27,6 +27,9 @@ import com.yahoo.vespa.hosted.node.admin.maintenance.acl.AclMaintainer;
 import com.yahoo.vespa.hosted.node.admin.maintenance.identity.CredentialsMaintainer;
 import com.yahoo.vespa.hosted.node.admin.maintenance.servicedump.VespaServiceDumper;
 import com.yahoo.vespa.hosted.node.admin.nodeadmin.ConvergenceException;
+import com.yahoo.vespa.hosted.node.admin.task.util.file.FileFinder;
+import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath;
+import com.yahoo.vespa.hosted.node.admin.task.util.fs.ContainerPath;
 
 import java.time.Clock;
 import java.time.Duration;
@@ -137,6 +140,31 @@ public class NodeAgentImpl implements NodeAgent {
         if (loopThread != null)
             throw new IllegalStateException("Can not re-start a node agent.");
 
+        // TODO: Remove after this has rolled out everywhere
+        int[] stats = new int[]{0, 0, 0};
+        ContainerPath vespaHome = initialContext.paths().underVespaHome("");
+        FileFinder.files(initialContext.paths().of("/")).forEachPath(path -> {
+            UnixPath unixPath = new UnixPath(path);
+
+            String permissions = unixPath.getPermissions();
+            if (!permissions.endsWith("---")) {
+                unixPath.setPermissions(permissions.substring(0, 6) + "---");
+                stats[0]++;
+            }
+
+            if (path.startsWith(vespaHome) && unixPath.getOwnerId() != initialContext.users().vespa().uid()) {
+                unixPath.setOwnerId(initialContext.users().vespa().uid());
+                stats[1]++;
+            }
+
+            if (path.startsWith(vespaHome) && unixPath.getGroupId() != initialContext.users().vespa().gid()) {
+                unixPath.setGroupId(initialContext.users().vespa().gid());
+                stats[2]++;
+            }
+        });
+        if (stats[0] + stats[1] + stats[2] > 0)
+            initialContext.log(logger, "chmod %d, chown UID %d, chown GID %d files", stats[0], stats[1], stats[2]);
+
         loopThread = new Thread(() -> {
             while (!terminated.get()) {
                 try {
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 964ed5e0e4d..2a2e3d611c9 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
@@ -26,6 +26,8 @@ import java.nio.file.attribute.FileAttributeView;
 import java.nio.file.attribute.GroupPrincipal;
 import java.nio.file.attribute.PosixFileAttributeView;
 import java.nio.file.attribute.PosixFileAttributes;
+import java.nio.file.attribute.PosixFilePermission;
+import java.nio.file.attribute.PosixFilePermissions;
 import java.nio.file.attribute.UserPrincipal;
 import java.nio.file.spi.FileSystemProvider;
 import java.util.HashMap;
@@ -44,6 +46,12 @@ import static com.yahoo.yolean.Exceptions.uncheck;
  * @author freva
  */
 class ContainerFileSystemProvider extends FileSystemProvider {
+
+    private static final FileAttribute<?> DEFAULT_FILE_PERMISSIONS = PosixFilePermissions.asFileAttribute(Set.of( // 0640
+            PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE, PosixFilePermission.GROUP_READ));
+    private static final FileAttribute<?> DEFAULT_DIRECTORY_PERMISSIONS = PosixFilePermissions.asFileAttribute(Set.of( // 0750
+            PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE, PosixFilePermission.OWNER_EXECUTE, PosixFilePermission.GROUP_READ, PosixFilePermission.GROUP_EXECUTE));
+
     private final ContainerFileSystem containerFs;
     private final ContainerUserPrincipalLookupService userPrincipalLookupService;
 
@@ -82,7 +90,8 @@ class ContainerFileSystemProvider extends FileSystemProvider {
         Path pathOnHost = pathOnHost(path);
         try (SecureDirectoryStream<Path> sds = leafDirectoryStream(pathOnHost)) {
             boolean existedBefore = Files.exists(pathOnHost);
-            SeekableByteChannel seekableByteChannel = sds.newByteChannel(pathOnHost.getFileName(), addNoFollow(options), attrs);
+            SeekableByteChannel seekableByteChannel = sds.newByteChannel(
+                    pathOnHost.getFileName(), addNoFollow(options), addPermissions(DEFAULT_FILE_PERMISSIONS, attrs));
             if (!existedBefore) fixOwnerToContainerRoot(toContainerPath(path));
             return seekableByteChannel;
         }
@@ -99,7 +108,7 @@ class ContainerFileSystemProvider extends FileSystemProvider {
     public void createDirectory(Path dir, FileAttribute<?>... attrs) throws IOException {
         Path pathOnHost = pathOnHost(dir);
         boolean existedBefore = Files.exists(pathOnHost);
-        provider(pathOnHost).createDirectory(pathOnHost);
+        provider(pathOnHost).createDirectory(pathOnHost, addPermissions(DEFAULT_DIRECTORY_PERMISSIONS, attrs));
         if (!existedBefore) fixOwnerToContainerRoot(toContainerPath(dir));
     }
 
@@ -324,4 +333,16 @@ class ContainerFileSystemProvider extends FileSystemProvider {
         copy[options.length] = LinkOption.NOFOLLOW_LINKS;
         return copy;
     }
+
+    private static FileAttribute<?>[] addPermissions(FileAttribute<?> defaultPermissions, FileAttribute<?>... attrs) {
+        for (FileAttribute<?> attr : attrs) {
+            if (attr.name().equals("posix:permissions") || attr.name().equals("unix:permissions"))
+                return attrs;
+        }
+
+        FileAttribute<?>[] copy = new FileAttribute<?>[attrs.length + 1];
+        System.arraycopy(attrs, 0, copy, 0, attrs.length);
+        copy[attrs.length] = defaultPermissions;
+        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 c3affccc32b..b26f0fe5bf8 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
@@ -16,6 +16,8 @@ import java.nio.file.LinkOption;
 import java.nio.file.Path;
 import java.nio.file.StandardCopyOption;
 import java.nio.file.StandardOpenOption;
+import java.nio.file.attribute.FileAttribute;
+import java.nio.file.attribute.PosixFilePermissions;
 import java.util.Map;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -174,6 +176,19 @@ class ContainerFileSystemTest {
         Files.writeString(file.pathOnHost(), "hello"); // Writing through host FS works
     }
 
+    @Test
+    public void permissions() throws IOException {
+        assertPermissions(Files.createDirectory(containerFs.getPath("/dir1")), "rwxr-x---");
+        assertPermissions(Files.createDirectory(containerFs.getPath("/dir2"), permissionsFromString("r-x-w-rw-")), "r-x-w-rw-");
+
+        assertPermissions(Files.createDirectories(containerFs.getPath("/sub/dir/leaf"), permissionsFromString("r-x-w-rw-")), "r-x-w-rw-");
+        assertPermissions(containerFs.getPath("/sub/dir"), "r-x-w-rw-"); // Non-leafs get the same permission as the leaf
+
+        // TODO: Uncomment when JimFS forwards attributes for SecureDirectoryStream::newByteChannel
+//        assertPermissions(Files.createFile(containerFs.getPath("/file1")), "rw-r-----");
+//        assertPermissions(Files.createFile(containerFs.getPath("/file2"), permissionsFromString("r-x-w-rw-")), "r-x-w-rw-");
+    }
+
     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);
@@ -184,4 +199,13 @@ class ContainerFileSystemTest {
         assertEquals(uid, attrs.get("uid"));
         assertEquals(gid, attrs.get("gid"));
     }
+
+    private static void assertPermissions(Path path, String expected) throws IOException {
+        String actual = PosixFilePermissions.toString(Files.getPosixFilePermissions(path));
+        assertEquals(expected, actual);
+    }
+
+    private static FileAttribute<?> permissionsFromString(String permissions) {
+        return PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString(permissions));
+    }
 }