From 0daba5682732dc4997c9dc1a31362e576c031db7 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 22 Nov 2024 10:49:42 +0100 Subject: [PATCH 01/10] fix: Calculation of OIDC endpoint --- crates/stackable-operator/CHANGELOG.md | 10 +- .../src/commons/authentication/oidc.rs | 118 ++++++++++++++++-- 2 files changed, 120 insertions(+), 8 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 69ca8485f..f3557dedd 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -4,6 +4,14 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Fixed + +- Fixed URL handling related to OIDC and `rootPath` with and without trailing slashes. Also added a bunch of tests ([#XXX]). + +### Changed + +- BREAKING: Made `DEFAULT_OIDC_WELLKNOWN_PATH` private. Use `AuthenticationProvider::well_known_url` instead ([#XXX]). + ## [0.81.0] - 2024-11-05 ### Added @@ -12,7 +20,7 @@ All notable changes to this project will be documented in this file. ### Changed -- BREAKING: Split `ListenerClass.spec.preferred_address_type` into a new `PreferredAddressType` type. Use `resolve_preferred_address_type()` to access the `AddressType` as before. ([#903]) +- BREAKING: Split `ListenerClass.spec.preferred_address_type` into a new `PreferredAddressType` type. Use `resolve_preferred_address_type()` to access the `AddressType` as before ([#903]). [#903]: https://github.com/stackabletech/operator-rs/pull/903 diff --git a/crates/stackable-operator/src/commons/authentication/oidc.rs b/crates/stackable-operator/src/commons/authentication/oidc.rs index d707b5657..7796ffe6e 100644 --- a/crates/stackable-operator/src/commons/authentication/oidc.rs +++ b/crates/stackable-operator/src/commons/authentication/oidc.rs @@ -17,19 +17,27 @@ use crate::commons::{ pub type Result = std::result::Result; -pub const DEFAULT_OIDC_WELLKNOWN_PATH: &str = ".well-known/openid-configuration"; pub const CLIENT_ID_SECRET_KEY: &str = "clientId"; pub const CLIENT_SECRET_SECRET_KEY: &str = "clientSecret"; +const DEFAULT_OIDC_WELLKNOWN_PATH: &str = "./.well-known/openid-configuration"; + #[derive(Debug, PartialEq, Snafu)] pub enum Error { #[snafu(display("failed to parse OIDC endpoint url"))] ParseOidcEndpointUrl { source: ParseError }, #[snafu(display( - "failed to set OIDC endpoint scheme '{scheme}' for endpoint url '{endpoint}'" + "failed to set OIDC endpoint scheme '{scheme}' for endpoint url \"{endpoint}\"" ))] SetOidcEndpointScheme { endpoint: Url, scheme: String }, + + #[snafu(display("failed to join the path {path:?} to URL \"{url}\""))] + JoinPath { + source: ParseError, + url: Url, + path: String, + }, } /// This struct contains configuration values to configure an OpenID Connect @@ -111,10 +119,8 @@ impl AuthenticationProvider { } } - /// Returns the OIDC endpoint [`Url`]. To append the default OIDC well-known - /// configuration path, use `url.join()`. This module provides the default - /// path at [`DEFAULT_OIDC_WELLKNOWN_PATH`]. - pub fn endpoint_url(&self) -> Result { + /// Base [`Url`] without any path set (so only protocol, host, port and such). + fn base_url(&self) -> Result { let mut url = Url::parse(&format!( "http://{host}:{port}", host = self.hostname.as_url_host(), @@ -132,10 +138,38 @@ impl AuthenticationProvider { })?; } - url.set_path(&self.root_path); Ok(url) } + /// Returns the OIDC endpoint [`Url`] without a trailing slash. + /// + /// To get the well-known URL, please use [`Self::well_known_url`]. + pub fn endpoint_url(&self) -> Result { + let mut url = self.base_url()?; + // Some tools can not cope with a trailing slash, so let's remove that + url.set_path(&self.root_path.trim_end_matches('/')); + Ok(url) + } + + /// Returns the well-known [`Url`] without a trailing slash. + /// + /// It is basically the [`Self::endpoint_url`] joined with + /// "./.well-known/openid-configuration", while watching out for URL joining madness. + pub fn well_known_url(&self) -> Result { + let mut url = self.base_url()?; + + // Url::join cuts of the part after the last slash :/ + // So we need to make sure we have a trailing slash, so that nothing get's cut of. + let mut root_path_with_trailing_slash = self.root_path.trim_end_matches('/').to_string(); + root_path_with_trailing_slash.push('/'); + url.set_path(&root_path_with_trailing_slash); + url.join(DEFAULT_OIDC_WELLKNOWN_PATH) + .with_context(|_| JoinPathSnafu { + url: url.clone(), + path: DEFAULT_OIDC_WELLKNOWN_PATH.to_owned(), + }) + } + /// Returns the port to be used, which is either user configured or defaulted based upon TLS usage pub fn port(&self) -> u16 { self.port @@ -246,6 +280,8 @@ pub struct ClientAuthenticationOptions { #[cfg(test)] mod test { + use rstest::rstest; + use super::*; #[test] @@ -338,6 +374,74 @@ mod test { ); } + #[rstest] + #[case("/", "https://my.keycloak.server/")] + #[case("/realms/sdp", "https://my.keycloak.server/realms/sdp")] + #[case("/realms/sdp/", "https://my.keycloak.server/realms/sdp")] + #[case("/realms/sdp//////", "https://my.keycloak.server/realms/sdp")] + #[case( + "/realms/my/realm/with/slashes//////", + "https://my.keycloak.server/realms/my/realm/with/slashes" + )] + fn root_path_endpoint_url(#[case] root_path: String, #[case] expected_endpoint_url: &str) { + let oidc = serde_yaml::from_str::(&format!( + " + hostname: my.keycloak.server + rootPath: {root_path} + scopes: [openid] + principalClaim: preferred_username + tls: + verification: + server: + caCert: + webPki: {{}} + " + )) + .unwrap(); + + assert_eq!(oidc.endpoint_url().unwrap().as_str(), expected_endpoint_url); + } + + #[rstest] + #[case("/", "https://my.keycloak.server/.well-known/openid-configuration")] + #[case( + "/realms/sdp", + "https://my.keycloak.server/realms/sdp/.well-known/openid-configuration" + )] + #[case( + "/realms/sdp/", + "https://my.keycloak.server/realms/sdp/.well-known/openid-configuration" + )] + #[case( + "/realms/sdp//////", + "https://my.keycloak.server/realms/sdp/.well-known/openid-configuration" + )] + #[case( + "/realms/my/realm/with/slashes//////", + "https://my.keycloak.server/realms/my/realm/with/slashes/.well-known/openid-configuration" + )] + fn root_path_well_known_url(#[case] root_path: String, #[case] expected_well_known_url: &str) { + let oidc = serde_yaml::from_str::(&format!( + " + hostname: my.keycloak.server + rootPath: {root_path} + scopes: [openid] + principalClaim: preferred_username + tls: + verification: + server: + caCert: + webPki: {{}} + " + )) + .unwrap(); + + assert_eq!( + oidc.well_known_url().unwrap().as_str(), + expected_well_known_url + ); + } + #[test] fn client_env_vars() { let secret_name = "my-keycloak-client"; From 2047bd764bae78df09f3365dad42ccb72c3d56ad Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 22 Nov 2024 11:05:38 +0100 Subject: [PATCH 02/10] changelog --- crates/stackable-operator/CHANGELOG.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index f3557dedd..e18a55cb8 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -6,11 +6,13 @@ All notable changes to this project will be documented in this file. ### Fixed -- Fixed URL handling related to OIDC and `rootPath` with and without trailing slashes. Also added a bunch of tests ([#XXX]). +- Fixed URL handling related to OIDC and `rootPath` with and without trailing slashes. Also added a bunch of tests ([#910]). ### Changed -- BREAKING: Made `DEFAULT_OIDC_WELLKNOWN_PATH` private. Use `AuthenticationProvider::well_known_url` instead ([#XXX]). +- BREAKING: Made `DEFAULT_OIDC_WELLKNOWN_PATH` private. Use `AuthenticationProvider::well_known_url` instead ([#910]). + +[#910]: https://github.com/stackabletech/operator-rs/pull/910 ## [0.81.0] - 2024-11-05 From 2d50d9bcbdf6efc1d6cd46dbe40bbe4a8e9138c5 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 22 Nov 2024 11:08:55 +0100 Subject: [PATCH 03/10] udpate tests --- .../src/commons/authentication/oidc.rs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/crates/stackable-operator/src/commons/authentication/oidc.rs b/crates/stackable-operator/src/commons/authentication/oidc.rs index 7796ffe6e..2ea9d3c38 100644 --- a/crates/stackable-operator/src/commons/authentication/oidc.rs +++ b/crates/stackable-operator/src/commons/authentication/oidc.rs @@ -375,26 +375,22 @@ mod test { } #[rstest] - #[case("/", "https://my.keycloak.server/")] - #[case("/realms/sdp", "https://my.keycloak.server/realms/sdp")] - #[case("/realms/sdp/", "https://my.keycloak.server/realms/sdp")] - #[case("/realms/sdp//////", "https://my.keycloak.server/realms/sdp")] + #[case("/", "http://my.keycloak.server:1234/")] + #[case("/realms/sdp", "http://my.keycloak.server:1234/realms/sdp")] + #[case("/realms/sdp/", "http://my.keycloak.server:1234/realms/sdp")] + #[case("/realms/sdp//////", "http://my.keycloak.server:1234/realms/sdp")] #[case( "/realms/my/realm/with/slashes//////", - "https://my.keycloak.server/realms/my/realm/with/slashes" + "http://my.keycloak.server:1234/realms/my/realm/with/slashes" )] fn root_path_endpoint_url(#[case] root_path: String, #[case] expected_endpoint_url: &str) { let oidc = serde_yaml::from_str::(&format!( " hostname: my.keycloak.server + port: 1234 rootPath: {root_path} scopes: [openid] principalClaim: preferred_username - tls: - verification: - server: - caCert: - webPki: {{}} " )) .unwrap(); From 0959e1dacd3781e7ee6f90ee91a42b0459218d07 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 22 Nov 2024 11:09:37 +0100 Subject: [PATCH 04/10] clippy --- crates/stackable-operator/src/commons/authentication/oidc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/commons/authentication/oidc.rs b/crates/stackable-operator/src/commons/authentication/oidc.rs index 2ea9d3c38..0521d8deb 100644 --- a/crates/stackable-operator/src/commons/authentication/oidc.rs +++ b/crates/stackable-operator/src/commons/authentication/oidc.rs @@ -147,7 +147,7 @@ impl AuthenticationProvider { pub fn endpoint_url(&self) -> Result { let mut url = self.base_url()?; // Some tools can not cope with a trailing slash, so let's remove that - url.set_path(&self.root_path.trim_end_matches('/')); + url.set_path(self.root_path.trim_end_matches('/')); Ok(url) } From bd566829cd0040dec47fd7cf83ad7b9646d1624a Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 22 Nov 2024 13:59:51 +0100 Subject: [PATCH 05/10] Update crates/stackable-operator/src/commons/authentication/oidc.rs Co-authored-by: Techassi --- crates/stackable-operator/src/commons/authentication/oidc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/commons/authentication/oidc.rs b/crates/stackable-operator/src/commons/authentication/oidc.rs index 0521d8deb..c15cfbc1b 100644 --- a/crates/stackable-operator/src/commons/authentication/oidc.rs +++ b/crates/stackable-operator/src/commons/authentication/oidc.rs @@ -20,7 +20,7 @@ pub type Result = std::result::Result; pub const CLIENT_ID_SECRET_KEY: &str = "clientId"; pub const CLIENT_SECRET_SECRET_KEY: &str = "clientSecret"; -const DEFAULT_OIDC_WELLKNOWN_PATH: &str = "./.well-known/openid-configuration"; +const DEFAULT_WELLKNOWN_OIDC_CONFIG_PATH: &str = "/.well-known/openid-configuration"; #[derive(Debug, PartialEq, Snafu)] pub enum Error { From 4dbb29d5e2631a4eba8777b9f008cf32a5001f7b Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 22 Nov 2024 14:06:01 +0100 Subject: [PATCH 06/10] fix tests again --- .../src/commons/authentication/oidc.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/crates/stackable-operator/src/commons/authentication/oidc.rs b/crates/stackable-operator/src/commons/authentication/oidc.rs index c15cfbc1b..d8fbcee42 100644 --- a/crates/stackable-operator/src/commons/authentication/oidc.rs +++ b/crates/stackable-operator/src/commons/authentication/oidc.rs @@ -20,7 +20,7 @@ pub type Result = std::result::Result; pub const CLIENT_ID_SECRET_KEY: &str = "clientId"; pub const CLIENT_SECRET_SECRET_KEY: &str = "clientSecret"; -const DEFAULT_WELLKNOWN_OIDC_CONFIG_PATH: &str = "/.well-known/openid-configuration"; +const DEFAULT_WELLKNOWN_OIDC_CONFIG_PATH: &str = ".well-known/openid-configuration"; #[derive(Debug, PartialEq, Snafu)] pub enum Error { @@ -163,10 +163,10 @@ impl AuthenticationProvider { let mut root_path_with_trailing_slash = self.root_path.trim_end_matches('/').to_string(); root_path_with_trailing_slash.push('/'); url.set_path(&root_path_with_trailing_slash); - url.join(DEFAULT_OIDC_WELLKNOWN_PATH) + url.join(DEFAULT_WELLKNOWN_OIDC_CONFIG_PATH) .with_context(|_| JoinPathSnafu { url: url.clone(), - path: DEFAULT_OIDC_WELLKNOWN_PATH.to_owned(), + path: DEFAULT_WELLKNOWN_OIDC_CONFIG_PATH.to_owned(), }) } @@ -346,12 +346,8 @@ mod test { .unwrap(); assert_eq!( - oidc.endpoint_url() - .unwrap() - .join(DEFAULT_OIDC_WELLKNOWN_PATH) - .unwrap() - .as_str(), - "https://my.keycloak.server/.well-known/openid-configuration" + oidc.endpoint_url().unwrap().as_str(), + "https://my.keycloak.server/" ); } From b0fa449d6cc2109248c1d87f320063b41804a2b4 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 22 Nov 2024 14:06:57 +0100 Subject: [PATCH 07/10] Apply suggestions from code review Co-authored-by: Techassi --- crates/stackable-operator/CHANGELOG.md | 2 +- .../stackable-operator/src/commons/authentication/oidc.rs | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index e18a55cb8..0a99d1e78 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -10,7 +10,7 @@ All notable changes to this project will be documented in this file. ### Changed -- BREAKING: Made `DEFAULT_OIDC_WELLKNOWN_PATH` private. Use `AuthenticationProvider::well_known_url` instead ([#910]). +- BREAKING: Made `DEFAULT_OIDC_WELLKNOWN_PATH` private. Use `AuthenticationProvider::well_known_config_url` instead ([#910]). [#910]: https://github.com/stackabletech/operator-rs/pull/910 diff --git a/crates/stackable-operator/src/commons/authentication/oidc.rs b/crates/stackable-operator/src/commons/authentication/oidc.rs index d8fbcee42..8aa787b57 100644 --- a/crates/stackable-operator/src/commons/authentication/oidc.rs +++ b/crates/stackable-operator/src/commons/authentication/oidc.rs @@ -119,7 +119,9 @@ impl AuthenticationProvider { } } - /// Base [`Url`] without any path set (so only protocol, host, port and such). + /// Returns the base [`Url`] without any path segments. + /// + /// The base url only contains the scheme, the host, and an optional port. fn base_url(&self) -> Result { let mut url = Url::parse(&format!( "http://{host}:{port}", @@ -143,7 +145,7 @@ impl AuthenticationProvider { /// Returns the OIDC endpoint [`Url`] without a trailing slash. /// - /// To get the well-known URL, please use [`Self::well_known_url`]. + /// To retrieve the well-known OIDC configuration url, please use [`Self::well_known_config_url`]. pub fn endpoint_url(&self) -> Result { let mut url = self.base_url()?; // Some tools can not cope with a trailing slash, so let's remove that From 4f93135f11b54bcb57fc39c6d79dba433a50918b Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 22 Nov 2024 14:11:34 +0100 Subject: [PATCH 08/10] Rename well_known_url -> well_known_config_url --- crates/stackable-operator/src/commons/authentication/oidc.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/src/commons/authentication/oidc.rs b/crates/stackable-operator/src/commons/authentication/oidc.rs index 8aa787b57..57d3a0e8e 100644 --- a/crates/stackable-operator/src/commons/authentication/oidc.rs +++ b/crates/stackable-operator/src/commons/authentication/oidc.rs @@ -157,7 +157,7 @@ impl AuthenticationProvider { /// /// It is basically the [`Self::endpoint_url`] joined with /// "./.well-known/openid-configuration", while watching out for URL joining madness. - pub fn well_known_url(&self) -> Result { + pub fn well_known_config_url(&self) -> Result { let mut url = self.base_url()?; // Url::join cuts of the part after the last slash :/ @@ -431,7 +431,7 @@ mod test { .unwrap(); assert_eq!( - oidc.well_known_url().unwrap().as_str(), + oidc.well_known_config_url().unwrap().as_str(), expected_well_known_url ); } From 9cea26304fdefc42753c34f3e3554e9c63625016 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 22 Nov 2024 14:57:27 +0100 Subject: [PATCH 09/10] Review feedback --- .../src/commons/authentication/oidc.rs | 32 ++++++++----------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/crates/stackable-operator/src/commons/authentication/oidc.rs b/crates/stackable-operator/src/commons/authentication/oidc.rs index 57d3a0e8e..579fa93ac 100644 --- a/crates/stackable-operator/src/commons/authentication/oidc.rs +++ b/crates/stackable-operator/src/commons/authentication/oidc.rs @@ -20,7 +20,8 @@ pub type Result = std::result::Result; pub const CLIENT_ID_SECRET_KEY: &str = "clientId"; pub const CLIENT_SECRET_SECRET_KEY: &str = "clientSecret"; -const DEFAULT_WELLKNOWN_OIDC_CONFIG_PATH: &str = ".well-known/openid-configuration"; +/// Do *not* use this for [`Url::join`], as the leading slash will erase the existing path! +const DEFAULT_WELLKNOWN_OIDC_CONFIG_PATH: &str = "/.well-known/openid-configuration"; #[derive(Debug, PartialEq, Snafu)] pub enum Error { @@ -31,13 +32,6 @@ pub enum Error { "failed to set OIDC endpoint scheme '{scheme}' for endpoint url \"{endpoint}\"" ))] SetOidcEndpointScheme { endpoint: Url, scheme: String }, - - #[snafu(display("failed to join the path {path:?} to URL \"{url}\""))] - JoinPath { - source: ParseError, - url: Url, - path: String, - }, } /// This struct contains configuration values to configure an OpenID Connect @@ -155,21 +149,23 @@ impl AuthenticationProvider { /// Returns the well-known [`Url`] without a trailing slash. /// - /// It is basically the [`Self::endpoint_url`] joined with - /// "./.well-known/openid-configuration", while watching out for URL joining madness. + /// The returned url is a combination of [`Self::endpoint_url`] joined with + /// the well-known OIDC configuration path `DEFAULT_WELLKNOWN_OIDC_CONFIG_PATH`. pub fn well_known_config_url(&self) -> Result { let mut url = self.base_url()?; - // Url::join cuts of the part after the last slash :/ - // So we need to make sure we have a trailing slash, so that nothing get's cut of. + // Taken from https://docs.rs/url/latest/url/struct.Url.html#method.join: + // A trailing slash is significant. Without it, the last path component is considered to be + // a “file” name to be removed to get at the “directory” that is used as the base. + // + // Because of that behavior, we first need to make sure that the root path doesn't contain + // any trailing slashes to finally append the well-known config path to the url. The path + // already contains a prefixed slash. let mut root_path_with_trailing_slash = self.root_path.trim_end_matches('/').to_string(); - root_path_with_trailing_slash.push('/'); + root_path_with_trailing_slash.push_str(DEFAULT_WELLKNOWN_OIDC_CONFIG_PATH); url.set_path(&root_path_with_trailing_slash); - url.join(DEFAULT_WELLKNOWN_OIDC_CONFIG_PATH) - .with_context(|_| JoinPathSnafu { - url: url.clone(), - path: DEFAULT_WELLKNOWN_OIDC_CONFIG_PATH.to_owned(), - }) + + Ok(url) } /// Returns the port to be used, which is either user configured or defaulted based upon TLS usage From 27f322d2f23bfb18dba0f0f216c8b94e51a49cf6 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 22 Nov 2024 15:40:25 +0100 Subject: [PATCH 10/10] Apply suggestions from code review Co-authored-by: Techassi --- crates/stackable-operator/src/commons/authentication/oidc.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/src/commons/authentication/oidc.rs b/crates/stackable-operator/src/commons/authentication/oidc.rs index 579fa93ac..ed19eebdb 100644 --- a/crates/stackable-operator/src/commons/authentication/oidc.rs +++ b/crates/stackable-operator/src/commons/authentication/oidc.rs @@ -113,7 +113,7 @@ impl AuthenticationProvider { } } - /// Returns the base [`Url`] without any path segments. + /// Returns the OIDC base [`Url`] without any path segments. /// /// The base url only contains the scheme, the host, and an optional port. fn base_url(&self) -> Result { @@ -147,7 +147,7 @@ impl AuthenticationProvider { Ok(url) } - /// Returns the well-known [`Url`] without a trailing slash. + /// Returns the well-known OIDC configuration [`Url`] without a trailing slash. /// /// The returned url is a combination of [`Self::endpoint_url`] joined with /// the well-known OIDC configuration path `DEFAULT_WELLKNOWN_OIDC_CONFIG_PATH`.