dcache,chimera: resolve symlink before applying Restriction

Motivation: When PnfsManager determines permissions based on Restrictions, it should compare the prefixes or scope claims to the actual path after all symlinks have been resolved. Modification: Define and implement the necessary internal APIs to support a single-call path resolution; for Postgres, implement the driver method as referencing a stored procedure. Make the necessary changes to PnfsManager to resolve symlinks before using the Restriction's `isRestricted()`. Result: Application of restrictions is correct in that the ownership and scope of the resolved link is examined. Target: master Request: 8.2 (should this be backported further?) Requires-notes: yes Patch: https://rb.dcache.org/r/13908/ Acked-by: Dmitry Acked-by: Tigran
dCache · Mar 15, 2023 · eb763b5 · eb763b5
1 parent c51c5c8
commit eb763b5
Show file tree

Hide file tree

Showing 11 changed files with 176 additions and 18 deletions.
diff --git a/modules/chimera/src/main/java/org/dcache/chimera/FileSystemProvider.java b/modules/chimera/src/main/java/org/dcache/chimera/FileSystemProvider.java
@@ -570,4 +570,11 @@ void setXattr(FsInode inode, String attr, byte[] value, SetXattrMode mode)
      */
     void removeXattr(FsInode inode, String attr) throws ChimeraFsException;
 
+    /**
+     * Returns the path with symlinks resolved.
+     *
+     * @param path to resolve.
+     * @return resolved path.
+     */
+    String resolvePath(String path) throws ChimeraFsException;
 }
diff --git a/modules/chimera/src/main/java/org/dcache/chimera/FsSqlDriver.java b/modules/chimera/src/main/java/org/dcache/chimera/FsSqlDriver.java
@@ -1785,6 +1785,25 @@ public boolean isForeignKeyError(SQLException e) {
         return e.getSQLState().equals("23503");
     }
 
