[Entitlements] Fix FileAccessTree paths ordering (#123689) (#123850)

libs/entitlement/src/main/java/org/elasticsearch/entitlement/initialization/EntitlementInitialization.java

@@ -65,11 +65,11 @@ import java.util.stream.Collectors;
 import java.util.stream.Stream;
 import java.util.stream.StreamSupport;
 
+import static org.elasticsearch.entitlement.runtime.policy.Platform.LINUX;
 import static org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement.BaseDir.DATA;
 import static org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement.BaseDir.SHARED_REPO;
 import static org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement.Mode.READ;
 import static org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement.Mode.READ_WRITE;
-import static org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement.Platform.LINUX;
 
 /**
  * Called by the agent during {@code agentmain} to configure the entitlement system,

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTree.java

@@ -22,12 +22,12 @@ import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.Comparator;
 import java.util.List;
 import java.util.Objects;
 import java.util.function.BiConsumer;
 
 import static org.elasticsearch.core.PathUtils.getDefaultFileSystem;
+import static org.elasticsearch.entitlement.runtime.policy.FileUtils.PATH_ORDER;
 
 public final class FileAccessTree {
 
@@ -238,30 +238,4 @@ public final class FileAccessTree {
     public int hashCode() {
         return Objects.hash(Arrays.hashCode(readPaths), Arrays.hashCode(writePaths));
     }
-
-    /**
-     * For our lexicographic sort trick to work correctly, we must have path separators sort before
-     * any other character so that files in a directory appear immediately after that directory.
-     * For example, we require [/a, /a/b, /a.xml] rather than the natural order [/a, /a.xml, /a/b].
-     */
-    private static final Comparator<String> PATH_ORDER = (s1, s2) -> {
-        Path p1 = Path.of(s1);
-        Path p2 = Path.of(s2);
-        var i1 = p1.iterator();
-        var i2 = p2.iterator();
-        while (i1.hasNext() && i2.hasNext()) {
-            int cmp = i1.next().compareTo(i2.next());
-            if (cmp != 0) {
-                return cmp;
-            }
-        }
-        if (i1.hasNext()) {
-            return 1;
-        } else if (i2.hasNext()) {
-            return -1;
-        } else {
-            assert p1.equals(p2);
-            return 0;
-        }
-    };
 }

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileUtils.java

@@ -0,0 +1,107 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the "Elastic License
+ * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
+ * Public License v 1"; you may not use this file except in compliance with, at
+ * your election, the "Elastic License 2.0", the "GNU Affero General Public
+ * License v3.0 only", or the "Server Side Public License, v 1".
+ */
+
+package org.elasticsearch.entitlement.runtime.policy;
+
+import org.elasticsearch.core.SuppressForbidden;
+
+import java.io.File;
+import java.util.Comparator;
+
+import static java.lang.Character.isLetter;
+
+public class FileUtils {
+
+    private FileUtils() {}
+
+    /**
+     * For our lexicographic sort trick to work correctly, we must have path separators sort before
+     * any other character so that files in a directory appear immediately after that directory.
+     * For example, we require [/a, /a/b, /a.xml] rather than the natural order [/a, /a.xml, /a/b].
+     */
+    static final Comparator<String> PATH_ORDER = (s1, s2) -> {
+        int len1 = s1.length();
+        int len2 = s2.length();
+        int lim = Math.min(len1, len2);
+        for (int k = 0; k < lim; k++) {
+            char c1 = s1.charAt(k);
+            char c2 = s2.charAt(k);
+            if (c1 == c2) {
+                continue;
+            }
+            boolean c1IsSeparator = isPathSeparator(c1);
+            boolean c2IsSeparator = isPathSeparator(c2);
+            if (c1IsSeparator == false || c2IsSeparator == false) {
+                if (c1IsSeparator) {
+                    return -1;
+                }
+                if (c2IsSeparator) {
+                    return 1;
+                }
+                return c1 - c2;
+            }
+        }
+        return len1 - len2;
+    };
+
+    @SuppressForbidden(reason = "we need the separator as a char, not a string")
+    private static boolean isPathSeparator(char c) {
+        return c == File.separatorChar;
+    }
+
+    /**
+     * Tests if a path is absolute or relative, taking into consideration both Unix and Windows conventions.
+     * Note that this leads to a conflict, resolved in favor of Unix rules: `/foo` can be either a Unix absolute path, or a Windows
+     * relative path with "wrong" directory separator (using non-canonical / in Windows).
+     * This method is intended to be used as validation for different file entitlements format: therefore it is preferable to reject a
+     * relative path that is definitely absolute on Unix, rather than accept it as a possible relative path on Windows (if that is the case,
+     * the developer can easily fix the path by using the correct platform separators).
+     */
+    public static boolean isAbsolutePath(String path) {
+        if (path.isEmpty()) {
+            return false;
+        }
+        if (path.charAt(0) == '/') {
+            // Unix/BSD absolute
+            return true;
+        }
+
+        return isWindowsAbsolutePath(path);
+    }
+
+    /**
+     * When testing for path separators in a platform-agnostic way, we may encounter both kinds of slashes, especially when
+     * processing windows paths. The JDK parses paths the same way under Windows.
+     */
+    static boolean isSlash(char c) {
+        return (c == '\\') || (c == '/');
+    }
+
+    private static boolean isWindowsAbsolutePath(String input) {
+        // if a prefix is present, we expected (long) UNC or (long) absolute
+        if (input.startsWith("\\\\?\\")) {
+            return true;
+        }
+
+        if (input.length() > 1) {
+            char c0 = input.charAt(0);
+            char c1 = input.charAt(1);
+            if (isSlash(c0) && isSlash(c1)) {
+                // Two slashes or more: UNC
+                return true;
+            }
+            if (isLetter(c0) && c1 == ':') {
+                // A drive: absolute
+                return true;
+            }
+        }
+        // Otherwise relative
+        return false;
+    }
+}

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/Platform.java

