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

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

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 <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.
> 

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.