Re: [PATCH proxmox 1/5] notify: Introduce xoauth2 module

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

On Tue, Feb 10, 2026 at 09:24:34AM +0100, Lukas Wagner wrote:
> On Mon Feb 9, 2026 at 9:34 AM CET, Arthur Bied-Charreton wrote:
> > On Fri, Feb 06, 2026 at 04:00:42PM +0100, Lukas Wagner wrote:
> >> Hi Arthur, thanks for the patch!
> >> 
> >> some notes inline.
> >> 
> >> On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> >> > [...]
> >> > diff --git a/proxmox-notify/Cargo.toml b/proxmox-notify/Cargo.toml
> >> > index bc63e19d..52493ef7 100644
> >> > --- a/proxmox-notify/Cargo.toml
> >> > +++ b/proxmox-notify/Cargo.toml
> >> > @@ -19,6 +19,10 @@ http = { workspace = true, optional = true }
> >> >  lettre = { workspace = true, optional = true }
> >> >  tracing.workspace = true
> >> >  mail-parser = { workspace = true, optional = true }
> >> > +oauth2 = { version = "5.0.0" }
> >> 
> >> This misses a 'default-features = false' here, otherwise it will still
> >> pull in reqwest.
> >> 
> >> Also, the oauth2 and ureq deps should be optional, since they are only
> >> required by the smtp endpoint, which is gated behind a (default)
> >> feature.
> >> 
> >> As an example, see how the `lettre` crate is handled, that one is also
> >> only needed for the smtp backend.
> >> 
> >> In general, make sure that something like
> >>   `cargo build --no-default-features --features smtp`
> >> works (also cross-check with other features).
> >> 
> >> > [...]
> >> >  pub mod endpoints;
> >> >  pub mod renderer;
> >> >  pub mod schema;
> >> > +pub mod xoauth2;
> >> 
> >> This module is only needed for the 'smtp' feature, so this should be
> >> gated with a 
> >> 
> >> #[cfg(feature = 'smtp')]
> >> 
> >> you might need to adapt some of the optional deps in Cargo.toml, e.g.
> >> due to your new module, 'smtp' now also requires 'proxmox-sys', etc.
> >> 
> > Will fix the feature/dependencies issues in v2, thanks for the examples!
> >
> >> >  
> >> >  #[derive(Debug)]
> >> >  pub enum Error {
> >> > diff --git a/proxmox-notify/src/xoauth2.rs b/proxmox-notify/src/xoauth2.rs
> >> > new file mode 100644
> >> > index 00000000..66faabfa
> >> > --- /dev/null
> >> > +++ b/proxmox-notify/src/xoauth2.rs
> >> > @@ -0,0 +1,146 @@
> >> > +use oauth2::{
> >> > +    basic::BasicClient, AccessToken, AuthUrl, ClientId, ClientSecret, RefreshToken, TokenResponse,
> >> > +    TokenUrl,
> >> > +};
> >> > +
> >> > +use crate::Error;
> >> > +
> >> > +/// `oauth2` dropped support for the `ureq` backend, this newtype circumvents this
> >> > +/// by implementing the `SyncHttpClient` trait. This allows us to avoid pulling in
> >> > +/// a different backend like `reqwest`.
> >> 
> >> Maybe clarify here that Debian patched out the ureq backend due to a
> >> version mismatch of the packaged ureq crate and the one pulled in by the
> >> oauth crate. Once this is resolved we might drop this custom client
> >> implementation again. oauth2 uses an old version of ureq; we might be
> >> able to contribute a PR to update it to a newer version.
> >> 
> >> You mentioned it in the commit message, but this would be useful here as
> >> well.
> >>
> > I already opened a PR in oauth2 [0], no answer yet. Will add more
> > details & a link to it to the doc comment.
> >
> > [0] https://github.com/ramosbugs/oauth2-rs/pull/338
> >
> 
> quoting from your PR: 
> 
>   'when trying to use the ureq backend for oauth2, I unfortunately had to
>   find out that while it is documented as a feature in the crate docs, it
>   is no longer available. This seems to be due to the API changes
>   introduced by ureq 3.x.'
> 
> This might be a bit confusing for the maintainer, you should probably
> explain that you are using the Debian-packaged version of oauth2 which
> patched out the support for the ureq backend due to the version
> conflict.
> 
Makes sense, I edited my PR, thanks :)

> >> > +
> >> > +/// Microsoft Identity Platform refresh tokens replace themselves
> >> > +/// upon every use, however the old one does *not* get revoked.
> >> > +///
> >> > +/// This means that every access token request yields both an access
> >> > +/// token AND a new refresh token, which should replace the old one.
> >> > +/// The old one should then be discarded.
> >> 
> >> This actually raises a very interesting question. Consider the case of a
> >> read-only cluster filesystem due to a missing quorum:
> >> 
> >>  - state file is read
> >>  - one or more smtp endpoints refresh their tokens while sending or due
> >>    to the periodic refresh
> >>  - state file is written, but fails due to pmxcfs being R/O
> >> 
> >> I guess we would need to find some way here to not 'lose' the token.
> >> 
> >> Some ideas:
> >>   - check *before* refreshing if we can actually write -> but this is
> >>     still racy
> >>   - when the write fails, save the token in some other location that is
> >>     not in the pmxcfs and then later, when pmxcfs is writable again,
> >>     update the statefile there - this could also be weird, in case
> >>     *multiple* nodes tried to send notifications at roughly the same
> >>     time - which one is the "correct" token then? Hopefully only one of
> >>     these nodes should be able to refresh the token in the first place,
> >>     but still something to keep in mind I think.
> >> 
> >> Maybe there is a better way though, these were just some initial ideas...
> >> 
> > I may have formulated this doc comment too conservatively: according to the
> > Microsoft docs on token lifetime [0], while the refresh token does get 
> > "replaced" at every use, the replaced token is not revoked, and each
> > token has a static lifetime of 90 days. 
> >
> > This means that in this case, I don't think we really *care* which token is
> > the correct one, since they are all still valid. We can afford to lose
> > some and persist the first new token we get after the file system
> > becomes writable again. 
> >
> > (All of this assuming that the Microsoft docs are correct, and the 
> > implementation does not bear any surprises, could not test it myself yet.)
> >
> > If the FS stays read-only for 90+ days, we need user interaction again to 
> > get a new refresh token anyway, since even the freshest of tokens would be
> > expired by then. Now that I think about it, we probably need a way to
> > request a new authorization from the user in the frontend. 
> 
> Ah, that's good to know - so that is hopefully not be an issue in the
> real world (but maybe still warrant a note in the documentation). Also
> let's hope that this does not become an issue with any other email
> provider we might support in the future.
> 
I will update the docs to reflect this choice better. 

There are some OAuth2 providers like Auth0 where you can configure
stricter token rotation [0] (e.g new token issued at every use and old one
instantly invalidated), but it seems to be more targeted at apps that
store refresh tokens in client memory, not long-term authorization like
the kind we need here.

[0] https://auth0.com/docs/secure/tokens/refresh-tokens/configure-refresh-token-rotation