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 3B10C1FF140 for ; Fri, 24 Apr 2026 09:51:08 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 1592FB903; Fri, 24 Apr 2026 09:51:05 +0200 (CEST) Date: Fri, 24 Apr 2026 09:50:56 +0200 From: Arthur Bied-Charreton To: Shannon Sterz Subject: Re: [PATCH proxmox v4 07/24] notify: smtp: Add state handling logic Message-ID: References: <20260421115957.402589-1-a.bied-charreton@proxmox.com> <20260421115957.402589-8-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: 1777016966945 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.779 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com] Message-ID-Hash: V2HKWXDX4DPHLDIUDNHYHQB6H4URMT4U X-Message-ID-Hash: V2HKWXDX4DPHLDIUDNHYHQB6H4URMT4U 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:35PM +0200, Shannon Sterz wrote: > On Tue Apr 21, 2026 at 1:59 PM CEST, Arthur Bied-Charreton wrote: > > Create new state file in add_endpoint, create/update existing one in > > update_endpoint and delete it in delete_endpoint. > > > > Add trigger_state_refresh to the Endpoint trait, with no-op default > > implementation. Implement it in SmtpEndpoint's Endpoint impl to trigger > > an OAuth2 token exchange, in order to rotate an existing token, or > > extend its lifetime. > > > > Since trigger_state_refresh is called in pveupdate, it may be called > > multiple times in quick succession by the different nodes in a > > cluster. In order to avoid unnecessary churn on the state files, the > > last_refreshed field is used to check if the state has been refreshed > > shortly before, and skip the update if that is the case. [...] > > @@ -335,6 +375,43 @@ impl Endpoint for SmtpEndpoint { > > fn disabled(&self) -> bool { > > self.config.disable.unwrap_or_default() > > } > > + > > + fn trigger_state_refresh(&self) -> Result<(), Error> { > > + let state = context().load_oauth_state(self.name())?; > > + > > + let Some(refresh_token) = &state.oauth2_refresh_token else { > > + return Ok(()); > > + }; > > + > > + // The refresh job is configured in pveupdate, which runs once for each node. > > + // Don't refresh if we already did it recently. > > + if SystemTime::now() > > + .duration_since(UNIX_EPOCH + Duration::from_secs(state.last_refreshed as u64)) > > + .map_err(|e| Error::Generic(e.to_string()))? > > + < SMTP_STATE_REFRESH_CUTOFF_SECONDS > > if im not mistaken, this may not work as inteded. since no lock is held > between the `load` and `save` calls, this scenario could happen: > > A: triggers a refresh, reads the old timestamp, triggers a proper > refresh > B: triggers a refresh, reads the old timestamp, triggers a proper > refresh > A: writes the refreshed token and timestamp > B: writes the refreshed token and timestamp > > since multiple tokens can be valid at the same time (from what i can > tell), it doesn't really matter which thread wins the race here in terms > of the token being valid after the refresh. however, this mechanism may > be less effective than intended when it comes to stopping refreshes in > quick succession. > > this would also be fixed by locking i suggested in a previous message. > That is true, thanks! I will add locks in v5, as mentioned in my response to your previous message [0]. [0] https://lore.proxmox.com/pve-devel/nh5pf5gl2z57q5kxzsyfsxpmypocefiawhrxondq5kzei6g4up@bb5v2jdkxqk2/T/#u > > + { > > + return Ok(()); > > + } > > + > > + let Some(auth_method) = self.config.auth_method.as_ref() else { > > + return Ok(()); > > + }; > > + > > + let state = match self > > + .get_access_token(refresh_token, auth_method)? > > + .refresh_token > > + { > > + Some(tok) => state.set_oauth2_refresh_token(Some(tok.into_secret())), // New token was returned, rotate > > + None => state, > > + } > > + .set_last_refreshed(proxmox_time::epoch_i64()); > > + > > + context().save_oauth_state(self.name(), Some(state))?; > > + > > + info!("OAuth2 state refreshed for endpoint `{}`", self.name()); > > + > > + Ok(()) > > + } > > } [...]