From 9ebfc85c55b6d49ca8ab144a3848a66fce6c6e8d Mon Sep 17 00:00:00 2001 From: Lars Francke Date: Thu, 22 Oct 2026 18:32:31 +0200 Subject: [PATCH 1/3] fix: Encode user ids and names as single URL path segments in UIF The Keycloak and Entra backends joined request-supplied user ids and names onto the base URL as text, so `/`, `?`, `#` and `..` in them changed which URL was requested. Both now append them as one percent-encoded path segment through `append_path_segments`, and the Keycloak backend sends the username as a query parameter. An empty, `.` or `..` segment and one containing a line break are rejected with `400`, as the url crate would drop or strip them and look up a different user. A Keycloak `rootPath` without a trailing slash is now kept, which `Url::join` dropped. --- CHANGELOG.md | 5 + Cargo.lock | 4 + Cargo.toml | 1 + rust/info-fetcher-commons/Cargo.toml | 3 + rust/info-fetcher-commons/src/utils/mod.rs | 1 + .../src/utils/url_path.rs | 133 ++++++++++ rust/user-info-fetcher/Cargo.toml | 1 + rust/user-info-fetcher/src/backend/entra.rs | 135 ++++++++-- .../user-info-fetcher/src/backend/keycloak.rs | 231 +++++++++++++++--- rust/user-info-fetcher/src/backend/mod.rs | 61 ++++- 10 files changed, 519 insertions(+), 56 deletions(-) create mode 100644 rust/info-fetcher-commons/src/utils/url_path.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f6e9786..26581016 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -92,6 +92,10 @@ All notable changes to this project will be documented in this file. See [our internal issue](https://github.com/stackabletech/hdfs-operator/issues/626) and [the fix](https://github.com/kube-rs/kube/pull/2042) for details ([#871]). - The reconciler now applies resources and derives the cluster status in discrete apply and update_status steps for the `opa_controller` ([#872]). +- The user-info-fetcher Keycloak and Entra backends now encode user ids and names as a single URL + path segment, and the Keycloak backend sends the username as a query parameter. An empty, `.` or + `..` id or name, or one containing a line break, is rejected with `400` ([#909]). +- The user-info-fetcher Keycloak backend now keeps a `rootPath` without a trailing slash ([#909]). [#873]: https://github.com/stackabletech/opa-operator/pull/873 [#852]: https://github.com/stackabletech/opa-operator/pull/852 @@ -113,6 +117,7 @@ All notable changes to this project will be documented in this file. [#905]: https://github.com/stackabletech/opa-operator/pull/905 [#906]: https://github.com/stackabletech/opa-operator/pull/906 [#908]: https://github.com/stackabletech/opa-operator/pull/908 +[#909]: https://github.com/stackabletech/opa-operator/pull/909 ## [26.7.0] - 2026-07-21 diff --git a/Cargo.lock b/Cargo.lock index 79c42d6c..f33216fd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1708,7 +1708,9 @@ dependencies = [ "hyper", "moka", "native-tls", + "percent-encoding", "reqwest", + "rstest", "rustls-pki-types", "serde", "serde_json", @@ -1717,6 +1719,7 @@ dependencies = [ "stackable-operator", "tokio", "tracing", + "url", ] [[package]] @@ -3782,6 +3785,7 @@ dependencies = [ "krb5", "ldap3", "native-tls", + "percent-encoding", "pin-project", "reqwest", "rustls-pki-types", diff --git a/Cargo.toml b/Cargo.toml index 63fbe0aa..511dedc1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,6 +29,7 @@ ldap3 = { version = "0.12", features = ["gssapi", "tls"] } md5 = "0.8" moka = { version = "0.12", default-features = false, features = ["future"] } native-tls = "0.2.12" +percent-encoding = "2.3" pin-project = "1.1" # `default-features = false` drops reqwest 0.13's `default-tls = ["rustls"]`; we select # `native-tls` (OpenSSL) to stay on the same TLS stack as ldap3 (see utils/tls.rs). The diff --git a/rust/info-fetcher-commons/Cargo.toml b/rust/info-fetcher-commons/Cargo.toml index bb8e82e7..3909cbba 100644 --- a/rust/info-fetcher-commons/Cargo.toml +++ b/rust/info-fetcher-commons/Cargo.toml @@ -23,6 +23,9 @@ serde_json.workspace = true snafu.workspace = true tracing.workspace = true tokio.workspace = true +url.workspace = true [dev-dependencies] futures.workspace = true +percent-encoding.workspace = true +rstest.workspace = true diff --git a/rust/info-fetcher-commons/src/utils/mod.rs b/rust/info-fetcher-commons/src/utils/mod.rs index 46cf2b0a..8045b5d7 100644 --- a/rust/info-fetcher-commons/src/utils/mod.rs +++ b/rust/info-fetcher-commons/src/utils/mod.rs @@ -3,3 +3,4 @@ pub mod http; pub mod secret; pub mod tls; pub mod token; +pub mod url_path; diff --git a/rust/info-fetcher-commons/src/utils/url_path.rs b/rust/info-fetcher-commons/src/utils/url_path.rs new file mode 100644 index 00000000..def99793 --- /dev/null +++ b/rust/info-fetcher-commons/src/utils/url_path.rs @@ -0,0 +1,133 @@ +use snafu::Snafu; +use url::Url; + +#[derive(Snafu, Debug)] +pub enum Error { + #[snafu(display("unable to use {segment:?} as a URL path segment"))] + InvalidPathSegment { segment: String }, + + #[snafu(display("unable to append path segments to {url:?}, as it cannot be a base"))] + CannotBeABase { url: Url }, +} + +/// Returns `base` with each of `segments` appended as exactly one path segment. +/// +/// Use this rather than [`Url::join`] or [`Url::set_path`] to create URLs from user/untrusted input. +/// Both those methods follow the specs but are easy to use insecurely because e.g. if you provide +/// an absolute URL they replace the host from `base` and a few other things (`../` treatment). +/// +/// Fails on an empty, `.` or `..` segment and on a segment containing a tab or line break. +pub fn append_path_segments(base: &Url, segments: &[&str]) -> Result { + let invalid_segment = segments.iter().find(|segment| { + matches!(**segment, "" | "." | "..") || segment.contains(['\t', '\n', '\r']) + }); + if let Some(segment) = invalid_segment { + return InvalidPathSegmentSnafu { segment: *segment }.fail(); + } + + let mut url = base.clone(); + url.path_segments_mut() + .map_err(|()| CannotBeABaseSnafu { url: base.clone() }.build())? + .pop_if_empty() + .extend(segments); + + Ok(url) +} + +#[cfg(test)] +mod tests { + use percent_encoding::percent_decode_str; + use rstest::rstest; + + use super::*; + + #[rstest] + #[case::root("http://keycloak:8080", "http://keycloak:8080/realms/my-realm")] + #[case::root_with_slash("http://keycloak:8080/", "http://keycloak:8080/realms/my-realm")] + #[case::root_path( + "http://keycloak:8080/auth", + "http://keycloak:8080/auth/realms/my-realm" + )] + #[case::root_path_with_slash( + "http://keycloak:8080/auth/", + "http://keycloak:8080/auth/realms/my-realm" + )] + fn append_keeps_the_base_path(#[case] base: &str, #[case] expected: &str) { + let base = Url::parse(base).expect("valid base URL"); + let url = append_path_segments(&base, &["realms", "my-realm"]).expect("valid segments"); + + assert_eq!(url.as_str(), expected); + } + + #[rstest] + #[case::absolute_url("https://evil.example.com/", "https:%2F%2Fevil.example.com%2F")] + #[case::protocol_relative("//evil.example.com", "%2F%2Fevil.example.com")] + #[case::traversal("../../realms/master", "..%2F..%2Frealms%2Fmaster")] + #[case::percent_encoded_dots("%2e%2e", "%252e%252e")] + #[case::backslash("..\\admin", "..%5Cadmin")] + #[case::query("alice?exact=false", "alice%3Fexact=false")] + #[case::fragment("alice#", "alice%23")] + #[case::guest_principal_name( + "alice_example.com#EXT#@contoso.com", + "alice_example.com%23EXT%23@contoso.com" + )] + fn append_encodes_a_hostile_segment(#[case] segment: &str, #[case] expected: &str) { + let base = Url::parse("http://keycloak:8080/users").expect("valid base URL"); + let url = append_path_segments(&base, &[segment]).expect("valid segment"); + + assert_eq!( + url.as_str(), + format!("http://keycloak:8080/users/{expected}") + ); + } + + /// The URL is sent as a string and parsed again by the server, so every character has to stay + /// inside its segment after a round trip, not only in the [`Url`] built here. + #[test] + fn append_keeps_every_character_inside_its_segment() { + let base = Url::parse("http://keycloak:8080/users").expect("valid base URL"); + let characters = (' '..='~').chain(['\0', '\x1b', '\x7f', 'ä', '€', '🦀']); + + for character in characters { + let segment = format!("a{character}b"); + let url = append_path_segments(&base, &[&segment]).expect("valid segment"); + let sent = Url::parse(url.as_str()).expect("the built URL parses"); + + assert_eq!(sent, url, "{segment:?} changes in a round trip"); + assert_eq!(sent.host_str(), Some("keycloak"), "{segment:?}"); + assert_eq!(sent.query(), None, "{segment:?}"); + assert_eq!(sent.fragment(), None, "{segment:?}"); + let sent_segments: Vec<&str> = sent.path_segments().into_iter().flatten().collect(); + let [_, sent_segment] = sent_segments.as_slice() else { + panic!("{segment:?} splits the path into {sent_segments:?}"); + }; + assert_eq!( + percent_decode_str(sent_segment).decode_utf8_lossy(), + segment, + "{segment:?} changes on its way to the server" + ); + } + } + + #[rstest] + #[case::empty("")] + #[case::current(".")] + #[case::parent("..")] + #[case::tab("ad\tmin")] + #[case::line_feed("ad\nmin")] + #[case::carriage_return("ad\rmin")] + fn append_rejects_a_segment_the_url_would_change(#[case] segment: &str) { + let base = Url::parse("http://keycloak:8080/users").expect("valid base URL"); + let error = append_path_segments(&base, &["admin", segment]).expect_err("rejected"); + + assert!(matches!(error, Error::InvalidPathSegment { .. }), "{error}"); + } + + #[test] + fn append_rejects_a_base_without_a_path() { + let base = Url::parse("mailto:admin@example.com").expect("valid base URL"); + let error = append_path_segments(&base, &["users"]).expect_err("rejected"); + + assert!(matches!(error, Error::CannotBeABase { .. }), "{error}"); + } +} diff --git a/rust/user-info-fetcher/Cargo.toml b/rust/user-info-fetcher/Cargo.toml index eb87ff70..2a3bee60 100644 --- a/rust/user-info-fetcher/Cargo.toml +++ b/rust/user-info-fetcher/Cargo.toml @@ -35,6 +35,7 @@ url.workspace = true uuid.workspace = true [dev-dependencies] +percent-encoding.workspace = true wiremock.workspace = true [build-dependencies] diff --git a/rust/user-info-fetcher/src/backend/entra.rs b/rust/user-info-fetcher/src/backend/entra.rs index 0a630161..7bdb0307 100644 --- a/rust/user-info-fetcher/src/backend/entra.rs +++ b/rust/user-info-fetcher/src/backend/entra.rs @@ -7,6 +7,7 @@ use info_fetcher_commons::utils::{ http::{is_unauthorized, send_json_request}, secret::Secret, token::{CachedToken, MintedToken, OAuthResponse}, + url_path::append_path_segments, }; use serde::Deserialize; use snafu::{ResultExt, Snafu}; @@ -54,6 +55,12 @@ pub enum Error { endpoint: String, }, + #[snafu(display("failed to construct the Graph URL for user {user:?}"))] + ConstructUserUrl { + source: utils::url_path::Error, + user: String, + }, + #[snafu(display("failed to construct HTTP client"))] ConstructHttpClient { source: reqwest::Error }, @@ -77,6 +84,11 @@ impl http_error::Error for Error { Self::UserNotFoundByName { .. } => StatusCode::NOT_FOUND, Self::RequestUserGroups { .. } => StatusCode::BAD_GATEWAY, Self::BuildEntraEndpoint { .. } => StatusCode::BAD_REQUEST, + Self::ConstructUserUrl { + source: utils::url_path::Error::InvalidPathSegment { .. }, + .. + } => StatusCode::BAD_REQUEST, + Self::ConstructUserUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::ConstructHttpClient { .. } => StatusCode::SERVICE_UNAVAILABLE, Self::ConfigureTls { .. } => StatusCode::SERVICE_UNAVAILABLE, Self::ReadClientId { .. } => StatusCode::SERVICE_UNAVAILABLE, @@ -243,7 +255,7 @@ impl ResolvedEntraBackend { let user_id = req.id.clone(); send_json_request::( self.http_client - .get(entra_backend.user_info(&user_id)) + .get(entra_backend.user_info(&user_id)?) .bearer_auth(access_token.expose()), ) .await @@ -260,7 +272,7 @@ impl ResolvedEntraBackend { let username = req.username.clone(); send_json_request::( self.http_client - .get(entra_backend.user_info(&username)) + .get(entra_backend.user_info(&username)?) .bearer_auth(access_token.expose()), ) .await @@ -274,7 +286,7 @@ impl ResolvedEntraBackend { }; let mut groups = Vec::new(); - let mut next_url = Some(entra_backend.group_info(&user_info.id)); + let mut next_url = Some(entra_backend.group_info(&user_info.id)?); let mut pages_remaining = MAX_GROUP_PAGES; while let Some(url) = next_url { @@ -356,22 +368,24 @@ impl EntraBackend { } // Works both with id/oid and userPrincipalName - pub fn user_info(&self, user: &str) -> Url { - let mut user_info_url = self.user_info_endpoint_url.clone(); - user_info_url.set_path(&format!("/v1.0/users/{user}")); - user_info_url + pub fn user_info(&self, user: &str) -> Result { + append_path_segments(&self.user_info_endpoint_url, &["v1.0", "users", user]) + .context(ConstructUserUrlSnafu { user }) } /// Requests the first page of the user's directory memberships. /// /// `memberOf` returns every directory object the user belongs to: groups as well as directory /// roles and administrative units. - pub fn group_info(&self, user: &str) -> Url { - let mut user_info_url = self.user_info_endpoint_url.clone(); - user_info_url.set_path(&format!("/v1.0/users/{user}/memberOf")); + pub fn group_info(&self, user: &str) -> Result { + let mut group_info_url = append_path_segments( + &self.user_info_endpoint_url, + &["v1.0", "users", user, "memberOf"], + ) + .context(ConstructUserUrlSnafu { user })?; // 999 is the largest page size Graph accepts, and keeps the number of round-trips down. - user_info_url.set_query(Some("$select=displayName&$top=999")); - user_info_url + group_info_url.set_query(Some("$select=displayName&$top=999")); + Ok(group_info_url) } /// Rebases an `@odata.nextLink` onto the configured user info endpoint. @@ -402,7 +416,10 @@ mod tests { }; use super::*; - use crate::UserInfoRequestById; + use crate::{ + UserInfoRequestById, + backend::test_mocks::{HOSTILE_USERS, assert_looked_up_below, received_lookups}, + }; const TENANT_ID: &str = "1234-5678-1234-5678"; const USER_ID: &str = "8765-4321-8765-4321"; @@ -479,10 +496,13 @@ mod tests { .await; } - async fn look_up_by_id(backend: &ResolvedEntraBackend) -> Result { + async fn look_up_by_id( + backend: &ResolvedEntraBackend, + id: &str, + ) -> Result { backend .get_user_info(&UserInfoRequest::UserInfoRequestById(UserInfoRequestById { - id: USER_ID.to_owned(), + id: id.to_owned(), })) .await } @@ -507,7 +527,7 @@ mod tests { let mock_server = MockServer::start().await; mock_token_and_failing_user(&mock_server, USER_ID, 404).await; - let error = look_up_by_id(&backend_for(&mock_server)) + let error = look_up_by_id(&backend_for(&mock_server), USER_ID) .await .expect_err("a user Entra does not have cannot be looked up"); @@ -527,7 +547,7 @@ mod tests { let mock_server = MockServer::start().await; mock_token_and_failing_user(&mock_server, USER_ID, status).await; - let error = look_up_by_id(&backend_for(&mock_server)) + let error = look_up_by_id(&backend_for(&mock_server), USER_ID) .await .expect_err("a failing lookup surfaces as an error"); @@ -560,6 +580,79 @@ mod tests { ); } + /// Mounts the token endpoint plus an answer of `404` to every lookup. + async fn mock_token_and_no_users(mock_server: &MockServer) { + mock_token(mock_server, Some(3600), 1).await; + Mock::given(method("GET")) + .respond_with(ResponseTemplate::new(404)) + .mount(mock_server) + .await; + } + + /// A request-supplied id stays one segment under `/v1.0/users/`, so `../` cannot reach another + /// Graph endpoint with the access token. + #[tokio::test] + async fn entra_keeps_a_hostile_id_inside_the_users_endpoint() { + for &user_id in HOSTILE_USERS { + let mock_server = MockServer::start().await; + mock_token_and_no_users(&mock_server).await; + + let error = look_up_by_id(&backend_for(&mock_server), user_id) + .await + .expect_err("Entra does not have the user"); + + assert!( + matches!(error, Error::UserNotFoundById { .. }), + "for {user_id:?} got {error}" + ); + assert_looked_up_below(&mock_server, "/v1.0/users/", user_id).await; + } + } + + /// Graph takes the principal name in the same path segment as the id, so it needs the same + /// protection. + #[tokio::test] + async fn entra_keeps_a_hostile_username_inside_the_users_endpoint() { + for &username in HOSTILE_USERS { + let mock_server = MockServer::start().await; + mock_token_and_no_users(&mock_server).await; + + let error = look_up_by_name(&backend_for(&mock_server), username) + .await + .expect_err("Entra does not have the user"); + + assert!( + matches!(error, Error::UserNotFoundByName { .. }), + "for {username:?} got {error}" + ); + assert_looked_up_below(&mock_server, "/v1.0/users/", username).await; + } + } + + /// An empty, `.` or `..` user would be dropped from the path, turning the lookup of one user + /// into a listing of all of them, and a line break would be stripped, looking up another user. + #[tokio::test] + async fn entra_rejects_a_user_that_cannot_be_a_path_segment() { + for username in ["", ".", "..", "ad\nmin"] { + let mock_server = MockServer::start().await; + mock_token(&mock_server, Some(3600), 1).await; + + let error = look_up_by_name(&backend_for(&mock_server), username) + .await + .expect_err("the user is rejected before Entra is asked"); + + assert!( + matches!(error, Error::ConstructUserUrl { .. }), + "for {username:?} got {error}" + ); + assert_eq!( + http_error::Error::status_code(&error), + StatusCode::BAD_REQUEST + ); + assert!(received_lookups(&mock_server).await.is_empty()); + } + } + async fn get_user_info_by_id(backend: &ResolvedEntraBackend) -> UserInfo { backend .get_user_info(&UserInfoRequest::UserInfoRequestById(UserInfoRequestById { @@ -723,11 +816,11 @@ mod tests { .unwrap() ); assert_eq!( - entra.user_info(user), + entra.user_info(user).expect("valid user"), Url::parse(&format!("https://graph.microsoft.com/v1.0/users/{user}")).unwrap() ); assert_eq!( - entra.group_info(user), + entra.group_info(user).expect("valid user"), Url::parse(&format!( "https://graph.microsoft.com/v1.0/users/{user}/memberOf?$select=displayName&$top=999" )) @@ -763,11 +856,11 @@ mod tests { .unwrap() ); assert_eq!( - entra.user_info(user), + entra.user_info(user).expect("valid user"), Url::parse(&format!("http://graph.mock.com:8080/v1.0/users/{user}")).unwrap() ); assert_eq!( - entra.group_info(user), + entra.group_info(user).expect("valid user"), Url::parse(&format!( "http://graph.mock.com:8080/v1.0/users/{user}/memberOf?$select=displayName&$top=999" )) diff --git a/rust/user-info-fetcher/src/backend/keycloak.rs b/rust/user-info-fetcher/src/backend/keycloak.rs index 0c4f440e..15ade271 100644 --- a/rust/user-info-fetcher/src/backend/keycloak.rs +++ b/rust/user-info-fetcher/src/backend/keycloak.rs @@ -7,6 +7,7 @@ use info_fetcher_commons::utils::{ http::{is_unauthorized, send_json_request}, secret::Secret, token::{CachedToken, MintedToken, OAuthResponse}, + url_path::append_path_segments, }; use serde::Deserialize; use snafu::{OptionExt, ResultExt, Snafu}; @@ -52,7 +53,13 @@ pub enum Error { ParseOidcEndpointUrl { source: oidc::v1alpha1::Error }, #[snafu(display("failed to construct OIDC endpoint path"))] - ConstructOidcEndpointPath { source: url::ParseError }, + ConstructOidcEndpointPath { source: utils::url_path::Error }, + + #[snafu(display("failed to construct the Keycloak URL for user id {user_id:?}"))] + ConstructUserUrl { + source: utils::url_path::Error, + user_id: String, + }, #[snafu(display("failed to construct HTTP client"))] ConstructHttpClient { source: reqwest::Error }, @@ -79,6 +86,11 @@ impl http_error::Error for Error { Self::RequestUserGroups { .. } => StatusCode::BAD_GATEWAY, Self::ParseOidcEndpointUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::ConstructOidcEndpointPath { .. } => StatusCode::INTERNAL_SERVER_ERROR, + Self::ConstructUserUrl { + source: utils::url_path::Error::InvalidPathSegment { .. }, + .. + } => StatusCode::BAD_REQUEST, + Self::ConstructUserUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::ConstructHttpClient { .. } => StatusCode::SERVICE_UNAVAILABLE, Self::ConfigureTls { .. } => StatusCode::SERVICE_UNAVAILABLE, Self::ReadClientId { .. } => StatusCode::SERVICE_UNAVAILABLE, @@ -216,16 +228,15 @@ impl ResolvedKeycloakBackend { client_secret: Secret, ) -> Result { let admin_realm = &self.config.admin_realm; + let token_url = append_path_segments( + keycloak_url, + &["realms", admin_realm, "protocol", "openid-connect", "token"], + ) + .context(ConstructOidcEndpointPathSnafu)?; let response = send_json_request::( self.http_client - .post( - keycloak_url - .join(&format!( - "realms/{admin_realm}/protocol/openid-connect/token" - )) - .context(ConstructOidcEndpointPathSnafu)?, - ) + .post(token_url) .basic_auth(&self.client_id, Some(client_secret.expose())) .form(&[("grant_type", "client_credentials")]), ) @@ -244,21 +255,19 @@ impl ResolvedKeycloakBackend { access_token: Secret, ) -> Result { let user_realm = &self.config.user_realm; - - let users_base_url = keycloak_url - .join(&format!("admin/realms/{user_realm}/users/")) - .context(ConstructOidcEndpointPathSnafu)?; + let users_url = + append_path_segments(keycloak_url, &["admin", "realms", user_realm, "users"]) + .context(ConstructOidcEndpointPathSnafu)?; let user_info = match req { UserInfoRequest::UserInfoRequestById(req) => { let user_id = req.id.clone(); + let user_url = append_path_segments(&users_url, &[&user_id]) + .context(ConstructUserUrlSnafu { user_id: &user_id })?; + send_json_request::( self.http_client - .get( - users_base_url - .join(&req.id) - .context(ConstructOidcEndpointPathSnafu)?, - ) + .get(user_url) .bearer_auth(access_token.expose()), ) .await @@ -273,13 +282,10 @@ impl ResolvedKeycloakBackend { } UserInfoRequest::UserInfoRequestByName(req) => { let username = &req.username; - let users_url = users_base_url - .join(&format!("?username={username}&exact=true")) - .context(ConstructOidcEndpointPathSnafu)?; - let users = send_json_request::>( self.http_client - .get(users_url) + .get(users_url.clone()) + .query(&[("username", username.as_str()), ("exact", "true")]) .bearer_auth(access_token.expose()), ) .await @@ -296,13 +302,15 @@ impl ResolvedKeycloakBackend { } }; + let groups_url = append_path_segments(&users_url, &[&user_info.id, "groups"]).context( + ConstructUserUrlSnafu { + user_id: &user_info.id, + }, + )?; + let groups = send_json_request::>( self.http_client - .get( - users_base_url - .join(&format!("{}/groups", user_info.id)) - .context(ConstructOidcEndpointPathSnafu)?, - ) + .get(groups_url) .bearer_auth(access_token.expose()), ) .await @@ -334,7 +342,13 @@ mod tests { }; use super::*; - use crate::{UserInfoRequestById, backend::keycloak::ResolvedKeycloakBackend}; + use crate::{ + UserInfoRequestById, UserInfoRequestByName, + backend::{ + keycloak::ResolvedKeycloakBackend, + test_mocks::{HOSTILE_USERS, assert_looked_up_below, received_lookups}, + }, + }; const ADMIN_REALM: &str = "master"; const USER_REALM: &str = "my-realm"; @@ -395,14 +409,28 @@ mod tests { async fn get_user_info_by_id( backend: &ResolvedKeycloakBackend, + id: &str, ) -> Result { backend .get_user_info(&UserInfoRequest::UserInfoRequestById(UserInfoRequestById { - id: USER_ID.to_owned(), + id: id.to_owned(), })) .await } + async fn get_user_info_by_name( + backend: &ResolvedKeycloakBackend, + username: &str, + ) -> Result { + backend + .get_user_info(&UserInfoRequest::UserInfoRequestByName( + UserInfoRequestByName { + username: username.to_owned(), + }, + )) + .await + } + /// The access token was minted for every single lookup, doubling the round trips. Keycloak reports /// how long it is valid for, so it only has to be minted once. #[tokio::test] @@ -412,8 +440,12 @@ mod tests { mock_user_and_groups(&mock_server, 200).await; let backend = backend_for(&mock_server); - get_user_info_by_id(&backend).await.expect("lookup works"); - get_user_info_by_id(&backend).await.expect("lookup works"); + get_user_info_by_id(&backend, USER_ID) + .await + .expect("lookup works"); + get_user_info_by_id(&backend, USER_ID) + .await + .expect("lookup works"); // The token endpoint's `.expect(1)` is verified when `mock_server` is dropped. } @@ -426,7 +458,7 @@ mod tests { mock_token(&mock_server, Some(3600), 1).await; mock_user_and_groups(&mock_server, 404).await; - let error = get_user_info_by_id(&backend_for(&mock_server)) + let error = get_user_info_by_id(&backend_for(&mock_server), USER_ID) .await .expect_err("a user Keycloak does not have cannot be looked up"); @@ -447,7 +479,7 @@ mod tests { mock_token(&mock_server, Some(3600), 1).await; mock_user_and_groups(&mock_server, status).await; - let error = get_user_info_by_id(&backend_for(&mock_server)) + let error = get_user_info_by_id(&backend_for(&mock_server), USER_ID) .await .expect_err("a failing lookup surfaces as an error"); @@ -462,6 +494,137 @@ mod tests { } } + /// A request-supplied id stays one segment under `users/`. + /// An absolute URL must never replace the host. + /// And `../` is also not allowed to point the admin token at another endpoint. + #[tokio::test] + async fn keycloak_keeps_a_hostile_id_inside_the_users_endpoint() { + for &user_id in HOSTILE_USERS { + let mock_server = MockServer::start().await; + mock_token(&mock_server, Some(3600), 1).await; + Mock::given(method("GET")) + .respond_with(ResponseTemplate::new(404)) + .mount(&mock_server) + .await; + + let error = get_user_info_by_id(&backend_for(&mock_server), user_id) + .await + .expect_err("Keycloak does not have the user"); + + assert!( + matches!(error, Error::UserNotFoundById { .. }), + "for {user_id:?} got {error}" + ); + let users_path = format!("/admin/realms/{USER_REALM}/users/"); + assert_looked_up_below(&mock_server, &users_path, user_id).await; + } + } + + /// A request-supplied username stays the value of the `username` query parameter, so it cannot + /// switch off the exact match or add parameters. + #[tokio::test] + async fn keycloak_keeps_a_hostile_username_inside_its_query_parameter() { + for &username in HOSTILE_USERS { + let mock_server = MockServer::start().await; + mock_token(&mock_server, Some(3600), 1).await; + Mock::given(method("GET")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(&mock_server) + .await; + + let error = get_user_info_by_name(&backend_for(&mock_server), username) + .await + .expect_err("Keycloak does not have the user"); + + assert!( + matches!(error, Error::UserNotFoundByName { .. }), + "for {username:?} got {error}" + ); + let lookups = received_lookups(&mock_server).await; + let [lookup] = lookups.as_slice() else { + panic!("{username:?} was not looked up exactly once: {lookups:?}"); + }; + let query: Vec<(String, String)> = lookup.url.query_pairs().into_owned().collect(); + assert_eq!( + lookup.url.path(), + format!("/admin/realms/{USER_REALM}/users"), + "{username:?}" + ); + assert_eq!( + query, + [ + ("username".to_owned(), username.to_owned()), + ("exact".to_owned(), "true".to_owned()), + ], + "{username:?}" + ); + } + } + + /// An empty, `.` or `..` id would be dropped from the path, turning the lookup of one user into a + /// listing of all of them, and a line break would be stripped, looking up another user. + #[tokio::test] + async fn keycloak_rejects_an_id_that_cannot_be_a_path_segment() { + for user_id in ["", ".", "..", "ad\nmin"] { + let mock_server = MockServer::start().await; + mock_token(&mock_server, Some(3600), 1).await; + + let error = get_user_info_by_id(&backend_for(&mock_server), user_id) + .await + .expect_err("the id is rejected before Keycloak is asked"); + + assert!( + matches!(error, Error::ConstructUserUrl { .. }), + "for {user_id:?} got {error}" + ); + assert_eq!( + http_error::Error::status_code(&error), + StatusCode::BAD_REQUEST + ); + assert!(received_lookups(&mock_server).await.is_empty()); + } + } + + /// A root path without a trailing slash is kept, so a Keycloak under `/auth` is asked at + /// `/auth/realms/...`. + #[tokio::test] + async fn keycloak_keeps_the_root_path() { + let mock_server = MockServer::start().await; + crate::backend::test_mocks::mock_token( + &mock_server, + format!("/auth/realms/{ADMIN_REALM}/protocol/openid-connect/token"), + Some(3600), + Some(1), + ) + .await; + Mock::given(method("GET")) + .and(path(format!( + "/auth/admin/realms/{USER_REALM}/users/{USER_ID}" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": USER_ID, + "username": "alice", + }))) + .expect(1) + .mount(&mock_server) + .await; + Mock::given(method("GET")) + .and(path(format!( + "/auth/admin/realms/{USER_REALM}/users/{USER_ID}/groups" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .expect(1) + .mount(&mock_server) + .await; + + let mut backend = backend_for(&mock_server); + backend.config.root_path = "/auth".to_owned(); + + get_user_info_by_id(&backend, USER_ID) + .await + .expect("Keycloak is reached under its root path"); + } + /// A token can stop being accepted before it expires, e.g. by being revoked. The rejection is the /// only way to find that out, so it has to trigger exactly one re-authentication. Not none /// (the lookup would keep failing until the token expired), and not a retry loop. @@ -471,7 +634,7 @@ mod tests { mock_token(&mock_server, Some(3600), 2).await; mock_user_and_groups(&mock_server, 401).await; - let error = get_user_info_by_id(&backend_for(&mock_server)) + let error = get_user_info_by_id(&backend_for(&mock_server), USER_ID) .await .expect_err("a permanently rejected token must surface as an error"); diff --git a/rust/user-info-fetcher/src/backend/mod.rs b/rust/user-info-fetcher/src/backend/mod.rs index f4198765..5771e471 100644 --- a/rust/user-info-fetcher/src/backend/mod.rs +++ b/rust/user-info-fetcher/src/backend/mod.rs @@ -6,11 +6,70 @@ pub mod xfsc_aas; #[cfg(test)] mod test_mocks { + use percent_encoding::percent_decode_str; use wiremock::{ - Mock, MockServer, ResponseTemplate, + Mock, MockServer, Request, ResponseTemplate, matchers::{method, path}, }; + /// User ids and names that address something other than the user when put into a URL as text: + /// another host, another path or extra query parameters. + pub(super) const HOSTILE_USERS: &[&str] = &[ + "https://evil.example.com/", + "//evil.example.com", + "../../master", + "..%2F..%2Fmaster", + "%2e%2e/%2e%2e/master", + "..\\..\\master", + "alice/groups", + "alice?briefRepresentation=true", + "alice&exact=false", + "alice#fragment", + "alice;x=1", + "alice@example.com", + ]; + + /// The `GET` requests `mock_server` received, leaving out the token requests. + pub(super) async fn received_lookups(mock_server: &MockServer) -> Vec { + let requests = mock_server + .received_requests() + .await + .expect("request recording is enabled by default"); + + requests + .into_iter() + .filter(|request| request.method.as_str() == "GET") + .collect() + } + + /// Asserts that `mock_server` received a single lookup, addressed to exactly one path segment + /// below `parent_path` that decodes to `user`. + pub(super) async fn assert_looked_up_below( + mock_server: &MockServer, + parent_path: &str, + user: &str, + ) { + let lookups = received_lookups(mock_server).await; + let [lookup] = lookups.as_slice() else { + panic!("{user:?} was not looked up exactly once: {lookups:?}"); + }; + + let path = lookup.url.path(); + let segment = path + .strip_prefix(parent_path) + .unwrap_or_else(|| panic!("{user:?} was looked up at {path}, outside {parent_path}")); + + assert!( + !segment.contains('/'), + "{user:?} was looked up at {path}, more than one segment below {parent_path}" + ); + assert_eq!( + percent_decode_str(segment).decode_utf8_lossy(), + user, + "{user:?} was looked up at {path}" + ); + } + /// Mounts an OAuth2 `client_credentials` token endpoint at `token_path`, answering with a token /// that is valid for `expires_in` seconds. Without an `expires_in` the token is deliberately not /// cached, see [`info_fetcher_commons::utils::token::CachedToken`]. From a81cb0b7cdc3af158b187caac844f6599fbd84ff Mon Sep 17 00:00:00 2001 From: Lars Francke Date: Fri, 23 Oct 2026 09:08:11 +0200 Subject: [PATCH 2/3] chore: Regenerate Cargo.nix --- Cargo.nix | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/Cargo.nix b/Cargo.nix index d3f68a88..034fa926 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -5536,12 +5536,24 @@ rec { name = "tracing"; packageId = "tracing"; } + { + name = "url"; + packageId = "url"; + } ]; devDependencies = [ { name = "futures"; packageId = "futures"; } + { + name = "percent-encoding"; + packageId = "percent-encoding"; + } + { + name = "rstest"; + packageId = "rstest"; + } ]; }; @@ -12603,6 +12615,10 @@ rec { } ]; devDependencies = [ + { + name = "percent-encoding"; + packageId = "percent-encoding"; + } { name = "wiremock"; packageId = "wiremock"; From e353540b47f1d8bce64ee7e35a2f436d4b7da0b7 Mon Sep 17 00:00:00 2001 From: Lars Francke Date: Fri, 23 Oct 2026 09:55:13 +0200 Subject: [PATCH 3/3] chore: Bump rustls-webpki to 0.103.16 --- Cargo.lock | 6 +++--- Cargo.nix | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f33216fd..d5c54628 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3248,9 +3248,9 @@ checksum = "eec689c0bc40ff2458a5977b6619cb718087084a18e02a131c599b62d05e1a5f" [[package]] name = "rustls-webpki" -version = "0.103.15" +version = "0.103.16" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f3c3cf1d8b1e7d4927e2d154c3fcb02979afb9939629c62cd9048d4f07b60ac2" +checksum = "4443b1b6d39d7c663139942cc4558262562d2c84123bb6282ef177009ca8b281" dependencies = [ "aws-lc-rs", "ring", @@ -4111,7 +4111,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" dependencies = [ "fastrand", - "getrandom 0.4.3", + "getrandom 0.3.4", "once_cell", "rustix", "windows-sys 0.61.2", diff --git a/Cargo.nix b/Cargo.nix index 034fa926..0c00baac 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -10794,9 +10794,9 @@ rec { }; "rustls-webpki" = rec { crateName = "rustls-webpki"; - version = "0.103.15"; + version = "0.103.16"; edition = "2021"; - sha256 = "1hhanq3lz384v4nccacnjfwsyy99n3yc6m6iw8kljz8yicfwzhzk"; + sha256 = "10djm2f00xzi5qlbcfqjhhn2smk2h9aw8b4l74qncz4xsfvb2hs4"; libName = "webpki"; dependencies = [ { @@ -13687,7 +13687,7 @@ rec { } { name = "getrandom"; - packageId = "getrandom 0.4.3"; + packageId = "getrandom 0.3.4"; optional = true; usesDefaultFeatures = false; target = { target, features }: ((target."unix" or false) || (target."windows" or false) || ("wasi" == target."os" or null));