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

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

Hi Arthur, thanks for the patch!

some notes inline.

On Wed Feb 4, 2026 at 5:13 PM CET, Arthur Bied-Charreton wrote:
> Prepare proxmox-notify to use the oauth2 for SMTP XOAUTH2 support.
>
> The xoauth2 module handles some of the implementation details related to
> supporting XOAUTH2 for SMTP notification targets.
>
> * Add a ureq::Agent newtype wrapper implementing the SyncHttpClient
>   trait to allow using ureq as oauth2 backend, since OAuth2 dropped the
>   ureq feature. Debian seems to have patched it out due to a ureq 2/3
>   version
>   mismatch [1].
> * Add get_{google,microsoft}_token functions
>
> [1]
> https://git.proxmox.com/?p=debcargo-conf.git;a=blob;f=src/oauth2/debian/patches/disable-ureq.patch;h=828b883a83a86927c5cd32df055226a5e78e8bea;hb=refs/heads/proxmox/trixie
>
> Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
> ---
>  proxmox-notify/Cargo.toml     |   4 +
>  proxmox-notify/debian/control |  10 ++-
>  proxmox-notify/src/lib.rs     |   1 +
>  proxmox-notify/src/xoauth2.rs | 146 ++++++++++++++++++++++++++++++++++
>  4 files changed, 159 insertions(+), 2 deletions(-)
>  create mode 100644 proxmox-notify/src/xoauth2.rs
>
> 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).

> +ureq = { version = "3.0.11", features = ["platform-verifier"] }
> +
> +
>  openssl.workspace = true
>  percent-encoding = { workspace = true, optional = true }
>  regex.workspace = true
> diff --git a/proxmox-notify/debian/control b/proxmox-notify/debian/control
> index e588e485..7770f5ee 100644
> --- a/proxmox-notify/debian/control
> +++ b/proxmox-notify/debian/control
> @@ -11,6 +11,7 @@ Build-Depends-Arch: cargo:native <!nocheck>,
>   librust-handlebars-5+default-dev <!nocheck>,
>   librust-http-1+default-dev <!nocheck>,
>   librust-lettre-0.11+default-dev (>= 0.11.1-~~) <!nocheck>,
> + librust-oauth2-5+default-dev <!nocheck>,
>   librust-openssl-0.10+default-dev <!nocheck>,
>   librust-percent-encoding-2+default-dev (>= 2.1-~~) <!nocheck>,
>   librust-proxmox-base64-1+default-dev <!nocheck>,
> @@ -33,7 +34,9 @@ Build-Depends-Arch: cargo:native <!nocheck>,
>   librust-serde-1+default-dev <!nocheck>,
>   librust-serde-1+derive-dev <!nocheck>,
>   librust-serde-json-1+default-dev <!nocheck>,
> - librust-tracing-0.1+default-dev <!nocheck>
> + librust-tracing-0.1+default-dev <!nocheck>,
> + librust-ureq-3+default-dev (>= 3.0.11-~~) <!nocheck>,
> + librust-ureq-3+platform-verifier-dev (>= 3.0.11-~~) <!nocheck>
>  Maintainer: Proxmox Support Team <support@proxmox.com>
>  Standards-Version: 4.7.2
>  Vcs-Git: git://git.proxmox.com/git/proxmox.git
> @@ -49,6 +52,7 @@ Depends:
>   librust-anyhow-1+default-dev,
>   librust-const-format-0.2+default-dev,
>   librust-handlebars-5+default-dev,
> + librust-oauth2-5+default-dev,
>   librust-openssl-0.10+default-dev,
>   librust-proxmox-http-error-1+default-dev,
>   librust-proxmox-human-byte-1+default-dev,
> @@ -65,7 +69,9 @@ Depends:
>   librust-serde-1+default-dev,
>   librust-serde-1+derive-dev,
>   librust-serde-json-1+default-dev,
> - librust-tracing-0.1+default-dev
> + librust-tracing-0.1+default-dev,
> + librust-ureq-3+default-dev (>= 3.0.11-~~),
> + librust-ureq-3+platform-verifier-dev (>= 3.0.11-~~)
>  Recommends:
>   librust-proxmox-notify+default-dev (= ${binary:Version})
>  Suggests:
> diff --git a/proxmox-notify/src/lib.rs b/proxmox-notify/src/lib.rs
> index 879f8326..1134027c 100644
> --- a/proxmox-notify/src/lib.rs
> +++ b/proxmox-notify/src/lib.rs
> @@ -24,6 +24,7 @@ pub mod context;
>  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.

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

