vincenzopalazzo
/
arti
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge branch 'mistrust-envvar' into 'main' 

Move environment-variable checking into fs-mistrust

Closes #483

See merge request tpo/core/arti!630
This commit is contained in:
Nick Mathewson 2022-07-19 20:46:47 +00:00
parent 6f1de00369 d2b64642e1
commit 414939bf8c
10 changed files with 257 additions and 106 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
crates/arti-client/semver.md Normal file
View File

@ -0,0 +1 @@
BREAKING: Removed some TorClientBuilder methods related to Mistrust, in favour of new fs-mistrust features.

56
crates/arti-client/src/builder.rs
View File

@ -3,7 +3,6 @@
#![allow(missing_docs, clippy::missing_docs_in_private_items)]
use crate::{err::ErrorDetail, BootstrapBehavior, Result, TorClient, TorClientConfig};
use fs_mistrust::Mistrust;
use std::sync::Arc;
use tor_dirmgr::DirMgrConfig;
use tor_rtcompat::Runtime;
@ -39,18 +38,6 @@ impl<R: Runtime> DirProviderBuilder<R> for DirMgrBuilder {
}
}
/// Rules about whether to replace the `Mistrust` from the configuration.
#[derive(Clone, Debug)]
enum FsMistrustOverride {
/// Disable the mistrust in the configuration if the the environment
/// variable `ARTI_FS_DISABLE_PERMISSION_CHECKS` is set.
FromEnvironment,
/// Disable the mistrust in the configuration unconditionally.
Disable,
/// Always use the mistrust in the configuration.
None,
}
/// An object for constructing a [`TorClient`].
///
/// Returned by [`TorClient::builder()`].
@ -64,8 +51,6 @@ pub struct TorClientBuilder<R: Runtime> {
/// How the client should behave when it is asked to do something on the Tor
/// network before `bootstrap()` is called.
bootstrap_behavior: BootstrapBehavior,
/// How the client should decide which file permissions to trust.
fs_mistrust_override: FsMistrustOverride,
/// Optional object to construct a DirProvider.
///
/// Wrapped in an Arc so that we don't need to force DirProviderBuilder to
@ -85,7 +70,6 @@ impl<R: Runtime> TorClientBuilder<R> {
runtime,
config: TorClientConfig::default(),
bootstrap_behavior: BootstrapBehavior::default(),
fs_mistrust_override: FsMistrustOverride::FromEnvironment,
dirmgr_builder: Arc::new(DirMgrBuilder {}),
#[cfg(feature = "dirfilter")]
dirfilter: None,
@ -109,35 +93,6 @@ impl<R: Runtime> TorClientBuilder<R> {
self
}
/// Build an [`TorClient`] that will not validate permissions and ownership
/// on the filesystem.
///
/// By default, these checks are configured with the `storage.permissions`
/// field of the configuration, and can be overridden with the
/// `ARTI_FS_DISABLE_PERMISSION_CHECKS` environment variable.
pub fn disable_fs_permission_checks(mut self) -> Self {
self.fs_mistrust_override = FsMistrustOverride::Disable;
self
}
/// Build a [`TorClient`] that will follow the permissions checks in
/// the `storage.permissions` field of the configuration, regardless of how the
/// environment is set.
pub fn ignore_fs_permission_checks_env_var(mut self) -> Self {
self.fs_mistrust_override = FsMistrustOverride::None;
self
}
/// Build a [`TorClient`] that will follow the permissions checks in the
/// `storage.permissions` field of the configuration, unless the
/// `ARTI_FS_DISABLE_PERMISSION_CHECKS` environment variable is set.
///
/// This is the default.
pub fn obey_fs_permission_checks_env_var(mut self) -> Self {
self.fs_mistrust_override = FsMistrustOverride::FromEnvironment;
self
}
/// Override the default function used to construct the directory provider.
///
/// Only available when compiled with the `experimental-api` feature: this
@ -187,21 +142,10 @@ impl<R: Runtime> TorClientBuilder<R> {
dirmgr_extensions.filter = self.dirfilter;
}
let override_mistrust: Option<Mistrust> = match self.fs_mistrust_override {
FsMistrustOverride::FromEnvironment
if crate::config::fs_permissions_checks_disabled_via_env() =>
{
Some(Mistrust::new_dangerously_trust_everyone())
}
FsMistrustOverride::Disable => Some(Mistrust::new_dangerously_trust_everyone()),
_ => None,
};
TorClient::create_inner(
self.runtime,
self.config,
self.bootstrap_behavior,
override_mistrust,
self.dirmgr_builder.as_ref(),
dirmgr_extensions,
)

18
crates/arti-client/src/client.rs
View File

@ -87,9 +87,6 @@ pub struct TorClient<R: Runtime> {
/// Shared boolean for whether we're currently in "dormant mode" or not.
dormant: Arc<Mutex<DropNotifyWatchSender<Option<DormantMode>>>>,
/// Settings for how we perform permissions checks on the filesystem.
fs_mistrust: fs_mistrust::Mistrust,
}
/// Preferences for whether a [`TorClient`] should bootstrap on its own or not.
@ -354,18 +351,18 @@ impl<R: Runtime> TorClient<R> {
runtime: R,
config: TorClientConfig,
autobootstrap: BootstrapBehavior,
mistrust: Option<fs_mistrust::Mistrust>,
dirmgr_builder: &dyn crate::builder::DirProviderBuilder<R>,
dirmgr_extensions: tor_dirmgr::config::DirMgrExtensions,
) -> StdResult<Self, ErrorDetail> {
let mistrust = mistrust.unwrap_or_else(|| config.storage.permissions().clone());
let dir_cfg = {
let mut c: tor_dirmgr::DirMgrConfig = config.dir_mgr_config(mistrust.clone())?;
let mut c: tor_dirmgr::DirMgrConfig = config.dir_mgr_config()?;
c.extensions = dirmgr_extensions;
c
};
let statemgr =
FsStateMgr::from_path_and_mistrust(config.storage.expand_state_dir()?, &mistrust)?;
let statemgr = FsStateMgr::from_path_and_mistrust(
config.storage.expand_state_dir()?,
config.storage.permissions(),
)?;
let addr_cfg = config.address_filter.clone();
let (status_sender, status_receiver) = postage::watch::channel();
@ -429,7 +426,6 @@ impl<R: Runtime> TorClient<R> {
bootstrap_in_progress: Arc::new(AsyncMutex::new(())),
should_bootstrap: autobootstrap,
dormant: Arc::new(Mutex::new(dormant_send)),
fs_mistrust: mistrust,
})
}
@ -558,9 +554,7 @@ impl<R: Runtime> TorClient<R> {
_ => {}
}
let dir_cfg = new_config
.dir_mgr_config(self.fs_mistrust.clone())
.map_err(wrap_err)?;
let dir_cfg = new_config.dir_mgr_config().map_err(wrap_err)?;
let state_cfg = new_config.storage.expand_state_dir().map_err(wrap_err)?;
let addr_cfg = &new_config.address_filter;
let timeout_cfg = &new_config.stream_timeouts;

21
crates/arti-client/src/config.rs
View File

@ -120,10 +120,13 @@ impl BuilderExt for MistrustBuilder {
type Built = Mistrust;
fn build_for_arti(&self) -> Result<Self::Built, ConfigBuildError> {
self.build().map_err(|e| ConfigBuildError::Invalid {
field: "permissions".to_string(),
problem: e.to_string(),
})
self.clone()
.controlled_by_env_var_if_not_set(FS_PERMISSIONS_CHECKS_DISABLE_VAR)
.build()
.map_err(|e| ConfigBuildError::Invalid {
field: "permissions".to_string(),
problem: e.to_string(),
})
}
}
@ -314,13 +317,13 @@ impl AsRef<tor_guardmgr::fallback::FallbackList> for TorClientConfig {
impl TorClientConfig {
/// Try to create a DirMgrConfig corresponding to this object.
#[rustfmt::skip]
pub(crate) fn dir_mgr_config(&self, mistrust: fs_mistrust::Mistrust) -> Result<dir::DirMgrConfig, ConfigBuildError> {
pub(crate) fn dir_mgr_config(&self) -> Result<dir::DirMgrConfig, ConfigBuildError> {
Ok(dir::DirMgrConfig {
network: self.tor_network .clone(),
schedule: self.download_schedule .clone(),
tolerance: self.download_tolerance .clone(),
cache_path: self.storage.expand_cache_dir()?,
cache_trust: mistrust,
cache_trust: self.storage.permissions.clone(),
override_net_params: self.override_net_params.clone(),
extensions: Default::default(),
})
@ -369,14 +372,18 @@ pub fn default_config_file() -> Result<PathBuf, CfgPathError> {
CfgPath::new("${ARTI_CONFIG}/arti.toml".into()).path()
}
/// The environment variable we look at when deciding whether to disable FS permissions checking.
pub const FS_PERMISSIONS_CHECKS_DISABLE_VAR: &str = "ARTI_FS_DISABLE_PERMISSION_CHECKS";
/// Return true if the environment has been set up to disable FS permissions
/// checking.
///
/// This function is exposed so that other tools can use the same checking rules
/// as `arti-client`. For more information, see
/// [`TorClientBuilder`](crate::TorClientBuilder).
#[deprecated(since = "0.5.0")]
pub fn fs_permissions_checks_disabled_via_env() -> bool {
std::env::var_os("ARTI_FS_DISABLE_PERMISSION_CHECKS").is_some()
std::env::var_os(FS_PERMISSIONS_CHECKS_DISABLE_VAR).is_some()
}
#[cfg(test)]

1
crates/arti/semver.md Normal file
View File

@ -0,0 +1 @@
BREAKING: run no longer takes fs_mistrust_disabled.

43
crates/arti/src/lib.rs
View File

@ -165,7 +165,7 @@ pub use cfg::{
};
pub use logging::{LoggingConfig, LoggingConfigBuilder};
use arti_client::config::{default_config_file, fs_permissions_checks_disabled_via_env};
use arti_client::config::default_config_file;
use arti_client::{TorClient, TorClientConfig};
use safelog::with_safe_logging_suppressed;
use tor_config::ConfigurationSources;
@ -223,18 +223,14 @@ pub async fn run<R: Runtime>(
config_sources: ConfigurationSources,
arti_config: ArtiConfig,
client_config: TorClientConfig,
fs_mistrust_disabled: bool,
) -> Result<()> {
// Using OnDemand arranges that, while we are bootstrapping, incoming connections wait
// for bootstrap to complete, rather than getting errors.
use arti_client::BootstrapBehavior::OnDemand;
use futures::FutureExt;
let mut client_builder = TorClient::with_runtime(runtime.clone())
let client_builder = TorClient::with_runtime(runtime.clone())
.config(client_config)
.bootstrap_behavior(OnDemand);
if fs_mistrust_disabled {
client_builder = client_builder.disable_fs_permission_checks();
}
let client = client_builder.create_unbootstrapped()?;
if arti_config.application().watch_configuration {
watch_cfg::watch_for_config_changes(config_sources, arti_config, client.clone())?;
@ -403,22 +399,30 @@ pub fn main_main() -> Result<()> {
let pre_config_logging_ret = tracing::dispatcher::with_default(&pre_config_logging, || {
let matches = clap_app.get_matches();
let fs_mistrust_disabled = fs_permissions_checks_disabled_via_env()
| matches.is_present("disable-fs-permission-checks");
let fs_mistrust_disabled = matches.is_present("disable-fs-permission-checks");
// A Mistrust object to use for loading our configuration. Elsewhere, we
// use the value _from_ the configuration.
let cfg_mistrust = if fs_mistrust_disabled {
fs_mistrust::Mistrust::new_dangerously_trust_everyone()
} else {
fs_mistrust::Mistrust::new()
fs_mistrust::MistrustBuilder::default()
.controlled_by_env_var(arti_client::config::FS_PERMISSIONS_CHECKS_DISABLE_VAR)
.build()
.expect("Could not construct default fs-mistrust")
};
let mut override_options: Vec<&str> =
matches.values_of("option").unwrap_or_default().collect();
if fs_mistrust_disabled {
override_options.push("storage.permissions.dangerously_trust_everyone=true");
}
let cfg_sources = {
let mut cfg_sources = ConfigurationSources::from_cmdline(
default_config_file()?,
matches.values_of_os("config-files").unwrap_or_default(),
matches.values_of("option").unwrap_or_default(),
override_options,
);
cfg_sources.set_mistrust(cfg_mistrust);
cfg_sources
@ -428,25 +432,13 @@ pub fn main_main() -> Result<()> {
let (config, client_config) =
tor_config::resolve::<ArtiCombinedConfig>(cfg).context("read configuration")?;
let log_mistrust = if fs_mistrust_disabled {
fs_mistrust::Mistrust::new_dangerously_trust_everyone()
} else {
client_config.fs_mistrust().clone()
};
let log_mistrust = client_config.fs_mistrust().clone();
Ok::<_, Error>((
matches,
cfg_sources,
config,
client_config,
fs_mistrust_disabled,
log_mistrust,
))
Ok::<_, Error>((matches, cfg_sources, config, client_config, log_mistrust))
})?;
// Sadly I don't seem to be able to persuade rustfmt to format the two lists of
// variable names identically.
let (matches, cfg_sources, config, client_config, fs_mistrust_disabled, log_mistrust) =
pre_config_logging_ret;
let (matches, cfg_sources, config, client_config, log_mistrust) = pre_config_logging_ret;
let _log_guards = logging::setup_logging(
config.logging(),
@ -486,7 +478,6 @@ pub fn main_main() -> Result<()> {
cfg_sources,
config,
client_config,
fs_mistrust_disabled,
))?;
Ok(())
} else {

1
crates/fs-mistrust/semver.md Normal file
View File

@ -0,0 +1 @@
MODIFIED: New facilities to configure how we look at the environment.

122
crates/fs-mistrust/src/disable.rs Normal file
View File

@ -0,0 +1,122 @@
//! Functionality for disabling `fs-mistrust` checks based on configuration or
//! environment variables.
use educe::Educe;
use std::env::{self, VarError};
/// Convenience type to indicate whether permission checks are disabled.
///
/// Used to avoid accidents with boolean meanings.
#[derive(Copy, Clone, Eq, PartialEq, Debug)]
pub(crate) enum Status {
/// We should indeed run permission checks, and treat some users as untrusted.
CheckPermissions,
/// We should treat every user as trusted, and therefore disable (most)
/// permissions checks.
DisableChecks,
}
impl Status {
/// Return true if this `Status` tells us to disable checks.
pub(crate) fn disabled(self) -> bool {
self == Status::DisableChecks
}
}
/// An environment variable which, if set, will cause a us to trust all users
/// (and therefore, in effect, to disable all permissions checks.)
pub const GLOBAL_DISABLE_VAR: &str = "FS_MISTRUST_DISABLE_PERMISSIONS_CHECKS";
/// Value to configure when permission checks should be disabled. This type is
/// set in the builder, and converted to a bool in the `Mistrust`.
#[derive(Clone, Debug, Educe, Eq, PartialEq)]
#[educe(Default)]
pub(crate) enum Disable {
/// Check a caller-provided environment variable, and honor it if it is set.
/// If it is not set, fall back to checking
/// `$FS_MISTRUST_DISABLE_PERMISSIONS_CHECKS`.
OnUserEnvVar(String),
/// Disable permissions checks if the value of
/// `$FS_MISTRUST_DISABLE_PERMISSIONS_CHECKS` is something other than "false",
/// "0", "no", etc..
///
/// This is the default.
#[educe(Default)]
OnGlobalEnvVar,
/// Perform permissions checks regardless of any values in the environment.
Never,
}
/// Convert the result of `std::env::var` to a boolean, if the variable is set.
///
/// Names that seem to say "don't disable" are treated as `Some(false)`. Any
/// other value is treated as `Some(true)`. (That is, we err on the side of
/// assuming that if you set a disable variable, you meant to disable.)
///
/// Absent environment vars, or those set to the empty string, are treated as
/// None.
#[allow(clippy::match_like_matches_macro)]
fn from_env_var_value(input: std::result::Result<String, VarError>) -> Option<Status> {
let mut s = match input {
Ok(s) => s,
Err(VarError::NotPresent) => return None,
Err(VarError::NotUnicode(_)) => return Some(Status::DisableChecks),
};
s.make_ascii_lowercase();
match s.as_ref() {
"" => None,
"0" | "no" | "never" | "false" | "n" => Some(Status::CheckPermissions),
_ => Some(Status::DisableChecks),
}
}
/// As `from_env_value`, but takes the name of the variable.
fn from_env_var(varname: &str) -> Option<Status> {
from_env_var_value(env::var(varname))
}
impl Disable {
/// Return true if, based on this [`Disable`] setting, and on the
/// environment, we should disable permissions checking.
pub(crate) fn should_disable_checks(&self) -> Status {
match self {
Disable::OnUserEnvVar(varname) => from_env_var(varname)
.or_else(|| from_env_var(GLOBAL_DISABLE_VAR))
.unwrap_or(Status::CheckPermissions),
Disable::OnGlobalEnvVar => {
from_env_var(GLOBAL_DISABLE_VAR).unwrap_or(Status::CheckPermissions)
}
Disable::Never => Status::CheckPermissions,
}
}
}
#[cfg(test)]
mod test {
use super::*;
#[test]
fn from_val() {
for word in ["yes", "1", "true", "certainly", "whatever"] {
assert_eq!(
from_env_var_value(Ok(word.into())),
Some(Status::DisableChecks)
);
}
for word in ["no", "0", "false", "NO", "Never", "n"] {
assert_eq!(
from_env_var_value(Ok(word.into())),
Some(Status::CheckPermissions)
);
}
assert_eq!(from_env_var_value(Ok("".into())), None);
assert_eq!(from_env_var_value(Err(VarError::NotPresent)), None);
assert_eq!(
from_env_var_value(Err(VarError::NotUnicode("".into()))),
Some(Status::DisableChecks)
);
}
}

4
crates/fs-mistrust/src/imp.rs
View File

@ -42,7 +42,7 @@ impl<'a> super::Verifier<'a> {
// to the code. It's not urgent, since the allocations won't cost much
// compared to the filesystem access.
pub(crate) fn check_errors(&self, path: &Path) -> impl Iterator<Item = Error> + '_ {
if self.mistrust.dangerously_trust_everyone {
if self.mistrust.is_disabled() {
// We don't want to walk the path in this case at all: we'll just
// look at the last element.
@ -88,7 +88,7 @@ impl<'a> super::Verifier<'a> {
pub(crate) fn check_content_errors(&self, path: &Path) -> impl Iterator<Item = Error> + '_ {
use std::sync::Arc;
if !self.check_contents || self.mistrust.dangerously_trust_everyone {
if !self.check_contents || self.mistrust.is_disabled() {
return boxed(std::iter::empty());
}

96
crates/fs-mistrust/src/lib.rs
View File

@ -280,6 +280,7 @@
//! <!-- @@ end lint list maintained by maint/add_warning @@ -->
mod dir;
mod disable;
mod err;
mod imp;
#[cfg(target_family = "unix")]
@ -298,6 +299,7 @@ use std::{
};
pub use dir::CheckedDir;
pub use disable::GLOBAL_DISABLE_VAR;
pub use err::Error;
/// A result type as returned by this crate
@ -337,9 +339,26 @@ pub struct Mistrust {
)]
ignore_prefix: Option<PathBuf>,
/// Are we configured to enable all permission and ownership tests?
#[builder(default, setter(custom))]
dangerously_trust_everyone: bool,
/// Are we configured to disable all permission and ownership tests?
///
/// (This field is present in the builder only.)
#[builder(setter(custom), field(type = "Option<bool>", build = "()"))]
dangerously_trust_everyone: (),
/// Should we check the environment to decide whether to disable permission
/// and ownership tests?
///
/// (This field is present in the builder only.)
#[builder(setter(custom), field(type = "Option<disable::Disable>", build = "()"))]
#[cfg_attr(feature = "serde", builder_field_attr(serde(skip)))]
disable_by_environment: (),
/// Internal value combining `dangerously_trust_everyone` and
/// `disable_by_environment` to decide whether we're doing permissions
/// checks or not.
#[builder(setter(custom), field(build = "self.should_be_enabled()"))]
#[cfg_attr(feature = "serde", builder_field_attr(serde(skip)))]
status: disable::Status,
/// What user ID do we trust by default (if any?)
#[cfg(target_family = "unix")]
@ -412,6 +431,8 @@ impl MistrustBuilder {
/// these checks optional, and still use [`CheckedDir`] without having to
/// implement separate code paths for the "checking on" and "checking off"
/// cases.
///
/// Setting this flag will supersede any value set in the environment.
pub fn dangerously_trust_everyone(&mut self) -> &mut Self {
self.dangerously_trust_everyone = Some(true);
self
@ -423,6 +444,69 @@ impl MistrustBuilder {
self.ignore_prefix = Some(None);
self
}
/// Configure this [`MistrustBuilder`] to become disabled based on the
/// environment variable `var`.
///
/// (If the variable is "false", "no", or "0", it will be treated as
/// false; other values are treated as true.)
///
/// If `var` is not set, then we'll look at
/// `$FS_MISTRUST_DISABLE_PERMISSIONS_CHECKS`.
pub fn controlled_by_env_var(&mut self, var: &str) -> &mut Self {
self.disable_by_environment = Some(disable::Disable::OnUserEnvVar(var.to_string()));
self
}
/// Like `controlled_by_env_var`, but do not override any previously set
/// environment settings.
///
/// (The `arti-client` wants this, so that it can inform a caller-supplied
/// `MistrustBuilder` about its Arti-specific env var, but only if the
/// caller has not already provided a variable of its own. Other code
/// embedding `fs-mistrust` may want it too.)
pub fn controlled_by_env_var_if_not_set(&mut self, var: &str) -> &mut Self {
if self.disable_by_environment.is_none() {
self.controlled_by_env_var(var)
} else {
self
}
}
/// Configure this [`MistrustBuilder`] to become disabled based on the
/// environment variable `$FS_MISTRUST_DISABLE_PERMISSIONS_CHECKS` only,
///
/// (If the variable is "false", "no", "0", or "", it will be treated as
/// false; other values are treated as true.)
///
/// This is the default.
pub fn controlled_by_default_env_var(&mut self) -> &mut Self {
self.disable_by_environment = Some(disable::Disable::OnGlobalEnvVar);
self
}
/// Configure this [`MistrustBuilder`] to never consult the environment to
/// see whether it should be disabled.
pub fn ignore_environment(&mut self) -> &mut Self {
self.disable_by_environment = Some(disable::Disable::Never);
self
}
/// Considering our settings, determine whether we should trust all users
/// (and thereby disable our permission checks.)
fn should_be_enabled(&self) -> disable::Status {
// If we've disabled checks in our configuration, then that settles it.
if self.dangerously_trust_everyone == Some(true) {
return disable::Status::DisableChecks;
}
// Otherwise, we use our "disable_by_environment" setting to see whether
// we should check the environment.
self.disable_by_environment
.as_ref()
.unwrap_or(&disable::Disable::default())
.should_disable_checks()
}
}
impl Default for Mistrust {
@ -529,6 +613,12 @@ impl Mistrust {
pub fn make_directory<P: AsRef<Path>>(&self, dir: P) -> Result<()> {
self.verifier().make_directory(dir)
}
/// Return true if this `Mistrust` object has been configured to trust all
/// users.
pub(crate) fn is_disabled(&self) -> bool {
self.status.disabled()
}
}
impl<'a> Verifier<'a> {