Re: [pbs-devel] [PATCH proxmox 1/3] rest-server: Add `BiAcceptBuilder`

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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 <m.carrara@proxmox.com>
> ---
>  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<Box<RateLimitedStream<TcpStream>>>;
> +#[cfg(not(feature = "rate-limited-stream"))]
> +type InsecureClientStreamResult = Pin<Box<TcpStream>>;

^ You can drop one set of `#[cfg]`s by using `Pin<Box<ClientStream>>` ;-)

> +
> +#[cfg(feature = "rate-limited-stream")]
> +type ClientStream = RateLimitedStream<TcpStream>;
> +
> +#[cfg(not(feature = "rate-limited-stream"))]
> +type ClientStream = TcpStream;
> +
> +pub struct BiAcceptBuilder {
> +    acceptor: Option<Arc<Mutex<SslAcceptor>>>,
> +    debug: bool,
> +    tcp_keepalive_time: u32,
> +    max_pending_accepts: usize,
> +
> +    #[cfg(feature = "rate-limited-stream")]
> +    lookup_rate_limiter: Option<Arc<LookupRateLimiter>>,
> +}

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<Sender<Insecure...>>` is `Some`.

Otherwise we're just duplicating too much.

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