[Qemu-devel] [RFC PATCH 0/3] Use FD passing to accept new VNC/chardev clients

[Qemu-devel] [RFC PATCH 0/3] Use FD passing to accept new VNC/chardev clients

[Daniel P. Berrange]

The VNC server in QEMU can be configured using either TCP or UNIX
sockets. Historically, libvirt apps have configured VNC using TCP,
but restricted to localhost (127.0.0.1) by default. This allows
apps like virt-manager to connect, while not exposing it to the
outside world by default. The downside, is that in fact any local
user on the host can connect. This is undesirable for many types
of deployment.

The alternative option would be to configured QEMU to use a well
known UNIX socket by default. While this works fine if QEMU is
running as the same UID/GID as the local user desiring access
to VNC, it isn't so great if the client is a different UID to
QEMU, since it won't be able to access the UNIX socket.

One option would be to make the UNIX socket world accessible and
then integrate QEMU with policykit directly to grant access to
selected local users. This would in fact work, but some users
have rightfully pointed out that if they already have a connection
to libvirt they should be able to connect to QEMU VNC without
needing to authenticate further.

This last issue can be solved by making use of FD passing over
UNIX domain sockets. The application desiring access to the VNC
server has a connection to libvirt. The create an anoymous
unbound UNIX socketpair, and pass one of the FDs to libvirtd
using a new libvirt API. libvirtd (optionally) checks they are
allowed for the specific VM in question, and if so, issues a
QEMU monitor command, passing the socketpair FD onto QEMU.
The client app now has a direct connection to the QEMU VNC
server.

With this functionality the typical deployment configuration
would be to setup QEMU to run on a well known UNIX socket by
default. If the app can access this socket then they can use
it directly, otherwise if they have sufficient authorization
via libvirt, they can indirectly connect to VNC using this
new FD passing.

This patch series is not intended for merging right now. It
is just a proof of concept I'm sending in case anyone else
was looking for something like this. It supports

 - Passing an FD to connect to VNC (tested)
 - Passing an FD to connect to any chardev configured
   with a socket backend (not tested yet)

Not currently wired up, but desired before proposing for
merging:

 - Passing an FD to connect to SPICE

[Qemu-devel] [PATCH 1/3] Store VNC auth scheme per-client as well as per-server

[Daniel P. Berrange]

A future patch will introduce a situation where different
clients may have different authentication schemes set.
When a new client arrives, copy the 'auth' and 'subauth'
fields from VncDisplay into the client's VncState, and
use the latter in all authentication functions.

* ui/vnc.h: Add 'auth' and 'subauth' to VncState
* ui/vnc-auth-sasl.c, ui/vnc-auth-vencrypt.c,
  ui/vnc.c: Make auth functions pull auth scheme
  from VncState instead of VncDisplay
---
 ui/vnc-auth-sasl.c     |    8 ++++----
 ui/vnc-auth-vencrypt.c |   18 +++++++++---------
 ui/vnc.c               |   39 ++++++++++++++++++++++++++-------------
 ui/vnc.h               |    2 ++
 4 files changed, 41 insertions(+), 26 deletions(-)

diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index 17a621a..8aac5ec 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -538,8 +538,8 @@ void start_auth_sasl(VncState *vs)
 
 #ifdef CONFIG_VNC_TLS
     /* Inform SASL that we've got an external SSF layer from TLS/x509 */
