[libvirt] [PATCH v3] qemu: Check for existence of provided *_tls_x509_cert_dir
https://bugzilla.redhat.com/show_bug.cgi?id=1458630 Introduce virQEMUDriverConfigTLSDirValidateResetDefault to validate that if any of the *_tls_x509_cert_dir values were set properly and reset the default value if the default_tls_x509_cert_dir changed. 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> --- v2: https://www.redhat.com/archives/libvir-list/2017-June/msg01341.html Follow-ups in July though. Changes since v2 - reduced verbosity in qemu.conf and adjusted the logic to create/call virQEMUDriverConfigTLSDirValidateResetDefault after all the values are read in order to validate the values and adjust the default if necessary. Tested by 1. Having everything commented out, w/ no /etc/pki/qemu: Works as expected 2. Uncomment the default_tls_x509_cert_dir, w/ no /etc/pki/qemu: Fails as expected 3. Uncomment each of the other *_tls_x509_cert_dir's when directory not exist: Fails as expected 4. Use a directory that exists for other _*tls_x509_cert_dirs: Works as expected 5. Change the default_tls_x509_cert_dir to an existing directory: Works as expected, each of the other uncommented *_tls_x509_cert_dirs will get the new value (tested via debug code) while one that used an existing default didn't change from it's default /etc/pki/libvirt-* src/qemu/qemu.conf | 8 +++++++ src/qemu/qemu_conf.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_conf.h | 4 ++++ src/qemu/qemu_driver.c | 3 +++ 4 files changed, 79 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index e6c0832..9526aed 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -13,6 +13,14 @@ # # dh-params.pem - the DH params configuration file # +# If the directory does not exist or 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. 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" diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 6f44cbf..87d2c2d 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -451,8 +451,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, if (!(conf = virConfReadFile(filename, 0))) goto cleanup; - if (virConfGetValueString(conf, "default_tls_x509_cert_dir", &cfg->defaultTLSx509certdir) < 0) + if ((rv = virConfGetValueString(conf, "default_tls_x509_cert_dir", &cfg->defaultTLSx509certdir)) < 0) goto cleanup; + cfg->checkdefaultTLSx509certdir = (rv == 1); if (virConfGetValueBool(conf, "default_tls_x509_verify", &cfg->defaultTLSx509verify) < 0) goto cleanup; if (virConfGetValueString(conf, "default_tls_x509_secret_uuid", @@ -872,6 +873,68 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, return ret; } + +/** + * @cfg: Recently config values + * + * Validate the recently read *_tls_x509_cert_dir values and if necessary + * update the default value to match the default_tls_x509_cert_dir + * + * Returns 0 on success, -1 on failure + */ +int +virQEMUDriverConfigTLSDirValidateResetDefault(virQEMUDriverConfigPtr cfg) +{ + bool newDefault = false; + + /* If the default entry was uncommented, then validate existence */ + if (cfg->checkdefaultTLSx509certdir) { + if (!virFileExists(cfg->defaultTLSx509certdir)) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("default_tls_x509_cert_dir directory '%s' " + "does not exist"), + cfg->defaultTLSx509certdir); + return -1; + } + if (STRNEQ(cfg->defaultTLSx509certdir, SYSCONFDIR "/pki/qemu")) + newDefault = true; + } + + /* We know virQEMUDriverConfigNew set the particular value to either + * it's default or default_tls_x509_cert_dir's default. So, if not the + * default default and the directory doesn't exist, then the entry was + * set in the config file to something that doesn't exist, so error. + * + * Also, if the defaultTLSx509certdir value was changed from the default, + * then we need to update the default for each setting as well to match + * the default_tls_x509_cert_dir. + */ +#define VALIDATE_TLS_X509_CERT_DIR(val) \ + do { \ + if (STRNEQ(cfg->val ## TLSx509certdir, SYSCONFDIR "/pki/qemu") && \ + !virFileExists(cfg->val ## TLSx509certdir)) { \ + virReportError(VIR_ERR_CONF_SYNTAX, \ + _(#val"_tls_x509_cert_dir directory '%s' " \ + "does not exist"), \ + cfg->val ## TLSx509certdir); \ + return -1; \ + } else if (newDefault) { \ + VIR_FREE(cfg->val ## TLSx509certdir); \ + if (VIR_STRDUP(cfg->val ## TLSx509certdir, \ + cfg->defaultTLSx509certdir) < 0) \ + return -1; \ + } \ + } while (0) + + VALIDATE_TLS_X509_CERT_DIR(vnc); + VALIDATE_TLS_X509_CERT_DIR(spice); + VALIDATE_TLS_X509_CERT_DIR(chardev); + VALIDATE_TLS_X509_CERT_DIR(migrate); + + return 0; +} + + virQEMUDriverConfigPtr virQEMUDriverGetConfig(virQEMUDriverPtr driver) { virQEMUDriverConfigPtr conf; diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 1407eef..fffa871 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -112,6 +112,7 @@ struct _virQEMUDriverConfig { char *nvramDir; char *defaultTLSx509certdir; + bool checkdefaultTLSx509certdir; bool defaultTLSx509verify; char *defaultTLSx509secretUUID; @@ -301,6 +302,9 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, const char *filename, bool privileged); +int +virQEMUDriverConfigTLSDirValidateResetDefault(virQEMUDriverConfigPtr cfg); + virQEMUDriverConfigPtr virQEMUDriverGetConfig(virQEMUDriverPtr driver); bool virQEMUDriverIsPrivileged(virQEMUDriverPtr driver); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6568def..2731f8e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -667,6 +667,9 @@ qemuStateInitialize(bool privileged, goto error; VIR_FREE(driverConf); + if (virQEMUDriverConfigTLSDirValidateResetDefault(cfg) < 0) + goto error; + if (virFileMakePath(cfg->stateDir) < 0) { virReportSystemError(errno, _("Failed to create state dir %s"), cfg->stateDir); -- 2.9.4
On Fri, Jul 21, 2017 at 02:23:25PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1458630
Introduce virQEMUDriverConfigTLSDirValidateResetDefault to validate that if any of the *_tls_x509_cert_dir values were set properly and reset the default value if the default_tls_x509_cert_dir changed.
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> ---
[...]
src/qemu/qemu.conf | 8 +++++++ src/qemu/qemu_conf.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_conf.h | 4 ++++ src/qemu/qemu_driver.c | 3 +++ 4 files changed, 79 insertions(+), 1 deletion(-)
[...]
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 6f44cbf..87d2c2d 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c
[...]
@@ -872,6 +873,68 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, return ret; }
+ +/** + * @cfg: Recently config values + * + * Validate the recently read *_tls_x509_cert_dir values and if necessary + * update the default value to match the default_tls_x509_cert_dir + * + * Returns 0 on success, -1 on failure + */ +int +virQEMUDriverConfigTLSDirValidateResetDefault(virQEMUDriverConfigPtr cfg)
Or just virQEMUDriverConfigValidate, for future expansion.
+{ + bool newDefault = false; + + /* If the default entry was uncommented, then validate existence */ + if (cfg->checkdefaultTLSx509certdir) { + if (!virFileExists(cfg->defaultTLSx509certdir)) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("default_tls_x509_cert_dir directory '%s' " + "does not exist"), + cfg->defaultTLSx509certdir); + return -1; + } + if (STRNEQ(cfg->defaultTLSx509certdir, SYSCONFDIR "/pki/qemu")) + newDefault = true;
This bool is only used to decide if we need to stdup the value to the more specific directory variables. If the user took the time to put the default value in the uncommented variable, it's only fair to have them suffer the performance penalty of one extra strdup in exchange for making this code clearer by dropping this bool. cfg->checkdefaultTLSx509certdir can be used (or renamed to 'defaultTLSx509certdirProvided'. Alternatively, the values can be left at NULL in virQEMUDriverConfigNew, this function would only validate the user-provided values and the defaults would be filled in by another function. (This has the benefit of the TLSdir logic being in one place. On the other hand, virQEMUDriverConfigNew would not give us a complete default config)
+ } + + /* We know virQEMUDriverConfigNew set the particular value to either + * it's default or default_tls_x509_cert_dir's default. So, if not the + * default default and the directory doesn't exist, then the entry was + * set in the config file to something that doesn't exist, so error. + * + * Also, if the defaultTLSx509certdir value was changed from the default, + * then we need to update the default for each setting as well to match + * the default_tls_x509_cert_dir. + */ +#define VALIDATE_TLS_X509_CERT_DIR(val) \ + do { \ + if (STRNEQ(cfg->val ## TLSx509certdir, SYSCONFDIR "/pki/qemu") && \
Okay, here we assume that SYSCONFDIR "/pki/qemu" means the value was untouched by the user for the simplicity of the code.
+ !virFileExists(cfg->val ## TLSx509certdir)) { \ + virReportError(VIR_ERR_CONF_SYNTAX, \ + _(#val"_tls_x509_cert_dir directory '%s' " \ + "does not exist"), \
This translatable string is untranslatable: msgid "_tls_x509_cert_dir directory '%s' does not exist" Either supply the whole variable name, or abolish macros and open-code the error message like we do for {state,lib,cache}Dir in qemuStateInitialize.
+ cfg->val ## TLSx509certdir); \ + return -1; \
+ } else if (newDefault) { \ + VIR_FREE(cfg->val ## TLSx509certdir); \ + if (VIR_STRDUP(cfg->val ## TLSx509certdir, \ + cfg->defaultTLSx509certdir) < 0) \ + return -1; \ + } \
This should not be in a macro called VALIDATE. Maybe virQEMUDriverConfigTLSDirSetDefaults?
+ } while (0) + + VALIDATE_TLS_X509_CERT_DIR(vnc); + VALIDATE_TLS_X509_CERT_DIR(spice); + VALIDATE_TLS_X509_CERT_DIR(chardev); + VALIDATE_TLS_X509_CERT_DIR(migrate); + + return 0; +} + + virQEMUDriverConfigPtr virQEMUDriverGetConfig(virQEMUDriverPtr driver) { virQEMUDriverConfigPtr conf;
[...] Jan
participants (2)
-
John Ferlan -
Ján Tomko