Re: [pbs-devel] [PATCH proxmox 1/2] http: teach the Client how to speak gzip

["Max Carrara" <m.carrara@proxmox.com>]

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