[libvirt] PATCH: Support VNC password for QEMU guests
This patch adds support for using the monitor interface to set the VNC password (qemu) change vnc password Password: ******** A minor tricky thing is that we can't just send the command and password all in one go, we must wait for the 'Password' prompt before sending the password. When doing this I noticed that virsh dumpxml has no way to request a secure XML dump (required to see the password element), nor did the virsh edit command set the SECURE or INACTIVE flags when changing the XML. qemu_conf.c | 45 ++++++++++++----------- qemu_driver.c | 112 ++++++++++++++++++++++++++++++++++++++++++++-------------- virsh.c | 30 ++++++++++----- 3 files changed, 131 insertions(+), 56 deletions(-) Daniel diff --git a/src/qemu_conf.c b/src/qemu_conf.c --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1138,37 +1138,42 @@ int qemudBuildCommandLine(virConnectPtr if (vm->def->graphics && vm->def->graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { - char vncdisplay[PATH_MAX]; - int ret; + virBuffer opt = VIR_BUFFER_INITIALIZER; + char *optstr; if (qemuCmdFlags & QEMUD_CMD_FLAG_VNC_COLON) { - char options[PATH_MAX] = ""; + if (vm->def->graphics->data.vnc.listenAddr) + virBufferAdd(&opt, vm->def->graphics->data.vnc.listenAddr, -1); + else if (driver->vncListen) + virBufferAdd(&opt, driver->vncListen, -1); + + virBufferVSprintf(&opt, ":%d", + vm->def->graphics->data.vnc.port - 5900); + + if (vm->def->graphics->data.vnc.passwd) + virBufferAddLit(&opt, ",password"); + if (driver->vncTLS) { - strcat(options, ",tls"); + virBufferAddLit(&opt, ",tls"); if (driver->vncTLSx509verify) { - strcat(options, ",x509verify="); + virBufferVSprintf(&opt, ",x509verify=%s", + driver->vncTLSx509certdir); } else { - strcat(options, ",x509="); + virBufferVSprintf(&opt, ",x509=%s", + driver->vncTLSx509certdir); } - strncat(options, driver->vncTLSx509certdir, - sizeof(options) - (strlen(driver->vncTLSx509certdir)-1)); - options[sizeof(options)-1] = '\0'; } - ret = snprintf(vncdisplay, sizeof(vncdisplay), "%s:%d%s", - (vm->def->graphics->data.vnc.listenAddr ? - vm->def->graphics->data.vnc.listenAddr : - (driver->vncListen ? driver->vncListen : "")), - vm->def->graphics->data.vnc.port - 5900, - options); } else { - ret = snprintf(vncdisplay, sizeof(vncdisplay), "%d", - vm->def->graphics->data.vnc.port - 5900); + virBufferVSprintf(&opt, "%d", + vm->def->graphics->data.vnc.port - 5900); } - if (ret < 0 || ret >= (int)sizeof(vncdisplay)) - goto error; + if (virBufferError(&opt)) + goto no_memory; + + optstr = virBufferContentAndReset(&opt); ADD_ARG_LIT("-vnc"); - ADD_ARG_LIT(vncdisplay); + ADD_ARG(optstr); if (vm->def->graphics->data.vnc.keymap) { ADD_ARG_LIT("-k"); ADD_ARG_LIT(vm->def->graphics->data.vnc.keymap); diff --git a/src/qemu_driver.c b/src/qemu_driver.c --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -74,6 +74,10 @@ /* For storing short-lived temporary files. */ #define TEMPDIR LOCAL_STATE_DIR "/cache/libvirt" +#define QEMU_CMD_PROMPT "\n(qemu) " +#define QEMU_PASSWD_PROMPT "Password: " + + static int qemudShutdown(void); #define qemudLog(level, msg...) fprintf(stderr, msg) @@ -138,9 +142,14 @@ static void qemudShutdownVMDaemon(virCon static int qemudDomainGetMaxVcpus(virDomainPtr dom); -static int qemudMonitorCommand (const virDomainObjPtr vm, - const char *cmd, - char **reply); +static int qemudMonitorCommand(const virDomainObjPtr vm, + const char *cmd, + char **reply); +static int qemudMonitorCommandExtra(const virDomainObjPtr vm, + const char *cmd, + const char *extra, + const char *extraPrompt, + char **reply); static struct qemud_driver *qemu_driver = NULL; @@ -1012,6 +1021,36 @@ qemudInitCpus(virConnectPtr conn, } +static int +qemudInitPasswords(virConnectPtr conn, + virDomainObjPtr vm) { + char *info = NULL; + + /* + * NB: Might have more passwords to set in the future. eg a qcow + * disk decryption password, but there's no monitor command + * for that yet... + */ + + if (vm->def->graphics && + vm->def->graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && + vm->def->graphics->data.vnc.passwd) { + + if (qemudMonitorCommandExtra(vm, "change vnc password", + vm->def->graphics->data.vnc.passwd, + QEMU_PASSWD_PROMPT, + &info) < 0) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + "%s", _("resume operation failed")); + return -1; + } + VIR_FREE(info); + } + + return 0; +} + + static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) { int i; @@ -1204,7 +1243,8 @@ static int qemudStartVMDaemon(virConnect if (ret == 0) { if ((qemudWaitForMonitor(conn, driver, vm, pos) < 0) || (qemudDetectVcpuPIDs(conn, vm) < 0) || - (qemudInitCpus(conn, vm, migrateFrom) < 0)) { + (qemudInitCpus(conn, vm, migrateFrom) < 0) || + (qemudInitPasswords(conn, vm) < 0)) { qemudShutdownVMDaemon(conn, driver, vm); return -1; } @@ -1314,12 +1354,15 @@ cleanup: } static int -qemudMonitorCommand (const virDomainObjPtr vm, - const char *cmd, - char **reply) { +qemudMonitorCommandExtra(const virDomainObjPtr vm, + const char *cmd, + const char *extra, + const char *extraPrompt, + char **reply) { int size = 0; char *buf = NULL; size_t cmdlen = strlen(cmd); + size_t extralen = extra ? strlen(extra) : 0; if (safewrite(vm->monitor, cmd, cmdlen) != cmdlen) return -1; @@ -1355,25 +1398,34 @@ qemudMonitorCommand (const virDomainObjP } /* Look for QEMU prompt to indicate completion */ - if (buf && ((tmp = strstr(buf, "\n(qemu) ")) != NULL)) { - char *commptr = NULL, *nlptr = NULL; - - /* Preserve the newline */ - tmp[1] = '\0'; - - /* The monitor doesn't dump clean output after we have written to - * it. Every character we write dumps a bunch of useless stuff, - * so the result looks like "cXcoXcomXcommXcommaXcommanXcommand" - * Try to throw away everything before the first full command - * occurence, and inbetween the command and the newline starting - * the response - */ - if ((commptr = strstr(buf, cmd))) - memmove(buf, commptr, strlen(commptr)+1); - if ((nlptr = strchr(buf, '\n'))) - memmove(buf+strlen(cmd), nlptr, strlen(nlptr)+1); - - break; + if (buf) { + if (extra) { + if (strstr(buf, extraPrompt) != NULL) { + if (safewrite(vm->monitor, extra, extralen) != extralen) + return -1; + if (safewrite(vm->monitor, "\r", 1) != 1) + return -1; + extra = NULL; + } + } else if ((tmp = strstr(buf, QEMU_CMD_PROMPT)) != NULL) { + char *commptr = NULL, *nlptr = NULL; + /* Preserve the newline */ + tmp[1] = '\0'; + + /* The monitor doesn't dump clean output after we have written to + * it. Every character we write dumps a bunch of useless stuff, + * so the result looks like "cXcoXcomXcommXcommaXcommanXcommand" + * Try to throw away everything before the first full command + * occurence, and inbetween the command and the newline starting + * the response + */ + if ((commptr = strstr(buf, cmd))) + memmove(buf, commptr, strlen(commptr)+1); + if ((nlptr = strchr(buf, '\n'))) + memmove(buf+strlen(cmd), nlptr, strlen(nlptr)+1); + + break; + } } pollagain: /* Need to wait for more data */ @@ -1403,6 +1455,14 @@ qemudMonitorCommand (const virDomainObjP return -1; } +static int +qemudMonitorCommand(const virDomainObjPtr vm, + const char *cmd, + char **reply) { + return qemudMonitorCommandExtra(vm, cmd, NULL, NULL, reply); +} + + /** * qemudProbe: * diff --git a/src/virsh.c b/src/virsh.c --- a/src/virsh.c +++ b/src/virsh.c @@ -2079,6 +2079,8 @@ static const vshCmdInfo info_dumpxml[] = static const vshCmdOptDef opts_dumpxml[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("domain name, id or uuid")}, + {"inactive", VSH_OT_BOOL, 0, gettext_noop("show inactive defined XML")}, + {"secure", VSH_OT_BOOL, 0, gettext_noop("include security sensitive data")}, {NULL, 0, 0, NULL} }; @@ -2088,14 +2090,22 @@ cmdDumpXML(vshControl *ctl, const vshCmd virDomainPtr dom; int ret = TRUE; char *dump; - - if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) - return FALSE; - - if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - return FALSE; - - dump = virDomainGetXMLDesc(dom, 0); + int flags = 0; + int inactive = vshCommandOptBool(cmd, "inactive"); + int secure = vshCommandOptBool(cmd, "secure"); + + if (inactive) + flags |= VIR_DOMAIN_XML_INACTIVE; + if(secure) + flags |= VIR_DOMAIN_XML_SECURE; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + return FALSE; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return FALSE; + + dump = virDomainGetXMLDesc(dom, flags); if (dump != NULL) { printf("%s", dump); free(dump); @@ -5374,7 +5384,7 @@ cmdEdit (vshControl *ctl, const vshCmd * goto cleanup; /* Get the XML configuration of the domain. */ - doc = virDomainGetXMLDesc (dom, 0); + doc = virDomainGetXMLDesc (dom, VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE); if (!doc) goto cleanup; @@ -5404,7 +5414,7 @@ cmdEdit (vshControl *ctl, const vshCmd * * it was being edited? This also catches problems such as us * losing a connection or the domain going away. */ - doc_reread = virDomainGetXMLDesc (dom, 0); + doc_reread = virDomainGetXMLDesc (dom, VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE); if (!doc_reread) goto cleanup; -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Tue, Jan 20, 2009 at 11:08:56PM +0000, Daniel P. Berrange wrote: [..snip..]
+static int +qemudInitPasswords(virConnectPtr conn, + virDomainObjPtr vm) { + char *info = NULL; + + /* + * NB: Might have more passwords to set in the future. eg a qcow + * disk decryption password, but there's no monitor command + * for that yet... + */ + + if (vm->def->graphics && + vm->def->graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && + vm->def->graphics->data.vnc.passwd) { + + if (qemudMonitorCommandExtra(vm, "change vnc password", + vm->def->graphics->data.vnc.passwd, + QEMU_PASSWD_PROMPT, + &info) < 0) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + "%s", _("resume operation failed")); \--- cut'n'paste?
+ return -1; + } + VIR_FREE(info); + } + + return 0; +} -- Guido
"Daniel P. Berrange" <berrange@redhat.com> wrote:
This patch adds support for using the monitor interface to set the VNC password
(qemu) change vnc password Password: ********
A minor tricky thing is that we can't just send the command and password all in one go, we must wait for the 'Password' prompt before sending the password.
When doing this I noticed that virsh dumpxml has no way to request a secure XML dump (required to see the password element), nor did the virsh edit command set the SECURE or INACTIVE flags when changing the XML. ... diff --git a/src/qemu_conf.c b/src/qemu_conf.c --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1138,37 +1138,42 @@ int qemudBuildCommandLine(virConnectPtr
if (vm->def->graphics && vm->def->graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { - char vncdisplay[PATH_MAX]; - int ret; + virBuffer opt = VIR_BUFFER_INITIALIZER; + char *optstr; ... + optstr = virBufferContentAndReset(&opt);
ADD_ARG_LIT("-vnc"); - ADD_ARG_LIT(vncdisplay); + ADD_ARG(optstr);
Looks fine. Just be sure to add this: VIR_FREE(optstr); Also, there are a few new lines longer than 80. BTW, can you outline what you do to test this? and I'll see about adding something to exercise the new code.
On Wed, Jan 21, 2009 at 01:48:25PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
This patch adds support for using the monitor interface to set the VNC password
(qemu) change vnc password Password: ********
A minor tricky thing is that we can't just send the command and password all in one go, we must wait for the 'Password' prompt before sending the password.
When doing this I noticed that virsh dumpxml has no way to request a secure XML dump (required to see the password element), nor did the virsh edit command set the SECURE or INACTIVE flags when changing the XML. ... diff --git a/src/qemu_conf.c b/src/qemu_conf.c --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1138,37 +1138,42 @@ int qemudBuildCommandLine(virConnectPtr
if (vm->def->graphics && vm->def->graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { - char vncdisplay[PATH_MAX]; - int ret; + virBuffer opt = VIR_BUFFER_INITIALIZER; + char *optstr; ... + optstr = virBufferContentAndReset(&opt);
ADD_ARG_LIT("-vnc"); - ADD_ARG_LIT(vncdisplay); + ADD_ARG(optstr);
Looks fine. Just be sure to add this:
VIR_FREE(optstr);
No need - ADD_ARG() owns the pointer - ADD_ARG_LIT() strdups, hence why I changed it.
BTW, can you outline what you do to test this? and I'll see about adding something to exercise the new code.
There's two parts to testing - The ARGV for QEMU - trivially to add more datafiles to qemuxml2argvtest.c - The monitor interaction We have no way to testing monitor interaction, and definitely do not want to be in the business of running real QEMU instances to test this. What I think we should do is to create a fake / mock QEMU binary in the tests directory, which simulates the monitor console. It would output pre-defined responses for each of the monitor commands we care about. This would let us unit test each of the individual functions that process monitor commands in isolation. It actally might be best to just use a self-pipe in the test case and have a thread to provide the server end of the monitor - that would let us trivially tell it what response we want to get back to each command we're about to test. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
No need - ADD_ARG() owns the pointer - ADD_ARG_LIT() strdups, hence why I changed it.
Ah. glad you didn't add it ;-)
BTW, can you outline what you do to test this? and I'll see about adding something to exercise the new code.
There's two parts to testing
- The ARGV for QEMU - trivially to add more datafiles to qemuxml2argvtest.c
I've just done that.
- The monitor interaction
We have no way to testing monitor interaction, and definitely do not want to be in the business of running real QEMU instances to test this. What I think we should do is to create a fake / mock QEMU binary in the tests directory, which simulates the monitor console. It would output pre-defined responses for each of the monitor commands we care about. This would let us unit test each of the individual functions that process monitor commands in isolation. It actally might be best to just use a self-pipe in the test case and have a thread to provide the server end of the monitor - that would let us trivially tell it what response we want to get back to each command we're about to test.
Sounds reasonable.
"Daniel P. Berrange" <berrange@redhat.com> wrote:
This patch adds support for using the monitor interface to set the VNC password
(qemu) change vnc password Password: ********
A minor tricky thing is that we can't just send the command and password all in one go, we must wait for the 'Password' prompt before sending the password.
When doing this I noticed that virsh dumpxml has no way to request a secure XML dump (required to see the password element), nor did the virsh edit command set the SECURE or INACTIVE flags when changing the XML.
qemu_conf.c | 45 ++++++++++++----------- qemu_driver.c | 112 ++++++++++++++++++++++++++++++++++++++++++++--------------
As you suggested, here's a test of the command-line/xml-processing part: A couple minor things I'm going to change eventually: 1) The name of every file in the qemuxml2argvdata/ directory starts with qemuxml2argv. I'm going to remove those suffixes and adjust the few callers. 2) Currently, each test case (pair of .args, .xml files) is checked in separately, giving no clue as to how similar the inputs of one test may be to another. Seeing such relationships would make it easier to add new tests. For example in this case, I've just added `, passwd="123456"' to the <graphics.../> line in the existing vnc-passwd.xml file: $ sed '/<graphics/s,/, passwd="123456"/,' qemuxml2argv-graphics-vnc.xml \ | diff -u - qemuxml2argv-vnc-passwd.xml - <graphics type='vnc' ... listen='127.0.0.1'/> + <graphics type='vnc' ... listen='127.0.0.1' passwd="123456"/> and appended ",password" to its .args file: $ sed 's/:3$/:3,password/' qemuxml2argv-graphics-vnc.args \ | diff -u - qemuxml2argv-vnc-passwd.args -LC_ALL=C ... -vnc 127.0.0.1:3 +LC_ALL=C ... -vnc 127.0.0.1:3,password
From ab770e0becc5b13cbd59f0f1219bb719f29e20d5 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 21 Jan 2009 17:22:59 +0100 Subject: [PATCH] tests: add an qemu/argv test for the new vnc-passwd feature
* tests/qemuxml2argvtest.c (mymain): Run the new test. * tests/qemuxml2argvdata/qemuxml2argv-vnc-passwd.xml: New file. * tests/qemuxml2argvdata/qemuxml2argv-vnc-passwd.args: Likewise. --- .../qemuxml2argvdata/qemuxml2argv-vnc-passwd.args | 1 + tests/qemuxml2argvdata/qemuxml2argv-vnc-passwd.xml | 24 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 + 3 files changed, 27 insertions(+), 0 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vnc-passwd.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-vnc-passwd.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-vnc-passwd.args b/tests/qemuxml2argvdata/qemuxml2argv-vnc-passwd.args new file mode 100644 index 0000000..30cc17b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-vnc-passwd.args @@ -0,0 +1 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -monitor pty -pidfile /nowhere/QEMUGuest1.pid -no-acpi -boot c -hda /dev/HostVG/QEMUGuest1 -net none -serial none -parallel none -usb -vnc 127.0.0.1:3,password diff --git a/tests/qemuxml2argvdata/qemuxml2argv-vnc-passwd.xml b/tests/qemuxml2argvdata/qemuxml2argv-vnc-passwd.xml new file mode 100644 index 0000000..da6ecf3 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-vnc-passwd.xml @@ -0,0 +1,24 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219200</memory> + <currentMemory>219200</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + </disk> + <input type='mouse' bus='ps2'/> + <graphics type='vnc' port='5903' autoport='no' listen='127.0.0.1' passwd="123456"/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 17684fd..5607ea0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -226,6 +226,8 @@ mymain(int argc, char **argv) DO_TEST("hostdev-pci-address", 0); + DO_TEST("vnc-passwd", 0); + virCapabilitiesFree(driver.caps); return(ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); -- 1.6.1.347.g0238
Daniel P. Berrange napsal(a):
This patch adds support for using the monitor interface to set the VNC password
(qemu) change vnc password Password: ********
A minor tricky thing is that we can't just send the command and password all in one go, we must wait for the 'Password' prompt before sending the password.
When doing this I noticed that virsh dumpxml has no way to request a secure XML dump (required to see the password element), nor did the virsh edit command set the SECURE or INACTIVE flags when changing the XML.
qemu_conf.c | 45 ++++++++++++----------- qemu_driver.c | 112 ++++++++++++++++++++++++++++++++++++++++++++-------------- virsh.c | 30 ++++++++++----- 3 files changed, 131 insertions(+), 56 deletions(-)
Daniel
I tried the patch and I've found that define command do not set the password as the SECURE flag is not set when defining the domain from XML file. This can be very unpleasant as it effectively disables the password when redefining the domain. I've tried this simple dirty hotfix diff -ur b/domain_conf.c a/domain_conf.c --- b/domain_conf.c 2008-12-23 14:20:02.000000000 +0100 +++ a/domain_conf.c 2009-01-25 20:12:47.000000000 +0100 @@ -2297,7 +2297,7 @@ } def = virDomainDefParseNode(conn, caps, xml, root, - VIR_DOMAIN_XML_INACTIVE); + VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE ); cleanup: xmlFreeParserCtxt (pctxt); and it works. However there must be some better way how to do it but I do not know the libvirt source code good enough to find it. Radek P.S. I've also found that virt-manager is missing handler for login button when asking for password but I will send email to virt-manager mailing list about it.
On Sun, Jan 25, 2009 at 09:22:53PM +0100, Radek Hladik wrote:
Daniel P. Berrange napsal(a):
This patch adds support for using the monitor interface to set the VNC password
(qemu) change vnc password Password: ********
A minor tricky thing is that we can't just send the command and password all in one go, we must wait for the 'Password' prompt before sending the password.
When doing this I noticed that virsh dumpxml has no way to request a secure XML dump (required to see the password element), nor did the virsh edit command set the SECURE or INACTIVE flags when changing the XML.
qemu_conf.c | 45 ++++++++++++----------- qemu_driver.c | 112 ++++++++++++++++++++++++++++++++++++++++++++-------------- virsh.c | 30 ++++++++++----- 3 files changed, 131 insertions(+), 56 deletions(-)
Daniel
I tried the patch and I've found that define command do not set the password as the SECURE flag is not set when defining the domain from XML file. This can be very unpleasant as it effectively disables the password when redefining the domain. I've tried this simple dirty hotfix
diff -ur b/domain_conf.c a/domain_conf.c --- b/domain_conf.c 2008-12-23 14:20:02.000000000 +0100 +++ a/domain_conf.c 2009-01-25 20:12:47.000000000 +0100 @@ -2297,7 +2297,7 @@ }
def = virDomainDefParseNode(conn, caps, xml, root, - VIR_DOMAIN_XML_INACTIVE); + VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE );
cleanup: xmlFreeParserCtxt (pctxt);
and it works. However there must be some better way how to do it but I do not know the libvirt source code good enough to find it.
That is the correct place to put the flag - I'll include this in my patches. We need to tell it to parse the passwd when loading the files from disk upon daemon startup. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Jan 26, 2009 at 11:15:01AM +0000, Daniel P. Berrange wrote:
On Sun, Jan 25, 2009 at 09:22:53PM +0100, Radek Hladik wrote:
Daniel P. Berrange napsal(a):
This patch adds support for using the monitor interface to set the VNC password
(qemu) change vnc password Password: ********
A minor tricky thing is that we can't just send the command and password all in one go, we must wait for the 'Password' prompt before sending the password.
When doing this I noticed that virsh dumpxml has no way to request a secure XML dump (required to see the password element), nor did the virsh edit command set the SECURE or INACTIVE flags when changing the XML.
qemu_conf.c | 45 ++++++++++++----------- qemu_driver.c | 112 ++++++++++++++++++++++++++++++++++++++++++++-------------- virsh.c | 30 ++++++++++----- 3 files changed, 131 insertions(+), 56 deletions(-)
Daniel
I tried the patch and I've found that define command do not set the password as the SECURE flag is not set when defining the domain from XML file. This can be very unpleasant as it effectively disables the password when redefining the domain. I've tried this simple dirty hotfix
diff -ur b/domain_conf.c a/domain_conf.c --- b/domain_conf.c 2008-12-23 14:20:02.000000000 +0100 +++ a/domain_conf.c 2009-01-25 20:12:47.000000000 +0100 @@ -2297,7 +2297,7 @@ }
def = virDomainDefParseNode(conn, caps, xml, root, - VIR_DOMAIN_XML_INACTIVE); + VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE );
cleanup: xmlFreeParserCtxt (pctxt);
and it works. However there must be some better way how to do it but I do not know the libvirt source code good enough to find it.
That is the correct place to put the flag - I'll include this in my patches. We need to tell it to parse the passwd when loading the files from disk upon daemon startup.
Actually this is not required. The password is always parsed from the XML. The secure flag is only required when generating XML for output. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Daniel P. Berrange napsal(a):
On Mon, Jan 26, 2009 at 11:15:01AM +0000, Daniel P. Berrange wrote:
On Sun, Jan 25, 2009 at 09:22:53PM +0100, Radek Hladik wrote:
Daniel P. Berrange napsal(a):
This patch adds support for using the monitor interface to set the VNC password
(qemu) change vnc password Password: ********
A minor tricky thing is that we can't just send the command and password all in one go, we must wait for the 'Password' prompt before sending the password.
When doing this I noticed that virsh dumpxml has no way to request a secure XML dump (required to see the password element), nor did the virsh edit command set the SECURE or INACTIVE flags when changing the XML.
qemu_conf.c | 45 ++++++++++++----------- qemu_driver.c | 112 ++++++++++++++++++++++++++++++++++++++++++++-------------- virsh.c | 30 ++++++++++----- 3 files changed, 131 insertions(+), 56 deletions(-)
Daniel
I tried the patch and I've found that define command do not set the password as the SECURE flag is not set when defining the domain from XML file. This can be very unpleasant as it effectively disables the password when redefining the domain. I've tried this simple dirty hotfix
diff -ur b/domain_conf.c a/domain_conf.c --- b/domain_conf.c 2008-12-23 14:20:02.000000000 +0100 +++ a/domain_conf.c 2009-01-25 20:12:47.000000000 +0100 @@ -2297,7 +2297,7 @@ }
def = virDomainDefParseNode(conn, caps, xml, root, - VIR_DOMAIN_XML_INACTIVE); + VIR_DOMAIN_XML_INACTIVE | VIR_DOMAIN_XML_SECURE );
cleanup: xmlFreeParserCtxt (pctxt);
and it works. However there must be some better way how to do it but I do not know the libvirt source code good enough to find it.
That is the correct place to put the flag - I'll include this in my patches. We need to tell it to parse the passwd when loading the files from disk upon daemon startup.
Actually this is not required. The password is always parsed from the XML. The secure flag is only required when generating XML for output.
Daniel
I am not sure about this. I've downloaded latest hourly snapshot yesterday, patched it with the VNC password patch and specified password via passwd attribute in xml. I've started the daemon and VNC required password - so far so good. However I've changed something other in xml file, reloaded the xml with virsh 'define test.xml' command and the machine didn't require VNC password any more. The supplied patch solved the issue... Radek
On Tue, Jan 20, 2009 at 11:08:56PM +0000, Daniel P. Berrange wrote:
This patch adds support for using the monitor interface to set the VNC password
(qemu) change vnc password Password: ********
A minor tricky thing is that we can't just send the command and password all in one go, we must wait for the 'Password' prompt before sending the password.
When doing this I noticed that virsh dumpxml has no way to request a secure XML dump (required to see the password element), nor did the virsh edit command set the SECURE or INACTIVE flags when changing the XML.
qemu_conf.c | 45 ++++++++++++----------- qemu_driver.c | 112 ++++++++++++++++++++++++++++++++++++++++++++-------------- virsh.c | 30 ++++++++++----- 3 files changed, 131 insertions(+), 56 deletions(-)
This update also makes it possible to set a global default password for all QEMU vms in /etc/libvirt/qemu.conf. This mirrors a similar option XenD has. A per-VM specified password always takes priority qemud/Makefile.am | 2 qemud/libvirtd_qemu.aug | 1 qemud/test_libvirtd.aug | 18 ++++++ qemud/test_libvirtd_qemu.aug | 33 +++++++----- src/qemu.conf | 11 ++++ src/qemu_conf.c | 58 ++++++++++++++------- src/qemu_conf.h | 1 src/qemu_driver.c | 116 +++++++++++++++++++++++++++++++++---------- src/uml_driver.c | 4 + src/virsh.c | 30 +++++++---- 10 files changed, 204 insertions(+), 70 deletions(-) Daniel diff --git a/qemud/Makefile.am b/qemud/Makefile.am --- a/qemud/Makefile.am +++ b/qemud/Makefile.am @@ -246,6 +246,8 @@ libvirtd.init: libvirtd.init.in check-local: test -x '$(AUGPARSE)' \ && '$(AUGPARSE)' -I $(srcdir) $(srcdir)/test_libvirtd.aug || : + test -x '$(AUGPARSE)' \ + && '$(AUGPARSE)' -I $(srcdir) $(srcdir)/test_libvirtd_qemu.aug || : else diff --git a/qemud/libvirtd_qemu.aug b/qemud/libvirtd_qemu.aug --- a/qemud/libvirtd_qemu.aug +++ b/qemud/libvirtd_qemu.aug @@ -26,6 +26,7 @@ module Libvirtd_qemu = | bool_entry "vnc_tls" | str_entry "vnc_tls_x509_cert_dir" | bool_entry "vnc_tls_x509_verify" + | str_entry "vnc_password" (* Each enty in the config is one of the following three ... *) let entry = vnc_entry diff --git a/qemud/test_libvirtd.aug b/qemud/test_libvirtd.aug --- a/qemud/test_libvirtd.aug +++ b/qemud/test_libvirtd.aug @@ -259,6 +259,15 @@ max_requests = 20 # this should be a small fraction of the global max_requests # and max_workers parameter max_client_requests = 5 + +# Logging level: +log_level = 4 + +# Logging outputs: +log_outputs=\"4:stderr\" + +# Logging filters: +log_filters=\"a\" " test Libvirtd.lns get conf = @@ -525,3 +534,12 @@ max_client_requests = 5 { "#comment" = "this should be a small fraction of the global max_requests" } { "#comment" = "and max_workers parameter" } { "max_client_requests" = "5" } + { "#empty" } + { "#comment" = "Logging level:" } + { "log_level" = "4" } + { "#empty" } + { "#comment" = "Logging outputs:" } + { "log_outputs" = "4:stderr" } + { "#empty" } + { "#comment" = "Logging filters:" } + { "log_filters" = "a" } diff --git a/qemud/test_libvirtd_qemu.aug b/qemud/test_libvirtd_qemu.aug --- a/qemud/test_libvirtd_qemu.aug +++ b/qemud/test_libvirtd_qemu.aug @@ -50,14 +50,16 @@ vnc_tls_x509_cert_dir = \"/etc/pki/libvi # vnc_tls_x509_verify = 1 -# Logging level: -log_level = 4 -# Logging outputs: -log_outputs="4:stderr" - -# Logging filters: -log_filters="" +# The default VNC password. Only 8 letters are significant for +# VNC passwords. This parameter is only used if the per-domain +# XML config does not already provide a password. To allow +# access without passwords, leave this commented out. An empty +# string will still enable passwords, but be rejected by QEMU +# effectively preventing any use of VNC. Obviously change this +# example here before you set this +# +vnc_password = \"XYZ12345\" " test Libvirtd_qemu.lns get conf = @@ -110,9 +112,14 @@ log_filters="" { "#comment" = "certificate signed by the CA in /etc/pki/libvirt-vnc/ca-cert.pem" } { "#comment" = "" } { "vnc_tls_x509_verify" = "1" } -{ "#comment" = "Logging level:" } -{ "log_level" = "4" } -{ "#comment" = "Logging outputs:" } -{ "log_outputs" = "4:stderr" } -{ "#comment" = "Logging filters" } -{ "log_filters" = "" } +{ "#empty" } +{ "#empty" } +{ "#comment" = "The default VNC password. Only 8 letters are significant for" } +{ "#comment" = "VNC passwords. This parameter is only used if the per-domain" } +{ "#comment" = "XML config does not already provide a password. To allow" } +{ "#comment" = "access without passwords, leave this commented out. An empty" } +{ "#comment" = "string will still enable passwords, but be rejected by QEMU" } +{ "#comment" = "effectively preventing any use of VNC. Obviously change this" } +{ "#comment" = "example here before you set this" } +{ "#comment" = "" } +{ "vnc_password" = "XYZ12345" } diff --git a/src/qemu.conf b/src/qemu.conf --- a/src/qemu.conf +++ b/src/qemu.conf @@ -47,3 +47,14 @@ # certificate signed by the CA in /etc/pki/libvirt-vnc/ca-cert.pem # # vnc_tls_x509_verify = 1 + + +# The default VNC password. Only 8 letters are significant for +# VNC passwords. This parameter is only used if the per-domain +# XML config does not already provide a password. To allow +# access without passwords, leave this commented out. An empty +# string will still enable passwords, but be rejected by QEMU +# effectively preventing any use of VNC. Obviously change this +# example here before you set this +# +# vnc_password = "XYZ12345" diff --git a/src/qemu_conf.c b/src/qemu_conf.c --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -129,6 +129,18 @@ int qemudLoadDriverConfig(struct qemud_d } } + p = virConfGetValue (conf, "vnc_password"); + CHECK_TYPE ("vnc_password", VIR_CONF_STRING); + if (p && p->str) { + VIR_FREE(driver->vncPassword); + if (!(driver->vncPassword = strdup(p->str))) { + qemudReportError(NULL, NULL, NULL, VIR_ERR_NO_MEMORY, + "%s", _("failed to allocate vnc_listen")); + virConfFree(conf); + return -1; + } + } + virConfFree (conf); return 0; } @@ -1138,37 +1150,43 @@ int qemudBuildCommandLine(virConnectPtr if (vm->def->graphics && vm->def->graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC) { - char vncdisplay[PATH_MAX]; - int ret; + virBuffer opt = VIR_BUFFER_INITIALIZER; + char *optstr; if (qemuCmdFlags & QEMUD_CMD_FLAG_VNC_COLON) { - char options[PATH_MAX] = ""; + if (vm->def->graphics->data.vnc.listenAddr) + virBufferAdd(&opt, vm->def->graphics->data.vnc.listenAddr, -1); + else if (driver->vncListen) + virBufferAdd(&opt, driver->vncListen, -1); + + virBufferVSprintf(&opt, ":%d", + vm->def->graphics->data.vnc.port - 5900); + + if (vm->def->graphics->data.vnc.passwd || + driver->vncPassword) + virBufferAddLit(&opt, ",password"); + if (driver->vncTLS) { - strcat(options, ",tls"); + virBufferAddLit(&opt, ",tls"); if (driver->vncTLSx509verify) { - strcat(options, ",x509verify="); + virBufferVSprintf(&opt, ",x509verify=%s", + driver->vncTLSx509certdir); } else { - strcat(options, ",x509="); + virBufferVSprintf(&opt, ",x509=%s", + driver->vncTLSx509certdir); } - strncat(options, driver->vncTLSx509certdir, - sizeof(options) - (strlen(driver->vncTLSx509certdir)-1)); - options[sizeof(options)-1] = '\0'; } - ret = snprintf(vncdisplay, sizeof(vncdisplay), "%s:%d%s", - (vm->def->graphics->data.vnc.listenAddr ? - vm->def->graphics->data.vnc.listenAddr : - (driver->vncListen ? driver->vncListen : "")), - vm->def->graphics->data.vnc.port - 5900, - options); } else { - ret = snprintf(vncdisplay, sizeof(vncdisplay), "%d", - vm->def->graphics->data.vnc.port - 5900); + virBufferVSprintf(&opt, "%d", + vm->def->graphics->data.vnc.port - 5900); } - if (ret < 0 || ret >= (int)sizeof(vncdisplay)) - goto error; + if (virBufferError(&opt)) + goto no_memory; + + optstr = virBufferContentAndReset(&opt); ADD_ARG_LIT("-vnc"); - ADD_ARG_LIT(vncdisplay); + ADD_ARG(optstr); if (vm->def->graphics->data.vnc.keymap) { ADD_ARG_LIT("-k"); ADD_ARG_LIT(vm->def->graphics->data.vnc.keymap); diff --git a/src/qemu_conf.h b/src/qemu_conf.h --- a/src/qemu_conf.h +++ b/src/qemu_conf.h @@ -68,6 +68,7 @@ struct qemud_driver { unsigned int vncTLSx509verify : 1; char *vncTLSx509certdir; char *vncListen; + char *vncPassword; virCapsPtr caps; diff --git a/src/qemu_driver.c b/src/qemu_driver.c --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -74,6 +74,10 @@ /* For storing short-lived temporary files. */ #define TEMPDIR LOCAL_STATE_DIR "/cache/libvirt" +#define QEMU_CMD_PROMPT "\n(qemu) " +#define QEMU_PASSWD_PROMPT "Password: " + + static int qemudShutdown(void); #define qemudLog(level, msg...) fprintf(stderr, msg) @@ -138,9 +142,14 @@ static void qemudShutdownVMDaemon(virCon static int qemudDomainGetMaxVcpus(virDomainPtr dom); -static int qemudMonitorCommand (const virDomainObjPtr vm, - const char *cmd, - char **reply); +static int qemudMonitorCommand(const virDomainObjPtr vm, + const char *cmd, + char **reply); +static int qemudMonitorCommandExtra(const virDomainObjPtr vm, + const char *cmd, + const char *extra, + const char *extraPrompt, + char **reply); static struct qemud_driver *qemu_driver = NULL; @@ -583,6 +592,7 @@ qemudShutdown(void) { VIR_FREE(qemu_driver->stateDir); VIR_FREE(qemu_driver->vncTLSx509certdir); VIR_FREE(qemu_driver->vncListen); + VIR_FREE(qemu_driver->vncPassword); /* Free domain callback list */ virDomainEventCallbackListFree(qemu_driver->domainEventCallbacks); @@ -1012,6 +1022,39 @@ qemudInitCpus(virConnectPtr conn, } +static int +qemudInitPasswords(virConnectPtr conn, + struct qemud_driver *driver, + virDomainObjPtr vm) { + char *info = NULL; + + /* + * NB: Might have more passwords to set in the future. eg a qcow + * disk decryption password, but there's no monitor command + * for that yet... + */ + + if (vm->def->graphics && + vm->def->graphics->type == VIR_DOMAIN_GRAPHICS_TYPE_VNC && + vm->def->graphics->data.vnc.passwd) { + + if (qemudMonitorCommandExtra(vm, "change vnc password", + vm->def->graphics->data.vnc.passwd ? + vm->def->graphics->data.vnc.passwd : + driver->vncPassword, + QEMU_PASSWD_PROMPT, + &info) < 0) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + "%s", _("setting VNC password failed")); + return -1; + } + VIR_FREE(info); + } + + return 0; +} + + static int qemudNextFreeVNCPort(struct qemud_driver *driver ATTRIBUTE_UNUSED) { int i; @@ -1204,7 +1247,8 @@ static int qemudStartVMDaemon(virConnect if (ret == 0) { if ((qemudWaitForMonitor(conn, driver, vm, pos) < 0) || (qemudDetectVcpuPIDs(conn, vm) < 0) || - (qemudInitCpus(conn, vm, migrateFrom) < 0)) { + (qemudInitCpus(conn, vm, migrateFrom) < 0) || + (qemudInitPasswords(conn, driver, vm) < 0)) { qemudShutdownVMDaemon(conn, driver, vm); return -1; } @@ -1314,12 +1358,15 @@ cleanup: } static int -qemudMonitorCommand (const virDomainObjPtr vm, - const char *cmd, - char **reply) { +qemudMonitorCommandExtra(const virDomainObjPtr vm, + const char *cmd, + const char *extra, + const char *extraPrompt, + char **reply) { int size = 0; char *buf = NULL; size_t cmdlen = strlen(cmd); + size_t extralen = extra ? strlen(extra) : 0; if (safewrite(vm->monitor, cmd, cmdlen) != cmdlen) return -1; @@ -1355,25 +1402,34 @@ qemudMonitorCommand (const virDomainObjP } /* Look for QEMU prompt to indicate completion */ - if (buf && ((tmp = strstr(buf, "\n(qemu) ")) != NULL)) { - char *commptr = NULL, *nlptr = NULL; - - /* Preserve the newline */ - tmp[1] = '\0'; - - /* The monitor doesn't dump clean output after we have written to - * it. Every character we write dumps a bunch of useless stuff, - * so the result looks like "cXcoXcomXcommXcommaXcommanXcommand" - * Try to throw away everything before the first full command - * occurence, and inbetween the command and the newline starting - * the response - */ - if ((commptr = strstr(buf, cmd))) - memmove(buf, commptr, strlen(commptr)+1); - if ((nlptr = strchr(buf, '\n'))) - memmove(buf+strlen(cmd), nlptr, strlen(nlptr)+1); - - break; + if (buf) { + if (extra) { + if (strstr(buf, extraPrompt) != NULL) { + if (safewrite(vm->monitor, extra, extralen) != extralen) + return -1; + if (safewrite(vm->monitor, "\r", 1) != 1) + return -1; + extra = NULL; + } + } else if ((tmp = strstr(buf, QEMU_CMD_PROMPT)) != NULL) { + char *commptr = NULL, *nlptr = NULL; + /* Preserve the newline */ + tmp[1] = '\0'; + + /* The monitor doesn't dump clean output after we have written to + * it. Every character we write dumps a bunch of useless stuff, + * so the result looks like "cXcoXcomXcommXcommaXcommanXcommand" + * Try to throw away everything before the first full command + * occurence, and inbetween the command and the newline starting + * the response + */ + if ((commptr = strstr(buf, cmd))) + memmove(buf, commptr, strlen(commptr)+1); + if ((nlptr = strchr(buf, '\n'))) + memmove(buf+strlen(cmd), nlptr, strlen(nlptr)+1); + + break; + } } pollagain: /* Need to wait for more data */ @@ -1403,6 +1459,14 @@ qemudMonitorCommand (const virDomainObjP return -1; } +static int +qemudMonitorCommand(const virDomainObjPtr vm, + const char *cmd, + char **reply) { + return qemudMonitorCommandExtra(vm, cmd, NULL, NULL, reply); +} + + /** * qemudProbe: * diff --git a/src/uml_driver.c b/src/uml_driver.c --- a/src/uml_driver.c +++ b/src/uml_driver.c @@ -324,6 +324,7 @@ umlStartup(void) { /* Don't have a dom0 so start from 1 */ uml_driver->nextvmid = 1; + uml_driver->inotifyWatch = -1; userdir = virGetUserDirectory(NULL, uid); if (!userdir) @@ -484,7 +485,8 @@ umlShutdown(void) { return -1; umlDriverLock(uml_driver); - virEventRemoveHandle(uml_driver->inotifyWatch); + if (uml_driver->inotifyWatch != -1) + virEventRemoveHandle(uml_driver->inotifyWatch); close(uml_driver->inotifyFD); virCapabilitiesFree(uml_driver->caps); diff --git a/src/virsh.c b/src/virsh.c --- a/src/virsh.c +++ b/src/virsh.c @@ -2079,6 +2079,8 @@ static const vshCmdInfo info_dumpxml[] = static const vshCmdOptDef opts_dumpxml[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("domain name, id or uuid")}, + {"inactive", VSH_OT_BOOL, 0, gettext_noop("show inactive defined XML")}, + {"secure", VSH_OT_BOOL, 0, gettext_noop("include security sensitive data")}, {NULL, 0, 0, NULL} }; @@ -2088,14 +2090,22 @@ cmdDumpXML(vshControl *ctl, const vshCmd virDomainPtr dom; int ret = TRUE; char *dump; - - if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) - return FALSE; - - if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) - return FALSE; - - dump = virDomainGetXMLDesc(dom, 0); + int flags = 0; + int inactive = vshCommandOptBool(cmd, "inactive"); + int secure = vshCommandOptBool(cmd, "secure"); + + if (inactive) + flags |= VIR_DOMAIN_XML_INACTIVE; + if(secure) + flags |= VIR_DOMAIN_XML_SECURE; + + if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) + return FALSE; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return FALSE; + + dump = virDomainGetXMLDesc(dom, flags); if (dump != NULL) { printf("%s", dump); free(dump); @@ -5374,7 +5384,7 @@ cmdEdit (vshControl *ctl, const vshCmd * goto cleanup; /* Get the XML configuration of the domain. */ - doc = virDomainGetXMLDesc (dom, 0); + doc = virDomainGetXMLDesc (dom, VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE); if (!doc) goto cleanup; @@ -5404,7 +5414,7 @@ cmdEdit (vshControl *ctl, const vshCmd * * it was being edited? This also catches problems such as us * losing a connection or the domain going away. */ - doc_reread = virDomainGetXMLDesc (dom, 0); + doc_reread = virDomainGetXMLDesc (dom, VIR_DOMAIN_XML_SECURE | VIR_DOMAIN_XML_INACTIVE); if (!doc_reread) goto cleanup; -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote: > On Tue, Jan 20, 2009 at 11:08:56PM +0000, Daniel P. Berrange wrote: >> This patch adds support for using the monitor interface to set the VNC >> password >> >> (qemu) change vnc password >> Password: ******** >> >> A minor tricky thing is that we can't just send the command and password >> all in one go, we must wait for the 'Password' prompt before sending the >> password. >> >> When doing this I noticed that virsh dumpxml has no way to request a >> secure XML dump (required to see the password element), nor did the >> virsh edit command set the SECURE or INACTIVE flags when changing >> the XML. >> >> qemu_conf.c | 45 ++++++++++++----------- >> qemu_driver.c | 112 ++++++++++++++++++++++++++++++++++++++++++++-------------- >> virsh.c | 30 ++++++++++----- >> 3 files changed, 131 insertions(+), 56 deletions(-) ... > + int flags = 0; > + int inactive = vshCommandOptBool(cmd, "inactive"); > + int secure = vshCommandOptBool(cmd, "secure"); > + > + if (inactive) > + flags |= VIR_DOMAIN_XML_INACTIVE; > + if(secure) > + flags |= VIR_DOMAIN_XML_SECURE; ACK. My only reservation is that this new --secure option currently means "also dump sensitive info" (passwords), which is sometimes _in_secure. So how about naming it --all instead?
On Wed, Jan 28, 2009 at 07:46:34PM +0100, Jim Meyering wrote: > "Daniel P. Berrange" <berrange@redhat.com> wrote: > > On Tue, Jan 20, 2009 at 11:08:56PM +0000, Daniel P. Berrange wrote: > >> This patch adds support for using the monitor interface to set the VNC > >> password > >> > >> (qemu) change vnc password > >> Password: ******** > >> > >> A minor tricky thing is that we can't just send the command and password > >> all in one go, we must wait for the 'Password' prompt before sending the > >> password. > >> > >> When doing this I noticed that virsh dumpxml has no way to request a > >> secure XML dump (required to see the password element), nor did the > >> virsh edit command set the SECURE or INACTIVE flags when changing > >> the XML. > >> > >> qemu_conf.c | 45 ++++++++++++----------- > >> qemu_driver.c | 112 ++++++++++++++++++++++++++++++++++++++++++++-------------- > >> virsh.c | 30 ++++++++++----- > >> 3 files changed, 131 insertions(+), 56 deletions(-) > ... > > + int flags = 0; > > + int inactive = vshCommandOptBool(cmd, "inactive"); > > + int secure = vshCommandOptBool(cmd, "secure"); > > + > > + if (inactive) > > + flags |= VIR_DOMAIN_XML_INACTIVE; > > + if(secure) > > + flags |= VIR_DOMAIN_XML_SECURE; > > ACK. > My only reservation is that this new --secure option currently means > "also dump sensitive info" (passwords), which is sometimes > _in_secure. So how about naming it --all instead? How about --security-info ? I think --all is probably a little too generic a term Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote: > On Wed, Jan 28, 2009 at 07:46:34PM +0100, Jim Meyering wrote: >> "Daniel P. Berrange" <berrange@redhat.com> wrote: >> > On Tue, Jan 20, 2009 at 11:08:56PM +0000, Daniel P. Berrange wrote: >> >> This patch adds support for using the monitor interface to set the VNC >> >> password >> >> >> >> (qemu) change vnc password >> >> Password: ******** >> >> >> >> A minor tricky thing is that we can't just send the command and password >> >> all in one go, we must wait for the 'Password' prompt before sending the >> >> password. >> >> >> >> When doing this I noticed that virsh dumpxml has no way to request a >> >> secure XML dump (required to see the password element), nor did the >> >> virsh edit command set the SECURE or INACTIVE flags when changing >> >> the XML. >> >> >> >> qemu_conf.c | 45 ++++++++++++----------- >> >> qemu_driver.c | 112 ++++++++++++++++++++++++++++++++++++++++++++-------------- >> >> virsh.c | 30 ++++++++++----- >> >> 3 files changed, 131 insertions(+), 56 deletions(-) >> ... >> > + int flags = 0; >> > + int inactive = vshCommandOptBool(cmd, "inactive"); >> > + int secure = vshCommandOptBool(cmd, "secure"); >> > + >> > + if (inactive) >> > + flags |= VIR_DOMAIN_XML_INACTIVE; >> > + if(secure) >> > + flags |= VIR_DOMAIN_XML_SECURE; >> >> ACK. >> My only reservation is that this new --secure option currently means >> "also dump sensitive info" (passwords), which is sometimes >> _in_secure. So how about naming it --all instead? > > How about --security-info ? I think --all is probably a little too > generic a term Sounds good.
participants (4)
-
Daniel P. Berrange -
Guido Günther -
Jim Meyering -
Radek Hladik