https://bugzilla.redhat.com/show_bug.cgi?id=1458630 Introduce virQEMUDriverConfigSetCertDir which will handle reading the qemu.conf config file specific setting for default, vnc, spice, chardev, and migrate. If a setting is provided, then validate the existence of the directory and overwrite the default set by virQEMUDriverConfigNew. Update the qemu.conf description for default to describe the consequences if the default directory path does not exist and as well as the descriptions for each of the *_tls_x509_cert_dir entries. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- v1: https://www.redhat.com/archives/libvir-list/2017-June/msg01278.html - Dropped the former 1/2 patch - Alter the logic of virQEMUDriverConfigSetCertDir to fail instead of VIR_INFO if an uncommented entry for one of the *_tls_x509_cert_dir has a path that does not exist. This will cause a libvirtd startup failure as opposed to the previous logic which would have failed only when a domain using TLS was started. - Alter the description for each of the values to more accurately describe what happens. src/qemu/qemu.conf | 29 ++++++++++++++++++++--------- src/qemu/qemu_conf.c | 38 +++++++++++++++++++++++++++++++++----- 2 files changed, 53 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index e6c0832..b0ccffb 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -3,7 +3,7 @@ # defaults are used. # Use of TLS requires that x509 certificates be issued. The default is -# to keep them in /etc/pki/qemu. This directory must contain +# to keep them in /etc/pki/qemu. This directory must exist and contain: # # ca-cert.pem - the CA master certificate # server-cert.pem - the server certificate signed with ca-cert.pem @@ -13,6 +13,12 @@ # # dh-params.pem - the DH params configuration file # +# If the directory does not exist or does not contain the necessary files, +# QEMU domains will fail to start if they are configured to use TLS. +# +# In order to overwrite the default path alter the following. If the provided +# path does not exist, then startup will fail. +# #default_tls_x509_cert_dir = "/etc/pki/qemu" @@ -79,8 +85,9 @@ # 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. +# If the default listed here does not exist, then the default /etc/pki/qemu +# is used. If uncommented and the provided path does not exist, then startup +# will fail. # #vnc_tls_x509_cert_dir = "/etc/pki/libvirt-vnc" @@ -164,8 +171,9 @@ # 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. +# If the default listed here does not exist, then the default /etc/pki/qemu +# is used. If uncommented and the provided path does not exist, then startup +# will fail. # #spice_tls_x509_cert_dir = "/etc/pki/libvirt-spice" @@ -216,8 +224,9 @@ # 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. +# If the default listed here does not exist, then the default /etc/pki/qemu +# is used. If uncommented and the provided path does not exist, then startup +# will fail. # #chardev_tls_x509_cert_dir = "/etc/pki/libvirt-chardev" @@ -252,8 +261,10 @@ # 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 +# default listed here does not exist, then the default /etc/pki/qemu is used. +# If uncommented and the provided path does not exist, then startup will fail. +# +# 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" diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 73c33d6..4eb6f0c 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -440,6 +440,34 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs, } +static int +virQEMUDriverConfigSetCertDir(virConfPtr conf, + const char *setting, + char **value) +{ + char *tlsCertDir = NULL; + + if (virConfGetValueString(conf, setting, &tlsCertDir) < 0) + return -1; + + if (!tlsCertDir) + return 0; + + if (!virFileExists(tlsCertDir)) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("directory '%s' does not exist for setting '%s'"), + tlsCertDir, setting); + VIR_FREE(tlsCertDir); + return -1; + } else { + VIR_FREE(*value); + VIR_STEAL_PTR(*value, tlsCertDir); + } + + return 0; +} + + int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, const char *filename, bool privileged) @@ -467,7 +495,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (!(conf = virConfReadFile(filename, 0))) goto cleanup; - if (virConfGetValueString(conf, "default_tls_x509_cert_dir", &cfg->defaultTLSx509certdir) < 0) + if (virQEMUDriverConfigSetCertDir(conf, "default_tls_x509_cert_dir", &cfg->defaultTLSx509certdir) < 0) goto cleanup; if (virConfGetValueBool(conf, "default_tls_x509_verify", &cfg->defaultTLSx509verify) < 0) goto cleanup; @@ -483,7 +511,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (rv == 0) cfg->vncTLSx509verify = cfg->defaultTLSx509verify; - if (virConfGetValueString(conf, "vnc_tls_x509_cert_dir", &cfg->vncTLSx509certdir) < 0) + if (virQEMUDriverConfigSetCertDir(conf, "vnc_tls_x509_cert_dir", &cfg->vncTLSx509certdir) < 0) goto cleanup; if (virConfGetValueString(conf, "vnc_listen", &cfg->vncListen) < 0) goto cleanup; @@ -521,7 +549,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (virConfGetValueBool(conf, "spice_tls", &cfg->spiceTLS) < 0) goto cleanup; - if (virConfGetValueString(conf, "spice_tls_x509_cert_dir", &cfg->spiceTLSx509certdir) < 0) + if (virQEMUDriverConfigSetCertDir(conf, "spice_tls_x509_cert_dir", &cfg->spiceTLSx509certdir) < 0) goto cleanup; if (virConfGetValueBool(conf, "spice_sasl", &cfg->spiceSASL) < 0) goto cleanup; @@ -541,8 +569,8 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; \ if (rv == 0) \ cfg->val## TLSx509verify = cfg->defaultTLSx509verify; \ - if (virConfGetValueString(conf, #val "_tls_x509_cert_dir", \ - &cfg->val## TLSx509certdir) < 0) \ + if (virQEMUDriverConfigSetCertDir(conf, #val "_tls_x509_cert_dir", \ + &cfg->val## TLSx509certdir) < 0) \ goto cleanup; \ if (virConfGetValueString(conf, \ #val "_tls_x509_secret_uuid", \

