Re: [PATCH proxmox 2/7] notify (smtp): Introduce state module

["Lukas Wagner" <l.wagner@proxmox.com>]

Hi Arthur!

Generally we'd write the commit subject rather like this:

 "notify: smtp: ...."

 instead of

 "notify (smtp): ..."

 (but not a big deal IMO, just mentioning it)


On Fri Feb 13, 2026 at 5:04 PM CET, Arthur Bied-Charreton wrote:
> The state module exports a new struct with associated functionality for
> loading, updating, and persisting the state for SMTP endpoints with
> OAuth2 configured as authentication method.
>
> The path to the state files, as well as their create options, are
> retrieved through new Context methods to allow portability between PVE
> and PBS.
>
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
>  proxmox-notify/src/context/mod.rs          |  6 ++
>  proxmox-notify/src/context/pbs.rs          |  8 +++
>  proxmox-notify/src/context/pve.rs          |  8 +++
>  proxmox-notify/src/context/test.rs         |  8 +++
>  proxmox-notify/src/endpoints/smtp.rs       |  3 +
>  proxmox-notify/src/endpoints/smtp/state.rs | 67 ++++++++++++++++++++++
>  6 files changed, 100 insertions(+)
>  create mode 100644 proxmox-notify/src/endpoints/smtp/state.rs
>
> diff --git a/proxmox-notify/src/context/mod.rs b/proxmox-notify/src/context/mod.rs
> index 8b6e2c43..492442f9 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;
>  
> +use proxmox_sys::fs::CreateOptions;
> +
>  use crate::renderer::TemplateSource;
>  use crate::Error;
>  
> @@ -32,6 +34,10 @@ pub trait Context: Send + Sync + Debug {
>          namespace: Option<&str>,
>          source: TemplateSource,
>      ) -> Result<Option<String>, Error>;
> +    /// Return the path to the state file for this context.
> +    fn state_file_path(&self, name: &str) -> String;

I think this should rather return a PathBuf. And rename `name` ->
`endpoint_name`?

> +    /// Create options to be used when writing files containing secrets.
> +    fn secret_create_options(&self) -> CreateOptions;

Due to CreateOptions being part of the `Context` trait now, we need to
pull in proxmox-sys unconditionally:

diff --git i/proxmox-notify/Cargo.toml w/proxmox-notify/Cargo.toml
index d816c695..35d5b8cb 100644
--- i/proxmox-notify/Cargo.toml
+++ w/proxmox-notify/Cargo.toml
@@ -36,17 +36,15 @@ 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"]
-smtp = ["dep:lettre", "dep:oauth2", "dep:ureq", "dep:http", "dep:proxmox-sys"]
+smtp = ["dep:lettre", "dep:oauth2", "dep:ureq", "dep:http"]
 webhook = ["dep:http", "dep:percent-encoding", "dep:proxmox-base64", "dep:proxmox-http"]



