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 <address@hidden>
>>>>>>>   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 <address@hidden>
>>>>>>> ---
>>>>>>>  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