From b1854b7865fec2f066dc3685f23ad6eb35d9d03f Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Fri, 22 Nov 2024 17:18:55 -0800 Subject: [PATCH] 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 --- .github/workflows/rust.yml | 6 +++ .site/spi/run_integration_tests.sh | 6 ++- Cargo.lock | 36 +++++++++--------- crates/spfs-cli/cmd-enter/src/cmd_enter.rs | 43 +++++++++++++++++++++- crates/spfs/Cargo.toml | 2 +- crates/spfs/src/bootstrap.rs | 24 +++++++----- crates/spfs/src/bootstrap_test.rs | 4 +- crates/spfs/src/config.rs | 19 ++++++++++ crates/spfs/src/runtime/mod.rs | 1 + crates/spfs/src/runtime/startup_csh.rs | 40 +++++++++++--------- crates/spfs/src/runtime/startup_ps.rs | 10 +++-- crates/spfs/src/runtime/startup_sh.rs | 40 +++++++++++--------- crates/spfs/src/runtime/storage.rs | 12 ++++-- cspell.json | 1 + docs/admin/config.md | 5 +++ 15 files changed, 174 insertions(+), 75 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index d0c7476f0c..23f1ba5b7a 100644 --- a/.github/workflows/rust.yml +++ b/.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.toml + [environment] + variable_names_to_preserve = ["TMPDIR"] + EOF - name: SPFS Integration Tests - Regular User run: | # Run tests as a normal user to verify privilege escalation diff --git a/.site/spi/run_integration_tests.sh b/.site/spi/run_integration_tests.sh index 88fb370082..40267ead53 100755 --- a/.site/spi/run_integration_tests.sh +++ b/.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.toml +[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 \ No newline at end of file +tests/run_privileged_tests.sh diff --git a/Cargo.lock b/Cargo.lock index 6808d3fb56..709108f889 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1860,9 +1860,9 @@ dependencies = [ [[package]] name = "itertools" -version = "0.12.0" +version = "0.12.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "25db6b064527c5d482d0423354fcd07a89a2dfe07b67892e62411946db7f07b0" +checksum = "ba291022dbbd398a455acf126c1e341954079855bc60dfdda641363bd6922569" dependencies = [ "either", ] @@ -3615,7 +3615,7 @@ dependencies = [ "glob", "hyper 0.14.27", "indicatif", - "itertools 0.10.5", + "itertools 0.12.1", "libc", "miette", "nix", @@ -3939,7 +3939,7 @@ dependencies = [ "async-trait", "dunce", "futures", - "itertools 0.12.0", + "itertools 0.12.1", "miette", "relative-path", "rstest 0.18.2", @@ -4004,7 +4004,7 @@ dependencies = [ "clap_complete", "colored", "futures", - "itertools 0.12.0", + "itertools 0.12.1", "miette", "rstest 0.18.2", "serde", @@ -4030,7 +4030,7 @@ dependencies = [ "clap 4.5.0", "colored", "futures", - "itertools 0.12.0", + "itertools 0.12.1", "miette", "nom", "nom-supreme", @@ -4078,7 +4078,7 @@ dependencies = [ "colored", "dunce", "futures", - "itertools 0.12.0", + "itertools 0.12.1", "miette", "serde", "serde_json", @@ -4151,7 +4151,7 @@ dependencies = [ "clap 4.5.0", "colored", "futures", - "itertools 0.12.0", + "itertools 0.12.1", "miette", "spfs", "spk-build", @@ -4216,7 +4216,7 @@ dependencies = [ "async-trait", "clap 4.5.0", "futures", - "itertools 0.12.0", + "itertools 0.12.1", "miette", "rstest 0.18.2", "spfs", @@ -4371,7 +4371,7 @@ dependencies = [ "ignore", "indexmap 2.2.3", "is_default_derive_macro", - "itertools 0.12.0", + "itertools 0.12.1", "miette", "nom", "proptest", @@ -4407,7 +4407,7 @@ dependencies = [ "format_serde_error", "ignore", "indexmap 2.2.3", - "itertools 0.12.0", + "itertools 0.12.1", "miette", "nom", "nom-supreme", @@ -4438,7 +4438,7 @@ dependencies = [ "colored", "data-encoding", "format_serde_error", - "itertools 0.12.0", + "itertools 0.12.1", "miette", "nom", "nom-supreme", @@ -4482,7 +4482,7 @@ dependencies = [ "ctrlc", "dyn-clone", "futures", - "itertools 0.12.0", + "itertools 0.12.1", "miette", "num-bigint", "num-format", @@ -4519,7 +4519,7 @@ dependencies = [ "ctrlc", "dyn-clone", "futures", - "itertools 0.12.0", + "itertools 0.12.1", "miette", "once_cell", "priority-queue", @@ -4554,7 +4554,7 @@ dependencies = [ "dyn-clone", "futures", "glob", - "itertools 0.12.0", + "itertools 0.12.1", "miette", "once_cell", "rstest 0.18.2", @@ -4574,7 +4574,7 @@ version = "0.42.0" dependencies = [ "colored", "console", - "itertools 0.12.0", + "itertools 0.12.1", "miette", "serde", "spfs", @@ -4592,7 +4592,7 @@ dependencies = [ "dashmap", "enum_dispatch", "futures", - "itertools 0.12.0", + "itertools 0.12.1", "miette", "once_cell", "rstest 0.18.2", @@ -4623,7 +4623,7 @@ dependencies = [ "glob", "ignore", "indexmap 2.2.3", - "itertools 0.12.0", + "itertools 0.12.1", "miette", "nom", "once_cell", diff --git a/crates/spfs-cli/cmd-enter/src/cmd_enter.rs b/crates/spfs-cli/cmd-enter/src/cmd_enter.rs index 6866dd4e82..46589677a4 100644 --- a/crates/spfs-cli/cmd-enter/src/cmd_enter.rs +++ b/crates/spfs-cli/cmd-enter/src/cmd_enter.rs @@ -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 { + 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, + /// 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, + /// 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)) diff --git a/crates/spfs/Cargo.toml b/crates/spfs/Cargo.toml index 68aee5831c..f923c7ab75 100644 --- a/crates/spfs/Cargo.toml +++ b/crates/spfs/Cargo.toml @@ -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"] } diff --git a/crates/spfs/src/bootstrap.rs b/crates/spfs/src/bootstrap.rs index 5836fcc41c..b8d5ec82e0 100644 --- a/crates/spfs/src/bootstrap.rs +++ b/crates/spfs/src/bootstrap.rs @@ -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([ diff --git a/crates/spfs/src/bootstrap_test.rs b/crates/spfs/src/bootstrap_test.rs index a039d9d77c..10cd2886f5 100644 --- a/crates/spfs/src/bootstrap_test.rs +++ b/crates/spfs/src/bootstrap_test.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"); diff --git a/crates/spfs/src/config.rs b/crates/spfs/src/config.rs index 2b15380a47..e4195b0762 100644 --- a/crates/spfs/src/config.rs +++ b/crates/spfs/src/config.rs @@ -464,6 +464,24 @@ pub struct Sentry { pub email_domain: Option, } +#[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, +} + #[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 { diff --git a/crates/spfs/src/runtime/mod.rs b/crates/spfs/src/runtime/mod.rs index 0ee10a9bac..3191c9b13f 100644 --- a/crates/spfs/src/runtime/mod.rs +++ b/crates/spfs/src/runtime/mod.rs @@ -24,6 +24,7 @@ pub use storage::{ BindMount, Config, Data, + EnvKeyValue, KeyValuePair, KeyValuePairBuf, LiveLayer, diff --git a/crates/spfs/src/runtime/startup_csh.rs b/crates/spfs/src/runtime/startup_csh.rs index c99983165d..201261ed3a 100644 --- a/crates/spfs/src/runtime/startup_csh.rs +++ b/crates/spfs/src/runtime/startup_csh.rs @@ -2,23 +2,29 @@ // SPDX-License-Identifier: Apache-2.0 // https://github.com/spkenv/spk -pub fn source(tmpdir: Option<&T>) -> String -where - T: AsRef, -{ - let tmpdir_replacement = tmpdir - .as_ref() - .map(|value| { - format!( - r#"# Re-assign $TMPDIR because this value is lost when -# exec'ing a privileged process. -setenv TMPDIR "{}" +use itertools::Itertools; -"#, - value.as_ref() - ) - }) - .unwrap_or_default(); +use super::EnvKeyValue; + +pub fn source(environment_overrides: &[EnvKeyValue]) -> String { + let mut env_replacement = String::new(); + for (position, key_value) in environment_overrides.iter().with_position() { + match position { + itertools::Position::First | itertools::Position::Only => { + env_replacement.push_str("# Re-assign variables as configured.\n"); + env_replacement.push_str("# The values of these variables may be lost when exec'ing a privileged process or unsharing the mount namespace.\n"); + } + _ => {} + }; + let value = key_value.1.replace("\"", "\\\""); + env_replacement.push_str(&format!("setenv {key} \"{value}\"\n", key = key_value.0)); + match position { + itertools::Position::Last | itertools::Position::Only => { + env_replacement.push('\n'); + } + _ => {} + }; + } format!( r#"#!/usr/bin/env csh @@ -32,7 +38,7 @@ else if ( -f ~/.cshrc ) then source ~/.cshrc || true endif -{tmpdir_replacement} +{env_replacement} set startup_dir="/spfs/etc/spfs/startup.d" if ( -d "${{startup_dir}}" != 0 ) then set filenames=`/bin/ls $startup_dir | grep '\.csh\s*$'` diff --git a/crates/spfs/src/runtime/startup_ps.rs b/crates/spfs/src/runtime/startup_ps.rs index f5e90daf0d..68395fde56 100644 --- a/crates/spfs/src/runtime/startup_ps.rs +++ b/crates/spfs/src/runtime/startup_ps.rs @@ -2,10 +2,12 @@ // SPDX-License-Identifier: Apache-2.0 // https://github.com/spkenv/spk -pub fn source(_tmpdir: Option<&T>) -> String -where - T: AsRef, -{ +use itertools::Itertools; + +use super::EnvKeyValue; + +pub fn source(_environment_overrides: &[EnvKeyValue]) -> String { + // TODO: Support environment overrides on Windows r#" param ( [string]$RunCommand diff --git a/crates/spfs/src/runtime/startup_sh.rs b/crates/spfs/src/runtime/startup_sh.rs index 9554a0d954..72f31f1555 100644 --- a/crates/spfs/src/runtime/startup_sh.rs +++ b/crates/spfs/src/runtime/startup_sh.rs @@ -2,23 +2,29 @@ // SPDX-License-Identifier: Apache-2.0 // https://github.com/spkenv/spk -pub fn source(tmpdir: Option<&T>) -> String -where - T: AsRef, -{ - let tmpdir_replacement = tmpdir - .as_ref() - .map(|value| { - format!( - r#"# Re-assign $TMPDIR because this value is lost when -# exec'ing a privileged process. -export TMPDIR="{}" +use itertools::Itertools; -"#, - value.as_ref() - ) - }) - .unwrap_or_default(); +use super::EnvKeyValue; + +pub fn source(environment_overrides: &[EnvKeyValue]) -> String { + let mut env_replacement = String::new(); + for (position, key_value) in environment_overrides.iter().with_position() { + match position { + itertools::Position::First | itertools::Position::Only => { + env_replacement.push_str("# Re-assign variables as configured.\n"); + env_replacement.push_str("# The values of these variables may be lost when exec'ing a privileged process or unsharing the mount namespace.\n"); + } + _ => {} + }; + let value = key_value.1.replace("\"", "\\\""); + env_replacement.push_str(&format!("export {key}=\"{value}\"\n", key = key_value.0)); + match position { + itertools::Position::Last | itertools::Position::Only => { + env_replacement.push('\n'); + } + _ => {} + }; + } format!( r#"#!/usr/bin/env sh @@ -26,7 +32,7 @@ if [ -f ~/.bashrc ]; then source ~/.bashrc || true fi -{tmpdir_replacement} +{env_replacement} startup_dir="/spfs/etc/spfs/startup.d" if [ -d "${{startup_dir}}" ]; then filenames=$(/bin/ls $startup_dir | grep '\.sh$') diff --git a/crates/spfs/src/runtime/storage.rs b/crates/spfs/src/runtime/storage.rs index 002fe1f038..4cd824b565 100644 --- a/crates/spfs/src/runtime/storage.rs +++ b/crates/spfs/src/runtime/storage.rs @@ -654,6 +654,10 @@ impl OwnedRuntime { } } +/// A key-value pair for setting environment variables. +#[derive(Clone, Debug)] +pub struct EnvKeyValue(pub String, pub String); + /// Represents an active spfs session. /// /// The runtime contains the working files for a spfs @@ -1021,24 +1025,24 @@ impl Runtime { /// defined location. pub fn ensure_startup_scripts( &self, - tmpdir_value_for_child_process: Option<&String>, + environment_overrides_for_child_process: &[EnvKeyValue], ) -> Result<()> { #[cfg(unix)] std::fs::write( &self.config.sh_startup_file, - startup_sh::source(tmpdir_value_for_child_process), + startup_sh::source(environment_overrides_for_child_process), ) .map_err(|err| Error::RuntimeWriteError(self.config.sh_startup_file.clone(), err))?; #[cfg(unix)] std::fs::write( &self.config.csh_startup_file, - startup_csh::source(tmpdir_value_for_child_process), + startup_csh::source(environment_overrides_for_child_process), ) .map_err(|err| Error::RuntimeWriteError(self.config.csh_startup_file.clone(), err))?; #[cfg(windows)] std::fs::write( &self.config.ps_startup_file, - startup_ps::source(tmpdir_value_for_child_process), + startup_ps::source(environment_overrides_for_child_process), ) .map_err(|err| Error::RuntimeWriteError(self.config.ps_startup_file.clone(), err))?; Ok(()) diff --git a/cspell.json b/cspell.json index 010e8ae2bb..9511b0c3ec 100644 --- a/cspell.json +++ b/cspell.json @@ -737,6 +737,7 @@ "unresolves", "unrestrictive", "unsetenv", + "unsharing", "untar", "unvalidated", "UOBZUVB", diff --git a/docs/admin/config.md b/docs/admin/config.md index f6228faa1f..139a3935ef 100644 --- a/docs/admin/config.md +++ b/docs/admin/config.md @@ -175,6 +175,11 @@ worker_threads = 2 # the number of blocking threads used for IO operations in the # runtime monitor process. max_blocking_threads = 2 + +# Optional environment variable names to preserve the value when creating an +# spfs runtime. +[environment] +variable_names_to_preserve = ["TMPDIR", "LD_LIBRARY_PATH"] ``` ### SPK Configuration