From 17a6049d73ce24216795ef8a952b25cac2cab453 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Mon, 4 Nov 2024 19:29:41 +0100 Subject: [PATCH 01/14] Adding start up commands for airflow --- rust/crd/src/lib.rs | 56 ++++++++++++++++--- .../operator-binary/src/airflow_controller.rs | 2 +- rust/operator-binary/src/env_vars.rs | 8 --- .../kuttl/oidc/install-keycloak.yaml.j2 | 18 +++++- 4 files changed, 66 insertions(+), 18 deletions(-) diff --git a/rust/crd/src/lib.rs b/rust/crd/src/lib.rs index d51e4e6a..0eccb7b9 100644 --- a/rust/crd/src/lib.rs +++ b/rust/crd/src/lib.rs @@ -1,5 +1,6 @@ -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; +use authentication::AirflowClientAuthenticationDetailsResolved; use git_sync::GitSync; use product_config::flask_app_config_writer::{FlaskAppConfigOptions, PythonType}; use serde::{Deserialize, Serialize}; @@ -38,7 +39,7 @@ use strum::{Display, EnumIter, EnumString, IntoEnumIterator}; use crate::{ affinity::{get_affinity, get_executor_affinity}, - authentication::AirflowClientAuthenticationDetails, + authentication::{AirflowAuthenticationClassResolved, AirflowClientAuthenticationDetails}, }; pub mod affinity; @@ -322,7 +323,10 @@ impl AirflowRole { /// components to have the same image/configuration (e.g. DAG folder location), even if not all /// configuration settings are used everywhere. For this reason we ensure that the webserver /// config file is in the Airflow home directory on all pods. - pub fn get_commands(&self) -> Vec { + pub fn get_commands( + &self, + auth_config: &AirflowClientAuthenticationDetailsResolved, + ) -> Vec { let mut command = vec![ format!("cp -RL {CONFIG_PATH}/{AIRFLOW_CONFIG_FILENAME} {AIRFLOW_HOME}/{AIRFLOW_CONFIG_FILENAME}"), // graceful shutdown part @@ -331,10 +335,14 @@ impl AirflowRole { ]; match &self { - AirflowRole::Webserver => command.extend(vec![ - "prepare_signal_handlers".to_string(), - "airflow webserver &".to_string(), - ]), + AirflowRole::Webserver => { + let auth_commands = vec![Self::authentication_start_commands(auth_config)]; + command.extend(vec![ + "prepare_signal_handlers".to_string(), + "airflow webserver &".to_string(), + ]); + command.extend(auth_commands); + } AirflowRole::Scheduler => command.extend(vec![ // Database initialization is limited to the scheduler, see https://github.com/stackabletech/airflow-operator/issues/259 "airflow db init".to_string(), @@ -363,7 +371,41 @@ impl AirflowRole { command } + fn authentication_start_commands( + auth_config: &AirflowClientAuthenticationDetailsResolved, + ) -> String { + let mut commands = Vec::new(); + + let mut tls_client_credentials = BTreeSet::new(); + for auth_class_resolved in &auth_config.authentication_classes_resolved { + match auth_class_resolved { + AirflowAuthenticationClassResolved::Oidc { provider, .. } => { + tls_client_credentials.insert(&provider.tls); + + // WebPKI will be handled implicitly + } + AirflowAuthenticationClassResolved::Ldap { .. } => {} + } + } + + for tls in tls_client_credentials { + commands.push(tls.tls_ca_cert_mount_path().map(|tls_ca_cert_mount_path| { + Self::add_cert_to_python_certifi_command(&tls_ca_cert_mount_path) + })); + } + + commands + .iter() + .flatten() + .cloned() + .collect::>() + .join("\n") + } + // Adding certificate to the mount path for airflow startup commands + fn add_cert_to_python_certifi_command(cert_file: &str) -> String { + format!("cat {cert_file} >> \"$(python -c 'import certifi; print(certifi.where())')\"") + } /// Will be used to expose service ports and - by extension - which roles should be /// created as services. pub fn get_http_port(&self) -> Option { diff --git a/rust/operator-binary/src/airflow_controller.rs b/rust/operator-binary/src/airflow_controller.rs index 97c96713..c5b9efbb 100644 --- a/rust/operator-binary/src/airflow_controller.rs +++ b/rust/operator-binary/src/airflow_controller.rs @@ -883,7 +883,7 @@ fn build_server_rolegroup_statefulset( .context(GracefulShutdownSnafu)?; let mut airflow_container_args = Vec::new(); - airflow_container_args.extend(airflow_role.get_commands()); + airflow_container_args.extend(airflow_role.get_commands(authentication_config)); airflow_container .image_from_product_image(resolved_product_image) diff --git a/rust/operator-binary/src/env_vars.rs b/rust/operator-binary/src/env_vars.rs index 5d581090..e1e77a5b 100644 --- a/rust/operator-binary/src/env_vars.rs +++ b/rust/operator-binary/src/env_vars.rs @@ -200,14 +200,6 @@ pub fn build_airflow_statefulset_envs( AirflowRole::Webserver => { let auth_vars = authentication_env_vars(auth_config); env.extend(auth_vars.into_iter().map(|var| (var.name.to_owned(), var))); - env.insert( - "REQUESTS_CA_BUNDLE".into(), - EnvVar { - name: "REQUESTS_CA_BUNDLE".to_string(), - value: Some("/stackable/secrets/tls/ca.crt".to_string()), - ..Default::default() - }, - ); } _ => {} } diff --git a/tests/templates/kuttl/oidc/install-keycloak.yaml.j2 b/tests/templates/kuttl/oidc/install-keycloak.yaml.j2 index 7197df2f..6c320e93 100644 --- a/tests/templates/kuttl/oidc/install-keycloak.yaml.j2 +++ b/tests/templates/kuttl/oidc/install-keycloak.yaml.j2 @@ -104,7 +104,7 @@ spec: csi: driver: secrets.stackable.tech volumeAttributes: - secrets.stackable.tech/class: tls + secrets.stackable.tech/class: tls-keycloak secrets.stackable.tech/scope: service=$INSTANCE_NAME --- apiVersion: v1 @@ -138,7 +138,7 @@ spec: verification: server: caCert: - secretClass: tls + secretClass: tls-keycloak --- apiVersion: v1 kind: ServiceAccount @@ -168,3 +168,17 @@ roleRef: kind: Role name: keycloak apiGroup: rbac.authorization.k8s.io + +--- +apiVersion: secrets.stackable.tech/v1alpha1 +kind: SecretClass +metadata: + name: tls-keycloak +spec: + backend: + autoTls: + ca: + secret: + name: secret-provisioner-tls-keycloak-ca + namespace: ${NAMESPACE} + autoGenerate: true From 8b0985886055dfa3effc2245ea52a03ed8619c4d Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Tue, 5 Nov 2024 09:52:38 +0100 Subject: [PATCH 02/14] Adding variable to env --- rust/crd/src/authentication.rs | 134 +++++++++++++++++++++++++-- rust/crd/src/lib.rs | 4 +- rust/operator-binary/src/env_vars.rs | 13 +++ 3 files changed, 141 insertions(+), 10 deletions(-) diff --git a/rust/crd/src/authentication.rs b/rust/crd/src/authentication.rs index d769f163..50d09b0a 100644 --- a/rust/crd/src/authentication.rs +++ b/rust/crd/src/authentication.rs @@ -4,10 +4,13 @@ use serde::{Deserialize, Serialize}; use snafu::{ensure, ResultExt, Snafu}; use stackable_operator::{ client::Client, - commons::authentication::{ - ldap, - oidc::{self, IdentityProviderHint}, - AuthenticationClass, AuthenticationClassProvider, ClientAuthenticationDetails, + commons::{ + authentication::{ + ldap, + oidc::{self, IdentityProviderHint}, + AuthenticationClass, AuthenticationClassProvider, ClientAuthenticationDetails, + }, + tls_verification::TlsClientDetails, }, schemars::{self, JsonSchema}, }; @@ -78,6 +81,8 @@ pub enum Error { supported: String, auth_class_name: String, }, + #[snafu(display("Currently only one CA certificate is supported."))] + MultipleCaCertsNotSupported, } type Result = std::result::Result; @@ -130,6 +135,7 @@ pub struct AirflowClientAuthenticationDetailsResolved { pub user_registration: bool, pub user_registration_role: String, pub sync_roles_at: FlaskRolesSyncMoment, + pub tls_ca_cert_mount_path: Option, } #[derive(Clone, Debug, Eq, PartialEq)] @@ -143,6 +149,24 @@ pub enum AirflowAuthenticationClassResolved { }, } +impl AirflowAuthenticationClassResolved { + pub fn tls_ca_cert_mount_path(&self) -> Option { + self.tls_client_details().tls_ca_cert_mount_path() + } + + pub fn tls_client_details(&self) -> &TlsClientDetails { + match self { + AirflowAuthenticationClassResolved::Ldap { + provider: ldap::AuthenticationProvider { tls, .. }, + } => tls, + AirflowAuthenticationClassResolved::Oidc { + provider: oidc::AuthenticationProvider { tls, .. }, + .. + } => tls, + } + } +} + impl AirflowClientAuthenticationDetailsResolved { pub async fn from( auth_details: &[AirflowClientAuthenticationDetails], @@ -254,12 +278,24 @@ impl AirflowClientAuthenticationDetailsResolved { None => sync_roles_at = Some(entry.sync_roles_at.to_owned()), } } + + let mut tls_ca_cert_mount_paths = resolved_auth_classes + .iter() + .filter_map(AirflowAuthenticationClassResolved::tls_ca_cert_mount_path) + .collect::>(); + let tls_ca_cert_mount_path = tls_ca_cert_mount_paths.pop_first(); + ensure!( + tls_ca_cert_mount_paths.is_empty(), + MultipleCaCertsNotSupportedSnafu + ); + Ok(AirflowClientAuthenticationDetailsResolved { authentication_classes_resolved: resolved_auth_classes, user_registration: user_registration.unwrap_or_else(default_user_registration), user_registration_role: user_registration_role .unwrap_or_else(default_user_registration_role), sync_roles_at: sync_roles_at.unwrap_or_else(FlaskRolesSyncMoment::default), + tls_ca_cert_mount_path, }) } @@ -336,7 +372,8 @@ mod tests { authentication_classes_resolved: Vec::default(), user_registration: default_user_registration(), user_registration_role: default_user_registration_role(), - sync_roles_at: FlaskRolesSyncMoment::default() + sync_roles_at: FlaskRolesSyncMoment::default(), + tls_ca_cert_mount_path: None }, auth_details_resolved ); @@ -362,6 +399,11 @@ mod tests { provider: ldap: hostname: my.ldap.server + tls: + verification: + server: + caCert: + secretClass: tls-keycloak "}, ) .await; @@ -369,11 +411,20 @@ mod tests { assert_eq!( AirflowClientAuthenticationDetailsResolved { authentication_classes_resolved: vec![AirflowAuthenticationClassResolved::Ldap { - provider: serde_yaml::from_str("hostname: my.ldap.server").unwrap() + provider: serde_yaml::from_str(indoc! {" + hostname: my.ldap.server + tls: + verification: + server: + caCert: + secretClass: tls + "}) + .unwrap() }], user_registration: false, user_registration_role: "Gamma".into(), - sync_roles_at: FlaskRolesSyncMoment::Login + sync_roles_at: FlaskRolesSyncMoment::Login, + tls_ca_cert_mount_path: Some("/stackable/secrets/tls-keycloak/ca.crt".into()), }, auth_details_resolved ); @@ -437,6 +488,11 @@ mod tests { - openid - email - profile + tls: + verification: + server: + caCert: + secretClass: tls-keycloak "}, ) .await; @@ -471,7 +527,13 @@ mod tests { HostName::try_from("second.oidc.server".to_string()).unwrap(), None, "/realms/test".into(), - TlsClientDetails { tls: None }, + TlsClientDetails { + tls: Some(Tls { + verification: TlsVerification::Server(TlsServerVerification { + ca_cert: CaCert::SecretClass("tls".into()) + }) + }) + }, "preferred_username".into(), vec!["openid".into(), "email".into(), "profile".into()], None @@ -485,7 +547,8 @@ mod tests { ], user_registration: false, user_registration_role: "Gamma".into(), - sync_roles_at: FlaskRolesSyncMoment::Login + sync_roles_at: FlaskRolesSyncMoment::Login, + tls_ca_cert_mount_path: Some("/stackable/secrets/tls/ca.crt".into()), }, auth_details_resolved ); @@ -765,6 +828,59 @@ mod tests { error_message ); } + #[tokio::test] + async fn reject_different_tls_ca_certs() { + let error_message = test_resolve_and_expect_error( + indoc! {" + - authenticationClass: oidc1 + oidc: + clientCredentialsSecret: airflow-oidc-client1 + - authenticationClass: oidc2 + oidc: + clientCredentialsSecret: airflow-oidc-client2 + "}, + indoc! {" + --- + apiVersion: authentication.stackable.tech/v1alpha1 + kind: AuthenticationClass + metadata: + name: oidc1 + spec: + provider: + oidc: + hostname: first.oidc.server + principalClaim: preferred_username + scopes: [] + tls: + verification: + server: + caCert: + secretClass: tls1 + --- + apiVersion: authentication.stackable.tech/v1alpha1 + kind: AuthenticationClass + metadata: + name: oidc2 + spec: + provider: + oidc: + hostname: second.oidc.server + principalClaim: preferred_username + scopes: [] + tls: + verification: + server: + caCert: + secretClass: tls2 + "}, + ) + .await; + + assert_eq!( + "Currently only one CA certificate is supported.", + error_message + ); + } #[tokio::test] async fn reject_wrong_principal_claim() { diff --git a/rust/crd/src/lib.rs b/rust/crd/src/lib.rs index 0eccb7b9..5b030849 100644 --- a/rust/crd/src/lib.rs +++ b/rust/crd/src/lib.rs @@ -4,7 +4,7 @@ use authentication::AirflowClientAuthenticationDetailsResolved; use git_sync::GitSync; use product_config::flask_app_config_writer::{FlaskAppConfigOptions, PythonType}; use serde::{Deserialize, Serialize}; -use snafu::{OptionExt, ResultExt, Snafu}; +use snafu::{ensure, OptionExt, ResultExt, Snafu}; use stackable_operator::{ commons::{ affinity::StackableAffinity, @@ -343,6 +343,7 @@ impl AirflowRole { ]); command.extend(auth_commands); } + AirflowRole::Scheduler => command.extend(vec![ // Database initialization is limited to the scheduler, see https://github.com/stackabletech/airflow-operator/issues/259 "airflow db init".to_string(), @@ -406,6 +407,7 @@ impl AirflowRole { fn add_cert_to_python_certifi_command(cert_file: &str) -> String { format!("cat {cert_file} >> \"$(python -c 'import certifi; print(certifi.where())')\"") } + /// Will be used to expose service ports and - by extension - which roles should be /// created as services. pub fn get_http_port(&self) -> Option { diff --git a/rust/operator-binary/src/env_vars.rs b/rust/operator-binary/src/env_vars.rs index e1e77a5b..046fe8fd 100644 --- a/rust/operator-binary/src/env_vars.rs +++ b/rust/operator-binary/src/env_vars.rs @@ -200,7 +200,20 @@ pub fn build_airflow_statefulset_envs( AirflowRole::Webserver => { let auth_vars = authentication_env_vars(auth_config); env.extend(auth_vars.into_iter().map(|var| (var.name.to_owned(), var))); + // TODO: Add TLS certificate to env. + + if let Some(tls_ca_cert_mount_path) = &auth_config.tls_ca_cert_mount_path { + env.insert( + "REQUESTS_CA_BUNDLE".into(), + EnvVar { + name: "REQUESTS_CA_BUNDLE".to_string(), + value: Some(tls_ca_cert_mount_path.to_owned()), + ..Default::default() + }, + ); + } } + _ => {} } // apply overrides last of all with a fixed ordering From 7846a18cfa0676223a9a2dbdcfdcab7c3a391b6e Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Tue, 5 Nov 2024 09:56:38 +0100 Subject: [PATCH 03/14] Making lints happy --- Cargo.lock | 6 +++--- rust/crd/src/lib.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 247ba76a..0efb5c36 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -803,9 +803,9 @@ dependencies = [ [[package]] name = "hashbrown" -version = "0.15.0" +version = "0.15.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1e087f84d4f86bf4b218b927129862374b72199ae7d8657835f1e89000eea4fb" +checksum = "3a9bfc1af68b1726ea47d3d5109de126281def866b33970e10fbab11b5dafab3" [[package]] name = "headers" @@ -1034,7 +1034,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "707907fe3c25f5424cce2cb7e1cbcafee6bdbe735ca90ef77c29e84591e5b9da" dependencies = [ "equivalent", - "hashbrown 0.15.0", + "hashbrown 0.15.1", ] [[package]] diff --git a/rust/crd/src/lib.rs b/rust/crd/src/lib.rs index 5b030849..2d581f30 100644 --- a/rust/crd/src/lib.rs +++ b/rust/crd/src/lib.rs @@ -4,7 +4,7 @@ use authentication::AirflowClientAuthenticationDetailsResolved; use git_sync::GitSync; use product_config::flask_app_config_writer::{FlaskAppConfigOptions, PythonType}; use serde::{Deserialize, Serialize}; -use snafu::{ensure, OptionExt, ResultExt, Snafu}; +use snafu::{OptionExt, ResultExt, Snafu}; use stackable_operator::{ commons::{ affinity::StackableAffinity, From 2a0f0dfe735043d3f4d73b88329dc85e7bf2a3dd Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Tue, 5 Nov 2024 11:05:41 +0100 Subject: [PATCH 04/14] fixing tests --- rust/operator-binary/src/config.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/rust/operator-binary/src/config.rs b/rust/operator-binary/src/config.rs index 2d7feff7..b9be716d 100644 --- a/rust/operator-binary/src/config.rs +++ b/rust/operator-binary/src/config.rs @@ -271,6 +271,7 @@ mod tests { user_registration: true, user_registration_role: "User".to_string(), sync_roles_at: FlaskRolesSyncMoment::Registration, + tls_ca_cert_mount_path: None, }; let mut result = BTreeMap::new(); @@ -314,6 +315,7 @@ mod tests { user_registration: true, user_registration_role: "Admin".to_string(), sync_roles_at: FlaskRolesSyncMoment::Registration, + tls_ca_cert_mount_path: Some("stackable/secrets/openldap-tls".to_string()), }; let mut result = BTreeMap::new(); @@ -394,6 +396,7 @@ mod tests { user_registration: default_user_registration(), user_registration_role: "Admin".to_string(), sync_roles_at: default_sync_roles_at(), + tls_ca_cert_mount_path: None, }; let mut result = BTreeMap::new(); From 51487faaf82663ebc16754d53a45ecca509cc3b7 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Tue, 5 Nov 2024 11:43:21 +0100 Subject: [PATCH 05/14] fixing unit tests --- rust/crd/src/authentication.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/crd/src/authentication.rs b/rust/crd/src/authentication.rs index 50d09b0a..01c9ba81 100644 --- a/rust/crd/src/authentication.rs +++ b/rust/crd/src/authentication.rs @@ -417,7 +417,7 @@ mod tests { verification: server: caCert: - secretClass: tls + secretClass: tls-keycloak "}) .unwrap() }], @@ -492,7 +492,7 @@ mod tests { verification: server: caCert: - secretClass: tls-keycloak + secretClass: tls "}, ) .await; From c3248e6c82a049aad2fa0064f22dca327e5d7bc8 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Tue, 5 Nov 2024 17:25:37 +0100 Subject: [PATCH 06/14] Looking for the problem --- rust/crd/src/lib.rs | 1 + rust/operator-binary/src/env_vars.rs | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/rust/crd/src/lib.rs b/rust/crd/src/lib.rs index 2d581f30..beefb503 100644 --- a/rust/crd/src/lib.rs +++ b/rust/crd/src/lib.rs @@ -336,6 +336,7 @@ impl AirflowRole { match &self { AirflowRole::Webserver => { + // Getting auth commands for AuthClass let auth_commands = vec![Self::authentication_start_commands(auth_config)]; command.extend(vec![ "prepare_signal_handlers".to_string(), diff --git a/rust/operator-binary/src/env_vars.rs b/rust/operator-binary/src/env_vars.rs index 046fe8fd..9c942122 100644 --- a/rust/operator-binary/src/env_vars.rs +++ b/rust/operator-binary/src/env_vars.rs @@ -200,7 +200,6 @@ pub fn build_airflow_statefulset_envs( AirflowRole::Webserver => { let auth_vars = authentication_env_vars(auth_config); env.extend(auth_vars.into_iter().map(|var| (var.name.to_owned(), var))); - // TODO: Add TLS certificate to env. if let Some(tls_ca_cert_mount_path) = &auth_config.tls_ca_cert_mount_path { env.insert( @@ -213,7 +212,6 @@ pub fn build_airflow_statefulset_envs( ); } } - _ => {} } // apply overrides last of all with a fixed ordering From d34c37fedd70cc5bc11f17684849727922a6289e Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Wed, 6 Nov 2024 09:42:06 +0100 Subject: [PATCH 07/14] Revert "Adding start up commands for airflow" This reverts commit 17a6049d73ce24216795ef8a952b25cac2cab453. --- rust/crd/src/lib.rs | 10 +++------- .../operator-binary/src/airflow_controller.rs | 2 +- rust/operator-binary/src/env_vars.rs | 19 ++++++++----------- .../kuttl/oidc/install-keycloak.yaml.j2 | 18 ++---------------- 4 files changed, 14 insertions(+), 35 deletions(-) diff --git a/rust/crd/src/lib.rs b/rust/crd/src/lib.rs index beefb503..d85e2fb4 100644 --- a/rust/crd/src/lib.rs +++ b/rust/crd/src/lib.rs @@ -1,6 +1,5 @@ -use std::collections::{BTreeMap, BTreeSet}; +use std::collections::BTreeMap; -use authentication::AirflowClientAuthenticationDetailsResolved; use git_sync::GitSync; use product_config::flask_app_config_writer::{FlaskAppConfigOptions, PythonType}; use serde::{Deserialize, Serialize}; @@ -39,7 +38,7 @@ use strum::{Display, EnumIter, EnumString, IntoEnumIterator}; use crate::{ affinity::{get_affinity, get_executor_affinity}, - authentication::{AirflowAuthenticationClassResolved, AirflowClientAuthenticationDetails}, + authentication::AirflowClientAuthenticationDetails, }; pub mod affinity; @@ -323,10 +322,7 @@ impl AirflowRole { /// components to have the same image/configuration (e.g. DAG folder location), even if not all /// configuration settings are used everywhere. For this reason we ensure that the webserver /// config file is in the Airflow home directory on all pods. - pub fn get_commands( - &self, - auth_config: &AirflowClientAuthenticationDetailsResolved, - ) -> Vec { + pub fn get_commands(&self) -> Vec { let mut command = vec![ format!("cp -RL {CONFIG_PATH}/{AIRFLOW_CONFIG_FILENAME} {AIRFLOW_HOME}/{AIRFLOW_CONFIG_FILENAME}"), // graceful shutdown part diff --git a/rust/operator-binary/src/airflow_controller.rs b/rust/operator-binary/src/airflow_controller.rs index c5b9efbb..97c96713 100644 --- a/rust/operator-binary/src/airflow_controller.rs +++ b/rust/operator-binary/src/airflow_controller.rs @@ -883,7 +883,7 @@ fn build_server_rolegroup_statefulset( .context(GracefulShutdownSnafu)?; let mut airflow_container_args = Vec::new(); - airflow_container_args.extend(airflow_role.get_commands(authentication_config)); + airflow_container_args.extend(airflow_role.get_commands()); airflow_container .image_from_product_image(resolved_product_image) diff --git a/rust/operator-binary/src/env_vars.rs b/rust/operator-binary/src/env_vars.rs index 9c942122..5d581090 100644 --- a/rust/operator-binary/src/env_vars.rs +++ b/rust/operator-binary/src/env_vars.rs @@ -200,17 +200,14 @@ pub fn build_airflow_statefulset_envs( AirflowRole::Webserver => { let auth_vars = authentication_env_vars(auth_config); env.extend(auth_vars.into_iter().map(|var| (var.name.to_owned(), var))); - - if let Some(tls_ca_cert_mount_path) = &auth_config.tls_ca_cert_mount_path { - env.insert( - "REQUESTS_CA_BUNDLE".into(), - EnvVar { - name: "REQUESTS_CA_BUNDLE".to_string(), - value: Some(tls_ca_cert_mount_path.to_owned()), - ..Default::default() - }, - ); - } + env.insert( + "REQUESTS_CA_BUNDLE".into(), + EnvVar { + name: "REQUESTS_CA_BUNDLE".to_string(), + value: Some("/stackable/secrets/tls/ca.crt".to_string()), + ..Default::default() + }, + ); } _ => {} } diff --git a/tests/templates/kuttl/oidc/install-keycloak.yaml.j2 b/tests/templates/kuttl/oidc/install-keycloak.yaml.j2 index 6c320e93..7197df2f 100644 --- a/tests/templates/kuttl/oidc/install-keycloak.yaml.j2 +++ b/tests/templates/kuttl/oidc/install-keycloak.yaml.j2 @@ -104,7 +104,7 @@ spec: csi: driver: secrets.stackable.tech volumeAttributes: - secrets.stackable.tech/class: tls-keycloak + secrets.stackable.tech/class: tls secrets.stackable.tech/scope: service=$INSTANCE_NAME --- apiVersion: v1 @@ -138,7 +138,7 @@ spec: verification: server: caCert: - secretClass: tls-keycloak + secretClass: tls --- apiVersion: v1 kind: ServiceAccount @@ -168,17 +168,3 @@ roleRef: kind: Role name: keycloak apiGroup: rbac.authorization.k8s.io - ---- -apiVersion: secrets.stackable.tech/v1alpha1 -kind: SecretClass -metadata: - name: tls-keycloak -spec: - backend: - autoTls: - ca: - secret: - name: secret-provisioner-tls-keycloak-ca - namespace: ${NAMESPACE} - autoGenerate: true From d75b60d92e148e79eff59bea3f206cdc74fba1f7 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Wed, 6 Nov 2024 09:49:58 +0100 Subject: [PATCH 08/14] Using python certify to use ca certs --- rust/crd/src/lib.rs | 10 ++++++++-- rust/operator-binary/src/airflow_controller.rs | 2 +- rust/operator-binary/src/env_vars.rs | 8 -------- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/rust/crd/src/lib.rs b/rust/crd/src/lib.rs index d85e2fb4..408af0ca 100644 --- a/rust/crd/src/lib.rs +++ b/rust/crd/src/lib.rs @@ -1,5 +1,8 @@ -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; +use authentication::{ + AirflowAuthenticationClassResolved, AirflowClientAuthenticationDetailsResolved, +}; use git_sync::GitSync; use product_config::flask_app_config_writer::{FlaskAppConfigOptions, PythonType}; use serde::{Deserialize, Serialize}; @@ -322,7 +325,10 @@ impl AirflowRole { /// components to have the same image/configuration (e.g. DAG folder location), even if not all /// configuration settings are used everywhere. For this reason we ensure that the webserver /// config file is in the Airflow home directory on all pods. - pub fn get_commands(&self) -> Vec { + pub fn get_commands( + &self, + auth_config: &AirflowClientAuthenticationDetailsResolved, + ) -> Vec { let mut command = vec![ format!("cp -RL {CONFIG_PATH}/{AIRFLOW_CONFIG_FILENAME} {AIRFLOW_HOME}/{AIRFLOW_CONFIG_FILENAME}"), // graceful shutdown part diff --git a/rust/operator-binary/src/airflow_controller.rs b/rust/operator-binary/src/airflow_controller.rs index 97c96713..c5b9efbb 100644 --- a/rust/operator-binary/src/airflow_controller.rs +++ b/rust/operator-binary/src/airflow_controller.rs @@ -883,7 +883,7 @@ fn build_server_rolegroup_statefulset( .context(GracefulShutdownSnafu)?; let mut airflow_container_args = Vec::new(); - airflow_container_args.extend(airflow_role.get_commands()); + airflow_container_args.extend(airflow_role.get_commands(authentication_config)); airflow_container .image_from_product_image(resolved_product_image) diff --git a/rust/operator-binary/src/env_vars.rs b/rust/operator-binary/src/env_vars.rs index 5d581090..e1e77a5b 100644 --- a/rust/operator-binary/src/env_vars.rs +++ b/rust/operator-binary/src/env_vars.rs @@ -200,14 +200,6 @@ pub fn build_airflow_statefulset_envs( AirflowRole::Webserver => { let auth_vars = authentication_env_vars(auth_config); env.extend(auth_vars.into_iter().map(|var| (var.name.to_owned(), var))); - env.insert( - "REQUESTS_CA_BUNDLE".into(), - EnvVar { - name: "REQUESTS_CA_BUNDLE".to_string(), - value: Some("/stackable/secrets/tls/ca.crt".to_string()), - ..Default::default() - }, - ); } _ => {} } From 3d8254a62a124f2fd5e804920c3f608baca412ba Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Wed, 6 Nov 2024 10:21:06 +0100 Subject: [PATCH 09/14] Adding comment to AirflowRole --- rust/crd/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rust/crd/src/lib.rs b/rust/crd/src/lib.rs index 408af0ca..8721ed8a 100644 --- a/rust/crd/src/lib.rs +++ b/rust/crd/src/lib.rs @@ -325,6 +325,8 @@ impl AirflowRole { /// components to have the same image/configuration (e.g. DAG folder location), even if not all /// configuration settings are used everywhere. For this reason we ensure that the webserver /// config file is in the Airflow home directory on all pods. + /// Only the webserver needs to know about authentication CA's which is added via python's certify + /// if authentication is enabled. pub fn get_commands( &self, auth_config: &AirflowClientAuthenticationDetailsResolved, From 9fbdf43e27757abf2cdbc0fc6b942e26efd9ad97 Mon Sep 17 00:00:00 2001 From: Maximilian Wittich <56642549+Maleware@users.noreply.github.com> Date: Wed, 6 Nov 2024 15:31:37 +0100 Subject: [PATCH 10/14] Apply suggestions from code review Applying Review Co-authored-by: Siegfried Weber --- rust/crd/src/authentication.rs | 70 +++------------------------------- 1 file changed, 5 insertions(+), 65 deletions(-) diff --git a/rust/crd/src/authentication.rs b/rust/crd/src/authentication.rs index 01c9ba81..defddeb7 100644 --- a/rust/crd/src/authentication.rs +++ b/rust/crd/src/authentication.rs @@ -81,8 +81,6 @@ pub enum Error { supported: String, auth_class_name: String, }, - #[snafu(display("Currently only one CA certificate is supported."))] - MultipleCaCertsNotSupported, } type Result = std::result::Result; @@ -135,7 +133,6 @@ pub struct AirflowClientAuthenticationDetailsResolved { pub user_registration: bool, pub user_registration_role: String, pub sync_roles_at: FlaskRolesSyncMoment, - pub tls_ca_cert_mount_path: Option, } #[derive(Clone, Debug, Eq, PartialEq)] @@ -149,24 +146,6 @@ pub enum AirflowAuthenticationClassResolved { }, } -impl AirflowAuthenticationClassResolved { - pub fn tls_ca_cert_mount_path(&self) -> Option { - self.tls_client_details().tls_ca_cert_mount_path() - } - - pub fn tls_client_details(&self) -> &TlsClientDetails { - match self { - AirflowAuthenticationClassResolved::Ldap { - provider: ldap::AuthenticationProvider { tls, .. }, - } => tls, - AirflowAuthenticationClassResolved::Oidc { - provider: oidc::AuthenticationProvider { tls, .. }, - .. - } => tls, - } - } -} - impl AirflowClientAuthenticationDetailsResolved { pub async fn from( auth_details: &[AirflowClientAuthenticationDetails], @@ -278,24 +257,12 @@ impl AirflowClientAuthenticationDetailsResolved { None => sync_roles_at = Some(entry.sync_roles_at.to_owned()), } } - - let mut tls_ca_cert_mount_paths = resolved_auth_classes - .iter() - .filter_map(AirflowAuthenticationClassResolved::tls_ca_cert_mount_path) - .collect::>(); - let tls_ca_cert_mount_path = tls_ca_cert_mount_paths.pop_first(); - ensure!( - tls_ca_cert_mount_paths.is_empty(), - MultipleCaCertsNotSupportedSnafu - ); - Ok(AirflowClientAuthenticationDetailsResolved { authentication_classes_resolved: resolved_auth_classes, user_registration: user_registration.unwrap_or_else(default_user_registration), user_registration_role: user_registration_role .unwrap_or_else(default_user_registration_role), sync_roles_at: sync_roles_at.unwrap_or_else(FlaskRolesSyncMoment::default), - tls_ca_cert_mount_path, }) } @@ -372,8 +339,7 @@ mod tests { authentication_classes_resolved: Vec::default(), user_registration: default_user_registration(), user_registration_role: default_user_registration_role(), - sync_roles_at: FlaskRolesSyncMoment::default(), - tls_ca_cert_mount_path: None + sync_roles_at: FlaskRolesSyncMoment::default() }, auth_details_resolved ); @@ -399,11 +365,6 @@ mod tests { provider: ldap: hostname: my.ldap.server - tls: - verification: - server: - caCert: - secretClass: tls-keycloak "}, ) .await; @@ -411,20 +372,11 @@ mod tests { assert_eq!( AirflowClientAuthenticationDetailsResolved { authentication_classes_resolved: vec![AirflowAuthenticationClassResolved::Ldap { - provider: serde_yaml::from_str(indoc! {" - hostname: my.ldap.server - tls: - verification: - server: - caCert: - secretClass: tls-keycloak - "}) - .unwrap() + provider: serde_yaml::from_str("hostname: my.ldap.server").unwrap() }], user_registration: false, user_registration_role: "Gamma".into(), - sync_roles_at: FlaskRolesSyncMoment::Login, - tls_ca_cert_mount_path: Some("/stackable/secrets/tls-keycloak/ca.crt".into()), + sync_roles_at: FlaskRolesSyncMoment::Login }, auth_details_resolved ); @@ -488,11 +440,6 @@ mod tests { - openid - email - profile - tls: - verification: - server: - caCert: - secretClass: tls "}, ) .await; @@ -527,13 +474,7 @@ mod tests { HostName::try_from("second.oidc.server".to_string()).unwrap(), None, "/realms/test".into(), - TlsClientDetails { - tls: Some(Tls { - verification: TlsVerification::Server(TlsServerVerification { - ca_cert: CaCert::SecretClass("tls".into()) - }) - }) - }, + TlsClientDetails { tls: None }, "preferred_username".into(), vec!["openid".into(), "email".into(), "profile".into()], None @@ -547,8 +488,7 @@ mod tests { ], user_registration: false, user_registration_role: "Gamma".into(), - sync_roles_at: FlaskRolesSyncMoment::Login, - tls_ca_cert_mount_path: Some("/stackable/secrets/tls/ca.crt".into()), + sync_roles_at: FlaskRolesSyncMoment::Login }, auth_details_resolved ); From 35e22f743776d4c3a934cee14df3299c59604c0c Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Wed, 6 Nov 2024 15:39:42 +0100 Subject: [PATCH 11/14] Further adapting review --- rust/crd/src/authentication.rs | 20 ++++++-------------- rust/operator-binary/src/config.rs | 3 --- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/rust/crd/src/authentication.rs b/rust/crd/src/authentication.rs index defddeb7..52ca758d 100644 --- a/rust/crd/src/authentication.rs +++ b/rust/crd/src/authentication.rs @@ -4,13 +4,10 @@ use serde::{Deserialize, Serialize}; use snafu::{ensure, ResultExt, Snafu}; use stackable_operator::{ client::Client, - commons::{ - authentication::{ - ldap, - oidc::{self, IdentityProviderHint}, - AuthenticationClass, AuthenticationClassProvider, ClientAuthenticationDetails, - }, - tls_verification::TlsClientDetails, + commons::authentication::{ + ldap, + oidc::{self, IdentityProviderHint}, + AuthenticationClass, AuthenticationClassProvider, ClientAuthenticationDetails, }, schemars::{self, JsonSchema}, }; @@ -769,8 +766,8 @@ mod tests { ); } #[tokio::test] - async fn reject_different_tls_ca_certs() { - let error_message = test_resolve_and_expect_error( + async fn accept_different_tls_ca_certs() { + let error_message = test_resolve_and_expect_success( indoc! {" - authenticationClass: oidc1 oidc: @@ -815,11 +812,6 @@ mod tests { "}, ) .await; - - assert_eq!( - "Currently only one CA certificate is supported.", - error_message - ); } #[tokio::test] diff --git a/rust/operator-binary/src/config.rs b/rust/operator-binary/src/config.rs index b9be716d..2d7feff7 100644 --- a/rust/operator-binary/src/config.rs +++ b/rust/operator-binary/src/config.rs @@ -271,7 +271,6 @@ mod tests { user_registration: true, user_registration_role: "User".to_string(), sync_roles_at: FlaskRolesSyncMoment::Registration, - tls_ca_cert_mount_path: None, }; let mut result = BTreeMap::new(); @@ -315,7 +314,6 @@ mod tests { user_registration: true, user_registration_role: "Admin".to_string(), sync_roles_at: FlaskRolesSyncMoment::Registration, - tls_ca_cert_mount_path: Some("stackable/secrets/openldap-tls".to_string()), }; let mut result = BTreeMap::new(); @@ -396,7 +394,6 @@ mod tests { user_registration: default_user_registration(), user_registration_role: "Admin".to_string(), sync_roles_at: default_sync_roles_at(), - tls_ca_cert_mount_path: None, }; let mut result = BTreeMap::new(); From a8baa1399b6100120a9776c2d056131f45d758cc Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Thu, 7 Nov 2024 11:00:25 +0100 Subject: [PATCH 12/14] remove reject_different_tls_ca_certs() --- rust/crd/src/authentication.rs | 49 ---------------------------------- 1 file changed, 49 deletions(-) diff --git a/rust/crd/src/authentication.rs b/rust/crd/src/authentication.rs index 52ca758d..4d64ef26 100644 --- a/rust/crd/src/authentication.rs +++ b/rust/crd/src/authentication.rs @@ -765,55 +765,6 @@ mod tests { error_message ); } - #[tokio::test] - async fn accept_different_tls_ca_certs() { - let error_message = test_resolve_and_expect_success( - indoc! {" - - authenticationClass: oidc1 - oidc: - clientCredentialsSecret: airflow-oidc-client1 - - authenticationClass: oidc2 - oidc: - clientCredentialsSecret: airflow-oidc-client2 - "}, - indoc! {" - --- - apiVersion: authentication.stackable.tech/v1alpha1 - kind: AuthenticationClass - metadata: - name: oidc1 - spec: - provider: - oidc: - hostname: first.oidc.server - principalClaim: preferred_username - scopes: [] - tls: - verification: - server: - caCert: - secretClass: tls1 - --- - apiVersion: authentication.stackable.tech/v1alpha1 - kind: AuthenticationClass - metadata: - name: oidc2 - spec: - provider: - oidc: - hostname: second.oidc.server - principalClaim: preferred_username - scopes: [] - tls: - verification: - server: - caCert: - secretClass: tls2 - "}, - ) - .await; - } - #[tokio::test] async fn reject_wrong_principal_claim() { let error_message = test_resolve_and_expect_error( From 187fbffdd173620a3aaa2c01def55efde4dc9836 Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Fri, 8 Nov 2024 16:57:38 +0100 Subject: [PATCH 13/14] Update Cargo.nix --- CHANGELOG.md | 3 ++- Cargo.nix | 9 ++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68f9cbac..d19d70e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ Use the env var `KUBERNETES_CLUSTER_DOMAIN` or the operator Helm chart property `kubernetesClusterDomain` to set a non-default cluster domain ([#518]). - Support for `2.9.3` ([#494]). - Experimental Support for `2.10.2` ([#512]). -- Add support for OpenID Connect ([#524]) +- Add support for OpenID Connect ([#524], [#530]) ### Changed @@ -32,6 +32,7 @@ [#518]: https://github.com/stackabletech/airflow-operator/pull/518 [#520]: https://github.com/stackabletech/airflow-operator/pull/520 [#524]: https://github.com/stackabletech/airflow-operator/pull/524 +[#530]: https://github.com/stackabletech/airflow-operator/pull/530 ## [24.7.0] - 2024-07-24 diff --git a/Cargo.nix b/Cargo.nix index afc575fd..22c13476 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -2284,18 +2284,17 @@ rec { }; resolvedDefaultFeatures = [ "ahash" "allocator-api2" "default" "inline-more" ]; }; - "hashbrown 0.15.0" = rec { + "hashbrown 0.15.1" = rec { crateName = "hashbrown"; - version = "0.15.0"; + version = "0.15.1"; edition = "2021"; - sha256 = "1yx4xq091s7i6mw6bn77k8cp4jrpcac149xr32rg8szqsj27y20y"; + sha256 = "1czsvasi3azv2079fcvbhvpisa16w6fi1mfk8zm2c5wbyqdgr6rs"; authors = [ "Amanieu d'Antras " ]; features = { "alloc" = [ "dep:alloc" ]; "allocator-api2" = [ "dep:allocator-api2" ]; - "borsh" = [ "dep:borsh" ]; "compiler_builtins" = [ "dep:compiler_builtins" ]; "core" = [ "dep:core" ]; "default" = [ "default-hasher" "inline-more" "allocator-api2" "equivalent" "raw-entry" ]; @@ -3062,7 +3061,7 @@ rec { } { name = "hashbrown"; - packageId = "hashbrown 0.15.0"; + packageId = "hashbrown 0.15.1"; usesDefaultFeatures = false; } ]; From b4d0e2fb818c5144a293d0583d4232eef9216661 Mon Sep 17 00:00:00 2001 From: Maxi Wittich Date: Mon, 11 Nov 2024 09:19:38 +0100 Subject: [PATCH 14/14] Adding code review --- rust/crd/src/authentication.rs | 1 + rust/crd/src/lib.rs | 14 +++++--------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/rust/crd/src/authentication.rs b/rust/crd/src/authentication.rs index 4d64ef26..d769f163 100644 --- a/rust/crd/src/authentication.rs +++ b/rust/crd/src/authentication.rs @@ -765,6 +765,7 @@ mod tests { error_message ); } + #[tokio::test] async fn reject_wrong_principal_claim() { let error_message = test_resolve_and_expect_error( diff --git a/rust/crd/src/lib.rs b/rust/crd/src/lib.rs index 8721ed8a..7d7be665 100644 --- a/rust/crd/src/lib.rs +++ b/rust/crd/src/lib.rs @@ -341,12 +341,11 @@ impl AirflowRole { match &self { AirflowRole::Webserver => { // Getting auth commands for AuthClass - let auth_commands = vec![Self::authentication_start_commands(auth_config)]; + command.extend(Self::authentication_start_commands(auth_config)); command.extend(vec![ "prepare_signal_handlers".to_string(), "airflow webserver &".to_string(), ]); - command.extend(auth_commands); } AirflowRole::Scheduler => command.extend(vec![ @@ -377,9 +376,10 @@ impl AirflowRole { command } + fn authentication_start_commands( auth_config: &AirflowClientAuthenticationDetailsResolved, - ) -> String { + ) -> Vec { let mut commands = Vec::new(); let mut tls_client_credentials = BTreeSet::new(); @@ -401,13 +401,9 @@ impl AirflowRole { })); } - commands - .iter() - .flatten() - .cloned() - .collect::>() - .join("\n") + commands.iter().flatten().cloned().collect::>() } + // Adding certificate to the mount path for airflow startup commands fn add_cert_to_python_certifi_command(cert_file: &str) -> String { format!("cat {cert_file} >> \"$(python -c 'import certifi; print(certifi.where())')\"")