[libvirt] [PATCH] virsh: fixed domdisplay command
The 'domdisplay' command didn't properly evaluate '--include-password' option. --- tools/virsh.c | 35 +++++++++++++++++++++++------------ 1 files changed, 23 insertions(+), 12 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 5888d6c..e0765ba 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -66,6 +66,7 @@ #include "virtypedparam.h" #include "intprops.h" #include "conf/virdomainlist.h" +#include "datatypes.h" static char *progname; @@ -13882,7 +13883,16 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd) goto cleanup; } - doc = virDomainGetXMLDesc(dom, 0); + if (!vshCommandOptBool(cmd, "include-password")) + doc = virDomainGetXMLDesc(dom, 0); + else { + if (ctl->conn->flags & VIR_DOMAIN_XML_SECURE) { + vshError(ctl, _("Cannot get password with read-only connection")); + goto cleanup; + } + doc = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_SECURE); + } + if (!doc) goto cleanup; @@ -13944,19 +13954,20 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd) if (tmp) tls_port = 0; - if (vshCommandOptBool(cmd, "daemon")) { - /* Create our XPATH lookup for the SPICE password */ - virAsprintf(&xpath, "string(/domain/devices/graphics" + /* Create our XPATH lookup for the SPICE password */ + virAsprintf(&xpath, "string(/domain/devices/graphics" "[@type='%s']/@passwd)", scheme[iter]); - if (!xpath) { - virReportOOMError(); - goto cleanup; - } - - /* Attempt to get the SPICE password */ - passwd = virXPathString(xpath, ctxt); - VIR_FREE(xpath); + if (!xpath) { + virReportOOMError(); + goto cleanup; } + + /* Attempt to get the SPICE password + * + * This will return NULL automatically if the + * virDomainGetXMLDesc wasn't secure */ + passwd = virXPathString(xpath, ctxt); + VIR_FREE(xpath); } /* Build up the full URI, starting with the scheme */ -- 1.7.8.6
On Mon, Jul 23, 2012 at 1:51 PM, Martin Kletzander <mkletzan@redhat.com> wrote:
The 'domdisplay' command didn't properly evaluate '--include-password' option. --- tools/virsh.c | 35 +++++++++++++++++++++++------------ 1 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 5888d6c..e0765ba 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -66,6 +66,7 @@ #include "virtypedparam.h" #include "intprops.h" #include "conf/virdomainlist.h" +#include "datatypes.h"
Why exactly is this necessary? No new types are being used that aren't used throughout this whole file.
static char *progname;
@@ -13882,7 +13883,16 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd) goto cleanup; }
- doc = virDomainGetXMLDesc(dom, 0); + if (!vshCommandOptBool(cmd, "include-password")) + doc = virDomainGetXMLDesc(dom, 0); + else { + if (ctl->conn->flags & VIR_DOMAIN_XML_SECURE) { + vshError(ctl, _("Cannot get password with read-only connection")); + goto cleanup; + } + doc = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_SECURE); + } +
I would do like all the other commands and define unsigned int flags = 0; at the top and if include-password is set just do flags |= VIR_DOMAIN_XML_SECURE; and always call virDomainGetXMLDesc() with flags passed. Look at cmdSnapshotCurrent for an example.
if (!doc) goto cleanup;
@@ -13944,19 +13954,20 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd) if (tmp) tls_port = 0;
- if (vshCommandOptBool(cmd, "daemon")) { - /* Create our XPATH lookup for the SPICE password */ - virAsprintf(&xpath, "string(/domain/devices/graphics" + /* Create our XPATH lookup for the SPICE password */ + virAsprintf(&xpath, "string(/domain/devices/graphics" "[@type='%s']/@passwd)", scheme[iter]); - if (!xpath) { - virReportOOMError(); - goto cleanup; - } - - /* Attempt to get the SPICE password */ - passwd = virXPathString(xpath, ctxt); - VIR_FREE(xpath); + if (!xpath) { + virReportOOMError(); + goto cleanup; } + + /* Attempt to get the SPICE password + * + * This will return NULL automatically if the + * virDomainGetXMLDesc wasn't secure */ + passwd = virXPathString(xpath, ctxt); + VIR_FREE(xpath); }
/* Build up the full URI, starting with the scheme */ -- 1.7.8.6
Getting rid of the conditional on include-password (which was incorrectly stated as 'daemon') doesn't seem correct. Leave the logic as is and just fix 'daemon' to say 'include-password'. -- Doug Goldstein
On Mon, Jul 23, 2012 at 02:25:23PM -0500, Doug Goldstein wrote:
On Mon, Jul 23, 2012 at 1:51 PM, Martin Kletzander <mkletzan@redhat.com> wrote:
The 'domdisplay' command didn't properly evaluate '--include-password' option. --- tools/virsh.c | 35 +++++++++++++++++++++++------------ 1 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 5888d6c..e0765ba 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -66,6 +66,7 @@ #include "virtypedparam.h" #include "intprops.h" #include "conf/virdomainlist.h" +#include "datatypes.h"
Why exactly is this necessary? No new types are being used that aren't used throughout this whole file.
Indeed, datatypes.c is for the libvirt private details of the public objects. virsh is a public API consumer, so has no justification for using this include. 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 :|
On 07/23/2012 09:42 PM, Daniel P. Berrange wrote:
On Mon, Jul 23, 2012 at 02:25:23PM -0500, Doug Goldstein wrote:
On Mon, Jul 23, 2012 at 1:51 PM, Martin Kletzander <mkletzan@redhat.com> wrote:
The 'domdisplay' command didn't properly evaluate '--include-password' option. --- tools/virsh.c | 35 +++++++++++++++++++++++------------ 1 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 5888d6c..e0765ba 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -66,6 +66,7 @@ #include "virtypedparam.h" #include "intprops.h" #include "conf/virdomainlist.h" +#include "datatypes.h"
Why exactly is this necessary? No new types are being used that aren't used throughout this whole file.
Indeed, datatypes.c is for the libvirt private details of the public objects. virsh is a public API consumer, so has no justification for using this include.
I wasn't able to dereference virConnectPtr (conn->flags) without this include, so gcc ended with an error. Martin
Daniel
On Tue, Jul 24, 2012 at 09:16:38AM +0200, Martin Kletzander wrote:
On 07/23/2012 09:42 PM, Daniel P. Berrange wrote:
On Mon, Jul 23, 2012 at 02:25:23PM -0500, Doug Goldstein wrote:
On Mon, Jul 23, 2012 at 1:51 PM, Martin Kletzander <mkletzan@redhat.com> wrote:
The 'domdisplay' command didn't properly evaluate '--include-password' option. --- tools/virsh.c | 35 +++++++++++++++++++++++------------ 1 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 5888d6c..e0765ba 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -66,6 +66,7 @@ #include "virtypedparam.h" #include "intprops.h" #include "conf/virdomainlist.h" +#include "datatypes.h"
Why exactly is this necessary? No new types are being used that aren't used throughout this whole file.
Indeed, datatypes.c is for the libvirt private details of the public objects. virsh is a public API consumer, so has no justification for using this include.
I wasn't able to dereference virConnectPtr (conn->flags) without this include, so gcc ended with an error.
virsh has no business touching conn->flags at all. That is private libvirt data, not for public consumption. 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 :|
On 07/24/2012 09:58 AM, Daniel P. Berrange wrote:
On Tue, Jul 24, 2012 at 09:16:38AM +0200, Martin Kletzander wrote:
On 07/23/2012 09:42 PM, Daniel P. Berrange wrote:
On Mon, Jul 23, 2012 at 02:25:23PM -0500, Doug Goldstein wrote:
On Mon, Jul 23, 2012 at 1:51 PM, Martin Kletzander <mkletzan@redhat.com> wrote:
The 'domdisplay' command didn't properly evaluate '--include-password' option. --- tools/virsh.c | 35 +++++++++++++++++++++++------------ 1 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 5888d6c..e0765ba 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -66,6 +66,7 @@ #include "virtypedparam.h" #include "intprops.h" #include "conf/virdomainlist.h" +#include "datatypes.h"
Why exactly is this necessary? No new types are being used that aren't used throughout this whole file.
Indeed, datatypes.c is for the libvirt private details of the public objects. virsh is a public API consumer, so has no justification for using this include.
I wasn't able to dereference virConnectPtr (conn->flags) without this include, so gcc ended with an error.
virsh has no business touching conn->flags at all. That is private libvirt data, not for public consumption.
That makes sense. I didn't know where exactly get the flags (dom->conn->flags maybe?), but it won't be in v2 anyway. Martin
On 07/23/2012 12:51 PM, Martin Kletzander wrote:
The 'domdisplay' command didn't properly evaluate '--include-password' option. --- tools/virsh.c | 35 +++++++++++++++++++++++------------ 1 files changed, 23 insertions(+), 12 deletions(-)
In addition to Doug's review...
- doc = virDomainGetXMLDesc(dom, 0); + if (!vshCommandOptBool(cmd, "include-password")) + doc = virDomainGetXMLDesc(dom, 0); + else { + if (ctl->conn->flags & VIR_DOMAIN_XML_SECURE) { + vshError(ctl, _("Cannot get password with read-only connection")); + goto cleanup; + }
We shouldn't have to do this filtering here. Just attempt the access always (when the options say to); it (better) fail at the driver level if the connection was read-only, for less work here in virsh, and so that we can actually validate that the security checking is being done at the driver level. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/23/2012 09:42 PM, Eric Blake wrote:
On 07/23/2012 12:51 PM, Martin Kletzander wrote:
The 'domdisplay' command didn't properly evaluate '--include-password' option. --- tools/virsh.c | 35 +++++++++++++++++++++++------------ 1 files changed, 23 insertions(+), 12 deletions(-)
In addition to Doug's review...
- doc = virDomainGetXMLDesc(dom, 0); + if (!vshCommandOptBool(cmd, "include-password")) + doc = virDomainGetXMLDesc(dom, 0); + else { + if (ctl->conn->flags & VIR_DOMAIN_XML_SECURE) { + vshError(ctl, _("Cannot get password with read-only connection")); + goto cleanup; + }
We shouldn't have to do this filtering here. Just attempt the access always (when the options say to); it (better) fail at the driver level if the connection was read-only, for less work here in virsh, and so that we can actually validate that the security checking is being done at the driver level.
I see now the patch is all wrong, the check was supposed to be (ctl->conn->flags & VIR_CONNECT_RO) && VIR_DOMAIN_XML_SECURE anyway. I can omit the check, it will just result in different error message for the user, no problem with that. Sending a v2 (I never thought there could be this many errors in such a small patch). Martin
participants (4)
-
Daniel P. Berrange -
Doug Goldstein -
Eric Blake -
Martin Kletzander