From 9a729aa2f8364fb736b3e998f51480c6f5a78d02 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 24 Feb 2022 18:48:52 +0000 Subject: [PATCH 01/16] arti-hyper: Disable clippy::clone_on_ref_ptr As per https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/355#note_2781816 disable this here for now, pending a decision on !352 --- crates/arti-hyper/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/arti-hyper/src/lib.rs b/crates/arti-hyper/src/lib.rs index 6559e1682..b5edf58db 100644 --- a/crates/arti-hyper/src/lib.rs +++ b/crates/arti-hyper/src/lib.rs @@ -11,7 +11,6 @@ #![deny(clippy::cargo_common_metadata)] #![deny(clippy::cast_lossless)] #![deny(clippy::checked_conversions)] -#![warn(clippy::clone_on_ref_ptr)] #![warn(clippy::cognitive_complexity)] #![deny(clippy::debug_assert_with_mut_call)] #![deny(clippy::exhaustive_enums)] From c08a3170937c64d8cf699584867e5fe3fcc51fb6 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 24 Feb 2022 18:10:08 +0000 Subject: [PATCH 02/16] Introduce ErrorKind::OtherRemote arti-hyper wants to be able to have a kind for TLS failure. Given that arti-hyper is above arti-client, this shows that callers above arti-client might need to invent kinds for their own errors. Possibly this means we need other Other errors for other locations. If we have pluggable components we might even want OtherTorError. --- crates/tor-error/src/lib.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/tor-error/src/lib.rs b/crates/tor-error/src/lib.rs index 51f9bcd63..b1c6ce6a0 100644 --- a/crates/tor-error/src/lib.rs +++ b/crates/tor-error/src/lib.rs @@ -424,6 +424,15 @@ pub enum ErrorKind { #[display(fmt = "remote hostname lookup failure")] RemoteHostNotFound, + /// Something else went wrong on the far side of the Tor network + /// + /// This might be an error reported by the exit; + /// or when using a higher-layer protocol over a Tor connection, + /// this might be an error reported by the remote host within that higher protocol, + /// or a problem detected locally but relating to that higher protocol. + #[display(fmt = "remote error")] + OtherRemote, + /// An operation failed, and the relay in question reported that it's too /// busy to answer our request. #[display(fmt = "relay too busy")] From 148cd61f2bee21bcd9174903634a51593bb6e4f7 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Thu, 24 Feb 2022 17:30:56 +0000 Subject: [PATCH 03/16] Manually implement Clone for ArtiHttpConnector This will be needed in a moment and doing it now makes the next patch smaller and hence easier to read. --- crates/arti-hyper/src/lib.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/arti-hyper/src/lib.rs b/crates/arti-hyper/src/lib.rs index b5edf58db..43f7c5d33 100644 --- a/crates/arti-hyper/src/lib.rs +++ b/crates/arti-hyper/src/lib.rs @@ -90,12 +90,18 @@ impl tor_error::HasKind for ConnectionError { /// A `hyper` connector to proxy HTTP connections via the Tor network, using Arti. /// /// Only supports plaintext HTTP for now. -#[derive(Clone)] pub struct ArtiHttpConnector { /// The client client: TorClient, } +impl Clone for ArtiHttpConnector { + fn clone(&self) -> Self { + let client = self.client.clone(); + Self { client } + } +} + impl ArtiHttpConnector { /// Make a new `ArtiHttpConnector` using an Arti `TorClient` object. pub fn new(client: TorClient) -> Self { From c397c772e64df4877e4bedd4fbdaa77dddc85541 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Fri, 25 Feb 2022 11:24:19 +0000 Subject: [PATCH 04/16] arti-hyper: Provide TLS connector and make space for TLS stream Add tls_conn field to ArtiHttpConnector (and argument to constructor). Introduce MaybeHttpsStream and use it in ArtiHttpConnection. Have the example program pass the native TLS connector. Currently the TLS connector and the HTTPS variant are not used, but this commit is very noisy and fomrulaic, so I have split out the code to use them into a separate commit for easier preparation and review. --- Cargo.lock | 207 +++++++++++++++++++++++++++- crates/arti-hyper/Cargo.toml | 2 + crates/arti-hyper/examples/hyper.rs | 5 +- crates/arti-hyper/src/lib.rs | 76 +++++++--- 4 files changed, 264 insertions(+), 26 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 65aebaac1..6c425cca7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -41,6 +41,15 @@ dependencies = [ "version_check", ] +[[package]] +name = "aho-corasick" +version = "0.7.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1e37cfd5e7657ada45f742d6e99ca5788580b5c529dc78faf11ece6dc702656f" +dependencies = [ + "memchr", +] + [[package]] name = "ansi_term" version = "0.12.1" @@ -166,6 +175,8 @@ dependencies = [ "hyper", "pin-project", "thiserror", + "tls-api", + "tls-api-native-tls", "tokio", "tor-error", "tor-rtcompat", @@ -323,7 +334,7 @@ checksum = "9c86f33abd5a4f3e2d6d9251a9e0c6a7e52eb1113caf893dae8429bf4a53f378" dependencies = [ "futures-lite", "rustls", - "webpki", + "webpki 0.21.4", ] [[package]] @@ -971,6 +982,19 @@ version = "1.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e78d4f1cc4ae33bbfc157ed5d5a5ef3bc29227303d595861deb238fcec4e9457" +[[package]] +name = "env_logger" +version = "0.5.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "15b0a4d2e39f8420210be8b27eeda28029729e2fd4291019455016c348240c38" +dependencies = [ + "atty", + "humantime 1.3.0", + "log", + "regex", + "termcolor", +] + [[package]] name = "event-listener" version = "2.5.2" @@ -1103,6 +1127,12 @@ dependencies = [ "winapi 0.3.9", ] +[[package]] +name = "fuchsia-cprng" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a06f77d526c1a601b7c4cdd98f54b5eaabffc14d5f2f0296febdc7f357c6d3ba" + [[package]] name = "fuchsia-zircon" version = "0.3.3" @@ -1387,6 +1417,15 @@ version = "1.0.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c4a1e36c821dbe04574f602848a19f742f4fb3c98d40449f11bcad18d6b17421" +[[package]] +name = "humantime" +version = "1.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "df004cfca50ef23c36850aaaa59ad52cc70d0e90243c3c7737a4dd32dc7a3c4f" +dependencies = [ + "quick-error", +] + [[package]] name = "humantime" version = "2.1.0" @@ -1399,7 +1438,7 @@ version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ac34a56cfd4acddb469cc7fff187ed5ac36f498ba085caf8bbc725e3ff474058" dependencies = [ - "humantime", + "humantime 2.1.0", "serde", ] @@ -1997,6 +2036,17 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8835116a5c179084a830efb3adc117ab007512b535bc1a21c991d3b32a6b44dd" +[[package]] +name = "pem" +version = "0.8.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fd56cbd21fea48d0c440b41cd69c589faacade08c992d9a54e471b79d0fd13eb" +dependencies = [ + "base64", + "once_cell", + "regex", +] + [[package]] name = "pem-rfc7468" version = "0.2.4" @@ -2166,6 +2216,12 @@ dependencies = [ "unicode-xid", ] +[[package]] +name = "quick-error" +version = "1.2.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a1d01941d82fa2ab50be1e79e6714289dd7cde78eba4c074bc5a4374f650dfe0" + [[package]] name = "quickcheck" version = "1.0.3" @@ -2184,6 +2240,19 @@ dependencies = [ "proc-macro2", ] +[[package]] +name = "rand" +version = "0.4.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "552840b97013b1a26992c11eac34bdd778e464601a4c2054b5f0bff7c6761293" +dependencies = [ + "fuchsia-cprng", + "libc", + "rand_core 0.3.1", + "rdrand", + "winapi 0.3.9", +] + [[package]] name = "rand" version = "0.7.3" @@ -2228,6 +2297,21 @@ dependencies = [ "rand_core 0.6.3", ] +[[package]] +name = "rand_core" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7a6fdeb83b075e8266dcc8762c22776f6877a63111121f5f8c7411e5be7eed4b" +dependencies = [ + "rand_core 0.4.2", +] + +[[package]] +name = "rand_core" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9c33a3c44ca05fa6f1807d8e6743f3824e8509beca625669633be0acbdf509dc" + [[package]] name = "rand_core" version = "0.5.1" @@ -2255,6 +2339,15 @@ dependencies = [ "rand_core 0.5.1", ] +[[package]] +name = "rdrand" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "678054eb77286b51581ba43620cc911abf02758c91f93f479767aed0f90458b2" +dependencies = [ + "rand_core 0.3.1", +] + [[package]] name = "redox_syscall" version = "0.2.10" @@ -2280,6 +2373,8 @@ version = "1.5.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d07a8629359eb56f1e2fb1652bb04212c072a87ba68546a04065d525673ac461" dependencies = [ + "aho-corasick", + "memchr", "regex-syntax", ] @@ -2330,7 +2425,7 @@ dependencies = [ "libc", "once_cell", "spin", - "untrusted", + "untrusted 0.7.1", "web-sys", "winapi 0.3.9", ] @@ -2405,7 +2500,7 @@ dependencies = [ "log", "ring", "sct", - "webpki", + "webpki 0.21.4", ] [[package]] @@ -2456,7 +2551,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b362b83898e0e69f38515b82ee15aa80636befe47c3b6d3d89a911e78fc228ce" dependencies = [ "ring", - "untrusted", + "untrusted 0.7.1", ] [[package]] @@ -2718,6 +2813,16 @@ dependencies = [ "unicode-xid", ] +[[package]] +name = "tempdir" +version = "0.3.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "15f2b5fb00ccdf689e0149d1b1b3c03fead81c2b37735d812fa8bddbbf41b6d8" +dependencies = [ + "rand 0.4.6", + "remove_dir_all", +] + [[package]] name = "tempfile" version = "3.3.0" @@ -2732,6 +2837,25 @@ dependencies = [ "winapi 0.3.9", ] +[[package]] +name = "termcolor" +version = "1.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2dfed899f0eb03f32ee8c6a0aabdb8a7949659e3466561fc0adf54e26d88c5f4" +dependencies = [ + "winapi-util", +] + +[[package]] +name = "test-cert-gen" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3208d0ae2e3736d4ac2f6ba2229c4d9bbd54080e228e662a7684eabcf13ff419" +dependencies = [ + "pem", + "tempdir", +] + [[package]] name = "textwrap" version = "0.11.0" @@ -2804,6 +2928,53 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cda74da7e1a664f795bb1f8a87ec406fb89a02522cf6e50620d016add6dbbf5c" +[[package]] +name = "tls-api" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b7dded74ddc6d4a98f9f94f17f1c4d796e4af3cb5fba9e7655f157a036ee7de0" +dependencies = [ + "anyhow", + "log", + "pem", + "tempdir", + "thiserror", + "tokio", + "void", + "webpki 0.22.0", +] + +[[package]] +name = "tls-api-native-tls" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c547db405b51a4e549f803c980572f3cb3957dff153b04e3e7aebb1fc5f249b4" +dependencies = [ + "anyhow", + "native-tls", + "thiserror", + "tls-api", + "tls-api-test", + "tokio", +] + +[[package]] +name = "tls-api-test" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "344ab291be7ed9ab296fc28153fe3ac1e430f44c4dfb3f1324a3c09bbbb5f104" +dependencies = [ + "anyhow", + "env_logger", + "log", + "pem", + "test-cert-gen", + "tls-api", + "tokio", + "untrusted 0.6.2", + "webpki 0.22.0", +] + [[package]] name = "tokio" version = "1.17.0" @@ -3480,6 +3651,12 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8ccb82d61f80a663efe1f787a51b16b5a51e3314d6ac365b08639f52387b33f3" +[[package]] +name = "untrusted" +version = "0.6.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "55cd1f4b4e96b46aeb8d4855db4a7a9bd96eeeb5c6a1ab54593328761642ce2f" + [[package]] name = "untrusted" version = "0.7.1" @@ -3532,6 +3709,12 @@ version = "0.9.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "49874b5167b65d7193b8aba1567f5c7d93d001cafc34600cee003eda787e483f" +[[package]] +name = "void" +version = "1.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6a02e4885ed3bc0f2de90ea6dd45ebcbb66dacffe03547fadbb0eeae2770887d" + [[package]] name = "waker-fn" version = "1.1.0" @@ -3666,7 +3849,17 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b8e38c0608262c46d4a56202ebabdeb094cef7e560ca7a226c6bf055188aa4ea" dependencies = [ "ring", - "untrusted", + "untrusted 0.7.1", +] + +[[package]] +name = "webpki" +version = "0.22.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f095d78192e208183081cc07bc5515ef55216397af48b873e5edcd72637fa1bd" +dependencies = [ + "ring", + "untrusted 0.7.1", ] [[package]] @@ -3792,7 +3985,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9fb2bc2a902d992cd5f471ee3ab0ffd6603047a4207384562755b9d6de977518" dependencies = [ "ring", - "untrusted", + "untrusted 0.7.1", ] [[package]] diff --git a/crates/arti-hyper/Cargo.toml b/crates/arti-hyper/Cargo.toml index f630de9c8..a8f04b7dd 100644 --- a/crates/arti-hyper/Cargo.toml +++ b/crates/arti-hyper/Cargo.toml @@ -28,6 +28,8 @@ hyper = { version = "0.14", features = ["http1", "client", "runtime"] } pin-project = "1" tokio = { package = "tokio", version = "1.7", features = ["rt", "rt-multi-thread", "io-util", "net", "time", "macros" ] } thiserror = "1" +tls-api = { version = "0.7" } +tls-api-native-tls = { version = "0.7.0" } tor-error = { path="../tor-error", version = "0.0.1" } tor-rtcompat = { path="../tor-rtcompat", version = "0.0.4", features=["tokio"] } diff --git a/crates/arti-hyper/examples/hyper.rs b/crates/arti-hyper/examples/hyper.rs index 2f72a3cb5..d4b4702d4 100644 --- a/crates/arti-hyper/examples/hyper.rs +++ b/crates/arti-hyper/examples/hyper.rs @@ -5,6 +5,7 @@ use anyhow::Result; use arti_client::{TorClient, TorClientConfig}; use hyper::Body; use std::convert::TryInto; +use tls_api::{TlsConnector, TlsConnectorBuilder}; use tor_rtcompat::tokio::TokioNativeTlsRuntime; #[tokio::main] @@ -34,8 +35,10 @@ async fn main() -> Result<()> { // (This takes a while to gather the necessary consensus state, etc.) let tor_client = TorClient::create_bootstrapped(rt, config).await?; + let tls_connector = tls_api_native_tls::TlsConnector::builder()?.build()?; + // The `ArtiHttpConnector` lets us make HTTP requests via the Tor network. - let tor_connector = ArtiHttpConnector::new(tor_client); + let tor_connector = ArtiHttpConnector::new(tor_client, tls_connector); let http = hyper::Client::builder().build::<_, Body>(tor_connector); // The rest is just standard usage of Hyper. diff --git a/crates/arti-hyper/src/lib.rs b/crates/arti-hyper/src/lib.rs index 43f7c5d33..9c9bed24e 100644 --- a/crates/arti-hyper/src/lib.rs +++ b/crates/arti-hyper/src/lib.rs @@ -36,6 +36,7 @@ use std::future::Future; use std::io::Error; use std::pin::Pin; +use std::sync::Arc; use std::task::{Context, Poll}; use arti_client::{DataStream, IntoTorAddr, TorClient}; @@ -45,6 +46,7 @@ use hyper::http::Uri; use hyper::service::Service; use pin_project::pin_project; use thiserror::Error; +use tls_api::TlsConnector as TlsConn; // This is different from tor_rtompat::TlsConnector use tokio::io::{AsyncRead, AsyncWrite, ReadBuf}; use tor_rtcompat::Runtime; @@ -90,35 +92,59 @@ impl tor_error::HasKind for ConnectionError { /// A `hyper` connector to proxy HTTP connections via the Tor network, using Arti. /// /// Only supports plaintext HTTP for now. -pub struct ArtiHttpConnector { +/// +/// TC is the TLS to used *across* Tor to connect to the origin server. +/// This is a different Rust type to the TLS used *by* Tor to connect to relays etc. +/// It might even be a different underlying TLS implementation +/// (although that is usually not a particularly good idea). +pub struct ArtiHttpConnector { /// The client client: TorClient, + + /// TLS for using across Tor. + #[allow(dead_code)] // TODO + tls_conn: Arc, } -impl Clone for ArtiHttpConnector { +// #[derive(Clone)] infers a TC: Clone bound +impl Clone for ArtiHttpConnector { fn clone(&self) -> Self { let client = self.client.clone(); - Self { client } + let tls_conn = self.tls_conn.clone(); + Self { client, tls_conn } } } -impl ArtiHttpConnector { +impl ArtiHttpConnector { /// Make a new `ArtiHttpConnector` using an Arti `TorClient` object. - pub fn new(client: TorClient) -> Self { - Self { client } + pub fn new(client: TorClient, tls_conn: TC) -> Self { + let tls_conn = tls_conn.into(); + Self { client, tls_conn } } } /// Wrapper type that makes an Arti `DataStream` implement necessary traits to be used as /// a `hyper` connection object (mainly `Connection`). #[pin_project] -pub struct ArtiHttpConnection { +pub struct ArtiHttpConnection { /// The stream #[pin] - inner: DataStream, + inner: MaybeHttpsStream, } -impl Connection for ArtiHttpConnection { +/// The actual actual stream; might be TLS, might not +#[pin_project(project = MaybeHttpsStreamProj)] +#[allow(clippy::large_enum_variant)] +enum MaybeHttpsStream { + /// http + Http(#[pin] DataStream), + + /// https + #[allow(dead_code)] // TODO + Https(#[pin] TC::TlsStream), +} + +impl Connection for ArtiHttpConnection { fn connected(&self) -> Connected { Connected::new() } @@ -126,31 +152,43 @@ impl Connection for ArtiHttpConnection { // These trait implementations just defer to the inner `DataStream`; the wrapper type is just // there to implement the `Connection` trait. -impl AsyncRead for ArtiHttpConnection { +impl AsyncRead for ArtiHttpConnection { fn poll_read( self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &mut ReadBuf<'_>, ) -> Poll> { - self.project().inner.poll_read(cx, buf) + match self.project().inner.project() { + MaybeHttpsStreamProj::Http(ds) => ds.poll_read(cx, buf), + MaybeHttpsStreamProj::Https(t) => t.poll_read(cx, buf), + } } } -impl AsyncWrite for ArtiHttpConnection { +impl AsyncWrite for ArtiHttpConnection { fn poll_write( self: Pin<&mut Self>, cx: &mut Context<'_>, buf: &[u8], ) -> Poll> { - self.project().inner.poll_write(cx, buf) + match self.project().inner.project() { + MaybeHttpsStreamProj::Http(ds) => ds.poll_write(cx, buf), + MaybeHttpsStreamProj::Https(t) => t.poll_write(cx, buf), + } } fn poll_flush(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { - self.project().inner.poll_flush(cx) + match self.project().inner.project() { + MaybeHttpsStreamProj::Http(ds) => ds.poll_flush(cx), + MaybeHttpsStreamProj::Https(t) => t.poll_flush(cx), + } } fn poll_shutdown(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { - self.project().inner.poll_shutdown(cx) + match self.project().inner.project() { + MaybeHttpsStreamProj::Http(ds) => ds.poll_shutdown(cx), + MaybeHttpsStreamProj::Https(t) => t.poll_shutdown(cx), + } } } @@ -168,8 +206,8 @@ fn uri_to_host_port(uri: Uri) -> Result<(String, u16), ConnectionError> { Ok((host.to_owned(), port)) } -impl Service for ArtiHttpConnector { - type Response = ArtiHttpConnection; +impl Service for ArtiHttpConnector { + type Response = ArtiHttpConnection; type Error = ConnectionError; #[allow(clippy::type_complexity)] type Future = Pin> + Send>>; @@ -191,7 +229,9 @@ impl Service for ArtiHttpConnector { .into_tor_addr() .map_err(arti_client::Error::from)?; let ds = client.connect(addr).await?; - Ok(ArtiHttpConnection { inner: ds }) + Ok(ArtiHttpConnection { + inner: MaybeHttpsStream::Http(ds), + }) }) } } From c24c3af81ba4a5e899d8bda2a66c511a2c9ed504 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Fri, 25 Feb 2022 11:49:42 +0000 Subject: [PATCH 05/16] arti-hyper: Actually support TLS --- crates/arti-hyper/Cargo.toml | 2 +- crates/arti-hyper/examples/hyper.rs | 1 - crates/arti-hyper/src/lib.rs | 55 ++++++++++++++++++++--------- 3 files changed, 39 insertions(+), 19 deletions(-) diff --git a/crates/arti-hyper/Cargo.toml b/crates/arti-hyper/Cargo.toml index a8f04b7dd..2074630c9 100644 --- a/crates/arti-hyper/Cargo.toml +++ b/crates/arti-hyper/Cargo.toml @@ -23,6 +23,7 @@ static = [ "arti-client/static" ] experimental-api = [] [dependencies] +anyhow = "1.0.23" arti-client = { path="../arti-client", version = "0.0.4"} hyper = { version = "0.14", features = ["http1", "client", "runtime"] } pin-project = "1" @@ -34,5 +35,4 @@ tor-error = { path="../tor-error", version = "0.0.1" } tor-rtcompat = { path="../tor-rtcompat", version = "0.0.4", features=["tokio"] } [dev-dependencies] -anyhow = "1.0.23" tracing-subscriber = "0.3.0" diff --git a/crates/arti-hyper/examples/hyper.rs b/crates/arti-hyper/examples/hyper.rs index d4b4702d4..9e9f192b9 100644 --- a/crates/arti-hyper/examples/hyper.rs +++ b/crates/arti-hyper/examples/hyper.rs @@ -1,4 +1,3 @@ -/// TODO this ought to support https! use arti_hyper::*; use anyhow::Result; diff --git a/crates/arti-hyper/src/lib.rs b/crates/arti-hyper/src/lib.rs index 9c9bed24e..8c51359ee 100644 --- a/crates/arti-hyper/src/lib.rs +++ b/crates/arti-hyper/src/lib.rs @@ -1,7 +1,4 @@ //! High-level layer for making http(s) requests the Tor network as a client. -//! -//! Work-in-progress. -//! This is **not suitable for use** right now because it does not support HTTPs. #![deny(missing_docs)] #![warn(noop_method_call)] @@ -73,6 +70,10 @@ pub enum ConnectionError { /// Tor connection failed #[error("Tor connection failed")] Arti(#[from] arti_client::Error), + + /// TLS connection failed + #[error("TLS connection failed")] + TLS(#[source] Arc), } /// We implement this for form's sake @@ -84,7 +85,8 @@ impl tor_error::HasKind for ConnectionError { match self { CE::UnsupportedUriScheme{..} => EK::NotImplemented, CE::MissingHostname{..} => EK::BadApiUsage, - CE::Arti(e) => e.kind(), + CE::Arti(e) => e.kind(), + CE::TLS(_) => EK::OtherRemote, } } } @@ -102,7 +104,6 @@ pub struct ArtiHttpConnector { client: TorClient, /// TLS for using across Tor. - #[allow(dead_code)] // TODO tls_conn: Arc, } @@ -140,7 +141,6 @@ enum MaybeHttpsStream { Http(#[pin] DataStream), /// https - #[allow(dead_code)] // TODO Https(#[pin] TC::TlsStream), } @@ -192,18 +192,29 @@ impl AsyncWrite for ArtiHttpConnection { } } -/// Convert uri to host and port -fn uri_to_host_port(uri: Uri) -> Result<(String, u16), ConnectionError> { - if uri.scheme() != Some(&Scheme::HTTP) { - return Err(ConnectionError::UnsupportedUriScheme { uri }); - } +/// Convert uri to http[s] host and port, and whether to do tls +fn uri_to_host_port_tls(uri: Uri) -> Result<(String, u16, bool), ConnectionError> { + let use_tls = { + // Scheme doesn't derive PartialEq so can't be matched on + let scheme = uri.scheme(); + if scheme == Some(&Scheme::HTTP) { + false + } else if scheme == Some(&Scheme::HTTPS) { + true + } else { + return Err(ConnectionError::UnsupportedUriScheme { uri }); + } + }; let host = match uri.host() { Some(h) => h, _ => return Err(ConnectionError::MissingHostname { uri }), }; - let port = uri.port().map(|x| x.as_u16()).unwrap_or(80); + let port = uri + .port() + .map(|x| x.as_u16()) + .unwrap_or(if use_tls { 443 } else { 80 }); - Ok((host.to_owned(), port)) + Ok((host.to_owned(), port, use_tls)) } impl Service for ArtiHttpConnector { @@ -221,17 +232,27 @@ impl Service for ArtiHttpConnector { // underlying handles required to make Tor connections internally). // We use this to avoid the returned future having to borrow `self`. let client = self.client.clone(); + let tls_conn = self.tls_conn.clone(); Box::pin(async move { // Extract the host and port to connect to from the URI. - let (host, port) = uri_to_host_port(req)?; + let (host, port, use_tls) = uri_to_host_port_tls(req)?; // Initiate a new Tor connection, producing a `DataStream` if successful. let addr = (&host as &str, port) .into_tor_addr() .map_err(arti_client::Error::from)?; let ds = client.connect(addr).await?; - Ok(ArtiHttpConnection { - inner: MaybeHttpsStream::Http(ds), - }) + + let inner = if use_tls { + let conn = tls_conn + .connect_impl_tls_stream(&host, ds) + .await + .map_err(|e| ConnectionError::TLS(e.into()))?; + MaybeHttpsStream::Https(conn) + } else { + MaybeHttpsStream::Http(ds) + }; + + Ok(ArtiHttpConnection { inner }) }) } } From 51c9cec1f1969920498e9dd41db6cd606ce8092e Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Fri, 25 Feb 2022 12:17:29 +0000 Subject: [PATCH 06/16] arti-hyper: Clarify what the TLS features do. --- crates/arti-hyper/README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/arti-hyper/README.md b/crates/arti-hyper/README.md index a759a9b68..5ebf73de5 100644 --- a/crates/arti-hyper/README.md +++ b/crates/arti-hyper/README.md @@ -11,6 +11,7 @@ we might break them or remove them between patch versions. `error_detail` -- Make the `TorError` type transparent, and expose the `Error` within. Note that the resulting APIs are not stable. -`native-tls` (default), `rustls` -- Select TLS libraries to support. +`native-tls` (default), `rustls` -- Select TLS libraries to use for Tor's purposes. +(The end-to-end TLS to the origin server is separate, and handled via `tls-api`.) License: MIT OR Apache-2.0 From 85e7d4b088e682476973f6b6bedc92cec1ea9bda Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Fri, 25 Feb 2022 12:18:54 +0000 Subject: [PATCH 07/16] arti-hyper: Drop description of nonexistent error-detail feature --- crates/arti-hyper/README.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/crates/arti-hyper/README.md b/crates/arti-hyper/README.md index 5ebf73de5..9861a3bdf 100644 --- a/crates/arti-hyper/README.md +++ b/crates/arti-hyper/README.md @@ -8,9 +8,6 @@ High-level layer for making http(s) requests the Tor network as a client. Note that these APIs are NOT covered by semantic versioning guarantees: we might break them or remove them between patch versions. -`error_detail` -- Make the `TorError` type transparent, and expose the `Error` within. -Note that the resulting APIs are not stable. - `native-tls` (default), `rustls` -- Select TLS libraries to use for Tor's purposes. (The end-to-end TLS to the origin server is separate, and handled via `tls-api`.) From 4705234a497cc4a8e15436f00d7db486e666e79d Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Fri, 25 Feb 2022 12:19:55 +0000 Subject: [PATCH 08/16] arti-hyper: Fix duplicative doc comment not to be wrong --- crates/arti-hyper/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/arti-hyper/src/lib.rs b/crates/arti-hyper/src/lib.rs index 8c51359ee..d25cb575e 100644 --- a/crates/arti-hyper/src/lib.rs +++ b/crates/arti-hyper/src/lib.rs @@ -60,7 +60,7 @@ pub enum ConnectionError { uri: Uri, }, - /// Unsupported URI scheme + /// Missing hostname #[error("Missing hostname in {uri:?}")] MissingHostname { /// URI From adfabe6cd1a7c4c9039f859bfcc2146465a3227b Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 28 Feb 2022 13:12:20 +0000 Subject: [PATCH 09/16] arti-hyper: Box the http variant (bare DataStream) As per https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/355#note_2781819 --- crates/arti-hyper/src/lib.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/crates/arti-hyper/src/lib.rs b/crates/arti-hyper/src/lib.rs index d25cb575e..42e289902 100644 --- a/crates/arti-hyper/src/lib.rs +++ b/crates/arti-hyper/src/lib.rs @@ -135,10 +135,9 @@ pub struct ArtiHttpConnection { /// The actual actual stream; might be TLS, might not #[pin_project(project = MaybeHttpsStreamProj)] -#[allow(clippy::large_enum_variant)] enum MaybeHttpsStream { /// http - Http(#[pin] DataStream), + Http(Pin>), // Tc:TlsStream is generally boxed; box this one too /// https Https(#[pin] TC::TlsStream), @@ -159,7 +158,7 @@ impl AsyncRead for ArtiHttpConnection { buf: &mut ReadBuf<'_>, ) -> Poll> { match self.project().inner.project() { - MaybeHttpsStreamProj::Http(ds) => ds.poll_read(cx, buf), + MaybeHttpsStreamProj::Http(ds) => ds.as_mut().poll_read(cx, buf), MaybeHttpsStreamProj::Https(t) => t.poll_read(cx, buf), } } @@ -172,21 +171,21 @@ impl AsyncWrite for ArtiHttpConnection { buf: &[u8], ) -> Poll> { match self.project().inner.project() { - MaybeHttpsStreamProj::Http(ds) => ds.poll_write(cx, buf), + MaybeHttpsStreamProj::Http(ds) => ds.as_mut().poll_write(cx, buf), MaybeHttpsStreamProj::Https(t) => t.poll_write(cx, buf), } } fn poll_flush(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { match self.project().inner.project() { - MaybeHttpsStreamProj::Http(ds) => ds.poll_flush(cx), + MaybeHttpsStreamProj::Http(ds) => ds.as_mut().poll_flush(cx), MaybeHttpsStreamProj::Https(t) => t.poll_flush(cx), } } fn poll_shutdown(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll> { match self.project().inner.project() { - MaybeHttpsStreamProj::Http(ds) => ds.poll_shutdown(cx), + MaybeHttpsStreamProj::Http(ds) => ds.as_mut().poll_shutdown(cx), MaybeHttpsStreamProj::Https(t) => t.poll_shutdown(cx), } } @@ -249,7 +248,7 @@ impl Service for ArtiHttpConnector { .map_err(|e| ConnectionError::TLS(e.into()))?; MaybeHttpsStream::Https(conn) } else { - MaybeHttpsStream::Http(ds) + MaybeHttpsStream::Http(Box::new(ds).into()) }; Ok(ArtiHttpConnection { inner }) From 551bc690452f121825262166c8a25e6868be33d7 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 28 Feb 2022 13:16:25 +0000 Subject: [PATCH 10/16] arti-hyper: Abolish a bool in favour of a custom private enum As per https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/355#note_2781820 --- crates/arti-hyper/src/lib.rs | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/crates/arti-hyper/src/lib.rs b/crates/arti-hyper/src/lib.rs index 42e289902..7751033b0 100644 --- a/crates/arti-hyper/src/lib.rs +++ b/crates/arti-hyper/src/lib.rs @@ -191,15 +191,19 @@ impl AsyncWrite for ArtiHttpConnection { } } +#[derive(Debug, Clone, Copy, Eq, PartialEq)] +/// Are we doing TLS? +enum UseTls { Bare, Tls } + /// Convert uri to http[s] host and port, and whether to do tls -fn uri_to_host_port_tls(uri: Uri) -> Result<(String, u16, bool), ConnectionError> { +fn uri_to_host_port_tls(uri: Uri) -> Result<(String, u16, UseTls), ConnectionError> { let use_tls = { // Scheme doesn't derive PartialEq so can't be matched on let scheme = uri.scheme(); if scheme == Some(&Scheme::HTTP) { - false + UseTls::Bare } else if scheme == Some(&Scheme::HTTPS) { - true + UseTls::Tls } else { return Err(ConnectionError::UnsupportedUriScheme { uri }); } @@ -211,7 +215,7 @@ fn uri_to_host_port_tls(uri: Uri) -> Result<(String, u16, bool), ConnectionError let port = uri .port() .map(|x| x.as_u16()) - .unwrap_or(if use_tls { 443 } else { 80 }); + .unwrap_or(match use_tls { UseTls::Tls => 443, UseTls::Bare => 80 }); Ok((host.to_owned(), port, use_tls)) } @@ -241,14 +245,17 @@ impl Service for ArtiHttpConnector { .map_err(arti_client::Error::from)?; let ds = client.connect(addr).await?; - let inner = if use_tls { - let conn = tls_conn - .connect_impl_tls_stream(&host, ds) - .await - .map_err(|e| ConnectionError::TLS(e.into()))?; - MaybeHttpsStream::Https(conn) - } else { - MaybeHttpsStream::Http(Box::new(ds).into()) + let inner = match use_tls { + UseTls::Tls => { + let conn = tls_conn + .connect_impl_tls_stream(&host, ds) + .await + .map_err(|e| ConnectionError::TLS(e.into()))?; + MaybeHttpsStream::Https(conn) + }, + UseTls::Bare => { + MaybeHttpsStream::Http(Box::new(ds).into()) + }, }; Ok(ArtiHttpConnection { inner }) From 4425171245e0ee40d6f4a2a7a6f31371e2859653 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 28 Feb 2022 13:17:41 +0000 Subject: [PATCH 11/16] Fix rustfmt I disagree with all these. Whatever. --- crates/arti-hyper/src/lib.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/crates/arti-hyper/src/lib.rs b/crates/arti-hyper/src/lib.rs index 7751033b0..a3e7b6fb3 100644 --- a/crates/arti-hyper/src/lib.rs +++ b/crates/arti-hyper/src/lib.rs @@ -193,7 +193,10 @@ impl AsyncWrite for ArtiHttpConnection { #[derive(Debug, Clone, Copy, Eq, PartialEq)] /// Are we doing TLS? -enum UseTls { Bare, Tls } +enum UseTls { + Bare, + Tls, +} /// Convert uri to http[s] host and port, and whether to do tls fn uri_to_host_port_tls(uri: Uri) -> Result<(String, u16, UseTls), ConnectionError> { @@ -212,10 +215,10 @@ fn uri_to_host_port_tls(uri: Uri) -> Result<(String, u16, UseTls), ConnectionErr Some(h) => h, _ => return Err(ConnectionError::MissingHostname { uri }), }; - let port = uri - .port() - .map(|x| x.as_u16()) - .unwrap_or(match use_tls { UseTls::Tls => 443, UseTls::Bare => 80 }); + let port = uri.port().map(|x| x.as_u16()).unwrap_or(match use_tls { + UseTls::Tls => 443, + UseTls::Bare => 80, + }); Ok((host.to_owned(), port, use_tls)) } @@ -252,10 +255,8 @@ impl Service for ArtiHttpConnector { .await .map_err(|e| ConnectionError::TLS(e.into()))?; MaybeHttpsStream::Https(conn) - }, - UseTls::Bare => { - MaybeHttpsStream::Http(Box::new(ds).into()) - }, + } + UseTls::Bare => MaybeHttpsStream::Http(Box::new(ds).into()), }; Ok(ArtiHttpConnection { inner }) From 8898bbb4218b4858aabe80d5b049a8a025c1c7a9 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 28 Feb 2022 13:27:44 +0000 Subject: [PATCH 12/16] EK::RemoteProtocolFailed replaces OtherRemote As per https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/355#note_2781816 --- crates/arti-hyper/src/lib.rs | 2 +- crates/tor-error/src/lib.rs | 20 ++++++++++++++------ 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/crates/arti-hyper/src/lib.rs b/crates/arti-hyper/src/lib.rs index a3e7b6fb3..f17c1cdb2 100644 --- a/crates/arti-hyper/src/lib.rs +++ b/crates/arti-hyper/src/lib.rs @@ -86,7 +86,7 @@ impl tor_error::HasKind for ConnectionError { CE::UnsupportedUriScheme{..} => EK::NotImplemented, CE::MissingHostname{..} => EK::BadApiUsage, CE::Arti(e) => e.kind(), - CE::TLS(_) => EK::OtherRemote, + CE::TLS(_) => EK::RemoteProtocolFailed, } } } diff --git a/crates/tor-error/src/lib.rs b/crates/tor-error/src/lib.rs index b1c6ce6a0..70fbebfeb 100644 --- a/crates/tor-error/src/lib.rs +++ b/crates/tor-error/src/lib.rs @@ -424,14 +424,22 @@ pub enum ErrorKind { #[display(fmt = "remote hostname lookup failure")] RemoteHostNotFound, - /// Something else went wrong on the far side of the Tor network + /// Trouble involving a protocol we're using with a peer on the far side of the Tor network /// - /// This might be an error reported by the exit; - /// or when using a higher-layer protocol over a Tor connection, - /// this might be an error reported by the remote host within that higher protocol, + /// We were using a higher-layer protocol over a Tor connection, + /// and something went wrong. + /// This might be an error reported by the remote host within that higher protocol, /// or a problem detected locally but relating to that higher protocol. - #[display(fmt = "remote error")] - OtherRemote, + /// + /// The nature of the problem can vary: + /// examples could include: + /// failure to agree suitable parameters (incompatibility); + /// authentication problems (eg, TLS certificate trouble); + /// protocol violation by the peer; + /// peer refusing to provide service; + /// etc. + #[display(fmt = "remote protocol failedr")] + RemoteProtocolFailed, /// An operation failed, and the relay in question reported that it's too /// busy to answer our request. From cdbb904664c66378168e01528248030d1004c183 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 28 Feb 2022 14:39:43 +0000 Subject: [PATCH 13/16] arti-hyper: Add vacuous doc comments as required by clippy --- crates/arti-hyper/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/arti-hyper/src/lib.rs b/crates/arti-hyper/src/lib.rs index f17c1cdb2..f321eacfe 100644 --- a/crates/arti-hyper/src/lib.rs +++ b/crates/arti-hyper/src/lib.rs @@ -194,7 +194,10 @@ impl AsyncWrite for ArtiHttpConnection { #[derive(Debug, Clone, Copy, Eq, PartialEq)] /// Are we doing TLS? enum UseTls { + /// No Bare, + + /// Yes Tls, } From ec25f68b9bf002034886256b18e6247b13a0018b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 28 Feb 2022 14:47:25 +0000 Subject: [PATCH 14/16] Fix typo in message --- crates/tor-error/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/tor-error/src/lib.rs b/crates/tor-error/src/lib.rs index 70fbebfeb..0a4b994f8 100644 --- a/crates/tor-error/src/lib.rs +++ b/crates/tor-error/src/lib.rs @@ -438,7 +438,7 @@ pub enum ErrorKind { /// protocol violation by the peer; /// peer refusing to provide service; /// etc. - #[display(fmt = "remote protocol failedr")] + #[display(fmt = "remote protocol failed")] RemoteProtocolFailed, /// An operation failed, and the relay in question reported that it's too From 9618f85237998df37d540ee12568798e8346c820 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Mon, 28 Feb 2022 17:09:58 +0000 Subject: [PATCH 15/16] maint/downgrade_dependencies: Upgrade zeroize_derive, env_logger The *earlier* versions of these crates pull in *dependencies* that violate our MSRV policy by requiring a *later* version of Rust. Empirically, env_logger 0.5.4 would be enough, but practice here seems to be to just say "cargo update". --- maint/downgrade_dependencies | 2 ++ 1 file changed, 2 insertions(+) diff --git a/maint/downgrade_dependencies b/maint/downgrade_dependencies index 569ab7150..ffa17311b 100755 --- a/maint/downgrade_dependencies +++ b/maint/downgrade_dependencies @@ -9,4 +9,6 @@ cargo +nightly update -Z minimal-versions cargo update \ -p crc32fast \ -p quote:0.6.3 \ + -p zeroize_derive:1.1.1 \ + -p env_logger:0.5.0 \ -p synstructure:0.12.0 From c646d3adca46c162737a2729226c085405c310f7 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 28 Feb 2022 13:25:14 -0500 Subject: [PATCH 16/16] Add nix exception to downgrade_dependencies. --- maint/downgrade_dependencies | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/maint/downgrade_dependencies b/maint/downgrade_dependencies index ffa17311b..59a24e617 100755 --- a/maint/downgrade_dependencies +++ b/maint/downgrade_dependencies @@ -11,4 +11,5 @@ cargo update \ -p quote:0.6.3 \ -p zeroize_derive:1.1.1 \ -p env_logger:0.5.0 \ - -p synstructure:0.12.0 + -p synstructure:0.12.0 \ + -p nix:0.4.2