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 BAB2E1FF16B for ; Fri, 7 Nov 2025 09:30:26 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4F9FB9C43; Fri, 7 Nov 2025 09:31:08 +0100 (CET) Message-ID: <693a32d2-1b3f-4c67-91f6-46b27c2669d1@proxmox.com> Date: Fri, 7 Nov 2025 09:30:32 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Christian Ebner , Proxmox Backup Server development discussion References: <20250909085245.91641-1-h.laimer@proxmox.com> <862824b0-6308-43bc-8304-ed93bf186356@proxmox.com> Content-Language: en-US From: Hannes Laimer In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1762504212778 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.040 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{, -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 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