From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 14805E227 for ; Tue, 18 Jul 2023 07:47:01 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id EC4B015EBB for ; Tue, 18 Jul 2023 07:46:30 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 18 Jul 2023 07:46:29 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id A32D742F35 for ; Tue, 18 Jul 2023 07:46:29 +0200 (CEST) Message-ID: <0f5792c2-282f-ea1a-0fd3-2fd1b7c5c6ad@proxmox.com> Date: Tue, 18 Jul 2023 07:46:28 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Content-Language: en-US To: Wolfgang Bumiller Cc: pbs-devel@lists.proxmox.com References: <20230622091526.812422-1-m.carrara@proxmox.com> <20230622091526.812422-2-m.carrara@proxmox.com> From: Max Carrara In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.038 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 NICE_REPLY_A -0.097 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [connection.rs] Subject: Re: [pbs-devel] [PATCH proxmox 1/3] rest-server: Add `BiAcceptBuilder` 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: , X-List-Received-Date: Tue, 18 Jul 2023 05:47:01 -0000 On 7/14/23 11:20, Wolfgang Bumiller wrote: > On Thu, Jun 22, 2023 at 11:15:24AM +0200, Max Carrara wrote: >> This builder is similar to `AcceptBuilder`, but is also able to differ >> between plain TCP streams and TCP streams running TLS. >> >> It does so by peeking into the stream's buffer and checking whether >> the client is initiating a TLS handshake. >> >> Newly accepted plain TCP streams are sent along via a separate channel >> in order to clearly distinguish between "secure" and "insecure" >> connections. >> >> Signed-off-by: Max Carrara >> --- >> proxmox-rest-server/src/connection.rs | 327 ++++++++++++++++++++++++++ >> 1 file changed, 327 insertions(+) >> >> diff --git a/proxmox-rest-server/src/connection.rs b/proxmox-rest-server/src/connection.rs >> index 7681f00..937b5d7 100644 >> --- a/proxmox-rest-server/src/connection.rs >> +++ b/proxmox-rest-server/src/connection.rs >> @@ -302,3 +302,330 @@ impl AcceptBuilder { >> } >> } >> } >> + >> +#[cfg(feature = "rate-limited-stream")] >> +type InsecureClientStreamResult = Pin>>; >> +#[cfg(not(feature = "rate-limited-stream"))] >> +type InsecureClientStreamResult = Pin>; > > ^ You can drop one set of `#[cfg]`s by using `Pin>` ;-) > >> + >> +#[cfg(feature = "rate-limited-stream")] >> +type ClientStream = RateLimitedStream; >> + >> +#[cfg(not(feature = "rate-limited-stream"))] >> +type ClientStream = TcpStream; >> + >> +pub struct BiAcceptBuilder { >> + acceptor: Option>>, >> + debug: bool, >> + tcp_keepalive_time: u32, >> + max_pending_accepts: usize, >> + >> + #[cfg(feature = "rate-limited-stream")] >> + lookup_rate_limiter: Option>, >> +} > > 90% of this whole thing is a copy of `AcceptBuilder`. > I'd argue that we should be able to instead add this version's > `accept()` method to the regular `AcceptBuilder` as another variant with > a different name, eg. `accept_with_tls_optional()`. > > The `accept_connections()` task AFAICT is also just the original split > in 2 with the tls check in between. It should be fine to just change the > original to this with the tls check covered by whether an > `Option>` is `Some`. > > Otherwise we're just duplicating too much. > I had realized this while writing this series; I decided to go for a separate type because I wasn't sure whether I'd be breaking an API or not (but as you had already mentioned off list, this isn't the case here). > The only other change is that the tls acceptor is now optional. > Do we even have a use case for where we need potentially-rate-limited > non-tls streams? > If so, this could also be another accept method. > > In fact, given the point where the acceptor is actually used, perhaps we > should drop it from the struct entirely and instead pass it along to the > `accept()` methods: > - accept_tls(acceptor) -> stream > - accept_optional_tls(acceptor) -> (tls stream, insecure stream) > - accept_direct(acceptor) -> nontls-stream > > ^ dropping the original `accept` on purpose ensure all crate users get > updated accordingly Good point actually! This seems like the right way to go.