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 EB67D1FF139 for ; Tue, 10 Feb 2026 11:23:37 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id BC0EE1B9CC; Tue, 10 Feb 2026 11:24:20 +0100 (CET) Date: Tue, 10 Feb 2026 11:23:45 +0100 From: Arthur Bied-Charreton To: Lukas Wagner Subject: Re: [PATCH proxmox 1/5] notify: Introduce xoauth2 module Message-ID: References: <20260204161354.458814-1-a.bied-charreton@proxmox.com> <20260204161354.458814-2-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1770718941697 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.894 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: SC4JYUR7DJIFFGAAQTAV6RMEPGK7JHJ6 X-Message-ID-Hash: SC4JYUR7DJIFFGAAQTAV6RMEPGK7JHJ6 X-MailFrom: a.bied-charreton@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 Tue, Feb 10, 2026 at 09:24:34AM +0100, Lukas Wagner wrote: > 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. > Makes sense, I edited my PR, 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. > I will update the docs to reflect this choice better. There are some OAuth2 providers like Auth0 where you can configure stricter token rotation [0] (e.g new token issued at every use and old one instantly invalidated), but it seems to be more targeted at apps that store refresh tokens in client memory, not long-term authorization like the kind we need here. [0] https://auth0.com/docs/secure/tokens/refresh-tokens/configure-refresh-token-rotation