[pve-devel] [PATCH http-server 0/2] remove unnecessary websocket code

[pve-devel] [PATCH http-server 0/2] remove unnecessary websocket code

[Dominik Csapak]

we do not need/want any websocket subprotocol handling, nor do
we need 'base64' encoding/decoding, so drop it

we can drop the protocols in the clients (novnc/xtermjs) with
the next major release after this is released, else the cluster console
will not work. the compatibility code here can then be dropped one major
release after

so if we opt to apply it now for 6.4 still, we may get away with
dropping the client protocol for 7.x, and the compat code with 8.0

else we have to wait for 8.0 to drop the client code, and 9.0
for the compat code

Dominik Csapak (2):
  AnyEvent/websocket_proxy: remove 'base64' handling
  AnyEvent/websocket_proxy: drop handling of websocket subprotocols

 src/PVE/APIServer/AnyEvent.pm | 34 +++++++---------------------------
 1 file changed, 7 insertions(+), 27 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH http-server 1/2] AnyEvent/websocket_proxy: remove 'base64' handling

[Dominik Csapak]

novnc does not support this anymore since 2015, and neither does
our xtermjs client. it is also not listed in IANAs list of websocket
protocols [0].

so simply drop it and only send out binary frames and don't decode text frames

0: https://www.iana.org/assignments/websocket/websocket.xml#subprotocol-name

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/APIServer/AnyEvent.pm | 21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index f0e2e68..a7d31cc 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -496,15 +496,6 @@ sub websocket_proxy {
 
 	my $max_payload_size = 128*1024;
 
-	my $binary;
-	if ($wsproto eq 'binary') {
-	    $binary = 1;
-	} elsif ($wsproto eq 'base64') {
-	    $binary = 0;
-	} else {
-	    die "websocket_proxy: unsupported protocol '$wsproto'\n";
-	}
-
 	if ($param->{port}) {
 	    $remhost = 'localhost';
 	    $remport = $param->{port};
@@ -520,13 +511,9 @@ sub websocket_proxy {
 
 	    my $string;
 	    my $payload;
-	    if ($binary) {
-		$string = $opcode ? $opcode : "\x82"; # binary frame
-		$payload = $$data;
-	    } else {
-		$string = $opcode ? $opcode : "\x81"; # text frame
-		$payload = encode_base64($$data, '');
-	    }
+
+	    $string = $opcode ? $opcode : "\x82"; # binary frame
+	    $payload = $$data;
 
 	    my $payload_len = length($payload);
 	    if ($payload_len <= 125) {
@@ -635,8 +622,6 @@ sub websocket_proxy {
 			$payload ^= $mask;
 		    }
 
-		    $payload = decode_base64($payload) if !$binary;
-
 		    if ($opcode == 1 || $opcode == 2) {
 			$reqstate->{proxyhdl}->push_write($payload) if $reqstate->{proxyhdl};
 		    } elsif ($opcode == 8) {
-- 
2.20.1

[pve-devel] [PATCH http-server 2/2] AnyEvent/websocket_proxy: drop handling of websocket subprotocols

[Dominik Csapak]

We do not support any, and we only ever send binary frames, so drop
trying to parse the header.

For compatibility with current clients (novnc, pve-xtermjs), we have
to reply with the protocols it sent.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/APIServer/AnyEvent.pm | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index a7d31cc..6f0abb7 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -650,11 +650,13 @@ sub websocket_proxy {
 
 	    # todo: use stop_read/start_read if write buffer grows to much
 
+	    # for backwards, compatibility,  we have to reply with the websocket
+	    # subprotocol from the request
 	    my $res = "$proto 101 Switching Protocols\015\012" .
 		"Upgrade: websocket\015\012" .
 		"Connection: upgrade\015\012" .
 		"Sec-WebSocket-Accept: $wsaccept\015\012" .
-		"Sec-WebSocket-Protocol: $wsproto\015\012" .
+		($wsproto ne "" ? "Sec-WebSocket-Protocol: $wsproto\015\012" : "") .
 		"\015\012";
 
 	    $self->dprint($res);
@@ -902,14 +904,7 @@ sub handle_api2_request {
 	    die "unable to upgrade to protocol '$upgrade'\n" if !$upgrade || ($upgrade ne 'websocket');
 	    my $wsver = $r->header('sec-websocket-version');
 	    die "unsupported websocket-version '$wsver'\n" if !$wsver || ($wsver ne '13');
-	    my $wsproto_str = $r->header('sec-websocket-protocol');
-	    die "missing websocket-protocol header" if !$wsproto_str;
-	    my $wsproto;
-	    foreach my $p (PVE::Tools::split_list($wsproto_str)) {
-		$wsproto = $p if !$wsproto && $p eq 'base64';
-		$wsproto = $p if $p eq 'binary';
-	    }
-	    die "unsupported websocket-protocol protocol '$wsproto_str'\n" if !$wsproto;
+	    my $wsproto = $r->header('sec-websocket-protocol') // "";
 	    my $wskey = $r->header('sec-websocket-key');
 	    die "missing websocket-key\n" if !$wskey;
 	    # Note: Digest::SHA::sha1_base64 has wrong padding
-- 
2.20.1

[pve-devel] applied-series: [PATCH http-server 0/2] remove unnecessary websocket code

[Fabian Grünbichler]

to stable-6 and master

On May 17, 2021 3:07 pm, Dominik Csapak wrote:
> we do not need/want any websocket subprotocol handling, nor do
> we need 'base64' encoding/decoding, so drop it
> 
> we can drop the protocols in the clients (novnc/xtermjs) with
> the next major release after this is released, else the cluster console
> will not work. the compatibility code here can then be dropped one major
> release after
> 
> so if we opt to apply it now for 6.4 still, we may get away with
> dropping the client protocol for 7.x, and the compat code with 8.0
> 
> else we have to wait for 8.0 to drop the client code, and 9.0
> for the compat code
> 
> Dominik Csapak (2):
>   AnyEvent/websocket_proxy: remove 'base64' handling
>   AnyEvent/websocket_proxy: drop handling of websocket subprotocols
> 
>  src/PVE/APIServer/AnyEvent.pm | 34 +++++++---------------------------
>  1 file changed, 7 insertions(+), 27 deletions(-)
> 
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>