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

["Shannon Sterz" <s.sterz@proxmox.com>]

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,
>          }
>      }