Who / what uses QMP command add_client?

Who / what uses QMP command add_client?

[Markus Armbruster]

QMP command add_client's schema:

    ##
    # @add_client:
    #
    # Allow client connections for VNC, Spice and socket based
    # character devices to be passed in to QEMU via SCM_RIGHTS.
    #
    # @protocol: protocol name. Valid names are "vnc", "spice", "@dbus-display" or
    #            the name of a character device (eg. from -chardev id=XXXX)
    #
    # @fdname: file descriptor name previously passed via 'getfd' command
    #
    # @skipauth: whether to skip authentication. Only applies
    #            to "vnc" and "spice" protocols
    #
    # @tls: whether to perform TLS. Only applies to the "spice"
    #       protocol
    #
    # Returns: nothing on success.
    #
    # Since: 0.14
    #
    # Example:
    #
    # -> { "execute": "add_client", "arguments": { "protocol": "vnc",
    #                                              "fdname": "myclient" } }
    # <- { "return": {} }
    #
    ##
    { 'command': 'add_client',
      'data': { 'protocol': 'str', 'fdname': 'str', '*skipauth': 'bool',
                '*tls': 'bool' } }

Spot the design flaw!

It's overloaded @protocol.  Two issues.

One, character device IDs "vnc", "spice", "@dbus-display" don't work
here.  If we ever add another protocol, we make another device ID not
work.  Perhaps this is why Marc-André chose "@dbus-display", which
otherwise looks like a typo :)

Two, introspection can't tell us what protocols are supported.

As far as I can tell, libvirt does not use this with character devices.
It does use the other three protocols.

Are there any known uses with character devices?

If not, any ideas why one would want to use the command with character
devices?

Re: Who / what uses QMP command add_client?

[Daniel P. Berrangé]

On Tue, Nov 29, 2022 at 03:54:56PM +0100, Markus Armbruster wrote:
> QMP command add_client's schema:
> 
>     ##
>     # @add_client:
>     #
>     # Allow client connections for VNC, Spice and socket based
>     # character devices to be passed in to QEMU via SCM_RIGHTS.
>     #
>     # @protocol: protocol name. Valid names are "vnc", "spice", "@dbus-display" or
>     #            the name of a character device (eg. from -chardev id=XXXX)
>     #
>     # @fdname: file descriptor name previously passed via 'getfd' command
>     #
>     # @skipauth: whether to skip authentication. Only applies
>     #            to "vnc" and "spice" protocols
>     #
>     # @tls: whether to perform TLS. Only applies to the "spice"
>     #       protocol
>     #
>     # Returns: nothing on success.
>     #
>     # Since: 0.14
>     #
>     # Example:
>     #
>     # -> { "execute": "add_client", "arguments": { "protocol": "vnc",
>     #                                              "fdname": "myclient" } }
>     # <- { "return": {} }
>     #
>     ##
>     { 'command': 'add_client',
>       'data': { 'protocol': 'str', 'fdname': 'str', '*skipauth': 'bool',
>                 '*tls': 'bool' } }
> 
> Spot the design flaw!
> 
> It's overloaded @protocol.  Two issues.

My bad. Can't imagine why I called its impl add_graphics_client
but then made it work for graphics clients and chardevs all
the way back in day 1.

> 
> One, character device IDs "vnc", "spice", "@dbus-display" don't work
> here.  If we ever add another protocol, we make another device ID not
> work.  Perhaps this is why Marc-André chose "@dbus-display", which
> otherwise looks like a typo :)
> 
> Two, introspection can't tell us what protocols are supported.
> 
> As far as I can tell, libvirt does not use this with character devices.
> It does use the other three protocols.
> 
> Are there any known uses with character devices?
> 
> If not, any ideas why one would want to use the command with character
> devices?

Ordinarily a client will directly connect() to QEMU to setup the
socket connection. Depending on the protocol this may involve both
TLS negotiation and authentication. This is a good thing when exposed
over a public IP address. It is tedious when connecting from a local
client though.

The idea behind the 'add_client' method was to enable short circuiting
of encryption and authentication, for local only clients. For example,
virt-viewer/virt-manager can do socketpair() and pass one of the FDs
across to QEMU, and bypass any VNC authentication. This is still secure,
as FD passing is mediated by libvirt which the app has already
authenticated against.