@@ -0,0 +1,35 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the "Elastic License
+ * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
+ * Public License v 1"; you may not use this file except in compliance with, at
+ * your election, the "Elastic License 2.0", the "GNU Affero General Public
+ * License v3.0 only", or the "Server Side Public License, v 1".
+ */
+
+package org.elasticsearch.entitlement.runtime.policy;
+
+public enum Platform {
+    LINUX,
+    MACOS,
+    WINDOWS;
+
+    private static final Platform current = findCurrent();
+
+    private static Platform findCurrent() {
+        String os = System.getProperty("os.name");
+        if (os.startsWith("Linux")) {
+            return LINUX;
+        } else if (os.startsWith("Mac OS")) {
+            return MACOS;
+        } else if (os.startsWith("Windows")) {
+            return WINDOWS;
+        } else {
+            throw new AssertionError("Unsupported platform [" + os + "]");
+        }
+    }
+
+    public boolean isCurrent() {
+        return this == current;
+    }
+}

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlement.java

@@ -10,7 +10,9 @@
 package org.elasticsearch.entitlement.runtime.policy.entitlements;
 
 import org.elasticsearch.entitlement.runtime.policy.ExternalEntitlement;
+import org.elasticsearch.entitlement.runtime.policy.FileUtils;
 import org.elasticsearch.entitlement.runtime.policy.PathLookup;
+import org.elasticsearch.entitlement.runtime.policy.Platform;
 import org.elasticsearch.entitlement.runtime.policy.PolicyValidationException;
 
 import java.nio.file.Path;
@@ -23,8 +25,6 @@ import java.util.Objects;
 import java.util.function.BiFunction;
 import java.util.stream.Stream;
 
-import static java.lang.Character.isLetter;
-
 /**
  * Describes a file entitlement with a path and mode.
  */
@@ -44,31 +44,6 @@ public record FilesEntitlement(List<FileData> filesData) implements Entitlement
         HOME
     }
 