+    /**
+     * Should return the actual absolute path with all symlinks followed.
+     *
+     * This base implementation composes inode2path(path2inode).
+     *
+     * @param root node from which to begin path three walk.
+     * @param path to check for symlinks.
+     * @return path with all symlinks resolved; if does not exist, return the original path.
+     * @throws ChimeraFsException
+     * @throws SQLException
+     */
+    String resolvePath(FsInode root, String path) throws ChimeraFsException, SQLException {
+        FsInode pathInode = path2inode(root, path);
+        if (pathInode == null) {
+            return path;
+        }
+        return inode2path(pathInode, root);
+    }
+
     /**
      * creates an instance of org.dcache.chimera.&lt;dialect&gt;FsSqlDriver or default driver, if
      * specific driver not available

diff --git a/modules/chimera/src/main/java/org/dcache/chimera/JdbcFs.java b/modules/chimera/src/main/java/org/dcache/chimera/JdbcFs.java
@@ -1572,6 +1572,15 @@ public List<PinInfo> listPins(FsInode pnfsid) throws ChimeraFsException {
         throw new ChimeraFsException(NOT_IMPL);
     }
 
+    @Override
+    public String resolvePath(String path) throws ChimeraFsException {
+        try {
+            return _sqlDriver.resolvePath(new RootInode(this, _sqlDriver.getRootInumber()), path);
+        } catch (SQLException e) {
+            throw new ChimeraFsException(e.getMessage(), e);
+        }
+    }
+
     private interface FallibleTransactionCallback<T> {
 
         T doInTransaction(TransactionStatus status) throws ChimeraFsException;

diff --git a/modules/chimera/src/main/java/org/dcache/chimera/PgSQL95FsSqlDriver.java b/modules/chimera/src/main/java/org/dcache/chimera/PgSQL95FsSqlDriver.java
@@ -274,6 +274,22 @@ List<FsInode> path2inodes(FsInode root, String path) throws ChimeraFsException {
               });
     }
 
+    @Override
+    String resolvePath(FsInode root, String path) {
+        String normalizedPath = normalizePath(path);
+        return _jdbc.query("SELECT * FROM resolve_path(?, ?)",
+              ps -> {
+                  ps.setLong(1, root.ino());
+                  ps.setString(2, normalizedPath);
+              },
+              rs -> {
+                  if (rs.next()) {
+                      return rs.getString(1);
+                  }
+                  return null;
+              });
+    }
+
     @Override
     void copyAcl(FsInode source, FsInode inode, RsType type, EnumSet<AceFlags> mask,
           EnumSet<AceFlags> flags) {

diff --git a/modules/chimera/src/main/resources/org/dcache/chimera/changelog/changelog-master.xml b/modules/chimera/src/main/resources/org/dcache/chimera/changelog/changelog-master.xml
@@ -24,5 +24,5 @@
     <include file="org/dcache/chimera/changelog/changeset-6.2.xml"/>
     <include file="org/dcache/chimera/changelog/changeset-7.1.xml"/>
     <include file="org/dcache/chimera/changelog/changeset-7.2.xml"/>
-
+    <include file="org/dcache/chimera/changelog/changeset-8.2.xml"/>
 </databaseChangeLog>
diff --git a/modules/chimera/src/main/resources/org/dcache/chimera/changelog/changeset-8.2.xml b/modules/chimera/src/main/resources/org/dcache/chimera/changelog/changeset-8.2.xml
@@ -0,0 +1,27 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<databaseChangeLog
+        xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+        xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog
+     http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.1.xsd">
+
+    <!-- For safety in backporting, we introduce a separate procedure rather
+         than trying to integrate/modify an existing one. -->
+    <changeSet id="34" author="arossi" dbms="postgresql">
+        <comment>Symlink resolution calling inumber2path(path2inumber)</comment>
+        <createProcedure>
+            CREATE OR REPLACE FUNCTION resolve_path(root bigint, path varchar) RETURNS varchar
+                AS
+            $$
+                BEGIN
+                IF path LIKE '/%' THEN
+                    path := substring(path from 2);
+                END IF;
+                return inumber2path(path2inumber(root, path));
+            END;
+            $$ LANGUAGE plpgsql;
+        </createProcedure>
+        <rollback>
+        </rollback>
+    </changeSet>
+</databaseChangeLog>
diff --git a/...s/dcache-chimera/src/main/java/org/dcache/chimera/namespace/ChimeraNameSpaceProvider.java b/...s/dcache-chimera/src/main/java/org/dcache/chimera/namespace/ChimeraNameSpaceProvider.java
@@ -1924,4 +1924,20 @@ public void removeLabel(Subject subject, FsPath path, String label) throws Cache
         }
 
     }
+
+    @Override
+    public String resolveSymlinks(Subject subject, String path) throws CacheException {
+        try {
+            /*
+             *  All calls to this method are done by the PnfsManager itself as ROOT.
+             */
+            if (!Subjects.ROOT.equals(subject)) {
+                throw new PermissionDeniedCacheException("Subject not allowed to call this method.");
+            }
+            return _fs.resolvePath(path);
+        } catch (ChimeraFsException e) {
+            throw new CacheException("Failed to resolve symlinks for " + path
+                  + ": " + Exceptions.messageOrClassName(e), e);
+        }
+    }
 }
diff --git a/modules/dcache/src/main/java/diskCacheV111/namespace/ForwardingNameSpaceProvider.java b/modules/dcache/src/main/java/diskCacheV111/namespace/ForwardingNameSpaceProvider.java
@@ -219,4 +219,9 @@ public void removeLabel(Subject subject, FsPath path, String name) throws CacheE
         delegate().removeLabel(subject, path, name);
 
     }
+
+    @Override
+    public String resolveSymlinks(Subject subject, String path) throws CacheException {
+        return delegate().resolveSymlinks(subject, path);
+    }
 }
diff --git a/modules/dcache/src/main/java/diskCacheV111/namespace/NameSpaceProvider.java b/modules/dcache/src/main/java/diskCacheV111/namespace/NameSpaceProvider.java
@@ -443,5 +443,13 @@ void removeExtendedAttribute(Subject subject, FsPath path, String name)
     void listVirtualDirectory(Subject subject, String path, Range<Integer> range,
                          Set<FileAttribute> attrs, ListHandler handler)  throws  CacheException;
 
-
+    /**
+     * Resolves symlinks in the given path.
+     *
+     * @param subject user making the request.
+     * @param path full path to be resolved.
+     * @return path with symlinks resolved.
+     * @throws CacheException
+     */
+    String resolveSymlinks(Subject subject, String path) throws CacheException;
 }
