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 862801FF16B for ; Fri, 7 Nov 2025 12:11:35 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DBA52EB19; Fri, 7 Nov 2025 12:12:16 +0100 (CET) Message-ID: <13f733b7-72a7-4eff-8a48-a5b611ec8cb9@proxmox.com> Date: Fri, 7 Nov 2025 12:12:12 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox Backup Server development discussion , Hannes Laimer References: <20250909085245.91641-1-h.laimer@proxmox.com> <20250909085245.91641-4-h.laimer@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20250909085245.91641-4-h.laimer@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1762513912786 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.046 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 Subject: Re: [pbs-devel] [PATCH proxmox 3/3] rest-server: add use tag field to RateLimitedStreams X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" comments inline On 9/9/25 10:53 AM, Hannes Laimer wrote: > Similarly to how the IP is attached we also attach the user that is > authenticated on this connections. Since this is only used for rate > limiting this is behind the "rate-limited-stream" feature. > > Signed-off-by: Hannes Laimer > --- > proxmox-rest-server/src/connection.rs | 16 +++++- > proxmox-rest-server/src/rest.rs | 72 ++++++++++++++++++++++++++- > 2 files changed, 85 insertions(+), 3 deletions(-) > > diff --git a/proxmox-rest-server/src/connection.rs b/proxmox-rest-server/src/connection.rs > index 9511b7cb..a9c3ccb3 100644 > --- a/proxmox-rest-server/src/connection.rs > +++ b/proxmox-rest-server/src/connection.rs > @@ -165,7 +165,10 @@ type InsecureClientStreamResult = Pin>; > type ClientStreamResult = Pin>>; > > #[cfg(feature = "rate-limited-stream")] > -type LookupRateLimiter = dyn Fn(std::net::SocketAddr) -> (Option, Option) > +type LookupRateLimiter = dyn Fn( > + std::net::SocketAddr, > + Option, this could be a simple `Tag` or slice thereof > + ) -> (Option, Option) > + Send > + Sync > + 'static; > @@ -369,7 +372,16 @@ impl AcceptBuilder { > > #[cfg(feature = "rate-limited-stream")] > let socket = match self.lookup_rate_limiter.clone() { > - Some(lookup) => RateLimitedStream::with_limiter_update_cb(socket, move || lookup(peer)), > + Some(lookup) => { > + let user_tag: Arc>> = Arc::new(Mutex::new(None)); > + let user_tag_cb = Arc::clone(&user_tag); > + let mut s = RateLimitedStream::with_limiter_update_cb(socket, move || { > + let user = user_tag_cb.lock().unwrap().clone(); > + lookup(peer, user) > + }); > + s.set_user_tag_handle(user_tag); > + s if passed along as parameter to the callback function as suggested in the previous patch, this would simplify to ``` Some(lookup) => RateLimitedStream::with_limiter_update_cb(socket, move |tag| { lookup(peer, tag) }), and the set_user_tag_handle() can be obsolete, setting the tag will now always be done in the constructor or when updated on the rate limited stream > + } > None => RateLimitedStream::with_limiter(socket, None, None), > }; > > diff --git a/proxmox-rest-server/src/rest.rs b/proxmox-rest-server/src/rest.rs > index 035a9537..c7d833a2 100644 > --- a/proxmox-rest-server/src/rest.rs > +++ b/proxmox-rest-server/src/rest.rs > @@ -86,10 +86,26 @@ impl RestServer { > } > } > > - pub fn api_service(&self, peer: &dyn PeerAddress) -> Result { > + #[cfg(not(feature = "rate-limited-stream"))] > + pub fn api_service(&self, peer: &T) -> Result > + where > + T: PeerAddress + ?Sized, > + { > + Ok(ApiService { > + peer: peer.peer_addr()?, > + api_config: Arc::clone(&self.api_config), > + }) > + } > + > + #[cfg(feature = "rate-limited-stream")] > + pub fn api_service(&self, peer: &T) -> Result > + where > + T: PeerAddress + PeerUser + ?Sized, > + { > Ok(ApiService { > peer: peer.peer_addr()?, > api_config: Arc::clone(&self.api_config), > + user_tag: peer.user_tag_handle(), > }) > } > } > @@ -185,6 +201,11 @@ pub trait PeerAddress { > fn peer_addr(&self) -> Result; > } > > +#[cfg(feature = "rate-limited-stream")] > +pub trait PeerUser { > + fn user_tag_handle(&self) -> Option>>>; > +} > + > // tokio_openssl's SslStream requires the stream to be pinned in order to accept it, and we need to > // accept before the peer address is requested, so let's just generally implement this for > // Pin> > @@ -221,6 +242,41 @@ impl PeerAddress for proxmox_http::RateLimitedStream { > } > } > > +#[cfg(feature = "rate-limited-stream")] > +impl PeerUser for Pin> { > + fn user_tag_handle(&self) -> Option>>> { > + T::user_tag_handle(&**self) > + } > +} > + > +#[cfg(feature = "rate-limited-stream")] > +impl PeerUser for tokio_openssl::SslStream { > + fn user_tag_handle(&self) -> Option>>> { > + self.get_ref().user_tag_handle() > + } > +} > + > +#[cfg(feature = "rate-limited-stream")] > +impl PeerUser for tokio::net::TcpStream { > + fn user_tag_handle(&self) -> Option>>> { > + None > + } > +} > + > +#[cfg(feature = "rate-limited-stream")] > +impl PeerUser for tokio::net::UnixStream { > + fn user_tag_handle(&self) -> Option>>> { > + None > + } > +} > + > +#[cfg(feature = "rate-limited-stream")] > +impl PeerUser for proxmox_http::RateLimitedStream { > + fn user_tag_handle(&self) -> Option>>> { > + self.user_tag_handle() > + } > +} > + > // Helper [Service] containing the peer Address > // > // The lower level connection [Service] implementation on > @@ -233,6 +289,8 @@ impl PeerAddress for proxmox_http::RateLimitedStream { > pub struct ApiService { > pub peer: std::net::SocketAddr, > pub api_config: Arc, > + #[cfg(feature = "rate-limited-stream")] > + pub user_tag: Option>>>, > } > > impl ApiService { > @@ -357,6 +415,8 @@ impl Service> for ApiService { > Some(proxied_peer) => proxied_peer, > None => self.peer, > }; > + #[cfg(feature = "rate-limited-stream")] > + let user_tag = self.user_tag.clone(); > > let header = self.api_config > .auth_cookie_name > @@ -394,6 +454,16 @@ impl Service> for ApiService { > } > } > > + #[cfg(feature = "rate-limited-stream")] > + { > + if let Some(handle) = user_tag { > + if let Some(ext) = response.extensions().get::() { > + let mut guard = handle.lock().unwrap(); > + *guard = Some(ext.0.clone()); > + } so this is were the tag is set. This is however only after the response is generated. Maybe we could pass this as callback method to the `handle_request` which internally also does the auth, so it can be set there already? > + } > + } > + > let logger = config.get_access_log(); > log_response(logger, &peer, method, &path, &response, user_agent); > Ok(response) _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel