[Entitlements] Fix FileAccessTree paths ordering (#123689)

Author: ldematte

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

@@ -64,11 +64,11 @@
 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.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 {
 
@@ -236,30 +236,4 @@ public boolean equals(Object o) {
     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.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 enum BaseDir {
         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 @@ static FileData ofRelativePath(Path relativePath, BaseDir baseDir, Mode mode) {
         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 static FilesEntitlement build(List<Object> paths) {
                 }
                 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.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 void testNormalizePath() {
         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 void testJdkAccess() {
 
     @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);
@@ -315,6 +341,31 @@ public void testInvalidExclusiveAccess() {
         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);
     }