diff --git a/modules/dcache/src/main/java/diskCacheV111/namespace/PnfsManagerV3.java b/modules/dcache/src/main/java/diskCacheV111/namespace/PnfsManagerV3.java
@@ -29,6 +29,7 @@
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Splitter;
 import com.google.common.base.Strings;
+import com.google.common.base.Throwables;
 import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
@@ -208,7 +209,6 @@ public class PnfsManagerV3
 
     private boolean useParentHashOnCreate;
 
-
     /**
      * Whether to use folding.
      */
@@ -1800,7 +1800,7 @@ private void checkMkdirAllowed(PnfsCreateEntryMessage message)
          * the directory '/data/test-1' should succeed.  However, an attempt to
          * mkdir '/data/test-1/item1' should fail.
          */
-        if (restriction.hasUnrestrictedChild(UPLOAD, path)) {
+        if (restriction.hasUnrestrictedChild(UPLOAD, resolveSymlinks(path.toString()))) {
             return;
         }
 
@@ -1823,7 +1823,8 @@ private void checkMkdirAllowed(PnfsCreateEntryMessage message)
          * In example 2, the parent is 'test-1' and the user is not allowed to
          * upload this file.
          */
-        if (!path.isRoot() && !restriction.isRestricted(UPLOAD, path.parent())) {
+        if (!path.isRoot() && !restriction.isRestricted(UPLOAD,
+              resolveSymlinks(path.parent().toString()))) {
             return;
         }
 
@@ -2069,7 +2070,7 @@ public void rename(PnfsRenameMessage msg) {
             checkRestriction(msg, MANAGE, FsPath.create(sourcePath).parent());
             checkRestriction(msg, MANAGE, FsPath.create(destinationPath).parent());
             boolean overwrite = msg.getOverwrite()
-                  && !msg.getRestriction().isRestricted(DELETE, FsPath.create(destinationPath));
+                  && !msg.getRestriction().isRestricted(DELETE, resolveSymlinks(destinationPath));
             _nameSpaceProvider.rename(msg.getSubject(), pnfsId, sourcePath, destinationPath,
                   overwrite);
         } catch (CacheException e) {
@@ -2181,7 +2182,8 @@ private void sendPartialReply() {
         @Override
         public void addEntry(String name, FileAttributes attrs) {
             if (Subjects.isRoot(_subject)
-                  || !_restriction.isRestricted(READ_METADATA, _directory, name)) {
+                  || !_restriction.isRestricted(READ_METADATA,
+                  resolveSymlinks(_directory.toString()), name)) {
                 long now = System.currentTimeMillis();
                 _msg.addEntry(name, attrs);
                 if (_msg.getEntries().size() >= _directoryListLimit ||
@@ -2629,7 +2631,7 @@ void processPnfsMessage(CellMessage message, PnfsMessage pnfsMessage) {
     }
 
     @Transactional
-    private boolean processMessageTransactionally(CellMessage message, PnfsMessage pnfsMessage) {
+    boolean processMessageTransactionally(CellMessage message, PnfsMessage pnfsMessage) {
         if (pnfsMessage instanceof PnfsAddCacheLocationMessage) {
             addCacheLocation((PnfsAddCacheLocationMessage) pnfsMessage);
         } else if (pnfsMessage instanceof PnfsClearCacheLocationMessage) {
@@ -3247,7 +3249,7 @@ private static Activity toActivity(AccessMask mask) {
         throw new RuntimeException("Unexpected AccessMask: " + mask);
     }
 
-    private static void checkRestrictionOnParent(PnfsMessage message, Activity activity)
+    private void checkRestrictionOnParent(PnfsMessage message, Activity activity)
           throws PermissionDeniedCacheException {
         if (!Subjects.isRoot(message.getSubject())) {
             FsPath path = message.getFsPath();
@@ -3258,7 +3260,7 @@ private static void checkRestrictionOnParent(PnfsMessage message, Activity activ
         }
     }
 
-    private static void checkRestriction(PnfsMessage message, Activity activity)
+    private void checkRestriction(PnfsMessage message, Activity activity)
           throws PermissionDeniedCacheException {
         if (!Subjects.isRoot(message.getSubject())) {
             FsPath path = message.getFsPath();
@@ -3272,32 +3274,78 @@ private static void checkRestriction(PnfsMessage message, Activity activity)
         }
     }
 
-    private static void checkRestriction(PnfsMessage message, Activity activity,
+    private void checkRestriction(PnfsMessage message, Activity activity,
           FsPath path) throws PermissionDeniedCacheException {
         if (!Subjects.isRoot(message.getSubject())) {
             checkRestriction(message.getRestriction(), message.getAccessMask(),
                   activity, path);
         }
     }
 
-    private static void checkRestriction(Restriction restriction, Set<AccessMask> mask,
+    private void checkRestriction(Restriction restriction, Set<AccessMask> mask,
           Activity activity, FsPath path) throws PermissionDeniedCacheException {
+        FsPath resolvedPath;
+        try {
+            if (activity == UPLOAD) {
+                /*
+                 *  The resolution will not work on the full path because it does not
+                 *  yet exist, so we resolve the parent and then compose the child path.
+                 */
+                FsPath resolvedParent = resolveSymlinks(path.parent().toString());
+                resolvedPath = resolvedParent.child(path.name());
+            } else {
+                Optional<FsPath> optional = resolveSymlinks(path.toString(), true);
+                if (optional.isEmpty()) {
+                    LOGGER.debug("skipping checkRestriction on unresolved path {}.", path);
+                    return;
+                }
+                resolvedPath = optional.get();
+            }
+        } catch (CacheException e) {
+            /*
+             *  Do not risk allowing the operation to proceed in this case because
+             *  the underlying error may not necessarily prevent the desired activity.
+             */
+            throw new PermissionDeniedCacheException(
+                  "Restriction " + restriction + " denied activity " + activity + " on " + path, e);
+        }
+
         if (mask.stream()
               .map(PnfsManagerV3::toActivity)
-              .anyMatch(a -> restriction.isRestricted(a, path))) {
+              .anyMatch(a -> restriction.isRestricted(a, resolvedPath))) {
 
             Set<AccessMask> denied = mask.stream()
-                  .filter(m -> restriction.isRestricted(toActivity(m), path))
+                  .filter(m -> restriction.isRestricted(toActivity(m), resolvedPath))
                   .collect(Collectors.toSet());
 
             throw new PermissionDeniedCacheException(
-                  "Restriction " + restriction + " denied access for " + denied + " on " + path);
+                  "Restriction " + restriction + " denied access for " + denied + " on "
+                        + resolvedPath);
         }
 
-        if (restriction.isRestricted(activity, path)) {
+        if (restriction.isRestricted(activity, resolvedPath)) {
             throw new PermissionDeniedCacheException(
-                  "Restriction " + restriction + " denied activity " + activity + " on " + path);
+                  "Restriction " + restriction + " denied activity " + activity + " on "
+                        + resolvedPath);
+        }
+    }
+
+    private FsPath resolveSymlinks(String target) {
+        try {
+            return resolveSymlinks(target, false).get();
+        } catch (CacheException e) {
+            LOGGER.error("resolveSymlinks failed: {}; returning original target.",
+                  Throwables.getRootCause(e).toString());
+            return FsPath.create(target);
+        }
+    }
+
+    private Optional<FsPath> resolveSymlinks(String target, boolean allowNullReturn) throws CacheException {
+        String resolved = _nameSpaceProvider.resolveSymlinks(Subjects.ROOT, target);
+        if (Strings.emptyToNull(resolved) == null) {
+            return allowNullReturn ? Optional.empty() : Optional.of(FsPath.create(target));
         }
+        return Optional.of(FsPath.create(resolved));
     }
 
     /**

diff --git a/modules/dcache/src/main/java/org/dcache/auth/RemoteNameSpaceProvider.java b/modules/dcache/src/main/java/org/dcache/auth/RemoteNameSpaceProvider.java
@@ -302,7 +302,6 @@ public void removeExtendedAttribute(Subject subject, FsPath path, String name)
         _pnfs.request(message);
     }
 
-
     /**
      * Remove a label attribute from a file.
      *
@@ -318,6 +317,10 @@ public void removeLabel(Subject subject, FsPath path, String label) throws Cache
         message.setSubject(subject);
         message.setRestriction(Restrictions.none());
         _pnfs.request(message);
+    }
 
+    @Override
+    public String resolveSymlinks(Subject subject, String path) throws CacheException {
+        throw new UnsupportedOperationException("For internal use only.");
     }
 }