-    if (vs->vd->auth == VNC_AUTH_VENCRYPT &&
-        vs->vd->subauth == VNC_AUTH_VENCRYPT_X509SASL) {
+    if (vs->auth == VNC_AUTH_VENCRYPT &&
+        vs->subauth == VNC_AUTH_VENCRYPT_X509SASL) {
         gnutls_cipher_algorithm_t cipher;
         sasl_ssf_t ssf;
 
@@ -570,8 +570,8 @@ void start_auth_sasl(VncState *vs)
 #ifdef CONFIG_VNC_TLS
         /* Disable SSF, if using TLS+x509+SASL only. TLS without x509
            is not sufficiently strong */
-        || (vs->vd->auth == VNC_AUTH_VENCRYPT &&
-            vs->vd->subauth == VNC_AUTH_VENCRYPT_X509SASL)
+        || (vs->auth == VNC_AUTH_VENCRYPT &&
+            vs->subauth == VNC_AUTH_VENCRYPT_X509SASL)
 #endif /* CONFIG_VNC_TLS */
         ) {
         /* If we've got TLS or UNIX domain sock, we don't care about SSF */
diff --git a/ui/vnc-auth-vencrypt.c b/ui/vnc-auth-vencrypt.c
index 07c1691..674ba97 100644
--- a/ui/vnc-auth-vencrypt.c
+++ b/ui/vnc-auth-vencrypt.c
@@ -29,7 +29,7 @@
 
 static void start_auth_vencrypt_subauth(VncState *vs)
 {
-    switch (vs->vd->subauth) {
+    switch (vs->subauth) {
     case VNC_AUTH_VENCRYPT_TLSNONE:
     case VNC_AUTH_VENCRYPT_X509NONE:
        VNC_DEBUG("Accept TLS auth none\n");
@@ -51,7 +51,7 @@ static void start_auth_vencrypt_subauth(VncState *vs)
 #endif /* CONFIG_VNC_SASL */
 
     default: /* Should not be possible, but just in case */
-       VNC_DEBUG("Reject subauth %d server bug\n", vs->vd->auth);
+       VNC_DEBUG("Reject subauth %d server bug\n", vs->auth);
        vnc_write_u8(vs, 1);
        if (vs->minor >= 8) {
            static const char err[] = "Unsupported authentication type";
@@ -110,17 +110,17 @@ static void vnc_tls_handshake_io(void *opaque) {
 
 
 #define NEED_X509_AUTH(vs)                              \
-    ((vs)->vd->subauth == VNC_AUTH_VENCRYPT_X509NONE ||   \
-     (vs)->vd->subauth == VNC_AUTH_VENCRYPT_X509VNC ||    \
-     (vs)->vd->subauth == VNC_AUTH_VENCRYPT_X509PLAIN ||  \
-     (vs)->vd->subauth == VNC_AUTH_VENCRYPT_X509SASL)
+    ((vs)->subauth == VNC_AUTH_VENCRYPT_X509NONE ||   \
+     (vs)->subauth == VNC_AUTH_VENCRYPT_X509VNC ||    \
+     (vs)->subauth == VNC_AUTH_VENCRYPT_X509PLAIN ||  \
+     (vs)->subauth == VNC_AUTH_VENCRYPT_X509SASL)
 
 
 static int protocol_client_vencrypt_auth(VncState *vs, uint8_t *data, size_t len)
 {
     int auth = read_u32(data, 0);
 
-    if (auth != vs->vd->subauth) {
+    if (auth != vs->subauth) {
         VNC_DEBUG("Rejecting auth %d\n", auth);
         vnc_write_u8(vs, 0); /* Reject auth */
         vnc_flush(vs);
@@ -153,10 +153,10 @@ static int protocol_client_vencrypt_init(VncState *vs, uint8_t *data, size_t len
         vnc_flush(vs);
         vnc_client_error(vs);
     } else {
-        VNC_DEBUG("Sending allowed auth %d\n", vs->vd->subauth);
+        VNC_DEBUG("Sending allowed auth %d\n", vs->subauth);
         vnc_write_u8(vs, 0); /* Accept version */
         vnc_write_u8(vs, 1); /* Number of sub-auths */
-        vnc_write_u32(vs, vs->vd->subauth); /* The supported auth */
+        vnc_write_u32(vs, vs->subauth); /* The supported auth */
         vnc_flush(vs);
         vnc_read_when(vs, protocol_client_vencrypt_auth, 4);
     }
diff --git a/ui/vnc.c b/ui/vnc.c
index 14f2930..39b5b51 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2124,7 +2124,7 @@ static int protocol_client_auth(VncState *vs, uint8_t *data, size_t len)
 {
     /* We only advertise 1 auth scheme at a time, so client
      * must pick the one we sent. Verify this */
-    if (data[0] != vs->vd->auth) { /* Reject auth */
+    if (data[0] != vs->auth) { /* Reject auth */
        VNC_DEBUG("Reject auth %d because it didn't match advertized\n", (int)data[0]);
        vnc_write_u32(vs, 1);
        if (vs->minor >= 8) {
@@ -2135,7 +2135,7 @@ static int protocol_client_auth(VncState *vs, uint8_t *data, size_t len)
        vnc_client_error(vs);
     } else { /* Accept requested auth */
        VNC_DEBUG("Client requested auth %d\n", (int)data[0]);
-       switch (vs->vd->auth) {
+       switch (vs->auth) {
        case VNC_AUTH_NONE:
            VNC_DEBUG("Accept auth none\n");
            if (vs->minor >= 8) {
@@ -2165,7 +2165,7 @@ static int protocol_client_auth(VncState *vs, uint8_t *data, size_t len)
 #endif /* CONFIG_VNC_SASL */
 
        default: /* Should not be possible, but just in case */
-           VNC_DEBUG("Reject auth %d server code bug\n", vs->vd->auth);
+           VNC_DEBUG("Reject auth %d server code bug\n", vs->auth);
            vnc_write_u8(vs, 1);
            if (vs->minor >= 8) {
                static const char err[] = "Authentication failed";
@@ -2210,26 +2210,26 @@ static int protocol_version(VncState *vs, uint8_t *version, size_t len)
         vs->minor = 3;
 
     if (vs->minor == 3) {
-        if (vs->vd->auth == VNC_AUTH_NONE) {
+        if (vs->auth == VNC_AUTH_NONE) {
             VNC_DEBUG("Tell client auth none\n");
-            vnc_write_u32(vs, vs->vd->auth);
+            vnc_write_u32(vs, vs->auth);
             vnc_flush(vs);
             start_client_init(vs);
-       } else if (vs->vd->auth == VNC_AUTH_VNC) {
+       } else if (vs->auth == VNC_AUTH_VNC) {
             VNC_DEBUG("Tell client VNC auth\n");
-            vnc_write_u32(vs, vs->vd->auth);
+            vnc_write_u32(vs, vs->auth);
             vnc_flush(vs);
             start_auth_vnc(vs);
        } else {
-            VNC_DEBUG("Unsupported auth %d for protocol 3.3\n", vs->vd->auth);
+            VNC_DEBUG("Unsupported auth %d for protocol 3.3\n", vs->auth);
             vnc_write_u32(vs, VNC_AUTH_INVALID);
             vnc_flush(vs);
             vnc_client_error(vs);
        }
     } else {
-        VNC_DEBUG("Telling client we support auth %d\n", vs->vd->auth);
+        VNC_DEBUG("Telling client we support auth %d\n", vs->auth);
         vnc_write_u8(vs, 1); /* num auth */
-        vnc_write_u8(vs, vs->vd->auth);
+        vnc_write_u8(vs, vs->auth);
         vnc_read_when(vs, protocol_client_auth, 1);
         vnc_flush(vs);
     }
@@ -2494,12 +2494,25 @@ static void vnc_remove_timer(VncDisplay *vd)
     }
 }
 
-static void vnc_connect(VncDisplay *vd, int csock)
+static void vnc_connect(VncDisplay *vd, int csock, int skipauth)
 {
     VncState *vs = qemu_mallocz(sizeof(VncState));
     int i;
 
     vs->csock = csock;
+
+    if (skipauth) {
+	vs->auth = VNC_AUTH_NONE;
+#ifdef CONFIG_VNC_TLS
+	vs->subauth = VNC_AUTH_INVALID;
+#endif
+    } else {
+	vs->auth = vd->auth;
+#ifdef CONFIG_VNC_TLS
+	vs->subauth = vd->subauth;
+#endif
+    }
+
     vs->lossy_rect = qemu_mallocz(VNC_STAT_ROWS * sizeof (*vs->lossy_rect));
     for (i = 0; i < VNC_STAT_ROWS; ++i) {
         vs->lossy_rect[i] = qemu_mallocz(VNC_STAT_COLS * sizeof (uint8_t));
@@ -2557,7 +2570,7 @@ static void vnc_listen_read(void *opaque)
 
     int csock = qemu_accept(vs->lsock, (struct sockaddr *)&addr, &addrlen);
     if (csock != -1) {
-        vnc_connect(vs, csock);
+        vnc_connect(vs, csock, 0);
     }
 }
 
@@ -2887,7 +2900,7 @@ int vnc_display_open(DisplayState *ds, const char *display)
         } else {
             int csock = vs->lsock;
             vs->lsock = -1;
-            vnc_connect(vs, csock);
+            vnc_connect(vs, csock, 0);
         }
         return 0;
 
diff --git a/ui/vnc.h b/ui/vnc.h
index f10c5dc..66689f1 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -256,8 +256,10 @@ struct VncState
     int major;
     int minor;
 
+    int auth;
     char challenge[VNC_AUTH_CHALLENGE_SIZE];
 #ifdef CONFIG_VNC_TLS
+    int subauth; /* Used by VeNCrypt */
     VncStateTLS tls;
 #endif
 #ifdef CONFIG_VNC_SASL
-- 
1.7.4.4

[Qemu-devel] [PATCH 2/3] Introduce a 'client_add' monitor command accepting an open FD

[Daniel P. Berrange]

Allow client connections for VNC and socket based character
devices to be passed in over the monitor using SCM_RIGHTS.

One intended usage scenario is to start QEMU with VNC on a
UNIX domain socket. An unprivileged user which cannot access
the UNIX domain socket, can then connect to QEMU's VNC server
by passing an open FD to libvirt, which passes it onto QEMU.

 { "execute": "get_fd", "arguments": { "fdname": "myclient" } }
 { "return": {} }
 { "execute": "add_client", "arguments": { "protocol": "vnc",
                                           "fdname": "myclient",
                                           "skipauth": true } }
 { "return": {} }

In this case 'protocol' can be 'vnc' or 'spice', or the name
of a character device (eg from -chardev id=XXXX)

The 'skipauth' parameter can be used to skip any configured
VNC authentication scheme, which is useful if the mgmt layer
talking to the monitor has already authenticated the client
in another way.

* console.h: Define 'vnc_display_add_client' method
* monitor.c: Implement 'client_add' command
* qemu-char.c, qemu-char.h: Add 'qemu_char_add_client' method
* qerror.c, qerror.h: Add QERR_ADD_CLIENT_FAILED
* qmp-commands.hx: Declare 'client_add' command
* ui/vnc.c: Implement 'vnc_display_add_client' method
---
 console.h       |    1 +
 monitor.c       |   32 ++++++++++++++++++++++++++++++++
 qemu-char.c     |   30 ++++++++++++++++++++++++------
 qemu-char.h     |    2 ++
 qerror.c        |    4 ++++
 qerror.h        |    3 +++
 qmp-commands.hx |   27 +++++++++++++++++++++++++++
 ui/vnc.c        |    7 +++++++
 8 files changed, 100 insertions(+), 6 deletions(-)

diff --git a/console.h b/console.h
index 64d1f09..84d3e8d 100644
--- a/console.h
+++ b/console.h
@@ -372,6 +372,7 @@ void cocoa_display_init(DisplayState *ds, int full_screen);
 void vnc_display_init(DisplayState *ds);
 void vnc_display_close(DisplayState *ds);
 int vnc_display_open(DisplayState *ds, const char *display);
+void vnc_display_add_client(DisplayState *ds, int csock, int skipauth);
 int vnc_display_disable_login(DisplayState *ds);
 char *vnc_display_local_addr(DisplayState *ds);
 #ifdef CONFIG_VNC
diff --git a/monitor.c b/monitor.c
index 6af6a4d..23c310e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1185,6 +1185,38 @@ static int expire_password(Monitor *mon, const QDict *qdict, QObject **ret_data)
     return -1;
 }
 
+static int add_graphics_client(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    const char *protocol  = qdict_get_str(qdict, "protocol");
+    const char *fdname = qdict_get_str(qdict, "fdname");
+    int skipauth = qdict_get_try_bool(qdict, "skipauth", 0);
+    CharDriverState *s;
+
+    if (strcmp(protocol, "spice") == 0) {
+        if (!using_spice) {
+            /* correct one? spice isn't a device ,,, */
+            qerror_report(QERR_DEVICE_NOT_ACTIVE, "spice");
+            return -1;
+        }
+	qerror_report(QERR_ADD_CLIENT_FAILED);
+	return -1;
+    } else if (strcmp(protocol, "vnc") == 0) {
+	int fd = monitor_get_fd(mon, fdname);
+	vnc_display_add_client(NULL, fd, skipauth);
+	return 0;
+    } else if ((s = qemu_chr_find(protocol)) != NULL) {
+	int fd = monitor_get_fd(mon, fdname);
+	if (qemu_chr_add_client(s, fd) < 0) {
+	    qerror_report(QERR_ADD_CLIENT_FAILED);
+	    return -1;
+	}
+	return 0;
+    }
+
+    qerror_report(QERR_INVALID_PARAMETER, "protocol");
+    return -1;
+}
+
 static int client_migrate_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     const char *protocol = qdict_get_str(qdict, "protocol");
diff --git a/qemu-char.c b/qemu-char.c
index fb13b28..4e168ee 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -168,6 +168,11 @@ int qemu_chr_get_msgfd(CharDriverState *s)
     return s->get_msgfd ? s->get_msgfd(s) : -1;
 }
 
+int qemu_chr_add_client(CharDriverState *s, int fd)
+{
+    return s->chr_add_client ? s->chr_add_client(s, fd) : -1;
+}
+
 void qemu_chr_accept_input(CharDriverState *s)
 {
     if (s->chr_accept_input)
@@ -2123,6 +2128,22 @@ static void socket_set_nodelay(int fd)
     setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (char *)&val, sizeof(val));
 }
 
+static int tcp_chr_add_client(CharDriverState *chr, int fd)
+{
+    TCPCharDriver *s = chr->opaque;
+    if (s->fd != -1)
+	return -1;
+
+    socket_set_nonblock(fd);
+    if (s->do_nodelay)
+        socket_set_nodelay(fd);
+    s->fd = fd;
+    qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
+    tcp_chr_connect(chr);
+
+    return 0;
+}
+
 static void tcp_chr_accept(void *opaque)
 {
     CharDriverState *chr = opaque;
@@ -2155,12 +2176,8 @@ static void tcp_chr_accept(void *opaque)
             break;
         }
     }
-    socket_set_nonblock(fd);
-    if (s->do_nodelay)
-        socket_set_nodelay(fd);
-    s->fd = fd;
-    qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
-    tcp_chr_connect(chr);
+    if (tcp_chr_add_client(chr, fd) < 0)
+	close(fd);
 }
 
 static void tcp_chr_close(CharDriverState *chr)
@@ -2230,6 +2247,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
     chr->chr_write = tcp_chr_write;
     chr->chr_close = tcp_chr_close;
     chr->get_msgfd = tcp_get_msgfd;
+    chr->chr_add_client = tcp_chr_add_client;
 
     if (is_listen) {
         s->listen_fd = fd;
diff --git a/qemu-char.h b/qemu-char.h
index 892c6da..f361c6d 100644
--- a/qemu-char.h
+++ b/qemu-char.h
@@ -57,6 +57,7 @@ struct CharDriverState {
     void (*chr_update_read_handler)(struct CharDriverState *s);
     int (*chr_ioctl)(struct CharDriverState *s, int cmd, void *arg);
     int (*get_msgfd)(struct CharDriverState *s);
+    int (*chr_add_client)(struct CharDriverState *chr, int fd);
     IOEventHandler *chr_event;
     IOCanReadHandler *chr_can_read;
     IOReadHandler *chr_read;
@@ -99,6 +100,7 @@ int qemu_chr_can_read(CharDriverState *s);
 void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len);
 int qemu_chr_get_msgfd(CharDriverState *s);
 void qemu_chr_accept_input(CharDriverState *s);
+int qemu_chr_add_client(CharDriverState *s, int fd);
 void qemu_chr_info_print(Monitor *mon, const QObject *ret_data);
 void qemu_chr_info(Monitor *mon, QObject **ret_data);
 CharDriverState *qemu_chr_find(const char *name);
diff --git a/qerror.c b/qerror.c
index d7fcd93..79be1ba 100644
--- a/qerror.c
+++ b/qerror.c
@@ -193,6 +193,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Could not set password",
     },
     {
+        .error_fmt = QERR_ADD_CLIENT_FAILED,
+        .desc      = "Could not add client",
+    },
+    {
         .error_fmt = QERR_TOO_MANY_FILES,
         .desc      = "Too many open files",
     },
diff --git a/qerror.h b/qerror.h
index 16c830d..25773cd 100644
--- a/qerror.h
+++ b/qerror.h
@@ -163,6 +163,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_SET_PASSWD_FAILED \
     "{ 'class': 'SetPasswdFailed', 'data': {} }"
 
+#define QERR_ADD_CLIENT_FAILED \
+    "{ 'class': 'AddClientFailed', 'data': {} }"
+
 #define QERR_TOO_MANY_FILES \
     "{ 'class': 'TooManyFiles', 'data': {} }"
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 92c5c3a..1e19abc 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -885,6 +885,33 @@ Example:
 EQMP
 
     {
+        .name       = "add_client",
+        .args_type  = "protocol:s,fdname:s,skipauth:b?",
+        .params     = "protocol fdname skipauth",
+        .help       = "add a graphics client",
+        .user_print = monitor_user_noop,
+        .mhandler.cmd_new = add_graphics_client,
+    },
+
+SQMP
+add_client
+----------
+
+Add a graphics client
+
+Arguments:
+
+- "protocol": protocol name (json-string)
+- "fdname": file descriptor name (json-string)
+
+Example:
+
+-> { "execute": "add_client", "arguments": { "protocol": "vnc",
+                                             "fdname": "myclient" } }
+<- { "return": {} }
+
+EQMP
+    {
         .name       = "qmp_capabilities",
         .args_type  = "",
         .params     = "",
diff --git a/ui/vnc.c b/ui/vnc.c
index 39b5b51..8602adc 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -2924,3 +2924,10 @@ int vnc_display_open(DisplayState *ds, const char *display)
     }
     return qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_read, NULL, vs);
 }
+
+void vnc_display_add_client(DisplayState *ds, int csock, int skipauth)
+{
+    VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
+
+    return vnc_connect(vs, csock, skipauth);
+}
-- 
1.7.4.4

[Qemu-devel] [PATCH 3/3] Remove unused USES_X509_AUTH macro from VNC sasl code

[Daniel P. Berrange]

The USES_X509_AUTH macro is defined in several VNC files,
but not used in all of them. Remove the unused definition.

* ui/vnc-auth-sasl.c: Remove USES_X509_AUTH macro
---
 ui/vnc-auth-sasl.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index 8aac5ec..15af49b 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -491,13 +491,6 @@ static int protocol_client_auth_sasl_mechname_len(VncState *vs, uint8_t *data, s
     return 0;
 }
 
-#define USES_X509_AUTH(vs)                              \
-    ((vs)->subauth == VNC_AUTH_VENCRYPT_X509NONE ||   \
-     (vs)->subauth == VNC_AUTH_VENCRYPT_X509VNC ||    \
-     (vs)->subauth == VNC_AUTH_VENCRYPT_X509PLAIN ||  \
-     (vs)->subauth == VNC_AUTH_VENCRYPT_X509SASL)
-
-
 void start_auth_sasl(VncState *vs)
 {
     const char *mechlist = NULL;
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 1/3] Store VNC auth scheme per-client as well as per-server

[Anthony Liguori]

On 06/23/2011 07:31 AM, Daniel P. Berrange wrote:
> A future patch will introduce a situation where different
> clients may have different authentication schemes set.
> When a new client arrives, copy the 'auth' and 'subauth'
> fields from VncDisplay into the client's VncState, and
> use the latter in all authentication functions.
>
> * ui/vnc.h: Add 'auth' and 'subauth' to VncState
> * ui/vnc-auth-sasl.c, ui/vnc-auth-vencrypt.c,
>    ui/vnc.c: Make auth functions pull auth scheme
>    from VncState instead of VncDisplay
> ---
>   ui/vnc-auth-sasl.c     |    8 ++++----
>   ui/vnc-auth-vencrypt.c |   18 +++++++++---------
>   ui/vnc.c               |   39 ++++++++++++++++++++++++++-------------
>   ui/vnc.h               |    2 ++
>   4 files changed, 41 insertions(+), 26 deletions(-)

Applied.  Thanks.

Regards,

Anthony Liguori

>
> diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
> index 17a621a..8aac5ec 100644
> --- a/ui/vnc-auth-sasl.c
> +++ b/ui/vnc-auth-sasl.c
> @@ -538,8 +538,8 @@ void start_auth_sasl(VncState *vs)
>
>   #ifdef CONFIG_VNC_TLS
>       /* Inform SASL that we've got an external SSF layer from TLS/x509 */
> -    if (vs->vd->auth == VNC_AUTH_VENCRYPT&&
> -        vs->vd->subauth == VNC_AUTH_VENCRYPT_X509SASL) {
> +    if (vs->auth == VNC_AUTH_VENCRYPT&&
> +        vs->subauth == VNC_AUTH_VENCRYPT_X509SASL) {
>           gnutls_cipher_algorithm_t cipher;
>           sasl_ssf_t ssf;
>
> @@ -570,8 +570,8 @@ void start_auth_sasl(VncState *vs)
>   #ifdef CONFIG_VNC_TLS
>           /* Disable SSF, if using TLS+x509+SASL only. TLS without x509
>              is not sufficiently strong */
> -        || (vs->vd->auth == VNC_AUTH_VENCRYPT&&
> -            vs->vd->subauth == VNC_AUTH_VENCRYPT_X509SASL)
> +        || (vs->auth == VNC_AUTH_VENCRYPT&&
> +            vs->subauth == VNC_AUTH_VENCRYPT_X509SASL)
>   #endif /* CONFIG_VNC_TLS */
>           ) {
>           /* If we've got TLS or UNIX domain sock, we don't care about SSF */
> diff --git a/ui/vnc-auth-vencrypt.c b/ui/vnc-auth-vencrypt.c
> index 07c1691..674ba97 100644
> --- a/ui/vnc-auth-vencrypt.c
> +++ b/ui/vnc-auth-vencrypt.c
> @@ -29,7 +29,7 @@
>
>   static void start_auth_vencrypt_subauth(VncState *vs)
>   {
> -    switch (vs->vd->subauth) {
> +    switch (vs->subauth) {
>       case VNC_AUTH_VENCRYPT_TLSNONE:
>       case VNC_AUTH_VENCRYPT_X509NONE:
>          VNC_DEBUG("Accept TLS auth none\n");
> @@ -51,7 +51,7 @@ static void start_auth_vencrypt_subauth(VncState *vs)
>   #endif /* CONFIG_VNC_SASL */
>
>       default: /* Should not be possible, but just in case */
> -       VNC_DEBUG("Reject subauth %d server bug\n", vs->vd->auth);
> +       VNC_DEBUG("Reject subauth %d server bug\n", vs->auth);
>          vnc_write_u8(vs, 1);
>          if (vs->minor>= 8) {
>              static const char err[] = "Unsupported authentication type";
> @@ -110,17 +110,17 @@ static void vnc_tls_handshake_io(void *opaque) {
>
>
>   #define NEED_X509_AUTH(vs)                              \
> -    ((vs)->vd->subauth == VNC_AUTH_VENCRYPT_X509NONE ||   \
> -     (vs)->vd->subauth == VNC_AUTH_VENCRYPT_X509VNC ||    \
> -     (vs)->vd->subauth == VNC_AUTH_VENCRYPT_X509PLAIN ||  \
> -     (vs)->vd->subauth == VNC_AUTH_VENCRYPT_X509SASL)
> +    ((vs)->subauth == VNC_AUTH_VENCRYPT_X509NONE ||   \
> +     (vs)->subauth == VNC_AUTH_VENCRYPT_X509VNC ||    \
> +     (vs)->subauth == VNC_AUTH_VENCRYPT_X509PLAIN ||  \
> +     (vs)->subauth == VNC_AUTH_VENCRYPT_X509SASL)
>
>
>   static int protocol_client_vencrypt_auth(VncState *vs, uint8_t *data, size_t len)
>   {
>       int auth = read_u32(data, 0);
>
> -    if (auth != vs->vd->subauth) {
> +    if (auth != vs->subauth) {
>           VNC_DEBUG("Rejecting auth %d\n", auth);
>           vnc_write_u8(vs, 0); /* Reject auth */
>           vnc_flush(vs);
> @@ -153,10 +153,10 @@ static int protocol_client_vencrypt_init(VncState *vs, uint8_t *data, size_t len
>           vnc_flush(vs);
>           vnc_client_error(vs);
>       } else {
> -        VNC_DEBUG("Sending allowed auth %d\n", vs->vd->subauth);
> +        VNC_DEBUG("Sending allowed auth %d\n", vs->subauth);
>           vnc_write_u8(vs, 0); /* Accept version */
>           vnc_write_u8(vs, 1); /* Number of sub-auths */
> -        vnc_write_u32(vs, vs->vd->subauth); /* The supported auth */
> +        vnc_write_u32(vs, vs->subauth); /* The supported auth */
>           vnc_flush(vs);
>           vnc_read_when(vs, protocol_client_vencrypt_auth, 4);
>       }
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 14f2930..39b5b51 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2124,7 +2124,7 @@ static int protocol_client_auth(VncState *vs, uint8_t *data, size_t len)
>   {
>       /* We only advertise 1 auth scheme at a time, so client
>        * must pick the one we sent. Verify this */
> -    if (data[0] != vs->vd->auth) { /* Reject auth */
> +    if (data[0] != vs->auth) { /* Reject auth */
>          VNC_DEBUG("Reject auth %d because it didn't match advertized\n", (int)data[0]);
>          vnc_write_u32(vs, 1);
>          if (vs->minor>= 8) {
> @@ -2135,7 +2135,7 @@ static int protocol_client_auth(VncState *vs, uint8_t *data, size_t len)
>          vnc_client_error(vs);
>       } else { /* Accept requested auth */
>          VNC_DEBUG("Client requested auth %d\n", (int)data[0]);
> -       switch (vs->vd->auth) {
> +       switch (vs->auth) {
>          case VNC_AUTH_NONE:
>              VNC_DEBUG("Accept auth none\n");
>              if (vs->minor>= 8) {
> @@ -2165,7 +2165,7 @@ static int protocol_client_auth(VncState *vs, uint8_t *data, size_t len)
>   #endif /* CONFIG_VNC_SASL */
>
>          default: /* Should not be possible, but just in case */
> -           VNC_DEBUG("Reject auth %d server code bug\n", vs->vd->auth);
> +           VNC_DEBUG("Reject auth %d server code bug\n", vs->auth);
>              vnc_write_u8(vs, 1);
>              if (vs->minor>= 8) {
>                  static const char err[] = "Authentication failed";
> @@ -2210,26 +2210,26 @@ static int protocol_version(VncState *vs, uint8_t *version, size_t len)
>           vs->minor = 3;
>
>       if (vs->minor == 3) {
> -        if (vs->vd->auth == VNC_AUTH_NONE) {
> +        if (vs->auth == VNC_AUTH_NONE) {
>               VNC_DEBUG("Tell client auth none\n");
> -            vnc_write_u32(vs, vs->vd->auth);
> +            vnc_write_u32(vs, vs->auth);
>               vnc_flush(vs);
>               start_client_init(vs);
> -       } else if (vs->vd->auth == VNC_AUTH_VNC) {
> +       } else if (vs->auth == VNC_AUTH_VNC) {
>               VNC_DEBUG("Tell client VNC auth\n");
> -            vnc_write_u32(vs, vs->vd->auth);
> +            vnc_write_u32(vs, vs->auth);
>               vnc_flush(vs);
>               start_auth_vnc(vs);
>          } else {
> -            VNC_DEBUG("Unsupported auth %d for protocol 3.3\n", vs->vd->auth);
> +            VNC_DEBUG("Unsupported auth %d for protocol 3.3\n", vs->auth);
>               vnc_write_u32(vs, VNC_AUTH_INVALID);
>               vnc_flush(vs);
>               vnc_client_error(vs);
>          }
>       } else {
> -        VNC_DEBUG("Telling client we support auth %d\n", vs->vd->auth);
> +        VNC_DEBUG("Telling client we support auth %d\n", vs->auth);
>           vnc_write_u8(vs, 1); /* num auth */
> -        vnc_write_u8(vs, vs->vd->auth);
> +        vnc_write_u8(vs, vs->auth);
>           vnc_read_when(vs, protocol_client_auth, 1);
>           vnc_flush(vs);
>       }
> @@ -2494,12 +2494,25 @@ static void vnc_remove_timer(VncDisplay *vd)
>       }
>   }
>
> -static void vnc_connect(VncDisplay *vd, int csock)
> +static void vnc_connect(VncDisplay *vd, int csock, int skipauth)
>   {
>       VncState *vs = qemu_mallocz(sizeof(VncState));
>       int i;
>
>       vs->csock = csock;
> +
> +    if (skipauth) {
> +	vs->auth = VNC_AUTH_NONE;
> +#ifdef CONFIG_VNC_TLS
> +	vs->subauth = VNC_AUTH_INVALID;
> +#endif
> +    } else {
> +	vs->auth = vd->auth;
> +#ifdef CONFIG_VNC_TLS
> +	vs->subauth = vd->subauth;
> +#endif
> +    }
> +
>       vs->lossy_rect = qemu_mallocz(VNC_STAT_ROWS * sizeof (*vs->lossy_rect));
>       for (i = 0; i<  VNC_STAT_ROWS; ++i) {
>           vs->lossy_rect[i] = qemu_mallocz(VNC_STAT_COLS * sizeof (uint8_t));
> @@ -2557,7 +2570,7 @@ static void vnc_listen_read(void *opaque)
>
>       int csock = qemu_accept(vs->lsock, (struct sockaddr *)&addr,&addrlen);
>       if (csock != -1) {
> -        vnc_connect(vs, csock);
> +        vnc_connect(vs, csock, 0);
>       }
>   }
>
> @@ -2887,7 +2900,7 @@ int vnc_display_open(DisplayState *ds, const char *display)
>           } else {
>               int csock = vs->lsock;
>               vs->lsock = -1;
> -            vnc_connect(vs, csock);
> +            vnc_connect(vs, csock, 0);
>           }
>           return 0;
>
> diff --git a/ui/vnc.h b/ui/vnc.h
> index f10c5dc..66689f1 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -256,8 +256,10 @@ struct VncState
>       int major;
>       int minor;
>
> +    int auth;
>       char challenge[VNC_AUTH_CHALLENGE_SIZE];
>   #ifdef CONFIG_VNC_TLS
> +    int subauth; /* Used by VeNCrypt */
>       VncStateTLS tls;
>   #endif
>   #ifdef CONFIG_VNC_SASL

Re: [Qemu-devel] [PATCH 2/3] Introduce a 'client_add' monitor command accepting an open FD

[Fabien Chouteau]

On 23/06/2011 14:31, Daniel P. Berrange wrote:
> Allow client connections for VNC and socket based character
> devices to be passed in over the monitor using SCM_RIGHTS.
> 

Hello Daniel,

I'm a little bit late but it seems that there's a build error with this patch
when VNC is disabled.

$./configure --target-list=ppc-softmmu --disable-vnc
...
$ make
...
monitor.o: In function `add_graphics_client':
.../monitor.c:1205: undefined reference to `vnc_display_add_client'

Regards,

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH 2/3] Introduce a 'client_add' monitor command accepting an open FD

[Daniel P. Berrange]

On Tue, Jul 26, 2011 at 05:20:16PM +0200, Fabien Chouteau wrote:
> On 23/06/2011 14:31, Daniel P. Berrange wrote:
> > Allow client connections for VNC and socket based character
> > devices to be passed in over the monitor using SCM_RIGHTS.
> > 
> 
> Hello Daniel,
> 
> I'm a little bit late but it seems that there's a build error with this patch
> when VNC is disabled.
> 
> $./configure --target-list=ppc-softmmu --disable-vnc
> ...
> $ make
> ...
> monitor.o: In function `add_graphics_client':
> .../monitor.c:1205: undefined reference to `vnc_display_add_client'

There was already a patch posted yesterday which should fix this
problem:

  Subject: [PATCH] monitor: fix build breakage with --disable-vnc

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 2/3] Introduce a 'client_add' monitor command accepting an open FD

[Fabien Chouteau]

On 26/07/2011 17:29, Daniel P. Berrange wrote:
> On Tue, Jul 26, 2011 at 05:20:16PM +0200, Fabien Chouteau wrote:
>> On 23/06/2011 14:31, Daniel P. Berrange wrote:
>>> Allow client connections for VNC and socket based character
>>> devices to be passed in over the monitor using SCM_RIGHTS.
>>>
>>
>> Hello Daniel,
>>
>> I'm a little bit late but it seems that there's a build error with this patch
>> when VNC is disabled.
>>
>> $./configure --target-list=ppc-softmmu --disable-vnc
>> ...
>> $ make
>> ...
>> monitor.o: In function `add_graphics_client':
>> .../monitor.c:1205: undefined reference to `vnc_display_add_client'
> 
> There was already a patch posted yesterday which should fix this
> problem:
> 
>   Subject: [PATCH] monitor: fix build breakage with --disable-vnc
> 

OK, thanks.

Sorry about the noise...

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH 2/3] Introduce a 'client_add' monitor command accepting an open FD

[Anthony Liguori]

On 06/23/2011 07:31 AM, Daniel P. Berrange wrote:
> Allow client connections for VNC and socket based character
> devices to be passed in over the monitor using SCM_RIGHTS.
>
> One intended usage scenario is to start QEMU with VNC on a
> UNIX domain socket. An unprivileged user which cannot access
> the UNIX domain socket, can then connect to QEMU's VNC server
> by passing an open FD to libvirt, which passes it onto QEMU.
>
>   { "execute": "get_fd", "arguments": { "fdname": "myclient" } }
>   { "return": {} }
>   { "execute": "add_client", "arguments": { "protocol": "vnc",
>                                             "fdname": "myclient",
>                                             "skipauth": true } }
>   { "return": {} }
>
> In this case 'protocol' can be 'vnc' or 'spice', or the name
> of a character device (eg from -chardev id=XXXX)
>
> The 'skipauth' parameter can be used to skip any configured
> VNC authentication scheme, which is useful if the mgmt layer
> talking to the monitor has already authenticated the client
> in another way.
>
> * console.h: Define 'vnc_display_add_client' method
> * monitor.c: Implement 'client_add' command
> * qemu-char.c, qemu-char.h: Add 'qemu_char_add_client' method

I don't feel all that good about this anymore.  The notion of adding a 
fd to an arbitrary character device is a big layering violation and 
doesn't make much sense conceptually.

A chardev cannot have multiple connections.  It seems like the use-case 
is much better suited to having an fd: protocol to create char devices with.

I'd like to partially revert this removing the qemu_chr_add_client 
interface.

Regards,

Anthony Liguori

> * qerror.c, qerror.h: Add QERR_ADD_CLIENT_FAILED
> * qmp-commands.hx: Declare 'client_add' command
> * ui/vnc.c: Implement 'vnc_display_add_client' method
> ---
>   console.h       |    1 +
>   monitor.c       |   32 ++++++++++++++++++++++++++++++++
>   qemu-char.c     |   30 ++++++++++++++++++++++++------
>   qemu-char.h     |    2 ++
>   qerror.c        |    4 ++++
>   qerror.h        |    3 +++
>   qmp-commands.hx |   27 +++++++++++++++++++++++++++
>   ui/vnc.c        |    7 +++++++
>   8 files changed, 100 insertions(+), 6 deletions(-)
>
> diff --git a/console.h b/console.h
> index 64d1f09..84d3e8d 100644
> --- a/console.h
> +++ b/console.h
> @@ -372,6 +372,7 @@ void cocoa_display_init(DisplayState *ds, int full_screen);
>   void vnc_display_init(DisplayState *ds);
>   void vnc_display_close(DisplayState *ds);
>   int vnc_display_open(DisplayState *ds, const char *display);
> +void vnc_display_add_client(DisplayState *ds, int csock, int skipauth);
>   int vnc_display_disable_login(DisplayState *ds);
>   char *vnc_display_local_addr(DisplayState *ds);
>   #ifdef CONFIG_VNC
> diff --git a/monitor.c b/monitor.c
> index 6af6a4d..23c310e 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1185,6 +1185,38 @@ static int expire_password(Monitor *mon, const QDict *qdict, QObject **ret_data)
>       return -1;
>   }
>
> +static int add_graphics_client(Monitor *mon, const QDict *qdict, QObject **ret_data)
> +{
> +    const char *protocol  = qdict_get_str(qdict, "protocol");
> +    const char *fdname = qdict_get_str(qdict, "fdname");
> +    int skipauth = qdict_get_try_bool(qdict, "skipauth", 0);
> +    CharDriverState *s;
> +
> +    if (strcmp(protocol, "spice") == 0) {
> +        if (!using_spice) {
> +            /* correct one? spice isn't a device ,,, */
> +            qerror_report(QERR_DEVICE_NOT_ACTIVE, "spice");
> +            return -1;
> +        }
> +	qerror_report(QERR_ADD_CLIENT_FAILED);
> +	return -1;
> +    } else if (strcmp(protocol, "vnc") == 0) {
> +	int fd = monitor_get_fd(mon, fdname);
> +	vnc_display_add_client(NULL, fd, skipauth);
> +	return 0;
> +    } else if ((s = qemu_chr_find(protocol)) != NULL) {
> +	int fd = monitor_get_fd(mon, fdname);
> +	if (qemu_chr_add_client(s, fd)<  0) {
> +	    qerror_report(QERR_ADD_CLIENT_FAILED);
> +	    return -1;
> +	}
> +	return 0;
> +    }
> +
> +    qerror_report(QERR_INVALID_PARAMETER, "protocol");
> +    return -1;
> +}
> +
>   static int client_migrate_info(Monitor *mon, const QDict *qdict, QObject **ret_data)
>   {
>       const char *protocol = qdict_get_str(qdict, "protocol");
> diff --git a/qemu-char.c b/qemu-char.c
> index fb13b28..4e168ee 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -168,6 +168,11 @@ int qemu_chr_get_msgfd(CharDriverState *s)
>       return s->get_msgfd ? s->get_msgfd(s) : -1;
>   }
>
> +int qemu_chr_add_client(CharDriverState *s, int fd)
> +{
> +    return s->chr_add_client ? s->chr_add_client(s, fd) : -1;
> +}
> +
>   void qemu_chr_accept_input(CharDriverState *s)
>   {
>       if (s->chr_accept_input)
> @@ -2123,6 +2128,22 @@ static void socket_set_nodelay(int fd)
>       setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (char *)&val, sizeof(val));
>   }
>
> +static int tcp_chr_add_client(CharDriverState *chr, int fd)
> +{
> +    TCPCharDriver *s = chr->opaque;
> +    if (s->fd != -1)
> +	return -1;
> +
> +    socket_set_nonblock(fd);
> +    if (s->do_nodelay)
> +        socket_set_nodelay(fd);
> +    s->fd = fd;
> +    qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
> +    tcp_chr_connect(chr);
> +
> +    return 0;
> +}
> +
>   static void tcp_chr_accept(void *opaque)
>   {
>       CharDriverState *chr = opaque;
> @@ -2155,12 +2176,8 @@ static void tcp_chr_accept(void *opaque)
>               break;
>           }
>       }
> -    socket_set_nonblock(fd);
> -    if (s->do_nodelay)
> -        socket_set_nodelay(fd);
> -    s->fd = fd;
> -    qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
> -    tcp_chr_connect(chr);
> +    if (tcp_chr_add_client(chr, fd)<  0)
> +	close(fd);
>   }
>
>   static void tcp_chr_close(CharDriverState *chr)
> @@ -2230,6 +2247,7 @@ static CharDriverState *qemu_chr_open_socket(QemuOpts *opts)
>       chr->chr_write = tcp_chr_write;
>       chr->chr_close = tcp_chr_close;
>       chr->get_msgfd = tcp_get_msgfd;
> +    chr->chr_add_client = tcp_chr_add_client;
>
>       if (is_listen) {
>           s->listen_fd = fd;
> diff --git a/qemu-char.h b/qemu-char.h
> index 892c6da..f361c6d 100644
> --- a/qemu-char.h
> +++ b/qemu-char.h
> @@ -57,6 +57,7 @@ struct CharDriverState {
>       void (*chr_update_read_handler)(struct CharDriverState *s);
>       int (*chr_ioctl)(struct CharDriverState *s, int cmd, void *arg);
>       int (*get_msgfd)(struct CharDriverState *s);
> +    int (*chr_add_client)(struct CharDriverState *chr, int fd);
>       IOEventHandler *chr_event;
>       IOCanReadHandler *chr_can_read;
>       IOReadHandler *chr_read;
> @@ -99,6 +100,7 @@ int qemu_chr_can_read(CharDriverState *s);
>   void qemu_chr_read(CharDriverState *s, uint8_t *buf, int len);
>   int qemu_chr_get_msgfd(CharDriverState *s);
>   void qemu_chr_accept_input(CharDriverState *s);
> +int qemu_chr_add_client(CharDriverState *s, int fd);
>   void qemu_chr_info_print(Monitor *mon, const QObject *ret_data);
>   void qemu_chr_info(Monitor *mon, QObject **ret_data);
>   CharDriverState *qemu_chr_find(const char *name);
> diff --git a/qerror.c b/qerror.c
> index d7fcd93..79be1ba 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -193,6 +193,10 @@ static const QErrorStringTable qerror_table[] = {
>           .desc      = "Could not set password",
>       },
>       {
> +        .error_fmt = QERR_ADD_CLIENT_FAILED,
> +        .desc      = "Could not add client",
> +    },
> +    {
>           .error_fmt = QERR_TOO_MANY_FILES,
>           .desc      = "Too many open files",
>       },
> diff --git a/qerror.h b/qerror.h
> index 16c830d..25773cd 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -163,6 +163,9 @@ QError *qobject_to_qerror(const QObject *obj);
>   #define QERR_SET_PASSWD_FAILED \
>       "{ 'class': 'SetPasswdFailed', 'data': {} }"
>
> +#define QERR_ADD_CLIENT_FAILED \
> +    "{ 'class': 'AddClientFailed', 'data': {} }"
> +
>   #define QERR_TOO_MANY_FILES \
>       "{ 'class': 'TooManyFiles', 'data': {} }"
>
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 92c5c3a..1e19abc 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -885,6 +885,33 @@ Example:
>   EQMP
>
>       {
> +        .name       = "add_client",
> +        .args_type  = "protocol:s,fdname:s,skipauth:b?",
> +        .params     = "protocol fdname skipauth",
> +        .help       = "add a graphics client",
> +        .user_print = monitor_user_noop,
> +        .mhandler.cmd_new = add_graphics_client,
> +    },
> +
> +SQMP
> +add_client
> +----------
> +
> +Add a graphics client
> +
> +Arguments:
> +
> +- "protocol": protocol name (json-string)
> +- "fdname": file descriptor name (json-string)
> +
> +Example:
> +
> +->  { "execute": "add_client", "arguments": { "protocol": "vnc",
> +                                             "fdname": "myclient" } }
> +<- { "return": {} }
> +
> +EQMP
> +    {
>           .name       = "qmp_capabilities",
>           .args_type  = "",
>           .params     = "",
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 39b5b51..8602adc 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -2924,3 +2924,10 @@ int vnc_display_open(DisplayState *ds, const char *display)
>       }
>       return qemu_set_fd_handler2(vs->lsock, NULL, vnc_listen_read, NULL, vs);
>   }
> +
> +void vnc_display_add_client(DisplayState *ds, int csock, int skipauth)
> +{
> +    VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
> +
> +    return vnc_connect(vs, csock, skipauth);
> +}

Re: [Qemu-devel] [PATCH 2/3] Introduce a 'client_add' monitor command accepting an open FD

[Daniel P. Berrange]

On Sat, Aug 06, 2011 at 09:38:03AM -0500, Anthony Liguori wrote:
> On 06/23/2011 07:31 AM, Daniel P. Berrange wrote:
> >Allow client connections for VNC and socket based character
> >devices to be passed in over the monitor using SCM_RIGHTS.
> >
> >One intended usage scenario is to start QEMU with VNC on a
> >UNIX domain socket. An unprivileged user which cannot access
> >the UNIX domain socket, can then connect to QEMU's VNC server
> >by passing an open FD to libvirt, which passes it onto QEMU.
> >
> >  { "execute": "get_fd", "arguments": { "fdname": "myclient" } }
> >  { "return": {} }
> >  { "execute": "add_client", "arguments": { "protocol": "vnc",
> >                                            "fdname": "myclient",
> >                                            "skipauth": true } }
> >  { "return": {} }
> >
> >In this case 'protocol' can be 'vnc' or 'spice', or the name
> >of a character device (eg from -chardev id=XXXX)
> >
> >The 'skipauth' parameter can be used to skip any configured
> >VNC authentication scheme, which is useful if the mgmt layer
> >talking to the monitor has already authenticated the client
> >in another way.
> >
> >* console.h: Define 'vnc_display_add_client' method
> >* monitor.c: Implement 'client_add' command
> >* qemu-char.c, qemu-char.h: Add 'qemu_char_add_client' method
> 
> I don't feel all that good about this anymore.  The notion of adding
> a fd to an arbitrary character device is a big layering violation
> and doesn't make much sense conceptually.
> 
> A chardev cannot have multiple connections.  It seems like the
> use-case is much better suited to having an fd: protocol to create
> char devices with.

The same idea of being able to configure a VNC server to listen
on a UNIX socket path by default, but be able to inject the client
connection via libvirt + FD passing, also applies to chardevs IMHO.
So having a fd: protocol chardev doesn't seem right to me. Even if
we don't support multiple connections, it is still useful to be able
to pass in the initial connection, for TCP/UNIX based chardevs.

> I'd like to partially revert this removing the qemu_chr_add_client
> interface.

The VNC bit was the most immediately useful part to me, so having the
chardev bits now is not critical.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|