diff --git a/doc/bin/relay/auth.md b/doc/bin/relay/auth.md index e6762b2776a..3e7fbc5eab8 100644 --- a/doc/bin/relay/auth.md +++ b/doc/bin/relay/auth.md @@ -37,7 +37,10 @@ bytes as unpadded base64url), `role` the client declared at SETUP `name` (first SAN DNS name, else CN, else the fingerprint; a server cannot tell which), `fingerprint` (SHA-256 of the leaf), `expires`, `issuer`. Nothing is parsed on the server's behalf: the `jwt` query parameter is a convention of `moq auth serve`, not of -the relay, and the relay forwards a SETUP token without reading it. An `end` adds `reason`, `duration` in seconds, and `bytes` sent and +the relay, and the relay forwards a SETUP token without reading it. A path with +a `.` or `..` segment, percent-encoded or not, is refused before the server is +asked: a URL transport resolves those away, but a SETUP path arrives verbatim. +An `end` adds `reason`, `duration` in seconds, and `bytes` sent and received. **Grant.** `publish` and `subscribe` as pattern unions (`foo/**` is a subtree, @@ -120,7 +123,9 @@ only. HTTPS verifies the URL's hostname with system roots. For a private CA, set `auth.root` to PEM paths (or repeat `--auth-root`); these replace system trust for auth only and reload for new connections when rotated. Mesh roots, fingerprints, insecure mode, and hostname overrides never apply to auth. -There is no shared secret. +There is no shared secret. Auth requests never follow redirects, so a redirect +cannot forward a credential to another destination. Point `--auth-url` at the +server's final URL; a redirect is reported as an auth-server outage. **Patterns.** A grant is any pattern union and means exactly what it says: `foo/**` is a subtree, a bare `foo` is that one broadcast, `live/*` is every diff --git a/rs/moq-auth/src/client.rs b/rs/moq-auth/src/client.rs index 271ef4cb0b0..ed0ab272986 100644 --- a/rs/moq-auth/src/client.rs +++ b/rs/moq-auth/src/client.rs @@ -32,7 +32,11 @@ impl Client { /// caller's client identity and roots; `unix://` speaks HTTP over the socket at /// the URL's path. Anything else is refused here rather than at the first session. pub fn new(url: Url, tls: Option) -> crate::Result { - let builder = reqwest::Client::builder().timeout(TIMEOUT); + // Every request carries a credential in its body, which a followed 307 or + // 308 would resend to wherever it points, so refuse redirects. + let builder = reqwest::Client::builder() + .timeout(TIMEOUT) + .redirect(reqwest::redirect::Policy::none()); let (builder, url) = match url.scheme() { "http" => { @@ -87,29 +91,35 @@ trait Post: Clone + Send + Sync + 'static { } impl Post for Client { - /// A 2xx carries the grant, a 401 or 403 refuses, and everything else is an - /// outage the caller decides about. + /// A 2xx carries the grant, a 401 or 403 refuses, and everything else, a redirect + /// included, is an outage the caller decides about. async fn post(&self, request: &Request) -> crate::Result { let response = self.http.post(self.url.clone()).json(request).send().await?; let status = response.status(); if status == reqwest::StatusCode::UNAUTHORIZED || status == reqwest::StatusCode::FORBIDDEN { return Err(Error::Refused); } - if !status.is_success() { - return Err(Error::Unavailable(format!("auth server answered {status}"))); - } + check(status)?; Ok(response.json().await?) } async fn post_end(&self, request: &Request) -> crate::Result<()> { - self.http - .post(self.url.clone()) - .json(request) - .send() - .await? - .error_for_status()?; - Ok(()) + let response = self.http.post(self.url.clone()).json(request).send().await?; + check(response.status()) + } +} + +/// Refuse anything but a 2xx, naming a redirect as one, since it is not followed. +fn check(status: reqwest::StatusCode) -> crate::Result<()> { + if status.is_redirection() { + return Err(Error::Unavailable(format!( + "auth server answered {status}; redirects are not followed" + ))); + } + if !status.is_success() { + return Err(Error::Unavailable(format!("auth server answered {status}"))); } + Ok(()) } /// Admit a session through `server` and spawn the driver behind its lease. @@ -473,6 +483,53 @@ mod tests { assert!(matches!(err, Error::Refused), "{err}"); } + #[tokio::test] + async fn redirects_are_refused_without_resending_the_credential() { + let foreign = MockServer::start().await; + Mock::given(wiremock::matchers::any()) + .respond_with(ResponseTemplate::new(200).set_body_json(grant(None, None))) + .mount(&foreign) + .await; + + for status in [307, 308, 301, 302, 303] { + let location = format!("{}/foreign", foreign.uri()); + let auth = server(Log::default(), move |_| { + ResponseTemplate::new(status).insert_header("location", location.clone()) + }) + .await; + let client = client(&auth); + for event in [ + Event::Connect, + Event::Revalidate, + Event::End { + reason: Reason::Dropped, + duration: Duration::ZERO, + bytes: Bytes::default(), + }, + ] { + let mut request = request(); + request.query = Some("jwt=secret".into()); + request.event = event; + let result = if matches!(request.event, Event::End { .. }) { + Post::post_end(&client, &request).await + } else { + Post::post(&client, &request).await.map(|_| ()) + }; + assert!( + foreign.received_requests().await.unwrap().is_empty(), + "{status} forwarded {:?} to the redirect destination", + request.event + ); + let err = result.unwrap_err(); + assert!( + matches!(&err, Error::Unavailable(message) if message.contains("redirects are not followed")), + "{status}: {err}" + ); + } + assert_eq!(auth.received_requests().await.unwrap().len(), 3); + } + } + #[tokio::test] async fn http_error_redacts_url() { // A freed port refuses the connection, so reqwest fails with the dialed URL attached. diff --git a/rs/moq-relay/src/auth.rs b/rs/moq-relay/src/auth.rs index 1c659bae416..7d2ff9e86a0 100644 --- a/rs/moq-relay/src/auth.rs +++ b/rs/moq-relay/src/auth.rs @@ -632,7 +632,14 @@ impl Auth { } /// Admit a session: the lease it holds, carrying the scope the origin applies. + /// + /// A path naming a `.` or `..` segment is refused before the decider sees it. A + /// URL transport resolves those away, but an in-band SETUP Path arrives verbatim, + /// and `/tenant-a/../tenant-b` would reach the decider spelled under `tenant-a`. pub async fn admit(&self, request: Request) -> Result { + if navigates(&request.path) { + return Err(Error::Request(format!("path {:?} has a dot segment", request.path))); + } let path = request.path.clone(); let (reply, answer) = oneshot::channel(); self.admissions @@ -655,6 +662,12 @@ impl Auth { } } +/// Whether `path` has a segment URL resolution reads as `.` or `..`, percent-encoded or not. +fn navigates(path: &str) -> bool { + path.split('/') + .any(|segment| matches!(segment.to_ascii_lowercase().replace("%2e", ".").as_str(), "." | "..")) +} + /// The sessions an embedded [`Auth`] is waiting to admit, in arrival order. /// /// The transport has already accepted each one; it waits for its answer, so a @@ -779,6 +792,41 @@ mod tests { assert!(grant.publish.is_empty()); } + #[tokio::test(start_paused = true)] + async fn admit_refuses_dot_segments_before_the_decider() { + let (auth, mut admissions) = Auth::embedded("relay-1"); + for path in [ + "/tenant-a/../tenant-b", + ".", + "/..", + "/a/./b", + "/a/.", + "/%2e", + "/%2E/room", + "/a/.%2e", + "/a/%2E./b", + "/a/%2e%2E/b", + ] { + let result = auth.admit(auth.request(moq_auth::Transport::Quic, path)).await; + assert!( + matches!(result, Err(Error::Request(_))), + "{path} was not refused as a bad path" + ); + assert!(admissions.0.try_recv().is_err(), "{path} reached the decider"); + } + + // Names containing dots keep their exact spelling at the admission boundary. + for path in ["/a/...", "/.hidden", "/a/..b", "/a/%2e-hidden", "/a/%252e"] { + let decide = async { + let admission = admissions.next().await.unwrap(); + assert_eq!(admission.request.path, path); + admission.grant(lease::Consumer::fixed(Grant::new(patterns(&["**"]), patterns(&["**"])))); + }; + let (result, ()) = tokio::join!(auth.admit(auth.request(moq_auth::Transport::Quic, path)), decide); + assert!(result.is_ok(), "{path} was refused"); + } + } + /// The public rules are rooted at `/`, like a token with an empty root. Bare `**` /// reads the same either way, which is how rooting them at the dialed path went /// unnoticed: `anon/**` at `/rooms/123` granted `rooms/123/anon/**`.