> +pub struct UreqSyncHttpClient(ureq::Agent);
> +
> +impl Default for UreqSyncHttpClient {
> +    /// Set `max_redirects` to 0 to prevent SSRF, see
> +    /// https://docs.rs/oauth2/latest/oauth2/#security-warning
> +    fn default() -> Self {
> +        Self(ureq::Agent::new_with_config(
> +            ureq::Agent::config_builder().max_redirects(0).build(),
> +        ))
> +    }
> +}
> +
> +impl oauth2::SyncHttpClient for UreqSyncHttpClient {
> +    type Error = oauth2::HttpClientError<ureq::Error>;
> +
> +    fn call(&self, request: oauth2::HttpRequest) -> Result<oauth2::HttpResponse, Self::Error> {
> +        let uri = request.uri().to_string();
> +
> +        let response = match request.method() {
> +            &http::Method::POST => {
> +                let req = request
> +                    .headers()
> +                    .iter()
> +                    .fold(self.0.post(&uri), |req, (name, value)| {
> +                        req.header(name, value)
> +                    });
> +                req.send(request.body()).map_err(Box::new)?
> +            }
> +            &http::Method::GET => {
> +                let req = request
> +                    .headers()
> +                    .iter()
> +                    .fold(self.0.get(&uri), |req, (name, value)| {
> +                        req.header(name, value)
> +                    });
> +                req.call().map_err(Box::new)?
> +            }
> +            m => {
> +                return Err(oauth2::HttpClientError::Other(format!(
> +                    "unexpected method: {m}"
> +                )));
> +            }
> +        };
> +
> +        let mut builder = http::Response::builder().status(response.status());
> +
> +        if let Some(content_type) = response.headers().get(http::header::CONTENT_TYPE) {
> +            builder = builder.header(http::header::CONTENT_TYPE, content_type);
> +        }
> +
> +        let (_, mut body) = response.into_parts();
> +
> +        let body = body.read_to_vec().map_err(Box::new)?;
> +
> +        builder.body(body).map_err(oauth2::HttpClientError::Http)
> +    }
> +}
> +
> +pub struct TokenExchangeResult {
> +    pub access_token: AccessToken,
> +    pub refresh_token: Option<RefreshToken>,
> +}

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

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

> +///
> +/// 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 :)

> +
> +    Ok(TokenExchangeResult {
> +        access_token: token_result.access_token().clone(),
> +        refresh_token: token_result.refresh_token().cloned(),
> +    })
> +}
> +
> +/// Google refresh tokens' TTL is extended at every use. As long as
> +/// a token has been used at least once in the past 6 months, and no
> +/// other expiration reason applies, we can keep the same token.
> +///
> +/// To make sure the token does not expire, it should be enough to periodically
> +/// make an access token request. If the token becomes invalid for whatever
> +/// other reason, we need user intervention to get a new one.
> +///
> +/// https://developers.google.com/identity/protocols/oauth2#expiration
> +pub fn get_google_token(
> +    client_id: ClientId,
> +    client_secret: ClientSecret,
> +    refresh_token: RefreshToken,
> +) -> Result<TokenExchangeResult, Error> {
> +    let client = BasicClient::new(client_id)
> +        .set_client_secret(client_secret)
> +        .set_auth_uri(
> +            AuthUrl::new("https://accounts.google.com/o/oauth2/v2/auth".into())
> +                .map_err(|e| Error::Generic(format!("invalid auth URL: {e}")))?,
> +        )
> +        .set_token_uri(
> +            TokenUrl::new("https://oauth2.googleapis.com/token".into())
> +                .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}")))?;
> +
> +    Ok(TokenExchangeResult {
> +        access_token: token_result.access_token().clone(),
> +        refresh_token: token_result.refresh_token().cloned(),
> +    })
> +}