From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 77F9A1FF139 for ; Tue, 10 Feb 2026 09:23:58 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D8A281873D; Tue, 10 Feb 2026 09:24:40 +0100 (CET) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 10 Feb 2026 09:24:34 +0100 Message-Id: Subject: Re: [PATCH proxmox 1/5] notify: Introduce xoauth2 module From: "Lukas Wagner" To: "Arthur Bied-Charreton" , "Lukas Wagner" 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: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1770711790497 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 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: SJQMT67KWGK5CEJTYVUAIOW7JYEMTMY5 X-Message-ID-Hash: SJQMT67KWGK5CEJTYVUAIOW7JYEMTMY5 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 CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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! >>=20 >> some notes inline. >>=20 >> 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 =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" } >>=20 >> This misses a 'default-features =3D false' here, otherwise it will still >> pull in reqwest. >>=20 >> 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. >>=20 >> As an example, see how the `lettre` crate is handled, that one is also >> only needed for the smtp backend. >>=20 >> In general, make sure that something like >> `cargo build --no-default-features --features smtp` >> works (also cross-check with other features). >>=20 >> > [...] >> > pub mod endpoints; >> > pub mod renderer; >> > pub mod schema; >> > +pub mod xoauth2; >>=20 >> This module is only needed for the 'smtp' feature, so this should be >> gated with a=20 >>=20 >> #[cfg(feature =3D 'smtp')] >>=20 >> 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 > Will fix the feature/dependencies issues in v2, thanks for the examples! > >> > =20 >> > #[derive(Debug)] >> > pub enum Error { >> > diff --git a/proxmox-notify/src/xoauth2.rs b/proxmox-notify/src/xoauth= 2.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 cir= cumvents this >> > +/// by implementing the `SyncHttpClient` trait. This allows us to avo= id pulling in >> > +/// a different backend like `reqwest`. >>=20 >> 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. >>=20 >> 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:=20 '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. >>=20 >> 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). >>=20 > 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. >>=20 >> This actually raises a very interesting question. Consider the case of a >> read-only cluster filesystem due to a missing quorum: >>=20 >> - 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 >>=20 >> I guess we would need to find some way here to not 'lose' the token. >>=20 >> 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. >>=20 >> Maybe there is a better way though, these were just some initial ideas..= . >>=20 > I may have formulated this doc comment too conservatively: according to t= he > Microsoft docs on token lifetime [0], while the refresh token does get=20 > "replaced" at every use, the replaced token is not revoked, and each > token has a static lifetime of 90 days.=20 > > 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.=20 > > (All of this assuming that the Microsoft docs are correct, and the=20 > 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= =20 > get a new refresh token anyway, since even the freshest of tokens would b= e > 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.=20 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-tok= ens#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 { >> > + let client =3D BasicClient::new(client_id) >> > + .set_client_secret(client_secret) >> > + .set_auth_uri( >> > + AuthUrl::new("https://login.microsoftonline.com/common/oa= uth2/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 tok= en: {e}")))?; >>=20 >> 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 :) >>=20 > > 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.c= om/T/#u >> > + >> > + Ok(TokenExchangeResult { >> > + access_token: token_result.access_token().clone(), >> > + refresh_token: token_result.refresh_token().cloned(), >> > + }) >> > +}