From 4fa0122dde2b664ebd10bfbe46da54c396dffdb3 Mon Sep 17 00:00:00 2001 From: eta Date: Wed, 27 Oct 2021 17:40:00 +0100 Subject: [PATCH] Improve and future-proof the `arti` CLI This switches out `arti`'s argument-parsing library with `clap`, which is a lot more featureful (and very widely used within the Rust ecosystem). We also now use a lot of `clap`'s features to improve the CLI experience: - The CLI now expects a subcommand (currently, either "help", or "proxy" for the existing SOCKS proxy behaviour). This should let us add additional non-SOCKS-proxy features to arti in future. - `clap` supports default values determined at runtime, so the way the default config file is loaded was changed: now, we determine the OS-specific path for said file before invoking `clap`, so the help command can show it properly. - The behaviour of `tor_config` was also changed; now, one simply specifies a list of configuration files to load, together with whether they're required. - That function also way overused generics; this has been fixed. - Instead of using the ARTI_LOG environment variable to configure logging, one now uses the `-l, --log-level` CLI option. (The intent is for this option to be more discoverable by users.) - The `proxy` subcommand allows the user to override the SOCKS port used on the CLI without editing the config file. --- .gitlab-ci.yml | 2 +- CONTRIBUTING.md | 2 +- Cargo.lock | 106 +++++++++------- README.md | 2 +- crates/arti/Cargo.toml | 2 +- crates/arti/src/arti_defaults.toml | 2 +- crates/arti/src/main.rs | 195 +++++++++++++++++++---------- crates/tor-config/src/lib.rs | 76 ++++------- 8 files changed, 220 insertions(+), 167 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 5ddc4e723..b2865152d 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -78,7 +78,7 @@ integration: - ./chutney/chutney start chutney/networks/basic - CHUTNEY_START_TIME=180 ./chutney/chutney wait_for_bootstrap chutney/networks/basic - ./chutney/chutney verify chutney/networks/basic - - ./target/x86_64-unknown-linux-gnu/debug/arti -f chutney/net/nodes/arti.toml & sleep 5 + - ./target/x86_64-unknown-linux-gnu/debug/arti proxy -c chutney/net/nodes/arti.toml & sleep 5 - curl http://example.com -vs --socks5-hostname 127.0.0.1:9150 -o /dev/null - kill -s INT %%; wait - ./chutney/chutney stop chutney/networks/basic diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 69509de5a..543fffdec 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -101,7 +101,7 @@ To do so, we will launch arti independently from Tor Browser. Build arti with `cargo build --release`. After that launch it with some basic configuration parameters: - $ ./target/release/arti -c "socks_port = 9150" -c "logging.trace_filter = 'debug'" + $ ./target/release/arti proxy -l debug -p 9150 This will ensure that arti sets its SOCKS port on 9150. Now we need to launch Tor Browser and instruct it to use that SOCKS port. diff --git a/Cargo.lock b/Cargo.lock index cd9b286d6..e2144a039 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -32,6 +32,15 @@ dependencies = [ "version_check", ] +[[package]] +name = "ansi_term" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ee49baf6cb617b853aa8d93bf420db2383fab46d314482ca2803b40d5fde979b" +dependencies = [ + "winapi", +] + [[package]] name = "ansi_term" version = "0.12.1" @@ -47,35 +56,6 @@ version = "1.0.44" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "61604a8f862e1d5c3229fdd78f8b02c68dcf73a4c4b05fd636d12240aaa242c1" -[[package]] -name = "argh" -version = "0.1.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f023c76cd7975f9969f8e29f0e461decbdc7f51048ce43427107a3d192f1c9bf" -dependencies = [ - "argh_derive", - "argh_shared", -] - -[[package]] -name = "argh_derive" -version = "0.1.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "48ad219abc0c06ca788aface2e3a1970587e3413ab70acd20e54b6ec524c1f8f" -dependencies = [ - "argh_shared", - "heck", - "proc-macro2", - "quote", - "syn", -] - -[[package]] -name = "argh_shared" -version = "0.1.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "38de00daab4eac7d753e97697066238d67ce9d7e2d823ab4f72fe14af29f3f33" - [[package]] name = "arrayref" version = "0.3.6" @@ -93,9 +73,9 @@ name = "arti" version = "0.0.0" dependencies = [ "anyhow", - "argh", "arti-client", "async-ctrlc", + "clap", "config", "futures", "libc", @@ -357,6 +337,17 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "065374052e7df7ee4047b1160cca5e1467a12351a40b3da123c870ba0b8eda2a" +[[package]] +name = "atty" +version = "0.2.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d9b39be18770d11421cdb1b9947a45dd3f37e93092cbf377614828a319d5fee8" +dependencies = [ + "hermit-abi", + "libc", + "winapi", +] + [[package]] name = "autocfg" version = "0.1.7" @@ -492,6 +483,21 @@ dependencies = [ "generic-array", ] +[[package]] +name = "clap" +version = "2.33.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "37e58ac78573c40708d45522f0d80fa2f01cc4f9b4e2bf749807255454312002" +dependencies = [ + "ansi_term 0.11.0", + "atty", + "bitflags", + "strsim 0.8.0", + "textwrap", + "unicode-width", + "vec_map", +] + [[package]] name = "concurrent-queue" version = "1.2.2" @@ -662,7 +668,7 @@ dependencies = [ "ident_case", "proc-macro2", "quote", - "strsim", + "strsim 0.10.0", "syn", ] @@ -1105,15 +1111,6 @@ dependencies = [ "hashbrown", ] -[[package]] -name = "heck" -version = "0.3.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6d621efb26863f0e9924c6ac577e8275e5e6b77455db64ffa6c65c904e9e132c" -dependencies = [ - "unicode-segmentation", -] - [[package]] name = "hermit-abi" version = "0.1.19" @@ -2299,6 +2296,12 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a2eb9349b6444b326872e140eb1cf5e7c522154d69e7a0ffb0fb81c06b37543f" +[[package]] +name = "strsim" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8ea5119cdb4c55b55d432abb513a0429384878c15dde60cc77b1c99de1a95a6a" + [[package]] name = "strsim" version = "0.10.0" @@ -2348,6 +2351,15 @@ dependencies = [ "winapi", ] +[[package]] +name = "textwrap" +version = "0.11.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d326610f408c7a4eb6f51c37c330e496b08506c9457c9d34287ecc38809fb060" +dependencies = [ + "unicode-width", +] + [[package]] name = "thiserror" version = "1.0.29" @@ -2945,7 +2957,7 @@ version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5cf865b5ddc38e503a29c41c4843e616a73028ae18c637bc3eb2afaef4909c84" dependencies = [ - "ansi_term", + "ansi_term 0.12.1", "lazy_static", "matchers", "regex", @@ -2991,10 +3003,10 @@ dependencies = [ ] [[package]] -name = "unicode-segmentation" -version = "1.8.0" +name = "unicode-width" +version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8895849a949e7845e06bd6dc1aa51731a103c42707010a5b591c0038fb73385b" +checksum = "3ed742d4ea2bd1176e236172c8429aaf54486e7ac098db29ffe6529e0ce50973" [[package]] name = "unicode-xid" @@ -3030,6 +3042,12 @@ version = "0.2.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "accd4ea62f7bb7a82fe23066fb0957d48ef677f6eeb8215f372f52e48bb32426" +[[package]] +name = "vec_map" +version = "0.8.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f1bddf1187be692e79c5ffeab891132dfb0f236ed36a43c7ed39f1165ee20191" + [[package]] name = "version_check" version = "0.9.3" diff --git a/README.md b/README.md index e038a218f..359b1f556 100644 --- a/README.md +++ b/README.md @@ -68,7 +68,7 @@ download more than one directory per run. To try it out, run the demo program in `arti` as follows. It will open a SOCKS proxy on port 9150. - % cargo run --release + % cargo run --release -- proxy Again, do not use this program yet if you seriously need anonymity, privacy, security, or stability. diff --git a/crates/arti/Cargo.toml b/crates/arti/Cargo.toml index 6c46acc4b..c4597f5d4 100644 --- a/crates/arti/Cargo.toml +++ b/crates/arti/Cargo.toml @@ -33,7 +33,7 @@ rlimit = "0.6.2" serde = { version = "1.0.124", features = ["derive"] } tracing-subscriber = { version = "0.3.0", features = ["env-filter"] } tokio-crate = { package="tokio", version = "1.7.0", optional = true, features = ["signal"] } -argh = "0.1.6" +clap = "2.33" tracing-journald = { version = "0.2.0", optional = true } [target.'cfg(unix)'.dependencies] diff --git a/crates/arti/src/arti_defaults.toml b/crates/arti/src/arti_defaults.toml index dc1b82eca..25836f24d 100644 --- a/crates/arti/src/arti_defaults.toml +++ b/crates/arti/src/arti_defaults.toml @@ -17,7 +17,7 @@ journald = false # It can be as simple as a single loglevel, or as complicated as a # list with per-module settings. # -# You can override this setting with the ARTI_LOG environment variable. +# You can override this setting with the -l, --log-level command-line option. # # Example: # trace_filter = "info,tor_proto::channel=trace" diff --git a/crates/arti/src/main.rs b/crates/arti/src/main.rs index 82493192b..d90c3e192 100644 --- a/crates/arti/src/main.rs +++ b/crates/arti/src/main.rs @@ -14,16 +14,15 @@ //! It will listen on port 9150 by default, but you can override this in //! the configuration. //! -//! # Command-line arguments +//! # Command-line interface //! //! (This is not stable; future versions will break this.) //! -//! `-f ` overrides the location to search for a -//! configuration file to the list of configuration file. You can use -//! this multiple times: All files will be loaded and merged. +//! `arti` uses the [`clap`](https://docs.rs/clap/) crate for command-line +//! argument parsing; run `arti help` to get it to print its documentation. //! -//! `-c =` sets a configuration option to be applied after all -//! configuration files are loaded. +//! The only currently implemented subcommand is `arti proxy`; try +//! `arti help proxy` for a list of options you can pass to it. //! //! # Configuration //! @@ -99,28 +98,15 @@ use tor_config::CfgPath; use tor_rtcompat::{Runtime, SpawnBlocking}; use anyhow::Result; -use argh::FromArgs; +use clap::{App, AppSettings, Arg, SubCommand}; use serde::Deserialize; use std::collections::HashMap; +use std::path::PathBuf; use tracing::{info, warn}; use tracing_subscriber::layer::SubscriberExt; use tracing_subscriber::prelude::*; use tracing_subscriber::{fmt, registry, EnvFilter}; -#[derive(FromArgs, Debug, Clone)] -/// Connect to the Tor network, open a SOCKS port, and proxy -/// traffic. -/// -/// This is a demo; you get no stability guarantee. -struct Args { - /// override the default location(s) for the configuration file - #[argh(option, short = 'f')] - rc: Vec, - /// override a configuration option (uses toml syntax) - #[argh(option, short = 'c')] - cfg: Vec, -} - /// Default options to use for our configuration. const ARTI_DEFAULTS: &str = concat!(include_str!("./arti_defaults.toml"),); @@ -131,7 +117,7 @@ struct LoggingConfig { /// Filtering directives that determine tracing levels as described at /// /// - /// You can override this setting with the environment variable ARTI_LOG. + /// You can override this setting with the -l, --log-level command line parameter. /// /// Example: "info,tor_proto::channel=trace" trace_filter: String, @@ -254,19 +240,15 @@ fn filt_from_str_verbose(s: &str, source: &str) -> EnvFilter { } /// Set up logging -fn setup_logging(config: &ArtiConfig) { - use std::env; - - let env_filter = match env::var("ARTI_LOG") - .as_ref() - .map(|s| filt_from_str_verbose(s, "ARTI_LOG environment variable")) - { - Ok(f) => f, - Err(_) => filt_from_str_verbose( - config.logging.trace_filter.as_str(), - "trace_filter configuration option", - ), - }; +fn setup_logging(config: &ArtiConfig, cli: Option<&str>) { + let env_filter = + match cli.map(|s| filt_from_str_verbose(s, "--log-level command line parameter")) { + Some(f) => f, + None => filt_from_str_verbose( + config.logging.trace_filter.as_str(), + "trace_filter configuration option", + ), + }; let registry = registry().with(fmt::Layer::default()).with(env_filter); @@ -284,50 +266,135 @@ fn setup_logging(config: &ArtiConfig) { } fn main() -> Result<()> { - let args: Args = argh::from_env(); - let dflt_config = tor_config::default_config_file(); + let dflt_config = tor_config::default_config_file().unwrap_or_else(|| "./config.toml".into()); + + let matches = + App::new("Arti") + .version(env!("CARGO_PKG_VERSION")) + .author("The Tor Project Developers") + .about("A Rust Tor implementation.") + // HACK(eta): clap generates "arti [OPTIONS] " for this usage string by + // default, but then fails to parse options properly if you do put them + // before the subcommand. + // We just declare all options as `global` and then require them to be + // put after the subcommand, hence this new usage string. + .usage("arti [OPTIONS]") + .arg( + Arg::with_name("config-files") + .short("c") + .long("config") + .takes_value(true) + .value_name("FILE") + .default_value_os(dflt_config.as_ref()) + .multiple(true) + // NOTE: don't forget the `global` flag on all arguments declared at this level! + .global(true) + .help("Specify which config file(s) to read."), + ) + .arg( + Arg::with_name("option") + .short("o") + .takes_value(true) + .value_name("KEY=VALUE") + .multiple(true) + .global(true) + .help("Override config file parameters, using TOML-like syntax."), + ) + .arg( + Arg::with_name("loglevel") + .short("l") + .long("log-level") + .global(true) + .takes_value(true) + .value_name("LEVEL") + .help("Override the log level (usually one of 'trace', 'debug', 'info', 'warn', 'error')."), + ) + .subcommand( + SubCommand::with_name("proxy") + .about( + "Run Arti in SOCKS proxy mode, proxying connections through the Tor network.", + ) + .arg( + Arg::with_name("socks-port") + .short("p") + .takes_value(true) + .value_name("PORT") + .help("Port to listen on for SOCKS connections (overrides the port in the config if specified).") + ) + ) + .setting(AppSettings::SubcommandRequiredElseHelp) + .get_matches(); let mut cfg = config::Config::new(); cfg.merge(config::File::from_str( ARTI_DEFAULTS, config::FileFormat::Toml, ))?; - tor_config::load(&mut cfg, &dflt_config, &args.rc, &args.cfg)?; + + let config_files = matches + .values_of_os("config-files") + // This shouldn't actually be possible given we specify a default. + .expect("no config files provided") + .into_iter() + // The second value in this 2-tuple specifies whether the config file is "required" (as in, + // failure to load it is an error). All config files that aren't the default are required. + .map(|x| (PathBuf::from(x), x != dflt_config)) + .collect::>(); + + let additional_opts = matches + .values_of("option") + .map(|x| x.into_iter().map(ToOwned::to_owned).collect::>()) + .unwrap_or_else(Vec::new); + + tor_config::load(&mut cfg, &config_files, additional_opts)?; let config: ArtiConfig = cfg.try_into()?; - setup_logging(&config); + setup_logging(&config, matches.value_of("loglevel")); - let statecfg = config.storage.state_dir.path()?; - let dircfg = config.get_dir_config()?; - let circcfg = config.get_circ_config()?; - let addrcfg = config.addr_config; + if let Some(proxy_matches) = matches.subcommand_matches("proxy") { + let statecfg = config.storage.state_dir.path()?; + let dircfg = config.get_dir_config()?; + let circcfg = config.get_circ_config()?; + let addrcfg = config.addr_config; - let socks_port = match config.socks_port { - Some(s) => s, - None => { - info!("Nothing to do: no socks_port configured."); - return Ok(()); - } - }; + let socks_port = match (proxy_matches.value_of("socks-port"), config.socks_port) { + (Some(p), _) => p.parse().expect("Invalid port specified"), + (None, Some(s)) => s, + (None, None) => { + warn!( + "No SOCKS port set; specify -p PORT or use the `socks_port` configuration option." + ); + return Ok(()); + } + }; - let client_config = TorClientConfig::builder() - .state_cfg(statecfg) - .dir_cfg(dircfg) - .circ_cfg(circcfg) - .addr_cfg(addrcfg) - .build()?; + info!( + "Starting Arti {} in SOCKS proxy mode on port {}...", + env!("CARGO_PKG_VERSION"), + socks_port + ); - process::use_max_file_limit(); + let client_config = TorClientConfig::builder() + .state_cfg(statecfg) + .dir_cfg(dircfg) + .circ_cfg(circcfg) + .addr_cfg(addrcfg) + .build()?; - #[cfg(feature = "tokio")] - let runtime = tor_rtcompat::tokio::create_runtime()?; - #[cfg(all(feature = "async-std", not(feature = "tokio")))] - let runtime = tor_rtcompat::async_std::create_runtime()?; + process::use_max_file_limit(); - let rt_copy = runtime.clone(); - rt_copy.block_on(run(runtime, socks_port, client_config))?; - Ok(()) + #[cfg(feature = "tokio")] + let runtime = tor_rtcompat::tokio::create_runtime()?; + #[cfg(all(feature = "async-std", not(feature = "tokio")))] + let runtime = tor_rtcompat::async_std::create_runtime()?; + + let rt_copy = runtime.clone(); + rt_copy.block_on(run(runtime, socks_port, client_config))?; + Ok(()) + } else { + panic!("Subcommand added to clap subcommand list, but not yet implemented") + } } #[cfg(test)] diff --git a/crates/tor-config/src/lib.rs b/crates/tor-config/src/lib.rs index bed9956c9..f27c17537 100644 --- a/crates/tor-config/src/lib.rs +++ b/crates/tor-config/src/lib.rs @@ -45,53 +45,28 @@ pub use path::CfgPath; use std::path::{Path, PathBuf}; -/// Load a Config object based on a set of files and/or command-line -/// arguments. +/// Load configuration into an existing `Config` object from configuration files on disk (`files`), +/// and/or command-line arguments (`opts`). /// -/// The files should be toml; the command-line arguments have the extended -/// syntax of [`CmdLine`]. +/// `files` should be a list of TOML config file paths to read, with the boolean specifying whether +/// a failure to read this file should be an error or not. /// -/// If `default_path` is present, and there is no list of files, then use a -/// default file if it exists. -// -// XXXX Add an error type for this crate. -pub fn load<'a, P1, C1, P2, C2>( +/// `opts` are passed into a [`CmdLine`], and use the extended syntax of that mechanism. +pub fn load>( cfg: &mut config::Config, - default_path: &Option, - files: C1, - opts: C2, -) -> Result<(), config::ConfigError> -where - P1: AsRef + 'a, - C1: IntoIterator, - P2: AsRef + 'a, - C2: IntoIterator, - C2::Item: AsRef, -{ - let mut search_path = Vec::new(); - for f in files { - search_path.push(f.as_ref()); - } - let mut missing_ok = false; - if search_path.is_empty() { - if let Some(f) = default_path { - // XXXX shouldn't be println, but no logs exist yet. - println!("looking for default configuration in {:?}", f.as_ref()); - search_path.push(f.as_ref()); - missing_ok = true; - } - } - - for p in search_path { + files: &[(P, bool)], + opts: Vec, +) -> Result<(), config::ConfigError> { + for (path, required) in files { // Not going to use File::with_name here, since it doesn't // quite do what we want. - let f: config::File<_> = p.into(); - cfg.merge(f.format(config::FileFormat::Toml).required(!missing_ok))?; + let f: config::File<_> = path.as_ref().into(); + cfg.merge(f.format(config::FileFormat::Toml).required(*required))?; } let mut cmdline = CmdLine::new(); for opt in opts { - cmdline.push_toml_line(opt.as_ref().to_string()); + cmdline.push_toml_line(opt); } cfg.merge(cmdline)?; @@ -116,16 +91,12 @@ friends = 4242 "; #[test] - fn load_default() { + fn non_required_file() { let td = tempdir().unwrap(); let dflt = td.path().join("a_file"); + let files = vec![(dflt, false)]; let mut c = config::Config::new(); - let v: Vec<&'static str> = Vec::new(); - std::fs::write(&dflt, EX_TOML).unwrap(); - load(&mut c, &Some(dflt), &v, &v).unwrap(); - - assert_eq!(c.get_str("hello.friends").unwrap(), "4242".to_string()); - assert_eq!(c.get_str("hello.world").unwrap(), "stuff".to_string()); + load(&mut c, &files, Default::default()).unwrap(); } static EX2_TOML: &str = " @@ -134,16 +105,14 @@ world = \"nonsense\" "; #[test] - fn load_one_file() { + fn both_required_and_not() { let td = tempdir().unwrap(); let dflt = td.path().join("a_file"); let cf = td.path().join("other_file"); let mut c = config::Config::new(); - std::fs::write(&dflt, EX_TOML).unwrap(); std::fs::write(&cf, EX2_TOML).unwrap(); - let v = vec![cf]; - let v2: Vec<&'static str> = Vec::new(); - load(&mut c, &Some(dflt), &v, &v2).unwrap(); + let files = vec![(dflt, false), (cf, true)]; + load(&mut c, &files, Default::default()).unwrap(); assert!(c.get_str("hello.friends").is_err()); assert_eq!(c.get_str("hello.world").unwrap(), "nonsense".to_string()); @@ -157,10 +126,9 @@ world = \"nonsense\" let mut c = config::Config::new(); std::fs::write(&cf1, EX_TOML).unwrap(); std::fs::write(&cf2, EX2_TOML).unwrap(); - let v = vec![cf1, cf2]; - let v2 = vec!["other.var=present"]; - let d: Option = None; - load(&mut c, &d, &v, &v2).unwrap(); + let v = vec![(cf1, true), (cf2, true)]; + let v2 = vec!["other.var=present".to_string()]; + load(&mut c, &v, v2).unwrap(); assert_eq!(c.get_str("hello.friends").unwrap(), "4242".to_string()); assert_eq!(c.get_str("hello.world").unwrap(), "nonsense".to_string());