-    public enum Platform {
-        LINUX,
-        MACOS,
-        WINDOWS;
-
-        private static final Platform current = findCurrent();
-
-        private static Platform findCurrent() {
-            String os = System.getProperty("os.name");
-            if (os.startsWith("Linux")) {
-                return LINUX;
-            } else if (os.startsWith("Mac OS")) {
-                return MACOS;
-            } else if (os.startsWith("Windows")) {
-                return WINDOWS;
-            } else {
-                throw new AssertionError("Unsupported platform [" + os + "]");
-            }
-        }
-
-        public boolean isCurrent() {
-            return this == current;
-        }
-    }
-
     public sealed interface FileData {
 
         Stream<Path> resolvePaths(PathLookup pathLookup);
@@ -94,50 +69,6 @@ public record FilesEntitlement(List<FileData> filesData) implements Entitlement
         static FileData ofPathSetting(String setting, BaseDir baseDir, Mode mode) {
             return new PathSettingFileData(setting, baseDir, mode, null, false);
         }
-
-        /**
-         * Tests if a path is absolute or relative, taking into consideration both Unix and Windows conventions.
-         * Note that this leads to a conflict, resolved in favor of Unix rules: `/foo` can be either a Unix absolute path, or a Windows
-         * relative path with "wrong" directory separator (using non-canonical slash in Windows).
-         */
-        static boolean isAbsolutePath(String path) {
-            if (path.isEmpty()) {
-                return false;
-            }
-            if (path.charAt(0) == '/') {
-                // Unix/BSD absolute
-                return true;
-            }
-            return isWindowsAbsolutePath(path);
-        }
-
-        private static boolean isSlash(char c) {
-            return (c == '\\') || (c == '/');
-        }
-
-        private static boolean isWindowsAbsolutePath(String input) {
-            // if a prefix is present, we expected (long) UNC or (long) absolute
-            if (input.startsWith("\\\\?\\")) {
-                return true;
-            }
-
-            if (input.length() > 1) {
-                char c0 = input.charAt(0);
-                char c1 = input.charAt(1);
-                char c = 0;
-                int next = 2;
-                if (isSlash(c0) && isSlash(c1)) {
-                    // Two slashes or more: UNC
-                    return true;
-                }
-                if (isLetter(c0) && c1 == ':') {
-                    // A drive: absolute
-                    return true;
-                }
-            }
-            // Otherwise relative
-            return false;
-        }
     }
 
     private sealed interface RelativeFileData extends FileData {
@@ -367,13 +298,13 @@ public record FilesEntitlement(List<FileData> filesData) implements Entitlement
                 }
                 BaseDir baseDir = parseBaseDir(relativeTo);
                 Path relativePath = Path.of(relativePathAsString);
