1:51 p.m.
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
2:25 p.m.
On Mon, Jul 23, 2012 at 1:51 PM, Martin Kletzander <mkletzan(a)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
2:42 p.m.
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(a)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 :|
2:16 a.m.
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(a)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
2:58 a.m.
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(a)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 :|
3:33 a.m.
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(a)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
2:42 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:22 a.m.
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
4540
days inactive
4541
days old
7 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Doug Goldstein -
Eric Blake -
Martin Kletzander