[Qemu-devel] [PATCH] ui: fix regression in x509verify parameter for VNC server

[Qemu-devel] [PATCH] ui: fix regression in x509verify parameter for VNC server

[Daniel P. Berrange]

The 'x509verify' parameter is documented as taking a path to the
x509 certificates, ie the same syntax as the 'x509' parameter.

  commit 4db14629c38611061fc19ec6927405923de84f08
  Author: Gerd Hoffmann <kraxel@redhat.com>
  Date:   Tue Sep 16 12:33:03 2014 +0200

    vnc: switch to QemuOpts, allow multiple servers

caused a regression by turning 'x509verify' into a boolean
parameter instead. This breaks setup from libvirt and is not
consistent with the docs.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 ui/vnc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 10a2724..37290e7 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -3304,7 +3304,7 @@ static QemuOptsList qemu_vnc_opts = {
             .type = QEMU_OPT_BOOL,
         },{
             .name = "x509verify",
-            .type = QEMU_OPT_BOOL,
+            .type = QEMU_OPT_STRING,
         },{
             .name = "acl",
             .type = QEMU_OPT_BOOL,
@@ -3391,9 +3391,14 @@ void vnc_display_open(const char *id, Error **errp)
 #ifdef CONFIG_VNC_TLS
     tls  = qemu_opt_get_bool(opts, "tls", false);
     path = qemu_opt_get(opts, "x509");
+    if (!path) {
+        path = qemu_opt_get(opts, "x509verify");
+        if (path) {
+            vs->tls.x509verify = true;
+        }
+    }
     if (path) {
         x509 = true;
-        vs->tls.x509verify = qemu_opt_get_bool(opts, "x509verify", false);
         if (vnc_tls_set_x509_creds_dir(vs, path) < 0) {
             error_setg(errp, "Failed to find x509 certificates/keys in %s",
                        path);
-- 
2.1.0

Re: [Qemu-devel] [PATCH] ui: fix regression in x509verify parameter for VNC server

[Gonglei]

On 2015/3/11 0:27, Daniel P. Berrange wrote:
> The 'x509verify' parameter is documented as taking a path to the
> x509 certificates, ie the same syntax as the 'x509' parameter.
> 
>   commit 4db14629c38611061fc19ec6927405923de84f08
>   Author: Gerd Hoffmann <kraxel@redhat.com>
>   Date:   Tue Sep 16 12:33:03 2014 +0200
> 
>     vnc: switch to QemuOpts, allow multiple servers
> 
> caused a regression by turning 'x509verify' into a boolean
> parameter instead. This breaks setup from libvirt and is not
> consistent with the docs.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  ui/vnc.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 10a2724..37290e7 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -3304,7 +3304,7 @@ static QemuOptsList qemu_vnc_opts = {
>              .type = QEMU_OPT_BOOL,
>          },{
>              .name = "x509verify",
> -            .type = QEMU_OPT_BOOL,
> +            .type = QEMU_OPT_STRING,
>          },{
>              .name = "acl",
>              .type = QEMU_OPT_BOOL,
> @@ -3391,9 +3391,14 @@ void vnc_display_open(const char *id, Error **errp)
>  #ifdef CONFIG_VNC_TLS
>      tls  = qemu_opt_get_bool(opts, "tls", false);
>      path = qemu_opt_get(opts, "x509");
> +    if (!path) {
> +        path = qemu_opt_get(opts, "x509verify");
> +        if (path) {
> +            vs->tls.x509verify = true;
> +        }
> +    }
>      if (path) {
>          x509 = true;
> -        vs->tls.x509verify = qemu_opt_get_bool(opts, "x509verify", false);

We still need to get the x509verify value, while both 'x509' and 'x509verify'
are configured, isn't it?

Regards,
-Gonglei

>          if (vnc_tls_set_x509_creds_dir(vs, path) < 0) {
>              error_setg(errp, "Failed to find x509 certificates/keys in %s",
>                         path);
> 

Re: [Qemu-devel] [PATCH] ui: fix regression in x509verify parameter for VNC server

[Daniel P. Berrange]

On Wed, Mar 11, 2015 at 09:48:46AM +0800, Gonglei wrote:
> On 2015/3/11 0:27, Daniel P. Berrange wrote:
> > The 'x509verify' parameter is documented as taking a path to the
> > x509 certificates, ie the same syntax as the 'x509' parameter.
> > 
> >   commit 4db14629c38611061fc19ec6927405923de84f08
> >   Author: Gerd Hoffmann <kraxel@redhat.com>
> >   Date:   Tue Sep 16 12:33:03 2014 +0200
> > 
> >     vnc: switch to QemuOpts, allow multiple servers
> > 
> > caused a regression by turning 'x509verify' into a boolean
> > parameter instead. This breaks setup from libvirt and is not
> > consistent with the docs.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  ui/vnc.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/ui/vnc.c b/ui/vnc.c
> > index 10a2724..37290e7 100644
> > --- a/ui/vnc.c
> > +++ b/ui/vnc.c
> > @@ -3304,7 +3304,7 @@ static QemuOptsList qemu_vnc_opts = {
> >              .type = QEMU_OPT_BOOL,
> >          },{
> >              .name = "x509verify",
> > -            .type = QEMU_OPT_BOOL,
> > +            .type = QEMU_OPT_STRING,
> >          },{
> >              .name = "acl",
> >              .type = QEMU_OPT_BOOL,
> > @@ -3391,9 +3391,14 @@ void vnc_display_open(const char *id, Error **errp)
> >  #ifdef CONFIG_VNC_TLS
> >      tls  = qemu_opt_get_bool(opts, "tls", false);
> >      path = qemu_opt_get(opts, "x509");
> > +    if (!path) {
> > +        path = qemu_opt_get(opts, "x509verify");
> > +        if (path) {
> > +            vs->tls.x509verify = true;
> > +        }
> > +    }
> >      if (path) {
> >          x509 = true;
> > -        vs->tls.x509verify = qemu_opt_get_bool(opts, "x509verify", false);
> 
> We still need to get the x509verify value, while both 'x509' and 'x509verify'
> are configured, isn't it?

Err, the code 7 lines earlier gets the x509verify value. The x509 and x509verify
parameters are never to be specified at the same time - either one or the other
is used.


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] ui: fix regression in x509verify parameter for VNC server

[Gonglei]

On 2015/3/11 17:45, Daniel P. Berrange wrote:
> On Wed, Mar 11, 2015 at 09:48:46AM +0800, Gonglei wrote:
>> On 2015/3/11 0:27, Daniel P. Berrange wrote:
>>> The 'x509verify' parameter is documented as taking a path to the
>>> x509 certificates, ie the same syntax as the 'x509' parameter.
>>>
>>>   commit 4db14629c38611061fc19ec6927405923de84f08
>>>   Author: Gerd Hoffmann <kraxel@redhat.com>
>>>   Date:   Tue Sep 16 12:33:03 2014 +0200
>>>
>>>     vnc: switch to QemuOpts, allow multiple servers
>>>
>>> caused a regression by turning 'x509verify' into a boolean
>>> parameter instead. This breaks setup from libvirt and is not
>>> consistent with the docs.
>>>
>>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>>> ---
>>>  ui/vnc.c | 9 +++++++--
>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/ui/vnc.c b/ui/vnc.c
>>> index 10a2724..37290e7 100644
>>> --- a/ui/vnc.c
>>> +++ b/ui/vnc.c
>>> @@ -3304,7 +3304,7 @@ static QemuOptsList qemu_vnc_opts = {
>>>              .type = QEMU_OPT_BOOL,
>>>          },{
>>>              .name = "x509verify",
>>> -            .type = QEMU_OPT_BOOL,
>>> +            .type = QEMU_OPT_STRING,
>>>          },{
>>>              .name = "acl",
>>>              .type = QEMU_OPT_BOOL,
>>> @@ -3391,9 +3391,14 @@ void vnc_display_open(const char *id, Error **errp)
>>>  #ifdef CONFIG_VNC_TLS
>>>      tls  = qemu_opt_get_bool(opts, "tls", false);
>>>      path = qemu_opt_get(opts, "x509");
>>> +    if (!path) {
>>> +        path = qemu_opt_get(opts, "x509verify");
>>> +        if (path) {
>>> +            vs->tls.x509verify = true;
>>> +        }
>>> +    }
>>>      if (path) {
>>>          x509 = true;
>>> -        vs->tls.x509verify = qemu_opt_get_bool(opts, "x509verify", false);
>>
>> We still need to get the x509verify value, while both 'x509' and 'x509verify'
>> are configured, isn't it?
> 
> Err, the code 7 lines earlier gets the x509verify value. The x509 and x509verify
> parameters are never to be specified at the same time - either one or the other
> is used.
> 
I noticed that there are several places that check x509verify (or vs->tls.x509verify)
value:

./ui/vnc-auth-vencrypt.c:85:    if (vs->vd->tls.x509verify) {
./ui/vnc-tls.c:260:            if (vs->vd->tls.x509verify) {
./ui/vnc-tls.c:388:            if (vs->vd->tls.x509verify) {
./ui/vnc.c:3212:    vs->tls.x509verify = 0;
./ui/vnc.c:3306:            .name = "x509verify",
./ui/vnc.c:3396:        vs->tls.x509verify = qemu_opt_get_bool(opts, "x509verify", false);
./ui/vnc.c:3456:    if (acl && x509 && vs->tls.x509verify) {

If only 'x509' parameter is specified, can vnc-tls work after applying this patch?
Am I missing something?

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH] ui: fix regression in x509verify parameter for VNC server

[Daniel P. Berrange]

On Wed, Mar 11, 2015 at 07:07:49PM +0800, Gonglei wrote:
> On 2015/3/11 17:45, Daniel P. Berrange wrote:
> > On Wed, Mar 11, 2015 at 09:48:46AM +0800, Gonglei wrote:
> >> On 2015/3/11 0:27, Daniel P. Berrange wrote:
> >>> The 'x509verify' parameter is documented as taking a path to the
> >>> x509 certificates, ie the same syntax as the 'x509' parameter.
> >>>
> >>>   commit 4db14629c38611061fc19ec6927405923de84f08
> >>>   Author: Gerd Hoffmann <kraxel@redhat.com>
> >>>   Date:   Tue Sep 16 12:33:03 2014 +0200
> >>>
> >>>     vnc: switch to QemuOpts, allow multiple servers
> >>>
> >>> caused a regression by turning 'x509verify' into a boolean
> >>> parameter instead. This breaks setup from libvirt and is not
> >>> consistent with the docs.
> >>>
> >>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> >>> ---
> >>>  ui/vnc.c | 9 +++++++--
> >>>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/ui/vnc.c b/ui/vnc.c
> >>> index 10a2724..37290e7 100644
> >>> --- a/ui/vnc.c
> >>> +++ b/ui/vnc.c
> >>> @@ -3304,7 +3304,7 @@ static QemuOptsList qemu_vnc_opts = {
> >>>              .type = QEMU_OPT_BOOL,
> >>>          },{
> >>>              .name = "x509verify",
> >>> -            .type = QEMU_OPT_BOOL,
> >>> +            .type = QEMU_OPT_STRING,
> >>>          },{
> >>>              .name = "acl",
> >>>              .type = QEMU_OPT_BOOL,
> >>> @@ -3391,9 +3391,14 @@ void vnc_display_open(const char *id, Error **errp)
> >>>  #ifdef CONFIG_VNC_TLS
> >>>      tls  = qemu_opt_get_bool(opts, "tls", false);
> >>>      path = qemu_opt_get(opts, "x509");
> >>> +    if (!path) {
> >>> +        path = qemu_opt_get(opts, "x509verify");
> >>> +        if (path) {
> >>> +            vs->tls.x509verify = true;
> >>> +        }
> >>> +    }
> >>>      if (path) {
> >>>          x509 = true;
> >>> -        vs->tls.x509verify = qemu_opt_get_bool(opts, "x509verify", false);
> >>
> >> We still need to get the x509verify value, while both 'x509' and 'x509verify'
> >> are configured, isn't it?
> > 
> > Err, the code 7 lines earlier gets the x509verify value. The x509 and x509verify
> > parameters are never to be specified at the same time - either one or the other
> > is used.
> > 
> I noticed that there are several places that check x509verify (or vs->tls.x509verify)
> value:
> 
> ./ui/vnc-auth-vencrypt.c:85:    if (vs->vd->tls.x509verify) {
> ./ui/vnc-tls.c:260:            if (vs->vd->tls.x509verify) {
> ./ui/vnc-tls.c:388:            if (vs->vd->tls.x509verify) {
> ./ui/vnc.c:3212:    vs->tls.x509verify = 0;
> ./ui/vnc.c:3306:            .name = "x509verify",
> ./ui/vnc.c:3396:        vs->tls.x509verify = qemu_opt_get_bool(opts, "x509verify", false);
> ./ui/vnc.c:3456:    if (acl && x509 && vs->tls.x509verify) {
> 
> If only 'x509' parameter is specified, can vnc-tls work after applying this patch?
> Am I missing something?

x509 only says that the server must provide x509 certs to the client. The
x509verify says that the server must also verify the client's certificate.
So there are obviously extra codepaths for the latter.

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] ui: fix regression in x509verify parameter for VNC server

[Gonglei]

On 2015/3/11 19:10, Daniel P. Berrange wrote:
> On Wed, Mar 11, 2015 at 07:07:49PM +0800, Gonglei wrote:
>> On 2015/3/11 17:45, Daniel P. Berrange wrote:
>>> On Wed, Mar 11, 2015 at 09:48:46AM +0800, Gonglei wrote:
>>>> On 2015/3/11 0:27, Daniel P. Berrange wrote:
>>>>> The 'x509verify' parameter is documented as taking a path to the
>>>>> x509 certificates, ie the same syntax as the 'x509' parameter.
>>>>>
>>>>>   commit 4db14629c38611061fc19ec6927405923de84f08
>>>>>   Author: Gerd Hoffmann <kraxel@redhat.com>
>>>>>   Date:   Tue Sep 16 12:33:03 2014 +0200
>>>>>
>>>>>     vnc: switch to QemuOpts, allow multiple servers
>>>>>
>>>>> caused a regression by turning 'x509verify' into a boolean
>>>>> parameter instead. This breaks setup from libvirt and is not
>>>>> consistent with the docs.
>>>>>
>>>>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>>>>> ---
>>>>>  ui/vnc.c | 9 +++++++--
>>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/ui/vnc.c b/ui/vnc.c
>>>>> index 10a2724..37290e7 100644
>>>>> --- a/ui/vnc.c
>>>>> +++ b/ui/vnc.c
>>>>> @@ -3304,7 +3304,7 @@ static QemuOptsList qemu_vnc_opts = {
>>>>>              .type = QEMU_OPT_BOOL,
>>>>>          },{
>>>>>              .name = "x509verify",
>>>>> -            .type = QEMU_OPT_BOOL,
>>>>> +            .type = QEMU_OPT_STRING,
>>>>>          },{
>>>>>              .name = "acl",
>>>>>              .type = QEMU_OPT_BOOL,
>>>>> @@ -3391,9 +3391,14 @@ void vnc_display_open(const char *id, Error **errp)
>>>>>  #ifdef CONFIG_VNC_TLS
>>>>>      tls  = qemu_opt_get_bool(opts, "tls", false);
>>>>>      path = qemu_opt_get(opts, "x509");
>>>>> +    if (!path) {
>>>>> +        path = qemu_opt_get(opts, "x509verify");
>>>>> +        if (path) {
>>>>> +            vs->tls.x509verify = true;
>>>>> +        }
>>>>> +    }
>>>>>      if (path) {
>>>>>          x509 = true;
>>>>> -        vs->tls.x509verify = qemu_opt_get_bool(opts, "x509verify", false);
>>>>
>>>> We still need to get the x509verify value, while both 'x509' and 'x509verify'
>>>> are configured, isn't it?
>>>
>>> Err, the code 7 lines earlier gets the x509verify value. The x509 and x509verify
>>> parameters are never to be specified at the same time - either one or the other
>>> is used.
>>>
>> I noticed that there are several places that check x509verify (or vs->tls.x509verify)
>> value:
>>
>> ./ui/vnc-auth-vencrypt.c:85:    if (vs->vd->tls.x509verify) {
>> ./ui/vnc-tls.c:260:            if (vs->vd->tls.x509verify) {
>> ./ui/vnc-tls.c:388:            if (vs->vd->tls.x509verify) {
>> ./ui/vnc.c:3212:    vs->tls.x509verify = 0;
>> ./ui/vnc.c:3306:            .name = "x509verify",
>> ./ui/vnc.c:3396:        vs->tls.x509verify = qemu_opt_get_bool(opts, "x509verify", false);
>> ./ui/vnc.c:3456:    if (acl && x509 && vs->tls.x509verify) {
>>
>> If only 'x509' parameter is specified, can vnc-tls work after applying this patch?
>> Am I missing something?
> 
> x509 only says that the server must provide x509 certs to the client. The
> x509verify says that the server must also verify the client's certificate.
> So there are obviously extra codepaths for the latter.
> 
Okay, thanks for your explanation. :)

But since their different functions, Qemu should support the scenario that
those two parameters are specified at the same time (by Qemu command
line) IMHO because they are not mutually exclusive, isn't it?

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH] ui: fix regression in x509verify parameter for VNC server

[Daniel P. Berrange]

On Wed, Mar 11, 2015 at 07:24:58PM +0800, Gonglei wrote:
> On 2015/3/11 19:10, Daniel P. Berrange wrote:
> > On Wed, Mar 11, 2015 at 07:07:49PM +0800, Gonglei wrote:
> >> On 2015/3/11 17:45, Daniel P. Berrange wrote:
> >>> On Wed, Mar 11, 2015 at 09:48:46AM +0800, Gonglei wrote:
> >>>> On 2015/3/11 0:27, Daniel P. Berrange wrote:
> >>>>> The 'x509verify' parameter is documented as taking a path to the
> >>>>> x509 certificates, ie the same syntax as the 'x509' parameter.
> >>>>>
> >>>>>   commit 4db14629c38611061fc19ec6927405923de84f08
> >>>>>   Author: Gerd Hoffmann <kraxel@redhat.com>
> >>>>>   Date:   Tue Sep 16 12:33:03 2014 +0200
> >>>>>
> >>>>>     vnc: switch to QemuOpts, allow multiple servers
> >>>>>
> >>>>> caused a regression by turning 'x509verify' into a boolean
> >>>>> parameter instead. This breaks setup from libvirt and is not
> >>>>> consistent with the docs.
> >>>>>
> >>>>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> >>>>> ---
> >>>>>  ui/vnc.c | 9 +++++++--
> >>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/ui/vnc.c b/ui/vnc.c
> >>>>> index 10a2724..37290e7 100644
> >>>>> --- a/ui/vnc.c
> >>>>> +++ b/ui/vnc.c
> >>>>> @@ -3304,7 +3304,7 @@ static QemuOptsList qemu_vnc_opts = {
> >>>>>              .type = QEMU_OPT_BOOL,
> >>>>>          },{
> >>>>>              .name = "x509verify",
> >>>>> -            .type = QEMU_OPT_BOOL,
> >>>>> +            .type = QEMU_OPT_STRING,
> >>>>>          },{
> >>>>>              .name = "acl",
> >>>>>              .type = QEMU_OPT_BOOL,
> >>>>> @@ -3391,9 +3391,14 @@ void vnc_display_open(const char *id, Error **errp)
> >>>>>  #ifdef CONFIG_VNC_TLS
> >>>>>      tls  = qemu_opt_get_bool(opts, "tls", false);
> >>>>>      path = qemu_opt_get(opts, "x509");
> >>>>> +    if (!path) {
> >>>>> +        path = qemu_opt_get(opts, "x509verify");
> >>>>> +        if (path) {
> >>>>> +            vs->tls.x509verify = true;
> >>>>> +        }
> >>>>> +    }
> >>>>>      if (path) {
> >>>>>          x509 = true;
> >>>>> -        vs->tls.x509verify = qemu_opt_get_bool(opts, "x509verify", false);
> >>>>
> >>>> We still need to get the x509verify value, while both 'x509' and 'x509verify'
> >>>> are configured, isn't it?
> >>>
> >>> Err, the code 7 lines earlier gets the x509verify value. The x509 and x509verify
> >>> parameters are never to be specified at the same time - either one or the other
> >>> is used.
> >>>
> >> I noticed that there are several places that check x509verify (or vs->tls.x509verify)
> >> value:
> >>
> >> ./ui/vnc-auth-vencrypt.c:85:    if (vs->vd->tls.x509verify) {
> >> ./ui/vnc-tls.c:260:            if (vs->vd->tls.x509verify) {
> >> ./ui/vnc-tls.c:388:            if (vs->vd->tls.x509verify) {
> >> ./ui/vnc.c:3212:    vs->tls.x509verify = 0;
> >> ./ui/vnc.c:3306:            .name = "x509verify",
> >> ./ui/vnc.c:3396:        vs->tls.x509verify = qemu_opt_get_bool(opts, "x509verify", false);
> >> ./ui/vnc.c:3456:    if (acl && x509 && vs->tls.x509verify) {
> >>
> >> If only 'x509' parameter is specified, can vnc-tls work after applying this patch?
> >> Am I missing something?
> > 
> > x509 only says that the server must provide x509 certs to the client. The
> > x509verify says that the server must also verify the client's certificate.
> > So there are obviously extra codepaths for the latter.
> > 
> Okay, thanks for your explanation. :)
> 
> But since their different functions, Qemu should support the scenario that
> those two parameters are specified at the same time (by Qemu command
> line) IMHO because they are not mutually exclusive, isn't it?

It does not make sense to supply both at the same time, as that would
mean you were giving QEMU two sets of x509 certificates for the same
server. Also since x509verify is a superset of x509, there is no reason
to need to specify both at once.

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] ui: fix regression in x509verify parameter for VNC server

[Gonglei]

On 2015/3/11 19:27, Daniel P. Berrange wrote:
> On Wed, Mar 11, 2015 at 07:24:58PM +0800, Gonglei wrote:
>> On 2015/3/11 19:10, Daniel P. Berrange wrote:
>>> On Wed, Mar 11, 2015 at 07:07:49PM +0800, Gonglei wrote:
>>>> On 2015/3/11 17:45, Daniel P. Berrange wrote:
>>>>> On Wed, Mar 11, 2015 at 09:48:46AM +0800, Gonglei wrote:
>>>>>> On 2015/3/11 0:27, Daniel P. Berrange wrote:
>>>>>>> The 'x509verify' parameter is documented as taking a path to the
>>>>>>> x509 certificates, ie the same syntax as the 'x509' parameter.
>>>>>>>
>>>>>>>   commit 4db14629c38611061fc19ec6927405923de84f08
>>>>>>>   Author: Gerd Hoffmann <kraxel@redhat.com>
>>>>>>>   Date:   Tue Sep 16 12:33:03 2014 +0200
>>>>>>>
>>>>>>>     vnc: switch to QemuOpts, allow multiple servers
>>>>>>>
>>>>>>> caused a regression by turning 'x509verify' into a boolean
>>>>>>> parameter instead. This breaks setup from libvirt and is not
>>>>>>> consistent with the docs.
>>>>>>>
>>>>>>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>>>>>>> ---
>>>>>>>  ui/vnc.c | 9 +++++++--
>>>>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/ui/vnc.c b/ui/vnc.c
>>>>>>> index 10a2724..37290e7 100644
>>>>>>> --- a/ui/vnc.c
>>>>>>> +++ b/ui/vnc.c
>>>>>>> @@ -3304,7 +3304,7 @@ static QemuOptsList qemu_vnc_opts = {
>>>>>>>              .type = QEMU_OPT_BOOL,
>>>>>>>          },{
>>>>>>>              .name = "x509verify",
>>>>>>> -            .type = QEMU_OPT_BOOL,
>>>>>>> +            .type = QEMU_OPT_STRING,
>>>>>>>          },{
>>>>>>>              .name = "acl",
>>>>>>>              .type = QEMU_OPT_BOOL,
>>>>>>> @@ -3391,9 +3391,14 @@ void vnc_display_open(const char *id, Error **errp)
>>>>>>>  #ifdef CONFIG_VNC_TLS
>>>>>>>      tls  = qemu_opt_get_bool(opts, "tls", false);
>>>>>>>      path = qemu_opt_get(opts, "x509");
>>>>>>> +    if (!path) {
>>>>>>> +        path = qemu_opt_get(opts, "x509verify");
>>>>>>> +        if (path) {
>>>>>>> +            vs->tls.x509verify = true;
>>>>>>> +        }
>>>>>>> +    }
>>>>>>>      if (path) {
>>>>>>>          x509 = true;
>>>>>>> -        vs->tls.x509verify = qemu_opt_get_bool(opts, "x509verify", false);
>>>>>>
>>>>>> We still need to get the x509verify value, while both 'x509' and 'x509verify'
>>>>>> are configured, isn't it?
>>>>>
>>>>> Err, the code 7 lines earlier gets the x509verify value. The x509 and x509verify
>>>>> parameters are never to be specified at the same time - either one or the other
>>>>> is used.
>>>>>
>>>> I noticed that there are several places that check x509verify (or vs->tls.x509verify)
>>>> value:
>>>>
>>>> ./ui/vnc-auth-vencrypt.c:85:    if (vs->vd->tls.x509verify) {
>>>> ./ui/vnc-tls.c:260:            if (vs->vd->tls.x509verify) {
>>>> ./ui/vnc-tls.c:388:            if (vs->vd->tls.x509verify) {
>>>> ./ui/vnc.c:3212:    vs->tls.x509verify = 0;
>>>> ./ui/vnc.c:3306:            .name = "x509verify",
>>>> ./ui/vnc.c:3396:        vs->tls.x509verify = qemu_opt_get_bool(opts, "x509verify", false);
>>>> ./ui/vnc.c:3456:    if (acl && x509 && vs->tls.x509verify) {
>>>>
>>>> If only 'x509' parameter is specified, can vnc-tls work after applying this patch?
>>>> Am I missing something?
>>>
>>> x509 only says that the server must provide x509 certs to the client. The
>>> x509verify says that the server must also verify the client's certificate.
>>> So there are obviously extra codepaths for the latter.
>>>
>> Okay, thanks for your explanation. :)
>>
>> But since their different functions, Qemu should support the scenario that
>> those two parameters are specified at the same time (by Qemu command
>> line) IMHO because they are not mutually exclusive, isn't it?
> 
> It does not make sense to supply both at the same time, as that would
> mean you were giving QEMU two sets of x509 certificates for the same
> server. Also since x509verify is a superset of x509, there is no reason
> to need to specify both at once.
> 
> Regards,
> Daniel
> 
Get it, thanks.

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH] ui: fix regression in x509verify parameter for VNC server

[Gerd Hoffmann]

On Di, 2015-03-10 at 16:27 +0000, Daniel P. Berrange wrote:
> The 'x509verify' parameter is documented as taking a path to the
> x509 certificates, ie the same syntax as the 'x509' parameter.
> 
>   commit 4db14629c38611061fc19ec6927405923de84f08
>   Author: Gerd Hoffmann <kraxel@redhat.com>
>   Date:   Tue Sep 16 12:33:03 2014 +0200
> 
>     vnc: switch to QemuOpts, allow multiple servers
> 
> caused a regression by turning 'x509verify' into a boolean
> parameter instead. This breaks setup from libvirt and is not
> consistent with the docs.

added to vnc patch queue.

thanks,
  Gerd