diff --git a/crates/orbyn-device-client/src/api.rs b/crates/orbyn-device-client/src/api.rs index 9147a08..5b4ce9e 100644 --- a/crates/orbyn-device-client/src/api.rs +++ b/crates/orbyn-device-client/src/api.rs @@ -111,6 +111,10 @@ impl HttpApi { } let client = reqwest::Client::builder() .redirect(reqwest::redirect::Policy::none()) + // No proxy from the environment, loopback included: reqwest reads `HTTP_PROXY`, + // `HTTPS_PROXY` and `ALL_PROXY` by default and does not exempt 127.0.0.1, so a request + // with the bearer token would go to whatever the machine names as its proxy. + .no_proxy() .timeout(TIMEOUT) .connect_timeout(CONNECT_TIMEOUT) .https_only(!loopback) diff --git a/crates/orbyn-device-client/tests/no_proxy.rs b/crates/orbyn-device-client/tests/no_proxy.rs new file mode 100644 index 0000000..830bba6 --- /dev/null +++ b/crates/orbyn-device-client/tests/no_proxy.rs @@ -0,0 +1,116 @@ +//! The real transport never goes through a proxy named in the environment. A request to the +//! engine carries a bearer token, so a `HTTP_PROXY` or `ALL_PROXY` set on the machine (by +//! software, a profile or an attacker) must not be able to see it, even for a loopback address. +//! +//! The environment is process-wide, so each case runs the same test binary again as a child with +//! the proxy variables set. A loopback server stands for the engine and a second listener for the +//! proxy; the proxy must get no connection. + +use orbyn_device_client::{DeviceApi, HttpApi, Method, Request, Zeroizing}; +use std::process::Command; +use std::sync::Arc; +use std::sync::atomic::{AtomicUsize, Ordering}; +use tokio::io::{AsyncReadExt, AsyncWriteExt}; +use tokio::net::TcpListener; + +const CHILD: &str = "ORBYN_NO_PROXY_CHILD"; + +/// A listener that counts connections and answers `status`. +async fn listener(status: &'static str) -> (String, Arc) { + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let address = format!("http://{}", listener.local_addr().expect("addr")); + let count = Arc::new(AtomicUsize::new(0)); + let seen = count.clone(); + tokio::spawn(async move { + while let Ok((mut socket, _)) = listener.accept().await { + seen.fetch_add(1, Ordering::SeqCst); + tokio::spawn(async move { + let mut buffer = vec![0u8; 8192]; + let _ = socket.read(&mut buffer).await; + let reply = + format!("HTTP/1.1 {status}\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"); + let _ = socket.write_all(reply.as_bytes()).await; + let _ = socket.shutdown().await; + }); + } + }); + (address, count) +} + +/// Runs in the child: one authenticated request to the address in the environment. +#[tokio::test(flavor = "multi_thread")] +async fn child_sends_one_request() { + let Ok(address) = std::env::var(CHILD) else { + return; // Not a child: nothing to do. + }; + let response = HttpApi::new(&address) + .expect("loopback http is allowed") + .send(Request { + method: Method::Get, + path: "/v1/devices".to_owned(), + bearer: Some(Zeroizing::new("secret-token".to_owned())), + body: None, + }) + .await + .expect("an answer"); + assert_eq!(response.status, 204, "the answer came from the engine"); +} + +fn child(engine: &str, proxy: &str, variables: &[&str], no_proxy: Option<&str>) -> bool { + let mut command = Command::new(std::env::current_exe().expect("this test binary")); + command + .args(["--exact", "child_sends_one_request", "--nocapture"]) + .env(CHILD, engine); + for name in [ + "HTTP_PROXY", + "http_proxy", + "HTTPS_PROXY", + "https_proxy", + "ALL_PROXY", + "all_proxy", + "NO_PROXY", + "no_proxy", + ] { + command.env_remove(name); + } + for name in variables { + command.env(name, proxy); + } + if let Some(no_proxy) = no_proxy { + command.env("NO_PROXY", no_proxy).env("no_proxy", no_proxy); + } + command.status().expect("the child ran").success() +} + +#[tokio::test(flavor = "multi_thread")] +async fn a_proxy_in_the_environment_never_sees_a_request_to_the_engine() { + if std::env::var(CHILD).is_ok() { + return; // The child runs only `child_sends_one_request`. + } + for (variables, no_proxy) in [ + (&["HTTP_PROXY", "http_proxy"][..], None), + (&["ALL_PROXY", "all_proxy"][..], None), + (&["HTTPS_PROXY", "https_proxy"][..], None), + ( + &["HTTP_PROXY", "ALL_PROXY"][..], + // A NO_PROXY that does not name the engine's address changes nothing. + Some("example.com"), + ), + ] { + let (engine, reached_engine) = listener("204 No Content").await; + let (proxy, reached_proxy) = listener("502 Bad Gateway").await; + let (engine_for_child, proxy_for_child) = (engine.clone(), proxy.clone()); + let ok = tokio::task::spawn_blocking(move || { + child(&engine_for_child, &proxy_for_child, variables, no_proxy) + }) + .await + .expect("joined"); + assert_eq!( + reached_proxy.load(Ordering::SeqCst), + 0, + "{variables:?} NO_PROXY={no_proxy:?}: the proxy was contacted" + ); + assert!(ok, "{variables:?}: the request did not reach the engine"); + assert_eq!(reached_engine.load(Ordering::SeqCst), 1, "{variables:?}"); + } +} diff --git a/docs/device-client.md b/docs/device-client.md index 799c72c..f38c1c7 100644 --- a/docs/device-client.md +++ b/docs/device-client.md @@ -82,10 +82,11 @@ An error never carries a password, a token, a code, a key or a URL. `Request` an ## The transport -`HttpApi` is reqwest with **rustls and ring only** (no OpenSSL, no aws-lc). **Redirects are off**: a redirect could carry the bearer token elsewhere, and a redirect is returned as an answer. The address must be `https`, or `http` to a **loopback IP literal** (`127.0.0.1`, `[::1]`) for a local stack, with no user info, query or path. A name such as `localhost` is not a loopback literal, because a name can be made to point anywhere. `is_loopback_literal(address)` is public so the app's "local only" gate can use the same test. Calls time out at 30 seconds, connects at 10, and a response body is capped at 32 MiB (`with_body_limit` lowers it), counted **while reading**, so a server that sends no `Content-Length`, or lies about it, is stopped too. +`HttpApi` is reqwest with **rustls and ring only** (no OpenSSL, no aws-lc). **No proxy is used, whatever `HTTP_PROXY`, `HTTPS_PROXY`, `ALL_PROXY` or `NO_PROXY` say** (reqwest reads them by default and does not exempt loopback, so the bearer token would reach the proxy); **Redirects are off**: a redirect could carry the bearer token elsewhere, and a redirect is returned as an answer. The address must be `https`, or `http` to a **loopback IP literal** (`127.0.0.1`, `[::1]`) for a local stack, with no user info, query or path. A name such as `localhost` is not a loopback literal, because a name can be made to point anywhere. `is_loopback_literal(address)` is public so the app's "local only" gate can use the same test. Calls time out at 30 seconds, connects at 10, and a response body is capped at 32 MiB (`with_body_limit` lowers it), counted **while reading**, so a server that sends no `Content-Length`, or lies about it, is stopped too. ## Tests +- `tests/no_proxy.rs` runs the test binary again as a child with `HTTP_PROXY`, `ALL_PROXY` and `HTTPS_PROXY` set (and a `NO_PROXY` that does not name the engine) to a second listener, sends one authenticated request to a loopback engine and checks that the proxy gets no connection and the engine gets the request. - `tests/redirects.rs` serves 301, 302, 303, 307 and 308 from a loopback server with a `Location` to a second listener, sends every method with a bearer token through `HttpApi`, and checks that each redirect comes back as the answer and the second listener never gets a connection; it also checks that plain `http` is refused for any host that is not a loopback IP literal. - `tests/client.rs` runs the client against an in-memory engine (`tests/support`) that speaks the **real OPAQUE server side** and checks the real signatures: proof of work, invite, code, trusted start, second factor, re-login after eleven minutes, account gone, rate limit, and that no request body holds the password. The same file serves a raw HTTP reply to check that redirects are not followed and `Retry-After` is read. - `tests/local_stack.rs` (ignored by default, and skipped with a printed note unless `ORBYN_LOCAL_STACK=1`, so `cargo test -- --include-ignored` in CI passes without a stack) runs sign-up, a second pending device, a wrong password, the re-login and sign-out against the real engine on `scripts/local.sh up`, using `scripts/local.sh invite-code` and `scripts/local.sh latest-code `.