>  }
>  
>  #[cfg(not(test))]
> diff --git a/proxmox-notify/src/context/pbs.rs b/proxmox-notify/src/context/pbs.rs
> index 3e5da59c..4f93b45d 100644
> --- a/proxmox-notify/src/context/pbs.rs
> +++ b/proxmox-notify/src/context/pbs.rs
> @@ -125,6 +125,14 @@ impl Context for PBSContext {
>              .map_err(|err| Error::Generic(format!("could not load template: {err}")))?;
>          Ok(template_string)
>      }
> +
> +    fn state_file_path(&self, name: &str) -> String {
> +        format!("/var/lib/proxmox-backup/priv/notifications/{name}.json")
> +    }
> +
> +    fn secret_create_options(&self) -> proxmox_sys::fs::CreateOptions {
> +        proxmox_sys::fs::CreateOptions::new().perm(nix::sys::stat::Mode::from_bits_truncate(0o600))

(1) In the trait impl, you could get the appropriate CreateOptions via
proxmox-product-config...

> +    }
>  }
>  
>  #[cfg(test)]
> diff --git a/proxmox-notify/src/context/pve.rs b/proxmox-notify/src/context/pve.rs
> index a97cce26..e30f7b49 100644
> --- a/proxmox-notify/src/context/pve.rs
> +++ b/proxmox-notify/src/context/pve.rs
> @@ -74,6 +74,14 @@ impl Context for PVEContext {
>              .map_err(|err| Error::Generic(format!("could not load template: {err}")))?;
>          Ok(template_string)
>      }
> +
> +    fn state_file_path(&self, name: &str) -> String {
> +        format!("/etc/pve/priv/notifications/{name}.json")

I think `state` should be somewhere in the path -- so maybe one of

  - priv/notification-endpoint-state/{name}.json
  - priv/notification-state/{name}.json
  - priv/notifications/state-{name}.json

> +    }
> +
> +    fn secret_create_options(&self) -> proxmox_sys::fs::CreateOptions {
> +        proxmox_sys::fs::CreateOptions::new().perm(nix::sys::stat::Mode::from_bits_truncate(0o600))

(2) ... here as well, but you probably need to initialize
proxmox-product-config in the proxmox-perl-rs bindings with the correct
UIDs.

> +    }
>  }
>  
>  pub static PVE_CONTEXT: PVEContext = PVEContext;
> diff --git a/proxmox-notify/src/context/test.rs b/proxmox-notify/src/context/test.rs
> index 2c236b4c..7e29d36a 100644
> --- a/proxmox-notify/src/context/test.rs
> +++ b/proxmox-notify/src/context/test.rs
> @@ -40,4 +40,12 @@ impl Context for TestContext {
>      ) -> Result<Option<String>, Error> {
>          Ok(Some(String::new()))
>      }
> +
> +    fn state_file_path(&self, name: &str) -> String {
> +        format!("/tmp/notifications/{name}.json")
> +    }
> +
> +    fn secret_create_options(&self) -> proxmox_sys::fs::CreateOptions {
> +        proxmox_sys::fs::CreateOptions::new().perm(nix::sys::stat::Mode::from_bits_truncate(0o755))
> +    }
>  }
> diff --git a/proxmox-notify/src/endpoints/smtp.rs b/proxmox-notify/src/endpoints/smtp.rs
> index 277b70f4..699ed1c6 100644
> --- a/proxmox-notify/src/endpoints/smtp.rs
> +++ b/proxmox-notify/src/endpoints/smtp.rs
> @@ -23,8 +23,11 @@ const SMTP_SUBMISSION_STARTTLS_PORT: u16 = 587;
>  const SMTP_SUBMISSION_TLS_PORT: u16 = 465;
>  const SMTP_TIMEOUT: u16 = 5;
>  
> +mod state;
>  mod xoauth2;
>  
> +pub(crate) use state::State;
> +
>  #[api]
>  #[derive(Debug, Serialize, Deserialize, Default, Clone, Copy)]
>  #[serde(rename_all = "kebab-case")]
> diff --git a/proxmox-notify/src/endpoints/smtp/state.rs b/proxmox-notify/src/endpoints/smtp/state.rs
> new file mode 100644
> index 00000000..60bef590
> --- /dev/null
> +++ b/proxmox-notify/src/endpoints/smtp/state.rs
> @@ -0,0 +1,67 @@
> +use serde::{Deserialize, Serialize};
> +
> +use crate::{context::context, Error};
> +
> +#[derive(Serialize, Deserialize, Clone, Debug, Default)]
> +#[serde(rename_all = "kebab-case")]
> +pub(crate) struct State {
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub oauth2_refresh_token: Option<String>,
> +    pub last_refreshed: i64,
> +}
> +
> +impl State {
> +    /// Instantiate a new [`State`].
> +    pub(crate) fn new(oauth2_refresh_token: Option<String>) -> Self {
> +        Self {
> +            oauth2_refresh_token,
> +            last_refreshed: proxmox_time::epoch_i64(),
> +        }
> +    }
> +
> +    /// Load the state for the endpoint identified by `name`, instantiating a default object
> +    /// if no state exists.
> +    ///
> +    /// # Errors
> +    /// An [`Error`] is returned if deserialization of the state object fails.
> +    pub(crate) fn load(name: &str) -> Result<State, Error> {
> +        match proxmox_sys::fs::file_get_optional_contents(context().state_file_path(name))
> +            .map_err(|e| Error::ConfigDeserialization(e.into()))?
> +        {
> +            Some(bytes) => {
> +                serde_json::from_slice(&bytes).map_err(|e| Error::ConfigDeserialization(e.into()))
> +            }
> +            None => Ok(State::default()),
> +        }
> +    }
> +
> +    /// Persist the state for the endpoint identified by `name`.
> +    ///
> +    /// # Errors
> +    /// An [`Error`] is returned if serialization of the state object, or the final write, fail.
> +    pub(crate) fn store(self, name: &str) -> Result<(), Error> {
> +        let path = context().state_file_path(name);
> +        let parent = std::path::Path::new(&path).parent().unwrap();
> +
> +        proxmox_sys::fs::ensure_dir_exists(parent, &context().secret_create_options(), false)
> +            .map_err(|e| Error::ConfigSerialization(e.into()))?;

I'm getting a "bad permissions on "/etc/pve/priv/notifications" (0o700
!= 0o600)" error in the system/task logs when the token is refreshed.
The CreateOptions provided by the context are 0o600, but I think pmxcfs
automatically enforces the x-bit on directories, leading to 0o700 perms
on the '/etc/pve/priv/notifications/' directory, which ultimately gives
us this error. I think you either have to add a separate trait method
for the directory permissions/CreateOptions, or add the x-bit to the
existing CreateOptions yourself here at the callsite.

> +
> +        let s = serde_json::to_string_pretty(&self)
> +            .map_err(|e| Error::ConfigSerialization(e.into()))?;
> +
> +        proxmox_sys::fs::replace_file(path, s.as_bytes(), context().secret_create_options(), true)
> +            .map_err(|e| Error::ConfigSerialization(e.into()))
> +    }
> +
> +    /// Set `last_refreshed`.
> +    pub(crate) fn set_last_refreshed(mut self, last_refreshed: i64) -> Self {
> +        self.last_refreshed = last_refreshed;
> +        self
> +    }
> +
> +    /// Set `oauth2_refresh_token`.
> +    pub(crate) fn set_oauth2_refresh_token(mut self, oauth2_refresh_token: Option<String>) -> Self {
> +        self.oauth2_refresh_token = oauth2_refresh_token;
> +        self
> +    }
> +}