From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 0D0871FF139 for ; Tue, 10 Feb 2026 16:52:47 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 5E1A480A5; Tue, 10 Feb 2026 16:53:31 +0100 (CET) Content-Type: text/plain; charset=UTF-8 Date: Tue, 10 Feb 2026 16:52:51 +0100 Message-Id: From: "Lukas Wagner" To: "Arthur Bied-Charreton" , Subject: Re: [PATCH proxmox 4/5] notify: smtp: add OAuth2/XOAUTH2 authentication support Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260204161354.458814-1-a.bied-charreton@proxmox.com> <20260204161354.458814-5-a.bied-charreton@proxmox.com> In-Reply-To: <20260204161354.458814-5-a.bied-charreton@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1770738687014 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.038 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: UPNRLXQQWRAVJPIHM5EQBXHE7HLKDLKJ X-Message-ID-Hash: UPNRLXQQWRAVJPIHM5EQBXHE7HLKDLKJ X-MailFrom: l.wagner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Hey Arthur! Great work, looking really good so far. Left some comments inline. On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote: > Add Google & Microsoft OAuth2 support for SMTP notification targets. The > SmtpEndpoint implements refresh_state() to allow proactively refreshing > tokens through pveupdate. > > The SMTP API functions are updated to handle OAuth2 state. The refresh > token initially comes from the frontend, and it is more state than it is > configuration, therefore it is passed as a single parameter in > {add,update}_endpoint and persisted separately. > > Signed-off-by: Arthur Bied-Charreton > --- > proxmox-notify/src/api/smtp.rs | 144 ++++++++++++++---- > proxmox-notify/src/endpoints/smtp.rs | 210 +++++++++++++++++++++++++-- > 2 files changed, 315 insertions(+), 39 deletions(-) > > diff --git a/proxmox-notify/src/api/smtp.rs b/proxmox-notify/src/api/smtp= .rs > index 470701bf..9bd4826d 100644 > --- a/proxmox-notify/src/api/smtp.rs > +++ b/proxmox-notify/src/api/smtp.rs > @@ -1,11 +1,14 @@ > +use std::time::{SystemTime, UNIX_EPOCH}; > + > use proxmox_http_error::HttpError; > =20 > use crate::api::{http_bail, http_err}; > +use crate::context::context; > use crate::endpoints::smtp::{ > DeleteableSmtpProperty, SmtpConfig, SmtpConfigUpdater, SmtpPrivateCo= nfig, > - SmtpPrivateConfigUpdater, SMTP_TYPENAME, > + SmtpPrivateConfigUpdater, SmtpState, SMTP_TYPENAME, > }; > -use crate::Config; > +use crate::{Config, State}; > =20 > /// Get a list of all smtp endpoints. > /// > @@ -30,6 +33,30 @@ pub fn get_endpoint(config: &Config, name: &str) -> Re= sult .map_err(|_| http_err!(NOT_FOUND, "endpoint '{name}' not found")= ) > } > =20 > +/// Update the state for the endpoint `name`, and persist it at `context= ().state_file_path()`. > +fn update_state(name: &str, entry: Option) -> Result<(), Http= Error> { > + let mut state =3D State::from_path(context().state_file_path()).unwr= ap_or_default(); > + > + match entry { > + Some(entry) =3D> state.set(name, &entry).map_err(|e| { > + http_err!( > + INTERNAL_SERVER_ERROR, > + "could not update state for endpoint '{}': {e}", > + name > + ) > + })?, > + None =3D> state.remove(name), > + } > + > + state.persist(context().state_file_path()).map_err(|e| { > + http_err!( > + INTERNAL_SERVER_ERROR, > + "could not update state for endpoint '{}': {e}", > + name > + ) > + }) > +} > + > /// Add a new smtp endpoint. > /// > /// The caller is responsible for any needed permission checks. > @@ -38,10 +65,15 @@ pub fn get_endpoint(config: &Config, name: &str) -> R= esult /// - an entity with the same name already exists (`400 Bad request`) > /// - the configuration could not be saved (`500 Internal server error= `) > /// - mailto *and* mailto_user are both set to `None` > +/// > +/// `oauth2_refresh_token` is initially passed through the API when an O= Auth2 > +/// endpoint is created/updated, however its state is not managed throug= h a > +/// config, which is why it is passed separately. > pub fn add_endpoint( > config: &mut Config, > endpoint_config: SmtpConfig, > private_endpoint_config: SmtpPrivateConfig, > + oauth2_refresh_token: Option, > ) -> Result<(), HttpError> { > if endpoint_config.name !=3D private_endpoint_config.name { > // Programming error by the user of the crate, thus we panic > @@ -64,6 +96,17 @@ pub fn add_endpoint( > &endpoint_config.name, > )?; > =20 > + update_state( > + &endpoint_config.name, > + Some(SmtpState { > + oauth2_refresh_token, > + last_refreshed: SystemTime::now() > + .duration_since(UNIX_EPOCH) We already have a nice helper for this - proxmox_time::epoch_i64 > + .unwrap() > + .as_secs(), > + }), > + )?; > + > config > .config > .set_data(&endpoint_config.name, SMTP_TYPENAME, &endpoint_config= ) > @@ -76,6 +119,28 @@ pub fn add_endpoint( > }) > } > =20 > +/// Apply `updater` to the private config identified by `name`, and set > +/// the private config entry afterwards. > +pub fn update_private_config( This function does not need to be `pub`, I think? > + config: &mut Config, > + name: &str, > + updater: impl FnOnce(&mut SmtpPrivateConfig), nice idea btw, using a closure like this > +) -> Result<(), HttpError> { > + let mut private_config: SmtpPrivateConfig =3D config > + .private_config > + .lookup(SMTP_TYPENAME, name) > + .map_err(|e| { > + http_err!( > + INTERNAL_SERVER_ERROR, > + "no private config found for SMTP endpoint: {e}" > + ) > + })?; > + > + updater(&mut private_config); > + > + super::set_private_config_entry(config, private_config, SMTP_TYPENAM= E, name) > +} > + > /// Update existing smtp endpoint > /// > /// The caller is responsible for any needed permission checks. > @@ -83,11 +148,16 @@ pub fn add_endpoint( > /// Returns a `HttpError` if: > /// - the configuration could not be saved (`500 Internal server error= `) > /// - mailto *and* mailto_user are both set to `None` > +/// > +/// `oauth2_refresh_token` is initially passed through the API when an O= Auth2 > +/// endpoint is created/updated, however its state is not managed throug= h a > +/// config, which is why it is passed separately. > pub fn update_endpoint( > config: &mut Config, > name: &str, > updater: SmtpConfigUpdater, > private_endpoint_config_updater: SmtpPrivateConfigUpdater, > + oauth2_refresh_token: Option, > delete: Option<&[DeleteableSmtpProperty]>, > digest: Option<&[u8]>, > ) -> Result<(), HttpError> { > @@ -103,20 +173,20 @@ pub fn update_endpoint( > DeleteableSmtpProperty::Disable =3D> endpoint.disable = =3D None, > DeleteableSmtpProperty::Mailto =3D> endpoint.mailto.clea= r(), > DeleteableSmtpProperty::MailtoUser =3D> endpoint.mailto_= user.clear(), > - DeleteableSmtpProperty::Password =3D> super::set_private= _config_entry( > - config, > - SmtpPrivateConfig { > - name: name.to_string(), > - password: None, > - }, > - SMTP_TYPENAME, > - name, > - )?, > + DeleteableSmtpProperty::Password =3D> { > + update_private_config(config, name, |c| c.password = =3D None)? > + } > + DeleteableSmtpProperty::AuthMethod =3D> endpoint.auth_me= thod =3D None, > + DeleteableSmtpProperty::OAuth2ClientId =3D> endpoint.oau= th2_client_id =3D None, > + DeleteableSmtpProperty::OAuth2ClientSecret =3D> { > + update_private_config(config, name, |c| c.oauth2_cli= ent_secret =3D None)? > + } > + DeleteableSmtpProperty::OAuth2TenantId =3D> endpoint.oau= th2_tenant_id =3D None, > DeleteableSmtpProperty::Port =3D> endpoint.port =3D None= , > DeleteableSmtpProperty::Username =3D> endpoint.username = =3D None, > } > } > - } > + }; > =20 > if let Some(mailto) =3D updater.mailto { > endpoint.mailto =3D mailto; > @@ -139,29 +209,24 @@ pub fn update_endpoint( > if let Some(mode) =3D updater.mode { > endpoint.mode =3D Some(mode); > } > - if let Some(password) =3D private_endpoint_config_updater.password { > - super::set_private_config_entry( > - config, > - SmtpPrivateConfig { > - name: name.into(), > - password: Some(password), > - }, > - SMTP_TYPENAME, > - name, > - )?; > + if let Some(auth_method) =3D updater.auth_method { > + endpoint.auth_method =3D Some(auth_method); > } > - > if let Some(author) =3D updater.author { > endpoint.author =3D Some(author); > } > - > if let Some(comment) =3D updater.comment { > endpoint.comment =3D Some(comment); > } > - > if let Some(disable) =3D updater.disable { > endpoint.disable =3D Some(disable); > } > + if let Some(oauth2_client_id) =3D updater.oauth2_client_id { > + endpoint.oauth2_client_id =3D Some(oauth2_client_id); > + } > + if let Some(oauth2_tenant_id) =3D updater.oauth2_tenant_id { > + endpoint.oauth2_tenant_id =3D Some(oauth2_tenant_id); > + } > =20 > if endpoint.mailto.is_empty() && endpoint.mailto_user.is_empty() { > http_bail!( > @@ -170,6 +235,25 @@ pub fn update_endpoint( > ); > } > =20 > + let private_config =3D SmtpPrivateConfig { > + name: name.into(), > + password: private_endpoint_config_updater.password, > + oauth2_client_secret: private_endpoint_config_updater.oauth2_cli= ent_secret, > + }; > + > + super::set_private_config_entry(config, private_config, SMTP_TYPENAM= E, name)?; > + > + update_state( > + name, > + Some(SmtpState { > + oauth2_refresh_token, > + last_refreshed: SystemTime::now() > + .duration_since(UNIX_EPOCH) > + .unwrap() > + .as_secs(), same here regarding proxmox_time::epoch_i64 > + }), > + )?; > + > config > .config > .set_data(name, SMTP_TYPENAME, &endpoint) > @@ -196,6 +280,7 @@ pub fn delete_endpoint(config: &mut Config, name: &st= r) -> Result<(), HttpError> > =20 > super::remove_private_config_entry(config, name)?; > config.config.sections.remove(name); > + update_state(name, None)?; > =20 > Ok(()) > } > @@ -204,7 +289,7 @@ pub fn delete_endpoint(config: &mut Config, name: &st= r) -> Result<(), HttpError> > pub mod tests { > use super::*; > use crate::api::test_helpers::*; > - use crate::endpoints::smtp::SmtpMode; > + use crate::endpoints::smtp::{SmtpAuthMethod, SmtpMode}; > =20 > pub fn add_smtp_endpoint_for_test(config: &mut Config, name: &str) -= > Result<(), HttpError> { > add_endpoint( > @@ -217,6 +302,7 @@ pub mod tests { > author: Some("root".into()), > comment: Some("Comment".into()), > mode: Some(SmtpMode::StartTls), > + auth_method: Some(SmtpAuthMethod::Plain), > server: "localhost".into(), > port: Some(555), > username: Some("username".into()), > @@ -225,7 +311,9 @@ pub mod tests { > SmtpPrivateConfig { > name: name.into(), > password: Some("password".into()), > + oauth2_client_secret: None, > }, > + None, > )?; > =20 > assert!(get_endpoint(config, name).is_ok()); > @@ -256,6 +344,7 @@ pub mod tests { > Default::default(), > None, > None, > + None, > ) > .is_err()); > =20 > @@ -273,6 +362,7 @@ pub mod tests { > Default::default(), > Default::default(), > None, > + None, > Some(&[0; 32]), > ) > .is_err()); > @@ -304,6 +394,7 @@ pub mod tests { > }, > Default::default(), > None, > + None, > Some(&digest), > )?; > =20 > @@ -327,6 +418,7 @@ pub mod tests { > "smtp-endpoint", > Default::default(), > Default::default(), > + None, > Some(&[ > DeleteableSmtpProperty::Author, > DeleteableSmtpProperty::MailtoUser, > diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/en= dpoints/smtp.rs > index 69c4048c..b7194fff 100644 > --- a/proxmox-notify/src/endpoints/smtp.rs > +++ b/proxmox-notify/src/endpoints/smtp.rs > @@ -1,12 +1,15 @@ > use std::borrow::Cow; > -use std::time::Duration; > +use std::time::{Duration, SystemTime, UNIX_EPOCH}; > =20 > use lettre::message::header::{HeaderName, HeaderValue}; > use lettre::message::{Mailbox, MultiPart, SinglePart}; > +use lettre::transport::smtp::authentication::{Credentials, Mechanism}; > use lettre::transport::smtp::client::{Tls, TlsParameters}; > use lettre::{message::header::ContentType, Message, SmtpTransport, Trans= port}; > use serde::{Deserialize, Serialize}; > =20 > +use oauth2::{ClientId, ClientSecret, RefreshToken}; > + > use proxmox_schema::api_types::COMMENT_SCHEMA; > use proxmox_schema::{api, Updater}; > =20 > @@ -80,11 +83,21 @@ pub struct SmtpConfig { > pub port: Option, > #[serde(skip_serializing_if =3D "Option::is_none")] > pub mode: Option, > + /// Method to be used for authentication. > + #[serde(skip_serializing_if =3D "Option::is_none")] > + pub auth_method: Option, > /// Username to use during authentication. > /// If no username is set, no authentication will be performed. > /// The PLAIN and LOGIN authentication methods are supported > #[serde(skip_serializing_if =3D "Option::is_none")] > pub username: Option, > + /// Client ID for XOAUTH2 authentication method. > + #[serde(skip_serializing_if =3D "Option::is_none")] > + pub oauth2_client_id: Option, > + /// Tenant ID for XOAUTH2 authentication method. Only required for > + /// Microsoft Exchange Online OAuth2. > + #[serde(skip_serializing_if =3D "Option::is_none")] > + pub oauth2_tenant_id: Option, > /// Mail address to send a mail to. > #[serde(default, skip_serializing_if =3D "Vec::is_empty")] > #[updater(serde(skip_serializing_if =3D "Option::is_none"))] > @@ -131,12 +144,39 @@ pub enum DeleteableSmtpProperty { > MailtoUser, > /// Delete `password` > Password, > + /// Delete `auth_method` > + AuthMethod, > + /// Delete `oauth2_client_id` > + #[serde(rename =3D "oauth2-client-id")] > + OAuth2ClientId, > + /// Delete `oauth2_client_secret` > + #[serde(rename =3D "oauth2-client-secret")] > + OAuth2ClientSecret, > + /// Delete `oauth2_tenant_id` > + #[serde(rename =3D "oauth2-tenant-id")] > + OAuth2TenantId, > /// Delete `port` > Port, > /// Delete `username` > Username, > } > =20 > +/// Authentication mode to use for SMTP. > +#[api] > +#[derive(Serialize, Deserialize, Clone, Debug, Default)] You could also derive `Copy` here, then you don't need to clone it in some places in the code. > +#[serde(rename_all =3D "kebab-case")] > +pub enum SmtpAuthMethod { > + /// Username + password > + #[default] > + Plain, > + /// Google OAuth2 > + #[serde(rename =3D "google-oauth2")] > + GoogleOAuth2, > + /// Microsoft OAuth2 > + #[serde(rename =3D "microsoft-oauth2")] > + MicrosoftOAuth2, > +} > + > #[derive(Serialize, Deserialize, Clone, Debug, Default)] > #[serde(rename_all =3D "kebab-case")] > pub struct SmtpState { > @@ -157,9 +197,14 @@ pub struct SmtpPrivateConfig { > /// Name of the endpoint > #[updater(skip)] > pub name: String, > + > /// The password to use during authentication. > #[serde(skip_serializing_if =3D "Option::is_none")] > pub password: Option, > + > + /// OAuth2 client secret > + #[serde(skip_serializing_if =3D "Option::is_none")] > + pub oauth2_client_secret: Option, > } > =20 > /// A sendmail notification endpoint. > @@ -168,6 +213,60 @@ pub struct SmtpEndpoint { > pub private_config: SmtpPrivateConfig, > } > =20 > +impl SmtpEndpoint { > + fn get_access_token( > + &self, > + refresh_token: &str, > + auth_method: &SmtpAuthMethod, > + ) -> Result { > + let client_id =3D ClientId::new( > + self.config > + .oauth2_client_id > + .as_ref() > + .ok_or_else(|| Error::Generic("oauth2-client-id not set"= .into()))? > + .to_string(), > + ); > + let client_secret =3D ClientSecret::new( > + self.private_config > + .oauth2_client_secret > + .as_ref() > + .ok_or_else(|| Error::Generic("oauth2-client-secret not = set".into()))? > + .to_string(), > + ); > + let refresh_token =3D RefreshToken::new(refresh_token.into()); > + > + match auth_method { > + SmtpAuthMethod::GoogleOAuth2 =3D> { > + xoauth2::get_google_token(client_id, client_secret, refr= esh_token) > + } > + SmtpAuthMethod::MicrosoftOAuth2 =3D> xoauth2::get_microsoft_= token( > + client_id, > + client_secret, > + &self.config.oauth2_tenant_id.as_ref().ok_or(Error::Gene= ric( > + "tenant ID not set, required for Microsoft OAuth2".i= nto(), > + ))?, > + refresh_token, > + ), > + _ =3D> Err(Error::Generic("OAuth2 not configured".into())), > + } > + } > + > + /// Infer the auth method based on the presence of a password field = in the private config. > + /// > + /// This is required for backwards compatibility for configs created= before the `auth_method` > + /// field was added, i.e., the presence of a password implicitly mea= nt plain authentication > + /// was to be used. > + fn auth_method(&self) -> Option { > + self.config.auth_method.clone().or_else(|| { > + if self.private_config.password.is_some() { > + Some(SmtpAuthMethod::Plain) > + } else { > + None > + } > + }) > + } > +} > + > impl Endpoint for SmtpEndpoint { > fn send(&self, notification: &Notification, state: &mut State) -> Re= sult<(), Error> { > let mut endpoint_state =3D state.get_or_default::(sel= f.name())?; > @@ -190,23 +289,58 @@ impl Endpoint for SmtpEndpoint { > } > }; > =20 > - let mut transport_builder =3D SmtpTransport::builder_dangerous(&= self.config.server) > + let transport_builder =3D SmtpTransport::builder_dangerous(&self= .config.server) > .tls(tls) > .port(port) > .timeout(Some(Duration::from_secs(SMTP_TIMEOUT.into()))); > =20 > - if let Some(username) =3D self.config.username.as_deref() { > - if let Some(password) =3D self.private_config.password.as_de= ref() { > - transport_builder =3D transport_builder.credentials((use= rname, password).into()); > - } else { > - return Err(Error::NotifyFailed( > - self.name().into(), > - Box::new(Error::Generic( > - "username is set but no password was provided".t= o_owned(), > - )), > - )); > + let transport_builder =3D match &self.auth_method() { > + None =3D> transport_builder, > + Some(SmtpAuthMethod::Plain) =3D> match ( > + self.config.username.as_deref(), > + self.private_config.password.as_deref(), > + ) { > + (Some(username), Some(password)) =3D> { > + transport_builder.credentials((username, password).i= nto()) > + } > + (Some(_), None) =3D> { > + return Err(Error::NotifyFailed( > + self.name().into(), > + Box::new(Error::Generic( > + "username is set but no password was provide= d".to_owned(), > + )), > + )) > + } > + _ =3D> transport_builder, > + }, > + Some(method) =3D> { > + let token_exchange_result =3D self.get_access_token( > + endpoint_state > + .oauth2_refresh_token > + .as_ref() > + .ok_or(Error::NotifyFailed( > + self.name().into(), > + Box::new(Error::Generic("no refresh token fo= und for endpoint".into())), > + ))?, > + method, > + )?; > + > + if let Some(new_refresh_token) =3D token_exchange_result= .refresh_token { > + endpoint_state.oauth2_refresh_token =3D Some(new_ref= resh_token.into_secret()); > + } > + endpoint_state.last_refreshed =3D SystemTime::now() > + .duration_since(UNIX_EPOCH) > + .unwrap() > + .as_secs(); Same here regarding proxmox_time::epoch_i64 > + > + transport_builder > + .credentials(Credentials::new( > + self.config.from_address.clone(), > + token_exchange_result.access_token.into_secret()= , > + )) > + .authentication(vec![Mechanism::Xoauth2]) > } > - } > + }; > =20 > let transport =3D transport_builder.build(); I think it could be nice to move everything above this line to a separate method `build_smtp_tansport` - this method is getting rather long and this part is a very distinct step of the things performed here. > =20 > @@ -298,6 +432,56 @@ impl Endpoint for SmtpEndpoint { > fn disabled(&self) -> bool { > self.config.disable.unwrap_or_default() > } > + > + fn refresh_state(&self, state: &mut State) -> Result { > + let endpoint_state =3D match state.get::(self.name())= ? { > + None =3D> return Ok(false), > + Some(s) =3D> s, > + }; > + > + let Some(refresh_token) =3D endpoint_state.oauth2_refresh_token = else { > + return Ok(false); > + }; > + > + // The refresh job is configured in pveupdate, which runs once f= or each node. > + // Don't refresh if we already did it recently. > + if SystemTime::now() > + .duration_since(UNIX_EPOCH + Duration::from_secs(endpoint_st= ate.last_refreshed)) > + .map_err(|e| Error::Generic(e.to_string()))? > + < Duration::from_secs(60 * 60 * 12) same here regarding proxmox_time::epoch_i64 Also the cut-off duration should be const, not a magic number > + { > + return Ok(false); > + } > + > + let Some(auth_method) =3D self.config.auth_method.as_ref() else = { > + return Ok(false); > + }; > + > + let new_state =3D match self > + .get_access_token(&refresh_token, auth_method)? > + .refresh_token > + { > + // Microsoft OAuth2, new token was returned > + Some(new_refresh_token) =3D> SmtpState { > + oauth2_refresh_token: Some(new_refresh_token.into_secret= ()), > + last_refreshed: SystemTime::now() > + .duration_since(UNIX_EPOCH) > + .unwrap() > + .as_secs(), > + }, > + // Google OAuth2, refresh token's lifetime was extended > + None =3D> SmtpState { > + oauth2_refresh_token: Some(refresh_token), > + last_refreshed: SystemTime::now() > + .duration_since(UNIX_EPOCH) > + .unwrap() > + .as_secs(), > + }, > + }; > + > + state.set(self.name(), &new_state)?; > + Ok(true) It seems like you return Ok(true) in case that the state changed (token was refreshed) and Ok(false) if nothing changed, is this correct? The boolean parameter should be documented in the trait doc-comments. Also at the moment you don't seem to use the boolean part anywhere? I guess we could use it to determine whether we need to replace the existing state file at all (we don't if all endpoints returned `false`). > + } > } > =20 > /// Construct a lettre `Message` from a raw email message.