Bug 1458630 - The comments for {spice,vnc,migrate,chardev}_tls_x509_cert_dir are misleading
Summary: The comments for {spice,vnc,migrate,chardev}_tls_x509_cert_dir are misleading
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: libvirt
Version: 7.4
Hardware: x86_64
OS: Linux
medium
medium
Target Milestone: rc
: ---
Assignee: John Ferlan
QA Contact: Fangge Jin
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-06-05 03:36 UTC by Fangge Jin
Modified: 2018-04-10 10:48 UTC (History)
9 users (show)

Fixed In Version: libvirt-3.9.0-1.el7
Doc Type: No Doc Update
Doc Text:
undefined
Clone Of:
Environment:
Last Closed: 2018-04-10 10:46:43 UTC
Target Upstream Version:


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Product Errata RHEA-2018:0704 None None None 2018-04-10 10:48:00 UTC

Description Fangge Jin 2017-06-05 03:36:26 UTC
Description of problem:
In qemu.conf, it says:

# In order to override the default TLS certificate location for
# spice certificates, supply a valid path to the certificate directory.
# If the provided path does not exist then the default_tls_x509_cert_dir
# path will be used.
#
# spice_tls_x509_cert_dir = "/etc/pki/libvirt-spiceaaa"


In my understanding, the logic is:
1) If no spice_tls_x509_cert_dir is configured, then the default location default_tls_x509_cert_dir is used
2) If spice_tls_x509_cert_dir is configured and:
2-1) the configured path is valid, then spice_tls_x509_cert_dir is used
2-2) the configured path is invalid, then default_tls_x509_cert_dir is used.

But in my test, I found that logic 2-2) doesn't work. See the steps below. The same problem exists with spice/vnc/chardev/migrate.

Version-Release number of selected component:
libvirt-3.2.0-7.virtcov.el7.x86_64

How reproducible:
100%

Steps to Reproduce:
0. Directory /etc/pki/libvirt-spiceaaa doesn't exist:
# ll /etc/pki/libvirt-spiceaaa
ls: cannot access /etc/pki/libvirt-spiceaaa: No such file or directory

1. Config qemu.conf as below and restart libvirtd:
default_tls_x509_cert_dir = "/etc/pki/qemu"
spice_tls = 1
spice_tls_x509_cert_dir = "/etc/pki/libvirt-spiceaaa"

2. Prepare a guest with graphics type=spice:
# virsh dumpxml rhel7.4
...
    <graphics type='spice' autoport='yes' listen='0.0.0.0' keymap='en-us'>
      <listen type='address' address='0.0.0.0'/>
    </graphics>
...

3. Start guest:
# virsh start rhel7.4
error: Failed to start domain rhel7.4
error: internal error: process exited while connecting to monitor: 2017-06-03T03:16:27.495162Z qemu-kvm: -chardev pty,id=charserial0: char device redirected to /dev/pts/1 (label charserial0)
((null):17086): Spice-Warning **: reds.c:2697:reds_init_ssl: Could not load certificates from /etc/pki/libvirt-spiceaaa/server-cert.pem
2017-06-03T03:16:27.500815Z qemu-kvm: failed to initialize spice server

Actual results:
Libvirt uses wrong path for spice tls when start guest.

Expected results:
Libvirt uses correct path for spice tls when start guest, which should be "/etc/pki/qemu"

Additional info:
Same problem exists with vnc/migrate/chardev.

Comment 2 Jiri Denemark 2017-06-05 07:18:02 UTC
Libvirt uses the path you told it to use. default_tls_x509_cert_dir is only used if the more specific option is not set.

Comment 3 Jiri Denemark 2017-06-05 07:21:11 UTC
Oh actually I see, the comment is quite misleading... Let's use this bz to fix the comment.

Comment 4 John Ferlan 2017-06-28 19:31:39 UTC
Not entirely a qemu.conf doc bug, but more a bug of using a defined value. Prior to reading the qemu.conf file, the various settings are initialized to /etc/pki/qemu (e.g. the default).  When overwriting, the existence was not checked. I've sent a couple of patches to handle this, see cover letter:

https://www.redhat.com/archives/libvir-list/2017-June/msg01278.html

Ironically, the default /etc/pki/qemu is not checked for existence and it cannot be checked as that would cause failure to start libvirtd even if no domain used TLS. Failure to have /etc/pki/qemu and failure to have the valid contents of that directory would still be handled by QEMU failure. That's been added to qemu.conf as part of these patches.

