[pve-devel] [PATCH pve-http-server v2 1/1] fix unexpected EOF for client when closing TLS session

[pve-devel] [PATCH pve-http-server v2 1/1] fix unexpected EOF for client when closing TLS session

[Rob Rozestraten via pve-devel]

[-- Attachment #1: Type: message/rfc822, Size: 5965 bytes --]

From: Rob Rozestraten <admin@truthsolo.net>
To: pve-devel@lists.proxmox.com
Subject: [PATCH pve-http-server v2 1/1] fix unexpected EOF for client when closing TLS session
Date: Wed, 05 Mar 2025 21:45:01 +0000
Message-ID: <20250305214447.128975-2-admin@truthsolo.net>

When pve-http-server initiates the closure of a TLS session, it does not
send a TLS close notify, resulting in an unexpected EOF error on systems
with recent crypto policies. This can break functionality with other
applications, such as Foreman[0].

This behavior can be observed in the following cases:

 * client uses HTTP/1.0 (no keepalive; server closes connection)
 * client sends no data for 5 sec (timeout; server closes connection)
 * server responds with 400 (no keepalive; server closes connection)

This patch sends the TLS close notify prior to socket teardown,
resulting in clean closure of TLS connections and no client error.

It also moves shutdown() to after the clearing of handlers. The reason
for this is stoptls() must come before shutdown(), but it also triggers
on_drain(), which calls client_do_disconnect() again. The extra call to
client_do_disconnect() is avoided inside accept_connections() by commit
f737984, but perhaps clearing the handlers prior to shutdown() will
avoid it in all cases.

[0]: https://github.com/theforeman/foreman_fog_proxmox/issues/325

Signed-off-by: Rob Rozestraten <admin@truthsolo.net>
---

 changes since v1:
 * move stoptls() after clearing of handlers
 * move shutdown() after clearing of handlers

 src/PVE/APIServer/AnyEvent.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index 8a52836..e6d1028 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -141,11 +141,13 @@ sub client_do_disconnect {
     my $shutdown_hdl = sub {
 	my $hdl = shift;
 
-	shutdown($hdl->{fh}, 1);
 	# clear all handlers
 	$hdl->on_drain(undef);
 	$hdl->on_read(undef);
 	$hdl->on_eof(undef);
+
+	$hdl->stoptls();
+	shutdown($hdl->{fh}, 1);
     };
 
     if (my $proxyhdl = delete $reqstate->{proxyhdl}) {
-- 
2.48.1




[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[pve-devel] applied: [PATCH pve-http-server v2 1/1] fix unexpected EOF for client when closing TLS session

[Thomas Lamprecht]

On Wed, 05 Mar 2025 21:45:01 +0000, Rob Rozestraten via pve-devel wrote:
> When pve-http-server initiates the closure of a TLS session, it does not
> send a TLS close notify, resulting in an unexpected EOF error on systems
> with recent crypto policies. This can break functionality with other
> applications, such as Foreman[0].
> 
> This behavior can be observed in the following cases:
> 
> [...]

Applied, thanks!

[1/1] fix unexpected EOF for client when closing TLS session
      commit: 19ffdbf938692757bc1d1f0a24bf7fbccde21565


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

Re: [pve-devel] [PATCH pve-http-server v2 1/1] fix unexpected EOF for client when closing TLS session

[Fiona Ebner]

Am 05.03.25 um 22:45 schrieb Rob Rozestraten via pve-devel:
> When pve-http-server initiates the closure of a TLS session, it does not
> send a TLS close notify, resulting in an unexpected EOF error on systems
> with recent crypto policies. This can break functionality with other
> applications, such as Foreman[0].
> 
> This behavior can be observed in the following cases:
> 
>  * client uses HTTP/1.0 (no keepalive; server closes connection)
>  * client sends no data for 5 sec (timeout; server closes connection)
>  * server responds with 400 (no keepalive; server closes connection)
> 
> This patch sends the TLS close notify prior to socket teardown,
> resulting in clean closure of TLS connections and no client error.
> 
> It also moves shutdown() to after the clearing of handlers. The reason
> for this is stoptls() must come before shutdown(), but it also triggers
> on_drain(), which calls client_do_disconnect() again. The extra call to
> client_do_disconnect() is avoided inside accept_connections() by commit
> f737984, but perhaps clearing the handlers prior to shutdown() will
> avoid it in all cases.
> 
> [0]: https://github.com/theforeman/foreman_fog_proxmox/issues/325
> 

I feel like the questions regarding blocking/missing client ack from
Fabian from v1 are not answered yet:

> If I read the docs right, this could block (would that be an issue here?) and could potentially destroy the handle (so that might need to be rechecked afterwards to prevent spurious warnings?)
> 
> what happens if we initiate the teardown, and the client never acks it?


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

Re: [pve-devel] [PATCH pve-http-server v2 1/1] fix unexpected EOF for client when closing TLS session

[Fabian Grünbichler]

> Fiona Ebner <f.ebner@proxmox.com> hat am 08.04.2025 09:27 CEST geschrieben:
> 
>  
> Am 05.03.25 um 22:45 schrieb Rob Rozestraten via pve-devel:
> > When pve-http-server initiates the closure of a TLS session, it does not
> > send a TLS close notify, resulting in an unexpected EOF error on systems
> > with recent crypto policies. This can break functionality with other
> > applications, such as Foreman[0].
> > 
> > This behavior can be observed in the following cases:
> > 
> >  * client uses HTTP/1.0 (no keepalive; server closes connection)
> >  * client sends no data for 5 sec (timeout; server closes connection)
> >  * server responds with 400 (no keepalive; server closes connection)
> > 
> > This patch sends the TLS close notify prior to socket teardown,
> > resulting in clean closure of TLS connections and no client error.
> > 
> > It also moves shutdown() to after the clearing of handlers. The reason
> > for this is stoptls() must come before shutdown(), but it also triggers
> > on_drain(), which calls client_do_disconnect() again. The extra call to
> > client_do_disconnect() is avoided inside accept_connections() by commit
> > f737984, but perhaps clearing the handlers prior to shutdown() will
> > avoid it in all cases.
> > 
> > [0]: https://github.com/theforeman/foreman_fog_proxmox/issues/325
> > 
> 
> I feel like the questions regarding blocking/missing client ack from
> Fabian from v1 are not answered yet:
> 
> > If I read the docs right, this could block (would that be an issue here?) and could potentially destroy the handle (so that might need to be rechecked afterwards to prevent spurious warnings?)
> > 
> > what happens if we initiate the teardown, and the client never acks it?

there was some more input in a separate mail:

https://lore.proxmox.com/pve-devel/mailman.799.1741211155.293.pve-devel@lists.proxmox.com/


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

Re: [pve-devel] [PATCH pve-http-server v2 1/1] fix unexpected EOF for client when closing TLS session

[Fiona Ebner]

Am 08.04.25 um 10:48 schrieb Fabian Grünbichler:
>> Fiona Ebner <f.ebner@proxmox.com> hat am 08.04.2025 09:27 CEST geschrieben:
>> I feel like the questions regarding blocking/missing client ack from
>> Fabian from v1 are not answered yet:
>>
>>> If I read the docs right, this could block (would that be an issue here?) and could potentially destroy the handle (so that might need to be rechecked afterwards to prevent spurious warnings?)
>>>
>>> what happens if we initiate the teardown, and the client never acks it?
> 
> there was some more input in a separate mail:
> 
> https://lore.proxmox.com/pve-devel/mailman.799.1741211155.293.pve-devel@lists.proxmox.com/

Okay, great :)

It would be nice if mailman (or something) wouldn't mess up
mails/threads from external senders :/


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