[PATCH] ui/vnc.c: Allow websocket connections over AF_UNIX sockets

Pierre-Yves Ritschard posted 1 patch 1 year, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20221202151202.24851-1-pyr@spootnik.org
ui/vnc.c | 5 -----
1 file changed, 5 deletions(-)
[PATCH] ui/vnc.c: Allow websocket connections over AF_UNIX sockets
Posted by Pierre-Yves Ritschard 1 year, 3 months ago
Hi,

The provided patch allows the VNC websocket server of a qemu process to
be provided over AF_UNIX as it is already possible for standard TCP VNC
servers.

Now that many clients support websocket connections, some exclusively,
it can be useful to expose the VNC server. One such case is when a
proxy is present on a host machine, allowing it to proxy to a
deterministic location withouth having to keep track of port mappings.

Removing the condition as is done in the provided patch creates a 
functional server. If there's a downside to this approach I could not
figure it out while reading the code. My hunch was that the condition
was there for a reason, but it did not jump at me.

Cheers,
  - pyr

---
 ui/vnc.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git ui/vnc.c ui/vnc.c
index 88f55cbf3c..b01a655400 100644
--- ui/vnc.c
+++ ui/vnc.c
@@ -3729,11 +3729,6 @@ static int vnc_display_get_address(const char *addrstr,
         addr->type = SOCKET_ADDRESS_TYPE_UNIX;
         addr->u.q_unix.path = g_strdup(addrstr + 5);
 
-        if (websocket) {
-            error_setg(errp, "UNIX sockets not supported with websock");
-            goto cleanup;
-        }
-
         if (to) {
             error_setg(errp, "Port range not support with UNIX socket");
             goto cleanup;
-- 
2.38.1
Re: [PATCH] ui/vnc.c: Allow websocket connections over AF_UNIX sockets
Posted by Daniel P. Berrangé 1 year, 2 months ago
On Fri, Dec 02, 2022 at 04:12:04PM +0100, Pierre-Yves Ritschard wrote:
> Hi,
> 
> The provided patch allows the VNC websocket server of a qemu process to
> be provided over AF_UNIX as it is already possible for standard TCP VNC
> servers.
> 
> Now that many clients support websocket connections, some exclusively,
> it can be useful to expose the VNC server. One such case is when a
> proxy is present on a host machine, allowing it to proxy to a
> deterministic location withouth having to keep track of port mappings.
> 
> Removing the condition as is done in the provided patch creates a 
> functional server. If there's a downside to this approach I could not
> figure it out while reading the code. My hunch was that the condition
> was there for a reason, but it did not jump at me.
> 
> Cheers,
>   - pyr
> 
> ---
>  ui/vnc.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git ui/vnc.c ui/vnc.c
> index 88f55cbf3c..b01a655400 100644
> --- ui/vnc.c
> +++ ui/vnc.c
> @@ -3729,11 +3729,6 @@ static int vnc_display_get_address(const char *addrstr,
>          addr->type = SOCKET_ADDRESS_TYPE_UNIX;
>          addr->u.q_unix.path = g_strdup(addrstr + 5);
>  
> -        if (websocket) {
> -            error_setg(errp, "UNIX sockets not supported with websock");
> -            goto cleanup;
> -        }
> -

Allowing websockets is fine, but just removing this check is not
sufficient

The 'websocket=XXXX' parameter for -vnc takes two formats

   websocket=on|off

or

   websocket=portnum

In the case of on|off, the code takes the original VNC display
num and listens on   5700 + display for websockets, 590 + display
for non-websockets.

In the case of a explicit port, the code listens on that port.

Also we fail to actually handle 'off' correctly, just treating
it as a named port

$ qemu-system-x86_64  -vnc :1,websocket=off
qemu-system-x86_64: -vnc :1,websocket=off: address resolution failed for :off: Servname not supported for ai_socktype


Anyway given an argument

   -vnc  unix:/some/path,websocket=on

this cause causes QEMU to listen on a relative path 'on'. We need
to define what the semantics for websockets=on are going to be
for UNIX sockets. Should it append '.ws' to the main path ? Should
we just not allow websockets=on and document it must be an explicit
path at all times ?

We also need to document this in qemu-options.hx.

>          if (to) {
>              error_setg(errp, "Port range not support with UNIX socket");
>              goto cleanup;
> -- 
> 2.38.1
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH] ui/vnc.c: Allow websocket connections over AF_UNIX sockets
Posted by Pierre-Yves Ritschard 1 year, 2 months ago
Allowing websockets is fine, but just removing this check is not
> sufficient
>
> The 'websocket=XXXX' parameter for -vnc takes two formats
>
>    websocket=on|off
>
> or
>
>    websocket=portnum
>
> In the case of on|off, the code takes the original VNC display
> num and listens on   5700 + display for websockets, 590 + display
> for non-websockets.
>
> In the case of a explicit port, the code listens on that port.
>
> Also we fail to actually handle 'off' correctly, just treating
> it as a named port
>
> $ qemu-system-x86_64  -vnc :1,websocket=off
> qemu-system-x86_64: -vnc :1,websocket=off: address resolution failed for
> :off: Servname not supported for ai_socktype
>
>
> Anyway given an argument
>
>    -vnc  unix:/some/path,websocket=on
>
> this cause causes QEMU to listen on a relative path 'on'. We need
> to define what the semantics for websockets=on are going to be
> for UNIX sockets. Should it append '.ws' to the main path ? Should
> we just not allow websockets=on and document it must be an explicit
> path at all times ?
>
> We also need to document this in qemu-options.hx.
>
>
>
Thank you, these semantics weren't obvious to me, I will adapt accordingly
and post a new patch
Re: [PATCH] ui/vnc.c: Allow websocket connections over AF_UNIX sockets
Posted by Peter Maydell 1 year, 2 months ago
On Tue, 17 Jan 2023 at 13:05, Pierre-Yves Ritschard <pyr@spootnik.org> wrote:
>> Allowing websockets is fine, but just removing this check is not
>> sufficient

> Thank you, these semantics weren't obvious to me, I will adapt accordingly and post a new patch


When you do, please make sure you include a Signed-off-by: line.
This is how you say you're legally OK to contribute the change
and happy for it to go into QEMU, so we can't take code without one:

https://www.qemu.org/docs/master/devel/submitting-a-patch.html#patch-emails-must-include-a-signed-off-by-line

thanks
-- PMM
Re: [PATCH] ui/vnc.c: Allow websocket connections over AF_UNIX sockets
Posted by Laurent Vivier 1 year, 2 months ago
Hi Gerd,

If this patch is correct I can queue it via trivial branch.

Thanks,
Laurent

Le 02/12/2022 à 16:12, Pierre-Yves Ritschard a écrit :
> Hi,
> 
> The provided patch allows the VNC websocket server of a qemu process to
> be provided over AF_UNIX as it is already possible for standard TCP VNC
> servers.
> 
> Now that many clients support websocket connections, some exclusively,
> it can be useful to expose the VNC server. One such case is when a
> proxy is present on a host machine, allowing it to proxy to a
> deterministic location withouth having to keep track of port mappings.
> 
> Removing the condition as is done in the provided patch creates a
> functional server. If there's a downside to this approach I could not
> figure it out while reading the code. My hunch was that the condition
> was there for a reason, but it did not jump at me.
> 
> Cheers,
>    - pyr
> 
> ---
>   ui/vnc.c | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git ui/vnc.c ui/vnc.c
> index 88f55cbf3c..b01a655400 100644
> --- ui/vnc.c
> +++ ui/vnc.c
> @@ -3729,11 +3729,6 @@ static int vnc_display_get_address(const char *addrstr,
>           addr->type = SOCKET_ADDRESS_TYPE_UNIX;
>           addr->u.q_unix.path = g_strdup(addrstr + 5);
>   
> -        if (websocket) {
> -            error_setg(errp, "UNIX sockets not supported with websock");
> -            goto cleanup;
> -        }
> -
>           if (to) {
>               error_setg(errp, "Port range not support with UNIX socket");
>               goto cleanup;


Re: [PATCH] ui/vnc.c: Allow websocket connections over AF_UNIX sockets
Posted by Gerd Hoffmann 1 year, 2 months ago
On Mon, Jan 16, 2023 at 07:15:08PM +0100, Laurent Vivier wrote:
> Hi Gerd,
> 
> If this patch is correct I can queue it via trivial branch.

proxying tcp websocket connections to a unix socket looks like
a reasonable use case to me.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>
Re: [PATCH] ui/vnc.c: Allow websocket connections over AF_UNIX sockets
Posted by Daniel P. Berrangé 1 year, 2 months ago
On Tue, Jan 17, 2023 at 01:43:12PM +0100, Gerd Hoffmann wrote:
> On Mon, Jan 16, 2023 at 07:15:08PM +0100, Laurent Vivier wrote:
> > Hi Gerd,
> > 
> > If this patch is correct I can queue it via trivial branch.
> 
> proxying tcp websocket connections to a unix socket looks like
> a reasonable use case to me.
> 
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>

Please don't queue, this patch is not correct becasue it is failing
to validate the 'websocket' parameter at all.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH] ui/vnc.c: Allow websocket connections over AF_UNIX sockets
Posted by Laurent Vivier 1 year, 2 months ago
Le 17/01/2023 à 13:53, Daniel P. Berrangé a écrit :
> On Tue, Jan 17, 2023 at 01:43:12PM +0100, Gerd Hoffmann wrote:
>> On Mon, Jan 16, 2023 at 07:15:08PM +0100, Laurent Vivier wrote:
>>> Hi Gerd,
>>>
>>> If this patch is correct I can queue it via trivial branch.
>>
>> proxying tcp websocket connections to a unix socket looks like
>> a reasonable use case to me.
>>
>> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> Please don't queue, this patch is not correct becasue it is failing
> to validate the 'websocket' parameter at all.

OK, I will not.

Thank you for the comment.

Laurent


Re: [PATCH] ui/vnc.c: Allow websocket connections over AF_UNIX sockets
Posted by Pierre-Yves Ritschard 1 year, 2 months ago
> Please don't queue, this patch is not correct becasue it is failing
> to validate the 'websocket' parameter at all.
>
>
Hi,

Thanks for the review! I'm happy to adapt the patch to add validation,
though I am not quite sure what additional validation you would like to
see occur here.

Cheers,
Pierre-Yves