Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make environment variables to preserve configurable #1148

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion .site/spi/run_integration_tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
tests/run_privileged_tests.sh
36 changes: 18 additions & 18 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

43 changes: 41 additions & 2 deletions crates/spfs-cli/cmd-enter/src/cmd_enter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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?;
Expand Down Expand Up @@ -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))
Expand All @@ -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))
Expand Down
2 changes: 1 addition & 1 deletion crates/spfs/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"] }
Expand Down
24 changes: 15 additions & 9 deletions crates/spfs/src/bootstrap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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([
Expand Down
4 changes: 2 additions & 2 deletions crates/spfs/src/bootstrap_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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");
Expand Down
19 changes: 19 additions & 0 deletions crates/spfs/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -474,6 +492,7 @@ pub struct Config {
pub fuse: Fuse,
pub monitor: Monitor,
pub sentry: Sentry,
pub environment: Environment,
}

impl Config {
Expand Down
1 change: 1 addition & 0 deletions crates/spfs/src/runtime/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ pub use storage::{
BindMount,
Config,
Data,
EnvKeyValue,
KeyValuePair,
KeyValuePairBuf,
LiveLayer,
Expand Down
Loading
Loading