This is conceptually useful for any backend exposed as a network
socket, accepting ad-hoc client connections. So it is in scope for
chardevs, nbd, vnc, spice.

I notice another flaw for chardevs though - we're ignoring thue
'skipauth' parameter, so we're failing to skip TLS on chardevs
should anyone try to use this.

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: Who / what uses QMP command add_client?

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2386 bytes --]

Hi

On Tue, Nov 29, 2022 at 6:55 PM Markus Armbruster <armbru@redhat.com> wrote:

> QMP command add_client's schema:
>
>     ##
>     # @add_client:
>     #
>     # Allow client connections for VNC, Spice and socket based
>     # character devices to be passed in to QEMU via SCM_RIGHTS.
>     #
>     # @protocol: protocol name. Valid names are "vnc", "spice",
> "@dbus-display" or
>     #            the name of a character device (eg. from -chardev id=XXXX)
>     #
>     # @fdname: file descriptor name previously passed via 'getfd' command
>     #
>     # @skipauth: whether to skip authentication. Only applies
>     #            to "vnc" and "spice" protocols
>     #
>     # @tls: whether to perform TLS. Only applies to the "spice"
>     #       protocol
>     #
>     # Returns: nothing on success.
>     #
>     # Since: 0.14
>     #
>     # Example:
>     #
>     # -> { "execute": "add_client", "arguments": { "protocol": "vnc",
>     #                                              "fdname": "myclient" } }
>     # <- { "return": {} }
>     #
>     ##
>     { 'command': 'add_client',
>       'data': { 'protocol': 'str', 'fdname': 'str', '*skipauth': 'bool',
>                 '*tls': 'bool' } }
>
> Spot the design flaw!
>
> It's overloaded @protocol.  Two issues.
>
> One, character device IDs "vnc", "spice", "@dbus-display" don't work
> here.  If we ever add another protocol, we make another device ID not
> work.  Perhaps this is why Marc-André chose "@dbus-display", which
> otherwise looks like a typo :)
>

That's right, I tried to avoid conflicting with chardev ID namespace. IDs
can't start with '@'.

btw, I have a few patches pending to extend add_client for windows sockets.

I also have patches to check if fds are actually sockets, since that
command doesn't make much sense with other fds.


> Two, introspection can't tell us what protocols are supported.
>

Hmm, not really a big deal I suppose. You would have both compile-time and
run-time config. There are other means to introspect the display protocol,
like query-vnc or query-spice. I thought I had something covering dbus as
well, but I can't find it, I'll look at it.

Let me know if you plan to touch that command, it will likely conflict with
my work. I plan to submit it soon after the release, but I might do it
earlier.

[-- Attachment #2: Type: text/html, Size: 3501 bytes --]

Re: Who / what uses QMP command add_client?

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Nov 29, 2022 at 03:54:56PM +0100, Markus Armbruster wrote:
>> QMP command add_client's schema:
>> 
>>     ##
>>     # @add_client:
>>     #
>>     # Allow client connections for VNC, Spice and socket based
>>     # character devices to be passed in to QEMU via SCM_RIGHTS.
>>     #
>>     # @protocol: protocol name. Valid names are "vnc", "spice", "@dbus-display" or
>>     #            the name of a character device (eg. from -chardev id=XXXX)
>>     #
>>     # @fdname: file descriptor name previously passed via 'getfd' command
>>     #
>>     # @skipauth: whether to skip authentication. Only applies
>>     #            to "vnc" and "spice" protocols
>>     #
>>     # @tls: whether to perform TLS. Only applies to the "spice"
>>     #       protocol
>>     #
>>     # Returns: nothing on success.
>>     #
>>     # Since: 0.14
>>     #
>>     # Example:
>>     #
>>     # -> { "execute": "add_client", "arguments": { "protocol": "vnc",
>>     #                                              "fdname": "myclient" } }
>>     # <- { "return": {} }
>>     #
>>     ##
>>     { 'command': 'add_client',
>>       'data': { 'protocol': 'str', 'fdname': 'str', '*skipauth': 'bool',
>>                 '*tls': 'bool' } }
>> 
>> Spot the design flaw!
>> 
>> It's overloaded @protocol.  Two issues.
>
> My bad. Can't imagine why I called its impl add_graphics_client
> but then made it work for graphics clients and chardevs all
> the way back in day 1.

We had a lot less experience with QMP interface design back then.

The obvious fix (if we want to) is to add protocol "chardev" with
additional member for the chardev's ID, and deprecate use of chardev IDs
as protocol.

Compatibility break: a chardev with ID "chardev" no longer works.

Could also use "socket" instead of "chardev"if we're confident no other
chardev type will ever be needed here.

>> One, character device IDs "vnc", "spice", "@dbus-display" don't work
>> here.  If we ever add another protocol, we make another device ID not
>> work.  Perhaps this is why Marc-André chose "@dbus-display", which
>> otherwise looks like a typo :)
>> 
>> Two, introspection can't tell us what protocols are supported.
>> 
>> As far as I can tell, libvirt does not use this with character devices.
>> It does use the other three protocols.
>> 
>> Are there any known uses with character devices?

