[pbs-devel] [PATCH proxmox{, -backup} 0/6] add user specific rate-limits

[pbs-devel] [PATCH proxmox{, -backup} 0/6] add user specific rate-limits

[Hannes Laimer]

This adds support for specifying user specific rate-limits.
We add a user-tag to every rate-limited connection, with this present we
can limit the connection based on the authenticated user assiciated with
it.

Authentication happens after accept, so we can't set this right when we
accept a connection. Currently we initialize the handle on accept, we
then give this handle to the rate_limiter callback function. And on
completed authentication we set the user using this handle.
I did consider using a Peer -> User map in the cache, and just adding
entries on auth, but there isn't really a good way to clean those
entries. And peers(so IP:port) may end up being reused, and that would
be a problem. With the current approach we don't have this problem.

Currently rules with a user specified take priority over others. So:
user > IP only > neither, in case two rules match.

If users and networks are specified, the rule only applies if both
match. So, Any of the specified user connect from any of the specified
network.

And all of this ofc still only if the given timeframe matches.

Note: this is only for users, you can't specify individual tokens. But I
don't think that is much of a problem, it is probably even better like
this.

(I did look through BZ if there is an issue for this, I feel like there
should be, but did not find one)

proxmox:

Hannes Laimer (3):
  pbs-api-types: add users to traffic-control rule
  http: add user tag to rate-limited streams
  rest-server: add use tag field to RateLimitedStreams

 pbs-api-types/src/traffic_control.rs    |  9 ++++
 proxmox-http/src/rate_limited_stream.rs | 30 ++++++++++-
 proxmox-rest-server/src/connection.rs   | 16 +++++-
 proxmox-rest-server/src/rest.rs         | 72 ++++++++++++++++++++++++-
 4 files changed, 123 insertions(+), 4 deletions(-)


proxmox-backup:

Hannes Laimer (3):
  api: taffic-control: update/delete users on rule correctly
  traffic-control: handle users specified in a rule correctly
  ui: traffic-control: add users field in edit form and list

 src/api2/config/traffic_control.rs |   8 +++
 src/bin/proxmox-backup-proxy.rs    |   7 +-
 src/traffic_control_cache.rs       | 100 +++++++++++++++++++++++++----
 www/config/TrafficControlView.js   |   7 ++
 www/window/TrafficControlEdit.js   |  18 ++++++
 5 files changed, 126 insertions(+), 14 deletions(-)


Summary over all repositories:
  9 files changed, 249 insertions(+), 18 deletions(-)

-- 
Generated by git-murpp 0.8.1


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox 1/3] pbs-api-types: add users to traffic-control rule

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-api-types/src/traffic_control.rs | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/pbs-api-types/src/traffic_control.rs b/pbs-api-types/src/traffic_control.rs
index 2a359eda..12fc8c93 100644
--- a/pbs-api-types/src/traffic_control.rs
+++ b/pbs-api-types/src/traffic_control.rs
@@ -6,6 +6,7 @@ use proxmox_schema::{api, ApiType, Schema, StringSchema, Updater};
 use proxmox_schema::api_types::CIDR_SCHEMA;
 
 use crate::{DAILY_DURATION_FORMAT, PROXMOX_SAFE_ID_FORMAT, SINGLE_LINE_COMMENT_SCHEMA};
+use crate::Userid;
 
 pub const TRAFFIC_CONTROL_TIMEFRAME_SCHEMA: Schema =
     StringSchema::new("Timeframe to specify when the rule is active.")
@@ -125,6 +126,11 @@ pub struct ClientRateLimitConfig {
             },
             optional: true,
         },
+        users: {
+            type: Array,
+            items: { type: Userid },
+            optional: true,
+        },
     },
 )]
 #[derive(Clone, Serialize, Deserialize, PartialEq, Updater)]
@@ -146,6 +152,9 @@ pub struct TrafficControlRule {
     /// Enable the rule at specific times
     #[serde(skip_serializing_if = "Option::is_none")]
     pub timeframe: Option<Vec<String>>,
+    /// Rule applies to authenticated API requests of any of these users (overrides IP-only rules)
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub users: Option<Vec<Userid>>,
 }
 
 #[api(
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox 2/3] http: add user tag to rate-limited streams

[Hannes Laimer]

