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 F14291FF15E for ; Mon, 10 Nov 2025 09:58:36 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 32FF7102CD; Mon, 10 Nov 2025 09:59:22 +0100 (CET) Message-ID: <62f7c7f9-6fa1-4ccf-81f9-9fb3fc549570@proxmox.com> Date: Mon, 10 Nov 2025 09:59:18 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox Backup Server development discussion , Hannes Laimer References: <20251107132329.42965-1-h.laimer@proxmox.com> <20251107132329.42965-6-h.laimer@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20251107132329.42965-6-h.laimer@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1762765136480 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.048 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 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-backup v2 2/3] traffic-control: handle users specified in a rule correctly 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" 2 nits inline On 11/7/25 2:23 PM, Hannes Laimer wrote: > Signed-off-by: Hannes Laimer > --- > src/bin/proxmox-backup-proxy.rs | 12 +++- > src/traffic_control_cache.rs | 100 +++++++++++++++++++++++++++----- > 2 files changed, 98 insertions(+), 14 deletions(-) > > diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs > index cfd93f92..92a8cb3c 100644 > --- a/src/bin/proxmox-backup-proxy.rs > +++ b/src/bin/proxmox-backup-proxy.rs > @@ -17,6 +17,7 @@ use openssl::ssl::SslAcceptor; > use serde_json::{json, Value}; > > use proxmox_http::Body; > +use proxmox_http::RateLimiterTag; > use proxmox_lang::try_block; > use proxmox_router::{RpcEnvironment, RpcEnvironmentType}; > use proxmox_sys::fs::CreateOptions; > @@ -955,6 +956,7 @@ async fn run_traffic_control_updater() { > > fn lookup_rate_limiter( > peer: std::net::SocketAddr, > + tags: &[RateLimiterTag], > ) -> (Option, Option) { > let mut cache = TRAFFIC_CONTROL_CACHE.lock().unwrap(); > > @@ -962,7 +964,15 @@ fn lookup_rate_limiter( > > cache.reload(now); > > - let (_rule_name, read_limiter, write_limiter) = cache.lookup_rate_limiter(peer, now); > + let user = tags.iter().find_map(|tag| match tag { > + RateLimiterTag::User(user) => Some(user.as_str()), > + }); > + > + let authid = user.and_then(|s| s.parse::().ok()); > + let user_parsed = authid.as_ref().map(|auth_id| auth_id.user()); > + > + let (_rule_name, read_limiter, write_limiter) = > + cache.lookup_rate_limiter(peer, now, user_parsed); > > (read_limiter, write_limiter) > } > diff --git a/src/traffic_control_cache.rs b/src/traffic_control_cache.rs > index 830a8c04..0c2718d5 100644 > --- a/src/traffic_control_cache.rs > +++ b/src/traffic_control_cache.rs > @@ -13,7 +13,7 @@ use proxmox_section_config::SectionConfigData; > > use proxmox_time::{parse_daily_duration, DailyDuration, TmEditor}; > > -use pbs_api_types::TrafficControlRule; > +use pbs_api_types::{TrafficControlRule, Userid}; > > use pbs_config::ConfigVersionCache; > > @@ -322,6 +322,7 @@ impl TrafficControlCache { > /// > /// - Rules where timeframe does not match are skipped. > /// - Rules with smaller network size have higher priority. > + /// - Rules with users are prioritized over IP-only rules, if multiple match > /// > /// Behavior is undefined if more than one rule matches after > /// above selection. > @@ -329,10 +330,15 @@ impl TrafficControlCache { > &self, > peer: SocketAddr, > now: i64, > + user: Option<&Userid>, > ) -> (&str, Option, Option) { > let peer_ip = canonical_ip(peer.ip()); > > - log::debug!("lookup_rate_limiter: {:?}", peer_ip); > + log::debug!( > + "lookup_rate_limiter: {:?} - {}", > + peer_ip, nit: since touching this, the variable should be in lined in the format string > + user.map_or("(no user)", |u| u.as_str()) > + ); > > let now = match TmEditor::with_epoch(now, self.use_utc) { > Ok(now) => now, > @@ -342,19 +348,33 @@ impl TrafficControlCache { > } > }; > > - let mut last_rule_match = None; > + let mut last_rule_match: Option<(&ParsedTcRule, u8, bool)> = None; // (rule, netlen, is_user) nit: IMO this should get its own type def with a proper docstring > > for rule in self.rules.iter() { > if !timeframe_match(&rule.timeframe, &now) { > continue; > } > > + if let Some(ref rule_users) = rule.config.users { > + if let Some(cur_user) = user { > + if !rule_users.iter().any(|u| u == cur_user) { > + continue; > + } > + } else { > + continue; > + } > + } > + > if let Some(match_len) = network_match_len(&rule.networks, &peer_ip) { > + let is_user_rule = rule.config.users.is_some(); > match last_rule_match { > - None => last_rule_match = Some((rule, match_len)), > - Some((_, last_len)) => { > - if match_len > last_len { > - last_rule_match = Some((rule, match_len)); > + None => last_rule_match = Some((rule, match_len, is_user_rule)), > + Some((_, last_len, last_is_user)) => { > + // Prefer rules with users over IP-only rules; for same class use longest prefix > + if (is_user_rule && !last_is_user) > + || (is_user_rule == last_is_user && match_len > last_len) > + { > + last_rule_match = Some((rule, match_len, is_user_rule)); > } > } > } > @@ -362,7 +382,7 @@ impl TrafficControlCache { > } > > match last_rule_match { > - Some((rule, _)) => { > + Some((rule, _, _)) => { > match self.limiter_map.get(&rule.config.name) { > Some((read_limiter, write_limiter)) => ( > &rule.config.name, > @@ -454,34 +474,88 @@ rule: somewhere > let somewhere = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(1, 2, 3, 4)), 1234); > > let (rule, read_limiter, write_limiter) = > - cache.lookup_rate_limiter(somewhere, THURSDAY_80_00); > + cache.lookup_rate_limiter(somewhere, THURSDAY_80_00, None); > assert_eq!(rule, "somewhere"); > assert!(read_limiter.is_some()); > assert!(write_limiter.is_some()); > > - let (rule, read_limiter, write_limiter) = cache.lookup_rate_limiter(local, THURSDAY_19_00); > + let (rule, read_limiter, write_limiter) = > + cache.lookup_rate_limiter(local, THURSDAY_19_00, None); > assert_eq!(rule, "rule2"); > assert!(read_limiter.is_some()); > assert!(write_limiter.is_some()); > > let (rule, read_limiter, write_limiter) = > - cache.lookup_rate_limiter(gateway, THURSDAY_15_00); > + cache.lookup_rate_limiter(gateway, THURSDAY_15_00, None); > assert_eq!(rule, "rule1"); > assert!(read_limiter.is_some()); > assert!(write_limiter.is_some()); > > let (rule, read_limiter, write_limiter) = > - cache.lookup_rate_limiter(gateway, THURSDAY_19_00); > + cache.lookup_rate_limiter(gateway, THURSDAY_19_00, None); > assert_eq!(rule, "somewhere"); > assert!(read_limiter.is_some()); > assert!(write_limiter.is_some()); > > let (rule, read_limiter, write_limiter) = > - cache.lookup_rate_limiter(private, THURSDAY_19_00); > + cache.lookup_rate_limiter(private, THURSDAY_19_00, None); > assert_eq!(rule, "rule2"); > assert!(read_limiter.is_some()); > assert!(write_limiter.is_some()); > > Ok(()) > } > + > + #[test] > + fn test_user_based_rule_match_and_precedence() -> Result<(), Error> { > + // rule user1: user-specific for alice@pam on 192.168.2.0/24 > + // rule ip1: ip-only broader network also matches, but user rule should win > + // rule user2: user-specific for bob@pam but different network shouldn't match > + let config_data = " > +rule: user1 > + comment user rule for alice > + network 192.168.2.0/24 > + rate-in 50000000 > + rate-out 50000000 > + users alice@pam > + > +rule: ip1 > + network 192.168.2.0/24 > + rate-in 100000000 > + rate-out 100000000 > + > +rule: user2 > + network 10.0.0.0/8 > + rate-in 75000000 > + rate-out 75000000 > + users bob@pam > +"; > + > + let config = pbs_config::traffic_control::CONFIG.parse("testconfig", config_data)?; > + > + let mut cache = TrafficControlCache::new(); > + cache.use_utc = true; > + cache.use_shared_memory = false; // avoid permission problems in test environment > + > + cache.update_config(&config)?; > + > + const NOW: i64 = make_test_time(0, 12, 0); > + let peer = SocketAddr::new(IpAddr::V4(Ipv4Addr::new(192, 168, 2, 55)), 1234); > + > + // No user -> should match ip1 > + let (rule, _, _) = cache.lookup_rate_limiter(peer, NOW, None); > + assert_eq!(rule, "ip1"); > + > + // alice@pam -> should match user1 and take precedence over ip1 > + let alice = Userid::try_from("alice@pam".to_string())?; > + let (rule, _, _) = cache.lookup_rate_limiter(peer, NOW, Some(&alice)); > + assert_eq!(rule, "user1"); > + > + // bob@pam on same peer/network -> user2 is different network, should fall back to ip1 > + let bob = Userid::try_from("bob@pam".to_string())?; > + let (rule, _, _) = cache.lookup_rate_limiter(peer, NOW, Some(&bob)); > + assert_eq!(rule, "ip1"); > + > + Ok(()) > + } > } _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel