diff --git a/CHANGELOG.md b/CHANGELOG.md index a0740bb5..8bbf5e0d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,11 +19,13 @@ ### Fixed - Invalid `SupersetCluster`, `DruidConnection` or `AuthenticationClass` objects don't stop the operator from reconciling ([#551]). +- Fix OIDC endpoint construction in case the `rootPath` does have a trailing slash ([#569]). [#528]: https://github.com/stackabletech/superset-operator/pull/528 [#530]: https://github.com/stackabletech/superset-operator/pull/530 [#549]: https://github.com/stackabletech/superset-operator/pull/549 [#551]: https://github.com/stackabletech/superset-operator/pull/551 +[#569]: https://github.com/stackabletech/superset-operator/pull/569 ## [24.7.0] - 2024-07-24 diff --git a/Cargo.lock b/Cargo.lock index 93a4edb9..f06a6564 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -720,6 +720,12 @@ version = "0.3.31" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f90f7dce0722e95104fcb095585910c0977252f286e354b5e3bd38902cd99988" +[[package]] +name = "futures-timer" +version = "3.0.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f288b0a4f20f9a56b5d1da57e2227c661b7b16168e2f72365f57b63326e29b24" + [[package]] name = "futures-util" version = "0.3.31" @@ -1630,6 +1636,15 @@ dependencies = [ "zerocopy", ] +[[package]] +name = "proc-macro-crate" +version = "3.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8ecf48c7ca261d60b74ab1a7b20da18bede46776b2e55535cb958eb595c5fa7b" +dependencies = [ + "toml_edit", +] + [[package]] name = "proc-macro2" version = "1.0.89" @@ -1747,6 +1762,12 @@ version = "0.8.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2b15c43186be67a4fd63bee50d0303afffcef381492ebe2c5d87f324e1b8815c" +[[package]] +name = "relative-path" +version = "1.9.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ba39f3699c378cd8970968dcbff9c43159ea4cfbd88d43c00b22f2ef10a435d2" + [[package]] name = "ring" version = "0.17.8" @@ -1762,12 +1783,51 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "rstest" +version = "0.23.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0a2c585be59b6b5dd66a9d2084aa1d8bd52fbdb806eafdeffb52791147862035" +dependencies = [ + "futures 0.3.31", + "futures-timer", + "rstest_macros", + "rustc_version", +] + +[[package]] +name = "rstest_macros" +version = "0.23.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "825ea780781b15345a146be27eaefb05085e337e869bff01b4306a4fd4a9ad5a" +dependencies = [ + "cfg-if", + "glob", + "proc-macro-crate", + "proc-macro2", + "quote", + "regex", + "relative-path", + "rustc_version", + "syn 2.0.82", + "unicode-ident", +] + [[package]] name = "rustc-demangle" version = "0.1.24" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "719b953e2095829ee67db738b3bfa9fa368c94900df327b3f07fe6e794d2fe1f" +[[package]] +name = "rustc_version" +version = "0.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cfcb3a22ef46e85b45de6ee7e79d063319ebb6594faafcf1c225ea92ab6e9b92" +dependencies = [ + "semver", +] + [[package]] name = "rustls" version = "0.23.15" @@ -2112,8 +2172,8 @@ checksum = "6980e8d7511241f8acf4aebddbb1ff938df5eebe98691418c4468d0b72a96a67" [[package]] name = "stackable-operator" -version = "0.80.0" -source = "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.80.0#6fbe32300b60f95e0baa2ab0ff2daf961b06531c" +version = "0.82.0" +source = "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.82.0#415bbd031bd52e9c0c5392060235030e9930b46b" dependencies = [ "chrono", "clap", @@ -2151,7 +2211,7 @@ dependencies = [ [[package]] name = "stackable-operator-derive" version = "0.3.1" -source = "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.80.0#6fbe32300b60f95e0baa2ab0ff2daf961b06531c" +source = "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.82.0#415bbd031bd52e9c0c5392060235030e9930b46b" dependencies = [ "darling", "proc-macro2", @@ -2162,7 +2222,7 @@ dependencies = [ [[package]] name = "stackable-shared" version = "0.0.1" -source = "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.80.0#6fbe32300b60f95e0baa2ab0ff2daf961b06531c" +source = "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.82.0#415bbd031bd52e9c0c5392060235030e9930b46b" dependencies = [ "kube", "semver", @@ -2198,6 +2258,7 @@ dependencies = [ "futures 0.3.31", "indoc", "product-config", + "rstest", "serde", "snafu 0.8.5", "stackable-operator", @@ -2432,6 +2493,23 @@ dependencies = [ "tokio", ] +[[package]] +name = "toml_datetime" +version = "0.6.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0dd7358ecb8fc2f8d014bf86f6f638ce72ba252a2c3a2572f2a795f1d23efb41" + +[[package]] +name = "toml_edit" +version = "0.22.22" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4ae48d6208a266e853d946088ed816055e556cc6028c5e8e2b84d9fa5dd7c7f5" +dependencies = [ + "indexmap", + "toml_datetime", + "winnow", +] + [[package]] name = "tower" version = "0.5.1" @@ -2870,6 +2948,15 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "589f6da84c646204747d1270a2a5661ea66ed1cced2631d546fdfb155959f9ec" +[[package]] +name = "winnow" +version = "0.6.20" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "36c1fec1a2bb5866f07c25f68c26e565c4c200aebb96d7e55710c19d3e8ac49b" +dependencies = [ + "memchr", +] + [[package]] name = "xml-rs" version = "0.8.22" diff --git a/Cargo.nix b/Cargo.nix index c20595ce..ca9a0e33 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -6520,13 +6520,13 @@ rec { }; "stackable-operator" = rec { crateName = "stackable-operator"; - version = "0.80.0"; + version = "0.82.0"; edition = "2021"; workspace_member = null; src = pkgs.fetchgit { url = "https://github.com/stackabletech/operator-rs.git"; - rev = "6fbe32300b60f95e0baa2ab0ff2daf961b06531c"; - sha256 = "16jrq3wdwz63210jgmqbx3snrr15wxw6l1smqhzv7b7jpq8qvya3"; + rev = "415bbd031bd52e9c0c5392060235030e9930b46b"; + sha256 = "0phasjwb64rxgn5hs8vks92icmx9255bd5v9dms280clrfpcg4hy"; }; libName = "stackable_operator"; authors = [ @@ -6683,8 +6683,8 @@ rec { workspace_member = null; src = pkgs.fetchgit { url = "https://github.com/stackabletech/operator-rs.git"; - rev = "6fbe32300b60f95e0baa2ab0ff2daf961b06531c"; - sha256 = "16jrq3wdwz63210jgmqbx3snrr15wxw6l1smqhzv7b7jpq8qvya3"; + rev = "415bbd031bd52e9c0c5392060235030e9930b46b"; + sha256 = "0phasjwb64rxgn5hs8vks92icmx9255bd5v9dms280clrfpcg4hy"; }; procMacro = true; libName = "stackable_operator_derive"; @@ -6718,8 +6718,8 @@ rec { workspace_member = null; src = pkgs.fetchgit { url = "https://github.com/stackabletech/operator-rs.git"; - rev = "6fbe32300b60f95e0baa2ab0ff2daf961b06531c"; - sha256 = "16jrq3wdwz63210jgmqbx3snrr15wxw6l1smqhzv7b7jpq8qvya3"; + rev = "415bbd031bd52e9c0c5392060235030e9930b46b"; + sha256 = "0phasjwb64rxgn5hs8vks92icmx9255bd5v9dms280clrfpcg4hy"; }; libName = "stackable_shared"; authors = [ diff --git a/Cargo.toml b/Cargo.toml index 51f0e683..7683e120 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,14 +17,16 @@ fnv = "1.0" futures = { version = "0.3", features = ["compat"] } indoc = "2.0" product-config = { git = "https://github.com/stackabletech/product-config.git", tag = "0.7.0" } +rstest = "0.23" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" serde_yaml = "0.9" snafu = "0.8" -stackable-operator = { git = "https://github.com/stackabletech/operator-rs.git", tag = "stackable-operator-0.80.0" } +stackable-operator = { git = "https://github.com/stackabletech/operator-rs.git", tag = "stackable-operator-0.82.0" } strum = { version = "0.26", features = ["derive"] } tokio = { version = "1.40", features = ["full"] } tracing = "0.1" # [patch."https://github.com/stackabletech/operator-rs.git"] # stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch = "main" } +# stackable-operator = { path = "../operator-rs/crates/stackable-operator" } diff --git a/crate-hashes.json b/crate-hashes.json index 562fb18b..0ca37e6e 100644 --- a/crate-hashes.json +++ b/crate-hashes.json @@ -1,6 +1,6 @@ { - "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.80.0#stackable-operator-derive@0.3.1": "16jrq3wdwz63210jgmqbx3snrr15wxw6l1smqhzv7b7jpq8qvya3", - "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.80.0#stackable-operator@0.80.0": "16jrq3wdwz63210jgmqbx3snrr15wxw6l1smqhzv7b7jpq8qvya3", - "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.80.0#stackable-shared@0.0.1": "16jrq3wdwz63210jgmqbx3snrr15wxw6l1smqhzv7b7jpq8qvya3", + "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.82.0#stackable-operator-derive@0.3.1": "0phasjwb64rxgn5hs8vks92icmx9255bd5v9dms280clrfpcg4hy", + "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.82.0#stackable-operator@0.82.0": "0phasjwb64rxgn5hs8vks92icmx9255bd5v9dms280clrfpcg4hy", + "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.82.0#stackable-shared@0.0.1": "0phasjwb64rxgn5hs8vks92icmx9255bd5v9dms280clrfpcg4hy", "git+https://github.com/stackabletech/product-config.git?tag=0.7.0#product-config@0.7.0": "0gjsm80g6r75pm3824dcyiz4ysq1ka4c1if6k1mjm9cnd5ym0gny" } \ No newline at end of file diff --git a/rust/crd/src/authentication.rs b/rust/crd/src/authentication.rs index f4f38dfd..d972c1a8 100644 --- a/rust/crd/src/authentication.rs +++ b/rust/crd/src/authentication.rs @@ -141,7 +141,7 @@ pub enum SupersetAuthenticationClassResolved { }, Oidc { provider: oidc::AuthenticationProvider, - oidc: oidc::ClientAuthenticationOptions<()>, + client_auth_options: oidc::ClientAuthenticationOptions<()>, }, } @@ -310,7 +310,7 @@ impl SupersetClientAuthenticationDetailsResolved { Ok(SupersetAuthenticationClassResolved::Oidc { provider: provider.to_owned(), - oidc: auth_details + client_auth_options: auth_details .common .oidc_or_error(auth_class_name) .context(OidcConfigurationInvalidSnafu)? @@ -415,7 +415,7 @@ mod tests { oidc: hostname: first.oidc.server port: 443 - rootPath: /realms/main + rootPath: /realms/main/ principalClaim: preferred_username scopes: - openid @@ -436,7 +436,7 @@ mod tests { provider: oidc: hostname: second.oidc.server - rootPath: /realms/test + rootPath: /realms/test/ principalClaim: preferred_username scopes: - openid @@ -453,7 +453,7 @@ mod tests { provider: oidc::AuthenticationProvider::new( HostName::try_from("first.oidc.server".to_string()).unwrap(), Some(443), - "/realms/main".into(), + "/realms/main/".into(), TlsClientDetails { tls: Some(Tls { verification: TlsVerification::Server(TlsServerVerification { @@ -465,7 +465,7 @@ mod tests { vec!["openid".into(), "email".into(), "profile".into()], Some(IdentityProviderHint::Keycloak) ), - oidc: oidc::ClientAuthenticationOptions { + client_auth_options: oidc::ClientAuthenticationOptions { client_credentials_secret_ref: "superset-oidc-client1".into(), extra_scopes: vec!["groups".into()], product_specific_fields: () @@ -475,13 +475,13 @@ mod tests { provider: oidc::AuthenticationProvider::new( HostName::try_from("second.oidc.server".to_string()).unwrap(), None, - "/realms/test".into(), + "/realms/test/".into(), TlsClientDetails { tls: None }, "preferred_username".into(), vec!["openid".into(), "email".into(), "profile".into()], None ), - oidc: oidc::ClientAuthenticationOptions { + client_auth_options: oidc::ClientAuthenticationOptions { client_credentials_secret_ref: "superset-oidc-client2".into(), extra_scopes: Vec::new(), product_specific_fields: () diff --git a/rust/operator-binary/Cargo.toml b/rust/operator-binary/Cargo.toml index 68d171d9..261fd8e4 100644 --- a/rust/operator-binary/Cargo.toml +++ b/rust/operator-binary/Cargo.toml @@ -24,5 +24,8 @@ strum.workspace = true tokio.workspace = true tracing.workspace = true +[dev-dependencies] +rstest.workspace = true + [build-dependencies] built.workspace = true diff --git a/rust/operator-binary/src/config.rs b/rust/operator-binary/src/config.rs index 21c2b562..ee01a5f3 100644 --- a/rust/operator-binary/src/config.rs +++ b/rust/operator-binary/src/config.rs @@ -16,6 +16,16 @@ pub enum Error { FailedToCreateLdapEndpointUrl { source: stackable_operator::commons::authentication::ldap::Error, }, + + #[snafu(display("invalid OIDC endpoint"))] + InvalidOidcEndpoint { + source: stackable_operator::commons::authentication::oidc::Error, + }, + + #[snafu(display("invalid well-known OIDC configuration URL"))] + InvalidWellKnownConfigUrl { + source: stackable_operator::commons::authentication::oidc::Error, + }, } pub const PYTHON_IMPORTS: &[&str] = &[ @@ -79,7 +89,11 @@ fn append_authentication_config( .authentication_classes_resolved .iter() .filter_map(|auth_class| { - if let SupersetAuthenticationClassResolved::Oidc { provider, oidc } = auth_class { + if let SupersetAuthenticationClassResolved::Oidc { + provider, + client_auth_options: oidc, + } = auth_class + { Some((provider, oidc)) } else { None @@ -92,7 +106,7 @@ fn append_authentication_config( } if !oidc_providers.is_empty() { - append_oidc_config(config, &oidc_providers); + append_oidc_config(config, &oidc_providers)?; } config.insert( @@ -191,7 +205,7 @@ fn append_oidc_config( &oidc::AuthenticationProvider, &oidc::ClientAuthenticationOptions<()>, )], -) { +) -> Result<(), Error> { config.insert( SupersetConfigOptions::AuthType.to_string(), "AUTH_OAUTH".into(), @@ -214,6 +228,12 @@ fn append_oidc_config( let oauth_providers_config_entry = match oidc_provider { oidc::IdentityProviderHint::Keycloak => { + let endpoint_url = oidc.endpoint_url().context(InvalidOidcEndpointSnafu)?; + let mut api_base_url = endpoint_url.as_str().trim_end_matches('/').to_owned(); + api_base_url.push_str("/protocol/"); + let well_known_config_url = oidc + .well_known_config_url() + .context(InvalidWellKnownConfigUrlSnafu)?; formatdoc!( " {{ 'name': 'keycloak', @@ -225,11 +245,10 @@ fn append_oidc_config( 'client_kwargs': {{ 'scope': '{scopes}' }}, - 'api_base_url': '{url}/protocol/', - 'server_metadata_url': '{url}/.well-known/openid-configuration', + 'api_base_url': '{api_base_url}', + 'server_metadata_url': '{well_known_config_url}', }}, }}", - url = oidc.endpoint_url().unwrap(), scopes = scopes.join(" "), ) } @@ -248,4 +267,89 @@ fn append_oidc_config( joined_oauth_providers_config = oauth_providers_config.join(",\n") ), ); + + Ok(()) +} + +#[cfg(test)] +mod tests { + use rstest::rstest; + use stackable_operator::commons::tls_verification::{TlsClientDetails, TlsVerification}; + + use super::*; + + #[rstest] + #[case( + "/", + "https://keycloak.mycorp.org/protocol/", + "https://keycloak.mycorp.org/.well-known/openid-configuration" + )] + #[case( + "", + "https://keycloak.mycorp.org/protocol/", + "https://keycloak.mycorp.org/.well-known/openid-configuration" + )] + #[case( + "/realms/sdp", + "https://keycloak.mycorp.org/realms/sdp/protocol/", + "https://keycloak.mycorp.org/realms/sdp/.well-known/openid-configuration" + )] + #[case( + "/realms/sdp/", + "https://keycloak.mycorp.org/realms/sdp/protocol/", + "https://keycloak.mycorp.org/realms/sdp/.well-known/openid-configuration" + )] + #[case( + "/realms/sdp/////", + "https://keycloak.mycorp.org/realms/sdp/protocol/", + "https://keycloak.mycorp.org/realms/sdp/.well-known/openid-configuration" + )] + fn test_append_oidc_config( + #[case] root_path: String, + #[case] expected_api_base_url: &str, + #[case] expected_server_metadata_url: &str, + ) { + use stackable_operator::commons::tls_verification::{CaCert, Tls, TlsServerVerification}; + + let mut properties = BTreeMap::new(); + let provider = oidc::AuthenticationProvider::new( + "keycloak.mycorp.org".to_owned().try_into().unwrap(), + Some(443), + root_path, + TlsClientDetails { + tls: Some(Tls { + verification: TlsVerification::Server(TlsServerVerification { + ca_cert: CaCert::WebPki {}, + }), + }), + }, + "preferred_username".to_owned(), + vec!["openid".to_owned()], + None, + ); + let oidc = oidc::ClientAuthenticationOptions { + client_credentials_secret_ref: "nifi-keycloak-client".to_owned(), + extra_scopes: vec![], + product_specific_fields: (), + }; + + append_oidc_config(&mut properties, &[(&provider, &oidc)]) + .expect("OIDC config adding failed"); + + assert_eq!(properties.get("AUTH_TYPE"), Some(&"AUTH_OAUTH".to_owned())); + let oauth_providers = properties + .get("OAUTH_PROVIDERS") + .expect("OAUTH_PROVIDERS missing"); + + // This is neither valid yaml or json (it's Python code), so we can not easily parse it and have nice assertions. + // As we don't want to have a Python runtime just for this test, let's grep a bit... + assert!(oauth_providers.contains("'name': 'keycloak'")); + assert!(oauth_providers.contains("client_id': os.environ.get(")); + assert!(oauth_providers.contains("client_secret': os.environ.get(")); + assert!(oauth_providers.contains("'scope': 'openid'")); + assert!(oauth_providers.contains(&format!("'api_base_url': '{expected_api_base_url}'"))); + assert!(oauth_providers.contains(&format!( + "'server_metadata_url': '{expected_server_metadata_url}'" + ))); + } } diff --git a/rust/operator-binary/src/superset_controller.rs b/rust/operator-binary/src/superset_controller.rs index f47a2a68..cd02211b 100644 --- a/rust/operator-binary/src/superset_controller.rs +++ b/rust/operator-binary/src/superset_controller.rs @@ -991,7 +991,10 @@ fn authentication_env_vars( for auth_class_resolved in &auth_config.authentication_classes_resolved { match auth_class_resolved { SupersetAuthenticationClassResolved::Ldap { .. } => {} - SupersetAuthenticationClassResolved::Oidc { oidc, .. } => { + SupersetAuthenticationClassResolved::Oidc { + client_auth_options: oidc, + .. + } => { oidc_client_credentials_secrets .insert(oidc.client_credentials_secret_ref.to_owned()); } diff --git a/tests/templates/kuttl/oidc/install-keycloak.yaml.j2 b/tests/templates/kuttl/oidc/install-keycloak.yaml.j2 index 7197df2f..5509b5c5 100644 --- a/tests/templates/kuttl/oidc/install-keycloak.yaml.j2 +++ b/tests/templates/kuttl/oidc/install-keycloak.yaml.j2 @@ -127,7 +127,7 @@ spec: oidc: hostname: $INSTANCE_NAME.$NAMESPACE.svc.cluster.local port: 8443 - rootPath: /realms/$REALM + rootPath: /realms/$REALM/ scopes: - email - openid