See [*] below.

>> If not, any ideas why one would want to use the command with character
>> devices?
>
> Ordinarily a client will directly connect() to QEMU to setup the
> socket connection. Depending on the protocol this may involve both
> TLS negotiation and authentication. This is a good thing when exposed
> over a public IP address. It is tedious when connecting from a local
> client though.
>
> The idea behind the 'add_client' method was to enable short circuiting
> of encryption and authentication, for local only clients. For example,
> virt-viewer/virt-manager can do socketpair() and pass one of the FDs
> across to QEMU, and bypass any VNC authentication. This is still secure,
> as FD passing is mediated by libvirt which the app has already
> authenticated against.
>
> This is conceptually useful for any backend exposed as a network
> socket, accepting ad-hoc client connections. So it is in scope for
> chardevs, nbd, vnc, spice.

Does libvirt implement this with socket character devices?

[*] If yes, we have a known use.  Else we don't, I presume.

> I notice another flaw for chardevs though - we're ignoring thue
> 'skipauth' parameter, so we're failing to skip TLS on chardevs
> should anyone try to use this.
>
> With regards,
> Daniel

Re: Who / what uses QMP command add_client?

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Tue, Nov 29, 2022 at 6:55 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> QMP command add_client's schema:
>>
>>     ##
>>     # @add_client:
>>     #
>>     # Allow client connections for VNC, Spice and socket based
>>     # character devices to be passed in to QEMU via SCM_RIGHTS.
>>     #
>>     # @protocol: protocol name. Valid names are "vnc", "spice",
>> "@dbus-display" or
>>     #            the name of a character device (eg. from -chardev id=XXXX)
>>     #
>>     # @fdname: file descriptor name previously passed via 'getfd' command
>>     #
>>     # @skipauth: whether to skip authentication. Only applies
>>     #            to "vnc" and "spice" protocols
>>     #
>>     # @tls: whether to perform TLS. Only applies to the "spice"
>>     #       protocol
>>     #
>>     # Returns: nothing on success.
>>     #
>>     # Since: 0.14
>>     #
>>     # Example:
>>     #
>>     # -> { "execute": "add_client", "arguments": { "protocol": "vnc",
>>     #                                              "fdname": "myclient" } }
>>     # <- { "return": {} }
>>     #
>>     ##
>>     { 'command': 'add_client',
>>       'data': { 'protocol': 'str', 'fdname': 'str', '*skipauth': 'bool',
>>                 '*tls': 'bool' } }
>>
>> Spot the design flaw!
>>
>> It's overloaded @protocol.  Two issues.
>>
>> One, character device IDs "vnc", "spice", "@dbus-display" don't work
>> here.  If we ever add another protocol, we make another device ID not
>> work.  Perhaps this is why Marc-André chose "@dbus-display", which
>> otherwise looks like a typo :)
>>
>
> That's right, I tried to avoid conflicting with chardev ID namespace. IDs
> can't start with '@'.
>
> btw, I have a few patches pending to extend add_client for windows sockets.
>
> I also have patches to check if fds are actually sockets, since that
> command doesn't make much sense with other fds.
>
>
>> Two, introspection can't tell us what protocols are supported.
>
> Hmm, not really a big deal I suppose. You would have both compile-time and
> run-time config. There are other means to introspect the display protocol,
> like query-vnc or query-spice. I thought I had something covering dbus as
> well, but I can't find it, I'll look at it.

Defeating introspection like this is careless design.  I agree this one
is no big deal in practice.

> Let me know if you plan to touch that command, it will likely conflict with
> my work. I plan to submit it soon after the release, but I might do it
> earlier.

I looked at the command only because I'm trying to move crap out of
monitor/.

For PCI, that's "[PATCH 00/12] pci: Move and clean up monitor command
code".

add_client is a bit awkward.  It's in qapi/misc.json and
monitor/qmp-cmds.c.  If it was only about VNC and Spice, I'd move it to
qapi/ui.json and ui/.  If it was only about socket character devices, to
qapi/char.json and char/.  But it's both.  Unless we pick one of the two
arbitrarily, it's stuck in monitor/.

Re: Who / what uses QMP command add_client?

[Daniel P. Berrangé]

On Wed, Nov 30, 2022 at 01:30:43PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Tue, Nov 29, 2022 at 03:54:56PM +0100, Markus Armbruster wrote:
> >> QMP command add_client's schema:
> >> 
> >>     ##
> >>     # @add_client:
> >>     #
> >>     # Allow client connections for VNC, Spice and socket based
> >>     # character devices to be passed in to QEMU via SCM_RIGHTS.
> >>     #
> >>     # @protocol: protocol name. Valid names are "vnc", "spice", "@dbus-display" or
> >>     #            the name of a character device (eg. from -chardev id=XXXX)
> >>     #
> >>     # @fdname: file descriptor name previously passed via 'getfd' command
> >>     #
> >>     # @skipauth: whether to skip authentication. Only applies
> >>     #            to "vnc" and "spice" protocols
> >>     #
> >>     # @tls: whether to perform TLS. Only applies to the "spice"
> >>     #       protocol
> >>     #
> >>     # Returns: nothing on success.
> >>     #
> >>     # Since: 0.14
> >>     #
> >>     # Example:
> >>     #
> >>     # -> { "execute": "add_client", "arguments": { "protocol": "vnc",
> >>     #                                              "fdname": "myclient" } }
> >>     # <- { "return": {} }
> >>     #
> >>     ##
> >>     { 'command': 'add_client',
> >>       'data': { 'protocol': 'str', 'fdname': 'str', '*skipauth': 'bool',
> >>                 '*tls': 'bool' } }
> >> 
> >> Spot the design flaw!
> >> 
> >> It's overloaded @protocol.  Two issues.
> >
> > My bad. Can't imagine why I called its impl add_graphics_client
> > but then made it work for graphics clients and chardevs all
> > the way back in day 1.
> 
> We had a lot less experience with QMP interface design back then.
> 
> The obvious fix (if we want to) is to add protocol "chardev" with
> additional member for the chardev's ID, and deprecate use of chardev IDs
> as protocol.
> 
> Compatibility break: a chardev with ID "chardev" no longer works.
> 
> Could also use "socket" instead of "chardev"if we're confident no other
> chardev type will ever be needed here.

Or introduce a new 'id' field that are refer to a qdev ID, since
we can assign IDs to VNC/SPICE server instances too, when there
are multiple instances, and they'll be non-overlapping with
chardev IDs ?

IOW we make 'protocol' and 'id' both optional in QAPI schema, and
declare them mutually exclusive. Deprecate 'protocol' in favour
of 'id'. Then eventually delete 'protocol' and make 'id'
mandatory.


> >> Are there any known uses with character devices?
> 
> See [*] below.
> 
> >> If not, any ideas why one would want to use the command with character
> >> devices?
> >
> > Ordinarily a client will directly connect() to QEMU to setup the
> > socket connection. Depending on the protocol this may involve both
> > TLS negotiation and authentication. This is a good thing when exposed
> > over a public IP address. It is tedious when connecting from a local
> > client though.
> >
> > The idea behind the 'add_client' method was to enable short circuiting
> > of encryption and authentication, for local only clients. For example,
> > virt-viewer/virt-manager can do socketpair() and pass one of the FDs
> > across to QEMU, and bypass any VNC authentication. This is still secure,
> > as FD passing is mediated by libvirt which the app has already
> > authenticated against.
> >
> > This is conceptually useful for any backend exposed as a network
> > socket, accepting ad-hoc client connections. So it is in scope for
> > chardevs, nbd, vnc, spice.
> 
> Does libvirt implement this with socket character devices?

