diff --git a/tonic-xds/src/client/route.rs b/tonic-xds/src/client/route.rs index 27600d7eb..aa8d22145 100644 --- a/tonic-xds/src/client/route.rs +++ b/tonic-xds/src/client/route.rs @@ -40,6 +40,8 @@ use tower::{BoxError, Layer, Service}; pub(crate) struct RouteInput<'a> { /// The authority (host) of the request URI. pub authority: &'a str, + /// The request path (e.g. `/pkg.Service/Method`), used for path matching. + pub path: &'a str, /// The HTTP headers of the request. These can be used for header-based routing decisions. pub headers: &'a http::HeaderMap, } @@ -210,6 +212,7 @@ where } let route_input = RouteInput { authority: &authority, + path: request.uri().path(), headers: request.headers(), }; router.route(&route_input, &config)? @@ -293,6 +296,77 @@ mod tests { } } + #[tokio::test] + async fn matches_routes_on_the_request_path() { + use crate::xds::cache::XdsCache; + use crate::xds::resource::route_config::{ + PathSpecifierConfig, RouteConfig, RouteConfigAction, RouteConfigMatch, + RouteConfigResource, VirtualHostConfig, + }; + use crate::xds::routing::XdsRouter; + + fn prefix_route(prefix: &str, cluster: &str) -> RouteConfig { + RouteConfig { + match_criteria: RouteConfigMatch { + path_specifier: PathSpecifierConfig::Prefix(prefix.into()), + headers: vec![], + case_sensitive: true, + match_fraction: None, + }, + action: RouteConfigAction::Cluster(cluster.into()), + retry_config: None, + } + } + + let cache = XdsCache::new(); + cache.update_route_config(Arc::new(RouteConfigResource { + name: "rc".into(), + virtual_hosts: vec![VirtualHostConfig { + name: "vh".into(), + domains: vec!["greeter.svc:50051".into()], + // Most specific first; "/" matches anything, so picking it means + // the real path never reached matching. + routes: vec![ + prefix_route("/pkg.Greeter/", "greeter-cluster"), + prefix_route("/", "fallback-cluster"), + ], + }], + metadata: Default::default(), + })); + let xds_router = XdsRouter::new(&cache); + while xds_router.snapshot().is_none() { + tokio::task::yield_now().await; + } + let router: Arc = Arc::new(xds_router); + + let captured: Arc>> = Arc::new(Mutex::new(None)); + let sink = captured.clone(); + let inner = service_fn(move |req: Request<()>| { + let sink = sink.clone(); + async move { + *sink.lock().unwrap() = req + .extensions() + .get::() + .map(|d| d.cluster.clone()); + Ok::<_, BoxError>(http::Response::new(())) + } + }); + let svc = XdsRoutingLayer::new(router, None, Arc::from("greeter.svc:50051")).layer(inner); + + // The URI a tonic-generated client produces. + let req = Request::builder() + .uri("/pkg.Greeter/SayHello") + .body(()) + .unwrap(); + svc.oneshot(req).await.unwrap(); + + assert_eq!( + captured.lock().unwrap().as_deref(), + Some("greeter-cluster"), + "route matching did not see the request path", + ); + } + #[tokio::test] async fn interceptor_runs_when_config_arrives_late() { use crate::xds::cache::XdsCache; diff --git a/tonic-xds/src/xds/resource/mod.rs b/tonic-xds/src/xds/resource/mod.rs index 29ab86fd1..4e7eeadaf 100644 --- a/tonic-xds/src/xds/resource/mod.rs +++ b/tonic-xds/src/xds/resource/mod.rs @@ -41,6 +41,7 @@ pub(crate) mod hash_policy; pub(crate) mod listener; pub(crate) mod outlier_detection; pub(crate) mod route_config; +pub(crate) mod safe_regex; pub(crate) mod san_matcher; pub(crate) mod security; pub(crate) mod string_matcher; diff --git a/tonic-xds/src/xds/resource/route_config.rs b/tonic-xds/src/xds/resource/route_config.rs index ca2cd08c0..a86969f19 100644 --- a/tonic-xds/src/xds/resource/route_config.rs +++ b/tonic-xds/src/xds/resource/route_config.rs @@ -34,12 +34,11 @@ use envoy_types::pb::envoy::config::route::v3::{ RetryPolicy, RouteConfiguration, RouteMatch, route, route_action, route_match, }; use prost::Message; -use regex::Regex; use xds_client::resource::TypeUrl; use xds_client::{Error, Resource}; +use super::safe_regex::SafeRegex; use super::string_matcher::StringMatcher; - /// A `typed_filter_metadata` entry — a `google.protobuf.Any` (a type URL plus an /// encoded message value). #[derive(Debug, Clone)] @@ -276,7 +275,7 @@ pub(crate) struct RouteConfigMatch { pub(crate) enum PathSpecifierConfig { Prefix(String), Path(String), - SafeRegex(Regex), + SafeRegex(SafeRegex), } /// Header matching criteria. @@ -443,7 +442,7 @@ fn validate_route_match(rm: RouteMatch) -> xds_client::Result Some(route_match::PathSpecifier::Prefix(p)) => PathSpecifierConfig::Prefix(p), Some(route_match::PathSpecifier::Path(p)) => PathSpecifierConfig::Path(p), Some(route_match::PathSpecifier::SafeRegex(r)) => { - let re = Regex::new(&r.regex) + let re = SafeRegex::new(&r.regex) .map_err(|e| Error::Validation(format!("invalid path regex '{}': {e}", r.regex)))?; PathSpecifierConfig::SafeRegex(re) } @@ -516,7 +515,7 @@ fn validate_header_matcher( // SafeRegexMatch is deprecated in favor of StringMatch, which is handled below. #[allow(deprecated)] Some(HeaderMatchSpecifier::SafeRegexMatch(r)) => { - let re = Regex::new(&r.regex).map_err(|e| { + let re = SafeRegex::new(&r.regex).map_err(|e| { Error::Validation(format!("invalid header regex '{}': {e}", r.regex)) })?; HeaderMatchSpecifierConfig::String(StringMatcher::SafeRegex(re)) @@ -784,6 +783,89 @@ mod tests { )); } + #[test] + fn safe_regex_path_matcher_requires_a_full_match() { + use envoy_types::pb::envoy::r#type::matcher::v3::RegexMatcher; + + let unanchored_rm = RouteMatch { + path_specifier: Some(route_match::PathSpecifier::SafeRegex(RegexMatcher { + regex: r"/pkg\.Greeter/SayHello".to_string(), + ..Default::default() + })), + ..Default::default() + }; + + let matched = validate_route_match(unanchored_rm).expect("valid regex"); + let PathSpecifierConfig::SafeRegex(re) = matched.path_specifier else { + panic!("expected a SafeRegex path specifier"); + }; + + assert!( + re.is_match("/pkg.Greeter/SayHello"), + "exact path must match" + ); + assert!( + !re.is_match("/pkg.Greeter/SayHelloAgain"), + "a longer method sharing the prefix must not match" + ); + assert!( + !re.is_match("/other.Svc/x/pkg.Greeter/SayHello"), + "the pattern must not match as a substring of a longer path" + ); + } + + #[test] + fn safe_regex_path_matcher_anchors_each_alternation_branch() { + use envoy_types::pb::envoy::r#type::matcher::v3::RegexMatcher; + + let rm = RouteMatch { + path_specifier: Some(route_match::PathSpecifier::SafeRegex(RegexMatcher { + regex: "/a|/b".to_string(), + ..Default::default() + })), + ..Default::default() + }; + + let matched = validate_route_match(rm).expect("valid regex"); + let PathSpecifierConfig::SafeRegex(re) = matched.path_specifier else { + panic!("expected a SafeRegex path specifier"); + }; + + assert!(re.is_match("/a")); + assert!(re.is_match("/b")); + assert!( + !re.is_match("/aX"), + "an alternation branch must not match a longer path" + ); + } + + #[test] + fn safe_regex_header_matcher_requires_a_full_match() { + use envoy_types::pb::envoy::config::route::v3::HeaderMatcher; + use envoy_types::pb::envoy::config::route::v3::header_matcher::HeaderMatchSpecifier; + use envoy_types::pb::envoy::r#type::matcher::v3::RegexMatcher; + + #[allow(deprecated)] + let hm = HeaderMatcher { + name: "x-version".into(), + header_match_specifier: Some(HeaderMatchSpecifier::SafeRegexMatch(RegexMatcher { + regex: "v[0-9]+".into(), + ..Default::default() + })), + ..Default::default() + }; + + let matched = validate_header_matcher(hm).expect("valid regex"); + let HeaderMatchSpecifierConfig::String(m) = matched.match_specifier else { + panic!("expected a string matcher"); + }; + assert!(m.is_match("v2")); + assert!( + !m.is_match("v2-beta"), + "a longer value sharing the prefix must not match" + ); + } + #[test] fn test_cascade_weighted_clusters() { use envoy_types::pb::envoy::config::route::v3::{ diff --git a/tonic-xds/src/xds/resource/safe_regex.rs b/tonic-xds/src/xds/resource/safe_regex.rs new file mode 100644 index 000000000..61cd1eaf9 --- /dev/null +++ b/tonic-xds/src/xds/resource/safe_regex.rs @@ -0,0 +1,184 @@ +/* + * + * Copyright 2025 gRPC authors. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to + * deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + */ + +//! Full-match regex for `envoy.type.matcher.v3.RegexMatcher`. + +use regex::Regex; +use std::fmt; + +/// The anchors added by [`SafeRegex::new`], named so that [`SafeRegex::pattern`] +/// strips back exactly what was added. +const ANCHOR_PREFIX: &str = r"\A(?:"; +const ANCHOR_SUFFIX: &str = r")\z"; + +/// A `RegexMatcher` that matches only the entire input. +/// +/// Envoy requires a full match, but [`Regex::is_match`] searches anywhere in +/// the haystack. The anchors are applied on construction and the inner regex +/// is not exposed, so a substring match is unreachable by construction. +/// +/// Not for `RegexMatchAndSubstitute`, which matches portions of a string and +/// expects callers to supply their own anchors. +#[derive(Clone)] +pub(crate) struct SafeRegex(Regex); + +impl SafeRegex { + /// Compile `pattern` to match only the entire input. + /// + /// The non-capturing group keeps a top-level alternation from escaping the + /// anchors and confines any inline flags the pattern sets; `\A`/`\z` hold + /// regardless of those flags, whereas `^`/`$` become line anchors under + /// `(?m)`. + pub(crate) fn new(pattern: &str) -> Result { + // Splicing is only sound for a pattern that is valid alone: a free `)` + // closes ANCHOR_PREFIX early, leaving `foo)|bar(?:` spliced as the + // unanchored `\A(?:foo)|bar(?:)\z`. + Regex::new(pattern)?; + Regex::new(&format!("{ANCHOR_PREFIX}{pattern}{ANCHOR_SUFFIX}")).map(Self) + } + + /// Returns true if the regex matches `haystack` in its entirety. + pub(crate) fn is_match(&self, haystack: &str) -> bool { + self.0.is_match(haystack) + } + + /// The pattern as received, which is what `Debug` should report. + /// + /// Borrowed back out of the compiled regex rather than stored, since + /// `as_str` already retains the anchored form. + fn pattern(&self) -> &str { + let anchored = self.0.as_str(); + &anchored[ANCHOR_PREFIX.len()..anchored.len() - ANCHOR_SUFFIX.len()] + } +} + +impl fmt::Debug for SafeRegex { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_tuple("SafeRegex").field(&self.pattern()).finish() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn matches_only_the_entire_input() { + let re = SafeRegex::new(r"/pkg\.Greeter/SayHello").unwrap(); + assert!(re.is_match("/pkg.Greeter/SayHello")); + assert!( + !re.is_match("/pkg.Greeter/SayHelloAgain"), + "a longer input sharing the prefix must not match" + ); + assert!( + !re.is_match("/other/x/pkg.Greeter/SayHello"), + "the pattern must not match as a substring" + ); + } + + #[test] + fn anchors_every_alternation_branch() { + let re = SafeRegex::new("/a|/b").unwrap(); + assert!(re.is_match("/a")); + assert!(re.is_match("/b")); + assert!( + !re.is_match("/aX"), + "a top-level alternation must not escape the anchors" + ); + } + + #[test] + fn does_not_match_a_trailing_newline() { + let re = SafeRegex::new("/a").unwrap(); + assert!(!re.is_match("/a\n")); + } + + #[test] + fn inline_flags_cannot_widen_the_anchors() { + // A control plane could set `(?m)`; the anchors must still bind to the + // whole haystack rather than to a line within it. + let re = SafeRegex::new("(?m)/a").unwrap(); + assert!(!re.is_match("x\n/a\ny")); + } + + #[test] + fn an_invalid_pattern_is_rejected() { + assert!(SafeRegex::new("(unclosed").is_err()); + } + + #[test] + fn splicing_into_the_anchors_is_rejected() { + for pattern in ["foo)|bar(?:", "foo)(?:bar", ")(?:", "a)(?:b", r")\"] { + assert!( + SafeRegex::new(pattern).is_err(), + "{pattern:?} is not valid alone and must not be accepted" + ); + } + } + + #[test] + fn commenting_out_the_anchors_is_rejected() { + assert!(Regex::new("(?x)#").is_ok(), "valid on its own"); + assert!(SafeRegex::new("(?x)#").is_err()); + } + + #[test] + fn realistic_patterns_full_match() { + for (pattern, matches, rejects) in [ + (".*", "anything", "a\nb"), + ("/a|/b", "/b", "/bX"), + ("(?i)/Foo", "/fOO", "/Foo/bar"), + ("v[0-9]+", "v12", "v12a"), + ("[)]", ")", "))"), + (r"\)", ")", "x)"), + ("(?:a|b)+", "abab", "abc"), + ("/caf€/.*", "/caf€/x", "y/caf€/x"), + ( + r"/pkg\.[A-Za-z]+/.*", + "/pkg.Greeter/SayHello", + "/pkg.Greeter", + ), + ] { + let re = SafeRegex::new(pattern) + .unwrap_or_else(|e| panic!("{pattern:?} should compile: {e}")); + assert!(re.is_match(matches), "{pattern:?} should match {matches:?}"); + assert!( + !re.is_match(rejects), + "{pattern:?} should not match {rejects:?}" + ); + } + } + + #[test] + fn debug_reports_the_original_pattern() { + let re = SafeRegex::new("/a|/b").unwrap(); + assert_eq!(format!("{re:?}"), r#"SafeRegex("/a|/b")"#); + } + + #[test] + fn the_original_pattern_survives_multibyte_characters() { + let re = SafeRegex::new("/caf€/.*").unwrap(); + assert_eq!(format!("{re:?}"), r#"SafeRegex("/caf€/.*")"#); + } +} diff --git a/tonic-xds/src/xds/resource/string_matcher.rs b/tonic-xds/src/xds/resource/string_matcher.rs index 8df8907e8..3a359f6f2 100644 --- a/tonic-xds/src/xds/resource/string_matcher.rs +++ b/tonic-xds/src/xds/resource/string_matcher.rs @@ -31,9 +31,9 @@ //! Used wherever an xDS config carries a `StringMatcher` — HTTP header matching //! (gRFC A28) and SAN matching for server authorization (gRFC A29). +use super::safe_regex::SafeRegex; use envoy_types::pb::envoy::r#type::matcher::v3::StringMatcher as StringMatcherProto; use envoy_types::pb::envoy::r#type::matcher::v3::string_matcher::MatchPattern; -use regex::Regex; use xds_client::Error; /// Validated `envoy.type.matcher.v3.StringMatcher`. @@ -43,7 +43,7 @@ pub(crate) enum StringMatcher { Prefix { value: String, ignore_case: bool }, Suffix { value: String, ignore_case: bool }, Contains { value: String, ignore_case: bool }, - SafeRegex(Regex), + SafeRegex(SafeRegex), } impl StringMatcher { @@ -59,7 +59,7 @@ impl StringMatcher { Some(MatchPattern::Suffix(value)) => Ok(Self::Suffix { value, ignore_case }), Some(MatchPattern::Contains(value)) => Ok(Self::Contains { value, ignore_case }), Some(MatchPattern::SafeRegex(r)) => { - let re = Regex::new(&r.regex) + let re = SafeRegex::new(&r.regex) .map_err(|e| Error::Validation(format!("invalid regex '{}': {e}", r.regex)))?; Ok(Self::SafeRegex(re)) } @@ -224,6 +224,27 @@ mod tests { assert!(!m.is_match("xfoo123")); } + #[test] + fn safe_regex_requires_a_full_match() { + let m = StringMatcher::from_proto(proto( + MatchPattern::SafeRegex(RegexMatcher { + regex: "spiffe://td/ns/prod/sa/api".into(), + ..Default::default() + }), + false, + )) + .unwrap(); + assert!(m.is_match("spiffe://td/ns/prod/sa/api")); + assert!( + !m.is_match("spiffe://td/ns/prod/sa/api-canary"), + "a longer SAN sharing the prefix must not match" + ); + assert!( + !m.is_match("spiffe://evil/x?=spiffe://td/ns/prod/sa/api"), + "the pattern must not match as a substring of a longer SAN" + ); + } + #[test] fn safe_regex_invalid_is_rejected() { let err = StringMatcher::from_proto(proto( diff --git a/tonic-xds/src/xds/routing.rs b/tonic-xds/src/xds/routing.rs index 4a43e9d9b..7efa48538 100644 --- a/tonic-xds/src/xds/routing.rs +++ b/tonic-xds/src/xds/routing.rs @@ -128,20 +128,17 @@ impl Router for XdsRouter { input: &RouteInput<'_>, config: &RoutingSnapshot, ) -> Result { - resolve_route(config, input.authority, input.headers) + resolve_route(config, input.authority, input.path, input.headers) } } -/// Resolve a route decision from the given config, authority, and headers. +/// Resolve a route decision from the given config, authority, path, and headers. fn resolve_route( rc: &RoutingSnapshot, authority: &str, + path: &str, headers: &http::HeaderMap, ) -> Result { - let path = headers - .get(":path") - .and_then(|v| v.to_str().ok()) - .unwrap_or("/"); let route = rc.matched_route(authority, path, headers)?; let cluster = match &route.action { RouteConfigAction::Cluster(name) => name.clone(), @@ -403,6 +400,7 @@ mod tests { use crate::xds::resource::route_config::{ RouteConfig, RouteConfigAction, RouteConfigMatch, VirtualHostConfig, }; + use crate::xds::resource::safe_regex::SafeRegex; use crate::xds::resource::string_matcher::StringMatcher; fn simple_route(prefix: &str, cluster: &str) -> RouteConfig { @@ -664,7 +662,7 @@ mod tests { routes: vec![RouteConfig { match_criteria: RouteConfigMatch { path_specifier: PathSpecifierConfig::SafeRegex( - regex::Regex::new("^/svc/.*").unwrap(), + SafeRegex::new("/svc/.*").unwrap(), ), headers: vec![], case_sensitive: true, @@ -1037,7 +1035,7 @@ mod tests { headers: vec![HeaderMatcherConfig { name: "x-tag".into(), match_specifier: HeaderMatchSpecifierConfig::String( - StringMatcher::SafeRegex(regex::Regex::new("^v[0-9]+$").unwrap()), + StringMatcher::SafeRegex(SafeRegex::new("v[0-9]+").unwrap()), ), invert_match: false, }], @@ -1124,6 +1122,7 @@ mod tests { let headers = http::HeaderMap::new(); let input = RouteInput { authority: "my-service", + path: "/", headers: &headers, }; let config = router.snapshot().expect("config"); @@ -1142,6 +1141,7 @@ mod tests { let headers = http::HeaderMap::new(); let input = RouteInput { authority: "svc", + path: "/", headers: &headers, };