[pbs-devel] [PATCH v2 proxmox 0/3] Fix #5105: Overhaul TLS Handshake Checking Logic

[pbs-devel] [PATCH v2 proxmox 0/3] Fix #5105: Overhaul TLS Handshake Checking Logic

[Max Carrara]

Fix #5105: Overhaul TLS Handshake Checking Logic - v2
=====================================================

Notable Changes Since v1
------------------------

As discussed with Wolfgang off-list, instead of busy-waiting and
continuously yielding back to the event loop, patch 03 now makes use of
some lower-level functionality in tokio, which allows us to "retry"
peeking into the TCP stream's queue and raise an EAGAIN / EWOULDBLOCK if
we haven't received all required bytes to perform the TLS handshake
check.

The reason for this change is that streams behave incorrectly in terms
of edge-triggering [1], and we currently have no guarantee that we won't
run into related bugs when we're peeking into the stream's queue (or
that they won't affect us) in the future.

In short, the event loop isn't supposed to wake the task again if we
didn't receive enough bytes yet. With the change made to patch 03, what
happens is that we're only peeking into the stream's queue if we're told
that we can actually peek again.

All in all, we're not busy-waiting anymore while simultaneously ensuring
that our implementation will remain correct in the future.

Thanks to Wolfgang for all the help in this regard!

Older Versions
--------------

v1: https://lists.proxmox.com/pipermail/pbs-devel/2024-July/010091.html

References
----------

[1]: https://lwn.net/Articles/864947/

Summary of Changes
------------------

Max Carrara (3):
  rest-server: connection: clean up accept data flow
  rest-server: connection: log peer address on error
  fix #5105: rest-server: connection: overhaul TLS handshake check logic

 proxmox-rest-server/src/connection.rs | 206 ++++++++++++++------------
 1 file changed, 115 insertions(+), 91 deletions(-)

-- 
2.39.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH v2 proxmox 1/3] rest-server: connection: clean up accept data flow

[Max Carrara]

This adds the structs `AcceptState` and `AcceptFlags` and adapts
relevant method signatures of `AcceptBuilder` accordingly. This makes
it easier to add further parameters in the future.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * none

 proxmox-rest-server/src/connection.rs | 72 ++++++++++++++-------------
 1 file changed, 38 insertions(+), 34 deletions(-)

diff --git a/proxmox-rest-server/src/connection.rs b/proxmox-rest-server/src/connection.rs
index 34b585cb..243348c0 100644
--- a/proxmox-rest-server/src/connection.rs
+++ b/proxmox-rest-server/src/connection.rs
@@ -255,6 +255,16 @@ impl From<(ClientSender, InsecureClientSender)> for Sender {
     }
 }
 
