From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 39B981FF136 for ; Mon, 23 Mar 2026 13:26:27 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 79618158CC; Mon, 23 Mar 2026 13:26:46 +0100 (CET) Content-Type: text/plain; charset=UTF-8 Date: Mon, 23 Mar 2026 13:26:05 +0100 Message-Id: Subject: Re: [PATCH proxmox 2/7] notify (smtp): Introduce state module 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-3-a.bied-charreton@proxmox.com> In-Reply-To: <20260213160415.609868-3-a.bied-charreton@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774268719985 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: 6GXV77EPYJNE6GQOTGK44T4JHLKX6HUV X-Message-ID-Hash: 6GXV77EPYJNE6GQOTGK44T4JHLKX6HUV 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: 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 > --- > 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/conte= xt/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; > =20 > +use proxmox_sys::fs::CreateOptions; > + > use crate::renderer::TemplateSource; > use crate::Error; > =20 > @@ -32,6 +34,10 @@ pub trait Context: Send + Sync + Debug { > namespace: Option<&str>, > source: TemplateSource, > ) -> Result, 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 =3D { workspace =3D true, features =3D [= "api-macro", "api-types"] } proxmox-section-config =3D { workspace =3D true } proxmox-serde.workspace =3D true proxmox-sendmail =3D { workspace =3D true, optional =3D true } -proxmox-sys =3D { workspace =3D true, optional =3D true } +proxmox-sys =3D { workspace =3D true } proxmox-time.workspace =3D true proxmox-uuid =3D { workspace =3D true, features =3D ["serde"] } nix.workspace =3D true =20 [features] default =3D ["sendmail", "gotify", "smtp", "webhook"] -mail-forwarder =3D ["dep:mail-parser", "dep:proxmox-sys", "proxmox-sendmai= l/mail-forwarder"] -sendmail =3D ["dep:proxmox-sys", "dep:proxmox-sendmail"] +mail-forwarder =3D ["dep:mail-parser", "proxmox-sendmail/mail-forwarder"] +sendmail =3D ["dep:proxmox-sendmail"] gotify =3D ["dep:proxmox-http", "dep:http"] -pve-context =3D ["dep:proxmox-sys"] -pbs-context =3D ["dep:proxmox-sys"] -smtp =3D ["dep:lettre", "dep:oauth2", "dep:ureq", "dep:http", "dep:proxmox= -sys"] +smtp =3D ["dep:lettre", "dep:oauth2", "dep:ureq", "dep:http"] webhook =3D ["dep:http", "dep:percent-encoding", "dep:proxmox-base64", "de= p:proxmox-http"] > } > =20 > #[cfg(not(test))] > diff --git a/proxmox-notify/src/context/pbs.rs b/proxmox-notify/src/conte= xt/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 templa= te: {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... > + } > } > =20 > #[cfg(test)] > diff --git a/proxmox-notify/src/context/pve.rs b/proxmox-notify/src/conte= xt/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 templa= te: {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. > + } > } > =20 > pub static PVE_CONTEXT: PVEContext =3D PVEContext; > diff --git a/proxmox-notify/src/context/test.rs b/proxmox-notify/src/cont= ext/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, 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/en= dpoints/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 =3D 587; > const SMTP_SUBMISSION_TLS_PORT: u16 =3D 465; > const SMTP_TIMEOUT: u16 =3D 5; > =20 > +mod state; > mod xoauth2; > =20 > +pub(crate) use state::State; > + > #[api] > #[derive(Debug, Serialize, Deserialize, Default, Clone, Copy)] > #[serde(rename_all =3D "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 =3D "kebab-case")] > +pub(crate) struct State { > + #[serde(skip_serializing_if =3D "Option::is_none")] > + pub oauth2_refresh_token: Option, > + pub last_refreshed: i64, > +} > + > +impl State { > + /// Instantiate a new [`State`]. > + pub(crate) fn new(oauth2_refresh_token: Option) -> Self { > + Self { > + oauth2_refresh_token, > + last_refreshed: proxmox_time::epoch_i64(), > + } > + } > + > + /// Load the state for the endpoint identified by `name`, instantiat= ing 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 { > + match proxmox_sys::fs::file_get_optional_contents(context().stat= e_file_path(name)) > + .map_err(|e| Error::ConfigDeserialization(e.into()))? > + { > + Some(bytes) =3D> { > + serde_json::from_slice(&bytes).map_err(|e| Error::Config= Deserialization(e.into())) > + } > + None =3D> Ok(State::default()), > + } > + } > + > + /// Persist the state for the endpoint identified by `name`. > + /// > + /// # Errors > + /// An [`Error`] is returned if serialization of the state object, o= r the final write, fail. > + pub(crate) fn store(self, name: &str) -> Result<(), Error> { > + let path =3D context().state_file_path(name); > + let parent =3D std::path::Path::new(&path).parent().unwrap(); > + > + proxmox_sys::fs::ensure_dir_exists(parent, &context().secret_cre= ate_options(), false) > + .map_err(|e| Error::ConfigSerialization(e.into()))?; I'm getting a "bad permissions on "/etc/pve/priv/notifications" (0o700 !=3D 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 =3D serde_json::to_string_pretty(&self) > + .map_err(|e| Error::ConfigSerialization(e.into()))?; > + > + proxmox_sys::fs::replace_file(path, s.as_bytes(), context().secr= et_create_options(), true) > + .map_err(|e| Error::ConfigSerialization(e.into())) > + } > + > + /// Set `last_refreshed`. > + pub(crate) fn set_last_refreshed(mut self, last_refreshed: i64) -> S= elf { > + self.last_refreshed =3D last_refreshed; > + self > + } > + > + /// Set `oauth2_refresh_token`. > + pub(crate) fn set_oauth2_refresh_token(mut self, oauth2_refresh_toke= n: Option) -> Self { > + self.oauth2_refresh_token =3D oauth2_refresh_token; > + self > + } > +}