Merge branch 'less_arti_surface' into 'main'

Reduce the arti crate's API surface; improve semver documentation.

Closes #522, #530, and #532

See merge request tpo/core/arti!664

Cargo.lock

@@ -112,6 +112,7 @@ dependencies = [
  "tracing-journald",
  "tracing-subscriber",
  "trust-dns-proto",
+ "visibility",
  "winapi 0.3.9",
 ]
 

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))]

crates/arti/Cargo.toml

@@ -12,11 +12,13 @@ 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"]
+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"]
@@ -30,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"
@@ -57,7 +59,8 @@ 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 }
+visibility = { version = "0.0.1", optional = true }
 
 [dev-dependencies]
 itertools = "0.10.1"

crates/arti/semver.md

@@ -0,0 +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.
+

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<R: Runtime>(
+#[cfg_attr(feature = "experimental-api", visibility::make(pub))]
+pub(crate) async fn run_dns_resolver<R: Runtime>(
     runtime: R,
     tor_client: TorClient<R>,
     dns_port: u16,

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?;

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
@@ -88,12 +90,19 @@
 //! ## 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.
+//!    (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.
 //!
+//! [^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
@@ -150,13 +159,30 @@
 #![allow(clippy::print_stdout)]
 
 pub mod cfg;
-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::{
@@ -216,7 +242,8 @@ fn list_enabled_features() -> &'static [&'static str] {
 /// # Panics
 ///
 /// Currently, might panic if things go badly enough wrong
-pub async fn run<R: Runtime>(
+#[cfg_attr(feature = "experimental-api", visibility::make(pub))]
+async fn run<R: Runtime>(
     runtime: R,
     socks_port: u16,
     dns_port: u16,
@@ -246,6 +273,7 @@ pub async fn run<R: Runtime>(
         }));
     }
 
+    #[cfg(feature = "dns-proxy")]
     if dns_port != 0 {
         let runtime = runtime.clone();
         let client = client.isolated_client();
@@ -255,6 +283,12 @@ pub async fn run<R: Runtime>(
         }));
     }
 
+    #[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(());
@@ -275,12 +309,24 @@ pub async fn run<R: Runtime>(
     )
 }
 
-/// 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() -> Result<()> {
+#[cfg_attr(feature = "experimental-api", visibility::make(pub))]
+fn main_main<I, T>(cli_args: I) -> Result<()>
+where
+    I: IntoIterator<Item = T>,
+    T: Into<std::ffi::OsString> + 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.
@@ -396,7 +442,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");
 
@@ -485,6 +531,36 @@ 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.
+///
+/// # ⚠️ 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() {
-    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::<clap::Error>() {
+            Some(clap_err) => clap_err.exit(),
+            None => with_safe_logging_suppressed(|| tor_error::report_and_exit(e)),
+        },
+    }
 }

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<WorkerGuard>,
@@ -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>,

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),

crates/arti/src/socks.rs

@@ -46,7 +46,8 @@ See <a href="https://gitlab.torproject.org/tpo/core/arti/#todo-need-to-change-wh
 
 /// Find out which kind of address family we can/should use for a
 /// given `SocksRequest`.
-pub fn stream_preference(req: &SocksRequest, addr: &str) -> StreamPrefs {
+#[cfg_attr(feature = "experimental-api", visibility::make(pub))]
+fn stream_preference(req: &SocksRequest, addr: &str) -> StreamPrefs {
     let mut prefs = StreamPrefs::new();
     if addr.parse::<Ipv4Addr>().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<R: Runtime>(
+#[cfg_attr(feature = "experimental-api", visibility::make(pub))]
+pub(crate) async fn run_socks_proxy<R: Runtime>(
     runtime: R,
     tor_client: TorClient<R>,
     socks_port: u16,

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<R: Runtime>(
+#[cfg_attr(feature = "experimental-api", visibility::make(pub))]
+pub(crate) fn watch_for_config_changes<R: Runtime>(
     sources: ConfigurationSources,
     original: ArtiConfig,
     client: TorClient<R>,