On Thu, Jun 29, 2017 at 10:32:35AM -0400, John Ferlan wrote: > https://bugzilla.redhat.com/show_bug.cgi?id=1458630 > > Introduce virQEMUDriverConfigSetCertDir which will handle reading the > qemu.conf config file specific setting for default, vnc, spice, chardev, > and migrate. If a setting is provided, then validate the existence of the > directory and overwrite the default set by virQEMUDriverConfigNew. > > Update the qemu.conf description for default to describe the consequences > if the default directory path does not exist and as well as the > descriptions > for each of the *_tls_x509_cert_dir entries. > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > > v1: https://www.redhat.com/archives/libvir-list/2017-June/msg01278.html > > - Dropped the former 1/2 patch > > - Alter the logic of virQEMUDriverConfigSetCertDir to fail instead of > VIR_INFO if an uncommented entry for one of the *_tls_x509_cert_dir > has a path that does not exist. This will cause a libvirtd startup > failure as opposed to the previous logic which would have failed only > when a domain using TLS was started. > > - Alter the description for each of the values to more accurately > describe > what happens. > > src/qemu/qemu.conf | 29 ++++++++++++++++++++--------- > src/qemu/qemu_conf.c | 38 +++++++++++++++++++++++++++++++++----- > 2 files changed, 53 insertions(+), 14 deletions(-) > > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf > index e6c0832..b0ccffb 100644 > --- a/src/qemu/qemu.conf > +++ b/src/qemu/qemu.conf > @@ -3,7 +3,7 @@ > # defaults are used. > > # Use of TLS requires that x509 certificates be issued. The default is > -# to keep them in /etc/pki/qemu. This directory must contain > +# to keep them in /etc/pki/qemu. This directory must exist and contain: I suspect the POSIX specification mandates that the directory must exist in order to contain files. :)

> # > # ca-cert.pem - the CA master certificate > # server-cert.pem - the server certificate signed with ca-cert.pem > @@ -13,6 +13,12 @@ > # > # dh-params.pem - the DH params configuration file > # > +# If the directory does not exist or does not contain the necessary > files, > +# QEMU domains will fail to start if they are configured to use TLS. > +# Isn't this sufficiently covered by 'Use of TLS requires that x509 certificates be issued'?

> +# In order to overwrite the default path alter the following. If the > provided > +# path does not exist, then startup will fail. > +# To apply the configuration, you need to restart the daemon. And since daemon startup will fail, I think the user will be able to notice it. We should error out on incorrect paths as soon as we can, without mentioning it in the documentation.

> #default_tls_x509_cert_dir = "/etc/pki/qemu" > > > @@ -79,8 +85,9 @@ > > # 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. > +# If the default listed here does not exist, then the default > /etc/pki/qemu > +# is used. If I override default_tls_x509_cert_dir, without overriding all the specific *_tls_x509_cert_dir values, I expect they will all use my value, not the hardcoded default of /etc/pki/qemu. So the behavior described by the original comment makes more sense.

> If uncommented and the provided path does not exist, then startup > +# will fail. > # > #vnc_tls_x509_cert_dir = "/etc/pki/libvirt-vnc" >

> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c > index 73c33d6..4eb6f0c 100644 > --- a/src/qemu/qemu_conf.c > +++ b/src/qemu/qemu_conf.c > @@ -440,6 +440,34 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr > hugetlbfs, > } > > > +static int > +virQEMUDriverConfigSetCertDir(virConfPtr conf, > + const char *setting, > + char **value) > +{ > + char *tlsCertDir = NULL; > + > + if (virConfGetValueString(conf, setting, &tlsCertDir) < 0) > + return -1; > + > + if (!tlsCertDir)

> + return 0; > + > + if (!virFileExists(tlsCertDir)) { > + virReportError(VIR_ERR_CONF_SYNTAX, This is not a syntax error.

Validating these settings does not belong in virQEMUDriverConfigLoadFile; qemuStateInitialize or a function called from it would be a better place. Jan