[pbs-devel] [PATCH v2 proxmox 3/3] fix #5105: rest-server: connection: overhaul TLS handshake check logic
Wolfgang Bumiller
w.bumiller at proxmox.com
Tue Jul 9 12:29:14 CEST 2024
On Mon, Jul 08, 2024 at 06:48:17PM GMT, Max Carrara wrote:
> On rare occasions, the TLS "client hello" message [1] is delayed after
> a connection with the server was established, which causes HTTPS
> requests to fail before TLS was even negotiated. In these cases, the
> server would incorrectly respond with "HTTP/1.1 400 Bad Request"
> instead of closing the connection (or similar).
>
> The reasons for the "client hello" being delayed seem to vary; one
> user noticed that the issue went away completely after they turned off
> UFW [2]. Another user noticed (during private correspondence) that the
> issue only appeared when connecting to their PBS instance via WAN, but
> not from within their VPN. In the WAN case a firewall was also
> present. The same user kindly provided tcpdumps and strace logs on
> request.
>
> The issue was finally reproduced with the following Python script:
>
> import socket
> import time
>
> HOST: str = ...
> PORT: int = ...
>
> with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as sock:
> sock.connect((HOST, PORT))
> time.sleep(1.5) # simulate firewall / proxy / etc. delay
> sock.sendall(b"\x16\x03\x01\x02\x00")
> data = sock.recv(256)
> print(data)
>
> The additional delay before sending the first 5 bytes of the "client
> hello" message causes the handshake checking logic to incorrectly fall
> back to plain HTTP.
>
> All of this is fixed by the following:
>
> 1. Increase the timeout duration to 10 seconds (from 1)
> 2. Instead of falling back to plain HTTP, refuse to accept the
> connection if the TLS handshake wasn't initiated before the
> timeout limit is reached
> 3. Only accept plain HTTP if the first 5 bytes do not correspond to
> a TLS handshake fragment [3]
> 4. Do not take the last number of bytes that were in the buffer into
> account; instead, only perform the actual handshake check if
> 5 bytes are in the peek buffer using some of tokio's low-level
> functionality
>
> Regarding 1.: This should be generous enough for any client to be able
> to initiate a TLS handshake, despite its surrounding circumstances.
>
> Regarding 4.: While this is not 100% related to the issue, peeking into
> the buffer in this manner should ensure that our implementation here
> remains correct, even if the kernel's underlying behaviour regarding
> edge-triggering is changed [4]. At the same time, there's no need for
> busy-waiting and continuously yielding to the event loop anymore.
>
> [1]: https://www.rfc-editor.org/rfc/rfc8446.html#section-4.1.2
> [2]: https://forum.proxmox.com/threads/disable-default-http-redirects-on-8007.142312/post-675352
> [3]: https://www.rfc-editor.org/rfc/rfc8446.html#section-5.1
> [4]: https://lwn.net/Articles/864947/
>
> Signed-off-by: Max Carrara <m.carrara at proxmox.com>
> ---
> Changes v1 --> v2:
> * use `socket.async_io` instead of `socket.peek` and rework the future
> that performs the handshake check
> * adapt commit message
> * fix reference in commit message
>
> proxmox-rest-server/src/connection.rs | 110 ++++++++++++++------------
> 1 file changed, 61 insertions(+), 49 deletions(-)
>
> diff --git a/proxmox-rest-server/src/connection.rs b/proxmox-rest-server/src/connection.rs
> index 470021d7..63fa8640 100644
> --- a/proxmox-rest-server/src/connection.rs
> +++ b/proxmox-rest-server/src/connection.rs
> @@ -2,7 +2,9 @@
> //!
> //! Hyper building block.
>
> +use std::io;
> use std::net::SocketAddr;
> +use std::os::fd::FromRawFd;
> use std::os::unix::io::AsRawFd;
> use std::path::PathBuf;
> use std::pin::Pin;
> @@ -418,70 +420,80 @@ impl AcceptBuilder {
> secure_sender: ClientSender,
> insecure_sender: InsecureClientSender,
> ) {
> + const CLIENT_HANDSHAKE_TIMEOUT: Duration = Duration::from_secs(10);
> +
> let peer = state.peer;
>
> - let client_initiates_handshake = {
> - #[cfg(feature = "rate-limited-stream")]
> - let socket_ref = state.socket.inner();
> + #[cfg(feature = "rate-limited-stream")]
> + let socket_ref = state.socket.inner();
>
> - #[cfg(not(feature = "rate-limited-stream"))]
> - let socket_ref = &state.socket;
> + #[cfg(not(feature = "rate-limited-stream"))]
> + let socket_ref = &state.socket;
>
> - match Self::wait_for_client_tls_handshake(socket_ref).await {
> - Ok(initiates_handshake) => initiates_handshake,
> - Err(err) => {
> - log::error!("[{peer}] error checking for TLS handshake: {err}");
> - return;
> - }
> - }
> - };
> -
> - if !client_initiates_handshake {
> - let insecure_stream = Box::pin(state.socket);
> + let handshake_res =
> + Self::wait_for_client_tls_handshake(socket_ref, CLIENT_HANDSHAKE_TIMEOUT).await;
>
> - if insecure_sender.send(Ok(insecure_stream)).await.is_err() && flags.is_debug {
> - log::error!("[{peer}] detected closed connection channel")
> + match handshake_res {
> + Ok(true) => {
> + Self::do_accept_tls(state, flags, secure_sender).await;
> }
> + Ok(false) => {
> + let insecure_stream = Box::pin(state.socket);
>
> - return;
> + if let Err(send_err) = insecure_sender.send(Ok(insecure_stream)).await {
> + log::error!("[{peer}] failed to accept connection - connection channel closed: {send_err}");
> + }
> + }
> + Err(err) => {
> + log::error!("[{peer}] failed to check for TLS handshake: {err}");
> + }
> }
> -
> - Self::do_accept_tls(state, flags, secure_sender).await
> }
>
> - async fn wait_for_client_tls_handshake(incoming_stream: &TcpStream) -> Result<bool, Error> {
> - const MS_TIMEOUT: u64 = 1000;
> - const BYTES_BUF_SIZE: usize = 128;
> -
> - let mut buf = [0; BYTES_BUF_SIZE];
> - let mut last_peek_size = 0;
> + async fn wait_for_client_tls_handshake(
> + incoming_stream: &TcpStream,
> + timeout: Duration,
> + ) -> Result<bool, Error> {
> + const HANDSHAKE_BYTES_LEN: usize = 5;
>
> let future = async {
> - loop {
> - let peek_size = incoming_stream
> - .peek(&mut buf)
> - .await
> - .context("couldn't peek into incoming tcp stream")?;
> -
> - if contains_tls_handshake_fragment(&buf) {
> - return Ok(true);
> - }
> -
> - // No more new data came in
> - if peek_size == last_peek_size {
> - return Ok(false);
> - }
> -
> - last_peek_size = peek_size;
> -
> - // explicitly yield to event loop; this future otherwise blocks ad infinitum
> - tokio::task::yield_now().await;
> - }
> + incoming_stream
> + .async_io(tokio::io::Interest::READABLE, || {
> + let mut buf = [0; HANDSHAKE_BYTES_LEN];
> +
> + // Convert to standard lib TcpStream so we can peek without interfering
> + // with tokio's internals
> + let raw_fd = incoming_stream.as_raw_fd();
> + let std_stream = unsafe { std::net::TcpStream::from_raw_fd(raw_fd) };
> +
> + let peek_res = std_stream.peek(&mut buf);
> +
> + // Prevent destructor from being called, closing the connection
> + // and messing up invariants
> + std::mem::forget(std_stream);
^ Even better would be to use `std::mem::ManuallyDrop` around
`std_stream`, then we can just forget about it.
> +
> + match peek_res {
> + // If we didn't get enough bytes, raise an EAGAIN / EWOULDBLOCK which tells
> + // tokio to await the readiness of the socket again. This should normally
> + // only be used if the socket isn't actually ready, but is fine to do here
> + // in our case.
> + //
> + // This means we will peek into the stream's queue until we got
> + // HANDSHAKE_BYTE_LEN bytes or an error.
> + Ok(peek_len) if peek_len < HANDSHAKE_BYTES_LEN => {
> + Err(io::ErrorKind::WouldBlock.into())
> + }
> + // Either we got Ok(HANDSHAKE_BYTES_LEN) or some error.
> + res => res.map(|_| contains_tls_handshake_fragment(&buf)),
> + }
> + })
> + .await
> + .context("couldn't peek into incoming TCP stream")
> };
>
> - tokio::time::timeout(Duration::from_millis(MS_TIMEOUT), future)
> + tokio::time::timeout(timeout, future)
> .await
> - .unwrap_or(Ok(false))
> + .context("timed out while waiting for client to initiate TLS handshake")?
> }
> }
>
> --
> 2.39.2
More information about the pbs-devel
mailing list