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

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

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(())
> > +    }
> >  }
[...]