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

["Lukas Wagner" <l.wagner@proxmox.com>]

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.

>> 
>> 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. 

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.

>
> [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.
>

Definitely something for a future patch series. While this will
*technically* be a breaking change for this crate, none of the existing
callers do anything with the returned errors except logging it anyway.

I have some other ideas and changes planned for proxmox-notify anyway
(like pulling out the static context, as explained my integration
testing proposal on pdm-devel [0]), so the error refactoring might
be something that I would include there.

[0] https://lore.proxmox.com/all/20260129134418.307552-1-l.wagner@proxmox.com/T/#u

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