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)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id A8142BBCFF for ; Wed, 27 Mar 2024 12:44:57 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7F1E4315B5 for ; Wed, 27 Mar 2024 12:44:57 +0100 (CET) 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 ; Wed, 27 Mar 2024 12:44:55 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id ADD1C425A1 for ; Wed, 27 Mar 2024 12:44:55 +0100 (CET) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 27 Mar 2024 12:44:54 +0100 Message-Id: To: "Proxmox Backup Server development discussion" From: "Max Carrara" X-Mailer: aerc 0.17.0-72-g6a84f1331f1c References: <20240326152818.639452-1-m.sandoval@proxmox.com> In-Reply-To: <20240326152818.639452-1-m.sandoval@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.021 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 1/2] http: teach the Client how to speak gzip 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: Wed, 27 Mar 2024 11:44:57 -0000 On Tue Mar 26, 2024 at 4:28 PM CET, Maximiliano Sandoval wrote: > Proxmox VE already supports using gzip to encode and decode the body of > http requests. We teach the proxmox-http how to do the same. I like the general idea of this, but there are a few things I would like you to reconsider: 1. Your patches only affect the async client (`simple.rs`), but not the synchronous version (`sync.rs`) 2. The compression library you're using is fundamentally blocking (as in, doesn't provide an `async` interface) * While that probably doesn't pose much of a problem if the response's content is rather small, decompressing larger response bodies might cause the underlying executor to block unexpectedly 3. Your patches assume that the client *always* wants to accept compressed content, which is something you don't necessarily want. In particular, there was a compression side-channel attack on HTTPS that exploited compression being always on [0] that you should check out. I've also another example that might explain it a litte better [1]. This isn't to say that I'm fundamentally againt implementing better support for HTTP compression - in fact, I'm glad someone's tackling this! But I do want to encourage you to consider where you want compression and where you don't. Also, even if the difference seems negligible, you should refrain from calling blocking code when doing IO like that. So, IMO, some good opportunities arise here: 1. You could actually check where / when our server compresses data - it shouldn't e.g. compress sensitive information over HTTPS (if you read the two pages I referred to above). 2. You should check out `proxmox-compression` and see if there are any async wrappers for the compression algorithms you added. Should there be any, you can just use them obviously - if there are none, you should make some yourself (IMO). 3. This is a somewhat minor point, but since not everything can or should be compressed, maybe you can make support for compression composable somehow? As in, let the caller of the API decide when compression is sensible and when it isn't. So, I'm curious to see what you'll come up with! :) Apart from the above, there's not really else for me to mention - except that I'm always happy to see more testing being done. Very dank. FWIW, if you really want to test a couple complete request-response round trips with multiple different scenarios regarding compression, you can always spin up an executor with its own HTTP server in a separate thread, even in tests. Should your tests require any additional dependencies that are not used for building the crate, you can of course always tell cargo [2]. [0]: https://www.breachattack.com/ [1]: https://security.stackexchange.com/a/20407 [2]: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html= #development-dependencies