This handle is initialized whenever a connection in accepted, and the
user is filled in once this conenction is authenticated. This tag is
used for user specific rate-limiting.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 proxmox-http/src/rate_limited_stream.rs | 30 ++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/proxmox-http/src/rate_limited_stream.rs b/proxmox-http/src/rate_limited_stream.rs
index e9308a47..dcd167c0 100644
--- a/proxmox-http/src/rate_limited_stream.rs
+++ b/proxmox-http/src/rate_limited_stream.rs
@@ -26,6 +26,12 @@ pub struct RateLimitedStream<S> {
     write_delay: Option<Pin<Box<Sleep>>>,
     update_limiter_cb: Option<Box<RateLimiterCallback>>,
     last_limiter_update: Instant,
+    user_tag: Option<Arc<Mutex<Option<String>>>>,
+    // Since the option holding the handle is set on accept, we have to keep track of when/if the
+    // connection completes auth and the user tag was set. Without this we'd have to wait for normal
+    // update, so ~5s but auth happens also immediately after accept. Like this user rate-limits
+    // are applied as soons as the connection completes auth.
+    user_set: bool,
     stream: S,
 }
 
@@ -53,6 +59,8 @@ impl<S> RateLimitedStream<S> {
             write_delay: None,
             update_limiter_cb: None,
             last_limiter_update: Instant::now(),
+            user_tag: None,
+            user_set: false,
             stream,
         }
     }
