Re: [PATCH proxmox v4 07/24] notify: smtp: Add state handling logic

Arthur Bied-Charreton
2026-04-24T07:51:04Z

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(())
> > + }
> > } …