+struct AcceptState {
+    pub socket: InsecureClientStream,
+    pub acceptor: Arc<Mutex<SslAcceptor>>,
+    pub accept_counter: Arc<()>,
+}
+
+struct AcceptFlags {
+    pub is_debug: bool,
+}
+
 impl AcceptBuilder {
     async fn accept_connections(
         self,
@@ -285,24 +295,26 @@ impl AcceptBuilder {
                 continue;
             }
 
+            let state = AcceptState {
+                socket,
+                acceptor,
+                accept_counter,
+            };
+
+            let flags = AcceptFlags {
+                is_debug: self.debug,
+            };
+
             match sender {
                 Sender::Secure(ref secure_sender) => {
-                    let accept_future = Self::do_accept_tls(
-                        socket,
-                        acceptor,
-                        accept_counter,
-                        self.debug,
-                        secure_sender.clone(),
-                    );
+                    let accept_future = Self::do_accept_tls(state, flags, secure_sender.clone());
 
                     tokio::spawn(accept_future);
                 }
                 Sender::SecureAndInsecure(ref secure_sender, ref insecure_sender) => {
                     let accept_future = Self::do_accept_tls_optional(
-                        socket,
-                        acceptor,
-                        accept_counter,
-                        self.debug,
+                        state,
+                        flags,
                         secure_sender.clone(),
                         insecure_sender.clone(),
                     );
@@ -343,17 +355,11 @@ impl AcceptBuilder {
         Ok(socket)
     }
 
-    async fn do_accept_tls(
-        socket: InsecureClientStream,
-        acceptor: Arc<Mutex<SslAcceptor>>,
-        accept_counter: Arc<()>,
-        debug: bool,
-        secure_sender: ClientSender,
-    ) {
+    async fn do_accept_tls(state: AcceptState, flags: AcceptFlags, secure_sender: ClientSender) {
         let ssl = {
             // limit acceptor_guard scope
             // Acceptor can be reloaded using the command socket "reload-certificate" command
-            let acceptor_guard = acceptor.lock().unwrap();
+            let acceptor_guard = state.acceptor.lock().unwrap();
 
             match openssl::ssl::Ssl::new(acceptor_guard.context()) {
                 Ok(ssl) => ssl,
@@ -364,7 +370,7 @@ impl AcceptBuilder {
             }
         };
 
-        let secure_stream = match tokio_openssl::SslStream::new(ssl, socket) {
+        let secure_stream = match tokio_openssl::SslStream::new(ssl, state.socket) {
             Ok(stream) => stream,
             Err(err) => {
                 log::error!("failed to create SslStream using ssl and connection socket - {err}");
@@ -381,41 +387,39 @@ impl AcceptBuilder {
 
         match result {
             Ok(Ok(())) => {
-                if secure_sender.send(Ok(secure_stream)).await.is_err() && debug {
+                if secure_sender.send(Ok(secure_stream)).await.is_err() && flags.is_debug {
                     log::error!("detected closed connection channel");
                 }
             }
             Ok(Err(err)) => {
-                if debug {
+                if flags.is_debug {
                     log::error!("https handshake failed - {err}");
                 }
             }
             Err(_) => {
-                if debug {
+                if flags.is_debug {
                     log::error!("https handshake timeout");
                 }
             }
         }
 
-        drop(accept_counter); // decrease reference count
+        drop(state.accept_counter); // decrease reference count
     }
 
     async fn do_accept_tls_optional(
-        socket: InsecureClientStream,
-        acceptor: Arc<Mutex<SslAcceptor>>,
-        accept_counter: Arc<()>,
-        debug: bool,
+        state: AcceptState,
+        flags: AcceptFlags,
         secure_sender: ClientSender,
         insecure_sender: InsecureClientSender,
     ) {
         let client_initiates_handshake = {
             #[cfg(feature = "rate-limited-stream")]
-            let socket = socket.inner();
+            let socket_ref = state.socket.inner();
 
             #[cfg(not(feature = "rate-limited-stream"))]
-            let socket = &socket;
+            let socket_ref = &state.socket;
 
-            match Self::wait_for_client_tls_handshake(socket).await {
+            match Self::wait_for_client_tls_handshake(socket_ref).await {
                 Ok(initiates_handshake) => initiates_handshake,
                 Err(err) => {
                     log::error!("error checking for TLS handshake: {err}");
@@ -425,16 +429,16 @@ impl AcceptBuilder {
         };
 
         if !client_initiates_handshake {
-            let insecure_stream = Box::pin(socket);
+            let insecure_stream = Box::pin(state.socket);
 
-            if insecure_sender.send(Ok(insecure_stream)).await.is_err() && debug {
+            if insecure_sender.send(Ok(insecure_stream)).await.is_err() && flags.is_debug {
                 log::error!("detected closed connection channel")
             }
 
             return;
         }
 
-        Self::do_accept_tls(socket, acceptor, accept_counter, debug, secure_sender).await
+        Self::do_accept_tls(state, flags, secure_sender).await
     }
 
     async fn wait_for_client_tls_handshake(incoming_stream: &TcpStream) -> Result<bool, Error> {
-- 
2.39.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH v2 proxmox 2/3] rest-server: connection: log peer address on error

[Max Carrara]

.. in order to make debugging easier and logs more helpful.

Signed-off-by: Max Carrara <m.carrara@proxmox.com>
---
Changes v1 --> v2:
  * none

 proxmox-rest-server/src/connection.rs | 42 ++++++++++++++++-----------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/proxmox-rest-server/src/connection.rs b/proxmox-rest-server/src/connection.rs
index 243348c0..470021d7 100644
--- a/proxmox-rest-server/src/connection.rs
+++ b/proxmox-rest-server/src/connection.rs
@@ -2,6 +2,7 @@
 //!
 //! Hyper building block.
 
+use std::net::SocketAddr;
 use std::os::unix::io::AsRawFd;
 use std::path::PathBuf;
 use std::pin::Pin;
@@ -257,6 +258,7 @@ impl From<(ClientSender, InsecureClientSender)> for Sender {
 
 struct AcceptState {
     pub socket: InsecureClientStream,
+    pub peer: SocketAddr,
     pub acceptor: Arc<Mutex<SslAcceptor>>,
     pub accept_counter: Arc<()>,
 }
@@ -276,9 +278,9 @@ impl AcceptBuilder {
         let mut shutdown_future = crate::shutdown_future().fuse();
 
         loop {
-            let socket = futures::select! {
+            let (socket, peer) = futures::select! {
                 res = self.try_setup_socket(&listener).fuse() => match res {
-                    Ok(socket) => socket,
+                    Ok(socket_peer) => socket_peer,
                     Err(err) => {
                         log::error!("couldn't set up TCP socket: {err}");
                         continue;
@@ -291,12 +293,13 @@ impl AcceptBuilder {
             let accept_counter = Arc::clone(&accept_counter);
 
             if Arc::strong_count(&accept_counter) > self.max_pending_accepts {
-                log::error!("connection rejected - too many open connections");
+                log::error!("[{peer}] connection rejected - too many open connections");
                 continue;
             }
 
             let state = AcceptState {
                 socket,
+                peer,
                 acceptor,
                 accept_counter,
             };
@@ -328,7 +331,7 @@ impl AcceptBuilder {
     async fn try_setup_socket(
         &self,
         listener: &TcpListener,
-    ) -> Result<InsecureClientStream, Error> {
+    ) -> Result<(InsecureClientStream, SocketAddr), Error> {
         let (socket, peer) = match listener.accept().await {
             Ok(connection) => connection,
             Err(error) => {
@@ -338,10 +341,10 @@ impl AcceptBuilder {
 
         socket
             .set_nodelay(true)
-            .context("error while setting TCP_NODELAY on socket")?;
+            .with_context(|| format!("[{peer}] error while setting TCP_NODELAY on socket"))?;
 
         proxmox_sys::linux::socket::set_tcp_keepalive(socket.as_raw_fd(), self.tcp_keepalive_time)
-            .context("error while setting SO_KEEPALIVE on socket")?;
+            .with_context(|| format!("[{peer}] error while setting SO_KEEPALIVE on socket"))?;
 
         #[cfg(feature = "rate-limited-stream")]
         let socket = match self.lookup_rate_limiter.clone() {
@@ -349,13 +352,12 @@ impl AcceptBuilder {
             None => RateLimitedStream::with_limiter(socket, None, None),
         };
 
-        #[cfg(not(feature = "rate-limited-stream"))]
-        let _peer = peer;
-
-        Ok(socket)
+        Ok((socket, peer))
     }
 
     async fn do_accept_tls(state: AcceptState, flags: AcceptFlags, secure_sender: ClientSender) {
+        let peer = state.peer;
+
         let ssl = {
             // limit acceptor_guard scope
             // Acceptor can be reloaded using the command socket "reload-certificate" command
@@ -364,7 +366,9 @@ impl AcceptBuilder {
             match openssl::ssl::Ssl::new(acceptor_guard.context()) {
                 Ok(ssl) => ssl,
                 Err(err) => {
-                    log::error!("failed to create Ssl object from Acceptor context - {err}");
+                    log::error!(
+                        "[{peer}] failed to create Ssl object from Acceptor context - {err}"
+                    );
                     return;
                 }
             }
@@ -373,7 +377,9 @@ impl AcceptBuilder {
         let secure_stream = match tokio_openssl::SslStream::new(ssl, state.socket) {
             Ok(stream) => stream,
             Err(err) => {
-                log::error!("failed to create SslStream using ssl and connection socket - {err}");
+                log::error!(
+                    "[{peer}] failed to create SslStream using ssl and connection socket - {err}"
+                );
                 return;
             }
         };
@@ -388,17 +394,17 @@ impl AcceptBuilder {
         match result {
             Ok(Ok(())) => {
                 if secure_sender.send(Ok(secure_stream)).await.is_err() && flags.is_debug {
-                    log::error!("detected closed connection channel");
+                    log::error!("[{peer}] detected closed connection channel");
                 }
             }
             Ok(Err(err)) => {
                 if flags.is_debug {
-                    log::error!("https handshake failed - {err}");
+                    log::error!("[{peer}] https handshake failed - {err}");
                 }
             }
             Err(_) => {
                 if flags.is_debug {
-                    log::error!("https handshake timeout");
+                    log::error!("[{peer}] https handshake timeout");
                 }
             }
         }
@@ -412,6 +418,8 @@ impl AcceptBuilder {
         secure_sender: ClientSender,
         insecure_sender: InsecureClientSender,
     ) {
+        let peer = state.peer;
+
         let client_initiates_handshake = {
             #[cfg(feature = "rate-limited-stream")]
             let socket_ref = state.socket.inner();
@@ -422,7 +430,7 @@ impl AcceptBuilder {
             match Self::wait_for_client_tls_handshake(socket_ref).await {
                 Ok(initiates_handshake) => initiates_handshake,
                 Err(err) => {
-                    log::error!("error checking for TLS handshake: {err}");
+                    log::error!("[{peer}] error checking for TLS handshake: {err}");
                     return;
                 }
             }
@@ -432,7 +440,7 @@ impl AcceptBuilder {
             let insecure_stream = Box::pin(state.socket);
 
             if insecure_sender.send(Ok(insecure_stream)).await.is_err() && flags.is_debug {
-                log::error!("detected closed connection channel")
+                log::error!("[{peer}] detected closed connection channel")
             }
 
             return;
-- 
2.39.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH v2 proxmox 3/3] fix #5105: rest-server: connection: overhaul TLS handshake check logic

[Max Carrara]

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@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);
+
+                    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



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH v2 proxmox 3/3] fix #5105: rest-server: connection: overhaul TLS handshake check logic

[Wolfgang Bumiller]

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


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH v2 proxmox 3/3] fix #5105: rest-server: connection: overhaul TLS handshake check logic

[Max Carrara]

On Tue Jul 9, 2024 at 12:29 PM CEST, Wolfgang Bumiller wrote:
> 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@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.

Good point, thanks! Will send in a v3 soon.

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



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel