Message ID | 20221202151202.24851-1-pyr@spootnik.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | ui/vnc.c: Allow websocket connections over AF_UNIX sockets | expand |
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;
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>
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
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
> 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
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
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
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
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;