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 A24AC1FF16B for ; Fri, 7 Nov 2025 08:33:44 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id F323C8A34; Fri, 7 Nov 2025 08:34:25 +0100 (CET) Message-ID: <862824b0-6308-43bc-8304-ed93bf186356@proxmox.com> Date: Fri, 7 Nov 2025 08:33:53 +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> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20250909085245.91641-1-h.laimer@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1762500813798 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.047 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} 0/6] add user specific rate-limits 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" 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