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

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

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

> 
> In general, please add doc-strings for 'pub' functions. Also - I have
> not checked the other patches, yet, but also consider using 'pub(crate)'
> if these types are only needed inside this crate (which I think might be
> the case for these).
> 
Will do, 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. 

[0] https://learn.microsoft.com/en-us/entra/identity-platform/refresh-tokens#token-lifetime

> > +///
> > +/// https://learn.microsoft.com/en-us/entra/identity-platform/refresh-tokens#token-lifetime
> > +pub fn get_microsoft_token(
> > +    client_id: ClientId,
> > +    client_secret: ClientSecret,
> > +    tenant_id: &str,
> > +    refresh_token: RefreshToken,
> > +) -> Result<TokenExchangeResult, Error> {
> > +    let client = BasicClient::new(client_id)
> > +        .set_client_secret(client_secret)
> > +        .set_auth_uri(
> > +            AuthUrl::new("https://login.microsoftonline.com/common/oauth2/v2.0/authorize".into())
> > +                .map_err(|e| Error::Generic(format!("invalid auth URL: {e}")))?,
> > +        )
> > +        .set_token_uri(
> > +            TokenUrl::new(format!(
> > +                "https://login.microsoftonline.com/{tenant_id}/oauth2/v2.0/token"
> > +            ))
> > +            .map_err(|e| Error::Generic(format!("invalid token URL: {e}")))?,
> > +        );
> > +
> > +    let token_result = client
> > +        .exchange_refresh_token(&refresh_token)
> > +        .request(&UreqSyncHttpClient::default())
> > +        .map_err(|e| Error::Generic(format!("could not get access token: {e}")))?;
> 
> apologies for the annoying error handling in proxmox-notify, I've been
> meaning to make it more convenient and rethink the error variants a bit,
> but I simply have not gotten to it yet :)
> 

I may not have helped by using Error::Generic everywhere ^^'
Maybe we could add an AuthenticationError variant, and pull in thiserror
to streamline the error handdling a little? Might be something for a
follow-up series though.

> > +
> > +    Ok(TokenExchangeResult {
> > +        access_token: token_result.access_token().clone(),
> > +        refresh_token: token_result.refresh_token().cloned(),
> > +    })
> > +}