Make environment variables to preserve configurable

We were preserving $TMPDIR specfically in a hard-coded way and there was
no way to opt out of it if desired. Now it has come to light that
$LD_LIBRARY_PATH is also cleared when entering an spfs runtime. We would
like to preserve that value too. Add a new configuration option so each
site can decide to opt in to the preserving. Doing so may come with
risks of privilege escalation.

Signed-off-by: J Robert Ray <[email protected]>

Author: jrray

.github/workflows/rust.yml

@@ -102,6 +102,12 @@ jobs:
         run: |
           export PATH="$PWD/target/debug:$PATH"
           make packages.lint
+      - name: Configure SPFS for Integration tests
+        run: |
+          cat << EOF > /etc/spfs.conf
+          [environment]
+          variable_names_to_preserve = ["TMPDIR"]
+          EOF
       - name: SPFS Integration Tests - Regular User
         run: |
           # Run tests as a normal user to verify privilege escalation

.site/spi/run_integration_tests.sh

@@ -11,8 +11,12 @@ mkdir -p "$ORIGIN_REPO"
 # Pre-create a repo
 SPFS_REMOTE_origin_ADDRESS="file://${ORIGIN_REPO}?create=true" spfs ls-tags -r origin
 export SPFS_REMOTE_origin_ADDRESS="file://${ORIGIN_REPO}"
+cat << EOF > /etc/spfs.conf
+[environment]
+variable_names_to_preserve = ["TMPDIR"]
+EOF
 # Run tests as a normal user to verify privilege escalation
 useradd -m e2e
 su e2e -c /tests/run_tests.sh
 # Run tests that need root
-tests/run_privileged_tests.sh
+tests/run_privileged_tests.sh

Cargo.lock

@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 // https://github.com/spkenv/spk
 
+use std::borrow::Cow;
 use std::ffi::OsString;
 #[cfg(feature = "sentry")]
 use std::sync::atomic::Ordering;
@@ -12,6 +13,7 @@ use clap::{Args, Parser};
 use cli::configure_sentry;
 use miette::{Context, Result};
 use spfs::monitor::SPFS_MONITOR_FOREGROUND_LOGGING_VAR;
+use spfs::runtime::EnvKeyValue;
 use spfs::storage::fs::RenderSummary;
 use spfs_cli_common as cli;
 use spfs_cli_common::CommandName;
@@ -80,13 +82,34 @@ pub struct RemountArgs {
     enabled: bool,
 }
 
