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 55CF11FF136 for ; Mon, 23 Mar 2026 17:32:14 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8AE9A3245B; Mon, 23 Mar 2026 17:32:31 +0100 (CET) Date: Mon, 23 Mar 2026 17:32:23 +0100 From: Arthur Bied-Charreton To: Lukas Wagner Subject: Re: [PATCH proxmox 2/7] notify (smtp): Introduce state module Message-ID: References: <20260213160415.609868-1-a.bied-charreton@proxmox.com> <20260213160415.609868-3-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774283498694 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.836 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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: E2FBZKUF75RYZ5LUXBUMYYUCP65W775V X-Message-ID-Hash: E2FBZKUF75RYZ5LUXBUMYYUCP65W775V X-MailFrom: a.bied-charreton@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 CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Mon, Mar 23, 2026 at 01:26:05PM +0100, Lukas Wagner wrote: > 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) > ACK, got my conventions confused, my bad > [...] > > @@ -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`? > Thanks for catching that, I could not do the usual `P: AsRef` and somehow fell back to `String` directly.. > > + /// 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"] > > ACK > > > } > > > > #[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 > Makes sense, thanks for the suggestion. Will use the third one, it allows using the already existing /priv/notifications directory. > > + } > > + > > + 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. > ACK this and the other proxmox-product-config comment, will integrate that crate in v2, thanks [...] > > + /// 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. > Thanks for catching that! Will fix in v2 > > + > > + 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) -> Self { > > + self.oauth2_refresh_token = oauth2_refresh_token; > > + self > > + } > > +} >