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 E8C671FF13F for ; Thu, 23 Apr 2026 14:24:58 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 23646156AC; Thu, 23 Apr 2026 14:24:45 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 23 Apr 2026 14:24:35 +0200 Message-Id: To: "Arthur Bied-Charreton" , , Subject: Re: [PATCH proxmox v4 07/24] notify: smtp: Add state handling logic X-Mailer: aerc 0.20.0 References: <20260421115957.402589-1-a.bied-charreton@proxmox.com> <20260421115957.402589-8-a.bied-charreton@proxmox.com> In-Reply-To: <20260421115957.402589-8-a.bied-charreton@proxmox.com> From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1776946986619 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.121 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: DZ66EDRYUOAORM3JPJQLW3KAJVN4Y6CQ X-Message-ID-Hash: DZ66EDRYUOAORM3JPJQLW3KAJVN4Y6CQ X-MailFrom: s.sterz@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: On Tue Apr 21, 2026 at 1:59 PM CEST, Arthur Bied-Charreton wrote: > Create new state file in add_endpoint, create/update existing one in > update_endpoint and delete it in delete_endpoint. > > Add trigger_state_refresh to the Endpoint trait, with no-op default > implementation. Implement it in SmtpEndpoint's Endpoint impl to trigger > an OAuth2 token exchange, in order to rotate an existing token, or > extend its lifetime. > > Since trigger_state_refresh is called in pveupdate, it may be called > multiple times in quick succession by the different nodes in a > cluster. In order to avoid unnecessary churn on the state files, the > last_refreshed field is used to check if the state has been refreshed > shortly before, and skip the update if that is the case. > > Signed-off-by: Arthur Bied-Charreton > Reviewed-by: Lukas Wagner > --- > proxmox-notify/src/api/common.rs | 16 ++++++ > proxmox-notify/src/api/smtp.rs | 32 +++++++++++ > proxmox-notify/src/endpoints/smtp.rs | 79 +++++++++++++++++++++++++++- > proxmox-notify/src/lib.rs | 19 +++++++ > 4 files changed, 145 insertions(+), 1 deletion(-) > > diff --git a/proxmox-notify/src/api/common.rs b/proxmox-notify/src/api/co= mmon.rs > index fa2356e2..1e6b7d46 100644 > --- a/proxmox-notify/src/api/common.rs > +++ b/proxmox-notify/src/api/common.rs > @@ -3,6 +3,22 @@ use proxmox_http_error::HttpError; > use super::http_err; > use crate::{Bus, Config, Notification}; > > +/// Refresh all notification targets' internal state. > +/// > +/// The caller is responsible for any needed permission checks. > +pub fn trigger_state_refresh(config: &Config) -> Result<(), HttpError> { > + let bus =3D Bus::from_config(config).map_err(|err| { > + http_err!( > + INTERNAL_SERVER_ERROR, > + "Could not instantiate notification bus: {err}" > + ) > + })?; > + > + bus.trigger_state_refresh(); > + > + Ok(()) > +} > + > /// Send a notification to a given target. > /// > /// The caller is responsible for any needed permission checks. > diff --git a/proxmox-notify/src/api/smtp.rs b/proxmox-notify/src/api/smtp= .rs > index 71284f63..b0df47a8 100644 > --- a/proxmox-notify/src/api/smtp.rs > +++ b/proxmox-notify/src/api/smtp.rs > @@ -111,6 +111,19 @@ pub fn add_endpoint( > > let endpoint_config =3D infer_auth_method(endpoint_config, private_e= ndpoint_config); > > + if let Some(token) =3D oauth2_refresh_token { > + let oauth_state =3D State::new(token, proxmox_time::epoch_i64())= ; > + context() > + .save_oauth_state(&endpoint_config.name, Some(oauth_state)) > + .map_err(|e| { > + http_err!( > + INTERNAL_SERVER_ERROR, > + "could not create state file for '{}': {e}", > + &endpoint_config.name > + ) > + })?; > + } > + > config > .config > .set_data(&endpoint_config.name, SMTP_TYPENAME, &endpoint_config= ) > @@ -227,6 +240,18 @@ pub fn update_endpoint( > > let endpoint =3D infer_auth_method(endpoint, get_private_config(conf= ig, name)?); > > + if let Some(token) =3D oauth2_refresh_token { > + let oauth_state =3D context() > + .load_oauth_state(name) > + .map_err(|e| http_err!(INTERNAL_SERVER_ERROR, "{e}"))? > + .set_oauth2_refresh_token(Some(token)) > + .set_last_refreshed(proxmox_time::epoch_i64()); > + > + context() > + .save_oauth_state(name, Some(oauth_state)) > + .map_err(|e| http_err!(INTERNAL_SERVER_ERROR, "{e}"))?; > + } > + > config > .config > .set_data(name, SMTP_TYPENAME, &endpoint) > @@ -253,6 +278,13 @@ pub fn delete_endpoint(config: &mut Config, name: &s= tr) -> Result<(), HttpError> > > super::remove_private_config_entry(config, name)?; > > + context().save_oauth_state(name, None).map_err(|e| { > + http_err!( > + INTERNAL_SERVER_ERROR, > + "could not delete state for '{name}': {e}" > + ) > + })?; > + > config.config.sections.remove(name); > > Ok(()) > diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/en= dpoints/smtp.rs > index b92f96f0..3e7175d2 100644 > --- a/proxmox-notify/src/endpoints/smtp.rs > +++ b/proxmox-notify/src/endpoints/smtp.rs > @@ -1,11 +1,13 @@ > use std::borrow::Cow; > -use std::time::Duration; > +use std::time::{Duration, SystemTime, UNIX_EPOCH}; > > use lettre::message::header::{HeaderName, HeaderValue}; > use lettre::message::{Mailbox, MultiPart, SinglePart}; > use lettre::transport::smtp::client::{Tls, TlsParameters}; > use lettre::{message::header::ContentType, Message, SmtpTransport, Trans= port}; > +use oauth2::{ClientId, ClientSecret, RefreshToken}; > use serde::{Deserialize, Serialize}; > +use tracing::info; > > use proxmox_schema::api_types::COMMENT_SCHEMA; > use proxmox_schema::{api, Updater}; > @@ -22,6 +24,7 @@ const SMTP_PORT: u16 =3D 25; > const SMTP_SUBMISSION_STARTTLS_PORT: u16 =3D 587; > const SMTP_SUBMISSION_TLS_PORT: u16 =3D 465; > const SMTP_TIMEOUT: u16 =3D 5; > +const SMTP_STATE_REFRESH_CUTOFF_SECONDS: Duration =3D Duration::from_sec= s(60 * 60 * 12); > > mod xoauth2; > > @@ -204,6 +207,43 @@ pub struct SmtpEndpoint { > } > > 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::Gener= ic( > + "tenant ID not set, required for Microsoft OAuth2".i= nto(), > + ))?, > + refresh_token, > + ), > + _ =3D> Err(Error::Generic("OAuth2 not configured".into())), > + } > + } > + > fn build_transport(&self, tls: Tls, port: u16) -> Result { > let mut transport_builder =3D SmtpTransport::builder_dangerous(&= self.config.server) > .tls(tls) > @@ -335,6 +375,43 @@ impl Endpoint for SmtpEndpoint { > fn disabled(&self) -> bool { > self.config.disable.unwrap_or_default() > } > + > + fn trigger_state_refresh(&self) -> Result<(), Error> { > + let state =3D context().load_oauth_state(self.name())?; > + > + let Some(refresh_token) =3D &state.oauth2_refresh_token else { > + return Ok(()); > + }; > + > + // 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(state.last_= refreshed as u64)) > + .map_err(|e| Error::Generic(e.to_string()))? > + < SMTP_STATE_REFRESH_CUTOFF_SECONDS if im not mistaken, this may not work as inteded. since no lock is held between the `load` and `save` calls, this scenario could happen: A: triggers a refresh, reads the old timestamp, triggers a proper refresh B: triggers a refresh, reads the old timestamp, triggers a proper refresh A: writes the refreshed token and timestamp B: writes the refreshed token and timestamp since multiple tokens can be valid at the same time (from what i can tell), it doesn't really matter which thread wins the race here in terms of the token being valid after the refresh. however, this mechanism may be less effective than intended when it comes to stopping refreshes in quick succession. this would also be fixed by locking i suggested in a previous message. > + { > + return Ok(()); > + } > + > + let Some(auth_method) =3D self.config.auth_method.as_ref() else = { > + return Ok(()); > + }; > + > + let state =3D match self > + .get_access_token(refresh_token, auth_method)? > + .refresh_token > + { > + Some(tok) =3D> state.set_oauth2_refresh_token(Some(tok.into_= secret())), // New token was returned, rotate > + None =3D> state, > + } > + .set_last_refreshed(proxmox_time::epoch_i64()); > + > + context().save_oauth_state(self.name(), Some(state))?; > + > + info!("OAuth2 state refreshed for endpoint `{}`", self.name()); > + > + Ok(()) > + } > } > > /// Construct a lettre `Message` from a raw email message. > diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs > index d443b738..2025bd64 100644 > --- a/proxmox-notify/src/lib.rs > +++ b/proxmox-notify/src/lib.rs > @@ -169,6 +169,11 @@ pub trait Endpoint { > > /// Check if the endpoint is disabled > fn disabled(&self) -> bool; > + > + /// Refresh endpoint's state > + fn trigger_state_refresh(&self) -> Result<(), Error> { > + Ok(()) > + } > } > > #[derive(Debug, Clone, Serialize, Deserialize)] > @@ -605,6 +610,20 @@ impl Bus { > > Ok(()) > } > + > + /// Refresh all endpoints' internal state. > + /// > + /// This function works on a best effort basis, if an endpoint's sta= te cannot > + /// be updated for whatever reason, the error is logged and the next= one(s) > + /// are attempted. > + pub fn trigger_state_refresh(&self) { > + for (name, endpoint) in &self.endpoints { > + match endpoint.trigger_state_refresh() { > + Ok(()) =3D> debug!("triggered state refresh for endpoint= '{name}'"), > + Err(e) =3D> error!("could not trigger state refresh for = endpoint '{name}': {e}"), > + }; > + } > + } > } > > #[cfg(test)]