diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f6e9786..b4e557dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -92,6 +92,15 @@ 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 Entra backend now encodes the user id and the username as a single URL path + segment. An empty, `.` or `..` id or username, or one containing a line break, is rejected with + `400` ([#909]). +- The user-info-fetcher Keycloak backend now encodes the user id as a single URL path segment, and + sends the username as a query parameter instead of appending it to the URL as text, so that a + username can no longer add or override search parameters. An empty, `.` or `..` id, or one + containing a line break, is rejected with `400` ([#909]). +- The user-info-fetcher Keycloak backend no longer drops a configured `rootPath` from the URLs it + requests, so a Keycloak served below a sub-path, like `/auth`, can now be reached ([#909]). [#873]: https://github.com/stackabletech/opa-operator/pull/873 [#852]: https://github.com/stackabletech/opa-operator/pull/852 @@ -113,6 +122,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 bdb77a15..da9ea86e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3339,12 +3339,12 @@ dependencies = [ [[package]] name = "serde-saphyr" -version = "2.1.0" +version = "2.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3f4dc2714fc5632417a6e979602d7044bad8f2880c63fc7217087180acec31b6" +checksum = "d43fb82d550b07e6f929a58d7d0935182602f48ddead875f5ee313c41c8f07fe" dependencies = [ "annotate-snippets", - "base64 0.23.1", + "base64 0.22.1", "encoding_rs_io", "granit-parser", "nohash-hasher", @@ -4063,7 +4063,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 361690a4..0f24cd2b 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -11470,10 +11470,10 @@ rec { }; "serde-saphyr" = rec { crateName = "serde-saphyr"; - version = "2.1.0"; + version = "2.1.1"; edition = "2024"; crateBin = []; - sha256 = "1diixjn80w882xrgqqqci3rdifj4f0nn0yg9lqbj8qy59xqw4k9z"; + sha256 = "1zh7iwfc84z3brgqgbfyips049hq6l4pv3d557wyc1qbalnvhgyl"; libName = "serde_saphyr"; license = "MIT OR Apache-2.0"; dependencies = [ @@ -11484,7 +11484,7 @@ rec { } { name = "base64"; - packageId = "base64 0.23.1"; + packageId = "base64 0.22.1"; optional = true; } { @@ -13975,7 +13975,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)); diff --git a/rust/user-info-fetcher/src/backend/entra.rs b/rust/user-info-fetcher/src/backend/entra.rs index 0a630161..12506312 100644 --- a/rust/user-info-fetcher/src/backend/entra.rs +++ b/rust/user-info-fetcher/src/backend/entra.rs @@ -11,7 +11,10 @@ use info_fetcher_commons::utils::{ use serde::Deserialize; use snafu::{ResultExt, Snafu}; use stackable_opa_operator::crd::user_info_fetcher::v1alpha2; -use stackable_operator::commons::{networking::HostName, tls_verification::TlsClientDetails}; +use stackable_operator::{ + commons::{networking::HostName, tls_verification::TlsClientDetails}, + utils::UrlExt, +}; use url::Url; use crate::{UserInfo, UserInfoRequest, http_error}; @@ -54,6 +57,20 @@ pub enum Error { endpoint: String, }, + #[snafu(display("failed to construct the Graph URL for user {user:?}"))] + ConstructUserUrl { + source: stackable_operator::utils::UrlError, + user: String, + }, + + #[snafu(display( + "failed to construct the Graph groups URL for user id {user_id:?} returned by Entra" + ))] + ConstructUserGroupsUrl { + source: stackable_operator::utils::UrlError, + user_id: String, + }, + #[snafu(display("failed to construct HTTP client"))] ConstructHttpClient { source: reqwest::Error }, @@ -77,6 +94,18 @@ impl http_error::Error for Error { Self::UserNotFoundByName { .. } => StatusCode::NOT_FOUND, Self::RequestUserGroups { .. } => StatusCode::BAD_GATEWAY, Self::BuildEntraEndpoint { .. } => StatusCode::BAD_REQUEST, + Self::ConstructUserUrl { + source: stackable_operator::utils::UrlError::InvalidSegments { .. }, + .. + } => StatusCode::BAD_REQUEST, + Self::ConstructUserUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, + // The groups URL is built from the user id in Graph's own response, so an id that + // cannot be a path segment is a fault of the backend and not of the caller. + Self::ConstructUserGroupsUrl { + source: stackable_operator::utils::UrlError::InvalidSegments { .. }, + .. + } => StatusCode::BAD_GATEWAY, + Self::ConstructUserGroupsUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::ConstructHttpClient { .. } => StatusCode::SERVICE_UNAVAILABLE, Self::ConfigureTls { .. } => StatusCode::SERVICE_UNAVAILABLE, Self::ReadClientId { .. } => StatusCode::SERVICE_UNAVAILABLE, @@ -243,7 +272,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 +289,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 +303,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 +385,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 { + self.user_info_endpoint_url + .join_safe(&["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_id: &str) -> Result { + let mut group_info_url = self + .user_info_endpoint_url + .join_safe(&["v1.0", "users", user_id, "memberOf"]) + .context(ConstructUserGroupsUrlSnafu { user_id })?; // 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 +433,10 @@ mod tests { }; use super::*; - use crate::UserInfoRequestById; + use crate::{ + UserInfoRequestById, + backend::test_mocks::{HOSTILE_PATH_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 +513,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 +544,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 +564,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 +597,90 @@ 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. A username needs no test of its own: Graph addresses + /// the user by id or by principal name in that very same segment. + #[tokio::test] + async fn entra_keeps_a_hostile_id_inside_the_users_endpoint() { + for &user_id in HOSTILE_PATH_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; + } + } + + /// 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()); + } + } + + /// The group memberships are requested for the id in Graph's own answer, so an id that cannot be + /// a path segment is a fault of the backend and must not be reported as a bad request of the + /// caller. + #[tokio::test] + async fn entra_reports_an_unusable_id_in_its_own_answer_as_a_backend_fault() { + let mock_server = MockServer::start().await; + mock_token(&mock_server, Some(3600), 1).await; + Mock::given(method("GET")) + .and(path(format!("/v1.0/users/{USER_ID}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "", + "userPrincipalName": "alice@example.com", + }))) + .mount(&mock_server) + .await; + + let error = look_up_by_id(&backend_for(&mock_server), USER_ID) + .await + .expect_err("the group memberships of a user without an id cannot be requested"); + + assert!( + matches!(error, Error::ConstructUserGroupsUrl { .. }), + "{error}" + ); + assert_eq!( + http_error::Error::status_code(&error), + StatusCode::BAD_GATEWAY + ); + } + async fn get_user_info_by_id(backend: &ResolvedEntraBackend) -> UserInfo { backend .get_user_info(&UserInfoRequest::UserInfoRequestById(UserInfoRequestById { @@ -723,11 +844,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 +884,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..0daf12fe 100644 --- a/rust/user-info-fetcher/src/backend/keycloak.rs +++ b/rust/user-info-fetcher/src/backend/keycloak.rs @@ -11,7 +11,7 @@ use info_fetcher_commons::utils::{ use serde::Deserialize; use snafu::{OptionExt, ResultExt, Snafu}; use stackable_opa_operator::crd::user_info_fetcher::v1alpha2; -use stackable_operator::crd::authentication::oidc; +use stackable_operator::{crd::authentication::oidc, utils::UrlExt}; use url::Url; use crate::{UserInfo, UserInfoRequest, http_error}; @@ -52,7 +52,23 @@ pub enum Error { ParseOidcEndpointUrl { source: oidc::v1alpha1::Error }, #[snafu(display("failed to construct OIDC endpoint path"))] - ConstructOidcEndpointPath { source: url::ParseError }, + ConstructOidcEndpointPath { + source: stackable_operator::utils::UrlError, + }, + + #[snafu(display("failed to construct the Keycloak URL for user id {user_id:?}"))] + ConstructUserUrl { + source: stackable_operator::utils::UrlError, + user_id: String, + }, + + #[snafu(display( + "failed to construct the Keycloak groups URL for user id {user_id:?} returned by Keycloak" + ))] + ConstructUserGroupsUrl { + source: stackable_operator::utils::UrlError, + user_id: String, + }, #[snafu(display("failed to construct HTTP client"))] ConstructHttpClient { source: reqwest::Error }, @@ -79,6 +95,18 @@ 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: stackable_operator::utils::UrlError::InvalidSegments { .. }, + .. + } => StatusCode::BAD_REQUEST, + Self::ConstructUserUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, + // The groups URL is built from the user id in Keycloak's own response, so an id that + // cannot be a path segment is a fault of the backend and not of the caller. + Self::ConstructUserGroupsUrl { + source: stackable_operator::utils::UrlError::InvalidSegments { .. }, + .. + } => StatusCode::BAD_GATEWAY, + Self::ConstructUserGroupsUrl { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::ConstructHttpClient { .. } => StatusCode::SERVICE_UNAVAILABLE, Self::ConfigureTls { .. } => StatusCode::SERVICE_UNAVAILABLE, Self::ReadClientId { .. } => StatusCode::SERVICE_UNAVAILABLE, @@ -216,16 +244,13 @@ impl ResolvedKeycloakBackend { client_secret: Secret, ) -> Result { let admin_realm = &self.config.admin_realm; + let token_url = keycloak_url + .join_safe(&["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 +269,20 @@ 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/")) + let users_url = keycloak_url + .join_safe(&["admin", "realms", user_realm, "users"]) .context(ConstructOidcEndpointPathSnafu)?; let user_info = match req { UserInfoRequest::UserInfoRequestById(req) => { let user_id = req.id.clone(); + let user_url = users_url + .join_safe(&[&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 +297,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 +317,14 @@ impl ResolvedKeycloakBackend { } }; + let user_id = &user_info.id; + let groups_url = users_url + .join_safe(&[user_id, "groups"]) + .context(ConstructUserGroupsUrlSnafu { user_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 +356,15 @@ mod tests { }; use super::*; - use crate::{UserInfoRequestById, backend::keycloak::ResolvedKeycloakBackend}; + use crate::{ + UserInfoRequestById, UserInfoRequestByName, + backend::{ + keycloak::ResolvedKeycloakBackend, + test_mocks::{ + HOSTILE_PATH_USERS, HOSTILE_USERS, assert_looked_up_below, received_lookups, + }, + }, + }; const ADMIN_REALM: &str = "master"; const USER_REALM: &str = "my-realm"; @@ -395,14 +425,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 +456,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 +474,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 +495,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 +510,166 @@ 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_PATH_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()); + } + } + + /// The groups are requested for the id in Keycloak's own answer, so an id that cannot be a path + /// segment is a fault of the backend and must not be reported as a bad request of the caller. + #[tokio::test] + async fn keycloak_reports_an_unusable_id_in_its_own_answer_as_a_backend_fault() { + let mock_server = MockServer::start().await; + mock_token(&mock_server, Some(3600), 1).await; + Mock::given(method("GET")) + .and(path(format!("/admin/realms/{USER_REALM}/users/{USER_ID}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "id": "", + "username": "alice", + }))) + .mount(&mock_server) + .await; + + let error = get_user_info_by_id(&backend_for(&mock_server), USER_ID) + .await + .expect_err("the groups of a user without an id cannot be requested"); + + assert!( + matches!(error, Error::ConstructUserGroupsUrl { .. }), + "{error}" + ); + assert_eq!( + http_error::Error::status_code(&error), + StatusCode::BAD_GATEWAY + ); + } + + /// 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 +679,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..bb67fc76 100644 --- a/rust/user-info-fetcher/src/backend/mod.rs +++ b/rust/user-info-fetcher/src/backend/mod.rs @@ -6,11 +6,90 @@ pub mod xfsc_aas; #[cfg(test)] mod test_mocks { + use url::Url; use wiremock::{ - Mock, MockServer, ResponseTemplate, + Mock, MockServer, Request, ResponseTemplate, matchers::{method, path}, }; + /// User ids and names that reach for another path or host when put into a URL as text. One per + /// mechanism is enough here: that every other shape is encoded as well is a property of + /// [`stackable_operator::utils::UrlExt::join_safe`] and is covered by its own tests. What these + /// pin down is only that the request-supplied value is put through it at all. + pub(super) const HOSTILE_PATH_USERS: &[&str] = &[ + // Would climb out of the users endpoint. + "../../master", + // Would replace the host the access token is sent to. + "https://evil.example.com/", + // Would survive a single round of escaping and be decoded by the server. + "%2e%2e/%2e%2e/master", + ]; + + /// 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. Used where the value ends up as the + /// value of a query parameter rather than as a path segment, and is therefore escaped by + /// `reqwest` instead of by [`stackable_operator::utils::UrlExt::join_safe`]. + 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 `user` as exactly one path + /// segment below `parent_path`. + /// + /// The expected path is spelled out with [`url`]'s own encoder rather than by decoding what was + /// received. That still catches a call site which interpolates the user into the path as text, + /// and it saves this test helper a percent-decoder of its own. + 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 mut expected_url = + Url::parse("https://expected.example.com").expect("a constant URL parses"); + expected_url.set_path(parent_path); + expected_url + .path_segments_mut() + .expect("an https URL can be a base") + .pop_if_empty() + .push(user); + + assert_eq!( + lookup.url.path(), + expected_url.path(), + "{user:?} was looked up at {}", + lookup.url.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`].