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 6C61C1FF136 for ; Mon, 23 Mar 2026 13:25:57 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 45FE015726; Mon, 23 Mar 2026 13:26:16 +0100 (CET) Content-Type: text/plain; charset=UTF-8 Date: Mon, 23 Mar 2026 13:26:08 +0100 Message-Id: Subject: Re: [PATCH proxmox 5/7] notify (smtp): Add state handling logic From: "Lukas Wagner" To: "Arthur Bied-Charreton" , Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260213160415.609868-1-a.bied-charreton@proxmox.com> <20260213160415.609868-6-a.bied-charreton@proxmox.com> In-Reply-To: <20260213160415.609868-6-a.bied-charreton@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774268723057 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.049 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: QMZYEXHHDU3GUYHYYWNFONE6KMKAC26Z X-Message-ID-Hash: QMZYEXHHDU3GUYHYYWNFONE6KMKAC26Z 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: I think this misses removing the endpoint state file in `delete_endpoint`. Some more notes inline. On Fri Feb 13, 2026 at 5:04 PM CET, Arthur Bied-Charreton wrote: > Create new state file in add_endpoint, and create/update existing one in > update_endpoint. > > Add trigger_state_refresh to the Endpoint trait, with no-op default > implementation. Override trigger_state_refresh in SmtpEndpoint's > Endpoint impl to trigger an OAuth2 token exchange, in order to rotate > an existing token, or extend its lifetime. > > Signed-off-by: Arthur Bied-Charreton > --- > proxmox-notify/src/api/common.rs | 16 ++++++ > proxmox-notify/src/api/smtp.rs | 29 ++++++++++- > proxmox-notify/src/endpoints/smtp.rs | 75 +++++++++++++++++++++++++++- > proxmox-notify/src/lib.rs | 19 +++++++ > 4 files changed, 137 insertions(+), 2 deletions(-) > > 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}; > =20 > +/// 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 4231cdae..8992e789 100644 > --- a/proxmox-notify/src/api/smtp.rs > +++ b/proxmox-notify/src/api/smtp.rs > @@ -2,7 +2,7 @@ use proxmox_http_error::HttpError; > =20 > use crate::api::{http_bail, http_err}; > use crate::endpoints::smtp::{ > - DeleteableSmtpProperty, SmtpConfig, SmtpConfigUpdater, SmtpPrivateCo= nfig, > + self, DeleteableSmtpProperty, SmtpConfig, SmtpConfigUpdater, SmtpPri= vateConfig, > SmtpPrivateConfigUpdater, SMTP_TYPENAME, > }; > use crate::Config; > @@ -69,6 +69,16 @@ pub fn add_endpoint( > &endpoint_config.name, > )?; > =20 > + smtp::State::new(oauth2_refresh_token) > + .store(&endpoint_config.name) > + .map_err(|e| { > + http_err!( > + INTERNAL_SERVER_ERROR, > + "could not create state file for endpoint '{}': {e}", > + endpoint_config.name > + ) > + })?; > + > config > .config > .set_data(&endpoint_config.name, SMTP_TYPENAME, &endpoint_config= ) > @@ -206,6 +216,23 @@ pub fn update_endpoint( > } > })?; > =20 > + smtp::State::load(name) > + .map_err(|e| { > + http_err!( > + INTERNAL_SERVER_ERROR, > + "could not load state for endpoint '{name}': {e}" > + ) > + })? > + .set_oauth2_refresh_token(oauth2_refresh_token) > + .set_last_refreshed(proxmox_time::epoch_i64()) > + .store(name) > + .map_err(|e| { > + http_err!( > + INTERNAL_SERVER_ERROR, > + "could not persist state for endpoint '{name}': {e}" > + ) > + })?; > + > config > .config > .set_data(name, SMTP_TYPENAME, &endpoint) > diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/en= dpoints/smtp.rs > index 361c4da9..244799fd 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 > @@ -22,6 +25,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: u64 =3D 60 * 60 * 12; Duration::from_secs is actually a `const fn`, so you could const SMTP_STATE_REFRESH_CUTOFF: Duration =3D Duration::from_secs(12 * 60 *= 60); > =20 > mod state; > mod xoauth2; > @@ -205,6 +209,43 @@ pub struct SmtpEndpoint { > } > =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::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) > @@ -336,6 +377,38 @@ impl Endpoint for SmtpEndpoint { > fn disabled(&self) -> bool { > self.config.disable.unwrap_or_default() > } > + > + fn trigger_state_refresh(&self) -> Result<(), Error> { > + let state =3D State::load(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()))? > + < Duration::from_secs(SMTP_STATE_REFRESH_CUTOFF_SECONDS) This reminds me, we deal with "raw" timestamps so often (especially in PDM), we really need to have some better types/helpers for things like these... ofc out of scope for this series... > + { > + return Ok(()); > + } > + > + let Some(auth_method) =3D self.config.auth_method.as_ref() else = { > + return Ok(()); > + }; > + > + 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()) > + .store(self.name()) > + } > } > =20 > /// Construct a lettre `Message` from a raw email message. > diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs > index 879f8326..c1a5e535 100644 > --- a/proxmox-notify/src/lib.rs > +++ b/proxmox-notify/src/lib.rs > @@ -157,6 +157,11 @@ pub trait Endpoint { > =20 > /// Check if the endpoint is disabled > fn disabled(&self) -> bool; > + > + /// Refresh endpoint's state > + fn trigger_state_refresh(&self) -> Result<(), Error> { > + Ok(()) > + } > } > =20 > #[derive(Debug, Clone, Serialize, Deserialize)] > @@ -593,6 +598,20 @@ impl Bus { > =20 > 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> info!("triggered state refresh for endpoint = '{name}'"), I'd probably rather use `debug!` here and then `info!` only if one endpoint actually refreshed its token > + Err(e) =3D> error!("could not trigger state refresh for = endpoint '{name}': {e}"), > + }; > + } > + } > } > =20 > #[cfg(test)]