@@ -77,13 +85,25 @@ impl<S> RateLimitedStream<S> {
             write_delay: None,
             update_limiter_cb: Some(Box::new(update_limiter_cb)),
             last_limiter_update: Instant::now(),
+            user_tag: None,
+            user_set: false,
             stream,
         }
     }
 
     fn update_limiters(&mut self) {
         if let Some(ref update_limiter_cb) = self.update_limiter_cb {
-            if self.last_limiter_update.elapsed().as_secs() >= 5 {
+            let mut force_update = false;
+            if !self.user_set {
+                let current_user = self
+                    .user_tag
+                    .as_ref()
+                    .and_then(|h| h.lock().ok().and_then(|g| g.clone()));
+                self.user_set = current_user.is_some();
+                force_update = self.user_set;
+            }
+
+            if force_update || self.last_limiter_update.elapsed().as_secs() >= 5 {
                 self.last_limiter_update = Instant::now();
                 let (read_limiter, write_limiter) = update_limiter_cb();
                 self.read_limiter = read_limiter;
@@ -99,6 +119,14 @@ impl<S> RateLimitedStream<S> {
     pub fn inner_mut(&mut self) -> &mut S {
         &mut self.stream
     }
+
+    pub fn user_tag_handle(&self) -> Option<Arc<Mutex<Option<String>>>> {
+        self.user_tag.as_ref().map(Arc::clone)
+    }
+
+    pub fn set_user_tag_handle(&mut self, handle: Arc<Mutex<Option<String>>>) {
+        self.user_tag = Some(handle);
+    }
 }
 
 fn register_traffic(limiter: &(dyn ShareableRateLimit), count: usize) -> Option<Pin<Box<Sleep>>> {
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox 3/3] rest-server: add use tag field to RateLimitedStreams

[Hannes Laimer]

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 <h.laimer@proxmox.com>
---
 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<Box<InsecureClientStream>>;
 type ClientStreamResult = Pin<Box<SslStream<InsecureClientStream>>>;
 
 #[cfg(feature = "rate-limited-stream")]
-type LookupRateLimiter = dyn Fn(std::net::SocketAddr) -> (Option<SharedRateLimit>, Option<SharedRateLimit>)
+type LookupRateLimiter = dyn Fn(
+        std::net::SocketAddr,
+        Option<String>,
+    ) -> (Option<SharedRateLimit>, Option<SharedRateLimit>)
     + 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<Mutex<Option<String>>> = 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
+            }
             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<ApiService, Error> {
+    #[cfg(not(feature = "rate-limited-stream"))]
+    pub fn api_service<T>(&self, peer: &T) -> Result<ApiService, Error>
+    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<T>(&self, peer: &T) -> Result<ApiService, Error>
+    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<std::net::SocketAddr, Error>;
 }
 
+#[cfg(feature = "rate-limited-stream")]
+pub trait PeerUser {
+    fn user_tag_handle(&self) -> Option<Arc<Mutex<Option<String>>>>;
+}
+
 // 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<Box<T>>
@@ -221,6 +242,41 @@ impl<T: PeerAddress> PeerAddress for proxmox_http::RateLimitedStream<T> {
     }
 }
 
+#[cfg(feature = "rate-limited-stream")]
+impl<T: PeerUser> PeerUser for Pin<Box<T>> {
+    fn user_tag_handle(&self) -> Option<Arc<Mutex<Option<String>>>> {
+        T::user_tag_handle(&**self)
+    }
+}
+
+#[cfg(feature = "rate-limited-stream")]
+impl<T: PeerUser> PeerUser for tokio_openssl::SslStream<T> {
+    fn user_tag_handle(&self) -> Option<Arc<Mutex<Option<String>>>> {
+        self.get_ref().user_tag_handle()
+    }
+}
+
+#[cfg(feature = "rate-limited-stream")]
+impl PeerUser for tokio::net::TcpStream {
+    fn user_tag_handle(&self) -> Option<Arc<Mutex<Option<String>>>> {
+        None
+    }
+}
+
+#[cfg(feature = "rate-limited-stream")]
+impl PeerUser for tokio::net::UnixStream {
+    fn user_tag_handle(&self) -> Option<Arc<Mutex<Option<String>>>> {
+        None
+    }
+}
+
+#[cfg(feature = "rate-limited-stream")]
+impl<T> PeerUser for proxmox_http::RateLimitedStream<T> {
+    fn user_tag_handle(&self) -> Option<Arc<Mutex<Option<String>>>> {
+        self.user_tag_handle()
+    }
+}
+
 // Helper [Service] containing the peer Address
 //
 // The lower level connection [Service] implementation on
@@ -233,6 +289,8 @@ impl<T: PeerAddress> PeerAddress for proxmox_http::RateLimitedStream<T> {
 pub struct ApiService {
     pub peer: std::net::SocketAddr,
     pub api_config: Arc<ApiConfig>,
+    #[cfg(feature = "rate-limited-stream")]
+    pub user_tag: Option<Arc<Mutex<Option<String>>>>,
 }
 
 impl ApiService {
@@ -357,6 +415,8 @@ impl Service<Request<Incoming>> 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<Request<Incoming>> for ApiService {
                 }
             }
 
+            #[cfg(feature = "rate-limited-stream")]
+            {
+                if let Some(handle) = user_tag {
+                    if let Some(ext) = response.extensions().get::<AuthStringExtension>() {
+                        let mut guard = handle.lock().unwrap();
+                        *guard = Some(ext.0.clone());
+                    }
+                }
+            }
+
             let logger = config.get_access_log();
             log_response(logger, &peer, method, &path, &response, user_agent);
             Ok(response)
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 1/3] api: taffic-control: update/delete users on rule correctly

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/config/traffic_control.rs | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/api2/config/traffic_control.rs b/src/api2/config/traffic_control.rs
index e02aa20a..49b1267e 100644
--- a/src/api2/config/traffic_control.rs
+++ b/src/api2/config/traffic_control.rs
@@ -116,6 +116,8 @@ pub enum DeletableProperty {
     Comment,
     /// Delete the timeframe property
     Timeframe,
+    /// Delete the users property
+    Users,
 }
 
 // fixme: use  TrafficControlUpdater
@@ -187,6 +189,9 @@ pub fn update_traffic_control(
                 DeletableProperty::Timeframe => {
                     data.timeframe = None;
                 }
+                DeletableProperty::Users => {
+                    data.users = None;
+                }
             }
         }
     }
@@ -222,6 +227,9 @@ pub fn update_traffic_control(
     if update.timeframe.is_some() {
         data.timeframe = update.timeframe;
     }
+    if update.users.is_some() {
+        data.users = update.users;
+    }
 
     config.set_data(&name, "rule", &data)?;
 
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 2/3] traffic-control: handle users specified in a rule correctly

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/bin/proxmox-backup-proxy.rs |   7 ++-
 src/traffic_control_cache.rs    | 100 +++++++++++++++++++++++++++-----
 2 files changed, 93 insertions(+), 14 deletions(-)

diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index cfd93f92..c39e4587 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -955,6 +955,7 @@ async fn run_traffic_control_updater() {
 
 fn lookup_rate_limiter(
     peer: std::net::SocketAddr,
+    user: Option<String>,
 ) -> (Option<SharedRateLimit>, Option<SharedRateLimit>) {
     let mut cache = TRAFFIC_CONTROL_CACHE.lock().unwrap();
 
@@ -962,7 +963,11 @@ fn lookup_rate_limiter(
 
     cache.reload(now);
 
-    let (_rule_name, read_limiter, write_limiter) = cache.lookup_rate_limiter(peer, now);
+    let authid = user.and_then(|s| s.parse::<pbs_api_types::Authid>().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<SharedRateLimit>, Option<SharedRateLimit>) {
         let peer_ip = canonical_ip(peer.ip());
 
-        log::debug!("lookup_rate_limiter: {:?}", peer_ip);
+        log::debug!(
+            "lookup_rate_limiter: {:?} - {}",
+            peer_ip,
+            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)
 
         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(())
+    }
 }
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup 3/3] ui: traffic-control: add users field in edit form and list

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/config/TrafficControlView.js |  7 +++++++
 www/window/TrafficControlEdit.js | 18 ++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/www/config/TrafficControlView.js b/www/config/TrafficControlView.js
index 0b22d29a..5cfec82b 100644
--- a/www/config/TrafficControlView.js
+++ b/www/config/TrafficControlView.js
@@ -181,6 +181,13 @@ Ext.define('PBS.config.TrafficControlView', {
             renderer: 'render_bandwidth',
             dataIndex: 'burst-out',
         },
+        {
+            header: gettext('Users'),
+            flex: 3,
+            sortable: true,
+            renderer: (users) => (users ? Ext.String.htmlEncode(users.join(', ')) : ''),
+            dataIndex: 'users',
+        },
         {
             header: gettext('Networks'),
             flex: 3,
diff --git a/www/window/TrafficControlEdit.js b/www/window/TrafficControlEdit.js
index 0bbbf363..2063c107 100644
--- a/www/window/TrafficControlEdit.js
+++ b/www/window/TrafficControlEdit.js
@@ -215,6 +215,7 @@ Ext.define('PBS.window.TrafficControlEdit', {
                 PBS.Utils.delete_if_default(values, 'rate-out');
                 PBS.Utils.delete_if_default(values, 'burst-in');
                 PBS.Utils.delete_if_default(values, 'burst-out');
+                PBS.Utils.delete_if_default(values, 'users');
                 if (typeof values.delete === 'string') {
                     values.delete = values.delete.split(',');
                 }
@@ -276,6 +277,23 @@ Ext.define('PBS.window.TrafficControlEdit', {
         ],
 
         columnB: [
+            {
+                xtype: 'pmxUserSelector',
+                fieldLabel: gettext('Users'),
+                name: 'users',
+                multiSelect: true,
+                allowBlank: true,
+                cbind: {
+                    deleteEmpty: '{!isCreate}',
+                },
+                emptyText: gettext('Applies to all users'),
+                autoEl: {
+                    tag: 'div',
+                    'data-qtip': gettext(
+                        'Limit applies only to authenticated requests by these users. Overrides IP-only rules when both match. If networks are specified on this rule as well, it\'ll only apply if the users request comes from one of the specified networks.',
+                    ),
+                },
+            },
             {
                 xtype: 'proxmoxtextfield',
                 fieldLabel: gettext('Network(s)'),
-- 
2.47.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox{, -backup} 0/6] add user specific rate-limits

[Hannes Laimer]

ping

On 9/9/25 10:53, Hannes Laimer wrote:
> This adds support for specifying user specific rate-limits.
> We add a user-tag to every rate-limited connection, with this present we
> can limit the connection based on the authenticated user assiciated with
> it.
> 
> Authentication happens after accept, so we can't set this right when we
> accept a connection. Currently we initialize the handle on accept, we
> then give this handle to the rate_limiter callback function. And on
> completed authentication we set the user using this handle.
> I did consider using a Peer -> User map in the cache, and just adding
> entries on auth, but there isn't really a good way to clean those
> entries. And peers(so IP:port) may end up being reused, and that would
> be a problem. With the current approach we don't have this problem.
> 
> Currently rules with a user specified take priority over others. So:
> user > IP only > neither, in case two rules match.
> 
> If users and networks are specified, the rule only applies if both
> match. So, Any of the specified user connect from any of the specified
> network.
> 
> And all of this ofc still only if the given timeframe matches.
> 
> Note: this is only for users, you can't specify individual tokens. But I
> don't think that is much of a problem, it is probably even better like
> this.
> 
> (I did look through BZ if there is an issue for this, I feel like there
> should be, but did not find one)
> 
> proxmox:
> 
> Hannes Laimer (3):
>    pbs-api-types: add users to traffic-control rule
>    http: add user tag to rate-limited streams
>    rest-server: add use tag field to RateLimitedStreams
> 
>   pbs-api-types/src/traffic_control.rs    |  9 ++++
>   proxmox-http/src/rate_limited_stream.rs | 30 ++++++++++-
>   proxmox-rest-server/src/connection.rs   | 16 +++++-
>   proxmox-rest-server/src/rest.rs         | 72 ++++++++++++++++++++++++-
>   4 files changed, 123 insertions(+), 4 deletions(-)
> 
> 
> proxmox-backup:
> 
> Hannes Laimer (3):
>    api: taffic-control: update/delete users on rule correctly
>    traffic-control: handle users specified in a rule correctly
>    ui: traffic-control: add users field in edit form and list
> 
>   src/api2/config/traffic_control.rs |   8 +++
>   src/bin/proxmox-backup-proxy.rs    |   7 +-
>   src/traffic_control_cache.rs       | 100 +++++++++++++++++++++++++----
>   www/config/TrafficControlView.js   |   7 ++
>   www/window/TrafficControlEdit.js   |  18 ++++++
>   5 files changed, 126 insertions(+), 14 deletions(-)
> 
> 
> Summary over all repositories:
>    9 files changed, 249 insertions(+), 18 deletions(-)
> 



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox{, -backup} 0/6] add user specific rate-limits

[Christian Ebner]

On 9/9/25 10:53 AM, Hannes Laimer wrote:
> This adds support for specifying user specific rate-limits.
> We add a user-tag to every rate-limited connection, with this present we
> can limit the connection based on the authenticated user assiciated with
> it.
> 
> Authentication happens after accept, so we can't set this right when we
> accept a connection. Currently we initialize the handle on accept, we
> then give this handle to the rate_limiter callback function. And on
> completed authentication we set the user using this handle.
> I did consider using a Peer -> User map in the cache, and just adding
> entries on auth, but there isn't really a good way to clean those
> entries. And peers(so IP:port) may end up being reused, and that would
> be a problem. With the current approach we don't have this problem.
> 
> Currently rules with a user specified take priority over others. So:
> user > IP only > neither, in case two rules match.
> 
> If users and networks are specified, the rule only applies if both
> match. So, Any of the specified user connect from any of the specified
> network.
> 
> And all of this ofc still only if the given timeframe matches.
> 
> Note: this is only for users, you can't specify individual tokens. But I
> don't think that is much of a problem, it is probably even better like
> this.
> 
> (I did look through BZ if there is an issue for this, I feel like there
> should be, but did not find one)

Hi,
thanks for the patches, this is a very useful feature I think.

I've planned to have a more in-depth look at this series today, but from 
a first glance I see two possible issues which I think need to be addressed:

- The rate limiting happens on the RateLimitedStreams by token bucket 
filtering, this however being agnostic to the traffic flowing above that 
connection. And user authentication happens at request level. So while 
probably not very problematic in general since there will be a dedicated 
connection for different users, the same connection (TCP socket) could 
be shared by multiple users, the connection is however tagged by the 
first users after the first request being authenticated unless I'm 
missing something. So a second user reusing the same TCP connection will 
then get the limits of the first one?
- Tagging of the stream only happens *after* the first request being 
processed and the response being generated. This however means that this 
first request will never be limited, only subsequent requests are.
- It would probably make sense to keep the stream part as generic as 
possible, the stream should not be concerned about users. So maybe it 
would make sense to allow to set generic `tags` on connections, and pass 
this list of tags to the rate limiter callback, so it can determine the 
lowest rate limits compatible with the given tags from the ruleset. This 
would still apply the same limits to users reusing the same connection, 
but in a more abstract fashion.

What do you think?


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox{, -backup} 0/6] add user specific rate-limits

[Hannes Laimer]

On 11/7/25 08:33, Christian Ebner wrote:
> On 9/9/25 10:53 AM, Hannes Laimer wrote:
>> This adds support for specifying user specific rate-limits.
>> We add a user-tag to every rate-limited connection, with this present we
>> can limit the connection based on the authenticated user assiciated with
>> it.
>>
>> Authentication happens after accept, so we can't set this right when we
>> accept a connection. Currently we initialize the handle on accept, we
>> then give this handle to the rate_limiter callback function. And on
>> completed authentication we set the user using this handle.
>> I did consider using a Peer -> User map in the cache, and just adding
>> entries on auth, but there isn't really a good way to clean those
>> entries. And peers(so IP:port) may end up being reused, and that would
>> be a problem. With the current approach we don't have this problem.
>>
>> Currently rules with a user specified take priority over others. So:
>> user > IP only > neither, in case two rules match.
>>
>> If users and networks are specified, the rule only applies if both
>> match. So, Any of the specified user connect from any of the specified
>> network.
>>
>> And all of this ofc still only if the given timeframe matches.
>>
>> Note: this is only for users, you can't specify individual tokens. But I
>> don't think that is much of a problem, it is probably even better like
>> this.
>>
>> (I did look through BZ if there is an issue for this, I feel like there
>> should be, but did not find one)
> 
> Hi,
> thanks for the patches, this is a very useful feature I think.
> 
> I've planned to have a more in-depth look at this series today, but from 
> a first glance I see two possible issues which I think need to be 
> addressed:
> 

Thanks for taking a look!

> - The rate limiting happens on the RateLimitedStreams by token bucket 
> filtering, this however being agnostic to the traffic flowing above that 
> connection. And user authentication happens at request level. So while 
> probably not very problematic in general since there will be a dedicated 
> connection for different users, the same connection (TCP socket) could 
> be shared by multiple users, the connection is however tagged by the 
> first users after the first request being authenticated unless I'm 
> missing something. So a second user reusing the same TCP connection will 
> then get the limits of the first one?

Is that actually possible? I assumed new auth implies a new
connection(so the thing we tagged here). So a user could connect to the
PBS and not go through .accept() by the server?
Or do you mean after one connection is done a later one could end up
reusing the same port? In that case it would have to be accepted first
and go through auth again, no?

> - Tagging of the stream only happens *after* the first request being 
> processed and the response being generated. This however means that this 
> first request will never be limited, only subsequent requests are.

well, we can't before auth, we don't know who it is we're talking to,
no?

> - It would probably make sense to keep the stream part as generic as 
> possible, the stream should not be concerned about users. So maybe it 
> would make sense to allow to set generic `tags` on connections, and pass 
> this list of tags to the rate limiter callback, so it can determine the 
> lowest rate limits compatible with the given tags from the ruleset. This 
> would still apply the same limits to users reusing the same connection, 
> but in a more abstract fashion.

I did think about that, but I couldn't really come up with much of a
usecase. User is the only one I could think of that can't be done on
accept but has to be done after auth. But there may very well be some,
I just couldn't really think of any.

> 
> What do you think?



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox{, -backup} 0/6] add user specific rate-limits

[Christian Ebner]

On 11/7/25 8:44 AM, Hannes Laimer wrote:
> On 11/7/25 08:33, Christian Ebner wrote:
>> On 9/9/25 10:53 AM, Hannes Laimer wrote:
>>> This adds support for specifying user specific rate-limits.
>>> We add a user-tag to every rate-limited connection, with this present we
>>> can limit the connection based on the authenticated user assiciated with
>>> it.
>>>
>>> Authentication happens after accept, so we can't set this right when we
>>> accept a connection. Currently we initialize the handle on accept, we
>>> then give this handle to the rate_limiter callback function. And on
>>> completed authentication we set the user using this handle.
>>> I did consider using a Peer -> User map in the cache, and just adding
>>> entries on auth, but there isn't really a good way to clean those
>>> entries. And peers(so IP:port) may end up being reused, and that would
>>> be a problem. With the current approach we don't have this problem.
>>>
>>> Currently rules with a user specified take priority over others. So:
>>> user > IP only > neither, in case two rules match.
>>>
>>> If users and networks are specified, the rule only applies if both
>>> match. So, Any of the specified user connect from any of the specified
>>> network.
>>>
>>> And all of this ofc still only if the given timeframe matches.
>>>
>>> Note: this is only for users, you can't specify individual tokens. But I
>>> don't think that is much of a problem, it is probably even better like
>>> this.
>>>
>>> (I did look through BZ if there is an issue for this, I feel like there
>>> should be, but did not find one)
>>
>> Hi,
>> thanks for the patches, this is a very useful feature I think.
>>
>> I've planned to have a more in-depth look at this series today, but 
>> from a first glance I see two possible issues which I think need to be 
>> addressed:
>>
> 
> Thanks for taking a look!
> 
>> - The rate limiting happens on the RateLimitedStreams by token bucket 
>> filtering, this however being agnostic to the traffic flowing above 
>> that connection. And user authentication happens at request level. So 
>> while probably not very problematic in general since there will be a 
>> dedicated connection for different users, the same connection (TCP 
>> socket) could be shared by multiple users, the connection is however 
>> tagged by the first users after the first request being authenticated 
>> unless I'm missing something. So a second user reusing the same TCP 
>> connection will then get the limits of the first one?
> 
> Is that actually possible? I assumed new auth implies a new
> connection(so the thing we tagged here). So a user could connect to the
> PBS and not go through .accept() by the server?

As a client, I could be able to open a new TCP connection to the REST 
server via a connect() (accepted by the server via an accept()), perform 
the TLS handshake and then I have the connection via the socket. But 
using this socket, I can now send multiple HTTP requests with user auth 
from different users? This connection remains open for longer than just 
the single request, as otherwise your tagging after the first request 
would not work either?

> Or do you mean after one connection is done a later one could end up
> reusing the same port? In that case it would have to be accepted first
> and go through auth again, no?

What I mean is once the client established a connection via the socket, 
it can send multiple subsequent requests trough that socket, the socket 
being tagged by the first request being authenticated. Nothing forces 
the client to send the next request with the same credentials?

> 
>> - Tagging of the stream only happens *after* the first request being 
>> processed and the response being generated. This however means that 
>> this first request will never be limited, only subsequent requests are.
> 
> well, we can't before auth, we don't know who it is we're talking to,
> no?

Yes, but that's the point, it would require to see if one can already 
set the tag on the socket/stream right after request auth, not after 
actually processing the full request and use the information after 
response generation.

> 
>> - It would probably make sense to keep the stream part as generic as 
>> possible, the stream should not be concerned about users. So maybe it 
>> would make sense to allow to set generic `tags` on connections, and 
>> pass this list of tags to the rate limiter callback, so it can 
>> determine the lowest rate limits compatible with the given tags from 
>> the ruleset. This would still apply the same limits to users reusing 
>> the same connection, but in a more abstract fashion.
> 
> I did think about that, but I couldn't really come up with much of a
> usecase. User is the only one I could think of that can't be done on
> accept but has to be done after auth. But there may very well be some,
> I just couldn't really think of any.

Well, the point I'm trying to make here is that the rate limited stream 
should not be aware of the concept of a user, that is none of it's 
concern as that happens in higher levels of the OSI layers.

Therefore the suggestion to keep this as generic as possible. Tags could 
then be anything, not necessary related to users, although that is our 
usecase here.

So for the time being this probably only requires a bit of variable 
renaming, e.g. `RateLimitedStream::user_tag` to `RateLimitedStream::tag` 
and define that as e.g enum with variant `Tag::String(String)` and 
`Tag::Untagged` and even replace the `RateLimitedStream::user_set` since 
that can now be encoded by the `Tag::Untagged` variant?

This could then be extended to have different variants of tags and to 
allow multiple tags on the same TcpLimitedStream if ever required.

> 
>>
>> What do you think?
> 



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox{, -backup} 0/6] add user specific rate-limits

[Hannes Laimer]

On 11/7/25 09:16, Christian Ebner wrote:
> On 11/7/25 8:44 AM, Hannes Laimer wrote:
>> On 11/7/25 08:33, Christian Ebner wrote:
>>> On 9/9/25 10:53 AM, Hannes Laimer wrote:
>>>> This adds support for specifying user specific rate-limits.
>>>> We add a user-tag to every rate-limited connection, with this 
>>>> present we
>>>> can limit the connection based on the authenticated user assiciated 
>>>> with
>>>> it.
>>>>
>>>> Authentication happens after accept, so we can't set this right when we
>>>> accept a connection. Currently we initialize the handle on accept, we
>>>> then give this handle to the rate_limiter callback function. And on
>>>> completed authentication we set the user using this handle.
>>>> I did consider using a Peer -> User map in the cache, and just adding
>>>> entries on auth, but there isn't really a good way to clean those
>>>> entries. And peers(so IP:port) may end up being reused, and that would
>>>> be a problem. With the current approach we don't have this problem.
>>>>
>>>> Currently rules with a user specified take priority over others. So:
>>>> user > IP only > neither, in case two rules match.
>>>>
>>>> If users and networks are specified, the rule only applies if both
>>>> match. So, Any of the specified user connect from any of the specified
>>>> network.
>>>>
>>>> And all of this ofc still only if the given timeframe matches.
>>>>
>>>> Note: this is only for users, you can't specify individual tokens. 
>>>> But I
>>>> don't think that is much of a problem, it is probably even better like
>>>> this.
>>>>
>>>> (I did look through BZ if there is an issue for this, I feel like there
>>>> should be, but did not find one)
>>>
>>> Hi,
>>> thanks for the patches, this is a very useful feature I think.
>>>
>>> I've planned to have a more in-depth look at this series today, but 
>>> from a first glance I see two possible issues which I think need to 
>>> be addressed:
>>>
>>
>> Thanks for taking a look!
>>
>>> - The rate limiting happens on the RateLimitedStreams by token bucket 
>>> filtering, this however being agnostic to the traffic flowing above 
>>> that connection. And user authentication happens at request level. So 
>>> while probably not very problematic in general since there will be a 
>>> dedicated connection for different users, the same connection (TCP 
>>> socket) could be shared by multiple users, the connection is however 
>>> tagged by the first users after the first request being authenticated 
>>> unless I'm missing something. So a second user reusing the same TCP 
>>> connection will then get the limits of the first one?
>>
>> Is that actually possible? I assumed new auth implies a new
>> connection(so the thing we tagged here). So a user could connect to the
>> PBS and not go through .accept() by the server?
> 
> As a client, I could be able to open a new TCP connection to the REST 
> server via a connect() (accepted by the server via an accept()), perform 
> the TLS handshake and then I have the connection via the socket. But 
> using this socket, I can now send multiple HTTP requests with user auth 
> from different users? This connection remains open for longer than just 
> the single request, as otherwise your tagging after the first request 
> would not work either?
> 
>> Or do you mean after one connection is done a later one could end up
>> reusing the same port? In that case it would have to be accepted first
>> and go through auth again, no?
> 
> What I mean is once the client established a connection via the socket, 
> it can send multiple subsequent requests trough that socket, the socket 
> being tagged by the first request being authenticated. Nothing forces 
> the client to send the next request with the same credentials?
> 
>>
>>> - Tagging of the stream only happens *after* the first request being 
>>> processed and the response being generated. This however means that 
>>> this first request will never be limited, only subsequent requests are.
>>
>> well, we can't before auth, we don't know who it is we're talking to,
>> no?
> 
> Yes, but that's the point, it would require to see if one can already 
> set the tag on the socket/stream right after request auth, not after 
> actually processing the full request and use the information after 
> response generation.
> 
>>
>>> - It would probably make sense to keep the stream part as generic as 
>>> possible, the stream should not be concerned about users. So maybe it 
>>> would make sense to allow to set generic `tags` on connections, and 
>>> pass this list of tags to the rate limiter callback, so it can 
>>> determine the lowest rate limits compatible with the given tags from 
>>> the ruleset. This would still apply the same limits to users reusing 
>>> the same connection, but in a more abstract fashion.
>>
>> I did think about that, but I couldn't really come up with much of a
>> usecase. User is the only one I could think of that can't be done on
>> accept but has to be done after auth. But there may very well be some,
>> I just couldn't really think of any.
> 
> Well, the point I'm trying to make here is that the rate limited stream 
> should not be aware of the concept of a user, that is none of it's 
> concern as that happens in higher levels of the OSI layers.
> 
> Therefore the suggestion to keep this as generic as possible. Tags could 
> then be anything, not necessary related to users, although that is our 
> usecase here.
> 
> So for the time being this probably only requires a bit of variable 
> renaming, e.g. `RateLimitedStream::user_tag` to `RateLimitedStream::tag` 
> and define that as e.g enum with variant `Tag::String(String)` and 
> `Tag::Untagged` and even replace the `RateLimitedStream::user_set` since 
> that can now be encoded by the `Tag::Untagged` variant?
> 
> This could then be extended to have different variants of tags and to 
> allow multiple tags on the same TcpLimitedStream if ever required.
> 
>>
>>>
>>> What do you think?
>>
>

aah I see, sorry for the confusion and thanks for the clarification!
I'll send a v2!



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel