Change checkFileRead to throw IOException (#139320) (#139332)

* Change checkFileRead to throw IOException.

We used to transform every IOException aside from NoSuchFileException into a
NotEntitledException. However, denied entitlements never result in
IOExceptions, and the only method that uses this functionality is
Path.toRealPath which itself throws IOException, so there's no reason
to interpose and transmute the exception into NotEntitledException.

Just let 'er rip.

* Change assert false -> throw new AssertionError

Author: prdoyle

libs/entitlement/bridge/src/main/java/org/elasticsearch/entitlement/bridge/EntitlementChecker.java

@@ -15,6 +15,7 @@
 import java.io.FileDescriptor;
 import java.io.FileFilter;
 import java.io.FilenameFilter;
+import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.io.PrintStream;
@@ -66,7 +67,6 @@
 import java.nio.file.FileVisitOption;
 import java.nio.file.FileVisitor;
 import java.nio.file.LinkOption;
-import java.nio.file.NoSuchFileException;
 import java.nio.file.OpenOption;
 import java.nio.file.Path;
 import java.nio.file.WatchEvent;
@@ -100,6 +100,21 @@
 
 /**
  * Contains one "check" method for each distinct JDK method we want to instrument.
+ * <p>
+ * Methods starting with {@code check$} (with the dollar sign) follow a strict naming convention
+ * that allows them to be matched up directly against the corresponding target method or constructor
+ * by {@code InstrumentationService}.
+ * The naming convention uses dollar signs as separators,
+ * which works nicely because JCL methods don't use dollar signs.
+ * <p>
+ * Methods not starting with {@code check$} (for example, {@link #checkPathToRealPath})
+ * are processed by {@code DynamicInstrumentation} and follow a different convention.
+ * They are matched up with the appropriate implementation classes at runtime
+ * once we know what they are.
+ * <p>
+ * Some of these methods have a {@code throws} clause.
+ * The rule is that the check method should not throw a checked exception
+ * unless that exception is also thrown by the instrumented method under the same circumstances.
  */
 @SuppressWarnings("unused") // Called from instrumentation code inserted by the Entitlements agent
 public interface EntitlementChecker {
@@ -1243,7 +1258,14 @@ void checkNewByteChannel(
     void checkType(Class<?> callerClass, FileStore that);
 
     // path
-    void checkPathToRealPath(Class<?> callerClass, Path that, LinkOption... options) throws NoSuchFileException;
+
+    /**
+     * From {@link Path#toRealPath(LinkOption...)}...
+     *
+     * @throws  IOException
+     *          if the file does not exist or an I/O error occurs
+     */
+    void checkPathToRealPath(Class<?> callerClass, Path that, LinkOption... options) throws IOException;
 
     void checkPathRegister(Class<?> callerClass, Path that, WatchService watcher, WatchEvent.Kind<?>... events);
 

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

@@ -18,6 +18,7 @@
 import java.io.FileDescriptor;
 import java.io.FileFilter;
 import java.io.FilenameFilter;
+import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.io.PrintStream;
@@ -73,7 +74,6 @@
 import java.nio.file.FileVisitOption;
 import java.nio.file.FileVisitor;
 import java.nio.file.LinkOption;
-import java.nio.file.NoSuchFileException;
 import java.nio.file.OpenOption;
 import java.nio.file.Path;
 import java.nio.file.StandardOpenOption;
@@ -2737,7 +2737,7 @@ public void checkType(Class<?> callerClass, FileStore that) {
     }
 
     @Override
-    public void checkPathToRealPath(Class<?> callerClass, Path that, LinkOption... options) throws NoSuchFileException {
+    public void checkPathToRealPath(Class<?> callerClass, Path that, LinkOption... options) throws IOException {
         boolean followLinks = true;
         for (LinkOption option : options) {
             if (option == LinkOption.NOFOLLOW_LINKS) {

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

@@ -13,10 +13,10 @@
 import org.elasticsearch.entitlement.runtime.policy.entitlements.Entitlement;
 
 import java.io.File;
+import java.io.IOException;
 import java.net.JarURLConnection;
 import java.net.URL;
 import java.net.URLConnection;
-import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
 
 /**
@@ -52,7 +52,7 @@ public interface PolicyChecker {
 
     void checkFileRead(Class<?> callerClass, File file);
 
-    void checkFileRead(Class<?> callerClass, Path path, boolean followLinks) throws NoSuchFileException;
+    void checkFileRead(Class<?> callerClass, Path path, boolean followLinks) throws IOException;
 
     void checkFileRead(Class<?> callerClass, Path path);
 

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

@@ -34,7 +34,6 @@
 import java.net.URISyntaxException;
 import java.net.URL;
 import java.net.URLConnection;
-import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.List;
@@ -206,16 +205,13 @@ public void checkFileRead(Class<?> callerClass, File file) {
     public void checkFileRead(Class<?> callerClass, Path path) {
         try {
             checkFileRead(callerClass, path, false);
-        } catch (NoSuchFileException e) {
-            assert false : "NoSuchFileException should only be thrown when following links";
-            var notEntitledException = new NotEntitledException(e.getMessage());
-            notEntitledException.addSuppressed(e);
-            throw notEntitledException;
+        } catch (IOException e) {
+            throw new AssertionError("IOException should be impossible unless following links", e);
         }
     }
 
     @Override
-    public void checkFileRead(Class<?> callerClass, Path path, boolean followLinks) throws NoSuchFileException {
+    public void checkFileRead(Class<?> callerClass, Path path, boolean followLinks) throws IOException {
         if (isPathOnDefaultFilesystem(path) == false) {
             return;
         }
@@ -229,15 +225,9 @@ public void checkFileRead(Class<?> callerClass, Path path, boolean followLinks)
         Path realPath = null;
         boolean canRead = entitlements.fileAccess().canRead(path);
         if (canRead && followLinks) {
-            try {
-                realPath = path.toRealPath();
-                if (realPath.equals(path) == false) {
-                    canRead = entitlements.fileAccess().canRead(realPath);
-                }
-            } catch (NoSuchFileException e) {
-                throw e; // rethrow
-            } catch (IOException e) {
-                canRead = false;
+            realPath = path.toRealPath();
+            if (realPath.equals(path) == false) {
+                canRead = entitlements.fileAccess().canRead(realPath);
             }
         }
 

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

@@ -9,15 +9,24 @@
 
 package org.elasticsearch.entitlement.runtime.policy;
 
+import org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement;
+import org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement.FileData;
 import org.elasticsearch.test.ESTestCase;
 
 import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.NoSuchFileException;
+import java.nio.file.Path;
+import java.util.List;
+import java.util.Map;
 import java.util.Set;
 import java.util.stream.Stream;
 
 import static org.elasticsearch.entitlement.runtime.policy.PolicyManagerTests.NO_ENTITLEMENTS_MODULE;
 import static org.elasticsearch.entitlement.runtime.policy.PolicyManagerTests.TEST_PATH_LOOKUP;
+import static org.elasticsearch.entitlement.runtime.policy.PolicyManagerTests.createEmptyTestServerPolicy;
 import static org.elasticsearch.entitlement.runtime.policy.PolicyManagerTests.makeClassInItsOwnModule;
+import static org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement.Mode.READ;
 
 public class PolicyCheckerImplTests extends ESTestCase {
     public void testRequestingClassFastPath() throws IOException, ClassNotFoundException {
@@ -54,8 +63,72 @@ public void testRequestingModuleWithStackWalk() throws IOException, ClassNotFoun
         );
     }
 
+    /**
+     * Set up a situation where the file read entitlement check encounters
+     * a strange {@link IOException} that is not {@link NoSuchFileException}.
+     * Ensure that the IOException makes it out to the caller, rather than
+     * being transformed into a NotEntitledException.
+     */
+    public void testIOExceptionFollowingSymlink() throws IOException {
+        Path dir = createTempDir();
+        Path symlink = dir.resolve("symlink");
+        Path allegedDir = dir.resolve("not_a_dir");
+        Path target = allegedDir.resolve("target");
+        Files.createFile(allegedDir); // Not a dir!
+        Files.createSymbolicLink(symlink, target);
+
+        PathLookup testPathLookup = new PathLookup() {
+            @Override
+            public Path pidFile() {
+                throw new UnsupportedOperationException();
+            }
+
+            @Override
+            public Stream<Path> getBaseDirPaths(BaseDir baseDir) {
+                return Stream.empty();
+            }
+
+            @Override
+            public Stream<Path> resolveSettingPaths(BaseDir baseDir, String settingName) {
+                throw new UnsupportedOperationException();
+            }
+
+            @Override
+            public boolean isPathOnDefaultFilesystem(Path path) {
+                // We need this to be on the default filesystem or it will be trivially allowed
+                return true;
+            }
+        };
+
+        var policyManager = new TestPolicyManager(
+            createEmptyTestServerPolicy(),
+            List.of(),
+            Map.of(
+                "testComponent",
+                new Policy(
+                    "testPolicy",
+                    List.of(new Scope("testModule", List.of(new FilesEntitlement(List.of(FileData.ofPath(symlink, READ))))))
+                )
+            ),
+            c -> PolicyManager.PolicyScope.plugin("testComponent", "testModule"),
+            testPathLookup,
+            List.of(),
+            List.of()
+        );
+        policyManager.setActive(true);
+        policyManager.setTriviallyAllowingTestCode(false);
+
+        var checker = checker(NO_ENTITLEMENTS_MODULE, policyManager, testPathLookup);
+        var exception = assertThrows(IOException.class, () -> checker.checkFileRead(getClass(), symlink, true));
+    }
+
     private static PolicyCheckerImpl checker(Module entitlementsModule) {
-        return new PolicyCheckerImpl(Set.of(), entitlementsModule, null, TEST_PATH_LOOKUP);
+        // TODO: TEST_PATH_LOOKUP is always null at this point!
+        return checker(entitlementsModule, null, TEST_PATH_LOOKUP);
+    }
+
+    private static PolicyCheckerImpl checker(Module entitlementsModule, PolicyManager policyManager, PathLookup testPathLookup) {
+        return new PolicyCheckerImpl(Set.of(), entitlementsModule, policyManager, testPathLookup);
     }
 
 }