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 76D9A1FF140 for ; Fri, 24 Apr 2026 09:48:33 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A09FCB781; Fri, 24 Apr 2026 09:48:30 +0200 (CEST) Date: Fri, 24 Apr 2026 09:47:52 +0200 From: Arthur Bied-Charreton To: Shannon Sterz Subject: Re: [PATCH proxmox v4 03/24] notify: smtp: Introduce state management Message-ID: References: <20260421115957.402589-1-a.bied-charreton@proxmox.com> <20260421115957.402589-4-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: 1777016783844 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.783 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: 2X2Q5NR7DMA6ZOJ3IYMEFYR7WRP5QFEE X-Message-ID-Hash: 2X2Q5NR7DMA6ZOJ3IYMEFYR7WRP5QFEE 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: pbs-devel@lists.proxmox.com, 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 Thu, Apr 23, 2026 at 02:24:37PM +0200, Shannon Sterz wrote: > 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. > > > > [...] > > @@ -32,6 +34,21 @@ pub trait Context: Send + Sync + Debug { > > namespace: Option<&str>, > > source: TemplateSource, > > ) -> Result, 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; > > + /// 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) -> 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. > Yes you are right, I may have been too focused on refresh token validity and overlooked this - thanks a lot for catching it! > 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. > I will add locks in v5, I don't think we are gaining that much from a lock-free approach here, per-endpoint locking will be pretty cheap anyway. [...] > > + > > + /// 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>( > > + 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. > That unwrap is uncalled for yes, looking at the code again I realized this could just be if let Some(parent) = path.as_ref().parent() { create_path(parent, ...); } - thanks! > 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())) > > + } [...]