From 79c609e4f1019399c3158f3562e02caa68c89f6d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 8 Aug 2022 09:23:55 -0400 Subject: [PATCH 1/5] arti: Add a feature flag for dns-proxy. It remains on-by-default, so users shouldn't notice a difference, but it may help when we want to save a few bytes of binary size. Closes #532 --- crates/arti/Cargo.toml | 7 ++++--- crates/arti/src/lib.rs | 10 ++++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/crates/arti/Cargo.toml b/crates/arti/Cargo.toml index 8b309c3a2..a71309a7f 100644 --- a/crates/arti/Cargo.toml +++ b/crates/arti/Cargo.toml @@ -12,11 +12,12 @@ categories = ["command-line-utilities", "cryptography"] repository = "https://gitlab.torproject.org/tpo/core/arti.git/" [features] -default = ["tokio", "native-tls"] +default = ["tokio", "native-tls", "dns-proxy"] -full = ["async-std", "tokio", "native-tls", "journald", "arti-client/full"] +full = ["async-std", "tokio", "native-tls", "journald", "arti-client/full", "dns-proxy"] async-std = ["arti-client/async-std", "tor-rtcompat/async-std", "async-ctrlc", "once_cell"] +dns-proxy = ["trust-dns-proto"] tokio = ["tokio-crate", "arti-client/tokio", "tor-rtcompat/tokio"] native-tls = ["arti-client/native-tls", "tor-rtcompat/native-tls"] rustls = ["arti-client/rustls", "tor-rtcompat/rustls"] @@ -57,7 +58,7 @@ tracing = "0.1.18" tracing-appender = "0.2.0" tracing-journald = { version = "0.3.0", optional = true } tracing-subscriber = { version = "0.3.0", features = ["env-filter"] } -trust-dns-proto = "0.21.1" +trust-dns-proto = { version = "0.21.1", optional = true } [dev-dependencies] itertools = "0.10.1" diff --git a/crates/arti/src/lib.rs b/crates/arti/src/lib.rs index d5269f292..68925c0ae 100644 --- a/crates/arti/src/lib.rs +++ b/crates/arti/src/lib.rs @@ -52,6 +52,8 @@ //! (default) //! * `journald` -- Build with support for logging to the `journald` logging //! backend (available as part of systemd.) +//! * `dns-proxy` (default) -- Build with support for proxying certain simple +//! DNS queries over the Tor network. //! //! * `full` -- Build with all features above, along with all stable additive //! features from other arti crates. (This does not include experimental @@ -150,6 +152,7 @@ #![allow(clippy::print_stdout)] pub mod cfg; +#[cfg(feature = "dns-proxy")] pub mod dns; pub mod exit; pub mod logging; @@ -246,6 +249,7 @@ pub async fn run( })); } + #[cfg(feature = "dns-proxy")] if dns_port != 0 { let runtime = runtime.clone(); let client = client.isolated_client(); @@ -255,6 +259,12 @@ pub async fn run( })); } + #[cfg(not(feature = "dns-proxy"))] + if dns_port != 0 { + warn!("Tried to specify a DNS proxy port, but Arti was built without dns-proxy support."); + return Ok(()); + } + if proxy.is_empty() { warn!("No proxy port set; specify -p PORT (for `socks_port`) or -d PORT (for `dns_port`). Alternatively, use the `socks_port` or `dns_port` configuration option."); return Ok(()); From f548a6ac55b5f1feb92ea4ffe5c4882a2e19a739 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 8 Aug 2022 10:14:30 -0400 Subject: [PATCH 2/5] arti: Move most public APIs behind `experimental-api`. The remaining unconditionally public APIs are those related to our configuration objects, and the main_main() API. The rationale for making main_main() public is to have an actual entry point. The rationale for making the config APIs public is: 1. We really do intend for others to be able to read our configuration files using this API. 2. The structure of our configuration files is already part of our interface. Closes #530. --- Cargo.lock | 1 + crates/arti/Cargo.toml | 4 +++- crates/arti/semver.md | 3 +++ crates/arti/src/dns.rs | 3 ++- crates/arti/src/exit.rs | 3 ++- crates/arti/src/lib.rs | 26 ++++++++++++++++++++++---- crates/arti/src/logging.rs | 6 ++++-- crates/arti/src/process.rs | 3 ++- crates/arti/src/socks.rs | 6 ++++-- crates/arti/src/watch_cfg.rs | 3 ++- 10 files changed, 45 insertions(+), 13 deletions(-) create mode 100644 crates/arti/semver.md diff --git a/Cargo.lock b/Cargo.lock index 4e5732a1e..aa8cf2ea0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -112,6 +112,7 @@ dependencies = [ "tracing-journald", "tracing-subscriber", "trust-dns-proto", + "visibility", "winapi 0.3.9", ] diff --git a/crates/arti/Cargo.toml b/crates/arti/Cargo.toml index a71309a7f..3fbbd1077 100644 --- a/crates/arti/Cargo.toml +++ b/crates/arti/Cargo.toml @@ -18,6 +18,7 @@ full = ["async-std", "tokio", "native-tls", "journald", "arti-client/full", "dns async-std = ["arti-client/async-std", "tor-rtcompat/async-std", "async-ctrlc", "once_cell"] dns-proxy = ["trust-dns-proto"] +experimental-api = ["visibility"] tokio = ["tokio-crate", "arti-client/tokio", "tor-rtcompat/tokio"] native-tls = ["arti-client/native-tls", "tor-rtcompat/native-tls"] rustls = ["arti-client/rustls", "tor-rtcompat/rustls"] @@ -31,7 +32,7 @@ accel-openssl = ["arti-client/accel-openssl"] # This feature flag enables experimental features that are not supported. Turning it on may # void your API. -experimental = ["arti-client/experimental"] +experimental = ["arti-client/experimental", "experimental-api"] [dependencies] anyhow = "1.0.23" @@ -59,6 +60,7 @@ tracing-appender = "0.2.0" tracing-journald = { version = "0.3.0", optional = true } tracing-subscriber = { version = "0.3.0", features = ["env-filter"] } trust-dns-proto = { version = "0.21.1", optional = true } +visibility = { version = "0.0.1", optional = true } [dev-dependencies] itertools = "0.10.1" diff --git a/crates/arti/semver.md b/crates/arti/semver.md new file mode 100644 index 000000000..38c55509c --- /dev/null +++ b/crates/arti/semver.md @@ -0,0 +1,3 @@ +BREAKING: Most functions are now only available behind an experimental-api + feature. + diff --git a/crates/arti/src/dns.rs b/crates/arti/src/dns.rs index db426fc4f..e6618ca89 100644 --- a/crates/arti/src/dns.rs +++ b/crates/arti/src/dns.rs @@ -224,7 +224,8 @@ where } /// Launch a DNS resolver to listen on a given local port, and run indefinitely. -pub async fn run_dns_resolver( +#[cfg_attr(feature = "experimental-api", visibility::make(pub))] +pub(crate) async fn run_dns_resolver( runtime: R, tor_client: TorClient, dns_port: u16, diff --git a/crates/arti/src/exit.rs b/crates/arti/src/exit.rs index a75b8ad41..252a21210 100644 --- a/crates/arti/src/exit.rs +++ b/crates/arti/src/exit.rs @@ -8,7 +8,8 @@ use crate::Result; /// This function can have pretty kludgy side-effects: see /// documentation for `tokio::signal::ctrl_c` and `async_ctrlc` for /// caveats. Notably, you can only call this once with async_std. -pub async fn wait_for_ctrl_c() -> Result<()> { +#[cfg_attr(feature = "experimental-api", visibility::make(pub))] +pub(crate) async fn wait_for_ctrl_c() -> Result<()> { #[cfg(feature = "tokio")] { tokio_crate::signal::ctrl_c().await?; diff --git a/crates/arti/src/lib.rs b/crates/arti/src/lib.rs index 68925c0ae..e428263cf 100644 --- a/crates/arti/src/lib.rs +++ b/crates/arti/src/lib.rs @@ -93,6 +93,7 @@ //! versioning guarantees: we might break them or remove them between patch //! versions. //! +//! * `experimental-api` -- build with experimental, unstable API support. //! * `experimental` -- Build with all experimental features above, along with //! all experimental features from other arti crates. //! @@ -152,14 +153,30 @@ #![allow(clippy::print_stdout)] pub mod cfg; -#[cfg(feature = "dns-proxy")] -pub mod dns; -pub mod exit; pub mod logging; + +#[cfg(all(feature = "experimental-api", feature = "dns-proxy"))] +pub mod dns; +#[cfg(feature = "experimental-api")] +pub mod exit; +#[cfg(feature = "experimental-api")] pub mod process; +#[cfg(feature = "experimental-api")] pub mod socks; +#[cfg(feature = "experimental-api")] pub mod watch_cfg; +#[cfg(all(not(feature = "experimental-api"), feature = "dns-proxy"))] +mod dns; +#[cfg(not(feature = "experimental-api"))] +mod exit; +#[cfg(not(feature = "experimental-api"))] +mod process; +#[cfg(not(feature = "experimental-api"))] +mod socks; +#[cfg(not(feature = "experimental-api"))] +mod watch_cfg; + use std::fmt::Write; pub use cfg::{ @@ -219,7 +236,8 @@ fn list_enabled_features() -> &'static [&'static str] { /// # Panics /// /// Currently, might panic if things go badly enough wrong -pub async fn run( +#[cfg_attr(feature = "experimental-api", visibility::make(pub))] +async fn run( runtime: R, socks_port: u16, dns_port: u16, diff --git a/crates/arti/src/logging.rs b/crates/arti/src/logging.rs index 023b7cc22..8c737a0ee 100644 --- a/crates/arti/src/logging.rs +++ b/crates/arti/src/logging.rs @@ -259,7 +259,8 @@ where /// Opaque structure that gets dropped when the program is shutting down, /// after logs are no longer needed. The `Drop` impl flushes buffered messages. -pub struct LogGuards { +#[cfg_attr(feature = "experimental-api", visibility::make(pub))] +pub(crate) struct LogGuards { /// The actual list of guards we're returning. #[allow(unused)] guards: Vec, @@ -273,7 +274,8 @@ pub struct LogGuards { /// /// Note that the returned LogGuard must be dropped precisely when the program /// quits; they're used to ensure that all the log messages are flushed. -pub fn setup_logging( +#[cfg_attr(feature = "experimental-api", visibility::make(pub))] +pub(crate) fn setup_logging( config: &LoggingConfig, mistrust: &Mistrust, cli: Option<&str>, diff --git a/crates/arti/src/process.rs b/crates/arti/src/process.rs index 1c61b9b2a..05fdc1637 100644 --- a/crates/arti/src/process.rs +++ b/crates/arti/src/process.rs @@ -10,7 +10,8 @@ use crate::ArtiConfig; /// # Limitations /// /// This doesn't actually do anything on windows. -pub fn use_max_file_limit(config: &ArtiConfig) { +#[cfg_attr(feature = "experimental-api", visibility::make(pub))] +pub(crate) fn use_max_file_limit(config: &ArtiConfig) { match rlimit::increase_nofile_limit(config.system.max_files) { Ok(n) => tracing::debug!("Increased process file limit to {}", n), Err(e) => tracing::warn!("Error while increasing file limit: {}", e), diff --git a/crates/arti/src/socks.rs b/crates/arti/src/socks.rs index 097cf0da7..d65769088 100644 --- a/crates/arti/src/socks.rs +++ b/crates/arti/src/socks.rs @@ -46,7 +46,8 @@ See StreamPrefs { let mut prefs = StreamPrefs::new(); if addr.parse::().is_ok() { // If they asked for an IPv4 address correctly, nothing else will do. @@ -410,7 +411,8 @@ fn accept_err_is_fatal(err: &IoError) -> bool { /// Requires a `runtime` to use for launching tasks and handling /// timeouts, and a `tor_client` to use in connecting over the Tor /// network. -pub async fn run_socks_proxy( +#[cfg_attr(feature = "experimental-api", visibility::make(pub))] +pub(crate) async fn run_socks_proxy( runtime: R, tor_client: TorClient, socks_port: u16, diff --git a/crates/arti/src/watch_cfg.rs b/crates/arti/src/watch_cfg.rs index aba3f1a06..58ec9eae3 100644 --- a/crates/arti/src/watch_cfg.rs +++ b/crates/arti/src/watch_cfg.rs @@ -21,7 +21,8 @@ const POLL_INTERVAL: Duration = Duration::from_secs(10); /// /// Whenever one or more files in `files` changes, try to reload our /// configuration from them and tell TorClient about it. -pub fn watch_for_config_changes( +#[cfg_attr(feature = "experimental-api", visibility::make(pub))] +pub(crate) fn watch_for_config_changes( sources: ConfigurationSources, original: ArtiConfig, client: TorClient, From 3287c30f1c0766e70ca15fea9bece30887ea77d0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 8 Aug 2022 10:29:16 -0400 Subject: [PATCH 3/5] arti: `main_main` takes command-line arguments does not call exit() --- crates/arti/semver.md | 1 + crates/arti/src/lib.rs | 21 ++++++++++++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/crates/arti/semver.md b/crates/arti/semver.md index 38c55509c..8614f1725 100644 --- a/crates/arti/semver.md +++ b/crates/arti/semver.md @@ -1,3 +1,4 @@ BREAKING: Most functions are now only available behind an experimental-api feature. +BREAKING: main_main() now takes a set of command-line arguments. diff --git a/crates/arti/src/lib.rs b/crates/arti/src/lib.rs index e428263cf..77e9ee407 100644 --- a/crates/arti/src/lib.rs +++ b/crates/arti/src/lib.rs @@ -308,7 +308,11 @@ async fn run( /// # Panics /// /// Currently, might panic if wrong arguments are specified. -pub fn main_main() -> Result<()> { +pub fn main_main(cli_args: I) -> Result<()> +where + I: IntoIterator, + T: Into + Clone, +{ // We describe a default here, rather than using `default()`, because the // correct behavior is different depending on whether the filename is given // explicitly or not. @@ -424,7 +428,7 @@ pub fn main_main() -> Result<()> { .finish(); let pre_config_logging = tracing::Dispatch::new(pre_config_logging); let pre_config_logging_ret = tracing::dispatcher::with_default(&pre_config_logging, || { - let matches = clap_app.get_matches(); + let matches = clap_app.get_matches_from_safe(cli_args)?; let fs_mistrust_disabled = matches.is_present("disable-fs-permission-checks"); @@ -513,6 +517,17 @@ pub fn main_main() -> Result<()> { } /// Main program, callable directly from a binary crate's `main` +/// +/// This function behaves the same as `main_main()`, except: +/// * It takes command-line arguments from `std::env::args_os` rather than +/// from an argument. +/// * It exits the process with an appropriate error code on error. pub fn main() { - main_main().unwrap_or_else(|e| with_safe_logging_suppressed(|| tor_error::report_and_exit(e))); + match main_main(std::env::args_os()) { + Ok(()) => {} + Err(e) => match e.downcast_ref::() { + Some(clap_err) => clap_err.exit(), + None => with_safe_logging_suppressed(|| tor_error::report_and_exit(e)), + }, + } } From 32a78651d294aa096506f85ae9ddba0fdd14b5d4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 8 Aug 2022 10:40:32 -0400 Subject: [PATCH 4/5] Document more explicitly what "voiding a semver warranty" entails Closes #522. --- crates/arti-client/src/lib.rs | 6 +++++- crates/arti/src/lib.rs | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/crates/arti-client/src/lib.rs b/crates/arti-client/src/lib.rs index 1639acda8..d0810dc54 100644 --- a/crates/arti-client/src/lib.rs +++ b/crates/arti-client/src/lib.rs @@ -210,7 +210,7 @@ //! ## Experimental and unstable features //! //! Note that the APIs enabled by these features are NOT covered by semantic -//! versioning guarantees: we might break them or remove them between patch +//! versioning[^1] guarantees: we might break them or remove them between patch //! versions. //! //! * `experimental-api` -- build with experimental, unstable API support. @@ -220,6 +220,10 @@ //! //! * `experimental` -- Build with all experimental features above, along with //! all experimental features from other arti crates. +//! +//! [^1]: Remember, semantic versioning is what makes various `cargo` features +//! work reliably. To be explicit: if you want `cargo update` to _only_ make safe +//! changes, then you cannot enable these features. // @@ begin lint list maintained by maint/add_warning @@ #![cfg_attr(not(ci_arti_stable), allow(renamed_and_removed_lints))] diff --git a/crates/arti/src/lib.rs b/crates/arti/src/lib.rs index 77e9ee407..120260607 100644 --- a/crates/arti/src/lib.rs +++ b/crates/arti/src/lib.rs @@ -90,13 +90,17 @@ //! ## Experimental features //! //! Note that the APIs enabled by these features are NOT covered by semantic -//! versioning guarantees: we might break them or remove them between patch +//! versioning[^1] guarantees: we might break them or remove them between patch //! versions. //! //! * `experimental-api` -- build with experimental, unstable API support. //! * `experimental` -- Build with all experimental features above, along with //! all experimental features from other arti crates. //! +//! [^1]: Remember, semantic versioning is what makes various `cargo` features +//! work reliably. To be explicit, if you want `cargo update` to _only_ make +//! correct changes, then you cannot enable these features. +//! //! # Limitations //! //! There are many missing features. Among them: there's no onion service From 7d7cdcd749fff4c3e16c115119215b32c003d8b4 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 10 Aug 2022 11:35:22 -0400 Subject: [PATCH 5/5] Add a few dire warnings about main; make main_main experimental. --- crates/arti/src/lib.rs | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/crates/arti/src/lib.rs b/crates/arti/src/lib.rs index 120260607..42205b4c9 100644 --- a/crates/arti/src/lib.rs +++ b/crates/arti/src/lib.rs @@ -94,6 +94,8 @@ //! versions. //! //! * `experimental-api` -- build with experimental, unstable API support. +//! (Right now, most APIs in the `arti` crate are experimental, since this +//! crate was originally written to run as a binary only.) //! * `experimental` -- Build with all experimental features above, along with //! all experimental features from other arti crates. //! @@ -307,12 +309,20 @@ async fn run( ) } -/// Inner function to allow convenient error handling +/// Inner function, to handle a set of CLI arguments and return a single +/// `Result<()>` for convenient handling. +/// +/// # ⚠️ Warning! ⚠️ +/// +/// If your program needs to call this function, you are setting yourself up for +/// some serious maintenance headaches. See discussion on [`main`] and please +/// reach out to help us build you a better API. /// /// # Panics /// /// Currently, might panic if wrong arguments are specified. -pub fn main_main(cli_args: I) -> Result<()> +#[cfg_attr(feature = "experimental-api", visibility::make(pub))] +fn main_main(cli_args: I) -> Result<()> where I: IntoIterator, T: Into + Clone, @@ -526,6 +536,25 @@ where /// * It takes command-line arguments from `std::env::args_os` rather than /// from an argument. /// * It exits the process with an appropriate error code on error. +/// +/// # ⚠️ Warning ⚠️ +/// +/// Calling this function, or the related experimental function `main_main`, is +/// probably a bad idea for your code. It means that you are invoking Arti as +/// if from the command line, but keeping it embedded inside your process. Doing +/// this will block your process take over handling for several signal types, +/// possibly disable debugger attachment, and a lot more junk that a library +/// really has no business doing for you. It is not designed to run in this +/// way, and may give you strange results. +/// +/// If the functionality you want is available in [`arti_client`] crate, or from +/// a *non*-experimental API in this crate, it would be better for you to use +/// that API instead. +/// +/// Alternatively, if you _do_ need some underlying function from the `arti` +/// crate, it would be better for all of us if you had a stable interface to that +/// function. Please reach out to the Arti developers, so we can work together +/// to get you the stable API you need. pub fn main() { match main_main(std::env::args_os()) { Ok(()) => {}