* [PATCH proxmox v3 1/5] rest-server: connection: use enum for TLS handshake check result
2026-11-10 11:07 [PATCH proxmox v3 0/5] Fix #6738: Improve TLS Handshake Handling & Error Logging Max R. Carrara
@ 2026-11-10 11:07 ` Max R. Carrara
2026-11-10 11:07 ` [PATCH proxmox v3 2/5] rest-server: connection: add tests for TLS handshake check Max R. Carrara
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Max R. Carrara @ 2026-11-10 11:07 UTC (permalink / raw)
To: pbs-devel
A plain bool doesn't tell callers what `true` and `false` stand for,
and it also can't represent further outcomes of the check, such as the
client closing the connection before the check could complete.
Therefore, return a dedicated enum instead.
Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
proxmox-rest-server/src/connection.rs | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/proxmox-rest-server/src/connection.rs b/proxmox-rest-server/src/connection.rs
index 153a44b0..3062c4f7 100644
--- a/proxmox-rest-server/src/connection.rs
+++ b/proxmox-rest-server/src/connection.rs
@@ -297,10 +297,21 @@ struct AcceptState {
struct AcceptFlags {
is_debug: bool,
}
+/// Outcome of checking whether a client initiates a TLS handshake on a new connection.
+enum TlsHandshakeCheck {
+ /// The client sent the beginning of a TLS handshake.
+ ///
+ /// See [contains_tls_handshake_fragment] for details on how this is
+ /// checked.
+ Initiated,
+ /// The client sent data that does not start a TLS handshake.
+ NotInitiated,
+}
+
impl AcceptBuilder {
async fn accept_connections(
self,
listener: TcpListener,
acceptor: Arc<Mutex<SslAcceptor>>,
@@ -464,14 +475,14 @@ impl AcceptBuilder {
let handshake_res =
Self::wait_for_client_tls_handshake(socket_ref, CLIENT_HANDSHAKE_TIMEOUT).await;
match handshake_res {
- Ok(true) => {
+ Ok(TlsHandshakeCheck::Initiated) => {
Self::do_accept_tls(state, flags, secure_sender).await;
}
- Ok(false) => {
+ Ok(TlsHandshakeCheck::NotInitiated) => {
let insecure_stream = Box::pin(state.socket);
if let Err(send_err) = insecure_sender.send(Ok(insecure_stream)).await {
log::error!(
"[{peer}] failed to accept connection - connection channel closed: {send_err}"
@@ -485,11 +496,11 @@ impl AcceptBuilder {
}
async fn wait_for_client_tls_handshake(
incoming_stream: &TcpStream,
timeout: Duration,
- ) -> Result<bool, Error> {
+ ) -> Result<TlsHandshakeCheck, Error> {
const HANDSHAKE_BYTES_LEN: usize = 5;
let future = async {
let mut previous_peek_len = 0;
incoming_stream
@@ -525,11 +536,17 @@ impl AcceptBuilder {
previous_peek_len = peek_len;
Err(io::ErrorKind::WouldBlock.into())
}
}
// Either we got Ok(HANDSHAKE_BYTES_LEN) or some error.
- res => res.map(|_| contains_tls_handshake_fragment(&buf)),
+ res => res.map(|_| {
+ if contains_tls_handshake_fragment(&buf) {
+ TlsHandshakeCheck::Initiated
+ } else {
+ TlsHandshakeCheck::NotInitiated
+ }
+ }),
}
})
.await
.context("couldn't peek into incoming TCP stream")
};
--
2.47.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH proxmox v3 2/5] rest-server: connection: add tests for TLS handshake check
2026-11-10 11:07 [PATCH proxmox v3 0/5] Fix #6738: Improve TLS Handshake Handling & Error Logging Max R. Carrara
2026-11-10 11:07 ` [PATCH proxmox v3 1/5] rest-server: connection: use enum for TLS handshake check result Max R. Carrara
@ 2026-11-10 11:07 ` Max R. Carrara
2026-11-10 11:07 ` [PATCH proxmox v3 3/5] rest-server: connection: log cause of TLS handshake check errors Max R. Carrara
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Max R. Carrara @ 2026-11-10 11:07 UTC (permalink / raw)
To: pbs-devel
Checking whether a client initiates a TLS handshake depends on the
client behaves. There are four cases in total:
- the client sends enough data
- sends the data in chunks
- closes or resets the connection early
- or does nothing at all
None of these cases were covered by tests so far.
Therefore, run the TLS handshake check against real local connections
for each of these cases, so that changes in the outcomes or
regressions are caught.
Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
proxmox-rest-server/src/connection.rs | 192 ++++++++++++++++++++++++++
1 file changed, 192 insertions(+)
diff --git a/proxmox-rest-server/src/connection.rs b/proxmox-rest-server/src/connection.rs
index 3062c4f7..00ee771b 100644
--- a/proxmox-rest-server/src/connection.rs
+++ b/proxmox-rest-server/src/connection.rs
@@ -298,10 +298,11 @@ struct AcceptState {
struct AcceptFlags {
is_debug: bool,
}
/// Outcome of checking whether a client initiates a TLS handshake on a new connection.
+#[derive(Debug)]
enum TlsHandshakeCheck {
/// The client sent the beginning of a TLS handshake.
///
/// See [contains_tls_handshake_fragment] for details on how this is
/// checked.
@@ -590,12 +591,203 @@ fn contains_tls_handshake_fragment(buf: &[u8]) -> bool {
buf[0] == 0x16 && buf[1] == 0x3 && (((buf[3] as u16) << 8) + buf[4] as u16) <= CONTENT_SIZE
}
#[cfg(test)]
mod tests {
+ use std::io;
+ use std::time::Duration;
+
+ use anyhow::Error;
+ use tokio::io::AsyncWriteExt;
+ use tokio::net::TcpListener;
+ use tokio::net::TcpStream;
+
+ use super::AcceptBuilder;
+ use super::TlsHandshakeCheck;
use super::contains_tls_handshake_fragment;
+ /// Timeout for checks that are expected to finish on their own.
+ const CHECK_TIMEOUT: Duration = Duration::from_secs(10);
+
+ /// Timeout for checks that are expected to time out.
+ const SHORT_CHECK_TIMEOUT: Duration = Duration::from_millis(200);
+
+ /// Runs `client` against a fresh local connection and checks that connection on the server side
+ /// for a TLS handshake.
+ ///
+ /// If `client` returns its stream, the connection is kept open until the check has finished.
+ async fn check_client<F, Fut>(timeout: Duration, client: F) -> Result<TlsHandshakeCheck, Error>
+ where
+ F: FnOnce(TcpStream) -> Fut + Send + 'static,
+ Fut: Future<Output = Option<TcpStream>> + Send,
+ {
+ let listener = TcpListener::bind("127.0.0.1:0")
+ .await
+ .expect("failed to set up local TCP listener");
+
+ let addr = listener.local_addr().unwrap();
+
+ let client = tokio::spawn(async move {
+ let stream = TcpStream::connect(addr)
+ .await
+ .expect("failed to connect to address of local TCP listener");
+
+ client(stream).await
+ });
+
+ let (server_stream, _) = listener
+ .accept()
+ .await
+ .expect("failed to accept incoming connection");
+
+ let result = AcceptBuilder::wait_for_client_tls_handshake(&server_stream, timeout).await;
+
+ drop(client.await.unwrap());
+
+ result
+ }
+
+ /// Asserts that the check failed because of the connection itself, not because of a timeout.
+ fn assert_connection_error(result: Result<TlsHandshakeCheck, Error>) {
+ let err = result.expect_err("check must fail");
+ assert!(err.downcast_ref::<io::Error>().is_some(), "{err:#}");
+ }
+
+ fn assert_timeout_error(result: Result<TlsHandshakeCheck, Error>) {
+ let err = result.expect_err("check must time out");
+ assert_eq!(
+ format!("{err:#}"),
+ "timed out while waiting for client to initiate TLS handshake: deadline has elapsed",
+ );
+ }
+
+ #[tokio::test]
+ async fn a_tls_client_is_detected() {
+ let result = check_client(CHECK_TIMEOUT, |mut stream| async move {
+ stream
+ .write_all(&[0x16, 0x3, 0x1, 0x02, 0x00])
+ .await
+ .unwrap();
+ Some(stream)
+ })
+ .await;
+
+ assert!(
+ matches!(result, Ok(TlsHandshakeCheck::Initiated)),
+ "{result:?}"
+ );
+ }
+
+ #[tokio::test]
+ async fn a_plain_client_is_detected() {
+ let result = check_client(CHECK_TIMEOUT, |mut stream| async move {
+ stream.write_all(b"GET / HTTP/1.1\r\n").await.unwrap();
+ Some(stream)
+ })
+ .await;
+
+ assert!(
+ matches!(result, Ok(TlsHandshakeCheck::NotInitiated)),
+ "{result:?}"
+ );
+ }
+
+ #[tokio::test]
+ async fn a_client_sending_small_chunks_is_detected() {
+ let result = check_client(CHECK_TIMEOUT, |mut stream| async move {
+ for chunk in [&b"G"[..], b"E", b"T", b" / HTTP/1.1\r\n"] {
+ stream.write_all(chunk).await.unwrap();
+ tokio::time::sleep(Duration::from_millis(20)).await;
+ }
+ Some(stream)
+ })
+ .await;
+
+ assert!(
+ matches!(result, Ok(TlsHandshakeCheck::NotInitiated)),
+ "{result:?}"
+ );
+ }
+
+ #[tokio::test]
+ async fn a_client_closing_without_data_is_reported() {
+ let result = check_client(CHECK_TIMEOUT, |stream| async move {
+ drop(stream);
+ None
+ })
+ .await;
+
+ assert_connection_error(result);
+ }
+
+ #[tokio::test]
+ async fn a_client_closing_after_partial_data_is_reported() {
+ let result = check_client(CHECK_TIMEOUT, |mut stream| async move {
+ stream.write_all(&[0x16, 0x3]).await.unwrap();
+ drop(stream);
+ None
+ })
+ .await;
+
+ assert_connection_error(result);
+ }
+
+ #[tokio::test]
+ async fn a_client_shutting_down_after_partial_data_is_reported() {
+ let result = check_client(CHECK_TIMEOUT, |mut stream| async move {
+ stream.write_all(&[0x16, 0x3]).await.unwrap();
+ stream.shutdown().await.unwrap();
+ Some(stream)
+ })
+ .await;
+
+ assert_connection_error(result);
+ }
+
+ #[tokio::test]
+ async fn a_client_resetting_without_data_is_reported() {
+ let result = check_client(CHECK_TIMEOUT, |stream| async move {
+ stream.set_linger(Some(Duration::ZERO)).unwrap();
+ drop(stream);
+ None
+ })
+ .await;
+
+ assert_connection_error(result);
+ }
+
+ #[tokio::test]
+ async fn a_client_resetting_after_partial_data_is_reported() {
+ let result = check_client(CHECK_TIMEOUT, |mut stream| async move {
+ stream.write_all(&[0x16, 0x3]).await.unwrap();
+ stream.set_linger(Some(Duration::ZERO)).unwrap();
+ drop(stream);
+ None
+ })
+ .await;
+
+ assert_connection_error(result);
+ }
+
+ #[tokio::test]
+ async fn an_idle_client_times_out() {
+ let result = check_client(SHORT_CHECK_TIMEOUT, |stream| async move { Some(stream) }).await;
+
+ assert_timeout_error(result);
+ }
+
+ #[tokio::test]
+ async fn a_client_stalling_after_partial_data_times_out() {
+ let result = check_client(SHORT_CHECK_TIMEOUT, |mut stream| async move {
+ stream.write_all(&[0x16, 0x3]).await.unwrap();
+ Some(stream)
+ })
+ .await;
+
+ assert_timeout_error(result);
+ }
+
#[test]
fn a_handshake_fragment_is_detected() {
// content type 22, major version 3, any minor version, length within 2^14
assert!(contains_tls_handshake_fragment(&[
0x16, 0x3, 0x1, 0x02, 0x00
--
2.47.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH proxmox v3 3/5] rest-server: connection: log cause of TLS handshake check errors
2026-11-10 11:07 [PATCH proxmox v3 0/5] Fix #6738: Improve TLS Handshake Handling & Error Logging Max R. Carrara
2026-11-10 11:07 ` [PATCH proxmox v3 1/5] rest-server: connection: use enum for TLS handshake check result Max R. Carrara
2026-11-10 11:07 ` [PATCH proxmox v3 2/5] rest-server: connection: add tests for TLS handshake check Max R. Carrara
@ 2026-11-10 11:07 ` Max R. Carrara
2026-11-10 11:07 ` [PATCH proxmox v3 4/5] rest-server: connection: avoid dropping clients during TLS check Max R. Carrara
2026-11-10 11:07 ` [PATCH proxmox v3 5/5] fix #6738: rest-server: log early closed connections at debug level Max R. Carrara
4 siblings, 0 replies; 6+ messages in thread
From: Max R. Carrara @ 2026-11-10 11:07 UTC (permalink / raw)
To: pbs-devel
Errors from checking for a TLS handshake were logged with only their
outermost context, which hid the underlying OS error. For example, a
failed peek was only reported as "couldn't peek into incoming TCP
stream", without saying why it failed.
Log the whole error chain on a single line instead, so that the actual
reason shows up in the logs.
Also, do not keep the error from `tokio::time::timeout` in the error
context, because it doesn't tell us any additional information.
Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
proxmox-rest-server/src/connection.rs | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/proxmox-rest-server/src/connection.rs b/proxmox-rest-server/src/connection.rs
index 00ee771b..9b3c1371 100644
--- a/proxmox-rest-server/src/connection.rs
+++ b/proxmox-rest-server/src/connection.rs
@@ -489,11 +489,11 @@ impl AcceptBuilder {
"[{peer}] failed to accept connection - connection channel closed: {send_err}"
);
}
}
Err(err) => {
- log::error!("[{peer}] failed to check for TLS handshake: {err}");
+ log::error!("[{peer}] failed to check for TLS handshake: {err:#}");
}
}
}
async fn wait_for_client_tls_handshake(
@@ -550,13 +550,15 @@ impl AcceptBuilder {
})
.await
.context("couldn't peek into incoming TCP stream")
};
- tokio::time::timeout(timeout, future)
- .await
- .context("timed out while waiting for client to initiate TLS handshake")?
+ // The elapsed error carries no information beyond the timeout itself, so do not keep it as
+ // the cause.
+ tokio::time::timeout(timeout, future).await.map_err(|_| {
+ format_err!("timed out while waiting for client to initiate TLS handshake")
+ })?
}
}
/// Checks whether an [SSL 3.0 / TLS plaintext fragment][0] being part of a
/// SSL / TLS handshake is contained in the given buffer.
@@ -654,11 +656,11 @@ mod tests {
fn assert_timeout_error(result: Result<TlsHandshakeCheck, Error>) {
let err = result.expect_err("check must time out");
assert_eq!(
format!("{err:#}"),
- "timed out while waiting for client to initiate TLS handshake: deadline has elapsed",
+ "timed out while waiting for client to initiate TLS handshake",
);
}
#[tokio::test]
async fn a_tls_client_is_detected() {
--
2.47.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH proxmox v3 4/5] rest-server: connection: avoid dropping clients during TLS check
2026-11-10 11:07 [PATCH proxmox v3 0/5] Fix #6738: Improve TLS Handshake Handling & Error Logging Max R. Carrara
` (2 preceding siblings ...)
2026-11-10 11:07 ` [PATCH proxmox v3 3/5] rest-server: connection: log cause of TLS handshake check errors Max R. Carrara
@ 2026-11-10 11:07 ` Max R. Carrara
2026-11-10 11:07 ` [PATCH proxmox v3 5/5] fix #6738: rest-server: log early closed connections at debug level Max R. Carrara
4 siblings, 0 replies; 6+ messages in thread
From: Max R. Carrara @ 2026-11-10 11:07 UTC (permalink / raw)
To: pbs-devel
While waiting for enough data to check for a TLS handshake, the
connection was considered closed as soon as the socket was reported
readable twice in a row with the same amount of data queued.
This can also happen while the client is still sending: if more data
arrives after tokio received the readiness event for the previous data
but before the peek, tokio keeps the socket marked as readable, and
the next peek sees no new data. The connection was then dropped and an
error was logged, even though the client did nothing wrong.
Only clients whose initial message is shorter than the five bytes
needed for the check are affected (both plain and TLS ones).
Note that this should occur very rarely (if at all) in practice,
because most clients send more than 5 bytes at once. I cannot actually
think of any scenario where this might happen in practice (unless you
intentionally split up your messages), but it's better to fix this
anyway.
So, only consider the connection closed if:
- the peek returns no data at all
- or if the read side of the socket was closed by the client
Both of these cases are definite signs that no further data will
arrive.
(I'm being more elaborate below than I usually am, because the changes
are not trivial at all.)
First, replace `incoming_stream.async_io()` with a plain `loop` and
await the readiness of the socket explicitly via
`incoming_stream.ready()`.
This is necessary because the closure passed to `async_io()` does not
get any readiness information, which we however need to actually check
what the client did.
Use the readiness info to check for `is_read_closed()`, which means
that no more data will arrive on the socket beyond what's already
queued. Now we know for sure that the connection is being closed.
Second, use `try_io()` for the buffer peeking + handshake check. This
is basically like `async_io()`, except that we are not awaiting the
readiness of the socket. We awaited that already.
(So, `.ready()` + `.try_io()` = `.async_io()`, kinda.)
Third, match on the result of `try_io()` to check for a `WouldBlock`
error. If this is returned, it means that the socket either wasn't
*actually* ready [1], or that the client sent < 5 bytes. Either way,
the loop continues in that case.
Fourth, await `tokio::task::consume_budget()` directly to charge each
iteration against tokio's cooperative scheduling budget (like tokio's
own IO helpers), so that the loop cannot starve other tasks or the
timeout.
To summarize: We explicitly (or "manually") check for the
readiness of the socket to see whether the client is closing the
connection or not, instead of letting `async_io()` handle this under
the hood. We also play nice with tokio's scheduling budget.
This should ensure that no connections are dropped unintentionally
anymore, even if their occurrence is (or at least should be)
incredibly rare.
[1]: https://docs.rs/tokio/latest/tokio/net/struct.TcpStream.html#method.try_io
Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
proxmox-rest-server/src/connection.rs | 57 ++++++++++++++++-----------
1 file changed, 34 insertions(+), 23 deletions(-)
diff --git a/proxmox-rest-server/src/connection.rs b/proxmox-rest-server/src/connection.rs
index 9b3c1371..f116ef47 100644
--- a/proxmox-rest-server/src/connection.rs
+++ b/proxmox-rest-server/src/connection.rs
@@ -501,64 +501,75 @@ impl AcceptBuilder {
timeout: Duration,
) -> Result<TlsHandshakeCheck, Error> {
const HANDSHAKE_BYTES_LEN: usize = 5;
let future = async {
- let mut previous_peek_len = 0;
- incoming_stream
- .async_io(tokio::io::Interest::READABLE, || {
+ loop {
+ // Charge each iteration against tokio's cooperative scheduling
+ // budget, like tokio's own IO helpers do, so that the loop
+ // cannot starve other tasks or the timeout.
+ tokio::task::consume_budget().await;
+
+ let ready = incoming_stream.ready(tokio::io::Interest::READABLE).await?;
+
+ let peek_res = incoming_stream.try_io(tokio::io::Interest::READABLE, || {
let mut buf = [0; HANDSHAKE_BYTES_LEN];
use std::os::fd::{AsRawFd, FromRawFd};
// Convert to standard lib TcpStream so we can peek without interfering
// with tokio's internals. Wrap the stream in ManuallyDrop in order to prevent
// the destructor from being called, closing the connection and messing up
// invariants.
let raw_fd = incoming_stream.as_raw_fd();
let std_stream =
unsafe { ManuallyDrop::new(std::net::TcpStream::from_raw_fd(raw_fd)) };
- let peek_res = std_stream.peek(&mut buf);
-
- match peek_res {
+ match std_stream.peek(&mut buf) {
// 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.
+ // tokio to clear the readiness of the socket, so that we wait for more
+ // data. 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.
+ // Only stop waiting on definite signs of a closed connection:
+ // 1. an empty peek
+ // 2. a closed read side
+ //
+ // The socket can be marked as readable again without any new data, for
+ // example if more data arrived between tokio receiving a readiness event
+ // and `peek()`. So, readiness *alone* is not a sign of a closed connection.
Ok(peek_len) if peek_len < HANDSHAKE_BYTES_LEN => {
- // if we detect the same peek len again but still got a readable stream,
- // the connection was probably closed, so abort here
- if peek_len == previous_peek_len {
+ if peek_len == 0 || ready.is_read_closed() {
Err(io::ErrorKind::ConnectionAborted.into())
} else {
- previous_peek_len = peek_len;
Err(io::ErrorKind::WouldBlock.into())
}
}
// Either we got Ok(HANDSHAKE_BYTES_LEN) or some error.
res => res.map(|_| {
if contains_tls_handshake_fragment(&buf) {
TlsHandshakeCheck::Initiated
} else {
TlsHandshakeCheck::NotInitiated
}
}),
}
- })
- .await
- .context("couldn't peek into incoming TCP stream")
+ });
+
+ match peek_res {
+ Err(err) if err.kind() == io::ErrorKind::WouldBlock => continue,
+ res => break res,
+ }
+ }
};
- // The elapsed error carries no information beyond the timeout itself, so do not keep it as
- // the cause.
- tokio::time::timeout(timeout, future).await.map_err(|_| {
- format_err!("timed out while waiting for client to initiate TLS handshake")
- })?
+ tokio::time::timeout(timeout, future)
+ .await
+ .map_err(|_| {
+ format_err!("timed out while waiting for client to initiate TLS handshake")
+ })?
+ .context("couldn't peek into incoming TCP stream")
}
}
/// Checks whether an [SSL 3.0 / TLS plaintext fragment][0] being part of a
/// SSL / TLS handshake is contained in the given buffer.
--
2.47.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH proxmox v3 5/5] fix #6738: rest-server: log early closed connections at debug level
2026-11-10 11:07 [PATCH proxmox v3 0/5] Fix #6738: Improve TLS Handshake Handling & Error Logging Max R. Carrara
` (3 preceding siblings ...)
2026-11-10 11:07 ` [PATCH proxmox v3 4/5] rest-server: connection: avoid dropping clients during TLS check Max R. Carrara
@ 2026-11-10 11:07 ` Max R. Carrara
4 siblings, 0 replies; 6+ messages in thread
From: Max R. Carrara @ 2026-11-10 11:07 UTC (permalink / raw)
To: pbs-devel
When TLS is optional, the server waits for the first five bytes of a
new connection to check whether the client initiates a TLS handshake.
If the client closed the connection before sending these bytes, the
check failed with an error that was logged at error level. Health
checks and monitoring tools that only test whether the port is open
do exactly that, which floods the logs with these errors.
Report a connection that is closed before the check completes as a
separate outcome of the check, and log it only at debug level.
Treat a reset connection the same way, because some health checks
close their sockets like that to avoid lingering connections.
Use the debug log level instead of the acceptor's debug flag, which
other messages about misbehaving clients are tied to, so that these
messages can be enabled through the regular log level configuration.
Other errors are still logged at error level. This includes timeouts
of clients that connect but do not send enough data.
Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
proxmox-rest-server/src/connection.rs | 39 ++++++++++++++++++---------
1 file changed, 27 insertions(+), 12 deletions(-)
diff --git a/proxmox-rest-server/src/connection.rs b/proxmox-rest-server/src/connection.rs
index f116ef47..d2756c0b 100644
--- a/proxmox-rest-server/src/connection.rs
+++ b/proxmox-rest-server/src/connection.rs
@@ -307,10 +307,13 @@ enum TlsHandshakeCheck {
/// See [contains_tls_handshake_fragment] for details on how this is
/// checked.
Initiated,
/// The client sent data that does not start a TLS handshake.
NotInitiated,
+ /// The client closed or reset the connection before sending enough data for
+ /// the check.
+ ConnectionClosed,
}
impl AcceptBuilder {
async fn accept_connections(
self,
@@ -488,10 +491,17 @@ impl AcceptBuilder {
log::error!(
"[{peer}] failed to accept connection - connection channel closed: {send_err}"
);
}
}
+ Ok(TlsHandshakeCheck::ConnectionClosed) => {
+ // Health checks and port scanners often connect without sending
+ // anything, so keep this out of the logs by default
+ log::debug!(
+ "[{peer}] client closed the connection before the TLS handshake check completed"
+ );
+ }
Err(err) => {
log::error!("[{peer}] failed to check for TLS handshake: {err:#}");
}
}
}
@@ -537,16 +547,21 @@ impl AcceptBuilder {
// The socket can be marked as readable again without any new data, for
// example if more data arrived between tokio receiving a readiness event
// and `peek()`. So, readiness *alone* is not a sign of a closed connection.
Ok(peek_len) if peek_len < HANDSHAKE_BYTES_LEN => {
if peek_len == 0 || ready.is_read_closed() {
- Err(io::ErrorKind::ConnectionAborted.into())
+ Ok(TlsHandshakeCheck::ConnectionClosed)
} else {
Err(io::ErrorKind::WouldBlock.into())
}
}
- // Either we got Ok(HANDSHAKE_BYTES_LEN) or some error.
+ // Some clients, for example health checks, reset the connection by closing
+ // their socket with SO_LINGER set to zero to avoid lingering connections.
+ Err(err) if err.kind() == io::ErrorKind::ConnectionReset => {
+ Ok(TlsHandshakeCheck::ConnectionClosed)
+ }
+ // Either we got Ok(HANDSHAKE_BYTES_LEN) or some other error.
res => res.map(|_| {
if contains_tls_handshake_fragment(&buf) {
TlsHandshakeCheck::Initiated
} else {
TlsHandshakeCheck::NotInitiated
@@ -604,11 +619,10 @@ fn contains_tls_handshake_fragment(buf: &[u8]) -> bool {
buf[0] == 0x16 && buf[1] == 0x3 && (((buf[3] as u16) << 8) + buf[4] as u16) <= CONTENT_SIZE
}
#[cfg(test)]
mod tests {
- use std::io;
use std::time::Duration;
use anyhow::Error;
use tokio::io::AsyncWriteExt;
use tokio::net::TcpListener;
@@ -657,14 +671,15 @@ mod tests {
drop(client.await.unwrap());
result
}
- /// Asserts that the check failed because of the connection itself, not because of a timeout.
- fn assert_connection_error(result: Result<TlsHandshakeCheck, Error>) {
- let err = result.expect_err("check must fail");
- assert!(err.downcast_ref::<io::Error>().is_some(), "{err:#}");
+ fn assert_connection_closed(result: Result<TlsHandshakeCheck, Error>) {
+ assert!(
+ matches!(result, Ok(TlsHandshakeCheck::ConnectionClosed)),
+ "{result:?}"
+ );
}
fn assert_timeout_error(result: Result<TlsHandshakeCheck, Error>) {
let err = result.expect_err("check must time out");
assert_eq!(
@@ -727,60 +742,60 @@ mod tests {
drop(stream);
None
})
.await;
- assert_connection_error(result);
+ assert_connection_closed(result);
}
#[tokio::test]
async fn a_client_closing_after_partial_data_is_reported() {
let result = check_client(CHECK_TIMEOUT, |mut stream| async move {
stream.write_all(&[0x16, 0x3]).await.unwrap();
drop(stream);
None
})
.await;
- assert_connection_error(result);
+ assert_connection_closed(result);
}
#[tokio::test]
async fn a_client_shutting_down_after_partial_data_is_reported() {
let result = check_client(CHECK_TIMEOUT, |mut stream| async move {
stream.write_all(&[0x16, 0x3]).await.unwrap();
stream.shutdown().await.unwrap();
Some(stream)
})
.await;
- assert_connection_error(result);
+ assert_connection_closed(result);
}
#[tokio::test]
async fn a_client_resetting_without_data_is_reported() {
let result = check_client(CHECK_TIMEOUT, |stream| async move {
stream.set_linger(Some(Duration::ZERO)).unwrap();
drop(stream);
None
})
.await;
- assert_connection_error(result);
+ assert_connection_closed(result);
}
#[tokio::test]
async fn a_client_resetting_after_partial_data_is_reported() {
let result = check_client(CHECK_TIMEOUT, |mut stream| async move {
stream.write_all(&[0x16, 0x3]).await.unwrap();
stream.set_linger(Some(Duration::ZERO)).unwrap();
drop(stream);
None
})
.await;
- assert_connection_error(result);
+ assert_connection_closed(result);
}
#[tokio::test]
async fn an_idle_client_times_out() {
let result = check_client(SHORT_CHECK_TIMEOUT, |stream| async move { Some(stream) }).await;
--
2.47.3
^ permalink raw reply related [flat|nested] 6+ messages in thread