Comment 5 John Ferlan 2017-08-02 20:25:24 UTC
$ git describe dc4c2f75ab233561f47c8804667ed129dad05d69
v3.6.0-33-gdc4c2f7
$ git show
commit dc4c2f75ab233561f47c8804667ed129dad05d69
Author: John Ferlan <jferlan@redhat.com>
Date:   Thu Jun 29 08:27:55 2017 -0400

    qemu: Check for existence of provided *_tls_x509_cert_dir
    
...
    
    Introduce virQEMUDriverConfigTLSDirResetDefaults in order to check
    if the defaultTLSx509certdir was changed, then change the default
    for any other *TLSx509certdir that was not set to the default default.
    
    Introduce virQEMUDriverConfigValidate to validate the existence of
    any of the *_tls_x509_cert_dir values that were uncommented/set,
    incuding the default.
    
    Update the qemu.conf description for default to describe the consequences
    if the default directory path does not exist.
    
    Signed-off-by: John Ferlan <jferlan@redhat.com>

Comment 7 Fangge Jin 2017-10-26 08:47:38 UTC
Hi John

I tried to verify this bug with libvirt-3.8.0-1.virtcov.el7.x86_64.

The current behaviour is that if *_tls_x509_cert_dir is set to a path that doesn't exist, libvirtd will fail to start.

But the description in qemu.conf for {spice, vnc, migrate, chardev, vxhs}_tls_x509_cert_dir is not updated, it still says:

# In order to override the default TLS certificate location for
# spice certificates, supply a valid path to the certificate directory.
# If the provided path does not exist then the default_tls_x509_cert_dir
# path will be used.

Comment 8 Fangge Jin 2017-10-26 08:53:21 UTC
And the description for default_tls_x509_cert_dir is not correct, it says:

# If the directory does not exist or contain the necessary files, QEMU
# domains will fail to start if they are configured to use TLS.

Comment 9 John Ferlan 2017-10-26 12:30:21 UTC
The following excerpts are taken from source - tell me exactly what's wrong with the wording, provide an example of better wording, and I'll work to get it in.  If your system doesn't have the updated wording, then you may want to check if a libvirtd.conf.rpmnew file exists.

Tks -

John

...
# In order to overwrite the default path alter the following. This path
# definition will be used as the default path for other *_tls_x509_cert_dir
# configuration settings if their default path does not exist or is not
# specifically set.
#
#default_tls_x509_cert_dir = "/etc/pki/qemu"
...
# In order to override the default TLS certificate location for
# vnc certificates, supply a valid path to the certificate directory.
# If the provided path does not exist then the default_tls_x509_cert_dir
# path will be used.
#
#vnc_tls_x509_cert_dir = "/etc/pki/libvirt-vnc"
...
# In order to override the default TLS certificate location for
# spice certificates, supply a valid path to the certificate directory.
# If the provided path does not exist then the default_tls_x509_cert_dir
# path will be used.
#
#spice_tls_x509_cert_dir = "/etc/pki/libvirt-spice"
...
# In order to override the default TLS certificate location for character
# device TCP certificates, supply a valid path to the certificate directory.
# If the provided path does not exist then the default_tls_x509_cert_dir
# path will be used.
#
#chardev_tls_x509_cert_dir = "/etc/pki/libvirt-chardev"
...
#
# If the provided path does not exist then the default_tls_x509_cert_dir
# path will be used.
#
# VxHS block device clients expect the client certificate and key to be
# present in the certificate directory along with the CA master certificate.
# If using the default environment, default_tls_x509_verify must be configured.
# Since this is only a client the server-key.pem certificate is not needed.
# Thus a VxHS directory must contain the following:
#
#  ca-cert.pem - the CA master certificate
#  client-cert.pem - the client certificate signed with the ca-cert.pem
#  client-key.pem - the client private key
#
#vxhs_tls_x509_cert_dir = "/etc/pki/libvirt-vxhs"
...
# In order to override the default TLS certificate location for migration
# certificates, supply a valid path to the certificate directory. If the
# provided path does not exist then the default_tls_x509_cert_dir path
# will be used. Once/if a default certificate is enabled/defined, migration
# will then be able to use the certificate via migration API flags.
#
#migrate_tls_x509_cert_dir = "/etc/pki/libvirt-migrate"

Comment 10 Fangge Jin 2017-10-27 01:47:36 UTC
Hi John


1) For default_tls_x509_cert_dir, this sentence is not correct:

"# If the directory does not exist or contain the necessary files, QEMU
# domains will fail to start if they are configured to use TLS."


I think it should be like: 

"# If the directory does not exist, libvirtd will fail to start.
# If the directory doesn't contain the necessary files, QEMU
# domains will fail to start if they are configured to use TLS."


2) For other *_tls_x509_cert_dir, this sentence is not correct:

"# If the provided path does not exist then the default_tls_x509_cert_dir
# path will be used."


I think it should be like:

"# If the provided path does not exist, libvirtd will fail to start.
# If the default path for vnc_tls_cert_dir does not exist, then the 
# default_tls_x509_cert_dir path will be used."

Comment 11 John Ferlan 2017-10-27 09:28:38 UTC
No need for the needsinfo since you've provided the information I asked for!

In any case, for the non default tls path cases, it's more like:

# If the path is not provided, but vnc_tls = 1, then the
# default_tls_x509_cert_dir path will be used.

where "vnc" is also spice, chardev, vxhs, 


In any case, I've posted a patch - let's see what upstream says:

https://www.redhat.com/archives/libvir-list/2017-October/msg01276.html

Comment 12 John Ferlan 2017-10-27 09:55:51 UTC
Patch was accepted and pushed upstream:


commit c9f7a04e283ef43a0b358b2a337e884e20b86b3c
Author: John Ferlan <jferlan@redhat.com>
Date:   Fri Oct 27 05:23:25 2017 -0400

    qemu.conf: Clarify the various _tls_x509_cert_dir descriptions
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1458630
    
    Apparantly commit id 'dc4c2f75a' wasn't specific enough, so here's
    a few more clarifications.

$ git describe c9f7a04e283ef43a0b358b2a337e884e20b86b3c
CVE-2017-1000256-149-gc9f7a04e28
$

Comment 13 Fangge Jin 2017-11-07 10:20:11 UTC
Hi John

I still have other opinions about the description for *_tls_x509_cert_dir. Please have a look. Thank you!

Version: libvirt-3.9.0-1.el7.x86_64

1) For default_tls_x509_cert_dir, I think it will be more accurate if change the "or" to "and" in this sentence:

# In order to overwrite the default path alter the following. This path
# definition will be used as the default path for other *_tls_x509_cert_dir
# configuration settings if their default path does not exist or is not
                                                              ^^
# specifically set.
#
#default_tls_x509_cert_dir = "/etc/pki/qemu"


2) For other *_tls_x509_cert_dir, the following sentence is not correct. Because the actual behavior is that if the path is not provided, the default path /etc/pki/libvirt/vnc will used. If the default path doesn't exist, default_tls_x509_cert will be used.


# If the path is not provided, but vnc_tls = 1, then the
# default_tls_x509_cert_dir path will be used.
#
#vnc_tls_x509_cert_dir = "/etc/pki/libvirt-vnc"

Comment 14 John Ferlan 2017-11-07 17:10:02 UTC
1) I think "or" is fine there

For the non default TLS dirs if what's shown as the default directory exists, then that's saved in the cfg->*TLSx509certdir field at cfg initialization; however, if that directory doesn't exist, then whatever was used for the defaultTLSx509certdir is duplicated. So it's either the "provided" path for each other tls dir or it will be whatever the default is set to.

Although it's perhaps semantics, the "is not specifically set" implies uncommenting the field. If someone (for whatever reason) changed the commented field to something else and expected that to be used, they'd be mistaken.

Using "and" to me could imply that if someone supplied a path and it didn't exist, then the default would be used. That is not the case, if someone provides an uncommented path and it doesn't exist, then an error 

For example, if the "/etc/pki/libvirt-vnc" directory exists, then internally cfg->vncTLSx509certdir will be "/etc/pki/libvirt-vnc" regardless of whether vnc_tls = 1 is uncommented.  If the "/etc/pki/libvirt-vnc" doesn't exist, then 
cfg->vncTLSx509certdir will be whatever is found in cfg->defaultTLSx509certdir.


2) I disagree

While I see your point, I think if someone reads the default_tls_x509_cert_dir description they could easily infer that the path used will be either what's shown in the commented value for the *_tls_x509_cert_dir or whatever the default_tls_x509_cert_dir value was set to in the event that commented path doesn't exist. It's very difficult to describe though in "precise wording" that won't be too verbose.

The best this could be changed to would be:

"If the path is not provided or the default shown commented path does not exist, but vnc_tls = 1, then the default_tls_x509_cert_dir path will be used."

But to me that's really overkill since the default_tls_x509_cert_dir path indicates "...This path definition will be used as the default path for other *_tls_x509_cert_dir configuration settings if their default path does not exist or is not specifically set."

Comment 19 errata-xmlrpc 2018-04-10 10:46:43 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHEA-2018:0704


Note You need to log in before you can comment on or make changes to this bug.