slurm plugin validates UENV_MOUNT_LIST in the local context

src/slurm/mount.cpp

@@ -13,14 +13,65 @@
 #include <sys/types.h>
 #include <unistd.h>
 
+#include <fmt/format.h>
 #include <libmount/libmount.h>
 
-// #include "filesystem.hpp"
 #include <uenv/mount.h>
 #include <util/expected.h>
 
 namespace uenv {
 
+util::expected<void, std::string> mount_entry::validate() const {
+    namespace fs = std::filesystem;
+
+    auto mount = fs::path(mount_path);
+
+    // does the mount point exist?
+    if (!fs::exists(mount)) {
+        return util::unexpected(
+            fmt::format("the mount point '{}' does not exist", mount_path));
+    }
+
+    mount = fs::canonical(mount);
+
+    // is the mount point a directory?
+    if (!fs::is_directory(mount)) {
+        return util::unexpected(
+            fmt::format("the mount point '{}' is not a directory", mount_path));
+    }
+
+    auto sqfs = fs::path(sqfs_path);
+
+    // does the squashfs path exist?
+    if (!fs::exists(sqfs)) {
+        return util::unexpected(
+            fmt::format("the squashfs file '{}' does not exist", sqfs_path));
+    }
+
+    // remove symlink etc, so that we can test file and permissions
+    sqfs = fs::canonical(sqfs);
+
+    // is the squashfs path a file ?
+    if (!fs::is_regular_file(sqfs)) {
+        return util::unexpected(
+            fmt::format("the squashfs file '{}' is not a file", sqfs_path));
+    }
+
+    // do we have read access to the squashfs file
+    const fs::perms sqfs_perm = fs::status(sqfs).permissions();
+    auto satisfies = [&sqfs_perm](fs::perms c) {
+        return fs::perms::none != (sqfs_perm & c);
+    };
+    if (!(satisfies(fs::perms::owner_read) ||
+          satisfies(fs::perms::group_read))) {
+        return util::unexpected(
+            fmt::format("you do not have read access to the squashfs file '{}'",
+                        sqfs_path));
+    }
+
+    return {};
+}
+
 util::expected<void, std::string>
 do_mount(const std::vector<mount_entry>& mount_entries) {
     if (mount_entries.size() == 0) {

src/slurm/plugin.cpp

@@ -137,6 +137,22 @@ std::optional<std::string> getenv_wrapper(spank_t sp, const char* var) {
     throw ret;
 }
 
+util::expected<std::vector<uenv::mount_entry>, std::string>
+validate_uenv_mount_list(std::string mount_var) {
+    auto mount_list = uenv::parse_mount_list(mount_var);
+    if (!mount_list) {
+        return util::unexpected(mount_list.error().message());
+    }
+
+    for (auto& entry : *mount_list) {
+        if (auto valid = entry.validate(); !valid) {
+            return util::unexpected(valid.error());
+        }
+    }
+
+    return *mount_list;
+}
+
 static spank_option uenv_arg{
     (char*)"uenv",
     (char*)"<file>[:mount-point][,<file:mount-point>]*",
@@ -201,10 +217,10 @@ int init_post_opt_remote(spank_t sp) {
         return ESPANK_SUCCESS;
     }
 
-    auto mount_list = uenv::parse_mount_list(*mount_var);
+    auto mount_list = validate_uenv_mount_list(*mount_var);
     if (!mount_list) {
         slurm_error("internal error parsing the mount list: %s",
-                    mount_list.error().message().c_str());
+                    mount_list.error().c_str());
         return -ESPANK_ERROR;
     }
 
@@ -234,6 +250,21 @@ int init_post_opt_local_allocator(spank_t sp [[maybe_unused]]) {
                         args.view_description->c_str());
             return -ESPANK_ERROR;
         }
+
+        // check whether a uenv has been mounted in the calling environment.
+        // this will be mounted in the remote context, so check that:
+        // * the squashfs image exists
+        // * the user has read access to the squashfs image
+        // * the mount point exists
+        if (auto mount_var = getenv_wrapper(sp, "UENV_MOUNT_LIST")) {
+            if (auto mount_list = validate_uenv_mount_list(*mount_var);
+                !mount_list) {
+                slurm_error("invalid UENV_MOUNT_LIST: %s",
+                            mount_list.error().c_str());
+                return -ESPANK_ERROR;
+            }
+        }
+
         return ESPANK_SUCCESS;
     }
 

src/uenv/mount.h

@@ -1,7 +1,6 @@
 #pragma once
 
 #include <string>
-#include <vector>
 
 #include <util/expected.h>
 
@@ -10,6 +9,8 @@ namespace uenv {
 struct mount_entry {
     std::string sqfs_path;
     std::string mount_path;
+
+    util::expected<void, std::string> validate() const;
 };
 
 } // namespace uenv

test/integration/common.bash

@@ -31,8 +31,5 @@ function run_sbatch_unchecked() {
 
 function run_sbatch() {
   run_sbatch_unchecked "$@"
-  # workaround for `run sbatch --wait` always returning status==0
-  # grep slurm log for 'srun: error'
-  run bash -c "cat $slurm_log | grep -q 'srun: error'"
-  [ ! "${status}" -eq 0 ]
+  [ "${status}" -eq 0 ]
 }

test/integration/slurm.bats

@@ -7,6 +7,7 @@ function setup() {
     load ./common
 
     export REPOS=$(realpath ../scratch/repos)
+    export SQFS_LIB=$(realpath ../scratch/sqfs)
 
     unset UENV_MOUNT_LIST
 }
@@ -136,17 +137,59 @@ function teardown() {
     assert_output --partial 'invalid uenv description'
 }
 
+@test "sbatch" {
+    export UENV_REPO_PATH=$REPOS/apptool
+    run_sbatch <<EOF
+#!/bin/bash
+#SBATCH --uenv=app/42.0,tool
+set -e
+srun findmnt /user-environment
+srun findmnt /user-tools
+EOF
+}
+
 @test "sbatch override in srun" {
     export UENV_REPO_PATH=$REPOS/apptool
     # check that images mounted via sbatch --uenv are overriden when `--uenv` flag is given to srun
     run_sbatch <<EOF
 #!/bin/bash
 #SBATCH --uenv=app/42.0
 
+set -e
+
 # override --uenv and mount under /user-tools instead
 srun --uenv=tool findmnt /user-tools
 
 # override, /user-environment must not be mounted
 srun --uenv=tool bash -c '! findmnt /user-environment'
 EOF
 }
+
+@test "sbatch UENV_MOUNT_LIST with no --uenv" {
+    # this should be independent of the repo
+    unset UENV_REPO_PATH
+
+    export UENV_MOUNT_LIST=$SQFS_LIB/apptool/tool/store.squashfs:/user-tools
+    run_sbatch <<EOF
+#!/bin/bash
+set -e
+srun findmnt /user-tools
+srun bash -c '! findmnt /user-environment'
+EOF
+
+    # sbatch should error if sqfs does not exist (there is a typo "toool" in sqfs name)
+    export UENV_MOUNT_LIST=$SQFS_LIB/apptool/toool/store.squashfs:/user-tools
+    run run_sbatch <<EOF
+#!/bin/bash
+echo "should not get here"
+EOF
+    [ "${status}" -eq "1" ]
+
+    # sbatch should error if mount does not exist
+    export UENV_MOUNT_LIST=$SQFS_LIB/apptool/tool/store.squashfs:/user-toooools
+    run run_sbatch <<EOF
+#!/bin/bash
+echo "should not get here"
+EOF
+    [ "${status}" -eq "1" ]
+}

test/setup/setup_repos.bash

@@ -5,6 +5,7 @@ function setup_repo_apptool() {
     working=$(cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd)
 
     repo=${scratch}/repos/apptool
+    sqfs_path=${scratch}/sqfs/apptool
     sources=${working}/apptool
 
     echo "repo path ${repo}"
@@ -13,9 +14,11 @@ function setup_repo_apptool() {
 
     # clean up previous builds before starting
     rm -rf ${repo}
+    rm -rf ${sqfs_path}
 
     # create an empty repo
     mkdir -p ${repo}
+    mkdir -p ${sqfs_path}
 
     cp ${sources}/schema.sql.template schema.sql
 
@@ -30,10 +33,13 @@ function setup_repo_apptool() {
 
         echo ${sha} ${name}
 
-        img_path=$repo/images/${sha}
-        mkdir -p $img_path
-        mv ${sqfs} $img_path/store.squashfs
-        cp -R ${sources}/${name}/meta $img_path
+        for img_path in "$repo/images/${sha}" "$sqfs_path/$name"
+        do
+            mkdir -p $img_path
+            cp ${sqfs} $img_path/store.squashfs
+            cp -R ${sources}/${name}/meta $img_path
+        done
+        rm ${sqfs}
 
         sed -i "s|{${name}-sha}|${sha}|g" schema.sql
         sed -i "s|{${name}-id}|${id}|g" schema.sql