response to pr comments

jdconrad · Feb 25, 2025 · d9de8c7 · d9de8c7
1 parent 39b9ff7
commit d9de8c7
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 30 deletions.
diff --git a/...ntitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTree.java b/...ntitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTree.java
@@ -30,9 +30,21 @@
 
 public final class FileAccessTree {
 
+    /**
+     * An intermediary structure to help build exclusive paths for files entitlements.
+     */
     record ExclusiveFileEntitlement(String componentName, String moduleName, FilesEntitlement filesEntitlement) {}
 
-    record ExclusivePath(String componentName, String moduleName, String path) {}
+    /**
+     * An intermediary structure to help globally validate exclusive paths, and then build exclusive paths for individual modules.
+     */
+    record ExclusivePath(String componentName, String moduleName, String path) {
+
+        @Override
+        public String toString() {
+            return "[[" + componentName + "] [" + moduleName + "] [" + path + "]]";
+        }
+    }
 
     static List<ExclusivePath> buildExclusivePathList(List<ExclusiveFileEntitlement> exclusiveFileEntitlements, PathLookup pathLookup) {
         List<ExclusivePath> exclusivePaths = new ArrayList<>();
@@ -57,21 +69,7 @@ static void validateExclusivePaths(List<ExclusivePath> exclusivePaths) {
                 ExclusivePath nextPath = exclusivePaths.get(i);
                 if (currentExclusivePath.path().equals(nextPath.path) || isParent(currentExclusivePath.path(), nextPath.path())) {
                     throw new IllegalArgumentException(
-                        "duplicate/overlapping exclusive paths found in files entitlements: "
-                            + "[["
-                            + currentExclusivePath.componentName()
-                            + "] ["
-                            + currentExclusivePath.moduleName()
-                            + "] ["
-                            + currentExclusivePath.path()
-                            + "]] and [["
-
-                            + nextPath.componentName()
-                            + "] ["
-                            + nextPath.moduleName()
-                            + "] ["
-                            + nextPath.path()
-                            + "]]"
+                        "duplicate/overlapping exclusive paths found in files entitlements: " + currentExclusivePath + " and " + nextPath
                     );
                 }
                 currentExclusivePath = nextPath;
@@ -104,13 +102,6 @@ private FileAccessTree(
         List<String> writePaths = new ArrayList<>();
         BiConsumer<Path, Mode> addPath = (path, mode) -> {
             var normalized = normalizePath(path);
-            for (String exclusivePath : updatedExclusivePaths) {
-                if (exclusivePath.equals(normalized) || isParent(exclusivePath, normalized)) {
-                    throw new IllegalArgumentException(
-                        "[" + componentName + "] [" + moduleName + "] cannot use exclusive path [" + exclusivePath + "]"
-                    );
-                }
-            }
             if (mode == Mode.READ_WRITE) {
                 writePaths.add(normalized);
             }
@@ -211,9 +202,15 @@ static String normalizePath(Path path) {
     }
 
     private boolean checkPath(String path, String[] paths) {
-        if (paths.length == 0 || Arrays.binarySearch(exclusivePaths, path) >= 0) {
+        if (paths.length == 0) {
             return false;
         }
+
+        int endx = Arrays.binarySearch(exclusivePaths, path, PATH_ORDER);
+        if (endx < -1 && isParent(exclusivePaths[-endx - 2], path) || endx >= 0) {
+            return false;
+        }
+
         int ndx = Arrays.binarySearch(paths, path, PATH_ORDER);
         if (ndx < -1) {
             return isParent(paths[-ndx - 2], path);

diff --git a/...entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java b/...entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java
@@ -172,7 +172,13 @@ public PolicyManager(
         this.apmAgentPackageName = apmAgentPackageName;
         this.entitlementsModule = entitlementsModule;
         this.pathLookup = requireNonNull(pathLookup);
-        this.defaultFileAccess = FileAccessTree.of("", "", FilesEntitlement.EMPTY, pathLookup, List.of());
+        this.defaultFileAccess = FileAccessTree.of(
+            UNKNOWN_COMPONENT_NAME,
+            UNKNOWN_COMPONENT_NAME,
+            FilesEntitlement.EMPTY,
+            pathLookup,
+            List.of()
+        );
         this.mutedClasses = suppressFailureLogClasses;
 
         List<ExclusiveFileEntitlement> exclusiveFileEntitlements = new ArrayList<>();

diff --git a/...ement/src/test/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTreeTests.java b/...ement/src/test/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTreeTests.java
@@ -299,11 +299,21 @@ public void testBasicExclusiveAccess() {
     }
 
     public void testInvalidExclusiveAccess() {
-        var iae = assertThrows(
-            IllegalArgumentException.class,
-            () -> accessTree(entitlement("foo/bar", "read"), exclusivePaths("test-component", "diff-module", "foo"))
-        );
-        assertThat(iae.getMessage(), is("[test-component] [test-module] cannot use exclusive path [" + path("foo") + "]"));
+        var tree = accessTree(entitlement("a", "read"), exclusivePaths("diff-component", "diff-module", "a/b"));
+        assertThat(tree.canRead(path("a")), is(true));
+        assertThat(tree.canWrite(path("a")), is(false));
+        assertThat(tree.canRead(path("a/b")), is(false));
+        assertThat(tree.canWrite(path("a/b")), is(false));
+        assertThat(tree.canRead(path("a/b/c")), is(false));
+        assertThat(tree.canWrite(path("a/b/c")), is(false));
+        tree = accessTree(entitlement("a/b", "read"), exclusivePaths("diff-component", "diff-module", "a"));
+        assertThat(tree.canRead(path("a")), is(false));
+        assertThat(tree.canWrite(path("a")), is(false));
+        assertThat(tree.canRead(path("a/b")), is(false));
+        assertThat(tree.canWrite(path("a/b")), is(false));
+        tree = accessTree(entitlement("a", "read"), exclusivePaths("diff-component", "diff-module", "a"));
+        assertThat(tree.canRead(path("a")), is(false));
+        assertThat(tree.canWrite(path("a")), is(false));
     }
 
     FileAccessTree accessTree(FilesEntitlement entitlement, List<ExclusivePath> exclusivePaths) {