-                if (FileData.isAbsolutePath(relativePathAsString)) {
+                if (FileUtils.isAbsolutePath(relativePathAsString)) {
                     throw new PolicyValidationException("'relative_path' [" + relativePathAsString + "] must be relative");
                 }
                 fileData = FileData.ofRelativePath(relativePath, baseDir, mode);
             } else if (pathAsString != null) {
                 Path path = Path.of(pathAsString);
-                if (FileData.isAbsolutePath(pathAsString) == false) {
+                if (FileUtils.isAbsolutePath(pathAsString) == false) {
                     throw new PolicyValidationException("'path' [" + pathAsString + "] must be absolute");
                 }
                 fileData = FileData.ofPath(path, mode);

libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTreeTests.java

@@ -26,6 +26,7 @@ import java.util.List;
 import java.util.Map;
 
 import static org.elasticsearch.core.PathUtils.getDefaultFileSystem;
+import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
 
 @ESTestCase.WithoutSecurityManager
@@ -193,6 +194,29 @@ public class FileAccessTreeTests extends ESTestCase {
         assertThat(tree.canRead(path("")), is(false));
     }
 
+    public void testNormalizeDirectorySeparatorWindows() {
+        assumeTrue("normalization of windows paths", Platform.WINDOWS.isCurrent());
+
+        assertThat(FileAccessTree.normalizePath(Path.of("C:\\a\\b")), equalTo("C:\\a\\b"));
+        assertThat(FileAccessTree.normalizePath(Path.of("C:/a.xml")), equalTo("C:\\a.xml"));
+        assertThat(FileAccessTree.normalizePath(Path.of("C:/a/b.txt")), equalTo("C:\\a\\b.txt"));
+        assertThat(FileAccessTree.normalizePath(Path.of("C:/a/c\\foo.txt")), equalTo("C:\\a\\c\\foo.txt"));
+
+        var tree = accessTree(
+            entitlement("C:\\a\\b", "read", "C:/a.xml", "read", "C:/a/b.txt", "read", "C:/a/c\\foo.txt", "read"),
+            List.of()
+        );
+
+        assertThat(tree.canRead(Path.of("C:/a.xml")), is(true));
+        assertThat(tree.canRead(Path.of("C:\\a.xml")), is(true));
+        assertThat(tree.canRead(Path.of("C:/a/")), is(false));
+        assertThat(tree.canRead(Path.of("C:/a/b.txt")), is(true));
+        assertThat(tree.canRead(Path.of("C:/a/b/c.txt")), is(true));
+        assertThat(tree.canRead(Path.of("C:\\a\\b\\c.txt")), is(true));
+        assertThat(tree.canRead(Path.of("C:\\a\\c\\")), is(false));
+        assertThat(tree.canRead(Path.of("C:\\a\\c\\foo.txt")), is(true));
+    }
+
     public void testNormalizeTrailingSlashes() {
         var tree = accessTree(entitlement("/trailing/slash/", "read", "/no/trailing/slash", "read"), List.of());
         assertThat(tree.canRead(path("/trailing/slash")), is(true));
@@ -230,6 +254,8 @@ public class FileAccessTreeTests extends ESTestCase {
 
     @SuppressForbidden(reason = "don't care about the directory location in tests")
     public void testFollowLinks() throws IOException {
+        assumeFalse("Windows requires admin right to create symbolic links", Platform.WINDOWS.isCurrent());
+
         Path baseSourceDir = Files.createTempDirectory("fileaccess_source");
         Path source1Dir = baseSourceDir.resolve("source1");
         Files.createDirectory(source1Dir);
@@ -321,6 +347,31 @@ public class FileAccessTreeTests extends ESTestCase {
         assertThat(tree.canWrite(path("a")), is(false));
     }
 
+    public void testWindowsAbsolutPathAccess() {
+        assumeTrue("Specific to windows for paths with a root (DOS or UNC)", Platform.WINDOWS.isCurrent());
+
+        var fileAccessTree = FileAccessTree.of(
+            "test",
+            "test",
+            new FilesEntitlement(
+                List.of(
+                    FilesEntitlement.FileData.ofPath(Path.of("\\\\.\\pipe\\"), FilesEntitlement.Mode.READ),
+                    FilesEntitlement.FileData.ofPath(Path.of("D:\\.gradle"), FilesEntitlement.Mode.READ),
+                    FilesEntitlement.FileData.ofPath(Path.of("D:\\foo"), FilesEntitlement.Mode.READ),
+                    FilesEntitlement.FileData.ofPath(Path.of("C:\\foo"), FilesEntitlement.Mode.READ_WRITE)
+                )
+            ),
+            TEST_PATH_LOOKUP,
+            List.of()
+        );
+
+        assertThat(fileAccessTree.canRead(Path.of("\\\\.\\pipe\\bar")), is(true));
+        assertThat(fileAccessTree.canRead(Path.of("C:\\foo")), is(true));
+        assertThat(fileAccessTree.canWrite(Path.of("C:\\foo")), is(true));
+        assertThat(fileAccessTree.canRead(Path.of("D:\\foo")), is(true));
+        assertThat(fileAccessTree.canWrite(Path.of("D:\\foo")), is(false));
+    }
+
     FileAccessTree accessTree(FilesEntitlement entitlement, List<ExclusivePath> exclusivePaths) {
         return FileAccessTree.of("test-component", "test-module", entitlement, TEST_PATH_LOOKUP, exclusivePaths);
     }

libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/FileUtilsTests.java

@@ -0,0 +1,92 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the "Elastic License
+ * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
+ * Public License v 1"; you may not use this file except in compliance with, at
+ * your election, the "Elastic License 2.0", the "GNU Affero General Public
+ * License v3.0 only", or the "Server Side Public License, v 1".
+ */
+
+package org.elasticsearch.entitlement.runtime.policy;
+
+import org.elasticsearch.core.PathUtils;
+import org.elasticsearch.test.ESTestCase;
+
+import static org.elasticsearch.entitlement.runtime.policy.FileUtils.PATH_ORDER;
+import static org.elasticsearch.entitlement.runtime.policy.FileUtils.isAbsolutePath;
+import static org.hamcrest.Matchers.greaterThan;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.lessThan;
+
+public class FileUtilsTests extends ESTestCase {
+
+    public void testPathIsAbsolute() {
+        var windowsNamedPipe = "\\\\.\\pipe";
+        var windowsDosAbsolutePath = "C:\\temp";
+        var unixAbsolutePath = "/tmp/foo";
+        var unixStyleUncPath = "//C/temp";
+        var uncPath = "\\\\C\\temp";
+        var longPath = "\\\\?\\C:\\temp";
+
+        var relativePath = "foo";
+        var headingSlashRelativePath = "\\foo";
+
+        assertThat(isAbsolutePath(windowsNamedPipe), is(true));
+        assertThat(isAbsolutePath(windowsDosAbsolutePath), is(true));
+        assertThat(isAbsolutePath(unixAbsolutePath), is(true));
+        assertThat(isAbsolutePath(unixStyleUncPath), is(true));
+        assertThat(isAbsolutePath(uncPath), is(true));
+        assertThat(isAbsolutePath(longPath), is(true));
+
+        assertThat(isAbsolutePath(relativePath), is(false));
+        assertThat(isAbsolutePath(headingSlashRelativePath), is(false));
+        assertThat(isAbsolutePath(""), is(false));
+    }
+
+    public void testPathOrderPosix() {
+        assumeFalse("path ordering rules specific to non-Windows path styles", Platform.WINDOWS.isCurrent());
+
+        // Unix-style
+        // Directories come BEFORE files; note that this differs from natural lexicographical order
+        assertThat(PATH_ORDER.compare("/a/b", "/a.xml"), lessThan(0));
+
+        // Natural lexicographical order is respected in all the other cases
+        assertThat(PATH_ORDER.compare("/a/b", "/a/b.txt"), lessThan(0));
+        assertThat(PATH_ORDER.compare("/a/c", "/a/b.txt"), greaterThan(0));
+        assertThat(PATH_ORDER.compare("/a/b", "/a/b/foo.txt"), lessThan(0));
+
+        // Inverted-windows style
+        // Directories come BEFORE files; note that this differs from natural lexicographical order
+        assertThat(PATH_ORDER.compare("C:/a/b", "C:/a.xml"), lessThan(0));
+
+        // Natural lexicographical order is respected in all the other cases
+        assertThat(PATH_ORDER.compare("C:/a/b", "C:/a/b.txt"), lessThan(0));
+        assertThat(PATH_ORDER.compare("C:/a/c", "C:/a/b.txt"), greaterThan(0));
+        assertThat(PATH_ORDER.compare("C:/a/b", "C:/a/b/foo.txt"), lessThan(0));
+
+        // "\" is a valid file name character on Posix, test we treat it like that
+        assertThat(PATH_ORDER.compare("/a\\b", "/a/b.txt"), greaterThan(0));
+    }
+
+    public void testPathOrderWindows() {
+        assumeTrue("path ordering rules specific to Windows", Platform.WINDOWS.isCurrent());
+
+        // Directories come BEFORE files; note that this differs from natural lexicographical order
+        assertThat(PATH_ORDER.compare("C:\\a\\b", "C:\\a.xml"), lessThan(0));
+
+        // Natural lexicographical order is respected in all the other cases
+        assertThat(PATH_ORDER.compare("C:\\a\\b", "C:\\a\\b.txt"), lessThan(0));
+        assertThat(PATH_ORDER.compare("C:\\a\\b", "C:\\a\\b\\foo.txt"), lessThan(0));
+        assertThat(PATH_ORDER.compare("C:\\a\\c", "C:\\a\\b.txt"), greaterThan(0));
+    }
+
+    public void testPathOrderingSpecialCharacters() {
+        assertThat(PATH_ORDER.compare("aa\uD801\uDC28", "aa\uD801\uDC28"), is(0));
+        assertThat(PATH_ORDER.compare("aa\uD801\uDC28", "aa\uD801\uDC28a"), lessThan(0));
+
+        var s = PathUtils.getDefaultFileSystem().getSeparator();
+        // Similarly to the other tests, we assert that Directories come BEFORE files, even when names are special characters
+        assertThat(PATH_ORDER.compare(s + "\uD801\uDC28" + s + "b", s + "\uD801\uDC28.xml"), lessThan(0));
+        assertThat(PATH_ORDER.compare(s + "\uD801\uDC28" + s + "b", s + "b.xml"), greaterThan(0));
+    }
+}

libs/entitlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FileDataTests.java

@@ -1,41 +0,0 @@
-/*
- * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
- * or more contributor license agreements. Licensed under the "Elastic License
- * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
- * Public License v 1"; you may not use this file except in compliance with, at
- * your election, the "Elastic License 2.0", the "GNU Affero General Public
- * License v3.0 only", or the "Server Side Public License, v 1".
- */
-
-package org.elasticsearch.entitlement.runtime.policy.entitlements;
-
-import org.elasticsearch.test.ESTestCase;
-
-import static org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement.FileData.isAbsolutePath;
-import static org.hamcrest.Matchers.is;
-
-public class FileDataTests extends ESTestCase {
-
-    public void testPathIsAbsolute() {
-        var windowsNamedPipe = "\\\\.\\pipe";
-        var windowsDosAbsolutePath = "C:\\temp";
-        var unixAbsolutePath = "/tmp/foo";
-        var unixStyleUncPath = "//C/temp";
-        var uncPath = "\\\\C\\temp";
-        var longPath = "\\\\?\\C:\\temp";
-
-        var relativePath = "foo";
-        var headingSlashRelativePath = "\\foo";
-
-        assertThat(isAbsolutePath(windowsNamedPipe), is(true));
-        assertThat(isAbsolutePath(windowsDosAbsolutePath), is(true));
-        assertThat(isAbsolutePath(unixAbsolutePath), is(true));
-        assertThat(isAbsolutePath(unixStyleUncPath), is(true));
-        assertThat(isAbsolutePath(uncPath), is(true));
-        assertThat(isAbsolutePath(longPath), is(true));
-
-        assertThat(isAbsolutePath(relativePath), is(false));
-        assertThat(isAbsolutePath(headingSlashRelativePath), is(false));
-        assertThat(isAbsolutePath(""), is(false));
-    }
-}