Reduce duplicate and dead entitlements code (#121409) (#121426)

* Refactor: remove duplicate canWrite methods. This serves as a good example of how Path and File handling could be specialized in the future, but as long as they are identical, the duplication causes more harm than good. * Refactor: just one neverEntitled. The original motivation was to avoid allocating a lambda object on each call, but since that's a highly optimized operation in the JVM, it's unlikely to make a difference in practice, and this smacks of premature optimization. We're pretty liberal about lambdas elsewhere, so let's not sweat it here until we have some evidence that it matters. * Remove dead code
elastic · Jan 31, 2025 · 53e8731 · 53e8731
1 parent 3b679b6
commit 53e8731
Show file tree

Hide file tree

Showing 3 changed files with 3 additions and 76 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
@@ -9,10 +9,8 @@
 
 package org.elasticsearch.entitlement.runtime.policy;
 
-import org.elasticsearch.core.SuppressForbidden;
 import org.elasticsearch.entitlement.runtime.policy.entitlements.FileEntitlement;
 
-import java.io.File;
 import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -51,20 +49,10 @@ boolean canRead(Path path) {
         return checkPath(normalize(path), readPaths);
     }
 
-    @SuppressForbidden(reason = "Explicitly checking File apis")
-    boolean canRead(File file) {
-        return checkPath(normalize(file.toPath()), readPaths);
-    }
-
     boolean canWrite(Path path) {
         return checkPath(normalize(path), writePaths);
     }
 
-    @SuppressForbidden(reason = "Explicitly checking File apis")
-    boolean canWrite(File file) {
-        return checkPath(normalize(file.toPath()), writePaths);
-    }
-
     private static String normalize(Path path) {
         return path.toAbsolutePath().normalize().toString();
     }

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
@@ -169,23 +169,7 @@ private static void validateEntitlementsPerModule(String sourceName, String modu
     }
 
     public void checkStartProcess(Class<?> callerClass) {
-        neverEntitled(callerClass, "start process");
-    }
-
-    private void neverEntitled(Class<?> callerClass, String operationDescription) {
-        var requestingClass = requestingClass(callerClass);
-        if (isTriviallyAllowed(requestingClass)) {
-            return;
-        }
-
-        throw new NotEntitledException(
-            Strings.format(
-                "Not entitled: caller [%s], module [%s], operation [%s]",
-                callerClass,
-                requestingClass.getModule() == null ? "<none>" : requestingClass.getModule().getName(),
-                operationDescription
-            )
-        );
+        neverEntitled(callerClass, () -> "start process");
     }
 
     /**
@@ -241,31 +225,9 @@ public void checkChangeNetworkHandling(Class<?> callerClass) {
         checkChangeJVMGlobalState(callerClass);
     }
 
-    /**
-     * Check for operations that can access sensitive network information, e.g. secrets, tokens or SSL sessions
-     */
-    public void checkReadSensitiveNetworkInformation(Class<?> callerClass) {
-        neverEntitled(callerClass, "access sensitive network information");
-    }
-
     @SuppressForbidden(reason = "Explicitly checking File apis")
     public void checkFileRead(Class<?> callerClass, File file) {
-        var requestingClass = requestingClass(callerClass);
-        if (isTriviallyAllowed(requestingClass)) {
-            return;
-        }
-
-        ModuleEntitlements entitlements = getEntitlements(requestingClass);
-        if (entitlements.fileAccess().canRead(file) == false) {
-            throw new NotEntitledException(
-                Strings.format(
-                    "Not entitled: caller [%s], module [%s], entitlement [file], operation [read], path [%s]",
-                    callerClass,
-                    requestingClass.getModule(),
-                    file
-                )
-            );
-        }
+        checkFileRead(callerClass, file.toPath());
     }
 
     public void checkFileRead(Class<?> callerClass, Path path) {
@@ -289,22 +251,7 @@ public void checkFileRead(Class<?> callerClass, Path path) {
 
     @SuppressForbidden(reason = "Explicitly checking File apis")
     public void checkFileWrite(Class<?> callerClass, File file) {
-        var requestingClass = requestingClass(callerClass);
-        if (isTriviallyAllowed(requestingClass)) {
-            return;
-        }
-
-        ModuleEntitlements entitlements = getEntitlements(requestingClass);
-        if (entitlements.fileAccess().canWrite(file) == false) {
-            throw new NotEntitledException(
-                Strings.format(
-                    "Not entitled: caller [%s], module [%s], entitlement [file], operation [write], path [%s]",
-                    callerClass,
-                    requestingClass.getModule(),
-                    file
-                )
-            );
-        }
+        checkFileWrite(callerClass, file.toPath());
     }
 
     public void checkFileWrite(Class<?> callerClass, Path path) {

diff --git a/...lement/src/test/java/org/elasticsearch/entitlement/runtime/policy/PolicyManagerTests.java b/...lement/src/test/java/org/elasticsearch/entitlement/runtime/policy/PolicyManagerTests.java
@@ -238,7 +238,6 @@ public void testRequestingClassFastPath() throws IOException, ClassNotFoundExcep
     }
 
     public void testRequestingModuleWithStackWalk() throws IOException, ClassNotFoundException {
-        var agentsClass = new TestAgent();
         var entitlementsClass = makeClassInItsOwnModule();    // A class in the entitlements library itself
         var requestingClass = makeClassInItsOwnModule();      // This guy is always the right answer
         var instrumentedClass = makeClassInItsOwnModule();    // The class that called the check method
@@ -365,13 +364,6 @@ private static Class<?> makeClassInItsOwnModule() throws IOException, ClassNotFo
         return layer.findLoader("org.example.plugin").loadClass("q.B");
     }
 
-    private static Class<?> makeClassInItsOwnUnnamedModule() throws IOException, ClassNotFoundException {
-        final Path home = createTempDir();
-        Path jar = createMockPluginJar(home);
-        var layer = createLayerForJar(jar, "org.example.plugin");
-        return layer.findLoader("org.example.plugin").loadClass("q.B");
-    }
-
     private static PolicyManager policyManager(String agentsPackageName, Module entitlementsModule) {
         return new PolicyManager(createEmptyTestServerPolicy(), List.of(), Map.of(), c -> "test", agentsPackageName, entitlementsModule);
     }