Opps, I meant to say that libvirt only uses add_client for
graphics devices. We've never used it for chardevs AFAICT.

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: Who / what uses QMP command add_client?

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Nov 30, 2022 at 01:30:43PM +0100, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> 
>> > On Tue, Nov 29, 2022 at 03:54:56PM +0100, Markus Armbruster wrote:
>> >> QMP command add_client's schema:
>> >> 
>> >>     ##
>> >>     # @add_client:
>> >>     #
>> >>     # Allow client connections for VNC, Spice and socket based
>> >>     # character devices to be passed in to QEMU via SCM_RIGHTS.
>> >>     #
>> >>     # @protocol: protocol name. Valid names are "vnc", "spice", "@dbus-display" or
>> >>     #            the name of a character device (eg. from -chardev id=XXXX)
>> >>     #
>> >>     # @fdname: file descriptor name previously passed via 'getfd' command
>> >>     #
>> >>     # @skipauth: whether to skip authentication. Only applies
>> >>     #            to "vnc" and "spice" protocols
>> >>     #
>> >>     # @tls: whether to perform TLS. Only applies to the "spice"
>> >>     #       protocol
>> >>     #
>> >>     # Returns: nothing on success.
>> >>     #
>> >>     # Since: 0.14
>> >>     #
>> >>     # Example:
>> >>     #
>> >>     # -> { "execute": "add_client", "arguments": { "protocol": "vnc",
>> >>     #                                              "fdname": "myclient" } }
>> >>     # <- { "return": {} }
>> >>     #
>> >>     ##
>> >>     { 'command': 'add_client',
>> >>       'data': { 'protocol': 'str', 'fdname': 'str', '*skipauth': 'bool',
>> >>                 '*tls': 'bool' } }
>> >> 
>> >> Spot the design flaw!
>> >> 
>> >> It's overloaded @protocol.  Two issues.
>> >
>> > My bad. Can't imagine why I called its impl add_graphics_client
>> > but then made it work for graphics clients and chardevs all
>> > the way back in day 1.
>> 
>> We had a lot less experience with QMP interface design back then.
>> 
>> The obvious fix (if we want to) is to add protocol "chardev" with
>> additional member for the chardev's ID, and deprecate use of chardev IDs
>> as protocol.
>> 
>> Compatibility break: a chardev with ID "chardev" no longer works.
>> 
>> Could also use "socket" instead of "chardev"if we're confident no other
>> chardev type will ever be needed here.
>
> Or introduce a new 'id' field that are refer to a qdev ID, since
> we can assign IDs to VNC/SPICE server instances too, when there
> are multiple instances, and they'll be non-overlapping with
> chardev IDs ?
>
> IOW we make 'protocol' and 'id' both optional in QAPI schema, and
> declare them mutually exclusive. Deprecate 'protocol' in favour
> of 'id'. Then eventually delete 'protocol' and make 'id'
> mandatory.

This would work nicely if we had had the good sense to require IDs to be
globally unique.  As is, nothing stops you from naming one character
device, one vnc, and one spice configuration object each "foo".

Is the any appetite for fixing this?

>> >> Are there any known uses with character devices?
>> 
>> See [*] below.
>> 
>> >> If not, any ideas why one would want to use the command with character
>> >> devices?
>> >
>> > Ordinarily a client will directly connect() to QEMU to setup the
>> > socket connection. Depending on the protocol this may involve both
>> > TLS negotiation and authentication. This is a good thing when exposed
>> > over a public IP address. It is tedious when connecting from a local
>> > client though.
>> >
>> > The idea behind the 'add_client' method was to enable short circuiting
>> > of encryption and authentication, for local only clients. For example,
>> > virt-viewer/virt-manager can do socketpair() and pass one of the FDs
>> > across to QEMU, and bypass any VNC authentication. This is still secure,
>> > as FD passing is mediated by libvirt which the app has already
>> > authenticated against.
>> >
>> > This is conceptually useful for any backend exposed as a network
>> > socket, accepting ad-hoc client connections. So it is in scope for
>> > chardevs, nbd, vnc, spice.
>> 
>> Does libvirt implement this with socket character devices?
>
> Opps, I meant to say that libvirt only uses add_client for
> graphics devices. We've never used it for chardevs AFAICT.

Thanks!

>> [*] If yes, we have a known use.  Else we don't, I presume.

No known uses so far.

[...]