+fn parse_env_key_value(s: &str) -> Result<EnvKeyValue> {
+    let parts: Vec<&str> = s.splitn(2, '=').collect();
+    if parts.len() != 2 {
+        miette::bail!("Invalid environment key-value pair (missing '='): {s}");
+    }
+    if parts[0].is_empty() {
+        miette::bail!("Invalid environment key-value pair (empty key): {s}");
+    }
+    if parts[0].contains(|c: char| c.is_whitespace()) {
+        miette::bail!("Invalid environment key-value pair (key contains whitespace): {s}");
+    }
+    Ok(EnvKeyValue(parts[0].to_string(), parts[1].to_string()))
+}
+
 #[derive(Debug, Args)]
 #[group(id = "enter_grp")]
 pub struct EnterArgs {
     /// The value to set $TMPDIR to in new environment
+    ///
+    /// Deprecated: use --environment-override instead
     #[clap(long)]
     tmpdir: Option<String>,
 
+    /// Optional keys and values to set in the new environment, in the form
+    /// KEY=VALUE. This option can be repeated.
+    #[clap(long, value_parser = parse_env_key_value)]
+    environment_override: Vec<EnvKeyValue>,
+
     /// Put the rendering and syncing times into environment variables
     #[clap(long)]
     metrics_in_env: bool,
@@ -138,6 +161,7 @@ impl CmdEnter {
         // this function will eventually be required to discover the overlayfs
         // attributes. It can take many milliseconds to run so we prime the cache as
         // soon as possible in a separate thread
+
         std::thread::spawn(spfs::runtime::overlayfs::overlayfs_available_options_prime_cache);
 
         let mut runtime = self.load_runtime(config).await?;
@@ -231,7 +255,14 @@ impl CmdEnter {
                 }
             };
 
-            owned.ensure_startup_scripts(self.enter.tmpdir.as_ref())?;
+            let mut environment_overrides = Cow::Borrowed(&self.enter.environment_override);
+            if let Some(tmpdir) = &self.enter.tmpdir {
+                environment_overrides
+                    .to_mut()
+                    .push(EnvKeyValue("TMPDIR".to_string(), tmpdir.to_string()));
+            }
+
+            owned.ensure_startup_scripts(environment_overrides.as_slice())?;
             std::env::set_var("SPFS_RUNTIME", owned.name());
 
             Ok(Some(owned))
@@ -254,7 +285,15 @@ impl CmdEnter {
             let start_time = Instant::now();
             let render_summary = spfs::initialize_runtime(&mut owned).await?;
             self.report_render_summary(render_summary, start_time.elapsed().as_secs_f64());
-            owned.ensure_startup_scripts(self.enter.tmpdir.as_ref())?;
+
+            let mut environment_overrides = Cow::Borrowed(&self.enter.environment_override);
+            if let Some(tmpdir) = &self.enter.tmpdir {
+                environment_overrides
+                    .to_mut()
+                    .push(EnvKeyValue("TMPDIR".to_string(), tmpdir.to_string()));
+            }
+
+            owned.ensure_startup_scripts(environment_overrides.as_slice())?;
             std::env::set_var("SPFS_RUNTIME", owned.name());
 
             Ok(Some(owned))

crates/spfs-cli/cmd-enter/src/cmd_enter.rs

@@ -50,7 +50,7 @@ gitignore = "1.0"
 glob = { workspace = true }
 hyper = { version = "0.14.16", features = ["client"] }
 indicatif = { workspace = true }
-itertools = "0.10.3"
+itertools = { workspace = true }
 libc = { workspace = true }
 miette = { workspace = true }
 nix = { workspace = true, features = ["fs"] }

crates/spfs/Cargo.toml

@@ -351,15 +351,21 @@ where
 
     let mut enter_args = Vec::new();
 
-    // Capture the current $TMPDIR value here before it is lost when running
-    // privileged process spfs-enter.
-    if let Some(tmpdir_value_for_child_process) = std::env::var_os("TMPDIR") {
-        tracing::trace!(
-            ?tmpdir_value_for_child_process,
-            "capture existing value for $TMPDIR (build_spfs_enter_command)"
-        );
-
-        enter_args.extend(["--tmpdir".into(), tmpdir_value_for_child_process]);
+    // Capture the configured environment variable values here before they are
+    // possibly lost when running privileged process spfs-enter.
+    let config = crate::get_config()?;
+    for key in &config.environment.variable_names_to_preserve {
+        if let Ok(value) = std::env::var(key) {
+            tracing::trace!(
+                ?key,
+                ?value,
+                "capture existing variable (build_spfs_enter_command)"
+            );
+            enter_args.extend([
+                "--environment_override".into(),
+                format!("{key}={value}").into(),
+            ]);
+        }
     }
 
     enter_args.extend([

crates/spfs/src/bootstrap.rs

@@ -57,7 +57,7 @@ async fn test_shell_initialization_startup_scripts(
 
     let tmp_startup_dir = tmpdir.path().join("startup.d");
     std::fs::create_dir(&tmp_startup_dir).unwrap();
-    rt.ensure_startup_scripts(None).unwrap();
+    rt.ensure_startup_scripts(&[]).unwrap();
     for startup_script in &[&rt.config.sh_startup_file, &rt.config.csh_startup_file] {
         let mut cmd = Command::new("sed");
         cmd.arg("-i");
@@ -129,7 +129,7 @@ async fn test_shell_initialization_no_startup_scripts(shell: &str, tmpdir: tempf
 
     let tmp_startup_dir = tmpdir.path().join("startup.d");
     std::fs::create_dir(&tmp_startup_dir).unwrap();
-    rt.ensure_startup_scripts(None).unwrap();
+    rt.ensure_startup_scripts(&[]).unwrap();
     for startup_script in &[&rt.config.sh_startup_file, &rt.config.csh_startup_file] {
         let mut cmd = Command::new("sed");
         cmd.arg("-i");

crates/spfs/src/bootstrap_test.rs

@@ -464,6 +464,24 @@ pub struct Sentry {
     pub email_domain: Option<String>,
 }
 
+#[derive(Clone, Default, Debug, Deserialize, Serialize)]
+#[serde(default)]
+pub struct Environment {
+    /// Environment variables names to preserve when creating an spfs
+    /// environment.
+    ///
+    /// Most environment variables are preserved by default but a few are
+    /// cleared for security purposes. Known values include `TMPDIR` and
+    /// `LD_LIBRARY_PATH`. Any variable listed here will be propagated into a
+    /// new spfs runtime by capturing their values before running spfs-enter and
+    /// then setting them back to the captured values from inside the spfs
+    /// runtime startup script.
+    ///
+    /// Any variables listed here that are not present in the environment will
+    /// remain unset in the new spfs environment.
+    pub variable_names_to_preserve: Vec<String>,
+}
+
 #[derive(Clone, Debug, Default, Deserialize, Serialize)]
 #[serde(default)]
 pub struct Config {
@@ -474,6 +492,7 @@ pub struct Config {
     pub fuse: Fuse,
     pub monitor: Monitor,
     pub sentry: Sentry,
+    pub environment: Environment,
 }
 
 impl Config {

crates/spfs/src/config.rs

@@ -24,6 +24,7 @@ pub use storage::{
     BindMount,
     Config,
     Data,
+    EnvKeyValue,
     KeyValuePair,
     KeyValuePairBuf,
     LiveLayer,