Re: [PATCH proxmox v4 03/24] notify: smtp: Introduce state management

Shannon Sterz
2026-04-23T12:25:45Z

On Tue Apr 21, 2026 at 1:59 PM CEST, Arthur Bied-Charreton wrote:
> Export a new State struct in the xoauth2 module with associated
> functionality for loading, updating, and persisting the OAuth2 state
> for SMTP endpoints.
>
> The API for loading and saving the state is exposed through the
> Context trait, in order to make migration as easy as possible in
> a future where we might want to move towards KV storage instead
> of files for secret management. It is made specific to oauth state,
> because this implementation assumes invariants that hold for oauth2
> refresh tokens (documented in the smtp::xoauth2 module's doc comments),
> but are likely to be incorrect for other kinds of state that may be added
> in the future.
>
> The State struct is public in order to support the long-term goal for
> the Context trait to be implemented by the products themselves.
>
> The nix crate is added for the sys::stat::Mode struct, and
> proxmox-sys is now pulled in unconditionally since it is used in the
> Context implementations.
>
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> proxmox-notify/Cargo.toml | 12 ++-
> proxmox-notify/debian/control | 41 +++------
> proxmox-notify/src/context/mod.rs | 17 ++++
> proxmox-notify/src/context/pbs.rs | 23 +++++
> proxmox-notify/src/context/pve.rs | 25 ++++-
> proxmox-notify/src/context/test.rs | 22 +++++
> proxmox-notify/src/endpoints/smtp.rs | 2 +
> proxmox-notify/src/endpoints/smtp/xoauth2.rs | 97 ++++++++++++++++++++
> proxmox-notify/src/lib.rs | 12 +++
> 9 files changed, 218 insertions(+), 33 deletions(-)
>
> diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
> index c0d9921b..873a7d6f 100644
> --- a/proxmox-notify/Cargo.toml
> +++ b/proxmox-notify/Cargo.toml
> @@ -36,16 +36,18 @@ proxmox-schema = { workspace = true, features = ["api-macro", "api-types"] }
> proxmox-section-config = { workspace = true }
> proxmox-serde.workspace = true
> proxmox-sendmail = { workspace = true, optional = true }
> -proxmox-sys = { workspace = true, optional = true }
> +proxmox-sys = { workspace = true }
> proxmox-time.workspace = true
> proxmox-uuid = { workspace = true, features = ["serde"] }
> +nix = { workspace = true }
> +
>
> [features]
> default = ["sendmail", "gotify", "smtp", "webhook"]
> -mail-forwarder = ["dep:mail-parser", "dep:proxmox-sys", "proxmox-sendmail/mail-forwarder"]
> -sendmail = ["dep:proxmox-sys", "dep:proxmox-sendmail"]
> +mail-forwarder = ["dep:mail-parser", "proxmox-sendmail/mail-forwarder"]
> +sendmail = ["dep:proxmox-sendmail"]
> gotify = ["dep:proxmox-http", "dep:http"]
> -pve-context = ["dep:proxmox-sys"]
> -pbs-context = ["dep:proxmox-sys"]
> +pve-context = []
> +pbs-context = []
> smtp = ["dep:lettre", "dep:oauth2", "dep:ureq", "dep:http"]
> webhook = ["dep:http", "dep:percent-encoding", "dep:proxmox-base64", "dep:proxmox-http"]
> diff --git a/proxmox-notify/debian/control b/proxmox-notify/debian/control
> index 1b5c4068..b8db398c 100644
> --- a/proxmox-notify/debian/control
> +++ b/proxmox-notify/debian/control
> @@ -11,6 +11,7 @@ Build-Depends-Arch: cargo:native <!nocheck>,
> librust-handlebars-5+default-dev <!nocheck>,
> librust-http-1+default-dev <!nocheck>,
> librust-lettre-0.11+default-dev (>= 0.11.1-~~) <!nocheck>,
> + librust-nix-0.29+default-dev <!nocheck>,
> librust-oauth2-5-dev <!nocheck>,
> librust-openssl-0.10+default-dev <!nocheck>,
> librust-percent-encoding-2+default-dev (>= 2.1-~~) <!nocheck>,
> @@ -51,6 +52,7 @@ Depends:
> librust-anyhow-1+default-dev,
> librust-const-format-0.2+default-dev,
> librust-handlebars-5+default-dev,
> + librust-nix-0.29+default-dev,
> librust-openssl-0.10+default-dev,
> librust-proxmox-http-error-1+default-dev,
> librust-proxmox-human-byte-1+default-dev,
> @@ -60,6 +62,7 @@ Depends:
> librust-proxmox-section-config-3+default-dev (>= 3.1.0-~~),
> librust-proxmox-serde-1+default-dev,
> librust-proxmox-serde-1+serde-json-dev,
> + librust-proxmox-sys-1+default-dev (>= 1.0.1-~~),
> librust-proxmox-time-2+default-dev (>= 2.1.0-~~),
> librust-proxmox-uuid-1+default-dev (>= 1.1.0-~~),
> librust-proxmox-uuid-1+serde-dev (>= 1.1.0-~~),
> @@ -73,14 +76,21 @@ Recommends:
> Suggests:
> librust-proxmox-notify+gotify-dev (= ${binary:Version}),
> librust-proxmox-notify+mail-forwarder-dev (= ${binary:Version}),
> - librust-proxmox-notify+pbs-context-dev (= ${binary:Version}),
> librust-proxmox-notify+sendmail-dev (= ${binary:Version}),
> librust-proxmox-notify+smtp-dev (= ${binary:Version}),
> librust-proxmox-notify+webhook-dev (= ${binary:Version})
> Provides:
> + librust-proxmox-notify+pbs-context-dev (= ${binary:Version}),
> + librust-proxmox-notify+pve-context-dev (= ${binary:Version}),
> librust-proxmox-notify-1-dev (= ${binary:Version}),
> + librust-proxmox-notify-1+pbs-context-dev (= ${binary:Version}),
> + librust-proxmox-notify-1+pve-context-dev (= ${binary:Version}),
> librust-proxmox-notify-1.0-dev (= ${binary:Version}),
> - librust-proxmox-notify-1.0.3-dev (= ${binary:Version})
> + librust-proxmox-notify-1.0+pbs-context-dev (= ${binary:Version}),
> + librust-proxmox-notify-1.0+pve-context-dev (= ${binary:Version}),
> + librust-proxmox-notify-1.0.3-dev (= ${binary:Version}),
> + librust-proxmox-notify-1.0.3+pbs-context-dev (= ${binary:Version}),
> + librust-proxmox-notify-1.0.3+pve-context-dev (= ${binary:Version})
> Description: Notification base and plugins - Rust source code
> Source code for Debianized Rust crate "proxmox-notify"
>
> @@ -126,8 +136,7 @@ Depends:
> ${misc:Depends},
> librust-proxmox-notify-dev (= ${binary:Version}),
> librust-mail-parser-0.11+default-dev,
> - librust-proxmox-sendmail-1+mail-forwarder-dev (>= 1.0.2-~~),
> - librust-proxmox-sys-1+default-dev (>= 1.0.1-~~)
> + librust-proxmox-sendmail-1+mail-forwarder-dev (>= 1.0.2-~~)
> Provides:
> librust-proxmox-notify-1+mail-forwarder-dev (= ${binary:Version}),
> librust-proxmox-notify-1.0+mail-forwarder-dev (= ${binary:Version}),
> @@ -136,35 +145,13 @@ Description: Notification base and plugins - feature "mail-forwarder"
> This metapackage enables feature "mail-forwarder" for the Rust proxmox-notify
> crate, by pulling in any additional dependencies needed by that feature.
>
> -Package: librust-proxmox-notify+pbs-context-dev
> -Architecture: any
> -Multi-Arch: same
> -Depends:
> - ${misc:Depends},
> - librust-proxmox-notify-dev (= ${binary:Version}),
> - librust-proxmox-sys-1+default-dev (>= 1.0.1-~~)
> -Provides:
> - librust-proxmox-notify+pve-context-dev (= ${binary:Version}),
> - librust-proxmox-notify-1+pbs-context-dev (= ${binary:Version}),
> - librust-proxmox-notify-1+pve-context-dev (= ${binary:Version}),
> - librust-proxmox-notify-1.0+pbs-context-dev (= ${binary:Version}),
> - librust-proxmox-notify-1.0+pve-context-dev (= ${binary:Version}),
> - librust-proxmox-notify-1.0.3+pbs-context-dev (= ${binary:Version}),
> - librust-proxmox-notify-1.0.3+pve-context-dev (= ${binary:Version})
> -Description: Notification base and plugins - feature "pbs-context" and 1 more
> - This metapackage enables feature "pbs-context" for the Rust proxmox-notify
> - crate, by pulling in any additional dependencies needed by that feature.
> - .
> - Additionally, this package also provides the "pve-context" feature.
> -
> Package: librust-proxmox-notify+sendmail-dev
> Architecture: any
> Multi-Arch: same
> Depends:
> ${misc:Depends},
> librust-proxmox-notify-dev (= ${binary:Version}),
> - librust-proxmox-sendmail-1+default-dev (>= 1.0.2-~~),
> - librust-proxmox-sys-1+default-dev (>= 1.0.1-~~)
> + librust-proxmox-sendmail-1+default-dev (>= 1.0.2-~~)
> Provides:
> librust-proxmox-notify-1+sendmail-dev (= ${binary:Version}),
> librust-proxmox-notify-1.0+sendmail-dev (= ${binary:Version}),
> diff --git a/proxmox-notify/src/context/mod.rs b/proxmox-notify/src/context/mod.rs
> index 8b6e2c43..a3942e5f 100644
> --- a/proxmox-notify/src/context/mod.rs
> +++ b/proxmox-notify/src/context/mod.rs
> @@ -1,6 +1,8 @@
> use std::fmt::Debug;
> use std::sync::Mutex;
>
> +#[cfg(feature = "smtp")]
> +use crate::endpoints::smtp::State;
> use crate::renderer::TemplateSource;
> use crate::Error;
>
> @@ -32,6 +34,21 @@ pub trait Context: Send + Sync + Debug {
> namespace: Option<&str>,
> source: TemplateSource,
> ) -> Result<Option<String>, Error>;
> + /// Load OAuth state for `endpoint_name`.
> + ///
> + /// The state file does not need to be locked, it is okay to just let the faster node "win"
> + /// as long as the invariants documented by [`smtp::xoauth2::get_microsoft_token`] and
> + /// [`smtp::xoauth2::get_google_token`] hold, see those functions' doc comments for details.
> + #[cfg(feature = "smtp")]
> + fn load_oauth_state(&self, endpoint_name: &str) -> Result<State, Error>;
> + /// Save OAuth state `state` for `endpoint_name`. Passing `None` deletes
> + /// the state file for `endpoint_name`.
> + ///
> + /// The state file does not need to be locked, it is okay to just let the faster node "win"
> + /// as long as the invariants documented by [`smtp::xoauth2::get_microsoft_token`] and
> + /// [`smtp::xoauth2::get_google_token`] hold, see those functions' doc comments for details.
> + #[cfg(feature = "smtp")]
> + fn save_oauth_state(&self, endpoint_name: &str, state: Option<State>) -> Result<(), Error>;

i'm not sure the no-locking approach here works. if i understand
correctly, the following example is possible:

A: calls `trigger_state_refresh`, does not lock the notification config
B: calls `delete_smtp_endpoint`, locks the notification config
A: reads the refresh token
B: removes the endpoint and state file
A: finishes refreshing the token and safes it out

if we extend that example with a C that adds an endpoint with the same
name after B finishes, then it would suddenly be left with a state file
from the previous endpoint. similar examples can also happen when
calling `build_transport` since that also includes a read & write cycle
of the state file.

if the state file were locked properly, B would fail to acquire the lock
when trying to delete it (or have to wait until A finishes).

if this behaviour is fine, maybe this would benefit from some
documentation.

> }
>
> #[cfg(not(test))]
> diff --git a/proxmox-notify/src/context/pbs.rs b/proxmox-notify/src/context/pbs.rs
> index 3e5da59c..8c5fce6b 100644
> --- a/proxmox-notify/src/context/pbs.rs
> +++ b/proxmox-notify/src/context/pbs.rs
> @@ -1,5 +1,6 @@
> use std::path::Path;
>
> +use proxmox_sys::fs::CreateOptions;
> use serde::Deserialize;
> use tracing::error;
>
> @@ -7,6 +8,8 @@ use proxmox_schema::{ObjectSchema, Schema, StringSchema};
> use proxmox_section_config::{SectionConfig, SectionConfigPlugin};
>
> use crate::context::{common, Context};
> +#[cfg(feature = "smtp")]
> +use crate::endpoints::smtp::State;
> use crate::renderer::TemplateSource;
> use crate::Error;
>
> @@ -125,6 +128,26 @@ impl Context for PBSContext {
> .map_err(|err| Error::Generic(format!("could not load template: {err}")))?;
> Ok(template_string)
> }
> +
> + #[cfg(feature = "smtp")]
> + fn load_oauth_state(&self, endpoint_name: &str) -> Result<State, Error> {
> + let path =
> + format!("/var/lib/proxmox-backup/notifications/oauth-state/{endpoint_name}.json");
> + State::load(path)
> + }
> +
> + #[cfg(feature = "smtp")]
> + fn save_oauth_state(&self, endpoint_name: &str, state: Option<State>) -> Result<(), Error> {
> + let path =
> + format!("/var/lib/proxmox-backup/notifications/oauth-state/{endpoint_name}.json");
> + match state {
> + Some(s) => s.save(
> + path,
> + CreateOptions::new().perm(nix::sys::stat::Mode::from_bits_truncate(0o600)),
> + ),
> + None => Ok(State::delete(path)),
> + }
> + }
> }
>
> #[cfg(test)]
> diff --git a/proxmox-notify/src/context/pve.rs b/proxmox-notify/src/context/pve.rs
> index a97cce26..2befd53b 100644
> --- a/proxmox-notify/src/context/pve.rs
> +++ b/proxmox-notify/src/context/pve.rs
> @@ -1,7 +1,12 @@
> +use std::path::Path;
> +
> +use proxmox_sys::fs::CreateOptions;
> +
> use crate::context::{common, Context};
> +#[cfg(feature = "smtp")]
> +use crate::endpoints::smtp::State;
> use crate::renderer::TemplateSource;
> use crate::Error;
> -use std::path::Path;
>
> fn lookup_mail_address(content: &str, user: &str) -> Option<String> {
> common::normalize_for_return(content.lines().find_map(|line| {
> @@ -74,6 +79,24 @@ impl Context for PVEContext {
> .map_err(|err| Error::Generic(format!("could not load template: {err}")))?;
> Ok(template_string)
> }
> +
> + #[cfg(feature = "smtp")]
> + fn load_oauth_state(&self, endpoint_name: &str) -> Result<State, Error> {
> + let path = format!("/etc/pve/priv/notifications/oauth-state/{endpoint_name}.json");
> + State::load(path)
> + }
> +
> + #[cfg(feature = "smtp")]
> + fn save_oauth_state(&self, endpoint_name: &str, state: Option<State>) -> Result<(), Error> {
> + let path = format!("/etc/pve/priv/notifications/oauth-state/{endpoint_name}.json");
> + match state {
> + Some(s) => s.save(
> + path,
> + CreateOptions::new().perm(nix::sys::stat::Mode::from_bits_truncate(0o600)),
> + ),
> + None => Ok(State::delete(path)),
> + }
> + }
> }
>
> pub static PVE_CONTEXT: PVEContext = PVEContext;
> diff --git a/proxmox-notify/src/context/test.rs b/proxmox-notify/src/context/test.rs
> index 2c236b4c..9a653343 100644
> --- a/proxmox-notify/src/context/test.rs
> +++ b/proxmox-notify/src/context/test.rs
> @@ -1,4 +1,8 @@
> +use proxmox_sys::fs::CreateOptions;
> +
> use crate::context::Context;
> +#[cfg(feature = "smtp")]
> +use crate::endpoints::smtp::State;
> use crate::renderer::TemplateSource;
> use crate::Error;
>
> @@ -40,4 +44,22 @@ impl Context for TestContext {
> ) -> Result<Option<String>, Error> {
> Ok(Some(String::new()))
> }
> +
> + #[cfg(feature = "smtp")]
> + fn load_oauth_state(&self, endpoint_name: &str) -> Result<State, Error> {
> + let path = format!("/tmp/notifications/oauth-state/{endpoint_name}.json");
> + State::load(path)
> + }
> +
> + #[cfg(feature = "smtp")]
> + fn save_oauth_state(&self, endpoint_name: &str, state: Option<State>) -> Result<(), Error> {
> + let path = format!("/tmp/notifications/oauth-state/{endpoint_name}.json");
> + match state {
> + Some(s) => s.save(
> + path,
> + CreateOptions::new().perm(nix::sys::stat::Mode::from_bits_truncate(0o750)),
> + ),
> + None => Ok(State::delete(path)),
> + }
> + }
> }
> diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/endpoints/smtp.rs
> index d1cdb540..172bcdba 100644
> --- a/proxmox-notify/src/endpoints/smtp.rs
> +++ b/proxmox-notify/src/endpoints/smtp.rs
> @@ -25,6 +25,8 @@ const SMTP_TIMEOUT: u16 = 5;
>
> mod xoauth2;
>
> +pub use xoauth2::State;
> +
> #[api]
> #[derive(Debug, Serialize, Deserialize, Default, Clone, Copy)]
> #[serde(rename_all = "kebab-case")]
> diff --git a/proxmox-notify/src/endpoints/smtp/xoauth2.rs b/proxmox-notify/src/endpoints/smtp/xoauth2.rs
> index 06da0e79..1df5b447 100644
> --- a/proxmox-notify/src/endpoints/smtp/xoauth2.rs
> +++ b/proxmox-notify/src/endpoints/smtp/xoauth2.rs
> @@ -1,10 +1,107 @@
> +use std::path::Path;
> +
> use oauth2::{
> basic::BasicClient, AccessToken, AuthUrl, ClientId, ClientSecret, RefreshToken, TokenResponse,
> TokenUrl,
> };
> +use serde::{Deserialize, Serialize};
> +use tracing::{debug, error};
>
> use crate::Error;
>
> +#[derive(Serialize, Deserialize, Clone, Debug, Default)]
> +#[serde(rename_all = "kebab-case")]
> +/// Persistent state for XOAUTH2 SMTP endpoints.
> +pub struct State {
> + /// OAuth2 refresh token for this endpoint.
> + #[serde(skip_serializing_if = "Option::is_none")]
> + pub oauth2_refresh_token: Option<String>,
> + /// Unix timestamp (seconds) of the last successful token refresh.
> + pub last_refreshed: i64,
> +}
> +
> +impl State {
> + /// Instantiate a new [`State`]. `last_refreshed` is expected to be the UNIX
> + /// timestamp (seconds) of the instantiation time.
> + pub fn new(refresh_token: String, last_refreshed: i64) -> Self {
> + Self {
> + oauth2_refresh_token: Some(refresh_token),
> + last_refreshed,
> + }
> + }
> +
> + /// Load state from `path` instantiating a default object if no state exists.
> + ///
> + /// # Errors
> + /// An [`Error`] is returned if deserialization of the state object fails.
> + pub fn load<P: AsRef<Path>>(path: P) -> Result<State, Error> {
> + let path_str = path.as_ref().to_string_lossy();
> + match proxmox_sys::fs::file_get_optional_contents(&path)
> + .map_err(|e| Error::StateRetrieval(path_str.to_string(), e.into()))?
> + {
> + Some(bytes) => {
> + debug!("loaded state file from {path_str}");
> + serde_json::from_slice(&bytes)
> + .map_err(|e| Error::StateRetrieval(path_str.to_string(), e.into()))
> + }
> + None => {
> + debug!(
> + "no existing state file found for endpoint at {path_str}, creating empty state"
> + );
> + Ok(State::default())
> + }
> + }
> + }
> +
> + /// Persist the state at `path` with `options`.
> + ///
> + /// # Errors
> + /// An [`Error`] is returned if serialization of the state object, or the final write, fail.
> + pub fn save<P: AsRef<Path>>(
> + self,
> + path: P,
> + options: proxmox_sys::fs::CreateOptions,
> + ) -> Result<(), Error> {
> + let path_str = path.as_ref().to_string_lossy();
> + let parent = path.as_ref().parent().unwrap();
> +

imo returning an error here might be nicer. while this isn't likely to
happen, this would panic if called with `path` set to an empty string or
similar. if you want to not error out here, at least turn that into an
`expect` and document the panic in the comment above.

side note: thanks for the extensive documentation here in general!

> + debug!("attempting to persist state at {path_str}");
> +
> + proxmox_sys::fs::create_path(parent, Some(options), Some(options))
> + .map_err(|e| Error::StatePersistence(path_str.to_string(), e.into()))?;
> +
> + let s = serde_json::to_string_pretty(&self)
> + .map_err(|e| Error::StatePersistence(path_str.to_string(), e.into()))?;
> +
> + proxmox_sys::fs::replace_file(&path, s.as_bytes(), options, false)
> + .map_err(|e| Error::StatePersistence(path_str.to_string(), e.into()))
> + }
> +
> + /// Delete the state file at `path`.
> + ///
> + /// Errors are logged but not propagated.
> + pub fn delete<P: AsRef<Path>>(path: P) {
> + if let Err(e) = std::fs::remove_file(&path)
> + && e.kind() != std::io::ErrorKind::NotFound
> + {
> + let path_str = path.as_ref().to_string_lossy();
> + error!("could not delete state file at {path_str}: {e}");
> + }
> + }
> +
> + /// Set `last_refreshed`.
> + pub fn set_last_refreshed(mut self, last_refreshed: i64) -> Self {
> + self.last_refreshed = last_refreshed;
> + self
> + }
> +
> + /// Set `oauth2_refresh_token`.
> + pub fn set_oauth2_refresh_token(mut self, oauth2_refresh_token: Option<String>) -> Self {
> + self.oauth2_refresh_token = oauth2_refresh_token;
> + self
> + }
> +}
> +
> /// This newtype implements the `SyncHttpClient` trait for [`ureq::Agent`]. This allows
> /// us to avoid pulling in a different backend like `reqwest`.
> ///
> diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
> index 879f8326..619dd7db 100644
> --- a/proxmox-notify/src/lib.rs
> +++ b/proxmox-notify/src/lib.rs
> @@ -41,6 +41,10 @@ pub enum Error {
> FilterFailed(String),
> /// The notification's template string could not be rendered
> RenderError(Box<dyn StdError + Send + Sync>),
> + /// The state for an endpoint could not be persisted
> + StatePersistence(String, Box<dyn StdError + Send + Sync>),
> + /// The state for an endpoint could not be retrieved
> + StateRetrieval(String, Box<dyn StdError + Send + Sync>),
> /// Generic error for anything else
> Generic(String),
> }
> @@ -70,6 +74,12 @@ impl Display for Error {
> Error::FilterFailed(message) => {
> write!(f, "could not apply filter: {message}")
> }
> + Error::StatePersistence(path, err) => {
> + write!(f, "could not persist state at {path}: {err}")
> + }
> + Error::StateRetrieval(path, err) => {
> + write!(f, "could not retrieve state from {path}: {err}")
> + }
> Error::RenderError(err) => write!(f, "could not render notification template: {err}"),
> Error::Generic(message) => f.write_str(message),
> }
> @@ -86,6 +96,8 @@ impl StdError for Error {
> Error::TargetTestFailed(errs) => Some(&*errs[0]),
> Error::FilterFailed(_) => None,
> Error::RenderError(err) => Some(&**err),
> + Error::StatePersistence(_, err) => Some(&**err),
> + Error::StateRetrieval(_, err) => Some(&**err),
> Error::Generic(_) => None,
> }
> }