From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 783041FF13E for ; Fri, 06 Feb 2026 16:00:49 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7029375E5; Fri, 6 Feb 2026 16:01:21 +0100 (CET) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 06 Feb 2026 16:00:42 +0100 Message-Id: Subject: Re: [PATCH proxmox 1/5] notify: Introduce xoauth2 module From: "Lukas Wagner" To: "Arthur Bied-Charreton" , X-Mailer: aerc 0.21.0-0-g5549850facc2-dirty References: <20260204161354.458814-1-a.bied-charreton@proxmox.com> <20260204161354.458814-2-a.bied-charreton@proxmox.com> In-Reply-To: <20260204161354.458814-2-a.bied-charreton@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1770389963693 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.036 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment KAM_SHORT 0.001 Use of a URL Shortener for very short URL RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: ZYXFLD2AX6323YDL7KCXTOYJKYD4HPUE X-Message-ID-Hash: ZYXFLD2AX6323YDL7KCXTOYJKYD4HPUE X-MailFrom: l.wagner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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=3Ddebcargo-conf.git;a=3Dblob;f=3Dsrc/oauth2/de= bian/patches/disable-ureq.patch;h=3D828b883a83a86927c5cd32df055226a5e78e8be= a;hb=3Drefs/heads/proxmox/trixie > > Signed-off-by: Arthur Bied-Charreton > --- > 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 =3D { workspace =3D true, optional =3D true } > lettre =3D { workspace =3D true, optional =3D true } > tracing.workspace =3D true > mail-parser =3D { workspace =3D true, optional =3D true } > +oauth2 =3D { version =3D "5.0.0" } This misses a 'default-features =3D 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 =3D { version =3D "3.0.11", features =3D ["platform-verifier"] } > + > + > openssl.workspace =3D true > percent-encoding =3D { workspace =3D true, optional =3D true } > regex.workspace =3D true > diff --git a/proxmox-notify/debian/control b/proxmox-notify/debian/contro= l > index e588e485..7770f5ee 100644 > --- a/proxmox-notify/debian/control > +++ b/proxmox-notify/debian/control > @@ -11,6 +11,7 @@ Build-Depends-Arch: cargo:native , > librust-handlebars-5+default-dev , > librust-http-1+default-dev , > librust-lettre-0.11+default-dev (>=3D 0.11.1-~~) , > + librust-oauth2-5+default-dev , > librust-openssl-0.10+default-dev , > librust-percent-encoding-2+default-dev (>=3D 2.1-~~) , > librust-proxmox-base64-1+default-dev , > @@ -33,7 +34,9 @@ Build-Depends-Arch: cargo:native , > 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 (>=3D 3.0.11-~~) , > + librust-ureq-3+platform-verifier-dev (>=3D 3.0.11-~~) > Maintainer: Proxmox Support Team > 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 (>=3D 3.0.11-~~), > + librust-ureq-3+platform-verifier-dev (>=3D 3.0.11-~~) > Recommends: > librust-proxmox-notify+default-dev (=3D ${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=20 #[cfg(feature =3D '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. > =20 > #[derive(Debug)] > pub enum Error { > diff --git a/proxmox-notify/src/xoauth2.rs b/proxmox-notify/src/xoauth2.r= s > 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, Re= freshToken, TokenResponse, > + TokenUrl, > +}; > + > +use crate::Error; > + > +/// `oauth2` dropped support for the `ureq` backend, this newtype circum= vents 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 =3D oauth2::HttpClientError; > + > + fn call(&self, request: oauth2::HttpRequest) -> Result { > + let uri =3D request.uri().to_string(); > + > + let response =3D match request.method() { > + &http::Method::POST =3D> { > + let req =3D 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 =3D> { > + let req =3D request > + .headers() > + .iter() > + .fold(self.0.get(&uri), |req, (name, value)| { > + req.header(name, value) > + }); > + req.call().map_err(Box::new)? > + } > + m =3D> { > + return Err(oauth2::HttpClientError::Other(format!( > + "unexpected method: {m}" > + ))); > + } > + }; > + > + let mut builder =3D http::Response::builder().status(response.st= atus()); > + > + if let Some(content_type) =3D response.headers().get(http::heade= r::CONTENT_TYPE) { > + builder =3D builder.header(http::header::CONTENT_TYPE, conte= nt_type); > + } > + > + let (_, mut body) =3D response.into_parts(); > + > + let body =3D 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, > +} 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-to= kens#token-lifetime > +pub fn get_microsoft_token( > + client_id: ClientId, > + client_secret: ClientSecret, > + tenant_id: &str, > + refresh_token: RefreshToken, > +) -> Result { > + let client =3D BasicClient::new(client_id) > + .set_client_secret(client_secret) > + .set_auth_uri( > + AuthUrl::new("https://login.microsoftonline.com/common/oauth= 2/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 =3D 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 perio= dically > +/// make an access token request. If the token becomes invalid for whate= ver > +/// 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 { > + let client =3D 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 =3D 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(), > + }) > +}